test(harness): TOCTOU-safe shared port/spawn helpers across all 33 server-spawning suites - #284
Merged
Merged
Conversation
…conversion (task #18 WIP)
…pers Task #18. Every integration suite spawning a real moon process carried its own free_port() that binds :0 and DROPS the listener before the server spawns. Two failure modes kept hitting CI (latest: spsc_two_db on PR #283, which cost two full macOS rerolls): 1. Port TOCTOU — between probe drop and moon's bind, another test's probe or a concurrent outbound connection's ephemeral source port takes the port; moon exits with EADDRINUSE. 2. Dead-server blind poll — harnesses polled connect() for up to 30s without checking child liveness, so a lost bind race surfaced as "server never accepted: Connection refused" half a minute later with the real error unread in the server's stderr log. Fix: new shared tests/common/mod.rs with - reserve_port(): process-wide dedup set over kernel-chosen probe ports (kills intra-process reuse); - spawn_listening(spawn): polls TCP accept WHILE watching child.try_wait(), respawning on a fresh port the moment the child dies (external steals cannot be prevented, only recovered from; 3 attempts, then a loud panic pointing at the server's stderr log). All 33 suites converted. Conversion rules held throughout: - protocol-level readiness (PING/AUTH) stays with each suite — spawn_listening only guarantees "listening"; - kill-9/SIGTERM/restart tests keep their deliberate same-port+same-dir restart legs (only the FIRST spawn of a lifecycle goes through spawn_listening): coordinator_local_leg_durability (7 legs), sharded_multi_exec_durability, sharded_multi_exec_routing, spsc_wake_floor_red, vector_db_isolation; - expected-startup-failure tests (db_maxmemory_quota CLI validation, admin_auth hard02) keep direct spawns on reserve_port() ports; - txn_kv_wiring's in-process async server routes port choice through reserve_port() and keeps its await_server_ready poll (no Child to watch); its real-subprocess crash-recovery path uses spawn_listening; - no test assertions, timeouts, or CLI flags changed. Found en route (filed separately, not fixed here): admin_auth_cors_ratelimit is cfg(feature = "console")-gated and has 13 pre-existing compile errors from ureq API drift — invisible because the console CI job is skipped on PRs (task #37). Verified: cargo test --no-run clean on default AND tokio matrices; all 33 suites green (--no-fail-fast); 10-rep stress running 14 port-hungry suites CONCURRENTLY (the CI contention pattern) green; fmt clean. Refs: task #18, follow-up to PR #283 CI rerolls author: Tin Dang <tindang.ht97@gmail.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughChangesIntegration test port-flake sweep
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/common/mod.rs (1)
29-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwitch
HANDED_OUTtoparking_lot::Mutexand drop theunwrap()
A panic while holding this process-wide lock can poison laterreserve_port()calls and cascade failures across unrelated tests.parking_lotis already available here.🤖 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/common/mod.rs` around lines 29 - 49, The process-wide lock in reserve_port should use parking_lot::Mutex to avoid poisoning after a panic. Update the Mutex import and initialize HANDED_OUT with the parking_lot type, then remove unwrap() from HANDED_OUT.lock() while preserving the existing insert and retry behavior.Source: Coding guidelines
tests/db_maxmemory_quota.rs (1)
64-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating
spawn_moon_db_quotaandspawn_moon_db_quota_no_spill.The two functions are nearly identical, differing only in the
--disk-offload disableflag. A single function with adisable_spill: boolparameter would eliminate the duplication.♻️ Optional consolidation
fn spawn_moon_db_quota(dir: &std::path::Path, db_entries: &[&str]) -> (ServerGuard, u16) { - let (child, port) = common::spawn_listening(|port| { - let mut cmd = Command::new(find_moon_binary()); - cmd.args([ - "--port", &port.to_string(), - "--dir", &dir.to_string_lossy(), - "--shards", "1", - "--appendonly", "no", - "--maxmemory", "0", - "--maxmemory-policy", "noeviction", - "--databases", "16", - ]); - for entry in db_entries { - cmd.args(["--db-maxmemory", entry]); - } - cmd.stdout(std::fs::File::create(dir.join("moon.stdout.log")).expect("stdout log")) - .stderr(std::fs::File::create(dir.join("moon.stderr.log")).expect("stderr log")) - .spawn() - .expect("spawn moon") - }); - (ServerGuard(child), port) -} - -fn spawn_moon_db_quota_no_spill(dir: &std::path::Path, db_entries: &[&str]) -> (ServerGuard, u16) { + spawn_moon_db_quota_inner(dir, db_entries, false) +} + +fn spawn_moon_db_quota_no_spill(dir: &std::path::Path, db_entries: &[&str]) -> (ServerGuard, u16) { + spawn_moon_db_quota_inner(dir, db_entries, true) +} + +fn spawn_moon_db_quota_inner( + dir: &std::path::Path, + db_entries: &[&str], + disable_spill: bool, +) -> (ServerGuard, u16) { let (child, port) = common::spawn_listening(|port| { let mut cmd = Command::new(find_moon_binary()); cmd.args([ "--port", &port.to_string(), "--dir", &dir.to_string_lossy(), "--shards", "1", "--appendonly", "no", "--maxmemory", "0", "--maxmemory-policy", "noeviction", "--databases", "16", + ]); + if disable_spill { + cmd.args(["--disk-offload", "disable"]); + } + cmd.args([ "--disk-offload", "disable", ]); for entry in db_entries { cmd.args(["--db-maxmemory", entry]); } cmd.stdout(std::fs::File::create(dir.join("moon.stdout.log")).expect("stdout log")) .stderr(std::fs::File::create(dir.join("moon.stderr.log")).expect("stderr log")) .spawn() .expect("spawn moon") }); (ServerGuard(child), port) }Also applies to: 107-136
🤖 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/db_maxmemory_quota.rs` around lines 64 - 91, Consolidate spawn_moon_db_quota and spawn_moon_db_quota_no_spill into one helper that accepts a disable_spill: bool parameter. Keep the shared command construction in the unified function, and add --disk-offload disable only when disable_spill is true; update all callers to pass the appropriate value.
🤖 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/common/mod.rs`:
- Around line 84-101: Update the error path around child.try_wait and the
ACCEPT_DEADLINE assertion in spawn_listening to explicitly kill the
still-running child before panicking. Preserve the existing diagnostic messages
and normal respawn behavior for an exited child, ensuring both panic paths clean
up the process and its resources first.
In `@tests/memory_prometheus_kinds.rs`:
- Around line 62-85: Move the common::reserve_port() call into the closure
passed to common::spawn_listening, and use that per-attempt value when building
the --admin-port argument. Preserve the returned admin_port value after
spawn_listening so it represents the winning attempt’s port.
In `@tests/vector_db_isolation.rs`:
- Around line 118-119: Update spawn_moon_first to redirect the spawned server’s
stdout and stderr to log files in the test temporary directory, matching the
existing setup in vector_del_unindex.rs and wire_reachability_red.rs; remove
both Stdio::null() destinations while preserving the current log filenames and
panic-diagnostic behavior expected by spawn_listening.
---
Nitpick comments:
In `@tests/common/mod.rs`:
- Around line 29-49: The process-wide lock in reserve_port should use
parking_lot::Mutex to avoid poisoning after a panic. Update the Mutex import and
initialize HANDED_OUT with the parking_lot type, then remove unwrap() from
HANDED_OUT.lock() while preserving the existing insert and retry behavior.
In `@tests/db_maxmemory_quota.rs`:
- Around line 64-91: Consolidate spawn_moon_db_quota and
spawn_moon_db_quota_no_spill into one helper that accepts a disable_spill: bool
parameter. Keep the shared command construction in the unified function, and add
--disk-offload disable only when disable_spill is true; update all callers to
pass the appropriate value.
🪄 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: 13eb24b9-972c-4fc1-9424-e94b4a27fa7e
📒 Files selected for processing (35)
CHANGELOG.mdtests/admin_auth_cors_ratelimit.rstests/busy_poll_idle.rstests/client_tracking_invalidation.rstests/cmd_flush_dbsize_debug_memory.rstests/common/mod.rstests/container_growth_memory_accounting.rstests/coordinator_local_leg_durability.rstests/cross_shard_consistency_red.rstests/db_maxmemory_quota.rstests/flush_cross_shard_scatter.rstests/ft_search_yield_red.rstests/ft_yield_chunk_ab.rstests/mem_watchdog.rstests/memory_doctor_response.rstests/memory_prometheus_kinds.rstests/msetnx_cross_shard_reject.rstests/multishard_serve_smoke.rstests/oom_bypass_closure.rstests/pubsub_burst_delivery.rstests/pubsub_kv_ordering.rstests/pubsub_multi_channel_acl.rstests/resp3_hello.rstests/shard_panic_abort.rstests/sharded_multi_exec_durability.rstests/sharded_multi_exec_locality.rstests/sharded_multi_exec_routing.rstests/shardslice_live.rstests/sigterm_shutdown.rstests/spsc_two_db.rstests/spsc_wake_floor_red.rstests/txn_kv_wiring.rstests/vector_db_isolation.rstests/vector_del_unindex.rstests/wire_reachability_red.rs
Comment on lines
+84
to
+101
| match child.try_wait() { | ||
| Ok(Some(status)) => { | ||
| // Lost the bind race (or crashed at startup): give the | ||
| // next attempt a fresh port instead of polling a corpse. | ||
| eprintln!( | ||
| "spawn_listening: child exited {status} before accepting on \ | ||
| port {port} (attempt {attempt}/{ATTEMPTS}) — respawning" | ||
| ); | ||
| break; | ||
| } | ||
| Ok(None) => {} | ||
| Err(e) => panic!("spawn_listening: try_wait failed: {e}"), | ||
| } | ||
| assert!( | ||
| start.elapsed() < ACCEPT_DEADLINE, | ||
| "spawn_listening: live child never accepted on port {port} within \ | ||
| {ACCEPT_DEADLINE:?} — check the server log in the test's --dir" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Kill the child before panicking on both panic paths.
When try_wait returns Err (line 95) or the ACCEPT_DEADLINE assert fires (lines 97-101), the child is still alive but is dropped without kill(). std::process::Child::drop does NOT terminate the process — the server becomes an orphan, holding its port and resources, which can interfere with subsequent tests in the same binary.
🐛 Proposed fix
match child.try_wait() {
Ok(Some(status)) => {
// Lost the bind race (or crashed at startup): give the
// next attempt a fresh port instead of polling a corpse.
eprintln!(
"spawn_listening: child exited {status} before accepting on \
port {port} (attempt {attempt}/{ATTEMPTS}) — respawning"
);
break;
}
Ok(None) => {}
- Err(e) => panic!("spawn_listening: try_wait failed: {e}"),
+ Err(e) => {
+ let _ = child.kill();
+ let _ = child.wait();
+ panic!("spawn_listening: try_wait failed: {e}");
+ }
}
- assert!(
- start.elapsed() < ACCEPT_DEADLINE,
- "spawn_listening: live child never accepted on port {port} within \
- {ACCEPT_DEADLINE:?} — check the server log in the test's --dir"
- );
+ if start.elapsed() >= ACCEPT_DEADLINE {
+ let _ = child.kill();
+ let _ = child.wait();
+ panic!(
+ "spawn_listening: live child never accepted on port {port} within \
+ {ACCEPT_DEADLINE:?} — check the server log in the test's --dir"
+ );
+ }
std::thread::sleep(Duration::from_millis(50));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match child.try_wait() { | |
| Ok(Some(status)) => { | |
| // Lost the bind race (or crashed at startup): give the | |
| // next attempt a fresh port instead of polling a corpse. | |
| eprintln!( | |
| "spawn_listening: child exited {status} before accepting on \ | |
| port {port} (attempt {attempt}/{ATTEMPTS}) — respawning" | |
| ); | |
| break; | |
| } | |
| Ok(None) => {} | |
| Err(e) => panic!("spawn_listening: try_wait failed: {e}"), | |
| } | |
| assert!( | |
| start.elapsed() < ACCEPT_DEADLINE, | |
| "spawn_listening: live child never accepted on port {port} within \ | |
| {ACCEPT_DEADLINE:?} — check the server log in the test's --dir" | |
| ); | |
| match child.try_wait() { | |
| Ok(Some(status)) => { | |
| // Lost the bind race (or crashed at startup): give the | |
| // next attempt a fresh port instead of polling a corpse. | |
| eprintln!( | |
| "spawn_listening: child exited {status} before accepting on \ | |
| port {port} (attempt {attempt}/{ATTEMPTS}) — respawning" | |
| ); | |
| break; | |
| } | |
| Ok(None) => {} | |
| Err(e) => { | |
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| panic!("spawn_listening: try_wait failed: {e}"); | |
| } | |
| } | |
| if start.elapsed() >= ACCEPT_DEADLINE { | |
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| panic!( | |
| "spawn_listening: live child never accepted on port {port} within \ | |
| {ACCEPT_DEADLINE:?} — check the server log in the test's --dir" | |
| ); | |
| } | |
| std::thread::sleep(Duration::from_millis(50)); |
🤖 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/common/mod.rs` around lines 84 - 101, Update the error path around
child.try_wait and the ACCEPT_DEADLINE assertion in spawn_listening to
explicitly kill the still-running child before panicking. Preserve the existing
diagnostic messages and normal respawn behavior for an exited child, ensuring
both panic paths clean up the process and its resources first.
Comment on lines
+62
to
+85
| let admin_port = common::reserve_port(); | ||
| let tmp_dir = std::env::temp_dir().join(format!("moon-test-prom-{}", std::process::id())); | ||
| let _ = std::fs::create_dir_all(&tmp_dir); | ||
| let child = Command::new(&bin) | ||
| .args([ | ||
| "--port", | ||
| &port.to_string(), | ||
| "--shards", | ||
| "1", | ||
| "--admin-port", | ||
| &admin_port.to_string(), | ||
| "--appendonly", | ||
| "no", | ||
| "--dir", | ||
| tmp_dir.to_str().unwrap(), | ||
| "--disk-offload", | ||
| "disable", | ||
| ]) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; | ||
| let (child, port) = common::spawn_listening(|port| { | ||
| Command::new(&bin) | ||
| .args([ | ||
| "--port", | ||
| &port.to_string(), | ||
| "--shards", | ||
| "1", | ||
| "--admin-port", | ||
| &admin_port.to_string(), | ||
| "--appendonly", | ||
| "no", | ||
| "--dir", | ||
| tmp_dir.to_str().unwrap(), | ||
| "--disk-offload", | ||
| "disable", | ||
| ]) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .expect("spawn moon") | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Admin port TOCTOU not covered by spawn_listening retry logic.
admin_port is reserved once outside the closure (line 62) and captured by reference, so all three retry attempts inside spawn_listening reuse the same admin port. If an external process steals that port between reserve_port() dropping its probe and the server binding it, every retry fails identically and spawn_listening panics — the retry logic can't help because only the main port changes.
Move reserve_port() inside the closure so each attempt gets a fresh admin port:
🛡️ Proposed fix
- let admin_port = common::reserve_port();
let tmp_dir = std::env::temp_dir().join(format!("moon-test-prom-{}", std::process::id()));
let _ = std::fs::create_dir_all(&tmp_dir);
+ let mut admin_port = 0u16;
let (child, port) = common::spawn_listening(|port| {
+ admin_port = common::reserve_port();
Command::new(&bin)
.args([
"--port",
&port.to_string(),
"--shards",
"1",
"--admin-port",
- &admin_port.to_string(),
+ &admin_port.to_string(),
"--appendonly",
"no",
"--dir",
tmp_dir.to_str().unwrap(),
"--disk-offload",
"disable",
])
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
.expect("spawn moon")
});After spawn_listening returns, admin_port holds the winning attempt's value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let admin_port = common::reserve_port(); | |
| let tmp_dir = std::env::temp_dir().join(format!("moon-test-prom-{}", std::process::id())); | |
| let _ = std::fs::create_dir_all(&tmp_dir); | |
| let child = Command::new(&bin) | |
| .args([ | |
| "--port", | |
| &port.to_string(), | |
| "--shards", | |
| "1", | |
| "--admin-port", | |
| &admin_port.to_string(), | |
| "--appendonly", | |
| "no", | |
| "--dir", | |
| tmp_dir.to_str().unwrap(), | |
| "--disk-offload", | |
| "disable", | |
| ]) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .ok()?; | |
| let (child, port) = common::spawn_listening(|port| { | |
| Command::new(&bin) | |
| .args([ | |
| "--port", | |
| &port.to_string(), | |
| "--shards", | |
| "1", | |
| "--admin-port", | |
| &admin_port.to_string(), | |
| "--appendonly", | |
| "no", | |
| "--dir", | |
| tmp_dir.to_str().unwrap(), | |
| "--disk-offload", | |
| "disable", | |
| ]) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .expect("spawn moon") | |
| }); | |
| let tmp_dir = std::env::temp_dir().join(format!("moon-test-prom-{}", std::process::id())); | |
| let _ = std::fs::create_dir_all(&tmp_dir); | |
| let mut admin_port = 0u16; | |
| let (child, port) = common::spawn_listening(|port| { | |
| admin_port = common::reserve_port(); | |
| Command::new(&bin) | |
| .args([ | |
| "--port", | |
| &port.to_string(), | |
| "--shards", | |
| "1", | |
| "--admin-port", | |
| &admin_port.to_string(), | |
| "--appendonly", | |
| "no", | |
| "--dir", | |
| tmp_dir.to_str().unwrap(), | |
| "--disk-offload", | |
| "disable", | |
| ]) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .expect("spawn moon") | |
| }); |
🤖 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/memory_prometheus_kinds.rs` around lines 62 - 85, Move the
common::reserve_port() call into the closure passed to common::spawn_listening,
and use that per-attempt value when building the --admin-port argument. Preserve
the returned admin_port value after spawn_listening so it represents the winning
attempt’s port.
Comment on lines
+118
to
+119
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Redirect server stdout/stderr to log files instead of Stdio::null().
spawn_listening panics with messages like "check the server log in the test's --dir" and "read the server stderr log in the test's --dir", but spawn_moon_first discards both streams via Stdio::null(). When startup fails there are no logs to inspect, making the panic messages misleading. The other two files in this cohort (vector_del_unindex.rs and wire_reachability_red.rs) redirect to log files in the temp dir.
🛠️ Proposed fix
- .stdout(Stdio::null())
- .stderr(Stdio::null())
+ .stdout(std::fs::File::create(tmp_dir.join("moon.stdout.log")).expect("stdout log"))
+ .stderr(std::fs::File::create(tmp_dir.join("moon.stderr.log")).expect("stderr log"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .stdout(std::fs::File::create(tmp_dir.join("moon.stdout.log")).expect("stdout log")) | |
| .stderr(std::fs::File::create(tmp_dir.join("moon.stderr.log")).expect("stderr log")) |
🤖 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/vector_db_isolation.rs` around lines 118 - 119, Update spawn_moon_first
to redirect the spawned server’s stdout and stderr to log files in the test
temporary directory, matching the existing setup in vector_del_unindex.rs and
wire_reachability_red.rs; remove both Stdio::null() destinations while
preserving the current log filenames and panic-diagnostic behavior expected by
spawn_listening.
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 13, 2026
… drift (#321) Cargo.lock pins ureq 3.3.0 (Cargo.toml `ureq = "3"`), but tests/admin_auth_cors_ratelimit.rs (task #37) was still written against the ureq 2.x API: `.set()`, `.timeout()`, `ureq::request(method, url)`, `resp.header()`, and matching `ureq::Error::Status(code, resp)` to read headers off 4xx/5xx responses. ureq 3 renamed/removed all of these: `.header()` replaces `.set()`, per-request timeouts move to `.config().timeout_global(...).build()`, `ureq::request()` is gone in favor of method-named free functions (`ureq::options()`), and — the breaking part for these tests — `Error::StatusCode(u16)` no longer carries the response, so header assertions on error responses (www-authenticate on 401, retry-after on 429) were no longer reachable at all. Fix: build every request with `http_status_as_error(false)` so 4xx/5xx responses come back as `Ok(Response<Body>)` instead of `Err`, and assert on `resp.status()` / `resp.headers()` directly. This preserves every original assertion (bearer-token enforcement, tampered-signature rejection, CORS origin echo/elision, rate-limit 429 + retry-after, preflight bypass, healthz/readyz auth exemption) — nothing was weakened, only the mechanism for reaching the response headers changed. Verified: cargo test --release --features console --test admin_auth_cors_ratelimit (7/7 pass), cargo fmt --check, cargo clippy --no-default-features --features runtime-monoio,jemalloc,graph,text-index,console --tests -- -D warnings (clean). Refs: task #37 (console-feature test rot from PR #284 review) author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Sep 4, 2026
… and BITOP (#813) * fix(shard): persist the coordinator's single-shard local leg for COPY and BITOP `coordinate_copy` and `coordinate_bitop` each opened with a `num_shards == 1` fast path returning `run_local(..)` directly — "zero coordinator overhead on the 1-shard hot path". `run_local` mutates the keyspace and returns; it takes no `aof_pool` and no `repl_state`, so it appends nothing to the AOF/WAL and issues no replication LSN. That is silent data loss, and it violates a contract written 300 lines above it in the same file, on `persist_local_leg`: WAL append is external to `cmd_dispatch`, so the coordinator's in-process local legs (`run_local`, ..) MUST call this or their writes are lost on restart while the remote legs survive. Measured at `--shards 1 --appendonly yes --appendfsync everysec`, isolated per-runtime binaries, same host and script: runtime | COPY destination lost after SIGKILL + recovery --------+----------------------------------------------- monoio | 0 / 6 tokio | 6 / 6 On tokio, after `SET k` / `COPY k k2` / `SET k3` the AOF is 55 bytes holding only the two `set`s, and recovery logs "replayed 2 AOF commands" for three writes: the COPY was never appended. `BITOP` behaves identically. The monoio connection handler appends these commands on its own path, which is why the same probe is 0-of-6 there and why no CI leg but the tokio one ever saw it — it surfaced as a 3-of-3 `Check` failure in `cold_reconciliation_property_660` (seed 2, `prop:key:005` written by COPY, nil after recovery). The fix adds `run_local_persist`: run locally, then honour the same `persist_local_leg` contract the multi-shard same-owner branch already honours via `run_on_owner_persist`. COPY persists only on `:1` (`:0` is a refusal that writes nothing); BITOP persists on any non-error, since it always writes DEST (SET, or DEL when the combine is empty). Why no existing test caught it: `tests/coordinator_local_leg_durability.rs` — a suite that already contained `copy_dst_local_leg_persists_across_restart` — pinned `const SHARDS: u32 = 4` at module level since #284. At shards=4 the same-owner branch routes through `run_on_owner_persist`, so the bug cannot occur there. That constant becomes `SHARD_MATRIX = [1, 4]` and all seven cases now run at both counts (14 tests). Red/green: with the fix reverted, exactly `copy_dst_local_leg_..._s1` and `bitop_dest_local_leg_..._s1` fail and all twelve others pass, including both `_s4` controls. With it, 14/14 green on monoio and on tokio, all six `cold_reconciliation_property_660` seeds green under tokio, and `cargo clippy --all-targets -D warnings` clean on both feature sets. author: Tin Dang * fix(shard): skip the local-leg serialize when there is no AOF, and correct two claims Adversarial review findings on the parent commit. 1. `run_local_persist` built the `Vec<Frame>` and ran `serialize_command` before `persist_local_leg` could short-circuit on a `None` pool, so the `appendonly=no` path paid an allocation and a full argv copy for a record nobody would append. That is precisely the overhead the `num_shards == 1` early return exists to avoid, and the comment above it still promised "zero coordinator overhead". Now returns before building anything, matching the `wal_fanout_has_work` idiom. 2. The BITOP predicate comment was factually wrong. It claimed BITOP "always writes DEST on success (SET, or DEL when the combine is empty)". When the combine is empty AND dest did not exist, `db.remove` touches nothing. The `|_| true` predicate is still right, but as a deliberate over-log: `:0` cannot distinguish "deleted a real DEST" from "deleted nothing", and skipping on `:0` would risk dropping a real delete. Comment now says that instead of asserting something false. 3. Two `--appendfsync always` cases with no quiescing sleep, covering the zero-grace-period path that the `everysec` cases (which all sleep 1.5s) never touch. These do NOT close the gap I added them for, and the comment now says so. The review's point was that `*local_barrier_pending |= needs_barrier` is freely deletable. I deleted it, rebuilt tokio, and the suite is still 16/16 green — because the ack is followed by a GET round-trip before the kill, which gives the writer time to fsync anyway. Isolating the barrier needs a kill landing between the ack and the writer's own fsync, which a black-box test cannot reliably hit, and there is no barrier counter to assert on. Filed for a mechanism counter. 4. Documented that the `_s1` COPY/BITOP cases discriminate only under `runtime-tokio`. On monoio the coordinator is gated out at `num_shards <= 1`, so the handler logs the command itself and those cases pass with or without the fix — the same blindness that hid the bug for months. A green monoio run, `scripts/ci-local.sh` included, is not evidence this fix still works. Verified: 16/16 on monoio and tokio, `cargo fmt --check` clean, and `cargo clippy --all-targets -D warnings` clean on both feature sets. author: Tin Dang
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
2 participants
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
Task #18: sweep all 33 server-spawning integration suites onto a shared TOCTOU-safe port/spawn harness. This is the flake class that cost PR #283 two full macOS CI rerolls this week (
spsc_two_db: "server never accepted on port 50081: Connection refused" after 30s), and previously hittxn_kv_wiring(#16),sigterm/bgsave(#11), and others.The defect class
Every suite carried a copy-pasted
free_port()that binds:0, reads the port, and drops the listener before the server spawns:connect()for up to 30s without ever checking child liveness, so a lost bind race surfaced as "Connection refused" half a minute later, with the real error unread in the server's stderr log.The fix —
tests/common/mod.rsreserve_port()— process-wide dedup set over kernel-chosen probe ports (kills intra-process reuse).spawn_listening(spawn)— reserves a port, runs the caller's spawn closure, polls TCP accept while watchingchild.try_wait(), and respawns on a fresh port the moment a child dies (external steals can't be prevented, only recovered from; 3 attempts, then a loud panic pointing at the stderr log).Protocol-level readiness (PING/AUTH) stays with each suite —
spawn_listeningonly guarantees "listening".Conversion rules held across all 33 files
spawn_listening(coordinator_local_leg_durability×7,sharded_multi_exec_durability,sharded_multi_exec_routing,spsc_wake_floor_red,vector_db_isolation).db_maxmemory_quota,admin_authhard02) keep direct spawns onreserve_port()ports —spawn_listeningwould mask the assertion.txn_kv_wiring's in-process async server routes its port throughreserve_port()and keeps itsawait_server_readypoll (noChildto watch); its real-subprocess crash-recovery path usesspawn_listening.Verification
cargo test --no-runclean on default AND tokio matrices; fmt + clippy (--tests, both matrices) clean for every touched file.--no-fail-fast).Found en route (pre-existing, filed as task #37, not fixed here)
tests/admin_auth_cors_ratelimit.rsis#![cfg(feature = "console")]-gated and has 13 pre-existing compile errors from ureq API drift — invisible because the console-feature CI job is skipped on PRs (verified pre-existing viagit stash). Two other untouched test files (graph_restart_id_aliasing,crash_recovery_disk_offload_no_aof) carry pre-existing clippy---testsdebt that CI's Lint (which doesn't lint tests) never sees.Closes task #18.
Summary by CodeRabbit
Bug Fixes
Tests