MediaCache: content-addressed media store with ledger and GC sweep - #5838
Conversation
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 32m 45s ⏱️ + 2h 25m 5s Results for commit 23be42f. ± Comparison against earlier commit 028b89f. Realm Server Test Results 1 files ±0 1 suites ±0 17m 49s ⏱️ + 2m 58s Results for commit 23be42f. ± Comparison against earlier commit 028b89f. |
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Lens: This reviews the GC's central safety claim — that objects and their ledger rows can never drift apart — against the store/ledger code paths, since that invariant is the whole reason for a ledger-only (never bucket-listing) catalog. The adapter interface, dedupe-on-write, S3/local-disk parity, migration placement (additive → migrations/, down() drops the table), the coalesce decision, and the cron wiring all check out and faithfully follow their prerender-html-reconcile siblings.
Bottom line: no blocking issues — nothing writes to the store in this PR, so both findings below are latent. But two claims the code makes about itself don't hold, and both go live the moment the capture/serve integration lands: the GC job's stated priority, and the promise that a repointed-away object gets reclaimed. Neither needs to block this foundation PR; both should be resolved (or the claims softened) before capture is wired.
- GC is enqueued at
systemInitiatedPriority(1), co-equal with indexing, though its comment says "background tier (priority 0)" — see the thread onenqueueMediaCacheGcinscripts/media-cache-gc.ts. putMedia's upsert repoints a row'sobject_key, orphaning the prior object beyond GC's reach — see the thread onputMediainruntime-common/media-cache.ts.
Adjacent, out of scope — GC vs. a concurrent dedupe-put. Once capture is wired, putMedia → adapter.put skips the upload when head finds the key present (dedupe-on-write), and the sweep deletes objects and then rows in separate, non-transactional steps. A capture that produces bytes identical to an object the in-flight sweep has already selected for deletion will see it still present, skip the upload, and record a ledger row pointing at bytes the sweep then deletes — a row referencing a missing object. The 24h MEDIA_CACHE_GC_MIN_AGE_MS guards in-flight serves, but not concurrent puts. Worth a strategy (re-check references after the object delete inside a txn, or delete rows-then-objects with a re-reference check) when the capture path lands — explicitly not this PR.
| // sweep runs at the background tier (priority 0) so it never competes with | ||
| // indexing or user work. | ||
| export async function enqueueMediaCacheGc({ | ||
| priority = systemInitiatedPriority, |
There was a problem hiding this comment.
[Claude Code 🤖] GC is enqueued co-equal with indexing, not below it — the comment says priority 0 but the code passes priority 1.
enqueueMediaCacheGc defaults priority = systemInitiatedPriority, and in runtime-common/queue.ts that constant is 1 — the same tier as system-initiated indexing (realm-index-updater.ts publishes fullIndex/incremental at systemInitiatedPriority). The comment two lines up — "the sweep runs at the background tier (priority 0) so it never competes with indexing or user work" — describes systemInitiatedPrerenderHtmlPriority, which is 0, the actual all-priority-pool floor. So as written the sweep runs co-equal with indexing rather than beneath it, and the "priority 0" in the comment names the wrong constant.
The sibling reconcile job, which plays the same daily-background role, enqueues at systemInitiatedPrerenderHtmlPriority (see scripts/prerender-html-reconcile.ts). Matching it makes the code do what the comment claims — change the default here to systemInitiatedPrerenderHtmlPriority and update the import on line 3. Alternatively, if priority 1 is deliberate, correct the comment.
Class: regression (new code), non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 028b89f585 — the default is now systemInitiatedPrerenderHtmlPriority (0), matching the reconcile sibling, and the comment names the tier it actually runs at.
| // bytes → same key). The ledger write is an upsert on the capture identity: | ||
| // a re-capture that produced different bytes repoints the row at the new | ||
| // object, and the old object is reclaimed by the GC sweep once nothing else | ||
| // references it. |
There was a problem hiding this comment.
[Claude Code 🤖] This comment says the repointed-away object "is reclaimed by the GC sweep," but the sweep structurally cannot see it — those bytes leak.
putMedia upserts on media_cache_ledger_pkey = (realm_url, source_url, capture_spec_hash, source_generation). When a re-capture at that same identity produces different bytes, the upsert repoints object_key to the new hash — the media-cache-gc-test case "a re-capture upserts its row, repointing at the new bytes" pins exactly this, and asserts first.objectKey !== second.objectKey with the single row now pointing at the second. The previous object_key is now named by zero ledger rows.
The GC sweep (tasks/media-cache-gc.ts → findMediaCacheGcCandidates / findMediaCacheKeyReferenceCounts) only ever reclaims an object that a ledger row it is deleting still points at; it never enumerates the bucket (by design — the ledger is the sole catalog). An object named by no ledger row is therefore invisible to GC forever, so a repoint-away leaks its old bytes permanently. That contradicts this comment's "the old object is reclaimed by the GC sweep once nothing else references it," and the PR description's "never bytes the ledger no longer knows about."
Two ways out, both fine as follow-up once the capture path lands (nothing writes to the store in this PR, so this is latent, not a live leak):
- have
putMediaread the prior row'sobject_keyduring the upsert and, if it changed and no other row references it, delete the old object — with the caveat that this races a concurrent dedupe-put (see the review body); or - accept repoint-away leakage until a bucket-level reconciliation exists, and correct this comment to say so.
Which is intended? As written the comment overclaims what GC can do. Class: design gap surfaced by this PR's completeness claim, non-blocking / follow-up.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 028b89f585 — putMedia now reclaims inline: it reads the prior row during the upsert and, when the object key changed and no other ledger row references the old key, deletes the old object best-effort (refcount-guarded, so a dedupe-shared object survives). A failed or crash-interrupted delete leaks at most that one object and is logged; the doc comment now states exactly that instead of claiming the sweep covers it. Pinned by three new putMedia tests: repoint reclaims, shared-key repoint keeps the object, failed reclaim never fails the put.
The storage foundation for derived media (screenshots): objects keyed by the sha256 of their output bytes behind a MediaCacheAdapter interface (S3 for deployed environments, local disk for dev/tests), a media_cache_ledger table as the store's only catalog, and a reconcile-style media-cache-gc queue job (cron-enqueued, coalesced) that reclaims superseded generations, captures of tombstoned instances, and idle on-demand captures — deleting objects before their ledger rows so a crashed sweep leaves retryable rows, never untracked bytes. Serving and capture are deliberately not wired up here; nothing writes to the store in production yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ted objects The GC enqueue now uses systemInitiatedPrerenderHtmlPriority (0, the all-priority pool's floor) as its comment always claimed, matching the prerender-html reconcile sibling instead of running co-equal with indexing. putMedia now reclaims the object a repointing upsert strips of its last ledger reference: the sweep can never see an unreferenced object (the ledger is its only catalog), so without this a same-identity re-capture that changed bytes leaked the old object permanently. Reclaim is inline, refcount-guarded, and best-effort — a failed delete is logged as a leak and never fails the put. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
028b89f to
23be42f
Compare
What this adds
The storage foundation for derived media (screenshots). Nothing writes to the store in production yet — serving and capture integration are separate work — so this ships the store, its catalog, and its garbage collector, fully unit-tested.
Adapter interface (
packages/runtime-common/media-cache.ts, browser-safe):put/head/getStream/deleteover objects keyed by the sha256 of their output bytes — a true content address, soputis dedupe-on-write (an existing key already holds the bytes; the upload is skipped). No storage-backend types leak above the interface. Two node-only implementations inpackages/realm-server/media-cache/:undefined/no-op. Credentials ride the ECS task role, mirroringprerender/artifact-sink.ts.A worker picks its adapter from
MEDIA_CACHE_BUCKET(S3, with optionalMEDIA_CACHE_KEY_PREFIX/MEDIA_CACHE_REGION) orMEDIA_CACHE_DIR(disk); with neither set the store is disabled and the GC task no-ops.Ledger (
media_cache_ledger, additive migration): one row per capture — (realm, source instance, canonical capture-spec hash, generation) → object key, plus lane, content type, size, created/last-accessed stamps. The ledger is the store's only catalog: GC reclaims by scanning rows, never by listing the bucket. Several rows may share one object (dedupe), so an object is reclaimable only when its last referencing row goes.GC sweep (
media-cache-gcqueue job, cron-enqueued daily, coalesced likeprerender-html-reconcile): reclaims rows that areboxel_indexrow is deleted;No row younger than min-age is ever collected. Objects are deleted before their rows: a sweep that dies mid-way leaves rows behind for the next sweep to re-find (adapter deletes are idempotent), never bytes the ledger no longer knows about. An object delete that fails keeps its rows as the retry path.
Test plan
media-cache-adapter-test: both adapters — round-trip, absence handling, dedupe-on-write proof, S3 command construction/error mapping via a stub client; the sha256 content address is pinned against a known digest.media-cache-gc-test(real Postgres viasetupDB): each reclaim lane, the min-age guards, shared-object retention, failed-delete retry, no-adapter no-op, plusputMediaupsert/repoint andtouchMediaCacheEntry.worker-job-registration-testpins the new job type;pnpm make-schemasnapshot regenerated; typechecks clean across runtime-common, postgres, realm-server, host, ai-bot, billing, bot-runner.🤖 Generated with Claude Code