test(harness): sweep hardcoded binary paths + unguarded child spawns

[pilotspacex-byte]

Summary

Completes the harness-hygiene follow-up noted in #374 / issue #366: the incident chain (leaked test child + stale binary of unknown provenance) had two enabling patterns spread across tests/, both now swept.

Class A — binary resolution (33 suites)

Suites resolved the moon server binary via a bare ./target/release/moon, a CARGO_MANIFEST_DIR-relative guess, or a local find_moon_binary() copy that never checked CARGO_BIN_EXE_moon — on a shared checkout that can silently run a stale binary instead of the one cargo just built. All migrated to common::find_moon_binary() (MOON_BIN → CARGO_BIN_EXE_moon → target fallbacks). The 5 suites with a documented "skip gracefully when unbuilt" Option<PathBuf> contract keep their own resolver, with the CARGO_BIN_EXE_moon tier inserted ahead of the stale-path guess.

Class B — kill-on-drop guards (6 suites + 1 warning fix)

console_gateway_test, scan_fanout_multishard, allocator_mimalloc_smoke, perf_v0112_arenas_cap, replication_readonly_eval, replication_readonly_ws_mq held a bare Child across many assert!/.expect() calls with cleanup only at function end — a mid-test panic orphaned the server, the exact shape behind the #366 667%-CPU incident. All now use the MoonGuard kill-on-drop pattern from tests/bgsave_startup_race.rs (whose own leftover unused_mut warning is also fixed here).

Complex multi-restart harnesses (crash-recovery kill-9 cycles, jepsen_lite, instance_lock, sigterm_shutdown, …) were deliberately left untouched; already-correct suites left as-is. Full inventory in the commit message.

Verification

• cargo check --profile release-fast --tests green under default features, runtime-tokio,jemalloc (CI parity), and console,graph,text-index (covers every touched file's cfg-gate).
• Representative spot check: cargo test --profile release-fast --test perf_v0112_arenas_cap — 2/2 passed.
• Rebased onto main post-perf(persistence): idle-tick cheap wins — O(1) pagecache resident counter + cached-clock checkpoint trigger (#373 phase 1) #375 and re-verified; cargo fmt --check clean.

refs #366

Summary by CodeRabbit

• Bug Fixes

  • Improved integration-test reliability by consistently launching the binary built for the current test run.
  • Prevented test-started server processes from remaining active after failures or interrupted tests through automatic cleanup.
  • Reduced false failures caused by stale binaries and orphaned test processes.

• Documentation

  • Added a changelog entry describing the test-harness reliability improvements.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

📝 Walkthrough

Walkthrough

Integration test harnesses now centralize Moon executable discovery, prefer the Cargo-built binary in selected helpers, and use kill-on-drop guards in several tests to terminate and reap spawned servers.

Changes

Integration test harness hygiene

Layer / File(s)	Summary
Centralized Moon binary resolution CHANGELOG.md, tests/admin_auth_cors_ratelimit.rs, tests/aof_*, tests/cold_shadow_overwrite_resurrection.rs, tests/console_gateway_test.rs, tests/crash_*, tests/flush_cross_shard_scatter.rs, tests/info_memory_allocator_pagecache.rs, tests/memory_prometheus_kinds.rs, tests/replication_*, tests/scan_fanout_multishard.rs, tests/wal_group_commit.rs	Test launchers delegate Moon executable lookup to common::find_moon_binary() and update affected helpers to return PathBuf values where needed.
Cargo-built binary preference tests/pubsub_kv_ordering.rs, tests/pubsub_multi_channel_acl.rs, tests/sharded_multi_exec_*.rs	Existing local lookup helpers check CARGO_BIN_EXE_moon before probing release and debug target paths.
Kill-on-drop child cleanup tests/allocator_mimalloc_smoke.rs, tests/bgsave_startup_race.rs, tests/console_gateway_test.rs, tests/perf_v0112_arenas_cap.rs, tests/replication_readonly_*.rs, tests/scan_fanout_multishard.rs	Selected tests wrap spawned Moon children in MoonGuard and replace explicit kill/wait calls with guard drops.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• pilotspace/moon#229: Related integration-test binary resolution and fallback handling.
• pilotspace/moon#278: Related replication test kill-on-drop cleanup.
• pilotspace/moon#374: Related bgsave_startup_race.rs MoonGuard changes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the harness cleanup: hardcoded binary paths and unguarded child spawns.
Description check	✅ Passed	The description covers the main changes and verification, with only non-critical template sections like checklist, performance impact, and notes omitted.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/harness-hygiene-sweep

---

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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/pubsub_kv_ordering.rs (1)

29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated moon_binary() implementation across test suites.

The exact same Option<PathBuf> binary resolver is duplicated across 5 test suites.

• tests/pubsub_kv_ordering.rs#L29-L37: Extract the duplicated moon_binary logic into a shared common::find_moon_binary_opt() -> Option<PathBuf> helper and reuse it.
• tests/pubsub_multi_channel_acl.rs#L26-L34: Replace this duplicated logic with the extracted helper.
• tests/sharded_multi_exec_durability.rs#L36-L44: Replace this duplicated logic with the extracted helper.
• tests/sharded_multi_exec_locality.rs#L37-L45: Replace this duplicated logic with the extracted helper.
• tests/sharded_multi_exec_routing.rs#L31-L39: Replace this duplicated logic with the extracted helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/pubsub_kv_ordering.rs` around lines 29 - 37, Extract the duplicated
moon_binary resolver into shared common::find_moon_binary_opt() ->
Option<PathBuf>, preserving its existing CARGO_BIN_EXE_moon lookup and fallback
behavior. Update tests/pubsub_kv_ordering.rs:29-37,
tests/pubsub_multi_channel_acl.rs:26-34,
tests/sharded_multi_exec_durability.rs:36-44,
tests/sharded_multi_exec_locality.rs:37-45, and
tests/sharded_multi_exec_routing.rs:31-39 to call the shared helper instead of
maintaining local copies.

tests/console_gateway_test.rs (1)

49-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract MoonGuard to the common module to avoid code duplication.

The MoonGuard struct and its Drop implementation are identical across multiple test suites. Consider extracting this utility into the shared tests/common/mod.rs file to adhere to DRY principles and centralize the child-process cleanup logic.

• tests/console_gateway_test.rs#L49-L62: Remove this local definition and use the shared MoonGuard.
• tests/scan_fanout_multishard.rs#L72-L85: Remove this local definition and use the shared MoonGuard.

♻️ Proposed refactor (in `tests/common/mod.rs`)

use std::process::Child;

pub struct MoonGuard(pub Option<Child>);

impl Drop for MoonGuard {
    fn drop(&mut self) {
        if let Some(mut child) = self.0.take() {
            sigkill(&mut child); 
        }
    }
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/console_gateway_test.rs` around lines 49 - 62, Extract the duplicated
MoonGuard struct and Drop implementation into tests/common/mod.rs, using
common::sigkill for child cleanup. Remove the local definitions at
tests/console_gateway_test.rs lines 49-62 and tests/scan_fanout_multishard.rs
lines 72-85, then update both suites to use the shared common::MoonGuard.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/admin_auth_cors_ratelimit.rs`:
- Around line 46-51: Preserve the tests’ graceful-skip behavior by replacing
common::find_moon_binary() with an Option<PathBuf>-returning resolver in
tests/admin_auth_cors_ratelimit.rs lines 46-51,
tests/flush_cross_shard_scatter.rs lines 30-32, and
tests/info_memory_allocator_pagecache.rs line 32; retain the existing
!bin.exists() skip handling at each corresponding call site.

---

Nitpick comments:
In `@tests/console_gateway_test.rs`:
- Around line 49-62: Extract the duplicated MoonGuard struct and Drop
implementation into tests/common/mod.rs, using common::sigkill for child
cleanup. Remove the local definitions at tests/console_gateway_test.rs lines
49-62 and tests/scan_fanout_multishard.rs lines 72-85, then update both suites
to use the shared common::MoonGuard.

In `@tests/pubsub_kv_ordering.rs`:
- Around line 29-37: Extract the duplicated moon_binary resolver into shared
common::find_moon_binary_opt() -> Option<PathBuf>, preserving its existing
CARGO_BIN_EXE_moon lookup and fallback behavior. Update
tests/pubsub_kv_ordering.rs:29-37, tests/pubsub_multi_channel_acl.rs:26-34,
tests/sharded_multi_exec_durability.rs:36-44,
tests/sharded_multi_exec_locality.rs:37-45, and
tests/sharded_multi_exec_routing.rs:31-39 to call the shared helper instead of
maintaining local copies.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 731014d6-5f3f-41f4-aae8-94e9cee7db3c

📥 Commits

Reviewing files that changed from the base of the PR and between fb67a24 and 44b1ae6.

📒 Files selected for processing (37)

• CHANGELOG.md
• tests/admin_auth_cors_ratelimit.rs
• tests/allocator_mimalloc_smoke.rs
• tests/aof_fsync_err_subscribe_ordering.rs
• tests/aof_multidb_kill9.rs
• tests/aof_toplevel_multishard_refusal.rs
• tests/bgsave_startup_race.rs
• tests/cold_shadow_overwrite_resurrection.rs
• tests/console_gateway_test.rs
• tests/crash_matrix_per_shard_aof.rs
• tests/crash_matrix_per_shard_bgrewriteaof.rs
• tests/crash_recovery_cold_del_resurrection.rs
• tests/crash_recovery_disk_offload_no_aof.rs
• tests/crash_recovery_graph_durability.rs
• tests/crash_recovery_orphan_sweep_readiness.rs
• tests/crash_recovery_wal_recycle_legacy.rs
• tests/flush_cross_shard_scatter.rs
• tests/info_memory_allocator_pagecache.rs
• tests/memory_prometheus_kinds.rs
• tests/perf_v0112_arenas_cap.rs
• tests/pubsub_kv_ordering.rs
• tests/pubsub_multi_channel_acl.rs
• tests/replication_graph.rs
• tests/replication_hardening.rs
• tests/replication_mq.rs
• tests/replication_multishard.rs
• tests/replication_planes.rs
• tests/replication_readonly_eval.rs
• tests/replication_readonly_ws_mq.rs
• tests/replication_streaming.rs
• tests/replication_ttl_semantics.rs
• tests/replication_ws.rs
• tests/scan_fanout_multishard.rs
• tests/sharded_multi_exec_durability.rs
• tests/sharded_multi_exec_locality.rs
• tests/sharded_multi_exec_routing.rs
• tests/wal_group_commit.rs

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Dead code and broken graceful skip due to find_moon_binary() panicking.

common::find_moon_binary() panics if a binary is not found and only returns a verified existing path. This renders the downstream !bin.exists() checks unreachable, transforming the previous graceful-skip behavior into a test panic. If these suites were intended to keep their graceful skip contract, they should use an Option<PathBuf> resolver. If the panic is now desired, the dead code blocks should be removed.

• tests/admin_auth_cors_ratelimit.rs#L46-L51: The switch to common::find_moon_binary() renders the !bin.exists() check at Line 90 dead code. Switch to an Option<PathBuf> resolver or remove the dead block.
• tests/flush_cross_shard_scatter.rs#L30-L32: The switch renders the !bin.exists() check at Line 55 dead code. Switch to an Option<PathBuf> resolver or remove the dead block.
• tests/info_memory_allocator_pagecache.rs#L32-L32: The switch renders the !bin.exists() check at Line 58 dead code. Switch to an Option<PathBuf> resolver or remove the dead block.

📍 Affects 3 files

• tests/admin_auth_cors_ratelimit.rs#L46-L51 (this comment)
• tests/flush_cross_shard_scatter.rs#L30-L32
• tests/info_memory_allocator_pagecache.rs#L32-L32

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/admin_auth_cors_ratelimit.rs` around lines 46 - 51, Preserve the tests’
graceful-skip behavior by replacing common::find_moon_binary() with an
Option<PathBuf>-returning resolver in tests/admin_auth_cors_ratelimit.rs lines
46-51, tests/flush_cross_shard_scatter.rs lines 30-32, and
tests/info_memory_allocator_pagecache.rs line 32; retain the existing
!bin.exists() skip handling at each corresponding call site.