BACK-575 - Fail fast instead of silently losing concurrent task edits - #860
Conversation
updateTaskFromInput was an unlocked read-modify-write shared by CLI task edit, MCP task_edit and the web PUT handler, so two concurrent edits of one task silently dropped a write while both callers were told they succeeded. Adds FileSystem.withTaskLock beside withCreateLock, sharing its proper-lockfile mechanics through a new withLockTarget. The task lock fails fast (no retries): on contention the loser gets 'Edit failed: <id> is being modified by another process; retry if appropriate' and the caller decides whether to retry - nothing waits, merges or retries automatically. The CLI exits non-zero with that message, the web PUT returns 409 and MCP task_edit reports OPERATION_FAILED. The lock target is the task file itself because proper-lockfile keys its in-process registry by target path. Its lockfile lives under the project's backlog directory rather than the shared git common dir: an edit protects one file, so sibling worktrees editing their own copy of a task must not fail each other, and edits no longer spawn git rev-parse. The snapshot is re-read inside the lock. Locking only the write would still lose an update whenever one writer releases before the next acquires, because the second would apply its changes to a pre-lock snapshot. Out of scope, left as follow-ups: reorderTask/updateTasksBulk, draft edits via updateDraftFromInput, and the TUI external-editor write path. Concurrency proof reported by iRonin in withdrawn PR #852, whose test harness shape this adapts to fail-fast semantics. Refs #843
The Draft-status branch of updateTaskFromInput delegated to demoteTaskWithUpdates before the task lock was taken, so demotion's read-modify-write - apply input to a pre-lock snapshot, save the draft, unlink the task file - stayed unprotected. A concurrent edit could be told it succeeded and then have its write erased by the demote's stale snapshot. Reachable from the web PUT and MCP task_edit, whose status enum includes 'Draft'; plain CLI task edit rejects 'Draft'. The lock now lives inside demoteTaskWithUpdates with the same in-lock re-read. That placement, rather than wrapping the branch in updateTaskFromInput, is what closes the funnel: editTaskOrDraft calls demoteTaskWithUpdates directly, skipping updateTaskFromInput entirely, so the MCP path would otherwise stay open. The two locks never nest. Inside the task lock the demote waits on the create lock, so the order is always task lock then create lock. Every withCreateLock body was checked and none takes a task lock or re-enters the edit funnel, so the order is acyclic. Also maps ENOENT during lock acquisition - the task file moved or was removed between snapshot load and lock acquisition - to a clear message instead of a raw errno, and therefore to 409 / OPERATION_FAILED like other contention. Tests: the demote race, stretched by holding the create lock so the demote parks in its draft-id allocation, asserting the edit either fails loudly or survives in the draft; and the editTaskOrDraft entry point, asserting a held task lock blocks the demotion and leaves the task file intact. Refs #843
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3dd0a43e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Alex's agent: We verified this scenario empirically with three schedules: the reported interleaving, strict sequential A-then-B, and the proposed lock-before-read ordering all end in the identical state ( The lock's contract is fail-fast on overlapping writers plus an in-lock re-read, which guarantees every final state is explainable by a sequential ordering of the operations that reported success, with overlapping losers failing loudly. What remains is a client deciding |
1 participant
Fixes #843.
updateTaskFromInputwas an unlocked read-modify-write serving CLItask edit, MCPtask_edit, and the web PUT: two concurrent edits of the same task silently lost one write while both reported success (measured: 7 of 8 concurrent writes lost pre-fix).The edit funnel now runs inside a filesystem-level task lock (proper-lockfile,
retries: 0) with an in-lock re-read. On contention the second writer fails fast withEdit failed: TASK-X is being modified by another process; retry if appropriate.— no waiting, no merging, no auto-retry; the caller decides. CLI exits non-zero, the web server returns 409, MCP returns OPERATION_FAILED. The Draft-demotion branch is locked atdemoteTaskWithUpdatesitself so both its entry points (web funnel and MCP's direct call) are covered. Locks live underbacklog/.locks/(per-checkout, so sibling worktrees never falsely contend); a crashed holder's stale lock auto-breaks after 10s.Design reference and test-harness shape come from @iRonin's withdrawn PR #852 and the excellent #843 report; semantics differ deliberately (fail-fast per maintainer decision, vs wait-and-re-read).
Verification: 9-test concurrency suite plus a parallel smoke script (8 concurrent real-CLI writers: exactly one winner, losers loud, file content matches the winners exactly); each test confirmed to fail on the unfixed code; kill -9 stale-lock recovery verified; independently reviewed including a live exploit of the demote race, which is now closed. Full suite 1906 pass / 0 fail on the rebased head.