Commit ingestion transactions outside the ledger-range cache lock

[cjonas9]

What

rpcv1 requests no longer stall while a ledger is being ingested. The ingestion commit used to hold the ledger-range cache lock across the SQLite commit, so every request waited out the whole ledger write (tens to hundreds of milliseconds, once per ledger).

• Commit reads the post-trim oldest ledger inside its own write tx, commits, then publishes both cache bounds under one short lock. The cache only ever holds committed bounds, a failed commit leaves it untouched, and there is no trim predicate to keep in step with trimLedgers.
• getEvents validates startLedger and scans inside one read snapshot (store.EventReader.NewTx returns an EventReaderTx, a LedgerReaderTx that also scans events), as getLedgers and getTransactions already did.
• Snapshot readers take both bounds from their own snapshot (one statement, two index probes) and use the cached pair only for close times, falling back to the row reads when a commit landed after NewTx or the cache was reset. The cache is purely a fast path; no reader depends on it for correctness.
• The cache is primed from the stored bounds when the DB is opened and republished by every commit, so the lazy fills, the "0 means unknown" convention and the post-backfill ResetCache are gone; a zero latest simply means an empty DB.
• NewEventReader drops its never-read passphrase parameter.

Cost on a trivial in-memory DB: the snapshot range read is ~18µs against ~21µs for the old oldest-row read, so getLedgers/getTransactions get slightly cheaper and getEvents pays ~10µs over its old pooled path (BEGIN/ROLLBACK plus the bounds statement). Prepared statements would remove most of that across all three endpoints and are a follow-up.

See comments of #1003 for the speedup from this round of optimizations, and #1018 for what the original speeds are pre-optimization + pre-views-adoption.

Why

This created a universal speedup across each endpoint in RPCv1, whereas previously, we'd seen on-LCM-ingestion-periodic latency spikes: the commit stall set p99 on every endpoint.

Known limitations

For the microseconds between tx.Commit() returning and the publish, the pooled readers (getTransaction, getHealth, getFeeStats, sendTransaction) echo bounds one ledger behind in their responses. Snapshot readers are unaffected.

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[Copilot]

Copilot review overview

🟡 Changes recommended

Publishing the oldest bound before commit can reject valid requests and leaves stale cache state if the commit fails.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Publish ledger bounds only after transaction commit · New

What changed in this PR

Moves RPCv1 ingestion commits outside the ledger-range cache lock to reduce request latency.

Changes:

• Reads the oldest retained ledger during ingestion.
• Updates oldest/latest cache bounds around SQLite commits.
• Tests cache publication after retention trimming.

File	Description
sqlitedb/​ledger.go	Extracts oldest-ledger lookup.
sqlitedb/​ledger_test.go	Tests trim-time cache publication.
sqlitedb/​db.go	Reorders commit and cache updates.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

The central non-blocking concurrency behavior lacks a regression test.

Review effort: Balanced
Findings: 1

Open (1)

• Add concurrency test proving reads proceed during commit · New

Resolved since last review (1)

• Publish ledger bounds only after transaction commit

[Copilot]

Copilot review overview

🟡 Changes recommended

Deferred SQLite transactions can observe database bounds newer than their cached latest ledger, producing an invalid event range.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• BeginTx fails to pin cache snapshot before releasing lock · New
• Add concurrency test proving reads proceed during commit

[Copilot]

Copilot review overview

🔵 Needs a closer look

The concurrency-sensitive cache and SQLite snapshot changes warrant final human review despite no confirmed correctness defects.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🟡 Changes recommended

Concurrent commits can publish cache bounds out of order, and initialization errors leak the opened database session.

Review effort: Balanced
Findings: 1

Open (1)

• Prevent out-of-order cache publication from restoring stale bounds · New

Previously missed (1)

In code that hasn't changed since last review

Close SQLite session before returning initialization failure

cmd/​stellar-rpc/​internal/​rpcv1/​sqlitedb/​db.go:245

This new initialization failure path returns after the SQLite session has already been opened, but the caller receives no handle to close it. Close d before returning (as openSQLiteDB does on migration failure) so callers that handle the error do not leak the connection and database file descriptor.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The newly opened database session leaks when initial cache priming fails.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Prevent out-of-order cache publication from restoring stale bounds

Previously missed (1)

In code that hasn't changed since last review

Close session when cache priming fails

cmd/​stellar-rpc/​internal/​rpcv1/​sqlitedb/​db.go:245

If cache priming fails, both open functions return this error without closing the newly created session, leaving its connection pool and file descriptors owned by nobody. The migration-error path above explicitly closes the session; do the same here before returning.

[Copilot]

Copilot review overview

🔵 Needs a closer look

A cache-priming failure leaks the newly opened SQLite session.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Close SQLite session when cache priming fails

cmd/​stellar-rpc/​internal/​rpcv1/​sqlitedb/​db.go:245

If cache priming fails, both open functions return this error while leaving the newly opened SQLite session (and its connection pool) alive. This is especially visible to callers that retry opening the database. Close the session on this post-open initialization failure, as openSQLiteDB already does when migrations fail.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The central event trim-race guarantee lacks a regression test covering validation and scanning through the same snapshot.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Add regression test for event scanning on pinned transaction snapshots

cmd/​stellar-rpc/​internal/​methods/​get_events.go:132

The new range/scan snapshot guarantee is not covered by a regression test. The added ledger test only checks cache-bound reconciliation; it would not catch eventScanner accidentally using the pool instead of tx.tx, which would reintroduce the trim race this change is intended to fix. Add a test that pins an EventReaderTx snapshot, commits a trim, and verifies the transaction can still scan the event whose startLedger was validated in that snapshot.

[tamirms]

Approving at e101bc0. The cache is primed at open and republished once per commit, after the commit; snapshot readers take both bounds from their own transaction; getEvents validates and scans in one snapshot; getTransaction clamps its latest bound to a just-found transaction.

One non-blocking suggestion: Copilot's regression test for the snapshot guarantee, open an event snapshot, commit a trim underneath it, and check the snapshot still scans the validated event, needs no pause seam since SQLite's snapshot isolation makes it deterministic, and it is the one test that would catch the event scanner reading the pool again.

[cjonas9]

@tamirms Thanks! Also,

One non-blocking suggestion: Copilot's regression test for the snapshot guarantee, open an event snapshot, commit a trim underneath it, and check the snapshot still scans the validated event, needs no pause seam since SQLite's snapshot isolation makes it deterministic, and it is the one test that would catch the event scanner reading the pool again.

Added as TestEventReaderTx_SurvivesTrim. Two things had to differ from the sketch for it to test what it claims:

• NewTx does not establish the snapshot. SQLite's deferred BEGIN takes it at the first statement, so the test calls GetLedgerRange on the snapshot before trimming; without that, the trim lands first and the test passes for the wrong reason.
• The trim is two raw DELETEs with the same predicates trimLedgers/trimEvents use, not a Commit. Commit's TRUNCATE checkpoint blocks on any reader holding an older snapshot for the busy timeout (5s by default), which would make the test take five seconds for no extra coverage; snapshot isolation is the property under test and the deletes commit the same way.

It pins the snapshot's event count against the pool's zero after the trim. Mutation-checked with go test -overlay: wiring eventReaderTx's scanner to the pool instead of its tx fails it (expected 4, actual 0).

[Copilot]

Copilot review overview

🔵 Needs a closer look

The SQLite snapshot and cache-publication changes warrant final human verification under concurrent ingestion and request load.

Review effort: Balanced
Findings: None