Skip to content

test(tokio): reserve the failing redial listener - #6032

Merged
kixelated merged 7 commits into
mainfrom
quest/m1/test-flakes-2/tokio-redial
Oct 20, 2026
Merged

kixelated merged 7 commits into
mainfrom
quest/m1/test-flakes-2/tokio-redial

Conversation

@kixelated

@kixelated kixelated commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

a_failing_redial_reports_why_until_it_connects and a_fleet_drain_redials_through_a_fresh_resolve failed together in CI (attempt 1), started back to back and both timed out at 10 s. That was one event, not two flakes:

  1. The failing-redial test bound a port, dropped it, and kept redialing it, assuming nothing would ever listen there.
  2. The fleet test's first server bound that freed port, so the failing-redial client connected to it and never reported a failure.
  3. The fleet test then took that foreign session as its own session_a and drained it. Its own client never got the GOAWAY, so it never redialed relay B.

closing_between_retries_clears_the_failure uses the same released-port fixture, with the same exposure.

Approach

  • Both failing tests use the existing dead() fixture, a held listener that hangs up on each connection. It works the same on every platform.
  • They wait on the connection's state notifications instead of polling with sleeps and 10 s deadlines.
  • The failing-redial test asserts that the port stays reserved, that the public monitor reports the retry reason as a tcp::Error::Connect, and that it clears once the connection lands.
  • The fleet test's own listeners are already held, so it needs no change.
  • The quest is finished and deleted. Loaded-machine verification stays with the epic README's own work.

Decisions

No maintainer input; recommended options picked.

  • Fleet test diagnosis: ✅ treat its CI failure as collateral of the released port and finish the quest here. Alternative: keep the quest and this PR in draft until the fleet test fails on its own, which no run has shown.
  • Sibling closing_between_retries_clears_the_failure: ✅ move it to dead() in this PR, since it is the same root cause. Alternative: a separate quest.
  • a_refused_tcp_dial_keeps_retrying keeps its released port: ✅ connection refusal is what it tests, and a held listener cannot refuse a connection on every OS. Its retry window is 100 ms of paused time, so the exposure is small.

Impact

  • Public API: none. Wire: none. Production code: none.

Validation

  • Deterministic port takeover on the original fixture fails before the fix (earlier checkpoint).
  • 300 stress iterations of all three tests at load average 70 to 85 pass on this head. They also pass on the base, so the collision needs the full suite's other port users and doesn't reproduce in isolation.
  • just check passes on the merged head af5fc4e: 4,697 native tests, 515 capture, 216 playback, plus lint and docs.

Follow-ups

  • server::tests::a_failed_listen_binds_nothing has the same probe-then-release pattern, so a collision there fails its first bind. It belongs to the epic's loaded just check --all sweep.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 3 commits October 19, 2026 19:02
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Checkpoint: fixed the reproduced released-port race in the failing-redial fixture and replaced its sleep polling with state notifications. The test still asserts the public monitor reports the retry reason and clears it after connection.

Full just check passed at 5790779. Final fcf0b54 adds only the public-accessor test assertion; the focused pair, formatting, quest check (432 documents), and diff check pass there. Production code is unchanged.

Fleet-drain diagnosis and loaded-host/CI reliability remain unfinished, so the quest stays present and this PR stays draft. Suggested next step: quest-iterate with a concrete independent fleet failure trace. Nothing was merged, readied, or enabled for auto-merge.

(written by GPT-6.1-Sol)

kixelated and others added 3 commits October 19, 2026 21:46
A released port can become another test's server, as it did when the failing redial and the fleet drain failed together in CI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CI failures of both tests were one event: the failing redial released its port, the fleet test's first server bound it, and each test then talked to the other's peer. Holding the port removes both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Iterated: no automated review findings were posted, and CI was green.

  • Diagnosed the fleet-drain failure. In the linked CI attempt, both tests timed out together because the failing redial's released port became the fleet test's relay A. Each test then talked to the other's peer. The held-listener fix already covers this, so the quest is finished and deleted.
  • Moved closing_between_retries_clears_the_failure, which had the same released-port fixture, onto dead() and state notifications.
  • Merged origin/main. just check passes, and 300 loaded stress iterations of all three tests pass.

The decisions and their alternatives are in the description.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 20, 2026 05:31
@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 00ea9ae8-0eb2-4afa-ae3b-0b16951fd1ac
📥 Commits

Reviewing files that changed from the base of the PR and between 9b90cdd and af5fc4e.

📒 Files selected for processing: 3
  • quest/m1/test-flakes-2/README.md
  • quest/m1/test-flakes-2/tokio-redial.md
  • rs/moq-tokio/src/connection.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-20T09:01:51.339091Z 81d2dcd New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

…2/tokio-redial

# Conflicts:
#	quest/m1/test-flakes-2/README.md
@kixelated
kixelated added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit fae2c90 Oct 20, 2026
4 checks passed
@kixelated
kixelated deleted the quest/m1/test-flakes-2/tokio-redial branch October 20, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant