Skip to content

Fix lookup-table reaper deleting rows referenced by concurrent ingestion - #222

Merged
tamirms merged 3 commits into
mainfrom
fix-reaper-lookup-table-snapshot-race
Aug 15, 2026
Merged

Fix lookup-table reaper deleting rows referenced by concurrent ingestion#222
tamirms merged 3 commits into
mainfrom
fix-reaper-lookup-table-snapshot-race

Conversation

@tamirms

@tamirms tamirms commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

What

The lookup-table reaper could delete a history lookup row (history_accounts, history_assets, history_claimable_balances, history_liquidity_pools) while live ingestion was concurrently inserting a reference to it, leaving a dangling reference in the history tables (history_operation_participants, history_effects, history_trades, …). The lookup row's id is the join key for an account's history, so at the API level this surfaces as:

  • GET /accounts/{address}200 with the correct balance (the account looks healthy), but
  • GET /accounts/{address}/payments (and /operations, /effects, /trades) → 404, because the join to history_accounts returns no rows.

Once the address transacts again, the loader re-inserts it with a new id, so those endpoints return 200 again but silently omit every earlier record. The operation rows themselves are never deleted — only the join key — so a reingest of the retained range recovers them.

Root cause

constructDeleteLookupTableRowsQuery built the reaper's delete as a single statement that both took a FOR UPDATE row lock and ran the NOT EXISTS orphan check:

WITH ha_batch AS (SELECT id FROM history_accounts WHERE id IN (...) ORDER BY id ASC FOR UPDATE)
DELETE FROM history_accounts WHERE id IN (
  SELECT e1.id FROM ha_batch e1 WHERE NOT EXISTS (... references ...))

The FOR UPDATE lock was intended to make the reaper block until any concurrent ingestion (which locks the same rows FOR KEY SHARE) commits. It does block — but under the default READ COMMITTED isolation the statement's snapshot is fixed when the statement starts, before it blocks. When the ingestion transaction it blocked on commits, the NOT EXISTS sub-queries still evaluate against the pre-commit snapshot, don't see the just-inserted references, report the row as orphaned, and delete it. Blocking on the lock does not refresh the snapshot the check runs under.

Fix

Split the lock and the orphan check into two separate statements within the reaper's transaction:

  1. SELECT id FROM <table> WHERE id IN (...) ORDER BY id ASC FOR UPDATE — take the locks and wait out any in-flight ingestion.
  2. DELETE FROM <table> WHERE id IN (...) AND NOT EXISTS (... references ...) — a separate statement, so its snapshot is taken after the lock wait resolves and it observes the references committed by the ingestion transaction the reaper blocked on. The FOR UPDATE locks from statement 1 are held for the rest of the transaction, so no new ingestion can insert a reference in between.

Scope

This fixes concurrency with live ingestion, which is where the reaper runs concurrently by design. Reaping concurrently with db reingest range (the non---force path, which uses ConcurrentInserts and does not take the FOR KEY SHARE lock on the parent) is a separate, narrower window and is left as a follow-up.

Tests

  • TestReapDoesNotDeleteConcurrentlyReferencedRows (new): drives the exact interleaving — ingestion locks the account FOR KEY SHARE and inserts a referencing history_operation_participants row, uncommitted; the reaper blocks on the lock; ingestion commits; the reaper must leave the row in place. This test fails against the previous single-statement query (deletes the row) and passes with the fix.
  • Updated TestConstructDeleteLookupTableRowsQuery and added TestConstructLockLookupTableRowsQuery for the new query builders.

🤖 Generated with Claude Code

The lookup-table reaper could delete a history lookup row (history_accounts,
history_assets, history_claimable_balances, history_liquidity_pools) that live
ingestion was concurrently inserting a reference to, leaving a dangling
reference in the history tables (history_operation_participants,
history_effects, history_trades, ...). At the API level this surfaces as a 404
on an account's history endpoints, or a 200 that silently omits earlier records
once the address is re-inserted with a new id.

The reaper's DELETE combined the FOR UPDATE row lock and the NOT EXISTS orphan
check in a single statement. Under READ COMMITTED the statement's snapshot is
taken before it blocks on the lock, so once the ingestion transaction it
blocked on committed, the NOT EXISTS sub-queries still ran against the
pre-commit snapshot, did not see the newly inserted references, and the row was
deleted while in use. The FOR UPDATE lock made the reaper block but did not
refresh the snapshot used by the check.

Split the lock and the orphan check into two separate statements in the reaper
transaction: SELECT ... FOR UPDATE first, then DELETE ... WHERE NOT EXISTS.
Because the DELETE is a separate statement it runs under a fresh snapshot taken
after the lock wait resolves, so it observes the references committed by the
ingestion transaction the reaper blocked on.

Add a regression test that inserts a referencing child row while ingestion
holds the FOR KEY SHARE lock and asserts the reaper leaves the row in place.

This addresses concurrency with live ingestion. Reaping concurrently with
`db reingest range` (which does not take the FOR KEY SHARE lock on the parent)
is a separate follow-up.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 14, 2026 18:45

Copilot AI left a comment

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.

Pull request overview

Fixes a race between live ingestion and lookup-table reaping that could create dangling history references.

Changes:

  • Separates row locking from orphan deletion to obtain a fresh PostgreSQL snapshot.
  • Adds query-builder and concurrency regression tests.
  • Documents the fix in the changelog.

Reviewed changes

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

File Description
internal/db2/history/main.go Splits locking and deletion queries.
internal/db2/history/main_test.go Updates query-builder tests.
internal/db2/history/reap_concurrency_test.go Tests the concurrent ingestion race.
CHANGELOG.md Records the resolved issue.

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tamirms
tamirms force-pushed the fix-reaper-lookup-table-snapshot-race branch from 6301531 to fbada24 Compare August 14, 2026 18:57
@tamirms
tamirms requested a review from a team August 14, 2026 18:58
Comment thread internal/db2/history/main.go Outdated
Comment thread internal/db2/history/main.go Outdated
- Enforce that deleteLookupTableRows runs inside a transaction via q.GetTx(),
  matching the pattern in verify_lock.go, instead of only documenting it. The
  FOR UPDATE lock is meaningless outside a transaction.
- Run the lock query with SelectRaw instead of ExecRaw with a manually set
  SelectQueryType, so the query type is clear at the call site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tamirms
tamirms enabled auto-merge (squash) August 15, 2026 07:09
@tamirms
tamirms merged commit ab99589 into main Aug 15, 2026
18 checks passed
@tamirms
tamirms deleted the fix-reaper-lookup-table-snapshot-race branch August 15, 2026 09:41
Shaptic added a commit that referenced this pull request Aug 17, 2026
The lookup-table reaper fix landed on main after the release branch was cut, so
it filed itself under Unreleased. v28.0.0 is tagged on this PR's merge commit,
which has main as a parent, so #222 ships in it — move the entry into the
28.0.0 Fixed section where readers will look for it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

3 participants