fix(connect): close TOCTOU race in connect sync lock acquisition#113
Open
omergk28 wants to merge 1 commit into
Open
fix(connect): close TOCTOU race in connect sync lock acquisition#113omergk28 wants to merge 1 commit into
omergk28 wants to merge 1 commit into
Conversation
The sync lock was a stat-then-write: two processes racing past the existence check could both acquire, both load the same LastSequence, and both write duplicate entries into .context/hub/. Replace the pair with the atomic io.SafeTryLock (O_CREATE|O_EXCL, single syscall) and release via io.SafeUnlock — the same primitive the dream pass already uses, preserving the os.ErrExist contract for callers. - LockSentinel removed: the lock is the file's existence, not its content, and the racy write was the constant's only consumer. - state_test.go regression-pins the contract: 16-way contention yields exactly one winner, release frees the next sync, a corrupt state file does not leak the lock, and the lock path stays at <ctxDir>/hub/.sync.lock. Closes ActiveMemory#93. Spec: specs/fix-connect-sync-lock-toctou.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Omer Kocaoglu <omergk28@gmail.com>
45d4ae2 to
9e60ec8
Compare
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.
Summary
loadStateininternal/cli/connection/core/syncguardedctx connect syncwith a stat-then-write pair — racy by construction. Two processes racing past theos.Statcould both acquire, both load the sameLastSequence, and both write duplicate entries into.context/hub/.This PR replaces the pair with the atomic create-or-fail primitive the codebase already ships for exactly this purpose:
io.SafeTryLock(O_CREATE|O_EXCL, one syscall) +io.SafeUnlock, with prior art atinternal/cli/dream/core/pass/pass.go:62-70.!acquiredmaps to the pre-existingos.ErrExistcontract, so caller-facing behavior is unchanged.Closes #93.
Changes
internal/cli/connection/core/sync/state.go— atomic lock acquisition; release delegates toSafeUnlock(warn-on-failure logging kept)internal/config/hub/{hub.go,doc.go}—LockSentinelremoved: the lock is the file's existence, not its content, and the racy write was the constant's only consumerinternal/cli/connection/core/sync/state_test.go— new; the package previously had zero testsspecs/fix-connect-sync-lock-toctou.md— spec per the project's spec-per-commit invariantTests (mapped to the issue's Tests Required)
os.ErrExistTestLoadState_RejectsConcurrentSyncs(16-way; winners held until all results are collected, so the count is deterministic)TestLoadState_ReleaseRemovesLock+TestLoadState_ReleasesLockOnCorruptState(post-acquisition failure must not leak the lock)<ctxDir>/hub/.sync.lockTestLoadState_LockFileLocationVerification
winners = 16, want exactly 1, proving both that the race is real and that the test catches it. Against the fix: exactly 1 winner, 15 ×os.ErrExist.go test -race -count=10 ./internal/cli/connection/core/sync/— clean.Notes
.context/shared/— that's the pre-rename path; the renderer joinscfgHub.DirHub(render.go:32), so the spec and this PR say.context/hub/.flockalternative (Unix-only;SafeTryLockmatches the existing sentinel model).make auditfix that previously rode along in this PR has been split into fix(hack): guard empty-array expansion in lint-drift for bash 3.2 #117, per the contributing guide's one-concern-per-PR preference. This PR is now surgical on ctx connect sync lock has a TOCTOU race; concurrent syncs can both pass the check #93. (Runningmake auditlocally on stock-macOS bash 3.2 needs fix(hack): guard empty-array expansion in lint-drift for bash 3.2 #117; CI and Linux are unaffected.)🤖 Generated with Claude Code