Skip to content

test: delivery-invariant harness (model + concurrency + lifecycle) - #98

Open
WiktorStarczewski wants to merge 3 commits into
wiktor-stranded-cursor-resetfrom
wiktor-delivery-invariant-tests
Open

test: delivery-invariant harness (model + concurrency + lifecycle)#98
WiktorStarczewski wants to merge 3 commits into
wiktor-stranded-cursor-resetfrom
wiktor-delivery-invariant-tests

Conversation

@WiktorStarczewski

Copy link
Copy Markdown
Contributor

Stacked on #97 (wiktor-stranded-cursor-reset) — base is that branch, so this diff shows only the harness. It will retarget to main once #97 merges.

Why

Three separate "notes not delivered" incidents — the created_at timestamp-collision race, the :memory: pool-isolation bug, and a cursor stranded above a regressed seq high-water — were all the same invariant violated three ways:

A note stored under tag T is delivered to a fetcher for T exactly once when draining from cursor 0, and is never left permanently unreachable — regardless of cursor values, concurrency, cleanup, or DB recreation.

Every one was found in production/stress, not by a unit test, because example-based tests only cover the cases the author imagined. This PR adds a harness that explores the state space so a fresh bug in this class surfaces as a replayable failing seed.

What

crates/node/src/database/invariant_tests.rs (in-crate #[cfg(test)], zero new dependencies — seeded rand + a hand-rolled greedy shrinker + an RAII temp-file helper):

  • Layer 1 — model / property. A reference model of the intended fetch/cursor semantics runs in lockstep with the real Database over random op sequences (store / fetch-with-cursor / drain / recreate). On failure it prints the seed and the greedily-minimized failing sequence.
  • Layer 2 — seeded concurrency. Concurrent writers + readers against a real file-backed 16-connection pool (and a :memory: pool, guarding the pool-size=1 clamp); a final drain must deliver every note exactly once.
  • Layer 3 — lifecycle edges. Cross-recreation stranded-cursor recovery, cleanup_old_notes retention + high-water preservation, and cross-batch pagination.

6 new tests; CI-fast by default (finished in ~1.5s); deep soak via NTS_INV_ITERS / NTS_INV_LEN / NTS_INV_CONC_ITERS; reproduce via NTS_INV_SEED. Documented under Developer Guide → Testing.

Validation notes

  • The harness is not vacuous. I temporarily removed the model's stranded-reset rule and confirmed it caught the divergence and shrank a 40-op sequence to a 1-op repro — which also surfaced a real subtlety: on a fresh migrated DB, sqlite_sequence.seq for notes is 0 (not absent), so the backend correctly resets any cursor > 0 on an empty DB. The correct model and backend agree on this.
  • The file-backed concurrency test warms up one connection before the burst (runs migrations + WAL setup once), mirroring production init — otherwise ~16 pool connections race to run migration DDL on a brand-new file ("disk I/O error"), which is a cold-start artifact, not a completeness bug.

Verification

  • cargo test -p miden-note-transport-node27/27 pass (21 existing + 6 new).
  • clippy --locked --all-targets --workspace -D warnings — clean.
  • nightly fmt --check — clean.

@WiktorStarczewski

Copy link
Copy Markdown
Contributor Author

Review (internal + Codex) — findings applied

Two independent reviews. The harness did its job on day one: it surfaced a real production availability bug.

Production bug found & fixed (both reviews, HIGH/CRITICAL)

The file-backed concurrent test needed a warm-up because, without it, ~16 pool connections race to run migration DDL on a fresh volume ("disk I/O error"). That's not a test artifact: connections are created lazily and each runs migrations on creation, and Node::init spawns maintenance concurrently with grpc.serve() with no happens-before — so a restart thundering-herd on a fresh volume would hit this in prod. Fixed at the root: SqliteDatabase::connect now eagerly runs the schema migration on one connection before returning, and the test warm-up is removed (the file-backed concurrent test now passes because of the production fix, not around it).

A "CRITICAL" Codex finding — rejected as empirically false

Codex claimed the model's stranded-reset over-fires on an empty DB (arguing high_water_seq returns None on a fresh table), predicting the Layer-1 test fails spuriously >99% of the time, and to add && high_water > 0. I verified before implementing: added an arbiter test (fresh_db_resets_any_positive_cursor) proving a fresh DB returns effective == 0 (its sqlite_sequence row exists as 0), and ran Layer 1 for 2000 iterations green. Applying Codex's guard would have broken the harness. The conflict did pin a real doc inaccuracy in high_water_seq (said None "no note inserted" — actually Some(0)), now corrected.

Applied (both reviewers)

  • Independent stranded-heal oracle (Layer 1 only ever drained from 0): now also asserts drain-from-stranded == drain-from-0 on whatever epoch the random recreates leave — catches a heal regression even if the model mirrored it.
  • Concurrent readers are now an oracle, not decoration: per-pass duplicate-free assertion, every observed id must be a real stored id, and fetcher panics are propagated (previously swallowed).
  • Burst now exceeds one batch (8×80=640 > 500) and is env-tunable (NTS_INV_CONC_WRITERS/NOTES), so drains paginate across pages under live inserts.
  • Explicit duplicate-delivery assertions in Layer-1 DrainAll + final completeness (Layer 2 already had them).
  • Better diagnostics: Fetch first-diff index + id prefix; a drain-terminates eprintln so a stuck cursor isn't misread as a missing note.
  • Doc precision on determinism (seed replays the op sequence; ids are re-randomized), model↔backend high_water==0 coupling note, and nits.

Not changed (both agreed / verified correct): client_fetch rcursor replication (handler pinned by the #97 gRPC test), TempDbPath naming/cleanup, model↔AUTOINCREMENT seq sync.

Verification after fixes: 28/28 tests pass (~6s default; 1500–2000-iter sweeps green); clippy --locked --all-targets --workspace -D warnings clean; nightly fmt --check clean.

@WiktorStarczewski
WiktorStarczewski marked this pull request as ready for review July 2, 2026 15:36
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