a73x

a789f2ce

fix(agent): a backend signals only the processes it started, and says when it cannot

a73x   2026-08-06 09:12

Commit message
fix(agent): a backend signals only the processes it started, and says when it cannot

A pid is evidence only within the boot that wrote it. Agent state survives a
host reboot, pids are recycled, and reconcile reaches a reaped VM's Shutdown
before any boot-ID reasoning runs — so a stale pidfile meant the agent sent
SIGTERM, then SIGKILL, to whatever process now holds that number. On a Linux
host the agent is root, so nothing refuses on its behalf.

internal/agent/pidfile records the host's boot identifier beside the pid and
hands back a pid only when the boots match, so both backends refuse rather than
signal, and a VM from an earlier boot reaps cleanly instead. A pidfile with no
boot identifier is still treated as ours: refusing would read every guest
inherited across an agent upgrade as lost and boot a second hypervisor onto a
live disk, which is worse than the window it closes.

Destroy now reports a kill the kernel refused. Reconcile deletes a VM's record
on the strength of that nil, so promising success stranded a live hypervisor
holding an unlinked disk with nothing left to find it by.
docs/assumptions.md
Old New
@@ -37,6 +37,9 @@ running anything you care about on a machine that reboots.
37 **Proven** 2026-08-01 on bare metal: a host rebooted, and within a minute of 37 **Proven** 2026-08-01 on bare metal: a host rebooted, and within a minute of
38 boot a persistent guest was running again on the same address it held before, 38 boot a persistent guest was running again on the same address it held before,
39 its disk intact—while an ephemeral guest on the same host was failed by design. 39 its disk intact—while an ephemeral guest on the same host was failed by design.
40 Re-proven 2026-08-02 on the nested gate host after pidfiles became boot-scoped:
41 same address, same disk, and the agent correctly read the pre-reboot pidfile as
42 naming nothing of its own.
40 43
41 ### An image we cannot name is safe to treat as raw 44 ### An image we cannot name is safe to treat as raw
42 45
@@ -73,9 +76,11 @@ not viability.
73 vmnet's DHCP assigns after the guest boots, and the lease is readable without 76 vmnet's DHCP assigns after the guest boots, and the lease is readable without
74 privileges. Underpins discovering the address rather than assigning it, and 77 privileges. Underpins discovering the address rather than assigning it, and
75 rejecting both bootpd reservations and a userspace network stack. 78 rejecting both bootpd reservations and a userspace network stack.
76 **Partly proven**: lease read-back spiked on real hardware; the vfkit 79 **Proven** 2026-08-02 on an M1: an arm64 guest leased 192.168.64.9 from vmnet's
77 provisioner now consumes it, keyed on the VM's deterministic MAC, but no guest 80 bootpd, the agent found that lease under the VM's deterministic MAC, recorded
78 has been reached at an address discovered this way. 81 it, and `eitri ssh <name>` reached the guest through the gate on the strength of
82 it. The guest's `DHCPv4 Client ID` is the MAC, which is what makes the lookup
83 find the live lease rather than a stale one — see the DHCP-identifier entry.
79 84
80 ### Nobody depends on bring-your-own qcow2 85 ### Nobody depends on bring-your-own qcow2
81 86
@@ -275,3 +280,31 @@ bootloader with `createVariableStore`, root disk before seed, NAT'd NIC with our
275 MAC, a pty serial, rng) and round-trips back to the same command line. What that 280 MAC, a pty serial, rng) and round-trips back to the same command line. What that
276 does NOT prove is that the framework then boots it — only that vfkit will not 281 does NOT prove is that the framework then boots it — only that vfkit will not
277 reject it at the door. 282 reject it at the door.
283
284 ### A pid is evidence only within the boot that wrote it
285
286 Every backend records a guest's process id and later signals it — to stop it, to
287 kill it, to ask whether it is alive. The state directory outlives the host and
288 pids are recycled, so a pidfile written before the last boot names whatever now
289 holds that number. Underpins recording the host's boot identifier beside the pid
290 and refusing to signal across a mismatch, rather than trusting reconcile's own
291 boot-ID guard, which does not cover the path that matters: `reapVM` reaches
292 `Shutdown` before any of that reasoning runs, and on Linux the agent is root, so
293 nothing would refuse the signal on its behalf.
294 **Proven** 2026-08-02 on the nested gate host: a persistent guest was booted
295 (pid 20774, boot `1c7730e5…`), the host was rebooted (boot `676cac05…`), and the
296 agent read the stale pidfile as naming nothing of its own, restarted the guest
297 on its old address, and rewrote the pidfile against the new boot. The one thing
298 still unproven is the collision itself — pid 20774 happened to be unused after
299 the reboot, so no stranger was there to be spared.
300
301 ### An agent upgrade must not disown the guests it inherits
302
303 A pidfile with no boot identifier is treated as this agent's own, because it can
304 only have been written by the binary being replaced. Underpins rolling a
305 boot-scoped agent to a fleet whose guests were started by one that was not.
306 **Unproven in the field**: covered by unit tests on both backends, but the fleet
307 roll that would exercise it has not happened. The failure it guards against is
308 severe and quiet — refusing would read every live guest as lost and boot a
309 second hypervisor onto its disk — so the first roll should watch for a VM going
310 from running to creating rather than trusting the tests.
docs/shape.html
Old New
@@ -145,6 +145,8 @@
145 "synopsis": "Package cloudhv manages one cloud-hypervisor process per VM.", 145 "synopsis": "Package cloudhv manages one cloud-hypervisor process per VM.",
146 "imports": [ 146 "imports": [
147 "internal/agent/exec", 147 "internal/agent/exec",
148 "internal/agent/hostinfo",
149 "internal/agent/pidfile",
148 "internal/agent/state" 150 "internal/agent/state"
149 ] 151 ]
150 }, 152 },
@@ -200,6 +202,12 @@
200 ] 202 ]
201 }, 203 },
202 { 204 {
205 "importPath": "internal/agent/pidfile",
206 "plane": "data",
207 "synopsis": "Package pidfile records which process a VM's guest runs as, in a form that survives a reboot without lying about it.",
208 "imports": []
209 },
210 {
203 "importPath": "internal/agent/reconcile", 211 "importPath": "internal/agent/reconcile",
204 "plane": "data", 212 "plane": "data",
205 "synopsis": "Package reconcile implements the agent's level-triggered reconcile loop.", 213 "synopsis": "Package reconcile implements the agent's level-triggered reconcile loop.",
@@ -275,6 +283,7 @@
275 "imports": [ 283 "imports": [
276 "internal/agent/exec", 284 "internal/agent/exec",
277 "internal/agent/hostinfo", 285 "internal/agent/hostinfo",
286 "internal/agent/pidfile",
278 "internal/agent/state" 287 "internal/agent/state"
279 ] 288 ]
280 }, 289 },
docs/shape.json
Old New
@@ -94,6 +94,8 @@
94 "synopsis": "Package cloudhv manages one cloud-hypervisor process per VM.", 94 "synopsis": "Package cloudhv manages one cloud-hypervisor process per VM.",
95 "imports": [ 95 "imports": [
96 "internal/agent/exec", 96 "internal/agent/exec",
97 "internal/agent/hostinfo",
98 "internal/agent/pidfile",
97 "internal/agent/state" 99 "internal/agent/state"
98 ] 100 ]
99 }, 101 },
@@ -149,6 +151,12 @@
149 ] 151 ]
150 }, 152 },
151 { 153 {
154 "importPath": "internal/agent/pidfile",
155 "plane": "data",
156 "synopsis": "Package pidfile records which process a VM's guest runs as, in a form that survives a reboot without lying about it.",
157 "imports": []
158 },
159 {
152 "importPath": "internal/agent/reconcile", 160 "importPath": "internal/agent/reconcile",
153 "plane": "data", 161 "plane": "data",
154 "synopsis": "Package reconcile implements the agent's level-triggered reconcile loop.", 162 "synopsis": "Package reconcile implements the agent's level-triggered reconcile loop.",
@@ -224,6 +232,7 @@
224 "imports": [ 232 "imports": [
225 "internal/agent/exec", 233 "internal/agent/exec",
226 "internal/agent/hostinfo", 234 "internal/agent/hostinfo",
235 "internal/agent/pidfile",
227 "internal/agent/state" 236 "internal/agent/state"
228 ] 237 ]
229 }, 238 },
internal/agent/cloudhv/cloudhv.go
Old New
@@ -12,12 +12,13 @@ import (
12 "os" 12 "os"
13 "os/exec" 13 "os/exec"
14 "path/filepath" 14 "path/filepath"
15 "strconv"
16 "strings" 15 "strings"
17 "syscall" 16 "syscall"
18 "time" 17 "time"
19 18
20 agentexec "github.com/a73x/eitri/internal/agent/exec" 19 agentexec "github.com/a73x/eitri/internal/agent/exec"
20 "github.com/a73x/eitri/internal/agent/hostinfo"
21 "github.com/a73x/eitri/internal/agent/pidfile"
21 "github.com/a73x/eitri/internal/agent/state" 22 "github.com/a73x/eitri/internal/agent/state"
22 ) 23 )
23 24
@@ -65,6 +66,17 @@ type Provisioner struct {
65 run agentexec.Runner 66 run agentexec.Runner
66 net Network 67 net Network
67 68
69 // bootID identifies the host boot a VM's process was started in. It is the
70 // same reader reconcile's own boot-ID guard uses, so the two agree by
71 // construction. Injected so a test can move the host to a later boot
72 // without rebooting the box running it.
73 bootID func() string
74
75 // signal delivers a signal to a pid. Injected because the branch that
76 // matters — a kill the kernel refuses — cannot otherwise be reached without
77 // depending on what the box running the tests is allowed to signal.
78 signal func(pid int, sig syscall.Signal) error
79
68 // Pumps receives serial-pump lifecycle calls at Boot/Kill. nil = no-op. 80 // Pumps receives serial-pump lifecycle calls at Boot/Kill. nil = no-op.
69 Pumps PumpHooks 81 Pumps PumpHooks
70 } 82 }
@@ -73,7 +85,11 @@ type Provisioner struct {
73 // (buildArgs) are needed; net must not be, since a VM's network attachment is 85 // (buildArgs) are needed; net must not be, since a VM's network attachment is
74 // part of booting it. 86 // part of booting it.
75 func New(st *state.Store, chBin, firmware string, run agentexec.Runner, net Network) *Provisioner { 87 func New(st *state.Store, chBin, firmware string, run agentexec.Runner, net Network) *Provisioner {
76 return &Provisioner{st: st, chBin: chBin, firmware: firmware, run: run, net: net} 88 return &Provisioner{
89 st: st, chBin: chBin, firmware: firmware, run: run, net: net,
90 bootID: hostinfo.BootID,
91 signal: syscall.Kill,
92 }
77 } 93 }
78 94
79 // BootstrapDest maps a --ch-bin value to the filesystem path bootstrap may 95 // BootstrapDest maps a --ch-bin value to the filesystem path bootstrap may
@@ -268,15 +284,16 @@ func (p *Provisioner) Boot(ctx context.Context, vmID string, spec state.VMSpec)
268 // Close the log fd in the parent; the child has its own copy. 284 // Close the log fd in the parent; the child has its own copy.
269 _ = chLog.Close() 285 _ = chLog.Close()
270 286
271 // Write PID file so Running/Shutdown/Destroy can find the process later. 287 // Record the process so Running/Shutdown/Destroy can find it later — with
272 pidData := []byte(strconv.Itoa(cmd.Process.Pid)) 288 // the host boot it belongs to, because a pid alone stops meaning anything
273 if err := os.WriteFile(p.pidPath(vmID), pidData, 0o600); err != nil { 289 // the moment the host reboots (see ownedPID).
290 if err := pidfile.Write(p.pidPath(vmID), cmd.Process.Pid, p.bootID()); err != nil {
274 // Best effort — kill the orphan if we can't track it, and Wait to reap it 291 // Best effort — kill the orphan if we can't track it, and Wait to reap it
275 // (the async reaper below is not started on this path, so without Wait the 292 // (the async reaper below is not started on this path, so without Wait the
276 // killed child would linger as a zombie for the agent's whole lifetime). 293 // killed child would linger as a zombie for the agent's whole lifetime).
277 _ = cmd.Process.Kill() 294 _ = cmd.Process.Kill()
278 _ = cmd.Wait() 295 _ = cmd.Wait()
279 return fmt.Errorf("write pidfile %s: %w", vmID, err) 296 return fmt.Errorf("track cloud-hypervisor %s: %w", vmID, err)
280 } 297 }
281 298
282 // Reap child asynchronously; ignore exit error (VM may be killed intentionally). 299 // Reap child asynchronously; ignore exit error (VM may be killed intentionally).
@@ -305,31 +322,30 @@ func (p *Provisioner) attachNet(ctx context.Context, vmID string) error {
305 return p.net.CreateTap(ctx, vmID, ip) 322 return p.net.CreateTap(ctx, vmID, ip)
306 } 323 }
307 324
308 // readPID reads the PID file for vmID and returns the PID, or 0 on error. 325 // ownedPID is the pid of the cloud-hypervisor process THIS agent started for
309 func (p *Provisioner) readPID(vmID string) int { 326 // vmID, or 0 when there is none it may signal. The rule — and the reason a pid
310 raw, err := os.ReadFile(p.pidPath(vmID)) 327 // alone is not evidence after a reboot — lives in internal/agent/pidfile,
311 if err != nil { 328 // shared with the other backend so the two cannot disagree about whose process
312 return 0 329 // they are about to kill.
313 } 330 func (p *Provisioner) ownedPID(vmID string) int {
314 pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) 331 return pidfile.Owned(p.pidPath(vmID), p.bootID())
315 if err != nil {
316 return 0
317 }
318 return pid
319 } 332 }
320 333
321 // Running reports whether the cloud-hypervisor process for vmID is still alive. 334 // Running reports whether the cloud-hypervisor process for vmID is still alive,
322 // It reads the PID file and sends signal 0 (existence check). 335 // by pidfile and signal 0.
323 // 336 //
324 // PID-liveness only; a recycled PID after host reboot can false-positive. 337 // PID-liveness within this host boot. A pidfile written before the last boot
325 // The reconcile engine's boot-ID check (lost = boot ID changed) is the 338 // reports not-running outright (see ownedPID), so the recycled-pid
326 // authoritative reboot guard — do not trust Running() standalone across reboots. 339 // false-positive this used to concede is gone; within one boot, a pid this
340 // agent wrote cannot have been recycled while its process is alive. reconcile's
341 // own boot-ID check still decides what a lost VM means — this only decides
342 // which process, if any, is ours to ask about.
327 func (p *Provisioner) Running(vmID string) bool { 343 func (p *Provisioner) Running(vmID string) bool {
328 pid := p.readPID(vmID) 344 pid := p.ownedPID(vmID)
329 if pid == 0 { 345 if pid == 0 {
330 return false 346 return false
331 } 347 }
332 return syscall.Kill(pid, 0) == nil 348 return p.signal(pid, 0) == nil
333 } 349 }
334 350
335 // Address returns the address reserved for vmID on the bridge, or "" when the 351 // Address returns the address reserved for vmID on the bridge, or "" when the
@@ -378,11 +394,11 @@ func (p *Provisioner) Shutdown(ctx context.Context, vmID string) error {
378 394
379 // sigterm sends SIGTERM to the process identified by vmID's PID file. 395 // sigterm sends SIGTERM to the process identified by vmID's PID file.
380 func (p *Provisioner) sigterm(vmID string) error { 396 func (p *Provisioner) sigterm(vmID string) error {
381 pid := p.readPID(vmID) 397 pid := p.ownedPID(vmID)
382 if pid == 0 { 398 if pid == 0 {
383 return nil // already gone 399 return nil // already gone
384 } 400 }
385 if err := syscall.Kill(pid, syscall.SIGTERM); err != nil && err != syscall.ESRCH { 401 if err := p.signal(pid, syscall.SIGTERM); err != nil && err != syscall.ESRCH {
386 return fmt.Errorf("SIGTERM %s (pid %d): %w", vmID, pid, err) 402 return fmt.Errorf("SIGTERM %s (pid %d): %w", vmID, pid, err)
387 } 403 }
388 return nil 404 return nil
@@ -392,23 +408,27 @@ func (p *Provisioner) sigterm(vmID string) error {
392 // cloud-hypervisor process, its serial pump and socket, and its tap device with 408 // cloud-hypervisor process, its serial pump and socket, and its tap device with
393 // the DHCP reservation the tap carries. 409 // the DHCP reservation the tap carries.
394 // 410 //
395 // The kill's error is deliberately not propagated. SIGKILL against a pidfile is 411 // Both failures reach reconcile, because returning nil is a promise that
396 // idempotent, and the failures it can report (the process is gone, or is not 412 // nothing is left to reap and reconcile deletes the VM's record on the strength
397 // ours) are not ones a retry fixes. Releasing the tap IS retryable — `ip link 413 // of it. Releasing the tap is the retryable one — `ip link del` under an expired
398 // del` under an expired pass context fails, and an orphaned eit-XXXXXXXX device 414 // pass context fails, and an orphaned eit-XXXXXXXX device has nothing left to
399 // has nothing left to reap it (the agent's EnsureBridge does not sweep orphan 415 // reap it, since EnsureBridge does not sweep them. A kill the kernel refused is
400 // taps) — so that is the error that reaches reconcile and keeps the VM's record 416 // rarer and worse: the record and the VM directory would go while the guest
401 // for another tick. 417 // kept running against unlinked files, leaving a hypervisor on the host that
418 // nothing on it or in the fleet can name.
402 func (p *Provisioner) Destroy(ctx context.Context, vmID string) error { 419 func (p *Provisioner) Destroy(ctx context.Context, vmID string) error {
403 p.kill(vmID) 420 killErr := p.kill(vmID)
404 return p.net.DeleteTap(ctx, vmID) 421 if err := p.net.DeleteTap(ctx, vmID); err != nil {
422 return err
423 }
424 return killErr
405 } 425 }
406 426
407 // kill SIGKILLs the cloud-hypervisor process for vmID, stops its serial pump, 427 // kill SIGKILLs the cloud-hypervisor process for vmID, stops its serial pump,
408 // and removes the PID file and serial socket. It takes no context: killing a 428 // and removes the PID file and serial socket. It takes no context: killing a
409 // process by pidfile is a syscall, and pretending otherwise made callers think 429 // process by pidfile is a syscall, and pretending otherwise made callers think
410 // a cancelled context could skip a teardown. 430 // a cancelled context could skip a teardown.
411 func (p *Provisioner) kill(vmID string) { 431 func (p *Provisioner) kill(vmID string) error {
412 // Pump teardown + serial-socket removal come BEFORE the SIGKILL: a pump 432 // Pump teardown + serial-socket removal come BEFORE the SIGKILL: a pump
413 // leaked past a failed kill would dial a deleted path forever. ch.sock is 433 // leaked past a failed kill would dial a deleted path forever. ch.sock is
414 // deliberately left in place — the next Boot clears a stale API socket 434 // deliberately left in place — the next Boot clears a stale API socket
@@ -417,14 +437,16 @@ func (p *Provisioner) kill(vmID string) {
417 p.Pumps.Stop(vmID) 437 p.Pumps.Stop(vmID)
418 } 438 }
419 _ = os.Remove(p.st.SerialSocketPath(vmID)) 439 _ = os.Remove(p.st.SerialSocketPath(vmID))
420 pid := p.readPID(vmID) 440 pid := p.ownedPID(vmID)
421 if pid != 0 { 441 if pid != 0 {
422 if err := syscall.Kill(pid, syscall.SIGKILL); err != nil && err != syscall.ESRCH { 442 if err := p.signal(pid, syscall.SIGKILL); err != nil && err != syscall.ESRCH {
423 // Leave the pidfile: a SIGKILL that failed with anything but 443 // Keep the pidfile AND say so. A SIGKILL that failed with anything
424 // ESRCH has not proved the process gone, and deleting its only 444 // but ESRCH has not proved the process gone, so the record is the
425 // record would claim that it is. 445 // only handle left on it — and an error is what stops reconcile
426 return 446 // deleting the directory that record lives in.
447 return fmt.Errorf("SIGKILL %s (pid %d): %w", vmID, pid, err)
427 } 448 }
428 } 449 }
429 _ = os.Remove(p.pidPath(vmID)) 450 _ = os.Remove(p.pidPath(vmID))
451 return nil
430 } 452 }
internal/agent/cloudhv/cloudhv_test.go
Old New
@@ -10,9 +10,11 @@ import (
10 "path/filepath" 10 "path/filepath"
11 "strings" 11 "strings"
12 "sync/atomic" 12 "sync/atomic"
13 "syscall"
13 "testing" 14 "testing"
14 "time" 15 "time"
15 16
17 "github.com/a73x/eitri/internal/agent/pidfile"
16 "github.com/a73x/eitri/internal/agent/state" 18 "github.com/a73x/eitri/internal/agent/state"
17 "github.com/stretchr/testify/assert" 19 "github.com/stretchr/testify/assert"
18 "github.com/stretchr/testify/require" 20 "github.com/stretchr/testify/require"
@@ -585,3 +587,82 @@ func TestDestroyReleasesTheNetworkAndRetriesOnFailure(t *testing.T) {
585 require.NoError(t, p.Destroy(context.Background(), vmID)) 587 require.NoError(t, p.Destroy(context.Background(), vmID))
586 assert.Empty(t, p.Address(vmID), "destroy releases the VM's address") 588 assert.Empty(t, p.Address(vmID), "destroy releases the VM's address")
587 } 589 }
590
591 // TestDestroyReportsAKillTheKernelRefused pins the other half of the teardown
592 // contract. A nil from Destroy is the backend's promise that nothing is left to
593 // reap, and reconcile deletes the VM's record and its whole directory on the
594 // strength of it. If SIGKILL was refused, the guest is still running — and the
595 // pidfile is the only handle anything has on it.
596 func TestDestroyReportsAKillTheKernelRefused(t *testing.T) {
597 st, err := state.Open(t.TempDir())
598 require.NoError(t, err)
599 vmID := "vm-refused"
600 require.NoError(t, os.MkdirAll(st.VMDir(vmID), 0o755))
601 p := New(st, "ch", "fw", nil, newFakeNet())
602 require.NoError(t, pidfile.Write(p.pidPath(vmID), 4242, p.bootID()))
603 p.signal = func(int, syscall.Signal) error { return syscall.EPERM }
604
605 err = p.Destroy(context.Background(), vmID)
606
607 require.Error(t, err, "a refused kill must reach reconcile, or the record is deleted under a live guest")
608 assert.Contains(t, err.Error(), "SIGKILL")
609 _, statErr := os.Stat(p.pidPath(vmID))
610 assert.NoError(t, statErr, "the pidfile is the only handle left on that process")
611 }
612
613 func TestDestroyOfAProcessAlreadyGoneIsDone(t *testing.T) {
614 st, err := state.Open(t.TempDir())
615 require.NoError(t, err)
616 vmID := "vm-gone"
617 require.NoError(t, os.MkdirAll(st.VMDir(vmID), 0o755))
618 p := New(st, "ch", "fw", nil, newFakeNet())
619 require.NoError(t, pidfile.Write(p.pidPath(vmID), 4242, p.bootID()))
620 p.signal = func(int, syscall.Signal) error { return syscall.ESRCH }
621
622 // ESRCH is the answer that proves the guest gone, which is exactly what
623 // reconcile needs to hear before it deletes the record.
624 require.NoError(t, p.Destroy(context.Background(), vmID))
625 _, statErr := os.Stat(p.pidPath(vmID))
626 assert.True(t, os.IsNotExist(statErr))
627 }
628
629 // TestAPidFromAnEarlierBootIsNeverSignalled is the reboot guard. The state
630 // directory outlives the host and Linux recycles pids, so a pidfile written
631 // before the last boot names whatever now holds that number — and this agent
632 // runs as root, so nothing would refuse the signal on its behalf.
633 func TestAPidFromAnEarlierBootIsNeverSignalled(t *testing.T) {
634 st, err := state.Open(t.TempDir())
635 require.NoError(t, err)
636 vmID := "vm-rebooted"
637 require.NoError(t, os.MkdirAll(st.VMDir(vmID), 0o755))
638 p := New(st, "ch", "fw", nil, newFakeNet())
639 require.NoError(t, pidfile.Write(p.pidPath(vmID), 4242, "boot-before"))
640 p.bootID = func() string { return "boot-after" }
641 var signalled []syscall.Signal
642 p.signal = func(_ int, sig syscall.Signal) error {
643 signalled = append(signalled, sig)
644 return nil
645 }
646
647 assert.False(t, p.Running(vmID), "a process from a previous boot is not this VM")
648 require.NoError(t, p.Shutdown(context.Background(), vmID))
649 require.NoError(t, p.Destroy(context.Background(), vmID))
650
651 assert.Empty(t, signalled, "reapVM reaches Shutdown before any boot-ID reasoning — the refusal has to be here")
652 }
653
654 // TestAPidfileWithoutABootIDIsStillOurs covers the rolling upgrade: every VM
655 // running under the agent being replaced has a pidfile in the old format.
656 // Refusing them would read every live guest as lost and boot a SECOND
657 // hypervisor onto its disk.
658 func TestAPidfileWithoutABootIDIsStillOurs(t *testing.T) {
659 st, err := state.Open(t.TempDir())
660 require.NoError(t, err)
661 vmID := "vm-upgraded"
662 require.NoError(t, os.MkdirAll(st.VMDir(vmID), 0o755))
663 p := New(st, "ch", "fw", nil, newFakeNet())
664 require.NoError(t, os.WriteFile(p.pidPath(vmID), []byte("4242\n"), 0o600))
665 p.signal = func(int, syscall.Signal) error { return nil }
666
667 assert.True(t, p.Running(vmID), "an agent upgrade must not orphan the guests it inherits")
668 }
internal/agent/pidfile/pidfile.go
Old New
@@ -0,0 +1,68 @@
1 // Package pidfile records which process a VM's guest runs as, in a form that
2 // survives a reboot without lying about it.
3 //
4 // A pid on its own is not evidence. The agent's state directory outlives the
5 // host, pids are recycled out of a small space, and every backend's teardown
6 // path reaches for the recorded number and signals it — so a pidfile written
7 // before the last boot names whatever now happens to hold that number, and
8 // killing a VM means killing a stranger. Writing the host's boot identifier
9 // beside the pid is what turns "a process had this number once" into "this
10 // process is ours".
11 //
12 // The package is mechanism, not policy: it does not name the file, decide what
13 // a failed signal means, or read the boot id itself. Each backend keeps those.
14 // What it owns is the one rule both of them have to agree on, so that a second
15 // VMM driver cannot quietly disagree with the first about whose process it is
16 // about to kill.
17 package pidfile
18
19 import (
20 "fmt"
21 "os"
22 "strconv"
23 "strings"
24 )
25
26 // mode is the pidfile's permissions: readable by the agent alone, like every
27 // other per-VM artifact in the state directory.
28 const mode = 0o600
29
30 // Write records pid and the boot it was started in at path.
31 func Write(path string, pid int, bootID string) error {
32 if err := os.WriteFile(path, []byte(strconv.Itoa(pid)+"\n"+bootID+"\n"), mode); err != nil {
33 return fmt.Errorf("write pidfile %s: %w", path, err)
34 }
35 return nil
36 }
37
38 // Owned returns the pid recorded at path if this agent may signal it, and 0
39 // otherwise — no file, an unreadable one, or one written before bootID.
40 //
41 // Zero is deliberately the same answer for "there is no process" and "there is
42 // a process but it is not ours to touch", because callers do the same thing
43 // with both: report not-running, and signal nothing.
44 //
45 // A pidfile carrying no boot id at all is treated as OURS, and that is the
46 // load-bearing choice here. It can only have been written by an agent that
47 // predates this format, which is to say by the binary being replaced during a
48 // rolling upgrade. The cautious-looking answer — refuse, report not running —
49 // is the ruinous one: reconcile reads every live guest as lost and boots a
50 // SECOND hypervisor onto the same disk image, and two of them writing one disk
51 // corrupts the guest. Preserving running guests across an agent upgrade is a
52 // property this fleet relies on. The exposure is one boot wide: the next Boot
53 // rewrites the pidfile in this format.
54 func Owned(path, bootID string) int {
55 raw, err := os.ReadFile(path)
56 if err != nil {
57 return 0
58 }
59 line, rest, _ := strings.Cut(string(raw), "\n")
60 pid, err := strconv.Atoi(strings.TrimSpace(line))
61 if err != nil {
62 return 0
63 }
64 if boot := strings.TrimSpace(rest); boot != "" && boot != bootID {
65 return 0
66 }
67 return pid
68 }
internal/agent/pidfile/pidfile_test.go
Old New
@@ -0,0 +1,79 @@
1 package pidfile
2
3 import (
4 "os"
5 "path/filepath"
6 "testing"
7
8 "github.com/stretchr/testify/assert"
9 "github.com/stretchr/testify/require"
10 )
11
12 func tmpPath(t *testing.T) string {
13 t.Helper()
14 return filepath.Join(t.TempDir(), "vm.pid")
15 }
16
17 func TestOwnedReturnsAPidWrittenInThisBoot(t *testing.T) {
18 path := tmpPath(t)
19 require.NoError(t, Write(path, 4242, "boot-a"))
20
21 assert.Equal(t, 4242, Owned(path, "boot-a"))
22 }
23
24 func TestOwnedRefusesAPidFromAnEarlierBoot(t *testing.T) {
25 path := tmpPath(t)
26 require.NoError(t, Write(path, 4242, "boot-a"))
27
28 // The state directory survives a reboot and pids are recycled, so this
29 // number now names whatever happens to hold it. Signalling it would kill a
30 // stranger; reporting it running would leave a dead guest looking alive.
31 assert.Zero(t, Owned(path, "boot-b"))
32 }
33
34 func TestOwnedAcceptsAPidfileWithNoBootID(t *testing.T) {
35 path := tmpPath(t)
36 // What the agent being replaced during a rolling upgrade left behind.
37 require.NoError(t, os.WriteFile(path, []byte("4242\n"), mode))
38
39 // Refusing here would read every live guest as lost and boot a second
40 // hypervisor onto its disk — worse than the recycled-pid window this
41 // format closes, and the reason the answer is "ours".
42 assert.Equal(t, 4242, Owned(path, "boot-a"))
43 }
44
45 func TestOwnedIsZeroWhenThereIsNothingToSignal(t *testing.T) {
46 for name, write := range map[string]func(path string){
47 "no file": func(string) {},
48 "empty": func(p string) { require.NoError(t, os.WriteFile(p, nil, mode)) },
49 "not a pid": func(p string) { require.NoError(t, os.WriteFile(p, []byte("fnord\nboot-a\n"), mode)) },
50 "blank first": func(p string) { require.NoError(t, os.WriteFile(p, []byte("\nboot-a\n"), mode)) },
51 } {
52 t.Run(name, func(t *testing.T) {
53 path := tmpPath(t)
54 write(path)
55
56 // Zero is "signal nothing", and every caller reads it that way.
57 assert.Zero(t, Owned(path, "boot-a"))
58 })
59 }
60 }
61
62 func TestWriteIsReadBackByOwned(t *testing.T) {
63 path := tmpPath(t)
64 require.NoError(t, Write(path, 1, ""))
65
66 // An agent whose host cannot report a boot id (hostinfo is best-effort and
67 // answers "" on failure) writes an empty one, which reads back as the
68 // legacy shape — degraded to the old behaviour, never to a refusal.
69 assert.Equal(t, 1, Owned(path, "boot-a"))
70 }
71
72 func TestWriteReportsAPathItCannotUse(t *testing.T) {
73 err := Write(filepath.Join(t.TempDir(), "no-such-dir", "vm.pid"), 1, "boot-a")
74
75 // Boot treats this as fatal and kills the process it can no longer track,
76 // so the error has to arrive rather than being swallowed.
77 require.Error(t, err)
78 assert.Contains(t, err.Error(), "write pidfile")
79 }
internal/agent/vfkit/vfkit.go
Old New
@@ -32,6 +32,7 @@ import (
32 32
33 agentexec "github.com/a73x/eitri/internal/agent/exec" 33 agentexec "github.com/a73x/eitri/internal/agent/exec"
34 "github.com/a73x/eitri/internal/agent/hostinfo" 34 "github.com/a73x/eitri/internal/agent/hostinfo"
35 "github.com/a73x/eitri/internal/agent/pidfile"
35 "github.com/a73x/eitri/internal/agent/state" 36 "github.com/a73x/eitri/internal/agent/state"
36 ) 37 )
37 38
@@ -341,12 +342,12 @@ func (p *Provisioner) Boot(_ context.Context, vmID string, spec state.VMSpec) er
341 } 342 }
342 _ = vfLog.Close() // the child holds its own copy 343 _ = vfLog.Close() // the child holds its own copy
343 344
344 if err := os.WriteFile(p.pidPath(vmID), p.pidRecord(cmd.Process.Pid), 0o600); err != nil { 345 if err := pidfile.Write(p.pidPath(vmID), cmd.Process.Pid, p.bootID()); err != nil {
345 // Best effort: kill the orphan we can no longer track, and Wait to reap 346 // Best effort: kill the orphan we can no longer track, and Wait to reap
346 // it — the async reaper below is not started on this path. 347 // it — the async reaper below is not started on this path.
347 _ = cmd.Process.Kill() 348 _ = cmd.Process.Kill()
348 _ = cmd.Wait() 349 _ = cmd.Wait()
349 return fmt.Errorf("write pidfile %s: %w", vmID, err) 350 return fmt.Errorf("track vfkit %s: %w", vmID, err)
350 } 351 }
351 352
352 go func() { _ = cmd.Wait() }() // reap; exit status is not ours to judge 353 go func() { _ = cmd.Wait() }() // reap; exit status is not ours to judge
@@ -361,45 +362,13 @@ func (p *Provisioner) Boot(_ context.Context, vmID string, spec state.VMSpec) er
361 return nil 362 return nil
362 } 363 }
363 364
364 // pidRecord renders the pidfile: the process id, and the host boot it was 365 // ownedPID is the pid of the vfkit process THIS agent started for vmID, or 0
365 // started in. The boot id is there because the pid alone is not evidence of 366 // when there is none it may signal. The rule — and the reason a pid alone is
366 // anything after a reboot — see ownedPID. 367 // not evidence after a reboot — lives in internal/agent/pidfile, shared with
367 func (p *Provisioner) pidRecord(pid int) []byte { 368 // the other backend so the two cannot disagree about whose process they are
368 return []byte(strconv.Itoa(pid) + "\n" + p.bootID() + "\n") 369 // about to kill.
369 }
370
371 // ownedPID returns the pid of the vfkit process THIS agent started for vmID, or
372 // 0 when there is none it may signal.
373 //
374 // The boot id is what makes that a real answer rather than a hope. The state
375 // directory survives a reboot, macOS recycles pids out of a small space, and
376 // this backend's host is a laptop that reboots most days — so a pidfile written
377 // before the last boot names whatever now happens to hold that number. Reading
378 // it as ours makes Running() report a dead VM as up, and, on the reap path,
379 // SIGTERMs and then SIGKILLs an unrelated process on the user's machine.
380 // reconcile's own boot-id guard does not cover this: reapVM reaches Shutdown
381 // before any of that reasoning runs.
382 //
383 // A pidfile with no boot id at all is treated as ours. It can only have been
384 // written by an agent that predates this format, and the cheaper-looking answer
385 // — refuse, report not running — is the more expensive one: reconcile reads a
386 // live VM as lost and boots a SECOND vfkit onto the same disk image, and two
387 // hypervisors writing one disk corrupt the guest. The window is transitional,
388 // since the next Boot rewrites the pidfile in this format.
389 func (p *Provisioner) ownedPID(vmID string) int { 370 func (p *Provisioner) ownedPID(vmID string) int {
390 raw, err := os.ReadFile(p.pidPath(vmID)) 371 return pidfile.Owned(p.pidPath(vmID), p.bootID())
391 if err != nil {
392 return 0
393 }
394 line, rest, _ := strings.Cut(string(raw), "\n")
395 pid, err := strconv.Atoi(strings.TrimSpace(line))
396 if err != nil {
397 return 0
398 }
399 if boot := strings.TrimSpace(rest); boot != "" && boot != p.bootID() {
400 return 0
401 }
402 return pid
403 } 372 }
404 373
405 // Running reports whether the vfkit process for vmID is still alive, by 374 // Running reports whether the vfkit process for vmID is still alive, by
scripts/coverage.sh
Old New
@@ -23,7 +23,8 @@ declare -A FLOOR=(
23 [internal/agent/hostinfo]=95 23 [internal/agent/hostinfo]=95
24 [internal/agent/imagecache]=72 24 [internal/agent/imagecache]=72
25 [internal/agent/netenv]=76 25 [internal/agent/netenv]=76
26 [internal/agent/cloudhv]=40 26 [internal/agent/cloudhv]=80
27 [internal/agent/pidfile]=95
27 [internal/agent/vfkit]=85 28 [internal/agent/vfkit]=85
28 [internal/agent/syncclient]=74 29 [internal/agent/syncclient]=74
29 [internal/server/api]=76 30 [internal/server/api]=76