fix(onboard): reuse registered dashboard forward - #11097
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
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. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 7fbe756 in the TypeScript / code-coverage/cliThe overall line coverage in commit 7fbe756 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
|
PR Review Advisor finished for commit |
|
The latest #11074 failure classification confirms one shared onboarding boundary, but the current lsof/procfs ownership scanner conflicts with the accepted #10691 ForwardTcp model, which deliberately removed NemoClaw-owned PID/process inspection. I am superseding that mechanism in this draft with the existing Ready-sandbox identity/registry authority and deferred deployment verification, while preserving the strict occupied-port path for every unqualified listener. No competing PR will be opened. |
|
Follow-up after checking OpenShell 0.0.106 and the accepted #10691 contract: registry identity plus post-verification is not sufficient to supersede the listener scanner. This leaves no implementation that satisfies all current constraints simultaneously: #10691 forbids persisted process state and ForwardTcp process inspection/adoption; #11074 requires preserving the healthy service while rejecting every foreign listener; and this issue forbids a new registry, process heuristic, or lifecycle workaround. I have not changed or pushed this draft. @ericksoa, please choose one scoped authority: an attested OpenShell service lookup, an explicit amendment permitting bounded listener ownership inspection, or an accepted stop/start reconciliation contract. My earlier comment's registry-plus-post-verify direction is withdrawn because it would weaken fail-closed behavior. |
|
Superseded by the smaller replacement in #11128, which keeps the investigation and follow-up review on the reduced implementation. |
Outcome
Repeated onboarding of a Ready sandbox now reuses its registered, bound primary dashboard forward after proving both the sandbox identity and the live listener's ownership. Before this change, step 7 tried to allocate the persisted port again and failed with
Registered dashboard port 18789 is already occupied; it cannot be reallocated or adopted.Reason
Direct
ForwardTcpservices are not returned by the legacy forward-list compatibility path. During Ready-sandbox reuse, the dashboard reconciler therefore saw the existing listener but not its legacy owner and treated the registered port as a conflicting new allocation. Reachability alone is not ownership evidence, so reuse must retain the exact Ready-sandbox identity and identify the exact direct-forward process at the final ownership boundary.Related issues
Fixes #11074
Changes
lsofwhen available and a bounded Linux procfs fallback otherwise; ambiguous, changing, foreign, or unobservable ownership remains an error.lsofhost dependency or duplicate listener parser.Verification
npm run validate:pr— passed on commit7fbe756a2against canonicalmain685eab2f5ac0fbd18c07c44a39143abceac4ca03, including formatting, lint, repository checks, secret scan, source-shape budget, growth guardrails, commit lint, and CLI TypeScript.npm run build:cli— passed.npm run test:changed— passed for the final follow-up; 45 growth-guardrail tests and 23 affected CLI/integration tests passed. The preceding broader evidence revision passed 45 growth-guardrail tests and 525 affected CLI/plugin/E2E-support tests.npx tsx scripts/checks/e2e-mock-parity.mts --base origin/main --head HEAD— passed with both live reuse targets mapped to their fast orchestration and ownership tests.npm run test:e2e-phases:check— passed; 132 semantic tests across 88 live E2E files.NEMOCLAW_RUN_LIVE_E2E=1 npx vitest list --project e2e-live test/e2e/live/double-onboard.test.ts test/e2e/live/onboard-resume.test.ts— both extended targets register successfully.7fbe756a2, including CLI shards, static checks, package contracts, security scans, managed-image checks, and rootless-Podman qualification.7fbe756a2.Review notes
Twelve completed Advisor cycles identified progressively deeper gaps: reachability without ownership, a missing identity fence and Linux fallback, incomplete production handoff, bounded enumeration and PID-reuse concerns, direct constructor evidence, actionable recovery diagnostics, lost reuse intent in the agent-aware helper and machine-handler paths, an undeclared live-test dependency, missing real-process negative evidence, unsafe reliance on a stale skipped-step receipt without a live identity lease, ineffective alternate-port guidance for a resumed sandbox, missing helper-to-live-target ownership routing, incomplete route-repair resume evidence, unintended full-suite E2E fan-out from placing a focused helper under the shared-fixture boundary, a dashboard-boundary authority split that allowed the preservation Boolean without its identity lease, and a duplicate Linux listener scanner in live evidence. The first exact-head CI cycle also exposed eager identity observation outside the selected dashboard-reuse path. This revision addresses those findings and retains strict failure behavior outside proven reuse. The twelfth cycle found no further change-required issue.
Signed-off-by: San Dang sdang@nvidia.com