Record error docs when an incremental index batch fails before its visit loop#5569
Open
habdelra wants to merge 7 commits into
Open
Record error docs when an incremental index batch fails before its visit loop#5569habdelra wants to merge 7 commits into
habdelra wants to merge 7 commits into
Conversation
An incremental index job's invalidation / dependency-ordering / file-meta-prefetch phase runs before the per-URL visit loop, so #runVisitLoop's error isolation never covers a throw there. Such a throw dropped the whole batch with no error doc on any file: a large /_atomic push that failed in this phase returned HTTP success while a subset of instances silently never indexed and nothing was recorded. IndexRunner.incremental now wraps the setup phase and, on a throw, records a file-/instance-error doc for every URL the job was handed — on a fresh batch scoped to just those URLs so the in-flight batch's fan-out tombstones are never promoted (which would delete un-revisited dependents) — then rethrows so the queue still retries. loadResumedRows excludes error rows, so a transient failure re-visits and self-heals on the next attempt while a persistent one stays visible as an error doc. Adds a realm-server regression test: a 50-file atomic push that does not wait for indexing kicks off both the incremental-index and prerender_html jobs and both run to completion with no error docs; and a setup-phase failure records an error doc for every pushed file with no collateral deletion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review of the setup-phase recovery found it could delete a previously-good card: `#recordSetupPhaseError` records error rows on a fresh batch, and `applyBatchUpdates` promotes every working row for the argument URLs — including the `instance` tombstone the in-flight batch wrote during `invalidate()`. The fresh batch classified card-ness by re-reading the file, so any URL it failed to re-classify (a read blip, a delete whose file is gone, non-card on-disk content) left that tombstone to be promoted, removing the card from search. - Seed the fresh batch's live-type oracle from the production index (`Batch.recordProductionLiveTypes`) the way `invalidate()` does, so an existing card is written as an `instance-error` that overwrites the stale tombstone rather than letting the tombstone promote. - Skip `delete` URLs in the recovery: a delete whose job failed at setup must not be half-applied (neither resurrected as an error nor promoted from the in-flight tombstone) — leave production untouched and let the retry complete the delete. Extends the regression test with a delete-during-setup-failure case (a searchable link gives the delete a >=2-URL fan-out so the ordering step runs) asserting the existing card is left live and unerrored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three additions to the atomic-batch indexing tests: - An update whose on-disk content is no longer a card, failing at setup: the recovery classifies the URL from the production index (not the unparseable file) and preserves the existing card as an instance error instead of letting the in-flight tombstone delete it. - Re-pushing files whose batch failed at setup replaces their error docs with clean rows — the recovery path the error docs enable, with no operator full-reindex. - A from-scratch setup failure leaves the production index intact: that pass swaps only at its end, so a setup throw changes nothing. All failure-scenario waits are outcome-based (index rows, or the in-process fault counter) rather than job-table-based: a thrown job error is rejected with no in-queue retry, and a finalization lost to a serialization conflict can leave a job unfulfilled behind a long-lived reservation — so job status is not a settle signal tests can lean on. Corrects two code comments that described the rethrow as feeding an in-queue retry; re-attempts only follow reservation expiry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Two comments described the rethrown setup error as feeding a queue retry; a thrown job error rejects outright. Reworded to the real recovery paths (a later visit 404s a failed delete's stale row; error docs stay the durable signal). - The setup-phase failure test now waits on its outcome (error docs landing) instead of job status, matching the rest of the file; a finalization lost to a serialization conflict parks a job unfulfilled behind a long reservation, so status is not a settle signal. - The re-push test gates on the failed job leaving flight so the re-push cannot coalesce into the doomed in-flight job and reject with it. - Rename recordProductionLiveTypes to seedLiveTypesFromProduction — it seeds the live-type oracle and tombstones nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb452582e
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes incremental indexing failures visible when the job crashes before the per-URL visit loop starts (e.g. during invalidation fan-out, dependency ordering, or file-meta prefetch). It does so by recording per-URL error docs for the job’s URL set on a fresh batch, while still rethrowing so the job is rejected and remains visible to job accounting.
Changes:
- Add
Batch.seedLiveTypesFromProduction(urls)to seed “was this a live card?” from the production index without re-reading files. - Wrap incremental job setup in a
try/catchand, on setup-phase failure, record file/instance error docs via a fresh batch before rethrowing. - Add a new realm-server test suite covering large atomic pushes, setup-phase failures, delete semantics, and “heal by re-push” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/runtime-common/index-writer.ts | Adds seedLiveTypesFromProduction() helper to classify existing live types from production index. |
| packages/runtime-common/index-runner.ts | Adds setup-phase error recording path and refactors error buffering to support both visit-loop and setup-phase failures. |
| packages/realm-server/tests/index.ts | Registers the new atomic batch indexing test module. |
| packages/realm-server/tests/atomic-batch-indexing-test.ts | New integration-style test coverage for atomic pushes and setup-phase failure recovery behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Host Test Results 1 files 1 suites 3h 9m 57s ⏱️ Results for commit 0e39c09. Realm Server Test Results 1 files ±0 1 suites ±0 11m 49s ⏱️ + 1m 3s Results for commit 0e39c09. ± Comparison against earlier commit 0de8a1e. |
A failed incremental job can still have swapped setup-phase error docs into boxel_index, but cache invalidation and the incremental broadcast only ran via onInvalidation/onSettled after a successful job — leaving the new rows behind stale peer caches and silent subscribers until the next successful swap. enqueueUpdate gains an onFailed hook running inside the same deferred lifecycle; the realm wires it to the same cache wipe plus an incremental invalidation broadcast for the attempted URLs. The setup-phase failure test now asserts that broadcast lands. Also from review of the recovery path: - done() gains carryForwardRealmMeta: the recovery re-stamps the prior generation's realm_meta at its generation instead of recomputing from a working table still holding the failed pass's un-promoted fan-out tombstones, which would undercount live dependents in type summaries. (Reads join realm_meta to realm_generations.current_generation, so a row must exist at the new generation.) - The incremental pass's terminal progress event and prerender-affinity release now sit in an outer finally that covers the setup phase too. - The setup-failure warning logs a URL count plus a small sample rather than every URL; the error docs carry the full detail. - The fallback error message names the setup phase rather than just invalidation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… skill Mode C claimed a rejected indexing job commits nothing to boxel_index; an incremental job that fails in its setup phase commits error docs for its update-operation URLs before rejecting. Documents that exception's signature (N rows sharing one error_doc.message and job_id, no visit diagnostics, carried-forward realm_meta, invalidation broadcast despite rejection) and the re-push recovery, plus a covers-item in the skill description so the scenario triggers the skill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jurgenwerk
approved these changes
Jul 22, 2026
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.
A bulk
/_atomicpush (theboxel realm pushpath) returns 201 once its writes are durable and enqueues a single incremental index job covering every file — deliberately not waiting for indexing. The job's per-file visit loop isolates render failures by recording per-URL error docs, but the setup phase that runs before it — the invalidation fan-out, dependency ordering, and file-meta prefetch — had no such isolation: a throw there rejected the whole job with nothing persisted. Every file in the batch silently never indexed, typed-search counts stayed short of the push forever, and no error doc marked the gap. Larger batches do more setup work, so they hit this disproportionately.The incremental pass now catches setup-phase throws and records a file-error (and, for cards, instance-error) doc for the job's URLs before rethrowing, so the job still rejects and the failure stays visible to job accounting. The recording runs on a fresh batch scoped to just those URLs, which is load-bearing in three ways:
Batch.seedLiveTypesFromProduction) rather than re-reading the file — the read path may be exactly what failed, and a mis-classified existing card would otherwise have its in-flightinstancetombstone promoted, silently deleting it.The error docs never wedge a URL: error rows are excluded from job resume and invalidate like any other row, so re-pushing the affected files (or the next reindex) replaces them with clean content.
The new test file covers six scenarios: a 50-file push with no
waitForIndexthat fully indexes and prerenders on both job channels with zero error docs; a setup-phase throw recording error docs for every pushed file; a failed delete leaving the existing card live and clean; a failed update whose on-disk content no longer parses as a card preserving the card as an instance error rather than deleting it; a re-push replacing error docs with clean rows; and a failed from-scratch pass leaving the production index untouched. Failure-scenario waits are outcome-based (index rows, or an in-process fault counter) rather than job-status-based, because a thrown job error rejects with no in-queue retry and a finalization lost to a serialization conflict can park a job unfulfilled behind a long-lived reservation.🤖 Generated with Claude Code