Fix issues with setting item states - #5712
Conversation
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
There was a problem hiding this comment.
Pull request overview
This PR targets the race/consistency issues in GenericItem.applyState/setState around lastStateUpdate / lastStateChange, aiming to ensure listeners/events observe consistent timestamps and state as discussed in #5711.
Changes:
- Introduces synchronized state/timestamp mutation in
applyState, and captures “old” timestamps for inclusion in events. - Refactors the persistence-restore
setState(...)overload to route through the updatedapplyState(...)logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.lastStateChange = lastStateChange; | ||
| } | ||
|
|
||
| applyState(state, lastStateUpdate, source); |
There was a problem hiding this comment.
This is a correct observation. This method was explicitly created to be used with the restore strategy and should restore all values. applyState should not be called as it will update again, sending events and informing listeners should happen from here.
There was a problem hiding this comment.
It is probably worth adding a test to make sure this behaviour is never touched.
| ZonedDateTime oldLastStateUpdate; | ||
| ZonedDateTime oldLastStateChange; |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
AI review performed before manual maintainer review.
The change moves lastStateUpdate / lastStateChange updates before listener notification, which addresses the original race described in #5711. However, AI found two blocking areas that should be addressed before merging:
- The existing review thread about the persistence-restore
setState(...)overload is correct. That overload needs to preserve the complete caller-supplied state history rather than route throughapplyState(). A regression test covering the exact restoredstate,lastState,lastStateUpdate, andlastStateChangevalues should be added. - Synchronizing only the field mutation does not serialize concurrent state updates through listener/event publication. See the inline comment.
It would also be useful for the tests to cover concurrent updates, since simultaneous updates are one of the cases explicitly raised in #5711.
| this.lastState = oldState; | ||
| this.lastStateChange = timestamp; | ||
| } | ||
| } |
There was a problem hiding this comment.
The synchronization currently protects only the state/history mutation, not the complete update operation. After thread A releases this lock it can be suspended, thread B can apply a second update and publish its notifications/events, and then A can resume and publish the first update. This allows the externally submitted order to become S1 -> S2 followed by S0 -> S1, even though the mutations occurred in the opposite order.
There is a similar ordering issue with the timestamp: ZonedDateTime.now() is captured before acquiring this lock, so two concurrent callers can acquire the lock in the opposite order from their timestamps and make lastStateUpdate / lastStateChange move backwards.
Since #5711 explicitly calls out simultaneous updates, should timestamp capture, state/history mutation, and listener/event submission be serialized as one operation (or otherwise put through an ordered mechanism)?
Suggested clean up of GenericItem.{apply,set}State. See #5711
Fixes #5711