feat: crash-safe atomic+fsync writes for all persistent state - #99
Conversation
Atomic tmp+rename without fsync can still lose the write (or the rename)
on power loss, because the kernel may not have flushed either to disk.
Add a single durable write path used by all persistent state:
- atomicWrite(path, string|Buffer, {mode?}): mkdir, write temp, fsync the
temp file, rename into place, then fsync the directory so the rename
survives a crash. Temp file is cleaned up on failure.
- writeJsonAtomic delegates to it; add writeTextAtomic for text/yaml.
- directory fsync is best-effort (some filesystems reject it); file fsync
errors propagate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace bare writeFileSync / hand-rolled tmp+rename with the crash-safe atomicWrite primitive across persistent state: - authority stores: runtime-topology.yaml and runtime-exchange.yaml now fsync (file + dir) instead of tmp+rename without flush - state.json (lifecycle + persistence), project + global config, team config, projects registry, last-used, service-state snapshots - credentials (auth.json) keeps mode 0o600 with no world-readable window (the temp file is created 0o600; redundant chmod removed) - attachments (binary content + json metadata), dashboard ui-state, metadata endpoint text, dashboard client prefs, project-takeover state Regenerable high-frequency projections (statusline, live.md, status text) are intentionally left as-is; fsync there is cost without durability gain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A torn or unparseable authority file previously reset to empty on read, and the next write overwrote the evidence — silent data loss. Add quarantineCorruptFile(path): move the bad file to <path>.corrupt-<ts>, log it, then return the empty fallback. Wired into the authority readers: state.json, project + global config, projects registry, and metadata. Once quarantined the file no longer exists, so reads fall through to the fallback without re-triggering (no quarantine storm on hot readers). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a durable, crash-safe atomic write utility (atomicWrite, writeJsonAtomic, writeTextAtomic) and quarantine helper (quarantineCorruptFile), then replaces many direct synchronous file writes across config, state, metadata, attachments, and runtime stores to use those helpers. ChangesAtomic File Write Refactoring
Sequence Diagram(s)sequenceDiagram
participant Caller
participant atomicWrite
participant TempFile
participant Filesystem
participant ParentDir
Caller->>atomicWrite: write(path, data, {mode?})
atomicWrite->>TempFile: create unique .tmp and write bytes
TempFile->>Filesystem: fsync(temp fd)
atomicWrite->>Filesystem: close(temp fd)
atomicWrite->>Filesystem: rename .tmp -> final path
Filesystem->>ParentDir: fsync(parent directory)
ParentDir-->>Caller: return / throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/multiplexer/service-state-snapshot.ts (1)
77-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuarantine corrupt state file before treating as null.
When
JSON.parsefails here, the corrupt state file is silently discarded and overwritten with fresh snapshot data (line 86). This loses potentially valuable debugging information. Per the PR objectives, quarantine should be wired into readers for state.json.Add
quarantineCorruptFile(statePath)in the catch block to preserve the corrupt file before falling back tonull.🛡️ Suggested fix
try { existing = JSON.parse(readFileSync(statePath, "utf-8")) as SavedState; } catch { + quarantineCorruptFile(statePath); existing = null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multiplexer/service-state-snapshot.ts` around lines 77 - 83, When JSON.parse of the saved state at statePath fails in the try/catch that assigns to existing (SavedState), call quarantineCorruptFile(statePath) inside the catch before setting existing = null so the corrupt state.json is preserved for debugging; update the catch block in the read/parse logic that uses existsSync(statePath) / readFileSync to invoke quarantineCorruptFile(statePath) and then fall back to existing = null.
🧹 Nitpick comments (7)
src/paths.ts (1)
413-417: 💤 Low valueOptional: Remove redundant directory check.
The directory creation at lines 414–415 is redundant because
writeJsonAtomicensures the parent directory exists via its internalmkdirSync(dirname(path), { recursive: true })call.♻️ Proposed simplification
function saveRegistry(registry: ProjectsRegistry): void { - const dir = getGlobalAimuxDir(); - if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); writeJsonAtomic(getProjectsRegistryPath(), registry); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/paths.ts` around lines 413 - 417, The saveRegistry function currently does an explicit existsSync/mkdirSync on getGlobalAimuxDir() before calling writeJsonAtomic; remove the redundant directory check and creation and rely on writeJsonAtomic (which already calls mkdirSync(dirname(path), { recursive: true }) internally). Update saveRegistry to simply call writeJsonAtomic(getProjectsRegistryPath(), registry) and remove the getGlobalAimuxDir/existsSync/mkdirSync code paths to keep the function concise.src/config.ts (2)
243-249: 💤 Low valueOptional: Remove redundant directory check.
Same as
saveConfigabove—the directory creation at lines 244–246 is redundant becausewriteJsonAtomicensures the parent directory exists.♻️ Proposed simplification
export function saveGlobalConfig(config: Partial<AimuxConfig>): void { - const dir = getGlobalAimuxDir(); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } writeJsonAtomic(getGlobalConfigPath(), config); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 243 - 249, In saveGlobalConfig, remove the redundant directory existence check and mkdirSync (the block using getGlobalAimuxDir and existsSync/mkdirSync) and simply call writeJsonAtomic(getGlobalConfigPath(), config); rely on writeJsonAtomic to ensure the parent directory exists; update saveGlobalConfig to only compute getGlobalConfigPath() and call writeJsonAtomic to simplify the function (references: saveGlobalConfig, getGlobalAimuxDir, getGlobalConfigPath, writeJsonAtomic).
234-240: 💤 Low valueOptional: Remove redundant directory check.
The
existsSyncandmkdirSynccalls at lines 236–238 are redundant becausewriteJsonAtomicdelegates toatomicWrite, which already creates the parent directory viamkdirSync(dirname(path), { recursive: true }). Removing these lines would simplify the code without changing behavior.♻️ Proposed simplification
export function saveConfig(config: AimuxConfig): void { - const dir = getLocalAimuxDir(); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } writeJsonAtomic(getConfigPath(), config); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 234 - 240, The saveConfig function currently creates the local dir before calling writeJsonAtomic, which is redundant because writeJsonAtomic/atomicWrite already ensures the parent directory exists; remove the unused const dir = getLocalAimuxDir() and the if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }); } block and leave only writeJsonAtomic(getConfigPath(), config) inside saveConfig (refer to saveConfig, getLocalAimuxDir, writeJsonAtomic, getConfigPath).src/team.ts (2)
127-133: 💤 Low valueOptional: Remove redundant directory check.
The directory creation at lines 128–131 is redundant because
writeJsonAtomicensures the parent directory exists.♻️ Proposed simplification
export function saveTeamConfig(config: TeamConfig): void { - const dir = getLocalAimuxDir(); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } writeJsonAtomic(getProjectTeamPath(), config); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/team.ts` around lines 127 - 133, The saveTeamConfig function currently computes dir via getLocalAimuxDir() and performs an existsSync/mkdirSync check before calling writeJsonAtomic; remove the redundant directory check and the unused dir variable and simplify saveTeamConfig to just call writeJsonAtomic(getProjectTeamPath(), config) because writeJsonAtomic already ensures the parent directory exists; keep the function name saveTeamConfig and calls to getProjectTeamPath and writeJsonAtomic unchanged.
138-144: 💤 Low valueOptional: Remove redundant directory check.
Same as
saveTeamConfigabove—the directory creation at lines 139–142 is redundant becausewriteJsonAtomicensures the parent directory exists.♻️ Proposed simplification
export function saveGlobalTeamConfig(config: TeamConfig): void { - const dir = getGlobalAimuxDir(); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } writeJsonAtomic(getGlobalTeamPath(), config); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/team.ts` around lines 138 - 144, The directory existence check and mkdirSync calls inside saveGlobalTeamConfig are redundant because writeJsonAtomic already ensures the parent directory exists; remove the block that calls existsSync(getGlobalAimuxDir()) and mkdirSync(...), leaving saveGlobalTeamConfig to simply call writeJsonAtomic(getGlobalTeamPath(), config) (matching the behavior of saveTeamConfig).src/credentials.ts (2)
102-105: 💤 Low valueConsider removing the redundant
mkdirSynccall.
writeJsonAtomic→atomicWritealready callsmkdirSync(dirname(path), { recursive: true })internally, so the explicitmkdirSync(dir, { recursive: true })on line 104 is now redundant.♻️ Proposed simplification
function persistLastUsedState(projectRoot: string, state: LastUsedState): void { - const dir = getProjectStateDirFor(projectRoot); - mkdirSync(dir, { recursive: true }); writeJsonAtomic(getLastUsedPath(projectRoot), state); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/credentials.ts` around lines 102 - 105, Remove the redundant explicit directory creation: delete the mkdirSync(dir, { recursive: true }) call in src/credentials.ts (the one around line 104) because writeJsonAtomic/atomicWrite already invokes mkdirSync(dirname(path), { recursive: true }) internally; ensure you keep the writeJsonAtomic/atomicWrite invocation and do not add any other directory-creation logic.
211-212: 💤 Low valueConsider removing the redundant
ensureParentcall.
writeTextAtomic→atomicWritealready callsmkdirSync(dirname(path), { recursive: true })internally, so the explicitensureParent(textPath)on line 211 is now redundant.♻️ Proposed simplification
- const textPath = endpointTextPathFor(projectRoot); - ensureParent(textPath); - writeTextAtomic(textPath, `http://${endpoint.host}:${endpoint.port}\n`); + writeTextAtomic(endpointTextPathFor(projectRoot), `http://${endpoint.host}:${endpoint.port}\n`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/credentials.ts` around lines 211 - 212, Remove the redundant ensureParent(textPath) call before performing the atomic write: writeTextAtomic (which delegates to atomicWrite) already creates parent directories via mkdirSync(dirname(path), { recursive: true }), so delete the ensureParent invocation and leave the atomic write call (writeTextAtomic/atomicWrite) as the sole directory-creating mechanism to avoid duplicate work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/multiplexer/service-state-snapshot.ts`:
- Around line 77-83: When JSON.parse of the saved state at statePath fails in
the try/catch that assigns to existing (SavedState), call
quarantineCorruptFile(statePath) inside the catch before setting existing = null
so the corrupt state.json is preserved for debugging; update the catch block in
the read/parse logic that uses existsSync(statePath) / readFileSync to invoke
quarantineCorruptFile(statePath) and then fall back to existing = null.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 243-249: In saveGlobalConfig, remove the redundant directory
existence check and mkdirSync (the block using getGlobalAimuxDir and
existsSync/mkdirSync) and simply call writeJsonAtomic(getGlobalConfigPath(),
config); rely on writeJsonAtomic to ensure the parent directory exists; update
saveGlobalConfig to only compute getGlobalConfigPath() and call writeJsonAtomic
to simplify the function (references: saveGlobalConfig, getGlobalAimuxDir,
getGlobalConfigPath, writeJsonAtomic).
- Around line 234-240: The saveConfig function currently creates the local dir
before calling writeJsonAtomic, which is redundant because
writeJsonAtomic/atomicWrite already ensures the parent directory exists; remove
the unused const dir = getLocalAimuxDir() and the if (!existsSync(dir)) {
mkdirSync(dir, { recursive: true }); } block and leave only
writeJsonAtomic(getConfigPath(), config) inside saveConfig (refer to saveConfig,
getLocalAimuxDir, writeJsonAtomic, getConfigPath).
In `@src/credentials.ts`:
- Around line 102-105: Remove the redundant explicit directory creation: delete
the mkdirSync(dir, { recursive: true }) call in src/credentials.ts (the one
around line 104) because writeJsonAtomic/atomicWrite already invokes
mkdirSync(dirname(path), { recursive: true }) internally; ensure you keep the
writeJsonAtomic/atomicWrite invocation and do not add any other
directory-creation logic.
- Around line 211-212: Remove the redundant ensureParent(textPath) call before
performing the atomic write: writeTextAtomic (which delegates to atomicWrite)
already creates parent directories via mkdirSync(dirname(path), { recursive:
true }), so delete the ensureParent invocation and leave the atomic write call
(writeTextAtomic/atomicWrite) as the sole directory-creating mechanism to avoid
duplicate work.
In `@src/paths.ts`:
- Around line 413-417: The saveRegistry function currently does an explicit
existsSync/mkdirSync on getGlobalAimuxDir() before calling writeJsonAtomic;
remove the redundant directory check and creation and rely on writeJsonAtomic
(which already calls mkdirSync(dirname(path), { recursive: true }) internally).
Update saveRegistry to simply call writeJsonAtomic(getProjectsRegistryPath(),
registry) and remove the getGlobalAimuxDir/existsSync/mkdirSync code paths to
keep the function concise.
In `@src/team.ts`:
- Around line 127-133: The saveTeamConfig function currently computes dir via
getLocalAimuxDir() and performs an existsSync/mkdirSync check before calling
writeJsonAtomic; remove the redundant directory check and the unused dir
variable and simplify saveTeamConfig to just call
writeJsonAtomic(getProjectTeamPath(), config) because writeJsonAtomic already
ensures the parent directory exists; keep the function name saveTeamConfig and
calls to getProjectTeamPath and writeJsonAtomic unchanged.
- Around line 138-144: The directory existence check and mkdirSync calls inside
saveGlobalTeamConfig are redundant because writeJsonAtomic already ensures the
parent directory exists; remove the block that calls
existsSync(getGlobalAimuxDir()) and mkdirSync(...), leaving saveGlobalTeamConfig
to simply call writeJsonAtomic(getGlobalTeamPath(), config) (matching the
behavior of saveTeamConfig).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9bd1193-3c2e-4ded-ac1c-18e5c7eb3cdb
📒 Files selected for processing (18)
src/atomic-write.test.tssrc/atomic-write.tssrc/attachment-store.tssrc/config.tssrc/credentials.tssrc/dashboard/ui-state-store.tssrc/last-used.tssrc/metadata-server.tssrc/metadata-store.tssrc/multiplexer/persistence-methods.tssrc/multiplexer/runtime-lifecycle-methods.tssrc/multiplexer/service-state-snapshot.tssrc/multiplexer/services.tssrc/paths.tssrc/project-takeover.tssrc/runtime-core/exchange-store.tssrc/runtime-core/topology-store.tssrc/team.ts
CodeRabbit flagged service-state-snapshot reading state.json, nulling on parse failure, then unconditionally overwriting it — silently destroying the corrupt file. saveState's inline service-merge read had the same overwrite-on-corrupt shape. Both now quarantine before falling back. The other state.json readers (persistence-methods, services) keep their write inside the try, so they bail without overwriting on corruption and need no quarantine here. Addresses CodeRabbit feedback on PR #99. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the outside-diff comment on |
|
The |
Problem (epic W1)
State writes across aimux were not crash-safe:
fsyncanywhere — even the "atomic" tmp+rename writers (topology, exchange, metadata) could lose the rename on power loss because nothing was flushed to disk.writeFileSync(corruptible mid-write) onstate.json, config, team, registry, last-used, credentials, service snapshots, attachments, ui-state, etc.Change
Phase 1 — one durable primitive (
src/atomic-write.ts):atomicWrite(path, string|Buffer, {mode?})= mkdir → write temp → fsync(temp) → rename → fsync(dir), temp cleaned on failure.writeJsonAtomic/writeTextAtomicbuild on it. Directory fsync is best-effort (some filesystems reject it); file-fsync errors propagate.Phase 2 — route durable writers through it: both authority YAML stores (topology, exchange),
state.json, project+global config, team config, projects registry, last-used, service snapshots, credentials (keeps0o600, no world-readable window), attachments (binary + json), ui-state, metadata endpoint text, project-takeover. Regenerable high-frequency projections (statusline, live.md, status text) are intentionally left as-is — fsync there is cost without durability gain.Phase 3 — quarantine-on-corrupt:
quarantineCorruptFile()moves a bad authority file to<path>.corrupt-<ts>and logs it, instead of silently resetting. Wired into the readers for state.json, config, registry, and metadata. Once quarantined the file is gone, so hot readers fall through to the fallback without a quarantine storm.Verification
yarn verifygreen: typecheck + lint clean, 1016/1016 tests (newatomic-write.test.ts: primitive, mode, verbatim text, overwrite, quarantine, missing-file).Scope
Independent of the other epics. Does not change authority/ownership semantics (W2) or the CLI surface (W3).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Updates
Tests