fix(objectql): hydrate a tombstoned sys_file that still has a live holder

.changeset/file-hydration-tombstone-agreement.md

@@ -0,0 +1,10 @@
+---
+"@objectstack/objectql": minor
+"@objectstack/service-storage": patch
+---
+
+Record file-field hydration now answers the same question about a `sys_file` tombstone that the download path answers (#11427). `#10246` stopped `GET /api/v1/storage/files/:id` treating a tombstone (`status: 'deleted'` + `deleted_at`) as the last word — it asks the reap guard's own `findFileHolder` and serves the row for as long as something still holds it — but the record read kept the older `status === 'committed'` rule. One `sys_file` row therefore answered `200` at the download endpoint and a bare id inside a record payload, which UI and export render as "this record has no attachment".
+
+The population is narrow and unchanged in every other respect: `claimFile` already un-tombstones a field file synchronously when a record re-points at it, and attachments-scope files are never reached by field hydration, so what this closes is the residual the reap guard's sweep-time re-verification names — hook races, direct-driver writes, and future trash restore. A tombstone nothing holds still hydrates as a bare id, and a `pending` upload is untouched.
+
+The predicate is not re-derived in the engine. `ObjectQL` gains `registerHeldFileResolver` (type `HeldFileResolver`), which the storage plugin fills with `findHeldFiles` — the batched form of `findFileHolder`, asking the same union of `sys_attachment` join rows and the `ref_*` ownership columns. Batched because hydration runs over many rows per read: a read with no tombstone costs nothing, and the residual case costs one extra query for the whole read rather than one per file. Engines with no storage plugin keep tombstones un-hydrated exactly as before.

content/docs/permissions/attachments-access.mdx

@@ -109,13 +109,17 @@ can be shared across records). Reclamation is handled by the platform LifecycleS
  attachments-scope file is deleted, the file is tombstoned; a reap guard
  re-verifies zero references at sweep time and deletes the storage bytes
  before the row is reaped (abandoned `pending` uploads are reaped too).
- **A tombstone is recoverable state, not a delete — and downloads treat it that
- way.** Re-attaching the file, or re-claiming it through a record field, makes
- it downloadable again *immediately*, with no sweep in between: the download
- endpoints ask the same "is anything still holding this file?" question the
- reap guard asks before it reclaims anything. The row itself stays tombstoned
- until a sweep tidies it, and a file with no holder left still answers
- `FILE_NOT_FOUND` (404).
+ **A tombstone is recoverable state, not a delete — and every reader treats it
+ that way.** Re-attaching the file, or re-claiming it through a record field,
+ makes it readable again *immediately*, with no sweep in between: the download
+ endpoints **and record file-field hydration** ask the same "is anything still
+ holding this file?" question the reap guard asks before it reclaims anything.
+ Both reach it through that one predicate rather than each deciding for itself,
+ which is what stops the two surfaces answering differently about one row — a
+ file `GET /storage/files/:id` serves is a file a record read expands into
+ `{ id, name, size, mimeType, url }`. The row itself stays tombstoned until a
+ sweep tidies it, and a file with no holder left still answers
+ `FILE_NOT_FOUND` (404) and keeps its bare id in a record payload.
 - **`sys_upload_session`** — abandoned/terminal chunked-upload sessions are
  reaped, and a reap guard aborts the underlying backend multipart upload
  (S3 `AbortMultipartUpload` / local parts dir) first, so already-uploaded

packages/objectql/src/engine.ts

@@ -1898,6 +1898,24 @@ export type EngineMiddleware = (
  next: () => Promise<void>
 ) => Promise<void>;
 
+/**
+ * "Which of these tombstoned `sys_file` rows is something still holding?"
+ * (#11427).
+ *
+ * Takes the whole tombstoned set from ONE record read and returns the subset
+ * still held, as a set of stringified ids. Batched rather than per-row on
+ * purpose: hydration runs over many rows per read, so asking per file would be
+ * N queries per read.
+ *
+ * The engine declares this shape but never implements it — "still held" has one
+ * definition (`findFileHolder`, the union of `sys_attachment` join rows and the
+ * `ref_*` ownership columns) and it lives in the storage package. See
+ * `ObjectQL.registerHeldFileResolver`.
+ */
+export type HeldFileResolver = (
+ rows: Array<Record<string, any>>,
+) => Promise<Set<string>>;
+
 /**
  * The stack collections the engine decomposes into individual registry items —
  * ONE list, read by the ONE body both registration seams run
@@ -3072,6 +3090,33 @@ export class ObjectQL implements IObjectQLEngine {
  }
  }
 
+ /**
+ * "Which of these tombstoned `sys_file` rows is something still holding?"
+ * (#11427) — supplied by the storage plugin, never derived here.
+ *
+ * File-field hydration must answer the same question the download path
+ * answers (#10246) or one row gets two answers. That question has exactly one
+ * definition, `findFileHolder`, and it lives in `@objectstack/service-storage`
+ * — a package this one does not and must not depend on. So the engine
+ * declares the seam and the storage plugin fills it, the same handover
+ * `resolveFileHolder` makes to the download routes.
+ *
+ * BATCHED on purpose: hydration runs over many rows per read, so a per-row
+ * holder check would be N queries per read. The resolver takes the whole
+ * tombstoned set and returns the ids still held.
+ */
+ private _heldFileResolver?: HeldFileResolver;
+
+ /**
+ * Wire the batched holder question (#11427). Last registration wins; leaving
+ * it unwired keeps tombstoned files un-hydrated, which is what this engine
+ * did before the seam existed.
+ */
+ registerHeldFileResolver(fn: HeldFileResolver): void {
+ this._heldFileResolver = fn;
+ this.logger.debug('Registered held-file resolver for sys_file hydration');
+ }
+
  /**
  * Register a middleware function
  * Middlewares execute in onion model around every data operation.
@@ -8078,8 +8123,52 @@ export class ObjectQL implements IObjectQLEngine {
  }
 
  const fileMap = new Map<string, any>();
+ // [#11427] `committed` is servable and always was. A TOMBSTONE
+ // (`status: 'deleted'` + `deleted_at`) is recoverable state, not a delete:
+ // it is a claim about the future (this row is reapable once the grace
+ // window ends) that the sweep re-checks and often withdraws. #10246 already
+ // stopped the two download endpoints treating it as the last word — they
+ // ask the reap guard's own `findFileHolder` and serve the row for as long
+ // as something still holds it. This pass did not, so one `sys_file` row
+ // answered 200 at `/files/:id` and a bare id here: two read surfaces, two
+ // answers, and consumers render the bare id as "no attachment".
+ //
+ // ⛔ The predicate is NOT re-derived here. "Still held" has ONE definition
+ // (`findFileHolder`, a deliberate union of `sys_attachment` join rows AND
+ // the `ref_*` ownership columns) and it lives in the storage package, which
+ // this one cannot import. A copy narrower by a limb would hide files the
+ // sweep refuses to reap — the same defect one limb over — so the question
+ // arrives through {@link registerHeldFileResolver} instead, the same
+ // handover `resolveFileHolder` makes to the download routes. Unwired (bare
+ // kernel, no storage plugin, tests): tombstones stay hidden, exactly as
+ // before this existed.
+ const tombstoned: any[] = [];
  for (const row of fileRows) {
- if (row?.id != null && row.status === 'committed') fileMap.set(String(row.id), row);
+ if (row?.id == null) continue;
+ if (row.status === 'committed') fileMap.set(String(row.id), row);
+ else if (row.status === 'deleted') tombstoned.push(row);
+ }
+ // Lazy by construction: a batch with no tombstone — every ordinary read —
+ // costs nothing at all, and the resolver is BATCHED, so the residual case
+ // costs one extra query for the whole read rather than one per row.
+ if (tombstoned.length > 0 && this._heldFileResolver) {
+ try {
+ const held = await this._heldFileResolver(tombstoned);
+ for (const row of tombstoned) {
+ if (held?.has(String(row.id))) fileMap.set(String(row.id), row);
+ }
+ } catch (error) {
+ // Unreadable evidence is not evidence of a holder. Keep the ids
+ // un-hydrated — the answer this pass gave before #11427, and the same
+ // direction the download path fails in (`isServableForDownload`) and
+ // the reap guard fails in (it vetoes rather than reaps when it cannot
+ // tell). Distinct from the #6116 catch above, which covers the
+ // `sys_file` read itself and is untouched.
+ this.logger.warn(
+ 'sys_file holder check failed; tombstoned file fields keep their raw ids for this read',
+ { object: objectName, tombstonedIds: tombstoned.length, error: (error as Error)?.message },
+ );
+ }
  }
  if (fileMap.size === 0) return records;
 

packages/objectql/src/index.ts

@@ -63,7 +63,7 @@ export type { CompanionFieldMeta, CompanionObjectMeta } from './search-companion
 
 // Export Engine
 export { ObjectQL, ObjectRepository, ScopedContext } from './engine.js';
-export type { HookHandler, HookEntry, OperationContext, EngineMiddleware } from './engine.js';
+export type { HookHandler, HookEntry, OperationContext, EngineMiddleware, HeldFileResolver } from './engine.js';
 export type { AdmittedValueShapeViolationTally } from './engine.js';
 export { SummaryRecomputeError } from './summary-errors.js';
 export type { SummaryRecomputeFailure } from './summary-errors.js';

packages/services/service-storage/package.json

@@ -38,6 +38,7 @@
  },
  "devDependencies": {
  "@objectstack/objectql": "workspace:*",
+ "@objectstack/driver-sql": "workspace:*",
  "@types/node": "^26.2.0",
  "typescript": "^6.0.3",
  "vitest": "^4.1.10"

packages/services/service-storage/src/attachment-lifecycle.ts

@@ -365,6 +365,54 @@ export async function findFileHolder(
  return hasFieldReferenceOwner(row) ? 'field-owner' : null;
 }
 
+/**
+ * The BATCHED form of {@link findFileHolder} — "which of these files is still
+ * held?" — for callers holding many rows at once (#11427).
+ *
+ * Record file-field hydration is such a caller: it must reach the same verdict
+ * the download path reaches (#10246) or one `sys_file` row gets two answers,
+ * but it runs over many rows per read, so asking {@link findFileHolder} per
+ * file would be N queries per read. This asks the SAME union in at most one
+ * extra query for the whole batch.
+ *
+ * ⚠️ Same union, same limbs, deliberately in the cheaper order. {@link
+ * findFileHolder} asks the join-row limb first because it must NAME the
+ * surface; this one only needs "held or not", so it takes the free limb first:
+ * {@link hasFieldReferenceOwner} is a pure column test on rows the caller has
+ * already read, and every id it settles is an id the join-row query never has
+ * to carry. `||` commutes, so the verdict is identical either way — pinned as
+ * an equivalence in `tombstone-hydration-download-agreement.test.ts` rather
+ * than asserted here.
+ *
+ * Cost, stated rather than assumed:
+ * - no rows, or every row settled by the columns → ZERO queries;
+ * - otherwise → exactly ONE `$in` read of `sys_attachment`, whatever the
+ * number of files or records involved.
+ */
+export async function findHeldFiles(
+ engine: Pick<AttachmentLifecycleEngine, 'find'>,
+ rows: Array<Record<string, unknown>>,
+): Promise<Set<string>> {
+ const held = new Set<string>();
+ const needJoinCheck: string[] = [];
+ for (const row of rows) {
+ if (row?.id == null) continue;
+ const id = String(row.id);
+ // The free limb first — a pure test on a row already in hand.
+ if (hasFieldReferenceOwner(row)) held.add(id);
+ else needJoinCheck.push(id);
+ }
+ if (needJoinCheck.length === 0) return held;
+ const refs = await engine.find('sys_attachment', {
+ where: { file_id: { $in: needJoinCheck } },
+ context: { ...SYSTEM_CTX },
+ });
+ for (const ref of refs ?? []) {
+ if (ref?.file_id != null) held.add(String(ref.file_id));
+ }
+ return held;
+}
+
 /**
  * The `sys_file` reap guard ({@link LifecycleReapGuard} shape from
  * `@objectstack/objectql`, duck-typed here to avoid the dependency).

packages/services/service-storage/src/storage-service-plugin.ts

@@ -21,7 +21,7 @@ import { StorageMetadataStore } from './metadata-store.js';
 import type { FileRecord } from './metadata-store.js';
 import { registerStorageRoutes } from './storage-routes.js';
 import type { FileReadVerdict } from './storage-routes.js';
-import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder } from './attachment-lifecycle.js';
+import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder, findHeldFiles } from './attachment-lifecycle.js';
 import { installFileReferenceHooks } from './file-reference-lifecycle.js';
 import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js';
 import { SystemFile, SystemUploadSession } from './objects/index.js';
@@ -353,6 +353,22 @@ export class StorageServicePlugin implements Plugin {
  // guard below re-verifies the ownership columns — and re-reads the
  // deployment flag, fresh — before any byte is deleted.
  installFileReferenceHooks(engine as any, () => this.storage, ctx.logger);
+ // "Is anything still holding this tombstone?" for RECORD FILE-FIELD
+ // HYDRATION (#11427). The download path got this question in #10246;
+ // the record read kept the older `status === 'committed'` rule, so one
+ // `sys_file` row answered 200 at `/files/:id` and a bare id inside a
+ // record payload — which UI and export render as "no attachment".
+ //
+ // Handed over rather than re-derived, exactly like `resolveFileHolder`
+ // below, and BATCHED: hydration runs over many rows per read, so the
+ // engine passes the whole tombstoned set and `findHeldFiles` answers it
+ // in at most one extra query. Duck-typed so an older engine without the
+ // seam simply keeps tombstones un-hydrated.
+ if (typeof (engine as any).registerHeldFileResolver === 'function') {
+ (engine as any).registerHeldFileResolver(
+ (rows: Array<Record<string, unknown>>) => findHeldFiles(engine as any, rows),
+ );
+ }
  try {
  const lifecycle = ctx.getService<any>('lifecycle');
  if (lifecycle && typeof lifecycle.registerReapGuard === 'function') {