Repository navigation
fix(user): bound avatar queries and preserve profile response contracts - #120
cesar-carlos wants to merge 4 commits into
Conversation
CreateJID/ParseJID prefixes phone numbers with "+", which breaks the raw GetProfilePictureInfo IQ (Target=jid) and surfaces as "info query timed out" after ~75s on POST /user/avatar. Strip the prefix via CanonicalJID and ToNonAD before the request, matching the existing fix for reactions/typing/receipts. Closes evolution-foundation#76 Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCanonicalizes user JIDs before requesting profile picture info to avoid IQ timeouts and improves error logging for avatar retrieval failures. Sequence diagram for canonical JID in avatar lookupsequenceDiagram
actor User
participant ApiServer
participant userService
participant utils
participant whatsappClient
User->>ApiServer: POST /user/avatar
ApiServer->>userService: GetAvatar(data, instance)
userService->>userService: ParseJID(data.Phone)
userService->>utils: CanonicalJID(jid)
utils-->>userService: canonicalJID
userService->>userService: ToNonAD(canonicalJID)
userService->>whatsappClient: GetProfilePictureInfo(jid, preview)
alt IQ succeeds
whatsappClient-->>userService: pic
userService-->>ApiServer: avatar URL
ApiServer-->>User: 200 OK
else IQ times out
whatsappClient-->>userService: ErrIQTimedOut
userService->>userService: fmt.Errorf("get profile picture for jid: %w", ErrIQTimedOut)
userService-->>ApiServer: error
ApiServer-->>User: 500 info query timed out
end
File-Level Changes
Assessment against linked issues
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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/user/service/user_service.go" line_range="357-358" />
<code_context>
if err != nil {
u.loggerWrapper.GetLogger(instance.Id).LogError("[%s] GetProfilePictureInfo failed: %v", instance.Id, err)
- return nil, err
+ return nil, fmt.Errorf("get profile picture for %s: %w", jid, err)
}
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Revisit including the JID in the wrapped error for potential PII exposure.
Embedding the JID in the returned error introduces user-identifying data that may surface in logs or external error messages. Consider omitting or masking the JID in the error text, and if you need it for debugging, log it separately at an appropriate level while keeping the returned error generic.
```suggestion
u.loggerWrapper.GetLogger(instance.Id).LogError("[%s] GetProfilePictureInfo failed: %v", instance.Id, err)
return nil, fmt.Errorf("get profile picture: %w", err)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| u.loggerWrapper.GetLogger(instance.Id).LogError("[%s] GetProfilePictureInfo failed: %v", instance.Id, err) | ||
| return nil, err | ||
| return nil, fmt.Errorf("get profile picture for %s: %w", jid, err) |
There was a problem hiding this comment.
🚨 suggestion (security): Revisit including the JID in the wrapped error for potential PII exposure.
Embedding the JID in the returned error introduces user-identifying data that may surface in logs or external error messages. Consider omitting or masking the JID in the error text, and if you need it for debugging, log it separately at an appropriate level while keeping the returned error generic.
| u.loggerWrapper.GetLogger(instance.Id).LogError("[%s] GetProfilePictureInfo failed: %v", instance.Id, err) | |
| return nil, err | |
| return nil, fmt.Errorf("get profile picture for %s: %w", jid, err) | |
| u.loggerWrapper.GetLogger(instance.Id).LogError("[%s] GetProfilePictureInfo failed: %v", instance.Id, err) | |
| return nil, fmt.Errorf("get profile picture: %w", err) |
Canonical JID fix for /user/avatar timeout (PR evolution-foundation#120).
|
Closing from the author side: these changes are already merged into our fork The commits remain available on the PR branch if maintainers still want to review/merge into |
|
Reopened — keeping this PR available for upstream review/merge. |
Cap GetProfilePictureInfo at 8s using the request context so clients with ~12s read timeouts get HTTP 504 instead of a hung connection. Map WhatsApp rate-overlimit to HTTP 429. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Updated after production avatar failure report (Chatwoot account enrichment):
Please re-review the new commit on this branch. |
Prefer PN JID when the LID store has a mapping before the profile picture IQ. Replace fixed Sleep(2s) after StartInstance with a context-aware ready poll so avatar timeouts are not wasted on wait. Map context.Canceled to HTTP 504 like other request aborts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up improvements from review:
|
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>
Description
POST /user/avatarcould wait for WhatsApp's default IQ timeout, and the previous eight-second timeout started only after session preparation and LID lookup. This change shares an eight-second budget across startup database work, authenticated client readiness (up to two seconds), LID resolution, and the profile-picture IQ, while propagating HTTP cancellation.Phone and LID targets are canonicalized before the raw IQ. The authenticated instance owns both the client and the LID store; an unavailable mapping keeps the original LID, while timeout/cancellation stops the query. Preview/full selection is preserved,
ExistingIDstays empty, and no application-level IQ retry or image download is added.The review also fixes a panic when binding JSON
null, validates input before starting a session, preserves timeout causes, and rejects nil pictures, empty URLs, or success arriving after cancellation. Public error messages avoid exposing internal storage/session details. Invalid input, authentication failures, rate limits, and timeouts return 400, 401, 429, and 504 respectively; unavailable/hidden photos and other failures retain 500.The success response keeps
messageand the existing lowercaseurl,id,type,direct_path, andhashfields (hashis base64 or null). Typed response DTOs make Swagger match that JSON contract. The avatar wiki, API-key header, error documentation, and generated Swagger were updated; the Makefile uses the pinned Swagger generator with dependency/internal parsing and propagates generation failures.Client lookup/readiness uses a synchronized service-owned index with identity-aware cleanup, so a departing worker cannot remove its replacement. The launched session remains service-owned after an HTTP query ends. This protects query-side lookup; existing legacy client-map APIs outside that path are unchanged.
Related Issue
Closes #76
Related: #121 adds
PictureURLto/user/info; this PR remains focused on avatar queries and does not add that feature.Type of Change
Testing
Validated in Docker with Go 1.25.0, CGO enabled, and the project's pinned dependencies, both on this PR branch and after integration into local main:
make swagger(swag v1.16.3)go build ./...go vet ./...go test -timeout 5m ./...go test -race -timeout 5m ./...git diff --checkRegression tests cover shared deadlines/cancellation, startup SQL cancellation, canonical PN/LID/group targets, missing/failing LID mappings, preview/full parameters, nil/empty pictures, late success, error causes, concurrent client lookup/cleanup, authentication and instance isolation through the real middleware, malformed/null requests, and exact success JSON including base64/null hashes.
Against the previous PR head (
346a07f), the new HTTP regression cases reproduced the JSON-null panic and disclosure of a fake database credential in an internal-error response. Both pass with these corrections. Tests use fakes and SQL mocks; no live WhatsApp calls, real credentials, licenses, or production databases were used. No new live-response latency claim is made.Checklist