Repository navigation
test(minimald): give each own-address test process its own declared port - #2751
norrietaylor wants to merge 1 commit into
Conversation
A finalize at the shared 127.0.0.1 fallback probes the box's declared port with a real bind. Every own-address test declared 18080, and nextest runs tests as parallel processes, so two fallback tests scheduled together saw each other's probe and refused the activation as held by another program. Each test process now declares a port the kernel reports free.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Select the declared test port crates/minimald/src/box_actor/tests.rs |
A helper selects an ephemeral loopback port. The own-address and host-port conflict tests use it. |
Update routing and verdict assertions crates/minimald/src/box_actor/tests.rs |
Fallback-address, pending-verdict, and route assertions check the selected port. |
Update shared-address and handover assertions crates/minimald/src/box_actor/tests.rs |
Shared-address collision, restart, ordering, persisted-port, and handover assertions check the selected port. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Possibly related PRs
- gominimal/minimal#2352: Adds a finalize-time bind probe for declared host ports, which uses the declared-port test path updated here.
Suggested reviewers: bryan-minimal
Merge Risk: 
7185e
-
Parallel test processes can still intermittently fail during fallback finalization, but the risk is limited to this test workflow.
Pre-merge checks: 
-
Check name Status Explanation Title check The title uses the required Conventional Commit format and clearly describes the test change that assigns each own-address test process its own declared port. Description check The description explains the cause, fix, scope, and test results. It includes Summary and testing information, although it uses “Test plan” instead of the template’s “Testing” heading and omits the te… Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the cause, fix, scope, and test results. It includes Summary and testing information, although it uses “Test plan” instead of the template’s “Testing” heading and omits the template checklist.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 1 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit picked a port to test,
An ephemeral loopback guest.
The routes and logs now check it too,
Collision cases follow through.
No fixed old number in the nest.
Comment @coderabbitai help to get the list of available commands.
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. |
| fn declared_port() -> u16 { | ||
| static PORT: std::sync::OnceLock<u16> = std::sync::OnceLock::new(); | ||
| *PORT.get_or_init(|| { | ||
| std::net::TcpListener::bind((std::net::Ipv4Addr::LOCALHOST, 0)) | ||
| .and_then(|listener| listener.local_addr()) | ||
| .expect("the kernel hands out a free loopback port") | ||
| .port() | ||
| }) | ||
| } |
There was a problem hiding this comment.
LOW · correctness · confidence 0.50 · in declared_port
declared_port drops the probe listener before it returns, so the kernel can give the same port to another test process or to an outbound loopback connection. The race is much rarer than a shared fixed port, but it is not closed. A flaky Port ... is already in use refusal can still occur under parallel nextest runs.
What the reviewer checked
tests.rs:3338-3342 bind((LOCALHOST, 0)).and_then(|l| l.local_addr()).expect(..).port() // listener dropped at end of closure
Hand this to an agent
Act on a code-review finding.
File: crates/minimald/src/box_actor/tests.rs
Lines: 3336-3344
Category: correctness
Severity: low (reviewer confidence 0.50)
Finding:
`declared_port` drops the probe listener before it returns, so the kernel can give the same port to another test process or to an outbound loopback connection. The race is much rarer than a shared fixed port, but it is not closed. A flaky `Port ... is already in use` refusal can still occur under parallel nextest runs.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
|
Closing as a duplicate of #2752, per the repo owner. Both give each own-address test its own declared port so that a finalize's probe of Generated by Claude Code |
Summary
Fixes the red
CI (Linux native)lane onmainsince 5d339df (#2733) and de32bd3 (#2741):Cause. Since T100 (#2725), a finalize at the shared
127.0.0.1fallback probes each declared port with a realTcpListener::bind(host_ports_in_useinnet/policy.rs). Every own-address test inbox_actor/tests.rsdeclared host port 18080, and nextest runs each test as its own process in parallel. When two fallback tests run at the same moment, one sees the other's probe and refuses the activation as held by another program. In both failing CI runs,a_box_destroyed_inside_the_landing_window_is_not_resurrectedanda_box_destroyed_between_enumeration_and_grant_is_released_oncestarted together. The scheduling shifted when #2733 added tests. The failing test passes when run alone.Fix. This change is test-only.
declared_port()takes a port the kernel reports free, once per test process, and every former 18080 inbox_actor/tests.rsuses it. Tests in the same process still agree on one port, and parallel processes no longer share one. No product code changes.Test plan
Run in the Lima Linux VM (aarch64, 18 CPUs):
cargo nextest run -p minimald --no-fail-faston this branch: 1195 passed, 2 failed. Both failures also occur on unmodifiedmainin this VM:a_hook_launch_does_not_advise_unenforced_placement(cgroup EACCES) andthe_ruleset_rejects_udp_443_from_leaves_in_the_blocked_set. Neither involves this test file.--test-threads 16, 15 times.mainfailed 2 times; this branch failed 0 times.just fmt-checkSummary by CodeRabbit