From 149bc2985a0aaa0085e87291cca37d42a22807c6 Mon Sep 17 00:00:00 2001 From: Harivansh Rathi Date: Sat, 11 Apr 2026 02:08:57 +0000 Subject: [PATCH] fix: address CI failures and review feedback --- internal/daemon/daemon.go | 2 ++ internal/daemon/daemon_test.go | 35 ++++++++++++++-------- internal/daemon/lifecycle.go | 13 ++++---- internal/daemon/review_regressions_test.go | 4 +-- 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index a14ee21..6275508 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -47,6 +47,7 @@ type Daemon struct { reconfigureGuestIdentity func(context.Context, string, contracthost.MachineID, *contracthost.GuestConfig) error readGuestSSHPublicKey func(context.Context, string) (string, error) syncGuestFilesystem func(context.Context, string) error + shutdownGuest func(context.Context, string) error personalizeGuest func(context.Context, *model.MachineRecord, firecracker.MachineState) error locksMu sync.Mutex @@ -94,6 +95,7 @@ func New(cfg appconfig.Config, store store.Store, runtime Runtime) (*Daemon, err daemon.reconfigureGuestIdentity = daemon.reconfigureGuestIdentityOverSSH daemon.readGuestSSHPublicKey = readGuestSSHPublicKey daemon.syncGuestFilesystem = daemon.syncGuestFilesystemOverSSH + daemon.shutdownGuest = daemon.issueGuestPoweroff daemon.personalizeGuest = daemon.personalizeGuestConfig if err := daemon.ensureBackendSSHKeyPair(); err != nil { return nil, err diff --git a/internal/daemon/daemon_test.go b/internal/daemon/daemon_test.go index dde8bf6..a510bc7 100644 --- a/internal/daemon/daemon_test.go +++ b/internal/daemon/daemon_test.go @@ -21,13 +21,14 @@ import ( ) type fakeRuntime struct { - bootState firecracker.MachineState - bootCalls int - restoreCalls int - deleteCalls []firecracker.MachineState - lastSpec firecracker.MachineSpec - lastLoadSpec firecracker.SnapshotLoadSpec - mmdsWrites []any + bootState firecracker.MachineState + bootCalls int + restoreCalls int + deleteCalls []firecracker.MachineState + lastSpec firecracker.MachineSpec + lastLoadSpec firecracker.SnapshotLoadSpec + mmdsWrites []any + inspectOverride func(firecracker.MachineState) (*firecracker.MachineState, error) } func (f *fakeRuntime) Boot(_ context.Context, spec firecracker.MachineSpec, _ []firecracker.NetworkAllocation) (*firecracker.MachineState, error) { @@ -38,6 +39,9 @@ func (f *fakeRuntime) Boot(_ context.Context, spec firecracker.MachineSpec, _ [] } func (f *fakeRuntime) Inspect(state firecracker.MachineState) (*firecracker.MachineState, error) { + if f.inspectOverride != nil { + return f.inspectOverride(state) + } copy := state return ©, nil } @@ -256,9 +260,15 @@ func TestStopMachineSyncsGuestFilesystemBeforeDelete(t *testing.T) { t.Fatalf("create daemon: %v", err) } - var syncedHost string - hostDaemon.syncGuestFilesystem = func(_ context.Context, runtimeHost string) error { - syncedHost = runtimeHost + var shutdownHost string + hostDaemon.shutdownGuest = func(_ context.Context, runtimeHost string) error { + shutdownHost = runtimeHost + // Simulate the VM exiting after poweroff by making Inspect return stopped. + runtime.inspectOverride = func(state firecracker.MachineState) (*firecracker.MachineState, error) { + state.Phase = firecracker.PhaseStopped + state.PID = 0 + return &state, nil + } return nil } @@ -282,9 +292,10 @@ func TestStopMachineSyncsGuestFilesystemBeforeDelete(t *testing.T) { t.Fatalf("stop machine: %v", err) } - if syncedHost != "172.16.0.2" { - t.Fatalf("sync host mismatch: got %q want %q", syncedHost, "172.16.0.2") + if shutdownHost != "172.16.0.2" { + t.Fatalf("shutdown host mismatch: got %q want %q", shutdownHost, "172.16.0.2") } + // runtime.Delete is always called to clean up TAP device and runtime dir. if len(runtime.deleteCalls) != 1 { t.Fatalf("runtime delete call count mismatch: got %d want 1", len(runtime.deleteCalls)) } diff --git a/internal/daemon/lifecycle.go b/internal/daemon/lifecycle.go index 302db71..8078145 100644 --- a/internal/daemon/lifecycle.go +++ b/internal/daemon/lifecycle.go @@ -498,14 +498,13 @@ func (d *Daemon) stopMachineRecord(ctx context.Context, record *model.MachineRec d.stopMachineRelays(record.ID) d.stopPublishedPortsForMachine(record.ID) - cleanExit := false if record.Phase == contracthost.MachinePhaseRunning && strings.TrimSpace(record.RuntimeHost) != "" { - cleanExit = d.shutdownGuestClean(ctx, record) + d.shutdownGuestClean(ctx, record) } - if !cleanExit { - if err := d.runtime.Delete(ctx, machineToRuntimeState(*record)); err != nil { - return err - } + // Always call runtime.Delete: it cleans up the TAP device, runtime + // directory, and process (no-op if the process already exited). + if err := d.runtime.Delete(ctx, machineToRuntimeState(*record)); err != nil { + return err } record.Phase = contracthost.MachinePhaseStopped @@ -525,7 +524,7 @@ func (d *Daemon) shutdownGuestClean(ctx context.Context, record *model.MachineRe shutdownCtx, cancel := context.WithTimeout(ctx, defaultGuestStopTimeout) defer cancel() - if err := d.issueGuestPoweroff(shutdownCtx, record.RuntimeHost); err != nil { + if err := d.shutdownGuest(shutdownCtx, record.RuntimeHost); err != nil { fmt.Fprintf(os.Stderr, "warning: guest poweroff for %q failed: %v\n", record.ID, err) return false } diff --git a/internal/daemon/review_regressions_test.go b/internal/daemon/review_regressions_test.go index e49e59b..5322097 100644 --- a/internal/daemon/review_regressions_test.go +++ b/internal/daemon/review_regressions_test.go @@ -608,8 +608,8 @@ func TestStopMachineContinuesWhenGuestSyncFails(t *testing.T) { t.Fatalf("create daemon: %v", err) } stubGuestSSHPublicKeyReader(hostDaemon) - hostDaemon.syncGuestFilesystem = func(context.Context, string) error { - return errors.New("guest sync failed") + hostDaemon.shutdownGuest = func(context.Context, string) error { + return errors.New("guest shutdown failed") } now := time.Now().UTC()