fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry - #28050
fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry#28050ibetitsmike wants to merge 2 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdae156e22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| t.Cleanup(server.Close) | ||
| return &http.Response{ | ||
| StatusCode: http.StatusOK, | ||
| Body: contextBlockedBody{ctx: req.Context()}, |
There was a problem hiding this comment.
Would just waiting for req.Context() to expire in the roundTripFunc have the same effect here?
There was a problem hiding this comment.
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.
| })} | ||
|
|
||
| _, _, err := newTestDispatcher(t, server.Client(), server.URL, 50*time.Millisecond).Dispatch( | ||
| _, _, err := newTestDispatcher(t, client, "https://hooks.example.com/coder", 50*time.Millisecond).Dispatch( |
There was a problem hiding this comment.
Why change the server URL here?
There was a problem hiding this comment.
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.
Fixes the
TestDispatcherTimeoutNoRetryflake from CODAGT-919.Problem
The test asserted
requests.Load() == 1where the counter was incremented inside anhttptesthandler, under a hard 50 ms dispatch timeout. On a loaded runner (observed on thetest-go-pg (windows-2022)gauntlet) the deadline expired before the handler was scheduled, so timeout classification succeeded but the request count observed 0.Fix
Replace the real server with a
roundTripFunctransport stub (helper already in the file) that records the attempt synchronously, blocks onreq.Context().Done(), and returns the context error. The dispatcher's own 50 ms deadline still genuinely expires and takes the timeout classification path inpost(), and a retry regression would synchronously invoke the stub a second time, so both assertions keep guarding the same behavior without depending on scheduler timing.Validation
go test ./coderd/x/agenthooks/dispatch -run '^TestDispatcherTimeoutNoRetry$' -count=100and-race -count=40pass; full package suite passes with-race.dispatcher.gofails the test withexpected: "timeout", actual: "protocol_error", confirming the test still guards that branch.