Fix session teardown races and run cli tests under -race#267
Merged
Conversation
Running the cli tests under -race (possible since #265) flagged five session teardown races, three of them in the MCP stop path: Destroy freed the native session while another goroutine was still inside an FFI call on it. The exposure is structural, not a timing fluke - the goroutine that unparks a blocked Feed (the drain-resume and break_at auto-step goroutines OnBreak spawns) is by construction still alive at the moment the feed goroutine exits, which is exactly when front-end teardowns proceed to Destroy. A use-after-free on the native side, not just a Go-visible race. - Every session-crossing Runner method now registers an in-flight op; Destroy sets closed (refusing new ops), drains, waits out the ops, and only then frees the session. Idempotent, safe under concurrent calls. - ProcessOne registers too, so Destroy is safe even against a feed still in flight and a straggling source loop stops on its next event. - The OnBreak-spawned goroutines register unconditionally rather than through the closed gate: they are what unparks the Feed that Destroy is waiting out, so gating them would deadlock the teardown. Safe because they spawn under the feed's own held op. - After Destroy, control verbs no-op and inspection returns ErrSessionDestroyed instead of panicking on a freed session. - TestRunner_Destroy_WaitsForInflightStep pins the guarantee, mirroring the resume-then-teardown shape every front-end has; the race detector enforces it.
The other two races -race flagged: the harness cleanup destroyed the raw session while the adapter's continue handler was still inside its FFI call on it (the handler issues step verbs on its own goroutine and nothing joined it). Both setup helpers now register a runner.Destroy cleanup, which waits out in-flight session calls before freeing the session; the raw session.Destroy cleanup stays as the failure-path backstop and no-ops after the runner has destroyed the session. The history-store variant also closes its store.
The vscode dev loop had the same exposure the race detector caught in the MCP and DAP tests, just on untested paths: every in-loop teardown and restart destroyed the raw session while the adapter's handler goroutines could still be inside a session call. All seven in-loop exits now go through Runner.Destroy, which waits those calls out. The two pre-loop raw destroys stay - no server is serving yet, so nothing can race them. Enabling that meant fixing history-store ownership: Runner.Destroy used to close r.history, but the dev loop shares one store across every restart iteration's runner, and the first restart would have closed it under the next runner (silently - ProcessOne ignores insert errors). The Runner now only borrows the store; creators close it (the MCP server's closeSession after destroying the runner, dev's existing defer, test cleanups).
Unit and integration recipes both. The suite went from red (five data races) to green over the preceding commits, and -race is what found them all - this keeps it that way. It also doubles as a checkptr gate on the FFI boundary. Measured locally: unit suite 5s -> 50s wall on 12 cores; race-instrumented artifacts land in the Go build cache, so CI steady state pays far less. Validated against the nightly KurrentDB for the integration recipe.
- Pin the precondition the OnBreak goroutines' unconditional ops.Add relies on: Feed is only ever driven through ProcessOne. - State that a concurrent second Destroy returns before the first one's teardown completes. - Note the runtime's stranded-debug-command hazard, which a waiting Destroy now surfaces as a hang instead of a leaked goroutine (fix belongs runtime-side; ticketed separately).
George-Payne
added
bug
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo: the closed-gate no-ops on doStep/Pause/ClearBreakpoints returned nil, reporting a step that never happened and diverging from the inspection methods' ErrSessionDestroyed. Refuse loudly instead - every caller already routes these verbs' errors (the runtime's not-paused error uses the same paths). CollectState and GetPartitionState keep their existing nil-session soft contract; a closed runner reads the same as no session.
The drift check degrades to no-drift by design when the /info/options read misses nodeOptionsHTTPTimeout's 3s budget - it's advisory, and a status shouldn't stall on a firewalled HTTP port. A race-instrumented CI runner with every package binary in flight can miss that window, which read as an empty configDrift in the envelope (one CI flake, one pass, on identical code). The test now retries the command up to three times, so it asserts the envelope plumbing rather than a single 3-second slot.
The race-enabled CI run caught deploy.Recreate stranding a projection:
KurrentDB deletes asynchronously, so the rebuild's create can bounce off
the still-registered name ("Envelope callback expected Updated, received
Conflict instead") and leave the projection deleted but not recreated.
Not a test artifact - a real gaffer recreate against a busy server hits
the same window; the recovery text in the error message shows the
failure mode was anticipated rather than closed.
- deploy.Recreate: an optional RetryCreate predicate marks a create
error as settle-retryable; conflicts retry with backoff over a 10s
settle budget (per-attempt RPCTimeout unchanged) before giving up
with the existing recovery message. Context cancellation exits the
loop immediately.
- remote.IsCreateConflict: the predicate. The projections subsystem
replies Conflict through its command envelope inside an unclassified
gRPC Unknown, never AlreadyExists (documented on that sentinel), so
it matches the envelope text, plus the sentinel for a server that
someday returns the proper code.
- Wired at both call sites (gaffer recreate, deploy_recreate MCP tool)
and changesetted; unit tests pin retry-then-succeed, no-retry on
other failures, and cancellation.
waitForTimeout's 5s slack was starved out on CI with every package's race-instrumented binary in flight: the didClose lens-refresh test saw its condition true only after the budget expired. The parse pipeline caches before firing the refresh, so there is no ordering hole - the runner was just crawling. 30s costs passing tests nothing (waitFor returns on the first true poll); ctxTimeout scales with it.
The flake-check rerun caught TestServer_WorkspaceSymbolSkipsInvalidProjections waiting out its full budget: the walk-seeded gaffer.toml parse never landed. Diagnosis by construction: jsonrpc2.NewConn starts dispatching handlers the moment it returns, but Run stored s.conn/s.runCtxFn after constructing it. A fast client's initialize reply is sent by the read loop regardless of Run's progress, so its `initialized` can reach spawnWithCtx while runCtxFn is still nil - which silently drops the workspace walk, and nothing retries it. The same window could drop the watcher registration and initial diagnostics. Race instrumentation stretches the nanosecond window wide enough to hit on CI. Run now stores runCtx/cancel BEFORE constructing the conn, and handler dispatch blocks on a `ready` channel closed once the conn is stored, so no handler can observe the half-initialized state. Validated with 40 GOMAXPROCS=2 race iterations of the affected tests plus the full lsp suite under race.
George-Payne
added a commit
that referenced
this pull request
Jul 7, 2026
a debug verb racing a resume could strand its caller forever and then replay as a stale command at the next pause. The verbs did an unsynchronized paused check-then-enqueue while the command loop exits on the first Continue/Step without draining the queue; since #267's teardown gate, the stranded caller holds a session op, so the hang propagated to `Runner.Destroy` and wedged the MCP server's `closeSession` under its mutex. Drain-at-exit alone can't fix it - a verb that passed the check can enqueue after any drain - so the enqueue and the resume are made mutually atomic. - **`JintProjectionHandler.cs`** - `_debugSync` makes each verb's check+enqueue atomic with the command loop's resume+drain. Every loop exit (resume, exception, disposed-while-paused) goes through `ResumeAndDrainDebugCommands`: clear the pause state, then fail anything still queued, under the same lock - so an enqueued command is always either consumed or failed, never stranded and never carried across a pause (a stale `Continue` would have silently resumed a breakpoint the user never continued). All eight verbs check the command's error after their wait; `Dispose` drains as a backstop; an enqueue racing `CompleteAdding` gets the verb's own refusal message rather than the framework's. - **`DebugTests.cs`** - two regression tests: 25 iterations of 3 racing continues with join timeouts as strand detectors (fails pre-fix on the first iteration), and a stale-replay guard asserting the next pause waits for its own resume. - **`engine/runner_debug.go`** - the doc comments that recorded the hazard now state the guarantee (a verb always returns, so `Destroy`'s wait can't wedge on one). - **`.changeset/debug-command-strand.md`** - patch; the fix ships in the CLI's bundled runtime.
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
1 participant
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.
Turns
-raceon for the cli suite (possible since #265 removed the checkptr blocker) and fixes everything the detector and the race-loaded CI runs surfaced: five data races, two real product bugs, and two brittle test budgets. Merge-gated on three consecutive green CI runs on the head SHA.engine/runner.go+runner_debug.go- every session-crossing Runner method registers an in-flight op;Destroyrefuses new ops, drains, waits the in-flight ones out, then frees the session. Fixes all five flagged races (three via the MCP stop path, two via the DAP harness): the goroutine that unparks a blocked Feed is by construction still alive when teardowns run, soDestroywas freeing the native session under a live FFI call.ProcessOneregisters too, so a straggling feed can't race teardown. AfterDestroy, control verbs and inspection refuse withErrSessionDestroyedinstead of panicking or reporting a step that never happened.TestRunner_Destroy_WaitsForInflightSteppins the guarantee.cmd/dev.go+mcpserver/server.go+dap/adapter_test.go- all teardown paths route throughRunner.Destroy(the vscode dev loop had the same exposure on untested paths). History-store ownership moves to creators: the dev loop shares one store across restart iterations, so the Runner closing it would have silently dropped history after the first restart.deploy/apply.go+remote/errors.go- real bug caught by a race-loaded run: KurrentDB settles projection deletes asynchronously, sogaffer recreate/deploy_recreatecould bounce the rebuild's create off the still-registered name (envelopeConflict) and strand the projection deleted-but-not-recreated. The create now retries over a 10s settle window viaRetryCreate/remote.IsCreateConflict.lsp/lifecycle.go+handlers.go+server.go- real bug caught by rerun sampling:jsonrpc2.NewConnstarts dispatching handlers beforeRunstored the run state, so a fast client'sinitializedcould findrunCtxFnnil and silently lose the workspace walk (and watcher registration) for the whole session. The run context is now stored before the conn exists and handler dispatch gates on areadychannel closed once the conn is stored.waitForTimeoutgoes 5s -> 30s (free for passing tests).cli/justfile--raceontestandtest-integration; CI inherits viajust _test. Unit suite ~5s -> ~50s wall locally; race artifacts land in the Go build cache so CI steady state pays less. Integration validated under race against a nightly KurrentDB.runner-teardown-raceandrecreate-settle-retry(both patch).