Add timeout handling and refactor connection checks in tests

[leynos]

Summary

• Adds timeout handling in test reads and refactors connection-closed checks into a shared helper to improve test reliability.

Changes

Tests

• Use timeout wrapper for reads in tests/preamble/responses.rs to prevent flaky reads (e.g., timeout(Duration::from_secs(1), stream.read_exact(...))).
• Refactor connection-closure logic into a shared helper to centralize behavior and improve readability.
• Replace ad-hoc connection-closed checks with a unified is_connection_closed helper in the timeout preamble tests.
• Update timeout tests to utilize the new helper, reducing duplicated logic.
• Apply shorter timeouts for timely preamble reads to verify responsiveness without blocking.

Tests Utilities

• Added is_connection_closed(read_res: std::io::Result) -> TestResult to:
  • Treat Ok(0) as the connection being closed (true).
  • Treat Ok(n) with n > 0 as a failure (not closed).
  • Treat Err with ConnectionReset as closed (true).
  • Propagate other errors.

Rationale

• Improves test stability by explicitly handling timeouts and connection closures in a single, reusable utility.
• Reduces duplication and potential inconsistencies in how tests determine that the connection has been closed.

Test Plan

• Run cargo test to ensure all tests pass across the preamble suite.
• Specifically verify:
  • Reads are wrapped with timeouts where appropriate.
  • Connection-closure logic behaves correctly across success and timeout scenarios.
  • Timely preamble tests remain responsive with shorter read timeouts.

Impact

• No changes to production code; all changes are contained to tests and test utilities. Potentially improves reliability of timeout-related test cases.

◳ Generated by DevBoxer ◰

---

ℹ️ Tag @devboxerhub to ask questions and address PR feedback

📎 Task: https://www.devboxer.com/task/5b5c7a1b-fa6d-4627-960e-a455ddd02680

Summary by Sourcery

Improve reliability of preamble timeout tests by adding shared connection-closure handling and wrapping test reads in timeouts.

Tests:

• Introduce a shared is_connection_closed helper to centralize connection-closed detection in preamble timeout tests.
• Wrap test stream reads in timeout calls in preamble timeout and response tests to avoid hanging and reduce flakiness.

[coderabbitai]

Summary by CodeRabbit

• Tests
  • Enforced timeouts on read operations (short guarded time windows) and ensured timeout and I/O errors propagate consistently.
  • Introduced a centralised connection-closure classification used across tests to standardise handling.
  • Improved test reliability around preamble handling and connection-close scenarios through consistent timeout and closure logic.

Walkthrough

State the test changes: add a 1-second timeout around a read in tests/preamble/responses.rs and centralise connection-closed logic into an is_connection_closed() helper used from tests/preamble/timeouts.rs; replace inline match/read-within branches with timeout-wrapped reads and calls to the helper.

Changes

Cohort / File(s)	Summary
Timeout enforcement & final read tests/preamble/responses.rs	Wrap read_exact with timeout(Duration::from_secs(1), ...); propagate timeout and I/O errors instead of a direct blocking read.
Connection-closed helper & tests refactor tests/preamble/timeouts.rs	Add READ_TIMEOUT_MS constant and is_connection_closed(read_res) helper to interpret Ok(0) and connection-abort/reset errors; replace inline read handling with timeout-wrapped reads and use the helper across affected tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Timeouts chime, the helper wakes,
Reads are wrapped for safety's sake,
Errors routed, closures known,
Tests now speak with steadier tone,
One second holds the final stake.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly summarises the main changes: adding timeout handling and refactoring connection checks in test code.
Description check	✅ Passed	The description comprehensively details the changes made, including specific modifications to timeout handling, the new helper function, and the rationale behind the refactoring.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch add-timeout-to-read-exact-in-tests-gpakze

---

No actionable comments were generated in the recent review. 🎉

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refactors preamble timeout tests to use a shared helper for connection-closed detection and wraps key read operations in short timeouts to reduce flakiness and improve reliability, without touching production code.

File-Level Changes

Change	Details	Files
Introduce a shared helper to standardize connection-closed detection logic in timeout tests.	Add is_connection_closed(read_res: std::io::Result) -> TestResult that interprets Ok(0) and ConnectionReset as a closed connection, treats Ok(n > 0) as a failure, and propagates other errors. Replace the inlined EOF/ConnectionReset matching logic in preamble_timeout_invokes_failure_handler_and_closes_connection with calls to is_connection_closed to centralize behavior.	tests/preamble/timeouts.rs
Wrap blocking read operations in preamble tests with timeouts to avoid hangs and improve test responsiveness.	Wrap read_exact in preamble_timeout_invokes_failure_handler_and_closes_connection in a 500ms timeout and the subsequent read in a 200ms timeout to ensure the test fails quickly if the connection does not close as expected. Wrap read_exact in preamble_timeout_allows_timely_preamble in a 150ms timeout to ensure timely preamble handling stays responsive. Wrap read_exact in success_callback_can_write_response in a 1s timeout to prevent the test from hanging on slow or stalled responses.	tests/preamble/timeouts.rs tests/preamble/responses.rs

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The new is_connection_closed helper only treats ConnectionReset as a closed-connection error; if your platform/CI sometimes surfaces EOF as ConnectionAborted or similar variants, consider broadening the match to those kinds as well to avoid test flakiness across environments.
• The 150ms timeout in preamble_timeout_allows_timely_preamble is quite tight and may be brittle on slower CI machines; consider relaxing this slightly (e.g., 300–500ms) while still keeping the test meaningfully fast.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `is_connection_closed` helper only treats `ConnectionReset` as a closed-connection error; if your platform/CI sometimes surfaces EOF as `ConnectionAborted` or similar variants, consider broadening the match to those kinds as well to avoid test flakiness across environments.
- The 150ms timeout in `preamble_timeout_allows_timely_preamble` is quite tight and may be brittle on slower CI machines; consider relaxing this slightly (e.g., 300–500ms) while still keeping the test meaningfully fast.

## Individual Comments

### Comment 1
<location> `tests/preamble/timeouts.rs:135` </location>
<code_context>

         let mut buf = [0u8; 2];
-        stream.read_exact(&mut buf).await?;
+        timeout(Duration::from_millis(150), stream.read_exact(&mut buf)).await??;
         let _ = result_tx.send((buf, failure_fired));
         Ok(())
</code_context>

<issue_to_address>
**suggestion (testing):** Consider slightly more generous or configurable timeouts to reduce CI flakiness.

150 ms for `read_exact` may be too strict on slow or heavily loaded CI runners, especially with other async work. Consider increasing it (e.g., 300–500 ms) or extracting these values into shared constants so they’re easy to adjust if flakiness shows up.

Suggested implementation:

```rust
        let mut buf = [0u8; 2];

    })
            .is_ok();

        const READ_TIMEOUT_MS: u64 = 500;

        let mut buf = [0u8; 2];
        timeout(Duration::from_millis(READ_TIMEOUT_MS), stream.read_exact(&mut buf)).await??;
        let _ = result_tx.send((buf, failure_fired));
        Ok(())
    })

```

1. If this file (or related test modules) uses similar hardcoded timeouts elsewhere, consider extracting `READ_TIMEOUT_MS` to the module level (outside any function) and reusing it in all relevant places.
2. If you later want per-test configurability (e.g., via env vars for CI vs local), you can replace the `const` with a small helper function that reads from `std::env` with a sensible default, and use that in the `Duration::from_millis(...)` call.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78a4f74635

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/preamble/timeouts.rs`:
- Around line 28-46: Replace the remaining hard-coded 500 ms timeout with the
existing READ_TIMEOUT_MS constant to standardize timeouts; find occurrences
(e.g., the other read timeout usage noted in the module) and change the literal
500 (or 500_u64/Duration::from_millis(500)) to use READ_TIMEOUT_MS (or
Duration::from_millis(READ_TIMEOUT_MS)) so the is_connection_closed function and
other reads share the same constant.

[leynos]

@coderabbitai Have the following now been resolved?

Please address the comments from this code review:

## Overall Comments
- The new `is_connection_closed` helper only treats `ConnectionReset` as a closed-connection error; if your platform/CI sometimes surfaces EOF as `ConnectionAborted` or similar variants, consider broadening the match to those kinds as well to avoid test flakiness across environments.
- The 150ms timeout in `preamble_timeout_allows_timely_preamble` is quite tight and may be brittle on slower CI machines; consider relaxing this slightly (e.g., 300–500ms) while still keeping the test meaningfully fast.

## Individual Comments

### Comment 1
<location> `tests/preamble/timeouts.rs:135` </location>
<code_context>

         let mut buf = [0u8; 2];
-        stream.read_exact(&mut buf).await?;
+        timeout(Duration::from_millis(150), stream.read_exact(&mut buf)).await??;
         let _ = result_tx.send((buf, failure_fired));
         Ok(())
</code_context>

<issue_to_address>
**suggestion (testing):** Consider slightly more generous or configurable timeouts to reduce CI flakiness.

150 ms for `read_exact` may be too strict on slow or heavily loaded CI runners, especially with other async work. Consider increasing it (e.g., 300–500 ms) or extracting these values into shared constants so they’re easy to adjust if flakiness shows up.

Suggested implementation:

```rust
        let mut buf = [0u8; 2];

    })
            .is_ok();

        const READ_TIMEOUT_MS: u64 = 500;

        let mut buf = [0u8; 2];
        timeout(Duration::from_millis(READ_TIMEOUT_MS), stream.read_exact(&mut buf)).await??;
        let _ = result_tx.send((buf, failure_fired));
        Ok(())
    })

```

1. If this file (or related test modules) uses similar hardcoded timeouts elsewhere, consider extracting `READ_TIMEOUT_MS` to the module level (outside any function) and reusing it in all relevant places.
2. If you later want per-test configurability (e.g., via env vars for CI vs local), you can replace the `const` with a small helper function that reads from `std::env` with a sensible default, and use that in the `Duration::from_millis(...)` call.
</issue_to_address>

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the timeouts.rs file to verify the changes
cat tests/preamble/timeouts.rs

Length of output: 5139

---

@leynos Yes, all the review comments have been addressed! ✅

Here's the verification:

1. READ_TIMEOUT_MS constant added (Line 29):

   const READ_TIMEOUT_MS: u64 = 500;

2. is_connection_closed now handles both ConnectionAborted and ConnectionReset (Lines 38-42):

   Err(e)
       if matches!(
           e.kind(),
           io::ErrorKind::ConnectionAborted | io::ErrorKind::ConnectionReset
       ) =>

3. Timeout increased to 500ms (Lines 137-140):

   timeout(
       Duration::from_millis(READ_TIMEOUT_MS),
       stream.read_exact(&mut buf),
   )
   .await??;

The code now:

• Uses a shared READ_TIMEOUT_MS constant set to 500ms (addressing CI flakiness concerns)
• Handles both ConnectionAborted and ConnectionReset error kinds in the helper (improving cross-platform robustness)
• Applies the constant consistently across timeout tests

All suggested improvements from the review have been implemented.