review: 5 fixes, docs from hardware, and tests that prove what they name

cgroup/cgroup_test.go

@@ -292,7 +292,7 @@ func TestPlaceScopeReadGate(t *testing.T) {
 	}
 }
 
-func TestArmSetsQuotaThenBurst(t *testing.T) {
+func TestArmWritesQuotaAndBurst(t *testing.T) {
 	parent := t.TempDir()
 	dir := ScopeDir(parent, "A")
 	if err := os.Mkdir(dir, 0o755); err != nil {

cmd/core/metering_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/cocoonstack/cocoon/metering"
 	meteringfile "github.com/cocoonstack/cocoon/metering/file"
 	meteringstderr "github.com/cocoonstack/cocoon/metering/stderr"
+	"github.com/cocoonstack/cocoon/utils"
 )
 
 func TestBuildRecorderBackends(t *testing.T) {
@@ -38,15 +39,19 @@ func TestBuildRecorderBackends(t *testing.T) {
 
 func TestBuildFileRecorderCustomPath(t *testing.T) {
 	dir := t.TempDir()
+	path := dir + "/custom/ledger.jsonl"
 	conf := &config.Config{
 		RootDir: dir,
 		Metering: config.MeteringConfig{
 			Backend: config.MeteringFile,
-			File:    config.FileMeteringConfig{Path: dir + "/custom/ledger.jsonl"},
+			File:    config.FileMeteringConfig{Path: path},
 		},
 	}
 	r := buildRecorder(t.Context(), conf)
 	if _, ok := r.(*meteringfile.Recorder); !ok {
 		t.Fatalf("got %T, want *meteringfile.Recorder", r)
 	}
+	if !utils.FileExists(path) {
+		t.Errorf("ledger %s not created at the configured path", path)
+	}
 }

cmd/meta/convert/convert_test.go

@@ -108,6 +108,11 @@ func TestResumeClaimsCommittedTarget(t *testing.T) {
 	if err := copyNamespace(ctx, src, dst, spec.Decls[0]); err != nil {
 		t.Fatalf("copy: %v", err)
 	}
+	if err := src.Update(ctx, meta.Scope{Write: "vms"}, meta.CommitDurable, func(w meta.Writer) error {
+		return w.PutRaw(ctx, "vms", "records", "late", jsontext.Value(`{}`), false)
+	}); err != nil {
+		t.Fatalf("mutate source: %v", err)
+	}
 	_ = src.Close()
 	_ = dst.Close()
 

cmd/vm/logstream_test.go

@@ -40,13 +40,15 @@ func TestWatchFileDebounce(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	ch, err := watchFile(t.Context(), target, 2*time.Second)
+	const debounce = 200 * time.Millisecond
+	ch, err := watchFile(t.Context(), target, debounce)
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	start := time.Now()
 	for i := range 5 {
-		if err := utils.AtomicWriteFile(target, []byte{byte('0' + i)}, 0o644, utils.Sync); err != nil {
+		if err := utils.AtomicWriteFile(target, []byte{byte('0' + i)}, 0o644, utils.NoSync); err != nil {
 			t.Fatal(err)
 		}
 	}
@@ -56,11 +58,14 @@ func TestWatchFileDebounce(t *testing.T) {
 	case <-time.After(10 * time.Second):
 		t.Fatal("timed out waiting for debounced signal")
 	}
+	if elapsed := time.Since(start); elapsed < debounce {
+		t.Fatalf("signal after %s, want it held for the %s debounce", elapsed, debounce)
+	}
 
 	select {
 	case <-ch:
 		t.Fatal("unexpected second signal within debounce window")
-	case <-time.After(500 * time.Millisecond):
+	case <-time.After(debounce / 2):
 	}
 }
 

daemon/cache_test.go

@@ -35,7 +35,10 @@ func TestCacheEmitsNothingForAnUnchangedPass(t *testing.T) {
 	events, release := c.subscribe()
 	defer release()
 
-	c.publish([]VMStatus{st}, true, 0, time.Now())
+	again := st
+	again.VerifiedAt = st.VerifiedAt.Add(time.Second)
+	again.WatchedPID = 99
+	c.publish([]VMStatus{again}, true, 0, time.Now())
 	if got := collect(t, events); len(got) != 0 {
 		t.Errorf("got %+v, want no events when nothing a consumer acts on changed", got)
 	}

daemon/reconcile_test.go

@@ -19,7 +19,7 @@ func TestReconcileConvergesDeadRunningRecord(t *testing.T) {
 	f := newFake().put(runningRec("vm1", 7))
 	newTestDaemon(t, f).reconcile(t.Context())
 
-	if len(f.converged) != 1 || f.converged[0] != (convergeCall{vmID: "vm1", gen: 7}) {
+	if len(f.converged) != 1 || f.converged[0].vmID != "vm1" || f.converged[0].gen != 7 {
 		t.Fatalf("got %+v, want one convergence fenced on generation 7", f.converged)
 	}
 }
@@ -142,6 +142,9 @@ func TestHandleExitConvergesWatchedGeneration(t *testing.T) {
 	if len(f.converged) != 1 || f.converged[0].gen != 4 {
 		t.Fatalf("got %+v, want one convergence at generation 4", f.converged)
 	}
+	if !f.converged[0].at.Equal(at) {
+		t.Errorf("got stop dated %v, want the watcher's observation %v", f.converged[0].at, at)
+	}
 }
 
 func TestHandleExitDropsStaleGeneration(t *testing.T) {
@@ -218,6 +221,7 @@ func TestPassCountsDegradedVMsWithoutGoingUnready(t *testing.T) {
 type convergeCall struct {
 	vmID string
 	gen  uint64
+	at   time.Time
 }
 
 type fakeSupervisor struct {
@@ -283,12 +287,12 @@ func (f *fakeSupervisor) PeekRecord(_ context.Context, vmID string) (*hypervisor
 	return f.records[vmID], nil
 }
 
-func (f *fakeSupervisor) ConvergeDead(_ context.Context, vmID string, gen uint64, _ time.Time) error {
+func (f *fakeSupervisor) ConvergeDead(_ context.Context, vmID string, gen uint64, at time.Time) error {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 	rec := f.records[vmID]
 	if rec != nil && hypervisor.NeedsDeadConvergence(rec) && rec.TransitionGeneration == gen {
-		f.converged = append(f.converged, convergeCall{vmID: vmID, gen: gen})
+		f.converged = append(f.converged, convergeCall{vmID: vmID, gen: gen, at: at})
 		rec.State = types.VMStateStopped
 		rec.TransitionGeneration++
 	}

docs/daemon.md

@@ -74,6 +74,8 @@ a rotation, or run the `meta` metering backend instead.
 The `file` backend only ever appends: cocoon never rotates, truncates, or prunes
 the ledger, so it grows without bound until an external rotation trims it.
 
+Each entry is written after the state change it reports commits. A SIGKILL between the two loses that entry and a retry does not write it again, and a retried restore can repeat a stop, so downstream must tolerate an unpaired or repeated endpoint.
+
 `--gc-interval` runs the same sweep as a bare `cocoon gc` — orphans and stale
 records only. It never evicts healthy snapshots by access recency: the LRU policy
 — including the `--snapshot-size` storage cap — comes from the `gc --snapshot*`

docs/known-issues.md

@@ -73,6 +73,12 @@ On x86_64, cocoon uses [rust-hypervisor-firmware](https://github.com/cloud-hyper
 
 This is an upstream issue tracked in [rust-hypervisor-firmware#333](https://github.com/cloud-hypervisor/rust-hypervisor-firmware/issues/333) and [cloud-hypervisor#7356](https://github.com/cloud-hypervisor/cloud-hypervisor/issues/7356). As a workaround, use **OCI VM images** for Ubuntu 24.04 — OCI images use direct kernel boot and are not affected.
 
+## Data disk fsck sends an existing VM to the emergency shell
+
+Cidata from v0.7.6 and earlier gives a mounted data disk fstab pass number 2. If the host's `mke2fs` formats with a feature the guest's `e2fsck` does not know, such as `orphan_file` from a Debian 13 host with an Ubuntu 22.04 guest, the next boot fails the fsck and enters the emergency shell with no network and no sshd, despite `nofail`. New cidata writes pass number 0.
+
+**Recovery**: log in through `cocoon vm console`, set the last field of the disk's `/etc/fstab` line to 0, then stop and start the VM.
+
 ## Named netns path removed while a VM runs
 
 cocoon never removes a running VM's named network namespace. If an operator or a foreign GC runs `ip netns del`, it removes the name under `/var/run/netns`; the running VMM still holds the original namespace and its TAPs. Existing traffic may continue, and `vm list` still shows the recorded IP. NIC hot-add cannot open the missing netns path, while `cocoon vm exec` can still work over vsock.
@@ -171,6 +177,8 @@ Cloud Hypervisor uses a GED (Generic Event Device, `ACPI0013`) to deliver power-
 
 **Timing caveat**: the Windows ACPI shutdown handler needs ~60 seconds from cold boot to fully initialize (SAC appears at ~30s, handler ready ~30s later). Stopping a Windows VM before the handler is ready triggers the 30s `stop_timeout_seconds` fallback and escalates to force-kill. Clone-restored VMs inherit the ready ACPI state and shut down in ~8-13s immediately.
 
+**Background servicing**: after about 15 minutes of uptime, Windows runs maintenance such as `StartComponentCleanup`, and shutdown waits for `TrustedInstaller` to stop. In testing a cold-booted VM took 20s and a clone 73s to power off, with the power-button event logged at once, so `vm stop` can reach the 30s `stop_timeout_seconds` and force-kill. Raise `stop_timeout_seconds` for a Windows VM that must stop cleanly.
+
 **Previous consequence** (no longer applies with our firmware fork): `cocoon vm stop` always timed out on Windows VMs (default 30s), then fell back to `vm.shutdown` → SIGTERM → SIGKILL.
 
 **Previous workaround** (no longer needed with our firmware fork): shut down Windows guests via SSH or WinRM before stopping:

docs/vm.md

@@ -157,7 +157,7 @@ cocoon vm run --data-disk size=20G,name=raw,fstype=none <oci-image>
 
 ### Snapshot/Clone/Restore
 
-Phase 1 inherits data disks 1:1: snapshot reflinks each `data-<name>.raw` into the snapshot tar, clone re-creates them under the new VM's runDir (and regenerates cidata so cloud-init re-mounts on the new identity), and restore rolls all data disks back to the snapshot timepoint along with the rootfs and memory state. Cloud Hypervisor clones can additionally CREATE fresh data disks at clone time via `--data-disk` (hot-added after restore — the snapshot's device tree itself cannot grow); clone-created disks are hot-added after cidata is regenerated, so they are never auto-mounted — `mount=` has no effect on `vm clone --data-disk`; mount them inside the guest. Removing inherited disks at clone time is not supported, and Firecracker clones accept `--data-disk` only from a `--pci` snapshot (MMIO cannot hot-plug).
+Phase 1 inherits data disks 1:1: snapshot reflinks each `data-<name>.raw` into the snapshot tar, clone re-creates them under the new VM's runDir (and regenerates cidata so cloud-init re-mounts on the new identity), and restore rolls all data disks back to the snapshot timepoint along with the rootfs and memory state. Cloud Hypervisor clones can additionally CREATE fresh data disks at clone time via `--data-disk` (hot-added after restore — the snapshot's device tree itself cannot grow); clone-created disks are hot-added after cidata is regenerated, so that clone does not auto-mount them — `mount=` has no effect on it; mount them inside the guest. A clone of that clone inherits the disk like any other: its regenerated cidata lists the mount, which applies when the [post-clone guest setup](snapshots.md#post-clone-guest-setup) commands run. Removing inherited disks at clone time is not supported, and Firecracker clones accept `--data-disk` only from a `--pci` snapshot (MMIO cannot hot-plug).
 
 Restore preflight verifies sidecar integrity, file presence (vmstate, memory, COW, every `data-*.raw` — presence, not size; `--from-dir` and `snapshot import` separately check the COW size against the envelope), per-index Path/RO agreement between the sidecar and CH config.json, Role/Serial agreement between the sidecar and the VM record, and on Firecracker that every COW and data disk is recorded at this VM's own path, all **before** killing the running VM, so a malformed or imported snapshot fails fast and leaves the live VM untouched.
 

hypervisor/cgroupgc.go

@@ -14,6 +14,8 @@ import (
 	"github.com/cocoonstack/cocoon/utils"
 )
 
+var removeScope = cgroup.RemoveEmpty
+
 // removeCgroupScope reclaims id's scope after its VMM is confirmed dead; failure is left for the next converge/GC pass.
 func (b *Backend) removeCgroupScope(ctx context.Context, id string) {
 	if err := cgroup.Remove(ctx, b.Conf.CgroupParentDir(), id); err != nil {
@@ -42,7 +44,7 @@ func CgroupGCModule(parentDir string, vmInUse network.VMInUse) gc.Module[[]strin
 					continue
 				}
 				// A scope populated since the VM snapshot was read belongs to a launch in flight, not to an orphan
-				err := cgroup.RemoveEmpty(parentDir, id)
+				err := removeScope(parentDir, id)
 				if errors.Is(err, syscall.EBUSY) {
 					logger.Infof(ctx, "skipped scope vm-%s reason=populated", id)
 					continue

hypervisor/cgroupgc_test.go

@@ -2,9 +2,10 @@ package hypervisor
 
 import (
 	"context"
+	"io/fs"
 	"os"
-	"path/filepath"
 	"slices"
+	"syscall"
 	"testing"
 
 	"github.com/cocoonstack/cocoon/cgroup"
@@ -66,19 +67,27 @@ func TestCgroupGCModuleLeavesScopeOfAVMCreatedAfterTheSnapshot(t *testing.T) {
 	}
 }
 
-func TestCgroupGCModuleLeavesPopulatedScope(t *testing.T) {
-	parent := t.TempDir()
-	dir := cgroup.ScopeDir(parent, "BUSY")
-	if err := os.MkdirAll(filepath.Join(dir, "child"), 0o755); err != nil {
-		t.Fatalf("setup: %v", err)
-	}
+func TestCgroupGCModuleCollectRemoveFailures(t *testing.T) {
+	for _, tt := range []struct {
+		name    string
+		errno   syscall.Errno
+		wantErr bool
+	}{
+		{name: "kernel refuses a populated scope", errno: syscall.EBUSY},
+		{name: "any other refusal surfaces", errno: syscall.ENOTEMPTY, wantErr: true},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			orig := removeScope
+			removeScope = func(string, string) error {
+				return &fs.PathError{Op: "remove", Path: "vm-BUSY.scope", Err: tt.errno}
+			}
+			t.Cleanup(func() { removeScope = orig })
 
-	m := CgroupGCModule(parent, noVMInUse)
-	if err := m.Collect(t.Context(), []string{"BUSY"}, nil); err == nil {
-		t.Error("want error for populated scope, got nil")
-	}
-	if _, err := os.Stat(dir); err != nil {
-		t.Errorf("populated scope removed: %v", err)
+			err := CgroupGCModule(t.TempDir(), noVMInUse).Collect(t.Context(), []string{"BUSY"}, nil)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Collect = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
 	}
 }
 

hypervisor/cloudhypervisor/restore.go

@@ -130,9 +130,6 @@ func resolveRestoreMode(ctx context.Context, mode string, mem chMemory) string {
 
 // vm.restore replays the snapshot's guest MACs verbatim, so drifted NIC identity must fail first.
 func validateRestoreNICs(chCfg *chVMConfig, rec *hypervisor.VMRecord) error {
-	if len(chCfg.Nets) != len(rec.NetworkConfigs) {
-		return fmt.Errorf("snapshot has %d NICs, vm has %d; NIC identity must match for restore", len(chCfg.Nets), len(rec.NetworkConfigs))
-	}
 	macs := make([]string, len(chCfg.Nets))
 	for i, n := range chCfg.Nets {
 		macs[i] = n.MAC

hypervisor/cloudhypervisor/snapshot.go

@@ -46,6 +46,9 @@ func (ch *CloudHypervisor) snapshotSpec(ctx context.Context) hypervisor.Snapshot
 			if err := hotAttachedError(info.Config.Fs, info.Config.Devices, info.Config.Disks); err != nil {
 				return err
 			}
+			if len(info.Config.Nets) != len(rec.NetworkConfigs) {
+				return fmt.Errorf("vm %s has %d NICs but its record has %d; rerun vm net first", rec.ID, len(info.Config.Nets), len(rec.NetworkConfigs))
+			}
 			if err := waitBalloonSettled(ctx, hc, rec.ID, info); err != nil {
 				return err
 			}

hypervisor/cloudhypervisor/snapshot_test.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/cocoonstack/cocoon/hypervisor"
+	"github.com/cocoonstack/cocoon/types"
 )
 
 func TestSnapshotPauseRefusesHotAttachedBeforePausing(t *testing.T) {
@@ -60,6 +61,30 @@ func TestSnapshotPauseRefusesHotAttachedBeforePausing(t *testing.T) {
 	}
 }
 
+func TestSnapshotPauseRefusesANICCountOffItsRecord(t *testing.T) {
+	pauses := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/vm.info", func(w http.ResponseWriter, _ *http.Request) {
+		_ = json.MarshalWrite(w, chVMInfoResponse{State: chStateRunning, Config: chVMConfig{Nets: []chNet{{ID: "net0"}}}})
+	})
+	mux.HandleFunc("/api/v1/vm.pause", func(w http.ResponseWriter, _ *http.Request) {
+		pauses++
+		w.WriteHeader(http.StatusNoContent)
+	})
+	hc := &http.Client{Transport: handlerTransport{mux}}
+	rec := &hypervisor.VMRecord{}
+	rec.NetworkConfigs = []*types.NetworkConfig{{}, {}}
+
+	err := (&CloudHypervisor{}).snapshotSpec(t.Context()).Pause(rec, hc)
+
+	if err == nil || !strings.Contains(err.Error(), "rerun vm net") {
+		t.Fatalf("err = %v, want the capture refused until vm net converges", err)
+	}
+	if pauses != 0 {
+		t.Fatalf("pauses = %d, want the VM left running", pauses)
+	}
+}
+
 func TestSnapshotPauseWaitsForBalloonToSettle(t *testing.T) {
 	const (
 		mem     = int64(8 << 30)

hypervisor/firecracker/create.go

@@ -52,6 +52,43 @@ func EnsureVmlinuxBoot(boot *types.BootConfig) error {
 	return nil
 }
 
+// ensureVmlinux decompresses kernelPath if needed and returns the ELF path.
+func ensureVmlinux(kernelPath string) (string, error) {
+	f, err := os.Open(kernelPath) //nolint:gosec
+	if err != nil {
+		return "", fmt.Errorf("open kernel: %w", err)
+	}
+	var magic [4]byte
+	_, err = io.ReadFull(f, magic[:])
+	_ = f.Close()
+	if err != nil {
+		return "", fmt.Errorf("read kernel magic: %w", err)
+	}
+	if bytes.Equal(magic[:], elfMagic) {
+		return kernelPath, nil
+	}
+
+	vmlinuxPath := filepath.Join(filepath.Dir(kernelPath), "vmlinux")
+	if utils.FileExists(vmlinuxPath) {
+		return vmlinuxPath, nil
+	}
+
+	data, err := os.ReadFile(kernelPath) //nolint:gosec
+	if err != nil {
+		return "", fmt.Errorf("read kernel: %w", err)
+	}
+
+	decompressed, decompErr := decompressKernel(data)
+	if decompErr != nil {
+		return "", fmt.Errorf("decompress kernel from %s: %w (FC requires uncompressed ELF kernel)", kernelPath, decompErr)
+	}
+
+	if err := utils.AtomicWriteFile(vmlinuxPath, decompressed, 0o600, utils.Sync); err != nil {
+		return "", fmt.Errorf("write vmlinux: %w", err)
+	}
+	return vmlinuxPath, nil
+}
+
 func decompressKernel(data []byte) ([]byte, error) {
 	type kernelCodec struct {
 		name   string
@@ -122,40 +159,3 @@ func devPath(idx int) string {
 	}
 	return fmt.Sprintf("/dev/vd%c%c", 'a'+(idx/letters)-1, 'a'+idx%letters)
 }
-
-// ensureVmlinux decompresses kernelPath if needed and returns the ELF path.
-func ensureVmlinux(kernelPath string) (string, error) {
-	f, err := os.Open(kernelPath) //nolint:gosec
-	if err != nil {
-		return "", fmt.Errorf("open kernel: %w", err)
-	}
-	var magic [4]byte
-	_, err = io.ReadFull(f, magic[:])
-	_ = f.Close()
-	if err != nil {
-		return "", fmt.Errorf("read kernel magic: %w", err)
-	}
-	if bytes.Equal(magic[:], elfMagic) {
-		return kernelPath, nil
-	}
-
-	vmlinuxPath := filepath.Join(filepath.Dir(kernelPath), "vmlinux")
-	if utils.FileExists(vmlinuxPath) {
-		return vmlinuxPath, nil
-	}
-
-	data, err := os.ReadFile(kernelPath) //nolint:gosec
-	if err != nil {
-		return "", fmt.Errorf("read kernel: %w", err)
-	}
-
-	decompressed, decompErr := decompressKernel(data)
-	if decompErr != nil {
-		return "", fmt.Errorf("decompress kernel from %s: %w (FC requires uncompressed ELF kernel)", kernelPath, decompErr)
-	}
-
-	if err := utils.AtomicWriteFile(vmlinuxPath, decompressed, 0o600, utils.Sync); err != nil {
-		return "", fmt.Errorf("write vmlinux: %w", err)
-	}
-	return vmlinuxPath, nil
-}