Repository navigation
fix(minimald): follow the bound port in the transient-publish test - #1965
Conversation
The proxy's serve task is spawned the moment its listener binds, so a connect can land before the accept loop is ready (or, on a busy host, after another process took the probed port). proxy_get unwrapped the first connect, so a_transient_publish_failure_keeps_the_default_port flaked with ConnectionRefused (Os 111). Retry the connect briefly (5s timeout, 10ms backoff on ConnectionRefused) instead of unwrapping the first attempt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
[inbox-patch:clobber] This PR would revert recently merged workSimulating this merge shows it undoing commits that landed on the base branch within the last 3 day(s):
Before merging, restore the reverted region to its state on the base branch. The rest of this PR is unaffected. If a listed commit is one this PR intends to supersede, say so and merge — this check reports, it does not block. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe transient-publish test now reads the listener’s bound port from the publish warning. It uses that port to check failed publish suggestions and listener availability. It checks the default port only when the warning does not report a relocation. ChangesTransient publish test
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is identified. Run the Linux test as part of normal checks before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit checks the port with care, Comment |
The ConnectionRefused flake is not a connect racing the accept loop: the listener is bound (and listening) before any publish attempt, and the test only connects after two publish failures. It happens when a parallel process takes the probed port before the bind, so NET-025 relocates the bind and nothing listens on the probed port; a connect retry cannot recover from that. Read the bound port off the publish warning, connect there, and hold it to the probed port whenever the bind did not relocate. Drop the proxy_get retry.
|
Pushed b27e1ea, which replaces the connect retry. The ConnectionRefused is not a connect racing the accept loop: |
|
/review |
|
Approved: no findings at Nothing to do. Clobber: none: the surrounding |
There was a problem hiding this comment.
Third review applied; CI green, minimal-review approved.
Summary
Fixes a flaky test in
crates/minimald/src/server.rs:a_transient_publish_failure_keeps_the_default_portpanicked withConnectionRefused(Os 111) when it connected to the probed default port.The test picks a free port by binding a probe and dropping it, then hands that port to the proxy as its default. Between the drop and the proxy's bind, another process can take the port; NET-025 then relocates the bind ("selecting a free one") and nothing listens on the probed port. The test only connects after it has logged two failed publishes, so the listener is already accepting by then: a refused connect means the listener was not on the probed port, not that the accept loop was late.
Change
guest_port=field and connects there.guest_port == default_portunless the bind relocated (the log contains "selecting a free one").host_port == guest_port == bound_port, so a transient publish failure is still shown to keep the bound port with no walk.proxy_getis unchanged. This is a test-only change; no production code is touched.Verification
CI's Linux lanes are the proof: the test is
#[cfg(target_os = "linux")]andminimalddoes not build natively on macOS.Routing-Key: inbox-route/I_kwDOSUhdos8AAAABU-ETRg
Summary by CodeRabbit