fix(onboard): stop the gateway when its sandbox-bridge probe fails (#5513) - #5536
fix(onboard): stop the gateway when its sandbox-bridge probe fails (#5513)#5536abhi-0906 wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesGateway orphan cleanup on sandbox-bridge unreachability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 2193-2206: The src/lib/onboard.ts file has exceeded the growth
budget with a net addition of 17 lines, blocking the CI guardrail. To fix this,
reduce the net additions by either condensing the lengthy inline comment block
(the "// `#5513`:" explanation) that precedes the stopGatewayStartedDuringOnboard
function definition, or move the helper function and its supporting
documentation to a separate utility file. Focus on compressing the explanatory
text while preserving the essential logic and functionality of the
stopGatewayStartedDuringOnboard implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 841e023b-2b23-41d5-9288-3a50f900392a
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/gateway-sandbox-reachability.test.tssrc/lib/onboard/gateway-sandbox-reachability.ts
8aa9e98 to
bb23247
Compare
prekshivyas
left a comment
There was a problem hiding this comment.
Correct. `onUnreachable` injects at a single point before the abort, all three host-mode gateway paths share it via `sandboxBridgeProbeOptions`, and `stopDockerDriverGatewayProcess` is synchronous so no async-discard risk. Tests cover genuine-unreachable, successful probe, and soft `probe_unavailable` — exactly the right three cases.
|
✨ Thanks for the proposed fix adding an onUnreachable hook to stop the gateway when the sandbox-bridge probe fails during onboarding. This proposes a way to tear down the OpenShell gateway via stopDockerDriverGatewayProcess at the three host-mode gateway paths so stale listeners don't survive Docker restarts. Related open PRs: Related open issues: |
bb23247 to
ae141c9
Compare
Manual PR Review Advisor resultThis PR Review Advisor analysis was run manually via Run: https://github.com/NVIDIA/NemoClaw/actions/runs/28210492266 Recommendation:
PR Review AdvisorThe cleanup hook is well targeted for the direct standalone gateway paths, but the package-managed gateway path still probes without cleanup and the verifier can mask the original fatal probe error if cleanup throws. Required before merge
Resolve or justify before merge
In-scope improvements
Test follow-ups to resolve or justify
What looks good
|
…#5512) (#5541) ## Summary Follow-up to #5537 addressing the orphan-backup symptom in #5512. When the Docker GPU patch's recreate `docker run` fails **after** the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed *new* container and left the backup orphaned — stranding the sandbox with no live original, and colliding with `*-nemoclaw-gpu-backup-*` on the next retry (as reported in #5512). The supervisor-reconnect failure path already rolls back to the backup; this path didn't. ## Fix Reuse the existing rollback primitive (`rollbackToBackupContainer`) on the early-failure path: remove the failed new container, rename the backup back to the original name, and start it — restoring the pre-patch sandbox instead of leaking a backup container. - Adds `rollbackDockerGpuPatchOnRecreateFailure(refs, deps)` to `docker-gpu-patch-finalize.ts`, which resolves the real `docker start` / `docker rename` defaults (the recreate call path only carries a deps subset, so `dockerStart` would otherwise be unset). - Records `context.rolledBack` for failure diagnostics, matching the reconnect-failure path. - No `onboard.ts` change; all edits are under `src/lib/onboard/`. ## Testing - New composed test in `docker-gpu-patch-rollback.test.ts`: when `dockerRunDetached` fails, the backup is renamed back to the original and started, and is never left as an orphaned container. - `tsc -p tsconfig.src.json` clean; rollback / finalize / sandbox-create suites pass (18/18). ## Relationship to the WSL Docker Desktop chain - #5534 — gateway bind at `[2/8]` - #5536 — gateway cleanup on probe failure - #5537 — skip CDI GPU mode at `[6/8]` (makes the patch succeed on Docker Desktop WSL, so this early-failure path is no longer hit there) - this PR — restore the pre-patch sandbox for any *other* early GPU-recreate failure Refs #5512. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery for Docker GPU patch recreation failures: if the GPU-enabled recreate step fails after the original container is renamed, the system now performs a reliable rollback to the pre-patch sandbox state and cleans up the failed recreate attempt. * **Tests** * Added a rollback-path test for a recreate-phase `docker run --detach` failure, verifying restoration of the original container name, restart behavior, and correct cleanup (including ignoring the failed recreated container). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> --------- Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
4dbc7dc to
f045b25
Compare
Vitest E2E Target Results — ❌ Some jobs failedRun: 28548630243
|
f045b25 to
2276e65
Compare
Vitest E2E Target Results — ✅ All selected jobs passedRun: 28548713900
|
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| sandbox-operations |
81819ad to
e7a80eb
Compare
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| live |
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| gateway-health-honest | |
| openshell-gateway-auth-contract |
Vitest E2E Target Results — ✅ All selected jobs passedRun: 28553374003
|
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28553372297
|
e7a80eb to
2dd54b8
Compare
…VIDIA#5513) When onboard's [2/8] sandbox-bridge reachability probe fails, NemoClaw aborts via process.exit(1) without stopping the OpenShell gateway it started (or reused/adopted) earlier in the same run. The gateway is left running, bound to the loopback address, so the accompanying "restart Docker and re-run" hint is misleading: the stale listener survives a Docker restart and collides with the next attempt. Add an onUnreachable hook to verifySandboxBridgeGatewayReachableOrExit that fires only on a genuine unreachable result (not the soft probe_unavailable skip or a successful probe), and wire it at the three host-mode gateway paths in startDockerDriverGateway (fresh start, reuse, adopt) to tear the gateway down via the existing stopDockerDriverGatewayProcess(). That helper reads the pid file written in every path and only terminates a verified gateway process, so it is a safe no-op otherwise. Follow-up to the bind-address fix in NVIDIA#5534: that change keeps the probe from failing on Docker Desktop WSL in the first place, while this ensures any genuine probe failure no longer orphans the gateway. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
4ea12c6 to
70c54e3
Compare
|
Closing as superseded. The target bug #5513 was fixed and closed by #5754 ( Thanks @abhi-0906 for the fix — the underlying issue is resolved via #5754. Reopen if you think there's a case #5754 doesn't cover. |
…NVIDIA#5512) (NVIDIA#5541) ## Summary Follow-up to NVIDIA#5537 addressing the orphan-backup symptom in NVIDIA#5512. When the Docker GPU patch's recreate `docker run` fails **after** the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed *new* container and left the backup orphaned — stranding the sandbox with no live original, and colliding with `*-nemoclaw-gpu-backup-*` on the next retry (as reported in NVIDIA#5512). The supervisor-reconnect failure path already rolls back to the backup; this path didn't. ## Fix Reuse the existing rollback primitive (`rollbackToBackupContainer`) on the early-failure path: remove the failed new container, rename the backup back to the original name, and start it — restoring the pre-patch sandbox instead of leaking a backup container. - Adds `rollbackDockerGpuPatchOnRecreateFailure(refs, deps)` to `docker-gpu-patch-finalize.ts`, which resolves the real `docker start` / `docker rename` defaults (the recreate call path only carries a deps subset, so `dockerStart` would otherwise be unset). - Records `context.rolledBack` for failure diagnostics, matching the reconnect-failure path. - No `onboard.ts` change; all edits are under `src/lib/onboard/`. ## Testing - New composed test in `docker-gpu-patch-rollback.test.ts`: when `dockerRunDetached` fails, the backup is renamed back to the original and started, and is never left as an orphaned container. - `tsc -p tsconfig.src.json` clean; rollback / finalize / sandbox-create suites pass (18/18). ## Relationship to the WSL Docker Desktop chain - NVIDIA#5534 — gateway bind at `[2/8]` - NVIDIA#5536 — gateway cleanup on probe failure - NVIDIA#5537 — skip CDI GPU mode at `[6/8]` (makes the patch succeed on Docker Desktop WSL, so this early-failure path is no longer hit there) - this PR — restore the pre-patch sandbox for any *other* early GPU-recreate failure Refs NVIDIA#5512. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery for Docker GPU patch recreation failures: if the GPU-enabled recreate step fails after the original container is renamed, the system now performs a reliable rollback to the pre-patch sandbox state and cleans up the failed recreate attempt. * **Tests** * Added a rollback-path test for a recreate-phase `docker run --detach` failure, verifying restoration of the original container name, restart behavior, and correct cleanup (including ignoring the failed recreated container). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> --------- Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
Cleanup-only follow-up to #5754 (the merged Docker Desktop/WSL
host-gatewayretry fix that closed #5513). This PR does not try to re-implement the primary WSL reachability fix; it covers the orphaned-gateway aftermath that can still happen on any genuine sandbox-bridge failure. When onboard's[2/8]sandbox-bridge reachability probe fails, NemoClaw aborts viaprocess.exit(1)without stopping the OpenShell gateway it started (or reused/adopted) earlier in the same run. The gateway is left running, bound to the loopback address — so the accompanying "restart Docker and re-run" hint is misleading: the stale listener survives a Docker restart and collides with the next attempt (the orphaned-gateway symptom called out in #5513).Fix
onUnreachablehook toverifySandboxBridgeGatewayReachableOrExitthat fires only on a genuine unreachable result — not the softprobe_unavailableskip, and not a successful probe.startDockerDriverGateway(fresh start, reuse, adopt) to tear the gateway down via the existingstopDockerDriverGatewayProcess(). That helper reads the pid file written in every path and only terminates a verified gateway process, so it is a safe no-op otherwise.Relationship to #5754 / #5513: #5754 is the merged source fix for the Docker Desktop/WSL reachability race and closed #5513. This PR intentionally stays narrower: it ensures that any genuine probe failure — on any host — no longer orphans the gateway, and makes the "re-run onboard" guidance accurate.
Testing
gateway-sandbox-reachability.test.ts:onUnreachablefires on a genuine unreachable probe, and not on a successful probe or a softprobe_unavailableresult.tsc -p tsconfig.src.jsonclean; reachability + onboard-runtime suites pass (remaining failures are pre-existing Windows-only path / CLI-not-found tests, identical onmain).Refs #5513.
Summary by CodeRabbit
Bug Fixes
Tests
Signed-off-by: Abhimanyu Kumar abhimanyukumar7290@gmail.com