Repository navigation
fix(runtime): abandon a concurrency slot won after Shutdown cancelled the tails - #141
Conversation
Shutdown cancels detached tails one at a time. A waiter can win the slot that a cancelled handler frees before its own context is cancelled, and then run its handler. The new test widens that window and fails on main.
Ten rounds with 20000 child contexts still fail on main in 150 of 150 runs at -cpu=2, 4, and 96, and in 20 of 20 runs with -race. The test now takes about a fifth of the time. The commit also removes the 40 ms sleep from TestConcurrentSlotWaiterDrainedByShutdown.
… the tails Shutdown cancels detached tails one at a time. A tail whose handler returns first frees its slot, and a parked waiter can win that slot before its own context is cancelled. The waiter then ran its handler during Shutdown instead of abandoning the wait. A detached tail that wins a slot after the base context is cancelled now releases the slot and takes the abandoned path, with the same log and outcome. Sync dispatch is unchanged.
…oncurrencySlot Replace the detached flag with a wrapper for detached tails, so the sync path keeps the original function unchanged.
|
@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. Keep them coming! 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. |
…aiter first Without the sleep the waiter reaches the slot after Shutdown in almost every run, so TestConcurrentSlotWaiterDrainedByShutdown rarely checks a parked waiter. With the runtime fix the parked order no longer flakes.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Swish! 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. |
… wait (#144) ## Why The operator wants every open follow-up fixed and landed, one small reviewed PR each. `TestConcurrentBoundedByMaxConcurrent` checked the handler log once, 80 ms after the second event, then freed the slot. A slot wait that lets the second handler run after that one check passed every run. This PR makes the test check the bound for the waiter's whole wait. Follow-up from #141. ## What changed - The test runs in a `testing/synctest` bubble. After it posts the second event, it sleeps on the fake clock until 1 ns before the waiter's `DetachTimeout` deadline, then checks that only the first handler ran. The fake clock moves only while every goroutine in the bubble is durably blocked. So the check covers the full slot wait, and the test takes no wall-clock time. - `synctest.Wait` replaces the 2 s polling loops that checked "first handler started" and "second runs after the slot frees". ## Scope - Test-only. No production code and no export hook. - The 40 ms sleep in `TestConcurrentSlotWaiterDrainedByShutdown` stays as it is. #141 explains why. ## Blast radius The change touches one test. If the bound check fails, a `sync.OnceFunc` release frees the blocked handler, so the bubble reports the failure. Two limits remain. If the runtime never frees the slot, the test prints its failure and then `synctest` panics because blocked goroutines remain, which stops the package's test binary. If a goroutine blocks on a `sync.Mutex`, `synctest.Wait` waits until the `go test` timeout, so that failure can take up to 10 minutes to report. ## Verification - I committed two wrong runtimes on local throwaway branches and never pushed them. A: the slot channel holds `MaxConcurrent+1`. B: the slot wait gives up after 100 ms and runs the handler anyway. The old test caught A (960 of 960 loaded runs) and missed B (0 of 200 normal runs, 0 of 960 loaded runs). With B, every other `TestConcurrent*` test also passes. - The new test fails A and B in 400 of 400 normal runs (`-cpu=1,4`) and fails B in 960 of 960 loaded runs (16 CPU spinners, 24 batches of 40 at `-test.cpu=1`). On this head it passes 400 of 400 normal runs and 960 of 960 loaded runs. - vet, root, adapter, and example tests, `gofmt -l`, and `go test -race .` all pass on the head. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Why
The operator wants every open follow-up issue fixed and landed, one small reviewed PR each. This PR fixes #140. The issue blamed the 40 ms sleep in
TestConcurrentSlotWaiterDrainedByShutdown. Instrumented runs show a runtime race instead. In every failure, the waiter had already been waiting on the slot for about 40 ms when Shutdown started. Shutdown cancels the detached tails one at a time. The first handler can return and free its slot before Shutdown reaches the waiter's context. The waiter then wins the slot and runs its handler instead of abandoning the wait.What changed
acquireDetachedConcurrencySlotholds this check. The sync path still calls the unchangedacquireConcurrencySlot.TestConcurrentSlotFreedDuringShutdownIsAbandonedwidens the race window. It gives the first handler's context 20000 child contexts, which Shutdown cancels after it wakes that handler. Then it checks that the waiter never runs.TestConcurrentSlotWaiterDrainedByShutdownis unchanged. Its 40 ms sleep stays, because without it the waiter reaches the slot after Shutdown in almost every run, and the test then rarely checks a parked waiter.Scope
Blast radius
The change only affects a detached tail that wins a slot after Shutdown began. That tail's context was about to be cancelled anyway. The normal path makes one extra
baseCtx.Err()read per slot. All Concurrent, Queue, Deferred, Burst, and Admission tests pass without changes.Verification
-test.cpu=1) onTestConcurrentSlotWaiterDrainedByShutdown, with the 40 ms sleep in place. On main7c6ce4ait failed 8 of 960 runs, all atconcurrent_test.go:221(no "slot wait abandoned" log). On the head911ba61it failed 0 of 960 in each of three runs. This test is identical on main and on the head, so the runtime fix is the only difference. A run without the sleep cannot show this, because removing the sleep alone also gives 0 of 960.55cf4a2) in 150 of 150 runs at-cpu=2,4,96, and in 20 of 20 runs with-race. On the fix it passed 150 of 150.acquireConcurrencySlotfrom the tail fails the new test in 10 of 10 runs.Closes #140
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high