Repository navigation
fix(relay): run a handler's ordered prefix on the connection loop - #978
Merged
Merged
Conversation
Both pairs are written in one flush, so the loop reads them back to back and the ordering between their handlers decides the outcome. Neither test passes on this tree: the CLOSE finds nothing to abort and the restricted REQ is refused before its AUTH is handled.
`handlers::dispatch` gave every message's whole handler to the per-socket `JoinSet`, so two frames read in order could run in either order: a `CLOSE` could be handled ahead of its own `REQ` and find nothing to abort, and a restricted `REQ` could be refused before the `AUTH` that preceded it. `handlers::begin` runs each message's ordering-sensitive prefix on the loop task, before the next frame is read, and returns the continuation for the `JoinSet`. The prefixes are `subscribe` (through `begin_read`), `unsubscribe` and `auth`; `EVENT` and `COUNT` keep their whole body as one spawned future. Continuations stay concurrent, so a `CLOSE` during a gated invite mint still aborts the read.
The ADR states the chosen shape (a prefix on the connection loop), the two rejected shapes with their reasons, the `AUTH`->`REQ` symptom, the `EVENT`/`COUNT` exclusion and the guarantee's boundary: prefix order, not handler serialisation. Its "invite emit stays unguarded" paragraph no longer describes a `CLOSE` overtaking its `REQ` as a live precondition. The changeset carries the release note; the TypeScript tree is untouched.
The oracle waited 50 ms for the REQ handler to register and then read frames for a fixed 150 ms window, asserting the window held exactly one frame. Both are load-dependent: a loaded runner could miss the live event's OK in the window and fail with the ordering correct, and a short wait at the head would let the test pass without the defect being exercised. The obstacle frame is now an undecodable one, whose NOTICE the connection loop writes itself, so receiving it proves the loop read the REQ and CLOSE ahead of it. The live event is sent after that barrier and its own OK, on the client's two-second deadline, ends the wait; the only timed assertion left is the post-gate silence, where a bounded window is the assertion.
`begin`'s docblock promised that every statement a handler runs before its first suspension point has run in arrival order, which is false for the `Event` and `Count` arms: they are boxed whole, so none of their body is ordered. The postcondition is now stated about the prefix, with the single-caller precondition the guarantee rests on, and the two excluded arms say so at the edit site. The reason given for excluding `EVENT` and `COUNT` was untrue: the event limiter is keyed on this connection's address and each `EVENT` consults it from its own task, so two pipelined events may record their hits in either order. The module doc and ADR 0005 now give the criterion that does hold (no later frame needs an entry these handlers establish) and record that consequence as parity with the capture, whose limiter hit also sits after its first `await`. ADR 0005's cost clause now names the `AUTH` prefix, the expensive one, with its measured cost and the interleaving it gives up.
…t figures The event limiter is not keyed on the connection: `RateLimits::event_backoff` keys it on the event's author and window, in the table the whole relay shares, so the module doc and ADR 0005 now say that and keep the consequence — two pipelined `EVENT`s from one author record their hits in poll order. Both also give the capture's reason: it binds each `message` event to a handler it does not await, so its hits are unordered too. `begin`'s docblock said `None` means the prefix answered the message, which the `CLOSE` arm disproves: it returns `None` having written no frame. `None` now means the prefix handled the message and nothing is left to run, writing whatever frames its handler writes. The cost clause no longer presents a derived sum as a measurement: the `REQ` figure is named as its parts (0.11 µs normalisation plus 0.61–0.65 µs of subscription-table work, 0.72–0.76 µs), the verify as the 32.2–32.5 µs range it was measured over, and the attribution says this branch. It also drops the claim that the prefix costs only interleaving: the loop is occupied for those 34 µs, which caps a pipelining client's `AUTH` rate. ADR 0005's front matter now names both jobs in the title, the deciders' evidence and the context's four relationships.
Round 3's front-matter edit dropped a line from the ADR's Context paragraph, leaving "whose only consumer was the queue grow without limit" — the frame queue problem had vanished from the record. The clause is restored, so the paragraph again states the four relationships it counts. The module doc and ADR 0005 now say the one shared table the excluded handlers touch is relay-wide — the event limiter, keyed on the author and window — rather than denying that they touch per-connection state, which `COUNT`'s sink write and `EVENT`'s `remote_address` read contradict. The capture's parity mechanism is stated as it is: the socket fires each 'message' listener without awaiting it (:79, :198) and the handler is awaited inside that listener (:229), so two pipelined `EVENT`s run as interleaved chains there too. The old citation named the await as proof of the opposite.
🦋 Changeset detectedLatest commit: 9accda1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
 [Medium impact] This PR appears safe to merge; no new blocking issues were found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…parity Greptile P2 on #978, and it is right: the capture's AuthMessageHandler awaits isEventIdValid and isEventSignatureValid before addAuthenticatedPubkey, so a REQ pipelined behind an AUTH can be judged against a session that has not been recorded and be refused. Rust verifies and records on the connection loop, so the session exists before the next frame is read — a stronger guarantee. ADR 0005 now says so, names the cost (the read path waits for two signature checks, once per AUTH frame) and the module doc points at it. Behaviour is unchanged; only the claim about it was wrong.
This was referenced Oct 25, 2026
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.
Inbound frames were dispatched as independent tasks, so two frames read in order could run in either order. A
REQfollowed immediately by aCLOSEfor the same id could have theCLOSErun first — the subscription then registered with nothing left to abort it, and the read streamed toEOSEon a subscription the client had closed. This restores the capture's guarantee for the state that matters, and closes #958.Closes #958.
The defect, measured
Recon reproduced it before anything was designed: on a multi-thread runtime a
REQ+CLOSEpair written in one flush leaves the subscription registered and streaming in 30 of 30 runs (18/20 at two workers, 29/30 at four), and in 0 of 20 oncurrent_thread— which is why the whole suite was green.A second symptom came out of the same cause: a pipelined
AUTHfollowed by a restrictedREQwas refusedauth-requiredin 10 of 10 multi-thread runs, because theREQran before theAUTHthat unlocks it.The fix
handlers::dispatchis gone.handlers::begin(message, &Arc<HandlerContext>) -> Option<HandlerFuture>runs the message's ordering-sensitive prefix on the connection loop and hands back the continuation for the existingJoinSet:REQnormalise_subscription,can_subscribe,subscribe,begin_readCLOSEAUTHnip42::authenticate)EVENT,COUNTThe prefixes are plain
fns, so a suspension point inside one is not representable;connection.rsis untouched. The capture's guarantee is the same shape — it binds each'message'listener without awaiting it (web-socket-adapter.ts:79,:198) and awaits the handler inside that listener (:229), so its ordered prefix is likewise only the part before the firstawait.Cost of the move, measured on this branch:
CLOSE0.053 µs,REQ0.72–0.76 µs (0.11 µs normalisation + 0.61–0.65 µs subscription-table work),AUTH34.3 µs (32.2–32.5 µs verify + 1.28 µs id hash) per message on the loop. For comparison the capture's same verify costs 1.57–1.85 ms/op of CPU on Node — the port is 46–54× cheaper and blocks one socket rather than the only thread.Red before, green after
AC10 requires the evidence in the history, not the prose. At
2c56dce— the branch's first commit, tests only,1 file changed, 130 insertions(+)— both new tests fail on the unfixed tree:At the head both pass, and the ordering proof does not rest on a scheduler coincidence: the oracle anchors on a
NOTICEthat the connection loop itself writes for an undecodable third frame, so receiving it proves the loop read the two frames ahead of it. The only timed assertion left is a silence window where silence is the assertion.a_handler_prefix_runs_before_its_continuation_is_polledproves the same property without a scheduler at all.Verification
cargo test --workspace --all-features: 905 tests, 0 failed, 56 test targets, in debug and release.cargo fmt --all --checkclean; both clippy passes clean with no warning line; release build clean.2b2287f: 5/5 failures with the branch's test file over base sources, 3/3 in the approver's own run.Review
Four rounds; the executable change was stable from the first, and every finding after it was a sentence describing it. ADR 0005 records the decision, the two rejected shapes (serialized handlers, per-connection ordering token) and why.
security,uxanduistood down, with diff-grounded reasons in the review plan; the approver re-examined the security skip against the whole diff and judged it sound, noting that a security read of the next auth-adjacent job is worth its cost.Notes
.husky/pre-commitdrives the legacy pnpm checks and its.husky/_/husky.shis absent in the worktree, so commits usedHUSKY=0 git commit --no-verify. The Rust validation above ran in full at the head.EVENT/EOSEthe capture also emits for a closed id is rewrite: record the CLOSE-aborts-read decision the port already makes #957; the per-socket bound question is relay: the 8 MiB frame bound is per socket and sockets are unlimited #959; theEVENTlimiter's ordering between two pipelined events is recorded in the ADR as unspecified and matching the capture.