fix(health): add verifyDeployment() and fix false 'Health Offline' on 401 #3128
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -854,6 +854,32 @@ function runCaptureOpenshell( | |
| return runCapture(openshellArgv(args, opts), opts); | ||
| } | ||
|
|
||
| /** | ||
| * Execute a shell command inside a sandbox for post-deployment verification. | ||
| * Returns a structured result with status, stdout, stderr — or null if | ||
| * the sandbox is unreachable. Uses `openshell sandbox exec` with sh -c. | ||
| */ | ||
| function executeSandboxCommandForVerification( | ||
| sandboxName: string, | ||
| script: string, | ||
| ): { status: number; stdout: string; stderr: string } | null { | ||
| try { | ||
| const result = spawnSync( | ||
| getOpenshellBinary(), | ||
| ["sandbox", "exec", "-n", sandboxName, "--", "sh", "-c", script], | ||
| { encoding: "utf-8", timeout: 15000, stdio: ["ignore", "pipe", "pipe"] }, | ||
| ); | ||
| if (result.error) return null; | ||
| return { | ||
| status: result.status ?? 1, | ||
| stdout: (result.stdout || "").trim(), | ||
| stderr: (result.stderr || "").trim(), | ||
| }; | ||
| } catch { | ||
| return null; | ||
| } | ||
|
Comment on lines
+862
to
+880
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: # First, check if the file and function exist at the cited lines
head -n 880 src/lib/onboard.ts | tail -n 50Repository: NVIDIA/NemoClaw Length of output: 1728 🏁 Script executed: # Search for SSH-based sandbox probe implementations
rg "ssh|SSH" src/lib/onboard.ts -A 3 -B 3Repository: NVIDIA/NemoClaw Length of output: 4259 🏁 Script executed: # Look for device-auth E2E references
fd -type f "*.ts" -path "**e2e**" | head -20Repository: NVIDIA/NemoClaw Length of output: 230 🏁 Script executed: # Search for mentions of openshell sandbox exec reliability issues
rg "sandbox exec|false.*probe|000.*probe" . --type ts -A 2 -B 2Repository: NVIDIA/NemoClaw Length of output: 19657 🏁 Script executed: # Look for verifyDeployment function and related verification logic
rg "verifyDeployment|executeSandboxCommandForVerification" src/lib/onboard.ts -A 5 -B 2Repository: NVIDIA/NemoClaw Length of output: 1578 🏁 Script executed: # Check verify-deployment.ts for SSH fallback implementation
head -n 100 src/lib/verify-deployment.tsRepository: NVIDIA/NemoClaw Length of output: 3631 🏁 Script executed: # Search for the executeSandboxCommand callback usage in verify-deployment.ts
rg "executeSandboxCommand" src/lib/verify-deployment.ts -A 5 -B 2Repository: NVIDIA/NemoClaw Length of output: 1710 🏁 Script executed: # Look for SSH fallback in verify-deployment or related modules
rg "recoveredSsh|SSH|ssh" src/lib/verify-deployment.ts -A 3 -B 3Repository: NVIDIA/NemoClaw Length of output: 1248 🏁 Script executed: # Search for dashboard readiness probe implementation
rg "dashboard.*readiness|readiness.*dashboard" src/lib/onboard.ts -A 5 -B 5Repository: NVIDIA/NemoClaw Length of output: 41 🏁 Script executed: # Look for unbounded probes or dashboard readiness loop
rg "dashboardReady|dashboard.*health|18789|DASHBOARD_PORT" src/lib/onboard.ts -A 3 -B 3 | head -100Repository: NVIDIA/NemoClaw Length of output: 4510 🏁 Script executed: # Look for SSH fallback implementation pattern
rg "execSync|spawnSync.*ssh|ssh.*config" src/lib -A 5 -B 2 | head -80Repository: NVIDIA/NemoClaw Length of output: 4782 🏁 Script executed: # Search for the SSH-based probe mentioned in the review
rg "device-auth|SSH.*fallback|ssh-config" src/lib -A 3 -B 3Repository: NVIDIA/NemoClaw Length of output: 5917 🏁 Script executed: # Look at process-recovery.ts for SSH fallback implementation
cat src/lib/actions/sandbox/process-recovery.ts | head -100Repository: NVIDIA/NemoClaw Length of output: 3141 🏁 Script executed: # Check if there's a dashboard readiness loop in onboard.ts
rg "waitFor|ready|health.*dashboard|dashboard.*health" src/lib/onboard.ts -A 4 -B 2 | head -120Repository: NVIDIA/NemoClaw Length of output: 5757 🏁 Script executed: # Search for E2E tests mentioning device-auth
fd -type f "*.ts" | xargs rg "device-auth" 2>/dev/null | head -20Repository: NVIDIA/NemoClaw Length of output: 658 🏁 Script executed: # Check executeSandboxCommand in process-recovery.ts more completely
head -n 150 src/lib/actions/sandbox/process-recovery.ts | tail -n 80Repository: NVIDIA/NemoClaw Length of output: 2628 🏁 Script executed: # Search for how verify-deployment is actually used and if there's a wrapper
rg "verifyDeployment|executeSandboxCommandForVerification" src/lib/onboard.ts -B 5 -A 15 | head -80Repository: NVIDIA/NemoClaw Length of output: 2891 🏁 Script executed: # Look for comment or issue about sandbox exec being unreliable in CI
rg "false.*probe|000.*probe|sandbox exec.*flaky|sandbox exec.*unreliable" . --type ts -A 2 -B 2Repository: NVIDIA/NemoClaw Length of output: 868 🏁 Script executed: # Check if there's a dashboard readiness loop that's unbounded
rg "dashboard.*ready|waitFor.*dashboard|health.*loop" src/lib/onboard.ts -A 8 -B 3 | head -100Repository: NVIDIA/NemoClaw Length of output: 933 Consider using the SSH-based probe pattern that already exists in executeSandboxCommand. The The dashboard readiness loop (lines 5036–5047) is already bounded to 15 attempts, so no change needed there. 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Acknowledged — the SSH fallback is a valid hardening suggestion. However, The E2E test already demonstrated that
Contributor
There was a problem hiding this comment.
If you’d like, I can open a low-priority follow-up issue to track a gated SSH fallback (opt-in via env) in case we ever see production flakiness. Otherwise I’ll consider this thread resolved on your decision. ✏️ Learnings added
🧠 Learnings used |
||
| } | ||
|
|
||
| // URL/string utilities — delegated to src/lib/url-utils.ts | ||
| const { | ||
| compactText, | ||
|
|
@@ -5007,23 +5033,18 @@ async function createSandbox( | |
| // Wait for the branded dashboard to become fully ready (web server live) | ||
| // This prevents port forwards from connecting to a non-existent port | ||
| // or seeing 502/503 errors during initial load. | ||
| // Probes /health endpoint and accepts 200 or 401 (device auth) as "alive". | ||
| // Previously used `curl -sf` which failed on 401, causing false negatives. Fixes #2342. | ||
| console.log(" Waiting for NemoClaw dashboard to become ready..."); | ||
| const openshellBin = getOpenshellBinary(); | ||
| for (let i = 0; i < 15; i++) { | ||
| const readyOutput = runCaptureOpenshell( | ||
| ["sandbox", "exec", "-n", sandboxName, "--", "curl", "-so", "/dev/null", "-w", "%{http_code}", | ||
| "--max-time", "3", `http://localhost:${effectiveDashboardPort}/health`], | ||
| { ignoreError: true }, | ||
| ); | ||
| const readyCode = parseInt((readyOutput || "").trim(), 10) || 0; | ||
| if (readyCode === 200 || readyCode === 401) { | ||
| console.log(" ✓ Dashboard is live"); | ||
| break; | ||
| } | ||
|
|
@@ -9485,6 +9506,41 @@ async function onboard(opts: OnboardOptions = {}): Promise<void> { | |
| `providers/channels enabled to migrate them, then the file is removed automatically.`, | ||
| ); | ||
| } | ||
| // Post-deployment verification — confirm the full delivery chain is | ||
| // operational before telling the user "YOUR AGENT IS LIVE". Fixes #2342. | ||
| const verifyDeploymentModule: typeof import("./verify-deployment") = require("./verify-deployment"); | ||
| const _verifyChatUiUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${DASHBOARD_PORT}`; | ||
| const verifyChain = buildChain({ chatUiUrl: _verifyChatUiUrl, isWsl: isWsl(), wslHostAddress: getWslHostAddress() }); | ||
| const verificationResult = verifyDeploymentModule.verifyDeployment( | ||
| sandboxName, | ||
| verifyChain, | ||
| { | ||
| executeSandboxCommand: (name: string, script: string) => { | ||
| return executeSandboxCommandForVerification(name, script); | ||
| }, | ||
| probeHostPort: (port: number, probePath: string) => { | ||
| const result = runCapture( | ||
| ["curl", "-so", "/dev/null", "-w", "%{http_code}", "--max-time", "3", | ||
| `http://127.0.0.1:${port}${probePath}`], | ||
| { ignoreError: true }, | ||
| ); | ||
| return parseInt(result.trim(), 10) || 0; | ||
| }, | ||
| captureForwardList: () => { | ||
| const output = runCaptureOpenshell(["forward", "list"], { ignoreError: true }); | ||
| return output || null; | ||
| }, | ||
| getMessagingChannels: (_name: string) => selectedMessagingChannels || [], | ||
| providerExistsInGateway: (providerName: string) => providerExistsInGateway(providerName), | ||
| }, | ||
| ); | ||
|
|
||
| // Print verification diagnostics | ||
| const diagLines = verifyDeploymentModule.formatVerificationDiagnostics(verificationResult); | ||
| for (const line of diagLines) { | ||
| console.log(line); | ||
| } | ||
|
Comment on lines
+9509
to
+9542
Contributor
There was a problem hiding this comment. Gate onboarding success on Because Line 9483 already calls Suggested flow- onboardSession.completeSession(toSessionUpdates({ sandboxName, provider, model }));
- completed = true;
- // Onboarding finished successfully. Delete the legacy plaintext
- // credentials.json only when every staged *value* was actually pushed
- // to the gateway in this run.
- const allStagedMigrated =
- stagedLegacyKeys.length > 0 && stagedLegacyKeys.every((k) => migratedLegacyKeys.has(k));
- if (allStagedMigrated) {
- removeLegacyCredentialsFile();
- } else if (stagedLegacyKeys.length > 0) {
- ...
- }
const verifyDeploymentModule: typeof import("./verify-deployment") = require("./verify-deployment");
...
const verificationResult = verifyDeploymentModule.verifyDeployment(
sandboxName,
verifyChain,
{ ... },
);
const diagLines = verifyDeploymentModule.formatVerificationDiagnostics(verificationResult);
for (const line of diagLines) {
console.log(line);
}
+
+ if (!verificationResult.healthy) {
+ process.exit(1);
+ }
+
+ onboardSession.completeSession(toSessionUpdates({ sandboxName, provider, model }));
+ completed = true;
+ const allStagedMigrated =
+ stagedLegacyKeys.length > 0 && stagedLegacyKeys.every((k) => migratedLegacyKeys.has(k));
+ if (allStagedMigrated) {
+ removeLegacyCredentialsFile();
+ } else if (stagedLegacyKeys.length > 0) {
+ ...
+ }
printDashboard(sandboxName, model, provider, nimContainer, agent);🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Intentional design: verification is informational, not a gate. The rationale is that a partially-working deployment (e.g., inference route temporarily unavailable during cold start) is still better than no deployment — users can see the diagnostics and act on them. Blocking onboard completion on transient probe failures would cause frustrating retries for users whose sandboxes are actually functional. The
Contributor
There was a problem hiding this comment.
Optional low-friction tweaks you might consider (no behavior change by default):
If you want, I can open a follow-up issue proposing the opt-in strict flag and the final warning line. ✏️ Learnings added
🧠 Learnings used |
||
|
|
||
| printDashboard(sandboxName, model, provider, nimContainer, agent); | ||
| } finally { | ||
| releaseOnboardLock(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.