Repository navigation
[EPIC] File Write Safety Prevent Concurrent Write Races Data Corruption #1375
Description
Activity
- added 15 commits that reference this issue
on Aug 27, 2026 87 remaining items
- added a commit that references this issue
on Sep 28, 2026 easonLiangWorldedtech commented
on Sep 29, 2026 ContributorAuthorMore actionsAddendum — mutation-diff and visual readings after the 09-28/29 main-merge refresh (fresh heads: #1404 23e9451, #1406 6654bff, #1410 c232bd6, #1411 fb9aa06, #1412 d45c28d):
-
Chain-topology mutation readings confirmed (each unit's CI diff is measured against current main, which does not yet carry the lower units): feat(checkpoints): per-write checkpoints, task-start baseline, and perWriteCheckpoints setting (B1, #1375) #1404 (B1) = 71 changed executable lines, feat(checkpoints): per-task change journal with torn-tail repair (B2, #1375) #1406 (B1+B2) = 194, feat(checkpoints): per-step change cards and changeCardDetail setting (B3a, #1375) #1411 (B1+B2+B3a) = 393, feat(checkpoints): per-file and per-step rollback service (B3c, #1375) #1410 (B1+B2+B3a+B3c) = 487 — all under the 500 cap (runs green or in flight). feat(webview): change cards UI and rollback buttons (B3b, #1375) #1412 (full B1..B3b stack) reads 607 — this is the cumulative chain content, not a B3b unit overrun: the B3b-only unit delta (change cards UI + rollback buttons + restore-latest + open-in-editor) is ~120 changed executable lines. The 607 reading self-resolves once the lower units land bottom-up (B1 -> B2 -> B3a -> B3c -> B3b) and feat(webview): change cards UI and rollback buttons (B3b, #1375) #1412 is re-measured against a main carrying B1..B3c. No split is required; no unit exceeds the cap on its own delta.
-
Extension-host visual: every fws branch fails against the pre-fws main-era electron-chat-dark-sidebar.png reference (deterministic 432px diff, stable across repeats — the smoke scenario now renders the See New Changes / Restore Changes change-card buttons). feat(checkpoints): per-write checkpoints, task-start baseline, and perWriteCheckpoints setting (B1, #1375) #1404 and feat(checkpoints): per-task change journal with torn-tail repair (B2, #1375) #1406 were rebaselined from their own CI renders (heads 23e9451 / 6654bff); the upper units will carry the same rebaseline once their visual runs complete.
-
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsSplit plan for #1833 — issued before any split PR opens
#1833 breaches the mutation budget on its own delta: 1427 changed executable lines in the extension package (
MAX_CHANGED_LINES = 500), somutation-diffcannot be cleared by adding tests — it is a size/mutation-budget breach, not a failing check. The whole diff is also 8327 a+d (31 files), past the 1000 hard cap. Splitting is the answer here.Content source of record for every unit:
kind: commit, base7c291bb08(merge-base with main) → head6768ccfaf(the current #1833 head). No unit pins a stacked per-PR head. #1833 stays open as the final integration PR (base = main, the sole merge target); as units merge in order its delta shrinks to the last unit's content.Unit constitution — one provider group + one gate scope each. Budgets are measured standalone against each unit's own base. Every unit is under 500 executable changed lines and under 1000 a+d; each must pass a local Stryker preflight on its own delta (
node scripts/stryker-diff.mjs ci --base <unit-base> --head <unit-head>) before its push.# unit gate scope files exec lines a+d U1 fws/u1-atomic-publishatomic commit: rename, fsync, DACL restore, per-write staging dir, document encoding safeWriteText.ts(hunk-split) + its spec (hunk-split)~250 ~800 U2 fws/u2-rollback-fidelitybackup + rollback, and the partial-failure state when rollback also fails safeWriteText.ts(hunk-split) + its spec (hunk-split)~170 ~540 U3 fws/u3-lock-keycanonical advisory lock key + safety-net cleanup utils/safeWriteJson.ts,safeWriteJson.lockKey.spec.ts,safeWriteJson.test.ts167 538 U4 fws/u4-observation-completenessFileObservation.completeon the registrycore/task/observationRegistry.ts+ spec59 167 U5 fws/u5-read-scope-recordingread paths record completeness; truncation and clipping reported together ReadFileTool.ts,indentation-reader.ts, both specs,eslint-suppressions.json105 936 U6 fws/u6-guard-coreCAS core: check + publish under one lock acquisition guardedWrite.ts(hunk-split) + spec (hunk-split)~250 ~750 U7 fws/u7-guard-cancellationcancellation-aware queued guard + model-facing display path guardedWrite.ts(hunk-split) + spec (hunk-split)~168 ~527 U8 fws/u8-apply-patch-wiringapply_patch guarded publish + move completeness ApplyPatchTool.ts+applyPatchTool.execute.spec.ts105 691 U9 fws/u9-other-tool-wiringapply_diff / write_to_file / edit / edit_file / search_replace wiring + Task 6 exec files + 5 specs 54 462 U10 fws/u10-diffview-guarded-savesaveChanges()routes through the guard; kind matching; codecDiffViewProvider.ts(hunk-split) + spec (hunk-split)~180 ~880 U11 fws/u11-diffview-rejected-save-cleanupdiscard-only cleanup, placeholder removal under the shared lock DiffViewProvider.ts(hunk-split) + spec (hunk-split)~170 ~870 U12 fws/u12-diffview-teardown-scopetab identity by URI, reset scope, one teardown owner per provider DiffViewProvider.ts(hunk-split) + spec (hunk-split)~136 ~792 U13 fws/u13-task-history-deletedelete semantics under the canonical key TaskHistoryStore.ts+TaskHistoryStore.deleteSemantics.spec.ts23 308 Merge order: U1 → U2 → U3 → U4 → U5 → U6 → U7 → U8 → U9 → U10 → U11 → U12 → U13 → #1833 (final, base = main). Each unit's base is the previous unit's head; #1833 stays base = main.
Cross-unit shared symbols (owned by the first unit that lands, later units rebase and drop their copy):
resolveLockKey/canonicalKey— owned by U3, consumed by U6, U11, U13.GuardRejectedError+errorCode— owned by U6, consumed by U7, U8, U9, U10.FileObservation.complete— owned by U4, consumed by U5, U6, U8, U10.withFileLock— already on main; no unit redefines it.
Fidelity rule: a unit ports only content from the pinned source. Test content travels with the unit whose behaviour it proves. No cross-unit hunk may change behaviour that another unit's tests assert; hunk-split units are verified by re-running the split file set against the pinned diff.
Nothing in this plan is executed until this comment exists, which it now does. Units open one at a time in the order above.
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsPlan amendment before the first split PR opens
Two things the first pass could not resolve, fixed here so the plan is executable before any unit opens:
1. Topology is independent units, not a chain. Each unit ports a disjoint file set, so each can base directly on main and merge in dependency order: U1 → U2 → U3 → U4 → U5 → U6 → U7 → U8a → U8b → U8c → U9. The dependency order is real (U5 needs U1/U3, U6 and U7 need U5, U8 needs U5, U9 needs U2); the order is not cosmetic.
2. Three units cannot be split below the 1000 a+d hard cap without breaking the fidelity contract, because the file is new or its tests cannot be separated from the behaviour they prove. These are documented deviations, recorded in each PR body:
- U1 —
safeWriteText.tsis a new file (420 executable lines) and its 922-line spec tests that file as a unit. 1342 a+d. Executable lines 420, inside the CI cap. - U5 —
guardedWrite.ts(418 executable) + its 859-line spec. 1277 a+d. Executable lines inside the cap. - U8 —
DiffViewProvider.ts(486 executable) + its 2056-line spec. Split into three units by the behaviour the tests prove (save routing / rejected-save cleanup / teardown scope), which is the only split that keeps each test with the behaviour it covers.
Units U2, U3, U4, U6, U7, U9 stay inside both caps (538 / 167 / 936 / 691 / 462 / 308 a+d).
#1833 stays open as the integration PR: as units land, its delta shrinks. When the last unit merges, #1833 has no content left and is closed with a pointer to the units. Its existing review history is not carried into the units — each unit is reviewed on its own delta.
Content source of record for every unit stays
kind: commit, base7c291bb08→ head6768ccfaf. No unit ports content that is not in that diff.- U1 —
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsCorrection to the amendment: the units are not independent. Each unit's code calls symbols an earlier unit defines (
safeWriteJson→safeWriteText;guardedWrite→safeWriteText+FileObservation.complete; the tool wiring andDiffViewProvider→guardedWrite;TaskHistoryStore→ the canonical lock key), so a unit that bases on main cannot build before its dependency lands.Topology is therefore the stacked chain in dependency order, each unit based on the previous unit's head, with #1833 as the final integration PR (base = main, the sole merge target):
main ─ U1 ─ U2 ─ U3 ─ U4 ─ U5 ─ U6 ─ U7 ─ U8a ─ U8b ─ U8c ─ U9 ─ #1833Merge order is the same, and a unit is only merged after its base has merged. U3 and U4 are the only units that could stand alone; they stay in the chain so the bot sees one clean delta per PR.
- added a commit that references this issue
on Oct 5, 2026 easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsUnits opened (plan executed in order)
unit branch PR own delta U1 fws/u1-atomic-publish#1910 1342 a+d / 420 exec U2 fws/u2-lock-key#1911 541 a+d / 167 exec U3 fws/u3-observation-completeness#1912 167 a+d / 59 exec U4 fws/u4-read-scope-recording#1913 936 a+d / 105 exec U5 fws/u5-guard-core#1914 1277 a+d / 418 exec U6 fws/u6-apply-patch-wiring#1915 691 a+d / 105 exec U7 fws/u7-tool-wiring#1916 462 a+d / 54 exec U8 fws/u8-diffview-guarded-save#1917 2542 a+d / 486 exec U9 fws/u9-task-history-delete#1918 308 a+d / 23 exec Every unit is inside the 500 changed-executable-line cap, which is what
mutation-diffmeasures; the three units above the 1000 a+d hard cap carry the deviation in their own bodies.Each unit was verified standalone on its own base: its own suite green, ESLint
--max-warnings=0 --prune-suppressionsclean, Prettier clean,src/eslint-suppressions.jsonnever increased. Suite counts at the time of porting: U1 42, U2 23+1 skip and 4, U3 11, U4 94 and 48, U5 47, U6 20, U7 108+5 skip, U8 130, U9 14.Merge order is U1 → U2 → U3 → U4 → U5 → U6 → U7 → U8 → U9, then #1833 closes as the integration PR once nothing is left in it.
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsCorrected unit → PR mapping (U7 opened after U8/U9, so the earlier table was off):
unit branch PR own delta U1 fws/u1-atomic-publish#1910 1342 a+d / 420 exec U2 fws/u2-lock-key#1911 541 a+d / 167 exec U3 fws/u3-observation-completeness#1912 167 a+d / 59 exec U4 fws/u4-read-scope-recording#1913 936 a+d / 105 exec U5 fws/u5-guard-core#1914 1277 a+d / 418 exec U6 fws/u6-apply-patch-wiring#1915 691 a+d / 105 exec U7 fws/u7-tool-wiring#1918 462 a+d / 54 exec U8 fws/u8-diffview-guarded-save#1916 2542 a+d / 486 exec U9 fws/u9-task-history-delete#1917 308 a+d / 23 exec Merge order stays U1 → U2 → U3 → U4 → U5 → U6 → U7 (#1918) → U8 (#1916) → U9 (#1917), then #1833 closes as the integration PR once nothing is left in it.
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsCorrection — 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 commented
on Oct 5, 2026 ContributorAuthorMore actionsMutation 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:
unit own executable lines cumulative at its turn over 500? U1 171 171 no U2 27 198 no U3 12 210 no U4 37 247 no U5 225 472 no U6 52 524 yes — red until its base merges U7 33 557 yes — red until its base merges U8 215 772 yes — red until its base merges U9 12 784 yes — red until its base merges 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 commented
on Oct 5, 2026 ContributorAuthorMore actionsCorrection 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.unit PR own changed executable lines over 500? U1 #1910 203 no U2 #1911 27 no U3 #1912 12 no U4 #1913 37 no U5 #1914 225 no U6 #1915 52 no U7 #1918 33 no U8 #1916 215 no U9 #1917 12 no 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 commented
on Oct 5, 2026 ContributorAuthorMore actionsSplit 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:
- U1 (feat(file-safety): atomic text publish primitive (U1, #1375) #1910) —
RollbackFailureErrorneeds astringbackupPath, but the throw moved after cleanup so thestring | nullnarrowing was lost. The failure is now held as{ error, backupPath }. Pushed97b599d2e. - U3 (feat(task): observation registry with read completeness (U3, #1375) #1912) — the read tools call
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.- U1 (feat(file-safety): atomic text publish primitive (U1, #1375) #1910) —
easonLiangWorldedtech commented
on Oct 5, 2026 ContributorAuthorMore actionsCorrected merge order — U8 must land before U6
compilewas red on U6 (#1915) for a real dependency reason, not a test problem:core/tools/ApplyPatchTool.ts(247,5): error TS2554: Expected 2-5 arguments, but got 6. core/tools/ApplyPatchTool.ts(252,78): error TS2554: Expected 0-2 arguments, but got 3.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.
Problem
The agent's file-write path has no version guard, no atomic publish, and no per-step
visibility:
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).
task start creates no baseline at all.
overwritten on the next write.
safety.
Known issues
Current state (verified in code)
Agent write path
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.
read at write time — a natural staleness detector for edits, but write_to_file (full
overwrite) has no staleness check at all.
compare-and-swap is possible anywhere in the codebase.
filesystem guard (out of scope, below).
Internal state
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.
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).
and re-attach a severed subtask.
Concurrency
(Task.ts L515-517), no file-level isolation.
/tasks//checkpoints (ShadowCheckpointService), triggered on
user message send — not on writes.
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
one stat; the token is a disk fact, so all processes observe the same value.
ReadFileTool (one extra stat per read). Owner = task, so parent and subtask
observations are independent.
write fails, forcing a read first — the model re-reads and retries with the observed
version;
"stale version — re-read the file, then retry";
fails with "file not read yet — read the file, then retry".
self-heals (re-read → retry); the user sees the failure as a step event in chat.
publish, so concurrent subtask mutations to the same file are deterministically
ordered: one wins, the rest fail as stale.
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.
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.
safeWriteJson(..., { merge: keep existing if non-empty }) — atomic read-modify-write
under the advisory lock closes the TOCTOU.
feature/local-usage-stats, commit 1d1eb91).
locked-merge write.
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:
B — Per-step visibility + rollback
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.
file write: path, operation, checkpoint id, diff stats); torn-tail repair on load.
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
through the A4 atomic-publish helper; JSON state through safeWriteJson).
re-read-then-retry remediation, and the agent recovers automatically in the standard
loop.
the model recovers by reading first.
the user can view each step's diff and roll back per step or per file.
errors — deterministic, no corruption, no lost writes.
mcp_settings.json (MCP settings wiped when multiple windows open — race in McpHub.getMcpSettingsFilePath() direct fs.writeFile #1371).
(fix(task): guard saveClineMessages against abandoned tasks to prevent race in abandonSubtask #1021).
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).
delays on the default path).
Related PRs
Open (head: easonLiangWorldedtech's fork, base
main):Adjacent PRs to track (open on main):
Out of scope
visibility.
worktrees.