Skip to content

fix(settings): serialize settings.json writes so a quick-start cannot tear the file - #719

Merged
Ark0N merged 1 commit into
Ark0N:masterfrom
JDProfresh:fix/settings-json-write-race
Oct 27, 2026
Merged

Ark0N merged 1 commit into
Ark0N:masterfrom
JDProfresh:fix/settings-json-write-race

Conversation

@JDProfresh

Copy link
Copy Markdown
Contributor

POST /api/quick-start saves lastUsedCase with an un-awaited, non-atomic read-modify-write of settings.json, so it can still be writing when the next PUT /api/settings runs 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 red test/quick-start.test.ts job; 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:

Failed to read settings: SyntaxError: Unexpected non-whitespace character after JSON at position 31 (line 3 column 2)
    at WebServer.getDefaultClaudeMdPath (src/web/server.ts:2152:29)

Problem

  • The file is torn: the first 31 bytes are exactly {\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), so agentSkillEnabled reads as off and the skill is not injected: the ENOENT on SKILL.md the test reports.
  • It also loses data. Those 31 bytes are the PUT's merged result, so the PUT had already read the file as {}: 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.json with a plain fs.writeFile: quick-start's and Ralph start's lastUsedCase (neither awaited), PUT /api/settings, PUT /api/execution/model-config, PUT /api/cases/order, the case rename's caseOrder/lastUsedCase update and its rollback, and the boot reset of tunnelEnabled. Nothing serialized them, and writeFile truncates before it writes.

What it does

  • One writer, mutateSettingsFile() in src/web/settings-writer.ts, the same shape as mutateRegistryFile() for clis.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.
  • Every writer above goes through it, and quick-start and Ralph start await it before they answer (still non-fatal: a failure is logged, the session runs).
  • A settings.json that exists but is not a JSON object is refused instead of read as {}: the settings routes answer 409 CONFLICT naming 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 their lastUsedCase. The message does not repeat the parser's error, since that can quote the file and the file can hold voiceSettings.apiKey.
  • The case rename's rollback now puts back only the keys it wrote, so a settings save that lands between its write and a failed registry write is kept (it used to rewrite the whole file as it was).
  • Config-backup restore stays the one direct writer of the file, as it already is for 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.
  • The quick-start test's 2,100 ms sleep stays: it is still what outwaits the read cache for the gate.

Smaller behaviours that come with the new writer, each on purpose:

  • The file is written 0600: it can hold the Deepgram key, and config restore already writes it that way.
  • An empty (or whitespace-only) file starts from {} like a missing one: it holds no settings to lose, and a crash during the old truncating write could leave exactly that.
  • A symlinked settings.json is written through to its target: the rename lands on the real file, as the old writeFile followed the link, rather than replacing the link.
  • A read-only settings.json is now replaced: renaming over a file is governed by the directory, the same as mutateRegistryFile() does for clis.json.
  • A case rename against a settings.json that does not parse fails (500 with the refusal message) and is undone like any other registry failure: it cannot tell whether caseOrder names 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 to settings.json and 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 after lastUsedCase has 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; the 0600 mode; that a torn file (the CI bytes), a JSON array, null and a string are each refused by all three routes with the exact 409 envelope 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, the 200 instead of 409); 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 a 0444 file 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.
  • The mocked route tests (system-routes*) gained realpath/mkdir/rename/rm stubs in their node:fs/promises mock; their assertions are unchanged.
  • I removed each guard in turn (the queue, the chain's catch, the parse refusal, each non-object branch, the empty-file branch, the symlink follow, the mode, the temp cleanup, the 409 mapping, the two awaits, the rollback merge) and watched a test go red for each.

Verified

  • Observed: overlapping fs.writeFile calls tear a file with the exact CI message. In a standalone loop of 2,000 pairs (a long lastUsedCase write and a short agentSkillEnabled one started together), 470 left a file that fails JSON.parse with Unexpected non-whitespace character after JSON at position 31 (line 3 column 2).
  • Observed: npm test -- test/quick-start.test.ts passes 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.
  • Observed: the full gate (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 in test/deepseek-transcript.test.ts on 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 403 and 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 the 409 message reaches the user.

My tunnel PR for #706 (#718) also edits the PUT /api/settings handler, so whichever of the two merges second needs a small rebase.

… 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.
@Ark0N

Ark0N commented Oct 26, 2026

Copy link
Copy Markdown
Owner

Thank you @JDProfresh, this is a careful fix and a very good write-up. The PR puts every read-modify-write of settings.json behind one serialized, atomic writer (mutateSettingsFile()), makes quick-start and Ralph start wait for their lastUsedCase write, and refuses to overwrite a file that does not parse instead of reading it as {}. I ran your new test against master's route files and got the same 10 of 14 failures you describe; with the change all 14 pass, as do the touched route tests, typecheck, lint and format.

One thing to change before merge:

  1. Case rename ends sessions for a rename that then fails (src/web/routes/case-routes.ts:1875 and :1888). Inside withCaseLock the route ends every session that has the folder open, moves the folder, and only then calls mutateSettingsFile(). With a settings.json that is not a JSON object, a rename sent with stopSessions: true now ends the sessions, moves the folder back and answers 500 with stoppedSessions: 1. I reproduced it with a small app.inject test (one live session, the torn bytes from your test, { newName: 'beta', stopSessions: true }): cleanupSession was called and the rename failed. On master the same request renames the case, because the broken file read as {} and the caseOrder update was skipped. CLAUDE.md's Case rename paragraph holds the rule this breaks: the checks run before sessions are stopped, "so no session is ended for a move that cannot happen". Please check the file with the writer's own rule before the cleanupSession loop (a small exported probe in settings-writer.ts that runs readSettingsForWrite(), on the isAdmin path) and answer 409 CONFLICT with the writer's message there, or treat SettingsWriteRefusedError at line 1888 as non-fatal and log it the way quick-start and Ralph start do. Either is fine; please add a test with a live session, stopSessions: true and a torn file that asserts cleanupSession was not called.

Smaller things, fine to fold into the same push:

  • The rename answers this refusal as 500 OPERATION_FAILED while the three settings routes answer 409 CONFLICT (pinned in test/routes/case-rename-routes.test.ts:314). If you take the pre-check route above, answering 409 CONFLICT there makes them match; the Settings paragraph in docs/api-reference.md would then say so.
  • Two comments still describe the old in-place rewrite: src/hooks-config.ts:1161 ("settings.json is rewritten in place, so a read can land mid-write") and src/prompt-cache-keepwarm.ts:341 ("A read that lands mid-write is retried"). You updated the matching ones in system-routes.ts and config-backup-routes.ts; these two only need their reason brought up to date.

On the App Settings follow-up you mention (Save still shows "Settings saved" on the new 409, and the respawnPresets and runMode saves fail silently the same way): thank you for flagging it. Whether it has to ride in this PR or can follow it is still open on my side.

Once the rename change is in, this is ready to merge. Since #718 also edits the PUT /api/settings handler, whichever of the two lands second will need a small rebase.

@Ark0N
Ark0N merged commit 8fa5b34 into Ark0N:master Oct 27, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Oct 27, 2026
- 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>
Ark0N pushed a commit that referenced this pull request Oct 27, 2026
…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>
Ark0N pushed a commit that referenced this pull request Oct 27, 2026
…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>
@Ark0N

Ark0N commented Oct 27, 2026

Copy link
Copy Markdown
Owner

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: assertSettingsWritable() in settings-writer.ts runs the writer's own rule before the rename ends any session, and answers 409 CONFLICT like the three settings routes, with a test that a live session with stopSessions: true and a torn file is not ended. The probe sits before the session-starting check so that check and the case lock still run in the same tick. The two stale comments are updated too. The App Settings follow-up (Save showing "Settings saved" on a 409) is not in this release; it can come as its own PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants