Screenshot serving internals: MediaCache streaming, realm-read auth, cache headers - #5840
Conversation
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review focused on the two contracts this PR actually stakes its correctness on — the response bytes that reach the wire through the realm-server's Koa bridge, and the realm-read auth the route claims to inherit — since the addressing/resolver logic is deliberately a no-op seam here and gets its real test when the resolver lands.
Bottom line: no blocking issues. The serving core, cache-header policy, content-hash validators, GC-join widening, and the honest all-miss framing all check out. Two items are latent — both harmless in today's all-miss state, both primed to bite the moment the resolver PR turns the hit path on — and want a decision now rather than a surprise then.
Verifications that came back clean and aren't worth a thread: the public/private derivation matches serveLocalFile exactly; ifNoneMatchMatches handles *, lists, and W/ on both sides; the 304-before-object-existence ordering is correct (a matching validator means the client's cached bytes are still valid even if the object was reclaimed); the GC tombstone join's (i.url = r.source_url OR i.file_alias = r.source_url) widening matches the same idiom getInstance already uses and can't cross-match instances within a realm; and the _screenshot/ dispatch sits correctly after both checkPermission and the archived-realm seal.
Recommendations:
- The buffered async-iterable branch in
serveMediaCacheEntryreturns aUint8Arraybody that the Koa bridge drains throughwebStreamToText, corrupting image bytes — dead with today's Readable-returning adapters, but a trap the "safe because screenshot-sized" comment misdirects away from. Route it throughnodeStreamlike the streaming branch. (thread onmedia-cache-serving.ts) HEADis exempt fromcheckPermission, so unauthenticatedHEAD /_screenshot/on a private realm isn't a 401 — a no-op today, an existence/size/content-hash oracle once the resolver returns200s. Decide intended-or-not and pin it with aHEADtest before the hit path lights up. (thread on the dispatch inrealm.ts)
Adjacent, out of scope: mediaCacheVisibility is a third copy of the permissions['*']?.includes('read') ? 'public' : 'private' rule (serveLocalFile inlines its own); whoever consolidates the visibility derivation could fold serveLocalFile onto the shared helper. Not this PR.
| // A bare async iterable (the interface's minimum) is buffered whole. Safe | ||
| // because captures are screenshot-sized, and the entry carries the exact | ||
| // size; an adapter serving anything large should return a node Readable. |
There was a problem hiding this comment.
[Claude Code 🤖] The buffered-iterable branch returns a body shape the Koa bridge corrupts — the "safe" comment reasons about size, not the hazard the streaming branch just warned about. This path builds a Uint8Array body and hands it to createResponse, which does new Response(body, …). When the realm-server middleware unwraps that response (packages/realm-server/middleware/index.ts, the handlerResponseToContext block), it takes exactly the branch the streaming path above documents as corrupting: a native Response with a Uint8Array body exposes response.body as a web ReadableStream, so it misses if (nodeStream) and falls into else if (body instanceof ReadableStream) { ctxt.body = await webStreamToText(body) } — a UTF-8 decode over the image bytes. Same failure mode the isNodeReadable branch exists to avoid; this branch just re-introduces it for the non-Readable stream shape.
Verified. createResponse (create-response.ts:14) returns new Response(body, …); middleware index.ts:488-501 streams nodeStream verbatim and drains any other body (including the ReadableStream that a Uint8Array body becomes) through webStreamToText. The two production adapters both return node Readables (media-cache/s3-adapter.ts:105 — response.Body as Readable; media-cache/local-disk-adapter.ts:67 — createReadStream), so this branch is dead in production and the direct unit test (media-cache-serving-test.ts, streamShape='iterable') reads response.arrayBuffer() without going through the middleware — which is why it stays green while the real serving path would corrupt.
Scope: regression, non-blocking (unreachable with today's adapters). But it's a latent trap primed to fire the day any adapter returns a bare async iterable, and the comment's "Safe because captures are screenshot-sized" points at memory cost, not the body-shape mismatch that actually bites.
Way out: buffer, then wrap and ride nodeStream like the streaming branch, so both stream shapes leave through the one path the bridge streams verbatim:
let body = new Uint8Array(entry.sizeBytes);
let offset = 0;
for (let chunk of chunks) {
body.set(chunk, offset);
offset += chunk.length;
}
let response: ResponseWithNodeStream = createResponse({ body: null, init, requestContext });
response.nodeStream = Readable.from(Buffer.from(body));
return response;At minimum, correct the comment so it names the real constraint (must exit via nodeStream, not "small enough to buffer") — otherwise the next adapter author reads this as blessed.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in dde68a64e1 — the buffered branch is gone entirely: both interface-legal stream shapes now exit via nodeStream (an adapter's node Readable passes through; a bare async iterable is wrapped by a #media-cache-stream node/browser conditional import, since Readable.from can't live in browser-safe code). The comment now names the real constraint — the bridge streams only nodeStream verbatim — and the iterable-shape test asserts the bytes ride nodeStream rather than a Response body.
| // type. Placed after checkPermission so the route inherits realm-read | ||
| // auth exactly like any realm resource. | ||
| if ( | ||
| (request.method === 'GET' || request.method === 'HEAD') && |
There was a problem hiding this comment.
[Claude Code 🤖] Admitting HEAD here means the route does not uniformly inherit realm-read auth — checkPermission waves HEAD through without a JWT, so the PR description's "a private realm 401s without a JWT" holds for GET but not HEAD. checkPermission (realm.ts:4237-4248) returns early — no token required — when requiredPermission !== 'realm-owner' and request.method === 'HEAD'. A screenshot request is read, so an unauthenticated HEAD /_screenshot/{card} on a private realm sails past auth and into serveScreenshot.
Why it's latent, not live today. resolveScreenshotEntry always returns undefined, so every request — GET or HEAD — resolves to mediaCacheMissResponse (404), and even the instance() existence check can't be distinguished (both outcomes 404). No leak yet.
Why it's load-bearing. The moment the resolver lands (the next PR in this stack), serveMediaCacheEntry's HEAD branch returns 200 with content-length = the capture's size and etag = the object key, i.e. the content hash. That turns an unauthenticated HEAD into an existence + size + content-hash oracle over a private realm's captures, and the instance() gate turns into an unauthenticated instance-enumeration probe (200 vs 404 per card id). The HEAD auth exemption is a pre-existing, realm-wide checkPermission behavior — not introduced here — but this route is the first to pair it with a per-object content-hash validator and an existence signal, so it's this stack that makes it matter.
Scope: pre-existing, now load-bearing. Non-blocking for this PR (all-miss today), but it needs a decision before the resolver ships:
- If unauthenticated
HEADon a private realm is not intended: re-verifyreadinsideserveScreenshotforHEAD(or dropHEADfrom the dispatch and let the miss/hit path answer only authenticatedGETs), and pin it with aHEAD-on-private-realm test — the currentscreenshot-test.tsonly exercisesGETfor the 401/403 cases. - If it is intended: scope the description's auth claim to
GET, and add theHEADcase to the test so the exemption is asserted rather than incidental.
Either way the ask is: which is it, and pin the answer with a test before the hit path can return 200.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in dde68a64e1 — unauthenticated HEAD is not intended: the dispatch is GET-only now (no consumer of the route sends HEAD — image loads and crawlers GET), serveMediaCacheEntry's HEAD branch is removed as dead, and a private-realm test pins that HEAD never receives the route's response shape. The resolver-bearing PR stacked on this keeps the same posture.
Host Test Results 1 files ±0 1 suites ±0 2h 38m 38s ⏱️ + 3m 37s Results for commit 55f424a. ± Comparison against earlier commit fc79ba3. Realm Server Test Results 1 files ±0 1 suites ±0 14m 11s ⏱️ - 1m 33s Results for commit 55f424a. ± Comparison against earlier commit fc79ba3. For more details on these errors, see this check. |
dde68a6 to
fc79ba3
Compare
…th, cache headers
The `_screenshot/{instanceLocalPath}` realm route and the serving core it
delegates to. Dispatch is by path prefix inside internalHandle (the router
table keys on Accept, which image loads can't match), placed after
checkPermission so the route inherits realm-read auth — public realms serve
unauthenticated, private realms 401. The serving core streams a resolved
ledger entry via nodeStream with Content-Type from the ledger, ETag = the
content hash, short max-age + stale-while-revalidate scoped public/private
by realm readability, RFC-9110 If-None-Match 304s, and a last-accessed bump
that feeds the GC's on-demand age-out lane.
Addressing resolvers (declared-name manifests, capture-spec
canonicalization) plug into a resolution seam that today yields no entry,
so every request serves as an uncaptured miss: 404 with a short max-age.
The MediaCacheAdapter is threaded from env into every realm the server
mounts.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enshot dispatch Every media body now leaves through nodeStream — the one shape the Koa bridge streams verbatim (anything else drains through text and corrupts binary). A bare async-iterable adapter stream is wrapped via a node/browser conditional import instead of being buffered into a Response body the bridge would mangle. The _screenshot dispatch admits GET only: checkPermission exempts HEAD from auth realm-wide, so answering HEAD would give unauthenticated callers an existence/size/content-hash oracle over a private realm's captures once hits exist. No consumer of the route sends HEAD. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fc79ba3 to
55f424a
Compare
Stacked on #5838 (the MediaCache store); this PR adds the serving side that streams from it.
What this adds
The
_screenshot/{instanceLocalPath}realm route. Dispatch is by path prefix insideinternalHandlerather than the router table: the router keys routes on theAcceptheader, and the requests this route exists to serve —<img>loads, og:image fetches — sendimage/*-shaped Accept values that match no supported mime type. The branch sits aftercheckPermission, so the route inherits realm-read auth exactly like any realm resource: a world-readable realm serves unauthenticated (crawlers, og:image), a private realm 401s without a JWT and 403s without the read grant. Beyond realm read, the parent instance must be live in the index — per-instance ACLs get a place to land, and captures of a deleted instance stop serving as soon as its tombstone appears, ahead of GC reclaiming them.The serving core (
packages/runtime-common/media-cache-serving.ts, browser-safe):nodeStream(the Koa bridge drains any other body shape through text, which corrupts image bytes); a bare async-iterable adapter stream is buffered whole, sized by the ledger row.Content-Typecomes from the ledger (the adapters store no metadata);ETagis the entry's object key — the content hash — so revalidation is exact and a re-capture rotates the validator while the URL stays put.Cache-Controlispublic/privateby realm world-readability (same derivation asserveLocalFile),max-age=60, stale-while-revalidate=3600;If-None-Matchmatches (via the RFC-9110 helper:*, lists,W/) answer as bodyless 304s.last_accessed_at(best-effort), which is what keeps an in-use on-demand capture out of the GC's age-out lane. An entry whose object was reclaimed between ledger read and stream open serves as a miss, not an error.<img>pointing at a not-yet-captured name picks the image up on a later revalidation.Addressing is a seam, not yet a resolver.
name=(declared-screenshot manifests) and capture-spec canonicalization plug intoresolveScreenshotEntry; with neither resolver present, every request is an uncaptured miss.findMediaCacheEntry(exact-generation or newest-generation ledger lookup) is the query those resolvers will use. The ledger'ssource_urlconvention is pinned to the extensionless card-id form, and the GC tombstone join now matches eitherboxel_index.urlorfile_aliasso both URL shapes reclaim correctly.Threading:
MediaCacheAdapteris now a Realm constructor option, wired fromMEDIA_CACHE_BUCKET/MEDIA_CACHE_DIRonce per server and shared by every realm it mounts; without one, the route serves every request as a miss.Test plan
media-cache-serving-test(real Postgres): hit streaming with content-hash validators, public/private cache-control, 304 across allIf-None-Matchshapes, HEAD, the buffered-iterable path, reclaimed-object miss, and last-accessed bumps on 200/304/HEAD.realm-endpoints/screenshot-test(booted realm server, supertest): private realm 401/403/404-with-private-cache-control,name=miss, public realm unauthenticated miss with public cache-control.🤖 Generated with Claude Code