Skip to content

Commit b1863a5

Browse files
os-zhuangclaude
andauthored
feat(storage): enable released-field-file collection behind the per-deployment migration gate — ADR-0104 D3 wave 2 PR-5b (#3459) (#4208)
The gated final step of the file-as-reference sequence, both halves in one change as #3459 requires: - releaseOwnership tombstones a released committed file (status='deleted' + deleted_at, starting the declared 30-day grace window) — but only on a deployment whose adr-0104-file-references flag is verified, read through the engine's memoized seam (now public), failing closed every way of not knowing. - The sys_file reap guard's sweep-time re-verify extends to the ownership columns (a re-claimed file is un-tombstoned and vetoed) and re-reads the deployment flag fresh before any byte delete, so a regressed gate closes for already-written tombstones without a restart. Attachments-scope collection is unchanged and needs no flag. Unmigrated deployments keep today's behaviour exactly: release clears the ownership columns and files are retained forever. The R4 regression tests flip to the new invariant and lock the two halves together. Claude-Session: https://claude.ai/code/session_01KM46HedAWLbkuZyMBXn5wJ Co-authored-by: Claude <noreply@anthropic.com>
1 parent bca935b commit b1863a5

10 files changed

Lines changed: 399 additions & 57 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
"@objectstack/service-storage": minor
3+
---
4+
5+
feat(storage): released field files enter collection on deployments that verified their file migration — ADR-0104 D3 wave 2 PR-5b (#3459)
6+
7+
The gated, final step of the file-as-reference sequence. On a deployment whose
8+
`adr-0104-file-references` flag is verified (`os migrate files-to-references
9+
--apply`, #3617), releasing a field file's ownership — clearing the field, or
10+
deleting the owning record — now also tombstones the file
11+
(`status='deleted'` + `deleted_at`), which starts the `sys_file` lifecycle's
12+
declared 30-day grace window and, at its end, hands the row to the reap sweep.
13+
Re-referencing the id inside the window revives it, exactly like re-attaching
14+
an attachment.
15+
16+
**The two halves ship together, deliberately.** The same change extends the
17+
reap guard's sweep-time re-verify beyond `sys_attachment` join rows to the
18+
ownership columns: a tombstoned file whose `ref_*` columns name a current
19+
owner (re-claimed in the window, or a release/claim race) is un-tombstoned and
20+
vetoed. Tombstoning released files without that re-verify would have turned
21+
every release into a *guaranteed* byte delete — the guard's old check consults
22+
a table that is always empty for field files. This pairing was the standing
23+
hard constraint on #3459, locked by regression tests on both halves.
24+
25+
**Nothing changes for a deployment that has not migrated.** Release keeps
26+
clearing the ownership columns only, and released files are retained forever.
27+
Every way of not knowing — no flag row, an unreadable table, an engine that
28+
cannot be asked — reads as "not verified": the gate fails closed, toward
29+
retention. And the guard re-reads the flag *fresh* at sweep time (not the
30+
release path's memoized read), so a later failing migration run — a database
31+
that has drifted — closes the gate for already-written tombstones too, without
32+
a restart. Attachments-scope collection is unchanged and needs no flag.
33+
34+
The irreversible moment is therefore per deployment: day 30 after *that*
35+
deployment verified its migration and released a file — never the upgrade
36+
itself.
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
feat(objectql): `engine.isFileReferencesMigrationVerified()` is public — one memoized flag read for both in-process consumers (#3459 PR-5b)
6+
7+
The memoized per-deployment read of the `adr-0104-file-references` migration
8+
flag was private to the engine's media value-shape enforcement. The storage
9+
service's release path now asks the same question — may a released field file
10+
be tombstoned? — so the method is public and the release hooks reach it as an
11+
optional duck-typed member (an older engine or a test fake reads as "not
12+
verified", failing closed). One read, one invalidation
13+
(`invalidateDataMigrationFlags()`), no way for the two consumers to see
14+
different answers.

content/docs/deployment/cli.mdx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -657,7 +657,7 @@ Exit status is `0` only when the self-check passes, so CI can gate on it.
657657
| Once verified | Effect |
658658
| :--- | :--- |
659659
| **Media value shapes** | A malformed `file` / `image` / `avatar` / `video` / `audio` value is **rejected** (`400 invalid_type`) instead of warned about. Set `OS_ALLOW_LAX_MEDIA_VALUES=1` to re-open leniency while diagnosing. |
660-
| **Released-file collection** | Not yet shipped — the flag is the gate it will read. |
660+
| **Released-file collection** | A field file whose one owning record lets go (the field is cleared or the record deleted) is tombstoned into the declared 30-day grace window; re-referencing the id within the window revives it, and after it the platform sweep reclaims the row and its bytes. Unverified deployments keep every released file forever. |
661661

662662
Other value classes are unaffected: a `lookup` or `location` value keeps its own
663663
warn-first rollout, because this migration is evidence about *file* values and
@@ -670,7 +670,9 @@ later run that *fails* its self-check clears the flag's verified state, so a
670670
database that has drifted closes its own gate.
671671

672672
A running server reads the flag once; after migrating, **restart it** for
673-
enforcement to take effect.
673+
enforcement (and release-time tombstoning) to take effect. The sweep's final
674+
delete check re-reads the flag fresh, so a later failing run stops collection
675+
without a restart.
674676
</Callout>
675677

676678
### Scaffolding

content/docs/releases/v17.mdx

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -906,9 +906,19 @@ os migrate files-to-references --apply # converts, verifies, records the flag
906906
The run backfills legacy file-field values (inline metadata blobs, own-resolver
907907
URLs, `data:` URIs) into owned `sys_file` references and reconciles the ownership
908908
ledger against what records actually hold. The **deployment-level flag it
909-
records** — never the platform version — is what may later authorise
910-
irreversible behaviour, and media value shapes enforce only once *this*
911-
deployment has verified its own migration.
909+
records** — never the platform version — is what authorises irreversible
910+
behaviour, and media value shapes enforce only once *this* deployment has
911+
verified its own migration.
912+
913+
**Released-file collection is live behind that same flag** (#3459). On a
914+
verified deployment, a field file whose one owning record lets go — the field
915+
cleared, or the record deleted — is tombstoned into the declared 30-day grace
916+
window; re-referencing the id inside the window revives it, and past it the
917+
platform sweep re-verifies at delete time that nothing holds the file (join
918+
rows, ownership columns, and a fresh read of the flag itself) before
919+
reclaiming the row and its bytes. A deployment that never migrates keeps every
920+
released file forever: upgrading is not consent — passing your own
921+
migration's self-check is.
912922

913923
### Approvals: dynamic approver routing (#3447)
914924

packages/objectql/src/engine.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2203,15 +2203,21 @@ export class ObjectQL implements IDataEngine {
22032203
*
22042204
* Every way of not knowing answers `false` — no storage service (so no
22052205
* `sys_migration` object registered), no row, an unreadable table, a
2206-
* malformed row. That is the same posture the flag's other consumer takes:
2207-
* enforcement derives from evidence, and absent evidence is not permission.
2208-
* Here "no" means media value shapes keep warning instead of rejecting, so
2209-
* a deployment that cannot be asked keeps writing.
2206+
* malformed row. Enforcement derives from evidence, and absent evidence is
2207+
* not permission. Here "no" means media value shapes keep warning instead
2208+
* of rejecting, so a deployment that cannot be asked keeps writing.
2209+
*
2210+
* Public because the flag's other in-process consumer reads it through this
2211+
* same memoized seam: the storage service's release path (#3459 PR-5b) asks
2212+
* it whether a released field file may be tombstoned, duck-typed as an
2213+
* optional method so a fake or an older engine reads as "not verified".
2214+
* One read, one invalidation (`invalidateDataMigrationFlags`), no way for
2215+
* the two consumers to see different answers.
22102216
*
22112217
* Costs nothing on a kernel without the storage objects: the registry lookup
22122218
* short-circuits before any query.
22132219
*/
2214-
private async isFileReferencesMigrationVerified(): Promise<boolean> {
2220+
async isFileReferencesMigrationVerified(): Promise<boolean> {
22152221
if (!this.fileReferencesMigrationVerified) {
22162222
this.fileReferencesMigrationVerified = (async () => {
22172223
if (!this._registry.getObject(DATA_MIGRATION_FLAG_OBJECT)) return false;
@@ -2234,7 +2240,8 @@ export class ObjectQL implements IDataEngine {
22342240
if (verified) {
22352241
this.logger.info(
22362242
'[value-shape] this deployment has verified the file-as-reference migration — ' +
2237-
'media value shapes are enforced (ADR-0104 D1 / #3617)',
2243+
'media value shapes are enforced and released field files may be collected ' +
2244+
'(ADR-0104 / #3617)',
22382245
);
22392246
}
22402247
return verified;

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

Lines changed: 107 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,13 @@ describe('installAttachmentLifecycleHooks — tombstoning', () => {
180180
describe('createSysFileReapGuard', () => {
181181
const storage = () => ({ delete: vi.fn(async () => {}) }) as any;
182182

183-
it('confirms zero-ref tombstones after deleting the bytes', async () => {
183+
it('confirms zero-ref attachment tombstones after deleting the bytes — no migration flag needed', async () => {
184184
const engine = fakeEngine({ attachments: [], files: [] });
185185
const s = storage();
186186
const guard = createSysFileReapGuard(engine, () => s, silentLogger());
187187

188188
const confirmed = await guard('sys_file', [
189-
{ id: 'f1', key: 'attachments/f1.bin', status: 'deleted' },
189+
{ id: 'f1', key: 'attachments/f1.bin', status: 'deleted', scope: 'attachments' },
190190
]);
191191

192192
expect(s.delete).toHaveBeenCalledWith('attachments/f1.bin');
@@ -217,13 +217,117 @@ describe('createSysFileReapGuard', () => {
217217
const guard = createSysFileReapGuard(engine, () => s, logger);
218218

219219
const confirmed = await guard('sys_file', [
220-
{ id: 'f1', key: 'attachments/f1.bin', status: 'deleted' },
220+
{ id: 'f1', key: 'attachments/f1.bin', status: 'deleted', scope: 'attachments' },
221221
]);
222222

223223
expect(confirmed).toEqual([]);
224224
expect(logger.warn).toHaveBeenCalled();
225225
});
226226

227+
// ── Field-file lineage (#3459 PR-5b) ──────────────────────────────
228+
// The gated, irreversible half-pair: released field files only become byte
229+
// deletes when (a) nothing owns them at sweep time AND (b) this deployment's
230+
// file-as-reference migration flag is verified, re-read fresh each sweep.
231+
232+
it('reaps a released field file when the deployment gate is open', async () => {
233+
const engine = fakeEngine({ attachments: [], files: [] });
234+
const s = storage();
235+
const guard = createSysFileReapGuard(engine, () => s, silentLogger(), async () => true);
236+
237+
const confirmed = await guard('sys_file', [
238+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user', ref_object: null, ref_id: null },
239+
]);
240+
241+
expect(s.delete).toHaveBeenCalledWith('user/f1.png');
242+
expect(confirmed).toEqual(['f1']);
243+
});
244+
245+
/**
246+
* R4 REGRESSION (the "two halves ship together" lock, #3459 PR-5b). A
247+
* tombstoned file whose ownership columns name a current owner — re-claimed
248+
* inside the grace window, or a release/claim race — must be un-tombstoned
249+
* and vetoed, exactly like an attachment that regained join rows. Without
250+
* this, every release would be a guaranteed byte delete.
251+
*/
252+
it('vetoes and un-tombstones a field file that regained an owner (ownership re-verify)', async () => {
253+
const engine = fakeEngine({
254+
attachments: [],
255+
files: [{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user' }],
256+
});
257+
const s = storage();
258+
const guard = createSysFileReapGuard(engine, () => s, silentLogger(), async () => true);
259+
260+
const confirmed = await guard('sys_file', [
261+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user', ref_object: 'product', ref_id: 'p1', ref_field: 'image' },
262+
]);
263+
264+
expect(confirmed).toEqual([]);
265+
expect(s.delete).not.toHaveBeenCalled();
266+
expect(engine.updates[0].data).toMatchObject({ id: 'f1', status: 'committed', deleted_at: null });
267+
});
268+
269+
it('vetoes field-file tombstones when no gate callback is wired (fail closed)', async () => {
270+
const engine = fakeEngine({ attachments: [], files: [] });
271+
const s = storage();
272+
const logger = silentLogger();
273+
const guard = createSysFileReapGuard(engine, () => s, logger);
274+
275+
const confirmed = await guard('sys_file', [
276+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user' },
277+
]);
278+
279+
expect(confirmed).toEqual([]);
280+
expect(s.delete).not.toHaveBeenCalled();
281+
// Kept tombstoned — the observed release stands; only deletion is withheld.
282+
expect(engine.updates).toHaveLength(0);
283+
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('files-to-references'));
284+
});
285+
286+
it('vetoes field-file tombstones while the deployment gate is closed (flag regression)', async () => {
287+
const engine = fakeEngine({ attachments: [], files: [] });
288+
const s = storage();
289+
const guard = createSysFileReapGuard(engine, () => s, silentLogger(), async () => false);
290+
291+
const confirmed = await guard('sys_file', [
292+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user' },
293+
]);
294+
295+
expect(confirmed).toEqual([]);
296+
expect(s.delete).not.toHaveBeenCalled();
297+
});
298+
299+
it('a failing gate read vetoes field files but never blocks attachment reaps', async () => {
300+
const engine = fakeEngine({ attachments: [], files: [] });
301+
const s = storage();
302+
const guard = createSysFileReapGuard(engine, () => s, silentLogger(), async () => {
303+
throw new Error('sys_migration unreadable');
304+
});
305+
306+
const confirmed = await guard('sys_file', [
307+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user' },
308+
{ id: 'a1', key: 'attachments/a1.bin', status: 'deleted', scope: 'attachments' },
309+
]);
310+
311+
expect(confirmed).toEqual(['a1']);
312+
expect(s.delete).toHaveBeenCalledTimes(1);
313+
expect(s.delete).toHaveBeenCalledWith('attachments/a1.bin');
314+
});
315+
316+
it('reads the gate once per sweep batch', async () => {
317+
const engine = fakeEngine({ attachments: [], files: [] });
318+
const s = storage();
319+
const isOpen = vi.fn(async () => true);
320+
const guard = createSysFileReapGuard(engine, () => s, silentLogger(), isOpen);
321+
322+
await guard('sys_file', [
323+
{ id: 'f1', key: 'user/f1.png', status: 'deleted', scope: 'user' },
324+
{ id: 'f2', key: 'user/f2.png', status: 'deleted', scope: 'user' },
325+
{ id: 'f3', key: 'user/f3.png', status: 'deleted', scope: 'user' },
326+
]);
327+
328+
expect(isOpen).toHaveBeenCalledTimes(1);
329+
});
330+
227331
it('confirms abandoned pending uploads with best-effort byte cleanup', async () => {
228332
const engine = fakeEngine({ attachments: [], files: [] });
229333
const s = storage();

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

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,14 @@ import type { IStorageService } from '@objectstack/spec/contracts';
2424
* confirming the row delete. Detection and scheduling stay inside the
2525
* single platform sweep — ADR-0057 §3.3, no bespoke sweeper.
2626
*
27-
* Only `scope === 'attachments'` files are ever tombstoned: `Field.file` /
28-
* `Field.image` / avatar uploads use other scopes and reference files from
29-
* record columns the join-row count cannot see.
27+
* The hooks in THIS file only ever tombstone `scope === 'attachments'` files:
28+
* `Field.file` / `Field.image` / avatar uploads use other scopes and reference
29+
* files from record columns the join-row count cannot see. Field-owned files
30+
* have their own tombstone seam — `releaseOwnership` in
31+
* `file-reference-lifecycle.ts`, active only on a deployment that has verified
32+
* its file-as-reference migration (#3617) — and the reap guard below
33+
* re-verifies their ownership columns, and re-reads that deployment flag, at
34+
* sweep time (#3459 PR-5b).
3035
*/
3136

3237
/** Engine surface these installers need — duck-typed like the other
@@ -196,21 +201,47 @@ export function installAttachmentLifecycleHooks(
196201
*
197202
* - `pending`: the upload was never completed; bytes may or may not exist.
198203
* Best-effort byte delete, then confirm.
199-
* - `deleted`: re-verify ZERO sys_attachment references at sweep time.
200-
* References found (hook bypass, restore) → un-tombstone and veto.
201-
* Zero references → delete bytes; a byte-delete failure vetoes so the
202-
* row is retried next sweep (the row is the only pointer to the bytes —
203-
* dropping it first would leak the bytes forever).
204+
* - `deleted`: re-verify at sweep time that nothing holds the file on
205+
* EITHER surface — zero `sys_attachment` join rows AND empty ownership
206+
* columns (`ref_*`). Either found (hook bypass, restore, re-claim) →
207+
* un-tombstone and veto. A tombstone outside the `attachments` scope is
208+
* field-file lineage (#3459 PR-5b) and additionally requires this
209+
* deployment's `adr-0104-file-references` flag to be verified — re-read
210+
* fresh each sweep via `isCollectionOpen`, so a regression recorded since
211+
* (a later failing migration run clears `verified_at`) stops
212+
* already-written tombstones from becoming byte deletes, without a
213+
* restart. A closed gate vetoes but does NOT un-tombstone: the observed
214+
* release stands; only the permission to delete is withheld.
215+
* Clear on both counts → delete bytes; a byte-delete failure vetoes so
216+
* the row is retried next sweep (the row is the only pointer to the
217+
* bytes — dropping it first would leak the bytes forever).
204218
* - anything else: veto (shouldn't be a candidate; fail toward retention).
219+
*
220+
* `isCollectionOpen` absent (an older caller, a test fake) reads as "gate
221+
* closed": field-file tombstones are kept, attachments behave as always.
205222
*/
206223
export function createSysFileReapGuard(
207224
engine: AttachmentLifecycleEngine,
208225
getStorage: () => IStorageService | null | undefined,
209226
logger: AttachmentLifecycleLogger,
227+
isCollectionOpen?: () => Promise<boolean>,
210228
): (object: string, rows: Array<Record<string, unknown>>) => Promise<Array<string | number>> {
211229
return async (_object, rows) => {
212230
const confirmed: Array<string | number> = [];
213231
const storage = getStorage();
232+
// One fresh flag read per sweep batch, taken lazily so a batch with no
233+
// field-file tombstone costs nothing.
234+
let gate: Promise<boolean> | undefined;
235+
const collectionOpen = () =>
236+
(gate ??= (async () => {
237+
if (typeof isCollectionOpen !== 'function') return false;
238+
try {
239+
return (await isCollectionOpen()) === true;
240+
} catch {
241+
return false; // unreadable evidence → the gate is closed
242+
}
243+
})());
244+
let keptGateClosed = 0;
214245
for (const row of rows) {
215246
const id = row?.id as string | number | undefined;
216247
if (id === undefined || id === null) continue;
@@ -234,17 +265,22 @@ export function createSysFileReapGuard(
234265
limit: 1,
235266
context: { ...SYSTEM_CTX },
236267
});
237-
if (refs?.length) {
268+
const owned = row.ref_object != null && row.ref_id != null && row.ref_id !== '';
269+
if (refs?.length || owned) {
238270
await engine.update(
239271
'sys_file',
240272
{ id, status: 'committed', deleted_at: null },
241273
{ context: { ...SYSTEM_CTX } },
242274
);
243275
logger.info(
244-
`[storage] reap guard: sys_file ${id} regained references since tombstoning — un-tombstoned, not reaped`,
276+
`[storage] reap guard: sys_file ${id} regained ${refs?.length ? 'references' : 'an owner'} since tombstoning — un-tombstoned, not reaped`,
245277
);
246278
continue;
247279
}
280+
if (row.scope !== 'attachments' && !(await collectionOpen())) {
281+
keptGateClosed += 1;
282+
continue;
283+
}
248284
if (storage && typeof row.key === 'string' && row.key) await storage.delete(row.key);
249285
confirmed.push(id);
250286
} catch (err) {
@@ -256,6 +292,12 @@ export function createSysFileReapGuard(
256292
}
257293
// Not a state this guard reaps — veto (fail toward retention).
258294
}
295+
if (keptGateClosed > 0) {
296+
logger.info(
297+
`[storage] reap guard: kept ${keptGateClosed} released field file(s) — this deployment's ` +
298+
`file-as-reference migration is not verified (run \`os migrate files-to-references --apply\`)`,
299+
);
300+
}
259301
return confirmed;
260302
};
261303
}

0 commit comments

Comments
 (0)