Skip to content

test: fix a data race in the domain list test server - #25

Merged
patramsey merged 1 commit into
mainfrom
fix/domain-server-test-race
Aug 2, 2026
Merged

test: fix a data race in the domain list test server#25
patramsey merged 1 commit into
mainfrom
fix/domain-server-test-race

Conversation

@patramsey

Copy link
Copy Markdown
Owner

CI's Test job went red on a docs-only PR (#23), with a race report rather than a failed assertion:

WARNING: DATA RACE
Previous write at 0x00c000138090 by goroutine 24:
  cmd/domain.domainServer.func1()
      cmd/domain/list_get_test.go:49
--- FAIL: TestDomainList_AllFetchesAllPages (0.00s)
    testing.go:1712: race detected during execution of test

Cause

domainServer's handler appended each request URL to a captured slice with no synchronization:

var received []string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	received = append(received, r.URL.String())   // <- line 49

runList fetches pages 2..N concurrently through an errgroup with SetLimit(5), so that handler runs on several goroutines at once and two can append simultaneously. Concurrent unguarded append is a data race by definition, and the slice header write is what -race caught.

Not a product bug. The racing state is the test's own bookkeeping, not anything runList touches. The concurrent paging it exercises is deliberate and correct.

Fix

The append is mutex-guarded, and the recorder is returned as an accessor that takes the lock and returns a copy, rather than the raw *[]string it was before. The pointer form makes an unlocked read the path of least resistance at every call site; the accessor removes that option instead of relying on each caller to remember.

On verification — read this before approving

I could not reproduce this locally. With the mutex removed I ran 600 iterations (-count=200 across -cpu=1,2,4, and again under GOMAXPROCS=2) and never tripped the detector. The race needs two page requests genuinely in flight at once, and over loopback on a fast machine the first finishes before the second starts. The CI runner is slower and busier, so it interleaves.

So the evidence here is not a mutation test. It's the race detector's own stack trace naming this exact line and two httptest handler goroutines — which is conclusive about where the race is, and the fix guards precisely that. The meaningful confirmation is CI on this PR.

This has been failing intermittently on main all along; #23 is just where it surfaced. #24 passed twice with the same code, which is consistent with an intermittent race rather than anything either PR introduced.

Test plan

  • go vet ./cmd/domain/ clean
  • golangci-lint run — no issues
  • go test -race -count=1 ./cmd/domain/ passing; 10 consecutive -race runs of TestDomainList clean
  • Behavior unchanged — the same assertions run, reading through the accessor

CI's Test job failed on a docs-only PR with a race report, not a test
assertion:

  WARNING: DATA RACE
  Previous write at 0x00c000138090 by goroutine 24:
    cmd/domain.domainServer.func1()
        cmd/domain/list_get_test.go:49
  --- FAIL: TestDomainList_AllFetchesAllPages
      testing.go:1712: race detected during execution of test

domainServer's handler appended each request URL to a captured slice with
no synchronization. runList fetches pages 2..N concurrently through an
errgroup with SetLimit(5), so that handler runs on several goroutines at
once and two of them can append at the same time. Concurrent unguarded
append is a data race by definition, and the slice header write is the
one -race caught.

The append is now mutex-guarded, and the recorder is returned as an
accessor that takes the lock and returns a copy rather than as the raw
*[]string it was before. The pointer form makes an unlocked read the
path of least resistance at every call site; the accessor removes that
option rather than relying on each caller to remember.

Not a product bug: the racing state is the test's own bookkeeping, not
anything runList touches. The concurrent paging it exercises is
deliberate and correct.

Worth being explicit that I could not reproduce this locally. 600
iterations with the mutex removed -- -count=200 across -cpu=1,2,4, and
under GOMAXPROCS=2 -- never tripped the detector, because the race needs
two page requests genuinely in flight at once and over loopback on a
fast machine the first finishes before the second starts. The CI runner
is slower and busier, so it interleaves. The evidence the fix is right
is the detector's own stack trace naming this exact line and two
httptest handler goroutines, not a local repro. It has been failing
intermittently on main all along; a docs PR is just where it surfaced.
@patramsey
patramsey merged commit a8ae3aa into main Aug 2, 2026
3 checks passed
@patramsey
patramsey deleted the fix/domain-server-test-race branch August 2, 2026 18:46
patramsey added a commit that referenced this pull request Aug 2, 2026
#24 merged without a changelog entry because CHANGELOG.md itself only
arrived in #23, which landed at the same time. This backfills it.

Also corrects "No user-facing behavior changes yet", which #24 made
false: `namecom open` now rejects an argument that is not a plausible
domain name rather than passing it through. Someone who was relying on
`open` to accept an arbitrary string will notice, so it belongs under
Changed as well as Security.

The race fix in #25 is deliberately not listed. It is test-only
bookkeeping with no user-visible effect, and Keep a Changelog is for
notable changes rather than a commit log.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant