From fdfc5ab25467f7f6a4532d6b84f42689b9cda504 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Wed, 26 Aug 2026 09:30:52 -0500 Subject: [PATCH] fix(client): honour an already-cancelled context before sending request marshalled the frame, registered the pending channel and wrote the whole thing to the server before it ever looked at ctx, so a caller whose deadline had already blown still enqueued the job. The select at the bottom cannot cover for that. By the time it runs the response may already be sitting in respCh, and when both cases of a select are ready Go picks between them uniformly at random, so an expired context loses roughly half the time. That is what TestClient_ContextTimeout caught on ubuntu CI, where the read goroutine gets to deliver the response before the requesting goroutine reaches the select. It never reproduced on darwin: 400 runs under load came back clean, because locally the round trip is always slower than the few microseconds between writeFrame returning and the select. Forcing the response to land first, by sleeping in between, failed 27 times in 40. With the check in place that same forced ordering failed 0 in 40. All nine client calls across job, workflow and subscription route through request, so they all pick this up. --- client/client.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/client/client.go b/client/client.go index 2d3d3f5..7204c81 100644 --- a/client/client.go +++ b/client/client.go @@ -262,6 +262,21 @@ func (c *Client) tryReconnect() { // request sends a request frame and waits for the correlated response. func (c *Client) request(ctx context.Context, method string, data any) (*dwp.Frame, error) { + // Checked before anything is marshalled or sent, for two reasons. + // A caller whose deadline has already blown should not still cause a + // side effect on the server, and writeFrame below does not consult + // ctx at all, so without this an expired context still enqueues the + // job. The select at the end of this function cannot stand in for the + // check either: once the response has landed in respCh, both of its + // cases are ready, and Go picks between ready cases uniformly at + // random, so an expired context loses roughly half the time. That is + // what made TestClient_ContextTimeout flake on Linux CI, where the + // read goroutine can deliver the response before this goroutine + // reaches the select. + if err := ctx.Err(); err != nil { + return nil, err + } + frame := &dwp.Frame{ ID: dwp.GenerateFrameID(), Type: dwp.FrameRequest,