Skip to content

Two data races the detector finds: Sim.Close reads s.eng while the store writes it, and stubAPI swaps a handler under a live server #581

Description

@A13xB0

go test -race ./... fails in two packages. One is a test bug; the other is in
product code and is the one that matters.

Found on the v0.0.6 tag run, before the race job was moved to dispatch-only.

1. internal/app/session — product code

WARNING: DATA RACE
Write at 0x00c00015b760 by goroutine 3835:
  session.(*Sim).buildSeeded()  internal/app/session/engine.go:393
  session.(*Sim).build()        internal/app/session/engine.go:276
  session.Register.func3()      internal/app/session/verbs.go:169
  state.(*Store).Run()          internal/app/state/store.go:160
Previous read at 0x00c00015b760 by goroutine 3836:
  session.(*Sim).Close()        internal/app/session/enginefirmware.go:126
  session.registerUI.func3.gowrap2()  internal/app/session/ui.go:147
--- FAIL: TestVerbsAreSafeBeforeAnythingIsLoaded

buildSeeded assigns s.eng = engine.New(...) on the store goroutine,
while Close reads if s.eng == nil on the workbench's shutdown
goroutine
. Unsynchronised, on the same field.

What makes this worth reading rather than just fixing: the comment directly
above Close already knows it is called from another goroutine, and takes
care over exactly one field —

Called from the workbench's own shutdown goroutine rather than the store's,
so the served listeners are taken out under their lock and closed only once
it is released - a verb still running on the store's goroutine at the same
moment must never see a map mid-edit.

served is guarded. s.eng, read two lines later in the same function, is
not. The reasoning was right and was applied to one field of the pair.

Consequences are worse than a failed test: a close racing a build can read a
half-assigned s.eng, or read nil and skip the teardown entirely, leaking
the engine and its listeners.

2. internal/fetchasset_test.go — test only

WARNING: DATA RACE
Read at ... by goroutine 16:  net/http.serverHandler.ServeHTTP()
Previous write at ... by goroutine 12:
  internal_test.stubAPI()  internal/fetchasset_test.go:69
--- FAIL: TestFetchReleaseAsset
--- FAIL: TestFetchEmulatorTellsTheTwoStatesApart

stubAPI does httptest.NewServer(nil), which starts serving
immediately
, then assigns srv.Config.Handler = mux at line 69. The serve
loop reads Handler while the test writes it.

The shape is inherited from the handlers needing srv.URL, which does not
exist until the server is up. httptest.NewUnstartedServer(nil) fixes it
without losing that: build the mux (the closures read srv.URL at request
time, not at construction), assign srv.Config.Handler, then srv.Start().

Why this stood

The race job only ran on release tags, and a red detector never blocked
anything — package.yml is a separate workflow, so it sat red without
stopping a release. It is dispatch-only now, which makes running it a
deliberate act rather than noise nobody acts on. That change makes this issue
the record instead.

Reproduce: go test -race -count=1 ./internal/ ./internal/app/session/

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions