Skip to content

Attachment tombstoning silently no-ops on a PREDICATE delete — the beforeDelete→afterDelete stash dies with the per-row context #10240

Description

@os-warren

Found while implementing #10171 (the afterUpdate detach leg). Filed rather than fixed: #10171 is the UPDATE verb, this is the DELETE verb, and the blast radius differs enough to deserve its own review.

What is broken

installAttachmentLifecycleHooks (packages/services/service-storage/src/attachment-lifecycle.ts) hands file ids from beforeDelete to afterDelete on the hook context:

const STASH_KEY = '__attachmentFileIds';
// beforeDelete:  ctx[STASH_KEY] = [...fileIds];
// afterDelete:   const fileIds = Array.isArray(ctx?.[STASH_KEY]) ? ctx[STASH_KEY] : [];

Its comment states the premise: "the engine passes the SAME HookContext object to both events". That was true of the pre-#5574 batch dispatch. 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 each phase independently — dispatchPerRowBeforeHooks and buildPerRowAfterContexts in packages/objectql/src/engine.ts. dispatchPerRowBeforeHooks even 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 reaches afterDelete, fileIds is [], and no tombstone is ever written. The orphaned sys_file stays at status='committed' with deleted_at NULL — and sys_file's declared lifecycle nominates a sweep candidate only via ttl { field: 'deleted_at' } or retention { onlyWhen: { status: 'pending' } }, so it matches neither, the reap guard is never asked about it, and the bytes are stranded permanently. Same terminal state as #10171, reached through the delete verb.

Measured, on the wired engine

Driving real ObjectQL with the lifecycle hooks installed, one join row as the file's last reference:

case result
delete(where: { id: 'a1' }) — by-id f1status: "deleted", deleted_at set ✅
delete({ multi: true, where: { parent_id: 'r1' } }) — predicate f1status: "committed" ❌ no tombstone

And directly on the seam, with a probe stash set in beforeUpdate and read in afterUpdate (same context mechanics as the delete pair):

stash seen (dispatch.mode === 'record'):  SET
stash seen (dispatch.mode === 'per-row'): LOST

Suggested shape

Drop STASH_KEY and read the departed id from ctx.previous.file_id, which is what #10171's afterUpdate leg already does in this same file. The engine binds previous to the row's pre-image on both phases and both dispatch paths (by-id unconditionally since #7867; per-row from the batch's memoized prior-row read), so one read covers both — and it costs no extra round trip, since the demand is asked per object and attachment-access-hooks.ts already registers a beforeUpdate/beforeDelete here.

Doing so also collapses the two mechanisms this file would otherwise carry for one question ("what file did this join row point at before?"), which is the drift #10171 was filed against.

Worth checking in the same pass: beforeDelete's else if (ctx?.input?.options?.where) limb with MULTI_DELETE_RESOLVE_LIMIT. It exists to resolve a batch-shaped delete, but per-row dispatch always binds input.id, so it looks unreachable on today's engine — the same "a producer that never existed" shape #5906 removed from afterInsert in this file. Confirm before deleting.

Backlinks: #10171 (where found; the update verb's twin of this leak), #9719 (the other #5574 per-row-dispatch bypass on this same object), #2755 (where the tombstone flow landed), #5574 / #5846 (the dispatch change and the previous binding it enabled).


Generated by Claude Code

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions