test: harden two recurring flaky tests (sigterm readiness + bgsave existence)

[pilotspacex-byte]

Two integration tests flake under CI load and have each cost a full ~15-min re-run multiple times this week, taxing every merge. Root-caused, not papered over:

sigterm_shutdown.rs — wait_for_ready now takes &mut Child, checks try_wait() each 100ms tick and returns Readiness{Ready|Exited|TimedOut} so a dead child fails fast instead of burning the whole deadline; surfaces moon.stdout/stderr.log in the panic; doubles READY_TIMEOUT (60→120s) only when CI is set. No post-ready assertion touched.

integration.rs — new bounded wait_for_file(path, timeout) (50ms poll) replaces the immediate assert!(dump.rdb exists) after BGSAVE (a background save that replies before the file lands). Applied at all 3 affected sites (test_bgsave_creates_rdb_file, test_rdb_restore_on_startup, test_aof_priority_over_rdb). Magic-bytes assertion kept.

Verified: sigterm suite 7/7 ×3, the 3 integration tests green, clippy+fmt clean both feature sets. No blind sleeps added — bounded polling only.

Summary by CodeRabbit

• Tests
  • Improved persistence test reliability by waiting for database snapshot files to be fully written before continuing.
  • Hardened graceful-shutdown checks with CI-aware startup timing and faster detection of unexpected server exits.
  • Enhanced failure diagnostics with captured server output when readiness checks fail.
• Documentation
  • Updated the unreleased changelog with the test 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

The changes harden test timing around SIGTERM startup readiness and background RDB creation. SIGTERM tests now detect child exits and expose logs, while persistence tests poll for completed RDB files before continuing.

Changes

SIGTERM readiness diagnostics

Layer / File(s)	Summary
Readiness polling and diagnostics tests/sigterm_shutdown.rs, CHANGELOG.md	Readiness polling now supports CI-aware deadlines, early child-exit detection, captured log diagnostics, and explicit outcomes at both SIGTERM test call sites. The changelog documents the fix.

BGSAVE persistence timing

Layer / File(s)	Summary
RDB completion polling tests/integration.rs	Adds bounded polling for non-empty dump.rdb files, documents the limits of the existing sleep, and applies the helper to three persistence tests.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• pilotspace/moon#216: Modifies SIGTERM integration-test startup and readiness handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: hardening flaky SIGTERM readiness and BGSAVE existence tests.
Description check	✅ Passed	The description covers the problem, fixes, affected tests, and verification, but it skips explicit checklist and performance-impact sections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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/flaky-sigterm-bgsave-hardening

---

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]

🧹 Nitpick comments (1)

tests/sigterm_shutdown.rs (1)

222-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated readiness-check-and-panic pattern into a helper.

Both call sites repeat the same five-line block: compute ready_timeout(), call wait_for_ready, check for non-Ready, format readiness_failure_message, kill+wait the child, and panic. Extracting a helper eliminates the duplication and guarantees consistent cleanup behavior if a third call site is added later.

♻️ Optional refactor

+fn wait_for_ready_or_panic(
+    child: &mut std::process::Child,
+    port: u16,
+    dir: &std::path::Path,
+    label: &str,
+) {
+    let deadline = ready_timeout();
+    let outcome = wait_for_ready(child, port, deadline);
+    if !matches!(outcome, Readiness::Ready) {
+        let msg = readiness_failure_message(label, port, dir, deadline, &outcome);
+        let _ = child.kill();
+        let _ = child.wait();
+        panic!("{msg}");
+    }
+}

Then both call sites collapse to:

-    let deadline = ready_timeout();
-    let outcome = wait_for_ready(&mut child, port, deadline);
-    if !matches!(outcome, Readiness::Ready) {
-        let msg = readiness_failure_message(label, port, &dir, deadline, &outcome);
-        let _ = child.kill();
-        let _ = child.wait();
-        panic!("{msg}");
-    }
+    wait_for_ready_or_panic(&mut child, port, &dir, label);

and:

-    let deadline = ready_timeout();
-    let outcome = wait_for_ready(&mut child, port, deadline);
-    if !matches!(outcome, Readiness::Ready) {
-        let msg = readiness_failure_message("storm", port, &dir, deadline, &outcome);
-        let _ = child.kill();
-        let _ = child.wait();
-        panic!("{msg}");
-    }
+    wait_for_ready_or_panic(&mut child, port, &dir, "storm");

Also applies to: 366-372

🤖 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/sigterm_shutdown.rs` around lines 222 - 231, Extract the repeated
readiness handling from both call sites into a shared helper near the existing
readiness utilities. The helper should accept the child process, port, label,
and directory, compute ready_timeout(), call wait_for_ready(), and on non-Ready
outcomes build readiness_failure_message(), kill and wait for the child, then
panic; update both call sites to use it.

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

Nitpick comments:
In `@tests/sigterm_shutdown.rs`:
- Around line 222-231: Extract the repeated readiness handling from both call
sites into a shared helper near the existing readiness utilities. The helper
should accept the child process, port, label, and directory, compute
ready_timeout(), call wait_for_ready(), and on non-Ready outcomes build
readiness_failure_message(), kill and wait for the child, then panic; update
both call sites to use it.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c5ec4750-b5bc-404e-9633-c363ce9291e8

📥 Commits

Reviewing files that changed from the base of the PR and between 5da8246 and 5c117ae.

📒 Files selected for processing (3)

• CHANGELOG.md
• tests/integration.rs
• tests/sigterm_shutdown.rs