From 3fecc3eaaaa18b162765eb6c2da7672d681e8696 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 20:09:46 +0000 Subject: [PATCH] fix(service-storage): tombstone attachments on a predicate delete The beforeDelete -> afterDelete file-id hand-off rode on the hook context itself, on the premise that the engine passes the same HookContext to both events. Since #5574 (ADR-0058 Addendum II D1/D2) a predicate write dispatches one fresh context per matched row in each phase, so on a `multi: true` delete the stash never arrived, the orphan list was empty, and no tombstone was ever written -- leaving the file at status='committed' with its bytes stranded permanently, since an untombstoned orphan matches neither declared sweep policy on sys_file. Read the departed id from `ctx.previous.file_id` instead, which the engine binds on both phases and both dispatch paths (by-id unconditionally since #7867, per-row from the batch's doomed-row read). That is the slot the update verb's detach leg already reads, so the module now carries one mechanism for "what file did this join row point at before?" rather than two. The `beforeDelete` registration has no work left and is removed, together with its `MULTI_DELETE_RESOLVE_LIMIT` limb. That limb's unreachability was measured, not assumed: both branches of the live handler were counted while every delete shape the engine offers was driven through the wired engine, and the sibling id-branch (the positive control) fired on all six dispatchable shapes while the where-branch fired zero times -- including on `where: { id: { $in: [...] } }`, the batch-shaped delete it was written for. Fixes #10240 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/silver-pugs-tickle.md | 31 ++ .../src/attachment-lifecycle.test.ts | 275 ++++++++++++++++-- .../src/attachment-lifecycle.ts | 160 +++++----- .../src/storage-service-plugin.test.ts | 13 +- 4 files changed, 388 insertions(+), 91 deletions(-) create mode 100644 .changeset/silver-pugs-tickle.md diff --git a/.changeset/silver-pugs-tickle.md b/.changeset/silver-pugs-tickle.md new file mode 100644 index 0000000000..cb4b10f639 --- /dev/null +++ b/.changeset/silver-pugs-tickle.md @@ -0,0 +1,31 @@ +--- +'@objectstack/service-storage': patch +--- + +Fix attachment tombstoning silently no-opping on a predicate (`multi: true`) delete + +Deleting `sys_attachment` join rows by PREDICATE left the file they referenced at +`status='committed'` with `deleted_at` NULL, even when the deleted row was the +file's last reference. The tombstone hooks handed file ids from `beforeDelete` to +`afterDelete` on the hook context itself, on the premise that the engine passes +the same `HookContext` to both events. Since ADR-0058 Addendum II (D1/D2) a +predicate write dispatches one FRESH context per matched row in each phase, so +that hand-off never arrived and no tombstone was written. + +The bytes were stranded permanently rather than late: `sys_file`'s declared +lifecycle nominates a sweep candidate only via `ttl { field: 'deleted_at' }` or +`retention { onlyWhen: { status: 'pending' } }`, and an untombstoned orphan +matches neither — so the reap guard was never asked about it. The by-id delete +verb, and both dispatch paths of the update verb, were unaffected. + +The departed id now comes from `ctx.previous.file_id`, which the engine binds on +both phases and both dispatch paths — the same slot the update verb's detach leg +already reads. + +**What an upgrader needs to know.** New predicate deletes tombstone correctly +from this version on. Files ALREADY stranded by the old behaviour are not +retro-actively tombstoned by this change: they sit at `status='committed'` with +live storage bytes and no join row, and nothing in the platform sweep will +nominate them. Recovering that existing backlog needs a one-off reconciliation +pass over `sys_file` (attachments-scope, `status='committed'`, zero +`sys_attachment` references) and is deliberately not part of this fix. diff --git a/packages/services/service-storage/src/attachment-lifecycle.test.ts b/packages/services/service-storage/src/attachment-lifecycle.test.ts index 0067c72849..cc9d6b6d48 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.test.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.test.ts @@ -36,6 +36,7 @@ function fakeEngine(seed: { const engine: AttachmentLifecycleEngine & { tables: typeof tables; updates: typeof updates; + rowsMatching(where: Record): Array>; trigger(event: string, ctx: any): Promise; deleteRows(where: Record): void; } = { @@ -60,6 +61,13 @@ function fakeEngine(seed: { }, tables, updates, + /** Row lookup for the DRIVE HELPERS only. Deliberately not routed through + * `engine.find`: a best-effort case sabotages that seam to prove a handler + * survives a driver failure, and a harness that read through it would + * explode before the handler ever ran. */ + rowsMatching(where) { + return tables.sys_attachment.filter((r) => matches(r, where)); + }, async trigger(event, ctx) { for (const h of hooks.get(event) ?? []) await h(ctx); }, @@ -70,14 +78,61 @@ function fakeEngine(seed: { return engine; } -/** Drive a full engine-shaped delete: beforeDelete → row removal → afterDelete - * with ONE shared ctx object (mirrors engine.ts delete()). */ -async function driveDelete(engine: ReturnType, input: any, where: Record) { - const ctx: any = { object: 'sys_attachment', event: 'beforeDelete', input }; - await engine.trigger('beforeDelete', ctx); +/** + * Drive an engine-shaped DELETE. Both dispatch shapes `engine.ts delete()` + * produces are covered, because the difference between them is the whole + * subject of #10240: + * + * - by-id (`dispatch.mode === 'record'`): ONE HookContext across both phases, + * `previous` bound to the doomed row's pre-image — read UNCONDITIONALLY + * since #7867 (it is the read that also produces the 404, so it is never + * skipped). + * - predicate (`dispatch.mode === 'per-row'`, #5574 / ADR-0058 Addendum II + * D1/D2): one FRESH context per matched row in EACH phase, `previous` = + * that row, served from the single doomed-row read (#5038 D7). + * + * The freshness is modelled deliberately and is what a stash-based handler + * dies on: nothing a `before*` handler writes onto its own row context can + * reach the `after*` phase. + */ +async function driveDelete( + engine: ReturnType, + where: Record, + mode: 'record' | 'per-row' = 'record', +) { + const doomed = engine.rowsMatching(where).map((r) => ({ ...r })); + const options = mode === 'record' ? { where } : { multi: true, where }; + + if (mode === 'record') { + const ctx: any = { + object: 'sys_attachment', event: 'beforeDelete', + input: { id: where.id, options }, + previous: doomed[0], + dispatch: { mode, index: 0, scope: {} }, + }; + await engine.trigger('beforeDelete', ctx); + engine.deleteRows(where); + ctx.event = 'afterDelete'; + await engine.trigger('afterDelete', ctx); + return; + } + + for (let i = 0; i < doomed.length; i++) { + await engine.trigger('beforeDelete', { + object: 'sys_attachment', event: 'beforeDelete', + input: { id: doomed[i].id, options }, previous: doomed[i], + dispatch: { mode, index: i, scope: {} }, + }); + } engine.deleteRows(where); - ctx.event = 'afterDelete'; - await engine.trigger('afterDelete', ctx); + for (let i = 0; i < doomed.length; i++) { + // A per-row after-context is a FRESH object, never the before one. + await engine.trigger('afterDelete', { + object: 'sys_attachment', event: 'afterDelete', + input: { id: doomed[i].id, options }, previous: { ...doomed[i] }, + dispatch: { mode, index: i, scope: {} }, result: undefined, + }); + } } /** @@ -152,7 +207,7 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { }); installAttachmentLifecycleHooks(engine, silentLogger()); - await driveDelete(engine, { id: 'a1', options: {} }, { id: 'a1' }); + await driveDelete(engine, { id: 'a1' }); expect(engine.updates).toHaveLength(1); expect(engine.updates[0].data).toMatchObject({ id: 'f1', status: 'deleted' }); @@ -169,13 +224,19 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { }); installAttachmentLifecycleHooks(engine, silentLogger()); - await driveDelete(engine, { id: 'a1', options: {} }, { id: 'a1' }); + await driveDelete(engine, { id: 'a1' }); expect(engine.updates).toHaveLength(0); expect(engine.tables.sys_file[0].status).toBe('committed'); }); - it('resolves every affected file on a multi-delete (options.where)', async () => { + // [#10240] The case the module was silently blind to. The handler used to + // hand ids from `beforeDelete` to `afterDelete` on the context itself; a + // predicate delete gives each row a FRESH context per phase, so that stash + // never arrived and NO tombstone was written — a permanent strand, because + // an untombstoned orphan matches neither declared sweep policy on `sys_file`. + // Pinned on the fake here and on the wired engine at the bottom of the file. + it('[#10240] tombstones on the PREDICATE path — where a beforeDelete stash was lost', async () => { const engine = fakeEngine({ attachments: [ { id: 'a1', file_id: 'f1', parent_id: 'p1' }, @@ -186,13 +247,68 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { }); installAttachmentLifecycleHooks(engine, silentLogger()); - await driveDelete( - engine, - { id: undefined, options: { where: { parent_id: 'p1' }, multi: true } }, - { parent_id: 'p1' }, - ); + await driveDelete(engine, { parent_id: 'p1' }, 'per-row'); + // Every affected file is still resolved — and only the one that lost its + // LAST reference is tombstoned. expect(engine.updates.map((u) => u.data.id)).toEqual(['f1']); + expect(engine.tables.sys_file.find((f) => f.id === 'f1')).toMatchObject({ status: 'deleted' }); + expect(engine.tables.sys_file.find((f) => f.id === 'f2')).toMatchObject({ status: 'committed' }); + }); + + // [#10240] The other direction of the same pin. An implementation that only + // handled the multi path would pass the case above and fail this one, so + // both are kept — the by-id verb is the one that already worked. + it('[#10240] a by-id delete still tombstones (the path that already worked)', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f1', parent_id: 'p1' }], + files: [committedFile('f1')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveDelete(engine, { id: 'a1' }); + + expect(engine.updates.map((u) => u.data.id)).toEqual(['f1']); + }); + + // [#10240] The fail-safe direction, chosen to match the `afterUpdate` leg: + // no pre-image means the departed id is unknowable, so tombstone NOTHING and + // keep the file. A missed tombstone leaves an orphan lingering; a tombstone + // written off a guess puts real bytes on the reap path. + it('[#10240] no pre-image → tombstones nothing (fail toward retention)', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f1' }], + files: [committedFile('f1')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + engine.deleteRows({ id: 'a1' }); + await engine.trigger('afterDelete', { + object: 'sys_attachment', event: 'afterDelete', + input: { id: 'a1', options: { where: { id: 'a1' } } }, + dispatch: { mode: 'record', index: 0, scope: {} }, + // no `previous` + }); + + expect(engine.updates).toHaveLength(0); + expect(engine.tables.sys_file[0].status).toBe('committed'); + }); + + // [#10240] `beforeDelete` no longer carries any lifecycle work, so the + // module must not register one — a registration that stashes nothing is the + // second mechanism this card collapsed. + it('[#10240] registers no beforeDelete hook at all', async () => { + const events: string[] = []; + const engine = fakeEngine({ attachments: [], files: [] }); + const realRegister = engine.registerHook.bind(engine); + engine.registerHook = (event, handler, opts) => { + events.push(event); + return realRegister(event, handler, opts); + }; + installAttachmentLifecycleHooks(engine, silentLogger()); + + expect(events).not.toContain('beforeDelete'); + expect(events).toContain('afterDelete'); }); it('never tombstones non-attachments scopes (Field.file/avatar protection)', async () => { @@ -202,7 +318,7 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { }); installAttachmentLifecycleHooks(engine, silentLogger()); - await driveDelete(engine, { id: 'a1', options: {} }, { id: 'a1' }); + await driveDelete(engine, { id: 'a1' }); expect(engine.updates).toHaveLength(0); }); @@ -275,14 +391,16 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => { }); it('a failing lookup never blocks the delete (best-effort)', async () => { - const engine = fakeEngine({ attachments: [], files: [] }); - engine.findOne = async () => { + const engine = fakeEngine({ attachments: [{ id: 'a1', file_id: 'f1' }], files: [committedFile('f1')] }); + // The reference count is what the handler asks for first now that the + // departed id arrives on `ctx.previous` instead of a beforeDelete lookup. + engine.find = async () => { throw new Error('driver exploded'); }; const logger = silentLogger(); installAttachmentLifecycleHooks(engine, logger); - await expect(driveDelete(engine, { id: 'a1', options: {} }, { id: 'a1' })).resolves.toBeUndefined(); + await expect(driveDelete(engine, { id: 'a1' })).resolves.toBeUndefined(); expect(logger.warn).toHaveBeenCalled(); }); }); @@ -883,3 +1001,122 @@ describe('[#10171] the update leg through the wired engine', () => { expect(file('f1')).toMatchObject({ id: 'f1', status: 'deleted' }); }); }); + +/* ──────────────────────────────────────────────────────────────────────────── + * #10240 — the DELETE verb through the WIRED engine + * + * The card's own measurement, kept executable. Before this change the tree + * produced, in ONE run: + * + * DELETE by-id (dispatch record) : f1 -> status "deleted" ✅ + * DELETE predicate (dispatch per-row): f1 -> status "committed" ❌ + * UPDATE by-id (dispatch record) : f_old -> status "deleted" ✅ + * UPDATE predicate (dispatch per-row): f_old -> status "deleted" ✅ + * + * — one verb fixed by #10171, the other still leaking through per-row + * dispatch. The fakes above model that dispatch shape; these cases check the + * model against real `ObjectQL`, which is where a stash-vs-`previous` claim + * can actually be falsified. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#10240] the delete leg through the wired engine', () => { + it('a PREDICATE delete tombstones the file (`dispatch.mode === "per-row"`)', async () => { + // THE case this card exists for. A `beforeDelete` stash reaches + // `afterDelete` on the by-id path and is LOST here, because each matched + // row gets a fresh context per phase (#5574 / ADR-0058 Addendum II D1/D2). + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f1', parent_id: 'r1' }], + files: [committedFile('f1')], + }); + + await ql.delete('sys_attachment', { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + + expect(file('f1')).toMatchObject({ id: 'f1', status: 'deleted' }); + expect(typeof file('f1')!.deleted_at).toBe('string'); + }); + + it('a PREDICATE delete over several rows tombstones only the files that lost their LAST reference', async () => { + const { ql, file } = await bootWiredLifecycle({ + attachments: [ + { id: 'a1', file_id: 'f1', parent_id: 'r1' }, + { id: 'a2', file_id: 'f2', parent_id: 'r1' }, + { id: 'a3', file_id: 'f2', parent_id: 'r2' }, // f2 keeps a reference + ], + files: [committedFile('f1'), committedFile('f2')], + }); + + await ql.delete('sys_attachment', { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + + expect(file('f1')).toMatchObject({ status: 'deleted' }); + expect(file('f2')).toMatchObject({ status: 'committed' }); + }); + + it('both verbs now behave alike on BOTH dispatch paths — the four cases the card measured', async () => { + const seen: Record = {}; + + { + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f1', parent_id: 'r1' }], + files: [committedFile('f1')], + }); + await ql.delete('sys_attachment', { where: { id: 'a1' }, ...SYS_WRITE }); + seen['delete/by-id'] = file('f1')!.status; + } + { + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f1', parent_id: 'r1' }], + files: [committedFile('f1')], + }); + await ql.delete('sys_attachment', { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + seen['delete/predicate'] = file('f1')!.status; + } + { + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f_old', parent_id: 'r1' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + await ql.update('sys_attachment', { file_id: 'f_new' }, { where: { id: 'a1' }, ...SYS_WRITE }); + seen['update/by-id'] = file('f_old')!.status; + } + { + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f_old', parent_id: 'r1' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + await ql.update('sys_attachment', { file_id: 'f_new' }, { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + seen['update/predicate'] = file('f_old')!.status; + } + + expect(seen).toEqual({ + 'delete/by-id': 'deleted', + 'delete/predicate': 'deleted', + 'update/by-id': 'deleted', + 'update/predicate': 'deleted', + }); + }); + + it('a predicate delete that leaves the file still referenced tombstones nothing', async () => { + const { ql, file } = await bootWiredLifecycle({ + attachments: [ + { id: 'a1', file_id: 'f1', parent_id: 'r1' }, + { id: 'a2', file_id: 'f1', parent_id: 'r2' }, + ], + files: [committedFile('f1')], + }); + + await ql.delete('sys_attachment', { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + + expect(file('f1')).toMatchObject({ status: 'committed' }); + }); + + it('never tombstones a non-attachments scope on the predicate path either', async () => { + const { ql, file } = await bootWiredLifecycle({ + attachments: [{ id: 'a1', file_id: 'f1', parent_id: 'r1' }], + files: [committedFile('f1', 'user')], + }); + + await ql.delete('sys_attachment', { multi: true, where: { parent_id: 'r1' }, ...SYS_WRITE }); + + expect(file('f1')).toMatchObject({ status: 'committed' }); + }); +}); diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 8ecd3650e4..2bab7a718a 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -86,21 +86,6 @@ export interface AttachmentLifecycleLogger { const PACKAGE_ID = 'com.objectstack.service.storage'; const SYSTEM_CTX = { isSystem: true } as const; -/** Bound on join rows resolved per multi-delete — matches the reap-guard - * batch posture: bound one pass, converge across sweeps. */ -const MULTI_DELETE_RESOLVE_LIMIT = 1_000; - -/** Key under which beforeDelete stashes file ids for afterDelete (the engine - * passes the SAME HookContext object to both events). */ -const STASH_KEY = '__attachmentFileIds'; - -function asIdList(id: unknown): Array | null { - if (typeof id === 'string' || typeof id === 'number') return [id]; - if (id && typeof id === 'object' && Array.isArray((id as any).$in)) { - return (id as any).$in.filter((v: unknown) => typeof v === 'string' || typeof v === 'number'); - } - return null; -} /** * Tombstone every id in `fileIds` that no longer has a join row — the orphan @@ -149,50 +134,87 @@ export function installAttachmentLifecycleHooks( engine: AttachmentLifecycleEngine, logger: AttachmentLifecycleLogger, ): void { - // beforeDelete: resolve the file_id(s) of the join row(s) about to die — - // after the delete they are unreadable. Stash on the shared HookContext. - engine.registerHook( - 'beforeDelete', - async (ctx: any) => { - try { - const fileIds = new Set(); - const ids = asIdList(ctx?.input?.id); - if (ids) { - for (const id of ids) { - const row = await engine.findOne('sys_attachment', { where: { id }, context: { ...SYSTEM_CTX } }); - if (row?.file_id) fileIds.add(String(row.file_id)); - } - } else if (ctx?.input?.options?.where) { - const rows = await engine.find('sys_attachment', { - where: ctx.input.options.where, - limit: MULTI_DELETE_RESOLVE_LIMIT, - context: { ...SYSTEM_CTX }, - }); - for (const row of rows ?? []) { - if (row?.file_id) fileIds.add(String(row.file_id)); - } - } - ctx[STASH_KEY] = [...fileIds]; - } catch (err) { - // Never block the delete; the reap guard's sweep-time re-verification - // cannot resurrect a missed tombstone, but a missed tombstone only - // means the orphan lingers — fail toward retention, not data loss. - logger.warn( - `[storage] attachment lifecycle: failed to resolve file ids before delete (${(err as Error)?.message ?? err})`, - ); - ctx[STASH_KEY] = []; - } - }, - { object: 'sys_attachment', packageId: PACKAGE_ID }, - ); - - // afterDelete: any stashed file with zero remaining references is - // tombstoned — if (and only if) it is an attachments-scope committed file. + // afterDelete: the join row is gone, so the file it pointed at may now be an + // orphan — tombstone it if (and only if) it is an attachments-scope + // committed file with no remaining references. + // + // ── Why the departed id comes from `ctx.previous`, and NOT from a + // `beforeDelete` stash (#10240) ───────────────────────────────────────── + // This pair used to hand its ids over on the hook context itself + // (`ctx['__attachmentFileIds']`, written in a `beforeDelete` that resolved + // the doomed rows), on the premise — stated in its own comment — that "the + // engine passes the SAME HookContext object to both events". That was true + // of the pre-#5574 batch dispatch and is FALSE for a predicate write: since + // #5574 (ADR-0058 Addendum II, D1/D2) a `multi: true` write dispatches ONE + // CONTEXT PER MATCHED ROW, and those row contexts are fresh objects spread + // from the batch context in each phase independently + // (`dispatchPerRowBeforeHooks` / `buildPerRowAfterContexts` in objectql's + // `engine.ts`, which says so outright: "a per-row context is a fresh object, + // so a stash written on the context itself dies with the row that held it"). + // + // So on a PREDICATE delete the stash never arrived, `fileIds` was `[]`, and + // NO TOMBSTONE WAS EVER WRITTEN. That is not the module's "fail toward + // retention" bias, which buys a second look later: `sys_file`'s declared + // lifecycle nominates a sweep candidate only via `ttl { field: 'deleted_at' }` + // or `retention { onlyWhen: { status: 'pending' } }`, and an untombstoned + // orphan matches NEITHER — so it is never a candidate, the reap guard is + // never asked about it, and the bytes are stranded PERMANENTLY. Measured on + // the wired engine for #10240, both verbs in one run: + // + // DELETE by-id (dispatch record) : f1 -> status "deleted" OK + // DELETE predicate (dispatch per-row): f1 -> status "committed" LEAK + // UPDATE by-id (dispatch record) : f_old -> status "deleted" OK + // UPDATE predicate (dispatch per-row): f_old -> status "deleted" OK + // + // The two UPDATE rows are green because #10171 had already reached this + // conclusion for its verb; this handler now reads the id the same way, so + // the file carries ONE mechanism for "what file did this join row point at + // before?" rather than two that drift apart. + // + // `previous` has neither problem: the engine binds it to the row's PRE-IMAGE + // on BOTH phases and BOTH paths — by-id unconditionally since #7867 (the + // read that also produces the 404, so it is never skipped), per-row from the + // batch's single doomed-row read (#5038/#5574 D7). It costs no extra round + // trip on either path for the same reason. + // + // ── The `MULTI_DELETE_RESOLVE_LIMIT` limb that went with the stash ──────── + // The old `beforeDelete` had a second branch — `else if + // (ctx.input.options.where)`, resolving the doomed set itself under a + // 1_000-row cap — for a batch-shaped context that binds no `input.id`. It is + // removed here as a producer that never existed (the shape #5906 removed + // from `afterInsert` in this same file), and unreachability was MEASURED + // rather than assumed, with the sibling branch as the positive control. + // Both limbs of the live handler were counted while every delete shape the + // engine offers was driven through the wired engine: + // + // shape id-limb where-limb + // by-id 1 0 + // predicate multi (1 row) 1 0 + // predicate multi (3 rows) 3 0 + // multi, where { id: { $in: [..] } } 2 0 + // multi, where {} (match-all) 2 0 + // multi, NO where (unscoped) 2 0 + // non-multi, non-id where engine refuses: "Delete requires an ID + // or options.multi=true" + // + // Zero hits on the branch under test, while the control branch fires on the + // very predicate path the removed branch was written for. The mechanism + // agrees: all three sites that dispatch `beforeDelete` bind `input.id` to a + // scalar — by-id from `resolveEngineDeleteDispatch`, per-row to `row.id`, + // and the unscoped-multi dispatch (#9719) reaches only registrations that + // declared `dispatchUnscopedMultiWrite`, which this file never did, and by + // definition carries no `where` at all. engine.registerHook( 'afterDelete', async (ctx: any) => { - const fileIds: string[] = Array.isArray(ctx?.[STASH_KEY]) ? ctx[STASH_KEY] : []; - await tombstoneOrphanedFiles(engine, logger, fileIds); + // No pre-image (an engine that does not bind it) means the departed id + // is unknowable — tombstone nothing and KEEP the file. Retention-biased + // on purpose and identically to the `afterUpdate` leg below: a missed + // tombstone leaves an orphan lingering, while a tombstone written off a + // guess puts real bytes on the reap path. + const fileId = ctx?.previous?.file_id; + if (fileId === undefined || fileId === null || fileId === '') return; + await tombstoneOrphanedFiles(engine, logger, [String(fileId)]); }, { object: 'sys_attachment', packageId: PACKAGE_ID }, ); @@ -241,19 +263,19 @@ export function installAttachmentLifecycleHooks( // never asked about it, and the bytes are stranded permanently. // // ── Why the departed id comes from `ctx.previous`, and NOT from a - // beforeUpdate stash like the delete pair above ──────────────────────── - // The `beforeDelete` → `afterDelete` pair hands its ids over on the shared - // HookContext (STASH_KEY). Since #5574 (ADR-0058 Addendum II D1/D2) a - // PREDICATE write dispatches ONE CONTEXT PER MATCHED ROW, and those row - // contexts are fresh objects spread from the batch context in both phases - // (`dispatchPerRowBeforeHooks` / `buildPerRowAfterContexts` in objectql's - // `engine.ts`) — so a property a `before*` handler writes onto its own row - // context dies with that row and never reaches the `after*` phase. Measured - // on the wired engine for #10171: a stash set in `beforeUpdate` arrives in - // `afterUpdate` on the by-id path (`dispatch.mode === 'record'`) and is LOST - // on the predicate path (`dispatch.mode === 'per-row'`). A stash-based twin - // of the delete pair would therefore have been silently half-dead on exactly - // the multi-row updates that orphan the most files. + // beforeUpdate stash ──────────────────────────────────────────────────── + // Since #5574 (ADR-0058 Addendum II D1/D2) a PREDICATE write dispatches ONE + // CONTEXT PER MATCHED ROW, and those row contexts are fresh objects spread + // from the batch context in both phases (`dispatchPerRowBeforeHooks` / + // `buildPerRowAfterContexts` in objectql's `engine.ts`) — so a property a + // `before*` handler writes onto its own row context dies with that row and + // never reaches the `after*` phase. Measured on the wired engine for #10171: + // a stash set in `beforeUpdate` arrives in `afterUpdate` on the by-id path + // (`dispatch.mode === 'record'`) and is LOST on the predicate path + // (`dispatch.mode === 'per-row'`). A stash-based shape would therefore have + // been silently half-dead on exactly the multi-row updates that orphan the + // most files. The delete pair above WAS that shape and was exactly that + // half-dead until #10240; both verbs now read the same slot. // // `previous` has neither problem: the engine binds it to the row's PRE-IMAGE // on BOTH phases and BOTH paths (by-id since #7867 unconditionally, per-row diff --git a/packages/services/service-storage/src/storage-service-plugin.test.ts b/packages/services/service-storage/src/storage-service-plugin.test.ts index 89ade98f69..c970e3c7fd 100644 --- a/packages/services/service-storage/src/storage-service-plugin.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.test.ts @@ -354,16 +354,23 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () => await plugin.start(ctx); await ctx._flushReady(); - // Lifecycle hooks (beforeDelete/afterDelete/afterInsert, plus afterUpdate - // since #10171 gave the update verb its detach leg) + access hooks + // Lifecycle hooks (afterDelete/afterInsert, plus afterUpdate since #10171 + // gave the update verb its detach leg) + access hooks // (beforeInsert/beforeUpdate/beforeDelete, #10091 added the update verb) // — see attachment-lifecycle.ts and attachment-access-hooks.ts. + // + // [#10240] There is exactly ONE `beforeDelete` here, and it is the access + // guard's. The lifecycle module used to register a second one purely to + // stash file ids for its `afterDelete` — a hand-off a predicate delete + // silently dropped, because per-row dispatch gives each row a fresh + // context per phase (#5574). The id now comes from `ctx.previous`, so the + // registration has no work left and is gone; a second `beforeDelete` + // reappearing here means the stash mechanism came back with it. expect(hookEvents.sort()).toEqual([ 'afterDelete', 'afterInsert', 'afterUpdate', 'beforeDelete', - 'beforeDelete', 'beforeInsert', 'beforeUpdate', ]);