apollo_infra_utils: lease test port ranges below the ephemeral range - #14964
apollo_infra_utils: lease test port ranges below the ephemeral range#14964asaf-sw wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 5922ef0. Configure here.
| .unwrap_or_else(|error| panic!("Failed to create {lease_path:?}: {error}")), | ||
| Err(error) => panic!("Failed to open {lease_path:?}: {error}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Multi-user lease creation fails
Medium Severity
open_lease_file and the lease directory setup do not fully support the shared-machine case the comments describe. create_dir_all leaves apollo_test_port_slots at default 0755, so another user cannot create new slot files there. There is also a TOCTOU gap: after File::open returns NotFound, creating with write access panics with PermissionDenied if another user created the 0644 file in between, even though a read-only open would be lockable.
Reviewed by Cursor Bugbot for commit 5922ef0. Configure here.


Stacked on #14954, which fixes the interface half of this problem. This one closes the dominant remaining cause.
Problem
get_next_porthands a port number to a child process that binds it milliseconds later. If the kernel assigns that port to an outbound connection inside that window, the node dies withOs { code: 98, kind: AddrInUse }and takes the run down.That window is only dangerous for ports inside the kernel's ephemeral range (
/proc/sys/net/ipv4/ip_local_port_range,32768..60999by default). The harness handed out11000..63000, straddling it: in one integration run, 40 of the 203 allocated ports (19%) were inside the ephemeral range.The static partition cannot be moved below the floor. It reserves
MAX_NUMBER_OF_TESTS * MAX_NUMBER_OF_INSTANCES_PER_TEST * PORTS_PER_INSTANCE= 52000 ports for all 25 test identifiers simultaneously, while only 21768 ports exist below 32768.Shrinking
PORTS_PER_INSTANCEis not a way out either, which I found by trying it:integration_test_managerdraws5 * <services in the node>ports from a singleAvailablePorts, which is 60 for a distributed node and 35 for a hybrid one. A single measured run suggested a peak of 24 and would have led me to pick 32, breaking the distributed tests.Change
Ranges are leased at runtime instead of statically partitioned. Each slot is a file in a machine-wide temp directory held with an exclusive
flock, so concurrent test processes, including ones started from different checkouts, cannot be handed the same range. Only concurrently running tests consume slots, so the 272 slots that fit below the ephemeral floor are ample where the static scheme needed 52000 ports.TestIdentifierand the instance index survive as the label in the lease log line, so no call site changes.Two decisions worth flagging for review:
AvailablePortsis dropped. Call sites such ascreate_hybrid_component_configstake the ports they need and let theAvailablePortsgo while those ports stay in use for the rest of the test, so releasing on drop would hand a live range to another test. The OS releases the locks on exit, so a crashed test cannot leak a slot either.flockdoes not care whether the handle is writable, and on a shared machine another user's lease file is typically mode 0644, so requesting write access would fail on a slot that is perfectly lockable.A compile-time assert keeps every slot below
LOWEST_EPHEMERAL_PORT, so growing the budget past the floor is a build error rather than a new source of flakes.Verification
Full integration suite, all green:
An earlier run with
--no-captureconfirms the mechanism: nodes bound leased ports 11000, 11001, 11002, 11163 and 11165, with P2P dialing between them, every port below the ephemeral floor, and zeroAddrInUse.Unit tests cover that concurrently held leases do not overlap, that no allocated port reaches the ephemeral range, and that exhausting a range panics with a clear message.
Note for reviewers
This is a stacked PR, so
merge-gatekeeper-newwill time out on it with zero failed jobs until #14959 merges. That is the weakness #14959 fixes, not a problem with this change.