fix(test): stop asserting a freed ephemeral port stays free - #1399
Conversation
`tcp_readiness_distinguishes_live_and_free_endpoints` failed on a macOS
runner, on a PR touching only fbuild-library:
assertion failed: !tcp_endpoint_ready(address, Duration::from_millis(100)).await
The probe was right and the test was wrong. `tcp_endpoint_ready` is a plain
connect-with-timeout, so reporting "ready" means something genuinely
accepted. Once the listener is dropped the port returns to the ephemeral
pool and the OS may hand it to anyone — including a sibling test in the same
binary calling `bind_tcp_listener("127.0.0.1:0")`, since these run in
parallel. The test asserted a property the OS does not guarantee, then blamed
the probe when the OS exercised its freedom.
The live half is sound as a single sample: we hold the listener, so nothing
else can answer. Only the dead half needed changing — it now retries with a
freshly-bound-then-dropped port each attempt. A real defect in the probe
fails all eight; a port-reuse collision would have to win the race eight
times running.
Deliberately not a `>= `-style loosening of what is asserted: the assertion
still fails if a closed endpoint ever reads as ready, which is the property
worth having. What changed is the number of samples it takes before
concluding that, because one sample cannot distinguish "probe is broken"
from "someone took the port".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe TCP readiness test now retries checks against freshly bound and dropped ephemeral ports. It returns when readiness correctly fails and panics only when all attempts incorrectly report readiness. ChangesTCP readiness test
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized test-only change replaces a flaky assertion about ephemeral port reuse with repeated fresh-port checks while preserving the readiness behavior being tested; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Symptom
tcp_readiness_distinguishes_live_and_free_endpointsfailed on a macOSrunner, on #1398 — a PR touching only
fbuild-library:The probe is right; the test was wrong
tcp_endpoint_readyis a plain connect-with-timeout:So "ready" means something genuinely accepted a connection. The test then
asserts that after
drop(listener), nothing will.That is not a property the OS guarantees. Once the listener is dropped the
port returns to the ephemeral pool and can be handed to anyone — including
a sibling test in this same binary calling
bind_tcp_listener("127.0.0.1:0"), since tests run in parallel. When thathappened the probe correctly reported "ready" and the assertion blamed the
probe.
Fix
The live half is sound as a single sample — we hold the listener, so
nothing else can be answering. Unchanged.
The dead half now retries with a freshly-bound-then-dropped port each
attempt. A real defect in the probe fails all eight; a port-reuse collision
would have to win the race eight times running.
What this is not
It is not a
>=-style loosening. The assertion still fails if a closedendpoint ever reads as ready — that property is intact and is the one worth
having. What changed is how many samples it takes before concluding it, because
one sample cannot distinguish "the probe is broken" from "someone took the
port". Those need different fixes, and the old test reported the second as
the first.
Verification
soldr cargo test -p fbuild-core --lib platform::ipc— 2 passedsoldr cargo test -p fbuild-core --doc— 2 passed (added to my routine afterfix(esp8266): fetch the core from a release asset so submodules come with it #1398, where a
--lib-only check missed a doctest failure)-D warnings— cleanA timing race can't be proven fixed by a green run; the argument is that the
test no longer asserts something the OS is free to violate.
Summary by CodeRabbit