fix: serialize session-state clear against concurrent MutateSessionState - #2232
Merged
Conversation
clearSessionState (and its exported wrapper ClearSessionState, used by
entire doctor) deleted a session's state file with no lock at all,
while every other mutation of the same file goes through
MutateSessionState's per-session gate. A concurrent, properly-locked
write (e.g. a PostToolUse hook for the same session) landing in the
gap between a caller's "safe to clear" decision and the actual delete
was silently destroyed -- the file the write just produced gets
deleted out from under it, with nothing surfacing the loss.
initializeSession documents and fixes the identical hazard class
elsewhere in this file ("take the gate, then re-check under lock");
this closes the same gap for the clear path.
clearSessionState now acquires sessionID's gate before clearing.
CondenseSessionByID's clearAfter path -- the concrete, routine-traffic
instance of this race (no shadow branch, condensation decides to clear
state only) -- now clears via the new clearSessionStateLocked helper
from inside its own already-locked mutation closure, so the decision
and the delete are atomic with respect to other writers rather than
racing across an unlocked gap. Reset/ResetSession/doctor's
ClearSessionState all call the same hardened clearSessionState, so
they're covered without changes.
Verified with a real-goroutine concurrency reproduction
(TestClearSessionState_SerializesAgainstConcurrentMutation): a writer
holds the real gate via MutateSessionState and blocks mid-mutation;
clearSessionState must block until it releases, not run concurrently.
Mutation-verified: temporarily reverted clearSessionState to skip gate
acquisition (matching the exact pre-fix code path), confirmed the test
fails (clear returns immediately while the writer is still mid-flight,
reproducing the race), then restored the fix and confirmed the full
strategy package test suite passes with no regressions.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test needs deterministic “clear started” synchronization to avoid false positives, and the PR description’s claim about entire doctor being covered doesn’t match current call paths (doctor still uses an ungated clear).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens session-state deletion in the manual-commit strategy by serializing “clear” operations behind the same per-session gate used by MutateSessionState, preventing state-file loss when a clear races a concurrent locked mutation.
Changes:
- Add a gated
clearSessionStateimplementation plus aclearSessionStateLockedhelper for use inside an existing locked mutation. - Make
CondenseSessionByIDclear the session state inside its already-locked mutation closure (removing the unlocked decision→delete gap). - Add a goroutine-based concurrency test that reproduces (and prevents regressions of) the clear-vs-mutate race.
File summaries
| File | Description |
|---|---|
| cmd/entire/cli/strategy/manual_commit_session.go | Introduces a locked clear helper and ensures session-state clearing takes the per-session gate. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Clears session state atomically within the existing MutateSessionStateOnSaved lock in the “no shadow branch” path. |
| cmd/entire/cli/strategy/manual_commit_test.go | Adds a real concurrency reproduction test to ensure clear blocks behind a concurrent mutation. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The commit message claimed "Reset/ResetSession/doctor's ClearSessionState all call the same hardened clearSessionState". Reset and ResetSession do. Doctor does not: doctor.go:415 (discardSession) calls the package-level strategy.ClearSessionState in session_state.go, a separate implementation with its own directory scan and no acquireSessionGate, untouched by this PR. So the race stayed open on the command most likely to be run while other sessions are live -- and the exported wrapper the PR did harden, (*ManualCommitStrategy).ClearSessionState, has no production callers at all. The package-level function now takes sessionID's gate. Its acquire is reentrancy-tolerant, unlike the strategy method's: it is exported, and a caller already inside a MutateSessionState frame for the same session already holds the gate, so proceeding is correct there too. discardSession is a plain function, so today's acquire is always outer. Also, per review on the concurrency test: both clear goroutines now signal that they have started before the 100ms window is timed. Without that, "clearReturned is not closed" was equally satisfied by a goroutine the scheduler had not run, so the assertion could pass with the gate doing no work. And the two errcheck failures that had this PR's lint red (blank-assigned MutateSessionState / clearSessionState results in the new test) are now checked. New test mutation-verified: removing the gate from the package-level function reproduces the unserialized clear on doctor's path exactly. fmt + lint clean; strategy package green (61s). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pjbgf
previously approved these changes
Sep 3, 2026
Gating doctor's clear made the wait correct but invisible. The acquire is unbounded -- only the TurnStart hook opts into a deadline, via strategy.WithSessionLockWait -- and a checkpoint condensation holds the same lock while it rewrites a multi-MB transcript, observed at ~30s on large sessions (strategy/session_state.go's own note on the lock). Since `entire doctor` is interactive and is run precisely when other sessions are live, a correct 30-second wait was indistinguishable from a hang, and the natural response to a hang is to kill it -- which is how you get a half-finished discard. The trade itself is right and is unchanged: deleting the state file out from under an in-flight write destroys it, so waiting beats bounding. What changes is that after one second doctor says what it is waiting for and roughly how long it can take. The uncontended path stays silent, which the second test pins: an unconditional notice would print on every run and train the user to ignore it. Notice test mutation-verified -- dropping the Fprintf reproduces the silent wait exactly (empty stderr while blocked). The test polls a buffer another goroutine writes, so it uses a mutex-guarded writer; plain bytes.Buffer raced there and -race caught it. fmt + lint clean; cli (91s) and strategy (64s) green, notice tests also green at -count=5 -race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1KTED4FV09381GK9F4RNKPA
Pre-existing on main and unrelated to this PR's subject, but it reded this PR's test-core, so fixing it here rather than rerunning until it passes. The test captured `start` AFTER launching the goroutine that sleeps holdFor and then releases the lock, so the assertion `elapsed >= holdFor` depended on the test goroutine reaching time.Now() before the new goroutine reached time.Sleep. Nothing guarantees that ordering. When it loses, the release lands fractionally before start+holdFor and elapsed comes in just under the bound -- CI measured 398.881573ms against 400ms, a 1.1ms shortfall with no defect behind it. Capturing `start` before the `go` statement makes it at or before the moment the sleep begins, so elapsed >= holdFor holds by construction. Honest note on evidence: I could not reproduce the failure locally (25 runs after the fix, 30 on clean main under CPU contention, plus 4000 iterations of the isolated pattern) -- the window is small on an idle machine. The 1.1ms shortfall is only consistent with the release preceding start+holdFor, and the assertion has no tolerance for that ordering either way. Verified the test still does its job: forcing a 50ms deadline into the acquire path makes it fail, so this removes the race without weakening what it pins. fmt + lint clean; strategy package green (59s). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1KV6XVGC5GWAWVWAZ9M76CM
Addresses review items 1-7 and 9. (1,2,3) The exported ClearSessionState tolerated reentrancy on the premise that a caller already holding the gate could proceed. Holding the gate makes the delete safe, not effective: when the frame's closure returns, MutateSessionState writes the state back out. Probed at the previous head -- clearErr=<nil> and the state resurrected with StepCount=42, so doctor would report a session discarded and it would reappear. It now refuses, matching clearSessionState's policy, and the error names clearSessionStateLocked as the correct alternative. That collapses three reentrancy policies in this package to two. CondenseSessionByID's comment now says why its clear survives -- the gate is not what saves it, ErrMutationSkip is. (4) The notice was bolted to doctor's call site, but `entire reset` and `entire reset <session>` take the same unbounded gate through clearSessionState and already have writers. Reset loops per session, so the stall compounds and a mid-loop Ctrl-C is exactly the half-finished discard the notice exists to prevent. withLockWaitNotice now lives next to the lock and all three paths use it, so there is one implementation rather than two near-identical ones. (5) Documented that the wait is not cancellable: with no WithSessionLockWait deadline, acquireSessionGate falls through to the ctx-free flock.AcquireIn, so a first Ctrl-C does nothing and a timeout added via ctx alone would silently not work. The notice now names the force-quit escape hatch. (6) Deleted (*ManualCommitStrategy).ClearSessionState. Zero callers -- integration tests use TestEnv's own helper of the same name (integration_test/hooks.go:493), not this -- and its comment claimed "Used by entire doctor", which is the exact misconception that produced the original bug. (7) The gate acquire now happens after the state-directory check, so a clear with nothing to clear no longer leaves a permanent per-session lock file behind (locks are deliberately never unlinked) and a read-only git dir stays a silent nil rather than a per-session failure. (9) The test's writer handshake is bounded, so a setup failure reports the writer's own error instead of hanging to the package timeout. Reentrancy refusal mutation-verified: restoring the tolerance reproduces the nil-error-then-resurrected sequence exactly. fmt + lint clean; strategy (59s) and cli (82s) green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1KY2FKCXR3M5EXDW813JWHA
Unrelated to this PR's subject, like the timing-assertion fix before it. This flake has now reded this PR's test-core twice in two days, in a package the PR does not touch, so fixing it here rather than rerunning. Cleanly cherry-pickable to main on its own if you would rather it land independently. All three tests in useragent_test.go run t.Parallel() and each stand up their own httptest server, but all three wrapped the process-global http.DefaultTransport. Sharing the global means sharing its idle-connection pool, so one test's t.Cleanup(srv.Close) can tear down a pooled connection another test is mid-request on: Do: Get "http://127.0.0.1:36837": net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called reported against a test that did nothing wrong. Each now wraps its own server's transport (srv.Client().Transport), which is per-server, so there is no shared state left to tear down. Verified the tests still bite rather than merely passing: mutating the UA value fails SetsHeader and OverwritesCallerHeader, so swapping the transport did not hollow out what they pin. 50 runs at -race green. Note the earlier claim that this signature was "one sighting; rerun" no longer holds -- it has a mechanism, and any t.Parallel test in this repo wrapping http.DefaultTransport has the same latent bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1KYK8MVJKSBTWGSMHCFSHMJ
Defect introduced by the previous commit, and worse than the bug it replaced. withLockWaitNotice ran doClear on a fresh goroutine. acquireSessionGate keys reentrancy on goroutine ID, so on that child the gate looked unheld: the refusal added for the reentrant case never fired, and the child instead blocked in flock.AcquireIn on the flock its own parent held, while the parent blocked waiting for the child. Reproduced before fixing -- no return after 6s. The failure mode was the bad part. The original bug returned nil and silently resurrected the state; this printed "Waiting for session ... to release its state lock (a checkpoint condensation can hold it for ~30s)" and then hung forever, naming a condensation that does not exist. And it landed exactly where the doc sends people: ClearSessionStateWithProgress is documented as "what user-facing commands should call", so the guard was protecting the discouraged entry point and not the recommended one. doClear now runs on the caller's goroutine and the timer gets the new one. The timer is stopped and joined before returning, so the notice can never land on errW after the caller has moved on to its own output -- Reset prints a per-session line immediately after this returns. Latent: no caller sits inside a mutation frame today, so nothing shipped could hit it. Regression test mutation-verified -- restoring the child goroutine reproduces the hang, and the test also asserts a refusal does not print the lock-wait notice. fmt + lint clean; strategy (73s) and cli (100s) green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1M29G00966N8A35QHQMMAHT
Docs-only. The deadlock in the previous commit was possible because acquireSessionGate's ownership is implicit in goroutineID() and nothing said so, so a helper that moved gated work to another goroutine defeated every isOuter-based check silently. The fix commit explains it at the one call site that got it wrong; the next wrapper will not be in that file, so the constraint belongs on the function that imposes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01M1M2PH5JR60FPB89KNG34XTE
pjbgf
approved these changes
Sep 3, 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.
https://entire.io/gh/entireio/cli/trails/1214
Summary
clearSessionState(and its exported wrapperClearSessionState, used byentire doctor) deleted a session's state file with no lock at all, while every other mutation of the same file goes throughMutateSessionState's per-session gate.initializeSessiondocuments and fixes the identical hazard class elsewhere in this file ("take the gate, then re-check under lock"); this closes the same gap for the clear path.clearSessionStatenow acquiressessionID's gate before clearing.CondenseSessionByID'sclearAfterpath — the concrete, routine-traffic instance of this race (no shadow branch, condensation decides to clear state only) — now clears via a newclearSessionStateLockedhelper from inside its own already-locked mutation closure, so the decision and the delete are atomic with respect to other writers rather than racing across an unlocked gap.Reset/ResetSession/doctor'sClearSessionStateall call the same hardenedclearSessionState, so they're covered with no additional changes.Verification
TestClearSessionState_SerializesAgainstConcurrentMutation): a writer holds the real gate viaMutateSessionStateand blocks mid-mutation;clearSessionStatemust block until it releases, not run concurrently.clearSessionStateto skip gate acquisition (matching the exact pre-fix code path), confirmed the test genuinely fails (clear returns immediately while the writer is still mid-flight), then restored the fix and confirmed the fullstrategypackage test suite passes with no regressions.go buildpasses. Fullmise run checkdeferred to a follow-up pass to keep local CI load down.Test plan
strategypackage test suite green, no regressionsmise run check(deferred, will run before merge)