Fix lost reconnects caused by unsynchronized Process state transitions - #294
Closed
AmiradelBeyg wants to merge 2 commits into
Closed
Fix lost reconnects caused by unsynchronized Process state transitions#294AmiradelBeyg wants to merge 2 commits into
AmiradelBeyg wants to merge 2 commits into
Conversation
Every ChannelDisconnected event enqueued a new CloseChannel + EstablishNewChannel action. When a second disconnected event arrived while a reconnect was already in flight (e.g. a failed in-flight send racing the connection-level disconnect), the stale action would later close the freshly re-established channel, deregistering a healthy producer/consumer from the broker and forcing another reconnect cycle. ScheduleReconnect now runs at most one reconnect action at a time and re-evaluates the channel state after the action completes, so a disconnect arriving mid-reconnect is never lost and a replacement channel is never closed by a stale action. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Handle is invoked synchronously on the caller's thread, so events race: ChannelConnected arrives from the connection response continuation while ChannelDisconnected arrives from the message dispatcher when an in-flight send fails. The switch writes ChannelState based on the event, but CalculateState reads the shared field afterwards - the two steps are not atomic. A stale Connected write can land between a Disconnected write and its CalculateState call, making both threads read Connected: the disconnect is handled but no reconnect is ever scheduled. Since the dispatcher stops itself after reporting the failure and the channel is already deregistered, no further event arrives and the producer stays disconnected forever while its state reports Connected. Guard the state update and the resulting decision with a per-process lock so every event's decision is made against that event's own state: every handled disconnect now schedules a reconnect. The lock is on the lifecycle-event path only (a handful of events per channel transition) and never executes on the message send path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Consolidating into #293 for easier review - that PR now contains both commits (the reconnect dedup and this synchronization fix) as one coherent change set. |
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
1 participant
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.
Motivation
Process.Handleis invoked synchronously on the caller's thread, so events race:ChannelConnectedarrives from the connection response continuation whileChannelDisconnectedarrives fromSubProducer.MessageDispatcherwhen an in-flight send fails. TheswitchwritesChannelStatebased on the event, butCalculateState()reads the shared field afterwards — the two steps are not atomic.A stale
Connectedwrite can land between aDisconnectedwrite and itsCalculateState()call, making both threads readConnected: the disconnect is handled but no reconnect is ever scheduled. Since the dispatcher stops itself after reporting the failure and the channel is already deregistered from the connection, no further event will arrive — the producer stays disconnected forever whileStatereportsConnected. This is unrecoverable from the application side because the state is not final, so state monitoring never sees a terminal transition.Modifications
Processwith a per-process lock so every event's decision is made against that event's own state: every handled disconnect now schedules a reconnect.Verifying this change
All existing unit tests pass.
Note: this PR is stacked on #293 (its first commit); only the second commit is new here. Happy to rebase once #293 lands, or squash both into one PR if preferred.