fix: write settings.json and other critical config atomically - #1643
Open
elhoim wants to merge 1 commit into
Open
fix: write settings.json and other critical config atomically#1643elhoim wants to merge 1 commit into
elhoim wants to merge 1 commit into
Conversation
MergeSettings runs at SessionStart as `bun SettingsBackport.ts; bun MergeSettings.ts --output <settings path>` under a 15s hook timeout. The write was a truncate-then-write, so if the harness killed the process group after the file was truncated but before the ~60KB body flushed — or if the disk was full — settings.json was left as partial JSON. The next launch reports invalid settings and loses every hook, permission rule, env var and statusline setting. Route the settings writers through the existing atomicWriteText primitive (temp file + rename), which the tree already uses in hooks and TOOLS and which settings.system.json already allowlists a rename for. renameSync is atomic within a filesystem, so an interruption or ENOSPC now fails on the temp file and leaves the previous settings.json untouched. Content written is byte-for-byte unchanged. Also harden the primitive itself to remove its temp file when the write or rename throws, so a failed write leaves nothing behind. This benefits the five existing PULSE callers too. Sites converted (all whole-file rewrites of config that breaks the next launch if truncated): LIFEOS/TOOLS/MergeSettings.ts settings.json + merge snapshot LIFEOS/TOOLS/SettingsBackport.ts settings.user.json (a merge source) LIFEOS/TOOLS/SyncIdentityToSettings.ts settings.json skills/LifeOS/Tools/InstallSettings.ts settings.json (create + merge) skills/LifeOS/Tools/InstallHooks.ts settings.json hook registration skills/LifeOS/Tools/DeployComponents.ts settings.json statusline + keys hooks/lib/work-config.ts repo.json privacy attestation Tests assert the atomicity property directly — after a failed write the previous file is byte-identical and still parses — at the primitive and end to end through MergeSettings. Reverting atomic-write.ts to a plain writeFileSync fails exactly those three tests and no others.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
MergeSettings.tsrewritessettings.jsonwith a truncate-then-write. That file is the harness's entire configuration: hooks, permission rules, environment variables, statusline.It runs at SessionStart as
bun SettingsBackport.ts; bun MergeSettings.ts --output <settings>under a 15-second timeout. If the pair overruns — cold bun cache, slow disk, a busy machine — the harness kills the process group. If the kill lands after the file was truncated but before the ~60KB body is flushed, or the disk is full, what remains is partial JSON. The next launch reports invalid settings and every hook, permission rule and env var is gone.There is no crash needed for this to bite: the window is opened by an ordinary timeout on an ordinary startup path.
The repo already ships the right primitive at
PULSE/lib/atomic-write.ts(write to a unique temp file, thenrenameSyncinto place), andsettings.jsoneven allowlists a temp-file rename in its own permission rules. It just was not used here.The fix
All critical config writes go through the existing atomic primitive rather than a second implementation. Beyond the two sites originally identified, a search for the same pattern found five more, all now converted:
LIFEOS/TOOLS/MergeSettings.tsLIFEOS/TOOLS/SettingsBackport.tsLIFEOS/TOOLS/SyncIdentityToSettings.tshooks/lib/work-config.tsskills/LifeOS/Tools/DeployComponents.tsskills/LifeOS/Tools/InstallHooks.tsskills/LifeOS/Tools/InstallSettings.tsatomic-write.tsitself gained failure-path cleanup so a failed write cannot strand a temp file that a later run might mistake for real config.The content written is unchanged. This is purely about how it lands on disk.
Tests
test/pulse/lib/atomic-write.test.tsandtest/tools/MergeSettings.test.ts, 11 cases:Separately verified with an independent probe that induces a real failure — a read-only target directory, so the rename genuinely cannot complete — rather than mocking it:
The last two cases demonstrate the old behaviour for contrast:
open(path, "w")empties the file the instant it is called, before a single byte of new content exists. That is the window this PR closes.