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', ]);