Skip to content

fix(indexer): index operations atomically per transaction - #299

Open
patextreme wants to merge 1 commit into
mainfrom
fix/indexer-atomic-per-transaction
Open

fix(indexer): index operations atomically per transaction#299
patextreme wants to merge 1 commit into
mainfrom
fix/indexer-atomic-per-transaction

Conversation

@patextreme

@patextreme patextreme commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

run_indexer_loop committed each operation individually, so a multi-operation transaction passed through a state where only some of its operations were visible in the indexed views. This created a TOCTOU race:

  • GET /api/transactions/{tx_id} returns 200 as soon as any operation of a transaction is indexed.
  • DID resolution reads the indexed view.

A reader polling for confirmation could proceed while a batch was partially indexed and resolve a DID document missing operations — the source of intermittent prism-test failures.

Reproduced deterministically: a create + 25 chained-updates batch returned only 8/26 keys on iteration 46 of 60.

Fix

Index each transaction atomically so a transaction becomes visible to readers all-at-once.

1. Per-transaction commit — Operations are grouped by transaction ((block_number, absn)) and committed with a single insert_indexed_operations call (already wrapped in one DB transaction in both backends). After this, transaction confirmation (200) implies the full transaction is indexed.

2. Same-transaction storage chains — A storage update/deactivate referencing a root in the same transaction is resolved via an in-memory pending map (resolve_vdr_root) that walks the same-transaction chain and falls back to the DB for cross-transaction chains. Each phase is independently bounded by VDR_ROOT_SEARCH_MAX_DEPTH so the existing depth-limit guard still applies.

3. Pagination boundary (addressing review feedback)get_raw_operations_unindexed (sqlite + postgres) paginates with a 200-row limit, which could split a transaction across two pages and make the indexer commit it in two separate calls, reintroducing the race. When the page is full, the last transaction is now re-fetched in full so every returned transaction is complete and the per-transaction batching is never split by the page boundary.

Verification

Check Result
Indexer unit tests (batching, same-tx chain, cross-tx fallback, orphan-ignore) ✅ 4/4
Indexer integration tests (incl. depth-limit) ✅ 21/21
Storage: transaction-spanning-page-boundary returned complete (250-op tx) ✅ new
Storage: split-landing-mid-tx returns both txs complete ✅ new
node-storage sqlite tests ✅ 31/31
cargo clippy (indexer + storage) -D warnings ✅ clean
Conformance suite (MainSpec) ✅ 73/73, no regression
Stress: 40 iterations × 26-op batch, immediate read-back ✅ 0 partial reads (was 1/60)

Files

  • lib/did-prism-indexer/src/indexing.rs — per-transaction batching, resolve_vdr_root, PendingVdrEntry, hoisted depth const, unit tests.
  • lib/node-storage/src/backend/{sqlite,postgres}.rsget_raw_operations_unindexed returns complete transactions across the page boundary + regression tests.

@patextreme
patextreme requested a review from a team as a code owner July 26, 2026 19:26
@patextreme
patextreme force-pushed the fix/indexer-atomic-per-transaction branch from 0c711cc to e90a0f4 Compare July 26, 2026 19:28
run_indexer_loop previously committed each operation individually, so a
multi-operation transaction passed through a state where only some of its
operations were visible in the indexed views. This created a TOCTOU race:
GET /api/transactions/{tx_id} returns 200 as soon as any operation of a
transaction is indexed, while DID resolution reads the indexed view. A reader
polling for confirmation could observe a partial state and resolve a DID
document missing operations, producing intermittent conformance-test failures.

Operations are now grouped by transaction ((block_number, absn), which the
unindexed query already orders contiguously) and committed with a single
insert_indexed_operations call, which is already wrapped in one DB
transaction. A transaction therefore becomes visible to readers all at once,
so transaction confirmation implies the full transaction is indexed.

A storage update/deactivate may reference a root operation published in the
same transaction; since the batch is not committed until the transaction
boundary, such roots are resolved via an in-memory pending map
(resolve_vdr_root) that walks the same-transaction chain and falls back to the
database for cross-transaction chains. Each phase is independently bounded by
VDR_ROOT_SEARCH_MAX_DEPTH (hoisted from recursively_find_vdr_root) so the
existing depth-limit guard still applies.

get_raw_operations_unindexed (sqlite + postgres) paginates with a 200-row
limit, which could split a transaction across two pages and cause the indexer
to commit it in two separate calls, reintroducing the race. When the page is
full, the last transaction is now re-fetched in full so every returned
transaction is complete and the per-transaction batching is never split by the
page boundary.

Adds unit tests covering per-transaction batching, same-transaction storage
chain resolution without a DB read, cross-transaction DB fallback, and
ignoring orphans; and storage tests verifying a transaction spanning the page
boundary is returned complete.

Signed-off-by: Pat Losoponkul <patextreme@hotmail.com>
@patextreme
patextreme force-pushed the fix/indexer-atomic-per-transaction branch from e90a0f4 to b73af74 Compare July 27, 2026 03:02
@sonarqubecloud

Copy link
Copy Markdown

@patextreme

Copy link
Copy Markdown
Contributor Author

Followed up on an automated review of this PR. Two findings addressed:

  1. Pagination split (correctness)get_raw_operations_unindexed paginates with a 200-row limit and could split a transaction across two pages, causing the indexer to commit it in two separate calls and reintroduce the partial-visibility race this PR removes. Both backends now re-fetch the last transaction in full when the page is full, so every returned transaction is complete and the per-transaction batching is never split by the page boundary. Added storage regression tests (250-op single transaction; split landing mid-transaction).

  2. Depth-budget wording — clarified that resolve_vdr_root and recursively_find_vdr_root each have an independent VDR_ROOT_SEARCH_MAX_DEPTH budget (a chain crossing a transaction boundary can walk up to ~2× a single-phase chain), correcting the earlier "uniform" wording.

The carry-across-fetches alternative was ruled out: if a single transaction has ≥200 operations, its uncommitted rows would re-fill every page and the tail would never be fetched (infinite loop). Returning complete transactions from the source avoids this entirely.

All CI green: neoprism-check, coverage, run-conformance-test, lints, DCO.

@patextreme patextreme left a comment

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.

AI-assisted review — Verdict: Approve (would approve)

GitHub forbids approving your own PR, so posting this as a comment review. Assessment below.

Correctness — confirmed

  • Atomicity holds. insert_indexed_operations opens one DB transaction and commits once, in both sqlite.rs and postgres.rs, so batching a transaction's operations into a single call genuinely makes them visible atomically. GET /api/transactions/{tx_id} returning 200 now implies the full transaction is indexed.
  • Pagination boundary is handled. get_raw_operations_unindexed re-fetches the last transaction in full when the 200-row page is exactly full, so a transaction can never be split across pages and re-committed in two calls. The assumption that only the last sorted group can be truncated is correct, and the re-fetch is uncapped so it can't suffer second-order truncation.
  • Same-tx storage chains resolve correctly via the in-memory pending_vdr map (resolve_vdr_root), falling back to recursively_find_vdr_root for cross-tx chains. The osn ordering invariant this relies on is sound.

Test coverage — good

Batching (one insert per tx), same-tx chain resolution with zero DB lookups, cross-tx DB fallback, orphan-ignore, plus two pagination regression tests (250-op single tx across the boundary; a split landing mid-second-tx). The repro data (8/26 keys on iter 46 → 0/40 after the fix) is convincing.

Minor / non-blocking notes

  • Each resolution phase has an independent VDR_ROOT_SEARCH_MAX_DEPTH budget, so a chain crossing a tx boundary can walk ~2× a single-phase chain. Already documented; low practical concern.
  • The ~35-line pagination block is duplicated across the two backends, consistent with the files' existing mirroring convention.
  • Optional polish: a one-line comment near the pending_vdr.insert calls stating the osn-ordering invariant, and a test pinning the combined depth-budget behavior.

No blocking issues. CI green (neoprism-check, coverage, run-conformance-test, lints, DCO).

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.

1 participant