Repository navigation
fix(host-mode): sync the recovery files before and after they replace the record - #467
Merged
nissa-seru merged 11 commits intoOct 18, 2026
Merged
Conversation
… the record writeRecoveryFile, which writes the host-mode record and the deferred-write queue, wrote a temp file and renamed it over the record with nothing synced, so a crash soon after a write could lose the new record or leave it empty, where these files exist to be the record. A write or a rename that failed also left its temp file beside the record. It now syncs the temp file before the rename and the directory after it (not on win32), and when anything before the rename fails it removes the temp file and rethrows. A directory sync that fails after the rename is logged rather than thrown: the new record is already in place, and a throw would leave the callers saying it isn't, with the deferred-write queue not marked persisted and quiesce() rejecting while the record says quiesced. Tests drive the host-mode record through quiesce() with node:fs's calls recorded: the temp file is synced before the rename and the directory after it; a record that can't be replaced leaves no temp file; on win32 the directory isn't opened; and a failed directory sync leaves the new record in place and logs it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a directory the write made A review of the change found: - The fd-based write, fsync and close of the temp file throw errors that name no path, so a failed recovery write said only "EIO: i/o error, fsync". A failure before the rename now throws an error that names the record and says it was left as it was, with the original as its cause and its code kept. - The temp file's removal swallowed every error. One it can't remove, for anything but a missing file, is now logged with its path. - When the write had to make the record's directory, nothing synced the directory's new entry in its parent. The parent is now synced too. - The tests failed only the rename and the directory's sync. They now also fail the temp file's sync, which this change adds, and its removal; drive the deferred-writes record through a context write deferred while quiesced; write to a fresh directory; and check that the rename was tried, with its error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ting, and raise a failed directory sync as an ops alert A review of the round before found: - The parent sync covered only the record directory's own parent, and only when this very write made it. A write that made two levels left the upper one's entry unsynced, and a directory made by a write that then failed was never synced into its parent. mkdirSync answers the first directory it made, in the form it was given (probed under Node 24.22.0 and Bun 1.4.3), so each directory up to that one is now synced into its parent right after it's made, before anything is written. - mkdirSync ran before the try, so a directory that couldn't be made threw unwrapped. It now runs inside it, and its failure names the record like any other before the rename. The temp file is removed only if this write opened it, so the unlink no longer needs to ignore ENOENT. A failed directory sync was only logged. A failing directory sync usually means a failing disk, so it now raises an ops alert naming the record and saying it may not survive a crash. Directory syncs go through one helper, syncRecoveryDirectory, which skips win32. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rather than each directory a write makes A review of the round before found two gaps in syncing the directories a write makes, each before anything was written: - A sync of one that failed raised an ops alert and the write went on, though nothing was yet in place, so the reason for not throwing didn't hold for it. - A directory made by a write that then failed, or made in part by a mkdirSync that then threw, was never synced into its parent, since a later write's mkdirSync makes nothing. Every directory sync now comes after the rename: the record's directory, for the record's entry, and its parent, for the directory's own, on every write. So a directory any earlier write made is synced into its parent by the next write that succeeds, and every alert is raised with the new record in place. Levels above the parent are left to the filesystem. With main merged, quiesce() writes its record before it marks the host quiesced, so a quiesce whose temp file can't be synced rejects with the host still serving. The test of that failure now checks that too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…deferred-writes record's parent A review of the round before found: - Only the record's own directory had its sync failed in a test. The alert test now runs for the directory and for its parent, each expecting the new record in place and one ops alert naming it. - The deferred-writes test, named for syncing that record "the same way", didn't check its parent. It does now. - The writer's comment didn't say how far up it syncs. It now says the directories above the record's directory's parent are left to the filesystem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lose, and say which sync stays only logged A review of the round before found: - No test checked the 0600 mode the record is published with, which this change moved from writeFileSync's options into openSync, or that the write closes each descriptor it opens. The first test now checks both. - The directory-sync helper's comment gave a diagnosis, a failing disk, that nothing here checks. It now says only that a failure is raised as an ops alert naming the record. - The changelog entry for the quiesce-writes-first fix says that with a recovery file "a failed sync is still only logged", meaning the store's sync. Since a recovery file's own syncs now fail the write or raise an alert, it now says "a failed sync of the store". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sync fails The close of the temp file sits in a finally so it runs on every path, but only the successful write checked it. The test of a temp file whose sync fails now checks it too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st's title The test's title said that with a recovery file "a failed sync is only logged", as its changelog entry did. Its body fails only the store's sync, and a recovery file's own syncs now fail the write or raise an alert, so the title now says "a failed sync of the store", as the entry does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… as on win32 fsync answers EINVAL for a file that doesn't support synchronization, and some systems answer EBADF for a file opened read-only, as these directories are. syncRecoveryDirectory raised either as a recovery-sync ops alert, so where a directory can't be synced, every recovery write raised one, for a refusal that comes again on every write. Those two codes are now let go, as win32 is, the way PostgreSQL's fsync_fname_ext lets them go for a directory. Any other failure of a directory's sync, or of opening it, still raises the alert. The doc comments and the changelog entry say so. Tests: both directories' syncs refused with EINVAL, and with EBADF, are let go with no alert, the new record in place and each descriptor closed. The deferred-writes test now defers two context writes and checks that each rewrites the record, synced the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…at a crash leaves The writer's doc comment and the changelog entry said that a crash leaves the record as it was or the new one, whole. No test crashes the host, so the comment now says what the syncs do: the new record's data is asked to reach the disk before the rename publishes it. The changelog entry drops the sentence, since the ones before it say what is synced. The rest of the comment is rewrapped, unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Required label not found on this PR. |
nissa-seru
pushed a commit
to nissa-seru/agent-framework
that referenced
this pull request
Oct 18, 2026
nissa-seru
pushed a commit
to nissa-seru/agent-framework
that referenced
this pull request
Oct 18, 2026
main gained anima-research#457, anima-research#458, anima-research#459, anima-research#454, anima-research#460, anima-research#464, anima-research#412 and anima-research#467 since 86a3683. One block conflicts, in restorePersistedDeferredWrites: anima-research#454 narrowed its try to reading the recovery file, so that a file it can't read or take fails create(), and moved the parsing and numbering after it, while this branch changed that numbering (boot numbers the restored queue 1..N). The resolution takes main's side of the block, then makes this branch's change to main's numbering where it now stands: the queue is ordered by valid seq, then the rest in file order, numbered 1..N after the already-landed entries are left out, and the counter goes on from N; the file is rewritten for an entry without a valid seq, not for the new numbers. main's check for writes that already landed, which now refuses create() when a context can't be read, is kept as main has it, over the ordered entries. Everything else merged without a conflict. The only other change of main's within forty lines of one of this branch's is anima-research#457's to an ephemeral run's idle watchdog, which now rejects a run that stop() has begun to end as stopped; it sits above the run's finally, which this branch changes, and neither depends on the other. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nissa-seru
pushed a commit
to nissa-seru/agent-framework
that referenced
this pull request
Oct 18, 2026
main brings agent-framework anima-research#456 (the workspace file tools' fixes), anima-research#460 (editors' files and nested mounts), anima-research#410, anima-research#455, anima-research#457, anima-research#458, anima-research#459, anima-research#454, anima-research#464, anima-research#412 and anima-research#467. - write, edit and delete: anima-research#456 answers a change stored in the workspace but not on disk with one frame (storedButUnfinished), which leads with "stored, so don't send it again" and gives the push's reason. This branch had reworded those frames to say where force goes. Its frame goes and anima-research#456's stays: every reason a write's push can return names materialize where it offers force, and so do this branch's mode notes. Each success keeps its modeKept note, and commitToolChange keeps anima-research#456's onCommitted and answers { refused, modeKept }. - write: an overwrite keeps the entry's mode inside anima-research#456's guarded commit, as an edit does. - reconcile.ts: an effect that fails untouched names its kind, a chmod step's included, beside anima-research#456's account of what's in the way (inTheWay). - edit: anima-research#456's edit takes a change on disk in first, with a pass over its path. Where disk changed only a file's mode, that pass takes the mode in, as a listing does, and the edit's note read it as disk's copy having changed and the edit applying to that text. A mode taken in alone isn't a change to the text, so the note no longer counts one. The edit then writes at the mode disk has and leaves nothing owed. - anima-research#460: a push skips a path in another mount's directory before it plans, so it plans no chmod there either. - The changelog fragment drops its sentence about the write tools' frame, which anima-research#456's replaces, and no longer names an edit among what says that nothing records which side changed a mode after a refused widening, since an edit's pass takes disk's mode in first. - Tests: the test of a refused narrowing's frame takes anima-research#456's wording, the test of the force sentence goes with that sentence, and two tests pin an edit after a mode disk changed and after a refused widening. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 task done
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
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.
TL;DR: The host-mode record and the deferred-write queue are now synced to disk before they replace the old record, and their directory and its parent after the rename, except on Windows or where the filesystem can't sync a directory. A write that fails before the rename removes its temp file, logging one it can't, and its error names the record. Any other directory sync that fails raises an ops alert. On one Linux host, a recovery write now takes 3.5–4.0 ms at the median, against about 0.2 ms before.
Problem
writeRecoveryFile(src/framework.ts) writes the two recovery files that exist so a quiesce, and the context writes it holds back, survive a crash:host-mode.jsonanddeferred-writes.json. It writes the document to a temp file and renames that over the record, and nothing syncs either one. So when the call returns, nothing has asked for the new record to be on disk.A probe on
main(3af13bf, Node 24.22.0, Linux) recorded thenode:fscallsquiesce()makes on the recovery directory:writeFileSyncofhost-mode.json.<pid>.<uuid>.tmp, thenrenameSyncof it overhost-mode.json, and nofsyncSyncat all.quiesce()rejected withEISDIR, and the recovery directory still heldhost-mode.json.<pid>.<uuid>.tmp.Changes
writeRecoveryFilekeeps its name and signature, and both recovery files go through it, so both change:openSyncat 0600,writeFileSync,fsyncSync,closeSync), then renamed over the record, so the rename can't publish a record whose data was never asked to reach the disk.EINVALorEBADFis let go the same way. PostgreSQL lets the same two codes go when it syncs a directory, which it also opens read-only, since "Some OSes don't allow us to fsync directories at all" (fsync_fname_ext). On Linux,fsyncanswersEINVALfor a file that doesn't support synchronization (fsync(2)). An alert for such a refusal would fire on every write, since the refusal comes again each time.causeand itscodekept. The fd-based write, sync and close errors name no path on their own. The temp file is removed if this write opened it, and one that can't be removed is logged.persistDeferredWritesmarks the queue persisted only after the write returns (framework.ts:5486at this head), andquiesce()would reject while the record says quiesced.The comment that called this writer "the same idiom as the Discord awareness outbox" is gone, since the two no longer write alike.
On
main, #453's changelog entry and the title of its test intest/host-quiesce.test.tssay that with a recovery file a failed sync is only logged, meaning the store's sync. Both now say "a failed sync of the store", since a recovery file's own syncs now fail the write or raise an alert.The price. Each recovery write now waits for three syncs, the file's and two directories', all synchronous on the event loop. A benchmark wrote a 1.7 KB queue 100 times with
main's calls (writeFileSync, thenrenameSync) and 100 times with this change's, leaving out themkdirSyncboth make first, in two runs on one Linux host (ext4 on a virtual disk, Node 24.22.0, load average about 11). At the median, this change's writes took 3.5–4.0 ms andmain's took 0.18–0.19 ms. This change's writes took 10–20 ms at the 90th percentile and 61–67 ms at most. We haven't timed it on other disks or on an idle host.The host-mode record is written when
quiesce()orresume()changes it.persistDeferredWritesrewrites the deferred-writes record each time its queue changes, while the host is quiesced or a queue written then is still on disk (its guard,quiesced || deferredWritesPersisted). Each context write deferred during a quiesce changes the queue, so a burst of them costs one recovery write each.What each part is for
mainnothing asks for that order. It costs one sync on every write, one of the three in the price above. Nothing simpler asks for the data first.main, a write that fails after making its temp file leaves it in the recovery directory, as the probe in Problem showed for a failed rename, so the directory gains one file per such failure while a fault lasts. Removing it costs an unlink on the failure path, and a log line when the unlink fails too.EINVAL/EBADFskip. Where a filesystem can't sync a directory, the refusal comes again on every write, so an alert for it would fire on every write, and no operator could clear it. Letting the two codes go, as PostgreSQL does, costs one check in the catch. The alternatives are worse: alerting on every failure buries a real I/O failure among alerts nobody can act on, and alerting on none hides it.persistDeferredWriteswouldn't mark the queue persisted, andquiesce()would reject while the record says quiesced. The alert tells an operator that the record may not survive a crash, and reaches them where a log line alone wouldn't. It costs one alert path.tool-presentation.ts's has its own lock and checks, so its durability is a question of its own.Tests
test/recovery-file-durability.test.tsdrives both records through the framework withnode:fs'sopenSync,fsyncSync,closeSync,renameSyncandunlinkSyncrecorded, in twelve tests:quiesce()syncs the host-mode record's temp file before renaming it over the record, and the directory and its parent after, closing each descriptor it opened, and the record is published at 0600;mkdirSync's code, and opens and removes no temp file;quiesce()fails with the rename'sEISDIR, naming the record;process.platformset towin32, the temp file is still synced and no directory is opened;EINVAL, or withEBADF, are let go with no ops alert, each descriptor closed and the new record in place.Run on Linux (x86_64) with Node 24.22.0 and npm 11.19.1, at
3f8f1e9, this branch merged withmain(86a3683):host-quiesce.test.ts, 56 of 56.main'ssrc/framework.ts, all twelve new tests fail.8b8e96a: letting a directory sync'sEINVALandEBADFgo, lettingEBADFgo, and the alert for any other code. At4e7b984: the temp file's close moved out of itsfinally. At89ecd77: the temp file's 0600 mode, its close, and a directory's close. At8060125: the parent synced after the rename, and the ops alert for a failed directory sync. Atf404ed9: making the directory inside thetry, and the win32 check. Atbd592ae: removing the temp file only if this write opened it, the log of one that can't be removed, and the directory synced after the rename. At7633bd2: the temp file's sync and the error naming the record.The full suite is left to CI.
Not verified
process.platform, not on Windows.EINVALandEBADFinto the directories' syncs.opsAlertand reading its call, not by what it sends.Out of scope
src/recovery/discord-awareness-outbox.ts) writes its file with the same temp-and-rename and no sync. feat(awareness): make Discord awareness marks an explicit, one-shot choice that never gates MCPL #250, open, replaces that file's writer with a journal in the store.tool-presentation.ts's writer, for an operator's presentation file, doesn't sync either. It writes under its own lock with its own checks, so its durability is a question of its own.Overlaps
#453, now on
mainand merged here, hasrunQuiescewrite the record before it marks the host quiesced. So a quiesce whose write fails here rejects with the host still serving and the record as it was, which a test pins. #453's own test wrapswriteRecoveryFileby name, which this keeps.Written by Basil-1007 (Claude Opus 5.5). Hilda-2681 (Claude Opus 5.5) reviewed its code at
3f8f1e9.