Skip to content

fix(onboard): bind final handoff to replacement id - #10053

Merged
jyaunches merged 3 commits into
mainfrom
fix/v0055-final-handoff
Aug 24, 2026
Merged

fix(onboard): bind final handoff to replacement id#10053
jyaunches merged 3 commits into
mainfrom
fix/v0055-final-handoff

Conversation

@jyaunches

@jyaunches jyaunches commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Legacy v0.0.55 sandbox recovery reached Ready, deliberately stopped the replacement for final handoff, and then waited 900 seconds because lifecycle corroboration required the replacement's copied sandbox-name label to match the restored name. This change binds corroboration to the transaction-owned full container ID and the OpenShell managed-by label, so the replacement can restart without trusting mutable legacy name metadata.

Related Issue

Related to #9844

Changes

  • Query the exact 64-character replacement container ID during the OpenShell Error-row handoff check instead of filtering by the mutable sandbox-name label.
  • Keep the OpenShell managed-by label, exact single-result match, remaining deadline, and fail-closed handling for failed, absent, different, multiple, truncated, or throwing Docker queries.
  • Extend the existing finalization test to model stale legacy sandbox-name metadata while proving the full-ID query and event order.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests 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: The replacement remains bound to the transaction-owned full Docker ID and the OpenShell managed-by label. Failed queries, missing or noncanonical IDs, different IDs, multiple IDs, exceptions, and exhausted deadlines still fail closed; focused negative tests cover those cases.
  • 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; scripts/prepare-dgx-station-host.sh is unchanged.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • 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
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npm exec -- vitest run --project cli src/lib/onboard/docker-gpu-patch-finalize.test.ts src/lib/onboard/docker-gpu-sandbox-create-lifecycle.test.ts src/lib/onboard/docker-gpu-supervisor-reconnect.test.ts passed 65 tests; npm run typecheck:cli, npm run checks:repository, and git diff --check 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 run; this is a focused handoff predicate correction with the owning tests and repository checks passing.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

E2E failure evidence


Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved GPU container replacement validation by confirming the exact replacement container ID.
    • Prevented stale sandbox naming information from causing incorrect lifecycle decisions.
    • Added safeguards to ensure only the transaction-owned managed container is accepted during retirement.
  • Documentation

    • Clarified container validation requirements for Error and Deleting lifecycle states.

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
@jyaunches jyaunches self-assigned this Aug 24, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 46bb3a93-83e1-4cdf-a275-4d69e5aeceb0

📥 Commits

Reviewing files that changed from the base of the PR and between 67aab7e and 486603d.

📒 Files selected for processing (3)
  • src/lib/onboard/docker-gpu-patch-finalize.test.ts
  • src/lib/onboard/docker-gpu-patch-finalize.ts
  • src/lib/onboard/docker-gpu-supervisor-reconnect.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

GPU patch finalization now corroborates replacement containers by exact full ID and OpenShell management label. Sandbox-name metadata no longer affects validation. Tests cover Error and Deleting lifecycle states, including stale name metadata.

Changes

GPU replacement validation

Layer / File(s) Summary
Exact-ID Docker validator
src/lib/onboard/docker-gpu-patch-finalize.ts
Docker queries use --no-trunc, the full replacement ID, and OpenShell management-label filters. Invalid, empty, ambiguous, failed, or exceptional queries return false.
Lifecycle integration and coverage
src/lib/onboard/docker-gpu-patch-finalize.ts, src/lib/onboard/docker-gpu-patch-finalize.test.ts, src/lib/onboard/docker-gpu-supervisor-reconnect.ts
Lifecycle release uses exact-ID corroboration. Tests cover stale sandbox names and Error or Deleting states. Documentation describes the bounded corroboration behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 48660

This localized change binds final handoff verification to the transaction-owned replacement ID, preventing stale sandbox-name metadata from delaying recovery. No actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant LifecycleRelease
  participant isExactOpenShellReplacement
  participant Docker
  LifecycleRelease->>isExactOpenShellReplacement: verify full replacement ID
  isExactOpenShellReplacement->>Docker: query managed containers by exact ID
  Docker-->>isExactOpenShellReplacement: return matching containers
  isExactOpenShellReplacement-->>LifecycleRelease: return corroboration result
Loading

Suggested reviewers: prekshivyas, laitingsheng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: binding final handoff validation to the replacement container ID.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/v0055-final-handoff

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 5ac680c in the fix/v0055-final-hand... branch remains at 96%, unchanged from commit 1763792 in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 5ac680c in the fix/v0055-final-hand... branch remains at 84%, unchanged from commit b6e5936 in the main branch.

Show a line coverage summary of the most impacted files.
File main b6e5936 fix/v0055-final-hand... 5ac680c +/-
src/lib/messagi...atic-outputs.ts 91% 82% -9%
src/lib/messagi...reachability.ts 88% 81% -7%
src/lib/messagi...n-validation.ts 97% 96% -1%
src/lib/messagi...onfig-prompt.ts 94% 93% -1%
src/lib/messagi.../persistence.ts 91% 90% -1%
src/lib/messagi...annel-config.ts 92% 92% 0%
src/lib/messagi...els/metadata.ts 100% 100% 0%
src/lib/messagi...nding-engine.ts 100% 100% 0%
src/lib/messagi...rward-engine.ts 90% 90% 0%
src/lib/onboard...uild-context.ts 73% 74% +1%

Updated August 24, 2026 04:33 UTC

@jyaunches
jyaunches marked this pull request as ready for review August 24, 2026 04:05
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Blocking findings reported

Advisor assessment: Blockers require maintainer review
Next action: Review the blockers below.
Findings: 2 blockers · 0 warnings · 0 suggestions
Synthesis status: Completed · high confidence · 2 blockers · 0 warnings · 0 suggestions

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: onboard-repair, onboard-resume, cloud-onboard
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

Blockers

PRA-1 Blocker — Reserve time for final supervisor verification

  • Location: src/lib/onboard/docker-gpu-sandbox-create.ts:493
  • Category: security
  • Problem: The final handoff gives lifecycle-release observation the complete supervisor reconnect timeout, then verifies the restarted replacement only with time left from that same deadline.
  • Impact: When lifecycle release completes near the deadline, the replacement can restart but receive no supervisor reconnect probe. The code rejects the handoff after the backup container has been removed.
  • Fix: Reserve a bounded part of the final-handoff limit for waitForSupervisor, or use bounded phase limits under one explicit total deadline.
  • Verification: Read commitAfterReady in src/lib/onboard/docker-gpu-sandbox-create.ts and run the focused lifecycle test after adding a fake-clock case.
  • Test coverage: Add a docker-gpu-sandbox-create-lifecycle.test.ts case where lifecycle release consumes its allocation and verify waitForSupervisor receives a positive timeout and a successful reconnect completes the handoff.
  • Evidence: src/lib/onboard/docker-gpu-sandbox-create.ts:493-505 passes supervisorReconnectTimeoutSecs as lifecycleReleaseTimeoutSecs. src/lib/onboard/docker-gpu-sandbox-create.ts:513-531 calls waitForSupervisor only when remainingReconnectTimeoutSecs is greater than zero. src/lib/onboard/docker-gpu-patch-finalize.ts:151-175 waits for lifecycle release before it starts the replacement. src/lib/onboard/docker-gpu-sandbox-create-lifecycle.test.ts:44-108 verifies the ordinary successful path but does not cover lifecycle release consuming the final-handoff budget.

PRA-2 Blocker — Recover the stopped replacement after lifecycle-release failure

  • Location: src/lib/onboard/docker-gpu-patch-finalize.ts:151
  • Category: security
  • Problem: The finalizer stops the replacement and removes the backup before lifecycle-release observation. If that observation fails, it returns without restarting the known replacement or retaining a rollback container.
  • Impact: A transient lifecycle-query failure or timeout can leave the only sandbox container stopped. The failure path reports an uncertain sandbox state but does not provide a deterministic recovery action bound to the replacement container ID.
  • Fix: Retain a recoverable resource until lifecycle release succeeds, or record and surface a deterministic recovery action that operates only on the exact replacement container after the required lifecycle condition is known.
  • Verification: Trace finalizeDockerGpuPatchBackup from backup removal through the lifecycle-release failure outcome and inspect the failure diagnostics for an exact replacement recovery action.
  • Test coverage: Add a composed lifecycle-release failure case that verifies the chosen recovery path restores the transaction-owned replacement without operating on another sandbox.
  • Evidence: src/lib/onboard/docker-gpu-patch-finalize.ts:126-137 stops the replacement and removes the backup before lifecycle observation. src/lib/onboard/docker-gpu-patch-finalize.ts:151-168 returns lifecycleReleaseObserved=false and replacementRestarted=false when lifecycle release is not observed. src/lib/onboard/docker-gpu-sandbox-create.ts:507-531 converts that outcome into a final-handoff failure after the backup has been removed. src/lib/onboard/docker-gpu-patch-finalize.test.ts:350-384 verifies the stopped-replacement outcome but does not verify a recovery action.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@jyaunches
jyaunches enabled auto-merge (squash) August 24, 2026 04:38
@jyaunches
jyaunches disabled auto-merge August 24, 2026 04:45
@jyaunches
jyaunches merged commit 43b4094 into main Aug 24, 2026
69 checks passed
@jyaunches
jyaunches deleted the fix/v0055-final-handoff branch August 24, 2026 04:52
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants