From 3e4c636b6b44d974ca6e6768ddba5a8cc5cb8a73 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 11:12:05 +0000 Subject: [PATCH] fix(service-storage)!: engine write/read failures stop masquerading as success (#5216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `StorageMetadataStore` wrapped all eight of its `IDataEngine` calls in `try { … } catch { /* ignore */ }` — no logger, no rethrow, no degradation flag. `if (this.engine)` had already separated "no engine wired" out, so those catches could only fire on a RUNTIME failure of a wired engine, and every one was swallowed behind a process-local Map write that made the loss invisible inside the same process. A failed `sys_file` insert lost mostly-permanent business truth (#5202) while the API answered 200. With an engine wired, the engine is now the only store: - writes (createFile/updateFile/deleteFile, createSession/updateSession/ deleteSession) propagate as `StorageMetadataStoreError` and mirror NOTHING into the Map, so no shadow can make a lost write look landed; - reads (getFile/getSession) separate MISS from OUTAGE — `findOne` returning nothing still yields `null` (404 unchanged), a thrown engine error propagates rather than serving this worker's stale local guess; - the Map is now exactly what the class doc claimed: the engine-absent stand-in. `new StorageMetadataStore(null)` is unchanged in every respect. The error message carries the CONSEQUENCE and the FIX per AGENTS.md "Degradation log levels", and `objectName`/`operation`/`cause` identify the failure. No route needed editing: the storage handlers already wrap everything in `catch → sendError(500, 'INTERNAL', …)`, so a lost write is now a 500 and a read outage is a 500 instead of a false 404. Tests: metadata-store.test.ts (engine-null behaviour unchanged, no Map mirroring with an engine present, every write/read outage loud, miss still null) and storage-routes.metadata-outage.test.ts (the HTTP-visible half). Both fake engines route `delete` through `assertEngineDeleteDispatch` (#4550/#5197), which is why `@objectstack/objectql` joins devDependencies. Fixes #5216 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017MCKJaEomEqg4tvz4SzdNd --- .changeset/storage-metadata-loud-failure.md | 59 +++ .../services/service-storage/package.json | 1 + .../services/service-storage/src/index.ts | 4 +- .../src/metadata-store.test.ts | 360 ++++++++++++++++++ .../service-storage/src/metadata-store.ts | 233 +++++++++--- .../storage-routes.metadata-outage.test.ts | 268 +++++++++++++ pnpm-lock.yaml | 3 + 7 files changed, 863 insertions(+), 65 deletions(-) create mode 100644 .changeset/storage-metadata-loud-failure.md create mode 100644 packages/services/service-storage/src/metadata-store.test.ts create mode 100644 packages/services/service-storage/src/storage-routes.metadata-outage.test.ts diff --git a/.changeset/storage-metadata-loud-failure.md b/.changeset/storage-metadata-loud-failure.md new file mode 100644 index 0000000000..d48aa4799d --- /dev/null +++ b/.changeset/storage-metadata-loud-failure.md @@ -0,0 +1,59 @@ +--- +"@objectstack/service-storage": major +--- + +fix(service-storage)!: a `sys_file` / `sys_upload_session` write that never landed no longer reports success (#5216) + +`StorageMetadataStore` wrapped **all eight** of its `IDataEngine` calls in +`try { … } catch { /* ignore */ }` — no logger, no rethrow, no degradation flag. +Because `if (this.engine)` had already separated "no data engine wired" out, +those catches could only ever fire on a **runtime** failure of an engine that is +wired: a constraint violation, a connection blip, an RLS refusal, a table that +was never migrated. Every one of them was swallowed, and the store returned the +record it had just put in a process-local `Map`. + +The result on `sys_file` — mostly-permanent business truth with compliance value +(#5202) — was the shape AGENTS.md → "Degradation log levels" exists to forbid: +the bytes landed in the storage backend, the metadata row **never existed**, and +`POST /api/v1/storage/upload/presigned` answered `200 { success: true }` with a +`fileId` naming nothing. A read in the same process then found the Map shadow, +so even a self-check looked healthy — until the worker recycled and the +attachment became permanently unaddressable, with not one line of log pointing +at the cause. On `sys_upload_session` the same swallow made multi-worker chunked +uploads die as unexplained stalls instead of a diagnosable error. + +**What changes.** With a data engine wired, the engine is now the only store: + +- **Writes** (`createFile`, `updateFile`, `deleteFile`, `createSession`, + `updateSession`, `deleteSession`) propagate the failure as a new + `StorageMetadataStoreError` instead of returning a value. Nothing is mirrored + into the `Map`, so there is no in-process shadow left behind to make a lost + write look like a landed one. +- **Reads** (`getFile`, `getSession`) distinguish a **miss** from an **outage**. + `findOne` returning nothing is still a miss and still returns `null` (the REST + layer answers 404, unchanged). An engine that *throws* now propagates: + substituting this process's `Map` for an unreachable engine would dress a + stale or empty local guess up as the persisted answer, which under multiple + workers is a different wrong answer per worker. +- The process-local `Map` is now exactly what the class doc always claimed — + the stand-in for deployments with **no** engine wired (tests, dev). Behaviour + of `new StorageMetadataStore(null)` is unchanged in every respect. + +**Breaking, and where it shows.** No API signature changed; what changed is that +these calls can now reject. Requests that previously received `200` over a lost +write receive `500 INTERNAL` from the existing storage route handlers (they +already wrapped every handler in `catch → sendError(500, 'INTERNAL', …)`, so no +route needed editing), and a read attempted during an engine outage answers +`500` rather than a false `404 FILE_NOT_FOUND`. If you call +`StorageMetadataStore` directly, the six write methods and the two read methods +may now throw `StorageMetadataStoreError` — `error.objectName` +(`sys_file` / `sys_upload_session`), `error.operation` +(`insert` / `update` / `delete` / `findOne`) and `error.cause` (the engine's own +failure) identify it, and `error.message` states the consequence and the fix. + +There is nothing to migrate: no deployment can have been *relying* on the old +behaviour, because the old behaviour produced no signal to rely on. What a +deployment may newly *see* is a 500 that was previously an undetected data loss. +`StorageMetadataStoreError` and the `StorageMetadataOperation` type are exported +from `@objectstack/service-storage` for callers that want to tell a metadata +outage apart from any other 500. diff --git a/packages/services/service-storage/package.json b/packages/services/service-storage/package.json index d43bead3fd..03ecd5e599 100644 --- a/packages/services/service-storage/package.json +++ b/packages/services/service-storage/package.json @@ -37,6 +37,7 @@ } }, "devDependencies": { + "@objectstack/objectql": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/services/service-storage/src/index.ts b/packages/services/service-storage/src/index.ts index b8937070ab..bbc1231f69 100644 --- a/packages/services/service-storage/src/index.ts +++ b/packages/services/service-storage/src/index.ts @@ -7,8 +7,8 @@ export { LocalStorageAdapter } from './local-storage-adapter.js'; export type { LocalStorageAdapterOptions } from './local-storage-adapter.js'; export { S3StorageAdapter } from './s3-storage-adapter.js'; export type { S3StorageAdapterOptions } from './s3-storage-adapter.js'; -export { StorageMetadataStore } from './metadata-store.js'; -export type { FileRecord, UploadSessionRecord } from './metadata-store.js'; +export { StorageMetadataStore, StorageMetadataStoreError } from './metadata-store.js'; +export type { FileRecord, UploadSessionRecord, StorageMetadataOperation } from './metadata-store.js'; export { registerStorageRoutes } from './storage-routes.js'; export type { StorageRoutesOptions, FileReadVerdict } from './storage-routes.js'; export { SystemFile, SystemUploadSession } from './objects/index.js'; diff --git a/packages/services/service-storage/src/metadata-store.test.ts b/packages/services/service-storage/src/metadata-store.test.ts new file mode 100644 index 0000000000..80fc683395 --- /dev/null +++ b/packages/services/service-storage/src/metadata-store.test.ts @@ -0,0 +1,360 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #5216 — `StorageMetadataStore` used to wrap all eight of its `IDataEngine` +// calls in `try { … } catch { /* ignore */ }` with no logger and no rethrow. +// With an engine wired, that catch could only ever fire on a RUNTIME failure +// (`if (this.engine)` already separated "no engine" out), so a failed +// `sys_file` insert lost mostly-permanent business truth (#5202) while the +// route answered 200 over an in-process Map shadow that vanished with the +// worker. +// +// These tests pin the three properties the fix rests on: +// 1. `engine === null` behaves exactly as before — the Map IS the store; +// 2. with an engine present the Map is never written, so no shadow can make +// a lost write look like a landed one; +// 3. a failing engine surfaces: writes AND reads throw +// `StorageMetadataStoreError`, and a MISS (`findOne` → null) still +// returns `null` rather than throwing. + +import { describe, it, expect } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import type { IDataEngine } from '@objectstack/spec/contracts'; +import { + StorageMetadataStore, + StorageMetadataStoreError, + type FileRecord, + type UploadSessionRecord, +} from './metadata-store.js'; + +// --------------------------------------------------------------------------- +// Fake engine +// --------------------------------------------------------------------------- + +type EngineMethod = 'insert' | 'update' | 'delete' | 'findOne'; + +/** + * A minimal in-memory `IDataEngine` whose every method can be made to throw, + * simulating the runtime failures (`constraint`, `connection`, RLS refusal, + * missing table) the swallowed catches used to hide. + * + * `delete` opens with {@link assertEngineDeleteDispatch} — the producer's own + * dispatch predicate (#4550/#5197) — so this double can never accept a delete + * shape the real `ObjectQL.delete` refuses. + */ +function createFakeEngine(opts: { failing?: EngineMethod[] } = {}) { + const failing = new Set(opts.failing ?? []); + const rows = new Map>(); + const calls: Array<{ method: EngineMethod; object: string }> = []; + + const table = (object: string) => { + let t = rows.get(object); + if (!t) rows.set(object, (t = new Map())); + return t; + }; + const boom = (method: EngineMethod, object: string) => { + calls.push({ method, object }); + if (failing.has(method)) { + throw new Error(`simulated engine outage on ${method}(${object})`); + } + }; + + const engine = { + async insert(object: string, data: any) { + boom('insert', object); + table(object).set(String(data.id), { ...data }); + return { ...data }; + }, + async findOne(object: string, query?: any) { + boom('findOne', object); + const id = query?.where?.id; + return table(object).get(String(id)) ?? null; + }, + async update(object: string, data: any, options?: any) { + boom('update', object); + const id = String(options?.where?.id ?? data?.id); + const t = table(object); + if (!t.has(id)) return null; + t.set(id, { ...t.get(id), ...data }); + return { ...t.get(id) }; + }, + async delete(object: string, options?: any) { + assertEngineDeleteDispatch(options); + boom('delete', object); + const id = String(options?.where?.id); + return table(object).delete(id) ? 1 : 0; + }, + async find() { + return []; + }, + async count() { + return 0; + }, + async aggregate() { + return []; + }, + /** Rows the engine actually persisted, for "did the write land?" assertions. */ + _rows: (object: string) => [...table(object).values()], + /** Every engine call this store issued, in order. */ + _calls: calls, + /** Flip a method into/out of the failing set mid-test. */ + _setFailing: (method: EngineMethod, on: boolean) => { + if (on) failing.add(method); + else failing.delete(method); + }, + }; + return engine as typeof engine & IDataEngine; +} + +const fileRec = (id: string): FileRecord => ({ + id, + key: `user/${id}.txt`, + name: `${id}.txt`, + mime_type: 'text/plain', + size: 3, + status: 'pending', +}); + +const sessionRec = (id: string): UploadSessionRecord => ({ + id, + file_id: `file-of-${id}`, + key: `user/${id}.bin`, + filename: `${id}.bin`, + total_size: 100, + chunk_size: 50, + total_chunks: 2, + status: 'in_progress', +}); + +// --------------------------------------------------------------------------- +// 1. engine === null — the documented fallback, unchanged +// --------------------------------------------------------------------------- + +describe('StorageMetadataStore: no engine wired (the Map IS the store)', () => { + it('round-trips files through the process-local Map', async () => { + const store = new StorageMetadataStore(null); + + const created = await store.createFile(fileRec('f1')); + expect(created.id).toBe('f1'); + expect(created.created_at).toBeTruthy(); + expect(await store.getFile('f1')).toMatchObject({ id: 'f1', status: 'pending' }); + + const updated = await store.updateFile('f1', { status: 'committed', etag: 'abc' }); + expect(updated).toMatchObject({ status: 'committed', etag: 'abc' }); + expect(await store.getFile('f1')).toMatchObject({ status: 'committed' }); + + await store.deleteFile('f1'); + expect(await store.getFile('f1')).toBeNull(); + }); + + it('round-trips upload sessions through the process-local Map', async () => { + const store = new StorageMetadataStore(null); + + const created = await store.createSession(sessionRec('s1')); + expect(created).toMatchObject({ id: 's1', uploaded_chunks: 0, parts: '[]' }); + expect(await store.getSession('s1')).toMatchObject({ id: 's1' }); + + const updated = await store.updateSession('s1', { uploaded_chunks: 1, status: 'completing' }); + expect(updated).toMatchObject({ uploaded_chunks: 1, status: 'completing' }); + + await store.deleteSession('s1'); + expect(await store.getSession('s1')).toBeNull(); + }); + + it('updating / reading something absent is a miss, not an error', async () => { + const store = new StorageMetadataStore(null); + expect(await store.getFile('nope')).toBeNull(); + expect(await store.updateFile('nope', { status: 'committed' })).toBeNull(); + expect(await store.getSession('nope')).toBeNull(); + expect(await store.updateSession('nope', { status: 'completed' })).toBeNull(); + await expect(store.deleteFile('nope')).resolves.toBeUndefined(); + await expect(store.deleteSession('nope')).resolves.toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// 2. engine wired and healthy — one store, no Map shadow +// --------------------------------------------------------------------------- + +describe('StorageMetadataStore: engine wired and healthy', () => { + it('writes and reads sys_file through the engine', async () => { + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + + await store.createFile(fileRec('f1')); + expect(engine._rows('sys_file')).toHaveLength(1); + + await store.updateFile('f1', { status: 'committed' }); + expect(engine._rows('sys_file')[0]).toMatchObject({ status: 'committed' }); + + await store.deleteFile('f1'); + expect(engine._rows('sys_file')).toHaveLength(0); + expect(await store.getFile('f1')).toBeNull(); + }); + + it('writes and reads sys_upload_session through the engine', async () => { + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + + await store.createSession(sessionRec('s1')); + await store.updateSession('s1', { uploaded_chunks: 2, status: 'completed' }); + expect(engine._rows('sys_upload_session')[0]).toMatchObject({ + uploaded_chunks: 2, + status: 'completed', + }); + + await store.deleteSession('s1'); + expect(await store.getSession('s1')).toBeNull(); + }); + + it('mirrors NOTHING into the Map — a row that leaves the engine is gone', async () => { + // The #5216 mechanism in miniature: the old store wrote the Map first, so + // a read right after a lost engine write found the shadow and everything + // looked fine inside this process. With an engine wired the Map is never + // touched, so the engine is the only answer there is. + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + + await store.createFile(fileRec('f1')); + await store.createSession(sessionRec('s1')); + + // Another worker / an admin deletes the rows straight out of the engine. + await engine.delete('sys_file', { where: { id: 'f1' } }); + await engine.delete('sys_upload_session', { where: { id: 's1' } }); + + expect(await store.getFile('f1')).toBeNull(); + expect(await store.getSession('s1')).toBeNull(); + }); + + it('a findOne miss is a miss — null, not a throw', async () => { + const store = new StorageMetadataStore(createFakeEngine()); + expect(await store.getFile('never-existed')).toBeNull(); + expect(await store.getSession('never-existed')).toBeNull(); + expect(await store.updateFile('never-existed', { status: 'committed' })).toBeNull(); + expect(await store.updateSession('never-existed', { status: 'completed' })).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 3. engine wired and FAILING — loud, and no in-memory residue +// --------------------------------------------------------------------------- + +describe('StorageMetadataStore: engine wired and failing (#5216)', () => { + it('createFile throws instead of losing the sys_file row behind a 200', async () => { + const engine = createFakeEngine({ failing: ['insert'] }); + const store = new StorageMetadataStore(engine); + + await expect(store.createFile(fileRec('f1'))).rejects.toBeInstanceOf(StorageMetadataStoreError); + expect(engine._rows('sys_file')).toHaveLength(0); + + // No Map residue: once the engine recovers, the read is honest about the + // row never having been written. + engine._setFailing('insert', false); + expect(await store.getFile('f1')).toBeNull(); + }); + + it("the thrown error names the object, the consequence and the fix", async () => { + const engine = createFakeEngine({ failing: ['insert'] }); + const store = new StorageMetadataStore(engine); + + const err = await store.createFile(fileRec('f1')).catch((e: any) => e); + expect(err).toBeInstanceOf(StorageMetadataStoreError); + expect(err.name).toBe('StorageMetadataStoreError'); + expect(err.objectName).toBe('sys_file'); + expect(err.operation).toBe('insert'); + // CONSEQUENCE — what is not persisted (AGENTS.md → "Degradation log levels"). + expect(err.message).toContain('sys_file row was NOT written'); + // FIX — what restores durability. + expect(err.message).toContain('Restore the data engine'); + // The underlying failure is preserved, not replaced. + expect(err.message).toContain('simulated engine outage on insert(sys_file)'); + expect((err.cause as Error)?.message).toBe('simulated engine outage on insert(sys_file)'); + }); + + it.each([ + ['updateFile', 'update' as const, 'sys_file'], + ['deleteFile', 'delete' as const, 'sys_file'], + ])('%s propagates a %s outage', async (_method, failing, objectName) => { + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + await store.createFile(fileRec('f1')); + engine._setFailing(failing, true); + + const call = + failing === 'update' + ? store.updateFile('f1', { status: 'committed' }) + : store.deleteFile('f1'); + const err = await call.then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(StorageMetadataStoreError); + expect((err as StorageMetadataStoreError).objectName).toBe(objectName); + expect((err as StorageMetadataStoreError).operation).toBe(failing); + }); + + it('createSession throws instead of stranding later chunks on another worker', async () => { + const engine = createFakeEngine({ failing: ['insert'] }); + const store = new StorageMetadataStore(engine); + + const err = await store.createSession(sessionRec('s1')).catch((e: any) => e); + expect(err).toBeInstanceOf(StorageMetadataStoreError); + expect(err.objectName).toBe('sys_upload_session'); + expect(err.message).toContain('cannot find this upload'); + + engine._setFailing('insert', false); + expect(await store.getSession('s1')).toBeNull(); + }); + + it.each([ + ['updateSession', 'update' as const], + ['deleteSession', 'delete' as const], + ])('%s propagates a %s outage', async (_method, failing) => { + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + await store.createSession(sessionRec('s1')); + engine._setFailing(failing, true); + + const call = + failing === 'update' + ? store.updateSession('s1', { status: 'completed' }) + : store.deleteSession('s1'); + const err = await call.then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(StorageMetadataStoreError); + expect((err as StorageMetadataStoreError).objectName).toBe('sys_upload_session'); + expect((err as StorageMetadataStoreError).operation).toBe(failing); + }); + + it('a READ outage is loud — it never gets dressed up as "not found"', async () => { + const engine = createFakeEngine(); + const store = new StorageMetadataStore(engine); + await store.createFile(fileRec('f1')); + await store.createSession(sessionRec('s1')); + + engine._setFailing('findOne', true); + + const fileErr = await store.getFile('f1').catch((e: any) => e); + expect(fileErr).toBeInstanceOf(StorageMetadataStoreError); + expect(fileErr.operation).toBe('findOne'); + expect(fileErr.message).toContain('NOT the same as it being absent'); + + const sessionErr = await store.getSession('s1').catch((e: any) => e); + expect(sessionErr).toBeInstanceOf(StorageMetadataStoreError); + expect(sessionErr.objectName).toBe('sys_upload_session'); + expect(sessionErr.message).toContain('abort a live upload'); + }); + + it('a read outage inside updateFile/updateSession propagates too', async () => { + const engine = createFakeEngine({ failing: ['findOne'] }); + const store = new StorageMetadataStore(engine); + + await expect(store.updateFile('f1', { status: 'committed' })).rejects.toBeInstanceOf( + StorageMetadataStoreError, + ); + await expect(store.updateSession('s1', { status: 'completed' })).rejects.toBeInstanceOf( + StorageMetadataStoreError, + ); + }); +}); diff --git a/packages/services/service-storage/src/metadata-store.ts b/packages/services/service-storage/src/metadata-store.ts index bafe3de728..1548524c98 100644 --- a/packages/services/service-storage/src/metadata-store.ts +++ b/packages/services/service-storage/src/metadata-store.ts @@ -57,12 +57,113 @@ export interface UploadSessionRecord { updated_at?: string; } +/** The `IDataEngine` operations this store issues. */ +export type StorageMetadataOperation = 'insert' | 'update' | 'delete' | 'findOne'; + +/** + * A `sys_file` / `sys_upload_session` operation failed against a data engine + * that IS wired (#5216). + * + * This is deliberately NOT the same condition as "no data engine" — that one + * is served by the process-local Map and is not an error at all. An engine + * that is present and failing is a constraint violation, a connection blip, an + * RLS refusal or a missing table, and every one of those means the row the + * caller was told about does not exist. Swallowing it (what this store did + * until #5216) turns a diagnosable error into lost business truth: the bytes + * land in the backend, `sys_file` never records them, and the API answers 200. + * + * `message` carries the two things AGENTS.md → "Degradation log levels" makes + * a durability failure owe — the CONSEQUENCE (what is not persisted) and the + * FIX — because that string is what reaches the operator, through the REST + * layer's 500 body and through whatever logs the host keeps. + */ +export class StorageMetadataStoreError extends Error { + override readonly name = 'StorageMetadataStoreError'; + /** The object the failed operation targeted (`sys_file` / `sys_upload_session`). */ + readonly objectName: string; + /** The `IDataEngine` method that threw. */ + readonly operation: StorageMetadataOperation; + /** + * The engine failure this wraps. + * + * Declared here rather than inherited: this package compiles against + * `lib: ES2020`, which predates `Error.cause` — so the field, and the + * assignment in the constructor, are the whole of it. + */ + readonly cause: unknown; + + constructor( + objectName: string, + operation: StorageMetadataOperation, + consequence: string, + cause: unknown, + ) { + super( + `StorageMetadataStore: ${objectName} ${operation} failed against the data engine — ${consequence} ` + + 'Restore the data engine (connectivity / permissions / `' + + objectName + + '` schema migration); the process-local Map fallback serves only ' + + 'deployments with NO engine wired (tests, dev), so it cannot stand in here. ' + + `Cause: ${describeCause(cause)}`, + ); + this.objectName = objectName; + this.operation = operation; + this.cause = cause; + } +} + +function describeCause(cause: unknown): string { + if (cause instanceof Error) return `${cause.name}: ${cause.message}`; + return String(cause); +} + +// --------------------------------------------------------------------------- +// Consequence lines — one per (object, operation), stated as what is NOT true +// anymore if this call is allowed to fail quietly. +// --------------------------------------------------------------------------- + +const FILE_INSERT_CONSEQUENCE = + 'the sys_file row was NOT written, so the uploaded bytes have no durable record and are unaddressable after this process exits.'; +const FILE_UPDATE_CONSEQUENCE = + 'the sys_file row still holds its pre-update contents, so a commit / status / key change the caller is about to be told succeeded did not land.'; +const FILE_DELETE_CONSEQUENCE = + 'the sys_file row is still present, so a file reported as deleted remains addressable.'; +const FILE_READ_CONSEQUENCE = + 'the row could not be read at all, which is NOT the same as it being absent — answering "not found" here would report durable business truth as missing.'; + +const SESSION_INSERT_CONSEQUENCE = + 'the sys_upload_session row was NOT written, so subsequent chunk requests — which may land on any worker — cannot find this upload.'; +const SESSION_UPDATE_CONSEQUENCE = + 'the persisted upload progress is stale, so resuming or completing this upload will read the wrong part list.'; +const SESSION_DELETE_CONSEQUENCE = + 'the sys_upload_session row is still present, so an upload reported as cleaned up is still resumable.'; +const SESSION_READ_CONSEQUENCE = + 'the row could not be read at all, which is NOT the same as it being absent — answering "upload session not found" here would abort a live upload.'; + /** * Storage metadata persistence. * - * Backed by `IDataEngine` (objectql) when available — otherwise falls back to - * a process-local Map (suitable for tests and dev environments where the - * data engine isn't wired up). + * Backed by `IDataEngine` (objectql). The process-local `Map` is the + * **engine-absent stand-in** — what `new StorageMetadataStore(null)` gets in + * tests and in dev environments where the data engine isn't wired up. It is + * not a runtime fallback: when an engine IS present every read and write goes + * to it and nothing is mirrored into the Map, so no in-process shadow can make + * a write that never landed look like it did (#5216). + * + * **A wired engine that fails is not an absent engine**, and this store no + * longer conflates the two. Every engine call propagates its failure as a + * {@link StorageMetadataStoreError}; the REST layer turns that into a 500 + * instead of the 200 it used to answer over a `sys_file` row that was never + * written. `sys_file` is mostly-permanent business truth with compliance value + * (#5202), and a durability failure that still looks normal from the outside + * is precisely what AGENTS.md → "Degradation log levels" forbids. + * + * **A miss is still a miss.** `findOne` returning nothing means the row is not + * there, and the reader gets `null` (the REST layer answers 404). Only a + * thrown engine error — an outage — propagates. Silently substituting this + * process's Map for an unreachable engine would be worse than either: under + * multiple workers the Map holds only this process's shadow, so the "read" + * would dress a stale or empty local guess up as the persisted answer. */ export class StorageMetadataStore { private readonly files = new Map(); @@ -70,6 +171,23 @@ export class StorageMetadataStore { constructor(private readonly engine: IDataEngine | null) {} + /** + * Run one engine call, converting any failure into a + * {@link StorageMetadataStoreError} that names the consequence and the fix. + */ + private async engineOp( + objectName: string, + operation: StorageMetadataOperation, + consequence: string, + run: (engine: IDataEngine) => Promise, + ): Promise { + try { + return await run(this.engine!); + } catch (cause) { + throw new StorageMetadataStoreError(objectName, operation, consequence, cause); + } + } + // --------------------------------------------------------------------------- // Files // --------------------------------------------------------------------------- @@ -77,53 +195,46 @@ export class StorageMetadataStore { async createFile(rec: FileRecord): Promise { const now = new Date().toISOString(); const full: FileRecord = { created_at: now, updated_at: now, ...rec }; - this.files.set(full.id, full); - if (this.engine) { - try { - await this.engine.insert('sys_file', full); - } catch { - /* engine not available or schema not migrated — keep in-memory only */ - } + if (!this.engine) { + this.files.set(full.id, full); + return full; } + await this.engineOp('sys_file', 'insert', FILE_INSERT_CONSEQUENCE, (engine) => + engine.insert('sys_file', full), + ); return full; } async getFile(id: string): Promise { - if (this.engine) { - try { - const found = await this.engine.findOne('sys_file', { where: { id } }); - if (found) return found as FileRecord; - } catch { - /* fall through to memory */ - } - } - return this.files.get(id) ?? null; + if (!this.engine) return this.files.get(id) ?? null; + const found = await this.engineOp('sys_file', 'findOne', FILE_READ_CONSEQUENCE, (engine) => + engine.findOne('sys_file', { where: { id } }), + ); + return (found as FileRecord | null | undefined) ?? null; } async updateFile(id: string, patch: Partial): Promise { const existing = await this.getFile(id); if (!existing) return null; const merged: FileRecord = { ...existing, ...patch, id, updated_at: new Date().toISOString() }; - this.files.set(id, merged); - if (this.engine) { - try { - await this.engine.update('sys_file', merged as any, { where: { id } } as any); - } catch { - /* ignore */ - } + if (!this.engine) { + this.files.set(id, merged); + return merged; } + await this.engineOp('sys_file', 'update', FILE_UPDATE_CONSEQUENCE, (engine) => + engine.update('sys_file', merged as any, { where: { id } } as any), + ); return merged; } async deleteFile(id: string): Promise { - this.files.delete(id); - if (this.engine) { - try { - await this.engine.delete('sys_file', { where: { id } } as any); - } catch { - /* ignore */ - } + if (!this.engine) { + this.files.delete(id); + return; } + await this.engineOp('sys_file', 'delete', FILE_DELETE_CONSEQUENCE, (engine) => + engine.delete('sys_file', { where: { id } } as any), + ); } // --------------------------------------------------------------------------- @@ -140,27 +251,25 @@ export class StorageMetadataStore { updated_at: now, ...rec, }; - this.sessions.set(full.id, full); - if (this.engine) { - try { - await this.engine.insert('sys_upload_session', full); - } catch { - /* ignore */ - } + if (!this.engine) { + this.sessions.set(full.id, full); + return full; } + await this.engineOp('sys_upload_session', 'insert', SESSION_INSERT_CONSEQUENCE, (engine) => + engine.insert('sys_upload_session', full), + ); return full; } async getSession(id: string): Promise { - if (this.engine) { - try { - const found = await this.engine.findOne('sys_upload_session', { where: { id } }); - if (found) return found as UploadSessionRecord; - } catch { - /* ignore */ - } - } - return this.sessions.get(id) ?? null; + if (!this.engine) return this.sessions.get(id) ?? null; + const found = await this.engineOp( + 'sys_upload_session', + 'findOne', + SESSION_READ_CONSEQUENCE, + (engine) => engine.findOne('sys_upload_session', { where: { id } }), + ); + return (found as UploadSessionRecord | null | undefined) ?? null; } async updateSession(id: string, patch: Partial): Promise { @@ -172,25 +281,23 @@ export class StorageMetadataStore { id, updated_at: new Date().toISOString(), }; - this.sessions.set(id, merged); - if (this.engine) { - try { - await this.engine.update('sys_upload_session', merged as any, { where: { id } } as any); - } catch { - /* ignore */ - } + if (!this.engine) { + this.sessions.set(id, merged); + return merged; } + await this.engineOp('sys_upload_session', 'update', SESSION_UPDATE_CONSEQUENCE, (engine) => + engine.update('sys_upload_session', merged as any, { where: { id } } as any), + ); return merged; } async deleteSession(id: string): Promise { - this.sessions.delete(id); - if (this.engine) { - try { - await this.engine.delete('sys_upload_session', { where: { id } } as any); - } catch { - /* ignore */ - } + if (!this.engine) { + this.sessions.delete(id); + return; } + await this.engineOp('sys_upload_session', 'delete', SESSION_DELETE_CONSEQUENCE, (engine) => + engine.delete('sys_upload_session', { where: { id } } as any), + ); } } diff --git a/packages/services/service-storage/src/storage-routes.metadata-outage.test.ts b/packages/services/service-storage/src/storage-routes.metadata-outage.test.ts new file mode 100644 index 0000000000..b24b6dbf21 --- /dev/null +++ b/packages/services/service-storage/src/storage-routes.metadata-outage.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #5216, the half that is visible to a caller: a `sys_file` / +// `sys_upload_session` write that never landed must not come back as `200 { +// success: true }`. +// +// Before this change `StorageMetadataStore` swallowed every engine failure and +// returned the record it had just put in a process-local Map, so +// `POST /upload/presigned` answered 200 with a `fileId` naming a row that did +// not exist anywhere durable — and `GET /files/:id/url` answered 404 +// FILE_NOT_FOUND during an engine outage, which reports durable business truth +// as missing. Both now surface through the route handlers' existing +// `catch → sendError(500, 'INTERNAL')`, so the REST layer needed no change: +// making the store honest was enough. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { IHttpRequest, IHttpResponse, RouteHandler, IDataEngine } from '@objectstack/spec/contracts'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { LocalStorageAdapter } from './local-storage-adapter.js'; +import { StorageMetadataStore } from './metadata-store.js'; +import { registerStorageRoutes } from './storage-routes.js'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +function createMockHttpServer() { + const routes = new Map(); + return { + get: vi.fn((path: string, handler: RouteHandler) => { routes.set(`GET:${path}`, handler); }), + post: vi.fn((path: string, handler: RouteHandler) => { routes.set(`POST:${path}`, handler); }), + put: vi.fn((path: string, handler: RouteHandler) => { routes.set(`PUT:${path}`, handler); }), + delete: vi.fn(), + patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + _getHandler(method: string, path: string): RouteHandler | undefined { + return routes.get(`${method}:${path}`); + }, + }; +} + +function createMockReq(overrides: Partial = {}): IHttpRequest { + return { params: {}, query: {}, body: undefined, headers: {}, method: 'GET', path: '/', ...overrides }; +} + +function createMockRes(): IHttpResponse & { _status: number; _json: any } { + const res: any = { + _status: 200, + _json: null, + json(data: any) { res._json = data; }, + send(data: any) { res._sent = data; }, + status(code: number) { res._status = code; return res; }, + header(name: string, value: string) { res._headers = { ...(res._headers ?? {}), [name]: value }; return res; }, + }; + return res; +} + +type EngineMethod = 'insert' | 'update' | 'delete' | 'findOne'; + +/** + * In-memory engine double with per-method failure toggles. `delete` opens with + * the producer's own dispatch predicate (#4550/#5197) so it can never accept a + * call shape the real `ObjectQL.delete` refuses. + */ +function createFakeEngine(opts: { failing?: EngineMethod[] } = {}) { + const failing = new Set(opts.failing ?? []); + const rows = new Map>(); + const table = (object: string) => { + let t = rows.get(object); + if (!t) rows.set(object, (t = new Map())); + return t; + }; + const boom = (method: EngineMethod, object: string) => { + if (failing.has(method)) throw new Error(`simulated engine outage on ${method}(${object})`); + }; + const engine = { + async insert(object: string, data: any) { + boom('insert', object); + table(object).set(String(data.id), { ...data }); + return { ...data }; + }, + async findOne(object: string, query?: any) { + boom('findOne', object); + return table(object).get(String(query?.where?.id)) ?? null; + }, + async update(object: string, data: any, options?: any) { + boom('update', object); + const id = String(options?.where?.id ?? data?.id); + const t = table(object); + if (!t.has(id)) return null; + t.set(id, { ...t.get(id), ...data }); + return { ...t.get(id) }; + }, + async delete(object: string, options?: any) { + assertEngineDeleteDispatch(options); + boom('delete', object); + return table(object).delete(String(options?.where?.id)) ? 1 : 0; + }, + async find() { return []; }, + async count() { return 0; }, + async aggregate() { return []; }, + _rows: (object: string) => [...table(object).values()], + _setFailing: (method: EngineMethod, on: boolean) => { + if (on) failing.add(method); + else failing.delete(method); + }, + }; + return engine as typeof engine & IDataEngine; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('Storage routes: a metadata write that never landed is not a 200 (#5216)', () => { + let rootDir: string; + let adapter: LocalStorageAdapter; + let httpServer: ReturnType; + + const mount = (engine: IDataEngine) => { + const store = new StorageMetadataStore(engine); + registerStorageRoutes(httpServer as any, adapter, store, { basePath: '/api/v1/storage' }); + return store; + }; + + beforeEach(async () => { + rootDir = join(tmpdir(), `os-5216-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + adapter = new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); + httpServer = createMockHttpServer(); + }); + + afterEach(async () => { + if (rootDir) await fs.rm(rootDir, { recursive: true, force: true }); + }); + + it('POST /upload/presigned answers 500, not 200, when the sys_file insert fails', async () => { + const engine = createFakeEngine({ failing: ['insert'] }); + mount(engine); + + const handler = httpServer._getHandler('POST', '/api/v1/storage/upload/presigned')!; + const res = createMockRes(); + await handler( + createMockReq({ body: { filename: 'photo.jpg', mimeType: 'image/jpeg', size: 1024, scope: 'user' } }), + res, + ); + + expect(res._status).toBe(500); + expect(res._json.success).toBe(false); + expect(res._json.error.code).toBe('INTERNAL'); + expect(res._json.error.message).toContain('sys_file row was NOT written'); + expect(engine._rows('sys_file')).toHaveLength(0); + }); + + it('POST /upload/chunked answers 500 when the sys_upload_session insert fails', async () => { + const engine = createFakeEngine(); + mount(engine); + + // sys_file lands; the session insert is the one that fails. + const originalInsert = engine.insert.bind(engine); + (engine as any).insert = async (object: string, data: any) => { + if (object === 'sys_upload_session') throw new Error('simulated engine outage on insert(sys_upload_session)'); + return originalInsert(object, data); + }; + + const handler = httpServer._getHandler('POST', '/api/v1/storage/upload/chunked')!; + const res = createMockRes(); + await handler( + createMockReq({ body: { filename: 'big.bin', mimeType: 'application/octet-stream', totalSize: 10_000_000 } }), + res, + ); + + expect(res._status).toBe(500); + expect(res._json.error.code).toBe('INTERNAL'); + expect(res._json.error.message).toContain('cannot find this upload'); + }); + + it('POST /upload/complete answers 500, not 200, when the sys_file update fails', async () => { + const engine = createFakeEngine(); + const store = mount(engine); + await store.createFile({ id: 'f1', key: 'user/f1.txt', name: 'f1.txt', status: 'pending' }); + engine._setFailing('update', true); + + const handler = httpServer._getHandler('POST', '/api/v1/storage/upload/complete')!; + const res = createMockRes(); + await handler(createMockReq({ body: { fileId: 'f1', eTag: 'abc' } }), res); + + expect(res._status).toBe(500); + expect(res._json.error.message).toContain('did not land'); + // The row is untouched — which is exactly what the 500 now tells the caller. + expect(engine._rows('sys_file')[0]).toMatchObject({ status: 'pending' }); + }); + + it('GET /files/:fileId/url answers 500 on a read OUTAGE, not 404 FILE_NOT_FOUND', async () => { + const engine = createFakeEngine(); + const store = mount(engine); + await store.createFile({ id: 'f1', key: 'user/f1.txt', name: 'f1.txt', status: 'committed' }); + engine._setFailing('findOne', true); + + const handler = httpServer._getHandler('GET', '/api/v1/storage/files/:fileId/url')!; + const res = createMockRes(); + await handler(createMockReq({ params: { fileId: 'f1' } }), res); + + expect(res._status).toBe(500); + expect(res._json.error.code).toBe('INTERNAL'); + expect(res._json.error.message).toContain('NOT the same as it being absent'); + }); + + it('GET /files/:fileId/url still answers 404 on a genuine MISS', async () => { + mount(createFakeEngine()); + + const handler = httpServer._getHandler('GET', '/api/v1/storage/files/:fileId/url')!; + const res = createMockRes(); + await handler(createMockReq({ params: { fileId: 'never-existed' } }), res); + + expect(res._status).toBe(404); + expect(res._json.error.code).toBe('FILE_NOT_FOUND'); + }); + + it('GET /upload/chunked/:uploadId/progress answers 500 on a read outage, 404 on a miss', async () => { + const engine = createFakeEngine(); + const store = mount(engine); + await store.createSession({ + id: 'u1', + file_id: 'f1', + key: 'user/f1.bin', + filename: 'f1.bin', + total_size: 100, + chunk_size: 50, + total_chunks: 2, + status: 'in_progress', + }); + + const handler = httpServer._getHandler('GET', '/api/v1/storage/upload/chunked/:uploadId/progress')!; + + const miss = createMockRes(); + await handler(createMockReq({ params: { uploadId: 'nope' } }), miss); + expect(miss._status).toBe(404); + expect(miss._json.error.code).toBe('UPLOAD_SESSION_NOT_FOUND'); + + engine._setFailing('findOne', true); + const outage = createMockRes(); + await handler(createMockReq({ params: { uploadId: 'u1' } }), outage); + expect(outage._status).toBe(500); + expect(outage._json.error.message).toContain('abort a live upload'); + }); + + it('with NO engine wired the routes keep their in-memory behaviour', async () => { + const store = new StorageMetadataStore(null); + registerStorageRoutes(httpServer as any, adapter, store, { basePath: '/api/v1/storage' }); + + const presigned = httpServer._getHandler('POST', '/api/v1/storage/upload/presigned')!; + const res = createMockRes(); + await presigned( + createMockReq({ body: { filename: 'photo.jpg', mimeType: 'image/jpeg', size: 1024 } }), + res, + ); + + expect(res._status).toBe(200); + expect(res._json.success).toBe(true); + expect(await store.getFile(res._json.data.fileId)).toMatchObject({ status: 'pending' }); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7daa8feacb..0103aa1ce7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2310,6 +2310,9 @@ importers: specifier: workspace:* version: link:../../types devDependencies: + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@types/node': specifier: ^26.1.2 version: 26.1.2