Skip to content

Fix data races on entity/timer state and run tests with -race#16

Merged
dansimau merged 2 commits into
mainfrom
fix-data-races
Jul 18, 2026
Merged

Fix data races on entity/timer state and run tests with -race#16
dansimau merged 2 commits into
mainfrom
fix-data-races

Conversation

@dansimau

Copy link
Copy Markdown
Owner

Summary

Running go test -race ./... surfaced ~25 data races from three root causes. This fixes them and enables the race detector in make test so they're caught going forward.

Root causes & fixes

  • Entity.state (production). Written by the WebSocket reader goroutine (via StateChangeEvent) while read from automations, user code, and tests. Connection.mutex only serialized dispatch — not the entity's own public field. Now guarded by a sync.RWMutex in GetState/SetState/GetID. All entity subtypes (Light, BinarySensor, InputBoolean, …) embed *Entity, so they're all covered.
  • Timer.running/timer/ctx (production). Mutated by the clock.AfterFunc callback goroutine concurrently with StartContext/Cancel/IsRunning. Now guarded by a sync.Mutex, released before invoking the callback fn to avoid re-entrancy deadlocks.
  • Test-local bool flags in connection_metrics_test.go, written inside automation actions and read from the test goroutine. Converted to atomic.Bool (matching the existing atomic.Int32 pattern in TestLoopProtection).

Makefile

go test now runs with -race. Added SHELL := /bin/bash and set -o pipefail so 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 test passes end-to-end (coverage gate at 63.4%) and now exercises the race detector.
  • gofmt / go vet clean on the touched files.

🤖 Generated with Claude Code

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>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread Makefile
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>
@dansimau

Copy link
Copy Markdown
Owner Author

Thanks — good catch on make test going red under -race. Fixed in 2b9f80d.

Root cause: the failure wasn't a data race per se — it's the classic SQLite :memory: gotcha. In-memory databases are scoped to a single connection, but database/sql keeps a connection pool. The migration creates the logs table on one connection; a query that lands on a different pooled connection sees a fresh empty database → no such table: logs. -race shifts scheduling so the pool is more likely to hand out a second connection, which is why it surfaced there.

Fix: store.Open now pins in-memory DSNs to a single connection (SetMaxOpenConns(1)), so the whole Store shares one in-memory database. File-backed DBs keep the default pool (WAL still allows concurrent readers).

While in here I also fixed a couple of other things -race shook out on repeated runs (go test -race -count=3 ./...):

  • A real data race on the shared default logger's stopChan (written under lock in Start, read without the lock in pruneLogs/Stop).
  • TestTimer's flag was written in the callback goroutine and read in the test goroutine without synchronization (now atomic.Bool).
  • The metrics tests used fixed time.Sleep before asserting, which could flaky-fail under the slower race build — switched to WaitFor polling.

make test and go test -race -count=3 ./... are both green now.

@dansimau
dansimau merged commit 39245dc into main Jul 18, 2026
1 check passed
@dansimau
dansimau deleted the fix-data-races branch July 18, 2026 14:29
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