Skip to content

apollo_infra_utils: probe test ports on the interface servers actually bind - #14954

Open
asaf-sw wants to merge 1 commit into
mainfrom
asaf/ci-04-port-allocation-race
Open

apollo_infra_utils: probe test ports on the interface servers actually bind#14954
asaf-sw wants to merge 1 commit into
mainfrom
asaf/ci-04-port-allocation-race

Conversation

@asaf-sw

@asaf-sw asaf-sw commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes the test port probe to use the interface the servers actually bind. run-integration-tests has been red on main's nightly for 6+ consecutive nights with AddrInUse, because a holder on any non-loopback interface passed the old probe and then collided on bind.

This closes the interface half of the problem. The remaining TOCTOU window is closed by #14964.


Detailed Summary for AI Bots

Problem

Failed to start MonitoringEndpoint: Os { code: 98, kind: AddrInUse }

is_port_in_use probed 127.0.0.1 while the servers under test bind 0.0.0.0. CI runners have several interfaces.

Change

Probe the unspecified address, so the probe conflicts with exactly what a later bind would.

port_held_on_another_interface_is_reported_in_use binds a holder on 127.0.0.2 and fails on the old code. It reports and returns on macOS, which assigns only 127.0.0.1 to loopback unless an alias was added.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit aa117aa. Configure here.

Comment thread crates/apollo_infra_utils/src/test_utils_test.rs
@asaf-sw
asaf-sw force-pushed the asaf/ci-04-port-allocation-race branch from aa117aa to 571fc3a Compare August 13, 2026 05:44
@asaf-sw

asaf-sw commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Note on the run-integration-tests failure on this PR

It is the residual TOCTOU case described above, not a regression from this change. Evidence from the failing run (job 94356433877):

Failed to start MonitoringEndpoint: Os { code: 98, kind: AddrInUse }
Node NodeRunner { node_index: 3, node_execution_id: "hybrid_mempool" } stopped unexpectedly
Node service Hybrid(Mempool) unexpectedly stopped.

The harness logged 203 Allocated port lines and zero Skipping occupied port lines, so the new 0.0.0.0 probe made exactly the same decisions the old 127.0.0.1 probe would have made in this run. The allocation sequence is unchanged; the port passed the probe and was taken before the child process bound it.

The probe change can only ever report more ports as occupied, never fewer, so it cannot introduce an AddrInUse that the previous code avoided.

The same run also makes the ephemeral-range hazard concrete: of the 203 ports handed out (31800 to 32863), 40 (19%) fall inside the kernel's default ephemeral range 32768-60999, where an outbound connection can claim the port inside the TOCTOU window.

Closing that window needs one of the two follow-ups noted in the description: retry on AddrInUse at node startup instead of aborting the run, or shrink the per-test port budget so the whole range sits below the ephemeral floor. Happy to take either as the next PR. I have rerun the failed job in the meantime.

…y bind

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@asaf-sw
asaf-sw force-pushed the asaf/ci-04-port-allocation-race branch from 44ad89f to 9267186 Compare September 10, 2026 05:23
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.

2 participants