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) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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) <noreply@anthropic.com>`.
|
||||
Reference in New Issue
Block a user