Skip to content

apollo_infra_utils: lease test port ranges below the ephemeral range - #14964

Open
asaf-sw wants to merge 1 commit into
asaf/ci-04-port-allocation-racefrom
asaf/ci-11-lease-test-port-ranges
Open

apollo_infra_utils: lease test port ranges below the ephemeral range#14964
asaf-sw wants to merge 1 commit into
asaf/ci-04-port-allocation-racefrom
asaf/ci-11-lease-test-port-ranges

Conversation

@asaf-sw

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

Copy link
Copy Markdown
Contributor

Stacked on #14954, which fixes the interface half of this problem. This one closes the dominant remaining cause.

Problem

get_next_port hands 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 with Os { 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..60999 by default). The harness handed out 11000..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_INSTANCE is not a way out either, which I found by trying it: integration_test_manager draws 5 * <services in the node> ports from a single AvailablePorts, 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.

TestIdentifier and the instance index survive as the label in the lease log line, so no call site changes.

Two decisions worth flagging for review:

  • Leases are held for the lifetime of the process, not released when an AvailablePorts is dropped. Call sites such as create_hybrid_component_configs take the ports they need and let the AvailablePorts go 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.
  • The lease file is opened read-only where it already exists. flock does 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:

$ SEED=0 cargo nextest run -p apollo_integration_tests --test-threads 2 --no-fail-fast
Summary [ 168.909s] 18 tests run: 18 passed, 0 skipped

An earlier run with --no-capture confirms the mechanism: nodes bound leased ports 11000, 11001, 11002, 11163 and 11165, with P2P dialing between them, every port below the ephemeral floor, and zero AddrInUse.

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-new will time out on it with zero failed jobs until #14959 merges. That is the weakness #14959 fixes, not a problem with this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@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 5922ef0. Configure here.

.unwrap_or_else(|error| panic!("Failed to create {lease_path:?}: {error}")),
Err(error) => panic!("Failed to open {lease_path:?}: {error}"),
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5922ef0. Configure here.

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