test(router): align mint error tests with post-Phase-E flow - #111
Merged
Conversation
- Rename mint_commit_tx_failure_returns_503 → mint_pending_inscriptions_persist_failure_returns_503 to reflect post-PR-107/110 reality: dead_pool fails at the publisher's pre-broadcast pending_inscriptions INSERT, not at commit_mint_tx. Assertion now matches the wrapped error string. - Add mint_commit_mint_tx_failure_returns_503 to restore branch coverage of the post-broadcast commit_mint_tx Err path. Uses a live Postgres testcontainer + a BEFORE INSERT trigger on the accounts table; everything earlier in the mint flow succeeds, only the final accounts upsert raises, and the handler returns 503 with the 'Failed to persist mint commit transaction' body.
TaprootFreak
added
the
ci:full
The accept loop handled one connection then slept 60s before accepting the next, which serialised sequential mint tests behind the previous mint's keepalive sleep. The second connect_async hit its 15s timeout and the mint flow returned 503 'WS connect failed'. Phase E surfaced this because it added the first multi-mint-per- test scenario (mint_handler_two_sequential_mints_with_different_recipients_advance_cleanly). Pre-Phase-E every test ran exactly one mint so the bug was hidden. Fix: spawn a tokio task per accepted connection so the accept loop keeps draining new connects in parallel with the existing 60s keepalive.
…t test The existing race test relied on prepare_mint (~200ms in test build) being slow enough for the test thread to acquire the SMT lock and insert pk_0 between phase 2 entry and the phase-3 re-derive. Under CI load this race was lost intermittently — phase 3 ran first, saw no pk_0, proceeded to broadcast, returned 'Failed to broadcast mint inscription on-chain' instead of 'Concurrent mint detected'. Fix: add a second cfg(test)-only Notify (phase3_release) that the handler awaits between prepare_mint and the phase-3 re-derive. All test_state constructors pre-arm it so production-shaped tests proceed immediately. The concurrent-mint race test drains the pre-armed permit before spawning, then notify_one()s after injecting pk_0 — a hard happens-before edge that no timing variance can lose. This eliminates the flake observed on develop's heavy CI run 26411938492 Coverage Gate.
The previous Notify-based hold pre-armed exactly ONE permit. After the first mint consumed it, subsequent mints in the same process (e.g. mint_handler_two_sequential_mints_with_different_recipients_advance_cleanly) blocked forever — the test was observed SLOW [>720s] in the heavy gate's nextest run before timing out. A tokio::sync::Mutex<()> is the correct primitive: pre-unlocked, handler acquires + drops in one step for production-shaped tests (non-blocking), and the concurrent-mint race test holds the guard across pk_N injection then drops it. Reusable for any number of sequential mints without manual re-arming.
The three `i64::try_from(leaf_index)` sites in db.rs and the `debug_assert!` in state.rs::load_from_pg all guard a hypothetical > i64::MAX (or 32-bit usize-overflow) condition that cannot fire on any target we ship (Linux x86_64 / aarch64). The error/panic arms were flagged by the 100% Coverage Gate as uncoverable, contributing 13 uncovered lines for zero production benefit. Replace each `try_from + sqlx::Error::Encode` with a direct `as i64` cast (and `as usize` in state.rs), keeping a one-line invariant comment that documents the bound. No behaviour change on 64-bit targets, which is the only deployment target. Affected sites: - db::persist_state_tx - db::persist_state_and_mark_complete_tx - db::insert_root_index - state::State::load_from_pg
…t return
`create_and_broadcast_inscription` returned `Result<Option<(Txid, Txid)>>`
but the body has no path that yields `Ok(None)` — every success arm
builds `Ok(Some((c, r)))` and every failure surfaces as `Err`. The dead
`Ok(None)` arm in `router::mint_handler` together with the defensive
`if let Some(ctxid) = commit_txid_bytes { .. } else { .. }` fallback
contributed 13 uncovered lines to the Coverage Gate.
Flatten the API to `Result<(Txid, Txid), ...>`:
- publisher.rs: return tuple directly on Ok.
- router.rs: match yields `[u8; 32]` instead of `Option<[u8; 32]>`;
collapse the `if let Some(ctxid) { .. } else { .. }` to a single block.
- publisher_tests.rs: drop the now-redundant `.expect("Some((c,r))")`
unwrap layer at three call sites.
No behaviour change — the removed branches were unreachable.
The phase-3b state-advance Err arm in `mint_handler` (the 503 returned when `update_and_snapshot_for_persist` fails on an SMT key-collision-with-different-value) was uncovered by the 100% Coverage Gate. The race that produces this state in production needs two concurrent mints whose phase-2 re-derives BOTH pass and whose broadcasts both land before either state lock acquires — too brittle to reproduce deterministically with two real requests. Add a deterministic test that exercises the same code path: - Mirror `phase3_release_lock` with a new `state_advance_release_lock` test-only AppState field. The handler acquires + immediately drops it AFTER `create_and_broadcast_inscription` returns and BEFORE the phase-3b state lock. Production builds compile this out entirely (both the field and the hold point are `#[cfg(test)]`). - The new test holds the guard across a colliding-SMT injection at `pk_0`. When the guard drops, the handler's `state.update` observes the collision and returns 503 with the documented "in-process state advance failed" reason. Asserts also confirm the pending row stays at `reveal_broadcast`, the on-disk SMT/MMR/ root_index DID NOT advance (no atomic persist tx ran), and the scanner-replay path stays armed.
2 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 25, 2026
) deploy_dev_node + deploy_prd_node steps hit `client_loop: send disconnect: Broken pipe` exit 255 when `docker compose recreate` runs longer than the cloudflared tunnel's idle timeout (~60s with no remote stdout). The host-side deploy script keeps running and the container comes up correctly, but CI marks the step failed. Add ServerAliveInterval=30 + ServerAliveCountMax=8 — keepalive every 30s, 8 retries before declaring dead = 4 min of network silence tolerated. The deploy script's longest silent stretch (`docker compose pull` of a fresh image + container recreate) is bounded by the host's network + disk speed, comfortably under that ceiling. Observed on the PR #111 merge auto-deploy: run 26419696840 failed with the broken-pipe pattern, but ssh dfxdev-remote showed the container was already `Up (healthy)` — purely a CI-side artefact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Heavy-CI hotfix. Release PR #18 (
develop → main) Coverage Gate + Node + Shared Tests both failed onmint_commit_tx_failure_returns_503after PR #110 (Phase E) landed. The test asserted a pre-Phase-E error string that the new mint flow no longer produces with thedead_poolit uses.Root cause
Pre-Phase-E the FIRST DB write in
mint_handlerwasdb::commit_mint_tx. After PR #107 the FIRST DB write is the publisher'spending_inscriptions(constructed)INSERT. With the lazydead_poolthe failure now surfaces with the publisher-wrapped string"Failed to broadcast mint inscription on-chain", not the deeper"Failed to persist mint commit transaction"that the test was pinned to.Fix
mint_commit_tx_failure_returns_503→mint_pending_inscriptions_persist_failure_returns_503. Update assertion + doc-comment to match what the test actually exercises (publisher persistence layer failure).mint_commit_mint_tx_failure_returns_503to restore branch coverage of the post-broadcastcommit_mint_txErr path (router.rs ~1191). Uses a live Postgres testcontainer + aBEFORE INSERTtrigger on theaccountstable; pending_inscriptions persist, broadcast, in-memory state.update, andpersist_state_and_mark_complete_txall succeed, only the finalcommit_mint_txraises, handler returns 503 with the original error string.Local checks (all green)
cargo fmt --all --checkcargo clippy -p node --all-features --tests -- -D warningsTest plan
Lint & Buildgreenci:fulllabel: Node + Shared Tests + Coverage Gate green (these are the failing ones today on the Release PR)