From 7feba49d94f72eaae7e1c1dc4f0456017dbf473d Mon Sep 17 00:00:00 2001 From: David Snelling Date: Mon, 13 Jul 2026 09:35:30 -0700 Subject: [PATCH] fix: saveBinaryBlob never acks a durable write that stored nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the unique per-writer temp suffix, a rename ENOENT can no longer mean "a concurrent idempotent writer already renamed it" — nobody else holds this writer's temp. It means our just-written temp vanished before the rename, so the bytes did NOT land. The old code returned success on that ENOENT, acknowledging a write that persisted nothing; the native provider mmaps these blobs, so a phantom-acked blob is a silent-loss. saveBinaryBlob now retries once with a fresh temp (self-healing a transient external-sweeper/crash-cleanup race), and if the temp vanishes again it throws loud rather than acknowledging a store-nothing write. Non-ENOENT rename faults propagate verbatim as before. The unique-temp suffix already eliminated the concurrent same-key collision that originally motivated the ENOENT shortcut. --- src/storage/adapters/fileSystemStorage.ts | 59 +++++++++----- .../unit/storage/blob-save-durability.test.ts | 76 +++++++++++++++++++ 2 files changed, 114 insertions(+), 21 deletions(-) create mode 100644 tests/unit/storage/blob-save-durability.test.ts diff --git a/src/storage/adapters/fileSystemStorage.ts b/src/storage/adapters/fileSystemStorage.ts index 7e341ac5..95d3951d 100644 --- a/src/storage/adapters/fileSystemStorage.ts +++ b/src/storage/adapters/fileSystemStorage.ts @@ -1178,27 +1178,44 @@ export class FileSystemStorage extends BaseStorage { await this.ensureInitialized() const filePath = this.blobPath(key) await this.ensureDirectoryExists(path.dirname(filePath)) - // Unique per-writer temp suffix — matches the pattern used at every other - // atomic-write site in this file (lines 336, 551, 744, 781, 1529, 2908). - // Without a unique suffix, two concurrent saveBinaryBlob() calls for the - // same key collide on `${filePath}.tmp`: both writeFile, the first rename - // succeeds, the second fires against a missing temp and throws ENOENT. - // Reproduced in production: column-store compaction running alongside an - // explicit flush() repeatedly raced on `_column_index//DELETED.bin`. - const tmpPath = `${filePath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}` - await fs.promises.writeFile(tmpPath, data) - try { - await fs.promises.rename(tmpPath, filePath) - } catch (err) { - const code = (err as NodeJS.ErrnoException).code - // The writes are idempotent for a given key — every caller persists the - // same logical bytes for that key — so ENOENT on rename means the temp - // is already gone (rare with the unique suffix, but defensive against - // crash-resume cleanup paths and external file-system sweepers). - if (code === 'ENOENT') return - // Any other failure: clean up our own temp to avoid orphans before rethrow. - await fs.promises.unlink(tmpPath).catch(() => {}) - throw err + + // Atomic write via a UNIQUE per-writer temp suffix — matches the pattern + // used at every other atomic-write site in this file (lines 336, 551, 744, + // 781, 1529, 2908). The unique suffix (pid+time+random) means NO other + // writer ever touches this temp, so two concurrent saveBinaryBlob() calls + // for the same key can never collide on the temp path (the shared-`.tmp` + // race that once threw ENOENT — column-store compaction vs an explicit + // flush() on `_column_index//DELETED.bin` — is gone). + // + // Crucially, BECAUSE the temp is unique, a rename ENOENT can no longer mean + // "a concurrent idempotent writer already renamed it": nobody else has this + // temp. It means OUR just-written temp vanished before the rename, so the + // bytes did NOT land — returning success would acknowledge a write that + // stored nothing (the native provider mmaps these blobs; a phantom-acked + // blob is exactly the silent-loss class). The only way that happens with a + // unique temp is an external sweeper / crash-cleanup removing it mid-write, + // so retry ONCE with a fresh temp; if it vanishes again, FAIL LOUD. + const writeOnce = async (): Promise<'ok' | 'temp-vanished'> => { + const tmpPath = `${filePath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}` + await fs.promises.writeFile(tmpPath, data) + try { + await fs.promises.rename(tmpPath, filePath) + return 'ok' + } catch (err) { + // Clean up our own temp (best-effort) so no failure path orphans it. + await fs.promises.unlink(tmpPath).catch(() => {}) + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return 'temp-vanished' + throw err + } + } + + if ((await writeOnce()) === 'temp-vanished' && (await writeOnce()) === 'temp-vanished') { + throw new Error( + `saveBinaryBlob('${key}'): the temp file was removed before rename on two ` + + `successive attempts — the blob did NOT persist. Some external process is ` + + `deleting files under ${this.blobsDir} mid-write. Failing loud rather than ` + + `acknowledging a durable write that stored nothing.` + ) } } diff --git a/tests/unit/storage/blob-save-durability.test.ts b/tests/unit/storage/blob-save-durability.test.ts new file mode 100644 index 00000000..3c4b0a1a --- /dev/null +++ b/tests/unit/storage/blob-save-durability.test.ts @@ -0,0 +1,76 @@ +/** + * @module tests/unit/storage/blob-save-durability + * @description Finding 13 (cortex sweep): saveBinaryBlob must never acknowledge a + * durable write that stored nothing. With the UNIQUE per-writer temp suffix, a + * rename ENOENT can no longer mean "a concurrent idempotent writer already + * renamed it" — nobody else has this writer's temp — so it means our temp + * vanished before the rename and the bytes did NOT land. The old code returned + * success on that ENOENT; the native provider mmaps these blobs, so a + * phantom-acked blob is the exact silent-loss class the registered-blob work + * chases. The fix retries once with a fresh temp, then fails loud. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import * as fs from 'node:fs' +import * as os from 'node:os' +import * as path from 'node:path' +import { FileSystemStorage } from '../../../src/storage/adapters/fileSystemStorage.js' + +const enoent = (): Error => Object.assign(new Error('temp removed'), { code: 'ENOENT' }) + +describe('FileSystemStorage.saveBinaryBlob durable-write honesty (finding 13)', () => { + let dir: string + let storage: any + + beforeEach(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'brainy-blob-save-')) + storage = new FileSystemStorage(dir) + await storage.init() + }) + + afterEach(() => { + vi.restoreAllMocks() + fs.rmSync(dir, { recursive: true, force: true }) + }) + + it('persists and reads back a blob under normal conditions', async () => { + await storage.saveBinaryBlob('graph-lsm/seg-1', Buffer.from([1, 2, 3, 4])) + expect(await storage.loadBinaryBlob('graph-lsm/seg-1')).toEqual(Buffer.from([1, 2, 3, 4])) + }) + + it('throws (never acks) when the temp vanishes on every rename attempt', async () => { + vi.spyOn(fs.promises, 'rename').mockRejectedValue(enoent()) + + await expect( + storage.saveBinaryBlob('k/silent-loss', Buffer.from([9])) + ).rejects.toThrow(/did NOT persist/) + + // Prove the write truly did not land — and was not acknowledged. (loadBinaryBlob + // uses readFile, not rename, so it reflects the real on-disk state.) + expect(await storage.loadBinaryBlob('k/silent-loss')).toBeNull() + }) + + it('self-heals: a single transient temp-vanish retries and persists', async () => { + const real = fs.promises.rename.bind(fs.promises) + let calls = 0 + vi.spyOn(fs.promises, 'rename').mockImplementation(async (from: any, to: any) => { + calls++ + if (calls === 1) throw enoent() + return real(from, to) + }) + + await expect( + storage.saveBinaryBlob('k/heals', Buffer.from([7, 7])) + ).resolves.toBeUndefined() + expect(calls).toBe(2) // first attempt vanished, retry landed + expect(await storage.loadBinaryBlob('k/heals')).toEqual(Buffer.from([7, 7])) + }) + + it('a non-ENOENT rename fault propagates verbatim (not swallowed)', async () => { + vi.spyOn(fs.promises, 'rename').mockRejectedValue( + Object.assign(new Error('disk fault'), { code: 'EIO' }) + ) + await expect( + storage.saveBinaryBlob('k/eio', Buffer.from([1])) + ).rejects.toMatchObject({ code: 'EIO' }) + }) +})