Implement Fake.FireEvent(domainName string, event libvirt.DomainEvent...

[colonelpanik]

Goal

Implement Fake.FireEvent(domainName string, event libvirt.DomainEventType, detail int) in internal/libvirtfake/libvirtfake.go so the callback stored by RegisterDomainEventCallback can be invoked from a test, then add a tests/fleet/ scenario that fires DomainEventCrashed for a VM and asserts the daemon's handler transitions it to state=error (and that a VM with StateDetail=="operator-stop" is left alone).

Plan

Plan — Fake.FireEvent + a fleet scenario for the domain-event crash handler

Goal

1. Implement Fake.FireEvent(domainName string, event libvirt.DomainEventType, detail int) in
   internal/libvirtfake/libvirtfake.go, so the callback stored by
   RegisterDomainEventCallback can be driven from a test.
2. Add a tests/fleet/ scenario that fires DomainEventCrashed for a VM and asserts the
   daemon's handler moves it to state=error, while a VM carrying
   state_detail="operator-stop" is left untouched.

---

What exists today (facts a reviewer should check first)

Thing	Where	Today's behaviour
Callback type + event constants	internal/libvirt/client.go:23-34	DomainEventCallback func(domainName string, event DomainEventType, detail int); Started=0 Stopped=1 Crashed=2 Shutdown=3
Real client registration	internal/libvirt/client.go:139-146	stores c.eventCallback under c.mu
Backend interface	internal/grpcapi/libvirt_iface.go:141	RegisterDomainEventCallback is on LibvirtBackend purely so the same backend can be passed to daemon.Run
The handler under test	internal/daemon/daemon.go:596-614	anonymous closure registered on d.virt
Fake's stub	internal/libvirtfake/libvirtfake.go:1244-1247	no-op with the comment "Scenarios that want to drive callbacks can call Fake.FireEvent directly (TODO if needed)"

The handler's exact logic (daemon.go:598-613), which the fleet test must pin:

switch event {
case libvirt.DomainEventCrashed, libvirt.DomainEventStopped:
    vm, err := corrosion.GetVM(ctx, d.db, domName)
    if err != nil || vm == nil || vm.HostName != d.cfg.HostName { return }
    if vm.StateDetail == "operator-stop" { return }   // don't act on intentional stops
    slog.Warn(...)
    if err := corrosion.UpdateVMState(ctx, d.db, domName, "error",
        fmt.Sprintf("domain event: stopped (detail=%d). Check host dmesg for OOM.", detail)); err != nil {
        slog.Error(...)
        stateWriteMetrics.Failed(corrosion.OpVMState, corrosion.ClassifyWriteErr(err))
    }
}

⚠️ The finding that shapes this plan

Nothing in production ever calls c.eventCallback. Verified:

$ grep -rn "eventCallback\|RegisterDomainEventCallback" --include=*.go . | grep -v _test.go
internal/grpcapi/libvirt_iface.go:141:  RegisterDomainEventCallback(cb libvirt.DomainEventCallback)
internal/libvirt/client.go:45:          eventCallback DomainEventCallback
internal/libvirt/client.go:142/144:     (the setter)
internal/daemon/daemon.go:597:          (the registration)
internal/libvirtfake/libvirtfake.go:1244:(the fake's no-op)

internal/libvirt never subscribes to go-libvirt's lifecycle-event stream (no
LifecycleEvents/SubscribeEvents call anywhere), so the "immediate VM death detection
(#44)" path stores a callback that is never invoked. The only existing invocation in the
tree is internal/libvirt/client_test.go:46, which pokes the unexported field directly.
Real VM-death detection today comes from the polling health.VMChecker /
health.Reconciler.

Consequences, stated up front so nobody over-reads the new test:

• The fleet scenario proves the handler's decision logic and the fake's dispatch. It
  does not prove that a crashed qemu on a real host reaches the handler — that wiring
  does not exist.
• Wiring golibvirt lifecycle events into c.eventCallback is a real gap but is out of
  scope here; it is called out in "Follow-ups" and should be its own change with its own
  e2e coverage.
• This is also why the handler cannot be tested by "just calling the daemon" — it lives
  inside Daemon.Run, a ~700-line function nothing can invoke from a test.

---

Design decisions

D1 — FireEvent shape: match the existing comment exactly

func (f *Fake) FireEvent(domainName string, event libvirt.DomainEventType, detail int)

No return value, no *testing.T, no error. It is the fake's analogue of
SetState/SetStateReason — a scenario-driver, not an assertion helper.

D2 — store the callback under f.mu, invoke it outside the lock

The callback field joins the other per-domain state on the Fake struct and is written /
read under f.mu. But FireEvent must copy the callback out, release f.mu, and only
then invoke it. sync.Mutex is not reentrant, and the callback is arbitrary
test/daemon code that may call back into the fake (DomainExists, SetState,
DomainState, …). Holding the lock across the call is a self-deadlock waiting to happen —
this is the single most important correctness detail in the change, and it gets its own
unit test (T1c).

RegisterDomainEventCallback stays a safe no-op for the ~40 existing fleet tests: storing
a callback has no effect until something fires an event, and nothing fires one unless a
scenario calls FireEvent. A FireEvent with no callback registered returns silently.

D3 — extract the daemon handler so the test drives the real one (recommended: internal/health)

The fleet harness builds grpcapi.NewServerForTests(...) (tests/fleet/cluster.go:392-...);
it does not run daemon.Run. So the handler has to become a callable unit or the test
is testing a copy of it.

There is already a cautionary precedent in-tree:
tests/fleet/opjournal_recovery_test.go:17 says "The lookup below mirrors
Daemon.runOperationRecovery" — a hand-copy that can silently drift from the code it claims
to cover. Under CLAUDE.md's mutation-verify rule, re-copying the crash handler into the
test would produce a test that passes with the daemon's handler deleted. Not acceptable.

Recommendation: move the closure body into internal/health/domain_events.go.

// DomainEventHandler turns libvirt domain lifecycle events into cluster state.
// It is the event-driven counterpart of VMChecker's polling sweep and shares its
// rules: only this host's VMs, never an operator-stopped one.
type DomainEventHandler struct {
    hostName         string
    db               *corrosion.Client
    onStateWriteFail func(op, class string)
}

func NewDomainEventHandler(hostName string, db *corrosion.Client) *DomainEventHandler
func (h *DomainEventHandler) SetStateWriteFailObserver(fn func(op, class string))
func (h *DomainEventHandler) Callback(ctx context.Context) lv.DomainEventCallback

Why internal/health and not internal/daemon:

• Zero new dependencies for the fleet test binary. internal/health is already in
  tests/fleet's dep set (6 fleet tests import it). Importing internal/daemon instead
  would add 7 packages — daemon, ui, restapi, watchdog, firewall, failover, cephdeploy,
  including internal/ui's 2.2 MB of go:embed assets — to every fleet test build.
• Cohesion. health already owns runtime→cluster-state reconciliation. VMChecker
  is the polling version of this exact decision, carries the same operator-stop skip,
  and already uses the SetStateWriteFailObserver(func(op, class string)) convention this
  handler needs for stateWriteMetrics.Failed. health.NewReconciler(name, dir, db, virt)
  is likewise constructed directly by fleet tests today
  (tests/fleet/failover_invariants_test.go:127), so this matches the harness idiom.
• No cycle: health already imports corrosion and lv "internal/libvirt";
  daemon already imports health.

Alternative considered: export it from internal/daemon as
daemon.DomainEventHandler(...) and import internal/daemon from tests/fleet. Smaller
conceptual move (the code never leaves its package), and cmd/litevirt tests already
import internal/daemon, so it is not unprecedented. Rejected on the dependency-weight
and cohesion grounds above — but it is a legitimate reviewer preference, and switching to
it changes only which package the new file lives in, not the tests or the harness wiring
shape. If the reviewer prefers it, say so and it's a one-file relocation.

The move must be behaviour-identical — same switch, same guard order, same
state="error", same state_detail format string, same slog calls, same failure
observer. daemon.Run keeps its registration and its #44 comment:

// Register domain event callback for immediate VM death detection (#44).
eventHandler := health.NewDomainEventHandler(d.cfg.HostName, d.db)
eventHandler.SetStateWriteFailObserver(stateWriteMetrics.Failed)
d.virt.RegisterDomainEventCallback(eventHandler.Callback(ctx))

D4 — register the handler in the harness (cluster.go), not in the test file

Wire it once in buildServer, so every fleet node behaves like a real daemon and a
scenario only has to call n.Virt.FireEvent(...):

// Domain lifecycle events: the daemon registers this same handler on its libvirt
// client (internal/daemon Run). Wiring it here lets a scenario call
// n.Virt.FireEvent(...) and observe the daemon's real reaction. Inert for every
// other scenario — the fake dispatches nothing unless a test fires an event.
n.Virt.RegisterDomainEventCallback(
    health.NewDomainEventHandler(n.Name, n.DB).Callback(context.Background()))

This is what makes the wiring — not just the function — part of what the test covers:
deleting this line turns the scenario red (mutation M5 below). Blast radius on the other
fleet tests is nil, since nothing else calls FireEvent.

context.Background() is used because the harness has no daemon-lifetime ctx; the
handler's two corrosion calls are synchronous and complete inside FireEvent. Note it in
the comment.

D5 — assert on corrosion rows only

Every assertion reads corrosion.GetVM(ctx, node.DB, name) → .State / .StateDetail /
.HostName. No EventLog() counting, no call counters. (FireEvent does append a
fire-event entry to the fake's event log for debuggability, consistent with the rest of
the fake — but no test asserts on it.)

D6 — seed only through registered corrosion writers

tests/fleet/owner_epoch_test.go:100-104 records that the receiver's apply guard refuses
ad-hoc statement shapes, so a test that seeds with inline SQL breaks the moment its
mutations are pumped to a peer. Seeding therefore uses corrosion.InsertVM (which carries
StateDetail directly — internal/corrosion/vms.go InsertVMWithHardware, the INSERT INTO vms (..., state_detail, ...) statement), never a raw UPDATE.

---

Files to change

1. internal/libvirtfake/libvirtfake.go

• Add to the Fake struct, alongside reasons / ownerEpochs (~line 57-59):

  eventCB libvirt.DomainEventCallback // registered by RegisterDomainEventCallback; guarded by f.mu

  No New() change needed (nil zero value is the "nothing registered" state).
• Replace the stub at lines 1244-1247:

  func (f *Fake) RegisterDomainEventCallback(cb libvirt.DomainEventCallback) {
      f.mu.Lock()
      defer f.mu.Unlock()
      f.eventCB = cb
  }
  
  // FireEvent delivers a libvirt domain lifecycle event to the callback the daemon
  // registered, so a scenario can drive the crash/stop path without a real libvirtd.
  // A no-op when nothing is registered.
  //
  // The callback is copied out and f.mu is RELEASED before it runs: it is arbitrary
  // daemon code that may call back into the fake, and f.mu is not reentrant.
  func (f *Fake) FireEvent(domainName string, event libvirt.DomainEventType, detail int) {
      f.mu.Lock()
      f.record("fire-event", domainName, fmt.Sprintf("event=%d detail=%d", int(event), detail))
      cb := f.eventCB
      f.mu.Unlock()
      if cb == nil {
          return
      }
      cb(domainName, event, detail)
  }

  (f.record assumes the lock is held — see its callers — and uses f.Now, which New()
  sets to time.Now.)
• FireEvent deliberately does not mutate f.domains; a scenario that wants the
  fake's domain state to agree with the event calls SetState itself. Event injection and
  state are kept orthogonal, matching SetStateReason.
• No change to grpcapi.LibvirtBackend: FireEvent is a fake-only driver, like SetState.

2. internal/health/domain_events.go (new, ~55 lines)

The extracted handler per D3. Body copied verbatim from daemon.go:598-613, with
d.db→h.db, d.cfg.HostName→h.hostName, and the metric call routed through the
existing noteStateWriteFail-style helper (h.onStateWriteFail(corrosion.OpVMState, corrosion.ClassifyWriteErr(err)), nil-safe).

writecheck note: UpdateVMState is in its guarded set
(scripts/ci/writecheck/main.go), so the if err := corrosion.UpdateVMState(...); err != nil form must be preserved exactly. There are no path allowlists in the guard, so moving
the call between packages is fine as long as the error is still checked.

3. internal/daemon/daemon.go

Replace lines 596-614 with the three-line wiring in D3. Net: -17 lines. libvirt and
fmt imports may become unused in that file — check and drop only if so.

4. tests/fleet/cluster.go

In buildServer, right after n.Virt = libvirtfake.New() and the
grpcapi.NewServerForTests call, add the registration from D4. New imports:
internal/health (already in the fleet dep set); context is already imported.

5. tests/fleet/domain_event_test.go (new) — the scenario

6. internal/libvirtfake/libvirtfake_test.go (new) — first test file in that package

---

Tests

T1 — internal/libvirtfake/libvirtfake_test.go (unit)

• T1a TestFake_FireEvent_NoCallbackIsNoOp — New(), then
  FireEvent("vm", libvirt.DomainEventCrashed, 0); must not panic. Pins the constraint
  that the fake stays safe for scenarios that never register.
• T1b TestFake_FireEvent_DeliversToRegisteredCallback — register a recording
  callback, fire (name=..., DomainEventCrashed, detail=7), assert the callback saw
  exactly those three values; re-register a second callback and assert the first no longer
  receives.
• T1c TestFake_FireEvent_CallbackMayReenterTheFake — register a callback whose body
  calls f.DomainExists(name) and f.SetState(name, StateShutdown). Run FireEvent in a
  goroutine and select on a done channel vs time.After(2*time.Second), failing with
  t.Fatal on timeout — so the lock bug fails as a red test in 2s instead of hanging
  the package until the 10-minute panic timeout. This is the test that would catch someone
  "simplifying" FireEvent to defer f.mu.Unlock().

T2 — tests/fleet/domain_event_test.go (the required scenario)

TestFleet_DomainEventCrashed_MarksVMErrorAndSparesOperatorStop, Options{Nodes: 2},
nodes a (the host firing events) and b (peer):

Seed — three VMs via corrosion.InsertVM(ctx, a.DB, ...):

name	host_name	state	state_detail
vm-crash	a	running	(empty)
vm-opstop	a	stopped	operator-stop
vm-remote	b	running	(empty)

Then pumpMutations(t, c, a, b) (defined in owner_epoch_test.go, same package, reused by
5 other fleet files) so both nodes agree before the event, and
a.Virt.SetState("vm-crash", libvirtfake.StateShutdown) so the fake's view matches a
crashed domain.

Act — from node a:

a.Virt.FireEvent("vm-crash",  lv.DomainEventCrashed, 3)
a.Virt.FireEvent("vm-opstop", lv.DomainEventCrashed, 3)
a.Virt.FireEvent("vm-remote", lv.DomainEventCrashed, 3)
a.Virt.FireEvent("vm-crash2", lv.DomainEventStarted, 0)  // see assertion 4

Assert — all reads via corrosion.GetVM(ctx, a.DB, ...):

1. The required positive: vm-crash → State == "error" and StateDetail contains
   detail=3 (substring match on the format string, not the whole sentence — the OOM
   advice text is not a contract).
2. The required negative: vm-opstop → still State == "stopped",
   StateDetail == "operator-stop".
3. Foreign-host guard: vm-remote → still State == "running", HostName == b.Name.
   Cheap, same handler, and the guard is one line above the operator-stop one.
4. Non-lifecycle event ignored: a fourth VM vm-idle (host a, running) fired with
   DomainEventStarted stays running — pins the switch, so widening it to a bare
   default cannot pass.
5. Multi-node dimension (the reason this belongs in tests/fleet/, not a unit test):
   pumpMutations(t, c, a, b) again, then assert node b's vm-crash row also reads
   error. This runs the real spine — mutation_log → PushMutations over real
   gRPC/mTLS → applyStatementLWW + the receiver's statement-shape apply guard — and would
   catch the crash write being emitted in a shape a peer silently discards. b's
   vm-opstop and vm-remote must be unchanged there too.

Assertions 3-5 are additions beyond the literal ask; they cost ~15 lines and reuse the same
seed, but they are explicitly flagged here so the reviewer can trim them if they want the
scenario kept to exactly the two required cases.

T3 — mutation verification (CLAUDE.md: "a passing test proves nothing until you have seen it fail")

Each row: break it, run go test ./tests/fleet/ -run DomainEvent ./internal/libvirtfake/,
confirm red, restore. Record the results in the PR description.

#	Mutation	Must break
M1	RegisterDomainEventCallback back to a no-op (drop f.eventCB = cb)	T2 assertion 1, T1b
M2	FireEvent returns before invoking cb	T2 assertion 1, T1b
M3	Delete if vm.StateDetail == "operator-stop" { return }	T2 assertion 2
M4	Delete vm.HostName != h.hostName from the guard	T2 assertion 3
M5	Delete the RegisterDomainEventCallback line from cluster.go buildServer	T2 assertion 1
M6	UpdateVMState(..., "error", ...) → "stopped"	T2 assertion 1
M7	Widen the switch to act on every event type	T2 assertion 4
M8	FireEvent holds f.mu across cb(...) (defer f.mu.Unlock())	T1c (fails in 2s, does not hang)

Why the vacuity risk is real here: FireEvent no-ops when no callback is registered, so
a negative-only test ("operator-stop VM unchanged") would pass with the entire feature
deleted. M1/M5 are the mutations that prove it doesn't — which is why the positive
assertion and the negative assertion must live in the same test, sharing one
registration.

---

Verification

go build ./... && go vet ./...
go test ./internal/libvirtfake/ ./internal/health/ ./internal/daemon/ ./tests/fleet/ -count=1
go test ./...
make test-fleet-race            # the harness + a new cross-goroutine callback field
make ci-guards

Expected guard outcomes, and why:

• writecheck — unaffected; the UpdateVMState call keeps its if err := ... form
  (D3/§2). It has no path allowlist, so the package move is invisible to it.
• stmtshapecheck — unaffected; no new or modified SQL builder. The handler reuses
  corrosion.UpdateVMState verbatim. (reachable.go only tracks unexported builders of
  replicated statements.)
• check-schema-bump / ledger drift — no schema change.
• docs triangulation (cmd/litevirt/docs_triangulation_test.go) — no new CLI command,
  config key, or litevirt_* identifier, so no doc updates are owed. The existing
  litevirt_state_write_failures_total metric is reached by the same observer as before.

---

Reviewer notes / risks

1. Read the "⚠️ finding" section first. The handler this test pins has no production
   trigger. The change is still worth making — it converts a documented TODO into a working
   test seam and puts the handler's rules under test — but the PR description must not
   claim "crash detection is now tested end-to-end".
2. The handler moves packages. Confirm the extracted body is byte-for-behaviour
   identical to daemon.go:598-613: guard order, state="error", the
   "domain event: stopped (detail=%d). Check host dmesg for OOM." format, both slog
   lines, and stateWriteMetrics.Failed(corrosion.OpVMState, ClassifyWriteErr(err)). If
   the reviewer would rather it stay in internal/daemon (exported), that's a one-file
   relocation; see the alternative in D3.
3. Lock discipline in FireEvent is the one place a plausible-looking edit
   (defer f.mu.Unlock()) introduces a deadlock. T1c exists to catch exactly that and is
   written to fail fast rather than hang.
4. The crash write is name-only. UpdateVMState is
   UPDATE vms SET ... WHERE name = ? — the same shape owner_epoch_test.go documents as
   the 2026-08-01 rejoin-fight footgun, unlike the epoch-guarded UpdateVMStateAtEpoch.
   T2 assertion 5 will therefore show the write landing on the peer. That is today's
   behaviour, faithfully pinned; whether the crash handler should be epoch-guarded is a
   separate design question and is not changed here. If the team wants it guarded,
   assertion 5 is the test that would have to change with it.
5. Harness-level registration (D4) touches cluster.go, shared by ~40 fleet tests.
   It is inert without a FireEvent call, but it does mean every node now holds a closure
   over n.DB; a scenario that fired an event after Cluster.Stop() would hit a closed DB.
   No current test does.
6. internal/libvirtfake gains its first _test.go. Intentional — the fake now has
   behaviour (lock handoff, nil-callback path) that is worth pinning in place, not just
   plumbing.

Follow-ups (explicitly out of scope)

• Wire go-libvirt's lifecycle-event stream into Client.eventCallback so the daemon's
  handler has a real trigger, with tests/e2e/ coverage against a real crashed domain.
• Consider UpdateVMStateAtEpoch for the crash write once the handler is genuinely
  reachable (see note 4).
• Replace the hand-mirrored lookup in tests/fleet/opjournal_recovery_test.go:17 with the
  same extract-and-share treatment used here.

---

Deviations taken during implementation

1. T2 assertion 4 fired at an unseeded VM — would have been vacuous (plan bug)

The plan's Act block fires the non-lifecycle event at vm-crash2:

a.Virt.FireEvent("vm-crash2", lv.DomainEventStarted, 0)  // see assertion 4

but assertion 4 describes "a fourth VM vm-idle (host a, running)", and the seed table
lists only three VMs — neither vm-crash2 nor vm-idle is ever inserted.

That combination makes the assertion vacuous. Firing at a name with no vms row means
corrosion.GetVM returns nil, so the handler's vm == nil guard returns early whatever
the switch does. Mutation M7 ("widen the switch to act on every event type"), which
this assertion exists to catch, would have stayed green: vm-idle would have had no row to
read and no event fired at it either way.

Implemented instead: seed a fourth VM vm-idle (host a, state running) alongside the
other three, and fire DomainEventStarted at that name. Verified: M7 now fails on exactly
this assertion —

domain_event_test.go:114: vm-idle state = "error" after DomainEventStarted, want "running"

The seed table is therefore four rows, not three.

2. daemon.go imports — the plan's conditional resolved to "no change"

Plan §3 says the libvirt and fmt imports "may become unused in that file — check and drop
only if so". Checked: both are still used elsewhere in daemon.go (libvirt.Client,
libvirt.NewClient, libvirt.ResolveFirmwarePaths, and 20+ fmt. calls). No import changed.

3. Mutation-verification results (T3)

All eight ran; each was confirmed to have actually applied (not a silent no-op edit) before
its test run, and each restored cleanly afterwards.

#	Mutation	Result
M1	RegisterDomainEventCallback drops f.eventCB = cb	RED — T2 assertion 1 + T1b
M2	FireEvent returns before invoking cb	RED — T2 assertion 1 + T1b
M3	Delete the operator-stop guard	RED — T2 assertion 2
M4	Delete vm.HostName != h.hostName from the guard	RED — T2 assertion 3
M5	Delete the RegisterDomainEventCallback line from cluster.go	RED — T2 assertion 1
M6	Crash write records "stopped" instead of "error"	RED — T2 assertion 1
M7	Widen the switch to default:	RED — T2 assertion 4
M8	FireEvent holds f.mu across cb(...)	RED — T1c, in ~2s (no hang)

M3/M4/M6/M7 were re-run after the handler was re-indented (see deviation 4) and still go red.

4. Callback keeps the switch inline

A first pass split the body into a separate exported Handle(ctx, domName, event, detail)
method with Callback delegating to it. Nothing called Handle, and the plan's stated API
for the type is NewDomainEventHandler / SetStateWriteFailObserver / Callback — so the
extra exported method was dropped and the switch sits inline in the closure Callback
returns, which is also the exact shape it had in daemon.Run.

Verification run

go build ./..., go vet ./..., go test ./..., make test-fleet-race (477s, ok) and
make ci-guards all pass. As predicted in §Verification, writecheck, stmtshapecheck,
schema-bump/ledger-drift and docs triangulation were all unaffected.

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.