Isolate per-file indexing failures instead of aborting the whole realm#5389
Isolate per-file indexing failures instead of aborting the whole realm#5389richardhjtan wants to merge 3 commits into
Conversation
![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: 6ae5858e07
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR makes realm indexing more robust by preventing a single file’s transport-level prerender/visit failure from aborting the entire indexing batch (and therefore preventing batch.done() from promoting the working index into the live boxel_index table).
Changes:
- Updates
IndexRunner.tryToVisitto treat non-404 visit failures as per-file failures by writing a persistedfile-errorindex entry for the URL. - Ensures dependency-row cache invalidation and
stats.fileErrorsaccounting happen on this new error path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Host Test Results 1 files ± 0 1 suites ±0 2h 37m 44s ⏱️ + 17m 47s Results for commit caea2d7. ± Comparison against earlier commit 5e88b86. Realm Server Test Results 1 files ± 0 1 suites ±0 10m 46s ⏱️ + 1m 14s Results for commit caea2d7. ± Comparison against earlier commit 5e88b86. |
tryToVisit rethrew any non-404 error from a file visit, which unwound the entire fromScratch/incremental visit loop and skipped batch.done(). Since batch.done() is what promotes the working table into the live boxel_index, a single file's transport-level failure (e.g. a prerender timeout) discarded every other already-successfully-indexed file in the same job, leaving the realm mounted with an empty or stale search index and no error surfaced. Catch non-404 errors per file and record a file-error entry for that URL instead, mirroring the error_doc pattern already used for in-band render errors, so the rest of the realm still indexes and commits.
Batch.invalidate() tombstones every existing type at a URL up front, including the instance row for a previously-indexed card. The prior commit's file-error entry only overwrote the file tombstone, so batch.done() would promote the untouched instance tombstone and silently remove an existing card from search over a transient error (e.g. a prerender timeout) instead of preserving it with an error row the way the in-band render-error path does. Re-check whether the failed URL is a card and, if so, also write an instance-error entry alongside the file-error one.
Injects a wrapping Prerenderer whose prerenderVisit throws an abort-style transport error for one armed URL — the failure mode the in-band error paths can't reach, since they all require a prerender response to exist. Asserts that when an existing card's visit fails this way, the incremental job still commits (a sibling card stays searchable), the card's file and instance rows carry the error without being tombstoned, the instance keeps its last-known-good pristine_doc, and a later successful rewrite clears the error state. Without the isolation fix this test fails with the whole incremental-index job rejecting on the injected transport error.
richardhjtan
force-pushed
the
fix/index-runner-file-error-isolation
branch
from
5e88b86 to
caea2d7
Compare
Origin
Found while debugging the Virtual Piano App in the public catalog: its in-card song search showed "No Music Sheets found" when the app ran from the catalog realm, yet worked after remixing into a personal realm. First root cause: the local catalog realm's
boxel_indexwas completely empty — its from-scratch reindex had gotten through 652 of 1166 files, hit one stuck prerender-visit request (a 120s abort on a listing-thumbnail PNG), and the whole job died, discarding all the already-indexed rows. The realm still mounted "successfully," so every search against it silently returned nothing. This PR fixes that all-or-nothing failure mode. (The same investigation surfaced a second, independent bug in how search results are hydrated for alias-prefixed realms, fixed in #5388.)Summary
IndexRunner.tryToVisitrethrew any non-404 error from a file's visit (e.g. a prerender-visit timeout/abort), which unwound the entirefromScratch/incrementalvisit loop and skippedbatch.done().batch.done()is what promotes the working table into the liveboxel_index, a single file's transport-level failure discarded every other already-successfully-indexed file in the same job — leaving the realm mounted but with an empty or stale search index, and no error surfaced anywhere.error_docrow for just that card), but a transport-level failure — where no prerender response ever arrives — bypassed that model entirely and became all-or-nothing for the whole realm.file-errorentry for that URL, mirroring the existingerror_docpattern, so the rest of the realm still indexes and commits.Batch.invalidate()pre-tombstones every existing row type at a URL, so the file-error write alone would let an existing card's untouchedinstancetombstone be promoted — silently deleting a previously-good card from search over a transient error. The failed URL is re-checked and, when it's a card, aninstance-errorentry is written too, preserving the last-known-good instance the same way the in-band render-error path does.Test plan
indexing-test.ts› per-file failure isolation): a wrappingPrerendererthrows an abort-style transport error for one armed URL; asserts the incremental job still commits (sibling card stays searchable), both thefileandinstancerows carry the error without being tombstoned, the instance keeps its last-known-goodpristine_doc, and a later successful rewrite clears the error stateindexing-test.tsrun on the rebased branch: 62/62 passtsc --noEmitclean on changed filesvisitFileForIndexingrename from the prerender-split refactor)🤖 Generated with Claude Code