fix(fc): clone from a snapshot must not require a live source VM#122
Merged
Conversation
v0.5.0 moved clone locks into the source VM's run dir and added a source-record check, breaking every clone whose snapshot outlived its source (sandboxd warm-pool refill, import+clone). Clone locks now live under clone-locks/ as transient flocks: unlinked on release, inode re-verified on acquire, so rm/GC unlinks can never split two holders onto different inodes. GC reclaims crash-orphaned lock files and reserves the dir in the orphan scan; drive-redirect cleanup unwinds in reverse so a recreated shared source dir is removed.
sweepStaleCaptureDirs treated clone-locks/ (and db/) as VM run dirs and withOpsTryLock planted a permanent ops.lock inside them; share one reserved-name set between the orphan scan and the sweep list.
Adversarial review found two dead-source clone holes: the transiently recreated source dir is recordless, so a concurrent gc orphan scan could reap it mid-clone — the clone now holds the dir's ops lock (the same fence create uses for pre-record dirs), released and removed with the redirect window. And a clone killed mid-window leaves a dangling redirect symlink with no backup to trigger recovery — recoverStaleBackup now clears any symlink at a source drive path unconditionally. Dir creation moves out of createDriveRedirects, dropping createdDir and the reverse-unwind ordering.
…Backup Imported snapshot metadata is untrusted and COW/Data paths are exempt from the managed-root check; an unconditional remove would let a crafted cocoon.json delete an arbitrary host symlink. A backup beside the link or a target under the managed run root identifies cocoon's own redirects; anything else is preserved.
A foreign symlink whose target happens to point into the run root was still classified as a cocoon redirect. Every redirect cocoon creates sits inside the managed run root, so test the link path instead of its target — stricter, and drops the Readlink helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
v0.5.0 (#118) broke every FC clone whose snapshot outlived its source VM — the sandboxd warm-pool steady state (golden exported then destroyed, refills via
vm clone --from-dir) and FC snapshot import+clone generally. Two gates caused it: clone locks moved into the source VM's run dir with no MkdirAll (acquire flock <srcDir>/cow.raw.clone.lock: no such file or directory), andensureSourceAliverejected any source without a live record. 100% reproducible:vm run → snapshot save → snapshot export → vm rm → vm clone --from-dir.The restored contract: a successfully exported snapshot is a self-contained artifact — clone works regardless of whether the source VM still exists. The only residual coupling is the source-absolute drive path embedded in FC vmstate, resolved by the transient symlink redirect; it needs the source's directory path as a mount point for milliseconds, never the source VM itself.
Mechanism
NewTransient: unlink on Unlock (before funlock, under the held lock) + inode re-verify after every acquire (fstat vs path, requeue on mismatch). Deleting a lock file becomes race-free, which unlocks the rest:clone-locks/dir (sha8-named), transient — empty at rest, no fresh-inode split, no accumulation.ensureSourceAlivedeleted; a clone racingvm rmof its source now proceeds standalone instead of aborting.holdRedirectDirs: recreated source dirs are recordless, so the clone holds their ops locks (the same fence create uses for pre-record dirs) — the GC orphan scan cannot reap them mid-clone; released and removed with the redirect window.recoverStaleBackupheals a crashed clone's dangling redirect (dead source leaves no backup), guarded to only touch symlinks inside the managed run root — imported metadata is untrusted and COW/Data paths are exempt from root validation, so anything foreign is preserved.db,clone-locks) excluded from both the orphan scan and the capture-dir sweep (the sweep was planting a permanentops.lockinside them); crash-orphaned clone locks past the grace age reclaimed via TryLock + verified unlink.Evidence (bare-metal testbed, isolated root + pinned binaries)
clone-locks/empty at rest; source dir not resurrected.vm rm --force×5 rounds: all resolve clean;gccollects nothing.cocoon gcloop — 920,086 gc cycles, 0 failures, 12/12 clones healthy.go test -race ./...green (linux container),make lint0 issues on linux+darwin.Review dispositions
Adversarial review ran to convergence over four rounds. Accepted: the GC reap fence, the dangling-redirect heal, and the untrusted-metadata symlink guard (keyed on the link's own location, not its target). Declined with rationale:
Line budget: production net +127, tests net +192.
E2E catalog: FC-T13..T15 added in cocoon-specs (dead-source clone, concurrent dead-source clones, clone-vs-rm race) — the gap that let this regress: every prior clone case ran with the source alive.