Files
egress-proxies-operator/docs/reviews/2026-09-17-kvm2-provider-review/deep.md
T
kacerrandClaude Opus 5 8fa9e8aad9 Housekeeping: review docs into docs/, refreshed agent guide and settings
Four unrelated working-tree changes, none of them touching operator
behaviour:

- review/*.md -> docs/reviews/2026-09-17-kvm2-provider-review/, byte for
  byte, so the kvm2 review sits with the rest of the docs under a name
  that says which review it was.
- AGENTS.md: replaced the generic kubebuilder scaffold guide with what is
  actually true of this repo — the real component layout, the lint/test
  commands that work here, and the behavioural gotchas (replace-on-change
  hashing, the cloud-init Secret label, in-memory leases).
- .claude/settings.json: the allow list still carried /Users/jan.novak
  paths from the macOS checkout; re-pointed at /home/novakj and collapsed
  the one-off entries into prefixes.
- config/samples/proxy_kvm2_crawl_only_one.yaml: a one-VM cut of the crawl
  pool, the rest of the members left commented out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-18 08:12:27 +00:00

266 lines
36 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# kvm2 provider review: phase 2 deep dives
Ten `deep-reviewer` runs, one per escalation in `escalations.md`. Hypervisor access was
**read-only** (beelink, libvirt 11.6.0, QEMU 10.1.0); every probe that needs a write was left
unrun and is reproduced under "Probe to settle". Scratch programs live under the session
scratchpad, not in the repo. The orchestrator did not re-read code.
| ID | Verdict | One line |
|---|---|---|
| E1 | **CONFIRMED** (narrowed) | Blocking kill reproduced against x/crypto v0.54.0; pins all 3 reconcile workers ~15 min per incident, trigger is a bulk upload, not the name lock |
| E2 | **PLAUSIBLE** | Padded `<uid>` text does make GC delete a live VM (reproduced on a copy of the parser); no trigger libvirt produces on its own. `createdAt` half REFUTED |
| E3 | **mixed** | GC cross-provider: REFUTED. Re-type beyond S2.3: REFUTED. Shared `base-<key>.<format>` cache between kvm and kvm2: CONFIRMED |
| E4 | **CONFIRMED** (narrowed) | Rollout/leader-handover trigger REFUTED; the unfenced base is reachable in one process via the kvm provider or a second kvm2 entry on the same host+pool |
| E5 | **REFUTED** (~85%) | libvirtd finishes or unlinks a volume build regardless of client disconnect; overlay create is 2-11 ms on the host |
| E6 | (a) **CONFIRMED**, (b) **REFUTED** | Upstream race on `l.disconnected` is real; a peer-close right after ConnectOpen wedges a pooled link forever. No goroutine/fd growth over 1000 cycles |
| E7 | **PLAUSIBLE** | Unguarded in-place delete is real; corruption needs an admin `sha256` edit plus a host-side restart of an old overlay (`libvirt-guests` is enabled on beelink) |
| E8 | **CONFIRMED** | `available` is cached per pool in libvirtd (measured), and it is `f_bfree`, 24.7 GB above what QEMU can use on the kvm2 filesystem |
| E9 | (a) **REFUTED**, (b) **CONFIRMED** | Idempotency codes match libvirt 11.6 (observed 42/49/50/80). Codes 67 and the bare `ErrUnsupported` retry forever, per a recorded spec decision |
| E10 | **CONFIRMED** (libvirt half ~90%) | qemu-img accepts a short overlay (measured locally); guest sees a truncated disk, GPT rejected; Proxy stuck Provisioning forever |
Two facts the deep dives surfaced that correct earlier documents:
- **The kvm2 integration test has not run on beelink.** Step 11 is unchecked in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`; every `base-*` volume on the host is dated 2026-09-10 (kvm v1's test, same `test-image` key); both live `proxy-*` domains carry the kvm v1 metadata namespace. The "Checked" line of E2 in `escalations.md` (round trip asserted on libvirt 11.6) was a sweeper claim and is not yet true.
- **Sweep finding S5.3 is half wrong.** A zero `createdAt` does not endanger a live VM: `gc.go:118` tests `live[inst.UID]` before the age case, so it only affects true orphans. The future-dated-mark leak stands.
---
## E1 — wedged `markDead` starves the reconcile workers — CONFIRMED (narrowed)
**Premise reproduced** (`scratchpad/e1/main.go`, in-process SSH server behind a freezable TCP proxy, x/crypto v0.54.0). `sshSocket.Close` (`ssh.go:109-114`) calls `s.Conn.Close()` first: `channel.Close` (`channel.go:594`) → `sendMessage` → `writePacket` takes `ch.writeMu`, then `handshakeTransport.writePacket` (`handshake.go:579`) takes `t.mu`. A data writer stuck in a TCP write holds both. `ssh.Client.Close` is a bare TCP close (`connection.go:111`) and takes no locks.
| Scenario | Client send buffer | Writer blocked in | Channel `Close` |
|---|---|---|---|
| Bulk write, loopback default | 2.6 MB | `window.reserve` (no mutex) | 0 s |
| Bulk write, LAN-sized buffer | 256 K | `net.(*conn).Write` under `writeCipherPacket` | still blocked after 8 s |
| Small RPC-sized write only | 2.6 MB | not blocked | 0 s |
Precondition: the writer must be stuck in the TCP write, not SSH flow control (2 MiB window). That needs a send buffer below ~2 MiB, which the reviewer estimates (not measured) is the case on a LAN/Tailscale path during a bulk upload. In the blocked case a separate `client.Close()` released the channel close at once.
**Breaking path.**
1. Three Proxies placed on one kvm2 host with an uncached image. Worker 1 takes the name lock (`create.go:53`) and image lock (`image.go:87`), then `uploadImage` → `dialFresh` (`image.go:261`) → `StorageVolUpload` (`image.go:270`, 20 min budget). Workers 2 and 3 park in `imageLocks.acquire` (`kvm2.go:224-233`); the reconcile ctx has no deadline. `MaxConcurrentReconciles` is 3 (`proxy_controller.go:442`): all workers occupied.
2. Host is power-cut mid-upload. The upload goroutine blocks in the TCP write holding both mutexes.
3. The dedicated link's keepalive (`hosts.go:156-160`) cannot send (needs `t.mu`); after 15 s `hosts.go:158` calls `markDead`, whose `kill()` blocks in `s.Conn.Close()`. `close(l.closed)` (`backend.go:146`) is never reached.
4. Worker 1 sits in `call`'s select (`backend.go:176-187`): `done` cannot fire, `l.closed` is not closed, and a ctx cancel reaches `markDead` at `:180`, which blocks in `once.Do` behind the keepalive goroutine. Even cancellation is uninterruptible.
5. After ~15.5 min (`tcp_retries2=15`) or the 20 min budget everything unwinds Transient. Meanwhile no Proxy on any provider reconciles.
**Unknowns settled.** No `ReconciliationTimeout` is set (`cmd/main.go:288-295`; controller-runtime v0.24.1 applies one only when > 0), and one alone would not release worker 1. The name-lock part of the hypothesis is practically unreachable (controller-runtime never runs one key concurrently; Delete's RPCs are too small to fill the buffer). GC (`gc.go:104-130`, one goroutine, no deadline) normally moves on; it stalls only if the blocked kill is on the *pooled* link, whose only bulk write is the ~50 KiB seed upload (`create.go:210`): marginal. Of the five `markDead` sites, `hosts.go:153` runs under `e.sem` but only handshake bytes have been sent, so it does not block.
**Minimal fix.** (1) In `sshSocket.Close`, close the TCP connection first: `errors.Join(s.client.Close(), s.Conn.Close())`, filtering the expected "use of closed network connection", or drop `Conn.Close()`. (2) In `markDead` (`backend.go:138-148`), `close(l.closed)` before `l.kill()`, or run `kill` in a goroutine. (3) Regression test in `ssh_test.go`: frozen proxy, small client `SO_SNDBUF`, assert `Close` returns within 1 s while a writer is blocked. (4) Optional: set `ReconciliationTimeout` so image-lock waiters requeue.
**Probe to settle** (not run, needs a bulk upload in flight): `ss -tmi dst <beelink-ip>:22` on the operator host during an image upload; `tb` below ~2.2 MB means the blocking case applies.
## E2 — reformatted GC mark makes GC delete a live VM — PLAUSIBLE
Throwaway program (`scratchpad/e2/main.go`) with a verbatim copy of `instanceMetadata`, the same `time.Parse(time.RFC3339, …)` and the `gc.go:112-122` switch; clock 16 days after the mark, MinAge 10 m.
| Input | Parsed UID | zero time | GC |
|---|---|---|---|
| single line (as written) | clean | no | KEEP |
| libvirt's re-indent between children | clean | no | KEEP |
| `<uid> X </uid>` or newline-padded | padded | no | **DELETE** |
| uppercase UID | differs | no | **DELETE** |
| `<uid> </uid>` (whitespace only) | `" "` | no | **DELETE** |
| `<uid></uid>` | `""` | no | KEEP (guard `gc.go:113`) |
| bad/missing `createdAt`, UID clean | clean | yes | KEEP |
| padded `<managed>` | clean | no | KEEP (not listed) |
| future-dated, UID clean | clean | no | KEEP |
`encoding/xml` does not trim chardata. **Path:** padded or case-changed `<uid>` with `<managed>true</managed>` intact → `parseInstanceMetadata` (`metadata.go:66-78`) returns it untrimmed → `ListByTag` (`list.go:46-49`) emits it → `Sweeper.sweep` (`gc.go:113-121`): not empty, not live, older than MinAge → `prov.Delete` (`gc.go:125`) → `delete.go:21-53` destroys the domain and its disks. There is no second guard: no name-to-Proxy lookup, `Delete` does not re-read the mark, and provider scoping does not help because the owning provider lists it.
**Why only PLAUSIBLE.** On the real host libvirt puts newline + two spaces *between* children, strips the xmlns and leaves text bytes intact (`od -c` of the live `…/kvm` marks). Per libxml2, text-bearing elements are never re-indented, so a version change alone would not pad text. The reachable trigger is a hand edit or a tool that pads text nodes via `virsh edit`/`define`: rare, but the result is an unrecoverable delete. The old `internal/provider/kvm/metadata.go` has the same untrimmed field, and the two live VMs are kvm v1 domains.
**Refuted parts.** A bad `createdAt` never deletes a live VM (UID check comes first, consistent with `docs/provider-contract.md:238-241`). A future-dated mark is a leak only, and marks are stamped from the operator's clock (`create.go:127`), so host skew cannot cause it.
**Minimal fix.** At the end of `parseInstanceMetadata`: `TrimSpace` on `Managed`, `UID` (plus `ToLower`), `CreatedAt`. A whitespace-only UID then equals `""`, which `gc.go:113` keeps. Add table rows to `metadata_test.go`. Apply the same to `internal/provider/kvm/metadata.go`. Optional second guard in `gc.go`: skip and log when `provider.NameFromUID(inst.UID)` is not the name part of `inst.ID` (check the other providers' naming first).
**Probe to settle** (not run; use a scratch libvirt, not beelink):
```
virsh -c qemu:///system metadata <scratch-dom> https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2 --key kvm2 --set '<instance><managed>true</managed><uid> 3f1c2a9e-0000-4000-8000-000000000001 </uid><createdAt>2026-09-01T10:00:00Z</createdAt></instance>' --config
virsh -c qemu:///system metadata <scratch-dom> https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm2 | od -c
```
## E3 — kvm and kvm2 on the same hypervisor — three paths, three verdicts
**1. GC reconciles provider A's list against provider B's Proxies — REFUTED.** `gc.go:91-101` builds one global `live` set from every Proxy; sharing it only makes GC more conservative. kvm2 `ListByTag` (`list.go:38-48`) is namespace-filtered, and that was checked on the host: both live domains return the mark under `…/kvm` and nothing under `…/kvm2`. What remains is leaks: after a re-type the old type's orphans are never collected, and the scoping still rests on libvirt behaviour no unit test covers (S7.4).
**2. Same UID under both types; reverse direction — REFUTED beyond S2.3.** `NameFromUID` depends only on the UID and `spec.provider` is CEL-immutable (`proxy_types.go:221`), so the same name under both types only comes from re-typing an entry. Nothing prevents that: `config.go:93-135` checks only name uniqueness and block/type agreement, there is no webhook (`cmd/main.go:262`), `status.providerID` records no type, and the controller never compares `inst.UID` with `p.UID`. What gets deleted is still that Proxy's own instance. Exceptions: a tampered `status.providerID` (kvm2 `Delete` checks no mark, so `hosts/beelink/domains/docker-29` would destroy an unrelated VM); the reverse direction has no guard at all (kvm `Create` adopts any same-named domain, `kvm/kvm.go:177-181`; its `Get`/`Delete` check no mark); and after a re-type kvm2 `ensureOverlay` boots a stale overlay the kvm provider left behind (`create.go:171-177`), one Proxy's own disk only.
**3. Shared `base-<key>.<format>` cache — CONFIRMED** (kvm2 code + libvirt 11.6.0 source, `storage_driver.c`). The marker protocols are compatible (both write `<base>.sha256` with 64 hex; both key URL images as `sha256(url)[:16]`), so kvm2 adopts a kvm-cached base. On the host every base and marker dates from 2026-09-10 (kvm-made), both live overlays back onto `/srv/vms/images/base-debian-13.qcow2`, and the sample `kvm2-home` entry's `debian-13` key would share exactly that volume. Both integration tests use key `test-image`.
- **3a Digest mismatch.** `kvm-home` cached `debian-13` from a `latest` URL (digest D1). The operator gives `kvm2-home` `sha256: D2`. First kvm2 Create: `cachedImage` `image.go:129` → `:145` deletes marker and base. libvirt's `storageVolDelete` refuses only on `in_use`/`building` (`storage_driver.c:1844-1856`), not on backing-store use. kvm2 uploads a different image to the same path under the kvm provider's live overlays. With both entries pinned to different digests, every Create on either side deletes and refetches.
- **3b Concurrent cold fetch** (by source, not executed). Image locks are per `Provider` instance (`kvm2.go:111`; `kvm/image.go:62-74`), so this applies kvm-vs-kvm2 and kvm2-vs-kvm2 alike. libvirt holds `in_use` only while the stream is set up (`storage_driver.c:2421-2427`), so the volume is unguarded for the whole transfer. Entry B finds base-without-marker and deletes it (`kvm2/image.go:117-118`; `kvm/image.go:103-108`), recreates it and starts its own upload. A's upload finishes cleanly into the unlinked inode, the digest matches, A writes the marker, and `StorageVolGetPath` resolves A's handle to B's partial file: A boots a guest from a half-written base. If B is kvm2, `writeMarker` then fails `ErrStorageVolExist` and `image.go:241-243` discards the now-complete base under A's running domain.
**Minimal fix.** Give kvm2 its own prefix (`volume.go:17` → `kvm2-base-…`, one extra ~415 MB download per host). Reject two kvm/kvm2 entries with the same address+socket+pool at load, or treat `ErrStorageVolExist` on the marker as success when its digest equals ours. Put the digest in the base name when `sha256` is set. Make `removeDomain` require `meta.managed()` and the expected UID; give `kvm/kvm.go:177` the same guard. Add a fake keyed on the metadata URI (S7.4).
**Probe to settle 3b** (not run):
```sh
truncate -s 4G /tmp/probe-race.bin
virsh -c qemu:///system vol-create-as default probe-race.raw 4G --format raw
virsh -c qemu:///system vol-upload --pool default probe-race.raw /tmp/probe-race.bin &
sleep 1; virsh -c qemu:///system vol-delete --pool default probe-race.raw; echo "delete exit=$?"
wait; echo "upload exit=$?"; virsh -c qemu:///system vol-list default | grep probe-race; rm /tmp/probe-race.bin
```
## E4 — no cross-process fencing on the base volume — CONFIRMED (narrowed)
**Rollout / leader handover: REFUTED for the shipped manifests.** `manager.yaml:64` passes `--leader-elect` (the `false` default at `cmd/main.go:107` applies only to a bare binary or `make run`). No strategy is set, so RollingUpdate with surge 1: old and new pods coexist, but the reconciler is a leader-election runnable, `LeaderElectionReleaseOnCancel` is not set (`cmd/main.go:288-295`), and the old pod holds the lease until it dies. Lease 15 s / renew 10 s / retry 2 s; the kubelet SIGKILLs at 10 s (`manager.yaml:180`). On cancel kvm2 stops the upload at once (`backend.go:179-181` → `markDead`); `discardVolume` runs uncancelled for up to 30 s (`image.go:331`) but is cut by SIGKILL. The residual two-process window needs A's delete RPC to stall ~7 s while B dials, HEADs and creates within ~5 s: theoretical, and self-healing (`image.go:100`, `:135`). Other two-process cases: `make run` without `--leader-elect` against the cluster's hypervisor; `make test-integration-kvm` and `-kvm2` at the same time (same `test-image` key).
**In one process: CONFIRMED.** The kvm provider is an always-present second writer: same names (`kvm/image.go:44-57` vs `kvm2/volume.go:17-25`), same default pool, same marker format, separate locks (`kvm/image.go:77-87` vs `Provider.imageLocks`; `newProvider` builds fresh `keyedLocks` per entry, `kvm2.go:111`, so two kvm2 entries do not share one either), concurrent via `MaxConcurrentReconciles: 3`, documented as supported (`docs/kvm2-provider.md:6-8`) and shipped that way (`config/manager/providers_config.yaml`: both entries on `beelink`, key `debian-13`, default pool). The interleaving is E3-3b. Extra consequence: after A's marker is written, if the process dies during B's upload (rollout SIGKILL, OOM at the 128Mi limit) the pool keeps a **truncated base under a valid marker**, and nothing re-hashes the base (`cachedImage` compares marker text only), so every later Create on that host backs onto it until a manual `vol-delete`.
Caveat: that `StorageVolDelete` succeeds mid-stream came from memory here; the E3 reviewer read it in the 11.6.0 source, which corroborates it. Still unexecuted.
**Minimal fix.** (1) Remove the shared name (kvm2-specific prefix; include the entry name or share one process-wide `keyedLocks`). (2) Make the base appear atomically: upload to `<base>.part-<random>`, verify, then create the final name; treat `ErrStorageVolExist` as "someone else won" and re-check; never delete a name this call did not create, in particular not `discardVolume(base)` on a marker `ErrStorageVolExist` (`image.go:241-243`). (3) Store the size next to the digest and check the base's allocation before trusting the marker. Set `strategy: Recreate` and `LeaderElectionReleaseOnCancel` deliberately.
**Probe to settle** (not run):
```
virsh -c qemu:///system vol-create-as default e4probe.raw 2G --format raw
mkfifo /tmp/e4.fifo; ( head -c 2G /dev/zero | pv -L 20m > /tmp/e4.fifo & )
virsh -c qemu:///system vol-upload --pool default e4probe.raw /tmp/e4.fifo &
sleep 5
virsh -c qemu:///system vol-delete --pool default e4probe.raw # succeeds => confirmed path; "still in use" => narrowed
virsh -c qemu:///system vol-create-as default e4probe.raw 2G --format raw
virsh -c qemu:///system pool-refresh default
wait; virsh -c qemu:///system vol-delete --pool default e4probe.raw; rm /tmp/e4.fifo
```
## E5 — overlay atomicity under an abandoned RPC — REFUTED (~85%, libvirtd side from source memory)
`storageVolCreateXML` rejects a duplicate name, adds the volume with `building = true`, bumps asyncjobs, drops the pool lock, runs `qemu-img create` via `virCommandRun`; on build failure the file is unlinked and the volume removed from the list. It runs on an RPC worker thread holding a client reference, so a disconnect does not cancel it; storage objects have no close callbacks. After a client drop a name is either still building or complete. Pool refresh refuses while asyncjobs > 0, and kvm2's XML has no `<allocation>` and passes `flags = 0`, so the preallocating path is never taken. The spec's "creation is atomic" (`kvm2-provider-spec.md:227-229`) holds.
**Cost on the host.** All six pools are `type='dir'` on local storage. Three leftover overlays show birth-to-mtime gaps of 2-11 ms and are each exactly 196768 bytes, complete. A create over 30 s needs a stalled filesystem.
**Residual path** (loud, never boots a corrupt overlay): Create #1 stalls > 30 s, Create #2 gets `ErrStorageVolExist` while `building` is set; if it reaches `DomainCreate`, QEMU cannot open the file (write lock, or "not in qcow2 format") → Transient → next retry starts cleanly. If `qemu-img` ends in EIO, libvirtd removes the volume and the defined domain points at a missing file; every retry fails until the Proxy is deleted (this is sweep finding S2.4's mechanism).
**Fix options.** None needed for the hypothesis. Minimal: correct the comment at `create.go:151-153` and the spec line to state the real reasons. Hardening: `deleteVolumeIfPresent` before `StorageVolCreateXML` in `ensureOverlay`, as the seed already does (no domain exists at that point under the name lock; cost 2-11 ms; a building volume returns `ErrOperationInvalid`, already Transient).
**Probe to settle** (not run, ~420 MiB in `default`):
```sh
C=qemu:///system; V=review-e5-clone.qcow2
virsh -c $C vol-clone --pool default base-debian-13.qcow2 $V & P=$!
sleep 0.3; kill -9 $P
for i in $(seq 1 40); do virsh -c $C vol-info --pool default $V 2>&1 | tr '\n' ' '; echo; stat -c '%s' /srv/vms/images/$V 2>&1; sleep 0.5; done
virsh -c $C vol-dumpxml --pool default $V
until virsh -c $C vol-delete --pool default $V 2>&1 | grep -qv 'still being allocated'; do sleep 1; done
virsh -c $C vol-list default | grep review-e5- || echo clean
```
## E6 — go-libvirt when the socket is killed under it — (a) CONFIRMED, (b) REFUTED
Scratch module `scratchpad/lvleak/` (same go-libvirt and x/crypto versions; copies kvm2's `link`, `call`, `connect`, `Close`, `sshSocket` in shape).
**(a) Data race on `Libvirt.disconnected`, reported by `-race`** over a real in-process SSH `direct-streamlocal` channel: read in `waitAndDisconnect` (`libvirt.go:821`, goroutine started at `:297`), write in `ConnectToURI` (`:305`), unsynchronised. Interleaving: ConnectOpen reply is routed → socket dies → `listen` closes `s.disconnected` (`socket.go:208`) → `waitAndDisconnect` reads the old channel and returns (`:828`) → the requester installs a fresh channel at `:305` that nothing will ever close, and `Connect()` returns nil.
- **Hypothesised trigger (kvm2's own `markDead`): harmless.** `call` has already taken the timer/ctx branch; the nil result lands in the buffered `done` and is dropped; `hosts.go:153` discards the backend. Measured 5, 1 and 33 times per 1000 cycles, no effect.
- **Peer-close trigger: real defect.** If libvirtd or the forwarded socket goes away within ~1 ms of a successful open (e.g. a daemon restart), `Connect` wins, the watcher (`backend.go:229-236`) holds a channel that never closes, every later RPC fails at `SendPacket` with `syscall.EINVAL` (`socket.go:321-325`), which `errors.go:55` classifies Transient. No call result marks the link dead, `hostTable.get` (`hosts.go:75`) keeps returning it, and the SSH keepalive keeps succeeding if sshd is alive. The host answers "invalid argument" until the operator restarts. Wedged in 953/1000 (unix), 824/1000 (`net.Pipe`), 546/1000 (SSH), 186/500 (SSH, `-race`) when the peer closes right after the open reply. On the Check path the same loser only costs a 5.001 s `Close()`.
**(b) No accumulation.** Six scenarios (peer never answers then kill; RPC hangs; ConnectClose hangs into the close budget; peer closes mid-handshake; clean probe; kill racing the end of the handshake) x three transports, 1000 cycles each (500 under `-race`): goroutines +0, fds +0. Peak in flight 26.
**Side findings (need a misbehaving peer).** (i) `callback` does an unbuffered `c <- res` holding `cmux` (`rpc.go:76-84`): a duplicate reply deadlocks `listen`, leaks three goroutines per hit (+420 over 200 cycles), and a socket kill does not help. On the stream path serials do repeat, which is why the S4.1 leak also survives a kill. (ii) `socket.go:233-234` computes `int(length) - 28` and `make`s it: a length field under 28 panics "makeslice: len out of range" in the listen goroutine and takes the operator down (reproduced); a large bogus length allocates up to 4 GiB. Reachable if `host.Socket` points at something that is not libvirt.
**Minimal fix.** In `call` (`backend.go:177-178`): on `done`, if the error is `syscall.EINVAL` or `libvirt.ErrInterrupted`, `markDead` before returning; add a `backend_test.go` case where the peer closes right after the open reply. Sturdier: give `libvirt.New` a `net.Conn` wrapper whose `Read` calls `link.markDead` on any non-temporary error, dropping the dependence on `Disconnected()` and the 5 s Close stall. Upstream: assign `l.disconnected` before `go l.waitAndDisconnect()`; send outside the lock in `callback`; reject `length < 28` and oversized lengths.
## E7 — in-place base replacement under live overlays — PLAUSIBLE
**Confirmed part.** Admin sets or changes `images.<key>.sha256` and restarts the manager (config is not hot-reloaded). Next Create: `image.go:129` sees the mismatch, `:145` deletes marker and base with no in-use check and a `V(1)` log; `fetchImage` creates a new file at the same path. `TestEnsureImage_sha256ChangeRedownloads` (`image_test.go:394`) asserts exactly this. A mistyped `sha256` is worse: the base is deleted first, then `image.go:289` returns Permanent, leaving every old overlay with a missing backing file (loud). libvirt does not refuse (~95% from the reviewer; the E3 reviewer read it in source).
**Why corruption is only plausible.** Running VMs keep the old inode, and `on_reboot=restart` (`xml.go:217`) keeps the same QEMU process. Corruption needs a *new* QEMU process on an old overlay:
- Controller: safe. Stopped → Delete (`proxy_controller.go:216-225`) removes the overlay (`delete.go:91`).
- **`libvirt-guests` on host reboot.** Enabled on beelink (`multi-user.target.wants/libvirt-guests.service`); `/etc/default/libvirt-guests` could not be read. Stock `ON_BOOT=start` restarts every guest that was running at shutdown regardless of autostart; the reviewer is ~70% sure the distro default is also `ON_SHUTDOWN=suspend`. While the host is down `Get` is Transient, not Stopped, so nothing is deleted; after boot the operator normally sees Running and keeps the VM.
- A manual `virsh start`.
- Create adoption path (`create.go:73-87` calls `ensureRunning` without `ensureImage`): only when `providerID` is empty and the overlay already holds guest writes. Narrow.
The missing/unreadable-marker trigger is **not** reachable within kvm2 without an edit (marker errors are returned, not treated as unreadable, `image.go:119-125`). The second deleter that does exist is the kvm provider (E3/E4). Host state: both live domains running, persistent, autostart disabled, no managed save; the hazard is documented (`kvm2-provider-spec.md:91-93`, `kvm2-provider.md:313-318`), but the code contradicts the "add a new key" rule instead of enforcing it, and the guide's warning names `url` although a `url` change alone never refetches.
**Minimal fix.** At `image.go:129-130` do not delete; return `permanentf("base %s is cached with sha256 %s but %s is configured; add the image under a new key, …")`. Keep deletion for the no-marker and non-hex branches. Alternative: digest in the volume name. Either way log at `V(0)`.
**Probe to settle** (not run):
```
P=default; V="virsh -c qemu:///system"
$V vol-create-as $P review-e7-base.qcow2 16M --format qcow2
$V vol-create-as $P review-e7-overlay.qcow2 16M --format qcow2 --backing-vol review-e7-base.qcow2 --backing-vol-format qcow2
stat -c '%i' /srv/vms/images/review-e7-base.qcow2
$V vol-delete --pool $P review-e7-base.qcow2 # expected: succeeds
$V vol-create-as $P review-e7-base.qcow2 16M --format qcow2
stat -c '%i' /srv/vms/images/review-e7-base.qcow2 # expected: different inode
$V vol-delete --pool $P review-e7-overlay.qcow2; $V vol-delete --pool $P review-e7-base.qcow2
```
Read-only, for the owner: `grep -Ev '^\s*(#|$)' /etc/default/libvirt-guests` (empty output = defaults apply).
## E8 — pool-space precheck freshness — CONFIRMED, and staleness is the smaller of two errors
Measured 2026-09-17 08:28-08:30 UTC, read-only. kvm2's pool is `default` at `/srv/vms/images`.
| pool | filesystem | libvirt Available | `f_bfree` | `f_bavail` | libvirt − bfree | libvirt − bavail | vols |
|---|---|---|---|---|---|---|---|
| default (kvm2) | ubuntu--vg-vms--lv | 266,038,706,176 | 266,038,378,496 | 241,304,276,992 | +327,680 | **+24,734,429,184** | 24 |
| pool | same | 267,718,541,312 | 266,038,378,496 | 241,304,276,992 | +1,680,162,816 | +26,414,264,320 | 15 |
| vms | same | 267,718,541,312 | 266,038,378,496 | 241,304,276,992 | +1,680,162,816 | +26,414,264,320 | 9 |
| boot / images | ubuntu--lv | 73,877,696,512 | 73,918,029,824 | 68,532,543,488 | −40,333,312 | +5,345,153,024 | 0 / 2 |
| vms-2 | nvme1n1p1 | 820,622,606,336 | 820,602,261,504 | 770,575,294,464 | +20,344,832 | +50,047,311,872 | 2 |
- **Cached per pool.** Three pools on one filesystem report two different values; a call-time value could not differ. Pool A's allocation is invisible to pool B. Over 80 s `f_bfree` moved on the `images` filesystem while libvirt stayed frozen, 40 MB stale in the pessimistic direction, so the cache errs both ways. `default` is nearly exact only because libvirt touched it ~1.6 h earlier.
- **The root reserve.** libvirt reports `f_bfree`, which includes ext4's 5% reserve. Overlays are owned by `libvirt-qemu:kvm`, so guest writes stop at `f_bavail = 0` while libvirt still reports ≥ 24.7 GB.
**Failing path.** `create.go:92-100`: pool lookup → `imageSizeIfUncached` → `checkPoolSpace`, no refresh; `image.go:450-458` compares raw `available`; the only refresh is `image.go:235` after an upload, which never runs again once the base is cached. Overlays start at ~196 KB and grow invisibly (live ones at 1.47 and 1.54 GB of 10 GiB), as does `kube-node-33.qcow2` (50.6 GB) in the same directory. Reserve scenario (deterministic here): with `f_bavail` between 0 and ~10.7 GB the precheck passes for a 10 GiB disk, `ErrQuotaExceeded` never fires, and Create, the upload and the guests hit ENOSPC, the condition behind S4.1's wedge and S2.2's paused guests. `check.go:106-112` has the same defect: `poolAvailableGiB` over-reports by ~23 GiB plus drift.
**Minimal fix.** (1) `StoragePoolRefresh` immediately before `checkPoolSpace`, after `imageSizeIfUncached`; failure is Transient; only on the domain-not-defined branch. Cost: readdir + stat per volume (24 today), already paid at `image.go:235`. (2) Do not refresh in Check (a mutation every round on a shared host); document the semantics. (3) Add headroom to `neededBytes`: per-host `poolReserveGiB` or a fraction of `capacity` (already returned and discarded at `image.go:450`); or document `tune2fs -m 0`. (4) Make the fake's `StoragePoolGetInfo` return a cached value that only `StoragePoolRefresh` updates; add `TestCreate_refreshesPoolBeforeSpaceCheck`.
**Probe to settle** (for the owner): `virsh pool-info --bytes vms`, `virsh pool-refresh vms`, `pool-info` again; expect Available to drop ~1.68 GB and still sit ~24.7 GB above `df -B1 --output=avail /srv/vms`.
## E9 — libvirt error-code assumptions — (a) REFUTED, (b) CONFIRMED
**(a) Idempotency codes match libvirt 11.6.** Codes identified from libvirt's fixed per-code message prefix with `LIBVIRT_DEBUG=1`.
| Operation | kvm2 expects | Real | Evidence |
|---|---|---|---|
| `DomainLookupByName`, missing (`create.go:88`, `delete.go:57`, `get.go:34`) | 42 | 42 | observed |
| `StorageVolLookupByName`, missing (`image.go:117,134,176,424`) | 50 | 50 | observed |
| `StoragePoolLookupByName`, missing (`delete.go:84`) | 49 | 49 | observed |
| `DomainGetMetadata`, no kvm2 element (`get.go:81`) | 80 | 80 | observed on `docker-29`, `postgres`, and a kvm-v1-marked proxy domain; namespace scoping holds |
| `StorageVolCreateXML`, duplicate (`create.go:171`) | 90 | 90 | sourced, ~95% |
| `DomainDestroy` inactive (`delete.go:67`) | 55 | 55 | sourced, high |
| `DomainCreate` active (`create.go:236`) | 55 | 55 | sourced, high; `waitRunning` re-checks anyway |
| Destroy/Undefine vanished domain | 42 | 42 | sourced, high |
| `StorageVolDelete` vanished (`image.go:182`) | 50 | 50 | sourced, high |
`TestKVM2_lifecycle` would exercise 42 and 50 but not 90 (the second Create returns from the adoption branch before `ensureOverlay`), not 55 from Destroy, and a wrong 80 would not fail it (`list.go:39-45` logs and continues). Two fake-vs-real differences, neither breaking convergence: an overlay file unknown to libvirt returns 55 not 90; an inactive pool returns 55 from the marker lookup in `imageSizeIfUncached` (`create.go:96`) before `checkPoolSpace` (`create.go:100`), so the Permanent arm at `image.go:454-456` is unreachable through Create (`image_test.go:821-828` tests it by direct call).
**(b) Classification.** `classifyLibvirt` (`errors.go:58-71`) sends every unlisted code to Transient; `providerFailure` (`proxy_controller.go:344-345`) returns the error and sets **no condition**, so the Proxy shows nothing and retries under workqueue backoff, rerunning `writeSeed` each time when the failure is at define.
| Rejection | Where | Code | Evidence |
|---|---|---|---|
| Host has no usable KVM | define | 67 | sourced, high |
| No OVMF firmware | define | 9 | sourced, ~85% |
| Bridge missing | `DomainCreate` | 38 | sourced, high; `check.go` never looks at the bridge |
| host-passthrough unsupported | `DomainCreate` | 67 | sourced |
| Non-x86 host | define | 67 or 1 | observed 67 on the `domcapabilities --arch aarch64` path |
| Unknown machine type | define | 8 (already Permanent) | observed |
beelink rejects none of these. The bare `libvirt.ErrUnsupported` sentinel (`rpc.go:392-394`) bypasses `libvirtCode`; small impact. This follows a recorded decision: spec entry 13 (`kvm2-provider-spec.md:317`) "unknown means retry", and contract §5 makes Permanent sticky until the generation changes, so a host-side fix would not unlatch. The spec never looked at 67, and it already latches `ErrNoStoragePool`, which is also host-fixed.
**Minimal fix.** `ErrConfigUnsupported` → Permanent when `op == "create"`; `errors.Is(err, libvirt.ErrUnsupported)` → Permanent before the `libvirtCode` check; leave 9, 38, 1, 84 alone. Give the Transient arm of `providerFailure` a non-latching `Provisioned=False` condition with the message, or have the health checker read `ConnectGetDomainCapabilities` and check the bridge. In the integration test: destroy+undefine over `dialFresh` then Create again (exercises 90), destroy before step 9's Delete (exercises 55), assert "skipping domain with unreadable metadata" never logs.
**Probe to settle** (not run):
```
virsh -c qemu:///system vol-create-as default review-e9-dup.qcow2 1M --format qcow2 && \
LIBVIRT_DEBUG=1 virsh -c qemu:///system vol-create-as default review-e9-dup.qcow2 1M --format qcow2 2>&1 | grep ' error : ' ; \
virsh -c qemu:///system vol-delete --pool default review-e9-dup.qcow2
```
## E10 — overlay capacity below the base's virtual size — CONFIRMED (libvirt pass-through ~90%, unexecuted)
**Host numbers.** `base-debian-13.qcow2` and `base-test-image.qcow2`: libvirt capacity 3221225472 (3 GiB virtual), allocation 435683328. Live overlays: 10 GiB over that base. So the 10 GiB default has ~7 GiB margin; only `diskSizeGb: 1` or `2` breaks Debian 13 (Ubuntu ~3.5 GiB and Fedora ~5 GiB from memory). libvirt reports the true virtual size because of the refresh at `image.go:235`.
**Local experiment** (qemu-img 10.2.1; host has 10.1.0): `qemu-img create -f qcow2 -b <64M base> -F qcow2 short.qcow2 16384K` exits 0, virtual size 16 MiB, `qemu-img check` clean; reads inside 16 MiB match the base, a read at 16M fails `Input/output error`. A 64M GPT base under a 16M overlay: `sfdisk` sees only the protective MBR, `sgdisk -v` reports the disk smaller than the header says and the backup header in error. The Linux kernel rejects such a GPT, so there is no root device; the guest would load GRUB and the kernel then drop to initramfs (inferred, no VM booted). libvirt's `virStorageBackendCreateQemuImgCmdFromVol` appends the capacity with no size comparison (from source memory).
**Failing path.** `diskSizeGb: 2` passes validation (`config_kvm.go:133` rejects only negatives; `kvm2.go:127-128` changes only 0) → `checkPoolSpace` with the small number (`create.go:100`) → base cached (`:103`) → `ensureOverlay` sends `diskSizeGB << 30` (`create.go:107,158`; `volume.go:72` checks only positive) → domain defined and started, `waitRunning` sees Running, Create succeeds (`:148,260`) → `Get`: active, no address → `StateProvisioning` (`state.go:20-21`) → requeue at `ProvisioningPoll` forever (`proxy_controller.go:208-211`; there is no provisioning deadline). Correction to the hypothesis: the Proxy is stuck Provisioning with "waiting for the instance to run", not reported healthy. Nothing points at `diskSizeGb`, and raising it later does not repair that Proxy (`create.go:75-87`, `:171-177`). The kvm provider has the same hole (`kvm/volume.go:24-32`).
**Minimal fix.** Add `StorageVolGetInfo` to `backend` (libvirtBackend, `observed`, fake already stores `fakeVol.capacity` at `fake_test.go:494`). Between `ensureImage` and `ensureOverlay`, if the base's capacity exceeds `DiskSizeGB << 30` return `permanentf("kvm2.diskSizeGb %d is smaller than image %s's virtual size …")`. One or two RPCs on the first Create only. Run the same check on a reused overlay and recreate it when short. `TestCreate_diskSmallerThanImage`. One sentence in the `diskSizeGb` doc rows. A config-time check is not possible.
**Probe to settle** (not run; one ~200 KiB volume, self-cleaning):
```bash
V=review-e10-short.qcow2; P=default
virsh -c qemu:///system vol-create-as $P $V 1G --format qcow2 \
--backing-vol /srv/vms/images/base-test-image.qcow2 --backing-vol-format qcow2; echo "create exit=$?"
virsh -c qemu:///system vol-dumpxml --pool $P $V | grep -E "capacity|backingStore|<path>"
virsh -c qemu:///system vol-delete --pool $P $V; echo "delete exit=$?"
virsh -c qemu:///system vol-list $P | grep review-e10- || echo "clean"
```