Repository navigation
Conversation
Co-authored-by: Xuan Zhang <xuan@arcbox.dev>
There was a problem hiding this comment.
💡 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".
Greptile SummaryThe PR preserves kernel and rootfs provenance in durable sandbox records and reconstructs restore specifications from snapshot metadata.
Confidence Score: 4/5The 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
|
| 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
Reviews (2): Last reviewed commit: "fix(ci): satisfy rust 1.98 clippy in tes..." | Re-trigger Greptile
|
Fixed the red check suite on PR #706 and pushed commit Diagnosis: The failure (Unit tests → Clippy) was not introduced by this PR — it was pre-existing Rust 1.98 clippy lints in the Fix: I ran the full enumeration in CI's exact scope (without
Verified locally with the exact CI commands: Note: this commit is outside the PR's Task list (6/6 completed)
|
There was a problem hiding this comment.
ℹ️ 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_inputsno longer clearskernel/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_snapshotfillsrestore_spec.kernel/rootfs/geometry from the snapshot's meta before journaling, covering the Restore RPC (whoseSandboxSpecis 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 stayRecoverable). - 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.
openai-compatible/@cf/deepseek-ai/deepseek-v4-flash-0731 | 𝕏
a830a86 to
6896d12
Compare
There was a problem hiding this comment.
✅ 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 = ...)]ondo_boot,restore_vm,stage_rootfs_cow_or_copy, andstage_slot, plus#[allow(clippy::unused_async_trait_impl, reason = ...)]onpromote_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), andchunks_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 Testjob is red on this head, but its sole error —chunks_exact_to_as_chunksatvirt/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'stest-vm-linux-only scope.
glm-5.3-flash | 𝕏
|
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. |

Summary
Verification
cargo fmt --all --checkcargo 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_chunksFull 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
kernelandrootfswhen redacting consumed boot inputs atReady; workload secrets (cmd,env, etc.) are still stripped. Restore merges snapshot provenance into the journaledrestore_specbefore 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.