fix: data race on the bodyReadTimeout test global (TestStreamingSurvivesBodyDeadline) #539

New issue

Closed

opened 2026-08-20 22:57:15 +02:00 by dries · 0 comments

dries commented

2026-08-20 22:57:15 +02:00

Owner

Found while running go test -race ./internal/server/ during PR #538. Pre-existing on main (reproduced on a clean origin/main checkout at a1cd6577, fails within 3 runs); unrelated to that PR's changes, so filed separately.

go test -race ./internal/server/ -run TestStreamingSurvivesBodyDeadline -count=3
WARNING: DATA RACE
Write at ... by goroutine 133:
  server.TestStreamingSurvivesBodyDeadline.func1()  internal/server/body_deadline_test.go:201
  testing.(*common).Cleanup.func1()
Previous read at ... by goroutine 135:
  server.bodyDeadlineTestServer.func4()             internal/server/body_deadline_test.go:55
  net/http.HandlerFunc.ServeHTTP()

Cause

TestStreamingSurvivesBodyDeadline mutates the package-level bodyReadTimeout (body_deadline_test.go:200) and restores it in t.Cleanup (:201). The /sse handler installed by bodyDeadlineTestServer reads the same global in its flush loop (time.Sleep(bodyReadTimeout), :55). The test returns while that handler goroutine is still sleeping/looping, so the cleanup's write races the handler's read. The listener is never shut down, so nothing joins the in-flight handler.

Not a product-code bug — bodyReadTimeout is only written by tests — but it fails -race non-deterministically, and make test-race is a documented target.

Suggested fix

Any one of:

Have the SSE handler capture bodyReadTimeout into a local once at entry, so the loop doesn't re-read the global.
Give bodyDeadlineTestServer an httptest.Server-style shutdown (or a WaitGroup the handler signals) and join it in cleanup before restoring the global.
Thread the timeout through the server struct instead of a package global, so tests don't mutate shared state at all.

Acceptance criteria

go test -race ./internal/server/ -run TestStreamingSurvivesBodyDeadline -count=10 is clean.
make test-race passes for internal/server.
The test still proves what it guards: SSE responses outlive the body-read bound.

Found while running `go test -race ./internal/server/` during PR #538. **Pre-existing on main** (reproduced on a clean `origin/main` checkout at a1cd6577, fails within 3 runs); unrelated to that PR's changes, so filed separately. ``` go test -race ./internal/server/ -run TestStreamingSurvivesBodyDeadline -count=3 WARNING: DATA RACE Write at ... by goroutine 133: server.TestStreamingSurvivesBodyDeadline.func1() internal/server/body_deadline_test.go:201 testing.(*common).Cleanup.func1() Previous read at ... by goroutine 135: server.bodyDeadlineTestServer.func4() internal/server/body_deadline_test.go:55 net/http.HandlerFunc.ServeHTTP() ``` ## Cause `TestStreamingSurvivesBodyDeadline` mutates the package-level `bodyReadTimeout` (body_deadline_test.go:200) and restores it in `t.Cleanup` (:201). The `/sse` handler installed by `bodyDeadlineTestServer` reads the same global in its flush loop (`time.Sleep(bodyReadTimeout)`, :55). The test returns while that handler goroutine is still sleeping/looping, so the cleanup's write races the handler's read. The listener is never shut down, so nothing joins the in-flight handler. Not a product-code bug — `bodyReadTimeout` is only written by tests — but it fails `-race` non-deterministically, and `make test-race` is a documented target. ## Suggested fix Any one of: - Have the SSE handler capture `bodyReadTimeout` into a local once at entry, so the loop doesn't re-read the global. - Give `bodyDeadlineTestServer` an `httptest.Server`-style shutdown (or a WaitGroup the handler signals) and join it in cleanup before restoring the global. - Thread the timeout through the server struct instead of a package global, so tests don't mutate shared state at all. ## Acceptance criteria - [ ] `go test -race ./internal/server/ -run TestStreamingSurvivesBodyDeadline -count=10` is clean. - [ ] `make test-race` passes for internal/server. - [ ] The test still proves what it guards: SSE responses outlive the body-read bound.