refactor(husk): restore and resume the guest while dormant, not while a tenant waits#892
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds an opt-in prepare-time restore mode for the default multi-VM husk VM, propagates its configuration through controllers and CLIs, introduces an activation fast path with snapshot validation, and adds lifecycle tests and documentation updates. ChangesPrepare-time restore
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… a tenant waits Slice 2 of docs/superpowers/plans/2026-07-10-prepare-time-restore.md, building on the tap-at-prepare slice (#883). Together they move almost the whole warm-claim activate off the hot path. Measured on prod, one matched claim: the husk activate is 112.8 ms, of which the restore (vmstate_restore 22.9), the resume (2.4), and the guest-ready wait (40.6, largely demand fault-in under the lazy UFFD restore) are ~66 ms. All of it is a function of the pod and its template, not of the claim. Only the fork-correctness handshake (fresh entropy, the clock step, the per-VM network re-addressing, and the tenant secrets) and serving the sandbox API with the claim token actually need the claim. Behind --husk-prepare-restore (which requires --husk-prepare-egress-link, because Firecracker binds the baked NIC to the tap at snapshot load), a warm pod now loads its default VM's snapshot and RESUMES its guest while dormant. Activate takes a fast path when the instance is pre-restored: it skips the restore, the rootfs rebind, the resume, and the guest-ready wait, re-dials the already-running guest (sub-millisecond, the vsock listener has been up since the prepare-time resume), and pays only the handshake. The handshake-and-serve tail is extracted into activateHandshakeAndServe so both paths share it byte-for-byte. Fail-closed, which is the load-bearing part: - Prepare refuses to go dormant if the restore, rootfs rebind, resume, or guest-ready fails, so the pool never offers a half-restored pod. - Activate refuses a claim whose snapshot dir differs from the one pre-restored, with a named error, because a resumed guest cannot be reloaded and must never be served against the wrong image. - Only the DEFAULT VM is pre-restored. A co-located fork child (its own snapshot, childUFFD backend, per-fork tap) and the pre-warm generic child are excluded, so the fork paths are unchanged. - The ready conn proven at prepare is CLOSED, not held across dormancy where it could go stale. The dormant guest is a new surface and the threat model records it: it serves no tenant (the sandbox API is bound and the token delivered only at the claim), it is reseeded fail-closed at the claim's NotifyForked exactly as a restore-at-activate guest is (the dormancy window only enlarges the absolute clock step), and it holds its working set resident (~72 MiB/sandbox) versus ~0 for a never-loaded VMM. DEFAULT OFF at every layer: cmd/controller rejects --husk-prepare-restore without --husk-prepare-egress-link (and therefore without --multi-vm-fork), the pod builder emits --prepare-restore only inside the prepared-link block, and the stub no-ops unless the tap was prepared. Off, the restore path is byte-for-byte the activate-time one. NOT KVM-verified and NOT canaried on prod: this needs a firecracker-test gate and a one-pod canary before it may be called verified, and it is the highest-risk change in the latency effort (it resumes a live guest during dormancy on the tenant-VM boot path). Verified with the mock VMM (the state machine is engine-independent): TestPrepareRestoresAndResumesTheGuestWhileDormant (load+resume at prepare, none at activate), TestPrepareRestoreIsOptIn (default off is byte-for-byte activate-time), TestActivateFailsClosedOnASnapshotMismatchAfterPrepareRestore, TestBuildHuskPodThreadsPrepareRestore. Each fails on the unfixed code. go test -race ./internal/husk/, go test ./internal/controller/ ./cmd/husk-stub/, and golangci-lint clean for both GOOS=darwin and GOOS=linux. internal/husk is a security-sensitive path per CLAUDE.md and needs a named human reviewer. Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
stubbi
force-pushed
the
perf/husk-prepare-restore
branch
from
5617ff9 to
b9cf1fa
Compare
|
Rebased onto main now that #883 has merged (base retargeted to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/husk/multivm_network_test.go`:
- Around line 448-467: Extend
TestActivateFailsClosedOnASnapshotMismatchAfterPrepareRestore to assert that the
recording runner rr has no captured calls after Activate rejects the mismatched
snapshot. Check rr’s call collection using its existing field or accessor,
ensuring the assertion specifically verifies that no egress-filter mutation
occurred before the identity mismatch was detected.
In `@internal/husk/multivm.go`:
- Around line 660-688: Move the pre-restored snapshot identity check from the
inst.preRestored fast-path block to immediately after the empty SnapshotDir
validation, before the verify-on-activate gate and per-VM egress policy setup.
Reuse the existing mismatch error and return ActivateResult with OK false;
remove the later duplicate check so mismatched pre-restored claims cannot
trigger verification or network mutation.
- Around line 408-421: Update the prepareRestoreDefaultVM docstring to replace
the nonexistent opts.preWarmBoot precondition with the actual generic-child
exclusion condition, opts.skipRootfsClone || opts.reuseVM != nil, matching the
guard and nearby inline comment.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f09ef878-237a-4f0a-a13a-8756e7892d1c
📒 Files selected for processing (12)
cmd/controller/main.gocmd/controller/main_test.gocmd/husk-stub/main.godocs/superpowers/plans/2026-07-10-prepare-time-restore.mddocs/threat-model.mdinternal/controller/huskpod.gointernal/controller/huskpod_test.gointernal/controller/sandboxclaim_controller.gointernal/controller/sandboxpool_controller.gointernal/husk/multivm.gointernal/husk/multivm_network_test.gointernal/husk/stub.go
…effect Review found that the snapshot-identity gate fired too late. It sat at the pre-restored fast path, AFTER the verify-on-activate re-hash and AFTER the per-VM egress filter, so a claim naming a different snapshot than the one this pod pre-restored had already had the tenant's real egress policy installed on the tap fronting the wrong guest (and a re-verify run against the mismatched dir) before the "refusing to serve the wrong image" rejection fired. The VM never went active and never got secrets or a reseed, but the network mutation happened before the gate, which is the opposite of fail-closed. The identity check now runs FIRST, right after the empty-snapshot-dir guard, before any verify or egress mutation. The fast path keeps only a comment noting the invariant now holds. TestActivateFailsClosedOnASnapshotMismatchAfterPrepareRestore now records the net command count and asserts a mismatch adds NONE; with the gate moved back to its old position the test fails with "mutated the egress datapath (1 new net commands)". Also from review: two docstrings named a nonexistent opts.preWarmBoot; the actual guard is opts.skipRootfsClone || opts.reuseVM != nil, and the docstrings now say so. Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
|
All three addressed in Snapshot-mismatch fires too late (Major). Correct, and a genuine fail-closed violation. The identity check sat at the pre-restored fast path, AFTER the verify-on-activate re-hash and AFTER the per-VM egress filter, so a wrong-image claim had already installed the tenant policy on the tap fronting the (wrong) guest before the rejection fired. The check now runs FIRST, right after the empty-snapshot-dir guard, before any verify or egress mutation: if inst.preRestored && req.SnapshotDir != inst.preRestoredSnapshotDir {
// ... refuse before any side effect ...
}Assert no egress mutation on mismatch (Trivial). Done, and it is exactly what pins the fix. Stale
|
|
Prod observation after rolling v1.40.0 (which carries this PR, flags OFF): the warm-claim activate_rpc stage regressed from 98.2 ms P50 (v1.39.1, n=33 Ready claims, 24h window ending 2026-07-10T19:40Z) to 118.8 ms P50 (v1.40.0, n=100 sequential claims, 2026-07-12 ~17:00Z window), max 137. Every claim succeeded; the shift is uniform, not a tail. mark_pod_claimed and status_write_ready moved little (16.5 / 9.5 ms P50), so the +20 ms sits inside the husk activate round trip specifically. Both samples are prod, same node, same pool, steady state (the v1.40.0 sample starts 2h after the roll). Client-observed create median stayed flat (189.3 to 190.9 ms) because the gateway round-trip cuts (#895) landed in the same release and offset part of it; first exec also drifted up (136.8 to 163.1 ms median), which could be the same underlying cause or environment drift. Not asserting a root cause: the flag-off path is meant to be inert here, but the numbers moved with the roll. Raw stage medians and the method are reproducible from the controller "claim stage timing" logs for those windows if useful. |
1 participant
Stacked on #883 (slice 1, tap-at-prepare). Base is
perf/husk-prepare-egress-link; I will retarget tomainonce #883 merges. Review #883 first.Thinking Path
Linked Issues or Issue Description
Slice 2 of #889 (prepare-time restore). Closes most of the gap tracked in #871.
What Changed
Options.PrepareRestoreplus--prepare-restore(stub) and--husk-prepare-restore(controller), threaded throughHuskPodOptions. All DEFAULT OFF.prepareRestoreDefaultVMrunssetLiveCowMemSource/LoadSnapshotWithOverrides/PatchDrive/Resume/ guest-ready for the DEFAULT VM after the tap is up, then CLOSES the ready conn (not held across dormancy, where it could go stale).activateInstancetakes a fast path wheninst.preRestored: skip restore/rebind/resume/guest-ready, re-dial the already-running guest (sub-ms), pay only the handshake.activateHandshakeAndServe, shared by both paths byte-for-byte.docs/threat-model.mdrecords the dormant-resumed-guest surface; the plan marks slice 2 done.Fail-closed, the load-bearing part
Verification
go test -race ./internal/husk/ -count=1go test ./internal/controller/ ./cmd/husk-stub/ -count=1go build ./...,gofmt -lclean,golangci-lint runclean forGOOS=darwinandGOOS=linuxNew tests, each failing on the unfixed code (state machine is engine-independent, so the mock VMM exercises it):
TestPrepareRestoresAndResumesTheGuestWhileDormant(load+resume happen at Prepare, neither at Activate),TestPrepareRestoreIsOptIn(default off is byte-for-byte activate-time),TestActivateFailsClosedOnASnapshotMismatchAfterPrepareRestore,TestBuildHuskPodThreadsPrepareRestore.NOT KVM-verified and NOT canaried. The state machine is proven with the mock; the real restore/resume needs a
firecracker-testgate and a one-pod prod canary before it may be called verified. That work is tracked in #889.Risks
internal/huskis a security-sensitive path per CLAUDE.md and needs a named human reviewer. This is the highest-risk change in the latency effort: it resumes a live guest during dormancy on the tenant-VM boot path.The dormant guest is a new surface, recorded in the threat model. It serves no tenant during dormancy (the sandbox API is bound and the token delivered only at the claim), it is reseeded fail-closed at the claims
NotifyForkedexactly as a restore-at-activate guest is (the dormancy window only enlarges the absolute clock step), and it holds its working set resident (~72 MiB/sandbox) versus ~0 for a never-loaded VMM.Off (the default, and every deployment) the restore path is byte-for-byte the activate-time one, which
TestPrepareRestoreIsOptInpins. The startup guard rejects--husk-prepare-restorewithout--husk-prepare-egress-link(and therefore without--multi-vm-fork).Not deployed by merging: this is a flag-gated engine change that will not touch prod until an operator sets the flag, after the KVM gate and canary in #889.
Model Used
claude-opus-4-8), 1M context, extended thinking.Checklist
Summary by CodeRabbit
New Features
Documentation
Tests