kacerrandClaude Opus 5
0239fb7e69
sshSocket.Close was errors.Join(s.Conn.Close(), s.client.Close()). Go evaluates those 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 that same channel. Against a host that vanished without RST or FIN, that write blocks until the kernel stops retransmitting (tcp_retries2, ~15 min), and s.client.Close(), a bare net.Conn.Close() that takes no lock and unblocks the stalled writer through netpoll, was never reached. Close the client only; nothing wants the polite channel-close message, and reordering instead would leave an error to filter in a memoised closeErr. link.markDead compounded it by running kill() before close(l.closed), both inside a sync.Once. Reordering alone was not enough: sync.Once.Do blocks concurrent callers until the first body returns, so every other markDead — including the one in call is own ctx-cancel branch — queued behind the stalled kill, and cancelling the reconcile did not help. The once field is gone; the mutex the struct already had guards the close-exactly-once invariant, reason is set before the signal, and kill runs outside the lock. Blast radius was the whole operator, not one host: MaxConcurrentReconciles is 3 and ensureImage holds a per-image lock whose acquire is ctx-aware but reconcile contexts carry no deadline, so three Proxies on one host with an uncached image parked every worker. The same edits close a second route to the same pin: go-libvirt is Disconnect closes the net.Conn it was handed (libvirt.go:331 -> socket.go:164), and that conn is our sshSocket, so libvirtBackend.Close is Disconnect goroutine entered closeOnce and hung; the 5 s closeBudget released Close, which called markDead -> kill -> sshSocket.Close and blocked in the other goroutine is sync.Once. Verified against the pinned x/crypto v0.54.0 rather than assumed: the flow-control park (remoteWin.reserve, channel.go:268) is outside writeMu and only writePacket holds it across the TCP write, so a writer parked in SSH flow control never blocks the close and a writer inside the TCP write always does. To reproduce by hand, shrink SO_SNDBUF well under the 2 MiB channel window (channelWindowSize = 64 * channelMaxPacket) so the socket buffer fills before the window does. The kvm provider does not have this bug: it closes client only and never touches the forwarded channel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7.6 KiB
Execution: fix the kvm2 code-review findings
Plan: docs/plans/2026-09-17-1515-kvm2-review-fixes.md
- Step 0 — Save plan into the repo
- 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:
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.
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.