diff --git a/docs/plans/2026-09-17-1515-kvm2-review-fixes.md b/docs/plans/2026-09-17-1515-kvm2-review-fixes.md index 76c6032..70b0ba9 100644 --- a/docs/plans/2026-09-17-1515-kvm2-review-fixes.md +++ b/docs/plans/2026-09-17-1515-kvm2-review-fixes.md @@ -4,239 +4,375 @@ ## Context The review of `feat/kvm2-provider` (`review/REPORT.md`) confirmed 52 findings: 4 high, -20 medium, 28 low. We are fixing 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. Only **Step 1 (H1)** is detailed now — later steps get -detailed as we reach them, so the plan is not a fiction about work not yet designed. +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`, the same -campaign for the kvm provider. - -### The bug being fixed in Step 1 (H1) - -`sshSocket.Close` is what tears a kvm2 connection down. It is the `kill` of `link` -(`backend.go:216`), and killing the transport is the only way to abandon a go-libvirt call, -which takes no context and blocks. - -```go -// ssh.go:111 -s.closeErr = errors.Join(s.Conn.Close(), s.client.Close()) -``` - -Go evaluates those arguments left to right, so **the blocking close runs before the -unblocking one**: - -- `s.Conn` is the forwarded libvirt socket, a `*chanConn` over x/crypto's `*channel`. - `channel.Close` → `sendMessage` → `channel.writePacket` (`channel.go:223`), which takes - `ch.writeMu` and holds it across a raw TCP write. -- `s.client.Close()` is `connection.Close()` → a bare `net.Conn.Close()` on the TCP socket - (`connection.go:111`). It takes no lock, and Go's netpoll unblocks any pending write on it. - -While go-libvirt streams a base image through that same channel (`StorageVolUpload`), the -writer sits inside `writePacket` holding `ch.writeMu`. Verified in x/crypto v0.54.0: the -flow-control park (`remoteWin.reserve`, `channel.go:268`) happens **outside** `writeMu`, so -only a writer stuck in the TCP write blocks `Close`. When the host is power-cut or -partitioned (no RST, no FIN), that write blocks until the kernel stops retransmitting — -`tcp_retries2`, about 15 minutes. `Close` queues behind it and never reaches the client -close that would have freed everything. - -`markDead` then amplifies it (`backend.go:138-148`): - -```go -l.once.Do(func() { - ... - if l.kill != nil { l.kill() } // blocks here for ~15 min - close(l.closed) // never reached -}) -``` - -Three consequences, each independently bad: - -1. `l.closed` is never closed, so callers parked in `call`'s `case <-l.closed` are never released. -2. `isDead()` stays false, so `hostTable.get` keeps handing out the dying pooled backend and - each new call wedges the same way. -3. `sync.Once.Do` blocks *every* concurrent `markDead` caller — including `call`'s own - ctx-cancel branch (`backend.go:180`). **Cancelling the reconcile does not help.** - -Blast radius: `MaxConcurrentReconciles: 3` (`proxy_controller.go:442`) and the per-image lock -(`ensureImage`/`lockImage`), whose `keyedLocks.acquire` is ctx-aware but reconcile contexts -carry no deadline. Three Proxies on one host with an uncached image park all three workers -for ~15 minutes, stalling every Proxy on every provider. - -There is a **second route to the same pin**, fixed by the same edits: go-libvirt's -`Disconnect` closes the `net.Conn` it was handed (`libvirt.go:331` → `socket.go:164`), and -that conn *is* our `*sshSocket`. So `libvirtBackend.Close`'s `Disconnect` goroutine enters -`sshSocket.closeOnce` and hangs; the 5 s `closeBudget` releases `Close`, which calls -`markDead("closed")` → `kill()` → `sshSocket.Close()` → blocks in the other goroutine's -`sync.Once`. The budget does not protect it. - -The older kvm provider does not have this bug: it closes `client` only (`kvm/conn.go:429,456`) -and never closes the forwarded channel. kvm2 introduced it. +Precedent for this shape: `docs/plans/2026-09-10-2222-kvm-review-fixes.md`. ## Steps -- **Step 0** — Save this plan into the repo, commit alone, before any code. -- **Step 1** — H1: `sshSocket.Close` and `link.markDead` (detailed below). -- **Step 2+** — the remaining findings from `review/REPORT.md` in severity order (H2 upload - stream wedge, H3 `address()` swallowing errors, H4 shared base image cache, then the - mediums). Detailed when reached; add a checklist line per step as it is planned. +- [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 1 — H1: a stalled write must never delay tearing the connection down +## Step 2 — H2: a source error must not reach go-libvirt's stream sender -### 1a. `internal/provider/kvm2/ssh.go` — close the SSH client only +### The bug -Replace the body of `Close` (109-114) and correct the `sshSocket` doc comment (98-100), -which currently says Close "also closes the SSH client, which is what unblocks anything -reading the channel": +`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 -// sshSocket is the libvirt socket forwarded through one SSH connection. The -// embedded Conn is the channel; Close closes the SSH client, whose TCP -// connection carries it. Closing the channel too would be worse than useless: -// channel.Close writes a close message under the channel's write lock, so it -// would queue behind a transfer stuck in a TCP write — the case Close exists -// to survive. -type sshSocket struct { ... } // fields unchanged +// 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 +``` -func (s *sshSocket) Close() error { - s.closeOnce.Do(func() { s.closeErr = s.client.Close() }) - return s.closeErr +`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 } ``` -Dropping `s.Conn.Close()` rather than reordering it, because after the TCP conn is closed the -channel close returns one of several errors (`net.ErrClosed`, a `*net.OpError`, the transport's -sticky write error, or `io.EOF` once `mux.loop` has run `ch.close()`), and `closeErr` is -memoised — a filter that misses a case would put a spurious error in every later `Close` and -break `ssh_test.go:66,69`, which assert both closes return nil. Nothing needs the polite -channel-close message: `dialSSHSocket` builds the client solely for this one channel, the -`sshSocket` owns it exclusively, every Close is a full teardown, and `dialSSHSocket`'s own -error paths (181, 188) already tear down by closing the client. Closing the client is -sufficient: `mux.loop` exits and `chanList.dropAll()` → `ch.close()` unblocks the channel's -readers and writers. +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`. -`errors` stays imported (`errors.As` at 52 and 90). No production caller inspects the error: -`backend.go:216`, `hosts.go:78`, `check.go:90`, `image.go:265` all discard it. +Also fix the now-wrong comment at `get.go:100-103` ("only ctx ending is an error"). -### 1b. `internal/provider/kvm2/backend.go` — signal dead before killing, and drop the `sync.Once` +**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. -Reordering alone is not enough: `sync.Once.Do` blocks concurrent callers until the first body -returns, which is consequence 3 above. Delete the `once sync.Once` field from `link` (109) and -guard with the mutex the struct already has: +### Tests -```go -// markDead records why the connection is unusable, signals dead and then -// closes the transport. Signalling first means a kill that takes its time -// holds up neither the callers waiting on dead nor the next markDead. -func (l *link) markDead(reason string) { - l.mu.Lock() - if l.isDead() { - l.mu.Unlock() - return - } - l.reason = reason - close(l.closed) - l.mu.Unlock() - if l.kill != nil { - l.kill() - } -} -``` +- `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. -`isDead` is a non-blocking select on `l.closed` and takes no lock, so it is safe under `mu` -and keeps one source of truth. `mu` holds the invariant that exactly one goroutine closes the -channel and runs `kill`. `reason` is set before `close`, so a caller released by `<-l.closed` -always reads a populated `deadReason()`. `kill` runs outside the lock. +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`. -Update the comment at 136-137 ("closes the transport and signals dead" — now reversed). The -`link` doc (100-104) and `newLibvirtBackend`'s comment (210-212) stay accurate. +--- -Consequences checked, all safe: `call`'s five branches; `hostTable.get` may redial a few -microseconds before the old socket is gone (two TCP connections overlap briefly); -`libvirtBackend.Close`'s early return on `isDead()` skips no kill, since `isDead()` now means -"some goroutine has closed `closed` and is running `kill`", still exactly once; the disconnect -watcher and keepalive goroutine exit sooner instead of parking in `once.Do`; `hosts.go:153`, -which calls `markDead` while holding `e.sem`, is bounded by one `net.Conn.Close`. When `Close` -is itself the first marker, `kill` runs synchronously, so Close still returns only after the -socket is closed — `TestLibvirtBackend_closeIsIdempotent` is unaffected. +## Step 4 — H4: the base image cache must not be shared across provider entries -**Not** running `kill` in a goroutine: `Close` would return before the socket was really -closed, which `mockAdapter`'s cleanup and `image.go:265`'s `defer fresh.Close()` rely on, and -it would leave an fd-leak window on the failed-dial path. +### The bug -### 1c. Test — one, not four +`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. -Revised down from an earlier draft of four tests. What the other three would have asserted is -either already covered or no longer has a code path to guard: +Consequences, given `cachedImage` accepts *any* 64-hex marker when no `sha256` is configured and +never re-hashes the base: -- "Close tears the connection down properly" is covered by `TestDial_forwardsUnixSocket` - (ssh_test.go:66-71), which closes twice and asserts both return nil. After 1a, `Close` is a - single `s.client.Close()` — there is no ordering left to get wrong. -- A stalled-write end-to-end test (TCP proxy, shrunken `SO_SNDBUF`/`SO_RCVBUF`, window - priming) was dropped: ~150 lines of Linux-only, buffer-autotuning-dependent scaffolding - whose own false-green guard makes it *skip* when the kernel does not cooperate. Its value - was demonstrating the bug, which reading x/crypto already settles, and 1a removes the call - it would exercise. The repro recipe goes in the commit message instead. +- 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. -**`TestMarkDead_slowKillDoesNotBlockCallers`** — `backend_test.go`. Kept because 1b *adds* a -guarantee that does not exist today ("a slow kill blocks neither the callers waiting on dead -nor the next `markDead`") and nothing else in the suite touches it — -`TestCall_killedByAnotherCallerFailsTransient` passes `newLink("beelink", nil)`, a nil kill, so -it cannot see the property. It is also the plausible regression: restoring the `sync.Once` -looks like a correctness improvement and would silently reinstate the stall. +### The fix — the provider entry's name in the volume name -A `kill` that signals it has started and then blocks on a channel closed by `t.Cleanup`. -Reuses the existing `blockingFn` helper (backend_test.go:19). With a call parked in `l.run`, -assert within one second each: `<-l.dead()` closes; the parked call returns `ErrTransient`; a -second `markDead` from another goroutine returns; `killed.Load() == 1`; `deadReason()` is the -first reason. Fails by timeout against the current code. `t.Parallel()`, generous bounds, -`errors.Is` against `provider.ErrTransient`, per repo style. +`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`. -**Accepted risk:** nothing then stops someone re-adding `s.Conn.Close()` to `sshSocket.Close` -as tidiness. The guard is the doc comment in 1a saying outright that closing the channel would -queue behind a stalled transfer — in idiom for this package (`hosts.go:25-34`, -`create.go:151-153`), but a comment, not a test. +`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. -### 1d. Docs +**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: -`docs/kvm2-provider.md:451-453` says the adapter "kills the connection (socket and SSH -client)". It now closes the SSH client, which takes the forwarded socket with it — reword, and -optionally add the one-clause reason (a lock-free close is the only kind that survives a -transfer stuck in a TCP write) near the 20 m upload budget row. +- 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. -`docs/kvm2-provider-spec.md` entries 18 and 19 say the adapter "closes the underlying -connection" and describe the pool; neither is falsified, so no spec change. +**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. -### 1e. Explicitly out of scope +### Migration -- `ReconciliationTimeout` in `SetupWithManager`: does not exist in the repo today, and a - per-reconcile deadline changes the contract for every provider (`hosts.go` and `image.go` - both cite "no caller deadline"). Its own change, with its own spec entry. -- `sshSocket.keepalive` having no test (sweep finding S1.2) — separate finding. -- The kvm provider: verified not to have this bug. +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. -### Verification +### 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 test -race ./internal/provider/kvm2/ +go build ./... && go vet ./... +go test -race -count=1 ./internal/provider/kvm2/ make test ``` -Then the step that actually matters: **confirm the new test fails against the unfixed code.** -Stash the `markDead` edit while keeping the test, run it, watch it fail by timeout, restore. A -regression test that passes both ways is worthless. +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. -Record in the commit message: the reproduction recipe (shrunken `SO_SNDBUF` so the socket -buffer fills before the 2 MiB SSH window, and why `remoteWin.reserve` sitting *outside* -`writeMu` means only a writer inside `writePacket` can block the close), and the second route -through go-libvirt's `Disconnect` closing the `net.Conn` it was handed. +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. -### Commit - -One commit on `feat/kvm2-provider`: the two source edits, the one test, the doc reword, the -execution-file entry with its checklist line ticked, and the `CHANGELOG.md` entry (timestamp -from `date "+%Y-%m-%d %H:%M %Z"`) once the user confirms it works. Trailer: +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) `.