Make indexer module pre-warm populate the definition cache#5042
Open
habdelra wants to merge 6 commits into
Open
Make indexer module pre-warm populate the definition cache#5042habdelra wants to merge 6 commits into
habdelra wants to merge 6 commits into
Conversation
The worker constructs a bare CachingDefinitionLookup with no registered realm, so preWarmModulesTable's getCachedDefinitions call self-resolved a null context and persisted nothing — pre-warm reported success while doing nothing, leaving every type-filtered search to resolve its card definition on demand mid-render. Thread the cache context the IndexRunner already computes into a new explicit-context read-through populate (populateDefinitionCacheEntry), keyed identically to the visit-phase reader (realm-auth / realm-owner user id). Skip pre-warm with a warning if a private realm resolves an empty cache user id, so it never writes keys the reader cannot read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c58465ae
ℹ️ 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".
Resolving the pre-warm cache context fetches realm _info, which can transiently fail. Pre-warm is best-effort, so wrap the context lookup so a failure warns and skips pre-warm — letting the visit phase populate on demand — instead of throwing out of preWarmModulesTable and aborting the whole indexing run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 1h 45m 22s ⏱️ - 7m 26s Results for commit 9116388. ± Comparison against earlier commit 4565b4c. Realm Server Test Results 1 files ±0 1 suites ±0 13m 0s ⏱️ -24s Results for commit 9116388. ± Comparison against earlier commit 4565b4c. |
…-module-definition-pre-warm-is-a-silent-no-op-in-the-indexer
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a silent no-op in the indexer worker's module-cache pre-warm. The worker constructs a bare CachingDefinitionLookup with no registered realm; the previous self-resolving getCachedDefinitions path returned null from buildLookupContext and persisted nothing while reporting success. The fix introduces an explicit-context populate API and threads the same context the visit-phase reader uses.
Changes:
- New
populateDefinitionCacheEntry(args)onDefinitionLookup(and both implementations) accepting explicit cache context instead of self-resolving via#realms/remote probe. IndexRunner.preWarmModulesTablenow resolves cache context viagetModuleCacheContext()and calls the explicit populate; gracefully skips with a warning on context-resolution failure or on a private realm with empty cache user id.- Regression test in
definition-lookup-test.tsreproducing the bare-lookup no-op vs. explicit-context persist, and verifying the warmed key is read by a realm-scoped reader without re-prerendering.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/index-runner.ts | Resolve cache context, validate non-empty user id for private realms, and call new explicit-context populate. |
| packages/runtime-common/definition-lookup.ts | Add PopulateDefinitionCacheEntryArgs and populateDefinitionCacheEntry on interface + both implementations. |
| packages/realm-server/tests/definition-lookup-test.ts | Adds worker-bare-lookup regression test asserting no-op vs explicit-context persistence and key alignment. |
| packages/realm-server/tests/matches-filter-integration-test.ts | Update stub to satisfy expanded DefinitionLookup interface. |
| packages/host/tests/unit/index-query-engine-test.ts | Update stub to satisfy expanded DefinitionLookup interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pre-warm sweeps every realm .gts/.gjs to prime sibling card modules, so it also touches files that aren't cards and fail to prerender (e.g. a non-card realm.gts). Persisting those errors pollutes the modules cache with rows no reader requested. Thread a skipErrorPersist flag through the populate path so pre-warm returns without caching a non-missing prerender error; the on-demand visit-phase lookup still re-derives and caches the error if the module is genuinely needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fixture realm's indexing now pre-warms its card modules (including person.gts) into the modules cache. The lookupDefinition / invalidation / extensionless-invalidation tests asserted prerenderModule call counts assuming a cold cache, so a pre-warmed row made the first lookup a cache hit (0 calls instead of 1). Clear the modules table at the start of each, matching the pattern the other prerender-count tests in this module use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-warm exists to keep the prerender server's affinity-scoped tab pool from deadlocking on a mid-render sub-prerender. The in-browser realm (host tests run a Realm + IndexRunner inside a Chrome tab) has no separate prerender server or tab pool, so pre-warm is pointless there — and harmful: the in-browser realm shares the global card-reference prefix registry with the host, so populating the definition cache before the host registers a prefix bakes in keys the prefixed reader can't match (a real server only ever receives resolved URLs over the wire and never hits this). Skip pre-warm when running in the browser test environment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background and Goal
The indexer's module-definition pre-warm step runs in the worker, reports success, and persists zero definition-cache rows. Every type-filtered
store.search()during the visit phase then resolves its card definition on demand, firing a same-affinityprerenderModulemid-render — the exact mid-render sub-prerender serialization pre-warm exists to prevent, which contributes to render timeouts on aggregate-card-heavy realms.Root cause: the worker builds a bare
CachingDefinitionLookupwith no registered realm, sopreWarmModulesTable'sgetCachedDefinitionscall self-resolves a null cache context (buildLookupContextreturnsnull) and returns without persisting. Because it returns rather than throws, the failure counter stays 0 — so pre-warm logs success while doing nothing. Definitions still get cached, but lazily, by the realm-server during the visit phase — precisely the on-demand path pre-warm was meant to eliminate.Where to start
packages/runtime-common/index-runner.ts—preWarmModulesTablenow threads the contextgetModuleCacheContext()already computes into the populate call instead of relying on self-resolution.packages/runtime-common/definition-lookup.ts— newpopulateDefinitionCacheEntry(args)on theDefinitionLookupinterface: a read-through populate that takes explicit cache context rather than self-resolving it via#realms/ a remote probe.Key decisions
cacheScope: 'realm-auth',cacheUserId= realm-owner user id,resolvedRealmURL= realm URL) — the same context the read-only batch reader already uses — so the warmed row is the one the reader reads, not a mismatched key.