fix(whatsmeow): reuse a single capped sqlstore container (fixes Postgres connection leak) - #117
Conversation
…ne per StartClient
StartClient() called sqlstore.New() on every connect AND every reconnect, each
opening a brand-new *sql.DB with MaxOpenConns=0 (unlimited) and ConnMaxLifetime=0
(never expires), and the container was never closed. With instances reconnecting
in a loop (QR expiry, network blips, CONNECT_ON_STARTUP) this leaks one connection
pool per attempt and eventually exhausts Postgres ("sorry, too many clients
already").
Create the auth-store container once (sync.Once) with a bounded pool via
sqlstore.NewWithDB(), and reuse it across all clients — whatsmeow's container is
designed to hold multiple devices. Direct DB connection, capped pool.
…aching the error sync.Once memorized the first failure forever: a Postgres hiccup at the first StartClient left every future connect returning the same stale error until process restart. Replace it with a mutex that only memoizes success — failures retry on the next call. Adds a self-contained regression test using the sqlite path (no external services required).
Reviewer's GuideThis PR fixes a Postgres connection leak in the whatsmeow service by introducing a single, shared, capped sqlstore container that is lazily initialized and reused across all StartClient calls, and by ensuring only successful initializations are memoized; it also adds a regression test to validate retry and reuse semantics using the SQLite path. Sequence diagram for shared capped sqlstore container initialization and reusesequenceDiagram
participant WhatsmeowService as whatsmeowService
participant StartClient as StartClient
participant getAuthContainer as getAuthContainer
participant DB as sql.DB
participant Container as sqlstore.Container
WhatsmeowService->>StartClient: StartClient(cd)
StartClient->>getAuthContainer: getAuthContainer()
activate getAuthContainer
alt sharedAuthContainer already initialized
getAuthContainer-->>StartClient: sharedAuthContainer
else sharedAuthContainer is nil
getAuthContainer->>DB: sql.Open(dialect, address)
alt sql.Open error
getAuthContainer-->>StartClient: error
else sql.Open success
alt dialect == postgres
getAuthContainer->>DB: SetMaxOpenConns(20)
getAuthContainer->>DB: SetMaxIdleConns(5)
getAuthContainer->>DB: SetConnMaxLifetime(5min)
getAuthContainer->>DB: SetConnMaxIdleTime(2min)
else dialect == sqlite
getAuthContainer->>DB: SetMaxOpenConns(1)
end
getAuthContainer->>Container: NewWithDB(DB, dialect, dbLog)
getAuthContainer->>Container: Upgrade(context.Background())
alt Upgrade error
getAuthContainer->>DB: Close()
getAuthContainer-->>StartClient: error
else Upgrade success
getAuthContainer-->>StartClient: sharedAuthContainer
end
end
end
deactivate getAuthContainer
alt container returned
StartClient->>Container: (reuse for device auth)
else error returned
StartClient->>WhatsmeowService: log "Failed to get auth container"
end
File-Level Changes
Possibly linked issues
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:
- The sharedAuthContainer singleton is initialized from whatever whatsmeowService instance happens to call getAuthContainer first, so if different instances have different config (e.g., WaDebug or PostgresAuthDB) the later ones silently reuse a container built with mismatched settings; consider making the singleton initialization config-independent or enforcing a single, well-defined source of configuration.
- auth_container_retry_test mutates the package-level sharedAuthContainer state directly, which can create hidden coupling with other tests in this package; it would be safer to provide a reset helper or to structure getAuthContainer so its shared state can be injected/isolated for testing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The sharedAuthContainer singleton is initialized from whatever whatsmeowService instance happens to call getAuthContainer first, so if different instances have different config (e.g., WaDebug or PostgresAuthDB) the later ones silently reuse a container built with mismatched settings; consider making the singleton initialization config-independent or enforcing a single, well-defined source of configuration.
- auth_container_retry_test mutates the package-level sharedAuthContainer state directly, which can create hidden coupling with other tests in this package; it would be safer to provide a reset helper or to structure getAuthContainer so its shared state can be injected/isolated for testing.
## Individual Comments
### Comment 1
<location path="pkg/whatsmeow/service/auth_container_retry_test.go" line_range="23-31" />
<code_context>
+
+ // Falha: exPath aponta pra diretório inexistente — o SQLite não cria
+ // diretórios intermediários, então o Upgrade falha na primeira conexão.
+ bad := whatsmeowService{
+ config: &config.Config{},
+ exPath: filepath.Join(t.TempDir(), "does-not-exist"),
+ }
+ if _, err := bad.getAuthContainer(); err == nil {
+ t.Fatal("esperava erro com diretório inexistente, veio nil")
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that a failed getAuthContainer call does not populate the sharedAuthContainer singleton
Since this test is guarding the memoization bug, consider also asserting that `sharedAuthContainer` remains `nil` after the failed `getAuthContainer` call, e.g. `if sharedAuthContainer != nil { t.Fatalf("failed container should not be memoized") }`. That way the test explicitly verifies that only successful calls are cached.
```suggestion
// Falha: exPath aponta pra diretório inexistente — o SQLite não cria
// diretórios intermediários, então o Upgrade falha na primeira conexão.
bad := whatsmeowService{
config: &config.Config{},
exPath: filepath.Join(t.TempDir(), "does-not-exist"),
}
if _, err := bad.getAuthContainer(); err == nil {
t.Fatal("esperava erro com diretório inexistente, veio nil")
}
// Falha não deve ser memorizada no singleton
sharedAuthContainerMu.Lock()
if sharedAuthContainer != nil {
sharedAuthContainerMu.Unlock()
t.Fatalf("failed container should not be memoized (sharedAuthContainer = %#v)", sharedAuthContainer)
}
sharedAuthContainerMu.Unlock()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Good catch in theory, but in practice NewWhatsmeowService is called exactly once in main.go, so the singleton is always built from the single process-wide config — same behavior as the original #102 patch. Making the initialization config-independent feels like a broader refactor than this leak fix should carry; happy to open a follow-up issue if the maintainers want it. |
The test already resets the singleton at the start (under the mutex), and with the inline suggestion applied it now asserts failures aren't memoized. I'd prefer to keep the fix minimal, but happy to extract a reset helper in a follow-up if preferred. |
|
I tested these commits on top of an Evolution Go 0.7.2-based production - "github.com/EvolutionAPI/evolution-go/pkg/config"
+ "github.com/evolution-foundation/evolution-go/pkg/config"Without that change, The shared/capped auth container patch itself applied cleanly to our 0.7.2 |
Merges the Athene fork, which sits 11 commits ahead of the same upstream base and had already solved — in production — the three problems this fork was working through, plus a batch of upstream PRs we had not picked up. Only pkg/whatsmeow/service/whatsmeow.go conflicted (6 hunks); the other 63 files merged cleanly. Every conflict was resolved in favour of the incoming version, because each one was a place where both forks fixed the same bug and theirs is the one we chose to keep: - Connection leak: their getAuthContainer/sharedAuthContainer replaces our storeContainer/storeContainerHolder. Same fix, but theirs is a backport of upstream PR evolution-foundation#117, so it will converge with upstream instead of conflicting when that PR merges. It also caps SQLite at MaxOpenConns(1), which ours did not. - Reconnection: their runtime supervisor replaces our EnableAutoReconnect=true. Ours stopped the automatic disconnect loop but left ReconnectClient reachable from the API endpoint and the send retry, so those paths could still spawn a second client. Their runtime token refuses to start a second runtime for an instance from any path, waits for the previous goroutine to exit, deduplicates concurrent reconnects, backs off exponentially with jitter, and probes to confirm the reconnect actually connected. - Shared maps: ClientMapsMu closes the "fatal error: concurrent map writes" hole, which was still open on our side and kills the whole process rather than panicking a single request. Also arriving with the merge: upstream PRs evolution-foundation#149, evolution-foundation#135, evolution-foundation#34, evolution-foundation#100, evolution-foundation#143, evolution-foundation#137, evolution-foundation#120, evolution-foundation#130, evolution-foundation#151 and evolution-foundation#122; multi-webhook fan-out; POST /send/product; POST /user/savecontact; GET /server/stats and the dashboard; the whatsmeow update to 20260721; and removal of the 61 MB compiled binary that was tracked in git. Our GHCR workflow did not conflict and is preserved. store_container_test.go was adapted to the incoming implementation: it now exercises getAuthContainer, asserts against their MaxOpenConns of 20, and resets sharedAuthContainer between tests, since that container is a package-level global and would otherwise carry between tests and invalidate the pg_stat_activity measurements. It is kept alongside their auth_container_retry_test.go because it measures something theirs does not: the connection count the server actually sees. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
We hit this exact leak in production on v0.7.2: the engine opened a new uncapped connection per operation until Postgres returned We applied this patch on top of v0.7.2 and have been running it in production since 2026-08-18: connection count is now stable at 4–7 with zero leaked idle connections. Confirms the fix works. Would be great to see this merged and released. 🤖 Generated with Claude Code |
|
Triage notes from a contributor doing a pass over the open PRs on this leak (no write access here, so this is input for whoever has the merge button, not a decision). Of the 10 open PRs addressing it, this is the one worth converging on — it is the only one that ships a test, and @dev-guidolin's production report above is the only field validation any of them has. Two things below, plus one correction to the record. 1. There is no import blocker on this PROur tracker claimed this PR was blocked because the new test imports This PR targets So the import matches. Measured on @Clalber's comment is correct in its own context and should not be read as contradicting this: they tested "on top of an Evolution Go 0.7.2-based production branch", and a 0.7.2-based branch does use the 2. The new test fails on every run on Windows
One line, right after the t.Cleanup(func() { _ = c1.Close() })Measured 3/3 green with it (0.337s / 0.293s / 0.621s). No need to reset the package-level 3. Postgres branch: reuse the pool instead of opening a second oneIn the postgres branch, consider swapping the new container := sqlstore.NewWithDB(w.authDB, "postgres", dbLog)As it stands, this PR opens a second pool against the same DSN, which puts the ceiling at 25 + 20 = 45 connections against It also keeps this fix correct when The sqlite branch keeping its own Not in scope for this PR
One nit on the descriptionThe PR body says the pool is unbounded ( |
|
Another production confirmation on Setup: 5 instances, single Postgres ( What we observed:
The control case is what convinced me this PR is targeting the right place. After a container restart plus one manual So as a workaround for anyone hitting this before a fix lands: never let an unpaired instance sit in a reconnect loop. Two notes on the fix itself, from this angle:
Happy to test a build of this branch against the same workload if that is useful. |
6 participants
Description
StartClient()calledsqlstore.New()on every connect and every reconnect. Each call opens a fresh*sql.DBwith unbounded defaults(
MaxOpenConns=0,ConnMaxLifetime=0) and the resulting*sqlstore.Containeris never closed. Instances stuck in QR-expiry/reconnect loops leak oneconnection pool per attempt until Postgres hits
FATAL: sorry, too many clients already, after which no instance can connect.This PR supersedes #102 by @Ay0rus, which targeted
main— the same fix is rebased ontodevelop(applied cleanly, original authorship preserved in thefirst commit), plus a hardening of the initialization path:
sqlstore.Containerwith a capped pool (Postgres: 20 max open / 5 idle / 5min lifetime / 2min idle;SQLite: single connection), reused across all
StartClient()calls — whatsmeow's container is designed to hold multiple devices.sync.Oncewith a mutex that memoizes only success. Withsync.Once, a Postgres hiccup during the firstStartClientcached the error forever, leaving the service unable to connect until a process restart. Now failures retry on the next call.Reproduced locally before the fix: 15 calls to
/instance/reconnecttookevogo_authfrom 1 to 16 connections, growing unbounded afterwards (reached 32+within minutes via the internal QR-expiry restart loop). With the fix, the same procedure stays at 1-2 connections.
Related Issue
Closes #106, closes #109, closes #112
Supersedes #102 (same fix, retargeted at develop as requested, original commit authorship preserved)
Type of Change
Testing
Includes a self-contained regression test (
auth_container_retry_test.go) using the SQLite path — no external services required, verified in a cleangolang:1.25container. It asserts that (1) a failed container creation is not cached, (2) the next call retries and succeeds, and (3) subsequent callsreturn the same shared container instance.
Manual verification against a local Postgres:
evogo_auth/instance/reconnectScreenshots (if applicable)
N/A
Checklist
Additional Notes
GetDevice/NewDeviceper instance work as before.Summary by Sourcery
Reuse a single shared, capped SQL auth container for all WhatsApp client starts to eliminate database connection leaks and improve connection handling reliability.
Bug Fixes:
Enhancements:
Tests: