Skip to content

fix: advance the cursor to the ledger actually covered - #198

Open
Richard-tobi wants to merge 1 commit into
Miracle656:mainfrom
Richard-tobi:fix-advance-cursor-covered-ledger-180
Open

Richard-tobi wants to merge 1 commit into
Miracle656:mainfrom
Richard-tobi:fix-advance-cursor-covered-ledger-180

Conversation

@Richard-tobi

Copy link
Copy Markdown

Overview

fetchEvents (src/rpc.ts) read a single getEvents page, returned the network tip (resp.latestLedger) as the highest ledger, and discarded resp.cursor — so a page truncated at limit silently dropped the rest of the range. fetchEventsSafe's happy path passed that tip straight through as highestLedger, ignoring its endLedger argument, and pollOnce committed it as the cursor. The cursor therefore jumped past target = tip - TIP_LAG, defeating the propagation buffer that exists to avoid reading un-settled ledgers.

This change drains a full page by following the RPC cursor and advances the cursor only to the ledger those pages actually covered, bounded by endLedger.

Related Issue

Miracle656/wraith#180 — Advance the cursor to the ledger actually covered, not the chain tip.

Changes

[MODIFY] src/rpc.ts

  • fetchEvents now returns a FetchEventsPage (events, latestLedger, cursor, maxLedger) instead of { events, latestLedger }. cursor is the RPC paging token; maxLedger is the highest event.ledger actually seen in the page (0 when empty). Existing fields are kept, so current destructuring callers are unaffected.
  • fetchEvents accepts an optional options.cursor and builds the cursor-shaped request ({ cursor, limit, filters }) when continuing a page, and the range-shaped request ({ startLedger, limit, filters }) otherwise — matching the SDK's mutually exclusive Api.GetEventsRequest union.
  • Added EVENTS_PAGE_BUDGET (20) and the fetchRangePaged helper, which pages with the cursor while pages return full, drops events above endLedger, and reports whether a page-budget cut the range short.
  • fetchEventsSafe's happy path now returns highestLedger = min(last ledger fully covered, endLedger): latestLedger when the range drained, otherwise the last maxLedger observed when the budget was hit. It never returns the raw tip. The XDR bisection/skip branches are unchanged.

[MODIFY] src/indexer.ts

  • pollOnce's third parameter is renamed from latestLedger to endLedger (it is the poll target, tip - TIP_LAG), and the committed cursor is clamped with Math.min(highestLedger, endLedger) so no source can advance it past the requested window. The empty-batch and normal commit paths both persist the clamped cursor.

[ADD] src/__tests__/rpcPaging.test.ts

Jest tests with a mocked RPC getEvents on the cached server (the real request/response shaping still runs):

  • a first page of exactly limit events followed by a second page proves the second page is now ingested (and that the follow-up request used the cursor);
  • highestLedger never exceeds the passed endLedger and never equals the raw tip;
  • events above endLedger are dropped instead of ingesting un-settled ledgers;
  • the page budget stops the loop and only advances to the last covered event.

Verification Results

Tests added under src/__tests__/rpcPaging.test.ts (jest, same runner as the
existing fetchEventsSafe.test.ts).
NOT EXECUTED: this fix was produced through the GitHub REST API with no local
clone, so the repo's node_modules and jest runner were unavailable. The change
was reasoned against the SDK v15.0.1 Api.GetEventsRequest union and the existing
unit tests, but no test run or tsc pass was performed here.
Acceptance Criteria Status
fetchEvents returns the paging cursor and the maximum event.ledger seen, alongside latestLedger Implemented — FetchEventsPage { events, latestLedger, cursor, maxLedger }
A full page causes the caller to page with the cursor until the range is drained or a page budget is hit Implemented — fetchRangePaged follows cursor, bounded by EVENTS_PAGE_BUDGET
highestLedger is min(last ledger fully covered, endLedger) — never the raw tip Implemented — Math.min(truncated ? maxLedger : latestLedger, endLedger)
A test with a mocked RPC returning exactly limit events proves the second page is lost on main and ingested after Test added; not executed (no local clone)
A test proves the cursor never exceeds the endLedger passed in Test added; not executed (no local clone)

Closes #180

@drips-wave

drips-wave Bot commented Sep 26, 2026

Copy link
Copy Markdown

@Richard-tobi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strong work — the diagnosis is right, the cursor-paging drain is the correct shape, and the four tests in src/__tests__/rpcPaging.test.ts all pass locally against your branch (57 passed across rpcPaging + fetchEventsSafe + indexerSources + multiNetworkIndexer; tsc --noEmit and tsc -p tsconfig.test.json both clean after npx prisma generate). I checked the drain logic in both directions and I could not make it commit a cursor past unindexed data on the pollOnce path: the maxLedger >= endLedger break at fetchRangePaged and the page-budget break can both leave the top ledger partially covered, but pollOnce re-reads fromLedger inclusively, so that ledger is picked up on the next poll. That is correct, and it is worth a comment saying so, because it is the load-bearing assumption of the whole design.

What I can't merge yet is that the invariant your own docstring states — "the highest ledger fully covered by them — never the raw network tip" — is not upheld on two sibling paths, and both of them lose ledgers permanently.

1. src/indexer/parallel.ts — pollParallel takes the max, not the min

const highestLedger = results.reduce(
  (max, r) => Math.max(max, r.highestLedger),
  fromLedger,
);
await setLastIndexedLedger(highestLedger, net);

Before your change every worker returned the identical network tip, so Math.max was meaningless. Your change is what makes the per-worker values diverge: a partition whose pages hit EVENTS_PAGE_BUDGET now returns a genuinely lower highestLedger than a partition that drained cleanly. Math.max throws that signal away and commits the higher one, so the truncated partition's ledgers are skipped and never revisited — the exact failure #180 is about, just on the INGEST_WORKERS > 1 path. The committed cursor has to be the minimum across workers (seeded from toLedger, then floored at fromLedger so an empty window still makes progress). pollParallel also never clamps to toLedger, which the new pollOnce does.

2. src/rpc.ts — the bisect path still returns the raw tip

The single-ledger branch is untouched:

if (startLedger >= endLedger) {
  const { events, latestLedger } = await _fetchFn(startLedger, contractIds, limit, network);
  return { events, highestLedger: Math.max(startLedger, latestLedger) };

and the catch returns Math.max(lower.highestLedger, upper.highestLedger) with no clamp. So one XDR decode error anywhere in the range makes fetchEventsSafe hand back the network tip. pollOnce's new Math.min(highestLedger, endLedger) saves the serial path — that defensive clamp was a good instinct — but pollParallel has no such clamp, so on the parallel path an XDR error jumps the cursor straight to the tip and everything between is gone. Clamping inside fetchEventsSafe (both the single-ledger return and the bisect return) is the fix, and it makes the function actually honour its docstring for every caller, including src/ingest/backfill.ts.

A test for each would be good: one pollParallel case where one worker is truncated and one is not, asserting the committed ledger is the truncated one; and one fetchEventsSafe case where the bisect fires, asserting highestLedger <= endLedger.

Smaller things, not blockers:

  • EVENTS_PAGE_BUDGET = 20 with BATCH_SIZE defaulting to 10 000 means a single ledger would need ~200k matching events to livelock the loop (maxLedger === fromLedger → cursor does not advance). Not reachable in practice, but a one-line comment saying why the budget is safe would stop the next reader from having to work it out.
  • fetchRangePaged takes _fetchFn but the XDR bisect only wraps the whole range, so a decode error on page 17 re-fetches pages 1–16. Fine for now; worth a note.

Thanks — this is the right fix and the paging drain is the hard part. Wire the same clamp through the parallel and bisect paths and I'll merge it.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

@Richard-tobi

Copy link
Copy Markdown
Author

@Miracle656 Both blockers are fixed and the branch is rebased, so it merges again.

1. pollParallel commits the minimum — src/indexer/parallel.ts

const highestLedger = Math.max(
  fromLedger,
  results.reduce((min, r) => Math.min(min, r.highestLedger), toLedger),
);

The seed is toLedger, which also clamps the parallel path to the requested window (it had no clamp of its own), and the floor at fromLedger keeps an empty window advancing. The docstring is restated to match: the return is the cursor to commit — the lowest highestLedger across workers, clamped to [fromLedger, toLedger] — with a note on why a partition cut short by the page budget pins the whole window.

2. fetchEventsSafe clamps on every path — src/rpc.ts

  • single-ledger branch: Math.min(Math.max(startLedger, latestLedger), endLedger)
  • drained branch: truncated ? maxLedger : latestLedger, then Math.min(..., endLedger)
  • bisect return: an explicit Math.min(..., endLedger) over the merged halves

So the docstring holds for pollParallel and src/ingest/backfill.ts too, not only for pollOnce.

3. Tests

  • src/__tests__/parallelCursor.test.ts (new): two partitions, one truncated at 105 and one drained to 120 → committed cursor is 105; plus the empty-window floor case.
  • src/__tests__/fetchEventsSafe.test.ts: bisect fires on 100–101 and both single-ledger halves report the raw tip 9999 → highestLedger is 101.

4. The two smaller notes

The page-budget safety (EVENTS_PAGE_BUDGET vs a 10 000-event BATCH_SIZE, i.e. why it can't livelock the loop) is a comment above the constant, and the whole-range bisect cost is called out in the catch. The load-bearing pollOnce assumption — fromLedger is re-read inclusively, so a partially covered top ledger is picked up next poll — is a comment at the clamp.

Rebase

The min/Math.max fix had landed on the pre-#203 pollParallel signature, and main has since moved the workers onto the injected ParallelIo seams (d04392dc94), so update-branch reported a conflict in src/indexer/parallel.ts. The branch is now a merge of main at 353b5fac with the fix re-applied to the io-injected shape; nothing else from the PR was dropped or reverted, and the src/rpc.ts drain and src/indexer.ts clamp are merged with main's isXdrError narrowing in the same file.

Verification on the merge commit (00bae375), clean worktree

npx tsc --noEmit -p tsconfig.test.json   -> exit 0
npx jest --runInBand                     -> Test Suites 53 passed (53)
                                            Tests 637 passed, 1 skipped, 0 failed

mergeable_state is unstable only because the fork's CI run is action_required (awaiting approval); GitGuardian is green on this head.

@Richard-tobi

Copy link
Copy Markdown
Author

@Miracle656 Both blockers are on the head and I verified them independently at 00bae375.

  1. src/indexer/parallel.ts — pollParallel now commits Math.max(fromLedger, results.reduce((min, r) => Math.min(min, r.highestLedger), toLedger)): the lowest highestLedger across workers, clamped to [fromLedger, toLedger], so a partition cut short by the page budget pins the whole window.
  2. src/rpc.ts — fetchEventsSafe clamps on every path: single-ledger (Math.min(Math.max(startLedger, latestLedger), endLedger)), drained (Math.min(lastFullyCovered, endLedger)), and the bisect return (Math.min(Math.max(lower.highestLedger, upper.highestLedger), endLedger)).

Tests for each (src/__tests__/parallelCursor.test.ts, src/__tests__/fetchEventsSafe.test.ts) are present and pass.

Verification in a clean worktree (npm ci, npx prisma generate):

npx tsc --noEmit -p tsconfig.test.json  -> exit 0
npx jest --runInBand                    -> Test Suites 53 passed; Tests 637 passed, 1 skipped, 0 failed

Ready for re-review — thanks for the detailed pointers on where the clamp had to move.

@Richard-tobi
Richard-tobi force-pushed the fix-advance-cursor-covered-ledger-180 branch 2 times, most recently from fefe35b to 15edcb3 Compare October 15, 2026 00:15

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advance the cursor to the ledger actually covered, not the chain tip

3 participants