fix(whatsmeow): back off the reconnect loop instead of spinning forever - #197
Open
EcoosUP wants to merge 1 commit into
Open
fix(whatsmeow): back off the reconnect loop instead of spinning forever#197EcoosUP wants to merge 1 commit into
EcoosUP wants to merge 1 commit into
Conversation
An instance whose device was logged out from the phone never stops restarting:
Disconnected -> ReconnectClient -> instance comes up with no session
-> QR -> nobody scans -> max QR count -> forced logout -> Disconnected
Measured in production: 110 reconnects and 1135 QR codes in 50 minutes from a
single instance, stopping only when a human noticed.
Nothing counted those restarts, because from ReconnectClient's point of view
every turn succeeds — the instance really does come up. What fails afterwards is
the pairing, and nobody was measuring that.
The first 5 restarts inside a 15 minute window still go straight through: that
is the good case, a healthy instance that lost its websocket and has to return
within seconds. Past that the loop becomes a growing wait (5, 15, 30 minutes)
and the instance keeps trying — a backoff, not a give-up.
Only one goroutine waits per instance. Without that guard, every Disconnected
arriving during the wait would stack another one and the backoff would become
the loop it was written to stop.
Reviewer's GuideThe PR adds synchronized, per-instance reconnect-loop protection: normal reconnects remain immediate, but repeated disconnects enter a bounded repeating delay while concurrent disconnect events are coalesced. Successful connections clear the state so healthy instances recover quickly and do not inherit stale backoff. Sequence diagram for per-instance reconnect backoffsequenceDiagram
participant WhatsApp
participant Handler as myEventHandler
participant Guard as reconnectAllowed
participant Scheduler as ScheduledWait
participant Service as ReconnectClient
WhatsApp-->>Handler: events.Disconnected
Handler->>Guard: reconnectAllowed(instanceID)
alt first 5 restarts in 15 minutes
Guard-->>Handler: true, 0
Handler->>Service: ReconnectClient(instanceID)
else backoff required
Guard-->>Handler: false, wait
Handler->>Scheduler: time.Sleep(wait)
Scheduler->>Guard: reconnectWaitDone(instanceID)
Scheduler->>Service: ReconnectClient(instanceID)
else another wait is scheduled
Guard-->>Handler: false, -1
Handler-->>WhatsApp: skip duplicate reconnect
end
WhatsApp-->>Handler: events.Connected
Handler->>Guard: reconnectSucceeded(instanceID)
Guard-->>Handler: clear reconnect state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/whatsmeow/service/whatsmeow.go" line_range="346" />
<code_context>
+
+var (
+ reconnectMu sync.Mutex
+ reconnectTrack = map[string]*reconnectState{}
+)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Entries are added to the process-global `reconnectTrack` map on disconnects and are removed only after a `Connected` event. An instance that is deleted, manually stopped, or otherwise never reconnects leaves its state in the map indefinitely, so the map grows with stale per-instance entries for the lifetime of the process.
**Triggers:** When an instance is removed or stopped before emitting `events.Connected`.
**Suggested fix:** Delete the instance's reconnect state from every instance teardown/removal path, or add bounded cleanup for stale entries.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and a wrong threshold or backoff schedule can leave a valid instance disconnected for up to 30 minutes and can cause availability loss before the change is reverted. Reverting stops future waits, but it cannot restore the connection time already missed.
Blocking findings: pkg/whatsmeow/service/whatsmeow.go:346
|
|
||
| var ( | ||
| reconnectMu sync.Mutex | ||
| reconnectTrack = map[string]*reconnectState{} |
There was a problem hiding this comment.
issue (bug_risk): Entries are added to the process-global reconnectTrack map on disconnects and are removed only after a Connected event. An instance that is deleted, manually stopped, or otherwise never reconnects leaves its state in the map indefinitely, so the map grows with stale per-instance entries for the lifetime of the process.
Triggers: When an instance is removed or stopped before emitting events.Connected.
Suggested fix: Delete the instance's reconnect state from every instance teardown/removal path, or add bounded cleanup for stale entries.
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.
The problem
An instance whose device was logged out from the phone never stops restarting:
Measured in production, from one instance after a
Logged out for reason 401: logged out from another device:Why nothing caught it
Nothing counts those restarts, because from
ReconnectClient's point of view every turn succeeds — the instance really does come up. What fails afterwards is the pairing, and that was not being measured. A retry counter around the reconnect call would sit at zero the whole time.The shape of the fix
The first 5 restarts inside a 15 minute window go straight through. That is the good case and it must stay fast: a healthy instance that lost its websocket has to be back in seconds, and that is the common event.
Past that, the loop becomes a growing wait — 5, then 15, then 30 minutes, with the last step repeating. The instance keeps trying: this is a backoff, not a give-up. An instance with a valid session still heals on its own; what is lost is the hammering, from roughly 2 restarts a minute to 2 an hour.
The counter is cleared on
events.Connected, so a drop tomorrow does not inherit today's restarts.One detail worth flagging
Only one goroutine waits per instance (
scheduled). Without that guard, everyDisconnectedarriving during the wait would stack another waiting goroutine, and the backoff would become the very loop it was written to stop.Relationship to the other PRs
This is independent of #194 and #196, but the three came out of the same incident. #194 is the one that actually took the deployment down; this one stops the condition that triggered it. They can be merged in any order.
Testing
go build ./...againstmain, clean. Running in production, adapted to a fork that also carries #194 and #196.The thresholds are the ones that made sense for our load and are easy to disagree with — happy to make them configurable, or to change the numbers, if you prefer.
Summary by Sourcery
Add per-instance reconnect backoff to stop disconnected WhatsApp clients from hammering the restart loop while preserving rapid recovery for healthy sessions.
Bug Fixes:
Enhancements: