test(harness): pid-only --dir sweep + deadline-poll asserts (test-hygiene)

[TinDang97]

The queued test-hygiene sweep from the #438 campaign tail (after #444/#445/#446).

Pid-only temp dirs (8 sites, 6 suites)

temp_dir().join(format!("...-{}", pid)) resurrects a crashed run's leftover dir once the pid is reused — the server then reloads stale persistence state and the suite fails on ghost data (the documented stale-reload trap). Converted to tempfile:

• info_memory_allocator_pagecache / memory_doctor_response / memory_prometheus_kinds — the Moon harness owns a TempDir (removed on drop, after the child kill in the hand-written Drop body).
• vector_exact_rerank (3 sites) — RAII tempdirs; manual pre/post remove_dir_all pairs deleted, so cleanup now also happens when an assert panics.
• vector_db_isolation (2 sites) — unique random dirs via Builder + keep(); deliberately not RAII because the restart test shares one dir between two server handles (kill_keep_dir → same-port respawn), so only the collision-prone name changes.
• tls_park_keyupdate — RAII tempdir.

parked_idle_parity deadline-polls

The flaky asserts deferred from the #444 round, all environmental:

• CLIENT KILL registry-removal assert polls (10s deadline) instead of reading once — client-side close is instant (shutdown(2)), registry release needs the killed handler task scheduled; the read-once assert failed 3/3 on the starved 2-vCPU runner and passed solo.
• All 13 connects go through a connect_retry helper (10s deadline / 50ms backoff) — a freshly listening server can refuse first attempts under full-suite load (seen live at the F6 test's connect line during the fix(server): accept-loop HOL block, migration fd ownership, park auth gates (#438 F3–F6 + conn-secondary) #446 gates).
• Read deadlines 10s→30s in the reply helpers (deadline-bound; green runs unaffected).

Not fixed here, filed instead

client_tracking_invalidation's missing second-key invalidation push is product-side (a real RESP3 tracking client would miss the same push) → #448, with the merge-base A/B evidence from the #446 gates.

Gates

fmt + clippy -D warnings (both feature sets); all six converted suites + parked_idle_parity green on macOS monoio (31 tests); tokio leg green on the four suites that run there. Tests-only change — no src/ code touched, bench gates waived.

[coderabbitai]

Warning

Review limit reached

@TinDang97, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 00e5f29e-7666-4ab8-b850-da5fb7153f53

📥 Commits

Reviewing files that changed from the base of the PR and between cf3cee4 and 7066a90.

📒 Files selected for processing (8)

• CHANGELOG.md
• tests/info_memory_allocator_pagecache.rs
• tests/memory_doctor_response.rs
• tests/memory_prometheus_kinds.rs
• tests/parked_idle_parity.rs
• tests/tls_park_keyupdate.rs
• tests/vector_db_isolation.rs
• tests/vector_exact_rerank.rs

---

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

[qodo-code-review]

PR Summary by Qodo

Test harness: replace pid-only temp dirs + add deadline-based polling/retries

🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Replace pid-only --dir temp paths with tempfile to prevent stale data reuse after crashes.
• Make parked_idle_parity resilient to CI timing by adding connect retries, polling asserts, and
 longer read deadlines.
• Document the test-hygiene sweep and track the remaining product-side flake separately.

Diagram

graph TD
  A["Integration suites"] --> B["Moon harness"] --> C(["moon server process"])
  A --> E[["TCP test clients"]] --> F["deadline polls & retries"] --> C
  B --> D[("tempfile dirs")]

  subgraph Legend
    direction LR
    _t["Test code"] ~~~ _p(["Process"]) ~~~ _d[("Temp dir")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize tempdir + retry helpers in tests/common

• ➕ Reduces repeated inline comments and patterns across suites
• ➕ Makes future hygiene sweeps lower-effort and more consistent
• ➖ Requires a small refactor and agreement on common APIs
• ➖ Can be premature if patterns diverge per-suite

2. Use an off-the-shelf retry/backoff helper (crate or shared util)

• ➕ Standardized backoff/jitter and clearer intent
• ➕ Easier to tune without touching each test callsite
• ➖ Adds dependency surface (or more shared code) for a tests-only concern
• ➖ Overkill for a single suite’s connect flake

Recommendation: Current approach is appropriate for a hygiene sweep: it fixes the root cause (pid-reused stale dirs) with tempfile and locally hardens the known-flaky suite with deadline-based polling/retry. If similar patterns continue to appear, follow up by extracting connect_retry and tempdir creation into tests/common to keep future changes consistent.

Files changed (8) +122 / -66

Tests (7) +108 / -66

info_memory_allocator_pagecache.rsUse RAII TempDir in Moon harness for unique --dir cleanup +8/-6

Use RAII TempDir in Moon harness for unique --dir cleanup

• Replaces the pid-derived temp directory with 'tempfile::TempDir' stored on the 'Moon' struct. Removes manual 'remove_dir_all' from 'Drop', relying on RAII cleanup after the child process is killed and waited.

tests/info_memory_allocator_pagecache.rs

memory_doctor_response.rsSwitch Moon tmp dir to tempfile RAII to avoid stale state reuse +8/-6

Switch Moon tmp dir to tempfile RAII to avoid stale state reuse

• Converts the per-test '--dir' path from a pid-only name to a 'tempfile::TempDir'. Eliminates explicit directory deletion in 'Drop' and passes 'tmp_dir.path()' to the server.

tests/memory_doctor_response.rs

memory_prometheus_kinds.rsUse tempfile TempDir for unique persistence dir in Prometheus suite +8/-6

Use tempfile TempDir for unique persistence dir in Prometheus suite

• Updates the harness to hold a 'tempfile::TempDir' (instead of a pid-based PathBuf) for the server '--dir'. Removes manual cleanup and keeps the tempdir lifetime tied to the harness instance.

tests/memory_prometheus_kinds.rs

parked_idle_parity.rsAdd connect retry + deadline-polled asserts; widen read timeouts +53/-23

Add connect retry + deadline-polled asserts; widen read timeouts

• Introduces 'connect_retry' with a 10s deadline/50ms backoff to avoid transient connect failures under load. Changes the CLIENT KILL registry-removal assertion to poll until a 10s deadline, and widens multiple read timeouts from 10s to 30s to reduce CI timing flakes.

tests/parked_idle_parity.rs

tls_park_keyupdate.rsReplace pid-only cert temp dir with RAII tempfile TempDir +4/-5

Replace pid-only cert temp dir with RAII tempfile TempDir

• Creates a 'tempfile::TempDir' guard for TLS cert generation and uses its path for the test. Removes manual cleanup on early return and at test end, ensuring cleanup even on panic.

tests/tls_park_keyupdate.rs

vector_db_isolation.rsGenerate unique kept temp dirs for restart/shared-dir isolation tests +15/-6

Generate unique kept temp dirs for restart/shared-dir isolation tests

• Replaces pid-only dir names with 'tempfile::Builder' tempdirs using a prefix and '.keep()' to return a stable PathBuf. Preserves the existing manual cleanup model because the restart flow intentionally shares one directory between multiple server handles.

tests/vector_db_isolation.rs

vector_exact_rerank.rsUse RAII tempdirs for rerank persistence tests and remove manual cleanup +12/-14

Use RAII tempdirs for rerank persistence tests and remove manual cleanup

• Converts three pid-only temp directories to 'tempfile::TempDir' guards and uses their paths for persistence roundtrips. Deletes pre/post 'remove_dir_all' calls so cleanup still occurs on early exit or assertion panic.

tests/vector_exact_rerank.rs

Documentation (1) +14 / -0

CHANGELOG.mdDocument test-hygiene sweep and remaining tracked flake +14/-0

Document test-hygiene sweep and remaining tracked flake

• Adds a detailed changelog entry describing the pid-only temp dir stale-reload failure mode and the migration to 'tempfile'. Notes the 'parked_idle_parity' deadline-poll/retry hardening and references the remaining product-side flake as a separate issue.

CHANGELOG.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 📜 Skill insights (0)

1. Poll deadline ineffective ✗ Dismissed 🐞 Bug ☼ Reliability

Description

In parked_connection_visible_and_killable, the 10s deadline for polling CLIENT LIST can be
exceeded because each loop iteration calls command_reply() (blocking read with a 30s socket
timeout) before checking the deadline, so a wedged server can stall the test well past 10 seconds.

Code

tests/parked_idle_parity.rs[R229-232]

+    let deadline = std::time::Instant::now() + Duration::from_secs(10);
+    loop {
+        let list = command_reply(&mut control, "CLIENT LIST\r\n");
+        if !list.contains("name=parkvictim") {

Relevance

●● Moderate

No close precedent; team sometimes rejects extra timing-hardening, but this is a real deadline bug.

PR-#426

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

command_reply sets a 30s read timeout and blocks on read() until a full reply is assembled,
while the polling loop checks its 10s deadline only after command_reply returns; therefore one
iteration can block past the intended deadline.

tests/parked_idle_parity.rs[90-117]
tests/parked_idle_parity.rs[225-240]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`parked_connection_visible_and_killable` establishes a 10s deadline but calls `command_reply()` before checking it. Since `command_reply()` sets a 30s read timeout and performs blocking reads, the poll loop can run significantly longer than 10s.

### Issue Context
The PR intentionally widened read timeouts (10s→30s) and added a 10s deadline-polled assert for `CLIENT KILL` registry removal. The poll deadline should still be a real wall-clock cap.

### Fix Focus Areas
- tests/parked_idle_parity.rs[90-117]
- tests/parked_idle_parity.rs[225-240]

### Suggested fix
Make the poll deadline apply to the socket operation:
- Compute `remaining = deadline.saturating_duration_since(Instant::now())` each iteration.
- Set `control.set_read_timeout(Some(remaining.min(Duration::from_secs(…))))` before issuing `CLIENT LIST`, or add a `command_reply_with_timeout(stream, timeout)` helper.
- Alternatively, increase the poll deadline to match the 30s read timeout (less desirable if you want fast failure).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. to_str().unwrap() missing justification 📘 Rule violation ✧ Quality

Description

A modified line uses tmp_dir.path().to_str().unwrap() without the required
#[allow(clippy::unwrap_used)] plus a preceding safety justification comment. This violates the
unwrap-annotation requirement and may cause audit/clippy failures if unwrap ratchets are enforced
for tests too.

Code

tests/info_memory_allocator_pagecache.rs[81]

+                tmp_dir.path().to_str().unwrap(),

Relevance

● Weak

Very similar unannotated to_str().unwrap() in tests was previously rejected (tests policy not
enforced).

PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 302083 requires each .unwrap() in Rust diffs (including tests) to be either
removed or accompanied by an adjacent justification comment and #[allow(clippy::unwrap_used)] on
the containing scope. The cited changes still include tmp_dir.path().to_str().unwrap() in spawn
args (specifically the modified --dir argument) without the required allow attribute and preceding
safety justification comment, demonstrating non-compliance.

Rule 302083: Annotate safe unwrap calls with allow and justification
tests/info_memory_allocator_pagecache.rs[80-82]
tests/memory_doctor_response.rs[75-77]
tests/memory_prometheus_kinds.rs[84-87]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The diff introduces/keeps an `.unwrap()` without the required allow+justification pair (a safety justification comment immediately preceding an `#[allow(clippy::unwrap_used)]` on the containing scope), which violates the unwrap-annotation compliance requirement.

## Issue Context
PR Compliance ID 302083 requires every `.unwrap()` in Rust diffs (including tests) to be either removed or annotated with a `// ...` justification line immediately followed by `#[allow(clippy::unwrap_used)]` on the containing scope. The affected test code currently calls `tmp_dir.path().to_str().unwrap()` (e.g., for a `--dir` argument/spawn args) without that required allow/comment pair, which can trigger audit or clippy/ratchet failures.

## Fix Focus Areas
- tests/info_memory_allocator_pagecache.rs[78-82]
- tests/memory_doctor_response.rs[73-77]
- tests/memory_prometheus_kinds.rs[84-87]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. tempdir().unwrap() missing justification 📘 Rule violation ✧ Quality

Description

New test code calls tempfile::tempdir().unwrap() multiple times without the required
#[allow(clippy::unwrap_used)] plus a preceding justification comment. This violates the
unwrap-annotation requirement and may break unwrap audits if they apply to tests.

Code

tests/vector_exact_rerank.rs[R276-277]

+    let dir_guard = tempfile::tempdir().unwrap();
+    let dir = dir_guard.path().to_path_buf();

Relevance

● Weak

Matches prior rejected request to annotate/avoid unwrap() in test spawn/tempdir path handling.

PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 302083 requires a justification comment and #[allow(clippy::unwrap_used)] for
each .unwrap() in Rust diffs. The updated test introduces tempfile::tempdir().unwrap() in three
places without the required annotation pattern.

Rule 302083: Annotate safe unwrap calls with allow and justification
tests/vector_exact_rerank.rs[274-277]
tests/vector_exact_rerank.rs[321-324]
tests/vector_exact_rerank.rs[641-644]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The diff adds `.unwrap()` calls in tests without the required allow+justification pair.

## Issue Context
Compliance requires every `.unwrap()` to be either removed (preferred) or annotated with a `// ...` justification line immediately followed by `#[allow(clippy::unwrap_used)]` on the containing scope.

## Fix Focus Areas
- tests/vector_exact_rerank.rs[274-277]
- tests/vector_exact_rerank.rs[321-324]
- tests/vector_exact_rerank.rs[641-644]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 55 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}