Fix data races on entity/timer state and run tests with -race#16
Conversation
The race detector flagged concurrent access to shared state that the existing locking didn't cover: - Entity.state was written by the WebSocket reader goroutine (via StateChangeEvent) while being read from automations, user code, and tests. Connection.mutex only serialized dispatch, not the entity's own field. Guard it with a sync.RWMutex in GetState/SetState/GetID. - Timer.running/timer/ctx were mutated by the clock.AfterFunc callback goroutine concurrently with StartContext/Cancel/IsRunning. Guard them with a sync.Mutex (released before invoking the callback fn). - Test-local bool flags written inside automation actions in connection_metrics_test.go raced with the test goroutine; switch to atomic.Bool. Enable the race detector in `make test` so these are caught going forward, with SHELL := /bin/bash and `set -o pipefail` so a detected race actually fails the build instead of being swallowed by the pipe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b951e5262
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow-up to the race fixes, addressing review feedback (Codex bot + local
code review):
- store: pin in-memory SQLite to a single connection. :memory: databases
are per-connection, so a migration on one pooled connection is invisible
to a query on another ("no such table: logs"). Under -race the pool is
more likely to hand out a second connection, which is what made
`go test -race ./logger` flaky. File-backed DBs keep the default pool.
- logger: fix a data race on the shared default Service's stopChan. It was
written under lock in Start but read without the lock in pruneLogs's
select and in Stop. pruneLogs now watches a channel captured at start and
Stop takes the lock.
- timer: Cancel now clears running so IsRunning reports false after a
cancel; StartContext records fn/ctx on the Timer so resetting an existing
timer fires the latest callback instead of the one captured on first
start (t.ctx is now actually read, in the new fire method).
- tests: replace fixed time.Sleep + assert with WaitFor polling in the
metrics tests so they don't flaky-fail under the slower -race build, and
use atomic.Bool for the TestTimer flag (written in the callback goroutine,
read in the test goroutine).
Verified clean with `go test -race -count=3 ./...` and `make test`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — good catch on Root cause: the failure wasn't a data race per se — it's the classic SQLite Fix: While in here I also fixed a couple of other things
|
Summary
Running
go test -race ./...surfaced ~25 data races from three root causes. This fixes them and enables the race detector inmake testso they're caught going forward.Root causes & fixes
Entity.state(production). Written by the WebSocket reader goroutine (viaStateChangeEvent) while read from automations, user code, and tests.Connection.mutexonly serialized dispatch — not the entity's own public field. Now guarded by async.RWMutexinGetState/SetState/GetID. All entity subtypes (Light,BinarySensor,InputBoolean, …) embed*Entity, so they're all covered.Timer.running/timer/ctx(production). Mutated by theclock.AfterFunccallback goroutine concurrently withStartContext/Cancel/IsRunning. Now guarded by async.Mutex, released before invoking the callbackfnto avoid re-entrancy deadlocks.boolflags inconnection_metrics_test.go, written inside automation actions and read from the test goroutine. Converted toatomic.Bool(matching the existingatomic.Int32pattern inTestLoopProtection).Makefile
go testnow runs with-race. AddedSHELL := /bin/bashandset -o pipefailso a detected race actually fails the build instead of being swallowed by the pipe into the colouriser.Verification
go test -race -count=1 ./...clean across 3 fresh runs.make testpasses end-to-end (coverage gate at 63.4%) and now exercises the race detector.gofmt/go vetclean on the touched files.🤖 Generated with Claude Code