test(e2e): keep proxy env backup in sandbox - #6156
Merged
Merged
Conversation
Signed-off-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis PR modifies an e2e test to preserve ChangesProxy-env backup/restore refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
apurvvkumaria
changed the title
Contributor
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 2 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/live/issue-2478-crash-loop-recovery.test.ts (1)
295-331: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo guaranteed restore if an assertion between backup and restore throws.
moveProxyEnvToBackup(called at Line 474) andrestoreProxyEnvFromBackup(called at Line 486) are separated by the "missing proxy-env" scenario assertions. If any assertion in between throws, the restore never runs and/tmp/nemoclaw-proxy-env.shremains permanently backed up/missing for the rest of the sandbox lifetime, potentially cascading into failures in the restored-scenario and soak phases (or subsequent test runs reusing the sandbox).Consider wrapping the missing-proxy-env scenario in
try/finally(or registering anafterAll/afterEachcleanup) that callsrestoreProxyEnvFromBackupunconditionally.🔧 Example guard
- await moveProxyEnvToBackup(sandbox, instance.sandboxName); - // ... missing-proxy-env assertions ... - await restoreProxyEnvFromBackup(sandbox, instance.sandboxName); + await moveProxyEnvToBackup(sandbox, instance.sandboxName); + try { + // ... missing-proxy-env assertions ... + } finally { + await restoreProxyEnvFromBackup(sandbox, instance.sandboxName); + }🤖 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/e2e/live/issue-2478-crash-loop-recovery.test.ts` around lines 295 - 331, The missing-proxy-env scenario can leave PROXY_ENV_PATH backed up if an assertion fails before restore runs. Update the test around moveProxyEnvToBackup and restoreProxyEnvFromBackup to use a try/finally (or equivalent cleanup hook) so restoreProxyEnvFromBackup is always called after the temporary rename, even when the assertions in the middle throw. Keep the fix localized to the crash-loop recovery test flow that currently calls these helpers.
🧹 Nitpick comments (1)
test/e2e/live/issue-2478-crash-loop-recovery.test.ts (1)
295-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate inline
sandboxparameter type.
moveProxyEnvToBackupandrestoreProxyEnvFromBackupboth declare the identical inline type for thesandboxparameter. Extract a shared type (e.g.,SandboxExecClient) to avoid drift if the exec signature changes.♻️ Suggested extraction
+type SandboxExecClient = { + exec( + name: string, + command: string[], + options?: Record<string, unknown>, + ): Promise<{ exitCode: number | null; stdout: string; stderr: string }>; +}; + -async function moveProxyEnvToBackup( - sandbox: { - exec( - name: string, - command: string[], - options?: Record<string, unknown>, - ): Promise<{ exitCode: number | null; stdout: string; stderr: string }>; - }, - sandboxName: string, -): Promise<void> { +async function moveProxyEnvToBackup( + sandbox: SandboxExecClient, + sandboxName: string, +): Promise<void> {Apply the same substitution to
restoreProxyEnvFromBackup.🤖 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/e2e/live/issue-2478-crash-loop-recovery.test.ts` around lines 295 - 322, The `sandbox` parameter type is duplicated in both `moveProxyEnvToBackup` and `restoreProxyEnvFromBackup`, so extract the shared exec-capable type into a reusable alias (for example, `SandboxExecClient`) and use it in both helper signatures. Update the type definition near these helpers so the `exec` contract is declared once, then replace both inline object types with the shared alias to keep the two functions in sync if the signature changes.
🤖 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.
Outside diff comments:
In `@test/e2e/live/issue-2478-crash-loop-recovery.test.ts`:
- Around line 295-331: The missing-proxy-env scenario can leave PROXY_ENV_PATH
backed up if an assertion fails before restore runs. Update the test around
moveProxyEnvToBackup and restoreProxyEnvFromBackup to use a try/finally (or
equivalent cleanup hook) so restoreProxyEnvFromBackup is always called after the
temporary rename, even when the assertions in the middle throw. Keep the fix
localized to the crash-loop recovery test flow that currently calls these
helpers.
---
Nitpick comments:
In `@test/e2e/live/issue-2478-crash-loop-recovery.test.ts`:
- Around line 295-322: The `sandbox` parameter type is duplicated in both
`moveProxyEnvToBackup` and `restoreProxyEnvFromBackup`, so extract the shared
exec-capable type into a reusable alias (for example, `SandboxExecClient`) and
use it in both helper signatures. Update the type definition near these helpers
so the `exec` contract is declared once, then replace both inline object types
with the shared alias to keep the two functions in sync if the signature
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20187379-ab06-4e8e-8dae-805a2df4f1cb
📒 Files selected for processing (1)
test/e2e/live/issue-2478-crash-loop-recovery.test.ts
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Replace the issue-2478 E2E test's host-side Base64 snapshot transport with an atomic sandbox-local backup and restore. This removes the OpenShell single-argument size dependency while preserving the proxy environment's exact bytes, ownership, and mode across the process-only recovery scenario. ## Related Issue Refs NVIDIA#2478. Related to NVIDIA#6020 and NVIDIA#5595, which currently carry chunk-based workarounds for the same E2E failure. ## Changes - Move `/tmp/nemoclaw-proxy-env.sh` to a hidden backup in the same sandbox before exercising the missing-file recovery path. - Atomically move the backup back to the canonical path before the restored-path restart and soak. - Remove Base64 encoding, captured proxy configuration output, host round-trips, chunking, and argument-size coupling. - Keep the backup in `/tmp` because this scenario restarts only the process tree; it does not recreate the sandbox or wipe `/tmp`. ## 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 - [x] Existing tests cover changed behavior — justification: the exact `issue-2478-crash-loop-recovery` live target exercises the backup, missing-file recovery, restore, five crash cycles, and five-minute stability soak. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: test-harness-only implementation; no command, default, runtime behavior, public API, target ID, or documented E2E interface changed. - [x] 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: human review pending; no waiver requested. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: the broad local `test-cli` hook was not used as evidence after an unrelated host-toolchain failure; required CI remains authoritative. ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [ ] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) ### Test Evidence - Full focused live E2E: 1/1 passed in 709.49s with 5 crash cycles, atomic backup/restore, 20 stable PID samples, 5 successful inference probes, and 0 inference failures. - Focused smoke: 1/1 passed in 270.21s with 1 crash cycle and a 15-second soak. - `npm run typecheck:cli` - `npm run checks` - `npm run source-shape:check` - `npm run test-size:check` - `npx @biomejs/biome check test/e2e/live/issue-2478-crash-loop-recovery.test.ts` - Pre-push CLI TypeScript and package-version checks passed. --- Signed-off-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved end-to-end crash-loop recovery coverage by making the proxy environment file backup and restore flow more reliable. * The test now uses a simpler file move approach to preserve state across recovery steps, reducing flakiness during recovery scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com> Co-authored-by: Apurv Kumaria <36614+apurvvkumaria@users.noreply.github.com>
wscurran
added
area: e2e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the issue-2478 E2E test's host-side Base64 snapshot transport with an atomic sandbox-local backup and restore. This removes the OpenShell single-argument size dependency while preserving the proxy environment's exact bytes, ownership, and mode across the process-only recovery scenario.
Related Issue
Refs #2478. Related to #6020 and #5595, which currently carry chunk-based workarounds for the same E2E failure.
Changes
/tmp/nemoclaw-proxy-env.shto a hidden backup in the same sandbox before exercising the missing-file recovery path./tmpbecause this scenario restarts only the process tree; it does not recreate the sandbox or wipe/tmp.Type of Change
Quality Gates
issue-2478-crash-loop-recoverylive target exercises the backup, missing-file recovery, restore, five crash cycles, and five-minute stability soak.test-clihook was not used as evidence after an unrelated host-toolchain failure; required CI remains authoritative.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Test Evidence
npm run typecheck:clinpm run checksnpm run source-shape:checknpm run test-size:checknpx @biomejs/biome check test/e2e/live/issue-2478-crash-loop-recovery.test.tsSigned-off-by: Apurv Kumaria 36614+apurvvkumaria@users.noreply.github.com
Summary by CodeRabbit