Skip to content

Commit 465370f

Browse files
authored
fix(recovery): wait after legacy container handoff (#9423)
<!-- markdownlint-disable MD041 --> ## Summary Legacy gateway recovery now repeats the OpenShell readiness check and managed supervisor health check after the final replacement container restart. NemoClaw starts the primary dashboard/API host forward only after both checks pass. Recovery stops if Docker cannot stop or start the replacement container, or if either check fails. ### E2E claim - Root-cause key: `gateway recovery / legacy supervisor relaunch final handoff / post-finalize OpenShell readiness not re-established before primary forward start` - Source workflow: [run `32089878527`](https://github.com/NVIDIA/NemoClaw/actions/runs/32089878527), attempt `1` - Failed job: `Gateway: restores the guard chain after recreation / NVIDIA inference API key` ([job `95570032040`](https://github.com/NVIDIA/NemoClaw/actions/runs/32089878527/job/95570032040)) - Failure signature: `Probe failed: OpenClaw gateway is running in 'e2e-2701', but the primary dashboard/API host forward could not be re-established.` - Claim scope: legacy managed-supervisor recovery after the final replacement container restart. ## Related Issue Follow-up to #9364. Issue #9364 defines the legacy compatibility acceptance criterion. PR #9398 corrected its legacy fixture, which let the E2E test reach this later recovery failure. This change preserves the production supervisor allowlist. ## Changes - Run the shared final Docker handoff function in the legacy `sleep infinity` recovery regression test. - Repeat the pinned managed supervisor health check after Docker restarts the replacement container. - Repeat the OpenShell readiness check before NemoClaw starts the primary dashboard/API host forward. - Stop recovery and report the failed layer when final Docker or readiness verification fails. - Keep prerequisite recovery failures separate from forward failures and redact their diagnostics. - Preserve the production supervisor allowlist. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent security rubric review of commit under review `75c124bb66eadd9706da7fcef13581f93e0773e2` returned PASS with no findings. Live E2E for this commit remains required. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## 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 validate:pr` passed after refreshing `origin/main` 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: `npm exec -- vitest run --project integration test/process-recovery-supervisor-relaunch.test.ts` (35 passed); `npm exec -- vitest run --project cli src/lib/actions/sandbox/connect-flow.test.ts` (40 passed); `npm exec -- vitest run --project e2e-support test/e2e/support/launch-agent-turn.test.ts` (19 passed, 24 platform-skipped); `npm run typecheck:cli` passed; `npm run checks:repository` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this focused recovery change; PR CI will run for latest PR commit `75c124bb66eadd9706da7fcef13581f93e0773e2`. - [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) - [ ] 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) Documentation impact: No public documentation change. Existing recovery documentation already describes the corrected contract. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved sandbox recovery failure reporting with clearer, sanitized details. - Prevented irrelevant manual-forward guidance when recovery fails before forwarding. - Preserved the most direct recovery error when multiple failure layers are reported. - Improved supervisor relaunch recovery with readiness checks, rollback handling, container handoff validation, and cleanup warnings. - Correctly reports container identity changes and managed health failures as recovery failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
1 parent 023cc46 commit 465370f

5 files changed

Lines changed: 632 additions & 116 deletions

File tree

src/lib/actions/sandbox/connect-flow.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,4 +907,83 @@ describe("connectSandbox flow", () => {
907907
expect(logOutput).not.toContain("Probe complete");
908908
expect(exitSpy).toHaveBeenCalledWith(1);
909909
});
910+
911+
it("does not suggest a manual forward when gateway recovery fails before forward start", async () => {
912+
const harness = createConnectHarness({
913+
processCheck: {
914+
checked: true,
915+
wasRunning: false,
916+
recovered: false,
917+
forwardRecovered: false,
918+
recoveryFailureDetail:
919+
"the replacement container identity changed during the final managed supervisor health check",
920+
},
921+
});
922+
923+
await expect(harness.connectSandbox("alpha", { probeOnly: true })).rejects.toThrow(
924+
"process.exit(1)",
925+
);
926+
927+
const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0] ?? "")).join("\n");
928+
expect(errorOutput).toContain("NemoClaw could not recover the OpenClaw gateway in 'alpha'");
929+
expect(errorOutput).toContain(
930+
"the replacement container identity changed during the final managed supervisor health check",
931+
);
932+
expect(errorOutput).not.toContain("gateway is running");
933+
expect(errorOutput).not.toContain("openshell forward start");
934+
expect(harness.runAutoPairSpy).not.toHaveBeenCalled();
935+
expect(exitSpy).toHaveBeenCalledWith(1);
936+
});
937+
938+
it("redacts untrusted gateway recovery details before reporting them", async () => {
939+
const opaqueToken = "opaque-gateway-recovery-token";
940+
const harness = createConnectHarness({
941+
processCheck: {
942+
checked: true,
943+
wasRunning: false,
944+
recovered: false,
945+
forwardRecovered: false,
946+
recoveryFailureDetail: `OpenShell failed\nAuthorization: Bearer ${opaqueToken}\u001b[31m`,
947+
},
948+
});
949+
950+
await expect(harness.connectSandbox("alpha", { probeOnly: true })).rejects.toThrow(
951+
"process.exit(1)",
952+
);
953+
954+
const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0] ?? "")).join("\n");
955+
expect(errorOutput).toContain("Recovery detail:");
956+
expect(errorOutput).not.toContain(opaqueToken);
957+
expect(errorOutput).not.toContain("\u001b");
958+
expect(errorOutput).toMatch(/Recovery detail: .*\.$/mu);
959+
expect(exitSpy).toHaveBeenCalledWith(1);
960+
});
961+
962+
it("keeps a direct recovery failure detail separate from an earlier callback layer", () => {
963+
const harness = createConnectHarness();
964+
harness.checkAndRecoverSpy.mockImplementation(
965+
(
966+
_sandboxName: string,
967+
options?: {
968+
onRecoveryFailureLayer?: (layer: string, detail?: string) => void;
969+
},
970+
) => {
971+
options?.onRecoveryFailureLayer?.("supervisor not running", "SUPERVISOR_NOT_RUNNING");
972+
return {
973+
checked: true,
974+
wasRunning: false,
975+
recovered: false,
976+
forwardRecovered: false,
977+
recoveryFailureDetail:
978+
"the managed supervisor health check for the recreated sandbox did not pass",
979+
};
980+
},
981+
);
982+
983+
expect(harness.restoreSandboxStartupState("alpha")).toMatchObject({
984+
recoveryFailureDetail:
985+
"the managed supervisor health check for the recreated sandbox did not pass",
986+
recoveryFailureLayer: null,
987+
});
988+
});
910989
});

src/lib/actions/sandbox/connect.ts

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,21 @@ function exitOnForwardRecoveryFailure(
260260
process.exit(1);
261261
}
262262

263+
function exitOnGatewayRecoveryFailure(
264+
sandboxName: string,
265+
agentName: string,
266+
detail: string,
267+
): never {
268+
const safeDetail = sanitizeSandboxStartupRecoveryDetail(detail);
269+
const terminalPunctuation = /[.!?]$/u.test(safeDetail) ? "" : ".";
270+
console.error("");
271+
console.error(
272+
` Probe failed: NemoClaw could not recover the ${agentName} gateway in '${sandboxName}'.`,
273+
);
274+
console.error(` Recovery detail: ${safeDetail}${terminalPunctuation}`);
275+
process.exit(1);
276+
}
277+
263278
async function settlePortablePairingOrExit(sandboxName: string): Promise<boolean> {
264279
const result = await settlePortableOpenClawPairing(sandboxName);
265280
if (result.kind === "incomplete") {
@@ -318,6 +333,13 @@ async function runSandboxConnectProbe(sandboxName: string): Promise<void> {
318333
detail,
319334
);
320335
}
336+
if ("recoveryFailureDetail" in processCheck && processCheck.recoveryFailureDetail) {
337+
exitOnGatewayRecoveryFailure(
338+
sandboxName,
339+
agentName,
340+
String(processCheck.recoveryFailureDetail),
341+
);
342+
}
321343
if (processCheck.wasRunning) {
322344
await ensureSandboxInferenceRouteOrExit(sandboxName, agent);
323345
// Defense-in-depth scope-upgrade approval on the probe-only / `recover`
@@ -958,16 +980,22 @@ function maybeEnsureHermesToolGatewayBroker(sb: SandboxEntry | null): void {
958980
}
959981

960982
export function restoreSandboxStartupState(sandboxName: string): SandboxStartupRecoveryResult {
961-
let recoveryFailureDetail: string | null = null;
962-
let recoveryFailureLayer: GatewayRestartFailureLayer | null = null;
983+
let reportedRecoveryFailureDetail: string | null = null;
984+
let reportedRecoveryFailureLayer: GatewayRestartFailureLayer | null = null;
963985
const processCheck = checkAndRecoverSandboxProcesses(sandboxName, {
964986
quiet: true,
965987
onRecoveryFailureLayer: (layer, detail) => {
966-
recoveryFailureLayer = layer;
967-
recoveryFailureDetail = detail ?? null;
988+
reportedRecoveryFailureLayer = layer;
989+
reportedRecoveryFailureDetail = detail ?? null;
968990
},
969991
});
970-
return { ...processCheck, recoveryFailureDetail, recoveryFailureLayer };
992+
const directRecoveryFailureDetail =
993+
"recoveryFailureDetail" in processCheck ? processCheck.recoveryFailureDetail : null;
994+
const recoveryFailureDetail = directRecoveryFailureDetail ?? reportedRecoveryFailureDetail;
995+
const recoveryFailureLayer = directRecoveryFailureDetail
996+
? null
997+
: reportedRecoveryFailureLayer;
998+
return Object.assign(processCheck, { recoveryFailureDetail, recoveryFailureLayer });
971999
}
9721000

9731001
function restoreInteractiveTerminal(): void {

0 commit comments

Comments
 (0)