test: stabilize suite timing and close kittest wiring race

[lklimek]

Stacked on #882 (base: feat/v1.0-parity-batch). Three test-suite reliability fixes from Marvin's full-suite timing pass. No product code touched — only tests, one shared kittest helper, and #[ignore] attributes.

1. Ignore 49 Argon2id-bound secret-storage tests

The wallet_backend::{secret_access, identity_key_store, single_key, det_signer, hydration, secret_seam, wallet_seed_store} tests exercise real end-to-end crypto through platform_wallet_storage's public SecretStore API. Every public method (set/get/set_secret/get_secret/reprotect) — and the vault open itself — derives keys via Argon2::default() at production strength (64 MiB). There is no downstream fast-KDF hook yet: the crate's fast KdfParams/floor() path is pub(crate)-only. Upstream ask filed: dashpay/platform#4111.

In isolation each test is ~1s; the pain is aggregate. Under whole-suite parallelism, dozens of concurrent 64 MiB Argon2id derivations push the host into swap and inflate every test's wall-clock (Marvin observed 5–23s each). Ignoring them removes that peak memory pressure.

Judgment applied — not a rubber-stamp:

• Confirmed each of the 49 goes through fresh_store/open_secret_store (a real Argon2 vault) or a protected/password path. Tests that are slow for other reasons (e.g. wallet_lifecycle::stop_spv_in_place_*) were left enabled, matching Marvin's exclusions.
• The measured durations are contention-inflated: the same tests run in ~0.95s in isolation. The ~5s boundary in the source list is therefore noisy, so I did not expand the set beyond Marvin's traced list.
• Canary coverage retained: ~50 sibling vault tests in the same modules stay enabled (e.g. identity_key_store::store_get_delete_round_trip, secret_seam::ts_noleak_01_on_disk_vault_does_not_contain_raw_secret, wallet_seed_store::non_password_envelope_round_trips, single_key::sec_002_tc_sk_008_sign_uses_secret_store_path_not_bip32). The subsystem is not going blind while #4111 is pending.

CI coverage — action needed (workflow file is write-protected in this environment): these are offline, only slow, so CI should keep running them. .github/workflows/tests.yml has no --ignored lane today, so ignoring them would drop them from CI entirely. Please add this step to the test job (after "Run tests"):

- name: Run ignored secret-storage tests
  uses: actions-rs/cargo@v1
  with:
    command: test
    args: --all-features -p dash-evo-tool wallet_backend -- --ignored

The wallet_backend filter scopes to exactly these unit tests and does not pull in the network-dependent backend-e2e #[ignore] tests. The proper long-term fix is #4111: a public fast-KDF constructor lets all 49 run fast and be un-ignored.

2. Close the kittest wallet-backend wiring race

Symptom: contract_screen::document_action_defaults_to_app_scoped_identity intermittently panicked WalletBackendNotYetWired at insert_local_qualified_identity, passing on immediate re-run of the identical tree.

Root cause (traced from entry point): AppState::new → spawn_backend_init spawns wallet-backend wiring as a background tokio task that populates AppContext::wallet_backend (an ArcSwapOption). insert_local_qualified_identity → det_kv() → wallet_backend()? fast-fails until that store is populated. The mount helpers used a fixed run_steps(5), which does not wait on the async wiring — a race that only surfaces under CPU/swap contention (i.e. alongside finding #1's Argon2 tests).

Fix (DRY, one helper): new support::wait_for_wallet_backend(&mut harness) steps and polls the exact precondition wallet_backend().is_ok() (up to 30s) before returning the context — a bigger fixed step count would only narrow the window, not close it. mount_app, fresh_app_context, and every per-file mount helper that seeds identities (contract_screen, tools_screen, tokens_screen, dashpay_screen, register_dpns_name_screen; identity_hub_switcher/masternode_tab inherit it via mount_app) now gate on it. Full kittest suite: 215/215 pass.

3. Two nextest LEAK flags — false positive, confirmed

progress_overlay::log_overlay_state_bumps_progress_clock_on_content_change and identity::settings::pending_save_cleared_on_identity_change are pure synchronous struct-mutation+assertion tests — no threads, tokio, timers, or egui context, so no in-test leak is possible. Re-ran both in isolation under low load 6×, forced fresh — always PASS, sub-40ms, never LEAK. Confirms the environmental-noise hypothesis: nextest's 100ms wall-clock leak-timeout heuristic false-firing under the same contention finding #1 removes. No code change. (A .config/nextest.toml leak-timeout bump would suppress it globally but risks masking real future leaks by the grace window; deferred to maintainer judgment since finding #1 removes the underlying contention.)

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• master
• v1.0-dev

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dffc0511-6a4e-4928-a91d-4bc381b6ff38

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/test-suite-timing-and-flakiness

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

🕓 Ready for review — 4 ahead in queue (commit 9c2a82f)
Queue position: 5/5

[thepastaclaw]

Code Review

This is a clean, test-only reliability PR: 49 Argon2id-heavy wallet_backend secret-storage tests are correctly marked #[ignore] (verified count matches exactly across all 7 files), and a new wait_for_wallet_backend poll helper correctly closes a real WalletBackendNotYetWired kittest race in 5 of 6 places that had it. The main substantive gap, confirmed by direct inspection, is that .github/workflows/tests.yml has no --ignored lane, so the 49 tests silently stop running in CI, and that the same wiring-race migration was not applied to tests/kittest/import_single_key.rs, which still uses the pre-fix run_steps(5) pattern against the identical wallet_backend()? chokepoint.

Source: reviewers gpt-5.6-sol (Sol: general, rust-quality), claude-sonnet-5 (general, rust-quality), claude-opus-4-8 (general, rust-quality); verifier claude-sonnet-5.

🟡 2 suggestion(s) | 💬 1 nitpick(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `.github/workflows/tests.yml`:
- [SUGGESTION] .github/workflows/tests.yml:72-76: 49 newly-ignored secret-storage tests drop out of CI with no --ignored lane
  Verified: `grep -c '#\[ignore' src/wallet_backend/*.rs` confirms exactly 49 new #[ignore] attributes, spread across secret_access.rs (28), single_key.rs (8), identity_key_store.rs (5), det_signer.rs (2), hydration.rs (2), secret_seam.rs (2), wallet_seed_store.rs (2) — matching the diff stat exactly. Several are explicitly security-labeled (`sec_002_short_passphrase_is_rejected`, `sec_002_legacy_raw_payload_still_signs`, `ts_t2_ik_03_headless_protected_identity_key_is_unavailable`, `ts_lazy_03_protected_single_key_rewraps_to_tier2_via_chokepoint` — all confirmed present at their cited lines). The only test step in tests.yml (lines 72-76) runs plain `cargo test --all-features --workspace`, which skips #[ignore]'d tests, and the only `--ignored` invocations anywhere in the workflow are the commented-out backend-e2e block (lines 83-92). As merged, this silently removes CI enforcement of the secret-storage crypto/vault layer — the app's most security-sensitive subsystem per this repo's own secret_seam.rs chokepoint design — until a maintainer with `.github` write access adds the compensating step. The commit message discloses this explicitly and cites a real, common constraint (the authoring environment's `.github` directory being write-protected for this agent), so the gap is disclosed and requires a maintainer follow-up rather than being a silent regression the author is hiding. Given that legitimate tooling constraint and that this doesn't touch shipped product code, this is a strong suggestion rather than a merge blocker — but it should be treated as a near-term must-fix, ideally landed in the same merge window as this PR.

In `tests/kittest/import_single_key.rs`:
- [SUGGESTION] tests/kittest/import_single_key.rs:147-168: Same WalletBackendNotYetWired race this PR closes elsewhere is left open here
  Verified directly: `imported_single_key_is_visible_in_session` (line 141) still builds the harness and calls the fixed `harness.run_steps(5)` (line 152) followed immediately by `harness.state().current_app_context().clone()` (line 154) and `screen.import_single_key_for_test(...).expect(...)` (line 167-168) — the exact pre-fix pattern this PR replaces elsewhere. Traced the call chain: `import_single_key_for_test` (src/ui/wallets/wallets_screen/mod.rs:2250) calls `register_imported_single_key` (line 2231-2242), which calls `AppContext::import_single_key_wif` (src/context/wallet_lifecycle/registration.rs:40), whose first line is `let backend = self.wallet_backend()?;` — the identical fast-fail chokepoint (`WalletBackendNotYetWired` until `AppState::new`'s background wiring task completes) that `wait_for_wallet_backend` was introduced in this PR to close, and that this PR did apply to `contract_screen.rs`, `tokens_screen.rs`, `tools_screen.rs`, `dashpay_screen.rs`, and `register_dpns_name_screen.rs` (confirmed all five call `crate::support::wait_for_wallet_backend`). `import_single_key.rs` was not migrated and still risks an intermittent panic on the `.expect("import succeeds and syncs into the in-memory map")` under the same CPU/swap contention this PR is designed to tolerate — undermining the PR's claim that the wiring race is closed suite-wide.

[thepastaclaw]

💬 Nitpick: wait_for_wallet_backend's timeout assert isn't attributed to the calling test

Verified: wait_for_wallet_backend (lines 30-46) has no #[track_caller], so its internal assert! (lines 40-43) always reports its panic location as support.rs rather than the specific test that called it. The failure message itself is still informative (it names the 30s timeout), so this only makes backtrace/location triage slightly less direct — low-value but a one-line, essentially free fix.

Suggested change

	pub fn wait_for_wallet_backend(
	harness: &mut Harness<'static, dash_evo_tool::app::AppState>,
	) -> Arc<AppContext> {
	#[track_caller]
	pub fn wait_for_wallet_backend(
	harness: &mut Harness<'static, dash_evo_tool::app::AppState>,
	) -> Arc<AppContext> {

_{source: ['sonnet5-general']}