GET screenshot URL DSL: canonicalized specs, capture gate, persisted-else-on-demand - #5843
Conversation
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed as the next owner of the _screenshot/ route, tracing the miss path end to end (canonicalization → gate → congestion pre-check → sync-wait → persist) and checking it against the queue, media-cache, and index-query-engine machinery it leans on. Read against the real jobs/job_reservations schema, putMedia/findMediaCacheEntry, and both prerenderer implementations.
Bottom line: no blocking issues — the design holds and the tests pin the important edges (gate flip via indexed config, generation-bump re-capture, timeout-still-persists, congestion fail-fast, HEAD exclusion). Two things to reconcile, both non-blocking: a doc-vs-code contradiction about POST-published captures, and a missing job-coalesce that lets concurrent first-requests double-render.
- The
allowArbitraryScreenshotsfield doc (and itsserveScreenshottwin) claim POST-published captures serve as hits, but the POST handler passespersist: nulland never writes the ledger — see the inline thread onrealm-config.gts. Decide whether to wire POST persist or drop the clause. screenshot-cardregisters no coalesce handler, so simultaneous misses for one spec double-render — see the inline thread onrealm.ts. Follow-up.
Checked and clean (no action): the gate reads the indexed config and defaults closed when the config card is absent/unindexed; hits bypass the gate correctly (findMediaCacheEntry precedes the gate); the generation TOCTOU (card edited mid-capture) stores fresher-not-staler bytes under the read generation; the estimate SQL's columns/statuses all match job-tables.ts.
Adjacent, out of scope: the task's new (result as any)?.response ?? result unwrap changes the POST /_screenshot-card response body from {response, timings, pool} to the bare ScreenshotPrerenderResponse for deployments whose worker uses the local in-process prerenderer. Production workers use createRemotePrerenderer (already bare), so prod is unaffected and this aligns runtime with the declared Task<…, ScreenshotPrerenderResponse> type — flagging only in case a dev/test client reads attributes.response.*.
| // unbounded spec space reachable with only realm read, so it is off | ||
| // unless the realm turns it on; the gate blocks Chrome work only, never | ||
| // serving — declared screenshots and already-captured specs (including | ||
| // ones a write-holder produced via POST) stream regardless. Read from the |
There was a problem hiding this comment.
[Claude Code 🤖] The "via POST" hit claim contradicts the code — no POST path writes the ledger. This new field doc (and its twin comment in realm.ts serveScreenshot: "including captures a write-holder published via POST on a gated realm") tells an operator that already-captured specs "a write-holder produced via POST" stream regardless of the gate. But the only POST surface, handleScreenshotCard (POST /_screenshot-card), enqueues its job with persist: null, and the screenshot-card task calls putMedia only when persist is non-null — so a POST capture is returned in the response body and is never recorded in media_cache_ledger.
I traced every putMedia/persist caller in the tree: the sole ledger writers are the GET on-demand path (captureScreenshotOnDemand's persist: { ...entryKey, lane: 'on-demand' } and its response-body fallback putMedia). There is no POST→ledger writer, and declared-lane writers don't exist yet (the name= branch comment says the manifests "don't exist yet"). So today a POST-produced capture can never become a later GET hit.
Please reconcile one way — your call which:
- If POST should pre-warm the cache (a write-holder seeding captures for later GET hits is the useful reading), pass a real
persistfromhandleScreenshotCardinstead ofnull. - If POST is deliberately body-only — which
screenshot-card-test's new comment asserts — drop the "via POST" clause from both this field doc and theserveScreenshotcomment, so shipped base-card documentation doesn't promise a capability that isn't wired.
Class: doc/contract drift introduced by this PR (all three comments are new here). Non-blocking, but this is base-card documentation an operator reads to reason about the gate.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 6290ec99f7 — the field doc and the hit-path comment now describe exactly what the code does: any capture whose canonical spec already has a ledger entry streams regardless of the gate, with no claim about which surface wrote it. (The POST persist itself is real work, delivered by the next PR stacked on this one — its handler doc carries the publish-by-POST claim there, where it is wired.)
| // under. The requester already proved realm read; the capture is a | ||
| // realm-derived artifact, not a per-user view. | ||
| let owner = await this.getRealmOwnerUserId(); | ||
| let job = await enqueueScreenshotCardJob( |
There was a problem hiding this comment.
[Claude Code 🤖] Concurrent misses for the same spec each render — screenshot-card has no coalesce handler. enqueueScreenshotCardJob → publish calls getQueueJobCoalesceHandler('screenshot-card'), which returns undefined: nothing calls registerQueueJobDefinition for this job type (contrast tasks/copy.ts, tasks/full-reindex.ts, tasks/prerender-html-reconcile.ts, which each register a coalesce). So two GETs that miss on the same entryKey before either persists insert two distinct jobs; the screenshot:{realmURL} concurrency group serializes their execution but doesn't dedupe them, so the second still runs a full Chrome capture after the first and upserts the identical row.
The persist-then-hit design and the congestion pre-check kill the sequential retry case (a 503'd caller retries into a hit), but not the simultaneous first-request case — which is the traffic this PR newly invites (agents/scripts hitting a fresh spec). A coalesce handler keyed on (realmURL, cardId, format, sourceGeneration) that folds concurrent duplicates onto one canonical job — the pattern full-reindex already uses — would close it.
Class: follow-up (efficiency). Not a correctness bug, not blocking this PR.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 6290ec99f7 — screenshot-card now registers a coalesce handler: a queued or in-flight twin whose capture identity matches (cardId, format, runAs, and the full persist target, compared field-by-field since jsonb does not preserve key order) absorbs the incoming request as a join/late waiter. Pinned by a new test: two concurrent GETs for one uncaptured spec both 200 off a single render.
Host Test Results 1 files ±0 1 suites ±0 2h 36m 13s ⏱️ + 6m 3s Results for commit ecdfe33. ± Comparison against earlier commit 229edae. Realm Server Test Results 1 files ±0 1 suites ±0 15m 15s ⏱️ +9s Results for commit ecdfe33. ± Comparison against earlier commit 229edae. |
6290ec9 to
229edae
Compare
…ted-else-on-demand The _screenshot/ route now resolves capture-spec query params: a shared capture-spec module parses and canonicalizes them (sorted keys, defaults elided, unknown/reserved params 400 by name — validation shared with POST /_screenshot-card) into a spec hash, and the cache key pins the instance's own index generation, so an edited card never serves stale and an unchanged card is a pure ledger hit with zero Chrome work. A miss consults the new allowArbitraryScreenshots BooleanField on the base RealmConfig card, read from the realm's indexed config per request (a .realm.json edit takes effect with its index update, no restart). Gated realms 403 naming the flag — the gate blocks Chrome work, never serving. Open realms run the capture through the existing per-realm serialized screenshot-card job with a fail-fast congestion pre-check (queue depth × recent average capture duration vs the sync budget) and a bounded ~25s sync wait; over budget answers 503 + Retry-After. The job now persists its capture to the MediaCache itself (new persist args), so a timed-out wait still lands the capture and the client's retry is a ledger hit rather than a second render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e ledger screenshot-card jobs now register a coalesce handler: concurrent requests whose capture identity matches — card, format, render identity, persist target — fold onto one job (queued or in-flight), so simultaneous first requests for a fresh spec cost one render instead of one each. The per-realm concurrency group serializes execution but never deduped it. The gate documentation (RealmConfig field doc and the hit-path comment) now describes what the code does — any capture with a ledger entry serves regardless of the gate — without naming capture surfaces that do not write the ledger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
229edae to
ecdfe33
Compare
Stacked on #5840 (serving internals), which stacks on #5838 (the MediaCache store). This PR makes the
_screenshot/route real: hybrid persisted-else-on-demand serving with today's capture capabilities.What this adds
Capture-spec canonicalization (
packages/runtime-common/capture-spec.ts, shared with POST): flat unprefixed params parse into a spec whose canonical form (sorted keys, defaults elided) hashes into the ledger key —?format=isolated,?format=isolated&-noise, and the bare URL are one cache key. Today's parameter surface isformat(isolated/embedded, matching what the capture engine supports); the engine-reserved params (envelope,viewport,dsf,fullPage,clip,target) are refused by name, never ignored — ignoring would fold different capture intents onto one cache key. All parameter errors are 400s naming the offending field, with format validation shared verbatim withPOST /_screenshot-card.Cache key = (instance URL, canonical spec hash, the instance's own index generation). An edit bumps the generation, so an edited card can never serve a stale capture; an unchanged card is a pure ledger hit with zero Chrome work — and hits serve regardless of the gate, including captures a write-holder published via POST on a gated realm.
The
allowArbitraryScreenshotsgate: a new BooleanField on the baseRealmConfigcard (absent ⇒ false), read from the realm's indexed config on every gate check — editing.realm.jsontakes effect with its own index update, no restart, no cache to invalidate (pinned by a test that flips the indexed value between requests). A gated miss is a 403 whose body names the flag; the gate blocks Chrome work only, never serving.The miss path on an opted-in realm:
screenshot:lane × recent average capture duration (from the jobs table, 1h lookback, reservation-claim→finish); if that already exceeds the sync budget, immediate 503 + Retry-After computed from the same math — no doomed connection held against the queue.screenshot-cardjob (rendered as the realm's owner, the same identity an index pass renders under), waited on for up to 25s. The budget is cost posture, not transport: the realm-server ALB idle timeout is configured at 4000s, so the ticket's stay-well-under-the-ALB question is settled with margin.persistargs onscreenshot-card; the worker writes to the MediaCache before resolving): a wait that times out answers 503 + Retry-After while the render completes and lands durably, so the client's retry is a pure ledger hit — never a second render. The route also persists from the job response itself as a fallback for workers without a store configured.name=(declared screenshots) still misses — manifests are indexing-time artifacts that don't exist yet — and mixingname=with spec params is a 400. HEAD keeps the GET-only posture from the serving layer: even a captured spec answers HEAD from the generic handlers, pinned by test.Test plan
media-cache-dsl-test(real Postgres + real queue runner + realscreenshot-cardtask over a stub prerenderer — no Chrome, no HTTP server): canonicalization/hash identities and per-field 400s; gated-realm ledger hit with zero capture work; gated miss 403 naming the flag; indexed-config flip opening the gate with no restart; capture→persist→serve with hit dedupe on the second request; generation-bump re-capture; sync-wait timeout → 503 + Retry-After with the capture landing anyway and the retry re-rendering nothing; congested-lane fail-fast 503; HEAD exclusion; missing-instance miss. The POST handler suite pinspersist: nullon its published jobs; all media-cache and screenshot suites pass (44 tests); typechecks clean across runtime-common, realm-server, base (via host), host, ai-bot, billing.🤖 Generated with Claude Code