fix: Flaky online delete tests, and cover the health checks added in #5531

[vlntb]

High Level Overview of Change

Part 1: the flakiness

xrpl.app.LedgerMaster and xrpl.app.SHAMapStore are flaky in CI. The visible
symptoms vary by build type:

• Debug: the unit test worker aborts outright:
  Assertion failed: (("xrpl::LedgerMaster::missingFromCompleteLedgerRange : invalid parameters") && false),
  function missingFromCompleteLedgerRange, file LedgerMaster.cpp, line 1595
• Release: UNREACHABLE is a no-op, so execution continues and the run
  reports a large fan-out of downstream assertion failures instead
  (observed: 247 suites, 4728 cases, 117 failures).

Both come from the same place. The failures are unrelated to any feature branch
work. They reproduce on tests added alongside #5531 (pause online delete on
ledger-history gaps).

Part 2: the missing coverage

SHAMapStoreImp::healthWait() and the [node_db] validation #5531 are missing some unit-tests. This PR adds missing unit-tests.

Context of Change

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[xrplf-ai-reviewer]

This is a well-reasoned fix for genuine race conditions in two SHAMapStore-related test suites (LedgerMaster_test.cpp and SHAMapStore_test.cpp). The root cause — env.close() returning before the job queue has actually handed the ledger to SHAMapStore, causing rendezvous() to return immediately without doing meaningful work — is clearly diagnosed, and the fixes (draining the job queue first, replacing brittle exact-value/spin-loop assertions with range checks, and guarding missingFromCompleteLedgerRange against invalid ranges that would abort a Debug build) are sound and consistent across both files. The only notable nit is that the new syncStore helper is duplicated verbatim in both test files rather than shared.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[xrplf-ai-reviewer]

The changes add a syncStore() helper to drain the job queue before waiting on the SHAMapStore, and replace brittle exact-match/looping assertions with range checks and guards against triggering missingFromCompleteLedgerRange's UNREACHABLE precondition. The logic is well-reasoned and the comments explain the underlying races clearly. The main gap is that the new helper calls getJobQueue().rendezvous() with no timeout, which can still hang the test job even though the very next line takes care to use a timed overload for the store — this partially defeats the PR's stated goal of preventing hangs. The helper is also duplicated verbatim across the two test files.

[xrplf-ai-reviewer]

This is a solid, well-reasoned fix for flaky LedgerMaster and SHAMapStore tests: it drains the job queue before waiting on the SHAMapStore, replaces brittle exact-value assertions on lastRotated with range checks that tolerate legitimate ledger-coalescing timing, and guards the calls to missingFromCompleteLedgerRange so a desynced range reports a normal test failure instead of aborting the process via UNREACHABLE. No correctness issues were found in the changed lines; the one thing worth calling out is that the new syncStore() helper is duplicated verbatim (including its explanatory comments) across both test files.

[xrplf-ai-reviewer]

This is a test-only change that fixes flaky LedgerMaster/SHAMapStore tests by draining the job queue before checking SHAMapStore state, tolerating timing-dependent values instead of assuming fixed ones, guarding missingFromCompleteLedgerRange calls against precondition violations, and adding new coverage for healthWait() branches via a custom log-capturing sink. I traced through the new helpers (syncStore, parkInHealthWait, parkMidRotation, StoreLogs) and the ordering of mode changes, ledger closes, job-queue drains, and rendezvous calls checks out internally consistent with the documented invariants (store's coalescing of newLedger_, healthWait's branch conditions, clearPrior's per-row sleep giving a parking window). I did not find a concrete correctness bug, resource leak, or race condition in the added lines that meets the bar for confident flagging — the extensive comments accurately describe the synchronization reasoning, and the substring log-matching constants (kFullWait/kShortWait) are provably non-overlapping as claimed.

[Copilot]

Pull request overview

This PR strengthens and de-flakes the online-delete / ledger-gap unit tests by ensuring tests synchronize with asynchronous SHAMapStore processing, and by adding targeted coverage for SHAMapStoreImp config validation and healthWait() behavior introduced alongside #5531.

Changes:

• Replace direct SHAMapStore::rendezvous() usage in tests with a syncStore() helper that first drains the JobQueue, avoiding timing-dependent false “idle” observations.
• Add unit tests covering [node_db] online-delete configuration validation and multiple SHAMapStoreImp::healthWait() decision branches (mode/age/gap/building-ledger/circuit-breaker/stop).
• Harden ledger-range assertions to avoid Debug-build aborts from invalid missingFromCompleteLedgerRange() parameter ranges.

Reviewed changes

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

File	Description
src/test/app/SHAMapStore_test.cpp	Adds log-capturing + deterministic state-machine style tests for healthWait() and config validation; replaces rendezvous usage with syncStore() to eliminate races.
src/test/app/LedgerMaster_test.cpp	Uses syncStore() and relaxes assumptions about lastRotated initialization to avoid timing-dependent failures; guards missingFromCompleteLedgerRange() preconditions.

---

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

[xrplf-ai-reviewer]

This is a careful, well-documented rewrite of the flaky LedgerMaster/SHAMapStore tests (replacing hard-coded expected lastRotated values and unguarded missingFromCompleteLedgerRange calls that could hit UNREACHABLE with timing-tolerant checks and a syncStore() helper that properly drains the job queue before rendezvous), plus substantial new coverage for [node_db] config validation and healthWait() branches. The added StoreLogs/parkInHealthWait/parkMidRotation helpers are thoroughly commented, and the synchronization patterns (sleep-based polling for a ledger to appear, then a fixed settle delay) mirror an existing convention already used by testLedgerGaps(), so they don't introduce a new flakiness pattern. I traced the config-validation test invariant that storeConfigResult()'s caller must end on an accepted config (so ~AppBundle runs and clears the installed debug sink) and confirmed testConfig() satisfies it. I did not find any correctness, security, or resource-leak issues in the added/changed lines that meet the bar for flagging with confidence.

[Copilot]

🟢 Approval recommended

Changes are confined to test code, address concrete race/flakiness mechanisms with correct synchronization, and add targeted coverage for previously untested branches.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite