fix(stateio): hold lock across state read-modify-write to stop dropped RecordApplied entries - #56
Merged
Merged
Conversation
added 3 commits
July 7, 2026 15:48
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.
Root cause:
RecordApplieddid guard itsLoadState -> RecordApplied -> SaveStatesequence with astate.yamlflock, but Linuxflockis per-process. Concurrent goroutines in the same claudecm process could each acquire an exclusive lock descriptor for the same sidecar and then run the read-modify-write at the same time. Each goroutine loaded a stale snapshot ofstate.yaml, mutated only its own(tool, path)entry, and atomically renamed that snapshot over the previous one. The final writer won and silently dropped entries written by other goroutines/process-local callers.Interleaving diagram:
RecordApplied(claude_code, settings.json)and acquiresstate.yaml.claudecm-lock.state.yamlsnapshot S0.RecordApplied(codex, auth.json); Linux treats the flock as process-owned, so B also proceeds instead of waiting.SaveStatewrites snapshot S0+A with atomic temp+fsync+rename+parent-fsync.SaveStatewrites snapshot S0+B with the same atomic write ritual.state.yamlcontains B but not A; external-drift detection later sees A's file as missing/stale.Fix description: added
FileStorage.UpdateState, a shared state transaction helper that takes an in-process mutex, then the existing gofrs/flock sidecar lock on.claudecm/state.yaml, then performsLoadState -> mutate -> SaveStatewhile both guards are held.SaveStatestill uses the existing atomic temp-file write, fsync, rename, and parent-directory fsync path; the flock sidecar model is unchanged and there are no fallback paths.stateio.RecordAppliednow delegates to that helper, and switch's active-profile pointer plus all committed-file LastApplied anchors are written in one state transaction.Other-instances check: searched for
LoadState,SaveState,RecordApplied, and profile/state update call sites underinternal,cmd, andpkg. Found the same state read-modify-write pattern incmd/switch.go,cmd/rename.go,cmd/delete.go, andinternal/config/manager.go(SetActive/DeleteProfile). Those production writers now useUpdateState. Remaining directSaveStatecalls found by the search are test setup/helpers or read-only command paths usingLoadStateonly.Verification evidence:
Reproduction before the fix:
Focused race loop after the fix:
Full race suite:
Note: repeated exact
go test -race ./...attempts using the default/tmpon this host were disrupted by unrelated concurrent test processes and very slow ext4 fsync waits. The same command completed cleanly whenTMPDIR=/dev/shmmoved test temp homes off the contended root filesystem; the production write path and atomic fsync/rename invariants are unchanged.