diff --git a/.changeset/durability-swallow-batch-7.md b/.changeset/durability-swallow-batch-7.md new file mode 100644 index 0000000000..5c265fe97d --- /dev/null +++ b/.changeset/durability-swallow-batch-7.md @@ -0,0 +1,58 @@ +--- +'@objectstack/metadata-protocol': minor +'@objectstack/service-storage': minor +--- + +Report the refused writes three `catch { }` sites swallowed (#12981 batch 7) + +Three tier-1 DARK sites from the #12981 swallow-family worklist, across two +packages. Control flow is unchanged at every one of them — none of these +failures should abort the operation it sits inside — but none of them is silent +any more. + +**`metadata-protocol` — `reassignOrphanedMetadata` (the durability one).** +ADR-0070 D5's orphan-adoption loop dropped a refused `sys_metadata` update +whole: not logged, not rethrown, not carried on the response. The return line +reports `success: reassigned.length > 0`, so an adoption in which 99 of 100 +orphans were refused answered `{ success: true, reassignedCount: 1 }` — a +response identical in shape to a healthy run with one orphan to move — while +the 99 stayed orphans, with nothing retrying them and no record that they had +been tried. The loop now counts refusals and states the degradation **once** +after the loop at `console.error`, naming the count, the target package, the +driver's own sentence and the fix. `error` and not this file's usual +`console.warn`, by the AGENTS.md question this turns on: the system keeps +looking normal while something it claims to have persisted did not land. It is +the verdict `recordPackageCommit` in the same file already reaches on the same +sink, and the inverse of the one `clientFacingRowFailureText` records for its +`console.warn` (there the row reports `success: false` and the counters +reconcile; here neither holds). The response shape is untouched — no +`failedCount` was added. + +**`service-storage` — two sites at the tail of `StorageServicePlugin.start()`, +both functional, both `warn`.** + +- The settings-namespace binding ended in `catch { }` with a comment naming + only one of the two outcomes it caught. The settings service being **absent** + (a bare kernel, where nothing ever claimed the admin UI could swap adapters) + is now resolved on its own line and stays correctly silent; a binding that + **fails with the service present** is reported, because `start()` otherwise + completes into a healthy-looking boot whose storage settings screen is wired + to nothing — an operator's adapter or credential change is saved and never + applied. +- The `storage/test` probe cleanup swallowed its own failure in + `catch { /* ignore */ }`. The result returned beside it reports the *probe's* + failure, which is a different failure: one stray `__objectstack_probe__/…` + key accrued per failed test and the only record of its name died with the + frame. The refused cleanup now names the key it left behind. + +Both `service-storage` sites are `warn` on the merits, not by default: neither +is a durability degradation. Storage keeps serving from the adapter the +plugin's own options built, and the leaked probe object is inert content no +record references — AGENTS.md is explicit that escalating these is what makes +`error` unreadable. No sink type is changed at any of the three sites: the two +`service-storage` reports go to `PluginContext.logger`, whose `error` is +already non-optional, and `metadata-protocol` reports on `console`. + +Each repaired seam is pinned by a test that fails if it goes quiet again, plus +absence-asserting controls — declared as controls — so a seam that reports +unconditionally cannot pass. diff --git a/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts new file mode 100644 index 0000000000..6e415e6b3c --- /dev/null +++ b/packages/metadata-protocol/src/protocol.reassign-orphans-durability.test.ts @@ -0,0 +1,247 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12981 batch 7] A refused orphan rebind in `reassignOrphanedMetadata` must + * not be silent. + * + * ADR-0070 D5's adoption loop walked every package-less `sys_metadata` row and + * rebound it to a target base. Its `catch` was bare: + * + * } catch { + * /* skip a row that fails to update; report only what moved *\/ + * } + * + * and the return line below it reports `success: reassigned.length > 0`. Put + * together, an adoption in which 99 of 100 orphans were REFUSED answered + * `{ success: true, reassignedCount: 1 }` — a response byte-identical in shape + * to a healthy run that had exactly one orphan to move. The 99 stayed orphans, + * nothing retried them, and no line anywhere recorded that they had been tried. + * That is the AGENTS.md durability shape exactly: the system keeps looking + * normal while something it claims to have persisted did not land. + * + * ## What this file pins, and what it deliberately does NOT + * + * ONLY the silence changes. The loop must still skip the refused row and adopt + * the rest — aborting the adoption over one unwritable row would strand the + * rows that CAN move — and the response shape is untouched, because adding a + * `failedCount` is a contract change this card does not carry. Both halves are + * asserted below rather than assumed. + * + * ## The level, stated so it can be argued with + * + * `console.error`, not `console.warn`, and not by default: `console.warn` is + * this file's overwhelming idiom (51 sites) and `clientFacingRowFailureText` + * records the discriminator in prose — it chose `warn` "deliberately — nothing + * claimed to be persisted was silently dropped (the row reports + * `success: false` and the counters reconcile)". Here NEITHER holds. The + * matching precedent is in this same file: `recordPackageCommit` already + * answers `console.error` for a refused `sys_metadata_commit` write under a + * publish that reports success. + * + * ## ONE line, not one per row + * + * AGENTS.md → "Degradation log levels" requires the report be stated once, at + * the first occurrence, not once per failed write — and a `sys_metadata` write + * that is refused is refused for every row, so the per-row spelling would print + * one line per orphan in the environment. The count is pinned, not just the + * presence. + * + * ⚠️ Two cases below are CONTROLS, not pins: they assert an ABSENCE against a + * seam that logged nothing at all before this repair, so they stay green in + * both directions by construction and are not evidence in an ablation. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +// [#5619] The producer's OWN write-verb dispatch decision, so this double +// cannot accept an `update` shape ObjectQL refuses. From +// `@objectstack/metadata-core` and NOT `@objectstack/objectql` — objectql +// depends on THIS package, so that import would close a cycle turbo rejects. +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +interface MetaRow { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; +} + +/** + * Engine double over `sys_metadata` with a per-id refusal injector on `update`. + * + * The injection is keyed by ROW ID rather than being a global switch, because + * the defect's dangerous case is the PARTIAL one: an adoption where some rows + * move and some do not is the run that answers `success: true` while leaving + * orphans behind. A double that could only fail everything could not express + * it. + */ +function makeEngine(rows: MetaRow[]) { + const store = new Map(rows.map((r) => [r.id, { ...r }])); + const refuse = new Set(); + let updateAttempts = 0; + + const engine = { + async find(table: string, opts?: { where?: Record; limit?: number }) { + if (table !== 'sys_metadata') return []; + // This double implements NEITHER a `where` combinator NOR a bound, + // and REFUSES both rather than answering them silently. Every case + // in this file adopts env-wide orphans, so the producer passes + // `{ where: {} }` and no `limit`; the org-scoped `$or` branch and + // paging belong to tests that do not exist yet. A double looser + // than the engine it stands in for converts a green suite into no + // suite at all (#4434) — and the reason to refuse rather than + // approximate is that the approximation is invisible on the day the + // producer starts using the shape. + const where = opts?.where ?? {}; + if (Object.keys(where).length > 0) { + throw new Error(`fake engine: unsupported where ${JSON.stringify(where)}`); + } + if (opts?.limit !== undefined) { + throw new Error('fake engine: unsupported `limit` — this double holds no bound'); + } + return [...store.values()]; + }, + async update(_table: string, data: Record, opts: { where: Record }) { + assertEngineUpdateDispatch(data, opts); + updateAttempts += 1; + const id = String(opts.where.id); + if (refuse.has(id)) throw new Error(`write refused for ${id}: permission denied on sys_metadata`); + const row = store.get(id); + if (!row) return { id: null }; + Object.assign(row, data); + return { id }; + }, + }; + + return { + engine, + store, + refuseIds: (...ids: string[]) => ids.forEach((i) => refuse.add(i)), + updateAttempts: () => updateAttempts, + }; +} + +const orphan = (id: string): MetaRow => ({ + id, + type: 'object', + name: `obj_${id}`, + organization_id: null, + package_id: null, +}); + +/** The one sentence fragment an operator greps for. */ +const HEADLINE = 'orphaned metadata row(s) were NOT rebound'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +function spyConsole() { + return { + error: vi.spyOn(console, 'error').mockImplementation(() => {}), + warn: vi.spyOn(console, 'warn').mockImplementation(() => {}), + }; +} + +describe('reassignOrphanedMetadata: a refused rebind is reported (#12981)', () => { + // ⚠️ CONTROL, not a pin. Before the repair this seam logged nothing at any + // level, so "a healthy adoption says nothing" was already true — it stays + // green in BOTH directions and is not ablation evidence. It is here so the + // pins below cannot pass on a seam that reports unconditionally. + it('CONTROL: an adoption in which every row rebinds reports nothing', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b')]); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.reassignedCount).toBe(2); + expect(res.success).toBe(true); + expect(stub.updateAttempts()).toBe(2); + expect(spy.error).not.toHaveBeenCalled(); + expect(spy.warn).not.toHaveBeenCalled(); + }); + + it('reports a PARTIAL adoption — the run that still answers success: true', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b'), orphan('c')]); + stub.refuseIds('b', 'c'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + // Proof the writes were really attempted and really threw — otherwise + // every assertion below is about an adoption that never reached the seam. + expect(stub.updateAttempts()).toBe(3); + expect(stub.store.get('b')!.package_id).toBeNull(); + expect(stub.store.get('c')!.package_id).toBeNull(); + + // ⛔ The response is UNCHANGED: this is the shape that reads healthy. + expect(res.success).toBe(true); + expect(res.reassignedCount).toBe(1); + expect(res.reassigned).toEqual([{ type: 'object', name: 'obj_a' }]); + + // …and it is no longer the only thing that happened. + expect(spy.error).toHaveBeenCalledTimes(1); + const line = String(spy.error.mock.calls[0][0]); + expect(line).toContain(HEADLINE); + expect(line).toContain('2 of 3'); + expect(line).toContain('app.base'); + // The consequence and the fix, which AGENTS.md requires of this level. + expect(line).toContain('STILL orphans'); + expect(line).toContain('Fix: restore write access'); + // The driver's own sentence, so the operator is not left guessing why. + expect(line).toContain('permission denied on sys_metadata'); + }); + + it('reports a TOTAL refusal, where the response already says success: false', async () => { + const spy = spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b')]); + stub.refuseIds('a', 'b'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.success).toBe(false); + expect(res.reassignedCount).toBe(0); + expect(spy.error).toHaveBeenCalledTimes(1); + expect(String(spy.error.mock.calls[0][0])).toContain('2 of 2'); + }); + + it('states the degradation ONCE, not once per refused row', async () => { + const spy = spyConsole(); + const ids = ['a', 'b', 'c', 'd', 'e', 'f']; + const stub = makeEngine(ids.map(orphan)); + stub.refuseIds(...ids); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + // Six refused writes, ONE operator-facing line — AGENTS.md's "say it + // once, at the first degradation, not once per failed write". + expect(stub.updateAttempts()).toBe(6); + expect(spy.error).toHaveBeenCalledTimes(1); + expect(String(spy.error.mock.calls[0][0])).toContain('6 of 6'); + }); + + // ⚠️ CONTROL, not a pin — an INVARIANCE assertion. The pre-repair code + // returns exactly this too, so it stays green in both directions. It is + // here because the repair would be wrong if it changed control flow. + it('CONTROL: a refused row does not abort the rows that can still move', async () => { + spyConsole(); + const stub = makeEngine([orphan('a'), orphan('b'), orphan('c')]); + stub.refuseIds('a'); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + const res = await protocol.reassignOrphanedMetadata({ targetPackageId: 'app.base' }); + + expect(res.reassignedCount).toBe(2); + expect(stub.store.get('b')!.package_id).toBe('app.base'); + expect(stub.store.get('c')!.package_id).toBe('app.base'); + // The response shape is untouched: no `failedCount` was added. + expect(Object.keys(res).sort()).toEqual( + ['reassigned', 'reassignedCount', 'success', 'targetPackageId'].sort(), + ); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7e3ddb6d97..03be73cf57 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -17815,6 +17815,28 @@ export class ObjectStackProtocolImplementation implements ); const reassigned: Array<{ type: string; name: string }> = []; + // [#12981] The refusal channel this loop used to lack entirely. + // + // The catch below is the card's defining shape, and this method is the + // sharpest instance of it in the file: a refused `update` was dropped + // whole -- not logged, not rethrown, not carried on the response -- and + // the return then reported `success: reassigned.length > 0`. So an + // adoption where 99 of 100 orphans were refused answered + // `{success: true, reassignedCount: 1}`, byte-identical in shape to a + // healthy run with one orphan to move, and the 99 stayed orphans with + // nothing anywhere recording that they had been tried. Nothing retries + // them and no later boot reconstructs the attempt. + // + // A COUNTER plus one report AFTER the loop, deliberately, and not a + // `console.error` inside the catch: AGENTS.md -> "Degradation log + // levels" says an operator-facing degradation is stated ONCE, at the + // first occurrence, not once per failed write -- and a refused + // `sys_metadata` write is refused for every row, so the per-row + // spelling would print one line per orphan in the environment. This is + // the same shape #12923's shared refusal accumulator takes at the other + // repaired seams of this family. + let refusedCount = 0; + let firstRefusal = ''; for (const row of orphans) { try { await this.engine.update( @@ -17823,10 +17845,37 @@ export class ObjectStackProtocolImplementation implements { where: { id: row.id } }, ); reassigned.push({ type: row.type, name: row.name }); - } catch { - /* skip a row that fails to update; report only what moved */ + } catch (e: any) { + // Control flow is UNCHANGED: a row that cannot be rebound must + // not abort the adoption of the rows that can. Only the + // silence changes. + refusedCount += 1; + if (firstRefusal === '') firstRefusal = e?.message ?? String(e); } } + if (refusedCount > 0) { + // `error` and not `warn`, by the one question AGENTS.md turns this + // on -- after the degradation the system still looks normal from + // the outside while something it claims to have persisted did not + // land. It is the same verdict, on the same sink, that + // `recordPackageCommit` in this file already reaches for the + // `sys_metadata_commit` write, and the inverse of the one + // `clientFacingRowFailureText` records for its `console.warn` + // (there the row reports `success: false` and the counters + // reconcile, so nothing was silently dropped; here neither holds). + console.error( + `[Protocol] reassignOrphanedMetadata: ${refusedCount} of ${orphans.length} orphaned ` + + `metadata row(s) were NOT rebound to package '${request.targetPackageId}' -- the ` + + `update was REFUSED. The call still answers reassignedCount=${reassigned.length}` + + `${reassigned.length > 0 ? ' with success: true' : ''}, so nothing looks broken, but ` + + 'those rows are STILL orphans: they keep `package_id` null or the `sys_metadata` ' + + 'sentinel, this environment has NOT converged on the package-first model (ADR-0070 ' + + 'D5 completes when an environment has no orphans), and nothing retries them. ' + + `First refusal: ${firstRefusal}. Fix: restore write access to \`sys_metadata\` for ` + + 'the system context and run the adoption again -- it is idempotent, rows already ' + + 'bound to a real package are left untouched.', + ); + } return { success: reassigned.length > 0, reassignedCount: reassigned.length, diff --git a/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts new file mode 100644 index 0000000000..d191546357 --- /dev/null +++ b/packages/services/service-storage/src/storage-service-plugin.durability-swallow-repair.test.ts @@ -0,0 +1,224 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12981 batch 7] The two silent `catch` blocks at the tail of + * `StorageServicePlugin.start()`. + * + * ## Site A — the settings-namespace binding + * + * One `try` wrapped the whole "bind to the `storage` settings namespace" block + * and ended in `catch { // settings service not present }`. The comment named + * one outcome; the catch caught two: + * + * - the settings service is ABSENT (a bare kernel) — nothing ever claimed the + * admin UI could swap adapters, so silence is correct and reporting it + * would put a line on every bare-kernel boot; + * - the settings service is PRESENT and the binding FAILED part-way — + * `subscribe` or `registerAction` threw. `start()` then completes, the + * kernel reports a healthy boot, and the storage settings screen is wired + * to nothing: an operator's adapter change is saved and never applied. + * + * Resolving `getService('settings')` on its own line splits them, so the + * remaining catch covers exactly the second. + * + * ## Site B — the `storage/test` probe cleanup + * + * `try { await proxy.delete(probeKey); } catch { /* ignore *\/ }`. The `return` + * beside it reports the PROBE's failure — a different failure. The cleanup's + * own failure reached nobody, so one stray `__objectstack_probe__/…` key + * accrued per failed test and the only record of its name (minted per call from + * a timestamp plus a random suffix) died with the frame. + * + * ## The level at BOTH sites: `warn`, and decided on the merits + * + * Neither is a durability degradation. Site A is textbook FUNCTIONAL under + * AGENTS.md — "a capability is not enabled… the next person to use the missing + * thing finds out" — and storage itself keeps serving from the adapter the + * plugin's own options built. Site B is housekeeping that did not happen; the + * leaked object is inert probe content no record references. AGENTS.md is + * explicit that escalating these is what makes `error` unreadable. + * + * ⚠️ Two cases below are CONTROLS, not pins: they assert an ABSENCE against + * seams that logged nothing at all before this repair, so they stay green in + * both directions by construction and are not ablation evidence. + */ + +import { describe, it, expect } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IStorageService } from '@objectstack/spec/contracts'; +import { StorageServicePlugin } from './storage-service-plugin.js'; +import type { SwappableStorageService } from './swappable-storage-service.js'; + +function makeCtx() { + const services = new Map(); + const hooks: Array<() => Promise | void> = []; + const logs: { info: string[]; warn: string[]; error: string[] } = { info: [], warn: [], error: [] }; + const ctx: any = { + logger: { + info: (m: string) => { logs.info.push(String(m)); }, + warn: (m: string) => { logs.warn.push(String(m)); }, + error: (m: string) => { logs.error.push(String(m)); }, + }, + _logs: logs, + registerService: (name: string, svc: any) => { services.set(name, svc); }, + getService: (name: string): T => { + const s = services.get(name); + if (!s) throw new Error(`service '${name}' not registered`); + return s as T; + }, + hook: (event: string, fn: () => Promise | void) => { + if (event === 'kernel:ready') hooks.push(fn); + }, + _flushReady: async () => { for (const h of hooks) await h(); }, + }; + return ctx; +} + +/** + * A settings service whose `registerAction` can be made to throw. + * + * `registerAction` and not `getNamespace`, deliberately: `applySettings` has + * its own inner catch that already warns, so a failure there was never part of + * this family. `registerAction` is reached only through the OUTER try, which is + * the seam under test. + */ +function makeFakeSettings(opts: { failRegisterAction?: Error } = {}) { + const actions = new Map Promise>(); + return { + createClient: (_ns: string) => ({}), + getNamespace: async (_ns: string) => ({ values: {} }), + subscribe: (_ns: string, _fn: () => void) => {}, + registerAction: (ns: string, id: string, fn: (input: any) => Promise) => { + if (opts.failRegisterAction) throw opts.failRegisterAction; + actions.set(`${ns}/${id}`, fn); + }, + _runAction: async (ns: string, id: string, input: any) => { + const fn = actions.get(`${ns}/${id}`); + if (!fn) throw new Error(`no action ${ns}/${id}`); + return await fn(input); + }, + }; +} + +/** An adapter that refuses the verbs a test names, and counts what was tried. */ +function makeBrokenAdapter(refuse: { upload?: boolean; delete?: boolean }) { + const tried = { upload: 0, delete: 0 }; + const adapter = { + async upload(_k: string, _b: Buffer, _o?: unknown) { + tried.upload += 1; + if (refuse.upload) throw new Error('upload refused: bucket is read-only'); + return { key: _k } as never; + }, + async download(_k: string) { return Buffer.from('probe', 'utf-8'); }, + async delete(_k: string) { + tried.delete += 1; + if (refuse.delete) throw new Error('delete refused: bucket is read-only'); + }, + async exists(_k: string) { return true; }, + async list() { return { items: [] } as never; }, + async getUrl(k: string) { return `mem://${k}`; }, + }; + return { adapter: adapter as unknown as IStorageService, tried }; +} + +async function bootedPlugin(settings: unknown) { + const dir = await fs.mkdtemp(join(tmpdir(), 'oss-b7-')); + const plugin = new StorageServicePlugin({ + adapter: 'local', + local: { rootDir: dir }, + registerRoutes: false, + }); + const ctx = makeCtx(); + if (settings) ctx.registerService('settings', settings); + await plugin.init(ctx); + await plugin.start(ctx); + await ctx._flushReady(); + // Typed HERE rather than at each call site: the fake ctx is `any`, so + // `ctx.getService(…)` is a type argument on an untyped call (TS2347) and + // `as any` would be a slot-lookup erasure (#4251). A plain typed assignment + // is neither. + const storage: SwappableStorageService = ctx.getService('storage'); + return { plugin, ctx, dir, storage }; +} + +const BINDING_HEADLINE = 'settings namespace binding FAILED'; +const CLEANUP_HEADLINE = 'was NOT removed'; + +describe('StorageServicePlugin site A: settings binding (#12981)', () => { + // ⚠️ CONTROL, not a pin. The pre-repair code was silent here too, so this + // stays green in BOTH directions. It exists so the pin below cannot pass on + // a seam that reports unconditionally — and it pins the DIRECTION of the + // split: absence must stay silent. + it('CONTROL: a bare kernel with no settings service stays silent, and still starts', async () => { + const { ctx } = await bootedPlugin(null); + + expect(ctx._logs.warn.join('\n')).not.toContain(BINDING_HEADLINE); + expect(ctx._logs.error).toEqual([]); + // Storage itself is up: the absence really is only functional. + expect(ctx.getService('storage')).toBeTruthy(); + }); + + it('a binding that FAILS with the service PRESENT is reported, and names what stops working', async () => { + const boom = new Error('settings action registry is sealed'); + const settings = makeFakeSettings({ failRegisterAction: boom }); + + const { ctx } = await bootedPlugin(settings); + + const warned = ctx._logs.warn.filter((l: string) => l.includes(BINDING_HEADLINE)); + expect(warned).toHaveLength(1); + // The driver's own sentence, so the operator is not left guessing. + expect(warned[0]).toContain('settings action registry is sealed'); + // The consequence, concretely — and that the boot still looks healthy. + expect(warned[0]).toContain('will be saved but NOT applied'); + // The fix, which is the other half AGENTS.md requires of a degradation line. + expect(warned[0]).toContain('bindToSettings: false'); + // ⛔ Control flow unchanged: a failed binding must not fail the boot. + expect(ctx.getService('storage')).toBeTruthy(); + expect(ctx._logs.error).toEqual([]); + }); +}); + +describe('StorageServicePlugin site B: storage/test probe cleanup (#12981)', () => { + it('a refused probe cleanup is reported, and names the stray key', async () => { + const settings = makeFakeSettings(); + const { ctx, storage } = await bootedPlugin(settings); + const broken = makeBrokenAdapter({ upload: true, delete: true }); + storage.swap(broken.adapter); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + // Proof the seam was really reached: the probe upload threw and the + // cleanup delete was really attempted and really refused. + expect(broken.tried.upload).toBe(1); + expect(broken.tried.delete).toBe(1); + + const warned = ctx._logs.warn.filter((l: string) => l.includes(CLEANUP_HEADLINE)); + expect(warned).toHaveLength(1); + expect(warned[0]).toContain('__objectstack_probe__/'); + expect(warned[0]).toContain('delete refused: bucket is read-only'); + + // ⛔ The probe RESULT is untouched — the operator still reads the original + // failure, not the cleanup's. + expect(result.ok).toBe(false); + expect(result.severity).toBe('error'); + expect(result.message).toBe('upload refused: bucket is read-only'); + }); + + // ⚠️ CONTROL, not a pin — green in both directions, because the pre-repair + // code also said nothing when the cleanup succeeded. It is here so the pin + // above cannot pass on a seam that warns on every failed probe. + it('CONTROL: a probe failure whose cleanup SUCCEEDS reports nothing on that channel', async () => { + const settings = makeFakeSettings(); + const { ctx, storage } = await bootedPlugin(settings); + const broken = makeBrokenAdapter({ upload: true, delete: false }); + storage.swap(broken.adapter); + + const result = await settings._runAction('storage', 'test', { values: {} }); + + expect(broken.tried.delete).toBe(1); + expect(result.ok).toBe(false); + expect(ctx._logs.warn.join('\n')).not.toContain(CLEANUP_HEADLINE); + }); +}); diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 96f307ad68..51f623729c 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -458,9 +458,24 @@ export class StorageServicePlugin implements Plugin { // Allows the admin UI to swap adapters / credentials without // restart. Env-locked fields still win at the resolver layer. if (this.options.bindToSettings === false) return; + // [#12981] The settings service being ABSENT and the settings binding + // FAILING are two different outcomes, and one `try` used to spell them + // identically -- `catch { }` with a comment that named only the first. + // Resolving the service on its own line leaves the catch below covering + // exactly the second. + let settings: StorageSettingsSlot | undefined; + try { + settings = ctx.getService('settings'); + } catch { + // A DECLARED absence, and silence is correct here: a bare kernel + // registers no `settings` service, so nothing ever claimed the admin + // UI could swap adapters at runtime, and the manifest fallback handler + // stays. Reporting it would put a line on every bare-kernel boot -- + // the "warn nobody reads" that makes the real one below unreadable. + return; + } + if (!settings || typeof settings.createClient !== 'function') return; try { - const settings = ctx.getService('settings'); - if (!settings || typeof settings.createClient !== 'function') return; const applySettings = async () => { if (!this.storage) return; @@ -554,20 +569,89 @@ export class StorageServicePlugin implements Plugin { message: `Storage round-trip succeeded (adapter=${adapter}).`, }; } catch (err: any) { - // Best-effort cleanup - try { await (proxy as IStorageService).delete(probeKey); } catch { /* ignore */ } + // Best-effort cleanup. [#12981] The cleanup's OWN failure used to + // be swallowed by `catch { /* ignore */ }`. The `return` below + // reports the PROBE's failure, which is a different failure: the + // operator is told the round-trip did not work and is told + // nothing at all about the probe object that is still sitting in + // the bucket. One stray key accrues per failed test, with the + // only record of its name -- `probeKey` is minted per call from a + // timestamp and a random suffix -- lost when this frame returns. + // + // `warn`, not `error`: nothing the system claims to a caller as + // persisted was lost. This is housekeeping that did not happen, + // and the AGENTS.md rule is explicit that escalating a + // non-durability degradation is how `error` stops being read. + try { + await (proxy as IStorageService).delete(probeKey); + } catch (cleanupErr: any) { + ctx.logger.warn( + `StorageServicePlugin: the storage/test probe object '${probeKey}' was NOT removed — ` + + `the cleanup delete was refused (${cleanupErr?.message ?? cleanupErr}). The probe ` + + 'result returned below is unaffected and still reports the original failure. ' + + 'This object is inert probe content that no record references and nothing ' + + 'retries or later collects; delete it by hand if stray keys under ' + + '`__objectstack_probe__/` matter in this bucket.', + ); + } return { ok: false, severity: 'error', message: err?.message ?? String(err) }; } }); ctx.logger.info('StorageServicePlugin: registered settings action storage/test'); } - } catch { - // settings service not present — manifest fallback handler stays + } catch (err: any) { + // [#12981] Reached only when the settings service IS present and the + // binding above failed part-way -- `subscribe` or `registerAction` + // threw, or `applySettings`'s first call did. The plugin then finishes + // `start()` and the kernel reports a healthy boot, while the storage + // settings screen is wired to nothing: an operator's adapter or + // credential change is accepted by the form and never reaches this + // service, and the `storage/test` button answers "no action". + // + // `warn` and not `error`, by the AGENTS.md question: this is a + // FUNCTIONAL degradation, not a durability one. Nothing claimed to be + // persisted was dropped -- the adapter this plugin constructed keeps + // serving every upload and download, and the next person to use the + // missing capability finds out. Escalating it would train readers to + // skim `error`, which is what made the founding incident's `warn` + // unreadable. + ctx.logger.warn( + 'StorageServicePlugin: the `storage` settings namespace binding FAILED — ' + + (err?.message ?? String(err)) + + '. The settings service IS present, so this is not a bare kernel: adapter and ' + + 'credential changes made in the admin UI will be saved but NOT applied to this ' + + 'service, and the live `storage/test` probe may be unregistered. Storage itself is ' + + 'unaffected — the adapter built from this plugin\'s own options keeps serving. Fix: ' + + 'restart after repairing the settings service, or pass `bindToSettings: false` to ' + + 'make the static configuration deliberate.', + ); } }); } } +/** + * [#12981] The slice of the `settings` service `start()` uses. + * + * Named rather than erased to `any` because splitting the lookup from the + * declaration is exactly `check:slot-lookup`'s FOURTH erasure shape (#4251): + * `let x: any; try { x = ctx.getService('…'); }` erases the contract in a + * position none of the rule's other selectors reach. Everything past + * `createClient` is feature-detected at its call site, so it is declared + * OPTIONAL here rather than assumed — the declaration says what the code + * actually requires, which is the property that makes it worth writing. + */ +interface StorageSettingsSlot { + createClient(namespace: string): unknown; + getNamespace(namespace: string): Promise<{ values: Record }>; + subscribe?(namespace: string, listener: () => void): void; + registerAction?( + namespace: string, + id: string, + handler: (input: any) => Promise, + ): void; +} + /** * Look up the host's MetricsRegistry from the service registry, with * the canonical fallback chain (explicit override → registered service diff --git a/scripts/slot-lookup-baseline.json b/scripts/slot-lookup-baseline.json index 1c5c897a2c..bdca43723c 100644 --- a/scripts/slot-lookup-baseline.json +++ b/scripts/slot-lookup-baseline.json @@ -20,7 +20,7 @@ "packages/services/service-realtime/src/realtime-service-plugin.ts": 1, "packages/services/service-settings/src/settings-service-plugin.ts": 2, "packages/services/service-sms/src/sms-plugin.ts": 1, - "packages/services/service-storage/src/storage-service-plugin.ts": 6, + "packages/services/service-storage/src/storage-service-plugin.ts": 5, "packages/triggers/trigger-record-change/src/formula-context.test.ts": 2, "packages/triggers/trigger-record-change/src/multilookup-context.test.ts": 2, "packages/triggers/trigger-record-change/src/record-change-integration.test.ts": 11