From 5638535e79fa4a5748d980845abd0c044b93b72e Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 17:51:17 -0400 Subject: [PATCH 01/10] fix(host-mode): sync the recovery files before and after they replace 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 --- changelog.d/recovery-file-durability.fixed.md | 1 + src/framework.ts | 46 +++++- test/recovery-file-durability.test.ts | 136 ++++++++++++++++++ 3 files changed, 177 insertions(+), 6 deletions(-) create mode 100644 changelog.d/recovery-file-durability.fixed.md create mode 100644 test/recovery-file-durability.test.ts diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md new file mode 100644 index 000000000..b11bd93bc --- /dev/null +++ b/changelog.d/recovery-file-durability.fixed.md @@ -0,0 +1 @@ +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename (except on Windows), so a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record. A directory sync that fails after the rename is logged, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index 3455acb3b..16a687e93 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2,7 +2,10 @@ import { ToolPresentation, presentationTools, isPresentationTool, renderCatalogu import { dirname, join } from 'node:path'; import { INLINE_WITHHELD_TEXT, REFERENCE_LIMITS, classifyBlock, isInlineContradiction, isReferenceExpired, referenceRegistry, referenceStubOrNull, referenceTypeLabel, sanitizeLabel, unfetchableReason } from './mcpl/references.js'; import { ReferenceFetcher, DEFAULT_FETCH_MAX_BYTES, EAGER_FETCH_TIMEOUT_MS } from './mcpl/reference-fetcher.js'; -import { appendFileSync, existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs'; +import { + appendFileSync, closeSync, existsSync, fsyncSync, mkdirSync, openSync, readFileSync, renameSync, unlinkSync, + writeFileSync, +} from 'node:fs'; import { JsStore } from '@animalabs/chronicle'; import type { Membrane, ContentBlock, NormalizedRequest, YieldingStream, ToolResult as MembraneToolResult, ToolResultContentBlock } from '@animalabs/membrane'; import { MembraneError } from '@animalabs/membrane'; @@ -5297,13 +5300,44 @@ export class AgentFramework { return landed; } - /** Atomic JSON write for the recovery files (tmp + rename, 0600) — the - * same idiom as the Discord awareness outbox. */ + /** Atomic, durable JSON write for the recovery files. The document goes to + * a temp file at 0600, which is synced and renamed over the record, and + * the directory is then synced (not on win32), so a crash leaves the + * record as it was or the new one, whole. A failure before the rename + * removes the temp file, leaves the record as it was and throws. A + * directory sync that fails after the rename is logged, not thrown: the + * new record is in place, and the callers go on to record it as written. */ private writeRecoveryFile(path: string, document: unknown): void { - mkdirSync(dirname(path), { recursive: true }); + const directory = dirname(path); + mkdirSync(directory, { recursive: true }); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; - writeFileSync(temporary, `${JSON.stringify(document, null, 2)}\n`, { mode: 0o600 }); - renameSync(temporary, path); + try { + const fd = openSync(temporary, 'w', 0o600); + try { + writeFileSync(fd, `${JSON.stringify(document, null, 2)}\n`); + fsyncSync(fd); + } finally { + closeSync(fd); + } + renameSync(temporary, path); + } catch (err) { + try { unlinkSync(temporary); } catch { /* nothing to remove if it was never made */ } + throw err; + } + if (process.platform === 'win32') return; + try { + const fd = openSync(directory, 'r'); + try { + fsyncSync(fd); + } finally { + closeSync(fd); + } + } catch (err) { + console.error( + `[host-mode] ${path} is in place, but syncing its directory failed, so a crash could still lose this change:`, + err, + ); + } } private readRecoveryFile(path: string): unknown { diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts new file mode 100644 index 000000000..82ffcbf63 --- /dev/null +++ b/test/recovery-file-durability.test.ts @@ -0,0 +1,136 @@ +/** + * The recovery files (host mode, deferred writes) are written as a temp file + * synced and renamed over the record, with the directory synced after the + * rename. These drive the host-mode record through quiesce(), recording the + * node:fs calls the write makes. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs, { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { AgentFramework } from '../src/index.js'; +import { MockMembrane } from './helpers/mock-membrane.js'; + +async function withFramework(fn: (framework: AgentFramework, recovery: string) => Promise): Promise { + const dir = mkdtempSync(join(tmpdir(), 'recovery-durability-')); + const framework = await AgentFramework.create({ + storePath: join(dir, 'store'), + membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'agent', model: 'test-model', systemPrompt: 'test' }], + modules: [], + }); + try { + framework.start(); + await fn(framework, join(dir, 'store', 'recovery')); + } finally { + await framework.stop(); + rmSync(dir, { recursive: true, force: true }); + } +} + +interface FsCall { call: 'open' | 'fsync' | 'close' | 'rename'; path?: string; to?: string; fd?: number } + +/** Runs `fn` with node:fs's openSync, fsyncSync, closeSync and renameSync recorded in order, each fd named by + * the path it was opened for. `fail` may return an error for an fsync to throw instead of syncing. */ +async function recordingFs( + fn: () => Promise, + fail?: (call: FsCall) => Error | undefined, +): Promise { + const calls: FsCall[] = []; + const opened = new Map(); + const original = { openSync: fs.openSync, fsyncSync: fs.fsyncSync, closeSync: fs.closeSync, renameSync: fs.renameSync }; + fs.openSync = ((path: fs.PathLike, ...rest: unknown[]) => { + const fd = (original.openSync as (...args: unknown[]) => number)(path, ...rest); + opened.set(fd, String(path)); + calls.push({ call: 'open', path: String(path), fd }); + return fd; + }) as typeof fs.openSync; + fs.fsyncSync = ((fd: number) => { + const call: FsCall = { call: 'fsync', path: opened.get(fd), fd }; + calls.push(call); + const error = fail?.(call); + if (error) throw error; + original.fsyncSync(fd); + }) as typeof fs.fsyncSync; + fs.closeSync = ((fd: number) => { + calls.push({ call: 'close', path: opened.get(fd), fd }); + original.closeSync(fd); + }) as typeof fs.closeSync; + fs.renameSync = ((from: fs.PathLike, to: fs.PathLike) => { + calls.push({ call: 'rename', path: String(from), to: String(to) }); + original.renameSync(from, to); + }) as typeof fs.renameSync; + syncBuiltinESMExports(); + try { + await fn(); + } finally { + Object.assign(fs, original); + syncBuiltinESMExports(); + } + return calls; +} + +/** The rename that put the host-mode record in place, and the temp file it renamed. */ +function recordRename(calls: FsCall[], record: string): { at: number; temporary: string } { + const at = calls.findIndex((c) => c.call === 'rename' && c.to === record); + assert.ok(at >= 0, `a rename onto ${record}: ${JSON.stringify(calls)}`); + const temporary = calls[at].path!; + assert.match(temporary, /host-mode\.json\.\d+\.[0-9a-f-]+\.tmp$/); + return { at, temporary }; +} + +test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory after', async () => { + await withFramework(async (framework, recovery) => { + const record = join(recovery, 'host-mode.json'); + const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); + const rename = recordRename(calls, record); + const tempSync = calls.findIndex((c) => c.call === 'fsync' && c.path === rename.temporary); + assert.ok(tempSync >= 0 && tempSync < rename.at, `the temp file synced before the rename: ${JSON.stringify(calls)}`); + const dirSync = calls.findIndex((c, i) => i > rename.at && c.call === 'fsync' && c.path === recovery); + assert.ok(dirSync > rename.at, `the directory synced after the rename: ${JSON.stringify(calls)}`); + assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); + }); +}); + +test('a host-mode record that can\'t be replaced leaves no temp file behind', async () => { + await withFramework(async (framework, recovery) => { + // A directory where the record goes, so the rename fails. + mkdirSync(join(recovery, 'host-mode.json'), { recursive: true }); + await assert.rejects(framework.quiesce({ reason: 'test' })); + assert.deepEqual(readdirSync(recovery).filter((name) => name.endsWith('.tmp')), []); + }); +}); + +test('on win32 the temp file is still synced, and the directory is not opened to sync it', async () => { + await withFramework(async (framework, recovery) => { + const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; + Object.defineProperty(process, 'platform', { ...platform, value: 'win32' }); + let calls: FsCall[]; + try { + calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); + } finally { + Object.defineProperty(process, 'platform', platform); + } + const rename = recordRename(calls, join(recovery, 'host-mode.json')); + assert.ok(calls.some((c, i) => i < rename.at && c.call === 'fsync' && c.path === rename.temporary), JSON.stringify(calls)); + assert.ok(!calls.some((c) => c.call === 'open' && c.path === recovery), `the directory not opened: ${JSON.stringify(calls)}`); + }); +}); + +test('a directory sync that fails after the rename leaves the new record in place and says so', async (t) => { + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + await withFramework(async (framework, recovery) => { + const record = join(recovery, 'host-mode.json'); + const eio = () => Object.assign(new Error('EIO: i/o error, fsync'), { code: 'EIO' }); + await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.path === recovery ? eio() : undefined)); + assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); + assert.ok( + logged.some((line) => line.includes(`${record} is in place, but syncing its directory failed, so a crash could still lose this change`)), + logged.join('\n'), + ); + }); +}); From 7633bd274ac1b5f6c7fafbc79856c573e17166e1 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 18:19:47 -0400 Subject: [PATCH 02/10] fix(host-mode): name the record when a recovery write fails, and sync 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 --- changelog.d/recovery-file-durability.fixed.md | 2 +- src/framework.ts | 49 ++++--- test/recovery-file-durability.test.ts | 133 +++++++++++++++--- 3 files changed, 142 insertions(+), 42 deletions(-) diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md index b11bd93bc..42d1b6b85 100644 --- a/changelog.d/recovery-file-durability.fixed.md +++ b/changelog.d/recovery-file-durability.fixed.md @@ -1 +1 @@ -- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename (except on Windows), so a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record. A directory sync that fails after the rename is logged, since the new record is already in place. +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename (with its parent, when the write had to make the directory), except on Windows, where only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails after the rename is logged, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index 16a687e93..bcd7f4478 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5301,15 +5301,17 @@ export class AgentFramework { } /** Atomic, durable JSON write for the recovery files. The document goes to - * a temp file at 0600, which is synced and renamed over the record, and - * the directory is then synced (not on win32), so a crash leaves the - * record as it was or the new one, whole. A failure before the rename - * removes the temp file, leaves the record as it was and throws. A - * directory sync that fails after the rename is logged, not thrown: the - * new record is in place, and the callers go on to record it as written. */ + * a temp file at 0600, which is synced and renamed over the record. Then + * the directory is synced, and its parent too if this write had to make + * the directory, except on win32. So a crash leaves the record as it was + * or the new one, whole. A failure before the rename removes the temp file + * and throws an error saying the record was left as it was, with the + * original as its cause and its code kept. A directory sync that fails + * after the rename is logged, not thrown: the new record is in place, and + * the callers go on to record it as written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); - mkdirSync(directory, { recursive: true }); + const made = mkdirSync(directory, { recursive: true }); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; try { const fd = openSync(temporary, 'w', 0o600); @@ -5321,22 +5323,31 @@ export class AgentFramework { } renameSync(temporary, path); } catch (err) { - try { unlinkSync(temporary); } catch { /* nothing to remove if it was never made */ } - throw err; + try { + unlinkSync(temporary); + } catch (unlinkErr) { + if ((unlinkErr as NodeJS.ErrnoException).code !== 'ENOENT') { + console.error(`[host-mode] couldn't remove ${temporary} after a failed write:`, unlinkErr); + } + } + const cause = err as NodeJS.ErrnoException; + throw Object.assign(new Error(`${path} was left as it was: ${cause.message}`, { cause }), cause.code ? { code: cause.code } : {}); } if (process.platform === 'win32') return; - try { - const fd = openSync(directory, 'r'); + for (const synced of made === undefined ? [directory] : [directory, dirname(directory)]) { try { - fsyncSync(fd); - } finally { - closeSync(fd); + const fd = openSync(synced, 'r'); + try { + fsyncSync(fd); + } finally { + closeSync(fd); + } + } catch (err) { + console.error( + `[host-mode] ${path} is in place, but syncing ${synced} failed, so a crash could still lose this change:`, + err, + ); } - } catch (err) { - console.error( - `[host-mode] ${path} is in place, but syncing its directory failed, so a crash could still lose this change:`, - err, - ); } } diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index 82ffcbf63..39a40cc56 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -1,12 +1,12 @@ /** * The recovery files (host mode, deferred writes) are written as a temp file * synced and renamed over the record, with the directory synced after the - * rename. These drive the host-mode record through quiesce(), recording the - * node:fs calls the write makes. + * rename. These drive both records through the framework, recording the + * node:fs calls each write makes. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import fs, { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs'; +import fs, { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs'; import { syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -14,34 +14,40 @@ import { join } from 'node:path'; import { AgentFramework } from '../src/index.js'; import { MockMembrane } from './helpers/mock-membrane.js'; -async function withFramework(fn: (framework: AgentFramework, recovery: string) => Promise): Promise { +async function withFramework( + fn: (framework: AgentFramework, recovery: string, dir: string) => Promise, + config: { hostModePath?: (dir: string) => string } = {}, +): Promise { const dir = mkdtempSync(join(tmpdir(), 'recovery-durability-')); const framework = await AgentFramework.create({ storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), agents: [{ name: 'agent', model: 'test-model', systemPrompt: 'test' }], modules: [], + ...(config.hostModePath ? { hostModePath: config.hostModePath(dir) } : {}), }); try { framework.start(); - await fn(framework, join(dir, 'store', 'recovery')); + await fn(framework, join(dir, 'store', 'recovery'), dir); } finally { await framework.stop(); rmSync(dir, { recursive: true, force: true }); } } -interface FsCall { call: 'open' | 'fsync' | 'close' | 'rename'; path?: string; to?: string; fd?: number } +interface FsCall { call: 'open' | 'fsync' | 'close' | 'rename' | 'unlink'; path?: string; to?: string; fd?: number } -/** Runs `fn` with node:fs's openSync, fsyncSync, closeSync and renameSync recorded in order, each fd named by - * the path it was opened for. `fail` may return an error for an fsync to throw instead of syncing. */ +/** Runs `fn` with node:fs's openSync, fsyncSync, closeSync, renameSync and unlinkSync recorded in order, each fd + * named by the path it was opened for. `fail` may return an error for an fsync or an unlink to throw instead. */ async function recordingFs( fn: () => Promise, fail?: (call: FsCall) => Error | undefined, ): Promise { const calls: FsCall[] = []; const opened = new Map(); - const original = { openSync: fs.openSync, fsyncSync: fs.fsyncSync, closeSync: fs.closeSync, renameSync: fs.renameSync }; + const original = { + openSync: fs.openSync, fsyncSync: fs.fsyncSync, closeSync: fs.closeSync, renameSync: fs.renameSync, unlinkSync: fs.unlinkSync, + }; fs.openSync = ((path: fs.PathLike, ...rest: unknown[]) => { const fd = (original.openSync as (...args: unknown[]) => number)(path, ...rest); opened.set(fd, String(path)); @@ -63,6 +69,13 @@ async function recordingFs( calls.push({ call: 'rename', path: String(from), to: String(to) }); original.renameSync(from, to); }) as typeof fs.renameSync; + fs.unlinkSync = ((path: fs.PathLike) => { + const call: FsCall = { call: 'unlink', path: String(path) }; + calls.push(call); + const error = fail?.(call); + if (error) throw error; + original.unlinkSync(path); + }) as typeof fs.unlinkSync; syncBuiltinESMExports(); try { await fn(); @@ -73,37 +86,114 @@ async function recordingFs( return calls; } -/** The rename that put the host-mode record in place, and the temp file it renamed. */ +const fsError = (code: string, syscall: string) => Object.assign(new Error(`${code}: i/o error, ${syscall}`), { code, syscall }); +const isTemporaryOf = (record: string, path: string | undefined) => + path !== undefined && path.startsWith(`${record}.`) && path.endsWith('.tmp'); + +/** The rename that put a record in place, and the temp file it renamed. */ function recordRename(calls: FsCall[], record: string): { at: number; temporary: string } { const at = calls.findIndex((c) => c.call === 'rename' && c.to === record); assert.ok(at >= 0, `a rename onto ${record}: ${JSON.stringify(calls)}`); const temporary = calls[at].path!; - assert.match(temporary, /host-mode\.json\.\d+\.[0-9a-f-]+\.tmp$/); + assert.ok(isTemporaryOf(record, temporary), temporary); return { at, temporary }; } +/** The temp file synced before its rename onto `record`, and `directory` synced after it. */ +function assertSyncedAround(calls: FsCall[], record: string, directory: string): void { + const rename = recordRename(calls, record); + const tempSync = calls.findIndex((c) => c.call === 'fsync' && c.path === rename.temporary); + assert.ok(tempSync >= 0 && tempSync < rename.at, `the temp file synced before the rename: ${JSON.stringify(calls)}`); + assert.ok( + calls.some((c, i) => i > rename.at && c.call === 'fsync' && c.path === directory), + `${directory} synced after the rename: ${JSON.stringify(calls)}`, + ); +} + test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory after', async () => { await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); - const rename = recordRename(calls, record); - const tempSync = calls.findIndex((c) => c.call === 'fsync' && c.path === rename.temporary); - assert.ok(tempSync >= 0 && tempSync < rename.at, `the temp file synced before the rename: ${JSON.stringify(calls)}`); - const dirSync = calls.findIndex((c, i) => i > rename.at && c.call === 'fsync' && c.path === recovery); - assert.ok(dirSync > rename.at, `the directory synced after the rename: ${JSON.stringify(calls)}`); + assertSyncedAround(calls, record, recovery); assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); }); }); -test('a host-mode record that can\'t be replaced leaves no temp file behind', async () => { +test('a context write deferred while quiesced syncs the deferred-writes record the same way', async () => { + await withFramework(async (framework, recovery) => { + await framework.quiesce({ reason: 'test' }); + const add = (framework as unknown as { addMessage(participant: string, content: unknown[]): unknown }).addMessage.bind(framework); + const record = join(recovery, 'deferred-writes.json'); + const calls = await recordingFs(async () => { add('user', [{ type: 'text', text: 'held' }]); }); + assertSyncedAround(calls, record, recovery); + const pending = (JSON.parse(readFileSync(record, 'utf8')) as { pending: Array<{ content: Array<{ text: string }> }> }).pending; + assert.deepEqual(pending.map((entry) => entry.content[0]!.text), ['held']); + }); +}); + +test('a write that makes the recovery directory syncs its parent too', async () => { + await withFramework(async (framework, _recovery, dir) => { + const fresh = join(dir, 'fresh'); + const record = join(fresh, 'host-mode.json'); + assert.equal(existsSync(fresh), false); + const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); + assertSyncedAround(calls, record, fresh); + const rename = recordRename(calls, record); + assert.ok(calls.some((c, i) => i > rename.at && c.call === 'fsync' && c.path === dir), `${dir} synced: ${JSON.stringify(calls)}`); + }, { hostModePath: (dir) => join(dir, 'fresh', 'host-mode.json') }); +}); + +test('a host-mode record that can\'t be replaced leaves no temp file behind, and quiesce says the record was left as it was', async () => { await withFramework(async (framework, recovery) => { + const record = join(recovery, 'host-mode.json'); // A directory where the record goes, so the rename fails. - mkdirSync(join(recovery, 'host-mode.json'), { recursive: true }); - await assert.rejects(framework.quiesce({ reason: 'test' })); + mkdirSync(record, { recursive: true }); + let error: unknown; + const calls = await recordingFs(() => framework.quiesce({ reason: 'test' }).catch((e: unknown) => { error = e; })); + recordRename(calls, record); + assert.ok(error instanceof Error, String(error)); + assert.match(error.message, new RegExp(`^${record.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')} was left as it was: EISDIR`)); + assert.equal((error as NodeJS.ErrnoException).code, 'EISDIR'); assert.deepEqual(readdirSync(recovery).filter((name) => name.endsWith('.tmp')), []); }); }); +test('a temp file whose sync fails is removed, and the record keeps what it held', async () => { + await withFramework(async (framework, recovery) => { + const record = join(recovery, 'host-mode.json'); + await framework.quiesce({ reason: 'first' }); + await framework.resume(); + const before = readFileSync(record, 'utf8'); + let error: unknown; + const calls = await recordingFs( + () => framework.quiesce({ reason: 'test' }).catch((e: unknown) => { error = e; }), + (c) => (c.call === 'fsync' && isTemporaryOf(record, c.path) ? fsError('EIO', 'fsync') : undefined), + ); + assert.ok(error instanceof Error, String(error)); + assert.equal(error.message, `${record} was left as it was: EIO: i/o error, fsync`); + assert.ok(!calls.some((c) => c.call === 'rename' && c.to === record), `no rename: ${JSON.stringify(calls)}`); + assert.equal(readFileSync(record, 'utf8'), before); + assert.deepEqual(readdirSync(recovery).filter((name) => name.endsWith('.tmp')), []); + }); +}); + +test('a temp file that can\'t be removed after a failed write is named in the log', async (t) => { + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + await withFramework(async (framework, recovery) => { + const record = join(recovery, 'host-mode.json'); + let error: unknown; + const calls = await recordingFs( + () => framework.quiesce({ reason: 'test' }).catch((e: unknown) => { error = e; }), + (c) => (isTemporaryOf(record, c.path) && (c.call === 'fsync' || c.call === 'unlink') ? fsError(c.call === 'fsync' ? 'EIO' : 'EACCES', c.call) : undefined), + ); + assert.ok(error instanceof Error, String(error)); + const unlink = calls.find((c) => c.call === 'unlink' && isTemporaryOf(record, c.path)); + assert.ok(unlink, JSON.stringify(calls)); + assert.ok(logged.some((line) => line.includes(`couldn't remove ${unlink.path} after a failed write`)), logged.join('\n')); + }); +}); + test('on win32 the temp file is still synced, and the directory is not opened to sync it', async () => { await withFramework(async (framework, recovery) => { const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; @@ -125,11 +215,10 @@ test('a directory sync that fails after the rename leaves the new record in plac t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); - const eio = () => Object.assign(new Error('EIO: i/o error, fsync'), { code: 'EIO' }); - await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.path === recovery ? eio() : undefined)); + await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.call === 'fsync' && c.path === recovery ? fsError('EIO', 'fsync') : undefined)); assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); assert.ok( - logged.some((line) => line.includes(`${record} is in place, but syncing its directory failed, so a crash could still lose this change`)), + logged.some((line) => line.includes(`${record} is in place, but syncing ${recovery} failed, so a crash could still lose this change`)), logged.join('\n'), ); }); From bd592ae10908f5fe9910b3dd5ed56a91bbf89da8 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 18:37:43 -0400 Subject: [PATCH 03/10] fix(host-mode): sync each directory a recovery write makes before writing, 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 --- changelog.d/recovery-file-durability.fixed.md | 2 +- src/framework.ts | 68 ++++++++++++------- test/recovery-file-durability.test.ts | 62 ++++++++++++----- 3 files changed, 89 insertions(+), 43 deletions(-) diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md index 42d1b6b85..1083bb510 100644 --- a/changelog.d/recovery-file-durability.fixed.md +++ b/changelog.d/recovery-file-durability.fixed.md @@ -1 +1 @@ -- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename (with its parent, when the write had to make the directory), except on Windows, where only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails after the rename is logged, since the new record is already in place. +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename, with each directory the write has to make synced into its parent first. On Windows only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails raises an ops alert naming the record, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index bcd7f4478..68f4bfc46 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5301,20 +5301,32 @@ export class AgentFramework { } /** Atomic, durable JSON write for the recovery files. The document goes to - * a temp file at 0600, which is synced and renamed over the record. Then - * the directory is synced, and its parent too if this write had to make - * the directory, except on win32. So a crash leaves the record as it was - * or the new one, whole. A failure before the rename removes the temp file - * and throws an error saying the record was left as it was, with the - * original as its cause and its code kept. A directory sync that fails - * after the rename is logged, not thrown: the new record is in place, and - * the callers go on to record it as written. */ + * a temp file at 0600, which is synced and renamed over the record, and + * then the directory is synced. A directory this write makes is synced + * into its parent before anything is written to it. Directories are synced + * on POSIX only; win32 syncs the file. So a crash leaves the record as it + * was or the new one, whole. A failure before the rename removes the temp + * file, if it was made, and throws an error saying the record was left as + * it was, with the original as its cause and its code kept. A directory + * sync that fails is raised as an ops alert, not thrown: after the rename + * the new record is in place, and the callers go on to record it as + * written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); - const made = mkdirSync(directory, { recursive: true }); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; + let opened = false; try { + const made = mkdirSync(directory, { recursive: true }); + // mkdirSync answers the first directory it made, in the form it was + // given: each one it made, up to that one, is a new entry in its parent. + if (made !== undefined) { + for (let entry = directory; ; entry = dirname(entry)) { + this.syncRecoveryDirectory(dirname(entry), path); + if (entry === made || dirname(entry) === entry) break; + } + } const fd = openSync(temporary, 'w', 0o600); + opened = true; try { writeFileSync(fd, `${JSON.stringify(document, null, 2)}\n`); fsyncSync(fd); @@ -5323,31 +5335,37 @@ export class AgentFramework { } renameSync(temporary, path); } catch (err) { - try { - unlinkSync(temporary); - } catch (unlinkErr) { - if ((unlinkErr as NodeJS.ErrnoException).code !== 'ENOENT') { + if (opened) { + try { + unlinkSync(temporary); + } catch (unlinkErr) { console.error(`[host-mode] couldn't remove ${temporary} after a failed write:`, unlinkErr); } } const cause = err as NodeJS.ErrnoException; throw Object.assign(new Error(`${path} was left as it was: ${cause.message}`, { cause }), cause.code ? { code: cause.code } : {}); } + this.syncRecoveryDirectory(directory, path); + } + + /** Syncs a directory a recovery file is in, or one of its new parents, so + * its entries reach the disk; win32 is left out. A failure is raised as an + * ops alert naming the record, since a failing directory sync usually + * means a failing disk. */ + private syncRecoveryDirectory(directory: string, record: string): void { if (process.platform === 'win32') return; - for (const synced of made === undefined ? [directory] : [directory, dirname(directory)]) { + try { + const fd = openSync(directory, 'r'); try { - const fd = openSync(synced, 'r'); - try { - fsyncSync(fd); - } finally { - closeSync(fd); - } - } catch (err) { - console.error( - `[host-mode] ${path} is in place, but syncing ${synced} failed, so a crash could still lose this change:`, - err, - ); + fsyncSync(fd); + } finally { + closeSync(fd); } + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + this.opsAlert('recovery-sync', 'framework', + `syncing ${directory} failed, so ${record} may not survive a crash: ${reason}`, + { data: { record, directory, code: (err as NodeJS.ErrnoException).code } }); } } diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index 39a40cc56..1d3212c44 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -6,10 +6,10 @@ */ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import fs, { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs'; +import fs, { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { AgentFramework } from '../src/index.js'; import { MockMembrane } from './helpers/mock-membrane.js'; @@ -131,16 +131,40 @@ test('a context write deferred while quiesced syncs the deferred-writes record t }); }); -test('a write that makes the recovery directory syncs its parent too', async () => { +test('a write that makes the record\'s directories syncs each into its parent before writing, and no higher', async () => { await withFramework(async (framework, _recovery, dir) => { - const fresh = join(dir, 'fresh'); - const record = join(fresh, 'host-mode.json'); - assert.equal(existsSync(fresh), false); + const upper = join(dir, 'a'); + const lower = join(upper, 'b'); + const record = join(lower, 'host-mode.json'); + assert.equal(existsSync(upper), false); const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); - assertSyncedAround(calls, record, fresh); - const rename = recordRename(calls, record); - assert.ok(calls.some((c, i) => i > rename.at && c.call === 'fsync' && c.path === dir), `${dir} synced: ${JSON.stringify(calls)}`); - }, { hostModePath: (dir) => join(dir, 'fresh', 'host-mode.json') }); + assertSyncedAround(calls, record, lower); + const tempOpen = calls.findIndex((c) => c.call === 'open' && isTemporaryOf(record, c.path)); + const synced = (path: string) => calls.findIndex((c) => c.call === 'fsync' && c.path === path); + for (const parent of [upper, dir]) { + assert.ok(synced(parent) >= 0 && synced(parent) < tempOpen, `${parent} synced before the temp file is opened: ${JSON.stringify(calls)}`); + } + assert.equal(synced(dirname(dir)), -1, `${dirname(dir)}, which held no new directory, isn't synced`); + }, { hostModePath: (dir) => join(dir, 'a', 'b', 'host-mode.json') }); +}); + +test('a write whose directory can\'t be made says the record was left as it was, and tries to remove no temp file', async () => { + await withFramework(async (framework, _recovery, dir) => { + const blocker = join(dir, 'blocker'); + writeFileSync(blocker, 'a file where the record\'s directory goes'); + const record = join(blocker, 'host-mode.json'); + let error: unknown; + const calls = await recordingFs(() => framework.quiesce({ reason: 'test' }).catch((e: unknown) => { error = e; })); + assert.ok(error instanceof Error, String(error)); + const cause = error.cause as NodeJS.ErrnoException; + assert.ok(typeof cause?.code === 'string', String(cause)); + assert.equal(error.message, `${record} was left as it was: ${cause.message}`); + assert.equal((error as NodeJS.ErrnoException).code, cause.code); + assert.ok( + !calls.some((c) => (c.call === 'open' || c.call === 'unlink') && isTemporaryOf(record, c.path)), + `no temp file opened or removed: ${JSON.stringify(calls)}`, + ); + }, { hostModePath: (dir) => join(dir, 'blocker', 'host-mode.json') }); }); test('a host-mode record that can\'t be replaced leaves no temp file behind, and quiesce says the record was left as it was', async () => { @@ -210,16 +234,20 @@ test('on win32 the temp file is still synced, and the directory is not opened to }); }); -test('a directory sync that fails after the rename leaves the new record in place and says so', async (t) => { - const logged: string[] = []; - t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); +test('a directory sync that fails after the rename leaves the new record in place and raises an ops alert naming it', async () => { await withFramework(async (framework, recovery) => { + const alerts: Array<{ kind: string; agentName: string; message: string; data?: Record }> = []; + (framework as unknown as { opsAlert: unknown }).opsAlert = (kind: string, agentName: string, message: string, opts?: { data?: Record }) => { + alerts.push({ kind, agentName, message, data: opts?.data }); + }; const record = join(recovery, 'host-mode.json'); await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.call === 'fsync' && c.path === recovery ? fsError('EIO', 'fsync') : undefined)); assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); - assert.ok( - logged.some((line) => line.includes(`${record} is in place, but syncing ${recovery} failed, so a crash could still lose this change`)), - logged.join('\n'), - ); + assert.deepEqual(alerts, [{ + kind: 'recovery-sync', + agentName: 'framework', + message: `syncing ${recovery} failed, so ${record} may not survive a crash: EIO: i/o error, fsync`, + data: { record, directory: recovery, code: 'EIO' }, + }]); }); }); From f404ed93c90d3272c49185e949aa82bdf2abbf34 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 18:49:26 -0400 Subject: [PATCH 04/10] fix(host-mode): sync the recovery directory's parent on every write, 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 --- changelog.d/recovery-file-durability.fixed.md | 2 +- src/framework.ts | 29 +++++------- test/recovery-file-durability.test.ts | 46 ++++++++++--------- 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md index 1083bb510..1b8a78541 100644 --- a/changelog.d/recovery-file-durability.fixed.md +++ b/changelog.d/recovery-file-durability.fixed.md @@ -1 +1 @@ -- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory after the rename, with each directory the write has to make synced into its parent first. On Windows only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails raises an ops alert naming the record, since the new record is already in place. +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory and its parent after the rename. On Windows only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails raises an ops alert naming the record, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index 8494e0231..f82d32212 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5357,30 +5357,22 @@ export class AgentFramework { } /** Atomic, durable JSON write for the recovery files. The document goes to - * a temp file at 0600, which is synced and renamed over the record, and - * then the directory is synced. A directory this write makes is synced - * into its parent before anything is written to it. Directories are synced + * a temp file at 0600, which is synced and renamed over the record. Then + * the directory is synced, for the record's entry in it, and so is its + * parent, for the directory's own entry, which is new when a write, this + * one or an earlier one, had to make the directory. Directories are synced * on POSIX only; win32 syncs the file. So a crash leaves the record as it * was or the new one, whole. A failure before the rename removes the temp * file, if it was made, and throws an error saying the record was left as * it was, with the original as its cause and its code kept. A directory - * sync that fails is raised as an ops alert, not thrown: after the rename - * the new record is in place, and the callers go on to record it as - * written. */ + * sync that fails is raised as an ops alert, not thrown: the new record is + * in place, and the callers go on to record it as written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; let opened = false; try { - const made = mkdirSync(directory, { recursive: true }); - // mkdirSync answers the first directory it made, in the form it was - // given: each one it made, up to that one, is a new entry in its parent. - if (made !== undefined) { - for (let entry = directory; ; entry = dirname(entry)) { - this.syncRecoveryDirectory(dirname(entry), path); - if (entry === made || dirname(entry) === entry) break; - } - } + mkdirSync(directory, { recursive: true }); const fd = openSync(temporary, 'w', 0o600); opened = true; try { @@ -5402,11 +5394,12 @@ export class AgentFramework { throw Object.assign(new Error(`${path} was left as it was: ${cause.message}`, { cause }), cause.code ? { code: cause.code } : {}); } this.syncRecoveryDirectory(directory, path); + this.syncRecoveryDirectory(dirname(directory), path); } - /** Syncs a directory a recovery file is in, or one of its new parents, so - * its entries reach the disk; win32 is left out. A failure is raised as an - * ops alert naming the record, since a failing directory sync usually + /** Syncs the directory a recovery file is in, or that directory's parent, + * so its entries reach the disk; win32 is left out. A failure is raised as + * an ops alert naming the record, since a failing directory sync usually * means a failing disk. */ private syncRecoveryDirectory(directory: string, record: string): void { if (process.platform === 'win32') return; diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index 1d3212c44..5611f13b1 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -1,8 +1,8 @@ /** * The recovery files (host mode, deferred writes) are written as a temp file - * synced and renamed over the record, with the directory synced after the - * rename. These drive both records through the framework, recording the - * node:fs calls each write makes. + * synced and renamed over the record, with the directory and its parent + * synced after the rename. These drive both records through the framework, + * recording the node:fs calls each write makes. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -110,11 +110,12 @@ function assertSyncedAround(calls: FsCall[], record: string, directory: string): ); } -test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory after', async () => { +test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory and its parent after', async () => { await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); assertSyncedAround(calls, record, recovery); + assertSyncedAround(calls, record, dirname(recovery)); assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); }); }); @@ -131,21 +132,20 @@ test('a context write deferred while quiesced syncs the deferred-writes record t }); }); -test('a write that makes the record\'s directories syncs each into its parent before writing, and no higher', async () => { +test('a directory a failed write had to make is synced into its parent by the next write', async () => { await withFramework(async (framework, _recovery, dir) => { - const upper = join(dir, 'a'); - const lower = join(upper, 'b'); - const record = join(lower, 'host-mode.json'); - assert.equal(existsSync(upper), false); - const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); - assertSyncedAround(calls, record, lower); - const tempOpen = calls.findIndex((c) => c.call === 'open' && isTemporaryOf(record, c.path)); - const synced = (path: string) => calls.findIndex((c) => c.call === 'fsync' && c.path === path); - for (const parent of [upper, dir]) { - assert.ok(synced(parent) >= 0 && synced(parent) < tempOpen, `${parent} synced before the temp file is opened: ${JSON.stringify(calls)}`); - } - assert.equal(synced(dirname(dir)), -1, `${dirname(dir)}, which held no new directory, isn't synced`); - }, { hostModePath: (dir) => join(dir, 'a', 'b', 'host-mode.json') }); + const fresh = join(dir, 'fresh'); + const record = join(fresh, 'host-mode.json'); + assert.equal(existsSync(fresh), false); + await recordingFs( + () => framework.quiesce({ reason: 'first' }).catch(() => undefined), + (c) => (c.call === 'fsync' && isTemporaryOf(record, c.path) ? fsError('EIO', 'fsync') : undefined), + ); + assert.equal(existsSync(fresh), true, 'the failed write made the directory'); + const calls = await recordingFs(() => framework.quiesce({ reason: 'second' })); + assertSyncedAround(calls, record, fresh); + assertSyncedAround(calls, record, dir); + }, { hostModePath: (dir) => join(dir, 'fresh', 'host-mode.json') }); }); test('a write whose directory can\'t be made says the record was left as it was, and tries to remove no temp file', async () => { @@ -182,7 +182,7 @@ test('a host-mode record that can\'t be replaced leaves no temp file behind, and }); }); -test('a temp file whose sync fails is removed, and the record keeps what it held', async () => { +test('a quiesce whose temp file can\'t be synced rejects with the host still serving, the record as it was and no temp file', async () => { await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); await framework.quiesce({ reason: 'first' }); @@ -196,6 +196,7 @@ test('a temp file whose sync fails is removed, and the record keeps what it held assert.ok(error instanceof Error, String(error)); assert.equal(error.message, `${record} was left as it was: EIO: i/o error, fsync`); assert.ok(!calls.some((c) => c.call === 'rename' && c.to === record), `no rename: ${JSON.stringify(calls)}`); + assert.equal(framework.getHostModeStatus().quiesced, false, 'the host still serving'); assert.equal(readFileSync(record, 'utf8'), before); assert.deepEqual(readdirSync(recovery).filter((name) => name.endsWith('.tmp')), []); }); @@ -218,7 +219,7 @@ test('a temp file that can\'t be removed after a failed write is named in the lo }); }); -test('on win32 the temp file is still synced, and the directory is not opened to sync it', async () => { +test('on win32 the temp file is still synced, and no directory is opened to sync it', async () => { await withFramework(async (framework, recovery) => { const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; Object.defineProperty(process, 'platform', { ...platform, value: 'win32' }); @@ -230,7 +231,10 @@ test('on win32 the temp file is still synced, and the directory is not opened to } const rename = recordRename(calls, join(recovery, 'host-mode.json')); assert.ok(calls.some((c, i) => i < rename.at && c.call === 'fsync' && c.path === rename.temporary), JSON.stringify(calls)); - assert.ok(!calls.some((c) => c.call === 'open' && c.path === recovery), `the directory not opened: ${JSON.stringify(calls)}`); + assert.ok( + !calls.some((c) => c.call === 'open' && (c.path === recovery || c.path === dirname(recovery))), + `neither directory opened: ${JSON.stringify(calls)}`, + ); }); }); From 806012526171717731c79ee430ecbc836808b1b8 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 18:59:05 -0400 Subject: [PATCH 05/10] test(host-mode): fail the parent directory's sync too, and check the 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 --- src/framework.ts | 19 +++++++------- test/recovery-file-durability.test.ts | 36 +++++++++++++++------------ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index f82d32212..445286e81 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5358,15 +5358,16 @@ export class AgentFramework { /** Atomic, durable JSON write for the recovery files. The document goes to * a temp file at 0600, which is synced and renamed over the record. Then - * the directory is synced, for the record's entry in it, and so is its - * parent, for the directory's own entry, which is new when a write, this - * one or an earlier one, had to make the directory. Directories are synced - * on POSIX only; win32 syncs the file. So a crash leaves the record as it - * was or the new one, whole. A failure before the rename removes the temp - * file, if it was made, and throws an error saying the record was left as - * it was, with the original as its cause and its code kept. A directory - * sync that fails is raised as an ops alert, not thrown: the new record is - * in place, and the callers go on to record it as written. */ + * the directory is synced, for the record's entry in it, and its parent, + * for the directory's own entry, which is new when a write, this one or an + * earlier one, had to make the directory; levels above are left to the + * filesystem. Directories are synced on POSIX only; win32 syncs the file. + * So a crash leaves the record as it was or the new one, whole. A failure + * before the rename removes the temp file, if it was made, and throws an + * error saying the record was left as it was, with the original as its + * cause and its code kept. A directory sync that fails is raised as an ops + * alert, not thrown: the new record is in place, and the callers go on to + * record it as written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index 5611f13b1..5c7e2d423 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -127,6 +127,7 @@ test('a context write deferred while quiesced syncs the deferred-writes record t const record = join(recovery, 'deferred-writes.json'); const calls = await recordingFs(async () => { add('user', [{ type: 'text', text: 'held' }]); }); assertSyncedAround(calls, record, recovery); + assertSyncedAround(calls, record, dirname(recovery)); const pending = (JSON.parse(readFileSync(record, 'utf8')) as { pending: Array<{ content: Array<{ text: string }> }> }).pending; assert.deepEqual(pending.map((entry) => entry.content[0]!.text), ['held']); }); @@ -238,20 +239,23 @@ test('on win32 the temp file is still synced, and no directory is opened to sync }); }); -test('a directory sync that fails after the rename leaves the new record in place and raises an ops alert naming it', async () => { - await withFramework(async (framework, recovery) => { - const alerts: Array<{ kind: string; agentName: string; message: string; data?: Record }> = []; - (framework as unknown as { opsAlert: unknown }).opsAlert = (kind: string, agentName: string, message: string, opts?: { data?: Record }) => { - alerts.push({ kind, agentName, message, data: opts?.data }); - }; - const record = join(recovery, 'host-mode.json'); - await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.call === 'fsync' && c.path === recovery ? fsError('EIO', 'fsync') : undefined)); - assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); - assert.deepEqual(alerts, [{ - kind: 'recovery-sync', - agentName: 'framework', - message: `syncing ${recovery} failed, so ${record} may not survive a crash: EIO: i/o error, fsync`, - data: { record, directory: recovery, code: 'EIO' }, - }]); +for (const which of ['directory', 'parent'] as const) { + test(`a sync of the record's ${which} that fails after the rename leaves the new record in place and raises an ops alert naming it`, async () => { + await withFramework(async (framework, recovery) => { + const alerts: Array<{ kind: string; agentName: string; message: string; data?: Record }> = []; + (framework as unknown as { opsAlert: unknown }).opsAlert = (kind: string, agentName: string, message: string, opts?: { data?: Record }) => { + alerts.push({ kind, agentName, message, data: opts?.data }); + }; + const record = join(recovery, 'host-mode.json'); + const failing = which === 'directory' ? recovery : dirname(recovery); + await recordingFs(() => framework.quiesce({ reason: 'test' }), (c) => (c.call === 'fsync' && c.path === failing ? fsError('EIO', 'fsync') : undefined)); + assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); + assert.deepEqual(alerts, [{ + kind: 'recovery-sync', + agentName: 'framework', + message: `syncing ${failing} failed, so ${record} may not survive a crash: EIO: i/o error, fsync`, + data: { record, directory: failing, code: 'EIO' }, + }]); + }); }); -}); +} From 89ecd775efe396c1842b917dd20d7f8d4327dbbf Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 19:09:01 -0400 Subject: [PATCH 06/10] test(host-mode): check the record's 0600 mode and each descriptor's close, 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 --- changelog.d/quiesce-writes-first.fixed.md | 2 +- src/framework.ts | 3 +-- test/recovery-file-durability.test.ts | 15 +++++++++++++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/changelog.d/quiesce-writes-first.fixed.md b/changelog.d/quiesce-writes-first.fixed.md index ddb8939af..423521c92 100644 --- a/changelog.d/quiesce-writes-first.fixed.md +++ b/changelog.d/quiesce-writes-first.fixed.md @@ -1,4 +1,4 @@ - `quiesce()` and `resume()` now answer only once the host-mode record agrees with the host, or fail, apart from calls that overlap. - **`quiesce()`** writes the record before it marks the host quiesced. When that write failed, `quiesce()` rejected with the host quiesced in memory while its record said it was serving, so a restart booted serving, and a second `quiesce()` resolved without writing the record. Now a failed write leaves the host serving, as it was, and the next `quiesce()` quiesces and writes the record. - **`resume()`** still serves before it clears the record, so a failed clear leaves the record quiesced. A second `resume()` used to resolve without clearing it, so the next restart booted quiesced; it now clears the record. While a surgery holds the store, that clear is refused as `resume()` is on a quiesced host, and a `resume()` arriving while another is still flushing leaves the clear to that one. - - **Without a recovery file,** the store is the record, and a failed sync of the store after the write was only logged, so `quiesce()` resolved before its record was durable. It now fails `quiesce()` or `resume()`. The write is already in the store's log, though, so after a `quiesce()` that failed this way the record can still reach disk, and a restart can boot quiesced unless a `resume()` clears it first. With a recovery file, the file is the record, and a failed sync is still only logged. + - **Without a recovery file,** the store is the record, and a failed sync of the store after the write was only logged, so `quiesce()` resolved before its record was durable. It now fails `quiesce()` or `resume()`. The write is already in the store's log, though, so after a `quiesce()` that failed this way the record can still reach disk, and a restart can boot quiesced unless a `resume()` clears it first. With a recovery file, the file is the record, and a failed sync of the store is still only logged. diff --git a/src/framework.ts b/src/framework.ts index 445286e81..2b07425d7 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5400,8 +5400,7 @@ export class AgentFramework { /** Syncs the directory a recovery file is in, or that directory's parent, * so its entries reach the disk; win32 is left out. A failure is raised as - * an ops alert naming the record, since a failing directory sync usually - * means a failing disk. */ + * an ops alert naming the record. */ private syncRecoveryDirectory(directory: string, record: string): void { if (process.platform === 'win32') return; try { diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index 5c7e2d423..bec1d6b69 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -6,7 +6,7 @@ */ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import fs, { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import fs, { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'; import { syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; @@ -110,12 +110,23 @@ function assertSyncedAround(calls: FsCall[], record: string, directory: string): ); } -test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory and its parent after', async () => { +/** Every descriptor opened on a path `ours` names is closed before its number is opened again. */ +function assertEachClosed(calls: FsCall[], ours: (path: string | undefined) => boolean): void { + calls.forEach((c, i) => { + if (c.call !== 'open' || !ours(c.path)) return; + const next = calls.findIndex((d, j) => j > i && d.fd === c.fd && (d.call === 'close' || d.call === 'open')); + assert.ok(next >= 0 && calls[next]!.call === 'close', `${c.path} (fd ${c.fd}) closed: ${JSON.stringify(calls)}`); + }); +} + +test('quiesce syncs the host-mode record\'s temp file before renaming it over the record, and the directory and its parent after, closing each and publishing the record at 0600', async () => { await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); const calls = await recordingFs(() => framework.quiesce({ reason: 'test' })); assertSyncedAround(calls, record, recovery); assertSyncedAround(calls, record, dirname(recovery)); + assertEachClosed(calls, (path) => isTemporaryOf(record, path) || path === recovery || path === dirname(recovery)); + assert.equal(statSync(record).mode & 0o777, 0o600); assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); }); }); From 4e7b984b201f79a2b8c4f2ccb52fcce35c575c10 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 19:19:17 -0400 Subject: [PATCH 07/10] test(host-mode): check the temp file's descriptor is closed when its 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 --- test/recovery-file-durability.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index bec1d6b69..bf86b2e6e 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -194,7 +194,7 @@ test('a host-mode record that can\'t be replaced leaves no temp file behind, and }); }); -test('a quiesce whose temp file can\'t be synced rejects with the host still serving, the record as it was and no temp file', async () => { +test('a quiesce whose temp file can\'t be synced closes it and rejects with the host still serving, the record as it was and no temp file', async () => { await withFramework(async (framework, recovery) => { const record = join(recovery, 'host-mode.json'); await framework.quiesce({ reason: 'first' }); @@ -208,6 +208,7 @@ test('a quiesce whose temp file can\'t be synced rejects with the host still ser assert.ok(error instanceof Error, String(error)); assert.equal(error.message, `${record} was left as it was: EIO: i/o error, fsync`); assert.ok(!calls.some((c) => c.call === 'rename' && c.to === record), `no rename: ${JSON.stringify(calls)}`); + assertEachClosed(calls, (path) => isTemporaryOf(record, path)); assert.equal(framework.getHostModeStatus().quiesced, false, 'the host still serving'); assert.equal(readFileSync(record, 'utf8'), before); assert.deepEqual(readdirSync(recovery).filter((name) => name.endsWith('.tmp')), []); From d71a4dcfda4a6d9ab15b3d0a669ccfe4df4aaad7 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 19:26:56 -0400 Subject: [PATCH 08/10] test(host-mode): name the store's sync in the quiesce-writes-first test'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 --- test/host-quiesce.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/host-quiesce.test.ts b/test/host-quiesce.test.ts index 81207f91f..0eb1f518f 100644 --- a/test/host-quiesce.test.ts +++ b/test/host-quiesce.test.ts @@ -489,7 +489,7 @@ test("a resume() during another resume()'s flush leaves the record to it, and it } }); -test("store only, a host-mode change whose sync fails rejects; with a recovery file, the file is the record and a failed sync is only logged", async () => { +test("store only, a host-mode change whose sync fails rejects; with a recovery file, the file is the record and a failed sync of the store is only logged", async () => { const logged = captureErrors(); try { const slot = await hostModeFixture('store only'); From 8b8e96aa64a2a359524d3c106c798796dedbca82 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 19:48:41 -0400 Subject: [PATCH 09/10] fix(host-mode): let a directory sync refused with EINVAL or EBADF go, 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 --- changelog.d/recovery-file-durability.fixed.md | 2 +- src/framework.ts | 20 +++++++--- test/recovery-file-durability.test.ts | 38 ++++++++++++++++--- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md index 1b8a78541..b7bfe1f01 100644 --- a/changelog.d/recovery-file-durability.fixed.md +++ b/changelog.d/recovery-file-durability.fixed.md @@ -1 +1 @@ -- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory and its parent after the rename. On Windows only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails raises an ops alert naming the record, since the new record is already in place. +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory and its parent after the rename. On Windows, and where the filesystem refuses a directory's sync with `EINVAL` or `EBADF`, only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails for any other reason raises an ops alert naming the record, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index 2b07425d7..8105e71a9 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5361,13 +5361,15 @@ export class AgentFramework { * the directory is synced, for the record's entry in it, and its parent, * for the directory's own entry, which is new when a write, this one or an * earlier one, had to make the directory; levels above are left to the - * filesystem. Directories are synced on POSIX only; win32 syncs the file. + * filesystem. Directories aren't synced on win32, which syncs the file + * alone, or where the filesystem can't sync one (syncRecoveryDirectory). * So a crash leaves the record as it was or the new one, whole. A failure * before the rename removes the temp file, if it was made, and throws an * error saying the record was left as it was, with the original as its - * cause and its code kept. A directory sync that fails is raised as an ops - * alert, not thrown: the new record is in place, and the callers go on to - * record it as written. */ + * cause and its code kept. A directory sync that fails, other than where + * the filesystem can't sync one, is raised as an ops alert, not thrown: + * the new record is in place, and the callers go on to record it as + * written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; @@ -5399,14 +5401,20 @@ export class AgentFramework { } /** Syncs the directory a recovery file is in, or that directory's parent, - * so its entries reach the disk; win32 is left out. A failure is raised as - * an ops alert naming the record. */ + * so its entries reach the disk. win32 is left out. So is a sync refused + * with EINVAL or EBADF, the answer where the filesystem or system can't + * sync a directory: that refusal would come again on every write, and + * PostgreSQL's fsync_fname_ext lets the same two go. Any other failure is + * raised as an ops alert naming the record. */ private syncRecoveryDirectory(directory: string, record: string): void { if (process.platform === 'win32') return; try { const fd = openSync(directory, 'r'); try { fsyncSync(fd); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'EINVAL' && code !== 'EBADF') throw err; } finally { closeSync(fd); } diff --git a/test/recovery-file-durability.test.ts b/test/recovery-file-durability.test.ts index bf86b2e6e..c81cf1620 100644 --- a/test/recovery-file-durability.test.ts +++ b/test/recovery-file-durability.test.ts @@ -131,16 +131,25 @@ test('quiesce syncs the host-mode record\'s temp file before renaming it over th }); }); -test('a context write deferred while quiesced syncs the deferred-writes record the same way', async () => { +test('each context write deferred while quiesced rewrites the deferred-writes record, synced the same way', async () => { await withFramework(async (framework, recovery) => { await framework.quiesce({ reason: 'test' }); const add = (framework as unknown as { addMessage(participant: string, content: unknown[]): unknown }).addMessage.bind(framework); const record = join(recovery, 'deferred-writes.json'); - const calls = await recordingFs(async () => { add('user', [{ type: 'text', text: 'held' }]); }); - assertSyncedAround(calls, record, recovery); - assertSyncedAround(calls, record, dirname(recovery)); + const calls = await recordingFs(async () => { + add('user', [{ type: 'text', text: 'held' }]); + add('user', [{ type: 'text', text: 'held too' }]); + }); + const renames = calls.flatMap((c, i) => (c.call === 'rename' && c.to === record ? [i] : [])); + assert.equal(renames.length, 2, `a rewrite for each deferred write: ${JSON.stringify(calls)}`); + renames.forEach((_at, k) => { + // This rewrite's calls: from the rename before it to the one after. + const write = calls.slice(k === 0 ? 0 : renames[k - 1]! + 1, renames[k + 1] ?? calls.length); + assertSyncedAround(write, record, recovery); + assertSyncedAround(write, record, dirname(recovery)); + }); const pending = (JSON.parse(readFileSync(record, 'utf8')) as { pending: Array<{ content: Array<{ text: string }> }> }).pending; - assert.deepEqual(pending.map((entry) => entry.content[0]!.text), ['held']); + assert.deepEqual(pending.map((entry) => entry.content[0]!.text), ['held', 'held too']); }); }); @@ -271,3 +280,22 @@ for (const which of ['directory', 'parent'] as const) { }); }); } + +for (const code of ['EINVAL', 'EBADF'] as const) { + test(`directory syncs refused with ${code}, as where the filesystem can't sync a directory, are let go with no ops alert and the new record in place`, async () => { + await withFramework(async (framework, recovery) => { + const alerts: unknown[] = []; + (framework as unknown as { opsAlert: unknown }).opsAlert = (...args: unknown[]) => { alerts.push(args); }; + const record = join(recovery, 'host-mode.json'); + const directories = [recovery, dirname(recovery)]; + const calls = await recordingFs( + () => framework.quiesce({ reason: 'test' }), + (c) => (c.call === 'fsync' && directories.includes(c.path!) ? fsError(code, 'fsync') : undefined), + ); + for (const directory of directories) assertSyncedAround(calls, record, directory); + assertEachClosed(calls, (path) => path !== undefined && directories.includes(path)); + assert.equal(JSON.parse(readFileSync(record, 'utf8')).quiesced, true); + assert.deepEqual(alerts, []); + }); + }); +} From 3f8f1e93ec7ca91ac897a815dee075e2e99a57b1 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sat, 17 Oct 2026 20:03:49 -0400 Subject: [PATCH 10/10] docs(host-mode): say what the recovery writer's syncs ask for, not what 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 --- changelog.d/recovery-file-durability.fixed.md | 2 +- src/framework.ts | 26 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/changelog.d/recovery-file-durability.fixed.md b/changelog.d/recovery-file-durability.fixed.md index b7bfe1f01..b814ca080 100644 --- a/changelog.d/recovery-file-durability.fixed.md +++ b/changelog.d/recovery-file-durability.fixed.md @@ -1 +1 @@ -- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory and its parent after the rename. On Windows, and where the filesystem refuses a directory's sync with `EINVAL` or `EBADF`, only the file is synced. So a crash leaves the record as it was or the new one, whole. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails for any other reason raises an ops alert naming the record, since the new record is already in place. +- The host-mode and deferred-writes recovery files are now synced to disk before they replace the record, and their directory and its parent after the rename. On Windows, and where the filesystem refuses a directory's sync with `EINVAL` or `EBADF`, only the file is synced. A write that fails before the rename removes its `.tmp` file instead of leaving it beside the record, logs one it can't remove, and fails with an error that names the record and says it was left as it was. A directory sync that fails for any other reason raises an ops alert naming the record, since the new record is already in place. diff --git a/src/framework.ts b/src/framework.ts index 8105e71a9..9b7962fca 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -5357,19 +5357,19 @@ export class AgentFramework { } /** Atomic, durable JSON write for the recovery files. The document goes to - * a temp file at 0600, which is synced and renamed over the record. Then - * the directory is synced, for the record's entry in it, and its parent, - * for the directory's own entry, which is new when a write, this one or an - * earlier one, had to make the directory; levels above are left to the - * filesystem. Directories aren't synced on win32, which syncs the file - * alone, or where the filesystem can't sync one (syncRecoveryDirectory). - * So a crash leaves the record as it was or the new one, whole. A failure - * before the rename removes the temp file, if it was made, and throws an - * error saying the record was left as it was, with the original as its - * cause and its code kept. A directory sync that fails, other than where - * the filesystem can't sync one, is raised as an ops alert, not thrown: - * the new record is in place, and the callers go on to record it as - * written. */ + * a temp file at 0600, which is synced and renamed over the record, so + * the new record's data is asked to reach the disk before the rename + * publishes it. Then the directory is synced, for the record's entry in + * it, and its parent, for the directory's own entry, which is new when a + * write, this one or an earlier one, had to make the directory; levels + * above are left to the filesystem. Directories aren't synced on win32, + * which syncs the file alone, or where the filesystem can't sync one + * (syncRecoveryDirectory). A failure before the rename removes the temp + * file, if it was made, and throws an error saying the record was left as + * it was, with the original as its cause and its code kept. A directory + * sync that fails, other than where the filesystem can't sync one, is + * raised as an ops alert, not thrown: the new record is in place, and the + * callers go on to record it as written. */ private writeRecoveryFile(path: string, document: unknown): void { const directory = dirname(path); const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`;