Skip to content

Host: stamp user priority on outbound _federated-search by default#4866

Merged
habdelra merged 4 commits into
prerender-deadlock-pagepool-priorityfrom
prerender-deadlock-user-api-priority
May 19, 2026
Merged

Host: stamp user priority on outbound _federated-search by default#4866
habdelra merged 4 commits into
prerender-deadlock-pagepool-priorityfrom
prerender-deadlock-user-api-priority

Conversation

@habdelra
Copy link
Copy Markdown
Contributor

Stacked on top of #4863. Retarget base to `main` once #4863 merges.

Why

PR #4863 plumbed `x-boxel-job-priority` through every layer of the prerender / search stack — but only the producer-side prerender tab was stamping the header on outbound `_federated-search` calls. User-facing fetches (the host SPA in a real user's browser, external API callers) sent no priority, so any sub-`prerenderModule` fired by the realm-server for a `lookupDefinition` cache miss queued at priority 0 — behind any concurrent background indexing fan-out.

User clicks driving a search are user-initiated work and should outrank background indexing on the realm-server's PagePool. This PR closes that gap.

What changes

One function in `packages/host/app/services/store.ts`: `jobPriorityHeader()`.

Policy is two-state, gated by `__boxelDuringPrerender` (the same flag `duringPrerenderHeaders` already reads), not by the presence of `__boxelJobPriority`:

  1. Inside a prerender tab (`__boxelDuringPrerender === true`): forward the worker job's priority as-is. The render-runner sets `__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 (older render-runner build, test fixture), default to 0 — safe 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 that drive a search should outrank background fan-out.

External API callers needing a different priority (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."

Why gate on `__boxelDuringPrerender` and not on the presence of `__boxelJobPriority`

A prerender tab whose render-runner build predates the priority global (or a test fixture that doesn't set it) should still be treated as prerender-context (default 0), not as live user traffic (default 10). Gating on the prerender flag rather than the priority global gets that right.

Behavioral impact

User-facing searches:

  • Realm-server's `handle-search` reads the header, sets `opts.priority = 10` on the search.
  • `RealmIndexQueryEngine.lookupDefinitionForOpts` threads that into `lookupDefinition(codeRef, { priority: 10 })`.
  • If the definition is in the modules table → cache hit, no behavior change.
  • If the definition is missing → `CachingDefinitionLookup` fires a `prerenderModule` at priority 10. PagePool routes it ahead of any priority-0 background work.

Indexer-driven searches (the deadlock scenario PR #4863 targets):

External API callers (e.g. `sync-openrouter-models.ts`):

  • Stamps userInitiatedPriority (10) by default. If a batch tool wants priority 0, set `globalThis.__boxelJobPriority = 0` before issuing the request.

Tests

No new tests added in this PR — the change is a single function with a 2-line policy on top of the priority machinery PR #4863 already adds (and tests). Existing tests in `packages/realm-server/tests/definition-lookup-test.ts` cover the priority threading end-to-end on the indexer side.

Test plan

  • CI passes
  • After deploy: trigger a user-initiated search from the host UI; tail prerender-server logs for the resulting `prerenderModule` (if any) — confirm `priority=10` rather than `priority=0`

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
@habdelra habdelra requested a review from Copilot May 18, 2026 19:55
@habdelra
Copy link
Copy Markdown
Contributor Author

habdelra commented May 18, 2026

This is missing tests. i don't buy the comment that existing tests cover this, since we just added this functioanlity. We should at the very least assert the user initiated priority when not in rendering context

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the host SPA’s _federated-search request headers so user-initiated searches default to a higher queue priority on the realm-server’s PagePool, while prerender-context work preserves (or defaults) to system priority.

Changes:

  • Import userInitiatedPriority and stamp x-boxel-job-priority on outbound _federated-search requests by default when not in a prerender tab.
  • Gate behavior on globalThis.__boxelDuringPrerender: forward the prerender job’s priority (defaulting to 0 if missing/malformed) vs. default to userInitiatedPriority outside prerender.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/host/app/services/store.ts Outdated
Comment thread packages/host/app/services/store.ts Outdated
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) <noreply@anthropic.com>
@habdelra
Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] You're right, sorry — that excuse didn't hold up. Added packages/host/tests/unit/job-priority-header-test.ts in 2b74fc2 with focused coverage:

Outside prerender (the case you specifically called out):

  • No globals → userInitiatedPriority
  • __boxelDuringPrerender: falseuserInitiatedPriority
  • Truthy-but-non-true __boxelDuringPrerender ('yes', 1) → still userInitiatedPriority (covers the strict-=== true check from Copilot's other comment)

Inside prerender:

  • Explicit priority 10 → forwards 10
  • Explicit priority 0 → forwards 0 (must not upgrade — preserves system-initiated indexing)
  • Missing __boxelJobPriority → 0 (safe default)

Edge cases:

  • Explicit override outside prerender is honored (e.g. __boxelJobPriority = 3 → 3)
  • Malformed values (-1, 1.5, '10') fall through to the branch's default

Refactored the inline policy logic into an exported pure resolver resolveOutboundJobPriority so the contract can be pinned without acceptance-test scaffolding. The jobPriorityHeader wrapper just reads the globals and calls the resolver.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Preview deployments

Host Test Results

    1 files  ±    0      1 suites  ±0   1h 47m 14s ⏱️ - 1h 28m 9s
2 673 tests +    4  2 658 ✅ +    4  15 💤 ± 0  0 ❌ ±0 
2 692 runs   - 2 494  2 677 ✅  - 2 479  15 💤  - 15  0 ❌ ±0 

Results for commit 90bea13. ± Comparison against earlier commit 18f5504.

Realm Server Test Results

    1 files  ± 0      1 suites  ±0   8m 3s ⏱️ -20s
1 414 tests + 9  1 414 ✅ + 9  0 💤 ±0  0 ❌ ±0 
1 501 runs  +12  1 501 ✅ +12  0 💤 ±0  0 ❌ ±0 

Results for commit 90bea13. ± Comparison against earlier commit 18f5504.

…rity' into prerender-deadlock-user-api-priority
@habdelra habdelra requested a review from a team May 18, 2026 21:34
…rity' into prerender-deadlock-user-api-priority

# Conflicts:
#	packages/host/app/services/store.ts
@burieberry
Copy link
Copy Markdown
Contributor

The percy errors may have been caused by my topbar header refactor as part of #4626

@habdelra habdelra merged commit 572f941 into prerender-deadlock-pagepool-priority May 19, 2026
74 of 75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants