Skip to content

fix(onboard): bound gateway preflight probes#6754

Closed
ogarciarevett wants to merge 1 commit into
NVIDIA:mainfrom
ogarciarevett:fix/onboard-port-preflight
Closed

fix(onboard): bound gateway preflight probes#6754
ogarciarevett wants to merge 1 commit into
NVIDIA:mainfrom
ogarciarevett:fix/onboard-port-preflight

Conversation

@ogarciarevett

@ogarciarevett ogarciarevett commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Onboarding now bounds OpenShell gateway metadata probes so an unrelated listener cannot stall preflight indefinitely. Port-conflict failures also use the shared stderr-aware failure style.

Related Issue

Fixes #6752

Changes

  • Apply a five-second timeout to each OpenShell status and gateway-info probe used for gateway reuse detection.
  • Render the required-port conflict heading through the shared failLine helper.
  • Add regression coverage for the probe timeout and plain-stderr failure output.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: the required documentation review confirmed that existing gateway-reuse and port-conflict recovery guidance remains accurate; the timeout is an internal safety bound.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Pending maintainer review; no waiver requested.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project cli src/lib/onboard/gateway-reuse.test.ts src/lib/onboard/preflight-messages.test.ts (9 passed); npm run test:changed (1,057 passed)
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Not applicable to this focused onboarding change.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: ogarciarevett ogarciarevett@gmail.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved onboarding messages for unavailable ports with consistent formatting.
    • Prevented gateway inspection checks from hanging by applying a five-second timeout.
  • Tests

    • Added coverage for gateway inspection timeouts and port-conflict message formatting.

Signed-off-by: ogarciarevett <ogarciarevett@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 13, 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: 8ce0ba38-d6fd-40b8-80e1-0a2c0a3f8070

📥 Commits

Reviewing files that changed from the base of the PR and between 3de1de6 and 6076174.

📒 Files selected for processing (5)
  • src/lib/onboard.ts
  • src/lib/onboard/gateway-reuse.test.ts
  • src/lib/onboard/gateway-reuse.ts
  • src/lib/onboard/preflight-messages.test.ts
  • src/lib/onboard/preflight-messages.ts

📝 Walkthrough

Walkthrough

Onboarding preflight now formats port conflicts through the shared error renderer and applies a five-second timeout to gateway reuse inspection probes. Tests cover the rendered port message and timeout options passed to all three OpenShell capture calls.

Changes

Onboarding preflight diagnostics

Layer / File(s) Summary
Port-conflict message rendering
src/lib/onboard/preflight-messages.ts, src/lib/onboard.ts, src/lib/onboard/preflight-messages.test.ts
Adds and tests the shared port-unavailable error helper, then uses it for preflight port conflicts.
Gateway inspection timeout
src/lib/onboard/gateway-reuse.ts, src/lib/onboard/gateway-reuse.test.ts
Applies a 5-second timeout to status and gateway information probes and verifies all three calls use the timeout and ignoreError.

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

Possibly related PRs

Suggested labels: NV QA, bug-fix, area: sandbox

Suggested reviewers: cv, ericksoa, prekshivyas

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and matches the main change: bounding onboard gateway preflight probes.
Linked Issues check ✅ Passed The PR adds bounded gateway inspection timeouts and the shared port-unavailable formatter, matching the linked issue requirements.
Out of Scope Changes check ✅ Passed The changes are focused on gateway probe timeouts, port error formatting, and related tests, with no obvious unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: onboard-resume, onboard-repair, cloud-onboard
Optional E2E: double-onboard

Dispatch hint: onboard-resume,onboard-repair,cloud-onboard,double-onboard

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: target/main
Head: HEAD
Confidence: high

Required E2E

  • onboard-resume: Required deterministic lifecycle-state validation floor for onboarding changes; verifies interrupted onboarding can resume against real persisted state and runtime resources.
  • onboard-repair: Required deterministic lifecycle-state validation floor; verifies repair/reconciliation when recorded onboarding state and live sandbox state diverge.
  • cloud-onboard: Gateway inspection and port preflight run in the general onboarding path and can affect full hosted onboarding before sandbox creation.

Optional E2E

  • double-onboard: Directly exercises repeated onboarding with a healthy existing gateway and asserts that gateway reuse avoids port conflicts; it is the closest existing coverage for the modified reuse path.

New E2E recommendations

  • installer-onboarding (medium): Current live double-onboard coverage proves healthy gateway reuse, but does not exercise a foreign occupied listener whose OpenShell metadata inspection stalls. The new five-second probe timeout is only unit-covered.
    • Suggested test: Add a live onboarding preflight scenario that occupies the gateway port with a foreign listener and makes gateway metadata inspection unresponsive; assert onboarding fails or recovers within the bounded timeout without deleting foreign resources.

Dispatch hint

  • Workflow: E2E
  • jobs input: onboard-resume,onboard-repair,cloud-onboard,double-onboard

@sandl99

sandl99 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the detailed report and repro steps. We have a fix in review at #6759, which adds an early foreign-listener check, preserves gateway reuse, and adds regression coverage for the hang path. I’ll keep this issue open until that PR merges so GitHub can close it automatically.

@sandl99 sandl99 closed this Jul 13, 2026
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.

[DGX Station][All platforms] Port 8080 conflict can stall onboarding and lacks warning contrast

2 participants