fix(client): make abort actually cancel, and keep the mismatch escape hatch alive - #100
Open
V3RON wants to merge 1 commit into
Open
fix(client): make abort actually cancel, and keep the mismatch escape hatch alive#100V3RON wants to merge 1 commit into
V3RON wants to merge 1 commit into
Conversation
…ection open on a protocol mismatch (B3, B4) B3: requestLease's AbortSignal was a silent no-op for two ADR-0003-mandated cases: a client with no supplied principal (the daemon never reported its resolved default back), and the §4 proxy case (one connection, many requesterIds). Both tripped lease.cancel's requesterId===principal check into FORBIDDEN, which the abort path's bare catch swallowed and treated as "the connection died", resolving the caller's await with a real grant it had explicitly abandoned. Fixes, in order: - hello's reply now carries the daemon-resolved principal (helloReplySchema, DaemonServer#handleHello's hello region only); the client adopts it instead of defaulting to "". - lease.cancel's authorize hook is gated on the pending request's recorded owner (QueueControl.pendingRequestOwner, backed by the wait queue's existing per-waiter ownerId) rather than comparing requesterId to the principal directly -- this is the "do it properly" route: no core wait-queue schema change was needed, only a new read accessor threaded through LeaseAcquisitionCoordinator -> LeaseEngine -> the dispatcher's AuthorizeContext. This makes the §4 proxy case (principal "host", requesterId "agent-7") work as designed. - lease.request is documented as deliberately authorize-free (ADR §4: any agent may request under an arbitrary requesterId; ownerId is never client-supplied) so it no longer reads as inconsistent with lease.cancel. - the abort path's catch around lease.cancel now only swallows a transport-kind error (a genuinely dead connection); FORBIDDEN/BAD_REQUEST surface to the caller instead of being misread as connection death. Residual limitation: if a caller passes a requesterId it does not actually own and the daemon rejects the cancel with FORBIDDEN, that now surfaces immediately to the caller, but if the original lease.request later still grants, the grant is still tracked internally as held with nothing to release it (a pre-existing best-effort gap noted at #releaseAbandonedGrant, not touched by this fix). B4: the client closed the socket on any hello failure, including PROTOCOL_VERSION_UNSUPPORTED -- the one mismatch the daemon deliberately keeps the connection open for, so an admin client can still send daemon.stop instead of restarting the daemon and dropping every held lease (ADR §6). Now only that specific rejection keeps the connection open and returns a degraded client whose every operation but stopDaemon()/close() rejects with the captured error; every other handshake failure (including a bad credential) still closes and serves nothing.
V3RON
force-pushed
the
fix/0003-07-client-abort
branch
from
September 3, 2026 17:23
5be2fc8 to
f302446
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #99. Two blocking defects from the adversarial review of this stack.
The abort signal was a silent no-op that handed the caller the lease it abandoned
The client always sent an explicit
requesterIdon the abort'slease.cancel, andlease.cancel'sauthorizerequiredrequesterId === principalfor a non-admin session. Two independent triggers:""while the daemon fixed the connection principal to its owndefaultRequesterId— andhello's reply did not return it, so the client could not know it. The cancel wasFORBIDDEN.principal: "host"leasing underrequesterId: "agent-7"— was forbidden outright. That is the scenario the whole ADR exists for.In both cases the bare
catch(commented as handling only a dead connection) swallowed the rejection and returned the original request promise, so the caller'sawaitresolved with a real grant and the client tracked it as held. ADR §10 requires the opposite: "the caller never holds a lease it abandoned."Fixed properly rather than papered over:
hello's reply now carries the resolved principal, and the client adopts it instead of"".lease.cancelis now owner-aware. A newQueueControl.pendingRequestOwner(requesterId)reads the pending request's recordedownerId(already stored on the waiter), threaded throughLeaseEngineinto the dispatcher'sAuthorizeContext.authorizecompares the pending request's owner to the session principal, so the §4 proxy can cancel what it created. No core schema change was needed.catchis narrowed to transport-kind errors, so aFORBIDDEN/BAD_REQUESTfrom the cancel surfaces instead of being misread as "the connection died".lease.requestis now explicitly documented as deliberately authorize-free, resolving the incoherence the review noted (it accepted anyrequesterIdwhilelease.cancelrefused one).The client closed the socket on a protocol mismatch, killing §6's escape hatch
The daemon goes out of its way to keep the socket open after a failed range negotiation, and checks
daemon.stopahead of the mismatch gate, precisely so a mismatched admin client can stop it rather than restarting the daemon and dropping every held lease on the machine. The client closed the connection on anyhellofailure, sosimlock daemon stop— the exact command the error message tells the user to run — could not work.Not reachable at
{3,3}today, but it would ship broken into every 0.3.0 client and bite at the first version bump. Now aPROTOCOL_VERSION_UNSUPPORTEDrejection returns a degraded client permitting onlystopDaemon(); every other handshake failure, includingADMIN_AUTHENTICATION_FAILED, still closes and serves nothing.Residual limitation
If a caller supplies a
requesterIdit genuinely does not own, theFORBIDDENnow surfaces correctly, but a grant that still arrives afterwards is tracked with nothing releasing it. Pre-existing best-effort gap; the owner-aware check should make it unreachable in normal operation.The degraded client's own
principal/rolefields are best-effort, since a failedhellonever reports the daemon's resolved values — not load-bearing, becausestopDaemon()is gated by the daemon's credential check, not by anything the client claims about itself.