Address review feedback on out-of-order state fix (#14)#15
Closed
dansimau wants to merge 1 commit into
Closed
Conversation
- connection.go: gate the staleness guard on the incoming update's timestamp, not the currently held one. Updates that carry no LastUpdated (test fixtures, mock CallService state changes) were always treated as older than a previously synced timestamped state and silently dropped; they now apply. - hassws/client.go: stop the shared read loop from being blocked by a busy subscription. A subscription handler that makes a synchronous CallService waits on a result delivered by the same read loop, so a full subscription channel could deadlock it. Subscription frames are now drained off the response channel immediately into an unbounded, order-preserving queue (runOrderedHandler/frameQueue), decoupling handler execution from draining while keeping per-subscription order. - hassws/client.go: give one-shot requests (CallService, GetStates) a buffer of one instead of the large subscription buffer, and remove their listener once the response arrives so neither the listener nor its buffer lingers for the connection's lifetime. Adds regression tests: untimestamped update applies after a timestamped state; frameQueue ordering/unbounded behaviour; and that a slow handler never blocks the producer feeding runOrderedHandler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2JrD3FnPGqsQgnvXFNi2j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the three Codex review comments on #14. Targets the
fix/out-of-order-state-updatesbranch so it stacks onto that PR.Changes
1. Untimestamped updates apply after a timestamped sync (
connection.go)The staleness guard compared the currently held timestamp instead of the incoming one. After a timestamped sync (from
GetStatesor a real event), anystate_changedpayload without aLastUpdated— which the tests and the mockCallServicestate updates produce — looked "older" than the current state (zero time is before everything) and was dropped, so it never changed state or fired automations. The guard now only compares when the incoming update carries a timestamp.2. Read loop can no longer be blocked by a busy subscription (
hassws/client.go)A subscription handler that makes a synchronous
CallServicewaits on a result delivered by the same read loop. With a plain buffered channel, enough queued subscription frames would fill the buffer, block the read loop's send, and deadlock the pendingCallService(it would time out instead of completing). Subscription frames are now drained off the response channel immediately into an unbounded, order-preserving FIFO (runOrderedHandler/frameQueue), decoupling handler execution from draining while preserving per-subscription order.3. One-shot requests no longer retain a large buffer (
hassws/client.go)CallServiceandGetStateslisteners were given the 256-entry subscription buffer and were not removed until reconnect/shutdown, so every service call retained a large channel for the connection's lifetime. One-shot requests now use a buffer of one and their listener is removed as soon as the response arrives.Tests
TestUntimestampedUpdateAppliesAfterTimestampedState— untimestamped update applies (and fires automations) after a timestamped state.TestFrameQueueOrderAndUnbounded— FIFO order preserved, accepts more items than any fixed buffer with no consumer, drains fully before reporting closed.TestRunOrderedHandlerDoesNotBlockProducerOnSlowHandler— a handler parked mid-frame never blocks the producer feeding the channel (verified to fail if draining is recoupled to the handler).All existing tests continue to pass. (A pre-existing data race in
automations/TestHumanOverrideis unrelated to these changes and untouched.)🤖 Generated with Claude Code
Generated by Claude Code