Isolate per-file indexing failures instead of aborting the whole realm#5389
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 58m 57s ⏱️ - 1m 8s Results for commit 9a24bc3. ± Comparison against earlier commit fb08d83. Realm Server Test Results 1 files ±0 1 suites ±0 11m 44s ⏱️ -16s Results for commit 9a24bc3. ± Comparison against earlier commit fb08d83. |
5e88b86 to
caea2d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caea2d7a16
ℹ️ 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".
| if (url.href.endsWith('.json')) { | ||
| try { | ||
| let fileRef = await this.#reader.readFile(url); |
There was a problem hiding this comment.
This companion instance-error is only written if the failed URL can be read and parsed a second time. When the original non-404 failure happens before visitFileForIndexing parsed the file, for example a transient reader.readFile/storage error, this read throws too and the code only records the file-error; since Batch.invalidate() has already tombstoned the existing instance row for the same URL, batch.done() will promote that tombstone and remove the card from search despite the failure being transient. The fresh issue is that the new fallback depends on another successful read instead of preserving an existing instance row from the index/invalidated types when the read path is what failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in fb08d83 — the card decision now comes from the index (which live row types the batch tombstoned at that URL) rather than a second file read, so a read-path failure can't cause the tombstone promotion. Re-parse remains only for brand-new files with no prior row to protect.
| throw err; | ||
| return; | ||
| } | ||
| // A transport-level failure (prerender timeout/abort, network error) |
There was a problem hiding this comment.
just FYI prerender doesn't actually ever happen in this job, it's been split out into a different job
There was a problem hiding this comment.
[Claude Code 🤖] Thanks — reworded in fb08d83: the comment now says the failure is a transport-level failure of the index visit's own prerender-server request (or a reader error), and notes explicitly that HTML prerendering runs in a separate job.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed with a focus on how this interacts with the prerender-split work (the index/prerender job separation, the prerendered_html tables, and the two-channel error model), plus general correctness. Verdict: no conflict with the split — this is complementary — and the fix is sound. Two inline comments; the non-line-specific findings are below.
Split integration (all verified against main):
- The catch sits only in the index-visit loop, and its error rows land on
boxel_index.error_doc— exactly where index-channel errors belong now that render errors live onprerendered_html.error_doc. No cross-channel bleed. - The prerender job's own visit (
index-runner/prerender-html-visit.ts) already isolates per-URL failures and writes render-error rows with dep enrichment — so this PR closes the same all-or-nothing hole on the index side that the render side already handles. The two channels now share the isolation model. - A failed URL still flows to the spawned prerender job as an
updatechange, which handles its own rendering/failure independently; error rows written here get the batch's generation like any row, so freshness comparisons stay coherent; and the render-side reconcile sweep keys on render errors, so an index-error row causes no repair churn.
Correctness claims verified:
invalidate()does tombstone every existing row type up front, so theinstance-errorcompanion write is load-bearing — without it an existing card's untouchedinstancetombstone is promoted atdone(). Good catch folding that in.updateEntry's error path preserves last-known-good state (types favored,last_known_good_depsmerged into the error deps), so recovery-by-rewrite works — and the test proves it.- The writer's refuse-empty-error-message guard is satisfied via
coerceErrorMessage+ the expliciterror.messageassignment, avoiding the historical black-hole-row failure mode.
Non-contiguous observations (no change requested):
- Failure visibility shifts from loud to quiet: a from-scratch that used to die now "succeeds" with N error rows and warn logs — which is the point — but the signal now lives in
stats.fileErrors/instanceErrorsand queryablehas_errorrows rather than a failed job. Worth keeping in mind when eyeballing job dashboards. - No automatic retry for transport-failed rows: recovery requires something to re-index the URL (edit, dep invalidation, from-scratch). That matches existing error-row semantics; if transient infra failures turn out to be common in practice, an index-channel reconcile analog could be a follow-on — out of scope here.
The regression test is well-constructed: it injects the one failure mode the in-band error paths can't produce (an abort before any response document exists) at the real Prerenderer seam, and asserts the four things that matter — error rows un-tombstoned, pristine_doc preserved, sibling card still searchable (proving done() ran), and the error washing out on a successful rewrite.
| throw err; | ||
| return; | ||
| } | ||
| // A transport-level failure (prerender timeout/abort, network error) |
There was a problem hiding this comment.
[Claude Code 🤖] Building on the FYI above: worth rewording "prerender timeout/abort" so it doesn't read as HTML prerendering — this job's visit is the index pass (meta/types/icon/file-extract); HTML rendering runs in its own job. The failure class is still real here because the index pass rides the prerender server (prerenderVisit), so something like "a transport-level failure of the index visit's prerender-server request (timeout/abort, network error)" keeps the example accurate without implying HTML rendering happens in this job.
| // and write a matching instance-error row when it's a card, so | ||
| // the last-known-good instance survives the same as the in-band | ||
| // render-error path. | ||
| if (url.href.endsWith('.json')) { |
There was a problem hiding this comment.
[Claude Code 🤖] This companion write is what protects an existing card from the promoted instance tombstone, but it hinges on a second successful readFile + parse. When the original failure was a reader/storage error rather than a prerender-transport one, this re-read most likely fails the same way, we land in the catch below, and the tombstone gets promoted — the exact silent-delete this block exists to prevent. (The earlier automated comment gestures at this; I verified the path is reachable.)
Since the hazard is the tombstone, the index is a more reliable oracle than the file: decide "was this a card?" from whether the URL had a non-deleted instance row before invalidation — either query the production table for it or have invalidate()/tombstoneEntries() report the row types it tombstoned per URL — and write the instance-error on that basis. The source re-parse can stay as the fallback for the brand-new-file case, where there's no prior row and nothing to lose.
There was a problem hiding this comment.
[Claude Code 🤖] Implemented in fb08d83 as suggested: tombstoneEntries now records the live (non-deleted) row types it overwrote per URL, exposed as Batch.tombstonedLiveTypes(url), and tryToVisit decides "was this a card?" from that — no file read involved for existing cards, so a reader/storage failure can no longer promote the instance tombstone. The source re-parse stays as the fallback for the brand-new-file case only (no prior row to lose). Added a second regression test covering that fallback branch (new card + armed visit failure → both error rows land un-tombstoned); the existing test now exercises the oracle path since mango had a live instance row. 63/63 pass on indexing-test.ts. One honest caveat: the reader-failing-twice scenario itself isn't directly simulated (no public seam to break the realm adapter's read), but the decision path no longer depends on a read at all for existing cards, which is what closes the hazard.
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.
The companion instance-error write protected an existing card from its pre-tombstoned instance row, but hinged on a second successful readFile + parse. When the visit failed on the read path itself, the re-read failed the same way, only the file-error landed, and the instance tombstone was still promoted — the exact silent delete the write exists to prevent. The batch now records which live row types tombstoneEntries overwrote per URL, and tryToVisit consults that to decide whether the failed URL had a card instance to protect. The source re-parse remains only as the fallback for a brand-new file, which has no prior row to lose but should still surface its failure as an instance error. Also rewords the failure-mode comment: this job's visit is the index pass riding the prerender server; HTML prerendering runs in a separate job.
fb08d83 to
9a24bc3
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 transport-level failure of the index visit's own prerender-server request (timeout/abort), or a reader error — which unwound the entirefromScratch/incrementalvisit loop and skippedbatch.done(). (HTML prerendering runs in a separate job; the request at issue is the index pass's own visit.)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 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. And deciding "was this a card?" by re-reading the file was itself fragile: if the visit failed on the read path, the re-read fails the same way. The batch now records which live row typestombstoneEntriesoverwrote per URL (Batch.tombstonedLiveTypes), andtryToVisitwrites the companioninstance-errorbased on that index-derived answer — the source re-parse remains only as the fallback for brand-new files, which have no prior row to lose.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: 63/63 passtsc --noEmitclean on changed files🤖 Generated with Claude Code