Skip to content

fix(ssh-console): avoid test port allocation race#4142

Open
pbreton wants to merge 2 commits into
NVIDIA:mainfrom
pbreton:agent/fix-ssh-console-port-race
Open

fix(ssh-console): avoid test port allocation race#4142
pbreton wants to merge 2 commits into
NVIDIA:mainfrom
pbreton:agent/fix-ssh-console-port-race

Conversation

@pbreton

@pbreton pbreton commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • let the SSH and metrics listeners atomically select ephemeral ports during their real bind
  • expose the effective listener addresses through SpawnHandle
  • update the SSH-console integration-test helper to use those effective addresses

Root cause

The test helper previously bound 127.0.0.1:0, recorded the selected port, dropped the listener, and later asked ssh_console::spawn to bind that port again. Parallel tests or another process could claim the released port in between, intermittently causing Address already in use failures such as the metrics-listener failure in this CI run.

Binding port 0 in the actual SSH and metrics servers removes that time-of-check/time-of-use window. Configured nonzero production addresses retain their existing behavior.

Validation

  • cargo fmt --all -- --check
  • git diff --check
  • cargo test -p carbide-ssh-console --no-default-features reached the crate but is blocked on macOS by existing Linux-specific termios/ioctl assumptions in io_util.rs
  • Linux cross-checking was attempted but is blocked locally by the unavailable x86_64-linux-gnu-gcc toolchain required by crypto dependencies

The Linux CI integration suite will provide the full runtime validation.

@copy-pr-bot

copy-pr-bot Bot commented Jul 24, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 58cd2733-a381-4d28-85dd-57985f9caddc

📥 Commits

Reviewing files that changed from the base of the PR and between b3385fd and 91da854.

📒 Files selected for processing (1)
  • crates/ssh-console/tests/util/ssh_console_test_helper.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/ssh-console/tests/util/ssh_console_test_helper.rs

Summary by CodeRabbit

  • New Features

    • Added accessors to retrieve the actual SSH server and metrics server addresses after startup.
    • Server addresses now reflect the ports actually assigned at runtime, including dynamically allocated ports.
  • Bug Fixes

    • Improved startup address reporting and validation.
    • Added connectivity checks to confirm both services are reachable after launch.

Walkthrough

SSH and metrics servers now report their actual bound socket addresses. SpawnHandle exposes both addresses through public getters, and the test helper uses port-zero configuration before retrieving and validating the assigned addresses.

Changes

Bound Address Propagation

Layer / File(s) Summary
Capture listener addresses
crates/ssh-console/src/metrics.rs, crates/ssh-console/src/ssh_server.rs
Both servers derive addresses from their bound listeners, log or propagate them, and expose them through their respective handles.
Expose addresses from spawn
crates/ssh-console/src/lib.rs
SpawnHandle stores the SSH and metrics addresses and provides public getter methods.
Use runtime addresses in tests
crates/ssh-console/tests/util/ssh_console_test_helper.rs
The helper configures 127.0.0.1:0, retrieves the assigned addresses from SpawnHandle, checks non-zero ports, and verifies TCP connectivity.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the primary change: eliminating the SSH-console port allocation race.
Description check ✅ Passed The description matches the code changes and explains the race fix, address exposure, and test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@pbreton

pbreton commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@pbreton, I’ll perform a complete review of all changes in #4142, with particular attention to listener binding semantics, address propagation, test isolation, and regression risks.

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/ssh-console/tests/util/ssh_console_test_helper.rs (1)

97-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the runtime-bound addresses.

Assert that both returned ports are non-zero and that clients can connect using the addresses reported by SpawnHandle; this protects the port-zero propagation contract from regressing.

As per path instructions, prioritize missing tests for changed behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ssh-console/tests/util/ssh_console_test_helper.rs` around lines 97 -
98, Add regression coverage near the existing SpawnHandle setup for
listen_address and metrics_address: assert each reported port is non-zero, then
connect clients using both returned addresses and verify the connections
succeed. Keep the test focused on validating runtime-assigned port propagation
through SpawnHandle.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/ssh-console/tests/util/ssh_console_test_helper.rs`:
- Around line 97-98: Add regression coverage near the existing SpawnHandle setup
for listen_address and metrics_address: assert each reported port is non-zero,
then connect clients using both returned addresses and verify the connections
succeed. Keep the test focused on validating runtime-assigned port propagation
through SpawnHandle.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8d011ab7-afc9-46e9-b25c-55f291549169

📥 Commits

Reviewing files that changed from the base of the PR and between 0e91f18 and b3385fd.

📒 Files selected for processing (4)
  • crates/ssh-console/src/lib.rs
  • crates/ssh-console/src/metrics.rs
  • crates/ssh-console/src/ssh_server.rs
  • crates/ssh-console/tests/util/ssh_console_test_helper.rs

@pbreton
pbreton marked this pull request as ready for review July 25, 2026 00:06
@pbreton
pbreton requested a review from a team as a code owner July 25, 2026 00:06
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.

1 participant