Repository navigation
fix(runtime): harden the queue turn record against a double release and a State panic - #134
Conversation
TestQueueIdleScopeRetainsNoTurnRecord reads queueTurns through the new QueueTurnCount hook. It fails with the delete in leaveQueueScope removed.
…scope twice dispatch now builds the release in deferredRelease, with no change in behavior, so the QueueDeferredRelease hook gets the same release. TestQueueDeferredReleaseCalledTwiceLeavesItsScopeOnce fails: the second call drops the scope's turn record while another delivery is in flight.
The release that dispatch gives a deferred delivery wraps the idempotent release from admit, but a second call left the queue scope again. It could drop the turn record while another delivery was in flight, or trip the leaveQueueScope assert. sync.OnceFunc makes it idempotent like admit's.
…tale waiter Under DispatchSync, a panic from State.AcquireLock while a queue waiter polls skips queueForLock's clearPending. The waiter's leaveQueueScope then trips its assert in place of the State's panic, and so does a later duplicate for the scope. TestQueueStatePanicWhileWaiterPollsLeavesNoStaleWaiter recovers each panic as net/http does and fails on both.
queueForLock cleared the waiter's entry only on the abandoned and failed outcomes, so a panic from State.AcquireLock left it behind. One deferred clearPending now covers every exit. On the other exits the entry is already gone or belongs to a newer waiter, and clearPending leaves it.
ConcurrencyQueue's GoDoc relies on this rule, but only the Observer interface stated it.
The poll-call constants follow the iota pattern of queue_edge_test.go, and comments the code already states are gone. The ValidateThreadID GoDoc now names what a block there costs under ConcurrencyQueue.
|
@codex review |
|
@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 Review: Didn't find any major issues. Can't wait for the next one! 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. |
TestQueueStatePanicWhileWaiterPollsLeavesNoStaleWaiter checked only the turn record count, so a fix that deleted the leaveQueueScope assert instead of clearing the waiter still passed while the stale waiter stayed in c.pending. Add QueuePendingCount and assert it is 0 after B's panic.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Breezy! 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. |
#138) ## Why Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #135. Under `DispatchDeferred`, a `State` panic in the prelude unwinds to `net/http`, which recovers it. `dispatchDeferred` freed the admission slot only when the prelude returned, so the panicked delivery kept its slot and its queue turn record, and `Shutdown` timed out. ## What changed - `dispatchDeferred` frees the slot in a `defer` on every exit that hands no work to a tail, a panic included. The panic still reaches `net/http`. - The slot release is the `sync.OnceFunc` from #134, so no exit can release twice. - A new test serves the runtime behind a real `httptest.Server`. It panics `State.AcquireLock` under the queue strategy and `State.MarkEvent` under the burst strategy. ## Scope - The fix covers every strategy, because the leak was in `dispatchDeferred`. Before the fix, a `MarkEvent` panic leaked the slot under drop, debounce, burst, and concurrent too. - A panic in a detached tail goroutine still ends the process, as on `main`. - The panicked event keeps its in-memory acceptance entry, so a redelivery of the same event ID waits until its own request ends. That leak has a different cause and is left as a follow-up. ## Tradeoffs - A `recover` and re-panic would also free the slot. The `defer` with an ownership flag frees it and leaves the panic value and stack untouched. ## Blast Radius Only deferred dispatch changes. A delivery that resolves in the prelude now frees its slot as `dispatchDeferred` returns, not just before, which the caller cannot tell apart. ## Verification - Before the fix, the new test fails in both cases with `Shutdown ... context deadline exceeded`. The queue case also keeps 1 turn record. - Three wrong fixes fail. A silent `recover` fails the new test with status 200. A release without `OnceFunc` trips the `leaveQueueScope` assert in the queue tests. A release without the ownership flag fails two admission tests. - The new test and the queue 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 #135 --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
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.AcquireLockwhile a queue waiter polls, can trip theleaveQueueScopeassert, which panics in production. No test read the record, so a missingdeletepassed every Go test.What changed
dispatchnow wraps a deferred delivery's release insync.OnceFunc, like the release thatadmitreturns. A second call no longer leaves the queue scope again.queueForLockclears its waiter's pending entry in onedefer, in place of the two explicitclearPendingcalls. A panic fromStateno longer leaves a stale waiter behind the turn record.Adapter.ValidateThreadIDGoDoc now says that it must not block, the rule that theConcurrencyQueueGoDoc relies on.Scope
DispatchSynccase, becausenet/httprecovers a handler panic. A panic in a deferred tail goroutine still ends the process, as onmain.queueForLockand gets the same cleanup. Its outcomes do not change.Tradeoffs
deferinqueueForLock, because that function already ends the registration on every other exit, and the panic exit was the one it missed. A clear inleaveQueueScopewould need the delivery's event or admission sequence inqueueEntry, and the assert there could then never catch a leaked registration. With the newdeferremoved, the existing queue tests trip that assert.export_test.gohook. For that,dispatchbuilds the release in a new method,deferredRelease.Blast Radius
Only queue and debounce waiters change. Each waiter takes one more
queueMusection when its lock wait ends. An abandoned or failed waiter now clears its entry just after its log line, not just before it.Verification
TestQueueDeferredReleaseCalledTwiceLeavesItsScopeOncefails withturn records = 0 after one of two deliveries released twice, want 1.TestQueueStatePanicWhileWaiterPollsLeavesNoStaleWaiterfails because the waiter panics with the assert instead of the State's panic, and a later duplicate trips the same assert.deleteinleaveQueueScoperemoved,TestQueueIdleScopeRetainsNoTurnRecordfails in both dispatch modes.-race.go vetandgo testpass in every workspace module, andgo test -racepasses for the root package.Closes #129
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh