fix(sandbox): allocate the snapshot clone its own dashboard port#6749
Conversation
`snapshot restore --to <dst>` auto-creates the destination by spreading the source's registry entry, which carried the source's `dashboardPort` into the clone. The host forward for that port is owned by the source, so the clone's dashboard URL pointed at the source's dashboard, and the rebuild preflight — correctly refusing a port owned by another sandbox — rejected every rebuild of the clone permanently. Clearing the field instead of allocating would not help: rebuild of a dashboard-managed sandbox hard-fails without a persisted port (rebuild-gpu-opt-out.ts), and no connect/recovery path allocates one for an existing entry outside onboard. So the clone now gets its own port at registration, via the same `findAvailableDashboardPort` + cross-gateway registry occupancy view that onboard's `ensureDashboardForward` uses. Allocation runs before `sandbox create` so port-range exhaustion aborts without leaving an unregistered sandbox behind. Sources without a dashboard port (non-dashboard-managed agents) keep the field unset on the clone. The Hermes port fields flagged in the issue were audited and left inherited: `hermesDashboardInternalPort` is baked into the shared image (inheriting is correct), and changing the Hermes host-port behavior needs a Hermes-specific repro per the repo's Hermes policy. Fixes #6746 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSnapshot restore now allocates destination-owned dashboard ports for cross-sandbox clones, propagates Hermes dashboard settings, and performs allocation before destructive actions. Dashboard reservation locking is integrated into sandbox creation, with related tests and shared test-helper reuse. ChangesSandbox dashboard port lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotRestore
participant Openshell
participant PortAllocator
participant DashboardReservationLock
participant SandboxRegistry
SnapshotRestore->>Openshell: Query forward-list and registry occupancy
SnapshotRestore->>PortAllocator: Allocate destination dashboard port
PortAllocator-->>SnapshotRestore: Return port and dashboard environment
SnapshotRestore->>DashboardReservationLock: Serialize allocation and registration
DashboardReservationLock->>SandboxRegistry: Register clone with destination dashboard fields
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence E2E guidanceAdvisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output. Recommended coverage:
4 optional coverage items · 4 optional selectors · 0 new-test recommendations
This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/lib/actions/sandbox/snapshot.ts (1)
216-227: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPort allocation happens after the destructive delete in the
--forcerebuild path, risking sandbox loss.
allocateCloneDashboardPort(called here insideautoCreateSandboxFromSource) can throw and callsnapshotExit(1)on port-range exhaustion. InrunSnapshotRestoreUnlocked, whentargetExists(i.e.--forcerecreate),deleteSandboxForRestore(targetSandbox)runs beforeautoCreateSandboxFromSourceis invoked (Lines 959-975, unchanged). If the dashboard-port range is exhausted, the destination sandbox is deleted and the recreate then aborts, leaving no sandbox at that name at all.This is exactly the class of bug the existing
#3756 P1fix in this same file guards against for image resolution and gateway compatibility — both are resolved/validated before the destructive delete (Lines 889-896, 949-958). The new dashboard-port check should follow the same pattern: allocate/validate the port beforedeleteSandboxForRestore, then pass the pre-computed port intoautoCreateSandboxFromSourceinstead of recomputing it after the destination is already gone.As per path instructions,
src/lib/{sandbox/**,actions/sandbox/**,state/sandbox.ts}: "Destructive lifecycle operations must validate before mutation, preserve state/backup invariants, and cover failure, recovery, rebuild, and resume behavior without bypassing the public action boundary."🛡️ Proposed fix: hoist allocation before the destructive delete
const compatibility = checkGatewayRouteCompatibility({ gatewayName: sourceGatewayName, sandboxName: targetSandbox, route: lockedSourceEntry, sandboxes: registry.listSandboxes().sandboxes, }); if (!compatibility.ok) { console.error(` Error: ${formatGatewayRouteConflict(compatibility)}`); snapshotExit(1); } + // Validate dashboard-port availability before any destructive action, + // matching the `#3756` P1 pattern used for image/gateway checks above. + const dstDashboardPort = allocateCloneDashboardPort(targetSandbox, lockedSourceEntry); if (targetExists) { if (targetEntry) { verifyRestoreDestinationOnOwnGateway(targetSandbox); } deleteSandboxForRestore(targetSandbox); requireLiveSandboxesOnSandboxGateway( sandboxName, " Failed to re-select source sandbox gateway after deleting destination.", ); } await autoCreateSandboxFromSource( sandboxName, targetSandbox, lockedSourceEntry, lockedFromImage, + dstDashboardPort, );// autoCreateSandboxFromSource signature updated to accept the pre-allocated port async function autoCreateSandboxFromSource( srcName: string, dstName: string, srcEntry: SandboxEntry | { name: string }, fromImage: string, dstDashboardPort: number | null, ): Promise<void> { // remove the internal allocateCloneDashboardPort call here ...Once fixed, consider adding a regression test covering the
--forcerebuild path with port exhaustion to lock in the ordering.🤖 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/snapshot.ts` around lines 216 - 227, Move the allocateCloneDashboardPort call out of autoCreateSandboxFromSource and perform it in runSnapshotRestoreUnlocked before deleteSandboxForRestore when rebuilding an existing target. Pass the precomputed dashboard port into autoCreateSandboxFromSource, updating its signature and removing the internal allocation, so port exhaustion occurs before destructive deletion while normal creation behavior remains unchanged.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/actions/sandbox/snapshot.ts (1)
192-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate port-validity predicate.
The
typeof port !== "number" || !Number.isInteger(port) || port <= 0check here duplicates the identical predicate ingetRegistryOccupiedDashboardPorts(src/lib/onboard/dashboard-port.ts). Consider extracting a sharedisValidDashboardPort(port)helper to avoid the two copies drifting apart.🤖 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/snapshot.ts` around lines 192 - 197, The dashboard-port validity predicate is duplicated between allocateCloneDashboardPort and getRegistryOccupiedDashboardPorts. Extract a shared isValidDashboardPort helper in the dashboard-port utility module, then reuse it in both locations while preserving the existing positive-integer validation behavior.
🤖 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/snapshot.ts`:
- Around line 216-227: Move the allocateCloneDashboardPort call out of
autoCreateSandboxFromSource and perform it in runSnapshotRestoreUnlocked before
deleteSandboxForRestore when rebuilding an existing target. Pass the precomputed
dashboard port into autoCreateSandboxFromSource, updating its signature and
removing the internal allocation, so port exhaustion occurs before destructive
deletion while normal creation behavior remains unchanged.
---
Nitpick comments:
In `@src/lib/actions/sandbox/snapshot.ts`:
- Around line 192-197: The dashboard-port validity predicate is duplicated
between allocateCloneDashboardPort and getRegistryOccupiedDashboardPorts.
Extract a shared isValidDashboardPort helper in the dashboard-port utility
module, then reuse it in both locations while preserving the existing
positive-integer validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2cb0827e-4aba-4d40-8b3e-775dc0216c0c
📒 Files selected for processing (3)
src/lib/actions/sandbox/snapshot-restore-clone-ports.test.tssrc/lib/actions/sandbox/snapshot-restore-test-fixture.tssrc/lib/actions/sandbox/snapshot.ts
|
Thanks — re PRA-1 (establish the destination dashboard forward for the allocated port). The observation is accurate: after this change
So a host forward on the newly-allocated port Q would map host:Q → sandbox:Q, where nothing is listening (the gateway is on the source's baked port P). The only thing that makes the clone's dashboard live is a rebuild, which re-bakes the clone's own image with Q and forwards host:Q → sandbox:Q. That is exactly why this fix allocates Q at registration: it unblocks that rebuild, which is the issue's stated acceptance criterion ("Rebuilding a clone should not be permanently blocked by its own registration"). Why not the alternative the finding suggests (leave the field unset until first forward setup allocates it): for a dashboard-managed agent (OpenClaw, Hermes) rebuild hard-fails without a persisted port — On regression risk: Fully wiring a live dashboard into the restore path would mean triggering a re-bake inside |
…lete The dashboard-port allocation for a snapshot clone ran inside `autoCreateSandboxFromSource`, which in the `--force` recreate path is called after `deleteSandboxForRestore` has already removed the existing destination. On dashboard-port-range exhaustion the allocator calls `snapshotExit(1)`, so the destination was deleted and the recreate then aborted — leaving no sandbox at that name. Hoist the allocation into `runSnapshotRestoreUnlocked` before the destructive delete and pass the pre-computed port into `autoCreateSandboxFromSource`, matching the validate-before-mutation pattern the image-resolution and gateway-route checks already follow (#3756). Adds a regression test asserting that a `--force` restore aborts without deleting the destination when no dashboard port is free. Refs #6746 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
Good catch on the
Added a regression test ( |
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/actions/sandbox/snapshot.ts`:
- Around line 956-960: Update the restore flow around allocateCloneDashboardPort
and withGatewayRouteMutationLock so the selected dashboard port remains reserved
until the cloned sandbox registers it. Use a host-wide reservation spanning
concurrent restores from different gateways, or extend an existing
synchronization scope through registration, and release it on failure.
🪄 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: 4a8f4750-69e1-4bc0-9b06-4e378878f331
📒 Files selected for processing (2)
src/lib/actions/sandbox/snapshot-restore-clone-ports.test.tssrc/lib/actions/sandbox/snapshot.ts
|
Re the Re the cross-gateway allocation race — accurate observation, but it's a pre-existing property of the whole dashboard-port subsystem, not introduced here, so I'm scoping it out of this fix:
Happy to file a follow-up issue for "dashboard-port allocation lacks host-wide reservation across concurrent CLI invocations" covering all allocation sites (onboard, connect, restore) if maintainers want it addressed uniformly. |
wscurran
added
area: sandbox
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 3ac93bf. I agree the Advisor's prescribed dead-forward fix is not viable: the reused image listens on source port P and OpenShell forwards are same-port, so a Q forward cannot become live before rebuild. Issue #6746 permits allocate-at-registration. Two other blockers remain:
-
High — Hermes WebUI clones stay unrebuildable. snapshot.ts:294-307 spreads srcEntry, inherits hermesDashboardPort=P, then writes dashboardPort=Q. rebuild-durable-config.ts:95-109 requires the Hermes public port to equal Q, and rebuild-target-staging.ts:63-77 fails the mismatch. When Hermes WebUI is enabled, set hermesDashboardPort to dstDashboardPort while retaining image-baked hermesDashboardInternalPort, and add a Hermes-specific clone/rebuild acceptance test (or explicitly exclude Hermes pending a repro).
-
High — uphold CodeRabbit's cross-gateway TOCTOU finding. snapshot.ts:194-208,956-977 selects a free host port under a source-gateway-keyed lock, then only registers it after sandbox creation. Two restores on different gateways—or restore racing onboard—can select and durably register the same port. A similar pre-existing onboard gap does not make a new allocation site safe. Introduce a host-global cross-process reservation shared with canonical onboard allocation, held from occupancy selection until the port is durably registered/owned and released on every failure, with consistent lock ordering.
Add deterministic different-gateway/restore-vs-onboard/failure-release tests. After fixes, run exact-head onboard-resume, onboard-repair, state-backup-restore, and upgrade-stale-sandbox plus a clone-specific restore/rebuild live proof.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Maintainer review complete. The dashboard-port reservation and runtime propagation are covered across OpenClaw and Hermes clone paths, and the stale-upgrade E2E fixture now derives its complete Dockerfile.base context. Approved pending required CI/E2E completion.
<!-- markdownlint-disable MD041 --> ## Summary Release-prep documentation for v0.0.82 now summarizes user-facing changes merged since v0.0.81. It also closes stale wording in the stopped-sandbox backup, snapshot-clone, Ollama selection, and custom-policy authoring guidance. ## Changes - Add the `v0.0.82` section to `docs/about/release-notes.mdx` with links to the focused user guides. - Document that snapshot clones receive a destination-owned dashboard port before destructive replacement begins. - Align `backup-all` guidance with eligible stopped Docker-driver sandboxes that NemoClaw starts temporarily. - Describe the running and stopped Ollama menu states without claiming one fixed label. - Document runtime rejection of catch-all hosts in custom policy files. ### Source summary - [#6748](#6748) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, and `docs/reference/commands.mdx`: Summarize non-destructive sandbox `stop` and `start` commands. - [#6723](#6723) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/backup-restore.mdx`, and `docs/reference/commands.mdx`: Record temporary startup and cleanup for eligible stopped-sandbox backups. - [#6749](#6749) -> `docs/about/release-notes.mdx` and `docs/manage-sandboxes/backup-restore.mdx`: Document destination-owned dashboard ports for snapshot clones. - [#6764](#6764) -> `docs/about/release-notes.mdx`: Summarize installer handling of route-only onboarding placeholders. - [#6771](#6771) -> `docs/about/release-notes.mdx`, `docs/inference/set-up-vllm.mdx`, `docs/inference/choose-inference-provider.mdx`, `docs/reference/commands.mdx`, and `docs/reference/platform-support.mdx`: Summarize managed-vLLM storage gates, immutable image digests, and the explicit override boundary. - [#6759](#6759) -> `docs/about/release-notes.mdx`: Record early, actionable OpenShell gateway-port conflict diagnostics. - [#6753](#6753) -> `docs/about/release-notes.mdx` and `docs/inference/set-up-ollama.mdx`: Document truthful running and stopped Ollama menu states. - [#6776](#6776) -> `docs/about/release-notes.mdx`: Summarize proxy-independent loopback readiness checks. - [#6769](#6769) -> `docs/about/release-notes.mdx`: Record compatible endpoint and agent guidance when Chat Completions is unavailable. - [#6730](#6730) -> `docs/about/release-notes.mdx`: Summarize bounded reuse of an eligible successful Chat Completions check. - [#6768](#6768) -> `docs/about/release-notes.mdx`: Record route-reservation repair during resumed onboarding. - [#6742](#6742) -> `docs/about/release-notes.mdx`: Summarize pre-mutation resolution of secret-free sandbox create intent. - [#6721](#6721) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Record bounded cleanup of completed managed Deep Agents headless sessions. - [#6731](#6731) -> `docs/about/release-notes.mdx` and `docs/network-policy/customize-network-policy.mdx`: Document runtime rejection of catch-all custom-policy destinations. - [#6729](#6729) -> `docs/about/release-notes.mdx` and `docs/get-started/prerequisites.mdx`: Record the Node.js 22.19 minimum. - [#6735](#6735) -> `docs/about/release-notes.mdx` and `docs/reference/platform-support.mdx`: Summarize the Ubuntu 26.04 userspace contract without claiming pending host or live validation. - [#6775](#6775) -> `docs/about/release-notes.mdx` and `docs/resources/community-contributions.mdx`: Route independent solutions outside canonical supported-product documentation. - [#6740](#6740) -> `docs/about/release-notes.mdx`: Summarize the semantic dependency-upgrade contributor workflow. - [#6777](#6777) -> `docs/about/release-notes.mdx` and `docs/CONTRIBUTING.md`: Summarize the route-safe documentation-refactor workflow. - [#6741](#6741) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Summarize reviewed npm archive verification and audit enforcement. - [#6739](#6739) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Record the locked offline dependency graph for the managed OpenClaw WeChat runtime. - [#6737](#6737) -> `docs/about/release-notes.mdx`: Record removal of the messaging build plan from final OpenClaw and Hermes image environments. - [#6733](#6733) -> `docs/about/release-notes.mdx`: Summarize cached plugin dependency layers for source and blueprint rebuilds. ### Skipped from docs-skip - None. No commit or changed path in `v0.0.81..origin/main` matched `openclaw-sandbox-permissive.yaml` or `config-show`, and the drafted content contains none of the configured skip terms. ## 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 - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This is a documentation-only release-prep update; behavior is protected by the merged source PRs, and the documentation build validates the changed routes and agent variants. - [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: ## Verification - [x] PR description includes the DCO sign-off declaration 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 — tests are not applicable for this documentation-only change. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not run for this documentation-only change. - [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) — 0 errors; two pre-existing Fern warnings remain. - [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) — no new pages. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated release notes with improvements to sandbox recovery, onboarding, session management, policy validation, storage checks, and system requirements. * Clarified Ollama setup instructions and status labels. * Documented safer snapshot restoration, including dedicated ports and protection against destructive failures. * Expanded `backup-all` coverage to include eligible stopped sandboxes. * Added guidance rejecting broad or catch-all network destinations in custom policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
4 participants
Summary
snapshot restore --to <dst>auto-creates the destination by spreading the source's registry entry, so the clone was registered with the source'sdashboardPort. Because the host forward for that port is owned by the source, the clone'sdashboard-urlpointed at the source's dashboard and every rebuild of the clone was rejected by the rebuild preflight (Dashboard port NNNNN belongs to sandbox '<src>'.). The clone now gets its own dashboard port allocated at registration, sodashboard-urlreports the clone's own port and rebuild proceeds.Related Issue
Fixes #6746
Changes
src/lib/actions/sandbox/snapshot.ts: allocate a destination-owned dashboard port before any destructive--forceaction, then hold a host-wide reservation from selection through durable registration. Onboard and snapshot restore now share the same lock order: sandbox mutation → host dashboard reservation → gateway route mutation.CHAT_UI_URLandNEMOCLAW_DASHBOARD_PORTto the allocated port, so the in-sandbox listener, host forward, and registry all use the same destination port immediately after restore.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run typecheck:clipassed; full sandbox action suite 1,719 passed / 3 skipped with one unrelated timeout passing immediately in isolation; normal pre-commit, commit-msg, and pre-push hooks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests