Three small fixes: checkpoint pool shares, worker pool sizing, metric buckets by aditya1702 · Pull Request #705 · stellar/wallet-backend

aditya1702 · 2026-08-26T01:08:32Z

Three unrelated fixes. Read them in any order.

1. Checkpoint drops share rows for pools it skipped — internal/services/checkpoint.go

The checkpoint skips pool types liquidity_pools can't store, but it was saving their share rows anyway. With the new foreign key from #704, a share row with no matching pool now fails the whole checkpoint load. Share rows are held back until every pool has been read, then filtered against the skipped ones.

2. Worker pool sizes off GOMAXPROCS instead of NumCPU — internal/services/ingest.go

runtime.NumCPU() reports the node's cores, not the container's limit. A 6-core pod on a 36-core node was running 72 workers on 6 threads. GOMAXPROCS(0) respects the limit.

3. Metric buckets line up with the ledger close time — internal/metrics/ingestion.go

The histogram jumped 0.5 → 1 → 2 seconds, so a stage taking 0.9s and one taking 1.9s landed in the same p99 bucket. We need to know whether the slowest stage stays under the ledger close time, and the old buckets couldn't tell us. Boundaries are now 0.6 and 1.0, with 0.5 and 0.75 just below.

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

Copilot

Pull request overview

Addresses checkpoint pool integrity, container-aware worker sizing, and ingestion metric precision.

Changes:

Rejects unsupported checkpoint liquidity pools.
Sizes indexer workers from GOMAXPROCS.
Refines phase-duration histogram buckets.

Reviewed changes

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

Show a summary per file

File	Description
`internal/services/ingest.go`	Uses container-aware CPU concurrency.
`internal/services/checkpoint.go`	Adds fail-fast handling for unsupported pools.
`internal/services/checkpoint_test.go`	Tests unsupported-pool failure behavior.
`internal/metrics/ingestion.go`	Adds finer latency boundaries.
`internal/metrics/ingestion_test.go`	Updates expected bucket count.

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

+			// Unreachable from a decoded archive: LiquidityPoolType admits only
+			// constant-product, so the decoder rejects any other discriminant.
+			// Fail loudly rather than skip, because liquidity_pool_balances
+			// references liquidity_pools: silently dropping a pool would drop
+			// every holder's shares with it.
+			return fmt.Errorf("liquidity pool %s has body type %d, which liquidity_pools cannot represent",
+				processors.PoolIDToString(pool.LiquidityPoolId), pool.Body.Type)


Copilot

Pull request overview

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

Suppressed comments (2)

internal/metrics/ingestion_test.go:102

This only pins the number of buckets, not the new 0.5/0.6/0.75/1.0 grading boundaries that the change relies on. A different set of 13 boundaries would pass while silently changing p99 interpretation; assert each bucket's upper bound.

			assert.Len(t, h.GetBucket(), 13) // 13 custom boundaries

internal/services/checkpoint.go:418

This contradicts the PR's stated checkpoint fix. The description says unsupported pools are skipped and their share rows are filtered after the full scan, but this branch instead aborts the entire checkpoint; no share rows are held or filtered anywhere in batch. Either implement the documented filtering behavior (while retaining bounded flushing) or update the PR title/description to explicitly make aborting the intended behavior.

			return fmt.Errorf("liquidity pool %s has body type %d, which liquidity_pools cannot represent",
				processors.PoolIDToString(pool.LiquidityPoolId), pool.Body.Type)

Copilot

Pull request overview

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

Suppressed comments (1)

internal/services/checkpoint.go:416

The PR description says pool-share rows are “held back…then filtered against the skipped ones”, but this change makes an unsupported liquidity pool body abort the checkpoint load instead. Either update the PR description to reflect the new failure behavior, or implement the described hold-and-filter approach so unsupported pools can be skipped without failing the whole checkpoint.

			// Unreachable from a decoded archive: LiquidityPoolType admits only
			// constant-product, so the decoder rejects any other discriminant.
			// Fail loudly rather than skip, because liquidity_pool_balances
			// references liquidity_pools: silently dropping a pool would drop
			// every holder's shares with it.

Copilot

Pull request overview

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

Suppressed comments (1)

internal/services/checkpoint.go:418

This does not implement the checkpoint behavior described by the PR: unsupported pools now abort the entire load instead of recording the skipped pool IDs and filtering their share rows. Because the current archive decoder rejects every non-constant-product discriminant, this branch changes no real checkpoint today; if another body type becomes decodable, it makes checkpoint startup fail rather than preventing orphan shares. Please either implement the stated share filtering or update the PR's contract and justify the intentional fail-fast behavior.

			return fmt.Errorf("liquidity pool %s has body type %d, which liquidity_pools cannot represent",
				processors.PoolIDToString(pool.LiquidityPoolId), pool.Body.Type)

Copilot

Pull request overview

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

Suppressed comments (1)

internal/services/checkpoint.go:418

This implements the opposite of the checkpoint behavior described by the PR: an unrepresentable pool aborts the entire load, while its share rows are still streamed and never held or filtered. The stated fix says the checkpoint should complete after dropping shares for skipped pools, so either implement that buffering/filtering path (while preserving the existing bounded flush behavior) or update the PR scope and description to make the intentional fail-fast behavior explicit.

			return fmt.Errorf("liquidity pool %s has body type %d, which liquidity_pools cannot represent",
				processors.PoolIDToString(pool.LiquidityPoolId), pool.Body.Type)

Copilot

Copilot review overview

🟡 Changes recommended

The checkpoint behavior contradicts the stated filtering design, and the CLI help remains outdated.

Review effort: Balanced
Findings: 1 · 1

Open (2)

This does not implement the checkpoint behavior described by the PR: it aborts the entire load for…
Update backfill-workers help text for GOMAXPROCS default · New

Copilot

Copilot review overview

🔵 Needs a closer look

Checkpoint behavior contradicts the PR’s stated filtering strategy by aborting instead.

Review effort: Balanced
Findings: 1

Open (1)

This does not implement the checkpoint behavior described by the PR: it aborts the entire load for…

Resolved since last review (1)

Update backfill-workers help text for GOMAXPROCS default

The pool path skips liquidity pool bodies that liquidity_pools cannot represent (non-constant-product), but shares were captured unconditionally — and now that liquidity_pool_balances references liquidity_pools, a share row without its pool aborts the whole checkpoint load at commit. A share's pool entry can arrive in any bucket, so shares are held until every pool entry has been read, then joined to the final flush minus the skipped pools'. Covered by a regression test with an unsupported pool body.

…cores runtime.NumCPU() reports the node's cores in a container with a CPU limit, so a 6-core pod on a 36-core node ran 72 indexer workers over 6 schedulable threads — pure scheduler churn on CPU-bound work. GOMAXPROCS honors the limit.

…e bar The phase histogram stepped 0.5 -> 1 -> 2 seconds, so a stage sitting at 0.9s and a stage sitting at 1.9s both reported the same p99 bucket. The pipeline's contract is that the slowest stage's p99 stays under the ledger close time, so the metric could not grade the thing it exists to grade. Boundaries now land on 0.6s and 1s, which bracket the close times this service ingests at, with 0.5 and 0.75 just below them so movement between runs stays visible rather than rounded away.

Pool-share trustlines were buffered in memory for the whole bucket scan so the final flush could drop the shares of any pool the capture had skipped. The skip it defends against cannot occur: LiquidityPoolType has one enum value, its DecodeFrom rejects every other discriminant, and GetConstantProduct returns false only for a discriminant the decoder never produces. Reaching the branch in a test required hand-building a LiquidityPoolEntryBody the wire format cannot express. The buffering bought nothing else either. The load runs in one transaction and liquidity_pool_balances references liquidity_pools with a DEFERRABLE INITIALLY DEFERRED constraint, so insert ordering within the load was already irrelevant. Shares go straight into the batch again, and an unrepresentable pool body aborts the load instead of being skipped: if a second pool type ever arrives, dropping the pool would drop every holder's shares with it, and a checkpoint that quietly loses balances is worse than one that fails. processEntry returns an error to carry it, which the scan loop already propagates.

The ledger-indexer pool already sizes off GOMAXPROCS, which the Go 1.25+ runtime derives from the container CPU limit, while the backfill pool still defaulted to runtime.NumCPU() and so to the node's core count. Both pools now use the same basis, and the comments say where the limit comes from.

The test only counted the buckets, so swapping the 0.6 and 0.75 grading bars for any other values would still pass. Assert each upper bound instead.

Re-bracketing the phase histogram on the close-time bar dropped the two tail buckets, leaving 10s as the largest finite boundary. histogram_quantile reports the highest finite bound when the quantile falls in +Inf, so an 11-second and a 60-second stall would both read as 10s and a severe slowdown would be invisible. Keep the tail.

Copilot

Copilot review overview

🔵 Needs a closer look

The checkpoint implementation aborts on unsupported pools rather than performing the share filtering described by the PR.

Review effort: Balanced
Findings: 1

Open (1)

This does not implement the checkpoint behavior described by the PR: it aborts the entire load for…

aditya1702 mentioned this pull request Aug 26, 2026

Pipeline live ingestion into fetch/process/persist with seven parallel write streams #684

Closed

aditya1702 changed the title ~~Three independent fixes: checkpoint pool shares, indexer pool sizing, phase buckets~~ Three small fixes: checkpoint pool shares, worker pool sizing, metric buckets Aug 26, 2026

aditya1702 force-pushed the persist/2-fixes branch 2 times, most recently from 96178a8 to 8d9cf88 Compare August 27, 2026 02:52

aristidesstaffieri reviewed Aug 27, 2026

View reviewed changes

Comment thread internal/services/ingest.go

aditya1702 force-pushed the persist/2-fixes branch from 8d9cf88 to c31d259 Compare September 3, 2026 23:46

Copilot AI balanced review requested due to automatic review settings September 3, 2026 23:46

Copilot started reviewing on behalf of aditya1702 September 3, 2026 23:46

View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 19:30

aditya1702 force-pushed the persist/2-fixes branch from c31d259 to 5c034bc Compare September 4, 2026 19:30

Copilot started reviewing on behalf of aditya1702 September 4, 2026 19:31

View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 9, 2026 21:07

aditya1702 force-pushed the persist/2-fixes branch from 5c034bc to ad8b429 Compare September 9, 2026 21:07

Copilot started reviewing on behalf of aditya1702 September 9, 2026 21:07

View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread internal/services/ingest.go Outdated

JiahuiWho reviewed Sep 11, 2026

View reviewed changes

Comment thread internal/services/ingest.go Outdated

JiahuiWho reviewed Sep 11, 2026

View reviewed changes

Comment thread internal/services/ingest.go Outdated

aditya1702 self-assigned this Sep 12, 2026

aditya1702 added this to the Wallet Backend O3 Performance Testing milestone Sep 12, 2026

aditya1702 force-pushed the persist/2-fixes branch from ad8b429 to 6ba1a87 Compare September 15, 2026 15:41

aditya1702 force-pushed the persist/2-fixes branch from 6ba1a87 to 64ca108 Compare September 15, 2026 16:03

Base automatically changed from persist/1-schema to main September 15, 2026 18:18

aditya1702 force-pushed the persist/2-fixes branch from 64ca108 to 4be455c Compare September 15, 2026 18:18

Copilot AI review requested due to automatic review settings September 15, 2026 20:20

Copilot started reviewing on behalf of aditya1702 September 15, 2026 20:21

View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread internal/metrics/ingestion.go Outdated

Copilot AI review requested due to automatic review settings September 15, 2026 20:35

Copilot started reviewing on behalf of aditya1702 September 15, 2026 20:36

View session

aditya1702 requested a review from aristidesstaffieri September 15, 2026 20:36

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 1, 2026 21:06

aditya1702 force-pushed the persist/2-fixes branch from 97f2c32 to 6a0d1a9 Compare October 1, 2026 21:06

Copilot started reviewing on behalf of aditya1702 October 1, 2026 21:06

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread internal/ingest/ingest.go

aditya1702 force-pushed the persist/2-fixes branch from 6a0d1a9 to e6d75f4 Compare October 5, 2026 14:57

Copilot AI balanced review requested due to automatic review settings October 5, 2026 14:57

Copilot started reviewing on behalf of aditya1702 October 5, 2026 14:58

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

aditya1702 added 7 commits October 5, 2026 13:35

test(metrics): pin every phase-duration bucket boundary

6cfb49a

The test only counted the buckets, so swapping the 0.6 and 0.75 grading bars for any other values would still pass. Assert each upper bound instead.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 17:35

aditya1702 force-pushed the persist/2-fixes branch from e6d75f4 to 41a75e4 Compare October 5, 2026 17:35

Copilot started reviewing on behalf of aditya1702 October 5, 2026 17:36

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

aditya1702 merged commit e5310c0 into main Oct 5, 2026
10 checks passed

aditya1702 deleted the persist/2-fixes branch October 5, 2026 17:44

Navigation Menu

Repository navigation

Three small fixes: checkpoint pool shares, worker pool sizing, metric buckets - #705

Three small fixes: checkpoint pool shares, worker pool sizing, metric buckets#705
aditya1702 merged 7 commits into
mainfrom
persist/2-fixes

aditya1702 commented Aug 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aditya1702 commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aditya1702 commented Aug 26, 2026 •

edited

Loading