From 85ea319cc299e33fb4eff7cd6773666d27df52c4 Mon Sep 17 00:00:00 2001 From: kacerr Date: Thu, 17 Sep 2026 15:27:04 +0000 Subject: [PATCH] Plan the kvm2 code-review fixes The review of feat/kvm2-provider confirmed 52 findings (4 high, 20 medium, 28 low); review/REPORT.md has them. This is the campaign plan: one step and one commit per finding, highest severity first, with only the step in hand detailed so the plan is not a fiction about work not yet designed. Same shape as the kvm campaign in 2026-09-10-2222-kvm-review-fixes.md. Step 1 (H1) is detailed: sshSocket.Close runs the blocking close before the unblocking one, so a transfer stuck in a TCP write pins the teardown for as long as the kernel retransmits, and markDead compounds it by killing inside a sync.Once before signalling dead. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-17-1515-kvm2-review-fixes.md | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 docs/plans/2026-09-17-1515-kvm2-review-fixes.md diff --git a/docs/plans/2026-09-17-1515-kvm2-review-fixes.md b/docs/plans/2026-09-17-1515-kvm2-review-fixes.md new file mode 100644 index 0000000..76c6032 --- /dev/null +++ b/docs/plans/2026-09-17-1515-kvm2-review-fixes.md @@ -0,0 +1,242 @@ +# 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 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. + +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. + +## 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. + +--- + +## Step 1 — H1: a stalled write must never delay tearing the connection down + +### 1a. `internal/provider/kvm2/ssh.go` — close the SSH client only + +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": + +```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 + +func (s *sshSocket) Close() error { + s.closeOnce.Do(func() { s.closeErr = s.client.Close() }) + return s.closeErr +} +``` + +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. + +`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. + +### 1b. `internal/provider/kvm2/backend.go` — signal dead before killing, and drop the `sync.Once` + +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: + +```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() + } +} +``` + +`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. + +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. + +**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. + +### 1c. Test — one, not four + +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: + +- "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. + +**`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. + +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. + +**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. + +### 1d. Docs + +`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. + +`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. + +### 1e. Explicitly out of scope + +- `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. + +### Verification + +```bash +go build ./... +go test -race ./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. + +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. + +### 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: +`Co-Authored-By: Claude Opus 5 (1M context) `.