fix(stop): tear down the dashboard port-forward on stop (#7227)#7228
Conversation
`nemoclaw <sandbox> stop` stopped the container but never released the
host-side dashboard SSH port-forward it had created, so the `ssh -L
127.0.0.1:<port>` listener stayed alive after stop. As a result `status`
misclassified the cleanly-stopped sandbox via `sandbox_dashboard_port_conflict`
("the dashboard port is held by a foreign listener" — actually nemoclaw's own
leftover forward), and `start` / `recover` intermittently failed their
gateway-readiness probe while contending for the still-held port.
After the container is stopped, release the forward via the shared
`openshell forward stop <port> <sandbox>` teardown (a new
`teardownSandboxDashboardForward` helper that mirrors the forward-stop already
used by the onboard, destroy, and forward-recovery paths). Best-effort: the
teardown runs only after the container actually stopped, and never when the
docker stop failed (the container is still running, so its forward must stay).
Verified end-to-end: after stop no listener remains on the dashboard port,
`status` reports `sandbox_container_stopped` instead of the phantom conflict,
and repeated start/stop cycles restart to Ready deterministically.
Fixes #7227
Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSandbox lifecycle handling now releases host dashboard forwards during stop, distinguishes retryable and definitive recreated-sandbox readiness outcomes, restores forwarding after recovery before verification, and gates policy synchronization on control-plane readiness. Tests and timeout documentation reflect these flows. ChangesSandbox lifecycle forwarding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant stopSandbox
participant teardownSandboxDashboardForward
participant OpenShell
CLI->>stopSandbox: stop sandbox
stopSandbox->>teardownSandboxDashboardForward: teardown dashboard forward
teardownSandboxDashboardForward->>OpenShell: stop forward port sandbox gateway
OpenShell-->>teardownSandboxDashboardForward: cleanup result
stopSandbox-->>CLI: stop result
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…ed) stop Address PR review advisor PRA-1: the all-at-rest branch returned exit 0 before the forward teardown ran, so a repeated stop (or a stop after the forward was orphaned while openshell was unreachable) left the stale dashboard listener alive — the exact conflict this change removes. Invoke the best-effort teardown in the already-stopped success branch too, so a repeated stop always converges on no leftover listener. The no-cleanup behavior when a docker stop fails is unchanged (the container is still running, so its forward must stay). Fixes #7227 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
wscurran
added
area: cli
|
✨ Related open issues: |
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-7228.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Exact-head code/security review — PASS for 8c6f125 against ccf3dd4.
The previous phase-only post-policy wait was not sufficient because OpenShell can report a cached Ready phase before its supervisor session accepts commands. This head preserves the phase gate and additionally requires a bounded openshell sandbox exec --name <sandbox> -- true probe after policy mutation and before finalization. The probe reuses the existing exact retry classifier and NEMOCLAW_SANDBOX_READY_TIMEOUT; unexpected failures remain definitive. Focused tests pin phase-ready → policy mutation → phase-ready → control-plane-ready ordering.
- Secrets and credentials: PASS — no secret source, value, persistence, or logging change.
- Input validation and sanitization: PASS — the validated sandbox name is passed as an argv element; no shell interpolation or new parser is introduced.
- Authentication and authorization: PASS — OpenShell remains the control-plane authority and no identity, approval, or permission check is bypassed.
- Dependencies and supply chain: PASS — no dependency, image, action, or lockfile change.
- Error handling and logging: PASS — the post-policy probe is bounded by the documented readiness budget and fails closed with a distinct re-registration diagnostic.
- Cryptography and data protection: PASS — no cryptographic or protected-data behavior changes.
- Configuration and network exposure: PASS — no new listener, forward, destination, policy permission, or configuration surface is introduced; the existing timeout gains a documented call site.
- Security testing: PASS — focused policy suites passed 71 tests;
npm run test:changedpassed 99 files / 972 tests; CLI typecheck, Biome, hooks, gitleaks, docs, variants, andgit diff --checkpassed. - System security: PASS — finalization cannot proceed until both phase readiness and real OpenShell command execution are restored after policy application; existing managed-health and forward guards remain intact.
Product scope remains established by accepted issues #7227 and #7273. Documentation Writer review passed at this exact head with AGENTS blob 9c9b36d7f99ec7dd8f40af135f5a9e55354330d7. No code, security, or documentation finding remains; exact-head automated and credentialed E2E gates are still required before approval.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Exact-head code/security review — PASS for 51651d0 against ccf3dd4.
This exact-head delta is test-only and addresses PR Review Advisor warning PRA-1. It adds fail-closed coverage proving that a false post-policy OpenShell control-plane probe occurs after policy synchronization, invokes the existing fatal path with exit code 1, and does not return a successful selection. The runtime and documentation reviewed at 8c6f125 are unchanged.
- Secrets and credentials: PASS — no credential behavior or fixture data added.
- Input validation and sanitization: PASS — no runtime input path changed.
- Authentication and authorization: PASS — no runtime permission path changed.
- Dependencies and supply chain: PASS — no dependency, image, action, or lockfile change.
- Error handling and logging: PASS — the negative test now pins the distinct fatal re-registration path and exit code.
- Cryptography and data protection: PASS — no runtime or protected-data change.
- Configuration and network exposure: PASS — no runtime configuration, listener, forward, or policy change.
- Security testing: PASS — exact-head focused tests and
npm run test:changedpass; CLI typecheck, Biome, hooks, gitleaks, and pre-push pass. Runtime-head affected coverage passed 99 files / 972 tests. - System security: PASS — the test prevents regression from fail-closed post-policy command readiness to fail-open behavior.
Product scope remains established by accepted issues #7227 and #7273. Documentation Writer review passed at this exact head with AGENTS blob 9c9b36d7f99ec7dd8f40af135f5a9e55354330d7. No code, security, documentation, or current human-review finding remains; exact-head automated and credentialed E2E gates are still required before approval.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/policy-selection-recorded-tier.test.ts (1)
102-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
process.exitspy locally.Line 103 relies solely on
afterEach; wrap the assertions intry/finallyand callexit.mockRestore()so cleanup is guaranteed by the test that created it.Proposed fix
- await expect(setupPoliciesWithSelection(deps, "alpha", setupOptions)).rejects.toThrow( - "process.exit(1)", - ); - - expect(syncPresetSelection).toHaveBeenCalledWith("alpha", [], []); - expect(waitForSandboxControlPlaneReady).toHaveBeenCalledWith("alpha"); - expect(waitForSandboxControlPlaneReady.mock.invocationCallOrder[0]).toBeGreaterThan( - syncPresetSelection.mock.invocationCallOrder[0], - ); - expect(exit).toHaveBeenCalledWith(1); + try { + await expect(setupPoliciesWithSelection(deps, "alpha", setupOptions)).rejects.toThrow( + "process.exit(1)", + ); + + expect(syncPresetSelection).toHaveBeenCalledWith("alpha", [], []); + expect(waitForSandboxControlPlaneReady).toHaveBeenCalledWith("alpha"); + expect(waitForSandboxControlPlaneReady.mock.invocationCallOrder[0]).toBeGreaterThan( + syncPresetSelection.mock.invocationCallOrder[0], + ); + expect(exit).toHaveBeenCalledWith(1); + } finally { + exit.mockRestore(); + }Based on learnings, locally created spies/mocks must be explicitly restored, especially through
try/finallyfailure paths.🤖 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 `@src/lib/onboard/policy-selection-recorded-tier.test.ts` around lines 102 - 118, Update the test case around setupPoliciesWithSelection and its assertions to wrap the spy-dependent execution in try/finally, calling exit.mockRestore() in the finally block. Preserve the existing rejection and invocation assertions while ensuring the process.exit spy created by this test is restored even when an assertion fails.Source: Learnings
🤖 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 `@src/lib/onboard/policy-selection-recorded-tier.test.ts`:
- Around line 102-118: Update the test case around setupPoliciesWithSelection
and its assertions to wrap the spy-dependent execution in try/finally, calling
exit.mockRestore() in the finally block. Preserve the existing rejection and
invocation assertions while ensuring the process.exit spy created by this test
is restored even when an assertion fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b31b1726-09e4-419e-a58d-1dbbabed1575
📒 Files selected for processing (1)
src/lib/onboard/policy-selection-recorded-tier.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Exact-head security review — PASSReviewed head VerdictNo security blockers or warnings found. The change narrows retries to exact OpenShell transient states, keeps the managed-health guard fail-closed, preserves bounded deadlines, and reconciles forwarding only after authenticated managed recovery. It does not add dependencies, endpoints, credentials, network-policy changes, or new privilege boundaries. FindingsNo findings. Detailed analysis
Files reviewedAll 22 changed files: five docs pages; sandbox forward/process/stop source and tests; onboarding/finalization/policy source and tests; live double-onboard coverage; and integration regressions. Documentation Writer review: PASS on this exact head; This is a review comment, not an approval; required GitHub and live E2E gates are still running. |
|
@coderabbitai review Please review exact head |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/actions/sandbox/process-recovery.ts (2)
1145-1160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve unexpected readiness errors instead of labeling them as identity rejection.
The new catch converts every exception from
confirmRecoveredSandboxGatewayManagedintorelaunchedIdentityRejected, causingbeforeProbe(false)to treat transient or unexpected OpenShell failures as definitive identity failures. This also prevents retryable control-plane errors from reaching the modeled classifier and produces misleading failure details. Catch only a typed identity failure, or preserve unexpected errors for the readiness result path.🤖 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 `@src/lib/actions/sandbox/process-recovery.ts` around lines 1145 - 1160, The catch around confirmRecoveredSandboxGatewayManaged should mark relaunchedIdentityRejected only for the specific typed identity failure. Preserve and propagate unexpected or transient readiness/control-plane errors so beforeProbe(false) can classify them and retain their failure details, rather than converting every exception into a definitive identity rejection.
692-745: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMigrate the production caller to the result-based readiness API.
src/lib/onboard/finalization-deps.tsstill depends onwaitForRecreatedSandboxOpenShellReady, so the boolean wrapper is still a live runtime entrypoint. Either route that caller throughwaitForRecreatedSandboxOpenShellReadyResultor clearly bound the wrapper as compatibility with retirement criteria.🤖 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 `@src/lib/actions/sandbox/process-recovery.ts` around lines 692 - 745, Update the production caller in finalization-deps to use waitForRecreatedSandboxOpenShellReadyResult instead of the boolean waitForRecreatedSandboxOpenShellReady wrapper, and handle its result appropriately while preserving existing readiness behavior. Leave the wrapper only for non-production compatibility callers, or remove it if no callers remain.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.
Outside diff comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 1145-1160: The catch around confirmRecoveredSandboxGatewayManaged
should mark relaunchedIdentityRejected only for the specific typed identity
failure. Preserve and propagate unexpected or transient readiness/control-plane
errors so beforeProbe(false) can classify them and retain their failure details,
rather than converting every exception into a definitive identity rejection.
- Around line 692-745: Update the production caller in finalization-deps to use
waitForRecreatedSandboxOpenShellReadyResult instead of the boolean
waitForRecreatedSandboxOpenShellReady wrapper, and handle its result
appropriately while preserving existing readiness behavior. Leave the wrapper
only for non-production compatibility callers, or remove it if no callers
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fe2bbd42-1a22-494a-ad06-3b5a992f3149
📒 Files selected for processing (5)
docs/reference/commands.mdxsrc/lib/actions/sandbox/process-recovery.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/onboard/machine/handlers/finalization.test.tssrc/lib/onboard/machine/handlers/finalization.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/onboard/machine/handlers/finalization.test.ts
- src/lib/onboard/machine/handlers/finalization.ts
- docs/reference/commands.mdx
- src/lib/actions/sandbox/process-recovery.test.ts
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Sensitive-path security review for exact head 14387c79fa846c5e4b149236da370c48e6ddb17c against base 04e6dfa883071dda9df429c66e73168e1a995cba: PASS.
No security findings.
- Secrets and credentials — PASS: no credential values, secret material, or credential-boundary fallbacks were added.
- Input validation and sanitization — PASS: sandbox names, gateway names, and dashboard ports continue through registry-derived/validated values; OpenShell is invoked with an argument vector, not a shell command. Readiness matching is bounded to exact known control-plane states and a no-op
trueprobe. - Authentication and authorization — PASS: transactional recovery retains its pinned managed-supervisor health guard and does not introduce direct-Docker, SSH, or unauthenticated execution fallback. Post-policy readiness is proved through OpenShell command execution and fails closed.
- Dependencies — PASS: no dependency changes.
- Error handling and logging — PASS: unexpected OpenShell failures remain terminal; bounded retry covers only documented re-registration states or a timeout of the no-op readiness probe. Dashboard-forward cleanup is explicitly best-effort after successful container stop and cannot replace the container lifecycle result.
- Cryptography and data protection — PASS: no cryptographic or sensitive-data persistence changes.
- Configuration security — PASS: no new network destinations, exposed ports, wildcard policy permissions, debug modes, or container privilege changes.
- Security testing — PASS: positive and negative readiness cases cover sandbox-not-ready, supervisor reconnect, relay timeout, unrelated errors, command timeout, managed-health failure/inconclusive states, forward ownership, sibling isolation, post-policy fail-closed behavior, and final forward ordering.
- System security — PASS: stop targets only the selected sandbox's resolved dashboard forward and preserves sibling forwards; recovery remains fail-closed on identity/managed-health drift and does not start the primary forward until OpenShell re-registration succeeds.
Files reviewed: docs/inference/configure-inference-timeouts.mdx, docs/manage-sandboxes/recover-rebuild-sandboxes.mdx, docs/manage-sandboxes/run-sandboxes.mdx, docs/reference/commands.mdx, docs/reference/troubleshooting.mdx, src/lib/actions/sandbox/forward-recovery.ts, src/lib/actions/sandbox/process-recovery.ts, src/lib/actions/sandbox/process-recovery.test.ts, src/lib/actions/sandbox/stop.ts, src/lib/actions/sandbox/stop.test.ts, src/lib/onboard.ts, src/lib/onboard/finalization-deps.ts, src/lib/onboard/machine/final-flow-phases.runtime.test.ts, src/lib/onboard/machine/final-flow-phases.test.ts, src/lib/onboard/machine/handlers/finalization.ts, src/lib/onboard/machine/handlers/finalization.test.ts, src/lib/onboard/policy-selection.ts, src/lib/onboard/policy-selection-recorded-tier.test.ts, test/e2e/live/double-onboard.test.ts, test/onboard-preset-diff.test.ts, test/policy-tiers-onboard.test.ts, test/process-recovery-supervisor-relaunch.test.ts.
Exact-head ordinary CI and full controller-selected E2E remain separate readiness gates.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical pre-tag `## v0.0.95` release entry to `docs/changelog/2026-07-24.mdx`, before the existing v0.0.94 entry. The entry summarizes approved user-visible changes merged since v0.0.94 and excludes internal-only prerequisites. ## Changes - Adds the v0.0.95 summary and detailed bullets for gateway lifecycle, recovery, state transfer, inference compatibility, sandbox security, Discord policy, and E2E evidence. - Links each user-facing theme to the most specific published documentation. - Records the release entry in the shared native changelog used by the OpenClaw, Hermes, and Deep Agents guides. Source summary: - [#7246](#7246), [#7228](#7228), [#7267](#7267), [#7489](#7489), [#7509](#7509), [#7351](#7351), and [#7290](#7290) -> `docs/changelog/2026-07-24.mdx`: Gateway authority, forward teardown and retry, managed recovery, Hermes restart recovery, scoped uninstall, and orphan-aware backup behavior. - [#7344](#7344) and [#7416](#7416) -> `docs/changelog/2026-07-24.mdx`: Atomic SQLite restore and host download verification. - [#7476](#7476), [#7347](#7347), [#7281](#7281), [#7485](#7485), [#7491](#7491), and [#7422](#7422) -> `docs/changelog/2026-07-24.mdx`: Windows Ollama reuse, CDI fallback, bounded OpenRouter connection setup, Nemotron-3 request compatibility, and managed Deep Agents retry and provider-error behavior. - [#6884](#6884), [#7481](#7481), [#6878](#6878), [#7467](#7467), [#7502](#7502), [#7503](#7503), [#7504](#7504), and [#7486](#7486) -> `docs/changelog/2026-07-24.mdx`: Trusted base-image overrides, local rebuild images, runtime validation, config preservation, reviewed package updates, and fewer final-image payload layers. - [#7303](#7303) -> `docs/changelog/2026-07-24.mdx`: Scoped Discord application-command management. - [#7488](#7488), [#7465](#7465), [#7497](#7497), [#7464](#7464), [#7501](#7501), [#7494](#7494), and [#7493](#7493) -> `docs/changelog/2026-07-24.mdx`: Selected-test risk signals, retry cleanup, full root-image validation, direct-main Hermes setup, executed PR-gate evidence, nightly history, and runner wait reporting. - [#7447](#7447) is an internal pinned-runtime prerequisite and is intentionally excluded from canonical supported-integration documentation. - [#7370](#7370) adds maintainer-only advisory reconciliation tooling and does not change supported user behavior. - [#7495](#7495) updates existing documentation and does not add a new v0.0.95 behavior claim. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog structure, heading uniqueness, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-24.mdx`; writing rules, documentation style, factual release meaning, and published links reviewed at exact head `58b02f2bf`. - Agent: Codex documentation writer reviewer <!-- docs-review-head-sha: 58b02f2 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6 tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build passed with 0 errors and 2 Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new v0.0.95 changelog entry above v0.0.94. * Documented improved externally supervised gateway lifecycle ownership. * Improved snapshot restore reliability and SQLite state handling. * Tightened CLI `backup-all` behavior and host artifact verification. * Updated Windows onboarding guidance (including Ollama service reuse and CDI directory fallback). * Noted inference compatibility fixes, deeper agent failure classification, stricter base-image validation, updated Discord bot command permissions, and refined E2E release automation evidence handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
7 participants
Summary
nemoclaw <sandbox> stopreturned success but never tore down the host-side dashboard SSH port-forward it created, leaving anssh -L 127.0.0.1:<port>listener alive.statusthen misreported the cleanly-stopped sandbox as a foreignsandbox_dashboard_port_conflict, andstart/recoverintermittently failed while contending for the still-held port. This PR releases the forward on stop. Exact-head livedouble-onboardalso exposed the known recreated-sandbox readiness defect from #7273, so this branch includes the reviewed tri-state readiness fix from #7291 and keeps the restart regression enabled.Closes #7227.
Closes #7273.
Reproduction
On our Ubuntu 24.04 x86_64 test host (no GPU), ollama-local sandbox. The reporter is on DGX Spark aarch64; the root cause (host-side port-forward not torn down on stop) is platform-independent.
Phase: Ready.openshell forward list/ss -ltnp | grep <dashboard-port>— a nemoclaw-spawnedssh -Lforward holds the port.nemoclaw <sb> stop(exit 0; container Exited 137).nemoclaw <sb> status.Observed on
main(before fix)Observed on
fix/...(after fix)Analysis
stopSandbox(src/lib/actions/sandbox/stop.ts) stops the labeled container(s) and returns, but it never releases the host-side dashboard port-forward. That forward is a nemoclaw-managedopenshell forward(ssh -L 127.0.0.1:<port>); onboarding, destroy, and forward-recovery all tear it down withopenshell forward stop <port> <sandbox>, butstopdid not — so the listener outlives the container.getSandboxGatewayStateForStatus's classifier (gateway-failure-classifier.ts,sandbox_dashboard_port_conflict) then sees a stopped container plus a live listener on its dashboard port and reports a "foreign listener" conflict — the listener is nemoclaw's own leftover forward.start/recoverre-establish the forward and probe gateway readiness while the just-stopped forward is still bound, which is the intermittent failure.The exact-head E2E gate also exposed a finalization ordering gap:
cloud-onboardcreated the sandbox and launched OpenClaw, but post-policy process recovery invalidated the initial dashboard forward before final deployment verification. Finalization re-established forwards only for non-default agents and did so before recovery, so the delivered OpenClaw dashboard on127.0.0.1:18789returned connection refused. Existing unit review did not model recovery invalidating the forward; the new regression makes that state transition explicit.Fix
After the container is stopped, release the forward via a new
teardownSandboxDashboardForwardhelper (forward-recovery.ts) that runsopenshell forward stop <port> <sandbox>— reusing the exact teardown already used by onboard/destroy/forward-recovery, with the port fromresolveSandboxDashboardPort. The helper scopes cleanup to the registry-owned gateway, applies the standard 30-second OpenShell operation timeout, and waits up to 5 seconds for the supported OpenShell 0.0.85 listener to release the host port. It is best-effort (a stop must still free container resources when OpenShell is unreachable) and is wired intostopSandboxafter a successful Docker stop and for an already-stopped sandbox. It does not run when Docker stop fails because the container is still running and needs its forward.Tests (
stop.test.ts) pin the contract: the forward is torn down only after the container stops; it is not released when Docker stop fails; and a repeated stop releases a leftover forward for an already-at-rest sandbox. They also verify non-default gateway targeting, the operation timeout, delayed listener-release polling, and that a nonzero cleanup status skips the port probe.Finalization now reconciles the default OpenClaw or declared-agent forward after post-policy process recovery and before warm-up or deployment verification. The regression simulates recovery invalidating an initially-live forward and requires finalization to restore it before reporting the deployment healthy.
Changes
src/lib/actions/sandbox/process-recovery.tsand focused tests: preserve the reviewed fix: preserve sandbox recovery readiness guard #7291 tri-state readiness contract and retry only the exact OpenShell supervisor re-registration states, including relay-targetENOENT/ECONNREFUSED, and no-op readiness-probe timeout exposed by exact-headdouble-onboard.test/e2e/live/double-onboard.test.ts: retain the stop/start regression so live validation proves the recovery dependency instead of bypassing it.src/lib/actions/sandbox/forward-recovery.ts: newteardownSandboxDashboardForwardhelper.src/lib/actions/sandbox/stop.ts: release the dashboard forward after a successful stop.src/lib/actions/sandbox/stop.test.ts: pin the teardown contract and its boundaries.src/lib/onboard.tsand finalization tests: restore dashboard forwarding after post-policy process recovery, including the default OpenClaw path.src/lib/onboard/policy-selection.tsand focused tests: after applying policy presets, require both sandbox phase readiness and a real OpenShell command probe before finalization.NEMOCLAW_SANDBOX_READY_TIMEOUTand distinguish it from post-create cleanup.docs/manage-sandboxes/run-sandboxes.mdx: document dashboard-forward cleanup during stop.docs/reference/commands.mdx: document cleanup ordering, failure handling, and idempotency.Type of Change
Verification
mainpasses: 107 files / 1,060 tests; latest changed-path lane passes: 61 files / 582 tests.git diff --check, normal pre-commit/commit-msg hooks, pre-push hooks, and gitleaks pass.npm run docspasses with 0 errors and 2 existing warnings; agent variants are synchronized and valid.Documentation Writer Review
no-docs-neededba010ecc6428637282320f2a4de0580e36a06070against based87acc16b04c065ddc26c294a6c3af6795a43d7b. The reviewer confirmed the latest change only expands the existing bounded recreated-sandbox readiness retry for OpenShell 0.0.85 relay-target startup; it adds no command, output, configuration, deadline, workflow, or operator action. Existing recovery docs already cover bounded OpenShell re-registration and fail-closed behavior. Focused recovery tests pass 37/37;npm run test:changedpasses 582/582; CLI build/typecheck, Biome, and normal hooks pass.Codex DesktopAI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit