Persist custom-captureSpec screenshots under the full capture identity - #5893
Conversation
Extend the canonical capture identity (canonicalCaptureSpecString / captureSpecHash / canonicalCaptureSpecQuery) to viewport, deviceScaleFactor, fullPage, and clip, and unreserve those params in the GET _screenshot/ URL DSL (viewport=1280x800, dsf=2, fullPage=true, clip=0,0,400x300) with the same bounds validation and wording as the POST body. A custom capture now persists to the MediaCache under its spec hash and serves on its own durable GET URL: the POST handler's canonical-only ledger gate is removed, the worker task's persist refusal for override-carrying jobs is removed, and same-spec jobs coalesce via the persist identity. This also makes the POST timeout path's Retry-After honest for custom specs: the abandoned job's bytes land in the ledger and the retry answers from it with no second render. Engine defaults (800x600 viewport, scale 1, fullPage false) are pinned in runtime-common and elided during canonicalization, so every spelling of one capture geometry is one cache key; the prerender pool's restore viewport now shares that constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Host Test Results 1 files ± 0 1 suites ±0 2h 26m 59s ⏱️ - 13m 4s Results for commit 0e6eda2. ± Comparison against earlier commit a76f71c. Realm Server Test Results 1 files ± 0 1 suites ±0 17m 55s ⏱️ + 4m 35s Results for commit 0e6eda2. ± Comparison against earlier commit a76f71c. |
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the one thing the change rests on — the cache-key identity: that equal capture geometries canonicalize to a single ledger key and distinct ones never collide, that the GET DSL and the POST body resolve to the same identity, that the persisted key and the served _screenshot/ URL round-trip, and that dropping the canonical-only gates left nothing persisting or coalescing under the wrong identity. It did not re-cover the unchanged singular capture-path rendering or the stacked base branch.
No blocking issues. The custom-spec identity is consistent end to end — the ledger key, the served URL, and the coalesce predicate all derive from one full-spec hash — and equal geometries collapse to one canonical form across both surfaces, including the numeric-spelling and scientific-notation clip-offset cases the round-trip depends on. The realm-read check is preserved on the now-universal ledger fast path.
…creenshots-extend-the-canonical
Ports the full-capture-identity work onto the batch/envelope capture engine
that landed on main:
- parseScreenshotCaptureSpec keeps main's (raw, format) batch/envelope shape;
the GET DSL threads its format through and elideDefaults now also drops an
explicit DEFAULT_CAPTURE_VIEWPORT-sized viewport, so equal capture intents
hash identically on both surfaces, batch entries included.
- The handler's ledger identity covers custom singular geometry (persist +
serve under the full spec hash) while batches and fitted-format captures
stay capture-only: a batch has no single-capture identity, and fitted sits
outside the canonical (ledger/GET-DSL) serving contract.
- captures[].deviceScaleFactor prefers the engine-reported factor and falls
back to the spec's declared override on ledger serves.
- The clip-containment test expectation follows the batch-aware validator
wording ('exceeds the viewport width').
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
richardhjtan
left a comment
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
This review went after the one thing the change rests on — that the ledger key is a total function of capture intent: which fields the canonical form covers, whether the emitted URL round-trips back to the same identity, and what the deleted canonical-only gates were holding up. It did not re-review the batch or fitted capture paths, or the stacked base branch.
No blocking issues. The identity is correct for everything currently reachable; the asks are about the two fields it silently omits, and the producer check that was deleted rather than upgraded.
envelopeandcapturesare dropped from the canonical form and from the coalesce predicate, and only call-site gates keep that from mattering — threads oncanonicalCaptureSpecStringandsameCaptureSpec.- The worker's persist-identity-versus-rendered-spec check is gone; the new hash makes restoring it a one-liner — thread on the task.
- The served URL percent-encodes the clip commas that its own grammar documents unescaped — thread on
canonicalCaptureSpecQuery.
Red check: Boxel CLI Tests is failing on tests/integration/realm-ingest-card.test.ts and tests/integration/search.test.ts. The logs show ERR_CONNECTION_REFUSED against localhost:4201 during prerender standby warmup and a search failed: HTTP 404, which reads as the realm server never coming up rather than anything in this diff — but it needs a re-run or an explanation rather than being left red, since this PR does touch realm.ts.
| if (overrides.clip) { | ||
| canonical.clip = sortKeys(overrides.clip); | ||
| } | ||
| return JSON.stringify(sortKeys(canonical)); |
There was a problem hiding this comment.
[Claude Code 🤖] The canonical form now covers viewport / dsf / fullPage / clip, but CaptureSpec also admits envelope and captures, and both are dropped here without comment — same in canonicalCaptureSpecQuery.
Nothing reaches it today, and I traced both hash call sites to confirm that: the POST handler only builds a spec under isCaptureFormat(format) && !captureSpec?.captures, and the GET DSL's format roster excludes fitted while its grammar cannot express an envelope. The concern is where that totality lives — in two call sites, not in the type or in this function — while envelope / target params are named in the description as the next thing to land on this DSL. The day CaptureSpec widens, two captures differing only in envelope hash to one ledger key and the cache serves the wrong image, with nothing failing.
Cheapest way to make the omission unrepresentable rather than merely unreached:
export interface CaptureSpec
extends Omit<ScreenshotCaptureSpec, 'envelope' | 'captures'> {
format: CaptureFormat;
}Widening the identity then fails to compile here instead of colliding in the ledger at runtime.
Class: pre-existing shape, newly load-bearing — before this PR the canonical string only ever saw format. Non-blocking.
| return ( | ||
| (a.viewport?.width ?? null) === (b.viewport?.width ?? null) && | ||
| (a.viewport?.height ?? null) === (b.viewport?.height ?? null) && | ||
| (a.deviceScaleFactor ?? null) === (b.deviceScaleFactor ?? null) && | ||
| (a.fullPage ?? false) === (b.fullPage ?? false) && | ||
| (a.clip?.x ?? null) === (b.clip?.x ?? null) && | ||
| (a.clip?.y ?? null) === (b.clip?.y ?? null) && | ||
| (a.clip?.width ?? null) === (b.clip?.width ?? null) && | ||
| (a.clip?.height ?? null) === (b.clip?.height ?? null) | ||
| ); |
There was a problem hiding this comment.
[Claude Code 🤖] Same two fields missing here as in canonicalCaptureSpecString (see that thread): envelope and captures are not compared, while the comment above says a twin must match the whole capture identity. Unreachable for the same kind of reason — a job that could carry them has no persist, and the predicate inserts without one.
The ask is just that the two stay in step: whichever way the identity is narrowed or widened, this predicate is the second place that has to change, and nothing here says so.
Class: non-blocking.
| let decodeMs: number | undefined; | ||
| let persistMs: number | undefined; | ||
| if (persist && response.status === 'ready' && response.base64) { | ||
| if (hasCaptureSpecOverrides(captureSpec)) { | ||
| // The ledger identity in `persist` is the canonical capture's; it | ||
| // cannot represent viewport / scale / fullPage / clip overrides. | ||
| // Persisting an override render under it would serve that render on | ||
| // the canonical `_screenshot/` URL, so the persist is refused — the | ||
| // response still carries the bytes for the caller. | ||
| log.error( | ||
| `${jobIdentity(jobInfo)} screenshot-card job carries both a persist target and captureSpec overrides; refusing to persist a non-canonical capture under the canonical ledger identity`, | ||
| ); | ||
| } else if (!mediaCacheAdapter) { | ||
| if (!mediaCacheAdapter) { | ||
| log.warn( | ||
| `${jobIdentity(jobInfo)} screenshot-card asked to persist but this worker has no media cache adapter configured; skipping`, |
There was a problem hiding this comment.
[Claude Code 🤖] This deletes the only check that a persist identity and the spec actually rendered agree, and puts a comment in its place saying it is the producer's job. Both current producers do hold it — the GET lane hashes parsed.spec and threads captureSpecOverrides(spec) from that same value, and the POST handler derives spec and captureSpec from one parse — so this is about the invariant, not a live bug.
But the guard was removed rather than upgraded, and the new identity makes upgrading it cheap: the task holds persist.captureSpecHash, format, and captureSpec, so comparing await captureSpecHash({ format, ...(captureSpec ?? {}) }) against the persisted hash before writing restores exactly what was lost. A mismatch means this render is about to be served on some other spec's durable URL and stay there until the generation bumps; the cost of noticing is one sha256 against a Chrome render.
Class: regression introduced here (a check removed with nothing replacing it). Non-blocking.
| if (overrides.clip) { | ||
| searchParams.set( | ||
| 'clip', | ||
| `${overrides.clip.x},${overrides.clip.y},${overrides.clip.width}x${overrides.clip.height}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] URLSearchParams.toString() percent-encodes the commas, so what this emits is ?clip=0%2C0%2C400x300 — verified — while the grammar in the description and in the 400 message both tell the caller to write clip=0,0,400x300. It reparses correctly, so this is presentation, but it is the presentation of the public URL this change exists to mint, and , is a valid sub-delim in a query, so nothing requires the escape.
Assembling the query by hand, or unescaping %2C on the way out, makes the served URL match its own documented grammar.
Class: non-blocking.
2 participants
What this does
A screenshot capture's canonical identity now covers its full geometry —
viewport,deviceScaleFactor,fullPage,clip— not justformat. A custom-spec capture therefore persists to the MediaCache under its own spec hash and serves on its own durable GET_screenshot/URL, exactly like a format-only capture: ledger fast path, job coalescing, andcaptures[].urlall apply.capture-spec.ts):CaptureSpecextends the geometry override fields;canonicalCaptureSpecString/captureSpecHash/canonicalCaptureSpecQueryrepresent them with keys sorted (nested objects included) and default-valued fields elided, so every spelling of one capture geometry is one cache key. The engine defaults (800×600 viewport, scale 1, fullPage false) are pinned in runtime-common and shared with the prerender pool's viewport-restore constant, so the elision rule and the engine cannot drift.viewport=1280x800,dsf=2,fullPage=true,clip=0,0,400x300are unreserved in the GET DSL (envelopeandtargetstay reserved). The GET params translate into the POST body's shape and run through the one shared validator, so both surfaces accept and refuse identical geometry with identical wording; grammar-level failures name the param.clip's x/y parse asNumberbecause the validator admits any non-negative finite value — the served URL must reparse every spellingStringcan emit.Retry-Afteris a cheap-resume promise again — the retry answers from the ledger with no second render. The handler doc caveat describing the discarded-render behavior is deleted.allowArbitraryScreenshotsopt-in remains the deliberate cost boundary (documented at the miss path) — the parse bounds each capture's pixel cost, the serialized lane bounds concurrency, and the on-demand TTL reclaims idle entries. No per-instance spec-cardinality cap.Out of scope:
envelope/targetparams and declared-screenshotname=addressing.Tests
canonicalCaptureSpecQueryround-trips throughparseCaptureSpecParamsto the same canonical form (scientific-notation clip offsets included).media-cache-dsl-test(27),screenshot-card-test(36), and the serving/gc/adapter suites (35) all pass locally;ember-tscand eslint clean in runtime-common and realm-server.Stacked on #5871.
🤖 Generated with Claude Code