From 471cb477030b6a5ce57ad54cfec34cd805acf7e5 Mon Sep 17 00:00:00 2001 From: Luke Melia Date: Thu, 20 Aug 2026 18:41:28 -0400 Subject: [PATCH 1/2] Add GET screenshot URL DSL: canonicalized specs, capture gate, persisted-else-on-demand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/base/realm-config.gts | 9 + .../handlers/handle-screenshot-card.ts | 6 +- .../tests/helpers/fake-media-cache-adapter.ts | 43 ++ packages/realm-server/tests/helpers/index.ts | 13 +- packages/realm-server/tests/index.ts | 1 + .../tests/media-cache-dsl-test.ts | 461 ++++++++++++++++++ .../realm-server/tests/media-cache-gc-test.ts | 35 +- .../tests/media-cache-serving-test.ts | 35 +- .../tests/screenshot-card-test.ts | 3 + packages/runtime-common/capture-spec.ts | 98 ++++ packages/runtime-common/index.ts | 1 + .../runtime-common/jobs/screenshot-card.ts | 65 +++ packages/runtime-common/realm.ts | 245 +++++++++- .../runtime-common/tasks/screenshot-card.ts | 58 ++- 14 files changed, 981 insertions(+), 92 deletions(-) create mode 100644 packages/realm-server/tests/helpers/fake-media-cache-adapter.ts create mode 100644 packages/realm-server/tests/media-cache-dsl-test.ts create mode 100644 packages/runtime-common/capture-spec.ts diff --git a/packages/base/realm-config.gts b/packages/base/realm-config.gts index 83cd13ac361..ecf639576c2 100644 --- a/packages/base/realm-config.gts +++ b/packages/base/realm-config.gts @@ -684,6 +684,15 @@ export class RealmConfig extends CardDef { // automatically in that case) or when an operator otherwise needs // the full isolated render present in the index. @field includePrerenderedDefaultRealmIndex = contains(BooleanField); + // Opt-in for the realm's GET `_screenshot/` route to trigger NEW captures + // for arbitrary capture specs. Full captureSpec power on a GET is an + // 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 + // realm's indexed config at request time, so editing this takes effect + // with the index update, no restart. + @field allowArbitraryScreenshots = contains(BooleanField); @field cardTitle = contains(StringField, { computeVia: function (this: RealmConfig) { diff --git a/packages/realm-server/handlers/handle-screenshot-card.ts b/packages/realm-server/handlers/handle-screenshot-card.ts index 0e0ad5dc633..19a6caae9fb 100644 --- a/packages/realm-server/handlers/handle-screenshot-card.ts +++ b/packages/realm-server/handlers/handle-screenshot-card.ts @@ -1,5 +1,6 @@ import type Koa from 'koa'; +import { isCaptureFormat } from '@cardstack/runtime-common'; import { enqueueScreenshotCardJob } from '@cardstack/runtime-common/jobs/screenshot-card'; import { userInitiatedPriority } from '@cardstack/runtime-common/queue'; @@ -64,7 +65,9 @@ export default function handleScreenshotCard({ if (!cardId || typeof cardId !== 'string') { return sendResponseForBadRequest(ctxt, 'cardId is required'); } - if (format !== 'isolated' && format !== 'embedded') { + // Shared with the GET `_screenshot/` DSL so both surfaces accept exactly + // the same capture formats. + if (!isCaptureFormat(format)) { return sendResponseForBadRequest( ctxt, 'format must be "isolated" or "embedded"', @@ -88,6 +91,7 @@ export default function handleScreenshotCard({ runAs: userId, cardId, format, + persist: null, }, queue, dbAdapter, diff --git a/packages/realm-server/tests/helpers/fake-media-cache-adapter.ts b/packages/realm-server/tests/helpers/fake-media-cache-adapter.ts new file mode 100644 index 00000000000..1b5d8e44132 --- /dev/null +++ b/packages/realm-server/tests/helpers/fake-media-cache-adapter.ts @@ -0,0 +1,43 @@ +import { Readable } from 'node:stream'; +import type { MediaCacheAdapter } from '@cardstack/runtime-common'; + +// In-memory MediaCacheAdapter for tests: real bytes behind the interface, +// observable deletes, scriptable per-key delete failures, and a switch +// between the two stream shapes the serving layer handles (a node Readable, +// which streams via `nodeStream`, and a bare async iterable, which is +// buffered). +export class FakeMediaCacheAdapter implements MediaCacheAdapter { + objects = new Map(); + deleted: string[] = []; + failDeletesFor = new Set(); + streamShape: 'readable' | 'iterable' = 'readable'; + + async put(key: string, bytes: Uint8Array, _opts: { contentType: string }) { + if (!this.objects.has(key)) { + this.objects.set(key, bytes); + } + } + async head(key: string) { + let bytes = this.objects.get(key); + return bytes ? { size: bytes.length } : undefined; + } + async getStream(key: string) { + let bytes = this.objects.get(key); + if (!bytes) { + return undefined; + } + if (this.streamShape === 'readable') { + return Readable.from(Buffer.from(bytes)); + } + return (async function* () { + yield bytes; + })(); + } + async delete(key: string) { + if (this.failDeletesFor.has(key)) { + throw new Error(`simulated delete failure for ${key}`); + } + this.deleted.push(key); + this.objects.delete(key); + } +} diff --git a/packages/realm-server/tests/helpers/index.ts b/packages/realm-server/tests/helpers/index.ts index 8fee7f079aa..f8936b50cc3 100644 --- a/packages/realm-server/tests/helpers/index.ts +++ b/packages/realm-server/tests/helpers/index.ts @@ -1238,6 +1238,7 @@ export async function createRealm({ transpileCoordinator, fullIndexOnStartup, mediaCacheAdapter, + screenshotSyncWaitMs, }: { dir: string; definitionLookup: DefinitionLookup; @@ -1274,6 +1275,9 @@ export async function createRealm({ // MediaCache object store for the realm's `_screenshot/` route; absent // means every screenshot request serves as an uncaptured miss. mediaCacheAdapter?: MediaCacheAdapter; + // Shrinks the `_screenshot/` route's on-demand sync-wait budget so tests + // can exercise the 503 + Retry-After path without holding real time. + screenshotSyncWaitMs?: number; }): Promise<{ realm: Realm; adapter: RealmAdapter }> { await insertPermissions(dbAdapter, new URL(realmURL), permissions); @@ -1352,7 +1356,10 @@ export async function createRealm({ transpileCoordinator, mediaCacheAdapter, }, - fullIndexOnStartup ? { fullIndexOnStartup: true as const } : undefined, + { + ...(fullIndexOnStartup ? { fullIndexOnStartup: true as const } : {}), + ...(screenshotSyncWaitMs !== undefined ? { screenshotSyncWaitMs } : {}), + }, ); if (worker) { virtualNetwork.mount(realm.handle); @@ -3062,6 +3069,7 @@ export function realmConfigCardJSON( iconURL?: string; backgroundURL?: string; includePrerenderedDefaultRealmIndex?: boolean; + allowArbitraryScreenshots?: boolean; } = {}, ): string { let attrs: Record = {}; @@ -3078,6 +3086,9 @@ export function realmConfigCardJSON( attrs.includePrerenderedDefaultRealmIndex = config.includePrerenderedDefaultRealmIndex; } + if (config.allowArbitraryScreenshots !== undefined) { + attrs.allowArbitraryScreenshots = config.allowArbitraryScreenshots; + } return JSON.stringify({ data: { type: 'card', diff --git a/packages/realm-server/tests/index.ts b/packages/realm-server/tests/index.ts index 4d0e35a0966..cd2e2783054 100644 --- a/packages/realm-server/tests/index.ts +++ b/packages/realm-server/tests/index.ts @@ -264,6 +264,7 @@ const ALL_TEST_FILES: string[] = [ './media-cache-adapter-test', './media-cache-gc-test', './media-cache-serving-test', + './media-cache-dsl-test', './prerender-server-test', './prerender-manager-test', './prerender-host-shell-recycle-test', diff --git a/packages/realm-server/tests/media-cache-dsl-test.ts b/packages/realm-server/tests/media-cache-dsl-test.ts new file mode 100644 index 00000000000..56d485ebd01 --- /dev/null +++ b/packages/realm-server/tests/media-cache-dsl-test.ts @@ -0,0 +1,461 @@ +import QUnit from 'qunit'; +const { module, test } = QUnit; +import { basename } from 'path'; +import { mkdtemp } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { PgAdapter } from '@cardstack/postgres'; +import type { + DefinitionLookup, + IndexWriter, + Prerenderer, + QueuePublisher, + QueueRunner, + Realm, + ScreenshotPrerenderResponse, + VirtualNetwork as VirtualNetworkType, +} from '@cardstack/runtime-common'; +import { + Deferred, + VirtualNetwork, + asExpressions, + canonicalCaptureSpecString, + captureSpecHash, + findMediaCacheEntry, + insert, + logger, + parseCaptureSpecParams, + putMedia, + query, + screenshotCard, +} from '@cardstack/runtime-common'; + +import { FakeMediaCacheAdapter } from './helpers/fake-media-cache-adapter.ts'; +import { createRealm, insertJob, setupDB } from './helpers/index.ts'; +import { nodeStreamToBuffer } from '../stream.ts'; + +const REALM_URL = 'http://test-dsl-realm/'; +const OWNER = '@node-test_realm:localhost'; +const PNG_BYTES = new TextEncoder().encode('stub-png-bytes'); +const PNG_BASE64 = Buffer.from(PNG_BYTES).toString('base64'); +// Long enough that a healthy queue round-trip never times out; short enough +// that the deliberately-stalled timeout test doesn't drag the suite. +const SYNC_WAIT_MS = 2000; + +function params(qs: string): URLSearchParams { + return new URL(`http://x/?${qs}`).searchParams; +} + +module(basename(import.meta.filename), function () { + module('capture-spec canonicalization', function () { + test('the all-defaults spec canonicalizes to {} however it is spelled', async function (assert) { + let bare = parseCaptureSpecParams(params('')); + let explicit = parseCaptureSpecParams(params('format=isolated')); + assert.true('spec' in bare, 'the bare URL parses'); + assert.true('spec' in explicit, 'the explicit-default URL parses'); + if ('spec' in bare && 'spec' in explicit) { + assert.strictEqual(canonicalCaptureSpecString(bare.spec), '{}'); + assert.strictEqual( + await captureSpecHash(bare.spec), + await captureSpecHash(explicit.spec), + 'default-elision makes the two spellings one cache key', + ); + } + }); + + test('a non-default format is its own cache key', async function (assert) { + let embedded = parseCaptureSpecParams(params('format=embedded')); + assert.true('spec' in embedded, 'format=embedded parses'); + if ('spec' in embedded) { + assert.strictEqual( + canonicalCaptureSpecString(embedded.spec), + '{"format":"embedded"}', + ); + assert.notStrictEqual( + await captureSpecHash(embedded.spec), + await captureSpecHash({ format: 'isolated' }), + ); + } + }); + + test('errors name the offending field', function (assert) { + let unknown = parseCaptureSpecParams(params('sparkle=true')); + assert.deepEqual(unknown, { + error: { field: 'sparkle', message: 'unsupported parameter "sparkle"' }, + }); + + let reserved = parseCaptureSpecParams(params('viewport=1280x800')); + assert.strictEqual( + 'error' in reserved ? reserved.error.field : undefined, + 'viewport', + ); + + let badFormat = parseCaptureSpecParams(params('format=fancy')); + assert.deepEqual(badFormat, { + error: { + field: 'format', + message: 'format must be "isolated" or "embedded"', + }, + }); + + let repeated = parseCaptureSpecParams( + params('format=isolated&format=embedded'), + ); + assert.strictEqual( + 'error' in repeated ? repeated.error.field : undefined, + 'format', + ); + }); + }); + + module('GET _screenshot capture flow', function (hooks) { + let dbAdapter: PgAdapter; + let publisher: QueuePublisher; + let runner: QueueRunner; + let realm: Realm; + let adapter: FakeMediaCacheAdapter; + let virtualNetwork: VirtualNetworkType; + let captureCalls: number; + // When set, in-flight captures park on it — the lever for the sync-wait + // timeout test. + let captureGate: Deferred | undefined; + + setupDB(hooks, { + beforeEach: async ( + _dbAdapter: PgAdapter, + _publisher: QueuePublisher, + _runner: QueueRunner, + ): Promise => { + dbAdapter = _dbAdapter; + publisher = _publisher; + runner = _runner; + adapter = new FakeMediaCacheAdapter(); + captureCalls = 0; + captureGate = undefined; + virtualNetwork = new VirtualNetwork(); + ({ realm } = await createRealm({ + dir: await mkdtemp(join(tmpdir(), 'media-cache-dsl-test-')), + definitionLookup: { + forRealm() { + return this; + }, + } as unknown as DefinitionLookup, + realmURL: REALM_URL, + permissions: { + '*': ['read'], + [OWNER]: ['read', 'write', 'realm-owner'], + }, + virtualNetwork, + publisher, + dbAdapter, + mediaCacheAdapter: adapter, + screenshotSyncWaitMs: SYNC_WAIT_MS, + })); + }, + }); + + // Registers the real screenshot-card task on the test runner, with a + // stub prerenderer standing in for the Chrome pool. Only tests that + // want a capture to complete start the worker; the rest leave enqueued + // jobs unclaimed on purpose. + async function startWorker() { + let prerenderer = { + prerenderScreenshot: async (): Promise => { + captureCalls++; + if (captureGate) { + await captureGate.promise; + } + return { + status: 'ready', + base64: PNG_BASE64, + width: 8, + height: 6, + contentType: 'image/png', + }; + }, + } as unknown as Prerenderer; + await runner.register( + 'screenshot-card', + screenshotCard({ + dbAdapter, + queuePublisher: publisher, + prerenderer, + mediaCacheAdapter: adapter, + log: logger('media-cache-dsl-test'), + reportStatus: () => {}, + matrixURL: 'http://localhost:8008', + indexWriter: null as unknown as IndexWriter, + definitionLookup: null as unknown as DefinitionLookup, + virtualNetwork, + getReader: () => { + throw new Error('getReader is not used by screenshot-card'); + }, + getAuthedFetch: async () => globalThis.fetch, + createPrerenderAuth: () => 'test-auth', + }), + ); + await runner.start(); + } + + async function seedInstanceRow(localPath: string, generation = 1) { + let { nameExpressions, valueExpressions } = asExpressions( + { + url: `${REALM_URL}${localPath}.json`, + file_alias: `${REALM_URL}${localPath}`, + realm_url: REALM_URL, + type: 'instance', + generation, + last_modified: Date.now(), + resource_created_at: Date.now(), + is_deleted: false, + pristine_doc: { attributes: {} }, + }, + { jsonFields: ['pristine_doc'] }, + ); + await query( + dbAdapter, + insert('boxel_index', nameExpressions, valueExpressions), + ); + } + + async function seedRealmConfigRow(allowArbitraryScreenshots: boolean) { + let { nameExpressions, valueExpressions } = asExpressions( + { + url: `${REALM_URL}realm.json`, + file_alias: `${REALM_URL}realm`, + realm_url: REALM_URL, + type: 'instance', + generation: 1, + last_modified: Date.now(), + resource_created_at: Date.now(), + is_deleted: false, + pristine_doc: { attributes: { allowArbitraryScreenshots } }, + }, + { jsonFields: ['pristine_doc'] }, + ); + await query( + dbAdapter, + insert('boxel_index', nameExpressions, valueExpressions), + ); + } + + async function get(pathAndQuery: string, method = 'GET') { + let response = await realm.handle( + new Request(`${REALM_URL}${pathAndQuery}`, { method }), + ); + return response!; + } + + test('an already-captured spec serves on a gated realm with zero capture work', async function (assert) { + await seedInstanceRow('card-1'); + await putMedia(dbAdapter, adapter, { + realmURL: REALM_URL, + sourceURL: `${REALM_URL}card-1`, + captureSpecHash: await captureSpecHash({ format: 'isolated' }), + sourceGeneration: 1, + bytes: PNG_BYTES, + contentType: 'image/png', + lane: 'on-demand', + }); + + let response = await get('_screenshot/card-1'); + + assert.strictEqual(response.status, 200); + assert.strictEqual(response.headers.get('content-type'), 'image/png'); + assert.deepEqual( + [...(await nodeStreamToBuffer(response.nodeStream!))], + [...PNG_BYTES], + ); + assert.strictEqual(captureCalls, 0, 'no render work occurred'); + }); + + test('a gated miss is a 403 naming the flag', async function (assert) { + await seedInstanceRow('card-1'); + + let response = await get('_screenshot/card-1'); + + assert.strictEqual(response.status, 403); + assert.true( + (await response.text()).includes('allowArbitraryScreenshots'), + 'the refusal names the config flag', + ); + assert.strictEqual(captureCalls, 0); + }); + + test('flipping the indexed config opens the gate with no restart', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(false); + + assert.strictEqual((await get('_screenshot/card-1')).status, 403); + + // The flag is read from the indexed config on every request, so an + // index update is all it takes. + await query(dbAdapter, [ + `UPDATE boxel_index SET pristine_doc = '{"attributes":{"allowArbitraryScreenshots":true}}'::jsonb + WHERE url = '${REALM_URL}realm.json'`, + ]); + await startWorker(); + + let response = await get('_screenshot/card-1'); + assert.strictEqual(response.status, 200); + assert.strictEqual(captureCalls, 1); + }); + + test('an open realm captures on demand, persists, and then serves hits', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(true); + await startWorker(); + + let response = await get('_screenshot/card-1?format=embedded'); + assert.strictEqual(response.status, 200); + assert.strictEqual(response.headers.get('content-type'), 'image/png'); + assert.deepEqual( + [...(await nodeStreamToBuffer(response.nodeStream!))], + [...PNG_BYTES], + ); + assert.strictEqual(captureCalls, 1); + + let entry = await findMediaCacheEntry(dbAdapter, { + realmURL: REALM_URL, + sourceURL: `${REALM_URL}card-1`, + captureSpecHash: await captureSpecHash({ format: 'embedded' }), + sourceGeneration: 1, + }); + assert.strictEqual(entry?.lane, 'on-demand'); + + let second = await get('_screenshot/card-1?format=embedded'); + assert.strictEqual(second.status, 200); + assert.strictEqual(captureCalls, 1, 'the second request is a pure hit'); + }); + + test('an edited instance never serves a stale capture', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(true); + await startWorker(); + + await get('_screenshot/card-1'); + assert.strictEqual(captureCalls, 1); + + // An edit bumps the instance's index generation, which is part of the + // cache key. + await query(dbAdapter, [ + `UPDATE boxel_index SET generation = 2 WHERE url = '${REALM_URL}card-1.json'`, + ]); + + let response = await get('_screenshot/card-1'); + assert.strictEqual(response.status, 200); + assert.strictEqual(captureCalls, 2, 'the edited card re-captured'); + }); + + test('a sync wait that outruns the budget answers 503, and the capture still lands', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(true); + captureGate = new Deferred(); + await startWorker(); + + let response = await get('_screenshot/card-1'); + assert.strictEqual(response.status, 503); + assert.ok( + Number(response.headers.get('retry-after')) >= 1, + 'the 503 carries a Retry-After', + ); + + // The job kept running; once the render finishes it persists its own + // capture, so the client retry is a pure ledger hit. + captureGate.fulfill(); + captureGate = undefined; + let entryKey = { + realmURL: REALM_URL, + sourceURL: `${REALM_URL}card-1`, + captureSpecHash: await captureSpecHash({ format: 'isolated' }), + sourceGeneration: 1, + }; + let deadline = Date.now() + 10_000; + while ( + !(await findMediaCacheEntry(dbAdapter, entryKey)) && + Date.now() < deadline + ) { + await new Promise((resolve) => setTimeout(resolve, 50)); + } + assert.ok( + await findMediaCacheEntry(dbAdapter, entryKey), + 'the timed-out capture persisted anyway', + ); + let capturesSoFar = captureCalls; + let retry = await get('_screenshot/card-1'); + assert.strictEqual(retry.status, 200); + assert.strictEqual( + captureCalls, + capturesSoFar, + 'the retry re-rendered nothing', + ); + }); + + test('a congested lane fails fast with 503 + Retry-After', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(true); + // A queued capture already holds the realm's serialized lane; with no + // worker started it stays pending, and pending × the default capture + // estimate dwarfs the budget. + await insertJob(dbAdapter, { + job_type: 'screenshot-card', + concurrency_group: `screenshot:${REALM_URL}`, + }); + + let response = await get('_screenshot/card-1'); + + assert.strictEqual(response.status, 503); + assert.ok(Number(response.headers.get('retry-after')) >= 1); + assert.strictEqual(captureCalls, 0, 'nothing was enqueued or rendered'); + }); + + test('HEAD never reaches the screenshot route, even for a captured spec', async function (assert) { + // checkPermission exempts HEAD from auth realm-wide; the GET-only + // dispatch is what keeps HEAD from becoming an unauthenticated + // existence/size/content-hash oracle. Even a spec with a live capture + // answers a HEAD from the generic handlers, not this route. + await seedInstanceRow('card-1'); + await putMedia(dbAdapter, adapter, { + realmURL: REALM_URL, + sourceURL: `${REALM_URL}card-1`, + captureSpecHash: await captureSpecHash({ format: 'isolated' }), + sourceGeneration: 1, + bytes: PNG_BYTES, + contentType: 'image/png', + lane: 'on-demand', + }); + + let response = await get('_screenshot/card-1', 'HEAD'); + + assert.notStrictEqual(response.status, 200); + assert.strictEqual( + response.headers.get('etag'), + null, + 'no content-hash validator leaks', + ); + assert.strictEqual(captureCalls, 0); + }); + + test('parameter errors are 400s naming the field', async function (assert) { + await seedInstanceRow('card-1'); + + let unknown = await get('_screenshot/card-1?sparkle=true'); + assert.strictEqual(unknown.status, 400); + assert.true((await unknown.text()).includes('sparkle')); + + let mixed = await get('_screenshot/card-1?name=hero&format=embedded'); + assert.strictEqual(mixed.status, 400); + assert.true((await mixed.text()).includes('name cannot be combined')); + }); + + test('a missing instance is an uncaptured miss, not a capture attempt', async function (assert) { + await seedRealmConfigRow(true); + await startWorker(); + + let response = await get('_screenshot/nope'); + + assert.strictEqual(response.status, 404); + assert.strictEqual(captureCalls, 0); + }); + }); +}); diff --git a/packages/realm-server/tests/media-cache-gc-test.ts b/packages/realm-server/tests/media-cache-gc-test.ts index 17731505de1..3f08fea6e83 100644 --- a/packages/realm-server/tests/media-cache-gc-test.ts +++ b/packages/realm-server/tests/media-cache-gc-test.ts @@ -22,45 +22,12 @@ import { touchMediaCacheEntry, } from '@cardstack/runtime-common'; +import { FakeMediaCacheAdapter } from './helpers/fake-media-cache-adapter.ts'; import { setupDB } from './helpers/index.ts'; const HOUR = 60 * 60 * 1000; const DAY = 24 * HOUR; -// In-memory MediaCacheAdapter: enough store to observe what the sweep -// deletes, plus scriptable per-key delete failures. -class FakeMediaCacheAdapter implements MediaCacheAdapter { - objects = new Map(); - deleted: string[] = []; - failDeletesFor = new Set(); - - async put(key: string, bytes: Uint8Array, _opts: { contentType: string }) { - if (!this.objects.has(key)) { - this.objects.set(key, bytes); - } - } - async head(key: string) { - let bytes = this.objects.get(key); - return bytes ? { size: bytes.length } : undefined; - } - async getStream(key: string) { - let bytes = this.objects.get(key); - if (!bytes) { - return undefined; - } - return (async function* () { - yield bytes; - })(); - } - async delete(key: string) { - if (this.failDeletesFor.has(key)) { - throw new Error(`simulated delete failure for ${key}`); - } - this.deleted.push(key); - this.objects.delete(key); - } -} - module(basename(import.meta.filename), function (hooks) { let dbAdapter: PgAdapter; let adapter: FakeMediaCacheAdapter; diff --git a/packages/realm-server/tests/media-cache-serving-test.ts b/packages/realm-server/tests/media-cache-serving-test.ts index 4eb2086c89f..c5651a55bd4 100644 --- a/packages/realm-server/tests/media-cache-serving-test.ts +++ b/packages/realm-server/tests/media-cache-serving-test.ts @@ -1,10 +1,8 @@ import QUnit from 'qunit'; const { module, test } = QUnit; import { basename } from 'path'; -import { Readable } from 'node:stream'; import type { PgAdapter } from '@cardstack/postgres'; import type { - MediaCacheAdapter, MediaCacheEntry, QueuePublisher, Realm, @@ -21,43 +19,12 @@ import { } from '@cardstack/runtime-common'; import { nodeStreamToBuffer } from '../stream.ts'; +import { FakeMediaCacheAdapter } from './helpers/fake-media-cache-adapter.ts'; import { setupDB } from './helpers/index.ts'; const REALM_URL = 'http://test-realm/a/'; const BYTES = new TextEncoder().encode('png-bytes'); -// Minimal store: real bytes behind the interface, with a switch between the -// two stream shapes the serving layer handles (a node Readable, which -// streams via `nodeStream`, and a bare async iterable, which is buffered). -class FakeMediaCacheAdapter implements MediaCacheAdapter { - objects = new Map(); - streamShape: 'readable' | 'iterable' = 'readable'; - - async put(key: string, bytes: Uint8Array, _opts: { contentType: string }) { - this.objects.set(key, bytes); - } - async head(key: string) { - let bytes = this.objects.get(key); - return bytes ? { size: bytes.length } : undefined; - } - async getStream(key: string) { - let bytes = this.objects.get(key); - if (!bytes) { - return undefined; - } - if (this.streamShape === 'readable') { - return Readable.from(Buffer.from(bytes)); - } - return (async function* () { - yield bytes.slice(0, 3); - yield bytes.slice(3); - })(); - } - async delete(key: string) { - this.objects.delete(key); - } -} - function requestContext( permissions: Record = {}, ): RequestContext { diff --git a/packages/realm-server/tests/screenshot-card-test.ts b/packages/realm-server/tests/screenshot-card-test.ts index 84a7b1ea0a7..78882c552ad 100644 --- a/packages/realm-server/tests/screenshot-card-test.ts +++ b/packages/realm-server/tests/screenshot-card-test.ts @@ -150,6 +150,9 @@ module(basename(import.meta.filename), function () { runAs: '@someone:localhost', cardId, format: 'isolated', + // The POST surface returns the capture in its response body rather + // than recording it in the MediaCache ledger. + persist: null, }); }); diff --git a/packages/runtime-common/capture-spec.ts b/packages/runtime-common/capture-spec.ts new file mode 100644 index 00000000000..ad292db1d2a --- /dev/null +++ b/packages/runtime-common/capture-spec.ts @@ -0,0 +1,98 @@ +import { computeMediaCacheKey } from './media-cache.ts'; + +// The capture spec: every way a screenshot capture can be parameterized, +// shared by the POST /_screenshot-card body and the GET `_screenshot/` URL +// DSL so the two surfaces validate identically and one capture satisfies +// both. The spec's canonical form is what keys the MediaCache ledger, so +// everything here is deliberately strict: two requests that mean the same +// capture must canonicalize to the same string, and a parameter the engine +// cannot honor is refused by name rather than ignored (ignoring would fold +// different intents onto one cache key and serve the wrong image). + +export const CAPTURE_FORMATS = ['isolated', 'embedded'] as const; +export type CaptureFormat = (typeof CAPTURE_FORMATS)[number]; +export const DEFAULT_CAPTURE_FORMAT: CaptureFormat = 'isolated'; + +export function isCaptureFormat(value: unknown): value is CaptureFormat { + return (CAPTURE_FORMATS as readonly unknown[]).includes(value); +} + +export interface CaptureSpec { + format: CaptureFormat; +} + +// The DSL's parameter surface grows with the capture engine; these names are +// reserved for the engine capabilities the project's URL grammar assigns +// them, and refused (never ignored) while the engine lacks them. +const RESERVED_CAPTURE_PARAMS = new Set([ + 'envelope', + 'viewport', + 'dsf', + 'fullPage', + 'clip', + 'target', +]); + +export type CaptureSpecParseResult = + | { spec: CaptureSpec } + | { error: { field: string; message: string } }; + +// Parses the flat, unprefixed query params of a `_screenshot/` request into +// a spec. Strict on principle (see the module comment): unknown and +// reserved params, repeated params, and out-of-range values are each a 400 +// naming the offending field. `name=` addresses a declared screenshot, a +// different addressing form entirely — the route splits it off before +// calling this. +export function parseCaptureSpecParams( + searchParams: URLSearchParams, +): CaptureSpecParseResult { + for (let key of new Set(searchParams.keys())) { + if (key === 'format') { + continue; + } + let message = RESERVED_CAPTURE_PARAMS.has(key) + ? `parameter "${key}" is not supported by this capture engine` + : `unsupported parameter "${key}"`; + return { error: { field: key, message } }; + } + if (searchParams.getAll('format').length > 1) { + return { + error: { field: 'format', message: 'format may only be given once' }, + }; + } + let format = searchParams.get('format') ?? DEFAULT_CAPTURE_FORMAT; + if (!isCaptureFormat(format)) { + // Same wording as the POST /_screenshot-card validation. + return { + error: { + field: 'format', + message: 'format must be "isolated" or "embedded"', + }, + }; + } + return { spec: { format } }; +} + +// The canonical serialization: keys sorted, default-valued fields elided — +// so the all-defaults spec is `{}` however it was spelled, and any two +// requests meaning the same capture hash identically. +export function canonicalCaptureSpecString(spec: CaptureSpec): string { + let canonical: Record = {}; + if (spec.format !== DEFAULT_CAPTURE_FORMAT) { + canonical.format = spec.format; + } + return JSON.stringify( + Object.fromEntries( + Object.entries(canonical).sort(([a], [b]) => a.localeCompare(b)), + ), + ); +} + +// The ledger key component for a spec: the hash of its canonical form (the +// same sha256-hex the store uses for content addresses, though this one +// keys intent rather than bytes). +export async function captureSpecHash(spec: CaptureSpec): Promise { + return await computeMediaCacheKey( + new TextEncoder().encode(canonicalCaptureSpecString(spec)), + ); +} diff --git a/packages/runtime-common/index.ts b/packages/runtime-common/index.ts index bdd3aeaab8e..6a808e8a2fa 100644 --- a/packages/runtime-common/index.ts +++ b/packages/runtime-common/index.ts @@ -1064,6 +1064,7 @@ export * from './job-utils.ts'; export * from './prerender-html-reconcile.ts'; export * from './media-cache.ts'; export * from './media-cache-serving.ts'; +export * from './capture-spec.ts'; export * from './expression.ts'; export * from './searchable-parity.ts'; export * from './infer-content-type.ts'; diff --git a/packages/runtime-common/jobs/screenshot-card.ts b/packages/runtime-common/jobs/screenshot-card.ts index 9128e917611..bc4f3a8d0ad 100644 --- a/packages/runtime-common/jobs/screenshot-card.ts +++ b/packages/runtime-common/jobs/screenshot-card.ts @@ -1,9 +1,74 @@ +import { param, query, type Expression } from '../expression.ts'; import type { QueuePublisher } from '../queue.ts'; import type { ScreenshotPrerenderResponse, DBAdapter } from '../index.ts'; import type { ScreenshotCardArgs } from '../tasks/screenshot-card.ts'; export const SCREENSHOT_CARD_JOB_TIMEOUT_SEC = 60; +// How long a GET `_screenshot/` request holds its connection waiting for an +// on-demand capture before answering 503 + Retry-After. A cost-posture +// bound, not a transport one (the realm-server ALB's idle timeout is far +// above this): sync-wait callers are humans, agents, and scripts that honor +// Retry-After — HTML and crawlers live on the never-waiting `name=` path — +// so the hold is a courtesy and the Retry-After is the contract. +export const SCREENSHOT_SYNC_WAIT_BUDGET_MS = 25_000; + +// The stand-in capture estimate while the jobs table holds no recent +// screenshot history to average. +const DEFAULT_CAPTURE_ESTIMATE_MS = 10_000; +const CAPTURE_DURATION_LOOKBACK_HOURS = 1; + +export interface ScreenshotQueueEstimate { + pending: number; + avgCaptureMs: number; + // queue depth × average capture time: what a new arrival would wait + // behind the realm's serialized screenshot lane before its own capture + // even starts. + estimatedWaitMs: number; +} + +// The fail-fast congestion pre-check for a capture-triggering request: +// when the realm's serialized screenshot lane is already deep enough that a +// new arrival's wait would blow the sync budget, the caller answers 503 + +// Retry-After immediately instead of holding a doomed connection against +// the queue. The jobs-table SQL is Postgres-shaped; on any other adapter +// (an in-memory test realm) the estimate is zero, which simply skips the +// pre-check. +export async function estimateScreenshotQueueWait( + dbAdapter: DBAdapter, + concurrencyGroup: string, +): Promise { + if (dbAdapter.kind !== 'pg') { + return { pending: 0, avgCaptureMs: 0, estimatedWaitMs: 0 }; + } + let [pendingRows, durationRows] = await Promise.all([ + query(dbAdapter, [ + `SELECT COUNT(*) AS pending FROM jobs WHERE status = 'unfulfilled' AND concurrency_group =`, + param(concurrencyGroup), + ] as Expression) as Promise<{ pending: number | string }[]>, + // Execution time is reservation-claim → job-finish (there is no + // started_at column); resolved captures only, bounded lookback so the + // estimate tracks current conditions. + query(dbAdapter, [ + `SELECT AVG(EXTRACT(EPOCH FROM (j.finished_at - jr.created_at)) * 1000) AS avg_ms + FROM jobs j + JOIN job_reservations jr ON jr.job_id = j.id AND jr.completed_at IS NOT NULL + WHERE j.job_type = 'screenshot-card' + AND j.status = 'resolved' + AND j.finished_at > NOW() - INTERVAL '${CAPTURE_DURATION_LOOKBACK_HOURS} hours'`, + ] as Expression) as Promise<{ avg_ms: number | string | null }[]>, + ]); + let pending = Number(pendingRows[0]?.pending ?? 0); + let avgRaw = durationRows[0]?.avg_ms; + let avgCaptureMs = + avgRaw == null ? DEFAULT_CAPTURE_ESTIMATE_MS : Number(avgRaw); + return { + pending, + avgCaptureMs, + estimatedWaitMs: pending * avgCaptureMs, + }; +} + export async function enqueueScreenshotCardJob( args: ScreenshotCardArgs, queue: QueuePublisher, diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index a25e8780ae8..0cac9cef4a5 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -166,11 +166,26 @@ import { import { parseQuery } from './query.ts'; import type { Readable } from 'stream'; import { createResponse } from './create-response.ts'; -import type { MediaCacheAdapter, MediaCacheEntry } from './media-cache.ts'; +import { + captureSpecHash, + parseCaptureSpecParams, + type CaptureSpec, +} from './capture-spec.ts'; +import { + findMediaCacheEntry, + putMedia, + type MediaCacheAdapter, + type MediaCacheEntryKey, +} from './media-cache.ts'; import { mediaCacheMissResponse, serveMediaCacheEntry, } from './media-cache-serving.ts'; +import { + enqueueScreenshotCardJob, + estimateScreenshotQueueWait, + SCREENSHOT_SYNC_WAIT_BUDGET_MS, +} from './jobs/screenshot-card.ts'; import { mergeRelationships } from './merge-relationships.ts'; import { getCardDirectoryName } from './helpers/card-directory-name.ts'; import { @@ -846,6 +861,11 @@ interface Options { // removes both the startup wait and the prerender-pool contention it would // otherwise create with the tests. skipBootIndex?: true; + // How long a `_screenshot/` request holds its connection waiting for an + // on-demand capture before answering 503 + Retry-After. Defaults to + // SCREENSHOT_SYNC_WAIT_BUDGET_MS; tests shrink it to exercise the timeout + // path without holding real time. + screenshotSyncWaitMs?: number; } interface UpdateItem { @@ -965,6 +985,7 @@ export class Realm { #queue: QueuePublisher; #virtualNetwork: VirtualNetwork; #mediaCacheAdapter: MediaCacheAdapter | undefined; + #screenshotSyncWaitMs: number; #cachedRealmInfo: RealmInfo | null = null; // md5 of the JSON-stringified `#cachedRealmInfo`. Folded into the // card+json ETag so any path that nulls `#cachedRealmInfo` (e.g. @@ -1098,6 +1119,8 @@ export class Realm { this.#disableModuleCaching = Boolean(opts?.disableModuleCaching); this.#copiedFromRealm = opts?.copiedFromRealm; this.#mediaCacheAdapter = mediaCacheAdapter; + this.#screenshotSyncWaitMs = + opts?.screenshotSyncWaitMs ?? SCREENSHOT_SYNC_WAIT_BUDGET_MS; let owner: string | undefined; let _fetch = fetcher( virtualNetwork.fetch, @@ -4034,32 +4057,214 @@ export class Realm { if (instance?.type !== 'instance') { return mediaCacheMissResponse({ requestContext }); } - let entry = await this.resolveScreenshotEntry( - instanceURL, - new URL(request.url).searchParams, - ); - if (!entry) { + let searchParams = new URL(request.url).searchParams; + + // `name=` addresses a declared screenshot through the instance's + // manifest — a different addressing form from the capture-spec params, + // so mixing them is a request with two contradictory identities. + let name = searchParams.get('name'); + if (name !== null) { + let extraParams = [...new Set(searchParams.keys())].filter( + (key) => key !== 'name', + ); + if (extraParams.length > 0) { + return badRequest({ + message: `name cannot be combined with capture parameters ("${extraParams[0]}")`, + requestContext, + }); + } + // Declared-screenshot manifests are indexing-time artifacts; nothing + // publishes them, so every name is an uncaptured miss. return mediaCacheMissResponse({ requestContext }); } - return await serveMediaCacheEntry({ + + let parsed = parseCaptureSpecParams(searchParams); + if ('error' in parsed) { + return badRequest({ message: parsed.error.message, requestContext }); + } + + // The cache key pins the instance's own index generation: an edit bumps + // it, so an edited card can never serve a stale capture, and an + // unchanged card is a pure ledger hit with zero Chrome work. + let entryKey: MediaCacheEntryKey = { + realmURL: this.url, + sourceURL: instanceURL.href, + captureSpecHash: await captureSpecHash(parsed.spec), + sourceGeneration: Number(instance.generation), + }; + let entry = await findMediaCacheEntry(this.#dbAdapter, entryKey); + if (entry) { + // A hit costs zero Chrome work, so hits serve regardless of the + // realm's capture gate — including captures a write-holder published + // via POST on a gated realm. + return await serveMediaCacheEntry({ + request, + requestContext, + entry, + mediaCacheAdapter: this.#mediaCacheAdapter, + dbAdapter: this.#dbAdapter, + }); + } + return await this.captureScreenshotOnDemand( request, requestContext, - entry, - mediaCacheAdapter: this.#mediaCacheAdapter, - dbAdapter: this.#dbAdapter, + entryKey, + parsed.spec, + ); + } + + // The miss path: a capture no ledger entry satisfies. Full captureSpec + // power on an unauthenticated-reachable GET is an unbounded spec space, so + // new captures are per-realm opt-in (`allowArbitraryScreenshots` on the + // realm's config card — the gate blocks Chrome work, never serving), and + // an open realm's captures run through the same per-realm serialized + // screenshot queue as the POST endpoint, bounded by a sync-wait budget: + // - lane already too deep for the budget → immediate 503 + Retry-After + // (fail fast instead of holding a doomed connection); + // - otherwise enqueue and wait up to the budget; the job persists its + // capture to the MediaCache itself, so a wait that times out (503 + + // Retry-After) still lands the capture and the client's retry is a + // pure ledger hit. + private async captureScreenshotOnDemand( + request: Request, + requestContext: RequestContext, + entryKey: MediaCacheEntryKey, + spec: CaptureSpec, + ): Promise { + if (!(await this.allowsArbitraryScreenshots())) { + return responseWithError( + new CardError( + `This realm does not allow arbitrary screenshot captures: set "allowArbitraryScreenshots" to true on the realm's config card to enable them. Captures that already exist still serve.`, + { status: 403 }, + ), + requestContext, + ); + } + + let concurrencyGroup = `screenshot:${this.url}`; + let estimate = await estimateScreenshotQueueWait( + this.#dbAdapter, + concurrencyGroup, + ); + if (estimate.estimatedWaitMs > this.#screenshotSyncWaitMs) { + return this.screenshotRetryLater( + requestContext, + estimate.estimatedWaitMs, + ); + } + + // Render as the realm's owner — the same identity an index pass renders + // 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( + { + realmURL: this.url, + realmUsername: owner, + runAs: owner, + cardId: entryKey.sourceURL, + format: spec.format, + persist: { ...entryKey, lane: 'on-demand' }, + }, + this.#queue, + this.#dbAdapter, + userInitiatedPriority, + ); + + let timeoutHandle: ReturnType | undefined; + // `const` so the symbol gets a unique-symbol type and the race result + // narrows on comparison. + const timedOut = Symbol('sync-wait-timeout'); + try { + let outcome = await Promise.race([ + job.done, + new Promise((resolve) => { + timeoutHandle = setTimeout( + () => resolve(timedOut), + this.#screenshotSyncWaitMs, + ); + timeoutHandle.unref?.(); + }), + ]); + if (outcome === timedOut) { + // The job keeps running and persists its own capture; the retry + // hint is one average capture, since this request is now at the + // front of the lane. + return this.screenshotRetryLater( + requestContext, + Math.max(estimate.avgCaptureMs, 1000), + ); + } + // Prefer the ledger entry the job persisted; fall back to persisting + // here from the response for a worker that has no store configured. + let entry = await findMediaCacheEntry(this.#dbAdapter, entryKey); + if (!entry && outcome.status === 'ready' && outcome.base64) { + let binary = atob(outcome.base64); + let bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) { + bytes[i] = binary.charCodeAt(i); + } + await putMedia(this.#dbAdapter, this.#mediaCacheAdapter!, { + ...entryKey, + bytes, + contentType: outcome.contentType ?? 'image/png', + lane: 'on-demand', + }); + entry = await findMediaCacheEntry(this.#dbAdapter, entryKey); + } + if (!entry) { + return systemError({ + requestContext, + message: `screenshot capture failed for ${entryKey.sourceURL}`, + additionalError: outcome.error + ? new Error(String(outcome.error)) + : undefined, + }); + } + return await serveMediaCacheEntry({ + request, + requestContext, + entry, + mediaCacheAdapter: this.#mediaCacheAdapter!, + dbAdapter: this.#dbAdapter, + }); + } finally { + if (timeoutHandle) { + clearTimeout(timeoutHandle); + } + } + } + + private screenshotRetryLater( + requestContext: RequestContext, + estimatedWaitMs: number, + ): Response { + return createResponse({ + body: null, + init: { + status: 503, + headers: { + 'retry-after': String(Math.max(1, Math.ceil(estimatedWaitMs / 1000))), + }, + }, + requestContext, }); } - // Resolution seam for the route's addressing forms: `name=` resolves - // through the instance's declared-screenshot manifest, and capture-spec - // params resolve through canonicalization to a ledger entry (see - // `findMediaCacheEntry`). This method is where those resolvers plug in; - // with none available, every request is an uncaptured miss. - private async resolveScreenshotEntry( - _instanceURL: URL, - _searchParams: URLSearchParams, - ): Promise { - return undefined; + // The per-realm opt-in for GET-triggered captures, read from the realm's + // indexed config card on every check — so a `.realm.json` edit takes + // effect with its own index update, with no restart and no cache to + // invalidate. Absent, unindexed, or anything but `true` all read as + // gated. + private async allowsArbitraryScreenshots(): Promise { + let realmConfigCardURL = new URL( + this.paths.fileURL('realm.json').href.replace(/\.json$/, ''), + ); + let entry = await this.#realmIndexQueryEngine.instance(realmConfigCardURL); + if (entry?.type !== 'instance') { + return false; + } + return entry.instance.attributes?.allowArbitraryScreenshots === true; } private async serveLocalFile( diff --git a/packages/runtime-common/tasks/screenshot-card.ts b/packages/runtime-common/tasks/screenshot-card.ts index 55987d6b117..baafbcb8aef 100644 --- a/packages/runtime-common/tasks/screenshot-card.ts +++ b/packages/runtime-common/tasks/screenshot-card.ts @@ -6,17 +6,37 @@ import { fetchRealmPermissions, fetchUserPermissions, jobIdentity, + putMedia, + type MediaCacheLane, type ScreenshotPrerenderResponse, ensureFullMatrixUserId, ensureTrailingSlash, } from '../index.ts'; +// The ledger identity a capture persists under (see +// `ScreenshotCardArgs.persist`). +export interface ScreenshotPersistArgs extends JSONTypes.Object { + realmURL: string; + sourceURL: string; + captureSpecHash: string; + sourceGeneration: number; + lane: MediaCacheLane; +} + export interface ScreenshotCardArgs extends JSONTypes.Object { realmURL: string; realmUsername: string; runAs: string; cardId: string; format: 'isolated' | 'embedded'; + // When non-null, a successful capture is persisted to the MediaCache under + // this ledger identity before the job resolves. This is what makes a + // bounded-wait caller (the GET `_screenshot/` route's sync wait) safe to + // give up on: the capture still lands durably, and the caller's retry is + // a pure ledger hit instead of a second render. Non-optional `| null` + // rather than `?:` because the args are a `JSONTypes.Object`, whose index + // signature rejects `undefined`. + persist: ScreenshotPersistArgs | null; } export { screenshotCard }; @@ -28,9 +48,10 @@ const screenshotCard: Task = ({ prerenderer, createPrerenderAuth, matrixURL, + mediaCacheAdapter, }) => async function (args) { - let { jobInfo, realmURL, runAs, cardId, format } = args; + let { jobInfo, realmURL, runAs, cardId, format, persist } = args; log.debug( `${jobIdentity(jobInfo)} starting screenshot-card for job: ${JSON.stringify( { @@ -85,7 +106,40 @@ const screenshotCard: Task = ({ format, priority: jobInfo?.priority, }); + // The local (in-process) prerenderer resolves to `{response, timings, + // pool}` while the remote one resolves to the bare response; unwrap so + // the job result is one shape either way. + let response: ScreenshotPrerenderResponse = + (result as any)?.response ?? result; + + if (persist && response.status === 'ready' && response.base64) { + if (!mediaCacheAdapter) { + log.warn( + `${jobIdentity(jobInfo)} screenshot-card asked to persist but this worker has no media cache adapter configured; skipping`, + ); + } else { + // Persist failure must not fail the capture: the response still + // carries the bytes, so the caller can serve (or store) them itself. + try { + let binary = atob(response.base64); + let bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) { + bytes[i] = binary.charCodeAt(i); + } + await putMedia(dbAdapter, mediaCacheAdapter, { + ...persist, + bytes, + contentType: response.contentType ?? 'image/png', + }); + } catch (e: any) { + log.error( + `${jobIdentity(jobInfo)} screenshot-card failed to persist capture to the media cache`, + e, + ); + } + } + } reportStatus(jobInfo, 'finish'); - return result; + return response; }; From ecdfe3335134c89f23b3559e6f5bdd7c7007cd53 Mon Sep 17 00:00:00 2001 From: Luke Melia Date: Thu, 20 Aug 2026 19:01:30 -0400 Subject: [PATCH 2/2] Address review: coalesce duplicate captures, scope the gate doc to the ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/base/realm-config.gts | 8 +- packages/postgres/pg-queue.ts | 1 + .../tests/media-cache-dsl-test.ts | 45 +++++++++ .../runtime-common/jobs/screenshot-card.ts | 95 ++++++++++++++++++- packages/runtime-common/realm.ts | 4 +- 5 files changed, 145 insertions(+), 8 deletions(-) diff --git a/packages/base/realm-config.gts b/packages/base/realm-config.gts index ecf639576c2..e34ca317c79 100644 --- a/packages/base/realm-config.gts +++ b/packages/base/realm-config.gts @@ -688,10 +688,10 @@ export class RealmConfig extends CardDef { // for arbitrary capture specs. Full captureSpec power on a GET is an // 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 - // realm's indexed config at request time, so editing this takes effect - // with the index update, no restart. + // serving — any capture whose canonical spec already has a MediaCache + // ledger entry streams regardless. Read from the realm's indexed config + // at request time, so editing this takes effect with the index update, + // no restart. @field allowArbitraryScreenshots = contains(BooleanField); @field cardTitle = contains(StringField, { diff --git a/packages/postgres/pg-queue.ts b/packages/postgres/pg-queue.ts index dce2880c5b9..1648b1c1375 100644 --- a/packages/postgres/pg-queue.ts +++ b/packages/postgres/pg-queue.ts @@ -32,6 +32,7 @@ import { FROM_SCRATCH_JOB_TIMEOUT_SEC } from '@cardstack/runtime-common/tasks/in // Side-effect imports: these modules call registerQueueJobDefinition() at // load time, so any process that constructs a PgQueuePublisher gets the // coalesce handlers registered before publish() is called. +import '@cardstack/runtime-common/jobs/screenshot-card'; import '@cardstack/runtime-common/tasks/copy'; import '@cardstack/runtime-common/tasks/full-reindex'; import '@cardstack/runtime-common/tasks/media-cache-gc'; diff --git a/packages/realm-server/tests/media-cache-dsl-test.ts b/packages/realm-server/tests/media-cache-dsl-test.ts index 56d485ebd01..d4d05d550ca 100644 --- a/packages/realm-server/tests/media-cache-dsl-test.ts +++ b/packages/realm-server/tests/media-cache-dsl-test.ts @@ -391,6 +391,51 @@ module(basename(import.meta.filename), function () { ); }); + test('concurrent misses for one spec coalesce onto one capture', async function (assert) { + await seedInstanceRow('card-1'); + await seedRealmConfigRow(true); + // Recent capture history keeps the congestion pre-check's estimate + // under the budget while the first capture is in flight, so the + // second request reaches the queue and can coalesce instead of + // failing fast. + let job = await insertJob(dbAdapter, { + job_type: 'screenshot-card', + concurrency_group: `screenshot:${REALM_URL}`, + status: 'resolved', + finished_at: new Date().toISOString(), + result: {}, + }); + await query(dbAdapter, [ + `INSERT INTO job_reservations (job_id, created_at, locked_until, completed_at, worker_id) + VALUES (${Number(job.id)}, NOW() - INTERVAL '200 milliseconds', NOW(), NOW(), 'test-worker')`, + ]); + captureGate = new Deferred(); + await startWorker(); + + let first = get('_screenshot/card-1'); + // Wait for the first capture to be claimed and parked on the gate so + // the second request's publish sees it as an in-flight twin. + let deadline = Date.now() + 5000; + while (captureCalls === 0 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + let second = get('_screenshot/card-1'); + // Give the second request time to publish (and coalesce) before the + // render completes. + await new Promise((resolve) => setTimeout(resolve, 100)); + captureGate.fulfill(); + captureGate = undefined; + + let [firstResponse, secondResponse] = await Promise.all([first, second]); + assert.strictEqual(firstResponse.status, 200); + assert.strictEqual(secondResponse.status, 200); + assert.strictEqual( + captureCalls, + 1, + 'both requests were satisfied by one render', + ); + }); + test('a congested lane fails fast with 503 + Retry-After', async function (assert) { await seedInstanceRow('card-1'); await seedRealmConfigRow(true); diff --git a/packages/runtime-common/jobs/screenshot-card.ts b/packages/runtime-common/jobs/screenshot-card.ts index bc4f3a8d0ad..5e7757eaa5c 100644 --- a/packages/runtime-common/jobs/screenshot-card.ts +++ b/packages/runtime-common/jobs/screenshot-card.ts @@ -1,10 +1,101 @@ import { param, query, type Expression } from '../expression.ts'; -import type { QueuePublisher } from '../queue.ts'; +import { + registerQueueJobDefinition, + type QueueCoalesceContext, + type QueueCoalesceDecision, + type QueuePublisher, +} from '../queue.ts'; import type { ScreenshotPrerenderResponse, DBAdapter } from '../index.ts'; -import type { ScreenshotCardArgs } from '../tasks/screenshot-card.ts'; +import type { + ScreenshotCardArgs, + ScreenshotPersistArgs, +} from '../tasks/screenshot-card.ts'; export const SCREENSHOT_CARD_JOB_TIMEOUT_SEC = 60; +// Concurrent requests for one capture fold onto one job: the per-realm +// concurrency group serializes execution but does not dedupe, so without +// this two simultaneous misses for the same spec would each run a full +// render (the store's dedupe-on-write only saves the second upload, not the +// Chrome work). A twin must match the whole capture identity — card, format, +// render identity, and persist target — since joining hands the incoming +// caller the twin's result verbatim. Queued and in-flight twins both join; +// an in-flight join just registers a late waiter on the running job. +function chooseScreenshotCardCoalesceDecision( + context: QueueCoalesceContext, +): QueueCoalesceDecision { + let { incoming, candidates, inFlightCandidates } = context; + let incomingArgs = parseScreenshotCardArgs(incoming.args); + if (!incomingArgs) { + return { type: 'insert' }; + } + let twin = [...candidates, ...inFlightCandidates].find((candidate) => { + if (candidate.jobType !== incoming.jobType) { + return false; + } + let candidateArgs = parseScreenshotCardArgs(candidate.args); + return ( + candidateArgs !== undefined && + candidateArgs.cardId === incomingArgs.cardId && + candidateArgs.format === incomingArgs.format && + candidateArgs.runAs === incomingArgs.runAs && + samePersist(candidateArgs.persist, incomingArgs.persist) + ); + }); + if (!twin) { + return { type: 'insert' }; + } + return { type: 'join', jobId: twin.id }; +} + +function parseScreenshotCardArgs( + args: unknown, +): ScreenshotCardArgs | undefined { + let obj: unknown = args; + if (typeof args === 'string') { + try { + obj = JSON.parse(args); + } catch { + return undefined; + } + } + if (!obj || typeof obj !== 'object' || Array.isArray(obj)) { + return undefined; + } + let { cardId, format, runAs } = obj as Record; + if ( + typeof cardId !== 'string' || + typeof format !== 'string' || + typeof runAs !== 'string' + ) { + return undefined; + } + return obj as ScreenshotCardArgs; +} + +// Field-by-field rather than JSON.stringify: one side round-trips through +// jsonb, which does not preserve key order. +function samePersist( + a: ScreenshotPersistArgs | null | undefined, + b: ScreenshotPersistArgs | null | undefined, +): boolean { + if (!a || !b) { + return !a && !b; + } + return ( + a.realmURL === b.realmURL && + a.sourceURL === b.sourceURL && + a.captureSpecHash === b.captureSpecHash && + a.sourceGeneration === b.sourceGeneration && + a.lane === b.lane + ); +} + +registerQueueJobDefinition({ + jobType: 'screenshot-card', + coalesce: chooseScreenshotCardCoalesceDecision, +}); + // How long a GET `_screenshot/` request holds its connection waiting for an // on-demand capture before answering 503 + Retry-After. A cost-posture // bound, not a transport one (the realm-server ALB's idle timeout is far diff --git a/packages/runtime-common/realm.ts b/packages/runtime-common/realm.ts index 0cac9cef4a5..471130c5c60 100644 --- a/packages/runtime-common/realm.ts +++ b/packages/runtime-common/realm.ts @@ -4095,8 +4095,8 @@ export class Realm { let entry = await findMediaCacheEntry(this.#dbAdapter, entryKey); if (entry) { // A hit costs zero Chrome work, so hits serve regardless of the - // realm's capture gate — including captures a write-holder published - // via POST on a gated realm. + // realm's capture gate — however the capture came to be in the + // ledger. return await serveMediaCacheEntry({ request, requestContext,