Skip to content

937 follow-ups: getTransactions cursor guards, getHealth first-commit gate, snapshot gauge, method-table completeness test - #953

Open
karthikiyer56 wants to merge 7 commits into
feature/full-historyfrom
karthik/937-followups
Open

937 follow-ups: getTransactions cursor guards, getHealth first-commit gate, snapshot gauge, method-table completeness test#953
karthikiyer56 wants to merge 7 commits into
feature/full-historyfrom
karthik/937-followups

Conversation

@karthikiyer56

@karthikiyer56 karthikiyer56 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Executes the remaining #937 items against the current branch tip. Four items land here; H2 stays deferred in #937.

What changed

Item Change Files
G1 open_snapshots gauge: a process-wide tally of unreleased RocksDB snapshots, exported beside the serving-invariant counters. A leaked read view is visible on a dashboard, not only in a teardown log rpcv2/rocksdb/rocksdb.go, rpcv2/observability/observability.go
H1 Completeness test ties the per-method tables together: every MethodsConfig method must appear in limitsByMethod and be checked by validateService. Apply already ties limits to the served specs rpcv2/methods_completeness_test.go (test-only)
F1 + F2 getTransactions never returns a cursor below the request's cursor. A caught-up poller gets its cursor echoed back instead of "0" (#745) or a regressed cursor that re-delivers a consumed ledger methods/get_transactions.go, CHANGELOG.md
Review follow-up getLedgers gets the same caught-up contract: a cursor at or past the tip returns an empty page with the cursor echoed, instead of -32602. The cursor parse keeps only the lower-bound rejection; a max-uint32 cursor wraps to start 0 and stays rejected methods/get_ledgers.go, CHANGELOG.md
I1 v2 getHealth fails until this run's ingestion commits its first ledger. The registry records the boot-seeded latest ledger; the gate compares against it rpcv2/query/registry.go, rpcv2/jsonrpc.go

Wire-visible changes

Method Before After
getTransactions (v1 AND v2 — shared handler) Cursor at/past the tip returned "0"; resending it failed every poll with -32602. A consumed ledger's cursor regressed and the next poll re-delivered its transactions The request's cursor is echoed back on an empty caught-up page. The returned cursor always fetches what comes next. Fixes #745. CHANGELOG carries a loud entry
getLedgers (v1 AND v2 — shared handler) Resending the last page's cursor at the tip failed with -32602 — the same code a malformed cursor gets — until the next ledger closed Empty page with the cursor echoed back. Below-oldest cursors and explicit above-tip startLedger still error. CHANGELOG carries a loud entry
getHealth (v2 only) Healthy for up to max_healthy_ledger_latency after a restart, off the previous run's close time — even in a crash loop Fails with -32603 until the first commit of this run lands (about one ledger interval after boot). v1 unchanged

Not in this PR

Item Why
H2 (cold .idx reader cache) Deferred in #937: needs a written reader-lifetime design first; a wrong answer is a use-after-close on the read path

Notes

  • Each item is one commit, in the order reviewed.
  • open_snapshots mirrors the deferredCloseOps pattern: a package atomic read by a GaugeFunc.
  • The H1 test is reflection over MethodsConfig: adding a method field extends the checks with no test edits.

snapRefs only spoke at teardown; a leaked read view was invisible while
the process ran. A package-level tally mirrors deferredCloseOps and
observability exports it as open_snapshots.
MethodsConfig fields drive the check: limitsByMethod must key every
method, and validateService must reject a zero queue limit on each.
Apply already ties limits to the served specs, so the tables cannot
drift apart silently.
A caught-up poller used to get the zero cursor "0" (then -32602 on
every retry, #745) or a regressed cursor that re-delivered a consumed
ledger's transactions. The walk's result is now floored at the
request's cursor, so the returned token always fetches what comes
next. Shared handler: changes v1 wire behavior too; CHANGELOG says so.
Close times survive restarts, so a restarted node reported healthy off
the previous run's commit for up to max_healthy_ledger_latency. The
registry records the boot-seeded latest ledger; getHealth fails until
ingestion advances past it. v1 keeps the shared behavior.
@karthikiyer56
karthikiyer56 requested review from a team and a balanced review from Copilot August 26, 2026 01:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds follow-ups for pagination correctness, startup health gating, snapshot observability, and method-table completeness.

Changes:

  • Prevents getTransactions cursor regression.
  • Gates v2 health until the first post-boot ingestion commit.
  • Adds snapshot metrics and method-configuration completeness tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
CHANGELOG.md Documents cursor behavior changes.
methods/get_transactions.go Floors responses at the requested cursor.
methods/get_transactions_test.go Tests caught-up cursor handling.
rpcv2/jsonrpc.go Adds the health startup gate.
rpcv2/jsonrpc_test.go Tests pre/post-commit health responses.
rpcv2/methods_completeness_test.go Verifies method-table coverage.
rpcv2/observability/observability.go Exports the snapshot gauge.
rpcv2/observability/observability_test.go Verifies gauge registration.
rpcv2/query/registry.go Tracks the boot ledger sequence.
rpcv2/query/registry_test.go Tests commit-since-boot detection.
rpcv2/rocksdb/rocksdb.go Tracks unreleased snapshots globally.
rpcv2/rocksdb/snapshot_test.go Tests snapshot gauge accounting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// produces nothing and leaves the cursor below the request's own — at the
// zero value, or at the consumed ledger's start. Echo the request's cursor
// instead: the returned token must always fetch what comes next (#745).
if requestCursor != nil && cursor.ToInt64() < requestCursor.ToInt64() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While verifying the cursor guard here I checked how the other two paginated methods behave for the same caught-up poll. getEvents is fine: an empty page returns a max cursor at the last scanned ledger, which stays valid when resent at the tip. getLedgers still breaks the polling loop, and it fails with an error instead of an empty page:

  1. A poller reaches the tip ledger T. getLedgers returns its last page with cursor: "T".
  2. The poller resends cursor: "T". parseCursor computes start = T + 1 and rejects it, because IsLedgerWithinRange requires start <= T (get_ledgers.go, line 154).
  3. The poller gets -32602: "cursor ('T') must be between the oldest ledger: X and the latest ledger: T". Every poll fails until the next ledger closes.

This self-heals within one ledger interval, so nobody gets permanently stuck the way #745 clients did. But it is the steady state for anyone polling faster than the close interval, and the code is the same -32602 a malformed cursor gets, so a client cannot tell "wait and retry" apart from "my cursor is garbage" without parsing the message text. The handler already contains the intended behavior: the empty-result branch echoes request.Pagination.Cursor (line 106), but validation rejects the caught-up request before that branch can run, so it is unreachable for exactly the case it was written for.

Since the changelog entry in this PR states the contract generally ("the returned cursor now always does what the docs promise"), I think we should bring getLedgers in line, either as one more commit here or as an immediate follow-up, ideally landing in the same release so the caught-up contract changes once for both methods. The shape I have in mind mirrors the getTransactions fix: parseCursor keeps only the lower-bound rejection (a cursor below the oldest ledger is real data loss and should stay an error), and the handler returns early for a cursor at or past the tip, before end is computed or fetchLedgers runs:

// A caught-up poller's cursor points at or past the tip. Echo it back on an
// empty page instead of rejecting the server's own token.
if request.Pagination != nil && request.Pagination.Cursor != "" &&
	start > availableLedgerRange.LastLedger {
	return protocol.GetLedgersResponse{
		Ledgers:               []protocol.LedgerInfo{},
		LatestLedger:          ledgerRange.LastLedger.Sequence,
		LatestLedgerCloseTime: ledgerRange.LastLedger.CloseTime,
		OldestLedger:          ledgerRange.FirstLedger.Sequence,
		OldestLedgerCloseTime: ledgerRange.FirstLedger.CloseTime,
		Cursor:                request.Pagination.Cursor,
	}, nil
}

Two details worth pinning in tests: an explicit startLedger above the tip should stay an error, the same asymmetry getTransactions has (only the server-issued token gets the echo, an explicit out-of-range start is a client mistake), and the cursor "4294967295" makes start wrap to 0 in parseCursor, which the retained lower-bound check needs to catch. Placing the early return before end := start + uint32(limit) - 1 also keeps that addition from ever running with an above-tip start.

If you'd rather defer this, a row on #937 plus scoping the changelog sentence to getTransactions would keep the docs honest in the meantime.

@karthikiyer56 karthikiyer56 Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair callout — done in 37d8424, making getLedgers toe the same cursor-semantics line as getTransactions.

Resending the last page's cursor at the tip failed with the same
-32602 a malformed cursor gets, until the next ledger closed. The
cursor parse now rejects only the lower bound (below the oldest ledger
is data loss); at or past the tip returns an empty page with the
cursor echoed, matching getTransactions. A max-uint32 cursor wraps to
start 0 and stays rejected.
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.

Post-#908 follow-ups: store-boundary error observability, markErr enforcement, boot-window tip reads

3 participants