Eflowchat pg fix - #142
Eflowchat pg fix#142soyezeok wants to merge 3 commits into
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).
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Reviewer's GuideIntroduce a shared, singleton sqlstore.Container for WhatsApp auth storage with connection pooling limits, and add a test to validate retry-on-failure and reuse semantics. Sequence diagram for shared auth sqlstore.Container lifecycle and reusesequenceDiagram
participant Client1 as StartClient_1
participant Service as whatsmeowService
participant DB as sql_DB
participant Store as sqlstore_Container
Client1->>Service: StartClient(cd)
Service->>Service: getAuthContainer()
alt sharedAuthContainer already set
Service-->>Service: return sharedAuthContainer
else first successful creation
Service->>DB: sql.Open(dialect, address)
DB-->>Service: *sql.DB
alt dialect == postgres
Service->>DB: SetMaxOpenConns(20)
Service->>DB: SetMaxIdleConns(5)
Service->>DB: SetConnMaxLifetime(5m)
Service->>DB: SetConnMaxIdleTime(2m)
else dialect == sqlite
Service->>DB: SetMaxOpenConns(1)
end
Service->>Store: sqlstore.NewWithDB(db, dialect, dbLog)
Service->>Store: Upgrade(context.Background())
Store-->>Service: ok
Service-->>Service: sharedAuthContainer = container
end
Service-->>Client1: client started with container
%% Retry-on-failure behavior
Client1->>Service: StartClient(cd)
Service->>Service: getAuthContainer()
Service->>DB: sql.Open(...)
DB-->>Service: error
Service-->>Client1: log "Failed to get auth container" and return
Client1->>Service: StartClient(cd) retry
Service->>Service: getAuthContainer() (retries sql.Open)
%% Reuse from another client
participant Client2 as StartClient_2
Client2->>Service: StartClient(cd2)
Service->>Service: getAuthContainer()
Service-->>Client2: returns sharedAuthContainer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider threading a context from callers into getAuthContainer instead of using context.Background() so the Upgrade call can respect timeouts/cancellation during startup.
- The test directly mutates the sharedAuthContainer singleton; if more tests are added that also rely on the singleton state, it would be safer to centralize a reset helper to avoid inter-test coupling or ordering issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider threading a context from callers into getAuthContainer instead of using context.Background() so the Upgrade call can respect timeouts/cancellation during startup.
- The test directly mutates the sharedAuthContainer singleton; if more tests are added that also rely on the singleton state, it would be safer to centralize a reset helper to avoid inter-test coupling or ordering issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for this — the patch itself is sound. Closing it in favour of #117, which is the same change (the two diffs differ only in hunk offsets) and is where the review is happening. The reason to close rather than merge is the base branch: If you would like to keep your name on the fix, feel free to open it against |
1 similar comment
|
Thanks for this — the patch itself is sound. Closing it in favour of #117, which is the same change (the two diffs differ only in hunk offsets) and is where the review is happening. The reason to close rather than merge is the base branch: If you would like to keep your name on the fix, feel free to open it against |
Description
Related Issue
Closes #(issue_number)
Type of Change
Testing
Screenshots (if applicable)
Checklist
Additional Notes
Summary by Sourcery
Share and reuse a singleton auth database container in the WhatsApp service to avoid leaking connections and validate its retry and reuse semantics with tests.
Bug Fixes:
Enhancements:
Tests: