From 10b722651a605bf948b770b7b6379ef744fe737c Mon Sep 17 00:00:00 2001
From: jack <admin@nightc.com>
Date: Sun, 5 Jul 2026 11:03:11 +0800
Subject: [PATCH] fix(browser): prefer a delivered CDP response over the close
 signal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

wsCDP.send selected between the response channel and the connection-closed
signal. When a server writes a result/error frame and immediately drops the
socket, the read loop buffers the frame into the request's channel *and*
closes c.closed, leaving both select cases ready — so Go picked between them
at random and send would intermittently report "cdp connection closed during
<method>" instead of returning the actual response. This surfaced as a flaky
TestManagedBackendErrorPropagation on CI.

Recover the buffered response in the c.closed branch before reporting the
close (the read loop always delivers or closes every pending channel before
signaling c.closed, so a non-blocking receive is decisive). Harden the test
to loop the boom-then-close scenario so the race fails reliably instead of
~half the time; verified it reproduces the CI failure pre-fix (GOMAXPROCS=2,
count=3000) and passes post-fix.

Generated with Jack AI bot
---
 internal/browser/cdp.go      | 15 +++++++++++++++
 internal/browser/cdp_test.go | 28 +++++++++++++++++-----------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/internal/browser/cdp.go b/internal/browser/cdp.go
index 6fe82fa..b5d9f39 100644
--- a/internal/browser/cdp.go
+++ b/internal/browser/cdp.go
@@ -183,6 +183,21 @@ func (c *wsCDP) send(ctx context.Context, sessionID, method string, params any)
 		c.mu.Unlock()
 		return nil, ctx.Err()
 	case <-c.closed:
+		// The read loop signals c.closed only after delivering (or closing) every
+		// pending channel, so a response for THIS request may already be waiting in
+		// ch — e.g. a server that writes a result frame and immediately drops the
+		// socket. Prefer the real response over reporting the close; select alone
+		// would pick between the two ready cases at random (a source of flakes).
+		select {
+		case msg, ok := <-ch:
+			if ok {
+				if msg.Error != nil {
+					return nil, fmt.Errorf("%s: %w", method, msg.Error)
+				}
+				return msg.Result, nil
+			}
+		default:
+		}
 		return nil, fmt.Errorf("cdp connection closed during %s", method)
 	}
 }
diff --git a/internal/browser/cdp_test.go b/internal/browser/cdp_test.go
index eccd474..cf2e84e 100644
--- a/internal/browser/cdp_test.go
+++ b/internal/browser/cdp_test.go
@@ -86,7 +86,11 @@ func TestManagedBackendNewTabAndSend(t *testing.T) {
 }
 
 func TestManagedBackendErrorPropagation(t *testing.T) {
-	// A handler that returns nothing useful; drive the error path by closing.
+	// Reply with a CDP error frame and immediately drop the socket. The error
+	// frame and the connection-close then race inside send's select; the caller
+	// must still surface the "boom" error rather than a "connection closed" one.
+	// Loop so a regression (picking the close signal at random) fails reliably
+	// instead of only ~half the time.
 	up := websocket.Upgrader{}
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		conn, err := up.Upgrade(w, r, nil)
@@ -95,21 +99,23 @@ func TestManagedBackendErrorPropagation(t *testing.T) {
 		}
 		var msg cdpMessage
 		_ = conn.ReadJSON(&msg)
-		// Reply with a CDP error frame.
 		_ = conn.WriteJSON(cdpMessage{ID: msg.ID, Error: &cdpError{Code: -32000, Message: "boom"}})
 		_ = conn.Close()
 	}))
 	defer srv.Close()
 
-	ctx := context.Background()
-	backend, err := connectManaged(ctx, "ws"+strings.TrimPrefix(srv.URL, "http"), nil)
-	if err != nil {
-		t.Fatalf("connect: %v", err)
-	}
-	defer func() { _ = backend.Close() }()
-	_, err = backend.cdp.send(ctx, "", "Target.getTargets", nil)
-	if err == nil || !strings.Contains(err.Error(), "boom") {
-		t.Fatalf("expected boom error, got %v", err)
+	wsURL := "ws" + strings.TrimPrefix(srv.URL, "http")
+	for i := 0; i < 50; i++ {
+		ctx := context.Background()
+		backend, err := connectManaged(ctx, wsURL, nil)
+		if err != nil {
+			t.Fatalf("connect: %v", err)
+		}
+		_, err = backend.cdp.send(ctx, "", "Target.getTargets", nil)
+		_ = backend.Close()
+		if err == nil || !strings.Contains(err.Error(), "boom") {
+			t.Fatalf("iter %d: expected boom error, got %v", i, err)
+		}
 	}
 }