fix(onboard): preserve durable journal compatibility - #8080
Conversation
Recognize exact atomic-write leftovers and fail closed on agentless legacy journals. Upgrade legacy finalizations only with durable context. Compare provider-neutral durable receipts by canonical value. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Assert both events before comparing lifecycle order and require exact rollback finalization. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Preserve the exact E1 patch while advancing its parent to 654f8f3. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
📝 WalkthroughWalkthroughThe change adds canonical receipt comparison helpers, validates legacy Docker journal schemas, adds context-aware finalization upgrades, and improves durable write reconciliation. Tests cover reordered receipts, malformed records, filename identity mismatches, acknowledgement loss, event ordering, rollback state, and image validation. ChangesManaged bootstrap journal lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Docker as Docker managed bootstrap
participant Store as DockerManagedBootstrapJournalStore
participant Parser as Finalization parser
participant FileStore as Durable finalization file
Docker->>Store: Load finalization with authenticated context
Store->>Parser: Validate or upgrade record
Parser-->>Store: Return normalized record
Store->>FileStore: Persist upgraded record
FileStore-->>Store: Re-read persisted record
Store-->>Docker: Return validated finalization
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
ericksoa
added
bug-fix
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9096a96 in the TypeScript / code-coverage/cliThe overall coverage in commit 9096a96 in the Show a code coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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. |
![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 (5)
src/lib/onboard/managed-bootstrap/docker-journal.ts (3)
159-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare key sets instead of joined strings.
hasExactKeysjoins sorted keys with commas. A key that contains a comma can make two different key sets produce the same joined string. Downstream field validators still reject such input, so this is not exploitable today. A set comparison removes the ambiguity and reads more directly.♻️ Proposed refactor
function hasExactKeys(record: Readonly<Record<string, unknown>>, expected: readonly string[]) { - return Object.keys(record).sort().join(",") === [...expected].sort().join(","); + const keys = Object.keys(record); + return keys.length === expected.length && expected.every((key) => keys.includes(key)); }🤖 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/onboard/managed-bootstrap/docker-journal.ts` around lines 159 - 161, Update hasExactKeys to compare the sorted record keys and expected keys element-by-element, rather than joining them with commas. Preserve exact key-set matching, including matching lengths, while avoiding ambiguity when keys contain commas.
225-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the legacy normalizers are frozen schema snapshots.
normalizeLegacyDockerManagedBootstrapJournalduplicates most ofnormalizeDockerManagedBootstrapJournal, including the runtime-ID, name-distinctness, provider, rollback, and receipt invariants. The duplication is necessary, because the legacy canonical form must reproduce the exact historical byte sequence and must not change when the current schema evolves.That intent is not stated in the code. A future maintainer may try to deduplicate these branches and break historical record recognition. Add a short comment on this function that marks schema 1 and schema 2 as frozen and forbids sharing logic with the current normalizer.
♻️ Proposed comment
+// Frozen historical schemas. The canonical form produced here must reproduce the +// exact bytes written by the schema 1 and schema 2 writers. Do not share logic with +// normalizeDockerManagedBootstrapJournal, and do not update these branches when the +// current journal schema changes. function normalizeLegacyDockerManagedBootstrapJournal( journal: Readonly<Record<string, unknown>>, schemaVersion: 1 | 2, ): { readonly bootstrapIdentity: string; readonly canonical: string } {🤖 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/onboard/managed-bootstrap/docker-journal.ts` around lines 225 - 377, Add a short comment immediately above normalizeLegacyDockerManagedBootstrapJournal stating that the schema 1 and schema 2 branches are frozen historical snapshots and must not share implementation logic with normalizeDockerManagedBootstrapJournal, because their canonical byte output must remain unchanged.
810-825: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish a missing context from a mismatched context.
upgradeLegacyFinalizationthrows the samemissingAgent()error for two different conditions: no context supplied, and context supplied but not matching the record. The message states "lacks durable agent identity" in both cases. During recovery of a real interrupted bootstrap, an operator cannot tell whether the caller omitted the context or whether the durable record belongs to another transaction.Keep the same error type so callers still branch on it. Add a distinguishing detail for the mismatch case.
🤖 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/onboard/managed-bootstrap/docker-journal.ts` around lines 810 - 825, Update upgradeLegacyFinalization so a supplied context that fails matchesFinalizationContext throws the same DockerManagedBootstrapLegacyRecordRequiresAgentError type with a distinct mismatch detail, while preserving missingAgent() for absent context and retaining the existing matching path.src/lib/onboard/managed-bootstrap/docker.test.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
reverseKeysis defined three times in one directory. This PR adds the same helper to three test files insrc/lib/onboard/managed-bootstrap/. The shared root cause is that no shared test helper module exports it, whiledocker-test-fixture.tsalready serves as the shared test module for this directory.Export
reverseKeysonce fromsrc/lib/onboard/managed-bootstrap/docker-test-fixture.tsand import it in the three test files.
src/lib/onboard/managed-bootstrap/docker.test.ts#L20-L22: delete the local definition and importreverseKeysfrom./docker-test-fixture.src/lib/onboard/managed-bootstrap/docker-journal.test.ts#L29-L31: delete the local definition and importreverseKeysfrom./docker-test-fixture.src/lib/onboard/managed-bootstrap/adapter.test.ts#L47-L49: delete the local definition and importreverseKeysfrom./docker-test-fixture.🤖 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/onboard/managed-bootstrap/docker.test.ts` around lines 20 - 22, Export reverseKeys from src/lib/onboard/managed-bootstrap/docker-test-fixture.ts, then remove each local definition and import the shared helper from ./docker-test-fixture in src/lib/onboard/managed-bootstrap/docker.test.ts#L20-L22, src/lib/onboard/managed-bootstrap/docker-journal.test.ts#L29-L31, and src/lib/onboard/managed-bootstrap/adapter.test.ts#L47-L49.src/lib/onboard/managed-bootstrap/docker.ts (1)
1826-1842: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompute the finalization context once per call.
persistFinalizationcallsfinalizationContext(handle)three times. Each call recomputescreateManagedBootstrapPlanFingerprint(handle.plan), which hashes the canonical JSON of the whole plan.finalizationRecordrepeats the same computation. Hoist the context into one local constant and reuse it.♻️ Proposed refactor
const serialized = serializeDockerManagedBootstrapFinalizationRecord(record); + const context = finalizationContext(handle); try { - deps.journalStore.recordFinalization(record, finalizationContext(handle)); + deps.journalStore.recordFinalization(record, context); } catch (error) { - const recovered = deps.journalStore.loadFinalization( - handle.bootstrapIdentity, - finalizationContext(handle), - ); + const recovered = deps.journalStore.loadFinalization(handle.bootstrapIdentity, context); if ( !recovered || serializeDockerManagedBootstrapFinalizationRecord(recovered) !== serialized ) { throw error; } } - const persisted = deps.journalStore.loadFinalization( - handle.bootstrapIdentity, - finalizationContext(handle), - ); + const persisted = deps.journalStore.loadFinalization(handle.bootstrapIdentity, context);🤖 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/onboard/managed-bootstrap/docker.ts` around lines 1826 - 1842, Update persistFinalization to compute finalizationContext(handle) once in a local constant, then reuse it for journalStore.recordFinalization and both journalStore.loadFinalization calls; also reuse that context when constructing finalizationRecord to avoid repeating createManagedBootstrapPlanFingerprint.
🤖 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/onboard/managed-bootstrap/docker.ts`:
- Around line 1765-1777: Update prepareBootstrapReplacement and the finalization
flow around finalizationContext to reject any snapshot whose image differs from
handle.plan.image before journaling, or derive both the journaled snapshot image
and persistFinalization’s record from the same validated source. Ensure
divergent direct callers cannot create recovery-invalid finalization records.
---
Nitpick comments:
In `@src/lib/onboard/managed-bootstrap/docker-journal.ts`:
- Around line 159-161: Update hasExactKeys to compare the sorted record keys and
expected keys element-by-element, rather than joining them with commas. Preserve
exact key-set matching, including matching lengths, while avoiding ambiguity
when keys contain commas.
- Around line 225-377: Add a short comment immediately above
normalizeLegacyDockerManagedBootstrapJournal stating that the schema 1 and
schema 2 branches are frozen historical snapshots and must not share
implementation logic with normalizeDockerManagedBootstrapJournal, because their
canonical byte output must remain unchanged.
- Around line 810-825: Update upgradeLegacyFinalization so a supplied context
that fails matchesFinalizationContext throws the same
DockerManagedBootstrapLegacyRecordRequiresAgentError type with a distinct
mismatch detail, while preserving missingAgent() for absent context and
retaining the existing matching path.
In `@src/lib/onboard/managed-bootstrap/docker.test.ts`:
- Around line 20-22: Export reverseKeys from
src/lib/onboard/managed-bootstrap/docker-test-fixture.ts, then remove each local
definition and import the shared helper from ./docker-test-fixture in
src/lib/onboard/managed-bootstrap/docker.test.ts#L20-L22,
src/lib/onboard/managed-bootstrap/docker-journal.test.ts#L29-L31, and
src/lib/onboard/managed-bootstrap/adapter.test.ts#L47-L49.
In `@src/lib/onboard/managed-bootstrap/docker.ts`:
- Around line 1826-1842: Update persistFinalization to compute
finalizationContext(handle) once in a local constant, then reuse it for
journalStore.recordFinalization and both journalStore.loadFinalization calls;
also reuse that context when constructing finalizationRecord to avoid repeating
createManagedBootstrapPlanFingerprint.
🪄 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: 708ae29c-0bab-4d10-88b0-ea8b2b59fe8a
📒 Files selected for processing (8)
src/lib/onboard/managed-bootstrap/adapter.test.tssrc/lib/onboard/managed-bootstrap/adapter.tssrc/lib/onboard/managed-bootstrap/docker-journal.test.tssrc/lib/onboard/managed-bootstrap/docker-journal.tssrc/lib/onboard/managed-bootstrap/docker-test-fixture.tssrc/lib/onboard/managed-bootstrap/docker.test.tssrc/lib/onboard/managed-bootstrap/docker.tssrc/lib/onboard/managed-bootstrap/index.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head 9096a96. The durable-journal compatibility path keeps persisted schema/version handling fail-closed, preserves recovery semantics across adapter evolution, and is covered at the adapter, journal, and Docker boundaries. I found no blocking correctness, security, compatibility, or regression defect. The current dependency-resolution CI failures are inherited from the exact ancestor/base and are not attributable to this delta.
## 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/pr8080-source-9096a968. |
2 participants
Summary
Preserve managed-bootstrap recovery across exact atomic-write leftovers and historical durable journal schemas without weakening identity authority. Recovery now compares preparation and completion receipts canonically, upgrades only records backed by immutable evidence, and fails closed instead of inferring a missing agent.
Related Issue
Part of #7744.
Stack Position
9d4dc59c331aca42f4ead7bc8831db61d1deee0e9096a968f13e0c00fdaaa43e8f63e03993f11277db839e079517b66c087f4bdfce12a6be480f3eb111 files changed, 984 insertions(+), 118 deletions(-)Changes
This compatibility boundary is required because interrupted atomic writes and older durable records can remain on disk across upgrades. Ignoring exact temporary artifacts would strand recoverable state, while guessing an agent from mutable names or images would grant unsafe authority. The journal, adapter, and Docker composition tests protect the accepted schemas, exact-name boundary, canonical equality, and fail-closed behavior.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
Verification
9096a968f13e0c00fdaaa43e8f63e03993f11277is signed-DCO and GitHub Verified on exact fix(onboard): preserve shared-state commit authority #8078 base9d4dc59c331aca42f4ead7bc8831db61d1deee0e; the parent-relative slice patch isdb839e079517b66c087f4bdfce12a6be480f3eb1. 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 unavailable — the normal pre-push hook passed CLI typecheck and tag-sync checks; focused exact-head build, typecheck, managed-bootstrap, source-shape, Biome, patch-preservation, and diff checks also passed.9096a968f13e. The added fixture inventory keeps the provider-neutral source-shape tripwire exact.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit