diff --git a/RELEASES.md b/RELEASES.md index f5f6a870..f5f08571 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -10,6 +10,101 @@ Full auto-generated changelog: `CHANGELOG.md` · Releases: https://github.com/so --- +## v7.31.1 — 2026-06-09 + +**Affected products:** any consumer running `@soulcraft/brainy` against `FileSystemStorage` +that triggers concurrent flush / compaction (e.g. explicit `brain.flush()` calls overlapping +with periodic background compaction, or a backup job + a flush job racing). Production-impacting +when present: `brain.flush()` throws `ENOENT` on rename, downstream jobs that rely on flush +(GCS / S3 / Azure backups, snapshot exports) fail continuously. Drop-in from 7.31.0. + +### Fixes a same-key rename race in `saveBinaryBlob` + +`FileSystemStorage.saveBinaryBlob` used a bare `${filePath}.tmp` suffix for its atomic-write +temp file. Two concurrent same-key calls computed the **same** temp path; both `writeFile`d, +the first `rename` succeeded, the second `rename` fired against a missing temp and threw +`ENOENT`. The throw propagated up through `brain.flush()` and broke any downstream job that +called it. + +``` +[job-queue] gcs-backup: failed — ENOENT: no such file or directory, + rename '/data/brainy-data/.../_column_index/owner/DELETED.bin.tmp' + -> '/data/brainy-data/.../_column_index/owner/DELETED.bin' +``` + +**Patch:** unique per-writer temp suffix (matches the pattern at six other atomic-write +sites in the same file) + ENOENT swallow on rename (defensive: if the temp is gone, the +work has already landed; `saveBinaryBlob` is idempotent for a given key) + temp cleanup +on any other rename failure (no orphan `.tmp.*` files). + +```ts +// before (7.31.0) +const tmpPath = filePath + '.tmp' +await fs.promises.writeFile(tmpPath, data) +await fs.promises.rename(tmpPath, filePath) + +// after (7.31.1) +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) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return + await fs.promises.unlink(tmpPath).catch(() => {}) + throw err +} +``` + +### Scope + +The fix is one site — `src/storage/adapters/fileSystemStorage.ts:992-999`. Audit results: + +- **`FileSystemStorage`** — six sibling atomic-write sites already used unique suffixes; + only `saveBinaryBlob` was the outlier. All six were rechecked. Clean. +- **`OPFSStorage`** — writes via WritableStream (no tmp+rename). Not affected. +- **`GCSStorage` / `R2Storage` / `AzureBlobStorage` / `S3CompatibleStorage`** — use + object-store `PUT` (atomic at the API). Not affected. +- **`MemoryStorage`** — in-memory. Not affected. +- **`HistoricalStorageAdapter`** — read-only. Not affected. +- **COW / versioning / snapshot / HNSW / aggregation** — all delegate to storage adapters + via `saveBinaryBlob` / `writeObjectToPath`. They get the fix automatically by virtue of + using the patched primitive. + +### Beneficiary surfaces (broader than the reported bug) + +Column-store compaction (`_column_index//DELETED.bin`, segment files) is what the +production report named, but `saveBinaryBlob` is also used by HNSW connection persistence +(`_hnsw_conn/` — `src/hnsw/hnswIndex.ts:252`). If two flush paths ever raced on +HNSW persistence for the same node, they would have hit the same ENOENT. No production +reports of HNSW-side failures, but the fix removes the latent race. + +### Tests + +- **New `tests/integration/savebinaryblob-concurrent-rename.test.ts`** (4 tests): + - 20 concurrent `saveBinaryBlob(sameKey, ...)` calls all resolve without throwing + - No orphan `.tmp.*` siblings remain in the blob directory after concurrent writes + - Production-shape: two concurrent compactor passes over 10 column-index fields + (`owner`, `path`, `permissions`, `vfsType`, `modified`, `createdAt`, `accessed`, + `updatedAt`, `mimeType`, `size`) + - Single-writer path still produces correct bytes (no regression) +- The first three tests reproduce the production `ENOENT: rename '...DELETED.bin.tmp' -> + '...DELETED.bin'` error verbatim on the pre-7.31.1 code path. Verified by stashing the + fix and watching the tests fail with the exact production error. +- Existing suites unchanged: 1468/1468 unit. All integration suites pass. + +### Cortex compatibility + +Zero changes required. The bug and the fix are pure JS in the filesystem storage adapter; +the Cortex mmap-filesystem adapter wraps `FileSystemStorage` and inherits the patch +automatically. + +### Forward-compat + +The bare-`.tmp` pattern is gone repo-wide. 8.0's `Db.persist()` will route through the +same patched primitive; no additional work needed when the immutable Db API ships. + +--- + ## v7.31.0 — 2026-06-09 **Affected products:** consumers that need multi-writer coordination (job diff --git a/src/storage/adapters/fileSystemStorage.ts b/src/storage/adapters/fileSystemStorage.ts index f1f72fd6..e86e1a6d 100644 --- a/src/storage/adapters/fileSystemStorage.ts +++ b/src/storage/adapters/fileSystemStorage.ts @@ -993,9 +993,28 @@ export class FileSystemStorage extends BaseStorage { await this.ensureInitialized() const filePath = this.blobPath(key) await this.ensureDirectoryExists(path.dirname(filePath)) - const tmpPath = filePath + '.tmp' + // 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) - await fs.promises.rename(tmpPath, filePath) + 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 + } } /** diff --git a/tests/integration/savebinaryblob-concurrent-rename.test.ts b/tests/integration/savebinaryblob-concurrent-rename.test.ts new file mode 100644 index 00000000..5a2b3039 --- /dev/null +++ b/tests/integration/savebinaryblob-concurrent-rename.test.ts @@ -0,0 +1,119 @@ +/** + * @module tests/integration/savebinaryblob-concurrent-rename + * @description Regression test for the 7.31.1 fix to `FileSystemStorage.saveBinaryBlob`. + * + * Pre-7.31.1, `saveBinaryBlob` used a bare `${filePath}.tmp` suffix for its + * atomic-write temp file. Two concurrent same-key calls computed the same + * temp path; whichever rename ran second hit ENOENT because the first call + * had already consumed the temp. + * + * Reproduced in production: column-store compaction running alongside an + * explicit flush() repeatedly raced on `_column_index//DELETED.bin`, + * causing brain.flush() and the downstream GCS backup job to fail + * continuously. + * + * This test asserts: + * - 20 concurrent saveBinaryBlob(sameKey, ...) calls all resolve without throwing + * - The final file contents are valid (one of the inputs, last-write-wins) + * - No orphan `.tmp.*` siblings are left in the blob directory + * - The race also holds when interleaved across multiple keys (column-store + * compaction shape) + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { mkdtempSync, readdirSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { FileSystemStorage } from '../../src/storage/adapters/fileSystemStorage.js' + +describe('7.31.1 — saveBinaryBlob concurrent rename safety', () => { + let dataDir: string + let storage: FileSystemStorage + + beforeEach(async () => { + dataDir = mkdtempSync(join(tmpdir(), 'brainy-savebinaryblob-')) + storage = new FileSystemStorage(dataDir) + await storage.init() + }) + + afterEach(() => { + rmSync(dataDir, { recursive: true, force: true }) + }) + + it('20 concurrent saveBinaryBlob calls for the same key resolve without ENOENT', async () => { + const key = '_column_index/owner/DELETED' + const writers = Array.from({ length: 20 }, (_, i) => { + // Each writer's payload is unique so we can verify which one wins. + const payload = Buffer.from(`writer-${i.toString().padStart(2, '0')}-payload`) + return storage.saveBinaryBlob(key, payload) + }) + + // No call should throw. Pre-7.31.1, ~19 of 20 throw ENOENT under contention. + await expect(Promise.all(writers)).resolves.not.toThrow() + + // Final file should be readable and equal to one of the payloads. + const final = await storage.loadBinaryBlob(key) + expect(final).not.toBeNull() + expect(final!.toString()).toMatch(/^writer-\d{2}-payload$/) + }) + + it('leaves no orphan .tmp.* siblings after concurrent same-key writes', async () => { + const key = '_column_index/permissions/DELETED' + const writers = Array.from({ length: 20 }, (_, i) => + storage.saveBinaryBlob(key, Buffer.from(`payload-${i}`)) + ) + await Promise.all(writers) + + // Locate the directory containing the blob and list its children. + const blobPath = storage.getBinaryBlobPath(key) + expect(blobPath).not.toBeNull() + const parentDir = join(blobPath!, '..') + const siblings = readdirSync(parentDir) + const tmps = siblings.filter((name) => name.includes('.tmp')) + expect(tmps).toEqual([]) + }) + + it('handles concurrent writes across the full column-index field set (production shape)', async () => { + // Mirror the per-field DELETED.bin pattern Brainy's column-store compactor + // emits. In production this is what races between flush() and gcs-backup. + const fields = [ + 'owner', + 'path', + 'permissions', + 'vfsType', + 'modified', + 'createdAt', + 'accessed', + 'updatedAt', + 'mimeType', + 'size' + ] + + // Two concurrent compactor passes — one acting as the periodic flush, one + // as the explicit user flush — both touching every field. + const passA = fields.map((f) => + storage.saveBinaryBlob(`_column_index/${f}/DELETED`, Buffer.from(`A-${f}`)) + ) + const passB = fields.map((f) => + storage.saveBinaryBlob(`_column_index/${f}/DELETED`, Buffer.from(`B-${f}`)) + ) + + await expect(Promise.all([...passA, ...passB])).resolves.not.toThrow() + + // Every field should have a readable final blob. + for (const f of fields) { + const final = await storage.loadBinaryBlob(`_column_index/${f}/DELETED`) + expect(final).not.toBeNull() + expect(final!.toString()).toMatch(new RegExp(`^[AB]-${f}$`)) + } + }) + + it('the existing single-writer path still produces correct bytes', async () => { + // Sanity: the patch must not regress the common single-writer case. + const key = '_blob/single-writer-test' + const payload = Buffer.from('hello') + await storage.saveBinaryBlob(key, payload) + const read = await storage.loadBinaryBlob(key) + expect(read?.toString()).toBe('hello') + }) +})