CS-11123: Unwind cacheOnlyDefinitions workaround#4801
Conversation
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 2h 2m 15s ⏱️ + 16m 57s Results for commit 550d864. ± Comparison against earlier commit a05a3ee. Realm Server Test Results 1 files ±0 1 suites ±0 12m 8s ⏱️ +4s Results for commit 177a0f2. ± Comparison against earlier commit 550d864. |
There was a problem hiding this comment.
Pull request overview
This PR removes the now-obsolete “cache-only definitions during prerender” workaround and associated prerender-detection plumbing, based on the assumption that the modules table is pre-warmed ahead of renders so lookupDefinition no longer needs to trigger nested prerenders during indexing/search.
Changes:
- Removes
cacheOnlyDefinitionssupport end-to-end (API surface, options threading, and the meta-based short-circuit path) so searches always use the normal definition lookup path. - Deletes the prerender marker/header mechanism (
DURING_PRERENDER_HEADER, host-side header injection, and PagePool’sevaluateOnNewDocumentglobal injection). - Simplifies realm-server federated search handling to always call
searchRealms(realms, query)with no prerender-specific behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/search-utils.ts | Removes opts parameter from SearchableRealm.search and searchRealms(). |
| packages/runtime-common/realm.ts | Drops DURING_PRERENDER_HEADER + prerender detection; simplifies Realm.search() signature and usage. |
| packages/runtime-common/realm-index-query-engine.ts | Removes cacheOnlyDefinitions option and all cache-only/meta short-circuit branches; always uses live lookupDefinition. |
| packages/runtime-common/index-runner.ts | Updates explanatory comment describing why pre-warm exists (removes cache-only reference). |
| packages/realm-server/prerender/prerender-constants.ts | Removes re-export of the prerender marker header constant. |
| packages/realm-server/prerender/page-pool.ts | Removes prerender tab global injection (#markPageAsInPrerender) and its call sites. |
| packages/realm-server/handlers/handle-search.ts | Removes prerender-header handling; always federates search without cache-only mode. |
| packages/host/app/services/store.ts | Removes prerender header injection on _federated-search requests and the related helper/import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d1edacc to
483500f
Compare
f17b9a1 to
e6a0a7f
Compare
483500f to
76f6394
Compare
e6a0a7f to
209a065
Compare
76f6394 to
a05a3ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a05a3eed61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
209a065 to
d386c14
Compare
a05a3ee to
550d864
Compare
With pre-warm populating the modules table before every render fires,
`lookupDefinition` from inside `populateQueryFields` always hits a
populated DB row instead of spawning a nested prerender. The
`cacheOnlyDefinitions:true` short-circuit and the `__boxelDuringPrerender`
header plumbing become dead code in the indexing flow — remove them.
Removed pieces:
- `DURING_PRERENDER_HEADER` constant and `isDuringPrerenderRequest`
helper in `runtime-common/realm.ts`, the matching re-export in
`realm-server/prerender/prerender-constants.ts`, and the `import`
in `realm-server/handlers/handle-search.ts`.
- Host SPA's `duringPrerenderHeaders()` helper and both call sites in
`host/app/services/store.ts` (the two `_federated-search` fetches).
- Prerender page-pool's `#markPageAsInPrerender` (which injected
`globalThis.__boxelDuringPrerender = true` via
`evaluateOnNewDocument`) and its two call sites.
- `handle-search.ts`'s `cacheOnlyDefinitions = ctxt.get(...).length > 0`
branch; the search handler now just calls `searchRealms(realms, query)`.
- `Realm.search`'s `opts?: { cacheOnlyDefinitions?: boolean }` parameter
and the conditional spread on `searchCards` opts; `searchResponse`
no longer threads `cacheOnlyDefinitions` through.
- `searchRealms` / `SearchableRealm.search` lose the `opts` parameter.
- `RealmIndexQueryEngine.Options.cacheOnlyDefinitions` and its three
consumers (`fieldExpectsFileMeta`, `lookupDefinitionForOpts`,
`populateQueryFieldsFromMeta` short-circuit at the loadLinks layer).
- The `lookupDefinitionForOpts` helper itself collapses to a direct
`lookupDefinition` call — both call sites are inlined.
- The `populateQueryFieldsFromMeta` method is removed; the loadLinks
walker now always calls the live `populateQueryFields`.
The `QueryFieldMeta` type and the indexer's `queryFieldDefs` emission
in `host/app/utils/file-def-attributes-extractor.ts` are left alone —
they're still written into `resource.meta` and consumed elsewhere as
metadata; only the cache-only-read short-circuit they served is gone.
Gate: piranha must still complete after the unwind. If a regression
appears, the pre-warm closure is missing a module URL the renders
need — fix the pre-warm closure (signal sources, deps walk) rather
than re-adding the workaround.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
550d864 to
177a0f2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 177a0f27ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| await this.populateQueryFields(resource, realmURL, popOpts); | ||
| } | ||
| await this.populateQueryFields(resource, realmURL, popOpts); |
There was a problem hiding this comment.
Keep cache-only path for un-prewarmed search results
When a prerendered card’s query field returns resources outside the current invalidation batch (for example a search over existing authors/tags), loadLinks now always calls live populateQueryFields for those returned resources. Fresh evidence beyond the earlier proxy concern is that this layer walks arbitrary search result resources, while IndexRunner.preWarmModulesTable only warms modules derived from the invalidation URLs/deps; if one of these returned resources has an uncached definition, this call can still enter lookupDefinition and spawn a nested prerender while the original prerender tab is held. Please preserve a cache-only/meta path for resources that prewarm did not cover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code] The reachability claim is consistent with empirical bench data I collected before the PR was reframed as cleanup:
| Realm | SHA | Wall (s) | filesIndexed | instancesIndexed | instanceErrors |
|---|---|---|---|---|---|
user/prudent-octopus |
pre-warm (#4799 c85633d237) |
467 | 118 | 91 | 0 |
user/prudent-octopus |
+ unwind (this PR's prior SHA 550d86490d) |
550 | 118 | 90 | 1 |
Same realm, same protocol, same rig — the unwind reintroduced a cohort-instance error that the pre-warm PR's adoptsFrom fix had resolved on octopus. loadLinks's walk over query-field-returned resources is the obvious candidate path: pre-warm only covers the invalidation batch's deps closure, so a returned existing card with an uncached definition can still hit the live populateQueryFields → lookupDefinition → prerenderModule path while the original tab is held. The 256 fed-search hits on the unwind run (vs 176 with the safety net) is consistent — every render now walks the live path instead of degrading via populateQueryFieldsFromMeta.
Hassan's reply on the earlier proxy-renders concern still applies (a single-URL render has no batch deps closure to walk, so single-URL paths can't easily piggyback on pre-warm). The same shape applies here.
Tagging Hassan (@habdelra) to decide whether to keep just the parts of the unwind that don't surface this regression (drop the populateQueryFieldsFromMeta removal at line ~1104 + the Options.cacheOnlyDefinitions consumers, leave the host/page-pool/header plumbing removal in place), defer the unwind entirely, or accept the regression on these realms.
|
Closing this PR. The audit of which parts of the unwind are actually safe-to-remove vs. load-bearing came back as: the safety net is fully connected end-to-end (page-pool injection → host header → realm-server inbound branch → The truly-cosmetic parts that could survive (DURING_PRERENDER_HEADER re-export in The honest conclusion: until pre-warm's closure is extended to cover what Ship only #4799 (pre-warm) from this stack. |
Summary
Cleanup PR. With pre-warm (#4799) populating the modules table before every render fires,
lookupDefinitionfrom insidepopulateQueryFieldsalways hits a populated DB row instead of spawning a nested prerender. ThecacheOnlyDefinitions:trueshort-circuit and the__boxelDuringPrerenderheader plumbing are no longer load-bearing in the indexing flow — remove them.This is not a performance PR — the goal is to drop the workaround scaffolding now that the root-cause fix (pre-warm) is in. No measurable wall-clock change expected.
Removed pieces:
DURING_PRERENDER_HEADERconstant +isDuringPrerenderRequesthelper inruntime-common/realm.ts; matching re-export inrealm-server/prerender/prerender-constants.ts; import inrealm-server/handlers/handle-search.ts.duringPrerenderHeaders()helper and both_federated-searchfetch call sites inhost/app/services/store.ts.#markPageAsInPrerender(which injectedglobalThis.__boxelDuringPrerender = trueviaevaluateOnNewDocument) and its two call sites.handle-search.ts'scacheOnlyDefinitions = ctxt.get(...).length > 0branch; the search handler now just callssearchRealms(realms, query).Realm.search'sopts?: { cacheOnlyDefinitions?: boolean }parameter and the conditional spread onsearchCardsopts;searchResponseno longer threadscacheOnlyDefinitionsthrough.searchRealms/SearchableRealm.searchlose theoptsparameter.RealmIndexQueryEngine.Options.cacheOnlyDefinitionsand its three consumers (fieldExpectsFileMetacache-only branch,lookupDefinitionForOptscache-only branch,populateQueryFieldsFromMetashort-circuit at the loadLinks layer).lookupDefinitionForOptscollapses to a directlookupDefinitioncall — both call sites are inlined.populateQueryFieldsFromMetais removed entirely; the loadLinks walker now always calls the livepopulateQueryFields.Intentionally left alone:
QueryFieldMetaand the indexer'squeryFieldDefsemission inhost/app/utils/file-def-attributes-extractor.ts. They're still written intoresource.metaand consumed elsewhere as metadata; only the cache-only-read short-circuit they served is gone.Net diff: -169 lines across host, realm-server, runtime-common.
Built directly on top of #4799. Re-target this PR's base to
mainbefore merging.Test plan
DURING_PRERENDER_HEADER,duringPrerenderHeaders,__boxelDuringPrerender,markPageAsInPrerender,cacheOnlyDefinitions,populateQueryFieldsFromMeta, orlookupDefinitionForOptsin the source tree (excluding the index-runner'sWhy:comment about the historical deadlock).cacheOnlyDefinitionsflag implicitly.🤖 Generated with Claude Code