Skip to content

Commit c3c72a4

Browse files
Sam Altmanclaude
andauthored
fix(objectql): hydrate a tombstoned sys_file that still has a live holder (#11844)
* fix(objectql): hydrate a tombstoned sys_file that still has a live holder #10246 stopped the two download endpoints treating a `sys_file` tombstone as the last word: they ask the reap guard's own `findFileHolder` and serve the row for as long as something still holds it. Record file-field hydration was not part of that ruling and kept the older `status === 'committed'` rule, so one `sys_file` row answered 200 at `GET /api/v1/storage/files/:id` and a bare id inside a record payload — which UI and export render as "no attachment". The predicate is not re-derived in the engine. "Still held" has ONE definition, `findFileHolder`, a union of `sys_attachment` join rows and the `ref_*` ownership columns, and it lives in a package objectql cannot depend on. So the engine declares `registerHeldFileResolver` and the storage plugin fills it — the same handover `resolveFileHolder` makes to the download routes. Batched, because hydration runs over many rows per read: `findHeldFiles` takes the whole tombstoned set, settles every row whose ownership columns answer it with zero queries, and issues at most ONE `$in` read for the rest. A read with no tombstone — every ordinary read — costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * test(service-storage): resolve driver-memory from source in vitest `check:test-source-alias` — the agreement pin drives a REAL driver, and one read from dist/ would make it a verdict about build state rather than about the source beside it. objectql stays unaliased on purpose: it is a registered KNOWN_UNALIASED_TEST_IMPORTS entry, which is what lets an ablation of engine source, rebuilt, actually change what these tests observe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * test(service-storage): keep the new pin inside the type-check-debt ratchet `check:type-check-debt --re-measure` counts raw tsc errors per package against a shrink-only ledger; the new file added 6 (unused type import, the required packageId argument to registry.registerObject, and two untyped mocks whose calls[0][1] read as an empty tuple). Fixed rather than ledgered — the package re-measures at its frozen 51. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * docs(permissions): the tombstone question is asked by both readers, not just downloads The `sys_file` bullet scoped its "same question" claim to the download endpoints. That scoping was accurate only because record file-field hydration was NOT asking it — the divergence this branch fixes, written down as if it were the design, so a reader came away believing record reads do not participate. Widened to name both readers and to say why one predicate rather than two is what makes them agree. The boundary clause is kept and extended rather than softened: a file with no holder left still answers FILE_NOT_FOUND (404) and still keeps its bare id in a record payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * test(service-storage): migrate the agreement pin to sqlite :memory: `check:driver-memory-census` was red on this branch: the pin bound @objectstack/driver-memory by import and by devDependency, and the ledger covers neither. Migrated rather than ledgered. The two `ruled-permanent` entries exist because the in-memory driver's DISTINCT SEMANTICS are the subject — the schemaless arm of #4271, and the engine-fallback arm of #6468 where `supports = {}` puts the counter in the engine. This pin's subject is neither: it needs shared state across two read surfaces so the download verdict and the hydration verdict come from the same rows, and sqlite `:memory:` carries that identically. #5499 froze investment in the driver and #5704 made sqlite the test backend, so the ruled default already answers this — no ruling required, and no ledger row added by hand (the census still reports 2 ruled consumers, not 3). Checked before switching: every seeded key is declared, so there is no undeclared-field write for the SQL family to refuse, and nothing in the fixture is schemaless or leans on a driver-advertised capability. The vitest alias moves with the import; @objectstack/objectql stays ledgered-unaliased so an ablation of engine source, rebuilt, still changes what these tests observe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * chore(changeset): @objectstack/objectql is a minor — the public surface widens Contract review rejected the changeset for disagreeing with the PR's own clause-② declaration. Verified at this head rather than relayed: the type `HeldFileResolver` is re-exported from the package entry (packages/objectql/src/index.ts), and `registerHeldFileResolver` is a public method on `ObjectQL`, which that entry exports too. Both are ADDITIVE — nothing removed, no existing caller's behaviour changed — which is a minor, not a patch. `@objectstack/service-storage` stays `patch` on purpose and is untouched: `findHeldFiles` is deliberately absent from that package's index, so its public surface does not move (positive control: the symbol resolves in 3 files on this branch, none of them the entry). Refs #11427. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0010797 commit c3c72a4

10 files changed

Lines changed: 586 additions & 11 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@objectstack/objectql": minor
3+
"@objectstack/service-storage": patch
4+
---
5+
6+
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".
7+
8+
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.
9+
10+
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‎

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

‎packages/objectql/src/engine.ts‎

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1898,6 +1898,24 @@ export type EngineMiddleware = (
18981898
next: () => Promise<void>
18991899
) => Promise<void>;
19001900

1901+
/**
1902+
* "Which of these tombstoned `sys_file` rows is something still holding?"
1903+
* (#11427).
1904+
*
1905+
* Takes the whole tombstoned set from ONE record read and returns the subset
1906+
* still held, as a set of stringified ids. Batched rather than per-row on
1907+
* purpose: hydration runs over many rows per read, so asking per file would be
1908+
* N queries per read.
1909+
*
1910+
* The engine declares this shape but never implements it — "still held" has one
1911+
* definition (`findFileHolder`, the union of `sys_attachment` join rows and the
1912+
* `ref_*` ownership columns) and it lives in the storage package. See
1913+
* `ObjectQL.registerHeldFileResolver`.
1914+
*/
1915+
export type HeldFileResolver = (
1916+
rows: Array<Record<string, any>>,
1917+
) => Promise<Set<string>>;
1918+
19011919
/**
19021920
* The stack collections the engine decomposes into individual registry items —
19031921
* ONE list, read by the ONE body both registration seams run
@@ -3072,6 +3090,33 @@ export class ObjectQL implements IObjectQLEngine {
30723090
}
30733091
}
30743092

3093+
/**
3094+
* "Which of these tombstoned `sys_file` rows is something still holding?"
3095+
* (#11427) — supplied by the storage plugin, never derived here.
3096+
*
3097+
* File-field hydration must answer the same question the download path
3098+
* answers (#10246) or one row gets two answers. That question has exactly one
3099+
* definition, `findFileHolder`, and it lives in `@objectstack/service-storage`
3100+
* — a package this one does not and must not depend on. So the engine
3101+
* declares the seam and the storage plugin fills it, the same handover
3102+
* `resolveFileHolder` makes to the download routes.
3103+
*
3104+
* BATCHED on purpose: hydration runs over many rows per read, so a per-row
3105+
* holder check would be N queries per read. The resolver takes the whole
3106+
* tombstoned set and returns the ids still held.
3107+
*/
3108+
private _heldFileResolver?: HeldFileResolver;
3109+
3110+
/**
3111+
* Wire the batched holder question (#11427). Last registration wins; leaving
3112+
* it unwired keeps tombstoned files un-hydrated, which is what this engine
3113+
* did before the seam existed.
3114+
*/
3115+
registerHeldFileResolver(fn: HeldFileResolver): void {
3116+
this._heldFileResolver = fn;
3117+
this.logger.debug('Registered held-file resolver for sys_file hydration');
3118+
}
3119+
30753120
/**
30763121
* Register a middleware function
30773122
* Middlewares execute in onion model around every data operation.
@@ -8078,8 +8123,52 @@ export class ObjectQL implements IObjectQLEngine {
80788123
}
80798124

80808125
const fileMap = new Map<string, any>();
8126+
// [#11427] `committed` is servable and always was. A TOMBSTONE
8127+
// (`status: 'deleted'` + `deleted_at`) is recoverable state, not a delete:
8128+
// it is a claim about the future (this row is reapable once the grace
8129+
// window ends) that the sweep re-checks and often withdraws. #10246 already
8130+
// stopped the two download endpoints treating it as the last word — they
8131+
// ask the reap guard's own `findFileHolder` and serve the row for as long
8132+
// as something still holds it. This pass did not, so one `sys_file` row
8133+
// answered 200 at `/files/:id` and a bare id here: two read surfaces, two
8134+
// answers, and consumers render the bare id as "no attachment".
8135+
//
8136+
// ⛔ The predicate is NOT re-derived here. "Still held" has ONE definition
8137+
// (`findFileHolder`, a deliberate union of `sys_attachment` join rows AND
8138+
// the `ref_*` ownership columns) and it lives in the storage package, which
8139+
// this one cannot import. A copy narrower by a limb would hide files the
8140+
// sweep refuses to reap — the same defect one limb over — so the question
8141+
// arrives through {@link registerHeldFileResolver} instead, the same
8142+
// handover `resolveFileHolder` makes to the download routes. Unwired (bare
8143+
// kernel, no storage plugin, tests): tombstones stay hidden, exactly as
8144+
// before this existed.
8145+
const tombstoned: any[] = [];
80818146
for (const row of fileRows) {
8082-
if (row?.id != null && row.status === 'committed') fileMap.set(String(row.id), row);
8147+
if (row?.id == null) continue;
8148+
if (row.status === 'committed') fileMap.set(String(row.id), row);
8149+
else if (row.status === 'deleted') tombstoned.push(row);
8150+
}
8151+
// Lazy by construction: a batch with no tombstone — every ordinary read —
8152+
// costs nothing at all, and the resolver is BATCHED, so the residual case
8153+
// costs one extra query for the whole read rather than one per row.
8154+
if (tombstoned.length > 0 && this._heldFileResolver) {
8155+
try {
8156+
const held = await this._heldFileResolver(tombstoned);
8157+
for (const row of tombstoned) {
8158+
if (held?.has(String(row.id))) fileMap.set(String(row.id), row);
8159+
}
8160+
} catch (error) {
8161+
// Unreadable evidence is not evidence of a holder. Keep the ids
8162+
// un-hydrated — the answer this pass gave before #11427, and the same
8163+
// direction the download path fails in (`isServableForDownload`) and
8164+
// the reap guard fails in (it vetoes rather than reaps when it cannot
8165+
// tell). Distinct from the #6116 catch above, which covers the
8166+
// `sys_file` read itself and is untouched.
8167+
this.logger.warn(
8168+
'sys_file holder check failed; tombstoned file fields keep their raw ids for this read',
8169+
{ object: objectName, tombstonedIds: tombstoned.length, error: (error as Error)?.message },
8170+
);
8171+
}
80838172
}
80848173
if (fileMap.size === 0) return records;
80858174

‎packages/objectql/src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ export type { CompanionFieldMeta, CompanionObjectMeta } from './search-companion
6363

6464
// Export Engine
6565
export { ObjectQL, ObjectRepository, ScopedContext } from './engine.js';
66-
export type { HookHandler, HookEntry, OperationContext, EngineMiddleware } from './engine.js';
66+
export type { HookHandler, HookEntry, OperationContext, EngineMiddleware, HeldFileResolver } from './engine.js';
6767
export type { AdmittedValueShapeViolationTally } from './engine.js';
6868
export { SummaryRecomputeError } from './summary-errors.js';
6969
export type { SummaryRecomputeFailure } from './summary-errors.js';

‎packages/services/service-storage/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
},
3939
"devDependencies": {
4040
"@objectstack/objectql": "workspace:*",
41+
"@objectstack/driver-sql": "workspace:*",
4142
"@types/node": "^26.2.0",
4243
"typescript": "^6.0.3",
4344
"vitest": "^4.1.10"

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,54 @@ export async function findFileHolder(
365365
return hasFieldReferenceOwner(row) ? 'field-owner' : null;
366366
}
367367

368+
/**
369+
* The BATCHED form of {@link findFileHolder} — "which of these files is still
370+
* held?" — for callers holding many rows at once (#11427).
371+
*
372+
* Record file-field hydration is such a caller: it must reach the same verdict
373+
* the download path reaches (#10246) or one `sys_file` row gets two answers,
374+
* but it runs over many rows per read, so asking {@link findFileHolder} per
375+
* file would be N queries per read. This asks the SAME union in at most one
376+
* extra query for the whole batch.
377+
*
378+
* ⚠️ Same union, same limbs, deliberately in the cheaper order. {@link
379+
* findFileHolder} asks the join-row limb first because it must NAME the
380+
* surface; this one only needs "held or not", so it takes the free limb first:
381+
* {@link hasFieldReferenceOwner} is a pure column test on rows the caller has
382+
* already read, and every id it settles is an id the join-row query never has
383+
* to carry. `||` commutes, so the verdict is identical either way — pinned as
384+
* an equivalence in `tombstone-hydration-download-agreement.test.ts` rather
385+
* than asserted here.
386+
*
387+
* Cost, stated rather than assumed:
388+
* - no rows, or every row settled by the columns → ZERO queries;
389+
* - otherwise → exactly ONE `$in` read of `sys_attachment`, whatever the
390+
* number of files or records involved.
391+
*/
392+
export async function findHeldFiles(
393+
engine: Pick<AttachmentLifecycleEngine, 'find'>,
394+
rows: Array<Record<string, unknown>>,
395+
): Promise<Set<string>> {
396+
const held = new Set<string>();
397+
const needJoinCheck: string[] = [];
398+
for (const row of rows) {
399+
if (row?.id == null) continue;
400+
const id = String(row.id);
401+
// The free limb first — a pure test on a row already in hand.
402+
if (hasFieldReferenceOwner(row)) held.add(id);
403+
else needJoinCheck.push(id);
404+
}
405+
if (needJoinCheck.length === 0) return held;
406+
const refs = await engine.find('sys_attachment', {
407+
where: { file_id: { $in: needJoinCheck } },
408+
context: { ...SYSTEM_CTX },
409+
});
410+
for (const ref of refs ?? []) {
411+
if (ref?.file_id != null) held.add(String(ref.file_id));
412+
}
413+
return held;
414+
}
415+
368416
/**
369417
* The `sys_file` reap guard ({@link LifecycleReapGuard} shape from
370418
* `@objectstack/objectql`, duck-typed here to avoid the dependency).

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

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

0 commit comments

Comments
 (0)