perf(e2e): move recovery repetition into integration tests - #7942
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe PR adds structured managed-gateway completion parsing, post-restore health validation, PID-plus-start-identity recovery checks, disposition-aware probe output, deterministic repeated recovery tests, and updated Issue 2478 evidence wording. ChangesGateway recovery validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RecoveryTest
participant ConnectProbeOnly
participant ProcessRecovery
participant GatewayController
RecoveryTest->>GatewayController: pause expected process identity
RecoveryTest->>ConnectProbeOnly: run probe-only recovery
ConnectProbeOnly->>ProcessRecovery: check and recover sandbox processes
ProcessRecovery->>GatewayController: execute managed gateway control
GatewayController-->>ProcessRecovery: return structured completion and new PID
ProcessRecovery-->>ConnectProbeOnly: return managedControlCompletion
ConnectProbeOnly-->>RecoveryTest: report recovered or already-running gateway
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b268769 in the TypeScript / code-coverage/cliThe overall coverage in commit b268769 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7942.docs.buildwithfern.com/nemoclaw |
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. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
The deterministic refactor is aligned with #7919, and the focused integration test, semantic E2E phase validation, and docs build passed locally. One live-evidence gap remains: the retained check can pass without exercising the connect-driven recovery branch.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Re-reviewed the fixes through 0d8cf66. Commit 76e761c closes the blocking live-evidence gap by requiring the exact connect-driven recovery result, making SIGKILL failure visible, and removing the delay that allowed PID 1 auto-respawn to satisfy the test. The descriptor-based runtime-env assertion also clears the CodeQL path race, and the documentation now states the remaining evidence boundary precisely.
Focused local verification passed: 12 recovery integration tests, 18 production connect-flow tests, semantic E2E phase coverage for 116 tests across 73 files, docs validation, and diff checks. All required GitHub checks are green, including the live issue-2478 recovery job; CodeRabbit and PR Review Advisor report no actionable findings.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer disposition for |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
…iring Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![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
🧹 Nitpick comments (1)
test/process-recovery-supervisor-relaunch.test.ts (1)
254-262: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer container-ID matching over positional call count for detecting the post-restore probe.
Using
pinnedProbeCount === 3to mark the post-restore-health probe is fragile — it silently depends on exactly how many other pinned probes fire before it (fromconfirmMissingSupervisor, or any caller-side readiness check). The mock already knows the container ID that confirmRestoredManagedHealth targets ("replacement-container-id", verified separately at line 288-293); keying off that instead makes the test resilient to unrelated changes in probe-call ordering/count.♻️ Proposed refactor
- let pinnedProbeCount = 0; - const requestPinnedGatewaySupervisorAction = vi.fn(() => { - pinnedProbeCount += 1; - if (pinnedProbeCount === 1) { - return { status: 1, stdout: "", stderr: "SUPERVISOR_NOT_RUNNING" }; - } - if (pinnedProbeCount === 3) order.push("post-restore-health"); - return acceptedProbe; - }); + const requestPinnedGatewaySupervisorAction = vi.fn( + (_name: string, _action: string, _timeoutMs: number, containerId: string) => { + if (containerId !== "replacement-container-id") { + return { status: 1, stdout: "", stderr: "SUPERVISOR_NOT_RUNNING" }; + } + order.push("post-restore-health"); + return acceptedProbe; + }, + );Since I can't fully trace
checkAndRecoverSandboxProcesses(not part of this review batch) to confirm the exact call count this relies on, please double-check this against that implementation.Also applies to: 315-326
🤖 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 `@test/process-recovery-supervisor-relaunch.test.ts` around lines 254 - 262, Replace the positional pinnedProbeCount === 3 check in requestPinnedGatewaySupervisorAction with matching on the probe request’s container ID, recording post-restore-health when it targets "replacement-container-id". Apply the same container-ID-based detection to the related mock logic at the additional occurrence, while preserving the existing first-probe failure and acceptedProbe 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.
Inline comments:
In `@src/lib/actions/sandbox/supervisor-relaunch.ts`:
- Around line 255-273: Extract the repeated failure-outcome sequence from the
current branch into a small local helper near the surrounding supervisor
relaunch logic, including finalize with supervisorReady false,
stateBackupRemoved handling, completed assignment, and returning the outcome.
Replace this block and the existing equivalent branches around the other failure
paths with the helper, preserving each branch’s stateRestored value and existing
rollback behavior.
---
Nitpick comments:
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Around line 254-262: Replace the positional pinnedProbeCount === 3 check in
requestPinnedGatewaySupervisorAction with matching on the probe request’s
container ID, recording post-restore-health when it targets
"replacement-container-id". Apply the same container-ID-based detection to the
related mock logic at the additional occurrence, while preserving the existing
first-probe failure and acceptedProbe behavior.
🪄 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: 7bbc5964-2892-4254-bee0-5911d32b531d
📒 Files selected for processing (6)
src/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/supervisor-relaunch.test.tssrc/lib/actions/sandbox/supervisor-relaunch.tssrc/lib/state/state-file-restore.tstest/process-recovery-supervisor-relaunch.test.tstest/state-file-restore-command.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/process-recovery-supervisor-relaunch.test.ts (1)
111-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the failure fixture terminal
Zero polling currently prevents the fallback from converting failure into success, but
.mockReturnValue(acceptedProbe)still masks unexpected extra probes. Return the failing probe persistently in this case and reject further calls.🤖 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 `@test/process-recovery-supervisor-relaunch.test.ts` around lines 111 - 135, Update scriptedPinnedGatewayProbes so the failure scenario keeps returning postRestoreProbe instead of falling back to acceptedProbe, and make any unexpected additional probe invocation fail explicitly. Preserve the existing unavailable, accepted, and post-restore-health call sequence while removing the terminal success masking.Source: Path instructions
🤖 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.
Nitpick comments:
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Around line 111-135: Update scriptedPinnedGatewayProbes so the failure
scenario keeps returning postRestoreProbe instead of falling back to
acceptedProbe, and make any unexpected additional probe invocation fail
explicitly. Preserve the existing unavailable, accepted, and post-restore-health
call sequence while removing the terminal success masking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d95c0e2-226d-496e-84d0-c0f58cf804d4
📒 Files selected for processing (1)
test/process-recovery-supervisor-relaunch.test.ts
…iring Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Current-head PRA-1 disposition: no code change. A terminating signal lets PID 1 take the exit-driven auto-respawn path before connect, which can turn this into an already-running false pass. The retained test instead pins the PID and start identity, pauses that exact process, verifies its /proc state is T, requires the authenticated complete-ok disposition and exact recovered CLI result, then proves replacement identity and 15-second stability. This synchronization proves that connect drives recovery; the human P1 thread is addressed and the PR is approved. The partial advisor model failure did not identify a new implementation defect. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: ci
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated changelog entry for `v0.0.100` so the maintainer release plan can verify the pre-tag documentation prerequisite. The entry summarizes the user-facing changes merged since `v0.0.99` and links to the relevant guides. ## Changes - Add `docs/changelog/2026-07-31.mdx` with the exact `## v0.0.100` heading. - Cover restored OpenClaw pairing, transactional replacement, Deep Agents Code, onboarding recovery, lifecycle cleanup, Hermes builds, host provenance, documentation, and trusted E2E evidence. - Distinguish active Docker and Kubernetes runtime-bundle enforcement from the still-inactive managed shared-state transaction foundation. ## Source Coverage The release entry maps the doc-impacting merged PRs in the `v0.0.99..main` release range to `docs/changelog/2026-07-31.mdx`: #8021, #8024, #7973, #8028, #7947, #7788, #7884, #8023, #7969, #8020, #7989, #8000, #7907, #7942, #7567, #8013, #7955, #8017, #8014, #8015, #7629, #7644, #7821, #7971, and #7991. PR #7974 was reviewed after the final rebase and excluded because it changes internal maintainer-skill attribution policy and tests only; it does not change a user-facing product or documentation surface. ## 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 - [x] Existing tests cover changed behavior — justification: the changelog contract test validates the dated entry, version heading, SPDX form, and route constraints. - [ ] Tests not applicable — justification: - [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: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-31.mdx`; exact-head review passed for `6093f44f`; writing rules and documentation style reviewed; `npx vitest run test/changelog-docs.test.ts` passed 6/6; `npm run docs` passed with zero Fern errors and two generic Fern upgrade notices. - Agent: Codex Desktop <!-- docs-review-head-sha: 6093f44 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station host script changed. - 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: `npx vitest run test/changelog-docs.test.ts` passed 6/6 at `6093f44f`. - [ ] 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 a dated prose-only release entry. - [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) — validation passed with zero errors; Fern emitted two generic upgrade notices. - [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) — the changelog entry has the required parser-safe MDX SPDX header; dated changelog entries intentionally do not use page frontmatter. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.100. * Documented improvements to restore pairing, sandbox replacement, onboarding recovery, lifecycle cleanup, runtime handling, build support, host readiness, and end-to-end validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
5 participants
Summary
The existing recovery live E2E now identity-checks and terminates one production gateway, recovers it through
connect --probe-only, then verifies the recovered process identity for 15 seconds, while five repeated preparations run in a deterministic integration test.The removed phases consumed a median 8 minutes 49 seconds across five scheduled runs, so the 13-minute-50-second job is expected to complete in about five minutes.
The recovery result preserves the managed controller's exact
okversusalready-runningdisposition so PID 1 auto-respawn cannot be credited as connect-driven recovery.Legacy recovery proves OpenShell transport before state restoration, then exact-container-restarts the restored gateway and requires an authenticated
okresult plus settle proof before commit. It preserves the OpenClaw configuration's requiredsandbox:sandbox 0660mutable posture.The workflow job, schedule, selector, cleanup, and artifact upload remain unchanged.
Related Issue
Fixes #7919
Changes
issue-2478-crash-loop-recoveryfrom five live crash cycles, two proxy-state transitions, and a 300-second soak to one productionconnect --probe-onlyrecovery.already-runningproduces the ordinary running result rather than the recovered result.inference.local, cleanup, diagnostics, and 15-second process-identity stability assertions.okcompletion, rerun settle proof, and roll back on failure.0660mutable posture while retaining0640for generic state files.Type of Change
Quality Gates
b26876998d16004b72fedafaf087b05b94ad1982, commit-range diff SHA-256ccd137338bd2b510fb27c2701fd62c58d64000d45e3eb3ec4086dc4848d28258. The exact follow-up changes only the live acceptance test and checked-in evidence; it identity-checks the internally resolved PID/start identity before SIGTERM and adds no product runtime, credential, network, permission, dependency, or logging surface.Documentation Writer Review
docs-updated; the prior docs-writer review is retained, and this exact test/docs-only follow-up was reviewed serially at the user's request. The security evidence now states that the retained live lane terminates an identity-checked gateway before production recovery; the full documentation build passed.docs/manage-sandboxes/recover-rebuild-sandboxes.mdx;docs/reference/commands.mdx;docs/security/openclaw-2026.6.10-dependency-review.mdDGX 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 unavailableb26876998d, all 180 focused recovery tests passed with 4 intentional skips;npm run build:cli,npm run validate:pr, the full docs build, and normal pre-push CLI/type/version gates passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not run because this change narrows one live test and has focused integration, source, phase, type, docs, and hook evidence.npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and two pre-existing Fern warnings.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation