diff --git a/CHANGELOG.md b/CHANGELOG.md index 465f23f..e91e94c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,39 @@ +## v0.60.0 — proof-of-launch destroy gating + restore-test band-advance (campaign F1/F2) (2026-07-02) + +Fixes the pool-effects campaign's HIGH finding (F1, `CAMPAIGN-pool-effects-2026-07-01.md`): the bring-up +compensating rollback and the restore-test teardown fired `DestroyLXC` on the target vmid even when +`RestoreLXC` failed synchronously WITHOUT creating anything (PVE refusing a pre-existing vmid the +pool-blind duplicate guard / band scan couldn't see) — destroying a guest the transaction never made. +Only the pool ACL's 403 saved the non-pool subset; an in-pool pre-existing guest would have been +destroyed, and any broad-token deployment re-arms the bug. Root cause: SameTxnCreated/scratch provenance +was ASSUMED, never verified against proof-of-launch. The fix makes a `RestoreLXC` UPID the sole destroy +authorization, in ALL THREE destroy paths — the pool ACL is defense-in-depth again, not the guard. + +- **F1a `internal/reconcile/bringup.go` `runBringUp`:** the compensating-rollback defer is gated on + `launched` (set only after the restore POST is accepted). A synchronous restore failure (no UPID) + closes the owning entry terminal-failed WITHOUT any destroy. The pre-restore `OpStarted` journal + append is kept (crash-safety); `rollbackBringUp` is now only ever called launch-proven. +- **F1b `internal/reconcile/restoretest.go` `runScratchTest`:** same `launched` gate on + `teardownScratch` — a synchronous restore refusal never destroys the picked band vmid. +- **F1c `internal/reconcile/recover.go` `Recover`:** the no-UPID "POST never confirmed → abandon + fail-safe" check now runs BEFORE the Scratch/Rollback dispatch — a no-UPID Scratch/Rollback entry is + abandoned (marked failed, NO destroy) instead of destroy-by-vmid-existence. Recover is now safe by + DESIGN, not by the pool-blind "already gone" accident the campaign observed. +- **F2 `restoretest.go` `RunRestoreTest` band-advance:** a band vmid PVE refuses with "already exists" + (an invisible squatter — new `pveAlreadyExists`, mirrors `pveConfigLock`, never misclassifies a real + restore failure) is skipped and the next free band vmid tried (bounded by the band width; + `pickScratchVMID` gained an exclude set). A fully-occupied band → `Skipped` (scheduler raises no + "backup unrestorable" alert), never FAIL — one squatter no longer permanently breaks the restore-test. +- **Accepted residual (by design):** a crash in the one-statement window between obtaining the UPID and + journaling it leaks a half-built guest Recover won't destroy — cleanable, and vastly preferable to + destroying an innocent guest. +- Tests: red-proof companions verified (gates reverted → `TestRunBringUp_NoLaunchNoDestroy`, + `TestRunRestoreTest_RestoreNoLaunchNoTeardown`, `TestRecover_{BringUp,Scratch}NoUPIDAbandoned` all + fail with the innocent-guest destroy); no-regression `…LaunchedTaskFailureStillTearsDown` + rollback + table now includes an explicit restore-task-failure case; F2 advance + squatter-full-band-skips tests. + Live-validated on felhom-pve (provision onto existing 9001 → no destroy armed; restore-test advances + past a 990000 decoy). `go build/vet/test ./...` clean. + ## v0.59.0 — report backing device + capacity for a registry-sourced drive in /disks (2026-07-01) Completes the `/disks` representation for a registry-sourced (raw, no-PVE-storage) drive: the agent-view diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 9dfc19f..64d4ff4 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -45,7 +45,7 @@ import ( // version is the agent version. Overridable at build time with // -ldflags "-X main.version="; defaults to the in-repo CHANGELOG version. -var version = "0.59.0" +var version = "0.60.0" // runGuestHook is the PVE pre-start hook body (`felhom-agent guest-hook `). On the // pre-start phase it creates placeholder dirs for any absent bind-mount source so the guest always boots diff --git a/internal/reconcile/bringup.go b/internal/reconcile/bringup.go index 711609f..40593d6 100644 --- a/internal/reconcile/bringup.go +++ b/internal/reconcile/bringup.go @@ -192,16 +192,29 @@ func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpR base := JournalEntry{OpID: e.bringUpOpID(spec.VMID), VMID: spec.VMID, Kind: bringUpKind, Rollback: true} // OWN the rollback BEFORE any mutation. From here a crash leaves an in-flight Rollback - // entry meaning "VMID may be a half-built guest → destroy it" (Recover.recoverBringUp). + // entry; Recover destroys the vmid ONLY when the entry carries a restore UPID + // (proof-of-launch — see Recover's no-UPID abandon path). e.append(withState(base, OpStarted)) - // Compensating rollback on EVERY non-committed exit (defer): destroy the just-created - // guest. On success we set committed and KEEP it (the key difference from the restore-test). + // Compensating rollback on every non-committed exit AFTER the restore launched (defer): + // destroy the just-created guest. On success we set committed and KEEP it (the key + // difference from the restore-test). `launched` is the proof-of-launch gate (campaign + // pool-effects F1a): a restore that failed synchronously (no UPID — e.g. PVE refusing a + // vmid that already holds a guest the pool-blind duplicate guard can't see) created + // NOTHING, so the rollback must NEVER destroy the vmid — a pre-existing guest, possibly + // another customer's, may sit there. This must hold WITHOUT the pool ACL (that 403 is + // defense-in-depth, not the guard). The owning entry is then closed terminal-failed + // in-process (nothing exists to recover). committed := false + launched := false defer func() { if committed { return } + if !launched { + e.append(withState(base, OpFailed)) + return + } e.rollbackBringUp(ctx, base) }() @@ -210,9 +223,15 @@ func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpR VMID: spec.VMID, Archive: spec.Archive, Storage: spec.RestoreStorage, Pool: spec.Pool, }) if err != nil { + // No UPID ⇒ nothing was created ⇒ the defer closes the entry WITHOUT a destroy. res.Err = fmt.Errorf("reconcile: bring-up restore: %w", err) return } + // Proof-of-launch: the POST was accepted — from here a failure means a half-built guest the + // compensating rollback (or Recover, via the journaled UPID) must destroy. Accepted residual: + // a crash in the one-statement window before the UPID is journaled leaks a half-built guest + // that Recover won't destroy — cleanable, and preferable to destroying an innocent guest. + launched = true e.append(withUPID(base, upid, OpTaskRunning)) if _, err := e.waitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { res.Err = fmt.Errorf("reconcile: bring-up restore task: %w", err) @@ -333,10 +352,10 @@ func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpR } // rollbackBringUp destroys the just-created guest (benign ClassGuestDestroy via SameTxnCreated -// provenance) and records the owning entry terminal. Mirrors teardownScratch: ALWAYS attempts the -// destroy (idempotent — a restore-POST failure that created no guest just errors harmlessly and is -// left in-flight for Recover, which existence-checks). On any teardown failure it leaves the entry -// in-flight so Recover reaps the guest later — never force-destroys. +// provenance) and records the owning entry terminal. Called ONLY launch-proven (the restore POST +// was accepted — campaign pool-effects F1a): the SameTxnCreated provenance is then real, not +// assumed. On any teardown failure it leaves the entry in-flight so Recover reaps the guest later +// (via the journaled UPID) — never force-destroys. func (e *Engine) rollbackBringUp(ctx context.Context, base JournalEntry) { tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) defer cancel() @@ -432,6 +451,19 @@ func pveConfigLock(err error) bool { return strings.Contains(b, "can't lock file") || strings.Contains(b, "got timeout") } +// pveAlreadyExists reports whether err is PVE's synchronous refusal to create over an existing +// vmid ("CT already exists on node ''" — an APIError 500, observed live in the +// pool-effects campaign). By construction such a refusal returned no UPID: nothing was created. +// Used by the restore-test band-advance (F2) to distinguish "band vmid occupied by a guest the +// pool-blind list can't see" from a real restore failure — never misclassify the latter. +func pveAlreadyExists(err error) bool { + var ae *proxmox.APIError + if !errors.As(err, &ae) || ae.StatusCode != 500 { + return false + } + return strings.Contains(strings.ToLower(ae.Body), "already exists") +} + // waitTask waits a (possibly empty) UPID — "" is the clean synchronous path. func (e *Engine) waitTask(ctx context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error) { if upid == "" { diff --git a/internal/reconcile/bringup_test.go b/internal/reconcile/bringup_test.go index 5c9c03e..b925c39 100644 --- a/internal/reconcile/bringup_test.go +++ b/internal/reconcile/bringup_test.go @@ -255,7 +255,17 @@ func TestRunBringUp_CompensatingRollback(t *testing.T) { name string setup func(*fakeAPI) }{ - {"restore error", func(a *fakeAPI) { a.restoreErr = errors.New("restore boom") }}, + {"restore-task failure after launch", func(a *fakeAPI) { + // Scenario B (no-regression): the restore POST was ACCEPTED (UPID) but the task then + // fails → we created/started it → the compensating destroy MUST still fire. + a.restoreUPID = "UPID:demo:restore:8000:" + a.waitFunc = func(upid string) (proxmox.TaskStatus, error) { + if upid == "UPID:demo:restore:8000:" { + return proxmox.TaskStatus{}, errors.New("restore task failed") + } + return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil + } + }}, {"config real error", func(a *fakeAPI) { a.setFunc = func(int, map[string]string) (string, error) { return "", &proxmox.APIError{StatusCode: 500, Body: "some non-lock internal error"} @@ -300,6 +310,48 @@ func TestRunBringUp_CompensatingRollback(t *testing.T) { } } +// TestRunBringUp_NoLaunchNoDestroy is the F1a red-proof (campaign pool-effects): a restore that +// fails SYNCHRONOUSLY (no UPID — nothing created) must NOT arm the compensating destroy. The +// headline case is PVE refusing a vmid that already holds a guest the pool-blind duplicate guard +// couldn't see — the old defer would have destroyed that innocent pre-existing guest (only the +// pool ACL's 403 saved the non-pool subset; an in-pool one would have been destroyed). +// Red-proof: revert the `launched` gate in runBringUp and this test fails with destroys=[8000]. +func TestRunBringUp_NoLaunchNoDestroy(t *testing.T) { + const vmid = 8000 + cases := []struct { + name string + err error + }{ + {"PVE refuses pre-existing vmid", &proxmox.APIError{ + StatusCode: 500, Method: "POST", Path: "/nodes/x/lxc", + Body: `{"message":"CT 8000 already exists on node 'x'\n","data":null}`, + }}, + {"plain synchronous restore error", errors.New("restore boom")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}, restoreErr: tc.err} + e, j, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "vol", VMID: vmid, RestoreStorage: "local-lvm", Hostname: "h", + }) + if res.Pass || res.Err == nil { + t.Fatalf("must fail, got %+v", res) + } + // THE point: no destroy is even attempted — the txn created nothing. + if len(api.destroys) != 0 { + t.Fatalf("no-launch failure must NOT destroy the vmid (pre-existing guest!): destroys=%+v", api.destroys) + } + // And the owning entry is closed terminal in-process (not left for Recover). + if len(j.InFlight()) != 0 { + t.Errorf("owning entry must be terminal after a no-launch failure: %+v", j.InFlight()) + } + }) + } +} + func TestRunBringUp_DRPreservesContinuityIdentity(t *testing.T) { const vmid = 8001 api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} @@ -513,10 +565,11 @@ func TestRunBringUp_RejectsReservedAndExistingVMID(t *testing.T) { func TestRecover_HalfBuiltBringUpRolledBack(t *testing.T) { const vmid = 8000 - // The guest still exists at startup (agent crashed mid-bring-up) → Recover destroys it. + // The guest still exists at startup (agent crashed mid-bring-up AFTER the restore launched — + // the entry carries the UPID = the launch proof) → Recover destroys it. api := &fakeAPI{lxc: []proxmox.Guest{{VMID: vmid, Status: "running"}}} e, j, _ := newEngine(t, api, EmptyProvider{}) - if err := j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, State: OpTaskRunning, At: time.Now().UTC()}); err != nil { + if err := j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, UPID: "UPID:demo:restore:8000:", State: OpTaskRunning, At: time.Now().UTC()}); err != nil { t.Fatal(err) } res := e.Recover(context.Background()) @@ -533,10 +586,10 @@ func TestRecover_HalfBuiltBringUpRolledBack(t *testing.T) { func TestRecover_HalfBuiltBringUpAlreadyGone(t *testing.T) { const vmid = 8000 - // Crash after the restore POST failed (no guest) → idempotent clean, no destroy. + // Crash after a LAUNCHED restore whose guest is already gone → idempotent clean, no destroy. api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 9001}}} // 8000 absent e, j, _ := newEngine(t, api, EmptyProvider{}) - j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, State: OpStarted, At: time.Now().UTC()}) + j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, UPID: "UPID:demo:restore:8000:", State: OpTaskRunning, At: time.Now().UTC()}) res := e.Recover(context.Background()) if res.BringUpClean != 1 || len(api.destroys) != 0 { t.Fatalf("already-gone bring-up must be clean with no destroy: res=%+v destroys=%+v", res, api.destroys) @@ -546,6 +599,29 @@ func TestRecover_HalfBuiltBringUpAlreadyGone(t *testing.T) { } } +// TestRecover_BringUpNoUPIDAbandoned is the F1c red-proof (campaign pool-effects, Scenario C): a +// crash left a Rollback entry with NO UPID (the restore was never confirmed) while a guest sits +// at that vmid — e.g. a PRE-EXISTING guest the pool-blind list-existence check would have called +// "ours". Recover must ABANDON (fail-safe), never destroy: no journaled UPID ⇒ this transaction +// created nothing. Red-proof: route the entry to recoverBringUp (pre-fix order) and this fails +// with destroys=[8000]. +func TestRecover_BringUpNoUPIDAbandoned(t *testing.T) { + const vmid = 8000 + api := &fakeAPI{lxc: []proxmox.Guest{{VMID: vmid, Status: "stopped"}}} // a guest IS at the vmid + e, j, _ := newEngine(t, api, EmptyProvider{}) + j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, State: OpStarted, At: time.Now().UTC()}) + res := e.Recover(context.Background()) + if len(api.destroys) != 0 { + t.Fatalf("a no-UPID Rollback entry must NEVER destroy the vmid (pre-existing guest!): destroys=%+v", api.destroys) + } + if res.RolledBack != 1 || res.BringUpRolledBack != 0 { + t.Fatalf("entry must be abandoned via the no-UPID fail-safe path, got %+v", res) + } + if len(j.InFlight()) != 0 { + t.Errorf("abandoned entry must be terminal: %+v", j.InFlight()) + } +} + // lockBackoffFast shrinks the F4 retry backoff for tests and restores it after. func lockBackoffFast(t *testing.T) { t.Helper() diff --git a/internal/reconcile/engine_test.go b/internal/reconcile/engine_test.go index 21bc8c2..9e441a7 100644 --- a/internal/reconcile/engine_test.go +++ b/internal/reconcile/engine_test.go @@ -36,6 +36,9 @@ type fakeAPI struct { // restoreHook, when set, fires inside RestoreLXC (used to assert the owning journal entry // is written BEFORE the restore — crash-safety ordering). restoreHook func() + // restoreFunc, when set, backs RestoreLXC per-call (drives the F2 band-advance tests: + // per-vmid "already exists" vs success). Takes precedence over restoreUPID/restoreErr. + restoreFunc func(opts proxmox.RestoreLXCOptions) (string, error) starts []int stops []int @@ -58,7 +61,11 @@ func (f *fakeAPI) RestoreLXC(_ context.Context, opts proxmox.RestoreLXCOptions) } f.mu.Lock() f.restores = append(f.restores, opts) + fn := f.restoreFunc f.mu.Unlock() + if fn != nil { + return fn(opts) + } return f.restoreUPID, f.restoreErr } diff --git a/internal/reconcile/recover.go b/internal/reconcile/recover.go index c746ff3..c4c57f0 100644 --- a/internal/reconcile/recover.go +++ b/internal/reconcile/recover.go @@ -35,32 +35,41 @@ func (e *Engine) Recover(ctx context.Context) RecoverResult { for _, entry := range e.journal.InFlight() { res.Examined++ + // PROOF-OF-LAUNCH gate, checked FIRST (campaign pool-effects F1c): an entry with no + // journaled UPID means the restore/POST was never confirmed → this transaction created + // NOTHING → NEVER destroy the vmid. For Scratch/Rollback (guest-creating) entries this + // is load-bearing: a pre-existing guest — possibly another customer's, invisible to the + // pool-blind ListLXC existence check — may sit at that vmid, and the old destroy-if- + // exists path would have destroyed it under a broad token. Abandon fail-safe instead. + // Accepted residual: a crash between obtaining and journaling the UPID may leak a + // half-built guest (cleanable) — preferable to destroying an innocent one. + if entry.UPID == "" { + e.append(terminal(entry, OpFailed)) + res.RolledBack++ + e.logger.Warn("recover: in-flight op had no task id; marked failed (fail-safe, no destroy)", + "op_id", entry.OpID, "vmid", entry.VMID, "kind", entry.Kind) + continue + } + // Scratch entries (slice-6 restore-test) are resolved by TEARDOWN, not by // re-checking a sub-task UPID — a leaked scratch guest is the failure mode that // matters. Handle them BEFORE the generic UPID path (else the restore sub-task's OK - // status would mark the entry succeeded while the guest still exists → leak). + // status would mark the entry succeeded while the guest still exists → leak). The + // UPID they carry is the launch proof authorizing the destroy-by-existence below. if entry.Scratch { e.recoverScratch(ctx, entry, &res) continue } // Rollback entries (slice-7 bring-up) own a guest the agent was CREATING. An in-flight - // one means "VMID may be a half-built guest → destroy it" (compensating rollback) — same - // reason the Scratch path runs before the generic UPID path: the restore sub-task's OK - // status would otherwise mark the entry succeeded and leave a half-provisioned guest. + // launch-proven one means "VMID may be a half-built guest → destroy it" (compensating + // rollback) — same reason the Scratch path runs before the generic UPID path: the + // restore sub-task's OK status would otherwise mark the entry succeeded and leave a + // half-provisioned guest. if entry.Rollback { e.recoverBringUp(ctx, entry, &res) continue } - - if entry.UPID == "" { - // POST never confirmed → abandon (fail-safe). - e.append(terminal(entry, OpFailed)) - res.RolledBack++ - e.logger.Warn("recover: in-flight op had no task id; marked failed (fail-safe)", - "op_id", entry.OpID, "vmid", entry.VMID, "kind", entry.Kind) - continue - } st, err := e.api.TaskStatusOnce(ctx, entry.UPID) if err != nil { res.Unresolved++ diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index 7d8fd9d..0673c20 100644 --- a/internal/reconcile/restoretest.go +++ b/internal/reconcile/restoretest.go @@ -105,9 +105,11 @@ func IntentForScratchDestroy(hostID string, vmid int) Intent { // RunRestoreTest runs one restore-test on the per-guest queue lane of a fresh scratch VMID. // It journals a Scratch-owned entry BEFORE any mutation, so a crash anywhere after this -// point is recoverable (Recover destroys the scratch guest). Teardown runs on EVERY path -// (defer), including a failed verify. The returned Err is the TEST verdict's error (restore -// or boot failure), independent of teardown success. +// point is recoverable (Recover destroys a launch-proven scratch guest via its journaled +// UPID). Teardown runs on every launch-proven path (defer), including a failed verify — but +// NEVER when the restore failed before creating anything (proof-of-launch, campaign F1b). A +// band vmid PVE reports "already exists" is advanced past, not failed (F2). The returned Err +// is the TEST verdict's error (restore or boot failure), independent of teardown success. func (e *Engine) RunRestoreTest(ctx context.Context, spec RestoreTestSpec) RestoreTestResult { now := time.Now().UTC() res := RestoreTestResult{Archive: spec.Archive, SourceTier: spec.SourceTier, StartedAt: now} @@ -126,38 +128,68 @@ func (e *Engine) RunRestoreTest(ctx context.Context, spec RestoreTestSpec) Resto res.Err = fmt.Errorf("reconcile: restore-test list guests: %w", err) return res } - vmid, ok := pickScratchVMID(lxc, spec.ScratchMin, spec.ScratchMax) - if !ok { - // Full band (e.g. an accumulation of un-torn-down scratch guests) → skip, never - // panic or pick out-of-band. Recover will reap any genuinely leaked ones. - e.logger.Warn("restore-test skipped: no free scratch VMID in band", - "min", spec.ScratchMin, "max", spec.ScratchMax) - res.Skipped = true - return res - } - res.ScratchVMID = vmid + // Band-advance loop (campaign pool-effects F2): the band scan below is POOL-BLIND (the + // scoped token's ListLXC can't see non-pool guests), so a band vmid can look free while a + // squatter sits on it. PVE tells us at restore time ("already exists"); we then advance to + // the next band vmid instead of failing — one squatter must not permanently break the + // restore-test or raise a false "backup unrestorable" alert. Bounded by the band width. + occupied := make(map[int]bool) + for { + vmid, ok := pickScratchVMID(lxc, spec.ScratchMin, spec.ScratchMax, occupied) + if !ok { + // Band exhausted (in-use and/or invisible squatters) → skip, never panic, never + // pick out-of-band, never FAIL. Recover will reap any genuinely leaked ones. + e.logger.Warn("restore-test skipped: no free scratch VMID in band", + "min", spec.ScratchMin, "max", spec.ScratchMax, "occupied_invisible", len(occupied)) + res.Skipped = true + res.ScratchVMID = 0 + res.Err = nil + return res + } + res.ScratchVMID = vmid - // Serialize on the scratch VMID's lane (inherits §10), and capture the result. - ch := e.queue.Submit(vmid, func() error { - e.runScratchTest(ctx, vmid, spec, &res) - return res.Err - }) - <-ch + // Serialize on the scratch VMID's lane (inherits §10), and capture the result. + var vmidOccupied bool + ch := e.queue.Submit(vmid, func() error { + vmidOccupied = e.runScratchTest(ctx, vmid, spec, &res) + return res.Err + }) + <-ch + if !vmidOccupied { + break + } + e.logger.Warn("restore-test: band VMID occupied by a guest invisible to the token; advancing", + "vmid", vmid) + occupied[vmid] = true + } res.Duration = time.Since(now) return res } -// runScratchTest is the journaled body (runs on vmid's queue lane). -func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestSpec, res *RestoreTestResult) { +// runScratchTest is the journaled body (runs on vmid's queue lane). The occupied return is true +// ONLY when PVE synchronously refused the restore because the vmid already holds a guest (one +// the pool-blind band scan couldn't see) — the caller then advances to the next band vmid (F2). +func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestSpec, res *RestoreTestResult) (occupied bool) { base := JournalEntry{OpID: e.scratchOpID(vmid), VMID: vmid, Kind: scratchKind, Scratch: true} // OWN the scratch guest's cleanup BEFORE any mutation. From here, a crash is recoverable. e.append(withState(base, OpStarted)) - // Teardown ALWAYS runs (even on a failed verify). Uses a cancel-immune context so a - // daemon shutdown mid-test still tears down; if teardown fails, the entry stays - // in-flight and Recover reaps the guest on the next start. - defer e.teardownScratch(ctx, base) + // Teardown runs on every exit AFTER the restore launched (even on a failed verify), using a + // cancel-immune context so a daemon shutdown mid-test still tears down; if teardown fails, + // the entry stays in-flight and Recover reaps the guest on the next start (via the journaled + // UPID). `launched` is the proof-of-launch gate (campaign pool-effects F1b): a restore that + // failed synchronously (no UPID) created NOTHING, so teardown must NEVER destroy the vmid — + // an invisible pre-existing guest may sit there. The entry is then closed terminal-failed + // (nothing exists to recover). + launched := false + defer func() { + if launched { + e.teardownScratch(ctx, base) + return + } + e.append(withState(base, OpFailed)) + }() // 1. Restore into the fresh scratch VMID (benign create path). The UPID is for error // detection only — it does NOT make the Scratch entry terminal (teardown does). @@ -197,9 +229,18 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS VMID: vmid, Archive: spec.Archive, Storage: spec.RestoreStorage, MountOverrides: mountOverrides, Pool: DefaultPool, }) if err != nil { + if pveAlreadyExists(err) { + // The band vmid holds a guest the pool-blind scan couldn't see. Nothing was + // created; NOT a test verdict — the caller advances to the next band vmid (F2). + return true + } res.Err = fmt.Errorf("reconcile: restore-test restore: %w", err) - return + return false } + // Proof-of-launch: the POST was accepted — from here teardown owns the guest. Accepted + // residual: a crash before the next append leaks a scratch guest Recover won't destroy + // (no journaled UPID) — cleanable, and preferable to destroying an innocent guest. + launched = true e.append(withUPID(base, upid, OpTaskRunning)) if upid != "" { if _, err := e.api.WaitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { @@ -258,6 +299,7 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS } res.Pass = true res.Verified = "boot+running" + return false } // archiveVMID extracts the source VMID from a backup archive volid. Handles PBS volids @@ -414,15 +456,16 @@ func (e *Engine) waitRunning(ctx context.Context, vmid int, timeout time.Duratio } // pickScratchVMID returns the lowest free VMID in [min,max], excluding the standing 9999 -// scratch and any in-use guest. ok=false when the band is fully occupied (the test is then -// skipped, never run out-of-band). -func pickScratchVMID(lxc []proxmox.Guest, min, max int) (int, bool) { +// scratch, any in-use guest, and the caller's exclude set (band vmids PVE reported occupied by +// guests the pool-blind list can't see — the F2 band-advance). ok=false when the band is fully +// occupied (the test is then skipped, never run out-of-band). +func pickScratchVMID(lxc []proxmox.Guest, min, max int, exclude map[int]bool) (int, bool) { used := make(map[int]bool, len(lxc)) for _, g := range lxc { used[g.VMID] = true } for id := min; id <= max; id++ { - if id == 9999 || used[id] { + if id == 9999 || used[id] || exclude[id] { continue } return id, true diff --git a/internal/reconcile/restoretest_test.go b/internal/reconcile/restoretest_test.go index da83b85..c04cda7 100644 --- a/internal/reconcile/restoretest_test.go +++ b/internal/reconcile/restoretest_test.go @@ -210,7 +210,12 @@ func TestRunRestoreTest_TeardownOnFailedVerify(t *testing.T) { } } -func TestRunRestoreTest_RestoreFailureStillTearsDown(t *testing.T) { +// TestRunRestoreTest_RestoreNoLaunchNoTeardown is the F1b red-proof (campaign pool-effects): a +// restore that fails SYNCHRONOUSLY (no UPID — nothing created) must NOT run teardown. The old +// behavior destroyed the picked vmid anyway — the exact destroy-innocent-guest bug when a +// pool-invisible squatter occupied the band vmid. Red-proof: revert the `launched` gate in +// runScratchTest and this test fails with destroys=[990000]. +func TestRunRestoreTest_RestoreNoLaunchNoTeardown(t *testing.T) { api := &fakeAPI{restoreErr: errors.New("restore boom")} e, j, q := newEngine(t, api, EmptyProvider{}) defer q.Close() @@ -221,16 +226,112 @@ func TestRunRestoreTest_RestoreFailureStillTearsDown(t *testing.T) { if res.Pass || res.Err == nil { t.Fatalf("expected restore failure, got %+v", res) } - // Even though restore failed, the scratch entry was journaled BEFORE the restore, so - // teardown runs (idempotent — destroys the maybe-partial guest). - if len(api.destroys) != 1 { - t.Fatalf("teardown must run after a restore failure: %+v", api.destroys) + // THE point: no teardown — the txn created nothing at that vmid. + if len(api.destroys) != 0 { + t.Fatalf("a no-launch restore failure must NOT destroy the vmid: %+v", api.destroys) + } + // The entry is closed terminal in-process (not left for Recover). + if len(j.InFlight()) != 0 { + t.Errorf("scratch entry must be terminal: %+v", j.InFlight()) + } +} + +// TestRunRestoreTest_LaunchedTaskFailureStillTearsDown (no-regression, Scenario B): the restore +// POST was accepted (UPID) but the task then fails → we own the maybe-partial guest → teardown +// MUST still run. +func TestRunRestoreTest_LaunchedTaskFailureStillTearsDown(t *testing.T) { + const restoreUPID = "UPID:demo:restore:990000:" + api := &fakeAPI{ + restoreUPID: restoreUPID, + waitFunc: func(upid string) (proxmox.TaskStatus, error) { + if upid == restoreUPID { + return proxmox.TaskStatus{}, errors.New("restore task failed") + } + return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil + }, + } + e, j, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009, + }) + if res.Pass || res.Err == nil { + t.Fatalf("expected restore-task failure, got %+v", res) + } + if len(api.destroys) != 1 || api.destroys[0] != 990000 { + t.Fatalf("teardown must run after a LAUNCHED restore fails: %+v", api.destroys) } if len(j.InFlight()) != 0 { t.Errorf("scratch entry must be terminal: %+v", j.InFlight()) } } +// TestRunRestoreTest_AdvancesPastOccupiedBandVMID is the F2 test (campaign pool-effects): the +// first band vmid is refused "already exists" (an invisible squatter) → the test ADVANCES to the +// next band vmid and PASSES there; the squatter is never destroyed; no FAIL, no false alert. +func TestRunRestoreTest_AdvancesPastOccupiedBandVMID(t *testing.T) { + squatterRefusal := &proxmox.APIError{ + StatusCode: 500, Method: "POST", Path: "/nodes/x/lxc", + Body: `{"message":"CT 990000 already exists on node 'x'\n","data":null}`, + } + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{990001: scratchCfg()}} + api.restoreFunc = func(opts proxmox.RestoreLXCOptions) (string, error) { + if opts.VMID == 990000 { + return "", squatterRefusal + } + return "", nil // 990001 restores fine (synchronous OK path) + } + e, j, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009, + }) + if res.Skipped || !res.Pass || res.Err != nil { + t.Fatalf("must advance past the squatter and PASS, got %+v", res) + } + if res.ScratchVMID != 990001 { + t.Fatalf("must have advanced to 990001, got %d", res.ScratchVMID) + } + // The squatter at 990000 must NEVER be destroyed; only the own scratch at 990001 is. + for _, d := range api.destroys { + if d == 990000 { + t.Fatalf("the squatter at 990000 must never be destroyed: destroys=%+v", api.destroys) + } + } + if len(api.destroys) != 1 || api.destroys[0] != 990001 { + t.Fatalf("own scratch teardown expected at 990001: %+v", api.destroys) + } + if len(j.InFlight()) != 0 { + t.Errorf("all entries must be terminal: %+v", j.InFlight()) + } +} + +// TestRunRestoreTest_BandFullOfSquattersSkips (F2): every band vmid is refused "already exists" +// → Skipped (NOT a FAIL — no false "backup unrestorable" alert), nothing destroyed, bounded. +func TestRunRestoreTest_BandFullOfSquattersSkips(t *testing.T) { + api := &fakeAPI{} + api.restoreFunc = func(opts proxmox.RestoreLXCOptions) (string, error) { + return "", &proxmox.APIError{StatusCode: 500, Body: "CT already exists on node 'x'"} + } + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990002, + }) + if !res.Skipped || res.Err != nil || res.Pass { + t.Fatalf("a squatter-full band must SKIP (not fail), got %+v", res) + } + if len(api.destroys) != 0 { + t.Fatalf("nothing may be destroyed: %+v", api.destroys) + } + if len(api.restores) != 3 { + t.Errorf("must have tried each band vmid exactly once (bounded), got %d", len(api.restores)) + } +} + func TestRunRestoreTest_FullBandSkips(t *testing.T) { // Whole band occupied → skipped, never run / out-of-band. var guests []proxmox.Guest @@ -263,15 +364,23 @@ func TestRunRestoreTest_InvalidBandErrors(t *testing.T) { func TestPickScratchVMID(t *testing.T) { // excludes 9999 and in-use; lowest free. - got, ok := pickScratchVMID([]proxmox.Guest{{VMID: 990000}}, 990000, 990009) + got, ok := pickScratchVMID([]proxmox.Guest{{VMID: 990000}}, 990000, 990009, nil) if !ok || got != 990001 { t.Errorf("pick = %d,%v want 990001,true", got, ok) } // full band. full := []proxmox.Guest{{VMID: 990000}, {VMID: 990001}} - if _, ok := pickScratchVMID(full, 990000, 990001); ok { + if _, ok := pickScratchVMID(full, 990000, 990001, nil); ok { t.Error("full band must return ok=false") } + // the exclude set (F2 band-advance: vmids PVE reported occupied) is honored. + got, ok = pickScratchVMID(nil, 990000, 990009, map[int]bool{990000: true, 990001: true}) + if !ok || got != 990002 { + t.Errorf("exclude-set pick = %d,%v want 990002,true", got, ok) + } + if _, ok := pickScratchVMID(nil, 990000, 990001, map[int]bool{990000: true, 990001: true}); ok { + t.Error("a fully-excluded band must return ok=false") + } } func TestWithLinkDown(t *testing.T) { @@ -289,10 +398,11 @@ func TestWithLinkDown(t *testing.T) { // --- recover the leaked scratch guest (the headline crash-safety test) --- func TestRecover_LeakedScratchDestroyed(t *testing.T) { - // The scratch guest still exists at startup (agent crashed mid-test) → Recover destroys it. + // The scratch guest still exists at startup (agent crashed mid-test AFTER the restore + // launched — the entry carries the UPID = the launch proof) → Recover destroys it. api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 990000, Status: "running"}}} e, j, _ := newEngine(t, api, EmptyProvider{}) - if err := j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()}); err != nil { + if err := j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()}); err != nil { t.Fatal(err) } res := e.Recover(context.Background()) @@ -312,7 +422,7 @@ func TestRecover_LeakedScratchAlreadyGone(t *testing.T) { // idempotent clean (no destroy issued). api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 9001, Status: "stopped"}}} // 990000 absent e, j, _ := newEngine(t, api, EmptyProvider{}) - j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()}) + j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()}) res := e.Recover(context.Background()) if res.ScratchClean != 1 || len(api.destroys) != 0 { t.Fatalf("already-gone scratch must be clean with no destroy, got res=%+v destroys=%+v", res, api.destroys) @@ -325,7 +435,7 @@ func TestRecover_LeakedScratchAlreadyGone(t *testing.T) { func TestRecover_LeakedScratchListUnreadable(t *testing.T) { api := &fakeAPI{listErr: errors.New("api down")} e, j, _ := newEngine(t, api, EmptyProvider{}) - j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()}) + j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()}) res := e.Recover(context.Background()) if res.Unresolved != 1 || len(j.InFlight()) != 1 { t.Fatalf("unreadable list must leave the scratch in-flight for a later Recover, got res=%+v inflight=%d", res, len(j.InFlight())) @@ -335,6 +445,26 @@ func TestRecover_LeakedScratchListUnreadable(t *testing.T) { } } +// TestRecover_ScratchNoUPIDAbandoned (F1c, scratch twin of the bring-up test): a crash left a +// Scratch entry with NO UPID while a guest sits at the band vmid (an invisible squatter, or a +// pre-existing guest under a broad token). Recover must ABANDON, never destroy — no journaled +// UPID ⇒ the restore-test created nothing there. +func TestRecover_ScratchNoUPIDAbandoned(t *testing.T) { + api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 990000, Status: "stopped"}}} // a guest IS at the vmid + e, j, _ := newEngine(t, api, EmptyProvider{}) + j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpStarted, At: time.Now().UTC()}) + res := e.Recover(context.Background()) + if len(api.destroys) != 0 { + t.Fatalf("a no-UPID Scratch entry must NEVER destroy the vmid: destroys=%+v", api.destroys) + } + if res.RolledBack != 1 || res.ScratchDestroyed != 0 { + t.Fatalf("entry must be abandoned via the no-UPID fail-safe path, got %+v", res) + } + if len(j.InFlight()) != 0 { + t.Errorf("abandoned entry must be terminal: %+v", j.InFlight()) + } +} + // small string helpers (avoid importing strings in the test for one call). func contains2(s, sub string) bool { return indexOf(s, sub) >= 0 } func count(s, sub string) int {