# Plan: fix the kvm2 code-review findings **Created:** 2026-09-17 15:15 ## Context The review of `feat/kvm2-provider` (`review/REPORT.md`) confirmed 52 findings: 4 high, 20 medium, 28 low. We fix them one at a time, highest severity first, each as its own step and its own commit. This plan is the campaign; the execution file's checklist is the authoritative progress record. Steps are detailed as we reach them, so the plan is never a fiction about work not yet designed. Precedent for this shape: `docs/plans/2026-09-10-2222-kvm-review-fixes.md`. ## Steps - [x] **Step 0** — Save this plan into the repo, commit alone, before any code. - [x] **Step 1** — H1: `sshSocket.Close` ran the blocking close before the unblocking one, and `markDead` killed inside a `sync.Once` before signalling dead. Shipped in `0239fb7`; the reasoning is in the execution file. - [ ] **Step 2** — H2: an error during `StorageVolUpload` wedges go-libvirt's stream sender. - [ ] **Step 3** — H3: `address()` swallows every error and demotes healthy proxies. - [ ] **Step 4** — H4: the base image cache is shared across provider entries. - [ ] **Step 5+** — the mediums, from `review/REPORT.md`. Detailed when reached. --- ## Step 2 — H2: a source error must not reach go-libvirt's stream sender ### The bug `uploadImage` (`image.go:255-296`) hands a reader over `resp.Body` to `StorageVolUpload`. I verified the mechanism in the pinned `go-libvirt@v0.0.0-20260814190004`: ```go // rpc.go requestStream abort := make(chan bool) // UNBUFFERED outErr := make(chan error) // UNBUFFERED go func() { outErr <- l.socket.SendStream(serial, proc, program, out, abort) }() resp, err = l.processIncomingStream(c, in) if err != nil { abort <- true return resp, err // outErr is NEVER drained on this path } err = <-outErr ``` `SendStream` (`socket/socket.go`) holds a 4 MiB buffer, polls `abort` once per loop, and on a **non-EOF reader error** sends a `StatusError` packet and returns. It then blocks forever on `outErr <-`, because the main goroutine is still inside `processIncomingStream`. libvirtd answers the `StatusError` with an error, so `processIncomingStream` returns an error and the main goroutine blocks forever on `abort <- true`. **Deadlock: `StorageVolUpload` never returns.** kvm2's `call` rescues only the caller, after `budgets.transfer` = 20 minutes, and the socket kill does **not** unblock either goroutine — both are parked on channel sends, not on socket I/O, which is what the comment at `backend.go:152-156` gets wrong. So every failed upload leaks kvm2's call goroutine, go-libvirt's sender goroutine and 4 MiB, permanently. The pod limit is 128Mi and the reconciler retries on backoff. Three shapes, only the first of which is ours to fix: - **(A) reader error** — a truncated or failing HTTP body. The common case. - **(B) server-side error mid-upload** — ENOSPC, or the volume deleted underneath. After `SendStream` has returned there is nothing any reader, ctx or socket close can do: confirmed unfixable from our side. - **(C) our own abandonment** — ctx cancel or the 20 m budget kills the socket, which makes `processIncomingStream` error. Same tail as (B). ### The fix (workaround; the defect stays upstream) No fork, no `replace`. Two changes that between them close (A) and the realistic half of (C). **2a. Never hand go-libvirt a reader that errors** — `internal/provider/kvm2/backend.go`, at the seam rather than in `image.go`, so it covers the seed and marker uploads too and stays out of the file Step 4 rewrites: ```go type eofReader struct { r io.Reader mu sync.Mutex err error } func (e *eofReader) Read(b []byte) (int, error) { n, err := e.r.Read(b) if err == nil || err == io.EOF { return n, err } e.mu.Lock() if e.err == nil { e.err = err } e.mu.Unlock() return n, io.EOF } func (e *eofReader) sourceErr() error { e.mu.Lock(); defer e.mu.Unlock(); return e.err } ``` `libvirtBackend.StorageVolUpload` wraps `r` in one, and after a nil-error return converts a recorded `sourceErr()` into the returned error. `io.EOF` is the one outcome that ends the stream the normal way: libvirtd gets `StatusOK`, replies OK, `processIncomingStream` returns nil, the main goroutine reaches `err = <-outErr` and both goroutines unwind. `countingReader` is untouched — it sits under the wrapper, still sees the real error, so `uploadImage`'s `readErr != nil` branch and `TestEnsureImage_shortReadRemovesVolumeAndIsTransient` keep working verbatim. **Unverified, ~85%:** that libvirtd answers a short-but-clean stream finish with OK rather than an error. `virFDStreamWrite` enforces `length` only on writes *past* it and nothing compares `offset` to `length` on the close path, which is also why `virsh vol-upload` of a small file into a large volume works. If it answers with an error instead, we land back in a deadlock by a different door — but that is exactly today's behaviour, so the change is non-worsening either way. Probe recipe goes in the commit message; it needs a real libvirtd and a write, so it is for the user to run, not an agent. **2b. Make the transfer deadline land on the HTTP source, not on the libvirt socket.** `p.do` already builds the request with `http.NewRequestWithContext` (`image.go:360`), so the body is ctx-bound, and `http.DefaultClient` has no timeout of its own by deliberate choice (`kvm2.go:100`). Give the image GET in `fetchImage` a deadline of `defaultBudgets.transfer` minus a grace of one minute (precedent for reaching for `defaultBudgets` directly: `discardVolume`, `image.go:331`). A stalled mirror then ends by the *body* failing, which 2a turns into `io.EOF` and a clean unwind, roughly a minute before the libvirt budget would have killed the socket and leaked. That converts the realistic repeated (C) — a slow or stalling mirror on a 415 MB image — from a permanent leak into an ordinary Transient error. The socket kill stays as the backstop for libvirtd itself hanging, which is the residual we cannot close. **2c. Close the dedicated connection before unlinking the file.** Move the `discardVolume` call out of `uploadImage` (`image.go:291-294`) up into `fetchImage`'s upload-error branch, where the deferred `fresh.Close()` has already run, so libvirtd has let go of the stream's fd before the volume is deleted. `uploadImage` then drops its `be` parameter and its private logger — a net deletion. Doing it this way rather than adding an explicit `Close` keeps `image_test.go:573`'s `closeCount() == 1` assertion true. ### Tests, docs, scope One test: `TestEOFReader_sourceErrorNeverReachesTheStream` in `backend_test.go` — a table with "error after data", "error with no data", "plain io.EOF passes through", "first error wins". ~35 lines. **Deliberately not tested:** the abort protocol end to end. `libvirttest` reads a fixed 28-byte header per packet and implements no stream procedures, so pinning this hermetically would mean writing a libvirt RPC server stub — 150-250 lines whose only assertion is about *upstream's* goroutines. That is the bloat we cut for H1. Docs: a paragraph in `docs/kvm2-provider.md`'s *The image cache* section saying a truncated response is reported out of band and why, and a *Known gaps* bullet naming shapes (B) and (C) as an upstream defect with the two-character patch quoted, so a future reader knows it is not ours and what the escape hatch is: ```go abort := make(chan bool, 1) outErr := make(chan error, 1) ``` File that as an upstream issue/PR against `digitalocean/go-libvirt` and link it from the bullet. --- ## Step 3 — H3: an unreadable address must not look like a missing one ### The bug `address()` (`get.go:104-119`) tries the guest agent then the host's ARP table. Any error that is not `context.Canceled`/`DeadlineExceeded` is logged at V(2) and skipped; after both it returns `("", nil)`. `mapState(active, "")` is `StateProvisioning` (`state.go:20-21`), the controller clears `p.Status.IP` on Provisioning (`proxy_controller.go:208-211`), and the health engine prunes the proxy's accumulated state and calls `Metrics.ForgetProxy` in one tick once `EffectiveHost()` is empty. The trigger that matters: another reconcile's cancel or budget overrun kills the **shared** pooled connection, and `call` then returns `transientf("... connection is closed: ...")` with no ctx sentinel, which `get.go:108` does not recognise. `Get` returns a *successful* Instance in Provisioning with `IP: ""`, and a healthy proxy is pulled out of rotation by an unrelated reconcile's timeout. ### The fix A booting VM and a dead link are distinguishable: on beelink the agent source errors while the ARP source returns success-with-no-addresses (`docs/kvm2-provider-spec.md:146` — the host never sees the guest's traffic). So *one* source failing is normal; *every* source failing, or a connection already gone, is not. ```go func address(ctx context.Context, log logr.Logger, be backend, dom libvirt.Domain) (string, error) { errs := make([]error, 0, len(addressSources)) for _, s := range addressSources { ifaces, err := be.DomainInterfaceAddresses(ctx, dom, uint32(s.src), 0) if err != nil { if isDead(be) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return "", fmt.Errorf("reading addresses of domain %s from the %s source: %w", dom.Name, s.name, err) } log.V(2).Info("no addresses from source", "source", s.name, "error", err.Error()) errs = append(errs, fmt.Errorf("%s source: %w", s.name, err)) continue } if ip := firstIPv4(ifaces); ip != "" { return ip, nil } } if len(errs) == len(addressSources) { return "", fmt.Errorf("%w: reading addresses of domain %s: every source failed: %w", provider.ErrTransient, dom.Name, errors.Join(errs...)) } return "", nil } ``` The explicit `provider.ErrTransient` wrap is load-bearing, not decoration: without it `classify` picks the first libvirt code out of the join tree, and `ErrInvalidArg`/`ErrAccessDenied`/ `ErrNoSupport` map to **Permanent** — an address read failing would latch a *running* proxy as Failed, a worse version of the harm being fixed. `isDead(be)` (`backend.go:50-61`, already works through the `observed` decorator and the test fake) is not strictly needed — a dead link fails both sources anyway — but it fails faster, attributes better, and is the same predicate M15 will need for `ListByTag`. Also fix the now-wrong comment at `get.go:100-103` ("only ctx ending is an error"). **What this does not fix, on purpose.** A guest agent restarting inside a running VM still demotes it: the agent errors, ARP returns empty, and the provider cannot tell that from a VM that is still booting. The discriminator is the *previous* IP, which the provider deliberately does not hold (`TestGet_readsUIDAndCreatedAtFromBackend` pins "read back, not remembered") and the controller already has. File as a follow-up finding against `proxy_controller.go:208-211`: on Provisioning, keep a Proxy's existing IP and `Provisioned` condition for N polls before clearing, or give the health engine a grace period before pruning. The one kvm2-local alternative — treating "active, no address, domain older than a provisioning window" as an error using the mark's `createdAt` — is a design change wearing a bug fix's clothes, and it invents a provisioning deadline that belongs to the controller. Rejected. ### Tests - `TestGet_arpFallbackWhenAgentFails` (`get_test.go:135`) passes unchanged — one source of two. - `TestGet_addressErrorIsProvisioning` (`get_test.go:159`) must be **re-pointed**. It fails both sources with `ErrAgentUnresponsive`, which is not realistic: code 86 is agent-specific and the ARP source never produces it. Rewrite as `TestGet_agentFailsAndARPEmptyIsProvisioning` — agent errors, ARP returns no addresses and no error — which is the beelink boot and the behaviour the new rule must not break. Nothing covers it today. - One new test, `TestGet_everyAddressSourceFailingIsTransient`, two subtests: both sources error; and the pooled link is dead before the address read (the fake implements `liveness`). Assert Transient, not NotFound, not Permanent. Docs: `docs/kvm2-provider-spec.md:146-151` says "errors from either source mean 'no address yet'", now false. The `ErrTransient` row of the error-class table in `docs/kvm2-provider.md`. --- ## Step 4 — H4: the base image cache must not be shared across provider entries ### The bug `baseVolumeName` (`volume.go:17`) is `base-.`, with a `.sha256` marker beside it. `internal/provider/kvm/image.go:44-56` builds **byte-identical** names in the same pool — for URL images too (kvm takes `hex(sum[:8])`, kvm2 takes `hex(sum[:])[:16]`, the same 8 bytes) — with a completely separate lock table. The shipped config has a live `kvm-home` entry and a commented-out `kvm2-home` entry on the same host, the same key `debian-13` and the same default pool, and the comment tells the operator to enable it. Both integration tests use key `test-image`. On beelink both live overlays back onto `base-debian-13.qcow2`, which kvm created. Consequences, given `cachedImage` accepts *any* 64-hex marker when no `sha256` is configured and never re-hashes the base: - Two entries, one key, different URLs: whichever fetched first owns the file and the other silently boots the wrong image. - A concurrent cold fetch: B finds A's base-without-marker, deletes it, recreates and uploads its own; A finishes into the unlinked inode, writes the marker, and `StorageVolGetPath` resolves A's handle to B's partial file — a guest boots from a half-written base. - A `sha256` edit deletes the base in place under live overlays (`cachedImage` branch E), and libvirt does not refuse to delete a volume that running domains use as a backing store. ### The fix — the provider entry's name in the volume name `base--.`, e.g. `base-kvm2-home-debian-13.qcow2`, with the marker following automatically. Every production call site already goes through `imageSource.volumeName()` (`image.go:49`), so this is a small change: give `resolveImage` the provider name and add an `Entry` field to `imageSource`. `Provider.name` is already carried (`kvm2.go:104`). But provider names are validated only as non-empty and unique (`config.go:96-105`) — no charset rule — and a name with a `/` would produce an unusable volume name, so **`New` must reject an entry name that is not a safe volume name component**, using the existing `kvmIdentifier` shape. Fail fast at startup, in kvm2 only: tightening the rule for every provider type is a broader change than this fix. **What this buys, and what it does not.** It fixes the cross-entry sharing outright — kvm↔kvm2 and kvm2↔kvm2 — which is the wrong-image bug and the concurrent-fetch corruption, and it makes the existing per-`Provider` `imageLocks` correct rather than a latent bug, since no two entries can now contend for one name. It does **not** make the name mean the content, so two things stay open and must be said plainly rather than quietly skipped: - Editing `images..url` without changing the key is still a silent no-op. Already documented behaviour ("add a new key instead"); left as is. - Cross-*process* fencing still depends on the atomic-publish work (upload to a temp name, verify, then publish). E4 refuted the rollout and leader-handover triggers for the shipped manifests, so this is a narrow residual. Its own later commit. **One companion fix, in this commit**, because the naming alone does not stop it and it is the remaining path that unlinks a good file under a running domain: `cachedImage` branch E (`image.go:129-130`, digest differs from the configured `sha256`) currently deletes the marker and the base. Make it return Permanent instead — "this key is cached with digest X but Y is configured; add the image under a new key, or delete the Proxies using it and evict the base." That enforces the rule the contract already states instead of contradicting it. Keep the delete for the no-marker and non-hex-marker branches, where the base is genuine debris. ### Migration Nothing to do. The existing `base-debian-13.qcow2` on beelink is kvm's, kvm keeps using it untouched, and kvm2 downloads its own copy once per host (~415 MB) when `kvm2-home` is enabled. No window in which anything is replaced under a running guest. The pool needs room for both. ### Tests and docs Tests are mostly literals becoming helpers: `volume_test.go:122-125`, `image_test.go:184,190,196` (`wantVolume`), `create_test.go:64-70`, `delete_test.go:67`, `provider_test.go:409`; a small `fx.baseName()` helper covers the last three. `image_test.go:281,298` should assert the log field and the `kvm2.volume` span attribute **by prefix**, which keeps pinning that the key stays readable in logs. `kvm2_integration_test.go:481-484` switches from `baseVolumeName(...)` to `resolveImage(...).volumeName()`; its assertion that the base survives `Delete` is unchanged. Two new tests: - `TestBaseVolumeName_isPerProviderEntry` — two entries, same key, different names; the produced name never equals the kvm provider's `base-.`. - `TestNew_rejectsUnsafeEntryName` — an entry name with a `/` fails at construction. **Not tested:** that libvirt permits deleting an in-use backing file (established from libvirt source by the review; re-proving it needs a write on a real host), and the cross-process race (no hermetic way, and its fix is deferred). Docs: the *What lands on the host* table (`docs/kvm2-provider.md:198-212`), the eviction recipe (`:305-318`, which currently names `base-debian-13.qcow2` and would delete kvm's file), the migration section (`:346-370` — kvm2 never reuses a kvm base, one extra download, nothing to delete), the prerequisites note that the pool needs one base per distro **per entry**, the naming contract in `docs/kvm2-provider-spec.md` entries 4 and 10, the shared validator's message at `internal/provider/config_kvm.go:139-141` ("it becomes the cached volume name `base-.`", now wrong for kvm2), and the kvm2 block's comment in both `providers-config.yaml` files. Leave `docs/kvm-provider.md` alone; kvm is unchanged. --- ## Ordering **H2 → H3 → H4**, three commits. H3 is orthogonal: it touches only `get.go` and `get_test.go`. H2 and H4 both touch `image.go` but at different heights, and the order matters. H2's `image.go` edit is a deletion — `discardVolume` moves out of `uploadImage` and up into `fetchImage` — while H4 edits `imageSource.volumeName()`, `resolveImage`, and `cachedImage`'s digest branch ten lines below. Landing H2 first means H4 rebases cleanly; the reverse means H4 is written against a failure path H2 is about to restructure. Keeping H2's guard in `backend.go` rather than in `countingReader` is part of the same separation. Semantically, H2 makes `fetchImage` delete the base after the transfer connection is closed, and that delete is only unambiguously safe once names are per-entry — so H2's commit message should say the discard is still unfenced against a second entry sharing the name and that H4 closes it, and H4's should point back. The deferred atomic-publish work then lands exactly where H2 put the discard, on a volume this call created under a name no other entry can produce. ## Verification ```bash go build ./... && go vet ./... go test -race -count=1 ./internal/provider/kvm2/ make test ``` Per step, the check that actually matters is that the new test fails against the unfixed code — stash the source edit, keep the test, watch it fail, restore. For H2 that means asserting `eofReader` converts before the fix exists; for H3, that the dead-link subtest returns a successful Provisioning instance today; for H4, that two entries produce one name today. After H4, run the kvm2 integration test on beelink. **It has never run** — Step 11 is unchecked in `docs/plans-executions/2026-09-16-1333-kvm2-provider.md`, every `base-*` on the host predates kvm2, and both live domains carry the kvm v1 metadata namespace. It is the first end-to-end exercise of the new name and does a real cold download beside kvm's untouched base. Each step: its own commit on `feat/kvm2-provider` with the execution-file entry and its checklist line ticked. `CHANGELOG.md` lands with a "validate and record" step once the user confirms the work, as in the kvm campaign. Trailer: `Co-Authored-By: Claude Opus 5 (1M context) `.