Deep Review: 20260716-204108-pr-5216

Date2026-07-16 20:54
Repolima-vm/lima
Round1 (of PR 5216)
Author@AkihiroSuda
PR#5216 — portfwdserver: fix socket FD leak per forwarded connection
Branchfix-5210
Commits6920d840 portfwdserver: fix socket FD leak per forwarded connection
Review SHA6920d840efa25ec22fd78834354532e830470251
ReviewersClaude Opus 4.8 (effort: xhigh), Codex GPT 5.5 (effort: xhigh), Gemini 3.1 Pro (effort: default)
VerdictMerge as-is — the fix is correct, both halves are load-bearing, and both new tests fail on revert
Wall-clock time16 min 42 s

Executive Summary

The fix is correct and both halves are load-bearing: sync.Once + close(closeCh) makes teardown idempotent, and defer conn.Close() releases the guest FD. I verified the root cause against inetaf/tcpproxy's source and confirmed both new tests fail on revert. The three agents raised four test-and-style suggestions and one Important finding — half-close tears down the tunnel early — which is real but predates this PR, so I downgraded it. Merge as-is.

Codex found nothing. Gemini raised the half-close issue as Important. Claude raised four suggestions and independently classified half-close as pre-existing. I reproduced the teardown mechanism against the vendored tcpproxy source and ran the tests against the pre-fix tree; both checks confirm the fix and contradict the Important label. No finding blocks the merge.

Critical Issues

None.

Important Issues

None.

Gemini raised one Important finding (half-close tears down the guest connection before the response is relayed). The mechanism is real, but it predates this PR, so it moves to Design Observations as a pre-existing concern. See the Reconciliation note in the retro.

Suggestions

S1. Integration test asserts the FD release but not the goroutine termination Claude
pkg/portfwdserver/server_test.go
		assert.Assert(t, false, "Start did not return after the stream ended")
	}

	// If the tunnel server fully closed its side, writes from the guest
	// service eventually fail (RST). With a leaked FD they succeed forever.
	deadline := time.After(5 * time.Second)
	for {
		if _, err := guestConn.Write([]byte("x")); err != nil {
			return
		}
		select {
		case <-deadline:
			assert.Assert(t, false, "guest connection still writable; FD was not closed")
		case <-time.After(10 * time.Millisecond):
		}

Issue #5210 reports two leaks per forwarded connection: one FD and one goroutine. TestTunnelServerClosesGuestConn asserts only the FD, and it reaches that assertion through Start's defer conn.Close() rather than through HandleConn completing. Restore the blocking sends while keeping defer conn.Close() and the HandleConn goroutine wedges forever in rw.Close(), yet this test still passes. TestGRPCServerRWCloseNeverBlocks catches that case at the unit level, so the branch as a whole covers it; the gap is that the test whose doc comment claims to reproduce "the teardown sequence of tcpproxy.DialProxy.HandleConn" never verifies the sequence finishes.

Fix: assert the proxy goroutine settles after startDone, or expose its completion for the test to wait on.

S2. fakeTunnelStream.Recv ignores the stream context, confining the test to one teardown path Claude
pkg/portfwdserver/server_test.go
	recvCh chan *api.TunnelMessage
}

func (s *fakeTunnelStream) Context() context.Context { return s.ctx }

func (s *fakeTunnelStream) Recv() (*api.TunnelMessage, error) {
	msg, ok := <-s.recvCh
	if !ok {
		return nil, io.EOF
	}
	return msg, nil
}

func (s *fakeTunnelStream) Send(*api.TunnelMessage) error { return nil }

// TestTunnelServerClosesGuestConn verifies that the connection dialed to the
// guest service is fully closed (not just shut down for writing) when the

A real gRPC server stream unblocks a parked Recv() when the handler returns. This fake ignores s.ctx, so it unblocks only when the test closes recvCh. That confines coverage to the host-half-closes-first path. The two paths that produced the reported leak — the guest service closing first (CloseWrite) and the host cancelling the stream (ctx.Done, server.go:50-52) — cannot be driven through this fake, because Recv would park forever. The fake constrains what the suite can test rather than masking a live bug.

pkg/portfwdserver/server.go
	// goroutine is still blocked reading from the guest, and unblocks that
	// read so the goroutine can finish.
	defer conn.Close()
	rw := &GRPCServerRW{stream: stream, id: in.Id, closeCh: make(chan any)}
	go func() {
		<-ctx.Done()
		rw.Close()
	}()

	proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
		return conn, nil
	}}
	go proxy.HandleConn(rw)

Fix: select on both s.recvCh and s.ctx.Done(), returning s.ctx.Err() for the latter.

S3. chan any is vestigial now that closeCh only ever closes Claude
pkg/portfwdserver/server.go
}

type GRPCServerRW struct {
	id      string
	stream  api.GuestService_TunnelServer
	closeCh chan any
	// closeOnce guards closeCh. Close, CloseRead, and CloseWrite may be
	// called in any order and multiple times (e.g., tcpproxy calls
	// CloseRead/CloseWrite from each copy direction and then Close), so
	// they must be idempotent and must never block; otherwise the proxy
	// goroutine gets stuck and never closes the dialed guest connection,

chan any fit the old design, where each Close* sent a struct{}{} into a 1-slot buffer. Nothing is sent any more: the channel is only closed and received from once, so the element type misleads a reader into expecting a delivered value. chan struct{} is the dominant idiom in pkg/ (7 make(chan struct{}) against 4 make(chan any, two of which are the sites under review), and the new test already uses chan struct{} for its own done signal.

Fix: change the field and both construction sites (server.go:48, server_test.go:57) to chan struct{}.

S4. A GRPCServerRW built without closeCh now panics where it used to block Claude
pkg/portfwdserver/server.go
	return n, nil
}

func (g *GRPCServerRW) Close() error {
	logrus.Debugf("closing GRPCServerRW for id: %s", g.id)
	g.closeOnce.Do(func() { close(g.closeCh) })
	return nil
}

// By adding CloseRead and CloseWrite methods, GRPCServerRW can work with
// other than containers/gvisor-tap-vsock/pkg/tcpproxy, e.g., inetaf/tcpproxy, bicopy.Bicopy.

close() on a nil channel panics; the previous g.closeCh <- struct{}{} blocked forever. Nothing reachable panics today: every field is unexported and the type has no constructor, so the only valid construction is the composite literal in Start (server.go:48). The footgun is live in-package, though — TestGRPCServerRWReadShortBuffer (server_test.go:37) already builds one without closeCh, and adding a defer rw.Close() there would crash the test binary. In the guest agent such a panic would land in the HandleConn goroutine and take the process down.

pkg/portfwdserver/server.go
	// The tunnel is unusable once this function returns, so close the dialed
	// connection here as well. This both releases the FD even if the proxy
	// goroutine is still blocked reading from the guest, and unblocks that
	// read so the goroutine can finish.
	defer conn.Close()
	rw := &GRPCServerRW{stream: stream, id: in.Id, closeCh: make(chan any)}
	go func() {
		<-ctx.Done()
		rw.Close()
	}()
pkg/portfwdserver/server_test.go
	return msg, nil
}

func TestGRPCServerRWReadShortBuffer(t *testing.T) {
	payload := bytes.Repeat([]byte("x"), 100)
	rw := &GRPCServerRW{stream: &recvOnlyTunnelServer{msgs: []*api.TunnelMessage{{Data: payload}}}}

	var got []byte
	buf := make([]byte, 10)
	for len(got) < len(payload) {
		n, err := rw.Read(buf)

Fix: add a newGRPCServerRW constructor that owns the invariant, and build from it in Start and the tests.

Design Observations

Concerns

Half-close from the host tears down the guest connection before the response is relayed (future) Gemini Claude

Start returns as soon as any Close* fires, so a client that calls shutdown(SHUT_WR) and waits for a reply loses it. The chain: the host's io.Copy finishes, GrpcClientRW.CloseWrite() calls stream.CloseSend() (pkg/portfwd/client.go:129), the guest's Recv() returns io.EOF, and closeRead(rw) closes closeCh and ends the tunnel.

This predates the PR. I confirmed it by running the new tests against the merge-base: TestTunnelServerClosesGuestConn fails at the write loop (server_test.go:151), meaning it passed the startDone assertion — the old code also returned from Start on the same CloseRead, and lost the response to a dead stream. This PR changes only how the guest connection dies: a prompt close instead of a lingering leaked FD, which is strictly better.

Gemini proposed dropping closeOnce.Do from CloseRead. That enables half-close but changes teardown semantics well beyond a leak fix, and it needs its own tests — the guest→host direction then parks in conn.Read until the guest service closes or the host cancels the stream. Treat it as follow-up work, not a change to this PR.

Strengths

The root-cause analysis is exact and independently verifiable (in-scope) Claude

HandleConn registers defer dst.Close() (tcpproxy.go:390) before defer src.Close() (:396), so LIFO ordering means a blocked src.Close() permanently strands dst.Close(). rw receives exactly three Close* calls against one receive and a 1-slot buffer, so the third blocks. The commit message states this correctly.

defer conn.Close() also fixes an unreported UDP leak (in-scope) Claude

Datagram sockets never EOF, so for udp tunnels the guest→host io.Copy parks in conn.Read forever and HandleConn's second <-errc never fires — meaning sync.Once alone would not have fixed UDP. Closing conn from Start is what unblocks that read. The same reasoning applies to a TCP guest service that holds its side open, which is what the code comment at server.go:43-46 anticipates.

sync.Once + close() replaces a caveat with an enforced invariant (in-scope) Codex Claude

The one-shot-signal pattern preserves the single receive in Start while making repeated calls safe, and it retires the old "we can't close rw.closeCh" comment.

Testing Assessment

Both new tests are genuine regression tests, and each half of the fix has its own covering test. I verified this against the merge-base rather than taking the claim on trust:

Untested scenarios, ranked by risk: the guest service closing first (CloseWrite) — the highest-traffic production path from the #5210 repro; the host cancelling the stream (ctx.Done), which is the only path that reclaims a UDP tunnel; proxy goroutine termination (S1); and UDP tunnels, whose teardown differs materially yet no test passes Protocol: "udp". S2's fake blocks the first two.

Documentation Assessment

No gaps. The struct comment (server.go:71-76) records the invariant a future editor needs, and the stale "We can't close rw.closeCh" line was removed rather than left to mislead. No user-facing surface changed, so no changelog or README update is warranted.

Commit Structure

Clean. One self-contained commit, DCO signed-off, with tests and fix landing together. The message states the mechanism and its consequence without narrating the diff.

Acknowledged Limitations

The author documents the central trade-off at server.go:43-46: once Start returns the tunnel is unusable, so closing conn there releases the FD even while the proxy goroutine is still blocked reading from the guest. I verified this holds on both the TCP and UDP paths. The half-close concern above extends this rationale rather than contradicting it — the comment covers why closing is safe once Start returns, not when Start decides to return.

Unresolved Feedback

None. The reporter (@jhgaylor) confirmed the fix on the setup that produced #5210: 16 FDs at baseline and 16 after 301 connections, against ~0.83 FDs leaked per connection on stock 2.1.4.

Agent Performance Retro

Claude

Carried the review. Every consolidated suggestion is Claude's, and it was the only agent to trace the fix into the dependency and the gRPC runtime rather than reasoning from the diff. Two results stand out. It reached the half-close question independently of Gemini and classified it correctly as pre-existing, citing the blame SHA for the <-rw.closeCh line — the same conclusion my own merge-base run reached. It also found the UDP leak nobody else saw, including the author: because datagram sockets never EOF, sync.Once alone would not have fixed UDP, which makes defer conn.Close() necessary rather than defensive. Its self-verification was honest: it reported testing revert scenarios, and both claims I re-ran independently held up exactly. S4's severity label ("regression") is arguable for an unreachable path, but it presented the reachability analysis rather than hiding it.

Codex

Returned a clean no-findings review and ran the suite with -race, which I verified independently. For a 132-line diff that three reviewers agree is correct, "nothing to report" is a defensible outcome, not a failure — and Codex declined to invent filler, which has real value. Still, it was the least informative of the three: it accepted the PR description's root-cause story without checking it against tcpproxy, and it surfaced none of the four test and idiom gaps Claude found in the same files it marked "Reviewed, no issues". Its Design Observations correctly identified the sync.Once fit and that the fix addresses the FD rather than only the signal path.

Gemini

Produced the review's one Important finding, and the underlying mechanism is real: the host genuinely half-closes via stream.CloseSend(), which I confirmed at pkg/portfwd/client.go:129. Reaching that through the host-side client is a non-trivial cross-file trace that Codex missed entirely. Gemini then mislabeled it. It called the behavior a "regression" that "went unnoticed" while also tagging it gap, and its own text asserts both readings; the merge-base run settles it as pre-existing. Its proposed fix would change teardown semantics beyond the PR's scope. The UDP datagram-boundary claim in its strengths is also loose — Read splits a datagram when the caller's buffer is short, which TestGRPCServerRWReadShortBuffer exercises directly, so "exactly one datagram at a time" holds only because io.Copy uses a 32 KB buffer. Per the known-behavior note, it skipped git blame, which is precisely the tool that would have settled the regression question.

Summary

Claude Opus 4.8Codex GPT 5.5Gemini 3.1 Pro
Duration9m 19s2m 23s6m 45s
Findings4Snonenone
Tool calls24 (Bash 19, Read 5)22 (shell 22)—
Design observations411
False positives000
Unique insights500
Files reviewed222
Coverage misses010
Totals4Snonenone
Downgraded001 (I→S)
Dropped000

Claude provided the most value by a wide margin: all four surviving suggestions, the correct half-close classification, and the UDP insight that explains why both halves of the fix are needed. Gemini earned its slot by surfacing half-close at all — a real cross-file trace — but cost a verification round through mislabeling. Codex was cheap and fast, and its independent -race run corroborated the branch, yet it added no finding. The pairing that mattered was Claude and Gemini converging on the same code path from different directions, which is what made the severity question worth settling empirically.

Reconciliation. One severity change: Gemini P1 half-close (CloseRead breaks host-to-guest half-close): important → design observation (pre-existing concern, future). The mechanism verifies, but the merge-base run shows the old code returned from Start on the same CloseRead and lost the response identically, so the PR does not introduce it. Counted as a downgrade, not a false positive.

Review Process Notes

Skill improvements

Repo context updates