Skip to content

fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry #28050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ibetitsmike wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −9
Open

fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry #28050

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdae156
fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry
ibetitsmike Aug 12, 2026
ecf0af5
fix(coderd/x/agenthooks/dispatch): keep read-path timeout coverage in…
ibetitsmike Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions coderd/x/agenthooks/dispatch/dispatcher_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,19 +197,22 @@ func TestDispatcherTimeoutNoRetry(t *testing.T) {

event := newTestEvent(t, agenthooks.EventStop, agenthooks.StopData{})
var requests atomic.Int32
release := make(chan struct{})
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
// A real server cannot guarantee the handler runs before the short
// dispatch deadline on a loaded machine, so the transport records the
// attempt synchronously. The successful response with a body that blocks
// until the deadline expires keeps the read-path timeout branch covered.
client := &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
requests.Add(1)
w.WriteHeader(http.StatusOK)
assert.NoError(t, http.NewResponseController(w).Flush())
<-release
}))
t.Cleanup(server.Close)
return &http.Response{
StatusCode: http.StatusOK,
Body: contextBlockedBody{ctx: req.Context()},

@johnstcn johnstcn Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would just waiting for req.Context() to expire in the roundTripFunc have the same effect here?

@ibetitsmike ibetitsmike Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion-wise, yes: blocking in RoundTrip until req.Context() expires also yields timeout, just via the client.Do error path instead of the body read. I verified that variant passes.

The reason for the body-blocking stub is branch coverage: this test is the only one exercising the readErr timeout case in the dispatcher (the other timeout tests cancel before or during Do). The original httptest version also timed out mid-read (handler flushed headers, then blocked), and Codex's earlier comment on this PR flagged keeping that path covered. Measured both ways with the readErr timeout case deliberately misclassified as protocol_error: the RoundTrip-blocking variant still passes (coverage lost), the body-blocking stub fails as expected.

Mux replied on Mike's behalf.

Request: req,
}, nil
})}

_, _, err := newTestDispatcher(t, server.Client(), server.URL, 50*time.Millisecond).Dispatch(
_, _, err := newTestDispatcher(t, client, "https://hooks.example.com/coder", 50*time.Millisecond).Dispatch(

@johnstcn johnstcn Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the server URL here?

@ibetitsmike ibetitsmike Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server.URL no longer exists: the fix removes the httptest server entirely, and the stub transport intercepts the request before any dial, so this URL is never resolved or connected to. It only has to parse for request construction, so I used an example.com placeholder in the style of the other no-server tests in this file (http://hooks.example.com/coder, https://unused.test).

Mux replied on Mike's behalf.

testutil.Context(t, testutil.WaitLong), event,
)
close(release)
assertDispatchErrorClass(t, err, ResultTimeout)
require.Equal(t, int32(1), requests.Load())
}
Expand Down Expand Up @@ -709,6 +712,15 @@ func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
return f(req)
}

type contextBlockedBody struct{ ctx context.Context }

func (b contextBlockedBody) Read([]byte) (int, error) {
<-b.ctx.Done()
return 0, b.ctx.Err()
}

func (contextBlockedBody) Close() error { return nil }

func TestDispatcherCapacityClassRequired(t *testing.T) {
t.Parallel()

Expand Down
Loading