fix(sandbox): honor recovery wait override - #7897
Conversation
|
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 recovery now applies ChangesSandbox recovery readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FinalizationHandler
participant ProcessRecovery
participant ManagedHealth
participant OpenShell
FinalizationHandler->>ProcessRecovery: invoke recreated-sandbox readiness
ProcessRecovery->>ManagedHealth: run conditional beforeProbe
ManagedHealth-->>ProcessRecovery: return health result
ProcessRecovery->>OpenShell: probe OpenShell re-registration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/process-recovery.ts (1)
1321-1321: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not turn an absent health guard into an inconclusive guard.
nullcauses the readiness loop to skip the OpenShell probe and wait again (Lines 756-765). WhenconfirmRelaunchedManagedHealthis unavailable, this callback always returnsnull, so managed relaunch can never execute the readiness probe and eventually times out. OmitbeforeProbewhen the confirmer is absent, and add a regression test for that path.Suggested fix
- beforeProbe: (timeoutMs) => confirmRelaunchedManagedHealth?.(timeoutMs) ?? null, + ...(confirmRelaunchedManagedHealth + ? { beforeProbe: (timeoutMs) => confirmRelaunchedManagedHealth(timeoutMs) } + : {}),🤖 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` at line 1321, Update the managed relaunch setup around beforeProbe so the callback is only provided when confirmRelaunchedManagedHealth is available; do not return null for the absent-confirmation case. Preserve the existing timeout behavior when the confirmer exists, and add a regression test covering readiness proceeding without beforeProbe.
🤖 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/onboard/finalization-deps.ts`:
- Line 13: Keep the timeout behavior in finalization-deps.ts unchanged. Update
docs/inference/configure-inference-timeouts.mdx and docs/reference/commands.mdx
so post-policy OpenShell re-registration is documented under
NEMOCLAW_GATEWAY_RECOVERY_WAIT_SECONDS, with the 120-second default, rather than
NEMOCLAW_SANDBOX_READY_TIMEOUT.
---
Outside diff comments:
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Line 1321: Update the managed relaunch setup around beforeProbe so the
callback is only provided when confirmRelaunchedManagedHealth is available; do
not return null for the absent-confirmation case. Preserve the existing timeout
behavior when the confirmer exists, and add a regression test covering readiness
proceeding without beforeProbe.
🪄 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: 3ff89923-9f58-4083-ba9e-0161cb04487f
📒 Files selected for processing (8)
ci/source-architecture-budget.jsondocs/inference/configure-inference-timeouts.mdxdocs/manage-sandboxes/recover-rebuild-sandboxes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/process-recovery.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/onboard/finalization-deps.tstest/process-recovery-supervisor-relaunch.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Addressed both review findings in 95a4c71. The health guard is now omitted defensively when unavailable, and the timeout docs match the finalization path. |
|
Added the advisor-requested onboarding adapter regression in 40ecdc5. It verifies both timeout env vars still delegate to the recovery helper without an explicit timeout; focused coverage is now 57/57. |
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/onboard/finalization-deps.test.ts`:
- Around line 18-28: Update the test to exercise the exported
finalizationHandlerDeps.waitForSandboxControlPlaneReady through a
module-boundary mock rather than calling a separately created factory instance.
Ensure the mocked recovery helper reads the configured environment variables and
assert the effective timeout resolution, including override precedence, while
still verifying the sandbox identifier is forwarded.
🪄 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: 8834fb41-b8df-4ef8-a01c-d4b6ae8811ff
📒 Files selected for processing (2)
src/lib/onboard/finalization-deps.test.tssrc/lib/onboard/finalization-deps.ts
f40bf42 to
ac797e3
Compare
|
Rebased onto current main. Focused tests and |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
ac797e3 to
1c08080
Compare
|
Rebased onto current main and resolved the recovery-doc conflict. Recovery, finalization, and PR validation all pass. |
wscurran
added
area: sandbox
|
✨ Thanks for the PR. This fixes sandbox recovery wait behavior by honoring the Related open issues: Related open issues: |
|
Superseded by #8182. The replacement carries the same effective recovery-timeout behavior on current The original fork branch could not accept a normal current- |
<!-- markdownlint-disable MD041 --> ## Summary Managed recovery now applies `NEMOCLAW_GATEWAY_RECOVERY_WAIT_SECONDS` consistently to gateway-health and recreated-sandbox readiness waits. A valid operator override takes precedence over the caller's internal budget. Without one, OpenClaw gateway health uses 30 seconds, Hermes gateway health uses 90 seconds, and recreated-sandbox OpenShell registration uses 120 seconds when no other budget is supplied. This preserves Deepak Jain as the commit author while refreshing the effective change from #7897 onto current `main`. The original fork branch could not accept a normal merge commit because the local secret scan evaluated an unrelated test fixture inherited from `main`; no security check was bypassed. ## Related Issue Fixes #7893 Supersedes #7897 with the same effective behavior on current `main`. ## Changes - Give the recovery-wait environment override precedence over internal timeout values. - Remove the unrelated onboarding-readiness timeout from the two managed recreation paths. - Preserve readiness-before-commit ordering, immediate managed-health failure, rollback behavior, and the stopped primary forward after failure. - Cover override precedence and production call shapes with regression tests. - Document the distinct parser contracts and command timing for onboarding and recovery settings. - Lower the measured source fan-out budget after removing the unused dependency. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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] Docs updated for user-facing behavior changes - [ ] Docs 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: Codex Desktop completed a fresh nine-category security review of the nine-file pull-request diff at current branch revision `379fead30`: PASS in all categories with no findings. The current-main merge preserved the reviewed patch and accepted behavior. Recovery-wait input remains finite and nonnegative, the managed-health identity guard still runs before every readiness probe, failures remain fail-closed with rollback before any primary forward starts, no command or credential boundary changed, and no dependency was added. Gitleaks and private-key scans passed; 57 focused CLI tests and 20 focused integration tests passed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: No exception requested; all required checks must pass. ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/inference/configure-inference-timeouts.mdx`, `docs/manage-sandboxes/recover-rebuild-sandboxes.mdx`, and `docs/reference/commands.mdx` against current main, the recovery and onboarding-finalization implementations, focused tests, and the OpenClaw and Hermes manifests. Confirmed the 30-second OpenClaw health, 90-second Hermes health, and 120-second recreated-sandbox OpenShell registration fallbacks; finite nonnegative override precedence with fractional values preserved; immediate definitive managed-health failure; rollback behavior; and the stopped primary forward after failure. Fresh verification passed: 57 CLI tests, 20 integration tests, `npm run typecheck:cli`, `npm run typecheck`, test-title style, diff validation, and generated OpenClaw, Hermes, and Deep Agents variants. `npm run docs` completed with 0 errors and 2 warnings. The worktree remained clean. - Agent: Codex Desktop documentation writer subagent <!-- docs-review-head-sha: 379fead --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested revision: 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 - [x] Targeted behavior tests pass for the current change set — 77 tests passed across process recovery, finalization dependencies, and supervisor relaunch coverage - [x] Applicable broad gate passed — `npm run checks:repository` passed - [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) — passed with 0 errors and 2 existing warnings - [x] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional validation: - `npm run build:cli`: passed - `npm run typecheck:cli`: passed - `git diff --check`: passed - Pre-commit secret scan: passed --- Signed-off-by: Deepak Jain <deepujain@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved sandbox recovery with a dedicated gateway re-registration timeout. * Recovery failures now roll back cleanly while keeping the primary dashboard or API stopped. * Explicit recovery timeout settings override default readiness limits. * Improved timeout validation, including fractional values and fallback handling. * **Documentation** * Added configuration guidance, command examples, and phase-specific recovery timeout defaults. * Clarified sandbox recreation, onboarding readiness, and recovery behavior. * **Tests** * Expanded coverage for timeout selection, delegation, and sandbox readiness behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Deepak Jain <deepujain@gmail.com>
3 participants
Summary
Managed recovery now applies
NEMOCLAW_GATEWAY_RECOVERY_WAIT_SECONDSconsistently to both gateway-health and recreated-sandbox readiness waits.
The two recreate-readiness production paths use the shared 120-second default
instead of bypassing it with the unrelated 180-second onboarding budget.
Related Issue
Fixes #7893
Changes
timeout in both recovery wait functions.
supervisor relaunch readiness checks.
regression tests.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run src/lib/actions/sandbox/process-recovery.test.ts src/lib/onboard/finalization-deps.test.ts test/process-recovery-supervisor-relaunch.test.ts(57 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm testwas bounded after approximately 7 minutes; the focused production-path integration and required equivalent gates passed.npm run docsbuilds without warnings (doc changes only) — result: passed with 0 errors and 2 existing Fern warnings.Additional exact-head evidence:
npm run validate:pr: passed.npm run build:cli: passed.npm run typecheck:cli: passed.npm run lint: passed.npm run docs: passed with 0 errors.git diff --check: passed.recreated-readiness call omits the bypassing timeout, while the direct
readiness regression proves an operator override replaces an explicit
internal budget.
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
NEMOCLAW_GATEWAY_RECOVERY_WAIT_SECONDSto control the recovery wall-clock wait for gateway health and OpenShell re-registration, with override precedence overNEMOCLAW_SANDBOX_READY_TIMEOUT.recover, the system waits 120 seconds by default before starting the primary dashboard/API forwarding.NEMOCLAW_SANDBOX_READY_TIMEOUTnow applies only to post-create readiness; added clearer recovery examples and when to set the gateway recovery wait variable.