fix(sandbox): finish starting container recovery - #7879
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: San Dang <sdang@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:
📝 WalkthroughWalkthrough
ChangesSandbox recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant startSandbox
participant DockerRecovery
participant GatewayProbe
startSandbox->>DockerRecovery: recover sandbox
DockerRecovery-->>startSandbox: runtime running, health starting
startSandbox->>GatewayProbe: probe gateway health
GatewayProbe-->>startSandbox: gateway readiness result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 3cd3e78 in the TypeScript / code-coverage/cliThe overall coverage in commit 3cd3e78 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/start.ts (1)
36-43: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftUse a typed recovery state instead of matching diagnostic text.
startSandboxnow uses one exact human-readablerecovery.detailstring as a control-flow discriminator. Any wording change inrecoverDockerDriverSandboxwould silently turn this transient state back into a terminal failure. Add an explicit readiness/status field and keepdetailsolely for diagnostics.As per path instructions, actions should orchestrate while pure decisions belong in domain modules; this avoids a brittle cross-layer string contract.
🤖 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/start.ts` around lines 36 - 43, Replace the exact recovery.detail comparison in isRunningContainerStillStarting with an explicit typed readiness/status field returned by recoverDockerDriverSandbox. Update the recovery result and recoverDockerDriverSandbox to set that field for the running/starting transient state, and have startSandbox use it for control flow while retaining detail only for diagnostics.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 `@src/lib/actions/sandbox/start.ts`:
- Around line 36-43: Replace the exact recovery.detail comparison in
isRunningContainerStillStarting with an explicit typed readiness/status field
returned by recoverDockerDriverSandbox. Update the recovery result and
recoverDockerDriverSandbox to set that field for the running/starting transient
state, and have startSandbox use it for control flow while retaining detail only
for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0d2eba0-f7f5-45c9-899c-6adae5798a43
📒 Files selected for processing (2)
src/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.ts
Security ReviewVerdict: PASS. The change is safe to merge from a security perspective. It carries Docker's parsed runtime and health state as internal structured recovery metadata, and only No findings.
Files reviewed at head
|
Signed-off-by: San Dang <sdang@nvidia.com>
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: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: San Dang <sdang@nvidia.com>
Keep the exact same-sandbox phase-Error refusal retryable when OpenShell appends stderr diagnostics. The read-only probe remains bounded by the managed-health guard and existing deadline. Unrelated sandbox and terminal-phase failures remain definitive. Refs #7227 Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Trusted E2E result for exact PR head
Evidence: trusted E2E run 30604344756,
Conclusion: this is product-path evidence, not runner/cloud flake evidence. Do not merge yet; update the recovery implementation and rerun the trusted E2E on the new exact head. |
|
Trusted E2E follow-up for exact PR head
Evidence: controller 30605590819, trusted E2E child 30606139059, failed recovery job, artifact. The job checkout and The recovery target onboarded successfully, stopped the labeled sandbox container to simulate the reboot boundary, restarted the OpenShell gateway service, and then failed the required user-visible status proof with PR #7879 was closed unmerged by its author at 05:19:23 UTC while the run was active, which caused the remaining two jobs and report job to be cancelled. Therefore this run does not establish that |
3 participants
Summary
Sandbox
startcould stop after Docker reported a restarted container as running with health still starting, before the existing gateway recovery probe could finish the operation.This change treats only that transient state as recoverable and continues to the gateway and host-forward checks.
Changes
startrecovery flow when the restarted container is running and its Docker health check is still starting.double-onboardtarget returned.Type of Change
Quality Gates
startrepairs the agent runtime and host forwards. They do not classify the transient running-and-starting state as terminal.Documentation Writer Review
no-docs-neededstartrecovery contract. The exact-head review found no documentation gap; the added live E2E assertion preserves the observed terminal diagnostic as literal test evidence.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not run; the focused action test covers the two changed recovery branches.npm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit