test(ci): Windows-tolerant timing for two c10k integration tests

[TinDang97]

The post-merge main matrix went red on Check (Windows) for two brand-new c10k tests (#427, #430) while every other platform stayed green. Both are the slow Windows runner, not the code under test.

• acl_privileged_intercepts::privileged_intercepts_are_acl_gated_multi_shard — the minimal RESP client pumps a fixed 250 ms window per reply; on Windows a legitimately-delivered ACL SETUSER reply lands after the window closes → empty read → failed: "". Widened to 1500 ms on Windows (the pump accumulates until its deadline, so this only tolerates lag; nothing asserted changes).
• idle_timeout_sweep::idle_connection_is_closed_at_timeout — panicked at the upper bound (elapsed < 12 s): the connection was closed by timeout 2 (the behaviour under test), just later than Linux/macOS because the 1 Hz sweep is scheduled coarsely on Windows. Read ceiling 15→25 s, late ceiling 12→22 s on Windows only; the "closes at all" assertion is unchanged.

Both knobs are cfg!(windows)-gated, so Linux/macOS keep the tight originals. Full coverage preserved on every platform (not skipped). Test-only, no product code — skip-changelog.

Verified via workflow_dispatch of the CI matrix on this branch (Windows job).

Summary by CodeRabbit

• Tests
  • Improved cross-platform reliability for privileged command response timing tests.
  • Adjusted idle-timeout regression checks to account for platform-specific scheduling delays.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 28 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: c2d74da9-ea51-46c3-87eb-1fa9cf747044

📥 Commits

Reviewing files that changed from the base of the PR and between ab5aa60 and 7666e99.

📒 Files selected for processing (2)

• tests/acl_privileged_intercepts.rs
• tests/idle_timeout_sweep.rs

📝 Walkthrough

Walkthrough

The tests now use platform-specific timing thresholds. Windows receives wider response and idle-timeout windows. Other platforms retain the existing thresholds.

Changes

Platform-specific test timing

Layer / File(s)	Summary
Adjust test timing thresholds tests/acl_privileged_intercepts.rs, tests/idle_timeout_sweep.rs	ACL response reads use 1.5 seconds on Windows and 250 milliseconds elsewhere. Idle-timeout reads and late-close assertions use platform-specific ceilings.
Estimated code review effort: 2 (Simple)	~5 minutes

Possibly related PRs

• pilotspace/moon#427: Introduces the idle-timeout test suite updated here with Windows-specific timing thresholds.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies Windows-specific timing tolerance changes for two c10k integration tests.
Description check	✅ Passed	The description clearly explains the failures, platform-specific changes, scope, preserved assertions, and CI verification.
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 fix/c10k-windows-test-timing

---

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

Make two c10k integration tests Windows-tolerant by widening timing ceilings

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Add Windows-only headroom to the RESP reply pump window in an ACL c10k test.
• Relax Windows-only read/late timing ceilings in the idle-timeout sweep test.
• Keep tight original timing assertions on non-Windows platforms (no skips).

Diagram

graph TD
A["CI matrix"] --> B{"cfg!(windows)?"}
B -->|"Yes"| W["Windows timing headroom"]
W --> C["acl_privileged_intercepts test"] --> D["RESP reply window 1500ms"]
W --> E["idle_timeout_sweep test"] --> F["read/late ceilings 25s/22s"]
B -->|"No"| N["Keep tight timings"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Ignore/skip the tests on Windows

• ➕ Eliminates Windows flakiness immediately with minimal code changes
• ➖ Reduces cross-platform coverage and can mask real Windows-only regressions

2. Replace fixed ceilings with polling/backoff until condition or max deadline

• ➕ More robust against scheduler jitter and coarse timers
• ➕ Can assert the same behavior without fragile upper bounds
• ➖ More code/complexity in tests and can make failures slower to surface

3. Increase timeouts globally for these tests (all platforms)

• ➕ Simplifies logic by avoiding cfg!(windows) branches
• ➖ Needlessly slows feedback and relaxes timing guarantees on fast platforms

Recommendation: The current approach (Windows-only headroom via cfg!(windows)) is a good fit: it preserves strict timing on Linux/macOS while addressing known Windows CI scheduling variability without skipping coverage. Consider polling/backoff only if additional Windows timing flakes appear elsewhere and a more systemic test harness approach is warranted.

Files changed (2) +22 / -3

Tests (2) +22 / -3

acl_privileged_intercepts.rsWiden RESP reply pump window on Windows to avoid empty reads +11/-1

Widen RESP reply pump window on Windows to avoid empty reads

• Replaces the fixed 250ms per-reply pump window with a cfg!(windows)-gated reply_window. Windows uses 1500ms to tolerate CI scheduling/TCP lag while non-Windows keeps 250ms, preserving existing assertions.

tests/acl_privileged_intercepts.rs

idle_timeout_sweep.rsRelax Windows-only read and late-close ceilings for idle-timeout sweep +11/-2

Relax Windows-only read and late-close ceilings for idle-timeout sweep

• Introduces cfg!(windows)-gated (read_ceiling, late_ceiling) to accommodate coarser sweep scheduling on Windows CI. Updates the socket read timeout and the upper-bound assertion to use these platform-specific ceilings while keeping the core 'closes at all' behavior unchanged.

tests/idle_timeout_sweep.rs

[qodo-code-review]

Code Review by Qodo

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

1. Fixed-delay RESP pump 🐞 Bug ➹ Performance

Description

In tests/acl_privileged_intercepts.rs, Resp::cmd() uses pump() which keeps polling until its
deadline even after the reply is already received; increasing the Windows window to 1500ms therefore
adds a fixed ~1.5s delay per command and can substantially slow these integration tests on Windows.

Code

tests/acl_privileged_intercepts.rs[R159-162]

+            Duration::from_millis(250)
+        };
+        self.pump(reply_window);
        String::from_utf8_lossy(&self.buf).into_owned()

Relevance

●●● Strong

Repo has accepted making test RESP reads completion-aware to avoid timeouts/hangs; same pattern
fixes fixed-delay pump.

PR-#426
PR-#65

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR increases the reply_window to 1500ms on Windows, and cmd() always calls
pump(reply_window). But pump() loops until Instant::now() >= deadline and ignores read
timeouts, so after the reply arrives it continues polling (with the socket’s 100ms read timeout)
until the full window elapses. The test calls cmd() many times and runs for both shards=1 and
shards=4, so the fixed delay can accumulate substantially on Windows.

tests/acl_privileged_intercepts.rs[118-145]
tests/acl_privileged_intercepts.rs[147-163]
tests/acl_privileged_intercepts.rs[168-269]
PR-#426

Agent prompt

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

## Issue description
`Resp::pump()` runs until its deadline regardless of whether it already read a complete reply. With the PR’s Windows-only increase to 1500ms, each `cmd()` call can now incur a fixed ~1.5s polling window, significantly lengthening the test runtime on Windows.

## Issue Context
- `pump()` ignores timeout errors and continues looping until the deadline.
- `cmd()` clears the buffer, sends one command, then calls `pump(reply_window)`.
- The test issues many commands per run and runs twice (single-shard + multi-shard), so fixed per-command waits can accumulate.

## Fix Focus Areas
- tests/acl_privileged_intercepts.rs[135-145]
- tests/acl_privileged_intercepts.rs[147-163]
- tests/acl_privileged_intercepts.rs[168-269]

## Suggested implementation direction
Choose one:
1) **Deadline + early-exit on first timeout after progress**
  - Track whether any bytes were read (`saw_data: bool`).
  - On `Err(e)` where `e.kind()` is `TimedOut`/`WouldBlock`:
    - if `saw_data` is true, `break` (assume reply drained for now)
    - else continue until deadline
  - Keep the overall `deadline` as a hard cap for lag.

2) **RESP-aware read of a single frame**
  - Parse enough of the buffer to know when one full RESP reply is complete, then stop pumping.

This keeps the Windows tolerance for delayed first-byte delivery without adding a fixed multi-second wait to every command.

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

[qodo-code-review]

1. Fixed-delay resp pump 🐞 Bug ➹ Performance

In tests/acl_privileged_intercepts.rs, Resp::cmd() uses pump() which keeps polling until its
deadline even after the reply is already received; increasing the Windows window to 1500ms therefore
adds a fixed ~1.5s delay per command and can substantially slow these integration tests on Windows.

Agent Prompt

## Issue description
`Resp::pump()` runs until its deadline regardless of whether it already read a complete reply. With the PR’s Windows-only increase to 1500ms, each `cmd()` call can now incur a fixed ~1.5s polling window, significantly lengthening the test runtime on Windows.

## Issue Context
- `pump()` ignores timeout errors and continues looping until the deadline.
- `cmd()` clears the buffer, sends one command, then calls `pump(reply_window)`.
- The test issues many commands per run and runs twice (single-shard + multi-shard), so fixed per-command waits can accumulate.

## Fix Focus Areas
- tests/acl_privileged_intercepts.rs[135-145]
- tests/acl_privileged_intercepts.rs[147-163]
- tests/acl_privileged_intercepts.rs[168-269]

## Suggested implementation direction
Choose one:
1) **Deadline + early-exit on first timeout after progress**
   - Track whether any bytes were read (`saw_data: bool`).
   - On `Err(e)` where `e.kind()` is `TimedOut`/`WouldBlock`:
     - if `saw_data` is true, `break` (assume reply drained for now)
     - else continue until deadline
   - Keep the overall `deadline` as a hard cap for lag.

2) **RESP-aware read of a single frame**
   - Parse enough of the buffer to know when one full RESP reply is complete, then stop pumping.

This keeps the Windows tolerance for delayed first-byte delivery without adding a fixed multi-second wait to every command.

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