Repository navigation
fix(settings): serialize settings.json writes so a quick-start cannot tear the file - #719
Conversation
… tear the file
Quick-start saved lastUsedCase with an un-awaited, non-atomic read-modify-write, and the settings PUT, model config, case order, case rename, Ralph start and the boot tunnel reset each ran their own. Two overlapping truncate-then-write calls left one write's bytes over the tail of the other (the CI flake in test/quick-start.test.ts), and most writers then read the torn file as {} and wrote back only its own keys, erasing the rest of the user's settings.
All of them now go through one writer that runs one change at a time, replaces the file atomically, and refuses (409 CONFLICT) to overwrite a file that does not parse.
|
Thank you @JDProfresh, this is a careful fix and a very good write-up. The PR puts every read-modify-write of One thing to change before merge:
Smaller things, fine to fold into the same push:
On the App Settings follow-up you mention (Save still shows "Settings saved" on the new Once the rename change is in, this is ready to merge. Since #718 also edits the |
- src/web/settings-writer.ts:110: export assertSettingsWritable(), the writer's own read-for-write rule run on its own - src/web/routes/case-routes.ts:1860: an admin's rename checks settings.json before it ends any session or moves the folder, and answers 409 CONFLICT with the writer's message (placed before the session-starting check, so nothing is awaited between it and the lock) - test/routes/case-rename-routes.test.ts:314: the torn-file pin now expects 409 CONFLICT (no stoppedSessions, like the other pre-checks) - test/routes/case-rename-routes.test.ts:331: new test, a live session with stopSessions and a torn file: 409 CONFLICT, cleanupSession not called - docs/api-reference.md: the Settings paragraph says an admin's rename answers this refusal as 409 CONFLICT before ending sessions - src/hooks-config.ts:1161, src/prompt-cache-keepwarm.ts:341: comments describe the atomic writer, not an in-place rewrite Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s so a quick-start cannot tear the file Conflict in src/web/routes/system-routes.ts (PUT /api/settings) resolved by keeping both sides: #718's line that never stores an unauthorized tunnelEnabled:true runs first, then #719's serialized mutateSettingsFile() write replaces the inline read and plain writeFile. test/routes/system-routes-tunnel-guard.test.ts: #718's new tests asserted plain writeFile calls on settings.json. With #719 every write goes to a temp file renamed over it, so the assertions read writes through a settingsWrites() helper (writeFile contents keyed by their rename), and the concurrent-stop harness applies a write and the stop at the rename. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…header, and an interval for it app-wide and per case Conflict in CLAUDE.md (Adding Features, App setting) resolved by keeping both sides: #714's sentence that a setting the server acts on must be synced, after the `.strict()` clause, and #719's mutateSettingsFile() rule at the end of the bullet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged in 1.46.0 (https://github.com/Ark0N/Codeman/releases/tag/codeman%401.46.0). Thank you for tracing the red quick-start test back to a torn settings.json; the CI log you dug out made the cause unambiguous. Since there was no new push for the case-rename point, I applied it at merge with the pre-check option: |
POST /api/quick-startsaveslastUsedCasewith an un-awaited, non-atomic read-modify-write ofsettings.json, so it can still be writing when the nextPUT /api/settingsruns its own, and two overlapping truncate-then-write calls leave one write's bytes over the tail of the other.This corrects my guess on #712. There I blamed the 2 s
readSettings()cache for the redtest/quick-start.test.tsjob; the cache was not the cause (its timestamp is taken at the read, before the test's 2,100 ms sleep). The CI log of that run shows a torn file instead:Problem
{\n "agentSkillEnabled": true\n}, the PUT's whole write, followed by the tail of quick-start's longer one. Every reader then gets{}(readJsonConfig()falls back on any parse failure), soagentSkillEnabledreads as off and the skill is not injected: theENOENTonSKILL.mdthe test reports.{}: the PUT (and the model-config, case-order and Ralph writers) treat a file that does not parse as{}and write back only their own keys. After one tear, the next settings save erases every other setting in the file.Cause
Seven places did their own read-modify-write of the server's
settings.jsonwith a plainfs.writeFile: quick-start's and Ralph start'slastUsedCase(neither awaited),PUT /api/settings,PUT /api/execution/model-config,PUT /api/cases/order, the case rename'scaseOrder/lastUsedCaseupdate and its rollback, and the boot reset oftunnelEnabled. Nothing serialized them, andwriteFiletruncates before it writes.What it does
mutateSettingsFile()insrc/web/settings-writer.ts, the same shape asmutateRegistryFile()forclis.json: changes run one at a time on a promise chain, each write goes to its own temp file beside the target and is renamed over it, and a refused or failed change rejects only its caller.settings.jsonthat exists but is not a JSON object is refused instead of read as{}: the settings routes answer409 CONFLICTnaming the file and the fix (fix or delete it, or restore it from a config backup) and leave its bytes alone, and the two starts skip theirlastUsedCase. The message does not repeat the parser's error, since that can quote the file and the file can holdvoiceSettings.apiKey.clis.json: it must work on a file that does not parse, which the new writer refuses. CLAUDE.md and the API reference say so.Smaller behaviours that come with the new writer, each on purpose:
0600: it can hold the Deepgram key, and config restore already writes it that way.{}like a missing one: it holds no settings to lose, and a crash during the old truncating write could leave exactly that.settings.jsonis written through to its target: the rename lands on the real file, as the oldwriteFilefollowed the link, rather than replacing the link.settings.jsonis now replaced: renaming over a file is governed by the directory, the same asmutateRegistryFile()does forclis.json.settings.jsonthat does not parse fails (500with the refusal message) and is undone like any other registry failure: it cannot tell whethercaseOrdernames the case, and it used to skip the update silently.Tests
test/settings-json-write-serialization.test.ts(new, live server on port 0, real file): writes tosettings.jsonand its temp files are slowed by 200 ms (a busy CI disk), which turns the race into a deterministic failure on master. It checks that quick-start and Ralph start answer only afterlastUsedCasehas landed; that quick-start, the settings PUT, model config and case order fired at once leave all four keys; that a settings PUT racing a case rename keeps both; that the boot tunnel reset keeps the other keys; that a missing or empty file starts from{}; that a symlinked file is written through to its target; the0600mode; that a torn file (the CI bytes), a JSON array,nulland a string are each refused by all three routes with the exact409envelope and unchanged bytes; that quick-start and Ralph start still succeed against a torn file without touching it; and that a refusal does not wedge the queue once the file is fixed. On master 10 of its 14 tests fail (the lost key, the early answer, the umask-default mode, the200instead of409); with the change all 14 pass.test/routes/case-rename-routes.test.ts: a rename against a torn file fails without moving the folder or touching the file, and the rollback keeps a settings write made in between. The existing "settings.json cannot be written" test used a0444file to make the write fail; a rename over a read-only file succeeds, so the failure is now injected at the rename, and the test also checks no temp file is left behind.system-routes*) gainedrealpath/mkdir/rename/rmstubs in theirnode:fs/promisesmock; their assertions are unchanged.409mapping, the two awaits, the rollback merge) and watched a test go red for each.Verified
fs.writeFilecalls tear a file with the exact CI message. In a standalone loop of 2,000 pairs (a longlastUsedCasewrite and a shortagentSkillEnabledone started together), 470 left a file that failsJSON.parsewithUnexpected non-whitespace character after JSON at position 31 (line 3 column 2).npm test -- test/quick-start.test.tspasses 10 of 10 runs on this branch. Inferred, not reproduced in CI: that this removes the flake, since the tear needs the two writes to overlap, which my slowed-disk tests force for the lost update but cannot force for the torn bytes.npm test) passes except two tests that fail the same way on unmodified master on my machine (test/tui/tui-switch-guard.test.ts, tmux 3.2a, and the zstd frame test intest/deepseek-transcript.test.tson Node 26, which test(deepseek): stop pinning Node's one-shot zstd decoder to the first frame #710 fixes).Known follow-up, not in this PR: App Settings' Save still shows "Settings saved" and closes when the PUT is refused (it handles a
403and nothing else, so it did the same for any other failure before). I will send that as a separate PR once this one is in, so the409message reaches the user.My tunnel PR for #706 (#718) also edits the
PUT /api/settingshandler, so whichever of the two merges second needs a small rebase.