Skip to content

Commit a41069b

Browse files
claude[bot]claude
andauthored
fix(storage): serve a tombstoned sys_file that still has a live holder (#11428)
A sys_file tombstone is recoverable state, not a delete: the reap guard re-verifies references at sweep time, finds a regained holder, un-tombstones the row and vetoes the reap. But the guard is the only thing that ever asked, and sys_file's declared lifecycle (ttl { field: 'deleted_at', expireAfter: '30d' }) nominates a tombstone only AFTER the window expires. Inside the window the file was not a sweep candidate at all, while both download endpoints refused anything not 'committed' — so a live attachment could point at a file that 404s for up to 30 days and then silently starts working. Ruled direction 2: the judgement moves to the read side, the revival mechanism does not move at all. No lifecycle verb is added; nothing on the download path writes to the row. The two download handlers carried a copy each of `file.status !== 'committed'`. They now share one predicate, isServableForDownload, which serves 'committed' unconditionally, refuses 'pending' unconditionally, and asks opts.resolveFileHolder about a tombstone. The plugin binds that to findFileHolder — the reap guard's own definition of "is anything still holding this file?", imported rather than re-derived — so the read verdict and the sweep verdict are the same question answered by the same code. Tests pin the pair, not the widening: tombstoned + a live holder serves 200 on both endpoints while the guard vetoes and reclaims nothing; the last holder removed 404s on both endpoints while the guard confirms the reap and deletes the bytes. Boundaries pinned unmoved: pending, bare kernel, a throwing holder question, a released field file, and the full authorization gate. Repairs one prose claim this change falsifies: the platform-checklist item attachments-storage.sys-file-status-pipeline stated the refusal as "status != committed", which a runner would now score as a failure of the fixed behaviour. Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 Co-authored-by: Claude <noreply@anthropic.com>
1 parent d0e3a88 commit a41069b

7 files changed

Lines changed: 448 additions & 9 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
"@objectstack/service-storage": patch
3+
---
4+
5+
**Fix:** a tombstoned `sys_file` that something still holds is downloadable again — no 30-day 404 in between (#10246).
6+
7+
Re-pointing a `sys_attachment` join row onto a file inside its 30-day grace-window tombstone has always been byte-safe: the reap guard re-verifies references at sweep time, finds the new holder, un-tombstones the row and vetoes the reap. But the sweep is the only thing that ever asked, and `sys_file`'s declared lifecycle (`ttl { field: 'deleted_at', expireAfter: '30d' }`) nominates a tombstone only **after** the window expires — measured candidates inside the window: `[]`. So the file simply sat at `status='deleted'` while `GET /api/v1/storage/files/:fileId` and `/files/:fileId/url` refused anything not `committed`. A live attachment could point at a file that 404s for up to 30 days and then silently starts working.
8+
9+
**What changed:** the two download endpoints stop treating the tombstone as the last word. They now ask the reap guard's own `findFileHolder` — the single definition of "is anything still holding this file?", a union over `sys_attachment` join rows and the `ref_*` ownership columns — and serve the file for exactly as long as that answers yes.
10+
11+
**What did not change**, deliberately:
12+
13+
- **No lifecycle verb was added.** There is no un-tombstone, revive or resurrect on the read path; the download writes nothing to the row. Revival remains solely the sweep guard's, which is why the fix is a read-side predicate and not a second revival mechanism (the duplicate-mechanism hazard #10241 avoided). The tombstone stays, and the sweep still reaps when the last holder goes.
14+
- **`pending` is still refused.** Only the `deleted` limb widened; an upload that was never completed has no bytes to promise.
15+
- **Authorization is untouched.** A served tombstone goes through the same `authorizeFileRead` gate as any other file — `AUTH_REQUIRED` (401) and `ATTACHMENT_DOWNLOAD_DENIED` / `FILE_DOWNLOAD_DENIED` (403) are unaffected. Servability is not authorization.
16+
- **Bare kernels are unaffected.** With no data engine there is no holder question to ask, so tombstones stay refused exactly as before.
17+
18+
The read side and the sweep now answer the same question from the same code, so a file the download path serves is by construction a file the next sweep would veto rather than reap — and the instant the last holder goes, both flip together. That pair is what the new tests pin; the 404 text on the refusal changed from "File not found or not committed" to "File not found or not downloadable" to match (the `FILE_NOT_FOUND` code is unchanged).

content/docs/permissions/attachments-access.mdx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,13 @@ 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).
112119
- **`sys_upload_session`** — abandoned/terminal chunked-upload sessions are
113120
reaped, and a reap guard aborts the underlying backend multipart upload
114121
(S3 `AbortMultipartUpload` / local parts dir) first, so already-uploaded

docs/qa/platform-checklist/areas/attachments-storage.json

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@
433433
"title": "sys_file status pipeline: pending → committed → deleted (tombstone) with un-tombstone on re-attach; shared files never tombstone early",
434434
"since": "v15.1",
435435
"status": "active",
436-
"revision": 1,
436+
"revision": 2,
437437
"priority": "P2",
438438
"surface": "api",
439439
"personas": ["seeded admin (admin@objectos.ai)"],
@@ -445,11 +445,13 @@
445445
},
446446
"steps": [
447447
"presign an attachments-scope upload and read the sys_file row: status 'pending'",
448-
"attempt GET /api/v1/storage/files/<fileId>/url while still pending and capture the refusal (downloads only serve committed files)",
448+
"attempt GET /api/v1/storage/files/<fileId>/url while still pending and capture the refusal (a never-completed upload has no bytes to promise — this is the PENDING refusal, not a 'committed-only' rule)",
449449
"complete the upload; re-read: status 'committed'",
450450
"attach the file to 'Website Relaunch' AND 'Data Platform' (two sys_attachment join rows over ONE file — the Salesforce ContentDocumentLink share pattern)",
451451
"delete the 'Data Platform' join row and re-read sys_file: still 'committed' (a remaining reference blocks the tombstone)",
452452
"delete the LAST join row and re-read: status 'deleted' with deleted_at set (the tombstone)",
453+
"while tombstoned AND holder-less, GET /api/v1/storage/files/<fileId>/url: 404 FILE_NOT_FOUND",
454+
"attach a NEW join row onto the still-tombstoned file_id and immediately GET the download URL again, WITHOUT waiting for any sweep: 200 (#10246 — the download path asks the reap guard's own holder question, so a file the sweep would refuse to reap is a file the download path serves)",
453455
"re-attach the same file_id to 'Website Relaunch' within the grace window and re-read: status back to 'committed', deleted_at null",
454456
"verify a NON-attachments-scope file (e.g. an invoice-line receipt, scope from the field-upload path) is never tombstoned by these join-row hooks"
455457
],
@@ -461,11 +463,17 @@
461463
"evidence": "the read sequence, one per transition"
462464
},
463465
{
464-
"clause": "a pending (never-completed) file is not downloadable: the download routes answer 404 FILE_NOT_FOUND for status != committed",
466+
"clause": "a pending (never-completed) file is not downloadable: the download routes answer 404 FILE_NOT_FOUND while status is 'pending'",
465467
"oracle": "api",
466468
"verify": "the /url GET during the pending window returns 404 with that code",
467469
"evidence": "the 404 body"
468470
},
471+
{
472+
"clause": "the refusal is keyed to NO REMAINING HOLDER, not to the tombstone: a 'deleted' file that still has at least one live sys_attachment join row (or a live ref_* owner) downloads with 200 immediately, with no sweep in between; the tombstone row is NOT rewritten by the download (#10246)",
473+
"oracle": "api",
474+
"verify": "GET /url on the tombstoned file 404s while holder-less and 200s once a join row is attached, in the same grace window; a sys_file re-read after the 200 still shows status 'deleted' with deleted_at set",
475+
"evidence": "the two /url responses plus the post-download sys_file read"
476+
},
469477
{
470478
"clause": "one file shared by two join rows survives losing one of them — deleting an attachment deletes only the join row; the tombstone fires only when the LAST reference goes",
471479
"oracle": "api",
@@ -499,7 +507,8 @@
499507
"docs/plans/release-15.1-test-plan.md §C4 (#2755)"
500508
],
501509
"history": [
502-
{ "revision": 1, "date": "2026-08-07", "change": "new item: the full status pipeline as a variants matrix over the sys_file.status enum, with the shared-file and re-attach transitions from the lifecycle-hook source", "ref": "claude/platform-test-checklist-ocwugl" }
510+
{ "revision": 1, "date": "2026-08-07", "change": "new item: the full status pipeline as a variants matrix over the sys_file.status enum, with the shared-file and re-attach transitions from the lifecycle-hook source", "ref": "claude/platform-test-checklist-ocwugl" },
511+
{ "revision": 2, "date": "2026-08-23", "change": "clause 2 was FALSIFIED by #10246 and is repaired here, not merely re-worded. It stated the download refusal as 'status != committed', which was true when written and is not any more: the download routes now ask the reap guard's own findFileHolder before refusing a tombstone, so a 'deleted' file with a live sys_attachment join row (or a live ref_* owner) serves 200 inside the grace window instead of 404ing for up to 30 days and then silently starting to work once a sweep ran. A runner scoring the old clause would have marked the FIXED behaviour as a failure. The clause is now keyed to 'pending', which is the limb that genuinely still refuses; a new clause pins the widened one AS A PAIR (404 while holder-less, 200 once a holder exists, tombstone row unrewritten) because proving only the 200 proves that something got wider and not that the boundary held. Two steps added for the new clause, and step 2's parenthetical stopped asserting a committed-only rule. Note for the next author: docs-drift could not have caught this — it is symbol-anchor precision-first (#9192) and cannot see a prose claim inside a clause string.", "ref": "#10246" }
503512
]
504513
},
505514
{

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

Lines changed: 73 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from '
66
import { sendOk, sendError } from '@objectstack/types';
77
import type { StorageMetadataStore, FileRecord, UploadSessionRecord } from './metadata-store.js';
88
import type { LocalStorageAdapter } from './local-storage-adapter.js';
9+
// Type only. The PREDICATE is never re-implemented in this file (#10246): it
10+
// arrives through `opts.resolveFileHolder`, which the plugin binds to the reap
11+
// guard's own `findFileHolder`.
12+
import type { FileHolder } from './attachment-lifecycle.js';
913
import { contentDispositionValue } from './content-disposition.js';
1014

1115
/** Authorization verdict for an attachments-scope download (#2970 item 2). */
@@ -47,6 +51,35 @@ export interface StorageRoutesOptions {
4751
* When absent (bare kernels, tests), all downloads stay open (back-compat).
4852
*/
4953
authorizeFileRead?: (file: FileRecord, req: IHttpRequest) => Promise<FileReadVerdict>;
54+
/**
55+
* "Is anything still holding this file?" for a TOMBSTONED row (#10246).
56+
*
57+
* A `sys_file` tombstone (`status: 'deleted'` + `deleted_at`) is recoverable
58+
* state, not a delete: re-pointing a `sys_attachment` join row onto it, or
59+
* re-claiming it through the `ref_*` ownership columns, makes it live again
60+
* — and the sweep already honours that, un-tombstoning and vetoing the reap
61+
* instead of reclaiming the bytes. But the sweep is the only thing that ever
62+
* asks, and it asks only AFTER the declared 30d TTL expires, so inside the
63+
* grace window a live attachment pointed at a tombstone downloaded as 404
64+
* for up to 30 days and then silently started working.
65+
*
66+
* ⛔ This does NOT add a second revival mechanism. Revival stays solely the
67+
* sweep guard's; nothing on the read path writes to the row. What moves here
68+
* is the JUDGEMENT — the download path stops treating the tombstone as the
69+
* last word and asks the same question the guard asks.
70+
*
71+
* ⚠️ Wire this to `findFileHolder` (`attachment-lifecycle.ts`) and to
72+
* nothing else. That function is the ONE definition of "still held", a
73+
* deliberate union of the two surfaces that can hold a `sys_file` —
74+
* `sys_attachment` join rows AND the `ref_*` ownership columns — and it is
75+
* what decides whether the next sweep reaps this row. A read side that
76+
* re-derived a narrower question (join rows only, say) would refuse files
77+
* the sweep refuses to reap: the same defect, one limb over.
78+
*
79+
* Absent (bare kernels, no data engine, tests that don't wire it): tombstones
80+
* stay refused, exactly as before this option existed.
81+
*/
82+
resolveFileHolder?: (file: FileRecord) => Promise<FileHolder>;
5083
/**
5184
* TTL (seconds) for the signed URL minted on a GATED attachments download.
5285
* Short by design — the link is followed immediately after an explicit
@@ -133,6 +166,42 @@ export function registerStorageRoutes(
133166
return downloadTtl;
134167
};
135168

169+
// ── Download servability (#10246) ────────────────────────────────────
170+
// Written ONCE and called by both download endpoints. They used to carry a
171+
// copy each of `file.status !== 'committed'`, which is how a rule that needs
172+
// to widen turns into two rules that drift; `/files/:fileId/url` and
173+
// `/files/:fileId` are the same decision reached through two doors.
174+
//
175+
// - `committed` → servable, unconditionally and unchanged.
176+
// - `pending` → refused, unconditionally and unchanged: an upload that
177+
// was never completed has no bytes to promise.
178+
// - `deleted` → servable for exactly as long as something still holds
179+
// it. The tombstone is NOT the last word; it is a claim
180+
// about the future (this row is reapable when the grace
181+
// window ends) that the sweep re-checks and often
182+
// withdraws. Asking the guard's own question here makes
183+
// the two agree by construction: a file this returns
184+
// `true` for is a file the next sweep would un-tombstone
185+
// rather than reap, and the moment the last holder goes it
186+
// returns `false` again — same instant the sweep starts
187+
// reaping it.
188+
//
189+
// The row is never written to. Revival remains the sweep guard's alone
190+
// (triage's ruling on this card: 复活机制仍唯一归 sweep guard,判断移到读侧,
191+
// 不新增生命周期动词).
192+
const isServableForDownload = async (file: FileRecord): Promise<boolean> => {
193+
if (file.status === 'committed') return true;
194+
if (file.status !== 'deleted' || !opts.resolveFileHolder) return false;
195+
try {
196+
return (await opts.resolveFileHolder(file)) !== null;
197+
} catch {
198+
// Unreadable evidence is not evidence of a holder. Refuse — the same
199+
// answer this route gave before #10246, and the same direction the reap
200+
// guard fails in (it vetoes rather than reaps when it cannot tell).
201+
return false;
202+
}
203+
};
204+
136205
// ── Upload auth gate (#2755) ─────────────────────────────────────────
137206
// `false` ⇒ the 401 was already sent and the handler must stop.
138207
// `null` ⇒ open mode (no resolver wired) — proceed unauthenticated.
@@ -603,8 +672,8 @@ export function registerStorageRoutes(
603672
try {
604673
const { fileId } = req.params;
605674
const file = await store.getFile(fileId);
606-
if (!file || file.status !== 'committed') {
607-
sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed');
675+
if (!file || !(await isServableForDownload(file))) {
676+
sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not downloadable');
608677
return;
609678
}
610679

@@ -650,8 +719,8 @@ export function registerStorageRoutes(
650719
try {
651720
const { fileId } = req.params;
652721
const file = await store.getFile(fileId);
653-
if (!file || file.status !== 'committed') {
654-
sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed');
722+
if (!file || !(await isServableForDownload(file))) {
723+
sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not downloadable');
655724
return;
656725
}
657726

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

Lines changed: 11 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 } from './attachment-lifecycle.js';
24+
import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder } 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';
@@ -413,6 +413,16 @@ export class StorageServicePlugin implements Plugin {
413413
sessionTtl: this.options.sessionTtl,
414414
resolveSession: buildAuthSessionResolver(ctx),
415415
authorizeFileRead: buildFileReadAuthorizer(ctx, engine),
416+
// "Is anything still holding this tombstone?" on the READ side
417+
// (#10246) — the reap guard's own `findFileHolder`, handed over
418+
// rather than re-derived. One definition of "still held", asked by
419+
// the sweep before it reaps and by the download path before it
420+
// refuses, so the two cannot answer differently. No engine (bare
421+
// kernel) leaves it undefined and tombstones stay refused.
422+
resolveFileHolder:
423+
engine && typeof (engine as any).find === 'function'
424+
? (file: FileRecord) => findFileHolder(engine as any, file.id, file as any)
425+
: undefined,
416426
logger: ctx.logger,
417427
});
418428

0 commit comments

Comments
 (0)