fix(test): stop the serve tests racing on the shared log buffer - #1098
Merged
kgaughan merged 1 commit intoAug 6, 2026
Merged
Conversation
The serve tests capture log output by pointing the process-wide logger at a local buffer with log.SetOutput. Four tests do that, and three of them run with t.Parallel(), so they all run at once against whichever buffer was installed last: one test's request goroutines write through log.Printf while another test reads the same buffer with String(). The race detector catches it. make cov (go test -race) fails on master in 11 of 20 runs here, always with the same pair of stacks -- a bytes.Buffer grow from outputs.logTrace under TestServeHandlesConcurrentRequests against a bytes.Buffer String from TestServeNegotiatingContent or TestServeWithNoContentNegotiation. Guard the buffer with a mutex. The logger stays global and no test changes what it asserts; only the buffer becomes safe to touch from two goroutines. Same 12 runs of the same command are clean afterwards. Fixing the routing properly -- so a parallel test reads only its own log lines -- means giving each test its own logger, which is a larger change to how outputs and serve get their logger. This just stops CI failing. Signed-off-by: Victor Solano <victor.solanonunez@gmail.com>
kgaughan
approved these changes
Aug 6, 2026
3 tasks
vsolano9
added a commit
to vsolano9/goss
that referenced
this pull request
Aug 6, 2026
Picks up goss-org#1098 (serve-test log-buffer race fix), which is the cause of the stale red `coverage` check on this PR's previous head 6a9d2dd. Signed-off-by: Victor Solano <victor.solanonunez@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
make covfails onmasterroughly half the time. I hit it on #1097 and went looking for what I'd broken; it turned out to be already there, so here's the fix on its own.Measured
Same command as the
coveragejob,go test -race -coverpkg=./... -coverprofile=c.out ./..., on cleanmasterfbc8318:DATA RACEmaster(8 runs)master(12 more runs)Cause
Four serve tests capture log output the same way:
log.SetOutputis process-wide, but the buffer is per-test, and three of the four tests aret.Parallel()—TestServeWithNoContentNegotiation,TestServeNegotiatingContent,TestServeHandlesConcurrentRequests. Go runs those together after the sequential tests finish, so they share whichever buffer was installed last. One test's request goroutines write into it throughlog.Printfwhile another reads it withString().Every trace is the same shape:
TestServeHandlesConcurrentRequestsfires 16 concurrent requests, so it is usually the writer, but it isn't the culprit — it is doing exactly what it was added in #1092 to do. The shared buffer is the problem.Fix
A mutex-guarded buffer,
syncBuffer, in a newlogbuffer_test.go. Test-only, no production code touched. The logger stays global and no test changes what it asserts — only the buffer becomes safe to touch from two goroutines.What this deliberately does not fix
The routing is still wrong: a parallel test's log lines can land in another test's buffer. That is harmless today because the three parallel tests only
t.Logfthe buffer — the two tests that actually assert on it,TestServeCacheWithNoContentNegotiationandTestServeCacheNegotiatingContent, are sequential and finish before any parallel test starts.Fixing it properly means giving each test its own logger, which means changing how
outputsandserveget theirs. That is a real change to non-test code and it belongs in its own PR. Happy to do it if you want it.One thing worth recording: in 24 runs of the full suite under
-race, I sawTestServeCacheNegotiatingContent/immediately re-request but different accept header, cache should be warmfail once, asserting the cache was warm when the 100ms window had expired under race instrumentation. It did not reproduce in 12 further runs on eithermasteror this branch, so it looks like a separate load-sensitive flake rather than anything to do with this change. Flagging it rather than quietly leaving it out.AI assistance
This contribution was prepared with AI assistance. Every number above is from running the stated command locally, Go 1.26.5, macOS arm64.