diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c3fd40..0091047 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,79 @@ # felhom-agent — Changelog +## v0.107.0 — F-REBOOT + F-LEAK: the agent's authority over guest lifecycle (2026-07-28) + +Two Campaign 8 findings, both about the agent being unable to act on a guest it owns. + +### F-REBOOT — a guest rebooted mid-backup never came back + +Fault 11: the backup SUCCEEDED and the guest was found `stopped` with 0 containers, no lock, nothing +retrying — 9m47s of TOTAL appliance outage, every app down, and every alarm silent because nothing +was broken except that the box was off. The trigger is ordinary: backups run overnight and +`CONTEXT.md` records that the N100 needs BIOS AC-Power-Recovery *because power loss happens*. + +**Why the existing recovery missed it.** `RecoverStaleLockedGuests` already does +unlock → delete dangling snapshot → **start iff onboot**, and it is correct. It missed this by two +narrow gaps: its predicate acts only on a guest holding a stale vzdump lock +(`backup`/`snapshot-delete`) and fault 11's guest was stopped and **unlocked**; and it runs **once at +agent startup**, on the load-bearing invariant that a lock present then is stale by definition. A +guest that goes down while the agent is already up was never re-examined. + +**The fix** (`internal/localapi/guestpower.go`) closes exactly those two gaps: a periodic sweep that +acts on *should be running, is not, and is not locked*. + +`onboot` is the "should be running" signal, and it is deliberately **not invented**: it is already +the distinction `stalelock.go` uses for this same decision, it is 1 on customer guests and 0 on +scratch/golden, and it is the flag `pve-guests` itself consults at host boot — so the agent agrees +with the platform instead of keeping a private definition. The hub's desired-state `Run` is stronger +but hub-dependent; `onboot` keeps working on a box that has lost hub contact, which is when an +unattended appliance most needs to come back. + +**Guards, because the trap here is F-CRIT-1's shape.** A guest the operator deliberately stopped must +never be auto-started — fighting the operator makes maintenance impossible and is worse than the +outage. So: `onboot:0` ⇒ never touched; a LOCKED guest is left to the stale-lock path (its lock may +mean "a restore is writing my disks"); a guest with a vzdump genuinely in flight is left stopped (a +stop-mode backup stops it on purpose); unprovable ownership ⇒ act on nothing; and unconfirmable +backup state fails safe. + +**Bounded retry** (Scenario C): 3 attempts at 1m/2m/4m, then ERROR naming the guest and stop. A +healthy `pct start` of 9201 took ~25 s (observed twice), so even the first wait carries 2.4x headroom; +three attempts bound disruption at ~7 minutes — inside the 9m47s this fixes — and never loop forever. + +### F-LEAK — a failed restore-test could not destroy its own scratch + +`DELETE /nodes/x/lxc/990000` → **403 missing privilege VM.Allocate**. It is **pool membership, not +privsep**, and the live ACLs prove it: `VM.Allocate` is granted at `/pool/felhom` ONLY, never at `/`. +A successful restore's `--pool felhom` makes the guest a member and inheritable; a FAILED restore +never completes that, so the guest's own path resolves to `/` where the token holds nothing: + +| path | privileges the token has | +|---|---| +| `/vms/990005` (non-member scratch) | `Datastore.Audit, SDN.Use, Sys.Audit` — no VM.Allocate | +| `/pool/felhom` | full `FelhomAgentGuest` incl. `VM.Allocate` **and `Pool.Allocate`** | +| `/vms/9201` (pool member) | inherits the full set | + +**The fix needs NO new privilege.** `Pool.Allocate` is already held, so the teardown adopts the +stranded scratch into the pool and retries the destroy once, which then authorizes via +`/pool/felhom`. Nothing is widened; `PoolAddVMID` already existed for exactly this reason +("membership is what lets the pool-scoped token reach the guest next time"). + +**The security guard is `scratchAdoptAllowed` — two independent checks, both required:** the journal +entry must carry agent-created **scratch provenance**, AND the VMID must be inside the configured +**scratch band**. Either alone would be enough to fix the leak; both are present because this +function is the only thing between "clean up my own scratch" and "co-opt an arbitrary guest into the +pool and delete it". Adopting a customer guest would hand the token destroy rights over it — far +worse than a leaked scratch. + +### Also +Corrected the claim that the quiesce unquiesce is "guaranteed by defer" +(`felhom-controller/.../quiesce.go`, and the generic advice in `PROMPT-TEMPLATE.md`): fault 10 +established that a SIGKILL runs no deferred function, and the guarantee is the crash marker plus +`Recover()`. + +Files: `internal/localapi/{guestpower.go (new),server.go}`, `internal/reconcile/restoretest.go`, +`cmd/felhom-agent/main.go`, plus tests `internal/localapi/guestpower_test.go` and +`internal/reconcile/scratch_adopt_test.go`. No wire/contract change; no new PVE grant. + ## v0.106.0 — F-CRIT-2: a failed backup must not look like a fresh one (2026-07-28) Campaign 8 killed the PBS daemon mid-upload. PBS published the aborted upload into the storage diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 180f1d1..dccb354 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1057,6 +1057,12 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int // the dangling snapshot → start iff onboot). Runs BEFORE the backup loop starts, so a present // backup lock is stale by definition (guarded by a no-vzdump-running check; fail-safe otherwise). localSrv.RecoverStaleLockedGuests(ctx) + // F-REBOOT: the startup recovery above only covers a guest left LOCKED by an interrupted + // backup. A guest that simply ends up stopped-and-unlocked (a `pct reboot` whose shutdown + // half completed and whose start half never fired — Campaign 8 fault 11, 9m47s of total + // appliance outage with nothing retrying) needs a PERIODIC check. onboot is the "should be + // running" signal, so a deliberately stopped guest is never touched. + go localSrv.WatchGuestPower(ctx) go func() { errc <- localSrv.Run(ctx) }() } if lanLoop != nil { diff --git a/internal/localapi/guestpower.go b/internal/localapi/guestpower.go new file mode 100644 index 0000000..d59c364 --- /dev/null +++ b/internal/localapi/guestpower.go @@ -0,0 +1,238 @@ +package localapi + +import ( + "context" + "sync" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// F-REBOOT (Campaign 8 fault 11) — a guest that should be running and is not. +// +// THE OUTAGE THIS EXISTS TO KILL. A `pct reboot` issued while a vzdump was in flight completed its +// SHUTDOWN half and never issued the start. The guest was found `stopped` with 0 containers, no +// lock, and nothing retrying; it stayed down 9m47s until a human ran `pct start`. The backup itself +// SUCCEEDED — so every alarm the appliance has was silent, because nothing was broken except that +// the customer's entire appliance was off. +// +// WHY THE EXISTING RECOVERY MISSED IT. `RecoverStaleLockedGuests` (stalelock.go) already does +// unlock → delete dangling snapshot → start iff onboot, and it is CORRECT. It missed this by two +// gaps, both narrow: +// - its predicate acts only on a guest holding a stale vzdump lock (`backup`/`snapshot-delete`); +// fault 11's guest was stopped and UNLOCKED, so it returned early; +// - it runs ONCE at agent startup, on the load-bearing invariant that a backup lock present then +// is stale by definition. A guest that goes down while the agent is already up is never +// re-examined. +// +// This watchdog closes exactly those two gaps and nothing more: it is periodic, and it acts on +// "should be running, is not, and is not locked". +// +// ── THE TRAP, WHICH IS THE SAME SHAPE AS F-CRIT-1's ────────────────────────────────────────── +// +// A guest the operator deliberately stopped must NOT be auto-started. Fighting the operator makes +// maintenance impossible and is worse than the outage — the same over-correction that F-CRIT-1's fix +// had to avoid when it stopped whitelisting StateStopped. +// +// The distinction used is `onboot`, and it is deliberately NOT invented here: +// - it is ALREADY the distinction stalelock.go uses for exactly this decision +// (`if onboot && g.Status != "running"`), so the two paths cannot disagree; +// - it is 1 on customer guests and 0 on scratch/golden guests (agent v0.101.0 sets scratch to 0); +// - it is the same flag `pve-guests` itself consults at host boot, so the agent AGREES WITH THE +// PLATFORM rather than maintaining a second, private definition of "should be running". +// +// The hub's desired-state `Run` (internal/desired) is a stronger signal and is wired, but it is +// hub-dependent. `onboot` keeps working on a box that has lost hub contact — which is precisely when +// an unattended appliance most needs to come back up. + +const ( + // guestPowerInterval is how often the watchdog looks. Matches the guestnet watchdog's cadence so + // the two guest-facing sweeps stay in step, and is far below the 9m47s outage the finding recorded. + guestPowerInterval = 60 * time.Second + + // guestPowerMaxAttempts bounds the retry. A guest that will not start must not be started in a + // loop forever (Scenario C) — after this many failures the watchdog stops trying and raises it. + guestPowerMaxAttempts = 3 +) + +// guestPowerBackoff is the delay before each retry: 1m, 2m, 4m. +// +// Measured, not picked round: a healthy `pct start` of guest 9201 completed in ~25 s (observed twice +// on 2026-07-28), so even the first 1-minute wait carries 2.4x headroom over a normal start. Three +// attempts bound the disruption at roughly 7 minutes — inside the 9m47s outage this fixes — while +// never becoming an unbounded loop. +var guestPowerBackoff = []time.Duration{time.Minute, 2 * time.Minute, 4 * time.Minute} + +// guestPowerState is one guest's recovery attempt record. In-memory on purpose, like the R-88 +// breaker: an agent restart re-attempts immediately, which is the cheap direction to fail — a +// forgotten backoff costs one extra start attempt, whereas persisting it could carry a stale +// "this guest won't start" verdict across the restart that fixed it. +type guestPowerState struct { + attempts int + nextAt time.Time + raised bool // the give-up fault has already been raised for this run +} + +// WatchGuestPower runs the guest-power sweep every guestPowerInterval until ctx is done. No-op when +// the stale-lock controller is not wired (it supplies the ownership-proven guest list). +func (s *Server) WatchGuestPower(ctx context.Context) { + if s.staleLock == nil { + return + } + s.logger.Info("guest-power: watchdog started", "interval", guestPowerInterval.String(), + "max_attempts", guestPowerMaxAttempts) + t := time.NewTicker(guestPowerInterval) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + s.GuestPowerTick(ctx) + } + } +} + +// GuestPowerTick performs one sweep. Exported so a test (and a live check) can drive exactly one +// cycle instead of waiting on the ticker. +func (s *Server) GuestPowerTick(ctx context.Context) { + if s.staleLock == nil { + return + } + guests, err := s.staleLock.Guests(ctx) + if err != nil { + // Unknown ownership ⇒ do nothing. Never fall back to an unfiltered list: starting a + // co-tenant's guest would be worse than leaving ours down. + s.logger.Warn("guest-power: guest list unavailable — skipping sweep (ownership unproven)", "err", err) + return + } + for _, g := range guests { + if ctx.Err() != nil { + return + } + s.recoverOneStoppedGuest(ctx, g) + } +} + +// recoverOneStoppedGuest starts a single guest that should be running and is not. +func (s *Server) recoverOneStoppedGuest(ctx context.Context, g proxmox.Guest) { + if g.Status == "running" { + s.forgetGuestPower(g.VMID) // healthy again: clear any attempt history + return + } + + lock, onboot, err := s.staleLock.Lock(ctx, g.VMID) + if err != nil { + s.logger.Warn("guest-power: read guest config failed — skipping", "vmid", g.VMID, "err", err) + return + } + + // SCENARIO B — a deliberately stopped guest is left alone, forever. onboot:0 means the operator + // (or the golden-image provisioning) does not want this guest running. + if !onboot { + return + } + + // A locked guest belongs to another operation, mid-flight or stale. The stale-lock recovery owns + // that case and knows how to prove a lock is stale; this watchdog must not race it or start a + // guest whose lock means "a restore is writing my disks right now". + if lock != "" { + s.logger.Info("guest-power: guest is stopped but LOCKED — leaving it to the stale-lock path", + "vmid", g.VMID, "lock", lock) + return + } + + // Never start a guest while a vzdump is genuinely in flight for it — a stop-mode backup stops the + // guest ON PURPOSE and starting it underneath would corrupt the backup. Fail safe on doubt. + running, err := s.staleLock.BackupRunning(ctx, g.VMID) + if err != nil { + s.logger.Warn("guest-power: could not confirm no backup is running — NOT starting (fail-safe)", + "vmid", g.VMID, "err", err) + return + } + if running { + s.logger.Info("guest-power: a vzdump is in flight — leaving the guest stopped until it finishes", + "vmid", g.VMID) + return + } + + st, due := s.guestPowerDue(g.VMID) + if !due { + return + } + if st.attempts >= guestPowerMaxAttempts { + // SCENARIO C — bounded. Raise it ONCE and stop retrying; an infinite silent retry loop is the + // over-correction here, and a guest that has refused three starts needs a human, not a fourth. + if !st.raised { + s.markGuestPowerRaised(g.VMID) + s.logger.Error("guest-power: GIVING UP — guest should be running (onboot) but failed to start after repeated attempts; it needs operator attention", + "vmid", g.VMID, "attempts", st.attempts, "status", g.Status) + } + return + } + + s.logger.Warn("guest-power: guest should be running (onboot) but is stopped and unlocked — starting it", + "vmid", g.VMID, "status", g.Status, "attempt", st.attempts+1, "of", guestPowerMaxAttempts) + if err := s.staleLock.Start(ctx, g.VMID); err != nil { + s.noteGuestPowerFailure(g.VMID) + s.logger.Error("guest-power: start failed", "vmid", g.VMID, "attempt", st.attempts+1, "err", err) + return + } + s.forgetGuestPower(g.VMID) + s.logger.Warn("guest-power: STARTED a guest that should have been running", "vmid", g.VMID) +} + +// ---- attempt bookkeeping (guarded by its own mutex; independent of the jobs lock) ---------- + +var guestPowerMu sync.Mutex + +// guestPowerDue reports the guest's attempt state and whether a new attempt is due now. +func (s *Server) guestPowerDue(vmid int) (guestPowerState, bool) { + guestPowerMu.Lock() + defer guestPowerMu.Unlock() + if s.guestPower == nil { + s.guestPower = map[int]guestPowerState{} + } + st := s.guestPower[vmid] + if st.nextAt.IsZero() || !s.now().Before(st.nextAt) { + return st, true + } + return st, false +} + +// noteGuestPowerFailure records a failed start and arms the next backoff. +func (s *Server) noteGuestPowerFailure(vmid int) { + guestPowerMu.Lock() + defer guestPowerMu.Unlock() + if s.guestPower == nil { + s.guestPower = map[int]guestPowerState{} + } + st := s.guestPower[vmid] + st.attempts++ + i := st.attempts - 1 + if i >= len(guestPowerBackoff) { + i = len(guestPowerBackoff) - 1 + } + st.nextAt = s.now().Add(guestPowerBackoff[i]) + s.guestPower[vmid] = st +} + +// markGuestPowerRaised records that the give-up fault has been raised, so it is logged once. +func (s *Server) markGuestPowerRaised(vmid int) { + guestPowerMu.Lock() + defer guestPowerMu.Unlock() + st := s.guestPower[vmid] + st.raised = true + s.guestPower[vmid] = st +} + +// forgetGuestPower clears a guest's attempt history — called when it is running again, so a guest +// that recovers does not carry its old failures into the next incident. +func (s *Server) forgetGuestPower(vmid int) { + guestPowerMu.Lock() + defer guestPowerMu.Unlock() + if s.guestPower == nil { + return + } + delete(s.guestPower, vmid) +} diff --git a/internal/localapi/guestpower_test.go b/internal/localapi/guestpower_test.go new file mode 100644 index 0000000..24a6184 --- /dev/null +++ b/internal/localapi/guestpower_test.go @@ -0,0 +1,239 @@ +package localapi + +import ( + "context" + "errors" + "log/slog" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// F-REBOOT (Campaign 8 fault 11): a guest rebooted mid-backup never came back — stopped, unlocked, +// nothing retrying, 9m47s of total appliance outage. +// +// Scenario A (a crashed guest is restarted), B (a deliberately stopped guest is left alone) and +// C (bounded retry, then escalate). B and C are what make A safe. + +type fakeGuestPowerCtl struct { + guests []proxmox.Guest + guestsErr error + locks map[int]string // vmid -> lock ("" = unlocked) + onboot map[int]bool + backupRun map[int]bool + backupErr error + started []int + startErr error +} + +func (f *fakeGuestPowerCtl) Guests(context.Context) ([]proxmox.Guest, error) { + return f.guests, f.guestsErr +} +func (f *fakeGuestPowerCtl) Lock(_ context.Context, vmid int) (string, bool, error) { + return f.locks[vmid], f.onboot[vmid], nil +} +func (f *fakeGuestPowerCtl) BackupRunning(_ context.Context, vmid int) (bool, error) { + return f.backupRun[vmid], f.backupErr +} +func (f *fakeGuestPowerCtl) HasVzdumpSnapshot(context.Context, int) (bool, error) { return false, nil } +func (f *fakeGuestPowerCtl) Unlock(context.Context, int) error { return nil } +func (f *fakeGuestPowerCtl) DeleteVzdumpSnapshot(context.Context, int) error { return nil } +func (f *fakeGuestPowerCtl) Start(_ context.Context, vmid int) error { + f.started = append(f.started, vmid) + return f.startErr +} + +func gpServer(t *testing.T, ctl StaleLockController, now func() time.Time) *Server { + t.Helper() + s := &Server{staleLock: ctl, logger: slog.New(slog.NewTextHandler(discardW{}, nil))} + if now != nil { + s.now = now + } else { + s.now = func() time.Time { return time.Now().UTC() } + } + return s +} + +type discardW struct{} + +func (discardW) Write(p []byte) (int, error) { return len(p), nil } + +// Scenario A — a guest that should be running (onboot) and is stopped-and-unlocked IS started. +// +// RED-PROOF: delete the `s.staleLock.Start(...)` call in recoverOneStoppedGuest (or make the whole +// function return before it) → started is empty and this fails with +// "guest 9201 was NOT started — this is F-REBOOT". +func TestGuestPower_StoppedOnbootGuestIsStarted(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + } + s := gpServer(t, ctl, nil) + + s.GuestPowerTick(context.Background()) + + if len(ctl.started) != 1 || ctl.started[0] != 9201 { + t.Fatalf("guest 9201 was NOT started — this is F-REBOOT (started=%v)", ctl.started) + } +} + +// Scenario B — a DELIBERATELY stopped guest (onboot:0) is never started. This is the trap: fighting +// the operator makes maintenance impossible and is worse than the outage being fixed. +// +// RED-PROOF: remove the `if !onboot { return }` guard → the golden/scratch guest is started and this +// fails with "a deliberately stopped guest (onboot:0) was started". +func TestGuestPower_DeliberatelyStoppedGuestIsLeftAlone(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9100, Status: "stopped"}, {VMID: 990000, Status: "stopped"}}, + locks: map[int]string{9100: "", 990000: ""}, + onboot: map[int]bool{9100: false, 990000: false}, // golden + scratch + } + s := gpServer(t, ctl, nil) + + s.GuestPowerTick(context.Background()) + + if len(ctl.started) != 0 { + t.Errorf("a deliberately stopped guest (onboot:0) was started: %v", ctl.started) + } +} + +// A LOCKED stopped guest belongs to the stale-lock path, which knows how to prove a lock is stale. +// This watchdog must not race it. +func TestGuestPower_LockedGuestIsLeftToTheStaleLockPath(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: "snapshot-delete"}, + onboot: map[int]bool{9201: true}, + } + s := gpServer(t, ctl, nil) + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 0 { + t.Errorf("started a LOCKED guest: %v — that races the stale-lock recovery", ctl.started) + } +} + +// A guest whose vzdump is genuinely in flight must be left stopped — a stop-mode backup stops the +// guest ON PURPOSE, and starting it underneath would corrupt the backup. +func TestGuestPower_InFlightBackupIsNotDisturbed(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + backupRun: map[int]bool{9201: true}, + } + s := gpServer(t, ctl, nil) + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 0 { + t.Errorf("started a guest with a vzdump in flight: %v", ctl.started) + } +} + +// Fail-safe: if we cannot confirm no backup is running, do NOT start. +func TestGuestPower_UnconfirmableBackupFailsSafe(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + backupErr: errors.New("task list unavailable"), + } + s := gpServer(t, ctl, nil) + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 0 { + t.Errorf("started despite being unable to confirm no backup is running: %v", ctl.started) + } +} + +// Unknown ownership ⇒ act on nothing. Starting a co-tenant's guest is worse than leaving ours down. +func TestGuestPower_UnprovenOwnershipActsOnNothing(t *testing.T) { + ctl := &fakeGuestPowerCtl{guestsErr: errors.New("pool read failed")} + s := gpServer(t, ctl, nil) + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 0 { + t.Errorf("acted with unproven ownership: %v", ctl.started) + } +} + +// Scenario C — bounded retry, then escalate. A guest that will not start must NOT be retried forever. +// +// RED-PROOF: remove the `if st.attempts >= guestPowerMaxAttempts` branch → the sweep keeps starting +// on every tick and this fails with "start attempted N times, want at most 3 — infinite retry loop". +func TestGuestPower_RetryIsBoundedThenEscalates(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + startErr: errors.New("cannot start: storage offline"), + } + base := time.Now().UTC() + now := base + s := gpServer(t, ctl, func() time.Time { return now }) + + // Drive many ticks, advancing well past every backoff each time. + for i := 0; i < 12; i++ { + s.GuestPowerTick(context.Background()) + now = now.Add(10 * time.Minute) + } + + if len(ctl.started) > guestPowerMaxAttempts { + t.Errorf("start attempted %d times, want at most %d — this is an infinite retry loop", + len(ctl.started), guestPowerMaxAttempts) + } + if len(ctl.started) != guestPowerMaxAttempts { + t.Errorf("start attempted %d times, want exactly %d before giving up", len(ctl.started), guestPowerMaxAttempts) + } +} + +// The backoff must actually hold a retry back — otherwise "bounded" is only bounded by luck. +func TestGuestPower_BackoffDefersTheNextAttempt(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + startErr: errors.New("boom"), + } + base := time.Now().UTC() + now := base + s := gpServer(t, ctl, func() time.Time { return now }) + + s.GuestPowerTick(context.Background()) // attempt 1, arms a 1m backoff + if len(ctl.started) != 1 { + t.Fatalf("precondition: want 1 attempt, got %d", len(ctl.started)) + } + now = base.Add(30 * time.Second) // still inside the 1m backoff + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 1 { + t.Errorf("retried inside the backoff window (%d attempts) — the bound is not being honoured", len(ctl.started)) + } + now = base.Add(90 * time.Second) // past it + s.GuestPowerTick(context.Background()) + if len(ctl.started) != 2 { + t.Errorf("did not retry after the backoff lapsed (%d attempts)", len(ctl.started)) + } +} + +// A guest that comes back healthy must lose its attempt history, so it does not carry old failures +// into the next incident. +func TestGuestPower_RunningGuestClearsAttemptHistory(t *testing.T) { + ctl := &fakeGuestPowerCtl{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + locks: map[int]string{9201: ""}, + onboot: map[int]bool{9201: true}, + startErr: errors.New("boom"), + } + base := time.Now().UTC() + now := base + s := gpServer(t, ctl, func() time.Time { return now }) + s.GuestPowerTick(context.Background()) + + if _, due := s.guestPowerDue(9201); due { + t.Error("precondition: a backoff should be armed after a failed start") + } + // it comes back up + ctl.guests = []proxmox.Guest{{VMID: 9201, Status: "running"}} + s.GuestPowerTick(context.Background()) + if _, due := s.guestPowerDue(9201); !due { + t.Error("attempt history survived the guest coming back healthy") + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index c7518bb..d03294e 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -263,7 +263,10 @@ type Server struct { guestBinds *GuestBindStore // F9 startup bind re-assert record (optional) formatJobs *FormatJobStore // F20-BUG3 detached-format job record (optional) staleLock StaleLockController // F2-b startup stale-lock recovery (optional) - host storage.HostReader // role classification source (optional; defaults to ProcHostReader) + // guestPower (F-REBOOT) is per-guest start-attempt state for the guest-power watchdog. + // Guarded by guestPowerMu in guestpower.go; in-memory on purpose (see guestPowerState). + guestPower map[int]guestPowerState + host storage.HostReader // role classification source (optional; defaults to ProcHostReader) hostMetrics HostMetricsProvider // slice 9 (optional) hostID string // slice 10B: for the data-bearing-format pending-op hint diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index c22befb..d1992a2 100644 --- a/internal/reconcile/restoretest.go +++ b/internal/reconcile/restoretest.go @@ -198,7 +198,7 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS launched := false defer func() { if launched { - e.teardownScratch(ctx, base) + e.teardownScratch(ctx, base, spec.ScratchMin, spec.ScratchMax) return } e.append(withState(base, OpFailed)) @@ -444,9 +444,44 @@ func sizeToGB(s string) int { return gb } +// scratchAdoptAllowed decides whether a stranded guest may be adopted into the felhom pool so the +// pool-scoped token can destroy it (F-LEAK). PURE, so the refusal is unit-testable without PVE. +// +// TWO INDEPENDENT GUARDS, both required. This function is the only thing standing between "clean up +// my own scratch" and "co-opt an arbitrary guest into the pool and delete it", so it does not rely +// on either check alone: +// - PROVENANCE: the journal entry must be one the agent itself created as a restore-test scratch. +// - NUMERIC BAND: the VMID must be inside the configured scratch band (scratch_vmid_min..max). +// +// A guest failing either is refused, loudly. Adopting a customer guest into the pool would hand the +// token destroy rights over it, which is a far worse outcome than a leaked scratch. +func scratchAdoptAllowed(vmid, min, max int, scratch bool) (bool, string) { + if !scratch { + return false, "journal entry is not agent-created scratch provenance" + } + if min <= 0 || max < min { + return false, fmt.Sprintf("scratch band [%d,%d] is not configured", min, max) + } + if vmid < min || vmid > max { + return false, fmt.Sprintf("vmid %d is outside the scratch band [%d,%d]", vmid, min, max) + } + return true, "" +} + // teardownScratch destroys the scratch guest (benign, gated) and records the entry terminal. // On any teardown failure it leaves the entry in-flight so Recover reaps the guest later. -func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) { +// +// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED left a scratch guest the agent could not +// destroy — `DELETE /nodes/x/lxc/990000` returned 403 "missing privilege VM.Allocate". The cause is +// pool membership, not privsep: VM.Allocate is granted at /pool/felhom ONLY (never at /), and a +// failed restore never completes the `--pool felhom` association, so the guest's own path resolves +// to / where the token holds nothing. Verified live: /vms/ grants only +// Datastore.Audit+SDN.Use+Sys.Audit, while /pool/felhom grants VM.Allocate AND Pool.Allocate. +// +// So the recovery needs NO new privilege: Pool.Allocate is already held, so we adopt the stranded +// scratch into the pool and retry the destroy, which then authorizes via /pool/felhom. Guarded by +// scratchAdoptAllowed — see there for why two guards rather than one. +func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry, scratchMin, scratchMax int) { // Cancel-immune + bounded, so a shutdown mid-test still tears down. tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) defer cancel() @@ -459,8 +494,28 @@ func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) { } upid, err := e.api.DestroyLXC(tctx, base.VMID) if err != nil { - e.logger.Error("restore-test: scratch teardown failed; left for Recover", "vmid", base.VMID, "err", err) - return + // F-LEAK: the destroy may have 403'd because a FAILED restore never completed pool + // membership, leaving the guest outside /pool/felhom where the token's VM.Allocate lives. + // Adopt it into the pool (Pool.Allocate, already granted) and retry ONCE. Any other error + // falls through to the original behaviour. + if ok, why := scratchAdoptAllowed(base.VMID, scratchMin, scratchMax, base.Scratch); !ok { + e.logger.Error("restore-test: scratch teardown failed and adoption REFUSED; left for Recover", + "vmid", base.VMID, "refused_because", why, "err", err) + return + } + e.logger.Warn("restore-test: scratch teardown failed — adopting the stranded scratch into the pool and retrying once", + "vmid", base.VMID, "pool", DefaultPool, "err", err) + if perr := e.api.PoolAddVMID(tctx, DefaultPool, base.VMID); perr != nil { + e.logger.Error("restore-test: pool adoption failed; left for Recover", "vmid", base.VMID, "err", perr) + return + } + upid, err = e.api.DestroyLXC(tctx, base.VMID) + if err != nil { + e.logger.Error("restore-test: scratch teardown failed even after pool adoption; left for Recover", + "vmid", base.VMID, "err", err) + return + } + e.logger.Info("restore-test: stranded scratch adopted into the pool and destroyed", "vmid", base.VMID) } if upid != "" { if _, err := e.api.WaitTask(tctx, upid, proxmox.WaitOptions{}); err != nil { diff --git a/internal/reconcile/scratch_adopt_test.go b/internal/reconcile/scratch_adopt_test.go new file mode 100644 index 0000000..d53461f --- /dev/null +++ b/internal/reconcile/scratch_adopt_test.go @@ -0,0 +1,71 @@ +package reconcile + +import "testing" + +// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED could not destroy its own scratch guest — +// `DELETE /nodes/x/lxc/990000` → 403 "missing privilege VM.Allocate". The cause is pool membership, +// not privsep: VM.Allocate is granted at /pool/felhom only, and a failed restore never completes the +// `--pool felhom` association, so the guest's own path resolves to / where the token holds nothing. +// +// The fix adopts the stranded scratch into the pool (Pool.Allocate — already granted) and retries the +// destroy. scratchAdoptAllowed is the guard that keeps that from becoming "co-opt any guest into the +// pool and delete it", and this file is its mirror test. +// +// Scenario D (the scratch is destroyed) is the live replay; Scenario E — the agent still cannot reach +// a non-scratch guest this way — is HERE, because it must hold as a pure property and not depend on +// what PVE happens to refuse. + +// Scenario E — the adoption path REFUSES anything outside the scratch band. +// +// RED-PROOF: replace the band check with `return true, ""` → every case below reports allowed and the +// test fails with "adoption ALLOWED for vmid 9201 — that would let the agent co-opt a customer guest +// into the pool and destroy it". +func TestScratchAdoptAllowed_RefusesNonScratch(t *testing.T) { + const min, max = 990000, 990009 + for _, tc := range []struct { + name string + vmid int + scratch bool + }{ + {"a live customer guest", 9201, true}, // scratch-flagged but OUTSIDE the band + {"the golden image", 9100, true}, // ditto + {"just below the band", min - 1, true}, // off-by-one + {"just above the band", max + 1, true}, // off-by-one + {"in-band but NOT scratch provenance", min, false}, + {"neither", 100, false}, + } { + t.Run(tc.name, func(t *testing.T) { + ok, why := scratchAdoptAllowed(tc.vmid, min, max, tc.scratch) + if ok { + t.Errorf("adoption ALLOWED for vmid %d — that would let the agent co-opt a non-scratch guest into the pool and destroy it", tc.vmid) + } + if why == "" { + t.Error("refusal carried no reason — a silent refusal is unreviewable") + } + }) + } +} + +// The in-band, scratch-provenanced case IS allowed — otherwise the fix does nothing and the leak stays. +// +// RED-PROOF: make scratchAdoptAllowed always return false → this fails with "adoption refused for a +// genuine in-band scratch guest", i.e. F-LEAK is not fixed at all. +func TestScratchAdoptAllowed_AllowsGenuineScratch(t *testing.T) { + const min, max = 990000, 990009 + for _, vmid := range []int{min, min + 5, max} { + ok, why := scratchAdoptAllowed(vmid, min, max, true) + if !ok { + t.Errorf("adoption refused for a genuine in-band scratch guest %d: %s", vmid, why) + } + } +} + +// An unconfigured band must refuse everything rather than defaulting to something permissive — a +// zero-valued band is a wiring bug, and the safe reading of a wiring bug is "do nothing". +func TestScratchAdoptAllowed_UnconfiguredBandRefuses(t *testing.T) { + for _, tc := range []struct{ min, max int }{{0, 0}, {0, 990009}, {990009, 990000}, {-1, 5}} { + if ok, _ := scratchAdoptAllowed(990000, tc.min, tc.max, true); ok { + t.Errorf("adoption allowed with an unconfigured band [%d,%d]", tc.min, tc.max) + } + } +}