From 6a45c7aa18b28511010833a95524fa43910749f6 Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Mon, 18 May 2026 15:47:56 -0400 Subject: [PATCH 1/2] Host: stamp user priority on outbound _federated-search by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Companion change to PR #4863. That PR plumbed `x-boxel-job-priority` through every layer of the prerender / search stack, but only the producer-side prerender tab was stamping the header — user-facing fetches (the host SPA in a real user's browser, external API callers) sent no priority, so any sub-`prerenderModule` fired by `_federated-search` for a `lookupDefinition` cache miss queued behind concurrent background indexing at priority 0. User clicks that drive search are user-initiated work and should outrank background fan-out. Policy on the outbound `jobPriorityHeader()`: 1. Inside a prerender tab (`__boxelDuringPrerender === true`): forward `__boxelJobPriority` as-is, including 0. The render-runner sets the global from the worker job's priority; a 0 is meaningful (the originating job is system-initiated indexing) and must not be upgraded. Falls back to 0 if the global is absent. 2. Outside a prerender tab: stamp `userInitiatedPriority` (10) unless the caller has explicitly set `globalThis.__boxelJobPriority` to something else. Why gate on `__boxelDuringPrerender` rather than the presence of `__boxelJobPriority`: a prerender tab whose render-runner build predates the priority global, or a test fixture that didn't set it, should still be treated as prerender-context (default 0), not as live user traffic (default 10). Stacked on top of #4863 — that PR adds `X_BOXEL_JOB_PRIORITY_HEADER` and the `userInitiatedPriority` constant import is already wired through. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/host/app/services/store.ts | 53 ++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/packages/host/app/services/store.ts b/packages/host/app/services/store.ts index 65a4eb3477..41a51eef2c 100644 --- a/packages/host/app/services/store.ts +++ b/packages/host/app/services/store.ts @@ -36,6 +36,7 @@ import { X_BOXEL_CONSUMING_REALM_HEADER, X_BOXEL_JOB_ID_HEADER, X_BOXEL_JOB_PRIORITY_HEADER, + userInitiatedPriority, Deferred, delay, mergeRelationships, @@ -161,17 +162,53 @@ function jobIdHeader(): Record { return j ? { [X_BOXEL_JOB_ID_HEADER]: j } : {}; } -// Companion to `jobIdHeader()`. The prerender server's render-runner -// also injects `__boxelJobPriority` onto the page before transitioning -// into the render route. Outside a prerender tab the global is -// undefined and we send no header — user / API callers leave the -// realm-server's lookup paths to fall back to priority 0 as today. +// Companion to `jobIdHeader()`. Policy is two-state, gated by +// `__boxelDuringPrerender` (the same flag `duringPrerenderHeaders` +// reads), not by the presence of `__boxelJobPriority`: +// +// 1. Inside a prerender tab: forward the worker job's priority as-is. +// The render-runner injects `__boxelJobPriority` alongside +// `__boxelJobId` on each visit — a priority of 0 is meaningful +// (the originating job is system-initiated background indexing) +// and must be preserved, not upgraded. Sub-`prerenderModule` +// calls fired by `_federated-search` for a `lookupDefinition` +// cache miss inherit this priority so they don't outrun the +// parent. If `__boxelJobPriority` is missing here (older +// render-runner build, test fixture, etc.) treat as 0 — the +// safe default for prerender-context work. +// +// 2. Outside a prerender tab (the host SPA in a real user's browser, +// or any external API caller that isn't a prerender visit): +// stamp `userInitiatedPriority` (10). User clicks driving a +// search are by definition user-initiated work and should outrank +// background indexing on the realm-server's PagePool. Without +// this, a user search whose definition lookup misses the modules +// cache would fire its sub-prerender at priority 0 and queue +// behind concurrent indexing fan-out. +// +// External API callers that need a different priority (e.g. batch +// tooling that wants explicit routing) can override +// `globalThis.__boxelJobPriority` themselves — outside a prerender +// tab the policy is still "stamp what's there, default to user +// priority." function jobPriorityHeader(): Record { + let duringPrerender = ( + globalThis as unknown as { __boxelDuringPrerender?: boolean } + ).__boxelDuringPrerender; let p = (globalThis as unknown as { __boxelJobPriority?: number }) .__boxelJobPriority; - return typeof p === 'number' && Number.isSafeInteger(p) && p >= 0 - ? { [X_BOXEL_JOB_PRIORITY_HEADER]: String(p) } - : {}; + let valid = + typeof p === 'number' && Number.isSafeInteger(p) && p >= 0 ? p : undefined; + if (duringPrerender) { + // Forward as-is, including 0. Fall back to 0 if the global is + // absent or malformed. + return { [X_BOXEL_JOB_PRIORITY_HEADER]: String(valid ?? 0) }; + } + // Outside prerender: user-initiated unless the caller overrode the + // global with their own value. + return { + [X_BOXEL_JOB_PRIORITY_HEADER]: String(valid ?? userInitiatedPriority), + }; } const queryFieldSeedFromSearchSymbol = Symbol.for( 'cardstack-query-field-seed-from-search', From 2b74fc22d19ed9397326d5a5bfd61bb098c80a1b Mon Sep 17 00:00:00 2001 From: Hassan Abdel-Rahman Date: Mon, 18 May 2026 16:05:06 -0400 Subject: [PATCH 2/2] Address review: tighten === true check, fix doc comment, add unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from the Copilot review + Hassan's comment on PR #4866. 1. (Copilot) Reword doc comment to not imply external HTTP callers can affect this global. The helper only runs inside the host SPA's JS runtime. External HTTP callers — anything not in the host SPA — bypass the helper entirely and set `X-Boxel-Job-Priority` directly on their request if they care. 2. (Copilot) Tighten gating from truthy `if (duringPrerender)` to `if (duringPrerender === true)`. The global is typed as a boolean; a stray truthy string from a future code path shouldn't silently flip the policy from "user priority" to "preserve 0" — a real user-facing fetch would then queue behind background indexing. Strict-equality is the safer default. 3. (Hassan) Pin the policy with unit tests. Refactored the inline policy logic into an exported pure helper `resolveOutboundJobPriority({duringPrerender, jobPriority})` and wrote `tests/unit/job-priority-header-test.ts` covering: Outside prerender (user / API caller): - No globals → returns `userInitiatedPriority` - `__boxelDuringPrerender = false` → `userInitiatedPriority` - Explicit `__boxelJobPriority` override is honored (3 → 3, 0 → 0; 0 is NOT coerced to user priority) - Truthy but non-`true` `__boxelDuringPrerender` (`'yes'`, `1`) does NOT trigger forward-as-0 — guards against the issue above Inside prerender: - Explicit 10 → 10 - Explicit 0 → 0 (must not upgrade; preserves system-initiated) - Missing `__boxelJobPriority` → 0 (safe default for prerender- context work) - Malformed values (-1, 1.5, '10') fall through to the active branch's default `jobPriorityHeader` (the wrapper that reads the globals) is now a thin shim that calls `resolveOutboundJobPriority` with the actual globals. No behavioral change at the call sites. Note: I considered tightening `duringPrerenderHeaders()` similarly per Copilot's "and similarly in related helpers" suggestion, but left it alone — changing the existing `X-Boxel-During-Prerender` gating is broader scope than this PR and would change observable behavior at the realm-server's cache-only-definitions gate. Worth a separate cleanup if we want strict-boolean across all the related helpers. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/host/app/services/store.ts | 66 +++++--- .../tests/unit/job-priority-header-test.ts | 150 ++++++++++++++++++ 2 files changed, 194 insertions(+), 22 deletions(-) create mode 100644 packages/host/tests/unit/job-priority-header-test.ts diff --git a/packages/host/app/services/store.ts b/packages/host/app/services/store.ts index 41a51eef2c..fc447da681 100644 --- a/packages/host/app/services/store.ts +++ b/packages/host/app/services/store.ts @@ -177,8 +177,7 @@ function jobIdHeader(): Record { // render-runner build, test fixture, etc.) treat as 0 — the // safe default for prerender-context work. // -// 2. Outside a prerender tab (the host SPA in a real user's browser, -// or any external API caller that isn't a prerender visit): +// 2. Outside a prerender tab (the host SPA in a real user's browser): // stamp `userInitiatedPriority` (10). User clicks driving a // search are by definition user-initiated work and should outrank // background indexing on the realm-server's PagePool. Without @@ -186,28 +185,51 @@ function jobIdHeader(): Record { // cache would fire its sub-prerender at priority 0 and queue // behind concurrent indexing fan-out. // -// External API callers that need a different priority (e.g. batch -// tooling that wants explicit routing) can override -// `globalThis.__boxelJobPriority` themselves — outside a prerender -// tab the policy is still "stamp what's there, default to user -// priority." -function jobPriorityHeader(): Record { - let duringPrerender = ( - globalThis as unknown as { __boxelDuringPrerender?: boolean } - ).__boxelDuringPrerender; - let p = (globalThis as unknown as { __boxelJobPriority?: number }) - .__boxelJobPriority; +// External (non-host) HTTP callers — anything that doesn't run in +// the host SPA's JS runtime — bypass this helper entirely and set +// `X-Boxel-Job-Priority` directly on their request if they care. +// This helper covers the host SPA only. +// +// Both globals are checked with `=== true` / strict-number rather +// than truthy coercion: `__boxelDuringPrerender` is typed as a +// boolean and a stray truthy string from a future code path +// shouldn't silently flip the policy from "user-priority" to +// "preserve 0." +// Pure resolver — exported for the unit test in +// `tests/integration/job-priority-header-test.ts`. See the comment +// above for the policy rationale; the function is the literal +// translation of that policy to numbers. +export function resolveOutboundJobPriority({ + duringPrerender, + jobPriority, +}: { + duringPrerender: unknown; + jobPriority: unknown; +}): number { let valid = - typeof p === 'number' && Number.isSafeInteger(p) && p >= 0 ? p : undefined; - if (duringPrerender) { - // Forward as-is, including 0. Fall back to 0 if the global is - // absent or malformed. - return { [X_BOXEL_JOB_PRIORITY_HEADER]: String(valid ?? 0) }; - } - // Outside prerender: user-initiated unless the caller overrode the - // global with their own value. + typeof jobPriority === 'number' && + Number.isSafeInteger(jobPriority) && + jobPriority >= 0 + ? jobPriority + : undefined; + if (duringPrerender === true) { + return valid ?? 0; + } + return valid ?? userInitiatedPriority; +} + +function jobPriorityHeader(): Record { + let g = globalThis as unknown as { + __boxelDuringPrerender?: boolean; + __boxelJobPriority?: number; + }; return { - [X_BOXEL_JOB_PRIORITY_HEADER]: String(valid ?? userInitiatedPriority), + [X_BOXEL_JOB_PRIORITY_HEADER]: String( + resolveOutboundJobPriority({ + duringPrerender: g.__boxelDuringPrerender, + jobPriority: g.__boxelJobPriority, + }), + ), }; } const queryFieldSeedFromSearchSymbol = Symbol.for( diff --git a/packages/host/tests/unit/job-priority-header-test.ts b/packages/host/tests/unit/job-priority-header-test.ts new file mode 100644 index 0000000000..bc2cbd9e69 --- /dev/null +++ b/packages/host/tests/unit/job-priority-header-test.ts @@ -0,0 +1,150 @@ +import { module, test } from 'qunit'; + +import { userInitiatedPriority } from '@cardstack/runtime-common'; + +import { resolveOutboundJobPriority } from '@cardstack/host/services/store'; + +// Pure-resolver tests for the policy that decides what +// `X-Boxel-Job-Priority` value the host SPA stamps on outbound +// `_federated-search` calls. The function is module-internal logic +// extracted so its policy can be pinned without acceptance-test +// scaffolding. +// +// Two states gated by `__boxelDuringPrerender`: +// - inside prerender → forward (preserve 0) +// - outside prerender → user-initiated (10) by default +module('Unit | job-priority-header | resolveOutboundJobPriority', function () { + module('outside a prerender tab (user / API caller)', function () { + test('returns userInitiatedPriority when no global is set', function (assert) { + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: undefined, + jobPriority: undefined, + }), + userInitiatedPriority, + ); + }); + + test('returns userInitiatedPriority when __boxelDuringPrerender is false', function (assert) { + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: false, + jobPriority: undefined, + }), + userInitiatedPriority, + ); + }); + + test('honors an explicit override on __boxelJobPriority', function (assert) { + // Batch / scripting tooling running in the host SPA can set the + // global before issuing a fetch; outside a prerender tab we still + // forward what they set rather than overriding to user priority. + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: undefined, + jobPriority: 3, + }), + 3, + ); + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: false, + jobPriority: 0, + }), + 0, + 'override with 0 is preserved (not coerced to user priority)', + ); + }); + + test('rejects a truthy but non-boolean __boxelDuringPrerender — uses strict === true', function (assert) { + // If `__boxelDuringPrerender` somehow ended up as a stringy + // truthy value (e.g. set by a future code path that didn't + // coerce), the policy must NOT silently flip to "forward 0"; + // a real user-facing fetch would then queue behind background + // indexing. The check is `=== true` for exactly this reason. + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: 'yes', + jobPriority: undefined, + }), + userInitiatedPriority, + ); + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: 1, + jobPriority: undefined, + }), + userInitiatedPriority, + ); + }); + }); + + module('inside a prerender tab', function () { + test('forwards an explicit __boxelJobPriority of 10', function (assert) { + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: 10, + }), + 10, + ); + }); + + test('forwards an explicit __boxelJobPriority of 0 — must NOT upgrade', function (assert) { + // System-initiated indexing has priority 0. A + // `_federated-search` fired by the card render must preserve + // that or its sub-prerenders would outrank the parent job. + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: 0, + }), + 0, + ); + }); + + test('defaults to 0 when __boxelJobPriority is missing (older render-runner / test fixture)', function (assert) { + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: undefined, + }), + 0, + ); + }); + + test('rejects malformed __boxelJobPriority values', function (assert) { + // Non-number / negative / non-integer values fall through to + // the default for the active branch. + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: -1, + }), + 0, + ); + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: 1.5, + }), + 0, + ); + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: true, + jobPriority: '10', + }), + 0, + ); + assert.strictEqual( + resolveOutboundJobPriority({ + duringPrerender: false, + jobPriority: -1, + }), + userInitiatedPriority, + 'malformed value outside prerender → user-initiated default', + ); + }); + }); +});