fix: saveBinaryBlob never acks 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 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.
This commit is contained in:
parent
54c183668c
commit
7feba49d94
2 changed files with 114 additions and 21 deletions
|
|
@ -1178,30 +1178,47 @@ 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/<field>/DELETED.bin`.
|
||||
|
||||
// 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/<field>/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) {
|
||||
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.
|
||||
// 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.`
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Load the raw bytes stored under `key`, or `null` if the blob does not exist.
|
||||
*
|
||||
|
|
|
|||
76
tests/unit/storage/blob-save-durability.test.ts
Normal file
76
tests/unit/storage/blob-save-durability.test.ts
Normal file
|
|
@ -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' })
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue