fix: close leaked sqlstore container on client restart/reconnect - #200
Open
alexmagnoreis wants to merge 1 commit into
Open
alexmagnoreis wants to merge 1 commit into
alexmagnoreis wants to merge 1 commit into
Conversation
StartClient() created a brand new sqlstore.Container (and therefore a new *sql.DB connection pool) on every call, but the previous container was never closed. Since StartClient is re-invoked on every websocket reconnect (events.Disconnected, LoggedOut, ReconnectClient), each reconnect leaked a full connection pool that lived for the process lifetime, eventually exhausting Postgres max_connections after enough reconnects accumulate over days. Track the active container per instance and close the previous one before opening a new one in StartClient, and also close it when the instance is fully torn down in ClearInstanceCache.
Reviewer's GuidePrevents database connection-pool leaks across WhatsApp reconnects and instance teardown by tracking each instance’s sqlstore container, closing replaced containers before reopening, and cleaning up the active container during full cache clearance. Sequence diagram for closing the SQL container during client reconnectsequenceDiagram
participant Reconnect as ReconnectClient
participant Service as whatsmeowService
participant Store as sqlstore.Container
participant DB as SQL connection pool
Reconnect->>Service: StartClient(cd)
Service->>Store: Close()
Store->>DB: Close connections
Service->>Store: Open new container
Service->>Service: containerPointer[instanceId] = container
Sequence diagram for instance teardown cleanupsequenceDiagram
participant Teardown as ClearInstanceCache
participant Service as whatsmeowService
participant Store as sqlstore.Container
participant DB as SQL connection pool
Teardown->>Service: ClearInstanceCache(instanceId, token)
Service->>Store: Close()
Store->>DB: Close connections
Service->>Service: delete containerPointer[instanceId]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the old container is still being used during a concurrent restart or reconnect, closing it can terminate database connections and cause failed stores or a service outage. Reverting stops future closures, but requests and operations that already failed during the interruption cannot be undone.
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
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.
Problem
StartClient()creates a brand newsqlstore.Container(and therefore a new*sql.DBconnection pool) on every call, but the previous container is never closed.StartClientis re-invoked on every WhatsApp websocket reconnect —events.Disconnected→ReconnectClient()→StartInstance()→StartClient(), and also onLoggedOut/restart paths.ReconnectClient()carefully tears down the websocket, event handler and in-memory maps, but never closes the underlying DB pool.In production this showed up as a slow, unbounded growth of idle Postgres connections from the
evolution-goprocess — eventually exhaustingmax_connectionsand causingFATAL: sorry, too many clients alreadyfor every other service sharing that Postgres instance. Restarting the process temporarily fixes it (all leaked pools go away), but it comes back as reconnects accumulate over days.Fix
*sqlstore.Containerper instance in a newcontainerPointermap onwhatsmeowService.StartClient, close the previous container for that instance (if any) before opening a new one.ClearInstanceCache, close and clear the container when an instance is fully torn down.Minimal, targeted change — no behavior change for the happy path, just closes pools that were previously leaked.
Testing
No Go toolchain was available in the environment used to prepare this patch, so this has not been compiled/run locally. The change is small and mechanical (store a pointer, call
.Close(), matching the existingContainer.Close()API), but please run it through CI/your usual test suite before merging.Summary by Sourcery
Prevent SQL connection pool leaks by closing instance containers during client replacement and teardown.
Bug Fixes:
Enhancements: