diff --git a/.changeset/attachment-lifecycle-update-leg.md b/.changeset/attachment-lifecycle-update-leg.md new file mode 100644 index 0000000000..d4e3f087f0 --- /dev/null +++ b/.changeset/attachment-lifecycle-update-leg.md @@ -0,0 +1,13 @@ +--- +"@objectstack/service-storage": patch +--- + +**Bug fix (retention leak):** an UPDATE that re-points a `sys_attachment` row's `file_id` now detaches the PRIOR file the same way deleting that row would — tombstoning it when the re-pointed row was its last reference (#10171). + +`installAttachmentLifecycleHooks` registered only delete-side and insert-side handlers, so a `file_id` re-point left the old `sys_file` sitting at `status='committed'` with zero join rows and no `deleted_at`. That is not the module's "fail toward retention" bias, which buys a **later** look: `sys_file`'s declared lifecycle nominates a row for the sweep only through `ttl { field: 'deleted_at' }` or `retention { onlyWhen: { status: 'pending' } }`, and a silently detached file matches neither — so the reap guard is never asked about it and the storage bytes are stranded permanently, with no later re-examination. + +The new `afterUpdate` handler fires only when the payload actually carries `file_id` and the value actually changes, then runs the existing orphan rule (zero remaining join rows, attachments-scope, committed) on the prior id. It is best-effort like its siblings and never blocks the user's write; with no pre-image available it tombstones nothing, keeping the file. + +The departed id comes from the engine-bound pre-image `ctx.previous`, **not** from a `beforeUpdate` stash mirroring the delete pair. Since #5574 (ADR-0058 Addendum II D1/D2) a predicate write dispatches one fresh context per matched row in each phase, so a stash written in `beforeUpdate` reaches `afterUpdate` on the by-id path and is lost on the predicate path — a stash-based twin would have been silently half-dead on exactly the multi-row updates that orphan the most files. Reading `previous` also adds no driver round trip: the prior-row read is memoized per operation and already demanded on this object. + +**No revival leg was added**, deliberately. Re-pointing a row ONTO a grace-window tombstone is already handled by the reap guard's sweep-time re-verification, which resolves current references, un-tombstones the file and vetoes the reap rather than reclaiming bytes. A second revival mechanism here would be a duplicate answer to a question that already has one. diff --git a/packages/services/service-storage/src/attachment-lifecycle.test.ts b/packages/services/service-storage/src/attachment-lifecycle.test.ts index b2d3dba28c..0067c72849 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.test.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; import { installAttachmentLifecycleHooks, createSysFileReapGuard, @@ -79,6 +80,63 @@ async function driveDelete(engine: ReturnType, input: any, wh await engine.trigger('afterDelete', ctx); } +/** + * Drive an engine-shaped UPDATE. Both shapes the engine actually produces are + * covered by one helper, because the only thing that differs is `dispatch.mode` + * and whether a `before*` stash survives — and the handler under test reads + * neither a stash nor the mode: + * + * - by-id (`dispatch.mode === 'record'`): ONE HookContext for both phases; + * - predicate (`dispatch.mode === 'per-row'`, since #5574 / ADR-0058 + * Addendum II D1/D2): one FRESH context per matched row in each phase. + * + * `previous` is the row's pre-image on both — measured against the wired + * ObjectQL engine for #10171, and the reason this handler reads it rather than + * stashing across the phases the way the delete pair does. + */ +async function driveUpdate( + engine: ReturnType, + id: string, + patch: Record, + mode: 'record' | 'per-row' = 'record', +) { + const before = engine.tables.sys_attachment.find((r) => r.id === id); + const previous = before ? { ...before } : undefined; + const options = mode === 'record' ? { where: { id } } : { multi: true, where: { parent_id: before?.parent_id } }; + const beforeCtx: any = { + object: 'sys_attachment', event: 'beforeUpdate', + input: { id, data: patch, options }, previous, dispatch: { mode, index: 0, scope: {} }, + }; + await engine.trigger('beforeUpdate', beforeCtx); + if (before) Object.assign(before, patch); + // A per-row after-context is a FRESH object, never the before one. + const afterCtx: any = + mode === 'record' + ? Object.assign(beforeCtx, { event: 'afterUpdate', result: before ? { ...before } : undefined }) + : { + object: 'sys_attachment', event: 'afterUpdate', + input: { id, data: { ...patch }, options }, previous, + dispatch: { mode, index: 0, scope: {} }, result: before ? { ...before } : undefined, + }; + await engine.trigger('afterUpdate', afterCtx); + return afterCtx; +} + +/** The candidate filter `LifecycleService.reap()` derives from `sys_file`'s + * DECLARED lifecycle (`system-file.object.ts`): `ttl { field: 'deleted_at', + * expireAfter: '30d' }` and `retention { maxAge: '7d', onlyWhen: { status: + * 'pending' } }`. Nothing outside this set is ever handed to the reap guard. */ +const DAY_MS = 86_400_000; +function sweepCandidates(files: Array>, now: number) { + const ttlCutoff = new Date(now - 30 * DAY_MS).toISOString(); + const retentionCutoff = new Date(now - 7 * DAY_MS).toISOString(); + return files.filter( + (f) => + (typeof f.deleted_at === 'string' && f.deleted_at < ttlCutoff) || + (f.status === 'pending' && typeof f.created_at === 'string' && f.created_at < retentionCutoff), + ); +} + const committedFile = (id: string, scope = 'attachments') => ({ id, key: `attachments/${id}.bin`, @@ -494,3 +552,334 @@ describe('createUploadSessionReapGuard', () => { expect(confirmed).toEqual(['u6']); }); }); + +/* ──────────────────────────────────────────────────────────────────────────── + * #10171 — the UPDATE verb + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#10171] a `file_id` re-point detaches the prior file', () => { + it('tombstones the prior file when the re-pointed row was its LAST reference', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_old', parent_id: 'r1' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { file_id: 'f_new' }); + + const tombstone = engine.updates.find((u) => u.data.id === 'f_old'); + expect(tombstone?.data).toMatchObject({ id: 'f_old', status: 'deleted' }); + expect(typeof tombstone?.data.deleted_at).toBe('string'); + }); + + it('tombstones on the PREDICATE path too — where a beforeUpdate stash would have been lost', async () => { + // The shape #10171 proposed (beforeUpdate stash → afterUpdate) works by-id + // and silently does nothing here: since #5574 each matched row gets a FRESH + // context per phase, so anything written onto the before context dies with + // it. Reading `ctx.previous` is what makes both paths behave alike, and + // this case is the one that would catch a regression back to a stash. + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_old', parent_id: 'r1' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { file_id: 'f_new' }, 'per-row'); + + expect(engine.updates.find((u) => u.data.id === 'f_old')?.data).toMatchObject({ + id: 'f_old', + status: 'deleted', + }); + }); + + it('does NOT tombstone while another join row still references the prior file', async () => { + const engine = fakeEngine({ + attachments: [ + { id: 'a1', file_id: 'f_old' }, + { id: 'a2', file_id: 'f_old' }, // second parent, same file + ], + files: [committedFile('f_old'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { file_id: 'f_new' }); + + expect(engine.updates).toEqual([]); + expect(engine.tables.sys_file[0]).toMatchObject({ id: 'f_old', status: 'committed' }); + }); + + it('never tombstones non-attachments scopes (Field.file/avatar protection)', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_field' }], + files: [committedFile('f_field', 'field'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { file_id: 'f_new' }); + + expect(engine.updates).toEqual([]); + }); + + it('ignores an update whose payload does not carry `file_id`', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f1' }], + files: [committedFile('f1')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { description: 'renamed' }); + + expect(engine.updates).toEqual([]); + expect(engine.tables.sys_file[0]).toMatchObject({ status: 'committed' }); + }); + + it('a payload that re-states the SAME file_id detaches nothing', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f1' }], + files: [committedFile('f1')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + await driveUpdate(engine, 'a1', { file_id: 'f1' }); + + expect(engine.updates).toEqual([]); + }); + + it('no pre-image → tombstones nothing (fail toward retention)', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_old' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + + const ctx: any = { + object: 'sys_attachment', event: 'afterUpdate', + input: { id: 'a1', data: { file_id: 'f_new' }, options: { where: { id: 'a1' } } }, + dispatch: { mode: 'record', index: 0, scope: {} }, + }; + await engine.trigger('afterUpdate', ctx); + + expect(engine.updates).toEqual([]); + }); + + it('a failing lookup never blocks the update (best-effort)', async () => { + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_old' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + const logger = silentLogger(); + installAttachmentLifecycleHooks(engine, logger); + engine.find = async () => { throw new Error('driver down'); }; + + await expect(driveUpdate(engine, 'a1', { file_id: 'f_new' })).resolves.toBeDefined(); + expect(logger.warn).toHaveBeenCalled(); + }); +}); + +describe('[#10171] the leak this closes, and the half the reap guard already owned', () => { + it('an untombstoned orphan is never a sweep candidate — the leak is permanent, not deferred', async () => { + // Why the update leg has to exist at all: `sys_file`'s declared lifecycle + // can only ever nominate a row through `deleted_at` (ttl) or `status: + // 'pending'` (retention). A silently detached file carries neither, so the + // reap guard is never even asked about it — a permanent leak, which is + // outside this module's "fail toward retention" bias (that bias buys a + // LATER look, and here no later look exists). + const orphan = { ...committedFile('f_old') }; // detached, never tombstoned + expect(sweepCandidates([orphan], Date.now() + 365 * DAY_MS)).toEqual([]); + + // Once the update leg tombstones it, the same row IS nominated. + const engine = fakeEngine({ + attachments: [{ id: 'a1', file_id: 'f_old' }], + files: [committedFile('f_old'), committedFile('f_new')], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + await driveUpdate(engine, 'a1', { file_id: 'f_new' }); + + expect(sweepCandidates(engine.tables.sys_file, Date.now() + 365 * DAY_MS)).toEqual([ + expect.objectContaining({ id: 'f_old', status: 'deleted' }), + ]); + }); + + it('re-pointing a row ONTO a tombstoned file is revived by the sweep — so no revival leg is added here', async () => { + // #10171's second leg, measured rather than assumed: the reap guard's + // sweep-time re-verification resolves CURRENT references, so a re-pointed + // tombstone is un-tombstoned and vetoed instead of reaped. The bytes were + // never at risk, and an `afterUpdate` revival twin would be a second + // implementation of an answer that already exists. + const engine = fakeEngine({ + // `a2` keeps `f_other` referenced, so the detach half stays out of the + // way and this case is about the REVIVAL half alone. + attachments: [{ id: 'a1', file_id: 'f_other' }, { id: 'a2', file_id: 'f_other' }], + files: [ + { id: 'f_t', key: 'attachments/f_t.bin', scope: 'attachments', status: 'deleted', deleted_at: '2026-01-01T00:00:00Z' }, + committedFile('f_other'), + ], + }); + installAttachmentLifecycleHooks(engine, silentLogger()); + const s = { delete: vi.fn(async () => {}) } as any; + + await driveUpdate(engine, 'a1', { file_id: 'f_t' }); + // The update leg itself neither revives nor tombstones anything here. + expect(engine.updates).toEqual([]); + + const guard = createSysFileReapGuard(engine, () => s, silentLogger(), async () => true); + const confirmed = await guard('sys_file', [{ ...engine.tables.sys_file[0] }]); + + expect(confirmed).toEqual([]); + expect(s.delete).not.toHaveBeenCalled(); + expect(engine.tables.sys_file[0]).toMatchObject({ id: 'f_t', status: 'committed', deleted_at: null }); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * #10171 — the same leg through the WIRED engine + * + * The fakes above model the dispatch shapes; this section checks the model. + * The whole design rests on the engine binding `previous` to the row's + * pre-image on the after phase of BOTH dispatch paths — a fake that simply + * asserts that would be circular, so these cases drive real `ObjectQL`. + * ──────────────────────────────────────────────────────────────────────────── */ + +const wiredAttachmentObject = { + name: 'sys_attachment', label: 'Attachment', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + file_id: { name: 'file_id', label: 'File', type: 'text' as const }, + parent_id: { name: 'parent_id', label: 'Parent Id', type: 'text' as const }, + }, +}; +const wiredFileObject = { + name: 'sys_file', label: 'File', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + key: { name: 'key', label: 'Key', type: 'text' as const }, + scope: { name: 'scope', label: 'Scope', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + deleted_at: { name: 'deleted_at', label: 'Deleted At', type: 'text' as const }, + }, +}; + +/** In-memory driver whose WHERE matcher REFUSES combinators/operator values by + * throwing — the conforming shape `check-where-matcher-conformance.mjs` asks of + * a double (silently wrong answers are the defect class, not incompleteness). */ +function makeWiredDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { + let st = stores.get(o); + if (!st) { st = new Map(); stores.set(o, st); } + return st; + }; + const matches = (row: Record, where: unknown): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) throw new Error(`wired stub driver: unsupported combinator ${k}`); + if (v !== null && typeof v === 'object') throw new Error(`wired stub driver: unsupported operator value on ${k}`); + if ((row[k] ?? null) !== (v ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, stores, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; }, + async create(o: string, data: Record) { + const id = String(data.id); const row = { ...data, id }; storeFor(o).set(id, row); return row; + }, + async update(o: string, id: string, data: Record) { + const st = storeFor(o); const row = st.get(String(id)); if (!row) return null; + const next = { ...row, ...data, id: row.id }; st.set(String(id), next); return next; + }, + async delete(o: string, id: string) { return storeFor(o).delete(String(id)); }, + async count(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).length; }, + async deleteMany(o: string, ast: any) { + const doomed = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of doomed) storeFor(o).delete(String(r.id)); + return doomed.length; + }, + async updateMany(o: string, ast: any, data: Record) { + const hit = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of hit) storeFor(o).set(String(r.id), { ...r, ...data, id: r.id }); + return hit.length; + }, + }; + return d; +} + +async function bootWiredLifecycle(seed: { + attachments?: Array>; + files?: Array>; +}) { + const ql = new ObjectQL(); + const driver = makeWiredDriver(); + ql.registerDriver(driver, true); + await ql.init(); + ql.registry.registerObject(wiredAttachmentObject as any, 'app:test'); + ql.registry.registerObject(wiredFileObject as any, 'app:test'); + // `ql as any` mirrors the production wiring in storage-service-plugin.ts. + installAttachmentLifecycleHooks(ql as any, silentLogger()); + for (const o of ['sys_attachment', 'sys_file']) { + if (!driver.stores.get(o)) driver.stores.set(o, new Map()); + } + for (const r of seed.attachments ?? []) driver.stores.get('sys_attachment')!.set(String(r.id), { ...r }); + for (const r of seed.files ?? []) driver.stores.get('sys_file')!.set(String(r.id), { ...r }); + return { ql, file: (id: string) => driver.stores.get('sys_file')!.get(id) }; +} + +const SYS_WRITE = { context: { isSystem: true } } as any; + +describe('[#10171] the update leg through the wired engine', () => { + it('a by-id re-point tombstones the prior file (`dispatch.mode === "record"`)', async () => { + 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 }); + + expect(file('f_old')).toMatchObject({ id: 'f_old', status: 'deleted' }); + expect(typeof file('f_old')!.deleted_at).toBe('string'); + expect(file('f_new')).toMatchObject({ status: 'committed' }); + }); + + it('a PREDICATE re-point tombstones the prior file (`dispatch.mode === "per-row"`)', async () => { + // The case that decides the design. A `beforeUpdate` stash reaches + // `afterUpdate` 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) — + // so the shape #10171 proposed would pass the case above and fail this one. + 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 }); + + expect(file('f_old')).toMatchObject({ id: 'f_old', status: 'deleted' }); + }); + + it('a re-point that leaves the prior file still referenced tombstones nothing', async () => { + const { ql, file } = await bootWiredLifecycle({ + attachments: [ + { id: 'a1', file_id: 'f_old', parent_id: 'r1' }, + { id: 'a2', file_id: 'f_old', parent_id: 'r2' }, + ], + files: [committedFile('f_old'), committedFile('f_new')], + }); + + await ql.update('sys_attachment', { file_id: 'f_new' }, { where: { id: 'a1' }, ...SYS_WRITE }); + + expect(file('f_old')).toMatchObject({ status: 'committed' }); + }); + + it('the by-id DELETE leg still tombstones — the shared orphan helper did not move it', async () => { + 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 }); + + expect(file('f1')).toMatchObject({ id: 'f1', status: 'deleted' }); + }); +}); diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 83626e6fe1..8ecd3650e4 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -12,8 +12,10 @@ import type { IStorageService } from '@objectstack/spec/contracts'; * naive cascade. This module closes the resulting orphan leak: * * 1. Tombstone hooks (this file, installed on `sys_attachment`): when the - * LAST join row referencing an attachments-scope file is deleted, the - * `sys_file` row is marked `status='deleted'` + `deleted_at=now`. + * LAST join row referencing an attachments-scope file goes away, the + * `sys_file` row is marked `status='deleted'` + `deleted_at=now`. A join + * row goes away two ways, and both are covered: it is DELETED, or an + * UPDATE re-points its `file_id` at some other file (#10171). * Re-attaching before the grace window expires un-tombstones it. * 2. The `lifecycle` declaration on `sys_file` (system-file.object.ts): * the platform LifecycleService reaps tombstones `30d` after @@ -100,9 +102,47 @@ function asIdList(id: unknown): Array | null { return null; } +/** + * Tombstone every id in `fileIds` that no longer has a join row — the orphan + * rule, in ONE place because two write verbs now ask it (`afterDelete`, and + * `afterUpdate` for a `file_id` re-point). Two copies of "is this file an + * orphan, and may I tombstone it" is exactly the drift #10171 was filed + * against; the verbs differ only in how they name the departed file id. + * + * Best-effort throughout: a failure here must never fail the user's write. + */ +async function tombstoneOrphanedFiles( + engine: AttachmentLifecycleEngine, + logger: AttachmentLifecycleLogger, + fileIds: readonly string[], +): Promise { + for (const fileId of fileIds) { + try { + const remaining = await engine.find('sys_attachment', { + where: { file_id: fileId }, + limit: 1, + context: { ...SYSTEM_CTX }, + }); + if (remaining?.length) continue; + const file = await engine.findOne('sys_file', { where: { id: fileId }, context: { ...SYSTEM_CTX } }); + if (!file || file.scope !== 'attachments' || file.status !== 'committed') continue; + await engine.update( + 'sys_file', + { id: fileId, status: 'deleted', deleted_at: new Date().toISOString() }, + { context: { ...SYSTEM_CTX } }, + ); + logger.debug?.(`[storage] attachment lifecycle: tombstoned orphan sys_file ${fileId}`); + } catch (err) { + logger.warn( + `[storage] attachment lifecycle: failed to tombstone sys_file ${fileId} (${(err as Error)?.message ?? err})`, + ); + } + } +} + /** * Install the tombstone hooks on `sys_attachment`. Lifecycle bookkeeping - * must never block or fail a user's delete/insert — every handler is + * must never block or fail a user's delete/insert/update — every handler is * best-effort and only logs on failure. */ export function installAttachmentLifecycleHooks( @@ -152,28 +192,7 @@ export function installAttachmentLifecycleHooks( 'afterDelete', async (ctx: any) => { const fileIds: string[] = Array.isArray(ctx?.[STASH_KEY]) ? ctx[STASH_KEY] : []; - for (const fileId of fileIds) { - try { - const remaining = await engine.find('sys_attachment', { - where: { file_id: fileId }, - limit: 1, - context: { ...SYSTEM_CTX }, - }); - if (remaining?.length) continue; - const file = await engine.findOne('sys_file', { where: { id: fileId }, context: { ...SYSTEM_CTX } }); - if (!file || file.scope !== 'attachments' || file.status !== 'committed') continue; - await engine.update( - 'sys_file', - { id: fileId, status: 'deleted', deleted_at: new Date().toISOString() }, - { context: { ...SYSTEM_CTX } }, - ); - logger.debug?.(`[storage] attachment lifecycle: tombstoned orphan sys_file ${fileId}`); - } catch (err) { - logger.warn( - `[storage] attachment lifecycle: failed to tombstone sys_file ${fileId} (${(err as Error)?.message ?? err})`, - ); - } - } + await tombstoneOrphanedFiles(engine, logger, fileIds); }, { object: 'sys_attachment', packageId: PACKAGE_ID }, ); @@ -210,6 +229,67 @@ export function installAttachmentLifecycleHooks( }, { object: 'sys_attachment', packageId: PACKAGE_ID }, ); + + // afterUpdate: an UPDATE that re-points a join row's `file_id` detaches the + // PRIOR file exactly the way a delete of that row would — and until #10171 + // nothing said so, leaving a file with zero join rows sitting at + // `status='committed'`. That is not the module's "fail toward retention" + // bias, which buys a second look later: `sys_file`'s declared lifecycle + // makes a row 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. + // + // ── 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. + // + // `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 + // from the batch's prior-row read). It also costs no extra round trip — the + // prior-row read is memoized per operation and already demanded, because + // `attachment-access-hooks.ts` registers a `beforeUpdate` on this same + // object and the engine asks that demand PER OBJECT, not per handler. + // + // ── Why there is no revival leg here (deliberate, measured) ────────────── + // #10171 also asked for an `afterInsert`-style revival when an update points + // a row AT a tombstoned file. There is none, because the reap guard below + // already owns that question: it re-verifies references at sweep time and, + // finding the re-pointed join row, un-tombstones the file and vetoes the + // reap instead of reclaiming the bytes (`createSysFileReapGuard`, pinned by + // "vetoes and un-tombstones a row that regained references"). Measured for + // #10171: after a re-point onto a tombstoned file the guard confirms nothing + // and deletes no bytes. A second revival mechanism here would be a duplicate + // answer to a question that already has one — the failure mode being two + // implementations that drift apart, not a missing feature. + engine.registerHook( + 'afterUpdate', + async (ctx: any) => { + // Only a payload that actually re-points `file_id` can detach anything. + const data = ctx?.input?.data; + if (!data || typeof data !== 'object' || !('file_id' in data)) return; + // No pre-image (an engine that does not bind it, a row that was not + // there) means the prior id is unknowable — tombstone nothing and keep + // the file, which is the retention-biased side of the trade. + const priorFileId = ctx?.previous?.file_id; + if (priorFileId === undefined || priorFileId === null || priorFileId === '') return; + // A write that re-states the same id detaches nothing. + if (String(priorFileId) === String(data.file_id)) return; + await tombstoneOrphanedFiles(engine, logger, [String(priorFileId)]); + }, + { object: 'sys_attachment', packageId: PACKAGE_ID }, + ); } /** 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 1d32c6cce5..89ade98f69 100644 --- a/packages/services/service-storage/src/storage-service-plugin.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.test.ts @@ -354,12 +354,14 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () => await plugin.start(ctx); await ctx._flushReady(); - // Lifecycle hooks (beforeDelete/afterDelete/afterInsert) + access hooks + // Lifecycle hooks (beforeDelete/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. expect(hookEvents.sort()).toEqual([ 'afterDelete', 'afterInsert', + 'afterUpdate', 'beforeDelete', 'beforeDelete', 'beforeInsert',