diff --git a/docs/kvm2-provider.md b/docs/kvm2-provider.md index 74aedc5..7f41cda 100644 --- a/docs/kvm2-provider.md +++ b/docs/kvm2-provider.md @@ -450,9 +450,15 @@ applied by `call`): go-libvirt's calls take no context and block, so the only way to abandon one is to close the connection under it. That is what a cancelled or -overlong call does: the adapter kills the connection (socket and SSH -client), returns `ErrTransient` wrapping the context or deadline error, -and marks the connection dead so the next call redials. The consequence: +overlong call does: the adapter marks the connection dead so the next call +redials, kills it by closing the SSH client's TCP connection, which takes +the forwarded socket with it, and returns `ErrTransient` wrapping the +context or deadline error. It marks dead before it kills, and closes the +TCP connection rather than the forwarded channel, because both of those are +lock-free: closing the channel writes a close message under the channel's +write lock, so it would queue behind a transfer stuck in a TCP write to a +host that has gone away — for as long as the kernel retransmits, which is +the one case the kill exists to survive. The consequence: **other RPCs in flight on that host's pooled connection fail once with `ErrTransient`** and their callers retry on a fresh connection. In the reconciler that is one requeue with backoff; in GC it is one skipped diff --git a/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md new file mode 100644 index 0000000..f7d0bb4 --- /dev/null +++ b/docs/plans-executions/2026-09-17-1515-kvm2-review-fixes.md @@ -0,0 +1,124 @@ +# Execution: fix the kvm2 code-review findings +Plan: [docs/plans/2026-09-17-1515-kvm2-review-fixes.md](../plans/2026-09-17-1515-kvm2-review-fixes.md) + +- [x] Step 0 — Save plan into the repo +- [x] Step 1 — H1: a stalled write must not delay the teardown + +Remaining: the other 51 findings in `review/REPORT.md`, in severity order — H2 (the +`StorageVolUpload` stream wedge), H3 (`address()` swallowing errors), H4 (the shared base +image cache), then the mediums. Each gets its own checklist line here when it is planned, +so this list stays a record of decided work rather than a wish. + +## Step 0 — plan committed + +Copied out of `~/.claude/plans/` and committed alone before touching code, as the repo's +plan rules require. + +## Step 1 — H1: close the SSH client, and signal dead before killing + +Two edits, both small, in the order the bug unfolds. + +`sshSocket.Close` was `errors.Join(s.Conn.Close(), s.client.Close())`. Go evaluates those +arguments left to right, so the blocking close ran before the unblocking one. `s.Conn` is +the forwarded libvirt socket, and closing an SSH channel writes a close message under +`ch.writeMu`, which go-libvirt already holds while streaming a base image through the same +channel. Against a host that vanished without RST or FIN, that write blocks until the +kernel stops retransmitting — `tcp_retries2`, about 15 minutes. `s.client.Close()`, a bare +`net.Conn.Close()` on the TCP socket that takes no lock and unblocks the stalled writer +through netpoll, was never reached. It now closes the client only. + +**Dropping the channel close rather than reordering it** was the judgment call. Reordering +works, but afterwards the channel close returns an error — `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, so a filter that missed a case would poison +every later `Close` and break `ssh_test.go:66,69`, which assert both closes return nil. +Nothing wants the polite channel-close message: `dialSSHSocket` builds the client solely +for this one channel, the `sshSocket` owns it exclusively, and `dialSSHSocket`'s own error +paths already tear down by closing the client. + +`link.markDead` then compounded it by running `kill()` before `close(l.closed)`, and doing +both inside a `sync.Once`. Reordering alone would not have been enough: `sync.Once.Do` +blocks concurrent callers until the first body returns, so every other `markDead` — including +the one in `call`'s own ctx-cancel branch — queued behind the stalled kill. **Cancelling the +reconcile did not help.** The `once` field is gone; the mutex the struct already had now +guards the "close it exactly once" invariant, `reason` is set before the signal so a +released caller always reads a populated `deadReason()`, and `kill` runs outside the lock. + +**A second route to the same pin, found while planning and 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 entered `sshSocket.closeOnce` and hung there; the 5 s `closeBudget` +released `Close`, which called `markDead("closed")` → `kill()` → `sshSocket.Close()` → +blocked in the other goroutine's `sync.Once`. The budget did not protect it. + +The precondition was verified in the pinned x/crypto v0.54.0 rather than assumed: the +flow-control park (`remoteWin.reserve`, `channel.go:268`) happens *outside* `writeMu`, and +only `writePacket` holds it across the TCP write. So a writer parked in SSH flow control +never blocks the close; a writer inside the TCP write always does. Since the channel window +is 2 MiB (`channelWindowSize = 64 * channelMaxPacket`), reproducing it by hand needs a +kernel send buffer that fills first — shrink `SO_SNDBUF` well under 2 MiB. + +The older kvm provider does not have this bug: it closes `client` only +(`internal/provider/kvm/conn.go:429,456`) and never touches the forwarded channel. kvm2's +rewrite introduced it. + +### Tests: planned four, shipped one + +The first draft had three hermetic tests plus a `//go:build integration && linux` +stalled-write test with a TCP proxy and shrunken socket buffers. The user pushed back — +once the bug is fixed the connection closes properly, so what is left to test? — and they +were right about most of it. `TestDial_forwardsUnixSocket` already covers "Close tears the +connection down", and after the fix `Close` is a single call with no ordering to get wrong. +The stalled-write test was the worst of the four: ~150 lines of Linux-only, +buffer-autotuning-dependent scaffolding whose own false-green guard made it *skip* whenever +the kernel did not cooperate, to demonstrate a bug that reading x/crypto settles and that +the fix deletes the code path for. + +What survived is `TestMarkDead_slowKillDoesNotBlockCallers`, because the `markDead` change +*adds* a guarantee that did not exist before and nothing else touches it — +`TestCall_killedByAnotherCallerFailsTransient` passes a nil kill, so it cannot see the +property. It is also the plausible regression: restoring the `sync.Once` looks like a +correctness improvement. The call's own `fn` signals when it is in flight, so the parked +caller is genuinely parked rather than refused by the dead check on the way in. + +The step that actually validates a regression test is running it against the unfixed code. +`markDead` was reverted in place, the test run, and the fixed version restored from a copy: + +```bash +cp internal/provider/kvm2/backend.go "$SCRATCH/backend.go.fixed" +# ... revert markDead to the sync.Once form and re-add the once field ... +go test -race -count=1 -run 'TestMarkDead_slowKillDoesNotBlockCallers' ./internal/provider/kvm2/ +# --- FAIL: TestMarkDead_slowKillDoesNotBlockCallers (1.00s) +# backend_test.go:186: link not signalled dead while its kill is still running +cp "$SCRATCH/backend.go.fixed" internal/provider/kvm2/backend.go +``` + +It fails in a second with the right diagnostic rather than hanging the suite, because the +blocked kill is released by `t.Cleanup`. + +```bash +go build ./... && go vet ./... && go test -race -count=1 ./internal/provider/kvm2/ +``` + +### Worth noting + +**An accepted risk, deliberately not covered by a test.** Nothing now stops someone +re-adding `s.Conn.Close()` to `sshSocket.Close` as tidiness; it would look harmless and +every test would still pass, because in a healthy test the channel close returns instantly. +The guard is the doc comment on `sshSocket`, which says outright that closing the channel +would queue behind a stalled transfer. That is in idiom for this package — `hosts.go:25-34` +and `create.go:151-153` carry the same kind of hard-won decision — but it is a comment, not +a test, and the failure it prevents is a 15-minute operator-wide stall. + +**Left for later, on purpose.** `ReconciliationTimeout` in `SetupWithManager` does not exist +in this repo at all; adding a per-reconcile deadline would change the contract for every +provider, since `hosts.go` and `image.go` both cite "no caller deadline", so it needs its own +change and its own spec entry. `sshSocket.keepalive` still has no test (sweep finding S1.2). + +**Docs.** `docs/kvm2-provider.md` said the adapter "kills the connection (socket and SSH +client)", which is no longer true and was the description that made the bug look correct. +It now states the order (mark dead, then kill) and why both steps are lock-free. +`docs/kvm2-provider-spec.md` entries 18 and 19 were checked and are not falsified. + +**No CHANGELOG entry yet** — the repo's rule is to add one once the user confirms the change +works, so it lands with a later "validate and record" step, as in the kvm campaign. diff --git a/internal/provider/kvm2/backend.go b/internal/provider/kvm2/backend.go index 7a74306..22fa1e5 100644 --- a/internal/provider/kvm2/backend.go +++ b/internal/provider/kvm2/backend.go @@ -106,7 +106,6 @@ type link struct { label string kill func() - once sync.Once closed chan struct{} mu sync.Mutex reason string @@ -133,18 +132,23 @@ func (l *link) deadReason() string { return l.reason } -// markDead records why the connection is unusable, closes the transport and -// signals dead. Later calls are no-ops. +// markDead records why the connection is unusable, signals dead and then +// closes the transport. Later calls are no-ops. Signalling before killing, +// and killing outside the lock, is what keeps a kill that takes its time +// from holding up the callers waiting on dead or the next markDead: a +// transport close can block for as long as the kernel retransmits. func (l *link) markDead(reason string) { - l.once.Do(func() { - l.mu.Lock() - l.reason = reason + l.mu.Lock() + if l.isDead() { l.mu.Unlock() - if l.kill != nil { - l.kill() - } - close(l.closed) - }) + return + } + l.reason = reason + close(l.closed) + l.mu.Unlock() + if l.kill != nil { + l.kill() + } } // call executes a blocking go-libvirt call with a budget and returns its diff --git a/internal/provider/kvm2/backend_test.go b/internal/provider/kvm2/backend_test.go index 0da1b9f..8511db6 100644 --- a/internal/provider/kvm2/backend_test.go +++ b/internal/provider/kvm2/backend_test.go @@ -148,6 +148,71 @@ func TestCall_killedByAnotherCallerFailsTransient(t *testing.T) { } } +// TestMarkDead_slowKillDoesNotBlockCallers pins the guarantee the pooled +// connection rests on: a kill that takes its time — closing a transport can +// block for as long as the kernel retransmits to a host that went away — +// holds up neither the callers waiting on dead nor the next markDead. The +// call's own fn signals when it is in flight, so the parked caller is +// genuinely parked rather than refused by the dead check on the way in. +func TestMarkDead_slowKillDoesNotBlockCallers(t *testing.T) { + t.Parallel() + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + killing := make(chan struct{}) + var killed atomic.Int32 + l := newLink("beelink", func() { + killed.Add(1) + close(killing) + <-release + }) + + inflight := make(chan struct{}) + waiting := make(chan error, 1) + go func() { + waiting <- l.run(context.Background(), "StorageVolUpload", time.Minute, func() error { + close(inflight) + <-release + return nil + }) + }() + <-inflight + + go l.markDead("keepalive failed") + <-killing + + select { + case <-l.dead(): + case <-time.After(time.Second): + t.Fatal("link not signalled dead while its kill is still running") + } + select { + case err := <-waiting: + if !errors.Is(err, provider.ErrTransient) || !strings.Contains(err.Error(), "keepalive failed") { + t.Errorf("parked call error = %v, want ErrTransient with the death reason", err) + } + case <-time.After(time.Second): + t.Fatal("parked call not released while the kill is still running") + } + + second := make(chan struct{}) + go func() { + l.markDead("closed") + close(second) + }() + select { + case <-second: + case <-time.After(time.Second): + t.Fatal("second markDead blocked behind the running kill") + } + + if killed.Load() != 1 { + t.Errorf("kill hook called %d times, want 1", killed.Load()) + } + if !strings.Contains(l.deadReason(), "keepalive failed") { + t.Errorf("deadReason() = %q, want the first reason", l.deadReason()) + } +} + func TestBudgets_upload(t *testing.T) { t.Parallel() tests := []struct { diff --git a/internal/provider/kvm2/ssh.go b/internal/provider/kvm2/ssh.go index b0faeca..4d89328 100644 --- a/internal/provider/kvm2/ssh.go +++ b/internal/provider/kvm2/ssh.go @@ -96,8 +96,12 @@ func loadCredentials(cfg provider.KVMConfig) (credentials, error) { } // sshSocket is the libvirt socket forwarded through one SSH connection. The -// embedded Conn is the channel; Close also closes the SSH client, which is -// what unblocks anything reading the channel. +// embedded Conn is the channel; Close closes the SSH client, whose TCP +// connection carries it, and that alone unblocks the channel's readers and +// writers. Closing the channel as well would be worse than useless: its +// Close writes a close message under the channel's write lock, so it would +// queue behind a transfer stuck in a TCP write, which is the case Close +// exists to survive. type sshSocket struct { net.Conn client *ssh.Client @@ -107,9 +111,7 @@ type sshSocket struct { } func (s *sshSocket) Close() error { - s.closeOnce.Do(func() { - s.closeErr = errors.Join(s.Conn.Close(), s.client.Close()) - }) + s.closeOnce.Do(func() { s.closeErr = s.client.Close() }) return s.closeErr }