Skip to content

Commit 3d3fddf

Browse files
os-zhuangclaude
andauthored
feat(storage): legacy file-value backfill — ADR-0104 D3 wave 2 (PR-6) (#3535)
* feat(storage): legacy file-value backfill — ADR-0104 D3 wave 2 (PR-6) backfillFileReferences() converts the pre-reference forms a file/image/avatar/ video/audio field may hold — an inline metadata blob ({url, name, size, …}) or a bare URL string — into the reference form: an opaque sys_file id, owned by the record's field. What it will and will not convert: - A URL naming this platform's own resolver (…/storage/files/:id) already identifies a sys_file; the field is rewritten to the bare id, no bytes move. - A data: URI carries its bytes inline; they are uploaded, a sys_file is registered, and the field is rewritten to its id. - An external URL is REPORTED, NEVER CONVERTED. Re-hosting third-party content is a bandwidth, licensing and privacy decision that is not a migration's to make. ADR-0104 R7 retires these toward an explicit `url` field, which under AI authoring is the point: it stops "managed file" and "external link" being the same declaration. Dry run by default — nothing is written unless apply is set, and the dry-run report has the same shape as the applied one so the plan can be reviewed and diffed against what actually happened. Idempotent — a value already in reference form is recorded as already_id and left alone, so a partially completed run is safe to repeat. The backfill never writes the ref_* columns itself: it rewrites the record, and the claim hooks observe that write and record ownership. One claiming path, so there is nothing that can disagree with itself — asserted by a test that fails if the backfill ever starts touching sys_file directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd * docs(adr-0104): backfill must precede the write cutover, not follow it The 2026-07-27 addendum sequenced the write cutover before the backfill. That is wrong for the same reason the backfill must precede collection, one step further back: narrowing the accepted stored form while records still hold legacy values would reject any update that rewrites such a field, breaking working apps until the backfill catches up. Backfilling first leaves the cutover nothing legacy to reject. Also states plainly why the last two steps are held to different standards. The cutover breaks loudly and recoverably (a rejected write); enabling collection breaks silently and permanently (deleted bytes). Only the second earns the reconciliation evidence requirement, and neither it nor the migration run should happen without an explicit human decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd * fix(storage): parse resolver URLs structurally instead of with a backtracking regex CodeQL js/polynomial-redos: the resolver-URL pattern was anchored only at the tail, so the engine retried from every start position. The values it scans come straight out of tenant records, so a value repeating the matched prefix turns that into a polynomial blowup on attacker-influenced data. Replaced with slicing and splitting — linear in the URL's length whatever it contains, and clearer about what it actually accepts. Covered by a timing regression on an adversarial value plus a table of accepted shapes and near-misses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d1cabaa commit 3d3fddf

5 files changed

Lines changed: 805 additions & 9 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/service-storage": minor
3+
---
4+
5+
feat(storage): legacy file-value backfill — ADR-0104 D3 wave 2 (PR-6)
6+
7+
`backfillFileReferences()` converts the pre-reference forms a `file`/`image`/
8+
`avatar`/`video`/`audio` field may hold — an inline metadata blob
9+
(`{url, name, size, …}`) or a bare URL string — into the reference form: an
10+
opaque `sys_file` id, owned by the record's field.
11+
12+
What it will and will not convert:
13+
14+
- **A URL naming this platform's own resolver** (`…/storage/files/:id`) already
15+
identifies a `sys_file`; the field is rewritten to the bare id and no bytes
16+
move.
17+
- **A `data:` URI** carries its bytes inline; they are uploaded, a `sys_file` is
18+
registered, and the field is rewritten to its id.
19+
- **An external URL** is reported, never converted. Re-hosting third-party
20+
content is a bandwidth, licensing and privacy decision that is not a
21+
migration's to make — ADR-0104 R7 retires these toward an explicit `url`
22+
field, which under AI authoring is the point: it stops "managed file" and
23+
"external link" being the same declaration.
24+
25+
**Dry run by default** — nothing is written unless `apply` is set, and the
26+
dry-run report has the same shape as the applied one so the plan can be reviewed
27+
and diffed. **Idempotent** — a value already in reference form is recorded and
28+
left alone, so a partially-completed run is safe to repeat.
29+
30+
The backfill never writes the ownership columns itself: it rewrites the record,
31+
and the claim hooks observe that write and record ownership. One claiming path,
32+
so there is nothing that can disagree with itself. Run
33+
`verifyFileReferences()` afterwards to confirm the two agree — that
34+
reconciliation is the gate the irreversible collection change must pass.

docs/adr/0104-field-runtime-value-shape-contract.md

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -567,24 +567,42 @@ sequences as:
567567
actually holds an id, so it lands safely ahead of the cutover.
568568
2. **Governed download** — read authorisation derived from the one owning
569569
record; anonymous capability URL demoted to opt-in `acl: 'public_read'`.
570-
3. **Write cutover** (protocol major) — stored form narrows to an id;
570+
3. **Backfill** — converts legacy inline blobs and own-resolver URLs to
571+
`sys_file` references, which the claim hooks then own; external URLs are
572+
reported, never re-hosted. Dry-run by default, idempotent.
573+
4. **Write cutover** (protocol major) — stored form narrows to an id;
571574
`accept` / `maxSize` enforced. Still deletes nothing.
572-
4. **Backfill**`os migrate` converts legacy inline blobs to `sys_file` rows
573-
and claims them; external URLs reported, not converted.
574-
5. **Reconciliation soak**`os storage verify-references` compares what
575-
records actually hold against recorded ownership, reporting *over-claim*
576-
(safe: file retained longer than needed), *under-claim* (**blocking**: a held
577-
file with no owner would be treated as free), and unclaimed orphans.
575+
5. **Reconciliation soak**`verify-references` compares what records actually
576+
hold against recorded ownership, and splits disagreements by whether they can
577+
cause data loss: *blocking* (a held file nothing owns; a file held by a slot
578+
that does not own it; one file held by two slots) versus *advisory* (owned
579+
but no longer held — fails toward retention; unreferenced committed files —
580+
storage cost only).
578581
6. **GC enable***gated, irreversible*. Relaxing the `scope === 'attachments'`
579582
tombstone guardrail and extending the `sys_file` reap guard's sweep-time
580583
re-verify to the ownership columns **must ship in the same change**. Half of
581584
it is worse than none: tombstoning released files while the guard still
582585
re-verifies only `sys_attachment` — always empty for a field file — turns
583586
every release into a guaranteed byte delete rather than a risky one.
584587

588+
Backfill precedes the cutover for the same reason it precedes collection, one
589+
step further back: narrowing the accepted stored form while records still hold
590+
legacy values would reject any update that rewrites such a field, breaking
591+
working apps until the backfill catches up. Backfilling first leaves the cutover
592+
nothing legacy to reject.
593+
585594
R4's acceptance gate is now executable rather than aspirational: step 6 may not
586-
merge until step 5 reports **zero under-claims for ≥7 consecutive days** on real
587-
tenant data. R5 (public-posture inventory) and R6 (sub-key read scan) stand.
595+
merge until step 5 reports **zero blocking discrepancies for ≥7 consecutive
596+
days** on real tenant data. R5 (public-posture inventory) and R6 (sub-key read
597+
scan) stand.
598+
599+
Steps 4 and 6 are the two that can break something. Step 4 is a declared
600+
protocol major and breaks *loudly* — a rejected write, recoverable. Step 6
601+
breaks *silently and permanently* — deleted bytes. They are therefore held to
602+
different standards: step 4 rides the planned v17 window paired with the client
603+
adoption that consumes the new form, while step 6 additionally requires the
604+
reconciliation evidence above, and neither the enable nor the migration run
605+
should be performed without an explicit human decision.
588606

589607
### Why this stays inside ADR-0104 rather than a new ADR
590608

Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,299 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect, vi } from 'vitest';
4+
import {
5+
backfillFileReferences,
6+
formatBackfillReport,
7+
type BackfillEngine,
8+
} from './backfill-file-references.js';
9+
10+
const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() });
11+
12+
const REGISTRY: Record<string, any> = {
13+
sys_file: { fields: { id: { type: 'text' } } },
14+
product: {
15+
fields: {
16+
id: { type: 'text' },
17+
name: { type: 'text' },
18+
image: { type: 'image' },
19+
gallery: { type: 'image', multiple: true },
20+
},
21+
},
22+
tag: { fields: { id: { type: 'text' }, label: { type: 'text' } } },
23+
};
24+
25+
function fakeEngine(tables: Record<string, Array<Record<string, unknown>>>) {
26+
const calls: Array<{ op: string; object: string; arg: any }> = [];
27+
const engine: BackfillEngine & { tables: typeof tables; calls: typeof calls } = {
28+
getObject: (name) => REGISTRY[name],
29+
getConfigs: () => REGISTRY,
30+
async find(object, options: any) {
31+
const rows = tables[object] ?? [];
32+
const start = typeof options?.offset === 'number' ? options.offset : 0;
33+
const end = typeof options?.limit === 'number' ? start + options.limit : undefined;
34+
return rows.slice(start, end);
35+
},
36+
async insert(object, data: any) {
37+
calls.push({ op: 'insert', object, arg: data });
38+
(tables[object] ??= []).push({ ...data });
39+
return data;
40+
},
41+
async update(object, data: any) {
42+
calls.push({ op: 'update', object, arg: data });
43+
const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id));
44+
if (row) Object.assign(row, data);
45+
return row;
46+
},
47+
tables,
48+
calls,
49+
};
50+
return engine;
51+
}
52+
53+
const fakeStorage = () =>
54+
({
55+
upload: vi.fn(async () => {}),
56+
download: vi.fn(async () => Buffer.from('x')),
57+
delete: vi.fn(async () => {}),
58+
exists: vi.fn(async () => true),
59+
getInfo: vi.fn(async () => ({ key: 'k', size: 1, contentType: 'text/plain', lastModified: new Date() })),
60+
}) as any;
61+
62+
const run = (engine: any, opts: any = {}, storage: any = fakeStorage()) =>
63+
backfillFileReferences(engine, () => storage, silentLogger(), opts);
64+
65+
describe('backfillFileReferences (ADR-0104 D3 wave 2)', () => {
66+
it('rewrites a URL that already names a sys_file to the bare id, moving no bytes', async () => {
67+
const engine = fakeEngine({
68+
product: [{ id: 'p1', image: 'https://app.example.com/api/v1/storage/files/file_a' }],
69+
sys_file: [],
70+
});
71+
const storage = fakeStorage();
72+
73+
const report = await run(engine, { apply: true }, storage);
74+
75+
expect(engine.tables.product[0].image).toBe('file_a');
76+
expect(report.converted).toBe(1);
77+
expect(storage.upload).not.toHaveBeenCalled();
78+
expect(report.actions[0].kind).toBe('resolved_local_url');
79+
});
80+
81+
it('resolves the same URL when it is wrapped in a legacy inline blob', async () => {
82+
const engine = fakeEngine({
83+
product: [
84+
{ id: 'p1', image: { url: '/api/v1/storage/files/file_b', name: 'b.png', size: 12 } },
85+
],
86+
sys_file: [],
87+
});
88+
89+
await run(engine, { apply: true });
90+
91+
expect(engine.tables.product[0].image).toBe('file_b');
92+
});
93+
94+
it('uploads inline data: bytes and rewrites the field to the new id', async () => {
95+
const engine = fakeEngine({
96+
product: [{ id: 'p1', image: { url: 'data:image/png;base64,aGVsbG8=', name: 'hi.png' } }],
97+
sys_file: [],
98+
});
99+
const storage = fakeStorage();
100+
101+
const report = await run(engine, { apply: true }, storage);
102+
103+
expect(storage.upload).toHaveBeenCalledTimes(1);
104+
const created = engine.tables.sys_file[0];
105+
expect(created).toMatchObject({ name: 'hi.png', mime_type: 'image/png', status: 'committed' });
106+
expect(created.size).toBe(5); // "hello"
107+
expect(engine.tables.product[0].image).toBe(created.id);
108+
expect(report.converted).toBe(1);
109+
});
110+
111+
/**
112+
* Converting an external URL would mean fetching third-party content and
113+
* silently re-hosting it — a bandwidth/licensing/privacy decision that is not
114+
* a migration's to make.
115+
*/
116+
it('reports an external URL and never re-hosts it', async () => {
117+
const engine = fakeEngine({
118+
product: [{ id: 'p1', image: 'https://cdn.example.com/logo.png' }],
119+
sys_file: [],
120+
});
121+
const storage = fakeStorage();
122+
123+
const report = await run(engine, { apply: true }, storage);
124+
125+
expect(engine.tables.product[0].image).toBe('https://cdn.example.com/logo.png');
126+
expect(storage.upload).not.toHaveBeenCalled();
127+
expect(report.externalUrls).toBe(1);
128+
expect(report.converted).toBe(0);
129+
expect(formatBackfillReport(report)).toContain('NOT re-hosted');
130+
});
131+
132+
it('handles a multiple:true field element-wise', async () => {
133+
const engine = fakeEngine({
134+
product: [
135+
{
136+
id: 'p1',
137+
gallery: [
138+
'/api/v1/storage/files/file_a',
139+
'https://cdn.example.com/x.png',
140+
'file_already',
141+
],
142+
},
143+
],
144+
sys_file: [],
145+
});
146+
147+
const report = await run(engine, { apply: true });
148+
149+
expect(engine.tables.product[0].gallery).toEqual([
150+
'file_a',
151+
'https://cdn.example.com/x.png',
152+
'file_already',
153+
]);
154+
expect(report.converted).toBe(1);
155+
expect(report.externalUrls).toBe(1);
156+
expect(report.alreadyReferences).toBe(1);
157+
});
158+
159+
// ── Dry run ───────────────────────────────────────────────────────
160+
it('writes nothing on a dry run but reports the same plan', async () => {
161+
const engine = fakeEngine({
162+
product: [
163+
{ id: 'p1', image: '/api/v1/storage/files/file_a' },
164+
{ id: 'p2', image: 'data:text/plain;base64,aGk=' },
165+
],
166+
sys_file: [],
167+
});
168+
const storage = fakeStorage();
169+
170+
const report = await run(engine, {}, storage);
171+
172+
expect(report.applied).toBe(false);
173+
expect(report.converted).toBe(2);
174+
expect(engine.calls).toHaveLength(0);
175+
expect(engine.tables.product[0].image).toBe('/api/v1/storage/files/file_a');
176+
expect(storage.upload).not.toHaveBeenCalled();
177+
expect(formatBackfillReport(report)).toContain('DRY RUN');
178+
});
179+
180+
// ── Idempotence / safety ──────────────────────────────────────────
181+
it('is idempotent — a second run converts nothing', async () => {
182+
const engine = fakeEngine({
183+
product: [{ id: 'p1', image: '/api/v1/storage/files/file_a' }],
184+
sys_file: [],
185+
});
186+
187+
await run(engine, { apply: true });
188+
const second = await run(engine, { apply: true });
189+
190+
expect(second.converted).toBe(0);
191+
expect(second.alreadyReferences).toBe(1);
192+
});
193+
194+
it('never writes the ref_* columns itself — claiming stays on one path', async () => {
195+
const engine = fakeEngine({
196+
product: [{ id: 'p1', image: '/api/v1/storage/files/file_a' }],
197+
sys_file: [],
198+
});
199+
200+
await run(engine, { apply: true });
201+
202+
// The record write is what the claim hooks observe; the backfill must not
203+
// record ownership behind their back, or there would be two claiming paths
204+
// that could disagree.
205+
for (const c of engine.calls) {
206+
expect(c.arg).not.toHaveProperty('ref_object');
207+
expect(c.arg).not.toHaveProperty('ref_id');
208+
expect(c.arg).not.toHaveProperty('ref_field');
209+
}
210+
expect(engine.calls.filter((c) => c.object === 'sys_file')).toHaveLength(0);
211+
});
212+
213+
it('leaves data: values alone when no storage service is available', async () => {
214+
const engine = fakeEngine({
215+
product: [{ id: 'p1', image: 'data:text/plain;base64,aGk=' }],
216+
sys_file: [],
217+
});
218+
219+
const report = await backfillFileReferences(engine, () => null, silentLogger(), { apply: true });
220+
221+
expect(report.unresolvable).toBe(1);
222+
expect(report.converted).toBe(0);
223+
expect(engine.tables.product[0].image).toBe('data:text/plain;base64,aGk=');
224+
});
225+
226+
it('skips objects with no file-class fields', async () => {
227+
const engine = fakeEngine({ tag: [{ id: 't1', label: 'x' }], product: [], sys_file: [] });
228+
229+
const report = await run(engine, { apply: true });
230+
231+
expect(report.scannedObjects).toEqual(['product']);
232+
});
233+
234+
/**
235+
* The values scanned here come straight out of tenant records, so URL
236+
* matching must not be able to backtrack. A pattern anchored only at the tail
237+
* retries from every start position, which a value repeating the matched
238+
* prefix turns into a polynomial blowup (CodeQL js/polynomial-redos).
239+
*/
240+
it('matches resolver URLs in linear time on an adversarial value', async () => {
241+
// Many repetitions of the matched prefix, ending in a segment that is not
242+
// an id token — the shape that makes a tail-anchored pattern retry from
243+
// every start position.
244+
const hostile = '/storage/files/'.repeat(6000) + '!';
245+
const engine = fakeEngine({ product: [{ id: 'p1', image: hostile }], sys_file: [] });
246+
247+
const started = process.hrtime.bigint();
248+
const report = await run(engine, {});
249+
const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6;
250+
251+
// Not a reference (the trailing segment is not an id token) — and reaching
252+
// that verdict must not depend on how many times the prefix repeats.
253+
expect(report.converted).toBe(0);
254+
expect(elapsedMs).toBeLessThan(1000);
255+
});
256+
257+
it('accepts the resolver URL shapes and rejects near-misses', async () => {
258+
const engine = fakeEngine({
259+
product: [
260+
{ id: 'p1', image: 'https://app.example.com/api/v1/storage/files/aaa' },
261+
{ id: 'p2', image: '/api/v1/storage/files/bbb/url' },
262+
{ id: 'p3', image: '/api/v1/storage/files/ccc?v=2' },
263+
{ id: 'p4', image: '/api/v1/storage/files/ddd/' },
264+
{ id: 'p5', image: 'https://cdn.example.com/files/eee' }, // no /storage
265+
{ id: 'p6', image: '/api/v1/storage/files/' }, // no id
266+
],
267+
sys_file: [],
268+
});
269+
270+
await run(engine, { apply: true });
271+
272+
expect(engine.tables.product.map((r) => r.image)).toEqual([
273+
'aaa',
274+
'bbb',
275+
'ccc',
276+
'ddd',
277+
'https://cdn.example.com/files/eee',
278+
'/api/v1/storage/files/',
279+
]);
280+
});
281+
282+
it('pages through large objects and flags truncation at the bound', async () => {
283+
const many = Array.from({ length: 500 }, (_, i) => ({
284+
id: `p${i}`,
285+
image: `/api/v1/storage/files/file_${i}`,
286+
}));
287+
const engine = fakeEngine({ product: many, sys_file: [] });
288+
289+
const full = await run(engine, {});
290+
expect(full.scannedRecords).toBe(500);
291+
expect(full.truncated).toBe(false);
292+
293+
const bounded = await run(fakeEngine({ product: many, sys_file: [] }), {
294+
maxRecordsPerObject: 200,
295+
});
296+
expect(bounded.truncated).toBe(true);
297+
expect(formatBackfillReport(bounded)).toContain('truncated');
298+
});
299+
});

0 commit comments

Comments
 (0)