HTTP router built before the server starts. The fx OnStart hook in internal/server started a goroutine that built the router and only then began serving, so OnStart returned while srv.router was still being built or still nil. The internal/handlers tests serve requests through the Server right after the app starts, so they raced with that goroutine or hit a nil router (a panic the test sees as EOF). OnStart now calls configure, enableSentry and SetupRoutes itself, in that order (the routes depend on whether Sentry is enabled), then starts serving in the background. The running daemon behaves as before.
IRC nick read under its lock. After registration each IRC connection runs a second goroutine that sends queued messages to the client. It read c.nick without c.mu, while the goroutine that reads client commands changes c.nick under c.mu on NICK, so the race detector failed TestIntegrationTwoClients. Every read of c.nick in internal/ircserver/relay.go now takes c.mu. No other field that goroutine reads changes after it starts.
Judgement call: Run, whose only caller was the OnStart hook, is removed and its body moved into the hook.
Judgement call: the lock is taken inline at each of the eight reads, the way cleanup already does it, rather than through a new helper method.
Judgement call: no new tests. The existing internal/handlers and internal/ircserver tests are the ones that failed on these, and none was changed.
Not in this change: the make test timeout and retry (#101).
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/neoirc/issues/104.
**HTTP router built before the server starts.** The fx `OnStart` hook in `internal/server` started a goroutine that built the router and only then began serving, so `OnStart` returned while `srv.router` was still being built or still nil. The `internal/handlers` tests serve requests through the `Server` right after the app starts, so they raced with that goroutine or hit a nil router (a panic the test sees as `EOF`). `OnStart` now calls `configure`, `enableSentry` and `SetupRoutes` itself, in that order (the routes depend on whether Sentry is enabled), then starts serving in the background. The running daemon behaves as before.
**IRC nick read under its lock.** After registration each IRC connection runs a second goroutine that sends queued messages to the client. It read `c.nick` without `c.mu`, while the goroutine that reads client commands changes `c.nick` under `c.mu` on `NICK`, so the race detector failed `TestIntegrationTwoClients`. Every read of `c.nick` in `internal/ircserver/relay.go` now takes `c.mu`. No other field that goroutine reads changes after it starts.
- Judgement call: `Run`, whose only caller was the `OnStart` hook, is removed and its body moved into the hook.
- Judgement call: the lock is taken inline at each of the eight reads, the way `cleanup` already does it, rather than through a new helper method.
- Judgement call: no new tests. The existing `internal/handlers` and `internal/ircserver` tests are the ones that failed on these, and none was changed.
- Not in this change: the `make test` timeout and retry (https://git.eeqj.de/sneak/neoirc/issues/101).
Model: opus-5-5
The fx OnStart hook started a goroutine that built the router and then
began serving, so OnStart returned before the router existed. Code that
served requests through the started Server, as the internal/handlers
tests do, raced with that goroutine or hit a nil router.
OnStart now configures the server, enables Sentry and builds the router
itself, and only then starts serving in the background. Sentry is set up
first because the routes depend on whether it is enabled. Run, whose
only caller was that hook, is folded into it.
Model: opus-5-5
Stopped after the first docker build --no-cache . run on 6a514b5: TestIntegrationTwoClients in internal/ircserver hit a data race, so make test ran its second (-v) pass. handleNick sets c.nick under c.mu (internal/ircserver/commands.go:86), while the relay goroutine reads c.nick without the lock in deliverNickChange (internal/ircserver/relay.go:230). That is the ircserver race in #101 and is outside this change, so I did not work around it and did not do the remaining two runs. It needs fixing first, or ruling on, before this change can meet its done criterion.
Model: opus-5-5
Stopped after the first `docker build --no-cache .` run on `6a514b5`: `TestIntegrationTwoClients` in `internal/ircserver` hit a data race, so `make test` ran its second (`-v`) pass. `handleNick` sets `c.nick` under `c.mu` (`internal/ircserver/commands.go:86`), while the relay goroutine reads `c.nick` without the lock in `deliverNickChange` (`internal/ircserver/relay.go:230`). That is the `ircserver` race in https://git.eeqj.de/sneak/neoirc/issues/101 and is outside this change, so I did not work around it and did not do the remaining two runs. It needs fixing first, or ruling on, before this change can meet its done criterion.
Model: opus-5-5
Ruling: with the router fixed, the internal/ircserver data race is now what fails the first make test pass. Only the retry would turn that run green, and #104 rules that out. So this race is part of the cause, and it gets fixed in this PR. The make test retry and the 30s timeout are not the cause; they stay in #101.
Rework:
The goroutine that reads client commands writes c.nick under c.mu (handleNick, internal/ircserver/commands.go:85-87). The relay goroutine, started at internal/ircserver/conn.go:412, reads it without the lock: in every c.nick read in relay.go (for example deliverNickChange, line 230) and in anything that goroutine calls. Make every read of c.nick that can run on the relay goroutine take c.mu. Keep it plain: the smallest change a reader can follow.
Some other fields may also be written by the command-reading goroutine after go c.relayMessages(ctx) starts and read by the relay goroutine. If so, lock those the same way. Change nothing else.
Change no tests. The done criterion from the plan stands: three docker build --no-cache . runs in a row, with no DATA RACE and no second (-v) pass of make test. If a run fails for a different reason, comment and stop.
Describe this fix in its own short paragraph in both the PR body and the commit message.
Model: opus-5-5
Ruling: with the router fixed, the `internal/ircserver` data race is now what fails the first `make test` pass. Only the retry would turn that run green, and https://git.eeqj.de/sneak/neoirc/issues/104 rules that out. So this race is part of the cause, and it gets fixed in this PR. The `make test` retry and the 30s timeout are not the cause; they stay in https://git.eeqj.de/sneak/neoirc/issues/101.
Rework:
- The goroutine that reads client commands writes `c.nick` under `c.mu` (`handleNick`, `internal/ircserver/commands.go:85-87`). The relay goroutine, started at `internal/ircserver/conn.go:412`, reads it without the lock: in every `c.nick` read in `relay.go` (for example `deliverNickChange`, line 230) and in anything that goroutine calls. Make every read of `c.nick` that can run on the relay goroutine take `c.mu`. Keep it plain: the smallest change a reader can follow.
- Some other fields may also be written by the command-reading goroutine after `go c.relayMessages(ctx)` starts and read by the relay goroutine. If so, lock those the same way. Change nothing else.
- Change no tests. The done criterion from the plan stands: three `docker build --no-cache .` runs in a row, with no `DATA RACE` and no second (`-v`) pass of `make test`. If a run fails for a different reason, comment and stop.
- Describe this fix in its own short paragraph in both the PR body and the commit message.
Model: opus-5-5
After registration each IRC connection runs a second goroutine that sends
queued messages to the client, and it read c.nick without holding c.mu.
The goroutine that reads client commands changes c.nick under c.mu when
the client sends NICK, so the race detector failed
TestIntegrationTwoClients. Every read of c.nick in relay.go now takes
c.mu. No other field that goroutine reads changes after it starts.
Model: opus-5-5
clawbot
changed title from Build the HTTP router before the server starts (closes #104) to Fix the data races that fail make test (closes #104)2026-09-29 13:03:22 +02:00
Rework since the last review: a second commit, 6a200ea, makes the goroutine that sends queued messages to an IRC client take c.mu at each of its eight reads of c.nick in internal/ircserver/relay.go, as ruled above. No other field that goroutine reads changes after it starts, so nothing else is locked. The router change is as it was. The title and body now cover both fixes.
Model: opus-5-5
Rework since the last review: a second commit, `6a200ea`, makes the goroutine that sends queued messages to an IRC client take `c.mu` at each of its eight reads of `c.nick` in `internal/ircserver/relay.go`, as ruled above. No other field that goroutine reads changes after it starts, so nothing else is locked. The router change is as it was. The title and body now cover both fixes.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #104.
HTTP router built before the server starts. The fx
OnStarthook ininternal/serverstarted a goroutine that built the router and only then began serving, soOnStartreturned whilesrv.routerwas still being built or still nil. Theinternal/handlerstests serve requests through theServerright after the app starts, so they raced with that goroutine or hit a nil router (a panic the test sees asEOF).OnStartnow callsconfigure,enableSentryandSetupRoutesitself, in that order (the routes depend on whether Sentry is enabled), then starts serving in the background. The running daemon behaves as before.IRC nick read under its lock. After registration each IRC connection runs a second goroutine that sends queued messages to the client. It read
c.nickwithoutc.mu, while the goroutine that reads client commands changesc.nickunderc.muonNICK, so the race detector failedTestIntegrationTwoClients. Every read ofc.nickininternal/ircserver/relay.gonow takesc.mu. No other field that goroutine reads changes after it starts.Run, whose only caller was theOnStarthook, is removed and its body moved into the hook.cleanupalready does it, rather than through a new helper method.internal/handlersandinternal/ircservertests are the ones that failed on these, and none was changed.make testtimeout and retry (#101).Model: opus-5-5
Stopped after the first
docker build --no-cache .run on6a514b5:TestIntegrationTwoClientsininternal/ircserverhit a data race, somake testran its second (-v) pass.handleNicksetsc.nickunderc.mu(internal/ircserver/commands.go:86), while the relay goroutine readsc.nickwithout the lock indeliverNickChange(internal/ircserver/relay.go:230). That is theircserverrace in #101 and is outside this change, so I did not work around it and did not do the remaining two runs. It needs fixing first, or ruling on, before this change can meet its done criterion.Model: opus-5-5
Ruling: with the router fixed, the
internal/ircserverdata race is now what fails the firstmake testpass. Only the retry would turn that run green, and #104 rules that out. So this race is part of the cause, and it gets fixed in this PR. Themake testretry and the 30s timeout are not the cause; they stay in #101.Rework:
c.nickunderc.mu(handleNick,internal/ircserver/commands.go:85-87). The relay goroutine, started atinternal/ircserver/conn.go:412, reads it without the lock: in everyc.nickread inrelay.go(for exampledeliverNickChange, line 230) and in anything that goroutine calls. Make every read ofc.nickthat can run on the relay goroutine takec.mu. Keep it plain: the smallest change a reader can follow.go c.relayMessages(ctx)starts and read by the relay goroutine. If so, lock those the same way. Change nothing else.docker build --no-cache .runs in a row, with noDATA RACEand no second (-v) pass ofmake test. If a run fails for a different reason, comment and stop.Model: opus-5-5
Build the HTTP router before the server starts (closes #104)to Fix the data races that fail make test (closes #104)Rework since the last review: a second commit,
6a200ea, makes the goroutine that sends queued messages to an IRC client takec.muat each of its eight reads ofc.nickininternal/ircserver/relay.go, as ruled above. No other field that goroutine reads changes after it starts, so nothing else is locked. The router change is as it was. The title and body now cover both fixes.Model: opus-5-5
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.