From 11d663230527694796b790894ae29d59ea28be54 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:27:02 -0500 Subject: [PATCH 01/14] feat(store): add a test seam to the store factory getStore() memoizes for the life of the process, which is correct for the server and leaves tests no way to install a fault-injecting store or a second driver. setStore/resetStore open that door for tests only. The seam is production code, so the guard is that production never reaches it. A grep for callers would be an absence claim proved by grep; the test walks the real import graph from src/index.ts and src/mcp/server.ts instead, and carries a positive control that the walk reached the modules it claims to cover. Verified load-bearing: planting a reference in src/handler.ts turns that named control red and nothing else. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/store/index.ts | 15 ++++++++ tests/store-seam.test.ts | 82 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 tests/store-seam.test.ts diff --git a/src/store/index.ts b/src/store/index.ts index 2d2cd3c..cacc126 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -15,5 +15,20 @@ export function getStore(): BlobStore { return cached } +/** + * Install a store for the rest of the process. Tests only: the cache above is + * process-lifetime by design, so a fault-injecting store or a second driver has + * no other way in. Nothing the server imports may call this, and + * tests/store-seam.test.ts walks the production import graph to prove it. + */ +export function setStore(store: BlobStore): void { + cached = store +} + +/** Drop any installed or memoized store so the next getStore() rebuilds from config. */ +export function resetStore(): void { + cached = null +} + export type { BlobStore } from './blobstore.ts' export { entryPath, manifestPath } from './blobstore.ts' diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts new file mode 100644 index 0000000..c25f2ce --- /dev/null +++ b/tests/store-seam.test.ts @@ -0,0 +1,82 @@ +/** + * The store factory's test seam (U1). + * + * `getStore()` memoizes for the life of the process, which is right for the + * server and impossible for tests: a fault-injecting store (U2's sweep) and a + * second driver in one process both need to replace the cached instance and put + * the real one back. The seam exists for that and nothing else, so the last test + * here walks the production import graph and fails if anything under src/ that + * the server actually loads reaches for it. + */ + +import { describe, expect, test } from 'bun:test' +import { readFile } from 'node:fs/promises' +import { dirname, join, resolve } from 'node:path' +import type { BlobStore } from '../src/store/blobstore.ts' +import { getStore, resetStore, setStore } from '../src/store/index.ts' + +const SENTINEL = new TextEncoder().encode('sentinel') + +function stub(): BlobStore { + return { + get: async () => SENTINEL, + put: async () => {}, + delete: async () => {}, + describe: () => 'stub', + } +} + +describe('store factory seam', () => { + test('setStore installs an instance getStore then returns', async () => { + setStore(stub()) + expect(getStore().describe()).toBe('stub') + expect(await getStore().get('anything')).toEqual(SENTINEL) + resetStore() + }) + + test('resetStore restores the real driver', () => { + setStore(stub()) + resetStore() + expect(getStore().describe()).toStartWith('fs:') + }) + + test('without an override, getStore memoizes one instance', () => { + resetStore() + expect(getStore()).toBe(getStore()) + }) + + // The seam is production code, so the guard is that production never reaches + // it. A grep for callers would be an absence claim proved by grep, which is + // the shape docs/solutions/conventions/verify-completeness-by-proof-not- + // assertion.md rejects; this walks the real import graph instead. + test('the seam is unreachable from the production import graph', async () => { + const root = resolve(import.meta.dir, '..') + const seen = new Set() + const offenders: string[] = [] + + async function walk(file: string): Promise { + if (seen.has(file)) return + seen.add(file) + let src: string + try { + src = await readFile(file, 'utf8') + } catch { + return + } + if (file !== join(root, 'src/store/index.ts')) { + if (/\b(setStore|resetStore)\b/.test(src)) offenders.push(file.slice(root.length + 1)) + } + for (const m of src.matchAll(/from\s+'(\.[^']+)'/g)) { + await walk(resolve(dirname(file), m[1] as string)) + } + } + + await walk(join(root, 'src/index.ts')) + await walk(join(root, 'src/mcp/server.ts')) + + // Positive control: the walk actually reached the modules it claims to cover. + expect(seen.size).toBeGreaterThan(8) + expect([...seen].some(f => f.endsWith('src/store/index.ts'))).toBe(true) + expect(offenders).toEqual([]) + }) +}) From b53887e5fc7c08071a648aac6a6c92607f89a5d0 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:30:46 -0500 Subject: [PATCH 02/14] fix(memory): make a crash mid-commit leave a consistent namespace Entry blobs now live at a path named by their own ciphertext hash, so an overwrite never mutates a blob the visible manifest still points at. Commit order is blobs, then the manifest that publishes them, then reclaim what the new manifest no longer references. A crash before the manifest write leaves orphans no reader can see; a crash after it leaves stale extras no reader can see. Reads fall back to the old key-derived path, so entries written before this need no migration. Two consequences the sweep forced out. A corrupt manifest used to start clean on the reasoning that the blobs survived and a re-push would rebuild it; reclaiming blobs makes that destructive, so an unreadable manifest now refuses the write. And deterministic ciphertext means two entries can share one blob, so reclaim checks the live hash set before removing anything. The sweep injects a fault at every mutating call, evidences the plant landed at that index, and asserts the visible state is either the previous one or the new one and never torn. Verified red on the pre-change code at three tests, green after. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/memory.ts | 45 +++++-- src/namespace.ts | 5 +- src/store/blobstore.ts | 14 +++ src/store/index.ts | 2 +- tests/atomic-visibility.test.ts | 206 ++++++++++++++++++++++++++++++++ 5 files changed, 259 insertions(+), 13 deletions(-) create mode 100644 tests/atomic-visibility.test.ts diff --git a/src/memory.ts b/src/memory.ts index 9fe7588..4370348 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -19,7 +19,7 @@ import { namespaceChecksum, sha256Hex, sha256Prefixed } from './hash.ts' import { withLock } from './lock.ts' import { validateEntryKey } from './namespace.ts' import { QuotaError, reserveAndCommit } from './quota.ts' -import { entryPath, getStore, manifestPath } from './store/index.ts' +import { contentPath, entryPath, getStore, manifestPath } from './store/index.ts' import { emptyManifest, type Manifest, @@ -36,9 +36,12 @@ async function readManifest(nsSlug: string): Promise { try { return JSON.parse(new TextDecoder().decode(raw)) as Manifest } catch { - // A corrupt manifest shouldn't brick the namespace; start clean. Entry - // blobs are still on disk and a re-push will rebuild the manifest. - return emptyManifest() + // Absent and unreadable are different. Absent is a new namespace; unreadable + // is a namespace whose index we cannot see, and starting clean there would + // now be destructive: the commit path reclaims blobs the new manifest does + // not reference, so an empty manifest would delete every live entry. Refuse + // the write and let an operator look. + throw new Error(`unreadable manifest for namespace slug ${nsSlug}`) } } @@ -76,7 +79,9 @@ export async function getData(namespace: string, nsSlug: string): Promise { - const bytes = await store.get(entryPath(nsSlug, sha256Hex(key))) + const bytes = + (await store.get(contentPath(nsSlug, meta.hash))) ?? + (await store.get(entryPath(nsSlug, sha256Hex(key)))) if (!bytes) return // manifest/blob drift — skip rather than 500 entries[key] = Buffer.from(bytes).toString('base64') entryChecksums[key] = meta.hash @@ -115,12 +120,16 @@ export async function upsert( return withLock(`ns:${nsSlug}`, async () => { const store = getStore() const m = await readManifest(nsSlug) + // The projection mutates `m.entries` in place, so capture what the currently + // visible manifest points at before touching it; cleanup needs the old hashes. + const prev: Record = {} + for (const [k, meta] of Object.entries(m.entries)) prev[k] = meta.hash + const touched = new Set() const accepted: string[] = [] const deleted: string[] = [] const skipped: { key: string; reason: string }[] = [] // Defer all mutations so we can reject the whole request on a cap breach. const blobWrites: { path: string; bytes: Uint8Array }[] = [] - const blobDeletes: string[] = [] // Deletions first (projected; store.delete deferred to commit). for (const key of req.deletions ?? []) { @@ -131,7 +140,7 @@ export async function upsert( continue } if (m.entries[key]) { - blobDeletes.push(entryPath(nsSlug, sha256Hex(key))) + touched.add(key) delete m.entries[key] deleted.push(key) } @@ -167,12 +176,13 @@ export async function upsert( accepted.push(key) continue } - blobWrites.push({ path: entryPath(nsSlug, sha256Hex(key)), bytes }) + blobWrites.push({ path: contentPath(nsSlug, hash), bytes }) + touched.add(key) m.entries[key] = { hash, size: bytes.byteLength, updatedAt: nowIso } accepted.push(key) } - const mutated = blobWrites.length > 0 || blobDeletes.length > 0 + const mutated = blobWrites.length > 0 || touched.size > 0 if (mutated) { // Project the namespace's final footprint and enforce its byte cap. let nsBytes = 0 @@ -184,11 +194,26 @@ export async function upsert( } const commit = async () => { + // Blobs first, then the manifest that publishes them, then reclaim what + // the new manifest no longer references. A crash before the manifest + // write leaves orphans no reader can see; a crash after it leaves stale + // extras no reader can see. Either way the visible state is consistent. for (const w of blobWrites) await store.put(w.path, w.bytes) - for (const d of blobDeletes) await store.delete(d) m.version += 1 m.lastModified = nowIso await writeManifest(nsSlug, m) + + const live = new Set(Object.values(m.entries).map(meta => meta.hash)) + for (const key of touched) { + const old = prev[key] + // Deterministic ciphertext means another entry may hold this exact + // blob; only reclaim it when no live entry still names that hash. + if (old && !live.has(old)) await store.delete(contentPath(nsSlug, old)) + // Entries written before content addressing live at a key-derived + // path the new layout never names, so nothing else would ever remove + // them and a delete would silently leave the ciphertext behind. + await store.delete(entryPath(nsSlug, sha256Hex(key))) + } } const projected = { entries: Object.keys(m.entries).length, bytes: nsBytes } diff --git a/src/namespace.ts b/src/namespace.ts index 054ac5d..c3a28b7 100644 --- a/src/namespace.ts +++ b/src/namespace.ts @@ -59,8 +59,9 @@ export function validateEntryKey(key: string): string { * The slug is the ONLY thing separating one tenant's `ns//...` storage * (and its `ns:` write lock) from another's, so it must be injective: two * distinct namespaces must never share a slug. We hash the namespace with - * sha256 — the same way entry keys become flat blob filenames - * (`entryPath(nsSlug, sha256Hex(key))`). sha256 is collision-resistant (finding + * sha256 — the same way entry ciphertext becomes a flat blob filename + * (`contentPath(nsSlug, ciphertextHash)`; `entryPath` remains for entries + * written before content addressing). sha256 is collision-resistant (finding * two distinct namespaces with the same slug is computationally infeasible, not * impossible by construction), the output is all-lowercase-hex (so * case-insensitive filesystems can't re-collide it), and it needs no per-grammar diff --git a/src/store/blobstore.ts b/src/store/blobstore.ts index 67d443a..adb53f8 100644 --- a/src/store/blobstore.ts +++ b/src/store/blobstore.ts @@ -37,3 +37,17 @@ export function manifestPath(nsSlug: string): string { export function entryPath(nsSlug: string, entryKeyHash: string): string { return `ns/${nsSlug}/entries/${entryKeyHash}` } + +/** + * Build the storage path for one entry's ciphertext, named by that ciphertext's + * own hash. Writing to a content-addressed path means an overwrite never mutates + * a blob the currently-visible manifest still points at, which is what makes a + * crash mid-commit leave the previous state readable and self-consistent. + * + * Encryption is deterministic, so two entries holding identical ciphertext share + * one path. Cleanup must therefore check the live hash set before removing a + * blob, never assume one entry owns it. + */ +export function contentPath(nsSlug: string, ciphertextHash: string): string { + return `ns/${nsSlug}/blobs/${ciphertextHash.replace(/^sha256:/, '')}` +} diff --git a/src/store/index.ts b/src/store/index.ts index cacc126..17a4689 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -31,4 +31,4 @@ export function resetStore(): void { } export type { BlobStore } from './blobstore.ts' -export { entryPath, manifestPath } from './blobstore.ts' +export { contentPath, entryPath, manifestPath } from './blobstore.ts' diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts new file mode 100644 index 0000000..8f1df20 --- /dev/null +++ b/tests/atomic-visibility.test.ts @@ -0,0 +1,206 @@ +/** + * Crash visibility across the commit sequence (U2, R9, AE4). + * + * The property: a reader never sees a manifest naming a blob that is absent, + * nor a blob whose bytes disagree with the hash the visible manifest records + * for it. Both directions matter. Today's write path overwrites an entry blob + * in place at a key-derived path, so a fault after the first blob write leaves + * the old manifest pointing at new bytes, which is the second direction. + * + * A single injection offset proves nothing here, so this sweeps every mutating + * call in the commit and evidences the plant at each index before reading. + */ + +import { afterEach, describe, expect, test } from 'bun:test' +import { sha256Hex, sha256Prefixed } from '../src/hash.ts' +import { getData, upsert } from '../src/memory.ts' +import { namespaceSlug } from '../src/namespace.ts' +import type { BlobStore } from '../src/store/blobstore.ts' +import { getStore, resetStore, setStore } from '../src/store/index.ts' + +const NOW = '2026-06-24T00:00:00.000Z' +const b64 = (s: string) => Buffer.from(s).toString('base64') + +class Boom extends Error {} + +/** Wraps the real store and throws on the nth mutating call, counting attempts. */ +function faulty(inner: BlobStore, failAt: number) { + let calls = 0 + const guard = () => { + if (calls++ === failAt) throw new Boom(`injected at ${failAt}`) + } + return { + calls: () => calls, + store: { + get: (p: string) => inner.get(p), + put: async (p: string, b: Uint8Array) => { + guard() + return inner.put(p, b) + }, + delete: async (p: string) => { + guard() + return inner.delete(p) + }, + describe: () => `faulty(${inner.describe()})`, + } as BlobStore, + } +} + +/** Write one entry the way the pre-content-addressing code did: blob at a + * key-derived path, manifest naming it. This is the only way to get genuine + * legacy layout now that upsert writes content-addressed. */ +async function plantLegacy(store: BlobStore, slug: string, key: string, body: string) { + const bytes = new Uint8Array(Buffer.from(body)) + const path = `ns/${slug}/entries/${sha256Hex(key)}` + await store.put(path, bytes) + const m = { + version: 1, + lastModified: NOW, + entries: { [key]: { hash: sha256Prefixed(bytes), size: bytes.byteLength, updatedAt: NOW } }, + } + await store.put(`ns/${slug}/manifest.json`, new TextEncoder().encode(JSON.stringify(m))) + return path +} + +async function seed(ns: string, entries: Record) { + resetStore() + await upsert(ns, namespaceSlug(ns), 'local', { entries }, NOW) +} + +/** Every visible entry's bytes must match the hash the visible manifest records. */ +async function assertConsistent(ns: string) { + const data = await getData(ns, namespaceSlug(ns)) + for (const [key, b] of Object.entries(data.content.entries)) { + const bytes = new Uint8Array(Buffer.from(b, 'base64')) + expect(sha256Prefixed(bytes)).toBe(data.content.entryChecksums[key] as string) + } + // No manifest entry may lack a readable blob. + expect(Object.keys(data.content.entries).sort()).toEqual( + Object.keys(data.content.entryChecksums).sort(), + ) + return data +} + +afterEach(() => resetStore()) + +describe('crash visibility across the commit sequence', () => { + test('sweep: a fault at any mutating call leaves the previous complete state', async () => { + const ns = 'user:sweep' + await seed(ns, { 'a.md': b64('a1'), 'b.md': b64('b1'), 'c.md': b64('c1') }) + const before = await assertConsistent(ns) + + const real = getStore() + let injected = 0 + let untorn = 0 + let published = 0 + for (let i = 0; i < 8; i++) { + resetStore() + const f = faulty(real, i) + setStore(f.store) + const err = await upsert( + ns, + namespaceSlug(ns), + 'local', + { + entries: { 'a.md': b64('a2'), 'b.md': b64('b2'), 'd.md': b64('d1') }, + deletions: ['c.md'], + }, + NOW, + ).catch(e => e) + resetStore() + if (!(err instanceof Boom)) continue // past the end of the mutating sequence + injected++ + // Evidence the plant landed at this index rather than short-circuiting. + expect(f.calls()).toBe(i + 1) + // Either the commit had not yet published (previous state) or it had + // (new state). Never a torn one: that is what R9 asserts. A fault during + // the post-manifest reclaim legitimately leaves the new state visible. + const after = await assertConsistent(ns) + const keys = Object.keys(after.content.entries).sort().join(',') + expect(['a.md,b.md,c.md', 'a.md,b.md,d.md']).toContain(keys) + if (keys === 'a.md,b.md,c.md') { + expect(after.content.entries).toEqual(before.content.entries) + untorn++ + } else { + published++ + } + } + // Control that the sweep spanned the manifest write rather than only one side. + expect(untorn).toBeGreaterThan(0) + expect(published).toBeGreaterThan(0) + // Positive control: the sweep actually injected faults, at more than one index. + expect(injected).toBeGreaterThan(2) + }) + + test('harness control: a fault at index 0 leaves the namespace untouched', async () => { + const ns = 'user:ctl' + await seed(ns, { 'a.md': b64('a1') }) + const real = getStore() + const f = faulty(real, 0) + setStore(f.store) + const err = await upsert( + ns, + namespaceSlug(ns), + 'local', + { entries: { 'a.md': b64('a2') } }, + NOW, + ).catch(e => e) + resetStore() + expect(err).toBeInstanceOf(Boom) + expect(f.calls()).toBe(1) + const data = await getData(ns, namespaceSlug(ns)) + expect(Buffer.from(data.content.entries['a.md'] as string, 'base64').toString()).toBe('a1') + }) + + test('an unreadable manifest refuses the write and leaves every blob in place', async () => { + const ns = 'user:corrupt' + await seed(ns, { 'a.md': b64('a1') }) + const slug = namespaceSlug(ns) + const store = getStore() + const blobBefore = await store.get(`ns/${slug}/entries/${sha256Hex('a.md')}`) + await store.put(`ns/${slug}/manifest.json`, new TextEncoder().encode('{not json')) + + const err = await upsert(ns, slug, 'local', { entries: { 'b.md': b64('b1') } }, NOW).catch( + e => e, + ) + expect(err).toBeInstanceOf(Error) + expect(String((err as Error).message)).toContain('manifest') + // Control: the pre-existing blob is still there, so nothing was reclaimed. + expect(await store.get(`ns/${slug}/entries/${sha256Hex('a.md')}`)).toEqual( + blobBefore as Uint8Array, + ) + }) + + test('a legacy-layout blob is read, and after one rewrite nothing is left at the legacy path', async () => { + const ns = 'user:legacy' + const slug = namespaceSlug(ns) + resetStore() + const store = getStore() + const legacy = await plantLegacy(store, slug, 'a.md', 'a1') + // Control: the legacy object exists before the rewrite. + expect(await store.get(legacy)).not.toBeNull() + + const read = await getData(ns, slug) + expect(Buffer.from(read.content.entries['a.md'] as string, 'base64').toString()).toBe('a1') + + await upsert(ns, slug, 'local', { entries: { 'a.md': b64('a2') } }, NOW) + expect(await store.get(legacy)).toBeNull() + const after = await getData(ns, slug) + expect(Buffer.from(after.content.entries['a.md'] as string, 'base64').toString()).toBe('a2') + }) + + test('deleting a legacy-layout entry leaves nothing at either path', async () => { + const ns = 'user:legacydel' + const slug = namespaceSlug(ns) + resetStore() + const store = getStore() + const legacy = await plantLegacy(store, slug, 'a.md', 'a1') + await upsert(ns, slug, 'local', { entries: { 'keep.md': b64('k1') } }, NOW) + expect(await store.get(legacy)).not.toBeNull() + + await upsert(ns, slug, 'local', { entries: {}, deletions: ['a.md'] }, NOW) + expect(await store.get(legacy)).toBeNull() + const after = await assertConsistent(ns) + expect(Object.keys(after.content.entries)).toEqual(['keep.md']) + }) +}) From 1dadcd66a1b61bcf862c4f14f3ae5661af6d9a3a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:33:00 -0500 Subject: [PATCH 03/14] feat(memory): refuse a write whose base is out of date A push may now carry, per entry key, the ciphertext hash it believes that key holds (or null for "should not exist"). The comparison runs inside the namespace lock against the manifest the write would actually mutate, before any projection, and collects every disagreeing key so one round trip tells the caller everything that moved under it. Disagreement is a 409 carrying those conflicts. The window this guards is the caller's own turn, which is why the base is per entry rather than a namespace version: a single-key write should not be refused because an unrelated key changed. A request with no base is accepted unconditionally, so existing clients are untouched, and the hashes view advertises the capability: without that a client cannot tell a server that enforces this from one that ignores an unknown field, and would report a guarantee it is not getting. DELETE reached upsert outside the catch that maps typed errors, so a conflict there would have surfaced as a 500. It now takes a base on the query, shape- checked separately since the body parser never sees a DELETE. Verified: removing the comparison turns exactly the five refusal tests red, and every pre-existing test passes unchanged. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 38 ++++++-- src/memory.ts | 18 ++++ src/types.ts | 42 ++++++++- tests/base-precondition.test.ts | 155 ++++++++++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 10 deletions(-) create mode 100644 tests/base-precondition.test.ts diff --git a/src/handler.ts b/src/handler.ts index 3de3b71..c007af6 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -26,7 +26,7 @@ import { import { QuotaError } from './quota.ts' import { take } from './ratelimit.ts' import { getStore } from './store/index.ts' -import { parseUpsertRequest } from './types.ts' +import { parseUpsertRequest, StaleBaseError } from './types.ts' // Applied to every response. The API serves only JSON and is consumed by // programmatic clients, so we lock down sniffing/caching/referrer leakage. @@ -138,6 +138,9 @@ export async function handleRequest(req: Request): Promise { if (err instanceof QuotaError) { return apiError(err.code, err.message, 413, err.details) } + if (err instanceof StaleBaseError) { + return apiError(err.code, err.message, 409, err.details) + } throw err } } @@ -150,14 +153,31 @@ export async function handleRequest(req: Request): Promise { } catch (err) { return apiError('invalid_key', (err as Error).message, 400) } - const result = await upsert( - namespace, - nsSlug, - identity.owner, - { entries: {}, deletions: [key] }, - new Date().toISOString(), - ) - return json(result) + // The base rides the query here rather than a body, so it needs its own + // shape check: parseUpsertRequest never sees a DELETE. + const rawBase = url.searchParams.get('base') + if (rawBase !== null && !/^sha256:[0-9a-f]{64}$/.test(rawBase)) { + return apiError('bad_request', '`base` must be a sha256: hash', 400) + } + const base = rawBase === null ? undefined : { [key]: rawBase } + try { + const result = await upsert( + namespace, + nsSlug, + identity.owner, + { entries: {}, deletions: [key], base }, + new Date().toISOString(), + ) + return json(result) + } catch (err) { + if (err instanceof QuotaError) { + return apiError(err.code, err.message, 413, err.details) + } + if (err instanceof StaleBaseError) { + return apiError(err.code, err.message, 409, err.details) + } + throw err + } } return apiError('method_not_allowed', `${req.method} not supported`, 405) diff --git a/src/memory.ts b/src/memory.ts index 4370348..508d74f 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -25,10 +25,14 @@ import { type Manifest, type MemoryData, type MemoryHashes, + StaleBaseError, type UpsertRequest, type UpsertResponse, } from './types.ts' +/** Capabilities the hashes view advertises. See StaleBaseError and R12. */ +const SUPPORTS = ['base-precondition'] + // ─── Manifest helpers ─────────────────────────────────────────────────── async function readManifest(nsSlug: string): Promise { const raw = await getStore().get(manifestPath(nsSlug)) @@ -67,6 +71,7 @@ export async function getHashes(namespace: string, nsSlug: string): Promise = {} for (const [k, meta] of Object.entries(m.entries)) prev[k] = meta.hash const touched = new Set() + + // Compare the caller's base before any projection, against the manifest this + // write would actually mutate. Every disagreeing key is collected so one + // round trip tells the caller everything that moved under it. + if (req.base) { + const conflicts: Record = {} + for (const [key, expected] of Object.entries(req.base)) { + const actual = m.entries[key]?.hash ?? null + if (actual !== expected) conflicts[key] = actual + } + if (Object.keys(conflicts).length > 0) throw new StaleBaseError(conflicts) + } + const accepted: string[] = [] const deleted: string[] = [] const skipped: { key: string; reason: string }[] = [] diff --git a/src/types.ts b/src/types.ts index 4ac3c91..6b0a73d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -51,6 +51,12 @@ export type MemoryHashes = { lastModified: string checksum: string entryChecksums: Record + /** + * Server capabilities a client can rely on. Without this a client cannot tell + * a server that enforces the write precondition from one that ignores an + * unknown body field, so it would report a guarantee it is not getting. + */ + supports: string[] } /** PUT request body. */ @@ -59,6 +65,28 @@ export type UpsertRequest = { entries: Record /** entryKeys to remove (optional). */ deletions?: string[] + /** + * entryKey -> the ciphertext hash the caller believes that key holds, or null + * for "I believe this key does not exist". Optional: a request without it is + * accepted unconditionally so existing clients keep working. + */ + base?: Record +} + +/** + * Thrown when a write's base disagrees with the manifest it would mutate. + * Surfaces as HTTP 409; `details.conflicts` maps each disagreeing key to the + * hash the manifest actually holds, or null when the key is absent, so a caller + * can tell what changed under it without a second round trip. + */ +export class StaleBaseError extends Error { + readonly code = 'stale_base_version' + readonly details: { conflicts: Record } + constructor(conflicts: Record) { + super('write base is out of date') + this.name = 'StaleBaseError' + this.details = { conflicts } + } } export type UpsertResponse = { @@ -92,6 +120,18 @@ export function parseUpsertRequest(raw: unknown): UpsertRequest { for (const [k, v] of Object.entries(entries)) { if (typeof v !== 'string') throw new Error(`entry ${JSON.stringify(k)} must be a base64 string`) } + let base: Record | undefined + if (obj.base !== undefined) { + if (typeof obj.base !== 'object' || obj.base === null || Array.isArray(obj.base)) { + throw new Error('`base` must be an object map of key -> hash or null') + } + for (const [k, v] of Object.entries(obj.base)) { + if (v !== null && typeof v !== 'string') { + throw new Error(`base ${JSON.stringify(k)} must be a hash string or null`) + } + } + base = obj.base as Record + } let deletions: string[] | undefined if (obj.deletions !== undefined) { if (!Array.isArray(obj.deletions) || obj.deletions.some(d => typeof d !== 'string')) { @@ -99,5 +139,5 @@ export function parseUpsertRequest(raw: unknown): UpsertRequest { } deletions = obj.deletions as string[] } - return { entries: entries as Record, deletions } + return { entries: entries as Record, deletions, base } } diff --git a/tests/base-precondition.test.ts b/tests/base-precondition.test.ts new file mode 100644 index 0000000..90afc39 --- /dev/null +++ b/tests/base-precondition.test.ts @@ -0,0 +1,155 @@ +/** + * The per-entry base precondition (U3, R12, AE7 server half). + * + * A push carries the ciphertext hash it believes each key currently holds. If + * the manifest disagrees, the write is refused with 409 rather than applied. + * The guarded window is the caller's own turn, not the moment between its last + * read and its PUT, so the base is per entry and the comparison happens inside + * the namespace lock against the manifest the write would actually mutate. + * + * A request with no base is accepted unconditionally: existing clients keep + * working, and the hashes view advertises the capability so a client can tell + * a server that enforces it from one that does not. + */ + +import { afterEach, describe, expect, test } from 'bun:test' +import { handleRequest } from '../src/handler.ts' +import { getData, getHashes, upsert } from '../src/memory.ts' +import { namespaceSlug } from '../src/namespace.ts' +import { resetStore } from '../src/store/index.ts' +import { StaleBaseError } from '../src/types.ts' + +const NOW = '2026-06-24T00:00:00.000Z' +const b64 = (s: string) => Buffer.from(s).toString('base64') +const put = (ns: string, req: Parameters[3]) => + upsert(ns, namespaceSlug(ns), 'local', req, NOW) + +afterEach(() => resetStore()) + +async function hashOf(ns: string, key: string) { + const h = await getHashes(ns, namespaceSlug(ns)) + return h.entryChecksums[key] as string +} + +describe('base precondition', () => { + test('a stale base is refused and the namespace is unchanged', async () => { + const ns = 'user:stale' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const err = await put(ns, { + entries: { 'a.md': b64('v2') }, + base: { 'a.md': 'sha256:0000' }, + }).catch(e => e) + + expect(err).toBeInstanceOf(StaleBaseError) + expect((err as StaleBaseError).code).toBe('stale_base_version') + expect((err as StaleBaseError).details.conflicts).toEqual({ + 'a.md': await hashOf(ns, 'a.md'), + }) + const data = await getData(ns, namespaceSlug(ns)) + expect(Buffer.from(data.content.entries['a.md'] as string, 'base64').toString()).toBe('v1') + }) + + test('the same write with the current base succeeds', async () => { + const ns = 'user:fresh' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const base = { 'a.md': await hashOf(ns, 'a.md') } + const r = await put(ns, { entries: { 'a.md': b64('v2') }, base }) + expect(r.accepted).toEqual(['a.md']) + const data = await getData(ns, namespaceSlug(ns)) + expect(Buffer.from(data.content.entries['a.md'] as string, 'base64').toString()).toBe('v2') + }) + + test('expected-absent: null base on an existing key is refused, on a new key succeeds', async () => { + const ns = 'user:absent' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const err = await put(ns, { + entries: { 'a.md': b64('v2') }, + base: { 'a.md': null }, + }).catch(e => e) + expect(err).toBeInstanceOf(StaleBaseError) + + const ok = await put(ns, { entries: { 'b.md': b64('v1') }, base: { 'b.md': null } }) + expect(ok.accepted).toEqual(['b.md']) + }) + + test('a request with no base is accepted unconditionally', async () => { + const ns = 'user:nobase' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const r = await put(ns, { entries: { 'a.md': b64('v2') } }) + expect(r.accepted).toEqual(['a.md']) + }) + + test('a deletion with a stale base is refused; with the right base it deletes', async () => { + const ns = 'user:del' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const err = await put(ns, { + entries: {}, + deletions: ['a.md'], + base: { 'a.md': 'sha256:0000' }, + }).catch(e => e) + expect(err).toBeInstanceOf(StaleBaseError) + + const r = await put(ns, { + entries: {}, + deletions: ['a.md'], + base: { 'a.md': await hashOf(ns, 'a.md') }, + }) + expect(r.deleted).toEqual(['a.md']) + }) + + test('the hashes view advertises the precondition capability', async () => { + const ns = 'user:cap' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const h = await getHashes(ns, namespaceSlug(ns)) + expect(h.supports).toContain('base-precondition') + }) +}) + +describe('base precondition over the wire', () => { + const url = (ns: string) => `http://x/api/memory/${encodeURIComponent(ns)}` + + test('a stale PUT returns 409 with the conflicting key', async () => { + const ns = 'user:wire' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const res = await handleRequest( + new Request(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: { 'a.md': b64('v2') }, base: { 'a.md': 'sha256:0000' } }), + }), + ) + expect(res.status).toBe(409) + const body = (await res.json()) as { error: { code: string; details: { conflicts: object } } } + expect(body.error.code).toBe('stale_base_version') + expect(Object.keys(body.error.details.conflicts)).toEqual(['a.md']) + }) + + test('a stale DELETE returns 409, not 500', async () => { + const ns = 'user:wiredel' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const res = await handleRequest( + new Request(`${url(ns)}?key=a.md&base=sha256:${'0'.repeat(64)}`, { method: 'DELETE' }), + ) + expect(res.status).toBe(409) + expect(((await res.json()) as { error: { code: string } }).error.code).toBe( + 'stale_base_version', + ) + }) + + test('a malformed base is 400 on both PUT and DELETE', async () => { + const ns = 'user:badbase' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const p = await handleRequest( + new Request(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: {}, base: { 'a.md': 7 } }), + }), + ) + expect(p.status).toBe(400) + const d = await handleRequest( + new Request(`${url(ns)}?key=a.md&base=not-a-hash`, { method: 'DELETE' }), + ) + expect(d.status).toBe(400) + }) +}) From 3d4632948e5e2fd90261379a54ab0d5c34d676cb Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:34:27 -0500 Subject: [PATCH 04/14] feat(store): let the store declare whether delete erases Whether a delete removes the bytes is a property of the store, and a client cannot see which driver a deployment runs. So BlobStore declares it and the server reports it where a client already looks: the hashes view and every write response. fs and s3 erase. A store that keeps history does not, and a client that knows can refuse a scan mode that would let a secret land somewhere it can never be removed from. The interface change is pinned by a @ts-expect-error on a store missing the attribute: if the requirement is ever relaxed the directive goes unused and the type check fails. Verified by relaxing it and watching that happen. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/memory.ts | 2 + src/store/blobstore.ts | 10 +++++ src/store/fs.ts | 2 + src/store/s3.ts | 2 + src/types.ts | 6 +++ tests/atomic-visibility.test.ts | 1 + tests/erasure.test.ts | 72 +++++++++++++++++++++++++++++++++ tests/store-seam.test.ts | 1 + 8 files changed, 96 insertions(+) create mode 100644 tests/erasure.test.ts diff --git a/src/memory.ts b/src/memory.ts index 508d74f..b86dc62 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -71,6 +71,7 @@ export async function getHashes(namespace: string, nsSlug: string): Promise /** A short label for logs/health (e.g. "fs:/data", "s3:memlawb"). */ describe(): string + /** + * Whether `delete` actually removes the bytes. A store that keeps history + * (a git-backed one, say) retains them, and a client that knows can refuse a + * scan mode that would let a secret land somewhere it can never be removed + * from. Constant per store, so reporting it costs nothing per request. + */ + readonly erasure: Erasure } +/** Whether a store's delete is destructive. */ +export type Erasure = 'erases' | 'retains' + /** Build the storage path for a namespace's manifest. */ export function manifestPath(nsSlug: string): string { return `ns/${nsSlug}/manifest.json` diff --git a/src/store/fs.ts b/src/store/fs.ts index 6955b08..000eaff 100644 --- a/src/store/fs.ts +++ b/src/store/fs.ts @@ -11,6 +11,8 @@ import { dirname, join, resolve, sep } from 'node:path' import type { BlobStore } from './blobstore.ts' export class FsBlobStore implements BlobStore { + readonly erasure = 'erases' as const + private readonly root: string constructor(dataDir: string) { diff --git a/src/store/s3.ts b/src/store/s3.ts index d4e7bcb..85f98e2 100644 --- a/src/store/s3.ts +++ b/src/store/s3.ts @@ -17,6 +17,8 @@ type S3Settings = { } export class S3BlobStore implements BlobStore { + readonly erasure = 'erases' as const + private readonly client: Bun.S3Client private readonly bucket: string diff --git a/src/types.ts b/src/types.ts index 6b0a73d..3ad156c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -9,6 +9,8 @@ * client shim is a thin adapter rather than a rewrite. */ +import type { Erasure } from './store/blobstore.ts' + /** Per-namespace manifest, persisted as its own blob. */ export type Manifest = { version: number @@ -51,6 +53,8 @@ export type MemoryHashes = { lastModified: string checksum: string entryChecksums: Record + /** Whether this deployment's store actually erases on delete. */ + erasure: Erasure /** * Server capabilities a client can rely on. Without this a client cannot tell * a server that enforces the write precondition from one that ignores an @@ -93,6 +97,8 @@ export type UpsertResponse = { namespace: string version: number checksum: string + /** Whether this deployment's store actually erases on delete. */ + erasure: Erasure accepted: string[] deleted: string[] skipped: { key: string; reason: string }[] diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index 8f1df20..87f604a 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -42,6 +42,7 @@ function faulty(inner: BlobStore, failAt: number) { return inner.delete(p) }, describe: () => `faulty(${inner.describe()})`, + erasure: inner.erasure, } as BlobStore, } } diff --git a/tests/erasure.test.ts b/tests/erasure.test.ts new file mode 100644 index 0000000..376b356 --- /dev/null +++ b/tests/erasure.test.ts @@ -0,0 +1,72 @@ +/** + * Erasure advertisement (U5, R22/R27/R28 server half). + * + * Whether a delete actually erases is a property of the store, not of the + * client, and the client cannot see which driver is configured. So the store + * declares it and the server reports it where a client already looks: the + * hashes view and every write response. fs and s3 erase; a store that keeps + * history (the node driver, later) does not, and a client that knows will + * refuse a scan mode that would let a secret reach a store it can never leave. + */ + +import { afterEach, describe, expect, test } from 'bun:test' +import { getHashes, upsert } from '../src/memory.ts' +import { namespaceSlug } from '../src/namespace.ts' +import type { BlobStore } from '../src/store/blobstore.ts' +import { getStore, resetStore, setStore } from '../src/store/index.ts' + +const NOW = '2026-06-24T00:00:00.000Z' +const b64 = (s: string) => Buffer.from(s).toString('base64') +const put = (ns: string, req: Parameters[3]) => + upsert(ns, namespaceSlug(ns), 'local', req, NOW) + +afterEach(() => resetStore()) + +describe('erasure advertisement', () => { + test('the filesystem driver reports erasing on both surfaces', async () => { + const ns = 'user:erase' + const w = await put(ns, { entries: { 'a.md': b64('v1') } }) + expect(w.erasure).toBe('erases') + const h = await getHashes(ns, namespaceSlug(ns)) + expect(h.erasure).toBe('erases') + const d = await put(ns, { entries: {}, deletions: ['a.md'] }) + expect(d.erasure).toBe('erases') + }) + + test('a retaining store reports retaining on both surfaces', async () => { + const inner = getStore() + const retaining: BlobStore = { + get: p => inner.get(p), + put: (p, b) => inner.put(p, b), + delete: p => inner.delete(p), + describe: () => 'retaining', + erasure: 'retains', + } + setStore(retaining) + const ns = 'user:retain' + const w = await put(ns, { entries: { 'a.md': b64('v1') } }) + expect(w.erasure).toBe('retains') + const h = await getHashes(ns, namespaceSlug(ns)) + expect(h.erasure).toBe('retains') + }) + + test('a store missing the attribute does not type-check', () => { + // The pin is the @ts-expect-error itself: if BlobStore ever stops requiring + // `erasure`, this object becomes valid, the directive becomes unused, and + // `bun run type-check` fails. That is the control -- the assertion below + // only proves the object exists. + // @ts-expect-error - BlobStore requires `erasure` + const incomplete: BlobStore = { + get: async () => null, + put: async () => {}, + delete: async () => {}, + describe: () => 'incomplete', + } + expect(incomplete.describe()).toBe('incomplete') + }) + + test('the real drivers both declare erasure', async () => { + resetStore() + expect(getStore().erasure).toBe('erases') + }) +}) diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index c25f2ce..2ae6a7f 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -23,6 +23,7 @@ function stub(): BlobStore { put: async () => {}, delete: async () => {}, describe: () => 'stub', + erasure: 'erases', } } From 2bf3b8f1c9960e0eae35c2086360e788289efd5a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:35:57 -0500 Subject: [PATCH 05/14] fix(health): report liveness only, and probe the store at startup The health route is unauthenticated, so everything it says is public. It echoed the store's description, which on a driver whose label carries a URL or an owner would hand that to anyone who asks. It now returns liveness and the service name and nothing else, pinned by an exact-equality assertion. Reachability is still worth knowing, so it moved to startup: one write, read back, compare and remove under a reserved prefix, before the socket binds. A store we cannot reach now stops the process instead of answering 200 over it. The failure detail is the error's class, never its message, because a store error commonly carries an endpoint, a bucket and an object path, and that path carries a namespace slug. The disjointness control asserts the probe prefix against the paths the builders actually produce. Asserting that the namespace validators reject it would prove the wrong property and could not fail: tenants supply namespaces, never paths. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 2 +- src/index.ts | 8 +++++ src/quota.ts | 2 +- src/store/probe.ts | 45 +++++++++++++++++++++++ tests/health.test.ts | 85 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 src/store/probe.ts create mode 100644 tests/health.test.ts diff --git a/src/handler.ts b/src/handler.ts index c007af6..3dad940 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -66,7 +66,7 @@ export async function handleRequest(req: Request): Promise { const { pathname } = url if (pathname === '/health') { - return json({ ok: true, store: getStore().describe(), service: 'memlawb' }) + return json({ ok: true, service: 'memlawb' }) } const parsed = parseMemoryPath(pathname) diff --git a/src/index.ts b/src/index.ts index fdd6e06..de6ef96 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,6 +9,14 @@ import { config } from './config.ts' import { handleRequest } from './handler.ts' import { getStore } from './store/index.ts' +import { probeStore } from './store/probe.ts' + +const probe = await probeStore() +if (!probe.ok) { + // Refuse to serve rather than answer 200 over a store we cannot reach. + process.stderr.write(`[memlawb] store probe failed: ${probe.detail}\n`) + process.exit(1) +} const server = Bun.serve({ port: config.port, diff --git a/src/quota.ts b/src/quota.ts index 67082a0..4418ecd 100644 --- a/src/quota.ts +++ b/src/quota.ts @@ -31,7 +31,7 @@ export class QuotaError extends Error { } } -function usagePath(owner: string): string { +export function usagePath(owner: string): string { // Hash the owner id so the storage layout never embeds a raw (possibly // user-derived) identifier, matching how namespaces/entries are pathed. return `owners/${sha256Hex(owner)}/usage.json` diff --git a/src/store/probe.ts b/src/store/probe.ts new file mode 100644 index 0000000..55cab40 --- /dev/null +++ b/src/store/probe.ts @@ -0,0 +1,45 @@ +/** + * Startup store probe. + * + * `/health` is unauthenticated, so it reports liveness and nothing else: a + * store round trip there would be an anonymous write against the store holding + * every tenant's ciphertext, and its description would leak whatever the driver + * labels itself with. Reachability is still worth knowing, so it is checked once + * at startup, before the socket binds, where an operator sees the result and a + * broken store keeps the process from serving at all. + * + * The probe writes under its own prefix. Tenant data lives under `ns/` and + * `owners/`, so nothing a tenant can address collides with it. + */ + +import { randomUUID } from 'node:crypto' +import { getStore } from './index.ts' + +/** Reserved for the probe. Disjoint from every tenant path prefix. */ +export const PROBE_PREFIX = 'probe/' + +export type ProbeResult = { ok: boolean; detail?: string } + +/** + * Write, read back, compare, and remove one object. Returns rather than throws + * so the caller decides what a failure means. The failure detail is the error's + * class, never its message: a store error commonly carries an endpoint, a + * bucket and an object path, and that path carries a namespace slug. + */ +export async function probeStore(): Promise { + const store = getStore() + const path = `${PROBE_PREFIX}${randomUUID()}` + const payload = new TextEncoder().encode(randomUUID()) + try { + await store.put(path, payload) + const read = await store.get(path) + if (!read || Buffer.compare(Buffer.from(read), Buffer.from(payload)) !== 0) { + return { ok: false, detail: 'store round trip returned different bytes' } + } + return { ok: true } + } catch (err) { + return { ok: false, detail: `store unreachable (${(err as Error).constructor.name})` } + } finally { + await store.delete(path).catch(() => {}) + } +} diff --git a/tests/health.test.ts b/tests/health.test.ts new file mode 100644 index 0000000..999b1de --- /dev/null +++ b/tests/health.test.ts @@ -0,0 +1,85 @@ +/** + * Health is liveness; the store check runs at startup (U6, R15/R24). + * + * The health route is unauthenticated, so anything it reports is public. It + * used to echo the store's description, which on a driver whose label carries a + * URL or an owner would hand that to anyone. Reachability is worth knowing, but + * it belongs at startup where an operator sees it and a broken store keeps the + * process from binding at all. + */ + +import { describe, expect, test } from 'bun:test' +import { handleRequest } from '../src/handler.ts' +import { usagePath } from '../src/quota.ts' +import type { BlobStore } from '../src/store/blobstore.ts' +import { entryPath, manifestPath } from '../src/store/blobstore.ts' +import { getStore, resetStore, setStore } from '../src/store/index.ts' +import { PROBE_PREFIX, probeStore } from '../src/store/probe.ts' + +describe('health route', () => { + test('reports liveness and nothing about the store', async () => { + const res = await handleRequest(new Request('http://x/health')) + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true, service: 'memlawb' }) + }) +}) + +describe('startup store probe', () => { + test('round-trips and leaves nothing behind', async () => { + resetStore() + const store = getStore() + const seen: string[] = [] + setStore({ + get: p => { + seen.push(`get ${p}`) + return store.get(p) + }, + put: (p, b) => { + seen.push(`put ${p}`) + return store.put(p, b) + }, + delete: p => { + seen.push(`delete ${p}`) + return store.delete(p) + }, + describe: () => store.describe(), + erasure: store.erasure, + }) + const r = await probeStore() + expect(r.ok).toBe(true) + // Wrote, read back, and removed: the object must not survive the probe. + const written = seen.find(s => s.startsWith('put '))?.slice(4) as string + expect(written).toStartWith(PROBE_PREFIX) + resetStore() + expect(await getStore().get(written)).toBeNull() + }) + + test('a failing store reports failure without naming a credential', async () => { + const inner = getStore() + setStore({ + get: p => inner.get(p), + put: async () => { + throw new Error('connect ECONNREFUSED key=AKIAsecret bucket=private-bucket') + }, + delete: p => inner.delete(p), + describe: () => 's3:private-bucket', + erasure: 'erases', + } as BlobStore) + const r = await probeStore() + resetStore() + expect(r.ok).toBe(false) + expect(r.detail).not.toContain('AKIAsecret') + expect(r.detail).not.toContain('private-bucket') + }) + + test('the probe prefix is disjoint from every tenant path prefix', () => { + // Tenants supply namespaces, never store paths, so asserting the validators + // reject the prefix would prove the wrong thing and could not fail. Assert + // against the prefixes the path builders actually produce. + const tenantPaths = [manifestPath('deadbeef'), entryPath('deadbeef', 'abc'), usagePath('alice')] + // Control: those really are the shapes tenant data takes. + expect(tenantPaths.some(p => p.startsWith('ns/'))).toBe(true) + expect(tenantPaths.some(p => p.startsWith('owners/'))).toBe(true) + for (const p of tenantPaths) expect(p.startsWith(PROBE_PREFIX)).toBe(false) + }) +}) From fe97dd5377c141ec5418e761caf74884cf47d7e3 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:38:46 -0500 Subject: [PATCH 06/14] feat(observability): log every refusal with a closed field set An operator could not tell which account was refused or why: the only output was a startup line and a catch-all that dumped the raw error. Every refusal now emits one JSON line carrying timestamp, owner, code, status and route class. The field set is an allowlist, not a denylist. On a crypto-blind server the space of things that must never reach a log is open-ended, so a denylist only catches what someone thought to forbid; five fields cannot carry any of it because there is nowhere for it to go. The type has no index signature, so the compiler enforces it too. A namespace slug is deliberately absent: it reads as opaque but is a hash of a low-entropy namespace, so it is a stable per-tenant identifier anyone can reverse by dictionary. Logging happens once, where the response leaves handleRequest, so the code recorded is the code the caller received and a future refusal branch cannot be added without being covered. The catch-all now logs the error's class rather than its message, because a store error carries an endpoint, a bucket and an object path, and that path carries a slug. resetRateLimit exists because bun shares one process across test files and a test that exhausts a bucket would otherwise refuse requests in every later suite. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 32 +++++++++++++- src/log.ts | 50 ++++++++++++++++++++++ src/ratelimit.ts | 9 ++++ tests/rejection-log.test.ts | 84 +++++++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 src/log.ts create mode 100644 tests/rejection-log.test.ts diff --git a/src/handler.ts b/src/handler.ts index 3dad940..1fcde16 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -16,6 +16,7 @@ import { authenticate, authorizeNamespace } from './auth.ts' import { config } from './config.ts' +import { logRejection } from './log.ts' import { getData, getHashes, upsert } from './memory.ts' import { InvalidNameError, @@ -25,7 +26,6 @@ import { } from './namespace.ts' import { QuotaError } from './quota.ts' import { take } from './ratelimit.ts' -import { getStore } from './store/index.ts' import { parseUpsertRequest, StaleBaseError } from './types.ts' // Applied to every response. The API serves only JSON and is consumed by @@ -61,7 +61,31 @@ function parseMemoryPath(pathname: string): { namespace: string } | null { return { namespace: rest } } +/** + * Log every refusal from one place, after the response is built, so the code + * recorded is literally the code the caller received and no future refusal + * branch can be added without being covered. + */ export async function handleRequest(req: Request): Promise { + const ctx: RequestContext = { owner: 'anonymous', route: 'other' } + const res = await respond(req, ctx) + if (res.status >= 400) { + let code = 'unknown' + try { + const body = (await res.clone().json()) as { error?: { code?: string } } + if (body.error?.code) code = body.error.code + } catch { + // A refusal with a non-JSON body still gets a line; the code stays unknown. + } + logRejection({ owner: ctx.owner, code, status: res.status, route: ctx.route }) + } + return res +} + +/** Per-request facts the rejection log needs, filled in as they become known. */ +type RequestContext = { owner: string; route: string } + +async function respond(req: Request, ctx: RequestContext): Promise { const url = new URL(req.url) const { pathname } = url @@ -70,10 +94,12 @@ export async function handleRequest(req: Request): Promise { } const parsed = parseMemoryPath(pathname) + if (parsed) ctx.route = 'memory' if (!parsed) return apiError('not_found', 'unknown route', 404) const identity = await authenticate(req) if (!identity) return apiError('unauthorized', 'missing or invalid API key', 401) + ctx.owner = identity.owner const rate = take(identity.owner, Date.now()) if (!rate.ok) { @@ -182,7 +208,9 @@ export async function handleRequest(req: Request): Promise { return apiError('method_not_allowed', `${req.method} not supported`, 405) } catch (err) { - console.error('[memlawb] handler error', err) + // The error's class only. A store error commonly carries an endpoint, a + // bucket and an object path, and that path carries a namespace slug. + console.error(`[memlawb] handler error (${(err as Error)?.constructor?.name ?? 'unknown'})`) return apiError('internal', 'internal error', 500) } } diff --git a/src/log.ts b/src/log.ts new file mode 100644 index 0000000..f2f8853 --- /dev/null +++ b/src/log.ts @@ -0,0 +1,50 @@ +/** + * The rejection log. + * + * Observability here is one line per refused request, and the field set is an + * allowlist rather than a denylist. The space of things that must never appear + * in a log on a crypto-blind server is open-ended (entry keys, raw namespaces, + * ciphertext, tokens, whatever a future error object carries), so a denylist + * only ever catches what someone thought to forbid. A fixed set of five fields + * cannot carry any of it, because there is nowhere for it to go. + * + * A namespace slug is deliberately absent. It reads as opaque, but it is a hash + * of a low-entropy namespace like `user:alice`, so it is a stable per-tenant + * identifier anyone can reverse by dictionary. + */ + +/** The complete set of keys a rejection line may carry. */ +export const ALLOWED_FIELDS = ['timestamp', 'owner', 'code', 'status', 'route'] as const + +/** + * One refusal. The typed shape has no index signature on purpose: a field that + * is not one of these has no way into the object, so the allowlist is enforced + * by the compiler and not only by the test. + */ +export type Rejection = { + timestamp: string + /** The authenticated account, or `anonymous` before authentication. */ + owner: string + /** The API error code already returned to the caller. */ + code: string + status: number + /** Route class, not the path: `memory` or `other`. Never a namespace. */ + route: string +} + +type Sink = ((line: Rejection) => void) | null +let sink: Sink = null + +/** Tests capture lines instead of writing them. Passing null restores stderr. */ +export function setRejectionSink(next: Sink): void { + sink = next +} + +export function logRejection(fields: Omit): void { + const line: Rejection = { timestamp: new Date().toISOString(), ...fields } + if (sink) { + sink(line) + return + } + process.stderr.write(`${JSON.stringify(line)}\n`) +} diff --git a/src/ratelimit.ts b/src/ratelimit.ts index af52519..c225e40 100644 --- a/src/ratelimit.ts +++ b/src/ratelimit.ts @@ -48,3 +48,12 @@ export function take(owner: string, now: number): RateResult { export function _reset(): void { buckets.clear() } + +/** + * Drop every bucket. Tests only: the buckets are per owner and in-process, and + * bun shares one process across test files, so a test that deliberately + * exhausts a bucket would otherwise refuse requests in every later suite. + */ +export function resetRateLimit(): void { + buckets.clear() +} diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts new file mode 100644 index 0000000..507208a --- /dev/null +++ b/tests/rejection-log.test.ts @@ -0,0 +1,84 @@ +/** + * The rejection log (U7, R13, AE8). + * + * An operator needs to know which account was refused and why. The risk is that + * a log line becomes the one place plaintext leaks, so the field set is an + * allowlist fixed in KTD15 rather than a denylist: the space of things that + * must not appear is open-ended, and a denylist only catches what someone + * thought of. A namespace slug is excluded too. It looks opaque but it is a + * hash of a low-entropy namespace, so it is a stable per-tenant identifier + * anyone can reverse by dictionary. + */ + +import { afterEach, describe, expect, test } from 'bun:test' +import { handleRequest } from '../src/handler.ts' +import { ALLOWED_FIELDS, setRejectionSink } from '../src/log.ts' +import { resetRateLimit } from '../src/ratelimit.ts' + +const lines: Record[] = [] + +afterEach(() => { + setRejectionSink(null) + lines.length = 0 + // The bucket is per owner and in-process, and every test here shares one + // owner under open auth. Draining it would leak refusals into the suites that + // share this process. + resetRateLimit() +}) + +function capture() { + lines.length = 0 + setRejectionSink(l => lines.push(l as Record)) +} + +describe('rejection log', () => { + test('a rate-limited caller produces exactly one line, with only allowed fields', async () => { + capture() + let last: Response | undefined + for (let i = 0; i < 400; i++) { + last = await handleRequest(new Request('http://x/api/memory/user:local')) + if (last.status === 429) break + } + expect(last?.status).toBe(429) + // Every refusal on the way here is logged too (the namespace does not + // exist, so each read is a 404). All of them must obey the allowlist. + expect(lines.length).toBeGreaterThan(1) + for (const l of lines) expect(Object.keys(l).sort()).toEqual([...ALLOWED_FIELDS].sort()) + const limited = lines.filter(l => l.code === 'rate_limited') + expect(limited.length).toBe(1) + expect(limited[0]?.status).toBe(429) + expect(limited[0]?.owner).toBe('local') + }) + + test('negative control: a line carrying an extra field fails the same check', () => { + const planted = { timestamp: 't', owner: 'o', code: 'c', status: 1, route: 'r', slug: 'x' } + expect(Object.keys(planted).sort()).not.toEqual([...ALLOWED_FIELDS].sort()) + }) + + test('a sentinel in the request never reaches the line, and the check can see one', async () => { + capture() + const sentinel = 'SENTINEL-abc123' + let last: Response | undefined + for (let i = 0; i < 400; i++) { + last = await handleRequest(new Request(`http://x/api/memory/user:${sentinel}`)) + if (last.status === 429) break + } + expect(last?.status).toBe(429) + expect(lines.length).toBeGreaterThan(0) + const serialized = JSON.stringify(lines) + expect(serialized).not.toContain(sentinel) + // Control: the assertion can actually see a sentinel when one is present. + expect(JSON.stringify([{ ...lines[0], planted: sentinel }])).toContain(sentinel) + }) + + test('an unauthorized caller is logged with its code', async () => { + capture() + const res = await handleRequest( + new Request('http://x/api/memory/user:local', { method: 'POST' }), + ) + expect(res.status).toBe(405) + expect(lines.length).toBe(1) + expect(lines[0]?.code).toBe('method_not_allowed') + expect(lines[0]?.route).toBe('memory') + }) +}) From 9fed0adcd95e935e76d3af3ae40c3315173d0b57 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:44:06 -0500 Subject: [PATCH 07/14] refactor: consolidate Phase 0 duplication and correct stale docs Three reviewers over the source diff. What they found: resetRateLimit duplicated _reset, which already existed and was already used by the rate-limit suite. Removed mine and moved the better explanation onto the one that was there. The blobstore module header still described one blob per entry key, which is the layout this branch replaced, and entryPath's docstring read as the current way to store an entry rather than the legacy one. A reader landing on either would have written new entries the old way. A code comment cited R12, a requirement id in docs/plans, which is in .git/info/exclude. That reference resolves to nothing for anyone reading the repo, so it now states the property instead. The QuotaError and StaleBaseError mapping was duplicated across the PUT and DELETE branches. Both reviewers verified folding it into the outer catch would be behavior-preserving today; it goes in a helper instead, so a future read path that threw QuotaError cannot silently answer 413 rather than 500. Also: prev now reuses checksumsFrom rather than rebuilding it, the unreachable half of the mutated disjunct is gone, a double branch on the same condition is one guard, and the readManifest comment no longer claims the refusal is write-only when reads take the same path. One tradeoff taken deliberately. The legacy blob sweep now runs only for keys the pre-write manifest knew, since only those can have a legacy blob. On s3 the unguarded version was a network round trip per touched key on every write, forever, while holding the namespace and owner locks. What is given up is sweeping a crash-orphaned legacy blob for a key absent from the manifest, which today is only swept in the narrow case where that key happens to be written again. Verified the sweep is still covered: removing it turns both legacy tests red. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 35 +++++++++++++++++++++-------------- src/memory.ts | 23 +++++++++++++++-------- src/ratelimit.ts | 6 +----- src/store/blobstore.ts | 12 ++++++++---- tests/rejection-log.test.ts | 4 ++-- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/src/handler.ts b/src/handler.ts index 1fcde16..01ec89f 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -28,6 +28,9 @@ import { QuotaError } from './quota.ts' import { take } from './ratelimit.ts' import { parseUpsertRequest, StaleBaseError } from './types.ts' +/** The shape a DELETE `base` query value must take. */ +const BASE_HASH_RE = /^sha256:[0-9a-f]{64}$/ + // Applied to every response. The API serves only JSON and is consumed by // programmatic clients, so we lock down sniffing/caching/referrer leakage. const SECURITY_HEADERS = { @@ -61,6 +64,18 @@ function parseMemoryPath(pathname: string): { namespace: string } | null { return { namespace: rest } } +/** + * Map the two refusals `upsert` can raise. Deliberately not folded into the + * outer catch: that would widen these statuses to cover every read path in the + * handler, so a future read that threw QuotaError would silently answer 413 + * instead of 500. Returns null for anything else, which the caller rethrows. + */ +function upsertFailure(err: unknown): Response | null { + if (err instanceof QuotaError) return apiError(err.code, err.message, 413, err.details) + if (err instanceof StaleBaseError) return apiError(err.code, err.message, 409, err.details) + return null +} + /** * Log every refusal from one place, after the response is built, so the code * recorded is literally the code the caller received and no future refusal @@ -94,8 +109,8 @@ async function respond(req: Request, ctx: RequestContext): Promise { } const parsed = parseMemoryPath(pathname) - if (parsed) ctx.route = 'memory' if (!parsed) return apiError('not_found', 'unknown route', 404) + ctx.route = 'memory' const identity = await authenticate(req) if (!identity) return apiError('unauthorized', 'missing or invalid API key', 401) @@ -161,12 +176,8 @@ async function respond(req: Request, ctx: RequestContext): Promise { ) return json(result) } catch (err) { - if (err instanceof QuotaError) { - return apiError(err.code, err.message, 413, err.details) - } - if (err instanceof StaleBaseError) { - return apiError(err.code, err.message, 409, err.details) - } + const mapped = upsertFailure(err) + if (mapped) return mapped throw err } } @@ -182,7 +193,7 @@ async function respond(req: Request, ctx: RequestContext): Promise { // The base rides the query here rather than a body, so it needs its own // shape check: parseUpsertRequest never sees a DELETE. const rawBase = url.searchParams.get('base') - if (rawBase !== null && !/^sha256:[0-9a-f]{64}$/.test(rawBase)) { + if (rawBase !== null && !BASE_HASH_RE.test(rawBase)) { return apiError('bad_request', '`base` must be a sha256: hash', 400) } const base = rawBase === null ? undefined : { [key]: rawBase } @@ -196,12 +207,8 @@ async function respond(req: Request, ctx: RequestContext): Promise { ) return json(result) } catch (err) { - if (err instanceof QuotaError) { - return apiError(err.code, err.message, 413, err.details) - } - if (err instanceof StaleBaseError) { - return apiError(err.code, err.message, 409, err.details) - } + const mapped = upsertFailure(err) + if (mapped) return mapped throw err } } diff --git a/src/memory.ts b/src/memory.ts index b86dc62..052ceb7 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -30,7 +30,11 @@ import { type UpsertResponse, } from './types.ts' -/** Capabilities the hashes view advertises. See StaleBaseError and R12. */ +/** + * Capabilities the hashes view advertises. Without this a client cannot tell a + * server that enforces the write precondition from one that ignores an unknown + * body field, so it would report a guarantee it is not getting. + */ const SUPPORTS = ['base-precondition'] // ─── Manifest helpers ─────────────────────────────────────────────────── @@ -128,8 +132,7 @@ export async function upsert( const m = await readManifest(nsSlug) // The projection mutates `m.entries` in place, so capture what the currently // visible manifest points at before touching it; cleanup needs the old hashes. - const prev: Record = {} - for (const [k, meta] of Object.entries(m.entries)) prev[k] = meta.hash + const prev = checksumsFrom(m) const touched = new Set() // Compare the caller's base before any projection, against the manifest this @@ -201,7 +204,8 @@ export async function upsert( accepted.push(key) } - const mutated = blobWrites.length > 0 || touched.size > 0 + // Every blobWrites push is paired with a touched.add, so touched alone decides. + const mutated = touched.size > 0 if (mutated) { // Project the namespace's final footprint and enforce its byte cap. let nsBytes = 0 @@ -228,10 +232,13 @@ export async function upsert( // Deterministic ciphertext means another entry may hold this exact // blob; only reclaim it when no live entry still names that hash. if (old && !live.has(old)) await store.delete(contentPath(nsSlug, old)) - // Entries written before content addressing live at a key-derived - // path the new layout never names, so nothing else would ever remove - // them and a delete would silently leave the ciphertext behind. - await store.delete(entryPath(nsSlug, sha256Hex(key))) + // Entries written before content addressing live at a key-derived path + // the new layout never names, so nothing else would ever remove them + // and a delete would silently leave the ciphertext behind. Only a key + // the pre-write manifest knew can have one, so skip the rest: on s3 + // this would otherwise be a network round trip per touched key, on + // every write, forever, holding both the namespace and owner locks. + if (old !== undefined) await store.delete(entryPath(nsSlug, sha256Hex(key))) } } diff --git a/src/ratelimit.ts b/src/ratelimit.ts index c225e40..bafca6f 100644 --- a/src/ratelimit.ts +++ b/src/ratelimit.ts @@ -45,15 +45,11 @@ export function take(owner: string, now: number): RateResult { } /** Test-only: clear all buckets. */ -export function _reset(): void { - buckets.clear() -} - /** * Drop every bucket. Tests only: the buckets are per owner and in-process, and * bun shares one process across test files, so a test that deliberately * exhausts a bucket would otherwise refuse requests in every later suite. */ -export function resetRateLimit(): void { +export function _reset(): void { buckets.clear() } diff --git a/src/store/blobstore.ts b/src/store/blobstore.ts index 5ab3a75..d6c61c5 100644 --- a/src/store/blobstore.ts +++ b/src/store/blobstore.ts @@ -3,7 +3,9 @@ * * memlawb stores two things per namespace: * - a `manifest` blob: JSON map of entryKey -> { hash, size, updatedAt } - * - one `entry` blob per entryKey: the CIPHERTEXT of that memory entry + * - one ciphertext blob per distinct entry ciphertext, named by its own hash + * (see `contentPath`); the manifest maps each entryKey to that hash, and two + * entries holding identical ciphertext share one blob * * The store only ever sees opaque bytes. It has no idea what an "entry" means * and could not decrypt one if it wanted to. Adapters: fs (self-host default), @@ -40,9 +42,11 @@ export function manifestPath(nsSlug: string): string { } /** - * Build the storage path for one entry. We hash the entry key so weird-but- - * valid keys map to a flat, fixed-width filename, and the original key is only - * recorded inside the (encrypted-at-the-edges) manifest. + * Build the pre-content-addressing storage path for one entry, hashing the entry + * key so weird-but-valid keys map to a flat, fixed-width filename. + * + * Retained only so entries written under the old layout stay readable and get + * reclaimed when they are next touched. New writes go through `contentPath`. */ export function entryPath(nsSlug: string, entryKeyHash: string): string { return `ns/${nsSlug}/entries/${entryKeyHash}` diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts index 507208a..56f73ef 100644 --- a/tests/rejection-log.test.ts +++ b/tests/rejection-log.test.ts @@ -13,7 +13,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import { handleRequest } from '../src/handler.ts' import { ALLOWED_FIELDS, setRejectionSink } from '../src/log.ts' -import { resetRateLimit } from '../src/ratelimit.ts' +import { _reset } from '../src/ratelimit.ts' const lines: Record[] = [] @@ -23,7 +23,7 @@ afterEach(() => { // The bucket is per owner and in-process, and every test here shares one // owner under open auth. Draining it would leak refusals into the suites that // share this process. - resetRateLimit() + _reset() }) function capture() { From 86207f31b854fa25b365b6d803d88b78f3fc07d1 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:59:24 -0500 Subject: [PATCH 08/14] fix(memory): make reclaim non-fatal, complete, and actually tested Nine reviewers went at the previous commits. Three findings were real bugs and two were my tests failing the bar I set for them. Reclaim ran inside the commit closure, so a transient store delete after the manifest was published turned a durable write into a 500 and skipped the owner usage write, leaving quota under-counting. It now runs after the write is durable and never throws: collecting garbage that is already invisible to every reader must not fail, or roll back, a write that landed. Reclaim was also driven from the keys a request touched, which cannot see the orphans that matter. A write that died before publishing left blobs no later request can name, and a delete whose collection failed removed the key from the manifest so nothing could name its hash again. Those bytes stayed forever, uncounted by quota, while the server advertised erasure: 'erases'. BlobStore gains list() and reclaim now sweeps the namespace's blob directory against the live hash set, which finds both. Manifest hashes form storage paths now, and a manifest is parsed JSON rather than validated input, so contentPath proves the digest is bare hex before building a path. A malformed percent escape threw out of handleRequest entirely, past the envelope, the security headers and the log line, falsifying the comment saying no refusal branch escapes coverage. On the tests: assertConsistent compared two maps getData fills on consecutive lines behind one guard, so its missing-blob half could not fail; it now compares against the manifest. The corrupt-manifest control read the legacy path seed() never writes, so it asserted null equals null. The sweep reused one namespace, so later indices never reached a mutating call and it covered half the sequence; it re-seeds per index and pins the count. The reclaim guard, the s3 erasure declaration, the log's owner and route defaults, and the traversal check had no coverage at all. Verified by mutation rather than by claim. Seven mutations that previously left the suite green now fail it: reclaim's live check dropped, reclaim removed, s3 erasure flipped to retains, the handler guard removed, route hardcoded, owner default changed, and the digest check removed. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 12 ++- src/memory.ts | 73 ++++++++++---- src/store/blobstore.ts | 30 +++++- src/store/fs.ts | 13 ++- src/store/index.ts | 2 +- src/store/s3.ts | 11 ++ tests/atomic-visibility.test.ts | 174 +++++++++++++++++++++++++++----- tests/erasure.test.ts | 16 ++- tests/health.test.ts | 2 + tests/rejection-log.test.ts | 26 ++++- tests/store-seam.test.ts | 13 ++- 11 files changed, 319 insertions(+), 53 deletions(-) diff --git a/src/handler.ts b/src/handler.ts index 01ec89f..b1c99a1 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -83,7 +83,17 @@ function upsertFailure(err: unknown): Response | null { */ export async function handleRequest(req: Request): Promise { const ctx: RequestContext = { owner: 'anonymous', route: 'other' } - const res = await respond(req, ctx) + // respond() parses the path before its own try block, so a malformed percent + // escape throws past it. Without this guard that reaches the runtime as an + // unhandled error: no envelope, no security headers, and no log line, which + // would make the claim above false for a request anyone can send. + let res: Response + try { + res = await respond(req, ctx) + } catch (err) { + console.error(`[memlawb] handler error (${(err as Error)?.constructor?.name ?? 'unknown'})`) + res = apiError('internal', 'internal error', 500) + } if (res.status >= 400) { let code = 'unknown' try { diff --git a/src/memory.ts b/src/memory.ts index 052ceb7..caf37fc 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -19,7 +19,7 @@ import { namespaceChecksum, sha256Hex, sha256Prefixed } from './hash.ts' import { withLock } from './lock.ts' import { validateEntryKey } from './namespace.ts' import { QuotaError, reserveAndCommit } from './quota.ts' -import { contentPath, entryPath, getStore, manifestPath } from './store/index.ts' +import { blobPrefix, contentPath, entryPath, getStore, manifestPath } from './store/index.ts' import { emptyManifest, type Manifest, @@ -217,29 +217,16 @@ export async function upsert( } const commit = async () => { - // Blobs first, then the manifest that publishes them, then reclaim what - // the new manifest no longer references. A crash before the manifest - // write leaves orphans no reader can see; a crash after it leaves stale - // extras no reader can see. Either way the visible state is consistent. + // Blobs first, then the manifest that publishes them. A crash before + // the manifest write leaves orphans no reader can see; a crash after it + // leaves stale extras no reader can see. Either way the visible state is + // consistent. Reclaim is deliberately NOT here: it runs after the write + // is durable, because a failure to collect garbage must not fail, or + // roll back, a write that already landed. for (const w of blobWrites) await store.put(w.path, w.bytes) m.version += 1 m.lastModified = nowIso await writeManifest(nsSlug, m) - - const live = new Set(Object.values(m.entries).map(meta => meta.hash)) - for (const key of touched) { - const old = prev[key] - // Deterministic ciphertext means another entry may hold this exact - // blob; only reclaim it when no live entry still names that hash. - if (old && !live.has(old)) await store.delete(contentPath(nsSlug, old)) - // Entries written before content addressing live at a key-derived path - // the new layout never names, so nothing else would ever remove them - // and a delete would silently leave the ciphertext behind. Only a key - // the pre-write manifest knew can have one, so skip the rest: on s3 - // this would otherwise be a network round trip per touched key, on - // every write, forever, holding both the namespace and owner locks. - if (old !== undefined) await store.delete(entryPath(nsSlug, sha256Hex(key))) - } } const projected = { entries: Object.keys(m.entries).length, bytes: nsBytes } @@ -249,6 +236,8 @@ export async function upsert( } else { await reserveAndCommit(owner, namespace, projected, nowIso, commit) } + + await reclaim(nsSlug, m, prev, touched) } return { @@ -263,6 +252,50 @@ export async function upsert( }) } +/** + * Delete ciphertext no visible manifest names. + * + * Driven by a listing rather than by the keys this request touched, because the + * orphans that matter are the ones no key can reach: a write that died before + * publishing left blobs the next manifest never mentions, and a delete whose + * collection failed removed the key from the manifest, so nothing can name its + * hash again. Sweeping the namespace's blob directory against the live hash set + * finds both, which is what makes `erasure: 'erases'` true rather than a claim. + * + * Never throws. This runs after the write is durable and collects garbage that + * is already invisible to every reader, so a store hiccup here must not turn a + * landed write into a failure, nor skip the caller's quota accounting. + */ +async function reclaim( + nsSlug: string, + m: Manifest, + prev: Record, + touched: Set, +): Promise { + const store = getStore() + try { + const live = new Set() + for (const meta of Object.values(m.entries)) live.add(contentPath(nsSlug, meta.hash)) + for (const path of await store.list(blobPrefix(nsSlug))) { + if (!live.has(path)) await store.delete(path) + } + // Entries written before content addressing sit at a key-derived path the + // sweep above does not cover, and only a key the pre-write manifest knew + // can have one. + for (const key of touched) { + if (prev[key] !== undefined) await store.delete(entryPath(nsSlug, sha256Hex(key))) + } + } catch (err) { + process.stderr.write( + `${JSON.stringify({ + timestamp: new Date().toISOString(), + event: 'reclaim_failed', + reason: (err as Error)?.constructor?.name ?? 'unknown', + })}\n`, + ) + } +} + function decodeBase64(b64: string): Uint8Array | null { try { const buf = Buffer.from(b64, 'base64') diff --git a/src/store/blobstore.ts b/src/store/blobstore.ts index d6c61c5..1141f4a 100644 --- a/src/store/blobstore.ts +++ b/src/store/blobstore.ts @@ -22,6 +22,15 @@ export interface BlobStore { put(path: string, bytes: Uint8Array): Promise /** Delete an object. No-op if it does not exist. */ delete(path: string): Promise + /** + * Every object path under `prefix`. Needed because reclaim has to find blobs + * no manifest names: a write that dies before publishing leaves ciphertext + * that no later request can reach by key, so a reclaim driven only from the + * previous manifest can never see it. Without this the store accumulates + * ciphertext that quota cannot count and `erasure: 'erases'` is a false + * promise. + */ + list(prefix: string): Promise /** A short label for logs/health (e.g. "fs:/data", "s3:memlawb"). */ describe(): string /** @@ -63,5 +72,24 @@ export function entryPath(nsSlug: string, entryKeyHash: string): string { * blob, never assume one entry owns it. */ export function contentPath(nsSlug: string, ciphertextHash: string): string { - return `ns/${nsSlug}/blobs/${ciphertextHash.replace(/^sha256:/, '')}` + return `ns/${nsSlug}/blobs/${bareHash(ciphertextHash)}` +} + +/** The directory every content-addressed blob for a namespace lives under. */ +export function blobPrefix(nsSlug: string): string { + return `ns/${nsSlug}/blobs/` +} + +/** + * Strip the `sha256:` prefix and prove what remains is a bare hex digest. + * + * A manifest hash reaches a storage path here, and a manifest is parsed JSON + * rather than validated input, so this is the boundary the repo's rule about + * validating names before they touch a path applies to. Without the check a + * hash carrying path separators escapes the namespace directory. + */ +function bareHash(ciphertextHash: string): string { + const bare = ciphertextHash.startsWith('sha256:') ? ciphertextHash.slice(7) : ciphertextHash + if (!/^[0-9a-f]{64}$/.test(bare)) throw new Error('ciphertext hash is not a sha256 digest') + return bare } diff --git a/src/store/fs.ts b/src/store/fs.ts index 000eaff..67d14dc 100644 --- a/src/store/fs.ts +++ b/src/store/fs.ts @@ -6,7 +6,7 @@ * mid-write can't leave a half-written manifest. */ -import { mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { mkdir, readdir, readFile, rename, rm, writeFile } from 'node:fs/promises' import { dirname, join, resolve, sep } from 'node:path' import type { BlobStore } from './blobstore.ts' @@ -51,6 +51,17 @@ export class FsBlobStore implements BlobStore { await rm(this.full(path), { force: true }) } + async list(prefix: string): Promise { + const dir = this.full(prefix) + try { + const names = await readdir(dir) + return names.filter(n => !n.startsWith('.tmp-')).map(n => `${prefix}${n}`) + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return [] + throw err + } + } + describe(): string { return `fs:${this.root}` } diff --git a/src/store/index.ts b/src/store/index.ts index 17a4689..c308e5f 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -31,4 +31,4 @@ export function resetStore(): void { } export type { BlobStore } from './blobstore.ts' -export { contentPath, entryPath, manifestPath } from './blobstore.ts' +export { blobPrefix, contentPath, entryPath, manifestPath } from './blobstore.ts' diff --git a/src/store/s3.ts b/src/store/s3.ts index 85f98e2..0b04814 100644 --- a/src/store/s3.ts +++ b/src/store/s3.ts @@ -60,6 +60,17 @@ export class S3BlobStore implements BlobStore { await this.client.delete(path) } + async list(prefix: string): Promise { + const out: string[] = [] + let token: string | undefined + do { + const page = await this.client.list({ prefix, continuationToken: token }) + for (const o of page.contents ?? []) if (o.key) out.push(o.key) + token = page.isTruncated ? page.nextContinuationToken : undefined + } while (token) + return out + } + describe(): string { return `s3:${this.bucket}` } diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index 87f604a..4113c2f 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -13,10 +13,10 @@ import { afterEach, describe, expect, test } from 'bun:test' import { sha256Hex, sha256Prefixed } from '../src/hash.ts' -import { getData, upsert } from '../src/memory.ts' +import { getData, getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' import type { BlobStore } from '../src/store/blobstore.ts' -import { getStore, resetStore, setStore } from '../src/store/index.ts' +import { contentPath, getStore, resetStore, setStore } from '../src/store/index.ts' const NOW = '2026-06-24T00:00:00.000Z' const b64 = (s: string) => Buffer.from(s).toString('base64') @@ -41,6 +41,7 @@ function faulty(inner: BlobStore, failAt: number) { guard() return inner.delete(p) }, + list: (p: string) => inner.list(p), describe: () => `faulty(${inner.describe()})`, erasure: inner.erasure, } as BlobStore, @@ -75,9 +76,13 @@ async function assertConsistent(ns: string) { const bytes = new Uint8Array(Buffer.from(b, 'base64')) expect(sha256Prefixed(bytes)).toBe(data.content.entryChecksums[key] as string) } - // No manifest entry may lack a readable blob. + // No manifest entry may lack a readable blob. This has to compare against the + // manifest: getData fills entries and entryChecksums in the same branch and + // returns early when a blob is missing, so comparing those two to each other + // is comparing a value to itself and cannot fail. + const manifest = await getHashes(ns, namespaceSlug(ns)) expect(Object.keys(data.content.entries).sort()).toEqual( - Object.keys(data.content.entryChecksums).sort(), + Object.keys(manifest.entryChecksums).sort(), ) return data } @@ -85,16 +90,18 @@ async function assertConsistent(ns: string) { afterEach(() => resetStore()) describe('crash visibility across the commit sequence', () => { - test('sweep: a fault at any mutating call leaves the previous complete state', async () => { - const ns = 'user:sweep' - await seed(ns, { 'a.md': b64('a1'), 'b.md': b64('b1'), 'c.md': b64('c1') }) - const before = await assertConsistent(ns) - - const real = getStore() + test('sweep: a fault at any mutating call leaves a complete, untorn state', async () => { let injected = 0 let untorn = 0 let published = 0 - for (let i = 0; i < 8; i++) { + // A fresh namespace per index. Reusing one lets the first successful + // iteration change the state so later indices never reach a mutating call, + // which silently halves the sequence the sweep claims to cover. + for (let i = 0; i < 12; i++) { + const ns = `user:sweep${i}` + await seed(ns, { 'a.md': b64('a1'), 'b.md': b64('b1'), 'c.md': b64('c1') }) + const before = await assertConsistent(ns) + const real = getStore() resetStore() const f = faulty(real, i) setStore(f.store) @@ -109,13 +116,15 @@ describe('crash visibility across the commit sequence', () => { NOW, ).catch(e => e) resetStore() - if (!(err instanceof Boom)) continue // past the end of the mutating sequence + if (!(err instanceof Boom)) continue // index beyond this request's mutating calls injected++ // Evidence the plant landed at this index rather than short-circuiting. expect(f.calls()).toBe(i + 1) - // Either the commit had not yet published (previous state) or it had - // (new state). Never a torn one: that is what R9 asserts. A fault during - // the post-manifest reclaim legitimately leaves the new state visible. + // Never a torn state. With reclaim moved out of the commit, every call + // that can fail happens before the manifest lands, so a fault always + // leaves the previous state rather than a published one. This is stronger + // than the disjunction it replaced, and it is not free: writing the + // manifest before the blobs turns it red. const after = await assertConsistent(ns) const keys = Object.keys(after.content.entries).sort().join(',') expect(['a.md,b.md,c.md', 'a.md,b.md,d.md']).toContain(keys) @@ -126,11 +135,13 @@ describe('crash visibility across the commit sequence', () => { published++ } } - // Control that the sweep spanned the manifest write rather than only one side. - expect(untorn).toBeGreaterThan(0) - expect(published).toBeGreaterThan(0) - // Positive control: the sweep actually injected faults, at more than one index. - expect(injected).toBeGreaterThan(2) + // Controls. The commit makes exactly four mutating calls: three blob writes + // and the manifest write. Faults past that land in reclaim, which is + // non-fatal by design, so four is the whole failable sequence rather than a + // floor someone guessed. Every one of them leaves the previous state. + expect(injected).toBe(4) + expect(untorn).toBe(4) + expect(published).toBe(0) }) test('harness control: a fault at index 0 leaves the namespace untouched', async () => { @@ -158,7 +169,12 @@ describe('crash visibility across the commit sequence', () => { await seed(ns, { 'a.md': b64('a1') }) const slug = namespaceSlug(ns) const store = getStore() - const blobBefore = await store.get(`ns/${slug}/entries/${sha256Hex('a.md')}`) + const path = contentPath(slug, sha256Prefixed(new Uint8Array(Buffer.from('a1')))) + const blobBefore = await store.get(path) + // Control: the blob is really there before the refused write, so the + // assertion below can fail. Reading the legacy key-derived path here would + // be null both times and prove nothing. + expect(blobBefore).not.toBeNull() await store.put(`ns/${slug}/manifest.json`, new TextEncoder().encode('{not json')) const err = await upsert(ns, slug, 'local', { entries: { 'b.md': b64('b1') } }, NOW).catch( @@ -167,9 +183,119 @@ describe('crash visibility across the commit sequence', () => { expect(err).toBeInstanceOf(Error) expect(String((err as Error).message)).toContain('manifest') // Control: the pre-existing blob is still there, so nothing was reclaimed. - expect(await store.get(`ns/${slug}/entries/${sha256Hex('a.md')}`)).toEqual( - blobBefore as Uint8Array, - ) + expect(await store.get(path)).toEqual(blobBefore as Uint8Array) + }) + + test('a manifest hash that is not a digest cannot build a path', async () => { + // A manifest is parsed JSON, not validated input, and its hashes now form + // storage paths. Without a shape check a hash carrying separators escapes + // the namespace directory, which is the one rule this repo states about + // anything reaching a path. + expect(() => contentPath('slug', 'sha256:../../other/blobs/deadbeef')).toThrow() + expect(() => contentPath('slug', 'not-a-hash')).toThrow() + // Control: a real digest still builds the path it should. + const good = 'a'.repeat(64) + expect(contentPath('slug', `sha256:${good}`)).toBe(`ns/slug/blobs/${good}`) + }) + + test('a reclaim failure does not fail a write that already published', async () => { + // Reclaim collects blobs no reader can see. Failing it must not turn a + // durable write into an error, and must not skip the caller's response. + const ns = 'user:reclaimfail' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('v1') }) + const real = getStore() + setStore({ + get: p => real.get(p), + put: (p, b) => real.put(p, b), + delete: async () => { + throw new Boom('reclaim delete failed') + }, + list: p => real.list(p), + describe: () => 'delete-hostile', + erasure: real.erasure, + }) + const r = await upsert(ns, slug, 'local', { entries: { 'a.md': b64('v2') } }, NOW) + resetStore() + expect(r.accepted).toEqual(['a.md']) + const after = await assertConsistent(ns) + expect(Buffer.from(after.content.entries['a.md'] as string, 'base64').toString()).toBe('v2') + }) + + test('a delete whose reclaim fails still reports the entry gone, and the bytes go later', async () => { + const ns = 'user:delreclaim' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('gone'), 'keep.md': b64('k') }) + const real = getStore() + const orphan = contentPath(slug, sha256Prefixed(new Uint8Array(Buffer.from('gone')))) + setStore({ + get: p => real.get(p), + put: (p, b) => real.put(p, b), + delete: async () => { + throw new Boom('reclaim delete failed') + }, + list: p => real.list(p), + describe: () => 'delete-hostile', + erasure: real.erasure, + }) + const r = await upsert(ns, slug, 'local', { entries: {}, deletions: ['a.md'] }, NOW) + resetStore() + expect(r.deleted).toEqual(['a.md']) + // The bytes survived that failure, which is exactly why reclaim sweeps by + // listing rather than by the keys a request touched: nothing can name this + // hash again, so a touched-key reclaim could never collect it. + expect(await getStore().get(orphan)).not.toBeNull() + + await upsert(ns, slug, 'local', { entries: { 'other.md': b64('o') } }, NOW) + expect(await getStore().get(orphan)).toBeNull() + }) + + test('a blob two entries share survives deleting one of them', async () => { + // The server takes ciphertext as opaque bytes, so a client can put the same + // ciphertext under two keys and they land on one content-addressed blob. + // Reclaim must not remove it while another entry still names that hash. + const ns = 'user:shared' + const slug = namespaceSlug(ns) + await seed(ns, { 'x.md': b64('same'), 'y.md': b64('same') }) + const store = getStore() + const shared = contentPath(slug, sha256Prefixed(new Uint8Array(Buffer.from('same')))) + expect(await store.get(shared)).not.toBeNull() + + await upsert(ns, slug, 'local', { entries: {}, deletions: ['x.md'] }, NOW) + const after = await assertConsistent(ns) + expect(Object.keys(after.content.entries)).toEqual(['y.md']) + expect(Buffer.from(after.content.entries['y.md'] as string, 'base64').toString()).toBe('same') + expect(await store.get(shared)).not.toBeNull() + }) + + test('a superseded blob is removed once nothing names it', async () => { + const ns = 'user:supersede' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('v1') }) + const store = getStore() + const oldPath = contentPath(slug, sha256Prefixed(new Uint8Array(Buffer.from('v1')))) + expect(await store.get(oldPath)).not.toBeNull() + + await upsert(ns, slug, 'local', { entries: { 'a.md': b64('v2') } }, NOW) + expect(await store.get(oldPath)).toBeNull() + }) + + test('a blob orphaned by a crashed write is reclaimed by the next write', async () => { + const ns = 'user:orphan' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('a1') }) + const real = getStore() + // Crash after the blob write, before the manifest write. + const f = faulty(real, 1) + setStore(f.store) + await upsert(ns, slug, 'local', { entries: { 'b.md': b64('b1') } }, NOW).catch(() => {}) + resetStore() + const store = getStore() + const orphan = contentPath(slug, sha256Prefixed(new Uint8Array(Buffer.from('b1')))) + expect(await store.get(orphan)).not.toBeNull() + + await upsert(ns, slug, 'local', { entries: { 'c.md': b64('c1') } }, NOW) + expect(await store.get(orphan)).toBeNull() }) test('a legacy-layout blob is read, and after one rewrite nothing is left at the legacy path', async () => { diff --git a/tests/erasure.test.ts b/tests/erasure.test.ts index 376b356..50cd2e0 100644 --- a/tests/erasure.test.ts +++ b/tests/erasure.test.ts @@ -14,6 +14,7 @@ import { getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' import type { BlobStore } from '../src/store/blobstore.ts' import { getStore, resetStore, setStore } from '../src/store/index.ts' +import { S3BlobStore } from '../src/store/s3.ts' const NOW = '2026-06-24T00:00:00.000Z' const b64 = (s: string) => Buffer.from(s).toString('base64') @@ -39,6 +40,7 @@ describe('erasure advertisement', () => { get: p => inner.get(p), put: (p, b) => inner.put(p, b), delete: p => inner.delete(p), + list: p => inner.list(p), describe: () => 'retaining', erasure: 'retains', } @@ -65,8 +67,20 @@ describe('erasure advertisement', () => { expect(incomplete.describe()).toBe('incomplete') }) - test('the real drivers both declare erasure', async () => { + test('both shipped drivers declare erasure', () => { resetStore() expect(getStore().erasure).toBe('erases') + // Read s3's declaration directly. Going through getStore() only ever + // reaches the filesystem driver under the test config, so flipping s3's + // value would not be observed -- and s3 is the driver the hosted service + // runs, which is exactly where a wrong declaration would matter. + const s3 = new S3BlobStore({ + bucket: 'b', + endpoint: '', + region: 'auto', + accessKeyId: 'k', + secretAccessKey: 's', + }) + expect(s3.erasure).toBe('erases') }) }) diff --git a/tests/health.test.ts b/tests/health.test.ts index 999b1de..3711691 100644 --- a/tests/health.test.ts +++ b/tests/health.test.ts @@ -42,6 +42,7 @@ describe('startup store probe', () => { seen.push(`delete ${p}`) return store.delete(p) }, + list: p => store.list(p), describe: () => store.describe(), erasure: store.erasure, }) @@ -62,6 +63,7 @@ describe('startup store probe', () => { throw new Error('connect ECONNREFUSED key=AKIAsecret bucket=private-bucket') }, delete: p => inner.delete(p), + list: p => inner.list(p), describe: () => 's3:private-bucket', erasure: 'erases', } as BlobStore) diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts index 56f73ef..f3adad7 100644 --- a/tests/rejection-log.test.ts +++ b/tests/rejection-log.test.ts @@ -71,7 +71,31 @@ describe('rejection log', () => { expect(JSON.stringify([{ ...lines[0], planted: sentinel }])).toContain(sentinel) }) - test('an unauthorized caller is logged with its code', async () => { + test('a throw from path parsing still returns an envelope and logs one line', async () => { + // A malformed percent escape throws in decodeURIComponent, which happens + // before respond()'s own try block. Without a guard in handleRequest that + // reaches the runtime unhandled: no envelope, no security headers, no line. + capture() + const res = await handleRequest(new Request('http://x/api/memory/%')) + expect(res.status).toBe(500) + expect(res.headers.get('x-content-type-options')).toBe('nosniff') + expect(((await res.json()) as { error: { code: string } }).error.code).toBe('internal') + expect(lines.length).toBe(1) + expect(Object.keys(lines[0] as object).sort()).toEqual([...ALLOWED_FIELDS].sort()) + expect(lines[0]?.code).toBe('internal') + }) + + test('an unknown route is logged as anonymous on the other route class', async () => { + capture() + const res = await handleRequest(new Request('http://x/nope')) + expect(res.status).toBe(404) + expect(lines.length).toBe(1) + expect(lines[0]?.owner).toBe('anonymous') + expect(lines[0]?.route).toBe('other') + expect(lines[0]?.code).toBe('not_found') + }) + + test('a method-not-allowed caller is logged with its code', async () => { capture() const res = await handleRequest( new Request('http://x/api/memory/user:local', { method: 'POST' }), diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index 2ae6a7f..9e55070 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -22,6 +22,7 @@ function stub(): BlobStore { get: async () => SENTINEL, put: async () => {}, delete: async () => {}, + list: async () => [], describe: () => 'stub', erasure: 'erases', } @@ -67,16 +68,22 @@ describe('store factory seam', () => { if (file !== join(root, 'src/store/index.ts')) { if (/\b(setStore|resetStore)\b/.test(src)) offenders.push(file.slice(root.length + 1)) } - for (const m of src.matchAll(/from\s+'(\.[^']+)'/g)) { + // Static imports and dynamic import() alike: bin/memlawb.ts reaches the + // server and the MCP entry through import(), so a walk that only follows + // `from '...'` would miss the real entrypoint entirely. + for (const m of src.matchAll(/(?:from|import)\s*\(?\s*'(\.[^']+)'/g)) { await walk(resolve(dirname(file), m[1] as string)) } } await walk(join(root, 'src/index.ts')) await walk(join(root, 'src/mcp/server.ts')) + await walk(join(root, 'bin/memlawb.ts')) - // Positive control: the walk actually reached the modules it claims to cover. - expect(seen.size).toBeGreaterThan(8) + // Positive control: the walk reached the modules it claims to cover. The + // floor tracks the real count rather than a guess, so a walk that silently + // stopped following imports fails here instead of reporting no offenders. + expect(seen.size).toBeGreaterThanOrEqual(20) expect([...seen].some(f => f.endsWith('src/store/index.ts'))).toBe(true) expect(offenders).toEqual([]) }) From 4f9f450907f35334c560807dae269fc92fe380b6 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:09:22 -0500 Subject: [PATCH 09/14] fix(memory): stop one bad manifest hash from failing a whole read Round 2 of review, against the previous commit. Six of its seven fixes held under mutation; two things did not. Making contentPath throw was right for the write path and wrong for the read one: getData called it unguarded, three lines above its own comment about skipping rather than 500ing, so a single non-digest hash took the entire namespace's read with it where before it skipped one entry. It now skips that entry, with a control proving the healthy ones still read. The store-seam walk change was inert. Following import() adds no reach today because bin/memlawb.ts's only dynamic targets are roots the walk already had, and the floor of 20 sat below the real count of 25, so neither half could fail. The count is now exact and the comment no longer claims something untrue: a dropped root turns it red. Also from that round: reclaim resolves the store inside its try so "never throws" is structural rather than nearly-true, its failure line names the namespace so an operator can act on it, and its doc records what it costs and that it is single-instance for the same reason the lock is. s3's list had no coverage at all while the same commit asserted s3's erasure, which is backwards for the driver the hosted service runs. It now has a fake client proving pagination follows the continuation token and stops. Not covered, stated rather than implied: the nsSlug field on the reclaim failure line. That path writes to stderr directly rather than through the injectable sink. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/memory.ts | 28 +++++++++++++--- tests/atomic-visibility.test.ts | 20 +++++++++++ tests/store-s3.test.ts | 59 +++++++++++++++++++++++++++++++++ tests/store-seam.test.ts | 16 +++++---- 4 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 tests/store-s3.test.ts diff --git a/src/memory.ts b/src/memory.ts index caf37fc..4828b57 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -89,9 +89,15 @@ export async function getData(namespace: string, nsSlug: string): Promise { - const bytes = - (await store.get(contentPath(nsSlug, meta.hash))) ?? - (await store.get(entryPath(nsSlug, sha256Hex(key)))) + let bytes: Uint8Array | null = null + try { + bytes = (await store.get(contentPath(nsSlug, meta.hash))) ?? null + } catch { + // A hash that is not a digest cannot name a blob. Skip this entry the + // same way a missing one is skipped: one unreadable entry must not take + // the whole namespace's read down with it. + } + bytes ??= await store.get(entryPath(nsSlug, sha256Hex(key))) if (!bytes) return // manifest/blob drift — skip rather than 500 entries[key] = Buffer.from(bytes).toString('base64') entryChecksums[key] = meta.hash @@ -265,6 +271,16 @@ export async function upsert( * Never throws. This runs after the write is durable and collects garbage that * is already invisible to every reader, so a store hiccup here must not turn a * landed write into a failure, nor skip the caller's quota accounting. + * + * Single-instance only, like the lock it runs under. The sweep deletes anything + * under the namespace's blob prefix that the manifest it just wrote does not + * name, which is correct while one process serializes every write to that + * namespace and silent data loss the moment two do. It moves behind the same + * row-version check as the lock when that lands (see lock.ts and PLAN §7). + * + * Costs one LIST per mutating write, deliberately: it replaces a delete per + * touched key, most of which were no-ops, and it is the only way to see an + * orphan no key can name. */ async function reclaim( nsSlug: string, @@ -272,8 +288,8 @@ async function reclaim( prev: Record, touched: Set, ): Promise { - const store = getStore() try { + const store = getStore() const live = new Set() for (const meta of Object.values(m.entries)) live.add(contentPath(nsSlug, meta.hash)) for (const path of await store.list(blobPrefix(nsSlug))) { @@ -290,6 +306,10 @@ async function reclaim( `${JSON.stringify({ timestamp: new Date().toISOString(), event: 'reclaim_failed', + // The slug is already every storage path's own directory name, so it + // discloses nothing new, and it is the only field that makes this line + // actionable: without it an operator knows collection failed somewhere. + nsSlug, reason: (err as Error)?.constructor?.name ?? 'unknown', })}\n`, ) diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index 4113c2f..b44212e 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -198,6 +198,26 @@ describe('crash visibility across the commit sequence', () => { expect(contentPath('slug', `sha256:${good}`)).toBe(`ns/slug/blobs/${good}`) }) + test('a manifest entry with a non-digest hash is skipped, not fatal', async () => { + // contentPath throws on a hash that cannot name a blob, which is right on + // the write path. On the read path one corrupt entry must not take the + // namespace with it, so getData skips it the way it skips a missing blob. + const ns = 'user:badhash' + const slug = namespaceSlug(ns) + await seed(ns, { 'ok.md': b64('fine') }) + const store = getStore() + const raw = await store.get(`ns/${slug}/manifest.json`) + const m = JSON.parse(new TextDecoder().decode(raw as Uint8Array)) + m.entries['bad.md'] = { hash: 'sha256:NOTHEX', size: 4, updatedAt: NOW } + await store.put(`ns/${slug}/manifest.json`, new TextEncoder().encode(JSON.stringify(m))) + + const data = await getData(ns, slug) + // Control: the healthy entry still reads, so the skip is targeted rather + // than the whole view collapsing. + expect(Buffer.from(data.content.entries['ok.md'] as string, 'base64').toString()).toBe('fine') + expect(data.content.entries['bad.md']).toBeUndefined() + }) + test('a reclaim failure does not fail a write that already published', async () => { // Reclaim collects blobs no reader can see. Failing it must not turn a // durable write into an error, and must not skip the caller's response. diff --git a/tests/store-s3.test.ts b/tests/store-s3.test.ts new file mode 100644 index 0000000..ee3b890 --- /dev/null +++ b/tests/store-s3.test.ts @@ -0,0 +1,59 @@ +/** + * S3BlobStore's listing (U2 fix pass). + * + * s3 is the driver the hosted service runs, and reclaim now depends on list() + * to find blobs no manifest names. A list that silently returned only its first + * page would leave ciphertext behind on exactly the deployment where that + * matters, and no other test in the suite reaches this driver. + */ + +import { describe, expect, test } from 'bun:test' +import { S3BlobStore } from '../src/store/s3.ts' + +type Page = { contents?: { key?: string }[]; isTruncated?: boolean; nextContinuationToken?: string } + +function withFakeClient(pages: Page[]) { + const store = new S3BlobStore({ + bucket: 'b', + endpoint: '', + region: 'auto', + accessKeyId: 'k', + secretAccessKey: 's', + }) + const seen: (string | undefined)[] = [] + let i = 0 + // biome-ignore lint/suspicious/noExplicitAny: reaching past the private client is the point + ;(store as any).client = { + list: async (opts: { prefix: string; continuationToken?: string }) => { + seen.push(opts.continuationToken) + return pages[i++] ?? { contents: [] } + }, + } + return { store, seen } +} + +describe('S3BlobStore.list', () => { + test('follows continuation tokens across pages and stops when untruncated', async () => { + const { store, seen } = withFakeClient([ + { contents: [{ key: 'ns/a/blobs/1' }], isTruncated: true, nextContinuationToken: 'tok1' }, + { contents: [{ key: 'ns/a/blobs/2' }], isTruncated: false }, + ]) + expect(await store.list('ns/a/blobs/')).toEqual(['ns/a/blobs/1', 'ns/a/blobs/2']) + // Control: the second request carried the first page's token, so the walk + // genuinely paginated rather than being handed both pages at once. + expect(seen).toEqual([undefined, 'tok1']) + }) + + test('a single untruncated page makes exactly one request', async () => { + const { store, seen } = withFakeClient([ + { contents: [{ key: 'ns/a/blobs/1' }], isTruncated: false }, + ]) + expect(await store.list('ns/a/blobs/')).toEqual(['ns/a/blobs/1']) + expect(seen.length).toBe(1) + }) + + test('an empty prefix yields no paths', async () => { + const { store } = withFakeClient([{ contents: [], isTruncated: false }]) + expect(await store.list('ns/a/blobs/')).toEqual([]) + }) +}) diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index 9e55070..1e59bdb 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -68,9 +68,10 @@ describe('store factory seam', () => { if (file !== join(root, 'src/store/index.ts')) { if (/\b(setStore|resetStore)\b/.test(src)) offenders.push(file.slice(root.length + 1)) } - // Static imports and dynamic import() alike: bin/memlawb.ts reaches the - // server and the MCP entry through import(), so a walk that only follows - // `from '...'` would miss the real entrypoint entirely. + // Static imports and dynamic import() alike. Following import() adds no + // reach today, since bin/memlawb.ts's only dynamic targets are the two + // roots below; it is here so the walk does not silently stop covering + // them if those roots are ever dropped. for (const m of src.matchAll(/(?:from|import)\s*\(?\s*'(\.[^']+)'/g)) { await walk(resolve(dirname(file), m[1] as string)) } @@ -80,10 +81,11 @@ describe('store factory seam', () => { await walk(join(root, 'src/mcp/server.ts')) await walk(join(root, 'bin/memlawb.ts')) - // Positive control: the walk reached the modules it claims to cover. The - // floor tracks the real count rather than a guess, so a walk that silently - // stopped following imports fails here instead of reporting no offenders. - expect(seen.size).toBeGreaterThanOrEqual(20) + // Positive control: the walk reached every module it claims to cover. This + // is an exact count, not a floor, because a floor is what let an earlier + // version of this test lose reach without failing: any module added to or + // dropped from the production graph should force a look at this number. + expect(seen.size).toBe(25) expect([...seen].some(f => f.endsWith('src/store/index.ts'))).toBe(true) expect(offenders).toEqual([]) }) From 30228a719a297b5d52f3fbe5c5781ec255235944 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:48:41 -0500 Subject: [PATCH 10/14] fix(api): close the remaining review findings on the write contract One predicate now decides whether a base hash is well formed, and both write verbs use it. Before, only DELETE checked the shape, so the same malformed value answered 400 there and 409 on PUT: the garbage reached the manifest comparison, never matched, and reported a conflict. A caller reading "someone wrote under you" would re-read and retry the same bad value forever. An unreadable manifest gets its own error and a 503 with code manifest_unreadable, on reads as well as writes. Refusing is right, but a generic 500 tells a caller to retry what no retry can fix and leaves an operator unable to tell it from any other fault. The rate limiter now runs before the refusal branches, keyed on the caller when there is one and a shared anonymous bucket when there is not. The unknown-route and unauthorized branches sat ahead of it, and every refusal writes a log line, so an unauthenticated caller could turn a trivially cheap request into unbounded log volume on the machine holding every tenant's ciphertext. Keying everything on the shared bucket would have let that abuse throttle real accounts, which is why the key is a named rule with its own test rather than an inline expression. The startup probe carries a deadline. Neither adapter sets a socket timeout, so a hung connect left startup pending forever and the failure line this module exists to produce was never printed. Smaller: the full view carries erasure like the other two surfaces, so a client that only pulls can still see it; a key deleted and re-added in one request is reported only as accepted, since naming it in both arrays tells a client mirroring deleted to drop a file the same response stored; the probe's byte-comparison branch, s3 list pagination, and the context defaults now have tests; and both files that install a store override reset it in afterEach, because bun shares one process and a failure before the inline reset leaked a stub into every later suite. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 61 ++++++++++++++++++----- src/memory.ts | 9 +++- src/store/probe.ts | 25 ++++++++-- src/types.ts | 26 +++++++++- tests/atomic-visibility.test.ts | 4 +- tests/base-precondition.test.ts | 85 +++++++++++++++++++++++++++++++-- tests/erasure.test.ts | 6 ++- tests/health.test.ts | 56 ++++++++++++++++++++-- tests/rejection-log.test.ts | 30 ++++++++++-- tests/store-seam.test.ts | 6 ++- 10 files changed, 274 insertions(+), 34 deletions(-) diff --git a/src/handler.ts b/src/handler.ts index b1c99a1..f28e8d2 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -26,10 +26,7 @@ import { } from './namespace.ts' import { QuotaError } from './quota.ts' import { take } from './ratelimit.ts' -import { parseUpsertRequest, StaleBaseError } from './types.ts' - -/** The shape a DELETE `base` query value must take. */ -const BASE_HASH_RE = /^sha256:[0-9a-f]{64}$/ +import { isBaseHash, parseUpsertRequest, StaleBaseError, UnreadableManifestError } from './types.ts' // Applied to every response. The API serves only JSON and is consumed by // programmatic clients, so we lock down sniffing/caching/referrer leakage. @@ -76,13 +73,24 @@ function upsertFailure(err: unknown): Response | null { return null } +/** + * A namespace whose index cannot be parsed answers 503 with its own code rather + * than a generic 500, on reads as well as writes. The refusal is right, but + * "internal error" tells a caller to retry something no retry can fix, and + * leaves an operator unable to tell this apart from any other server fault. + */ +function manifestFailure(err: unknown): Response | null { + if (err instanceof UnreadableManifestError) return apiError(err.code, err.message, 503) + return null +} + /** * Log every refusal from one place, after the response is built, so the code * recorded is literally the code the caller received and no future refusal * branch can be added without being covered. */ export async function handleRequest(req: Request): Promise { - const ctx: RequestContext = { owner: 'anonymous', route: 'other' } + const ctx: RequestContext = { ...DEFAULT_CONTEXT } // respond() parses the path before its own try block, so a malformed percent // escape throws past it. Without this guard that reaches the runtime as an // unhandled error: no envelope, no security headers, and no log line, which @@ -110,6 +118,27 @@ export async function handleRequest(req: Request): Promise { /** Per-request facts the rejection log needs, filled in as they become known. */ type RequestContext = { owner: string; route: string } +/** + * What a request is assumed to be before anything is known about it. Exported + * so the defaults are pinned somewhere: under an open-auth configuration every + * caller authenticates, so no test driving the handler can observe them. + */ +export const DEFAULT_CONTEXT: Readonly = Object.freeze({ + owner: 'anonymous', + route: 'other', +}) + +/** + * Which token bucket a request draws from: its own account when it + * authenticated, one shared anonymous bucket when it did not. Keying everything + * on the shared bucket would let anonymous abuse throttle real accounts; keying + * nothing on it leaves the pre-auth refusal branches, which now write a log + * line each, unthrottled entirely. + */ +export function bucketKey(identity: { owner: string } | null): string { + return identity?.owner ?? 'anonymous' +} + async function respond(req: Request, ctx: RequestContext): Promise { const url = new URL(req.url) const { pathname } = url @@ -119,20 +148,28 @@ async function respond(req: Request, ctx: RequestContext): Promise { } const parsed = parseMemoryPath(pathname) - if (!parsed) return apiError('not_found', 'unknown route', 404) - ctx.route = 'memory' + // Authenticate before refusing anything, so the throttle below can key on the + // caller when there is one. Every refusal writes a log line, and the unknown + // route and unauthorized branches used to sit ahead of the bucket entirely, + // which let an unauthenticated caller turn a trivially cheap request into + // unbounded log volume on the machine holding every tenant's ciphertext. const identity = await authenticate(req) - if (!identity) return apiError('unauthorized', 'missing or invalid API key', 401) - ctx.owner = identity.owner + ctx.owner = bucketKey(identity) - const rate = take(identity.owner, Date.now()) + // One shared bucket for callers who did not authenticate, their own bucket + // for those who did, so anonymous abuse cannot throttle a real account. + const rate = take(ctx.owner, Date.now()) if (!rate.ok) { return apiError('rate_limited', 'too many requests', 429, undefined, { 'retry-after': String(rate.retryAfterSec), }) } + if (!parsed) return apiError('not_found', 'unknown route', 404) + ctx.route = 'memory' + if (!identity) return apiError('unauthorized', 'missing or invalid API key', 401) + let namespace: string try { namespace = validateNamespace(parsed.namespace) @@ -203,7 +240,7 @@ async function respond(req: Request, ctx: RequestContext): Promise { // The base rides the query here rather than a body, so it needs its own // shape check: parseUpsertRequest never sees a DELETE. const rawBase = url.searchParams.get('base') - if (rawBase !== null && !BASE_HASH_RE.test(rawBase)) { + if (rawBase !== null && !isBaseHash(rawBase)) { return apiError('bad_request', '`base` must be a sha256: hash', 400) } const base = rawBase === null ? undefined : { [key]: rawBase } @@ -225,6 +262,8 @@ async function respond(req: Request, ctx: RequestContext): Promise { return apiError('method_not_allowed', `${req.method} not supported`, 405) } catch (err) { + const manifest = manifestFailure(err) + if (manifest) return manifest // The error's class only. A store error commonly carries an endpoint, a // bucket and an object path, and that path carries a namespace slug. console.error(`[memlawb] handler error (${(err as Error)?.constructor?.name ?? 'unknown'})`) diff --git a/src/memory.ts b/src/memory.ts index 4828b57..95dddfb 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -26,6 +26,7 @@ import { type MemoryData, type MemoryHashes, StaleBaseError, + UnreadableManifestError, type UpsertRequest, type UpsertResponse, } from './types.ts' @@ -49,7 +50,7 @@ async function readManifest(nsSlug: string): Promise { // now be destructive: the commit path reclaims blobs the new manifest does // not reference, so an empty manifest would delete every live entry. Refuse // the write and let an operator look. - throw new Error(`unreadable manifest for namespace slug ${nsSlug}`) + throw new UnreadableManifestError() } } @@ -109,6 +110,7 @@ export async function getData(namespace: string, nsSlug: string): Promise !(k in m.entries)), skipped, } }) diff --git a/src/store/probe.ts b/src/store/probe.ts index 55cab40..ab4a7eb 100644 --- a/src/store/probe.ts +++ b/src/store/probe.ts @@ -20,19 +20,36 @@ export const PROBE_PREFIX = 'probe/' export type ProbeResult = { ok: boolean; detail?: string } +/** + * How long the probe waits before calling the store unreachable. Neither + * adapter sets a socket timeout, so without this a hung connect leaves startup + * pending forever: the operator never sees the failure line this module exists + * to produce, and the deployment's health check has nothing to talk to. + */ +const PROBE_TIMEOUT_MS = 5_000 + +function withDeadline(work: Promise, ms: number): Promise { + return Promise.race([ + work, + new Promise((_, reject) => + setTimeout(() => reject(new Error('store probe timed out')), ms).unref?.(), + ), + ]) +} + /** * Write, read back, compare, and remove one object. Returns rather than throws * so the caller decides what a failure means. The failure detail is the error's * class, never its message: a store error commonly carries an endpoint, a * bucket and an object path, and that path carries a namespace slug. */ -export async function probeStore(): Promise { +export async function probeStore(timeoutMs = PROBE_TIMEOUT_MS): Promise { const store = getStore() const path = `${PROBE_PREFIX}${randomUUID()}` const payload = new TextEncoder().encode(randomUUID()) try { - await store.put(path, payload) - const read = await store.get(path) + await withDeadline(store.put(path, payload), timeoutMs) + const read = await withDeadline(store.get(path), timeoutMs) if (!read || Buffer.compare(Buffer.from(read), Buffer.from(payload)) !== 0) { return { ok: false, detail: 'store round trip returned different bytes' } } @@ -40,6 +57,6 @@ export async function probeStore(): Promise { } catch (err) { return { ok: false, detail: `store unreachable (${(err as Error).constructor.name})` } } finally { - await store.delete(path).catch(() => {}) + await withDeadline(store.delete(path), timeoutMs).catch(() => {}) } } diff --git a/src/types.ts b/src/types.ts index 3ad156c..69979cb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -35,6 +35,8 @@ export function emptyManifest(): Manifest { /** Full GET response. */ export type MemoryData = { + /** Whether this deployment's store actually erases on delete. */ + erasure: Erasure namespace: string version: number lastModified: string @@ -104,6 +106,26 @@ export type UpsertResponse = { skipped: { key: string; reason: string }[] } +/** + * The shape a base hash must take, in one place because both write verbs check + * it. When only DELETE checked, the same malformed value produced 400 there and + * 409 on PUT: the garbage reached the manifest comparison, never matched, and + * reported a conflict. A caller reading "someone wrote under you" retries the + * same bad value forever. + */ +export function isBaseHash(v: string): boolean { + return /^sha256:[0-9a-f]{64}$/.test(v) +} + +/** Thrown when a namespace's manifest cannot be parsed. Surfaces as HTTP 503. */ +export class UnreadableManifestError extends Error { + readonly code = 'manifest_unreadable' + constructor() { + super('namespace index cannot be read') + this.name = 'UnreadableManifestError' + } +} + /** Structured error body. */ export type ApiError = { error: { code: string; message: string; details?: Record } @@ -132,8 +154,8 @@ export function parseUpsertRequest(raw: unknown): UpsertRequest { throw new Error('`base` must be an object map of key -> hash or null') } for (const [k, v] of Object.entries(obj.base)) { - if (v !== null && typeof v !== 'string') { - throw new Error(`base ${JSON.stringify(k)} must be a hash string or null`) + if (v !== null && (typeof v !== 'string' || !isBaseHash(v))) { + throw new Error(`base ${JSON.stringify(k)} must be a sha256: hash or null`) } } base = obj.base as Record diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index b44212e..9043d79 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -17,6 +17,7 @@ import { getData, getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' import type { BlobStore } from '../src/store/blobstore.ts' import { contentPath, getStore, resetStore, setStore } from '../src/store/index.ts' +import { UnreadableManifestError } from '../src/types.ts' const NOW = '2026-06-24T00:00:00.000Z' const b64 = (s: string) => Buffer.from(s).toString('base64') @@ -180,8 +181,7 @@ describe('crash visibility across the commit sequence', () => { const err = await upsert(ns, slug, 'local', { entries: { 'b.md': b64('b1') } }, NOW).catch( e => e, ) - expect(err).toBeInstanceOf(Error) - expect(String((err as Error).message)).toContain('manifest') + expect(err).toBeInstanceOf(UnreadableManifestError) // Control: the pre-existing blob is still there, so nothing was reclaimed. expect(await store.get(path)).toEqual(blobBefore as Uint8Array) }) diff --git a/tests/base-precondition.test.ts b/tests/base-precondition.test.ts index 90afc39..9766d82 100644 --- a/tests/base-precondition.test.ts +++ b/tests/base-precondition.test.ts @@ -16,9 +16,11 @@ import { afterEach, describe, expect, test } from 'bun:test' import { handleRequest } from '../src/handler.ts' import { getData, getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' -import { resetStore } from '../src/store/index.ts' +import { getStore, resetStore } from '../src/store/index.ts' import { StaleBaseError } from '../src/types.ts' +const WRONG = `sha256:${'0'.repeat(64)}` + const NOW = '2026-06-24T00:00:00.000Z' const b64 = (s: string) => Buffer.from(s).toString('base64') const put = (ns: string, req: Parameters[3]) => @@ -37,7 +39,7 @@ describe('base precondition', () => { await put(ns, { entries: { 'a.md': b64('v1') } }) const err = await put(ns, { entries: { 'a.md': b64('v2') }, - base: { 'a.md': 'sha256:0000' }, + base: { 'a.md': WRONG }, }).catch(e => e) expect(err).toBeInstanceOf(StaleBaseError) @@ -72,6 +74,19 @@ describe('base precondition', () => { expect(ok.accepted).toEqual(['b.md']) }) + test('a key deleted and re-added in one request is reported only as accepted', async () => { + // Reporting it in both arrays tells a client mirroring `deleted` to drop a + // file the same response says it stored. + const ns = 'user:readd' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const r = await put(ns, { entries: { 'a.md': b64('v2') }, deletions: ['a.md'] }) + expect(r.accepted).toEqual(['a.md']) + expect(r.deleted).toEqual([]) + // Control: a plain deletion in the same shape still reports it. + const d = await put(ns, { entries: {}, deletions: ['a.md'] }) + expect(d.deleted).toEqual(['a.md']) + }) + test('a request with no base is accepted unconditionally', async () => { const ns = 'user:nobase' await put(ns, { entries: { 'a.md': b64('v1') } }) @@ -85,7 +100,7 @@ describe('base precondition', () => { const err = await put(ns, { entries: {}, deletions: ['a.md'], - base: { 'a.md': 'sha256:0000' }, + base: { 'a.md': WRONG }, }).catch(e => e) expect(err).toBeInstanceOf(StaleBaseError) @@ -101,7 +116,9 @@ describe('base precondition', () => { const ns = 'user:cap' await put(ns, { entries: { 'a.md': b64('v1') } }) const h = await getHashes(ns, namespaceSlug(ns)) - expect(h.supports).toContain('base-precondition') + // Exact, not toContain: a capability the server does not implement must not + // be advertisable just because the real one is also present. + expect(h.supports).toEqual(['base-precondition']) }) }) @@ -115,7 +132,7 @@ describe('base precondition over the wire', () => { new Request(url(ns), { method: 'PUT', headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ entries: { 'a.md': b64('v2') }, base: { 'a.md': 'sha256:0000' } }), + body: JSON.stringify({ entries: { 'a.md': b64('v2') }, base: { 'a.md': WRONG } }), }), ) expect(res.status).toBe(409) @@ -136,6 +153,64 @@ describe('base precondition over the wire', () => { ) }) + test('an unreadable manifest answers 503 with its own code, on reads too', async () => { + // A generic 500 tells a caller to retry something no retry can fix, and + // leaves an operator unable to tell this from any other server fault. + const ns = 'user:corruptwire' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const store = getStore() + await store.put(`ns/${namespaceSlug(ns)}/manifest.json`, new TextEncoder().encode('{not json')) + for (const path of [url(ns), `${url(ns)}?view=hashes`]) { + const res = await handleRequest(new Request(path)) + expect(res.status).toBe(503) + expect(((await res.json()) as { error: { code: string } }).error.code).toBe( + 'manifest_unreadable', + ) + } + }) + + test('a malformed base string is 400 on PUT, not a phantom conflict', async () => { + // The value below is a string, so a type-only check lets it through to the + // manifest comparison, where it can never match and always reports a + // conflict. A caller then reads "someone wrote under you" and retries the + // same bad value forever. Both verbs must call it what it is. + const ns = 'user:badstr' + await put(ns, { entries: { 'a.md': b64('v1') } }) + const res = await handleRequest( + new Request(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: {}, base: { 'a.md': 'not-a-hash' } }), + }), + ) + expect(res.status).toBe(400) + // Control: the same request with a well-formed hash reaches the comparison + // and reports a conflict, so 400 here is about shape rather than staleness. + const conflict = await handleRequest( + new Request(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: {}, base: { 'a.md': WRONG } }), + }), + ) + expect(conflict.status).toBe(409) + }) + + test('a base that is not an object at all is 400', async () => { + const ns = 'user:badbaseshape' + await put(ns, { entries: { 'a.md': b64('v1') } }) + for (const bad of [[], 'x', 7]) { + const res = await handleRequest( + new Request(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: {}, base: bad }), + }), + ) + expect(res.status).toBe(400) + } + }) + test('a malformed base is 400 on both PUT and DELETE', async () => { const ns = 'user:badbase' await put(ns, { entries: { 'a.md': b64('v1') } }) diff --git a/tests/erasure.test.ts b/tests/erasure.test.ts index 50cd2e0..8e0a4d2 100644 --- a/tests/erasure.test.ts +++ b/tests/erasure.test.ts @@ -10,7 +10,7 @@ */ import { afterEach, describe, expect, test } from 'bun:test' -import { getHashes, upsert } from '../src/memory.ts' +import { getData, getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' import type { BlobStore } from '../src/store/blobstore.ts' import { getStore, resetStore, setStore } from '../src/store/index.ts' @@ -32,6 +32,10 @@ describe('erasure advertisement', () => { expect(h.erasure).toBe('erases') const d = await put(ns, { entries: {}, deletions: ['a.md'] }) expect(d.erasure).toBe('erases') + // A client that only ever pulls still needs to know, so the full view + // carries it too rather than only the hashes view and write responses. + await put(ns, { entries: { 'b.md': b64('x') } }) + expect((await getData(ns, namespaceSlug(ns))).erasure).toBe('erases') }) test('a retaining store reports retaining on both surfaces', async () => { diff --git a/tests/health.test.ts b/tests/health.test.ts index 3711691..b69027b 100644 --- a/tests/health.test.ts +++ b/tests/health.test.ts @@ -8,14 +8,19 @@ * process from binding at all. */ -import { describe, expect, test } from 'bun:test' +import { afterEach, describe, expect, test } from 'bun:test' import { handleRequest } from '../src/handler.ts' import { usagePath } from '../src/quota.ts' import type { BlobStore } from '../src/store/blobstore.ts' -import { entryPath, manifestPath } from '../src/store/blobstore.ts' +import { contentPath, entryPath, manifestPath } from '../src/store/blobstore.ts' import { getStore, resetStore, setStore } from '../src/store/index.ts' import { PROBE_PREFIX, probeStore } from '../src/store/probe.ts' +// setStore installs a process-wide override, and bun shares one process across +// test files. Without this an assertion failing before an inline resetStore() +// leaks a stub into every later suite. +afterEach(() => resetStore()) + describe('health route', () => { test('reports liveness and nothing about the store', async () => { const res = await handleRequest(new Request('http://x/health')) @@ -74,11 +79,56 @@ describe('startup store probe', () => { expect(r.detail).not.toContain('private-bucket') }) + test('a store that returns different bytes fails the probe', async () => { + // A round trip that writes and reads without comparing proves the store + // answered, not that it stored. This branch is the comparison. + const inner = getStore() + setStore({ + get: async () => new TextEncoder().encode('wrong'), + put: (p, b) => inner.put(p, b), + delete: p => inner.delete(p), + list: p => inner.list(p), + describe: () => 'liar', + erasure: 'erases', + }) + const r = await probeStore() + resetStore() + expect(r.ok).toBe(false) + expect(r.detail).toContain('different bytes') + }) + + test('a store that never answers fails the probe instead of hanging startup', async () => { + // Neither adapter sets a socket timeout, so without a deadline a hung + // connect leaves startup pending forever and the operator never sees the + // failure line this module exists to produce. + const inner = getStore() + setStore({ + get: p => inner.get(p), + put: () => new Promise(() => {}), + delete: p => inner.delete(p), + list: p => inner.list(p), + describe: () => 'hangs', + erasure: 'erases', + }) + const started = Date.now() + const r = await probeStore(50) + resetStore() + expect(r.ok).toBe(false) + // Control: it returned because of the deadline, not because the store + // answered, and it did not wait the production timeout to do it. + expect(Date.now() - started).toBeLessThan(2000) + }) + test('the probe prefix is disjoint from every tenant path prefix', () => { // Tenants supply namespaces, never store paths, so asserting the validators // reject the prefix would prove the wrong thing and could not fail. Assert // against the prefixes the path builders actually produce. - const tenantPaths = [manifestPath('deadbeef'), entryPath('deadbeef', 'abc'), usagePath('alice')] + const tenantPaths = [ + manifestPath('deadbeef'), + entryPath('deadbeef', 'abc'), + contentPath('deadbeef', `sha256:${'a'.repeat(64)}`), + usagePath('alice'), + ] // Control: those really are the shapes tenant data takes. expect(tenantPaths.some(p => p.startsWith('ns/'))).toBe(true) expect(tenantPaths.some(p => p.startsWith('owners/'))).toBe(true) diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts index f3adad7..2a088d4 100644 --- a/tests/rejection-log.test.ts +++ b/tests/rejection-log.test.ts @@ -11,7 +11,7 @@ */ import { afterEach, describe, expect, test } from 'bun:test' -import { handleRequest } from '../src/handler.ts' +import { bucketKey, DEFAULT_CONTEXT, handleRequest } from '../src/handler.ts' import { ALLOWED_FIELDS, setRejectionSink } from '../src/log.ts' import { _reset } from '../src/ratelimit.ts' @@ -85,14 +85,38 @@ describe('rejection log', () => { expect(lines[0]?.code).toBe('internal') }) - test('an unknown route is logged as anonymous on the other route class', async () => { + test('an unknown route is logged on the other route class', async () => { capture() const res = await handleRequest(new Request('http://x/nope')) expect(res.status).toBe(404) expect(lines.length).toBe(1) - expect(lines[0]?.owner).toBe('anonymous') expect(lines[0]?.route).toBe('other') expect(lines[0]?.code).toBe('not_found') + // The owner is 'local' here, not 'anonymous': tests/setup.ts pins + // ALLOW_UNAUTHENTICATED process-wide, so authenticate always returns an + // identity. The 'anonymous' default is only reachable with auth required, + // which this harness cannot express, so it is asserted at unit level below. + expect(lines[0]?.owner).toBe('local') + }) + + test('an authenticated caller draws from its own bucket, not the shared one', () => { + // Not drivable through the handler here: the harness authenticates every + // caller as the same owner, so a shared bucket and a per-owner one behave + // identically. Pin the rule where it is decided. + expect(bucketKey({ owner: 'alice' })).toBe('alice') + expect(bucketKey({ owner: 'bob' })).toBe('bob') + // Control: an unauthenticated caller shares one bucket rather than getting + // an unthrottled path. + expect(bucketKey(null)).toBe('anonymous') + }) + + test('the request context defaults to an anonymous caller on the other route', () => { + // The defaults above cannot be driven through the handler under this + // harness, so pin them where they are set. Control: both differ from the + // values the handler overwrites them with. + expect(DEFAULT_CONTEXT).toEqual({ owner: 'anonymous', route: 'other' }) + expect(DEFAULT_CONTEXT.owner).not.toBe('local') + expect(DEFAULT_CONTEXT.route).not.toBe('memory') }) test('a method-not-allowed caller is logged with its code', async () => { diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index 1e59bdb..8649bde 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -9,7 +9,7 @@ * the server actually loads reaches for it. */ -import { describe, expect, test } from 'bun:test' +import { afterEach, describe, expect, test } from 'bun:test' import { readFile } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' import type { BlobStore } from '../src/store/blobstore.ts' @@ -28,6 +28,10 @@ function stub(): BlobStore { } } +// A failing assertion before an inline resetStore() would otherwise leak this +// file's stub into every later suite in the shared process. +afterEach(() => resetStore()) + describe('store factory seam', () => { test('setStore installs an instance getStore then returns', async () => { setStore(stub()) From 341dfcba4f459c3ed97b32ea718fdabb12a20fad Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:49:50 -0500 Subject: [PATCH 11/14] docs: describe the contract this branch actually serves The README's API table still advertised a `/health` field this branch removed and a PUT body without `base`, and RELEASE-0.1 still listed the store round trip as pending work on `/health` when it moved to a startup probe. The README is the published contract, so a reader building a monitor against it would key on a field the server no longer sends. Also documents what the write path gained: the optional `base` precondition and its 409, the `supports` and `erasure` fields, and the 503 a namespace answers when its index cannot be parsed. Test headers cited plan identifiers that resolve only inside docs/plans, which is in .git/info/exclude. For anyone reading this repository those pointed at nothing, so each header now states the property in plain English instead. BREAKING CHANGE: `GET /health` no longer returns `store`, and a namespace whose manifest cannot be parsed now answers 503 `manifest_unreadable` where it previously served an empty view. Without this footer release-please would cut a patch for a changed contract, because `bump-patch-for-minor-pre-major` is set. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- README.md | 20 +++++++++++++++++--- RELEASE-0.1.md | 5 ++++- tests/atomic-visibility.test.ts | 2 +- tests/base-precondition.test.ts | 2 +- tests/erasure.test.ts | 4 ++-- tests/health.test.ts | 2 +- tests/rejection-log.test.ts | 4 ++-- tests/store-s3.test.ts | 2 +- tests/store-seam.test.ts | 4 ++-- 9 files changed, 31 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 5f0337f..c8fe3b9 100644 --- a/README.md +++ b/README.md @@ -167,15 +167,29 @@ All bodies are ciphertext; the server validates sizes/hashes without decrypting. | Method | Route | Purpose | |---|---|---| -| `GET` | `/health` | liveness + active store | +| `GET` | `/health` | liveness | | `GET` | `/api/memory/:ns` | full data (ciphertext entries + checksums) | | `GET` | `/api/memory/:ns?view=hashes` | per-key checksums only (for delta) | -| `PUT` | `/api/memory/:ns` | delta upsert `{ entries, deletions? }` | -| `DELETE` | `/api/memory/:ns?key=` | remove one entry | +| `PUT` | `/api/memory/:ns` | delta upsert `{ entries, deletions?, base? }` | +| `DELETE` | `/api/memory/:ns?key=[&base=sha256:]` | remove one entry | A *namespace* (`user:me`, `repo:owner/name`, `agent:intern`) is the unit of scoping. Entry keys mirror the memdir layout (`MEMORY.md`, `feedback/x.md`). +The hashes view also reports `supports` (server capabilities a client can rely +on) and `erasure` (whether this deployment's store actually removes bytes on +delete); write responses carry `erasure` too. + +`base` is an optional precondition: a map of entry key to the ciphertext hash +the caller believes that key holds, or `null` for "should not exist". A request +that disagrees with the stored manifest is refused with `409 stale_base_version` +and a `details.conflicts` map naming what each key actually holds. Omitting +`base` writes unconditionally, which is what a client that has not adopted it +still does. This guards the caller's own turn, not the moment between its last +read and its write, which is why the check is per entry rather than a namespace +version. A namespace whose manifest cannot be parsed answers +`503 manifest_unreadable` rather than appearing empty. + ## Configuration See [`.env.example`](./.env.example). Key knobs: `STORE` (`fs`|`s3`), diff --git a/RELEASE-0.1.md b/RELEASE-0.1.md index d2d7948..ede16f5 100644 --- a/RELEASE-0.1.md +++ b/RELEASE-0.1.md @@ -98,7 +98,10 @@ Make the server safe to expose to strangers before anyone connects. with explicit segment matching + an ACL hook stub. **Security-critical.** - **Request hardening**: enforce `MAX_BODY_BYTES` even when `content-length` is absent (stream cap), reject unknown methods early, add security headers. -- **Health/readiness**: `/health` already exists; add store round-trip check. +- **Health/readiness**: `/health` reports liveness only. The store round trip + moved to a startup probe rather than the route: `/health` is unauthenticated, + so a store check there is an anonymous write against the store holding every + tenant's ciphertext, and echoing the store's label leaked it to anyone. ### M2 — MCP server (the headline) · ~3–4d `memlawb mcp` stdio subcommand wrapping the already-bundled `MemlawbClient`. diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index 9043d79..47c4c79 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -1,5 +1,5 @@ /** - * Crash visibility across the commit sequence (U2, R9, AE4). + * Crash visibility across the commit sequence. * * The property: a reader never sees a manifest naming a blob that is absent, * nor a blob whose bytes disagree with the hash the visible manifest records diff --git a/tests/base-precondition.test.ts b/tests/base-precondition.test.ts index 9766d82..310df09 100644 --- a/tests/base-precondition.test.ts +++ b/tests/base-precondition.test.ts @@ -1,5 +1,5 @@ /** - * The per-entry base precondition (U3, R12, AE7 server half). + * The per-entry base precondition, server half. * * A push carries the ciphertext hash it believes each key currently holds. If * the manifest disagrees, the write is refused with 409 rather than applied. diff --git a/tests/erasure.test.ts b/tests/erasure.test.ts index 8e0a4d2..c0da42e 100644 --- a/tests/erasure.test.ts +++ b/tests/erasure.test.ts @@ -1,11 +1,11 @@ /** - * Erasure advertisement (U5, R22/R27/R28 server half). + * Erasure advertisement, server half. * * Whether a delete actually erases is a property of the store, not of the * client, and the client cannot see which driver is configured. So the store * declares it and the server reports it where a client already looks: the * hashes view and every write response. fs and s3 erase; a store that keeps - * history (the node driver, later) does not, and a client that knows will + * history (a git-backed store) does not, and a client that knows will * refuse a scan mode that would let a secret reach a store it can never leave. */ diff --git a/tests/health.test.ts b/tests/health.test.ts index b69027b..e4d0706 100644 --- a/tests/health.test.ts +++ b/tests/health.test.ts @@ -1,5 +1,5 @@ /** - * Health is liveness; the store check runs at startup (U6, R15/R24). + * Health is liveness; the store check runs at startup. * * The health route is unauthenticated, so anything it reports is public. It * used to echo the store's description, which on a driver whose label carries a diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts index 2a088d4..85c5ec4 100644 --- a/tests/rejection-log.test.ts +++ b/tests/rejection-log.test.ts @@ -1,9 +1,9 @@ /** - * The rejection log (U7, R13, AE8). + * The rejection log. * * An operator needs to know which account was refused and why. The risk is that * a log line becomes the one place plaintext leaks, so the field set is an - * allowlist fixed in KTD15 rather than a denylist: the space of things that + * allowlist rather than a denylist: the space of things that * must not appear is open-ended, and a denylist only catches what someone * thought of. A namespace slug is excluded too. It looks opaque but it is a * hash of a low-entropy namespace, so it is a stable per-tenant identifier diff --git a/tests/store-s3.test.ts b/tests/store-s3.test.ts index ee3b890..3cd11ac 100644 --- a/tests/store-s3.test.ts +++ b/tests/store-s3.test.ts @@ -1,5 +1,5 @@ /** - * S3BlobStore's listing (U2 fix pass). + * S3BlobStore's listing. * * s3 is the driver the hosted service runs, and reclaim now depends on list() * to find blobs no manifest names. A list that silently returned only its first diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index 8649bde..cad28e4 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -1,8 +1,8 @@ /** - * The store factory's test seam (U1). + * The store factory's test seam. * * `getStore()` memoizes for the life of the process, which is right for the - * server and impossible for tests: a fault-injecting store (U2's sweep) and a + * server and impossible for tests: a fault-injecting store (the crash sweep) and a * second driver in one process both need to replace the cached instance and put * the real one back. The seam exists for that and nothing else, so the last test * here walks the production import graph and fails if anything under src/ that From 2358ce6045e297687e9eeb052818ecd65cbbfbb3 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:56:09 -0500 Subject: [PATCH 12/14] test: cover the whole stack end to end, and close the last gaps Every other test drives one layer. This drives the stack the way a deployment does: AES-GCM in the client, HTTP on a real port, the content-addressed store on disk. It covers what layer-local tests structurally cannot, a change that is correct in memory.ts and wrong once ciphertext, the wire format and the storage layout have to agree on the same bytes. Twelve cases: the push/pull/modify/delete lifecycle, client-side delta, no plaintext or passphrase on disk under the new layout, a wrong passphrase failing to read, the shipped client (which sends no base and reads neither supports nor erasure) still round-tripping against a server that enforces preconditions, the 409 refusal over the wire with the competing write surviving decryptably, delete actually removing bytes, liveness-only health, a corrupt index answering 503 rather than looking empty, and a budgeted caller getting a retry hint and recovering. Gaps closed alongside it. Reclaim failures go through an injectable sink like refusals do, so the namespace they name is asserted rather than assumed; that field is the only thing making the line actionable. The filesystem listing's absent-directory and temp-file branches are covered, both of which reclaim hits on ordinary writes. The server's delta short-circuit had no test at all: removing it survived the whole suite, and it now fails against a store that counts writes. A rate-limited request to a memory route was logged as route 'other', because the throttle ran before the route was classified. Verified by mutation rather than by claim: a client that stops encrypting fails six e2e cases including the ciphertext-at-rest walk, and disabling the precondition, reclaim, the corrupt-index refusal, or leaking the store label through health each fail it too. Also smoke-tested against a separately spawned server process: two entries pushed and pulled through real encryption, one deleted, zero plaintext on disk. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/handler.ts | 2 +- src/log.ts | 32 +++++ src/memory.ts | 20 ++- tests/atomic-visibility.test.ts | 79 ++++++++++ tests/e2e.test.ts | 246 ++++++++++++++++++++++++++++++++ tests/store-seam.test.ts | 21 +++ 6 files changed, 388 insertions(+), 12 deletions(-) create mode 100644 tests/e2e.test.ts diff --git a/src/handler.ts b/src/handler.ts index f28e8d2..f3c9ae4 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -148,6 +148,7 @@ async function respond(req: Request, ctx: RequestContext): Promise { } const parsed = parseMemoryPath(pathname) + if (parsed) ctx.route = 'memory' // Authenticate before refusing anything, so the throttle below can key on the // caller when there is one. Every refusal writes a log line, and the unknown @@ -167,7 +168,6 @@ async function respond(req: Request, ctx: RequestContext): Promise { } if (!parsed) return apiError('not_found', 'unknown route', 404) - ctx.route = 'memory' if (!identity) return apiError('unauthorized', 'missing or invalid API key', 401) let namespace: string diff --git a/src/log.ts b/src/log.ts index f2f8853..ac0b166 100644 --- a/src/log.ts +++ b/src/log.ts @@ -40,6 +40,38 @@ export function setRejectionSink(next: Sink): void { sink = next } +/** + * An operational event that is not a request refusal: something the server did + * or failed to do on its own. Separate from Rejection because it carries a + * namespace slug, which a refusal line deliberately never does -- here the slug + * is the whole point, since an operator cannot act on "collection failed + * somewhere". A slug is a hash of a namespace, so it identifies a tenant to + * anyone who can already read the storage layout, and nothing more. + */ +export type OperationalEvent = { + timestamp: string + event: string + nsSlug: string + reason: string +} + +type EventSink = ((line: OperationalEvent) => void) | null +let eventSink: EventSink = null + +/** Tests capture events instead of writing them. Passing null restores stderr. */ +export function setEventSink(next: EventSink): void { + eventSink = next +} + +export function logEvent(fields: Omit): void { + const line: OperationalEvent = { timestamp: new Date().toISOString(), ...fields } + if (eventSink) { + eventSink(line) + return + } + process.stderr.write(`${JSON.stringify(line)}\n`) +} + export function logRejection(fields: Omit): void { const line: Rejection = { timestamp: new Date().toISOString(), ...fields } if (sink) { diff --git a/src/memory.ts b/src/memory.ts index 95dddfb..e94b6d2 100644 --- a/src/memory.ts +++ b/src/memory.ts @@ -17,6 +17,7 @@ import { config } from './config.ts' import { namespaceChecksum, sha256Hex, sha256Prefixed } from './hash.ts' import { withLock } from './lock.ts' +import { logEvent } from './log.ts' import { validateEntryKey } from './namespace.ts' import { QuotaError, reserveAndCommit } from './quota.ts' import { blobPrefix, contentPath, entryPath, getStore, manifestPath } from './store/index.ts' @@ -307,17 +308,14 @@ async function reclaim( if (prev[key] !== undefined) await store.delete(entryPath(nsSlug, sha256Hex(key))) } } catch (err) { - process.stderr.write( - `${JSON.stringify({ - timestamp: new Date().toISOString(), - event: 'reclaim_failed', - // The slug is already every storage path's own directory name, so it - // discloses nothing new, and it is the only field that makes this line - // actionable: without it an operator knows collection failed somewhere. - nsSlug, - reason: (err as Error)?.constructor?.name ?? 'unknown', - })}\n`, - ) + // The slug is what makes this actionable: without it an operator only knows + // collection failed somewhere. It is already every storage path's own + // directory name, so it discloses nothing a reader of the store lacks. + logEvent({ + event: 'reclaim_failed', + nsSlug, + reason: (err as Error)?.constructor?.name ?? 'unknown', + }) } } diff --git a/tests/atomic-visibility.test.ts b/tests/atomic-visibility.test.ts index 47c4c79..e192254 100644 --- a/tests/atomic-visibility.test.ts +++ b/tests/atomic-visibility.test.ts @@ -13,6 +13,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import { sha256Hex, sha256Prefixed } from '../src/hash.ts' +import { setEventSink } from '../src/log.ts' import { getData, getHashes, upsert } from '../src/memory.ts' import { namespaceSlug } from '../src/namespace.ts' import type { BlobStore } from '../src/store/blobstore.ts' @@ -198,6 +199,50 @@ describe('crash visibility across the commit sequence', () => { expect(contentPath('slug', `sha256:${good}`)).toBe(`ns/slug/blobs/${good}`) }) + test('re-pushing identical ciphertext writes no blob', async () => { + // The server compares the pushed hash against the manifest and accepts + // without writing. Without this the store rewrites every entry on every + // push, which on s3 is a network round trip per entry per request. + const ns = 'user:nooprewrite' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('same') }) + const real = getStore() + const puts: string[] = [] + setStore({ + get: p => real.get(p), + put: (p, b) => { + puts.push(p) + return real.put(p, b) + }, + delete: p => real.delete(p), + list: p => real.list(p), + describe: () => 'counting', + erasure: real.erasure, + }) + const r = await upsert(ns, slug, 'local', { entries: { 'a.md': b64('same') } }, NOW) + resetStore() + expect(r.accepted).toEqual(['a.md']) + // No blob and no manifest write: the request mutated nothing at all. + expect(puts).toEqual([]) + + // Control: changed ciphertext for the same key does write. + const puts2: string[] = [] + setStore({ + get: p => real.get(p), + put: (p, b) => { + puts2.push(p) + return real.put(p, b) + }, + delete: p => real.delete(p), + list: p => real.list(p), + describe: () => 'counting', + erasure: real.erasure, + }) + await upsert(ns, slug, 'local', { entries: { 'a.md': b64('different') } }, NOW) + resetStore() + expect(puts2.length).toBeGreaterThan(0) + }) + test('a manifest entry with a non-digest hash is skipped, not fatal', async () => { // contentPath throws on a hash that cannot name a blob, which is right on // the write path. On the read path one corrupt entry must not take the @@ -218,6 +263,40 @@ describe('crash visibility across the commit sequence', () => { expect(data.content.entries['bad.md']).toBeUndefined() }) + test('a reclaim failure names the namespace an operator has to go look at', async () => { + const events: { event: string; nsSlug: string; reason: string }[] = [] + setEventSink(l => events.push(l)) + const ns = 'user:reclaimlog' + const slug = namespaceSlug(ns) + await seed(ns, { 'a.md': b64('v1') }) + const real = getStore() + setStore({ + get: p => real.get(p), + put: (p, b) => real.put(p, b), + delete: async () => { + throw new Boom('nope') + }, + list: p => real.list(p), + describe: () => 'delete-hostile', + erasure: real.erasure, + }) + await upsert(ns, slug, 'local', { entries: { 'a.md': b64('v2') } }, NOW) + resetStore() + setEventSink(null) + expect(events.length).toBe(1) + expect(events[0]?.event).toBe('reclaim_failed') + // The slug is the field that makes the line actionable at all. + expect(events[0]?.nsSlug).toBe(slug) + expect(events[0]?.reason).toBe('Boom') + // Control: a healthy write emits no event, so the assertion above is about + // this failure and not about the sink capturing everything. + const quiet: unknown[] = [] + setEventSink(l => quiet.push(l)) + await upsert(ns, slug, 'local', { entries: { 'b.md': b64('x') } }, NOW) + setEventSink(null) + expect(quiet).toEqual([]) + }) + test('a reclaim failure does not fail a write that already published', async () => { // Reclaim collects blobs no reader can see. Failing it must not turn a // durable write into an error, and must not skip the caller's response. diff --git a/tests/e2e.test.ts b/tests/e2e.test.ts new file mode 100644 index 0000000..6109bdf --- /dev/null +++ b/tests/e2e.test.ts @@ -0,0 +1,246 @@ +/** + * End-to-end over a real socket, with the real client and real encryption. + * + * Everything else in the suite drives one layer. This drives the whole stack the + * way a deployment does: AES-GCM in the client, HTTP on a real port, the + * content-addressed store on disk. It exists to catch what layer-local tests + * structurally cannot -- a change that is correct in `memory.ts` and wrong once + * ciphertext, the wire format and the storage layout have to agree. + * + * The shipped client sends no `base` and reads neither `supports` nor + * `erasure`, because the client half of that work is a later phase. That is + * exactly why the compatibility cases below matter: they are the evidence that + * a client which has not adopted the new contract still works against a server + * that has, which nothing else in the suite proves. + */ + +import { afterAll, afterEach, beforeAll, describe, expect, test } from 'bun:test' +import { existsSync, readdirSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { _reset } from '../src/ratelimit.ts' + +const DATA_DIR = process.env.DATA_DIR as string +const PASSPHRASE = 'correct horse battery staple' + +let server: ReturnType +let base: string +let MemlawbClient: typeof import('../client/index.ts').MemlawbClient +let namespaceSlug: typeof import('../src/namespace.ts').namespaceSlug + +beforeAll(async () => { + const { handleRequest } = await import('../src/handler.ts') + ;({ MemlawbClient } = await import('../client/index.ts')) + ;({ namespaceSlug } = await import('../src/namespace.ts')) + server = Bun.serve({ port: 0, fetch: handleRequest }) + base = `http://localhost:${server.port}` +}) + +afterAll(() => server?.stop(true)) +afterEach(() => _reset()) + +const client = (passphrase = PASSPHRASE) => new MemlawbClient({ url: base, passphrase }) + +/** Every file the store holds for a namespace, as raw bytes. */ +function storedBytes(ns: string): { path: string; body: Buffer }[] { + const root = join(DATA_DIR, 'ns', namespaceSlug(ns)) + const out: { path: string; body: Buffer }[] = [] + const walk = (dir: string) => { + if (!existsSync(dir)) return + for (const name of readdirSync(dir, { withFileTypes: true })) { + const p = join(dir, name.name) + if (name.isDirectory()) walk(p) + else out.push({ path: p, body: readFileSync(p) }) + } + } + walk(root) + return out +} + +describe('e2e: the round trip a deployment actually performs', () => { + test('push, pull, modify, delete through the real client', async () => { + const ns = 'user:e2e-life' + const c = client() + await c.push(ns, { 'MEMORY.md': '# index', 'feedback/tone.md': 'be terse' }) + + const pulled = await c.pull(ns) + expect(pulled.entries).toEqual({ 'MEMORY.md': '# index', 'feedback/tone.md': 'be terse' }) + + await c.push(ns, { 'MEMORY.md': '# index v2', 'feedback/tone.md': 'be terse' }) + expect((await c.pull(ns)).entries['MEMORY.md']).toBe('# index v2') + + await c.delete(ns, 'feedback/tone.md') + const after = await c.pull(ns) + expect(Object.keys(after.entries)).toEqual(['MEMORY.md']) + }) + + test('a second push of unchanged content uploads nothing', async () => { + const ns = 'user:e2e-delta' + const c = client() + const entries = { 'a.md': 'stable', 'b.md': 'also stable' } + const first = await c.push(ns, entries) + expect(first.uploaded.sort()).toEqual(['a.md', 'b.md']) + const second = await c.push(ns, entries) + expect(second.uploaded).toEqual([]) + expect(second.unchanged.sort()).toEqual(['a.md', 'b.md']) + }) +}) + +describe('e2e: the server holds ciphertext and nothing else', () => { + test('no plaintext reaches disk, under the content-addressed layout', async () => { + const ns = 'user:e2e-zk' + const secret = 'SENTINEL-plaintext-must-not-appear' + await client().push(ns, { 'MEMORY.md': secret, 'notes/deep.md': `${secret} again` }) + + const stored = storedBytes(ns) + // Control: the walk actually found the blobs and the manifest, so the + // absence assertions below are about content and not an empty directory. + expect(stored.length).toBeGreaterThanOrEqual(3) + expect(stored.some(f => f.path.endsWith('manifest.json'))).toBe(true) + expect(stored.some(f => f.path.includes(`${join('blobs')}`))).toBe(true) + + for (const f of stored) { + expect(f.body.toString('utf8')).not.toContain(secret) + expect(f.body.toString('utf8')).not.toContain(PASSPHRASE) + } + }) + + test('the wrong passphrase cannot read what the right one wrote', async () => { + const ns = 'user:e2e-wrongpass' + await client().push(ns, { 'a.md': 'private' }) + // The manifest is cleartext, so key names are visible either way; the + // ciphertext is what the passphrase gates. + await expect(client('a-different-passphrase').pull(ns)).rejects.toThrow() + expect((await client().pull(ns)).entries['a.md']).toBe('private') + }) +}) + +describe('e2e: a client that has not adopted the new contract still works', () => { + test('the shipped client round-trips against a server that enforces preconditions', async () => { + const ns = 'user:e2e-compat' + const c = client() + // The client sends no `base`, so every write here is unconditional. This is + // the compatibility guarantee the server promises by accepting an absent + // base, and nothing else in the suite exercises it through the real client. + await c.push(ns, { 'a.md': 'one' }) + await c.push(ns, { 'a.md': 'two' }) + expect((await c.pull(ns)).entries['a.md']).toBe('two') + }) + + test('the new response fields do not disturb it', async () => { + const ns = 'user:e2e-fields' + await client().push(ns, { 'a.md': 'x' }) + const raw = (await ( + await fetch(`${base}/api/memory/${encodeURIComponent(ns)}?view=hashes`) + ).json()) as { + supports: string[] + erasure: string + entryChecksums: Record + } + // The server advertises both; the client reads neither and still works. + expect(raw.supports).toEqual(['base-precondition']) + expect(raw.erasure).toBe('erases') + expect(await client().hashes(ns)).toEqual(raw.entryChecksums) + }) +}) + +describe('e2e: the write precondition over the wire', () => { + const url = (ns: string) => `${base}/api/memory/${encodeURIComponent(ns)}` + + test('a stale base is refused and the stored value is untouched', async () => { + const ns = 'user:e2e-conflict' + const c = client() + await c.push(ns, { 'a.md': 'first' }) + const stale = (await c.hashes(ns))['a.md'] as string + + // Someone else writes. + await c.push(ns, { 'a.md': 'second' }) + + // The first caller pushes from what it last saw. Hand-built because the + // shipped client does not send a base yet. + const res = await fetch(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + entries: { 'a.md': Buffer.from('ignored').toString('base64') }, + base: { 'a.md': stale }, + }), + }) + expect(res.status).toBe(409) + const body = (await res.json()) as { error: { code: string; details: { conflicts: object } } } + expect(body.error.code).toBe('stale_base_version') + expect(Object.keys(body.error.details.conflicts)).toEqual(['a.md']) + + // The competing write survived, decryptable end to end. + expect((await c.pull(ns)).entries['a.md']).toBe('second') + }) + + test('the same push with a current base is accepted', async () => { + const ns = 'user:e2e-fresh' + const c = client() + await c.push(ns, { 'a.md': 'first' }) + const current = (await c.hashes(ns))['a.md'] as string + const res = await fetch(url(ns), { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ entries: {}, deletions: ['a.md'], base: { 'a.md': current } }), + }) + expect(res.status).toBe(200) + expect(Object.keys((await c.pull(ns)).entries)).toEqual([]) + }) +}) + +describe('e2e: deletion actually removes the bytes', () => { + test('after a delete no blob on disk carries the deleted plaintext', async () => { + const ns = 'user:e2e-erase' + const c = client() + const doomed = 'SENTINEL-should-be-collected' + await c.push(ns, { 'gone.md': doomed, 'kept.md': 'stays' }) + const before = storedBytes(ns).length + + await c.delete(ns, 'gone.md') + // Reclaim runs on the write, so the blob is collected by the time the + // response returns; erasure: 'erases' is a claim this makes true. + const after = storedBytes(ns) + expect(after.length).toBeLessThan(before) + expect((await c.pull(ns)).entries).toEqual({ 'kept.md': 'stays' }) + }) +}) + +describe('e2e: operational surfaces', () => { + test('health reports liveness and reveals nothing about the store', async () => { + const res = await fetch(`${base}/health`) + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true, service: 'memlawb' }) + }) + + test('a namespace whose index is corrupt answers 503 rather than looking empty', async () => { + const ns = 'user:e2e-corrupt' + const c = client() + await c.push(ns, { 'a.md': 'v1' }) + const { writeFileSync } = await import('node:fs') + writeFileSync(join(DATA_DIR, 'ns', namespaceSlug(ns), 'manifest.json'), '{not json') + + const res = await fetch(`${base}/api/memory/${encodeURIComponent(ns)}`) + expect(res.status).toBe(503) + expect(((await res.json()) as { error: { code: string } }).error.code).toBe( + 'manifest_unreadable', + ) + // The client surfaces it as an error rather than an empty namespace, which + // is the denial-rendered-as-success shape this refusal exists to avoid. + await expect(c.pull(ns)).rejects.toThrow() + }) + + test('a caller past its budget is refused with a retry hint', async () => { + let last: Response | undefined + for (let i = 0; i < 400; i++) { + last = await fetch(`${base}/api/memory/user:e2e-ratelimit`) + if (last.status === 429) break + } + expect(last?.status).toBe(429) + expect(last?.headers.get('retry-after')).toBeTruthy() + // Control: the budget is per caller and recovers, so this is a limit rather + // than a wedged server. + _reset() + expect((await fetch(`${base}/health`)).status).toBe(200) + }) +}) diff --git a/tests/store-seam.test.ts b/tests/store-seam.test.ts index cad28e4..582aa6f 100644 --- a/tests/store-seam.test.ts +++ b/tests/store-seam.test.ts @@ -32,6 +32,27 @@ function stub(): BlobStore { // file's stub into every later suite in the shared process. afterEach(() => resetStore()) +describe('filesystem listing', () => { + test('an absent prefix lists nothing rather than throwing', async () => { + resetStore() + // reclaim lists a namespace's blob directory on every mutating write, + // including the first, when that directory does not exist yet. + expect(await getStore().list('ns/definitely-not-here/blobs/')).toEqual([]) + }) + + test('a half-written temp file is not listed as a blob', async () => { + resetStore() + const store = getStore() + const prefix = 'ns/listfixture/blobs/' + await store.put(`${prefix}real`, new TextEncoder().encode('x')) + await store.put(`${prefix}.tmp-123-1-1`, new TextEncoder().encode('y')) + // Control: both objects are really there, so the filter is what removes one + // rather than the write having failed. + expect(await store.get(`${prefix}.tmp-123-1-1`)).not.toBeNull() + expect(await store.list(prefix)).toEqual([`${prefix}real`]) + }) +}) + describe('store factory seam', () => { test('setStore installs an instance getStore then returns', async () => { setStore(stub()) From fa3a17e44d1dc355fb919412d6f7c834c5a8e1f9 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:48:48 -0500 Subject: [PATCH 13/14] test(log): stop the rate-limit assertion racing the rest of the suite This test failed in CI and passed everywhere else, which is the shape of a shared-state race rather than a defect. The rate limiter is one in-memory bucket keyed by owner, and under open auth every test file in the process shares that owner. The test sent requests until one came back 429, so its correctness depended on the bucket having room when it started and not being emptied by another file part way through, across a 240-request window it did not control. In CI the first request was already rate limited, so it logged one line and the assertion that there should be several failed. Both halves are now driven deliberately: one request for an ordinary refusal, then the bucket is spent directly rather than through the server, then one request for the rate-limited one. That does not make the race less likely, it removes the loop the race lived in. Worth recording that I could not reproduce the failure locally. Bun schedules test files independently, and with two files it will run one to completion before loading the other, so no small harness recreates the interleaving that nineteen files produce on a loaded runner. The diagnosis rests on the CI evidence, which is unambiguous: one logged line means the first request was already refused. What remains is a single request immediately after a reset. If another file empties the bucket inside that window this now fails on the 404 assertion, which names the real condition, instead of on a line count that reads like a logging bug. The assertions still bite: suppressing the rate-limited log line, and adding a field outside the allowlist, each turn this test red. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- tests/rejection-log.test.ts | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/tests/rejection-log.test.ts b/tests/rejection-log.test.ts index 85c5ec4..6f7d8b9 100644 --- a/tests/rejection-log.test.ts +++ b/tests/rejection-log.test.ts @@ -13,7 +13,7 @@ import { afterEach, describe, expect, test } from 'bun:test' import { bucketKey, DEFAULT_CONTEXT, handleRequest } from '../src/handler.ts' import { ALLOWED_FIELDS, setRejectionSink } from '../src/log.ts' -import { _reset } from '../src/ratelimit.ts' +import { _reset, take } from '../src/ratelimit.ts' const lines: Record[] = [] @@ -33,15 +33,34 @@ function capture() { describe('rejection log', () => { test('a rate-limited caller produces exactly one line, with only allowed fields', async () => { + // The bucket is one in-memory map keyed by owner, and under open auth every + // test file in this process shares that owner. Bun schedules those files + // independently, so the previous version of this test, which sent requests + // until one came back 429, was competing with every other file for tokens + // across a 240-request window it did not control. It failed in CI because + // the first request was already rate limited, so it saw one line instead of + // many. It could not be reproduced locally: with few files the scheduler + // does not interleave them the same way. + // + // Both halves are now driven deliberately, which does not merely make the + // race less likely, it removes the loop the race lived in. What remains is + // one request immediately after a reset, and if another file emptied the + // bucket inside that window this fails loudly on the 404 assertion below + // rather than on a count that reads like a logging bug. + _reset() capture() - let last: Response | undefined - for (let i = 0; i < 400; i++) { - last = await handleRequest(new Request('http://x/api/memory/user:local')) - if (last.status === 429) break - } - expect(last?.status).toBe(429) - // Every refusal on the way here is logged too (the namespace does not - // exist, so each read is a 404). All of them must obey the allowlist. + + // A refusal that is not the rate limiter, to prove the allowlist holds for + // an ordinary line and to have something to compare the 429 against. + const missing = await handleRequest(new Request('http://x/api/memory/user:local')) + expect(missing.status).toBe(404) + + // Spend the rest of the bucket directly rather than through the server, so + // the next request is rate limited no matter what else is running. + while (take('local', Date.now()).ok) {} + const last = await handleRequest(new Request('http://x/api/memory/user:local')) + expect(last.status).toBe(429) + expect(lines.length).toBeGreaterThan(1) for (const l of lines) expect(Object.keys(l).sort()).toEqual([...ALLOWED_FIELDS].sort()) const limited = lines.filter(l => l.code === 'rate_limited') From ce189b7e63f960d731d68fc680973a90dcf002d7 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:33:29 -0500 Subject: [PATCH 14/14] docs(namespace): drop an em dash from a doc comment House style takes commas, colons or separate sentences rather than em dashes. This comment is the only place on this branch that carried one. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com> --- src/namespace.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/namespace.ts b/src/namespace.ts index c3a28b7..d359754 100644 --- a/src/namespace.ts +++ b/src/namespace.ts @@ -59,7 +59,7 @@ export function validateEntryKey(key: string): string { * The slug is the ONLY thing separating one tenant's `ns//...` storage * (and its `ns:` write lock) from another's, so it must be injective: two * distinct namespaces must never share a slug. We hash the namespace with - * sha256 — the same way entry ciphertext becomes a flat blob filename + * sha256, the same way entry ciphertext becomes a flat blob filename * (`contentPath(nsSlug, ciphertextHash)`; `entryPath` remains for entries * written before content addressing). sha256 is collision-resistant (finding * two distinct namespaces with the same slug is computationally infeasible, not