[aditya1702]

Start here: persistLedgerData in internal/services/ingest_live.go, then getAccountBalances in internal/serve/graphql/resolvers/account_balances.go.

Seven commits finishing the write path and keeping balances on a whole ledger. No migrations, no new flags. The live-ingest connection floor drops from 9 to 7.

Balances never reflect part of a ledger

Two things let the API show balances that don't match a whole ledger. This PR fixes both:

Cause	Fix
The balance and trustline siblings committed before the coordinating transaction that advances the cursor. After a crash, balances stayed ahead until replay caught up.	All current state commits with the cursor. Native, liquidity-pool and trustline balances are staged on the coordinating transaction next to the SAC balances, each with its foreign-key parents. The siblings are exactly the five bulk-COPY tables startup reconciliation covers, so the only crash residue is bulk rows above the cursor.
The balances field reads its 5 sources (native, trustlines, SAC, SEP-41, liquidity pools) as separate queries, so a ledger committing between two of them could show the old XLM next to the new USDC after a path payment.	One read-only REPEATABLE READ transaction around the 5 queries (db.RunInReadSnapshot, built on pgx.BeginTxFunc). The resolver walks the sources sequentially, so the transaction is used from one goroutine.

History reads were already consistent: each top-level read is bounded by the committed cursor and nested reads follow those rows. Balances and history fetched in separate requests (as Freighter does) can still straddle a ledger and converge on the next poll, as before.

Costs

• Ingest: the coordinator's serial path now includes the balance upserts. On dev mainnet the native-balance upsert takes about 11 ms per ledger and a whole persist about 57 ms, against a ~5 s ledger close.
• Serve: a balances request adds BEGIN and COMMIT round trips around queries that already ran one after another. History requests are unchanged.

One metric changes meaning: re-check any alert on it

insert_into_db now records one true wall-time value per persist commit. It used to split the commit time evenly across the ledgers in the batch, so a 4 s commit for 8 ledgers landed as eight 0.5 s observations and an overloaded write step graded as healthy. Batching only engages when persist has fallen behind, which is exactly when the dilution hid the problem. The observation also includes failed attempts and retry backoff. The per-ledger total duration keeps the amortized share so ledger durations stay comparable, so with batches larger than 1 the phases no longer sum to it. At batch size 1 nothing changes.

Correctness

1. Foreign keys are checked before the commit barrier. Every deferred foreign key now has its parent on the coordinating transaction, so SET CONSTRAINTS ALL IMMEDIATE checks them before any sibling commits. A violation rolls the batch back whole with a permanent 23503, instead of surfacing at the coordinating commit as a partial persist that crash-loops on every restart.
2. Live ingestion refuses to bootstrap from ledger 0. The cursor guard and the protocol compare-and-swap both address ledgerSeq-1, which wraps on an unsigned int, and a cursor of 0 reads as "not bootstrapped". Ledger numbers only originate at startup, so the check lives there.
3. The sibling set is a named constructor pinned by a test to data.BulkCopyTables. A sibling added without a reconciliation entry, or the reverse, fails the test instead of leaving orphan rows that crash re-ingestion. The same test ties the connection floor to the sibling count.

Handoff

4. The process→persist queue item carries the close time and the ContractData memo, not its own copy of the decoded LedgerCloseMeta. Both are derived at the end of the process pass. This is a simplification, not a memory saving: the memo's transactions still reference the meta, so it stays reachable until the ledger persists. The memo stays lazy, so extraction runs only when a processor that won its cursor swap requires contract data.

Cleanup

5. One shared COPY helper for the two account link tables. The two bodies were identical apart from table name, ID column and parent type; the public signatures are unchanged.
6. Startup reconciliation logs (at info) when the close-time bound is unresolvable and the deletes run unbounded, so a slow start can be attributed. That is expected after a checkpoint bootstrap or for an empty cursor ledger.
7. A comment pass over the persist path: each rule is stated once, leading with the rule, then the reason. The architecture and deploy docs describe the six-transaction commit set, the floor of 7, the balances snapshot (SEP-41 balances lag while protocol-migrate catches that protocol up) and how the phase metrics relate to the per-ledger duration.

Merging this finishes the chain.

---

PR 6 of 6 replacing #684 · schema → fixes → pipeline → parallel writes → table groups → final fixes

[Copilot]

Pull request overview

Improves correctness, resilience, and maintainability of the database persistence pipeline.

Changes:

• Preserves protocol classification across batches and rejects ledger zero.
• Makes commit barriers cancellation-safe and improves persistence metrics.
• Centralizes bulk-table and account-link COPY logic.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/services/protocol_processor.go	Clarifies protocol history transaction semantics.
internal/services/ingest.go	Tracks contract-to-WASM bindings.
internal/services/ingest_test.go	Adds persistence and classification regressions.
internal/services/ingest_live.go	Updates batching, commits, classification, and persistence.
internal/services/ingest_backfill.go	Reuses the authoritative bulk-table list.
internal/metrics/ingestion.go	Documents batch-level timing semantics.
internal/ingest/timescaledb.go	Derives hypertables from bulk-copy tables.
internal/ingest/ingest.go	Documents shutdown behavior.
internal/data/transactions.go	Uses the shared account-link COPY helper.
internal/data/operations.go	Uses the shared account-link COPY helper.
internal/data/ingest_store.go	Centralizes reconciliation table metadata.
internal/data/ingest_store_test.go	Reuses authoritative table metadata in tests.
internal/data/accounts.go	Adds the shared account-link COPY helper.

---

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

[aditya1702]

True, and it predates this PR: the detached commit context landed with the barrier in #711 (same lines at fef3fbff:329). A wedged COMMIT is bounded only by TCP keepalive or the pod being killed, and a kill is repaired by startup reconciliation, so nothing is lost; what is lost is a prompt shutdown. A finite deadline on the commit context is a follow-up on main, kept out of this PR so the stack's last commits stay reviewable.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

internal/services/ingest_test.go:1634

• This assertion checks only BulkCopyTables ⊆ siblings, which is the reverse of the failure described above it. Adding a new COPY sibling while omitting it from BulkCopyTables leaves every current assertion true, so the crash-recovery regression is not detected. Pin the complete ordered sibling set, including the two non-COPY families.

	for _, sibling := range siblings {
		streamed[sibling.name] = true
	}
	for _, target := range data.BulkCopyTables {
		assert.True(t, streamed[target.Table], "no persist sibling streams %s", target.Table)

internal/services/ingest_live.go:355

• context.WithoutCancel also removes any deadline, so a stalled commit can now block shutdown indefinitely while holding all eight persist connections. The advisory-lock cleanup in this file pairs WithoutCancel with a finite timeout for exactly this failure mode. Use a bounded detached context sized to complete the barrier within the deployment termination grace period.

	commitCtx := context.WithoutCancel(ctx)
	for i, s := range siblings {
		if commitErr := siblingTxs[i].Commit(commitCtx); commitErr != nil {

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

internal/services/ingest_test.go:2601

• This comment names a test that does not exist and describes the removed nil-plan/batch-cut design; the following test passes a populated classification map and exercises the new merged-plan behavior. Remove this stale block so the regression coverage is not misrepresented.

// Test_getEffectiveProtocolContracts_NilClassificationKeepsCommitted pins the
// mid-batch semantics: ledgers riding behind a batch head run with no
// classification plan (classification == nil), and their buffered contracts
// are pure re-observations — every binding was already seen committed, the
// batch cut guarantees it. Committed membership must stand untouched;

internal/services/ingest_live.go:969

• This “safe without any cut” claim contradicts the PR's binding-aware cut requirement and is not correct for a contract rebound later in the batch. Because the merged contract map keeps only the final binding, the plan does not resolve the earlier WASM; staging the earlier ledger then removes that contract from protocol membership and can silently drop its events. Restore the batch cut on binding changes, or retain every intermediate binding in the classification plan.

// Merging the inputs is what makes a batch safe without any cut. A contract
// bound to a wasm uploaded by an earlier ledger of the same batch would, per
// ledger, have to resolve that wasm's verdict from protocol_wasms — a pool
// read that cannot see rows the batch has not committed. Supplied together,
// prepareClassificationPlan classifies the wasm from its buffered bytecode

internal/services/ingest_test.go:1634

• This assertion is one-way: if a bulk sibling is accidentally removed from BulkCopyTables, the loop no longer checks it and the test still passes—the exact orphan-reconciliation failure this test claims to prevent. Compare the complete sibling-name set against the registry plus the two known balance siblings.

	for _, target := range data.BulkCopyTables {
		assert.True(t, streamed[target.Table], "no persist sibling streams %s", target.Table)

[Copilot]

Copilot review overview

🟡 Changes recommended

The implemented whole-batch classification model materially conflicts with the PR’s documented batching contract.

Review effort: Balanced
Findings: 4 · 3

Open (7)

• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Update PR description to reflect whole-batch classification semantics · New
• This comment still describes a per-item classification plan and says nil means this ledger had…
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

[Copilot]

Copilot review overview

🟡 Changes recommended

The key metric cardinality change lacks regression coverage, and a new test comment documents obsolete behavior.

Review effort: Balanced
Findings: 4 · 3

Open (7)

• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Add regression test for one-observation-per-commit metric semantics · New
• Update PR description to reflect whole-batch classification semantics
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

Resolved since last review (1)

• This comment still describes a per-item classification plan and says nil means this ledger had…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Detached commits can block shutdown indefinitely, and key batching and metrics guarantees need aligned documentation and regression coverage.

Review effort: Balanced
Findings: 4 · 3

Open (7)

• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Add regression test for one-observation-per-commit metric semantics
• Update PR description to reflect whole-batch classification semantics
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

[Copilot]

Copilot review overview

🟡 Changes recommended

One test still calls the changed helper with the removed argument, preventing the Go test package from compiling.

Review effort: Balanced
Findings: 1 · 4 · 3

Open (8)

• Update remaining oneLedger caller to the new signature · New
• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Add regression test for one-observation-per-commit metric semantics
• Update PR description to reflect whole-batch classification semantics
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

[Copilot]

Copilot review overview

🔵 Needs a closer look

The batching implementation materially differs from the documented design, leaving its intended correctness guarantees ambiguous.

Review effort: Balanced
Findings: 4 · 3

Open (7)

• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Add regression test for one-observation-per-commit metric semantics
• Update PR description to reflect whole-batch classification semantics
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

Resolved since last review (1)

• Update remaining oneLedger caller to the new signature

[Copilot]

Copilot review overview

🟡 Changes recommended

The implemented batching and queue-payload designs materially differ from the safety and memory behavior documented for the PR.

Review effort: Balanced
Findings: 5 · 3

Open (8)

• Processed ledger retains full transactions in the persistence queue · New
• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…
• Add regression test for one-observation-per-commit metric semantics
• Update PR description to reflect whole-batch classification semantics
• This block documents a nonexistent NilClassificationKeepsCommitted test and the old “mid-batch…

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🔵 Needs a closer look

Ledger consistency and crash recovery across six transactions warrant final human review and database-backed validation.

2 open issues

• The new shutdown-safety guarantee is not covered by a test: existing persist tests cover…
• The detached context also removes the caller's deadline, so a lost database connection or wedged…

4 resolved since last review

• Processed ledger retains full transactions in the persistence queue
• Using BulkCopyTables to define the test's expected tables makes this reconciliation regression…
• This does not actually bound the seen sets: the size is checked before adding the current batch, so…
• Update PR description to reflect whole-batch classification semantics

_{Review effort: Balanced}