diff --git a/packages/platform-node/src/file-storage.ts b/packages/platform-node/src/file-storage.ts index c9cf5b06..ba4db99b 100644 --- a/packages/platform-node/src/file-storage.ts +++ b/packages/platform-node/src/file-storage.ts @@ -1,12 +1,6 @@ -import { - existsSync, - mkdirSync, - readdirSync, - readFileSync, - renameSync, - unlinkSync, - writeFileSync, -} from 'node:fs'; +import { randomUUID } from 'node:crypto'; +import { existsSync, mkdirSync, readdirSync } from 'node:fs'; +import { link, readFile, rename, unlink, writeFile } from 'node:fs/promises'; import path from 'node:path'; import type { StorageAdapter } from '@noetic-tools/core'; @@ -22,6 +16,16 @@ function typedCast(value: unknown): T { return value; } +/** + * Narrow a caught value to a Node errno error. The async fs calls below + * replaced pre-flight `existsSync` checks — checking existence and then + * reading is a TOCTOU race once the read is awaited, so a missing file is + * detected from `ENOENT` on the operation itself. + */ +function isErrnoException(err: unknown): err is NodeJS.ErrnoException { + return typeof err === 'object' && err !== null && 'code' in err; +} + //#region Key <-> path mapping /** @@ -30,24 +34,75 @@ function typedCast(value: unknown): T { * `execution::frontier` round-trip to a single filename and back * without collisions. * + * Encoding is two-phase and unambiguous: first every literal underscore is + * escaped (`_` → `_u`), then URI-encoding escapes the rest, then `%` becomes + * `__`. The old single-phase scheme collapsed a key that legitimately + * contained `__` into a `%` on decode — `decodeKey('a__b')` threw and the + * key silently vanished from `list()` while `get()` still found it. + * * @internal */ const ENCODED_SEP = '__'; function encodeKey(key: string): string { - // URI-encode to escape non-filesystem-safe chars, then replace "%" with - // double-underscore so a decoded path still survives OS path delimiters. - return encodeURIComponent(key).replace(/%/g, ENCODED_SEP); + return encodeURIComponent(key.replaceAll('_', '_u')).replace(/%/g, ENCODED_SEP); } function decodeKey(encoded: string): string { - return decodeURIComponent(encoded.replaceAll(ENCODED_SEP, '%')); + return decodeURIComponent(encoded.replaceAll(ENCODED_SEP, '%')).replaceAll('_u', '_'); +} + +/** + * The pre-`_u`-escape encoder, preserved verbatim so files written by an + * earlier release stay readable. Any key containing `_` (or `%`) maps to a + * different filename under the current scheme, so a read miss retries here + * before reporting `null` — see `legacyFileFor`. + * + * @internal + */ +function legacyEncodeKey(key: string): string { + return encodeURIComponent(key).replace(/%/g, ENCODED_SEP); } function keyToPath(root: string, key: string): string { return path.join(root, `${encodeKey(key)}.json`); } +/** + * Resolve the legacy on-disk filename for `key`, or `null` when no distinct + * legacy read is warranted. + * + * Two cases are excluded deliberately: + * + * 1. The encodings agree (no `_`/`%` in the key) — the canonical read already + * covered that file, so a fallback would be a redundant `ENOENT`. + * 2. The legacy name is *also* the canonical name of some OTHER key. The + * legacy scheme is not injective against the new one: key `plain_nderscore` + * encodes canonically to `plain_underscore.json`, which is exactly the + * legacy filename for key `plain_underscore`. Falling back there would let + * `get('plain_underscore')` return a value that legitimately belongs to + * `plain_nderscore`, turning a missing read into a cross-key data leak — + * strictly worse than the `null` this fallback exists to avoid. + * + * @internal + */ +function legacyFileFor(root: string, key: string): string | null { + const legacy = legacyEncodeKey(key); + if (legacy === encodeKey(key)) { + return null; + } + let decoded: string | null = null; + try { + decoded = decodeKey(legacy); + } catch { + decoded = null; + } + if (decoded !== null && encodeKey(decoded) === legacy) { + return null; + } + return path.join(root, `${legacy}.json`); +} + function pathToKey(file: string): string | null { if (!file.endsWith('.json')) { return null; @@ -87,87 +142,202 @@ function ensureDir(dir: string): void { } } +function parseRaw(raw: string): T | null { + if (raw.length === 0) { + return null; + } + const parsed = JSON.parse(raw); + return typedCast(parsed); +} + +/** + * Read a file written under the pre-`_u`-escape encoding and fold it onto + * the canonical name so later reads take the fast path. + * + * Migration is best effort: this is a storage adapter, so a failed rename + * must not turn a successful read into a throw. On failure the legacy file + * stays put and the next read falls back again. + */ +async function readLegacy( + root: string, + keyIndex: Set, + key: string, + legacy: string, +): Promise { + let raw: string; + try { + raw = await readFile(legacy, 'utf8'); + } catch (err) { + if (isErrnoException(err) && err.code === 'ENOENT') { + return null; + } + console.warn(`createFileStorage: failed to read legacy file for "${key}":`, err); + return null; + } + let value: T | null; + try { + value = parseRaw(raw); + } catch (err) { + console.warn(`createFileStorage: failed to parse legacy file for "${key}":`, err); + return null; + } + // The legacy name may not decode to `key` (a key containing `__` did not + // survive the old decoder at all), so the construction-time scan can have + // missed it. Advertise it now that a read proved it exists. + keyIndex.add(key); + const canonical = keyToPath(root, key); + try { + // A hard link publishes the canonical name only when it is still absent. + // Unlike rename, it cannot overwrite a newer value written after the + // canonical read missed. + await link(legacy, canonical); + await unlink(legacy); + } catch (err) { + if (isErrnoException(err) && err.code === 'EEXIST') { + return readKey(root, keyIndex, key); + } + // Best effort; the value was read successfully and that is what matters. + } + return value; +} + +async function readKey(root: string, keyIndex: Set, key: string): Promise { + const file = keyToPath(root, key); + try { + const raw = await readFile(file, 'utf8'); + return parseRaw(raw); + } catch (err) { + if (!isErrnoException(err) || err.code !== 'ENOENT') { + console.warn(`createFileStorage: failed to read "${key}":`, err); + return null; + } + } + // Canonical name absent — the file may predate the `_u` escape pass. + const legacy = legacyFileFor(root, key); + if (legacy === null) { + return null; + } + return readLegacy(root, keyIndex, key, legacy); +} + +/** + * Drop a legacy-named file for `key`, best effort. Called on both `set` and + * `delete` so the legacy copy can never outlive the canonical one: without + * this, `delete(k)` then `set(k, v2)` then `delete(k)` would leave the + * legacy file behind for the fallback read to resurrect as a stale value. + */ +async function removeLegacy(root: string, key: string): Promise { + const legacy = legacyFileFor(root, key); + if (legacy === null) { + return; + } + try { + await unlink(legacy); + } catch (err) { + if (isErrnoException(err) && err.code === 'ENOENT') { + return; + } + console.warn(`createFileStorage: failed to remove legacy file for "${key}":`, err); + } +} + /** * @public * Create a file-backed `StorageAdapter` that writes each key to a JSON - * file under the configured root directory. Designed to be the default - * production-mode backing for checkpoint storage — the implementation is - * synchronous under the hood to minimise partial-write risk on crash, - * matching the expectation that checkpoint writes are small (kilobytes) - * and infrequent relative to step execution. + * file under the configured root directory. The default production-mode + * backing for checkpoint storage. + * + * Writes are async (`node:fs/promises`) via a .tmp sibling + atomic + * rename — core checkpoints after EVERY completed step, so a synchronous + * write here would block the event loop (token streaming, socket pumps, + * watchdog timers) once per step. The tmp+rename pattern keeps the + * "half-written file found on restart" window to the rename itself. + * + * `list(prefix)` is served from an in-memory key index seeded by one + * directory scan at construction and maintained on set/delete — the + * durable outbound queue and the step ledger call `list` on hot paths, + * and a per-call `readdir` over a flat root that also holds every ledger + * shard and IPC frame made each call O(total keys). * - * Not optimised for high-throughput workloads. If checkpoint volume - * becomes a bottleneck, swap for a database-backed adapter. + * The index assumes this adapter instance is the only writer to `root` + * for its lifetime (the same assumption the previous implementation made + * implicitly for read-modify-write sequences). Two live adapters over one + * root would see each other's writes via `get` but not via `list`. */ export function createFileStorage(options: CreateFileStorageOptions = {}): StorageAdapter { const root = options.root ?? defaultRoot(); ensureDir(root); - function readKey(key: string): T | null { - const file = keyToPath(root, key); - if (!existsSync(file)) { - return null; - } - try { - const raw = readFileSync(file, 'utf8'); - if (raw.length === 0) { - return null; - } - const parsed = JSON.parse(raw); - return typedCast(parsed); - } catch (err) { - console.warn(`createFileStorage: failed to read "${key}":`, err); - return null; + // Seed the key index from disk once; set/delete maintain it after that. + const keyIndex = new Set(); + for (const file of readdirSync(root)) { + const key = pathToKey(file); + if (key !== null) { + keyIndex.add(key); } } return { async get(key: string): Promise { - return readKey(key); + return readKey(root, keyIndex, key); }, async set(key: string, value: T): Promise { ensureDir(root); const file = keyToPath(root, key); - const tmp = `${file}.tmp`; - // Write via a .tmp sibling then rename — reduces the "half-written - // file found on restart" window to the rename itself. On crash mid- - // write the main file either still holds the previous value, or the - // rename completed. - writeFileSync(tmp, JSON.stringify(value)); - // `renameSync` is the atomic step on POSIX filesystems. - renameSync(tmp, file); + const tmp = `${file}.${randomUUID()}.tmp`; + // Write via a unique .tmp sibling then rename. Unique names keep + // concurrent writes to one key from racing over the same temp file. + try { + await writeFile(tmp, JSON.stringify(value)); + await rename(tmp, file); + } catch (err) { + await unlink(tmp).catch(() => undefined); + throw err; + } + keyIndex.add(key); + // Retire any legacy-named copy: the canonical file now holds the truth, + // and leaving the old one would give a later fallback read something + // stale to resurrect after a delete. + await removeLegacy(root, key); }, async delete(key: string): Promise { const file = keyToPath(root, key); - if (!existsSync(file)) { - return; - } + keyIndex.delete(key); + // Both names, or the fallback read would revive the legacy value. + await removeLegacy(root, key); try { - unlinkSync(file); + await unlink(file); } catch (err) { + if (isErrnoException(err) && err.code === 'ENOENT') { + return; + } console.warn(`createFileStorage: failed to delete "${key}":`, err); } }, async list(prefix: string): Promise { - if (!existsSync(root)) { - return []; - } - const files = readdirSync(root); const out: string[] = []; - for (const file of files) { - const key = pathToKey(file); - if (key?.startsWith(prefix)) { + for (const key of keyIndex) { + if (key.startsWith(prefix)) { out.push(key); } } - return out; + // Callers (step ledger, durable queue) depend on lexicographic order + // matching what a sorted directory listing produced. + return out.sort(); }, async getMany(keys: string[]): Promise> { - // Local disk has no per-key round trip to save, but implementing this keeps - // callers on one code path and skips the promise-per-key the fallback builds. + // Parallel reads — local disk has no per-key round trip, but the + // batch keeps callers on one code path and overlaps I/O waits. Each + // read resolves to its own key so the pairing survives the reorder + // a bare `Promise.all` over values would invite. + const entries = await Promise.all( + keys.map(async (key) => ({ + key, + value: await readKey(root, keyIndex, key), + })), + ); const found = new Map(); - for (const key of keys) { - const value = readKey(key); + for (const { key, value } of entries) { if (value === null) { continue; } diff --git a/packages/platform-node/test/file-storage.test.ts b/packages/platform-node/test/file-storage.test.ts index 870ae590..fbf3271d 100644 --- a/packages/platform-node/test/file-storage.test.ts +++ b/packages/platform-node/test/file-storage.test.ts @@ -5,7 +5,8 @@ */ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; -import { existsSync, mkdtempSync, rmSync } from 'node:fs'; +import assert from 'node:assert'; +import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { createFileStorage } from '../src/file-storage'; @@ -91,3 +92,262 @@ describe('createFileStorage', () => { expect(await storage.get('k')).toBe('v'); }); }); + +describe('key encoding round-trips (P6)', () => { + it('keys containing double underscores survive set → list → get', async () => { + const storage = createFileStorage({ + root, + }); + // The old scheme decoded '__' back into '%' and threw, making the key + // invisible to list() while get() still found it. + const nasty = [ + 'thread:__default__:itemLog:00000001', + 'a__b', + 'a_ub', + 'plain_underscore', + 'execution:abc:ledger:00000001', + ]; + for (const key of nasty) { + await storage.set(key, { + key, + }); + } + const listed = await storage.list(''); + for (const key of nasty) { + expect(listed).toContain(key); + expect( + await storage.get<{ + key: string; + }>(key), + ).toEqual({ + key, + }); + } + }); + + it('list is served from the index and stays correct across delete', async () => { + const storage = createFileStorage({ + root, + }); + await storage.set('p:1', 1); + await storage.set('p:2', 2); + await storage.delete('p:1'); + expect(await storage.list('p:')).toEqual([ + 'p:2', + ]); + }); + + it('serializes overlapping writes through unique temporary files', async () => { + const storage = createFileStorage({ + root, + }); + await Promise.all( + Array.from( + { + length: 20, + }, + (_, value) => storage.set('shared', value), + ), + ); + const stored = await storage.get('shared'); + expect(stored).toBeGreaterThanOrEqual(0); + expect(stored).toBeLessThan(20); + expect(readdirSync(root).filter((file) => file.endsWith('.tmp'))).toEqual([]); + }); + + it('a fresh adapter over an existing root seeds its index from disk', async () => { + const first = createFileStorage({ + root, + }); + await first.set('seeded:key__with__underscores', 42); + const second = createFileStorage({ + root, + }); + expect(await second.list('seeded:')).toEqual([ + 'seeded:key__with__underscores', + ]); + expect(await second.get('seeded:key__with__underscores')).toBe(42); + }); +}); + +describe('legacy on-disk key encoding (pre-_u-escape)', () => { + /** + * The encoder gained a `_` → `_u` pre-escape pass. That changed the + * filename for every key containing `_`, so files written by the previous + * release were still enumerable (`decodeKey` handles the old names) but + * unreadable — `get()` computed the new name and missed. These tests seed + * files under the OLD scheme directly and pin the read-side fallback. + */ + function legacyEncodeKey(key: string): string { + return encodeURIComponent(key).replace(/%/g, '__'); + } + + function seedLegacy(key: string, value: unknown): string { + const file = path.join(root, `${legacyEncodeKey(key)}.json`); + writeFileSync(file, JSON.stringify(value)); + return file; + } + + function currentEncodeKey(key: string): string { + return encodeURIComponent(key.replaceAll('_', '_u')).replace(/%/g, '__'); + } + + it('reads a key with a single underscore written by the old encoder', async () => { + // The scope-storage shape from the finding: layers///state. + const key = 'layers/my_layer/res_1/state'; + const legacyFile = seedLegacy(key, { + facts: [ + 'remembered', + ], + }); + // Pin the exact legacy filename so a future encoder change cannot make + // this test vacuous by seeding a name nothing ever wrote. + expect(path.basename(legacyFile)).toBe('layers__2Fmy_layer__2Fres_1__2Fstate.json'); + + const storage = createFileStorage({ + root, + }); + expect( + await storage.get<{ + facts: string[]; + }>(key), + ).toEqual({ + facts: [ + 'remembered', + ], + }); + }); + + it('reads a key containing __ written by the old encoder', async () => { + // The original data-loss case: the old decoder threw on this name, so the + // construction-time index scan skips it and only the read can surface it. + const key = 'thread:__default__:itemLog:00000001'; + seedLegacy(key, { + n: 7, + }); + const storage = createFileStorage({ + root, + }); + expect( + await storage.get<{ + n: number; + }>(key), + ).toEqual({ + n: 7, + }); + // A read proved the key exists, so list() must advertise it too. + expect(await storage.list('thread:')).toContain(key); + }); + + it('migrates the legacy file onto the canonical name so the second read is a fast path', async () => { + const key = 'layers/user_facts/user_123/state'; + seedLegacy(key, 'v1'); + const storage = createFileStorage({ + root, + }); + expect(await storage.get(key)).toBe('v1'); + + // After migration exactly one file remains, under the new encoding. + const files = readdirSync(root); + expect(files).toEqual([ + `${currentEncodeKey(key)}.json`, + ]); + expect(files).not.toContain(`${legacyEncodeKey(key)}.json`); + + // Second read resolves from the canonical name. + expect(await storage.get(key)).toBe('v1'); + }); + + it('set over a legacy key retires the legacy file and round-trips the new value', async () => { + const key = 'layers/my_layer/res_1/state'; + seedLegacy(key, 'stale'); + const storage = createFileStorage({ + root, + }); + await storage.set(key, 'fresh'); + expect(readdirSync(root)).toEqual([ + `${currentEncodeKey(key)}.json`, + ]); + expect(await storage.get(key)).toBe('fresh'); + }); + + it('delete removes the legacy file so the fallback cannot resurrect it', async () => { + const key = 'layers/my_layer/res_1/state'; + seedLegacy(key, 'stale'); + const storage = createFileStorage({ + root, + }); + await storage.delete(key); + expect(await storage.get(key)).toBeNull(); + expect(readdirSync(root)).toEqual([]); + expect(await storage.list('layers/')).toEqual([]); + }); + + it('delete → set → delete leaves no legacy copy behind', async () => { + const key = 'layers/my_layer/res_1/state'; + seedLegacy(key, 'stale'); + const storage = createFileStorage({ + root, + }); + await storage.delete(key); + await storage.set(key, 'recreated'); + expect(await storage.get(key)).toBe('recreated'); + await storage.delete(key); + // The stale legacy value must not come back through the fallback read. + expect(await storage.get(key)).toBeNull(); + expect(readdirSync(root)).toEqual([]); + }); + + it('getMany resolves legacy-encoded keys alongside canonical ones', async () => { + const legacyKey = 'layers/my_layer/res_1/state'; + seedLegacy(legacyKey, 'old'); + const storage = createFileStorage({ + root, + }); + await storage.set('layers/plain/res/state', 'new'); + // `getMany` is optional on the StorageAdapter contract; this adapter + // implements it, and the batch path must inherit the legacy fallback. + assert(storage.getMany); + const found = await storage.getMany([ + legacyKey, + 'layers/plain/res/state', + ]); + expect(found.get(legacyKey)).toBe('old'); + expect(found.get('layers/plain/res/state')).toBe('new'); + }); + + it('does not serve one key from another key’s canonical file', async () => { + // The legacy scheme is not injective against the new one: key + // 'plain_nderscore' encodes canonically to 'plain_underscore.json', which + // is also the LEGACY name for key 'plain_underscore'. A naive fallback + // would leak across keys — worse than the null it set out to avoid. + const storage = createFileStorage({ + root, + }); + await storage.set('plain_nderscore', 'belongs-to-nderscore'); + expect(await storage.get('plain_underscore')).toBeNull(); + expect(await storage.get('plain_nderscore')).toBe('belongs-to-nderscore'); + }); + + it('list stays consistent across a legacy read, set, and delete', async () => { + const key = 'layers/my_layer/res_1/state'; + seedLegacy(key, 'old'); + const storage = createFileStorage({ + root, + }); + // Seeded from the construction-time scan (the legacy name decodes). + expect(await storage.list('layers/')).toEqual([ + key, + ]); + expect(await storage.get(key)).toBe('old'); + expect(await storage.list('layers/')).toEqual([ + key, + ]); + await storage.set(key, 'new'); + expect(await storage.list('layers/')).toEqual([ + key, + ]); + await storage.delete(key); + expect(await storage.list('layers/')).toEqual([]); + }); +});