Skip to content

fix(computer): retain checkpoint restore provenance - #706

Closed
AprilNEA wants to merge 2 commits into
masterfrom
xuan/core-144
Closed

AprilNEA wants to merge 2 commits into
masterfrom
xuan/core-144

Conversation

@AprilNEA

@AprilNEA AprilNEA commented Aug 26, 2026 •

Copy link
Copy Markdown
Member

Summary

  • retain kernel and rootfs provenance in durable sandbox records after runtime inputs are redacted
  • rebuild the effective restore spec from snapshot provenance so adopted and restored computers can produce restorable checkpoints
  • reject legacy redacted records before publishing an invalid checkpoint
  • cover adopt → checkpoint → restore → exec, pause/resume after adoption, chained restores, and legacy records

Verification

  • cargo fmt --all --check
  • cargo test -p arcbox-computer-runtime (268 passed, 1 ignored)
  • cargo clippy -p arcbox-computer-runtime --all-targets --no-deps -- -D warnings -A clippy::result_large_err -A clippy::unused_async_trait_impl -A clippy::map_unwrap_or -A clippy::chunks_exact_to_as_chunks

Full unfiltered clippy is currently blocked by pre-existing Rust 1.98 lints addressed separately in #704.

Linear: CORE-144


Note

Medium Risk
Touches durable record shape and restore/checkpoint contracts on the snapshot lifecycle path; behavior change is intentional but affects adoption, pause/resume, and chained restores.

Overview
Fixes broken checkpoints after adoption or restore by keeping kernel/rootfs (and geometry on restore) in the durable sandbox record and snapshot lineage, so later captures write restorable metadata instead of empty paths.

Durable records no longer clear kernel and rootfs when redacting consumed boot inputs at Ready; workload secrets (cmd, env, etc.) are still stripped. Restore merges snapshot provenance into the journaled restore_spec before provisioning. Checkpoint fails fast with a failed precondition when the in-memory spec has no kernel or rootfs, blocking legacy v1 records that were redacted in place.

Integration coverage adds adopt → checkpoint → restore, chained restore → checkpoint → restore, legacy-record refusal, and updates the adopted pause/resume test to expect a successful resume.

Reviewed by Cursor Bugbot for commit 83b3b86. Bugbot is set up for automated code reviews on this repo. Configure here.

Co-authored-by: Xuan Zhang <xuan@arcbox.dev>
Copilot AI lite review requested due to automatic review settings August 26, 2026 04:31
@linear-code

linear-code Bot commented Aug 26, 2026

Copy link
Copy Markdown

CORE-144

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 83b3b86a87

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread computer/arcbox-computer-runtime/src/sandbox/record/phase.rs
@greptile-apps

greptile-apps Bot commented Aug 26, 2026 •

Copy link
Copy Markdown

Greptile Summary

The PR preserves kernel and rootfs provenance in durable sandbox records and reconstructs restore specifications from snapshot metadata.

  • Retains boot provenance after runtime-only inputs are redacted.
  • Rejects checkpoints when required kernel or rootfs provenance is unavailable.
  • Adds adoption, pause/resume, chained-restore, and legacy-record coverage.
  • Includes unrelated lint-compatible refactors in supporting crates.

Confidence Score: 4/5

The PR is not yet safe to merge because legacy snapshots without geometry still produce restored records and subsequent checkpoints with zero CPU and memory geometry.

Legacy snapshot metadata validly omits geometry, but the restore path leaves request defaults unchanged in that case, while later checkpoint capture treats those zero values as authoritative geometry.

Files Needing Attention: computer/arcbox-computer-runtime/src/sandbox/checkpoint.rs

Important Files Changed

Filename Overview
computer/arcbox-computer-runtime/src/sandbox/record/phase.rs Preserves kernel and rootfs paths while continuing to redact consumed workload inputs.
computer/arcbox-computer-runtime/src/sandbox/checkpoint.rs Reconstructs the effective restore specification from available snapshot provenance.
computer/arcbox-computer-runtime/src/lifecycle/tasks/checkpoint.rs Refuses checkpoint publication when kernel or rootfs provenance is missing.
computer/arcbox-computer-runtime/tests/manager_over_fakes.rs Adds lifecycle coverage for adoption, chained restores, legacy records, and adopted pause/resume.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Durable sandbox record] --> B[Adopt sandbox]
    S[Snapshot metadata] --> C[Build effective restore spec]
    C --> D[Restore sandbox]
    B --> E[Checkpoint]
    D --> E
    E --> F[Publish snapshot provenance]
    F --> C
Loading

Reviews (2): Last reviewed commit: "fix(ci): satisfy rust 1.98 clippy in tes..." | Re-trigger Greptile

Comment thread computer/arcbox-computer-runtime/src/sandbox/checkpoint.rs
@pullfrog

pullfrog Bot commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixed the red check suite on PR #706 and pushed commit fd4f1c3b to xuan/core-144.

Diagnosis: The failure (Unit tests → Clippy) was not introduced by this PR — it was pre-existing Rust 1.98 clippy lints in the test-vm-linux.yml clippy scope (-p arcbox-computer-runtime ... -p arcbox-snapshot -- -D warnings), the same toolchain breakage tracked in the author's own note and addressed in the open PR #704. The failing lint (clippy::drain_collect in arcbox-snapshot/src/template_catalog.rs:333) sits in a file this PR never touches.

Fix: I ran the full enumeration in CI's exact scope (without -D warnings) and fixed all 9 firing sites across 4 crates, mirroring #704's pending changes exactly (behavior-preserving):

  • arcbox-snapshot/template_catalog.rs — drain(..).collect() → std::mem::take
  • arcbox-vm-agent/main.rs — chunks_exact_mut(4) → as_chunks_mut::<4>()
  • arcbox-fc-driver/discover.rs — .ok().is_some_and(..) → .is_ok_and(..) (×2)
  • arcbox-computer-runtime — #[allow(clippy::result_large_err, …)] on the 4 boot/restore/stage entry points and #[allow(clippy::unused_async_trait_impl, …)] on promote_snapshot_to_template

Verified locally with the exact CI commands: cargo fmt -p <7 crates> --check ✓, cargo clippy -p <7 crates> -- -D warnings ✓ (exit 0), cargo test --lib --bins -p <7 crates> ✓ (517 passed, 0 failed).

Note: this commit is outside the PR's fix(computer) scope; it only unblocks the CI gate. If you'd prefer it on a separate PR against master instead of here (it overlaps #704's sweep), I can split it — but merging here is harmless since the fixes are byte-identical to #704's.

Task list (6/6 completed)

Pullfrog  | View workflow run | via Pullfrog | Using openai-compatible/@cf/deepseek-ai/deepseek-v4-flash-0731 | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Two rough edges worth a look — the fix itself is coherent, well-tested, and I found no correctness issue.

Reviewed changes — a single-commit fix that keeps kernel/rootfs provenance in the durable sandbox record after runtime-input redaction, fills a restored computer's effective spec from the source snapshot so its own checkpoint is restorable, and refuses to checkpoint records that lost their provenance (which also makes pause of a legacy adopted computer fail fast instead of writing an unrestorable snapshot — an intended behavior change).

  • Durable record retention (record/phase.rs): redact_runtime_inputs no longer clears kernel/rootfs, so adopted computers keep the paths a later checkpoint must record; the new unit test pins the exact redaction surface.
  • Restore provenance merge (sandbox/checkpoint.rs): restore_from_snapshot fills restore_spec.kernel/rootfs/geometry from the snapshot's meta before journaling, covering the Restore RPC (whose SandboxSpec is all defaults) and chained restores.
  • Checkpoint fail-fast guard (lifecycle/tasks/checkpoint.rs): refuses a checkpoint whose in-memory spec has no kernel/rootfs path, blocking legacy redacted records before the guest is frozen (so failures stay Recoverable).
  • Tests (tests/manager_over_fakes.rs): adopt → checkpoint → restore → exec, legacy-record refusal (asserting no partial snapshot is published), chained restore → checkpoint → restore, and the adopted pause/resume test upgraded to expect a successful resume. Each fails with the corresponding fix removed.

ℹ️ Nitpicks

  • RestoreRequest::spec's doc comment (sandbox/checkpoint.rs:22) says "the boot-recipe fields are ignored — the snapshot carries the boot state"; the new merge sources kernel/rootfs/geometry from the snapshot into the spec, so a one-line update would stop the two from reading as contradictory.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using openai-compatible/@cf/deepseek-ai/deepseek-v4-flash-0731 | 𝕏

Comment thread computer/arcbox-computer-runtime/src/sandbox/checkpoint.rs
Comment thread computer/arcbox-computer-runtime/src/lifecycle/tasks/checkpoint.rs
Copilot AI review requested due to automatic review settings August 26, 2026 04:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta since the prior review (83b3b86a) is the single commit fd4f1c3b, a mechanical clippy-1.98 compliance pass scoped to the test-vm-linux job:

  • Lint-compliance allows (computer/arcbox-computer-runtime): #[allow(clippy::result_large_err, reason = ...)] on do_boot, restore_vm, stage_rootfs_cow_or_copy, and stage_slot, plus #[allow(clippy::unused_async_trait_impl, reason = ...)] on promote_snapshot_to_template — annotations only, matching the neighboring allow style.
  • Equivalent-method substitutions: drain(..).collect() → std::mem::take (engine/arcbox-snapshot/src/template_catalog.rs), .ok().is_some_and(..) → .is_ok_and(..) (virt/arcbox-fc-driver/src/discover.rs), and chunks_exact_mut(4) → as_chunks_mut::<4>().0.iter_mut() (virt/arcbox-vm-agent/src/main.rs). Each is semantically identical — e.g. payload[8..] is 24 bytes, so both forms yield exactly six 4-byte chunks with no dropped remainder.
  • No tests were added or changed in this delta, and all Linux jobs (engine-layer check, unit, integration, E2E) are green on this head.

Both prior review threads remain resolved with the fixes landing in replacement PR #735, as agreed in discussion.

ℹ️ Informational

  • The macOS Build, Lint, and Test job is red on this head, but its sole error — chunks_exact_to_as_chunks at virt/arcbox-hypervisor/src/darwin/memory.rs:143 — is a pre-existing Rust 1.98 fire in a file this PR never touches, already fixed on master by the merged #704. It is not attributable to this PR and consistent with the commit's test-vm-linux-only scope.

Pullfrog  | View workflow run | Using glm-5.3-flash | 𝕏

@redwhisk

redwhisk commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Superseded by #735, merged into master at 2515bc9. The replacement retains durable boot assets, rejects missing restore geometry, preserves checkpoint error classification, and validates a jailed two-generation checkpoint chain with failure cleanup.

All four review threads have replacement evidence and are resolved. The original source branch remains available.

@redwhisk redwhisk closed this Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants