Repository navigation
fix(runtime): keep a stalled queue delivery from running after a newer turn - #124
Merged
Merged
Conversation
…er turn (#123) X hits the Lock Conflict and stalls in its dedupe mark. A finishes, C takes the free lock and runs, and then X registers into the empty waiter slot, waits for the lock, and runs last: handled = [A C X], want [A C]. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
…r turn A queue delivery that stalled in its prelude (lock call, routing, dedupe) could register or take the lock after a newer delivery had already taken the scope's turn, and then run last. Nothing recorded the newer turn once its waiter entry was gone. Each lock scope now keeps a queue turn record while any of its deliveries is in flight: the count of those deliveries and the admission sequence of the newest one that took a turn. registerPending and the prelude lock taker resolve a delivery older than that turn as superseded. A waiter takes its turn in commitPending, which joins the isPending recheck and the clear into one queueMu section. The admission sequence is now assigned when a delivery reaches its lock scope, in the same section that counts it, so no turn can be taken between the two. Only non-blocking work (the Observer callback and thread ID validation) runs before that point. Two more tests pin the lock-call stall and the newer-waiter case. Fixes #123 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a534116a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1a53411 moved the admission sequence to lock-scope entry. Under debounce, an older delivery delayed in thread ID validation then got a larger sequence than a newer waiter and displaced it, so debounce handled the stale event. On main the older delivery is refused. dispatch assigns the sequence again, as on main. enterQueueScope only counts a queue delivery in its scope's turn record. A queue delivery delayed before its scope is known now ends stale while a newer turn is still in flight. The GoDoc states the case that remains: a delivery delayed there until every newer delivery for the scope finished still runs afterwards, as on main. TestDebounceDeliveryDelayedInValidationCannotDisplaceNewerWaiter fails on 1a53411 with handled = [older] and passes here. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Member
Author
Readiness recordDecision: ready to merge, with follow-ups tracked in #129. Not merged: the owner merges.
Generated with |
ThomasK33
added a commit
that referenced
this pull request
Oct 29, 2026
…nd a State panic (#134) ## Why Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #129. An independent review of #124 found three gaps in the queue turn record. A second call to a deferred delivery's release, or a panic from `State.AcquireLock` while a queue waiter polls, can trip the `leaveQueueScope` assert, which panics in production. No test read the record, so a missing `delete` passed every Go test. ## What changed - `dispatch` now wraps a deferred delivery's release in `sync.OnceFunc`, like the release that `admit` returns. A second call no longer leaves the queue scope again. - `queueForLock` clears its waiter's pending entry in one `defer`, in place of the two explicit `clearPending` calls. A panic from `State` no longer leaves a stale waiter behind the turn record. - Three new tests pin the record. The `Adapter.ValidateThreadID` GoDoc now says that it must not block, the rule that the `ConcurrencyQueue` GoDoc relies on. ## Scope - The State-panic case is a `DispatchSync` case, because `net/http` recovers a handler panic. A panic in a deferred tail goroutine still ends the process, as on `main`. - Debounce shares `queueForLock` and gets the same cleanup. Its outcomes do not change. ## Tradeoffs - The issue offered two places to clear the waiter's entry. I picked the `defer` in `queueForLock`, because that function already ends the registration on every other exit, and the panic exit was the one it missed. A clear in `leaveQueueScope` would need the delivery's event or admission sequence in `queueEntry`, and the assert there could then never catch a leaked registration. With the new `defer` removed, the existing queue tests trip that assert. - No public path calls the release twice, so the test reaches it through a new `export_test.go` hook. For that, `dispatch` builds the release in a new method, `deferredRelease`. ## Blast Radius Only queue and debounce waiters change. Each waiter takes one more `queueMu` section when its lock wait ends. An abandoned or failed waiter now clears its entry just after its log line, not just before it. ## Verification - Each fix lands after its failing test. Before the fix, `TestQueueDeferredReleaseCalledTwiceLeavesItsScopeOnce` fails with `turn records = 0 after one of two deliveries released twice, want 1`. `TestQueueStatePanicWhileWaiterPollsLeavesNoStaleWaiter` fails because the waiter panics with the assert instead of the State's panic, and a later duplicate trips the same assert. - With the `delete` in `leaveQueueScope` removed, `TestQueueIdleScopeRetainsNoTurnRecord` fails in both dispatch modes. - The three new tests pass 20 of 20 runs under `-race`. `go vet` and `go test` pass in every workspace module, and `go test -race` passes for the root package. Closes #129 --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
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.
Why
Under
ConcurrencyQueue, the bot must never answer an older message after a newer one (ADR 0015 §3). A queue delivery that stalled in its prelude (in its lock call, routing, or dedupe) could still wait for the lock, or take it, after a newer delivery had taken the thread's turn. The stalled delivery then ran last (#123). #122 covered a waiter that was already parked. Nothing recorded that a newer delivery had taken its turn.What changed
queueTurn) while any of its deliveries is in flight.activecounts those deliveries, andwonholds the admission sequence of the newest one that took a turn.leaveQueueScopedrops the record when the last delivery reaches its outcome.takeQueueTurnreplacessupersedeOlderWaiter. A lock taker older thanwonreleases the lock and ends as superseded. Otherwise it setswonand supersedes an older waiter, as before.registerPendingrefuses a queue delivery older thanwon.commitPendingreplaces theisPendingrecheck and theclearPendingcall after a waiter acquires the lock. It is onequeueMusection, and it setswonfor the waiter.enterQueueScopecounts a queue delivery in its scope's turn record once its lock scope is known, before any State call. The admission sequence stays indispatch, as onmain.ConcurrencyQueueGoDoc states the rule and the case that remains.Scope
commitPending: a waiter that passes the recheck can no longer be logged as superseded and then run.Tradeoffs
Dispatchcallback, which must not block by its contract, and the adapter'sValidateThreadID, which takes no context. Every State call runs after it. A delivery delayed there until every newer delivery for the scope has finished still runs afterwards, as onmain. The GoDoc states this.dispatchfor every strategy.Blast Radius
Only the queue strategy changes, and only when a delivery stalls in its prelude long enough for a newer delivery to take the turn. That older delivery now ends as superseded (ignored outcome, debug log
chat queue waiter superseded) instead of running last. Five newassertcalls guard the record's invariants, and they panic in production if broken.Verification
TestQueueDeliveryStalledBeforeRegisteringNeverRunsAfterNewerLockTakerlands first (2268e7a). It fails withhandled = [A C X], want [A C]in deferred and sync dispatch, 3 of 3 runs.-race.TestDebounceDeliveryDelayedInValidationCannotDisplaceNewerWaiterfails on 1a53411 withhandled = [older]and passes onmainand on d7a7ccb.wonchecks inregisterPendingandtakeQueueTurn, thewonupdate in each commit path, and the record lifetime. Each break fails a test or trips an assert.go test -race ./...andgo vetin every workspace module pass.mainalong both stall paths. On d7a7ccb it passes with up to 4 deliveries (7.5 million states), under one stated assumption: a newer delivery never finishes while an older one is still in the pre-scope work. Without that assumption, a separate check fails along the documented residual path, as expected.Fixes #123
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh