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

Merged

coilyco-ops merged 1 commit from aos/claude/knob-race into main

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

coilyco-ops commented

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

Member

Found by running go test -race, which just test does not do, so main is green on this today.

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 call it and take t.Parallel(), so they rewrite maxToolResultBytes and its neighbours while other parallel tests read them.

Mine, and I had already caught it once

I introduced these in #1113. The same hazard bit me earlier in the same session on TestTheAdmissionBoundLeavesAQueueBehindThePool, where I dropped t.Parallel and wrote a comment explaining why. I then wrote three more parallel knob tests without applying my own lesson. The convention was already there to read: not one test in knoboverride_test.go takes t.Parallel.

The three now carry that same comment, so the next reader gets a reason rather than an unexplained absence.

go test -race over the package is clean, and just gate is green.

One thing left deliberately undone

just test runs without -race, so nothing in CI caught this and nothing would catch the next one. Whether the gate should race the suite is a real cost question: it roughly doubles a run, against #838's measurement of 57s for test today. I have noted it on #1136 rather than changing the gate inside a bug fix.

closes #1136

**Found by running `go test -race`, which `just test` does not do**, so main is green on this today. ``` 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 call it **and** take `t.Parallel()`, so they rewrite `maxToolResultBytes` and its neighbours while other parallel tests read them. ## Mine, and I had already caught it once I introduced these in #1113. The same hazard bit me earlier in the same session on `TestTheAdmissionBoundLeavesAQueueBehindThePool`, where I dropped `t.Parallel` and wrote a comment explaining why. **I then wrote three more parallel knob tests without applying my own lesson.** The convention was already there to read: not one test in `knoboverride_test.go` takes `t.Parallel`. The three now carry that same comment, so the next reader gets a reason rather than an unexplained absence. `go test -race` over the package is clean, and `just gate` is green. ## One thing left deliberately undone **`just test` runs without `-race`, so nothing in CI caught this and nothing would catch the next one.** Whether the gate should race the suite is a real cost question: it roughly doubles a run, against #838's measurement of 57s for `test` today. I have noted it on #1136 rather than changing the gate inside a bug fix. closes #1136

coilyco-ops added 1 commit

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

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

ci / image-build (pull_request) Successful in 49s

Details

ci / test (pull_request) Successful in 1m5s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

534845c01e

go test -race over internal/community reports a data race: applyKnobs
writes the package-level tuning variables, and three tests in
scratchreadbound_test.go call it while marked t.Parallel, so they rewrite
maxToolResultBytes under other parallel tests reading it.

just test does not pass -race, which is why CI is green on this.

Mine, from #1113. The same hazard bit me earlier in that session on
TestTheAdmissionBoundLeavesAQueueBehindThePool, where I dropped t.Parallel
and left a comment saying why, and I then wrote three more parallel knob
tests without applying it. The convention was already visible in
knoboverride_test.go, where no knob test takes t.Parallel.

The three carry the same comment as the admission test, so the next reader
gets the reason rather than an unexplained absence.

closes #1136

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: engineer

coilyco-ops merged commit c8e905274d into main

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

coilyco-ops deleted branch aos/claude/knob-race

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

coilyco-ops referenced this pull request from a commit

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

Merge pull request 'fix(test): stop three parallel tests writing the package knobs' (#1137) from aos/claude/knob-race into main

coilyco-ops referenced this pull request

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

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

coilyco-ops referenced this pull request

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