Skip to content

Fix entity state stuck out-of-sync via out-of-order event delivery#14

Merged
dansimau merged 3 commits into
mainfrom
fix/out-of-order-state-updates
Jul 18, 2026
Merged

Fix entity state stuck out-of-sync via out-of-order event delivery#14
dansimau merged 3 commits into
mainfrom
fix/out-of-order-state-updates

Conversation

@dansimau

@dansimau dansimau commented Jul 18, 2026

Copy link
Copy Markdown
Owner
  • Local entity state could get out of sync with Home Assistant, caused by out of order websocket message delivery due to spawning a goroutine
  • Fix is to restore ordering: buffer the per-listener response channel and send sequentially from the single read loop, instead of spawning a goroutine per message
  • A single FIFO sender preserves order; the buffer keeps the read loop from blocking so CallService responses still flow
  • Also add a staleness guard: drop incoming updates whose LastUpdated is older than the currently held state

Local entity state could get stuck disagreeing with Home Assistant (e.g.
HAL reports a light "on" while it is really "off"). The state dump showed a
mix of two events (state/last_changed from an "on" event, last_updated from
a later "off" event), i.e. a newer state overwritten by an older one.

Root cause was three things together:
- hassws/client.go dispatched every incoming frame in a new goroutine onto a
  shared unbuffered channel. Go gives no ordering among goroutines racing on
  a channel send, so rapid on/off events (plus the Hue strip's post-on
  attribute-settle events) could reach the handler reordered.
- Entity.SetState is an unconditional overwrite and there was no staleness
  check anywhere, so processing was strictly last-writer-wins.
- The state only self-healed on the next reconnect via syncStates.

Fix (two layers):
- Deliver events in order: buffer the per-listener response channel
  (eventChannelBufferSize) and send sequentially from the single read loop
  instead of spawning a goroutine per message. The buffer keeps the read loop
  from blocking so synchronous CallService responses still flow.
- Add a staleness guard in StateChangeEvent: drop an update whose LastUpdated
  is older than the currently held state (equal still applies). Also stop a
  nil NewState from overwriting the stored/persisted state with nil.

Adds tests for in-order delivery, stale-update rejection, and nil-state
handling.

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

responseListenerCh <- msgBytes

P2 Badge Avoid blocking the websocket reader on full event queues

When an automation triggered by a subscription event calls a synchronous service method (for example Light.TurnOn calls Connection.CallService at entity_light.go:64), the handler waits for a result that is delivered by this same websocket read loop. If more than eventChannelBufferSize subscription frames are already queued ahead of that result, this synchronous send blocks on the full subscription channel, so the reader can no longer read and deliver the service result and the action times out instead of completing. Preserving per-subscription order needs a queue/drainer that does not let one busy subscription stop the shared response reader.

ℹ️ 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 connection.go
Comment thread hassws/client.go
Addresses PR review (Codex): buffering the per-listener channel and sending
sequentially preserved order but did not fully decouple the read loop. A
subscription handler that makes a synchronous service call (e.g.
Light.TurnOn -> CallService) waits for a result delivered by the same read
loop; if more than eventChannelBufferSize frames were queued ahead of that
result, the read loop would block on the full event channel and the call
would time out.

Introduce dispatchInOrder: a drainer pulls frames off the read-loop channel
immediately into an unbounded, order-preserving queue, and a single feeder
invokes the handler in receipt order. The reader can never be stalled by a
slow or reentrant handler, while per-subscription order is preserved. Both
SubscribeEvents and SubscribeEventsRaw now use it; the channel buffer is now
just a small handoff/startup smoother.

Adds TestReaderNotBlockedByBlockedHandler, which blocks a handler while a
backlog larger than the buffer arrives and asserts a concurrent service call
still completes (fails against the previous buffer-only approach).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dansimau

Copy link
Copy Markdown
Owner Author

Good catch — the buffered channel + sequential send preserved ordering but didn't fully decouple the reader, so a handler blocked on a synchronous CallService could still stall it once the backlog exceeded the buffer.

Fixed in 5b3bb28 with a drainer (dispatchInOrder): a goroutine pulls frames off the read-loop channel immediately into an unbounded, order-preserving queue, and a separate feeder invokes the handler in receipt order. The reader is never blocked by a slow/reentrant handler, and per-subscription order is preserved. Both SubscribeEvents and SubscribeEventsRaw use it now; the channel buffer is demoted to a small handoff/startup smoother.

Added TestReaderNotBlockedByBlockedHandler: it blocks a handler on the first event while a backlog larger than the buffer (300 > 256) piles up, then asserts a concurrent service call still completes. It fails against the previous buffer-only approach ("read loop stalled") and passes with the drainer.

The dispatchInOrder drainer (a goroutine feeding an unbounded mutex/cond
queue) was more machinery than the failure it guarded against warrants. If
the event channel fills while a handler is blocked on a synchronous
CallService, the reader backpressures and the call can time out - but that is
rare (needs a burst larger than the buffer within the 3s call timeout), and
it self-heals: the call times out, the handler unblocks, the backlog drains.
No crash, no lost events, no state corruption.

Drop the drainer and instead size the per-listener channel buffer generously
(256 -> 8192; a chan []byte only preallocates slice headers, so this is
cheap). SubscribeEvents/SubscribeEventsRaw consume the channel directly in a
single goroutine again, which still preserves order (single consumer, FIFO
channel). The sequential send in the read loop and the staleness guard in
StateChangeEvent are unchanged, so the original out-of-order-state fix stands.

Remove TestReaderNotBlockedByBlockedHandler, which asserted a "reader never
stalls" guarantee we are intentionally no longer providing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dansimau

Copy link
Copy Markdown
Owner Author

Follow-up: I reconsidered the drainer (63605fe) and removed it in favour of a simpler large bounded channel.

Rationale: the worst case if the event channel fills while a handler is mid-CallService is a single dropped command + a ~3s hiccup, and it's self-healing — the call times out, the handler unblocks, the backlog drains, and the late reply is swallowed by the existing recover(). No crash, no lost events, no state corruption. It also requires a burst larger than the buffer within the 3s call timeout to occur at all.

Since a chan []byte buffer only preallocates slice headers, sizing it generously (256 → 8192) is cheap and pushes that probability to ~zero, without the drainer goroutine + unbounded mutex/cond queue. SubscribeEvents/SubscribeEventsRaw consume the channel directly again (single consumer + FIFO channel still preserves order). The out-of-order-state fix is unaffected — that comes from the sequential send + the staleness guard in StateChangeEvent, both unchanged.

Net: -118 lines vs the drainer version. I dropped TestReaderNotBlockedByBlockedHandler since it asserted a hard "reader never stalls" guarantee we're intentionally no longer making.

@dansimau
dansimau merged commit 148e8b1 into main Jul 18, 2026
1 check passed
@dansimau
dansimau deleted the fix/out-of-order-state-updates branch July 18, 2026 13:49
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