fix(edit): stop two C3 sessions from clobbering the same file - #55
Merged
Conversation
c3_edit serialized same-file edits with a per-file threading.Lock. That only holds within one process, and every Claude Code session spawns its own c3-mcp stdio server -- so two sessions editing one file could interleave their read -> replace -> write cycles and lose an edit with no error on either side. Create mode was worse: the lock was only obtained after the create branch had already returned, so two agents creating the same path both reported success and one file silently won. _edit_lock now composes the existing threading.Lock with the cross-process _FileLock already used by task_store and time_tracker, and is held across the whole cycle for create, single-edit and batch alike. handle_edit keeps path resolution and the vault/access guards, then hands off to _edit_locked under the lock -- policy denials still outrank contention, so an agent is never told a file is busy when it was never allowed to write it. The lock sidecar is machine-global, keyed by sha1 of the normcased resolved target path, in ~/.c3/edit_locks. It deliberately does not live under svc.project_path: c3_project(action='edit') proxies into handle_edit with the CALLER's svc, so a project-scoped sidecar would hand two agents editing one file two different locks. normcase rather than casefold, so two spellings collide on Windows and stay distinct on POSIX. _FileLock raises TimeoutError where threading.Lock blocked forever. Unbounded waiting would hang the MCP server behind a wedged holder, so contention surfaces as a [c3-lock:busy] refusal that names the anti-pattern -- agents reach for c3_shell within about two turns otherwise, as Mask Guard taught. Tests use a real second process. An in-process thread would prove nothing about the failure this fixes. docs/agent-locks.md is amended in place: the spec claimed ~15 LOC, missed the create-mode hole, missed the TimeoutError semantics, and implied the Layer A sidecar was project-scoped. Those corrections are recorded rather than quietly patched over. Claude-Session: https://claude.ai/code/session_01Sbd9NbQfDcoJvvURD7KCVf
CI caught a lock that could be keyed two ways, which is not a lock. handle_edit resolves a path before taking the sidecar, but _lock_sidecar did not resolve it itself -- so the exclusion tests, which computed the sidecar from the unresolved path, had the holder locking one file while c3_edit locked another. Every one of them passed while proving nothing. It stayed hidden because both environments I ran it in lack the constraint: Linux CI has a real /tmp, and this dev box has a username too short to be given an 8.3 alias. It surfaced on macOS, where TemporaryDirectory returns /var/... and resolve() rewrites it to /private/var/..., and on Windows CI, where the runner's home resolves RUNNER~1 to runneradmin. _lock_sidecar now resolves internally, so no caller can key it the other way -- the same reasoning as deriving FleetDeck's matcher from its gate rather than maintaining two lists. Non-strict resolve, so create mode still works on a path that does not exist yet; OSError falls back to hashing the given path rather than silently locking nothing. Tests: _hold now asserts its sidecar matches the resolved one, so a future drift fails naming its own cause instead of looking like "the edit wasn't blocked". Added test_sidecar_is_spelling_independent (verified non-tautological -- the ../ spelling produces a different digest without the fix) and test_sidecar_follows_symlinked_parent, which reproduces the macOS /var case directly and skips on Windows where symlinks need privilege. Recorded the macOS case-insensitive-volume residual in the coverage matrix rather than leaving it implied: there API.py and api.py are one file but get two sidecars. Claude-Session: https://claude.ai/code/session_01Sbd9NbQfDcoJvvURD7KCVf
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.
Implements Phase 1 of
docs/agent-locks.md(merged in #54), and amends the spec with what building it found.The bug
c3_editserialized same-file edits with a per-filethreading.Lock(cli/tools/edit.py:20). That holds within one process, and every Claude Code session spawns its ownc3-mcpstdio server. Two sessions editing one file could interleave their read → replace → write cycles and lose an edit, with no error on either side.Create mode was worse and wasn't in the spec: the lock was only obtained at
edit.py:298, after the create branch had already returned. Two agents creating the same path both reported success and one file silently won.The fix
_edit_lockcomposes the existingthreading.Lockwith the cross-process_FileLockalready used bytask_storeandtime_tracker, held across the whole cycle — create, single-edit and batch alike.handle_editkeeps path resolution and the vault/access guards, then hands off to_edit_lockedunder the lock. Ordering is deliberate: policy denials outrank contention, so an agent is never told a file is busy when it was never allowed to write it.Three decisions worth reviewing
Sidecar is machine-global, not project-scoped.
~/.c3/edit_locks/<sha1>.lock, keyed on the normcased resolved target path. It must not live undersvc.project_path, becausec3_project(action='edit')proxies intohandle_editwith the caller'ssvc— a project-scoped sidecar would hand two agents editing one file two different locks and no mutual exclusion at all. This doesn't contradict §2 of the spec: lease state is target-project-scoped; the Layer A mutex is machine-scoped and has no project identity.os.path.normcase, notcasefold. Windows paths are case-insensitive so two spellings must collide; POSIX paths are case-sensitive so they must not.test_case_follows_platformasserts exactly that, per platform.Contention is a refusal, not a wait.
_FileLockraisesTimeoutErrorwherethreading.Lockblocked forever. Unbounded waiting would hang the MCP server behind a wedged holder, so it surfaces as[c3-lock:busy]— with an explicit "do not route around it via c3_shell or native Write", because agents reach for a shell one-liner within about two turns otherwise. That lesson is already banked from Mask Guard.Tests
tests/test_edit_locking.py, 12 tests. The exclusion tests spawn a real second process to hold the lock — an in-process thread would prove nothing about the failure this fixes. Coverage: sidecar identity and platform case rules, edit/create/batch all blocked under contention, lock released cleanly so the next edit succeeds, and the uncontended path unchanged.Full suite: 1730 passed, 1 failure —
tests/test_shell_robustness.py::TestPopenUtf8::test_forces_utf8_env, which is pre-existing onmainand unrelated (shell.pysetsPYTHONIOENCODING='utf-8:surrogateescape', the test asserts'utf-8'). This branch touches onlyedit.py. Worth a separate one-line fix — it's red on main right now.Spec amendments
docs/agent-locks.mdis corrected in place rather than quietly patched: it claimed ~15 LOC (it's ~40 plus an extract-method refactor), missed the create-mode hole, missed theTimeoutErrorsemantics, and implied the Layer A sidecar was project-scoped. §5 records all three, §6 adds thebusyvshelddistinction, §13 marks P1–P2 done and keeps P3 gated on denial data.Companion change
Phase 2 lands in AgentSync (drknowhow/AgentSync) —
mcp__c3__c3_editadded tohook.EDIT_TOOLS, with the PreToolUse matcher now derived from that same tuple so the gate and the matcher can't drift apart again. That drift is how FleetDeck came to be silently blind in every C3 project.https://claude.ai/code/session_01Sbd9NbQfDcoJvvURD7KCVf