feat(indexer): backfill role for historical catch-up - #3582
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approving: the bug hunting system found a real bug (unhandled promise rejection crashing the process during backfill prefetch) plus several nits — these need human review before merge.
Extended reasoning...
Overview
This PR adds the backfill indexer role: a new BackfillRunnerService, planBackfill pure planner, BlockCommitterService.commitBatch() with chunked inserts and contiguity guard, plus env schema changes and wiring in index.ts. It's a large (size: L), first-of-its-kind feature touching a new concurrent fetch/commit pipeline, database write paths, and advisory-lock leadership logic shared conceptually with the existing sync runner.
Security risks
No injection, auth, or data-exposure concerns — all queries use Drizzle ORM parameterization and the advisory lock is a plain integer key. The main risk category here is correctness/availability: an unhandled promise rejection in the prefetch window can crash the whole indexer process via Node's default --unhandled-rejections=throw, bypassing the intended BACKFILL_FATAL graceful-exit path. This is a legitimate availability bug in a one-off Job that's meant to run unattended for hours during a mainnet rebuild.
Level of scrutiny
This warrants full human review. It's new pipeline code with real concurrency (bounded prefetch window, ordered sequential consumption) and a database commit path that batches inserts inside transactions alongside checkpoint advances — exactly the kind of subtle ordering/error-handling logic where a bug hunting pass surfacing a genuine crash bug (not just a nit) is a strong signal that a human should look, even though the PR's own test suite (24 new tests) is reasonably thorough.
Other factors
The bug hunting system found one confirmed correctness bug (unhandled rejection crash) and four nits (test convention violation, code-duplication with lodash/chunk, duplicated leadership logic between runners, and a check-ordering edge case in planBackfill). None of these are addressed yet in the diff. Given the confirmed bug's severity (process crash bypassing the designed error-handling path) and the size/novelty of this PR, deferring to human review is the correct call.
- attach a no-op catch to prefetched blocks so a rejection settling before its height is consumed cannot crash as an unhandled rejection - check range completion before tip validity so a finished range stays a no-op even when a lagging node reports a stale tip - extract the shared PgAdvisoryLeaderLock used by both runners - use lodash chunk instead of a local helper - build the pg client test double with a nested mock instead of a cast
…ss roles - extract retryWithBackoff and use it in both runners - retry backfill startup reads and idempotent batch commits so a transient blip no longer kills a multi-hour job - collapse the sync and backfill bootstrap cases into one helper
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether PgAdvisoryLeaderLock (src/db/pg-advisory-leader-lock.ts) lacking a dedicated spec file is a coverage gap — it's exercised indirectly through backfill-runner.service.spec.ts's pgClient fake, which simulates pg_try_advisory_lock/pg_backend_pid responses to drive the acquire path, so I did not flag it separately.
Extended reasoning...
This is a brief informational note, not a new review. The candidate issue (no unit tests for pg-advisory-leader-lock.ts) was investigated by checking backfill-runner.service.spec.ts's setup() function, where the pgClient.client.reserve() fake returns pg_try_advisory_lock/pg_backend_pid results that drive PgAdvisoryLeaderLock's acquire()/assertHeld() logic through the runner's own tests. That's real, if indirect, exercise of the lock's acquire path (not just a mock returning a canned value with no assertions on the lock's own behavior), so a missing dedicated spec file isn't a meaningful coverage gap worth blocking on.
…-safe writes Inserts were already idempotent; making the indexer_state checkpoint upsert monotonic (GREATEST) makes overlapping writers harmless, so the leader lock, its backend-pid liveness checks, and LeadershipLostError can all go. Single writers per role are enforced at the orchestration level instead.
There was a problem hiding this comment.
I reviewed this PR and did not find any new bugs this run. Because it adds a new backfill role that writes directly to the indexer database (batched inserts, checkpoint advancement, continuity verification) and changes process lifecycle/exit-code behavior, a human look is still worthwhile.
What was reviewed:
- Range planning/resume logic in
planBackfill, and batchedcommitBatchcontiguity + chunked inserts under the bind-parameter limit. - Re-checked the previously flagged SIGTERM-mid-backfill exit-0 path (
backfill-runner.service.ts) — this run concluded it mirrors the existing sync-runner shutdown pattern rather than being a new defect; worth a human opinion given it is Job (not Deployment) lifecycle semantics. - Re-checked the dead
inflight.get(height) ?? fetchfallback and the unconditionalupdatedAtbump on theGREATESTcheckpoint upsert — both harmless, no correctness impact. - Re-checked the
as unknown as ChainDatabasetest double — consistent with the existing pattern already used inblock-committer.service.spec.tsfor this hard-to-mock Drizzle type.
Extended reasoning...
This run re-examined several previously-flagged candidates (SIGTERM/exit-0 behavior, a dead fallback branch, the unconditional checkpoint updatedAt bump, and an as unknown as test double) and did not confirm any of them as bugs this pass. No new findings were produced.
The PR is large (size: L) and adds a new indexer run mode that writes directly to Postgres (batched, chunked inserts plus a monotonic checkpoint upsert) and changes process exit-code/lifecycle handling in index.ts. That combination — production DB writes plus K8s Job success/failure semantics — is exactly the kind of critical-path, large-scope change where I would keep the bar for auto-approval high regardless of a clean automated pass, so I am deferring rather than approving.
No security-sensitive surface (auth, crypto, permissions) is touched; the main risk category is data-correctness/operational (partial backfills silently marked complete, duplicate work from overlapping writers) which the PR explicitly designs around (idempotent inserts, monotonic checkpoint) rather than something introduced by an oversight.
… Job Addresses review findings on the backfill role: - A backfill stopped before finishing its range (SIGTERM mid-run) now throws RunnerInterruptedError so the process exits non-zero. Backfill runs as a K8s Job where exit 0 marks it Complete, so the previous clean exit silently left the range unfinished and unretried; it now resumes from its checkpoint on the next attempt. - Drop the unreachable prefetch fallback in the consume loop; the sliding window always has the current height inflight before it is awaited. - Back the retry delay with the global setTimeout so the fetch-retry test can advance it with fake timers instead of sleeping a real second.
A misconfigured role (e.g. a backfill Job missing BACKFILL_FROM/TO_HEIGHT) failed with a tsyringe "Cannot inject the dependency" wrapper around the ZodError plus an internal stack trace. Validate env eagerly at bootstrap and log a CONFIG_INVALID event listing the offending fields and messages instead.
…letion via return Fetch the checkpoint height and chain tip concurrently instead of serially, hoist the loop-invariant base height in the contiguity check, and have the backfill run return whether it completed so start() no longer re-reads the stopped flag.
…stopped flag A shutdown landing during the final commit left the range fully committed but reported it as interrupted, failing the Job and triggering a spurious retry. Completion is now derived from the last committed height reaching the range end. Also dedupes the identical getTipHeight helper into RpcClientPool so both runners share one implementation.
There was a problem hiding this comment.
I reviewed this PR and this run's automated bug hunt found no new issues. Given several rounds of real bugs already found and fixed in this PR (SIGTERM/exit-code handling for the K8s Job, a completion-tracking race, dead code, a test timer bug, and RPC-pool duplication), and that this introduces a new critical run mode (backfill) with checkpoint/transaction semantics that must stay correct under kill/restart, a human look is still worthwhile before merging into the collab branch.
What was reviewed: the backfill planner/runner lifecycle (resume, no-op re-run, out-of-order fetch ordering, contiguity, SIGTERM handling), the batched commit + checkpoint transaction (chunking, GREATEST upsert, contiguity guard), and the shared retry/continuity helpers extracted for both runners. Two additional candidates were checked and ruled out this run: start() reclassifying a genuinely concurrent fatal error as RunnerInterruptedError, and backfill range validity depending on a single RPC node's tip (a stale/lagging tip cannot mark an unfinished range complete, since planBackfill checks the checkpoint against toHeight first).
Extended reasoning...
Overview
This PR adds the backfill indexer role to apps/chain-indexer: a one-off catch-up run mode that reads a height range from RPC in parallel, decodes and commits blocks in contiguous batches, and checkpoints progress under a range-scoped stream so it can resume after a kill. It also refactors SyncRunnerService to remove the postgres advisory-lock leader election (replaced by conflict-ignoring inserts and a monotonic GREATEST checkpoint upsert), extracts shared retryTransient/ChainContinuityError/RunnerInterruptedError helpers used by both runners, and adds a getTipHeight method on the RPC pool. 19 files changed, ~24 new unit tests.
Security risks
No new external attack surface — this is an internal batch job reading from RPC nodes and writing to Postgres via parameterized Drizzle queries (no raw SQL interpolation of user input). The main risk category here is data-integrity, not security: a bug in the checkpoint/commit-ordering logic could corrupt or gap the indexed dataset, or cause a Kubernetes Job to be marked Complete/Failed incorrectly. Removing the advisory-lock leader election in favor of idempotent inserts + monotonic checkpoint upsert is a real design change, but it's well-reasoned and covered by tests, and the PR explicitly documents the operational constraint it now relies on (replicas: 1 / parallelism: 1).
Level of scrutiny
This warrants a higher level of scrutiny than a typical PR: it's XL-sized, touches the core commit/checkpoint transaction shared with live sync, and changes Job success/failure semantics that determine whether a multi-hour production backfill silently completes with gaps. Several real correctness bugs were already found and fixed across this review's rounds (SIGTERM landing mid-final-commit misreporting completion, exit code not reflecting an interrupted run, a dead-code fetch fallback, an unintentional real 1s sleep in a test, and duplicated getTipHeight logic) — the fact that this round's automated pass found nothing new is a good sign but doesn't offset the inherent complexity of the change.
Other factors
This is the first child PR into a collab branch (feat/indexer-scaffold-chain-indexer-app, #3579) rather than directly into main, so a human maintainer review before it lands there is part of the normal process regardless of the automated pass. Test coverage is solid (planner, runner ordering/resume/continuity/retry, committer batching, env schema), and the author has been responsive, fixing every issue raised in prior rounds with matching commits.
6a0a335
into
feat/indexer-scaffold-chain-indexer-app
1 participant
Why
Fixes CON-804
The rewrite targets a full mainnet rebuild in under 8 hours, and the live tail alone cannot rebuild a database from scratch. This adds the backfill run mode from the indexer v2 plan: a one-off job that catches the new database up over an explicit height range.
First child PR into the
feat/indexer-scaffold-chain-indexer-appcollab branch (#3579). CI's test matrix does not run against non-main bases, so it was verified locally: 51 unit tests, lint, and tsc all green.What
INDEXER_ROLE=backfillnow runs end to end: plan the range, fetch blocks from RPC in parallel (BACKFILL_CONCURRENCY, default 10), commit them strictly in order in batches (BACKFILL_BATCH_SIZE, default 200), log a throughput summary, then shut down and exit 0. The healthz server stays up during the run for Job liveness.planBackfill(pure function): resumes from the checkpoint, exits 0 immediately when the range is already covered, and fails instead of clamping whenBACKFILL_TO_HEIGHTis above the chain tip.BlockCommitterService.commitBatch(blocks, { stream }): contiguity guard, chunked inserts to stay under postgres.js's bind-parameter limit, and the checkpoint advance in the same transaction, so a kill mid-batch rolls back data and cursor together.commit()delegates with thesyncstream, so live sync behavior is unchanged.indexer_statestreambackfill:{from}-{to}; kill + restart resumes at checkpoint + 1 without gaps or duplicates (natural keys + conflict-ignoring inserts). Changing the range gets a fresh cursor.GREATEST), so overlapping writers on the same stream duplicate work but cannot corrupt data or move the cursor backwards; single writers per role are an orchestration concern (replicas: 1/ Jobparallelism: 1). This removed the advisory-lock class and its backend-pid liveness checks from both runners.BACKFILL_FROM_HEIGHT/BACKFILL_TO_HEIGHTare required for the role via a schema refinement and empty-string safe; other roles are unaffected.