fix(onboard): retain durable cleanup recovery - #8083
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
# Conflicts: # src/lib/onboard/managed-bootstrap/docker-journal.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a2ae901 in the TypeScript / code-coverage/cliThe overall coverage in commit a2ae901 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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. |
## Summary Hardens the dormant managed-bootstrap path so create outcomes are explicit, shared-state rollback remains transaction-owned, and recovery receipts replay durably across runtime providers. This consolidates the additive source work from #8077, #8078, #8080, and the already-incorporated behavior from #8083 without registering or activating a managed runtime. ## Related Issue Refs #7744 ## Changes - Return terminal managed-bootstrap outcomes and preserve explicit rollback evidence through Docker sandbox creation. - Keep application environment and shared-state rollback authority inside the managed-startup transaction, including environment-neutral status and rollback probes. - Move receipt comparison into the provider-neutral adapter, validate pre-journal snapshot identity, and retain exact atomic leftovers for durable replay. - Add focused lifecycle, transaction, compatibility, replay, and source-shape coverage while keeping the candidate provider inert. - Preserve the donor heads under `backup/podman-stack/pr8077-source-83e7fe53`, `backup/podman-stack/pr8078-source-9d4dc59c`, `backup/podman-stack/pr8080-source-9096a968`, and `backup/podman-stack/pr8083-source-a2ae901b`. The adapter contract is currently required by managed-bootstrap journal and runtime construction consumers. A Docker-local change is insufficient because later Podman and MXC-style providers must compare the same durable receipts without central runtime switches. The managed-bootstrap adapter, runtime, journal, shared-state, and source-shape tests protect that boundary. ## 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: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: No user-visible provider is registered or activated in this additive slice; the internal managed-bootstrap README documents the architecture change. - [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: Maintainer-authored implementation scope under #7744; the provider remains inert and repository advisors must still clear the exact head before merge. - [ ] 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: Updated `src/lib/onboard/managed-bootstrap/README.md`. The managed-bootstrap provider remains unregistered and unsupported in production, so no user-facing `docs/` change is required. - Agent: Codex Desktop <!-- docs-review-head-sha: b3973ce --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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: Targeted Vitest run covering all 12 changed test files passed 175/175 tests at `b3973cebb50d`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Local `npm test` was attempted and encountered widespread unrelated five-second timeouts across existing installer, package-contract, rebuild, inference, and policy tests; authoritative sharded CI is pending. - [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) --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Commit and rollback failures now surface reliably instead of being silently ignored. * Improved recovery when runtime finalization or supervisor reconnection fails. * Prevented mismatched container images from advancing through setup. * Strengthened rollback protection after a commit becomes durable. * **Compatibility** * Added support for valid legacy transaction manifests while rejecting malformed or incomplete data. * **Reliability** * Repeated commit or rollback requests now produce consistent results, including after acknowledgement failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Superseded by clean consolidated replacement #8225, now merged. The donor head remains preserved under backup/podman-stack/pr8083-source-a2ae901b. |
Summary
Retain process-orphaned managed-bootstrap rollback authority until the owning sandbox service removes the exact held runtime and Docker proves its absence. Recovery now reports every reconciled or retained transaction through a bounded provider-neutral contract, so one unreadable record cannot hide another and a new create blocks only when the retained authority can own its requested name.
Related Issue
Part of #7744.
Stack Position
9096a968f13e0c00fdaaa43e8f63e03993f11277a2ae901b6acc249811f16d28292f3652cac97a443c61a91c9e58b62f5137f1144c39a426b35c416914 files changed, 906 insertions(+), 117 deletions(-)Changes
owner-cleanup-requiredjournal phase reached only after image-owned shared state is restored and the exact replacement is absent.{ receipts, failures }evidence.The additional provider-neutral report is required because throwing on the first unreadable durable record loses evidence for later transactions, while treating every retained record as a global blocker unnecessarily strands unrelated sandbox names. The adapter, journal, Docker recovery, runtime-provider, and sandbox-create tests protect the bounded contract and keep provider-specific routing out of central orchestration.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedsrc/lib/onboard/managed-bootstrap/README.md; verified dormant provider-neutral recovery, owner-cleanup ordering and durability, bounded{ receipts, failures }, same-name and identity-unknown blocking, and unrelated-sandbox warnings against exact-head source and tests. The append-only ancestry merge changes no parent-relative behavior or documentation claim; exact-head focused tests, CLI build and typecheck, Biome lint/format, patch preservation, and diff checks passed.DGX Station Hardware Evidence
Verification
a2ae901b6acc249811f16d28292f3652cac97a44is signed-DCO and GitHub Verified on exact fix(onboard): preserve durable journal compatibility #8080 base9096a968f13e0c00fdaaa43e8f63e03993f11277; the parent-relative slice patch is3c61a91c9e58b62f5137f1144c39a426b35c4169. Focused exact-head qualification passed, and fresh public CI and protected E2E are running.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 --project cli src/lib/onboard/managed-bootstrap src/lib/onboard/sandbox-gpu-create-flow.test.ts src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts --coverage=false: 11 files and 173 tests passed.npm run build:cli,npm run typecheck:cli,npm run lint, Biome format, andgit diff --checkpassed ata2ae901b6acc.npm run test:fastcompleted 15,112 passing tests but the resource-saturated local run also produced 44 unrelated timeout/host-condition failures. Public exact-head CI and protected E2E are required for the authoritative broad result; fresh runs are active at the exact head above.npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 existing non-fatal Fern warnings.Signed-off-by: Aaron Erickson aerickson@nvidia.com