fix: guard shared instance maps with a RWMutex - #167
Open
FlavioPulli wants to merge 1 commit into
Open
Conversation
`killChannel`, `clientPointer` and `myClientPointer` are created in `main.go` and shared by reference across all service packages and every `MyClient`. `ReconnectClient` deletes from all three without a lock, `StartClient` writes to them, the `Disconnected` handler triggers `ReconnectClient` from a goroutine, and every HTTP handler reads them concurrently. Go fatals on concurrent map access, which kills the whole process and every connected instance with it. This adds a single exported `sync.RWMutex` in the whatsmeow service package, guarding the three maps everywhere they are touched. Lock scopes are kept minimal: values are copied out under `RLock` and the lock is released before any long call, so no network I/O happens while holding it. This is the `develop` counterpart of evolution-foundation#127, which targets `main`. The four hunks that did not carry over are for code that does not exist on this branch (`teardownQR`, `SubmitPasskeyResponse`, `ConfirmPasskey`); everything else applied, and I audited all 61 accesses to the three maps on `develop` to confirm none was left unguarded. `go build ./...` and `go vet ./pkg/...` clean.
Reviewer's GuideAdds a shared RWMutex to guard the three global instance-keyed maps (clientPointer, myClientPointer, killChannel) and updates all read/write sites across services to use it, carefully copying values under RLock and doing potentially blocking work after releasing the lock. Sequence diagram for ReconnectClient map access with ClientMapsMusequenceDiagram
participant WhatsmeowService as whatsmeowService.ReconnectClient
participant ClientMapsMu as ClientMapsMu
participant ClientMaps as clientPointer_myClientPointer_killChannel
WhatsmeowService->>ClientMapsMu: RLock()
WhatsmeowService->>ClientMaps: read clientPointer[instanceId]
WhatsmeowService->>ClientMaps: read myClientPointer[instanceId]
WhatsmeowService->>ClientMapsMu: RUnlock()
alt client exists
WhatsmeowService->>WhatsmeowService: client.IsConnected()/Disconnect()
WhatsmeowService->>WhatsmeowService: client.RemoveEventHandler(mycli.eventHandlerID)
end
WhatsmeowService->>ClientMapsMu: RLock()
WhatsmeowService->>ClientMaps: read killChannel[instanceId]
WhatsmeowService->>ClientMapsMu: RUnlock()
WhatsmeowService->>WhatsmeowService: select on killChan
WhatsmeowService->>ClientMapsMu: Lock()
WhatsmeowService->>ClientMaps: delete clientPointer[instanceId]
WhatsmeowService->>ClientMaps: delete myClientPointer[instanceId]
WhatsmeowService->>ClientMaps: delete killChannel[instanceId]
WhatsmeowService->>ClientMapsMu: Unlock()
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, and left some high level feedback:
- Given how widely
ClientMapsMuis now used, consider encapsulating the shared maps behind a small helper type withGet/Set/Deletemethods instead of exporting a bare mutex, so that future call sites are forced through a single, safer API and it’s harder to accidentally bypass locking. - Patterns like
killChan := m[id]; ... if killChan != nil { killChan <- true; close(killChan) }are repeated with subtle variations across services; extracting a shared helper for sending/closing kill channels would reduce duplication and make it easier to reason about their lifecycle and avoid edge-case races.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given how widely `ClientMapsMu` is now used, consider encapsulating the shared maps behind a small helper type with `Get/Set/Delete` methods instead of exporting a bare mutex, so that future call sites are forced through a single, safer API and it’s harder to accidentally bypass locking.
- Patterns like `killChan := m[id]; ... if killChan != nil { killChan <- true; close(killChan) }` are repeated with subtle variations across services; extracting a shared helper for sending/closing kill channels would reduce duplication and make it easier to reason about their lifecycle and avoid edge-case races.
## Individual Comments
### Comment 1
<location path="pkg/instance/service/instance_service.go" line_range="119-124" />
<code_context>
}
func (c *callService) ensureClientConnected(instanceId string) (*whatsmeow.Client, error) {
+ whatsmeow_service.ClientMapsMu.RLock()
client := c.clientPointer[instanceId]
+ whatsmeow_service.ClientMapsMu.RUnlock()
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against nil client before calling IsConnected/IsLoggedIn in ForceReconnect
`forceReconnectClient` is read from the map and used without a nil check. If the instance ID is missing or the entry was cleared, this can panic when calling `IsConnected()`.
You can keep the existing locking semantics while hardening the code by returning an explicit error when the client is missing, e.g.:
```go
whatsmeow_service.ClientMapsMu.RLock()
forceReconnectClient := i.clientPointer[instanceId]
whatsmeow_service.ClientMapsMu.RUnlock()
if forceReconnectClient == nil {
return fmt.Errorf("client %s not found for force reconnect", instanceId)
}
if forceReconnectClient.IsConnected() && forceReconnectClient.IsLoggedIn() {
return fmt.Errorf("client already connected")
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+119
to
124
| whatsmeow_service.ClientMapsMu.RLock() | ||
| client := i.clientPointer[instanceId] | ||
| whatsmeow_service.ClientMapsMu.RUnlock() | ||
| logger.LogInfo("[%s] Checking client connection status - Client exists: %v", instanceId, client != nil) | ||
|
|
||
| if client == nil { |
There was a problem hiding this comment.
issue (bug_risk): Guard against nil client before calling IsConnected/IsLoggedIn in ForceReconnect
forceReconnectClient is read from the map and used without a nil check. If the instance ID is missing or the entry was cleared, this can panic when calling IsConnected().
You can keep the existing locking semantics while hardening the code by returning an explicit error when the client is missing, e.g.:
whatsmeow_service.ClientMapsMu.RLock()
forceReconnectClient := i.clientPointer[instanceId]
whatsmeow_service.ClientMapsMu.RUnlock()
if forceReconnectClient == nil {
return fmt.Errorf("client %s not found for force reconnect", instanceId)
}
if forceReconnectClient.IsConnected() && forceReconnectClient.IsLoggedIn() {
return fmt.Errorf("client already connected")
}
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.
developcounterpart of #127, which targetsmain. Opening it here because on #128 @iagocotta asked that PRs go todevelop; #127 is the only one of my earlier PRs that did not get a replacement, and it is also the one with independent production confirmation from another user.Problem
killChannel,clientPointerandmyClientPointerare created inmain.goand shared by reference across all service packages and everyMyClient.ReconnectClientdeletes from all three without a lock,StartClientwrites to them, theDisconnectedhandler triggersReconnectClientfrom a goroutine, and every HTTP handler reads them concurrently.Go fatals on concurrent map access. These are runtime fatal errors, not panics, so
recover()is not an option: every occurrence kills the process and takes all instances down with it.The codebase already acknowledges the hazard — the comment on
teardownQRinpkg/whatsmeow/service/whatsmeow.go(onmain) calls these "unsynchronized service-wide maps" and notes that touching them from another goroutine "risks afatal error: concurrent map writes". That workaround protects a single call site; the maps stay unguarded everywhere else.Confirmed in production, twice, by two independent deployments
@Matheusagostinho reported on #127 (2026-08-04), running
0.7.2with 5 instances:with
RestartCount: 2matching the two fatals, and a goroutine dump throughinstance.Connect/handleQRCodes/webhookProducer.Produce— concurrent connects while another instance was in an active QR flow.We hit the same thing on our own deployment, and it is severe in a way the crash trace does not show: restarting the container does not bring the instances back. They stay down until an explicit
POST /instance/connectper instance, so every crash is a full outage until someone reconnects them by hand.Change
A single exported
sync.RWMutexin the whatsmeow service package, guarding the three maps everywhere they are touched. Lock scopes are kept minimal: values are copied out underRLockand the lock is released before any long call, so no network I/O happens while holding it.Notes on porting to
developFour hunks from #127 did not carry over because the code does not exist on this branch:
teardownQR,SubmitPasskeyResponseandConfirmPasskey. Everything else applied. Sincemainanddevelophave no common ancestor, I did not assume the site lists matched — I audited all 61 accesses to the three maps ondevelopand confirmed none was left unguarded, and that everyLock/RLockhas its matching release in the same file.go build ./...andgo vet ./pkg/...clean.@Matheusagostinho — a minimal reproduction with parallel
/instance/connectcalls would still be valuable, if the offer stands. It would make this much easier to accept than two independent field reports.Summary by Sourcery
Guard shared instance-level client, MyClient, and kill-channel maps with a global RWMutex across all services to prevent concurrent map access crashes.
Bug Fixes:
Enhancements: