From be434e1eb2db15cb4a0e97c2da33ca39341b1924 Mon Sep 17 00:00:00 2001
From: Viktor Liu <viktor@netbird.io>
Date: Tue, 21 Apr 2026 14:15:04 +0200
Subject: [PATCH] Address PR review: cancel on non-EOF copy errors, stricter
 cap test

- netrelay: only propagate CloseWrite on clean io.EOF; cancel both sides
  on any other copy error so a short write, reset, or broken pipe can't
  leave the opposite direction blocked.
- TestTCPCapPrefersTombstonedForEviction: assert both live pre-cap
  entries survive, not just that the tombstone is gone, so a regression
  that evicts a live entry instead of the tombstone is caught.
---
 client/firewall/uspfilter/conntrack/cap_test.go |  9 +++++++++
 util/netrelay/relay.go                          | 12 ++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/client/firewall/uspfilter/conntrack/cap_test.go b/client/firewall/uspfilter/conntrack/cap_test.go
index 7b7f814f1..ee6b72e7f 100644
--- a/client/firewall/uspfilter/conntrack/cap_test.go
+++ b/client/firewall/uspfilter/conntrack/cap_test.go
@@ -64,6 +64,15 @@ func TestTCPCapPrefersTombstonedForEviction(t *testing.T) {
 	require.False(t, tombstonedStillPresent,
 		"tombstoned entry should be evicted before live entries")
 	require.LessOrEqual(t, len(tracker.connections), 3)
+
+	// Both live pre-cap entries must survive: eviction must prefer the
+	// tombstone, not just satisfy the size bound by dropping any entry.
+	require.Contains(t, tracker.connections,
+		ConnKey{SrcIP: src, DstIP: dst, SrcPort: uint16(20000), DstPort: 80},
+		"live entries must not be evicted while a tombstone exists")
+	require.Contains(t, tracker.connections,
+		ConnKey{SrcIP: src, DstIP: dst, SrcPort: uint16(20002), DstPort: 80},
+		"live entries must not be evicted while a tombstone exists")
 }
 
 func TestUDPCapEvicts(t *testing.T) {
diff --git a/util/netrelay/relay.go b/util/netrelay/relay.go
index 662ce3e1d..a445d69be 100644
--- a/util/netrelay/relay.go
+++ b/util/netrelay/relay.go
@@ -108,7 +108,7 @@ func Relay(ctx context.Context, a, b io.ReadWriteCloser, opts Options) (aToB, bT
 	go func() {
 		defer wg.Done()
 		aToB, errAToB = copyTracked(b, a, &lastActivity)
-		if halfCloseSupported {
+		if halfCloseSupported && isCleanEOF(errAToB) {
 			halfClose(b)
 		} else {
 			cancel()
@@ -118,7 +118,7 @@ func Relay(ctx context.Context, a, b io.ReadWriteCloser, opts Options) (aToB, bT
 	go func() {
 		defer wg.Done()
 		bToA, errBToA = copyTracked(a, b, &lastActivity)
-		if halfCloseSupported {
+		if halfCloseSupported && isCleanEOF(errBToA) {
 			halfClose(a)
 		} else {
 			cancel()
@@ -209,6 +209,14 @@ func halfClose(conn io.ReadWriteCloser) {
 	}
 }
 
+// isCleanEOF reports whether a copy terminated on a graceful end-of-stream.
+// Only in that case is it correct to propagate the EOF via CloseWrite on the
+// peer; any other error means the flow is broken and both directions should
+// tear down.
+func isCleanEOF(err error) bool {
+	return err == nil || errors.Is(err, io.EOF)
+}
+
 func isExpectedCopyError(err error) bool {
 	return errors.Is(err, net.ErrClosed) ||
 		errors.Is(err, context.Canceled) ||