Repository navigation
[EPIC] File Write Safety Prevent Concurrent Write Races Data Corruption #1375
Copy link
Copy link
Open
Description
Problem
The agent's file-write path has no version guard, no atomic publish, and no per-step
visibility:
- Concurrent writers silently overwrite each other's work: parallel subtasks share one
workspace, multiple VS Code windows are separate extension-host processes, and the
JetBrains plugin has confirmed real corruption under two IDE instances ([BUG][regression] Global _index.json full rewrite is unsafe under concurrent tasks (real corruption under JetBrains multi-agent) #1231). - Model writes land directly in the shared workspace with no checkpoint to roll back to;
task start creates no baseline at all. - No staleness detection: a file that changed after the model read it is silently
overwritten on the next write. - The write path carries artificial latency (1 s delay per write) on top of the missing
safety.
Known issues
| # | Title | Status | Fix in this epic |
|---|---|---|---|
| #1371 | MCP settings wiped by concurrent windows | Open | A5 — root cause is the check-then-create TOCTOU in McpHub.getMcpSettingsFilePath() (L496-517), not initializeMcpServers() |
| #1021 | Race in abandonSubtask for saveClineMessages | Open | A5 — fire-and-forget saveClineMessages() re-attaches abandoned subtasks |
| #920 | Concurrent task history updates cause lost entries | Open | A5 — cross-instance (parallel tabs) regression test on the locked-merge write |
| #1231 | Global _index.json full rewrite unsafe (corruption confirmed) | Closed | Already fixed — per-task history_item.json + merge-under-lock; that is now the reference storage pattern |
| #1221 | Truncated tool-call args silently written to disk | Open | A6 (separate story) — parser/finalization defect, not a filesystem-safety problem |
| #376 | File-level write serialization | Open | Superseded by A3 |
| #370 | Extension-side task-scoping guard | Open | Related; stays open |
Current state (verified in code)
Agent write path
- WriteToFileTool.execute() (src/core/tools/WriteToFileTool.ts:29) →
DiffViewProvider.saveDirectly() (src/integrations/editor/DiffViewProvider.ts:1141) =
raw fs.writeFile(absolutePath, content): no lock, no version check, no atomic
publish, no fsync. This is the main agent write path. - EditFileTool (src/core/tools/EditFileTool.ts:139) validates old_string against a fresh
read at write time — a natural staleness detector for edits, but write_to_file (full
overwrite) has no staleness check at all. - ReadFileTool records no mtime/stat/checksum of the files it hands to the model, so no
compare-and-swap is possible anywhere in the codebase. - Auto-approval (src/core/auto-approval/) can skip user review entirely.
- ExecuteCommandTool runs shell commands in the workspace; shell writes bypass any
filesystem guard (out of scope, below).
Internal state
- safeWriteJson (src/utils/safeWriteJson.ts) is the JSON-state entry point (imported by
TaskHistoryStore, taskMessages, apiMessages, McpHub, webviewMessageHandler,
modelCache): proper-lockfile advisory lock, temp + rename, backup/rollback, merge
callback. Missing: fsync, Windows DACL preservation, and it does not cover the
workspace write path. - MCP settings wiped when multiple windows open — race in McpHub.getMcpSettingsFilePath() direct fs.writeFile #1371 root cause: getMcpSettingsFilePath() (McpHub.ts:496-517) does
fileExistsAtPath() then a raw fs.writeFile of an empty stub — two windows both see
"absent" and the second blind write truncates the file to a 122-byte stub (TOCTOU). - fix(task): guard saveClineMessages against abandoned tasks to prevent race in abandonSubtask #1021: fire-and-forget saveClineMessages() calls can resolve after abandonSubtask()
and re-attach a severed subtask.
Concurrency
- Parallel subtasks / new_task children: same process, shared workspacePath
(Task.ts L515-517), no file-level isolation. - Per-task isolation today = checkpoints shadow git at
/tasks//checkpoints (ShadowCheckpointService), triggered on
user message send — not on writes. - Cross-process (VS Code windows / JetBrains) coordination for workspace files: none.
- Worktrees (packages/core/src/worktree/) are user-UI-only: prompts, tools, and
Task.startTask contain no worktree references, and git worktree add is a full checkout
of the base commit. The agent flow does not use it; it cannot serve as the isolation
mechanism for this work.
Solution
A — Safety core: version guard + per-path serialization + atomic publish
- A1 Version token — dev:ino:size:mtimeNs:ctimeNs from one fs.stat (BigInt). Cost of
one stat; the token is a disk fact, so all processes observe the same value. - A2 Observation registry — per-task Map<absolutePath, version>, populated by
ReadFileTool (one extra stat per read). Owner = task, so parent and subtask
observations are independent. - A3 Guarded write and edit — compare-and-swap semantics on the write path:
- unobserved target → createIfAbsent: succeeds for new files; if the file exists the
write fails, forcing a read first — the model re-reads and retries with the observed
version; - observed-absent → createIfAbsent;
- observed-present → replaceIfVersion(version): a mismatch fails the tool call with
"stale version — re-read the file, then retry"; - edit keeps the literal-match check plus the version guard; an unobserved edit
fails with "file not read yet — read the file, then retry". - Failures carry a remediation suffix the model can act on, so the standard loop
self-heals (re-read → retry); the user sees the failure as a step event in chat. - A per-absolute-path tail-promise chain (in-process FIFO) wraps read → guard →
publish, so concurrent subtask mutations to the same file are deterministically
ordered: one wins, the rest fail as stale. - Cross-process stance (explicit): no workspace lockfile. Two processes editing
the same file are detected via the version token; the loser fails as stale and
re-reads. A lockfile would block the user's own editor.
- unobserved target → createIfAbsent: succeeds for new files; if the file exists the
- A4 Atomic publish — replace saveDirectly's raw fs.writeFile with: temp file in a
private per-write staging dir → fsync → rename; on Windows ReplaceFile (with DACL
copy), rename fallback. Generalize safeWriteJson's staging/backup/rollback into a
safeWriteText used by both file writes and JSON state. - A5 Internal-state firebreak
- MCP settings wiped when multiple windows open — race in McpHub.getMcpSettingsFilePath() direct fs.writeFile #1371: replace the raw stub write in getMcpSettingsFilePath() with
safeWriteJson(..., { merge: keep existing if non-empty }) — atomic read-modify-write
under the advisory lock closes the TOCTOU. - fix(task): guard saveClineMessages against abandoned tasks to prevent race in abandonSubtask #1021: guard in-flight saveClineMessages() with this.abandoned (extract from
feature/local-usage-stats, commit 1d1eb91). - [BUG] Concurrent task history updates from parallel tabs may cause lost entries #920: cross-instance (parallel tabs) regression test for updateTaskHistory on the
locked-merge write.
- MCP settings wiped when multiple windows open — race in McpHub.getMcpSettingsFilePath() direct fs.writeFile #1371: replace the raw stub write in getMcpSettingsFilePath() with
- A6 Truncated tool-call args (Truncated tool-call arguments can be silently written to disk (stale partial-parse nativeArgs reused on finalization failure) #1221, separate story) — a NativeToolCallParser
finalization failure must not reuse the streaming-phase nativeArgs (partial-json
output); fail the tool call with an explicit "arguments were truncated" error instead
of writing the truncated content to disk.
Latency — net write-path cost added by A: one stat + one fsync + one rename. On top
of that, remove the artificial delays:
| Current slow point | Location | Action |
|---|---|---|
| 1000 ms delay per write | DEFAULT_WRITE_DELAY_MS (packages/types/src/global-settings.ts:23) | default 0; keep the setting for users who want pacing |
| delay(300) before approval | WriteToFileTool.ts | remove |
| Blocking LSP diagnostics after save | saveDirectly tail | report asynchronously as a follow-up event |
| DiffView open/update/scroll choreography | default approval path | make the chat-diff (PREVENT_FOCUS_DISRUPTION) path the default |
B — Per-step visibility + rollback
- B1 Automatic checkpoint per file write — extend the existing checkpoints shadow
git so every successful write_to_file / edit_file / apply-patch records a checkpoint
(today only user message sends do). Task start becomes a real, O(1) baseline — not a
no-op, and not a worktree. - B2 Per-task change journal — append-only JSONL under the task dir (one entry per
file write: path, operation, checkpoint id, diff stats); torn-tail repair on load. - B3 Per-step change cards + rollback — per-step change cards in the existing chat
flow (reusing the unified diff + computeDiffStats already produced for approval) with
"N files changed this step", and rollback to any checkpoint per file or per step.
Auto-approval paths get the same cards after the fact — the user can always see and
undo what the agent did.
Acceptance criteria
- No raw fs.writeFile / fs.appendFile in the workspace file-write path (all writes
through the A4 atomic-publish helper; JSON state through safeWriteJson). - Stale writes (file changed since this task's last read) fail loudly with the
re-read-then-retry remediation, and the agent recovers automatically in the standard
loop. - A write to an existing file the task never read fails (createIfAbsent conflict) and
the model recovers by reading first. - Every successful file write produces a rollback-able checkpoint and a journal entry;
the user can view each step's diff and roll back per step or per file. - Concurrent subtasks writing the same file: exactly one wins, the others receive stale
errors — deterministic, no corruption, no lost writes. - Two-window regression: concurrent MCP settings initialization never truncates
mcp_settings.json (MCP settings wiped when multiple windows open — race in McpHub.getMcpSettingsFilePath() direct fs.writeFile #1371). - Abandoned-subtask regression: in-flight saves cannot re-attach an abandoned subtask
(fix(task): guard saveClineMessages against abandoned tasks to prevent race in abandonSubtask #1021). - Truncated tool-call regression: a cut-off write_to_file content argument is not
written to disk; the tool call errors instead (Truncated tool-call arguments can be silently written to disk (stale partial-parse nativeArgs reused on finalization failure) #1221). - Added guard latency < 50 ms p95 on a local volume (stat + rename only; no artificial
delays on the default path). - ESLint suppression counts do not increase; narrow Vitest suites for A and B pass.
Related PRs
Open (head: easonLiangWorldedtech's fork, base main):
| PR | Scope |
|---|---|
| #1380 | A5 — #1371: atomic mcp_settings.json stub creation via safeWriteJson + merge under the advisory lock |
| #1381 | Speed: DEFAULT_WRITE_DELAY_MS 1000 → 0, and the delay(300) before scrollToFirstDiff removed |
| #1382 | A5 — #1021: guard saveClineMessages against abandoned tasks (minimal main form of commit 1d1eb91) |
Adjacent PRs to track (open on main):
- feat(task-scheduler): enable concurrent delegated task fan-out #1046 — concurrent delegated task fan-out: increases concurrent writers, so the A3 guards become mandatory once it lands.
- [Fix] Task history disappears when user reopens a task #1319 — task-history safe-write rename-window retry + snapshot merge under the advisory lock: overlaps A5's internal-state scope; do not double-fix.
- feat(dashboard): add local usage statistics dashboard with session detail #1225 — usage stats dashboard: original home of the fix(task): guard saveClineMessages against abandoned tasks to prevent race in abandonSubtask #1021 fix commit (1d1eb91).
- fix(write-to-file): address partial filesystem error review #1066 — write_to_file partial filesystem error handling: the A6/Truncated tool-call arguments can be silently written to disk (stale partial-parse nativeArgs reused on finalization failure) #1221 parser fix should be stacked after it (same author, same code area).
Out of scope
- Shell-tool writes (ExecuteCommandTool).
- Cross-process workspace locking — the version token is the cross-process mechanism.
- Replacing the existing diff-editor approval UX: it is preserved; B adds after-the-fact
visibility. - Truncated tool-call arguments can be silently written to disk (stale partial-parse nativeArgs reused on finalization failure) #1221 is tracked here but implemented as its own story (parser layer).
- Worktrees remain a user-initiated feature; the agent flow uses checkpoints, not
worktrees.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity
fix(file-safety): derive the version token from exact BigInt stats (A1,
test(experiments): cover unset-experiment default routing in all writ…
test(experiments): add explicit enabled-state routing tests for chat-…
feat(task): per-task file observation registry (A2, Zoo-Code-Org#1375)
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
feat(editor): async post-save diagnostics on chat-diff save path (L1, Z…
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
feat(editor): async post-save diagnostics on chat-diff save path (L1, Z…
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
feat(editor): async post-save diagnostics on chat-diff save path (L1, Z…
feat(checkpoints): per-write checkpoints, task-start baseline, and pe…
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
feat(editor): async post-save diagnostics on chat-diff save path (L1, Z…
feat(file-safety): atomic text publish primitive + safeWriteJson refa…
95 remaining items
easonLiangWorldedtech commentedon Oct 5, 2026
Correction — units replayed on the current main tip
The first pass ported each unit's files as the content at
6768ccfaf. For five files that is not the same as main + the unit's delta: main had already changed them (src/core/task/Task.ts,src/eslint-suppressions.json,src/utils/safeWriteJson.ts,src/core/task-persistence/TaskHistoryStore.tsand its delete spec), so those units silently reverted main's newer work and GitHub marked themdirty. Two of the changes I had attributed to units (thereadFileTool.spec.ts98→96 andsafeWriteJson.ts4→3 suppression reductions) are already on main, so those units no longer carry them.Every unit has been replayed as current main tip
9af61f87e+ its own delta and force-pushed.git merge-treeagainst main is now clean for all nine, and each unit's own suite is green on its own base: U1 42, U2 27+1 skip, U3 11, U4 94 and 48, U5 47, U6 20, U7 108+5 skip, U8 130, U9 14.Own deltas after the replay: U1 1342, U2 538, U3 167, U4 932, U5 1277, U6 691, U7 539, U8 2542, U9 308. The three above the 1000 a+d hard cap (U1, U5, U8) carry the deviation in their own bodies; every unit is inside the 500 changed-executable-line cap that
mutation-diffmeasures.New heads: U1
d5f8a79c7, U2a37272266, U33ea43c371, U460b9221c7, U539203eeb2, U6be039ebda, U7ba6783934, U8a7df0c29e, U97d54871f6.easonLiangWorldedtech commentedon Oct 5, 2026
Mutation CI: how the split resolves it
scripts/stryker-diff.mjscaps changed executable lines per package at 500 (MAX_CHANGED_LINES = 500, line 11) and excludes__tests__,fixtures,test-utils,*.test|spec|visual.*and.d.tsfrom the count (packageForPath, line 181). So the number that matters is executable lines in non-test files, summed over the whole PR diff against main.Measured with the gate's own functions, per unit and cumulatively in merge order:
Each unit is inside the cap at the moment it is next in merge order: after U1–U5 have merged, U6's diff against main is its own 52 executable lines, U7's is 33, U8's is 215, U9's is 12. The red
mutation-diffon U6–U9 right now is the stacked view — the gate is measuring the unmerged base, not the unit. It clears unit by unit as the chain merges in the order U1 → U2 → U3 → U4 → U5 → U6 → U7 → U8 → U9.Current CI at the heads: U1 green, U2 green, U3–U5 running, U6–U9 red on
mutation-difffor the reason above. No unit needs a Stryker directive or an exclusion; the split is the fix.The a+d reviewability cap is a separate matter and is documented per unit (U1 1342, U5 1277, U8 2542 — new files whose specs test those files as a unit).
easonLiangWorldedtech commentedon Oct 5, 2026
Correction to the table in my previous comment: the cap is checked against each unit's own delta, not a running cumulative total, because
selectFromGitdiffsmerge-base(base, head)...headand once a unit has merged into main its content is no longer part of the next unit's diff.Every unit is inside the cap on its own delta.
mutation-diffis red on U6–U9 right now only because their bases have not merged yet, so the gate is measuring the whole unmerged chain instead of the unit. It goes green unit by unit in merge order U1 → U2 → U3 → U4 → U5 → U6 → U7 (#1918) → U8 (#1916) → U9 (#1917).Local preflight with the gate's own code (
node scripts/stryker-diff.mjs ci --base <unit base> --head <unit head>) is running per unit; U1's manifest check passes at 203 lines. The size cap is not what needs fixing on any unit — what still has to be verified per unit is the mutant side (valid mutants, survivors, no-coverage), which is the other half of the gate.easonLiangWorldedtech commentedon Oct 5, 2026
Split state — #1833 closed as superseded
#1833 is now closed: all 31 files of its delta are carried by the nine unit PRs, and the two suppression reductions it held (98→96 for
readFileTool.spec.ts, 4→3 forsafeWriteJson.ts) belong to the units that earn them — U4 (#1913) and U2 (#1911). Those reductions were missing from the unit heads, which is whycompile/lint was red there; they are now committed in the owning units.Two defects found from CI at the unit heads and fixed in the owning units:
RollbackFailureErrorneeds astringbackupPath, but the throw moved after cleanup so thestring | nullnarrowing was lost. The failure is now held as{ error, backupPath }. Pushed97b599d2e.task.observationRegistry, but the field was only declared in U7, so the mocked e2e run dereferenced undefined on theread_filesmoke tests. The registry is introduced by U3, so the import and field belong there. Pushed60376ca18.Chain replayed on the fixed heads (each unit carries its predecessors' corrections), verified at the top: tsc clean, 12 spec files / 489 tests pass (1 skipped), ESLint
--max-warnings=0clean:U1 97b599d2e → U2 625a976dc → U3 60376ca18 → U4 d7eab3d99 → U5 ca636d6f9 → U6 d08f69080 → U7 e993bcda5 → U8 5e72ea6fd → U9 9f3a4db7d.Merge order unchanged: U1 → U2 → U3 → U4 → U5 → U6 → U7 (#1918) → U8 (#1916) → U9 (#1917).
mutation-diffstays red on U6–U9 until their bases merge, because the gate measures the unmerged chain rather than the unit.easonLiangWorldedtech commentedon Oct 5, 2026
Corrected merge order — U8 must land before U6
compilewas red on U6 (#1915) for a real dependency reason, not a test problem:U6's
ApplyPatchToolcallstask.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs, "create"), but thewriteKindparameter is introduced by U8. A consumer cannot build before its provider, so the recorded order U1→…→U5→U6→U7→U8→U9 is not executable.U8 owns the
saveChangessignature; U7 does not touchApplyPatchTool.ts, so U7 does not depend on U6. The executable order is:U1 → U2 → U3 → U4 → U5 → U8 (#1916) → U6 (#1915) → U7 (#1918) → U9 (#1917)
Rebuilt and verified in that order (each unit carries its predecessors' corrections):
U1 97b599d2e → U2 625a976dc → U3 60376ca18 → U4 d7eab3d99 → U5 ca636d6f9 → U8 45b791264 → U6 22b6decf1 → U7 ae52376d8 → U9 667d01dec.Verification at the new chain top: tsc clean, 12 spec files / 489 tests pass (1 skipped), ESLint
--max-warnings=0clean. U8 also verified standalone on U5: tsc clean, 130 DiffViewProvider tests pass, ESLint clean.perf(write-path): remove artificial write delays by default (part of Z…
feat(editor): async post-save diagnostics on chat-diff save path (L1, Z…
fix(editor): reconcile the observation when a guarded publish lands w…
fix(utils): decide confinement once and reject a declared-empty root
fix: order cancellation against the guarded publish and session restart
test(diffview): keep the open() serialization test inside the lint su…
fix(editor): reconcile the observation when a guarded publish lands w…
fix(utils): decide confinement once and reject a declared-empty root
fix: order cancellation against the guarded publish and session restart
test(diffview): keep the open() serialization test inside the lint su…