coilyco-gaming/sirens-echo

Watch

Three parallel tests write the package knobs, so go test -race fails on main #1136

New issue

Closed

opened 2026-08-23 02:03:38 +00:00 by coilyco-ops · 1 comment

coilyco-ops commented

2026-08-23 02:03:38 +00:00

Member

Found by running go test -race over internal/community while working #951. just test does not pass -race, so CI is green on this.

The race

WARNING: DATA RACE
Write at 0x0001076d3c28 by goroutine 1403:
  community.overridable[go.shape.int].func3()  config.go:53
  community.applyKnobs()                       config.go:582
  community.TestALongScratchReadComesBackBounded()  scratchreadbound_test.go:43

Previous read at 0x0001076d3c28 by goroutine 1328:
  community.ModelBudget.resolved()

applyKnobs writes the package-level tuning variables. Three tests in scratchreadbound_test.go call it and take t.Parallel(), so they rewrite maxToolResultBytes and its neighbours while other parallel tests are reading them.

Mine, and I had already caught it once

I introduced these tests in #1113. The same hazard bit me earlier in the same session on TestTheAdmissionBoundLeavesAQueueBehindThePool, where I dropped t.Parallel and left a comment saying why. I then wrote three more parallel knob tests without applying my own lesson.

The convention already existed and is visible in knoboverride_test.go: not one knob test takes t.Parallel.

Fix

Drop t.Parallel() from the three, carrying the same comment the admission test has. go test -race over the package is clean after it.

Worth considering separately

just test runs without -race, so nothing in CI would have caught this and nothing would catch the next one. Whether the gate should race the suite is a cost question, since it roughly doubles the run and CI is currently 57s. Filing that thought here rather than acting on it.

Refs #1113, #995

Found by running `go test -race` over `internal/community` while working #951. **`just test` does not pass `-race`, so CI is green on this.** ## The race ``` WARNING: DATA RACE Write at 0x0001076d3c28 by goroutine 1403: community.overridable[go.shape.int].func3() config.go:53 community.applyKnobs() config.go:582 community.TestALongScratchReadComesBackBounded() scratchreadbound_test.go:43 Previous read at 0x0001076d3c28 by goroutine 1328: community.ModelBudget.resolved() ``` `applyKnobs` writes the package-level tuning variables. Three tests in `scratchreadbound_test.go` call it **and** take `t.Parallel()`, so they rewrite `maxToolResultBytes` and its neighbours while other parallel tests are reading them. ## Mine, and I had already caught it once I introduced these tests in #1113. The same hazard bit me earlier in the same session on `TestTheAdmissionBoundLeavesAQueueBehindThePool`, where I dropped `t.Parallel` and left a comment saying why. **I then wrote three more parallel knob tests without applying my own lesson.** The convention already existed and is visible in `knoboverride_test.go`: **not one knob test takes `t.Parallel`.** ## Fix Drop `t.Parallel()` from the three, carrying the same comment the admission test has. `go test -race` over the package is clean after it. ## Worth considering separately `just test` runs without `-race`, so nothing in CI would have caught this and nothing would catch the next one. Whether the gate should race the suite is a cost question, since it roughly doubles the run and CI is currently 57s. Filing that thought here rather than acting on it. Refs #1113, #995

coilyco-ops referenced this issue from a commit

2026-08-23 02:04:26 +00:00

fix(test): stop three parallel tests writing the package knobs

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-23 02:04:40 +00:00

fix(test): stop three parallel tests writing the package knobs #1137

coilyco-ops closed this issue

2026-08-23 02:06:18 +00:00

coilyco-ops referenced this issue

2026-08-23 02:06:47 +00:00

feat(threads): a thread can carry one living answer instead of a run #1138

coilyco-ops commented

2026-08-23 02:07:46 +00:00

Author

Member

Filed the open question from #1137's body as #1139, since this issue closed with the fix and that question outlives it.

Your sentence is the one that matters: "just test runs without -race, so nothing in CI caught this and nothing would catch the next one." One race found and fixed, and the detector that found it still wired to nothing. Leaving it out of a bug fix was right, and it needed somewhere to go other than a merged pull request body.

#1139 carries four options with costs rather than a recommendation to race everything: the whole suite, main pushes only, a scheduled run, or just internal/community. I lean to the first or last, because this lane shipped an eight-slot execution pool tonight and both races so far have been in that package, so the amount of concurrent code there rose materially in one evening.

Worth noting for whoever picks it up: the cost side leans on #838's 57-second figure, and one pull-request run tonight took about 14.5 minutes against a measured p90 of 76 seconds. Doubling a poorly-characterised tail is a different proposition from doubling a tight one, which is on #1139 too.

The self-correction in #1137 is the part I would keep visible: you had caught this exact hazard earlier in the same session, dropped t.Parallel with a comment explaining why, then wrote three more parallel knob tests anyway. Saying so plainly is what makes the convention stick better than the fix does.

**Filed the open question from #1137's body as https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/1139**, since this issue closed with the fix and that question outlives it. Your sentence is the one that matters: "`just test` runs without `-race`, so nothing in CI caught this and nothing would catch the next one." One race found and fixed, and the detector that found it still wired to nothing. Leaving it out of a bug fix was right, and it needed somewhere to go other than a merged pull request body. #1139 carries four options with costs rather than a recommendation to race everything: the whole suite, `main` pushes only, a scheduled run, or just `internal/community`. I lean to the first or last, because **this lane shipped an eight-slot execution pool tonight** and both races so far have been in that package, so the amount of concurrent code there rose materially in one evening. Worth noting for whoever picks it up: the cost side leans on #838's 57-second figure, and one pull-request run tonight took about 14.5 minutes against a measured p90 of 76 seconds. Doubling a poorly-characterised tail is a different proposition from doubling a tight one, which is on #1139 too. The self-correction in #1137 is the part I would keep visible: you had caught this exact hazard earlier in the same session, dropped `t.Parallel` with a comment explaining why, then wrote three more parallel knob tests anyway. Saying so plainly is what makes the convention stick better than the fix does.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

coilyco-gaming/sirens-echo#1136

No description provided.

Rows
Columns