Lochnair-Labs/irtt-rs
2
0 Fork 0
Code Issues 1 Pull Requests 4 Actions 8 Packages Projects Releases 6 Wiki Activity

test(cli): make integration synchronization deterministic #61

Merged
Lochnair merged 2 commits from test/cli-deterministic-synchronization into main 2026-08-10 16:21:01 +00:00
Conversation 4 Commits 2 Files Changed 1
+32 -121
Lochnair commented 2026-08-10 16:05:15 +00:00 (Migrated from github.com)
Copy link
Copy source
No description provided.
chatgpt-codex-connector[bot] (Migrated from github.com) reviewed 2026-08-10 16:08:33 +00:00
chatgpt-codex-connector[bot] (Migrated from github.com) left a comment
Copy link
Copy source

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e31e2d5be7

ℹ️ 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".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `e31e2d5be7` <details> <summary>ℹ️ About Codex in GitHub</summary> <br/> [Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). 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". </details>
crates/irtt-cli/tests/client_multi_target.rs
chatgpt-codex-connector[bot] (Migrated from github.com) commented 2026-08-10 16:08:33 +00:00
Copy link
Copy source

P2 Badge Keep the first-probe wait on the advertised timeout

Once the synchronization signal is deferred to this first-reply path, the socket's 500 ms read timeout becomes the real deadline rather than the receiver's stated 2 seconds. On a loaded Unix CI worker, the CLI can complete OPEN but be descheduled for more than 500 ms before sending its first probe; recv_request_timeout then exits, drops this sender, and both interruption tests panic even though their explicit synchronization timeout has not elapsed. Retain the longer timeout until the first probe/reply has been observed, then shorten it for shutdown cleanup.

AGENTS.md reference: AGENTS.md:L125-L130

Useful? React with 👍 / 👎.

**<sub><sub>![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)</sub></sub> Keep the first-probe wait on the advertised timeout** Once the synchronization signal is deferred to this first-reply path, the socket's 500 ms read timeout becomes the real deadline rather than the receiver's stated 2 seconds. On a loaded Unix CI worker, the CLI can complete OPEN but be descheduled for more than 500 ms before sending its first probe; `recv_request_timeout` then exits, drops this sender, and both interruption tests panic even though their explicit synchronization timeout has not elapsed. Retain the longer timeout until the first probe/reply has been observed, then shorten it for shutdown cleanup. AGENTS.md reference: [AGENTS.md:L125-L130](https://github.com/Lochnair/irtt-rs/blob/e31e2d5be778dc842f56c53062d69a60931382f7/AGENTS.md#L125-L130) Useful? React with 👍 / 👎.
Lochnair (Migrated from github.com) reviewed 2026-08-10 16:12:24 +00:00
crates/irtt-cli/tests/client_multi_target.rs
Lochnair (Migrated from github.com) commented 2026-08-10 16:12:24 +00:00
Copy link
Copy source

Fixed: the socket now keeps the longer (2s-bounded) read timeout until it actually receives the first probe, and only shortens to 500ms afterward to bound the post-interrupt shutdown wait. See 1f44f26.

🤖 Addressed by Claude Code

Fixed: the socket now keeps the longer (2s-bounded) read timeout until it actually receives the first probe, and only shortens to 500ms afterward to bound the post-interrupt shutdown wait. See 1f44f26. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
chatgpt-codex-connector[bot] (Migrated from github.com) reviewed 2026-08-10 16:17:13 +00:00
chatgpt-codex-connector[bot] (Migrated from github.com) left a comment
Copy link
Copy source

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99043444a1

ℹ️ 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".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `99043444a1` <details> <summary>ℹ️ About Codex in GitHub</summary> <br/> [Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). 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". </details>
crates/irtt-cli/tests/client_multi_target.rs
chatgpt-codex-connector[bot] (Migrated from github.com) commented 2026-08-10 16:17:13 +00:00
Copy link
Copy source

P2 Badge Wait for the client to consume the first reply

When the fake-server thread wins scheduling after send_to, this signal only proves that the UDP datagram was queued by the server, not that the managed client received and emitted it. The test thread can therefore send SIGINT before the client processes the reply, causing continuous_single_target_interruption_drains_final_events_and_succeeds to close without the required echo_reply row and fail nondeterministically. Synchronize on a client-observable event, such as reading that output row, before interrupting.

AGENTS.md reference: AGENTS.md:L125-L128

Useful? React with 👍 / 👎.

**<sub><sub>![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)</sub></sub> Wait for the client to consume the first reply** When the fake-server thread wins scheduling after `send_to`, this signal only proves that the UDP datagram was queued by the server, not that the managed client received and emitted it. The test thread can therefore send SIGINT before the client processes the reply, causing `continuous_single_target_interruption_drains_final_events_and_succeeds` to close without the required `echo_reply` row and fail nondeterministically. Synchronize on a client-observable event, such as reading that output row, before interrupting. AGENTS.md reference: [AGENTS.md:L125-L128](https://github.com/Lochnair/irtt-rs/blob/99043444a1d2c3cefc183ddc91627029acb1c1d3/AGENTS.md#L125-L128) Useful? React with 👍 / 👎.
Lochnair (Migrated from github.com) reviewed 2026-08-10 16:19:53 +00:00
crates/irtt-cli/tests/client_multi_target.rs
Lochnair (Migrated from github.com) commented 2026-08-10 16:19:53 +00:00
Copy link
Copy source

Not changing the synchronization here: gating SIGINT on reply-sent (rather than client-consumed) is intentional — this test specifically covers the case where a reply is still in flight/queued at interrupt time, and the echo_reply assertion proves shutdown drains it instead of losing it. Waiting for client-side consumption first would remove that scenario entirely. Added a comment (78151ff) documenting this so it's not mistaken for a race next time.

🤖 Addressed by Claude Code

Not changing the synchronization here: gating SIGINT on reply-sent (rather than client-consumed) is intentional — this test specifically covers the case where a reply is still in flight/queued at interrupt time, and the `echo_reply` assertion proves shutdown drains it instead of losing it. Waiting for client-side consumption first would remove that scenario entirely. Added a comment (78151ff) documenting this so it's not mistaken for a race next time. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
Sign in to join this conversation.
Reviewers
No Reviewers
chatgpt-codex-connector[bot]
Lochnair
Labels
Clear labels
bug
Something isn't working
 dependencies
documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
Lochnair renovate (Renovate Bot) Bot
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Lochnair-Labs/irtt-rs#61