fix(reload): prevent duplicate/destructive reload during transitions#129
Open
ViperTecCorporation wants to merge 4 commits intoclairton:mainfrom
Open
fix(reload): prevent duplicate/destructive reload during transitions#129ViperTecCorporation wants to merge 4 commits intoclairton:mainfrom
ViperTecCorporation wants to merge 4 commits intoclairton:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds two environment-driven defaults, makes AMQP broker publish conditional, implements per-phone debouncing and in-flight tracking for Baileys reloads, and introduces an optional forced logout flag propagated through logout interfaces, AMQP, job, and controller layers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Debounce as Debounce/Tracking
participant SessionStore
participant BaileysSvc as Baileys Service
participant CurrentClient
Client->>Debounce: Reload request (phone)
Debounce->>Debounce: Check inFlight / lastRunAt (debounce)
alt Recently in-flight or within debounce window
Debounce-->>Client: Skip reload
else Proceed
Debounce->>Debounce: Mark in-flight
Debounce->>SessionStore: Query isConnecting / isRestartRequired
alt Transitional state
Debounce-->>Client: Skip (transitional)
Debounce->>Debounce: Clear in-flight
else Safe to proceed
Debounce->>SessionStore: Query isOnline / isStandBy
SessionStore-->>Debounce: Status
Debounce->>CurrentClient: Disconnect if needed
BaileysSvc->>BaileysSvc: Perform reload
Debounce->>Debounce: Clear in-flight (finally)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/services/reload_baileys.ts`:
- Around line 35-71: Indent the entire body of the try block one level deeper to
improve readability: update the block that starts with try { ... } in the
ReloadBaileys.run flow (including calls to this.getConfig, super.run,
this.getClient, await config.getStore, usage of
sessionStore.isStatusConnecting/isStatusRestartRequired/isStatusOnline/isStatusStandBy,
currentClient.disconnect, the second this.getClient call, logger.info and the
finally ReloadBaileys.inFlightByPhone.delete(phone)) so all statements inside
try are consistently nested under try; ensure closing braces and the finally
remain aligned correctly with the try.
- Around line 10-11: The Map lastRunAtByPhone grows without bounds; add pruning
to avoid stale entries by implementing a cleanup policy: either cap size with an
LRU eviction or periodically remove entries older than a configurable TTL.
Concretely, add a method (e.g., pruneLastRunAtEntries) on the same class that
iterates lastRunAtByPhone and deletes keys with timestamp < Date.now() - TTL,
and invoke it regularly (setInterval) or call it opportunistically from the code
paths that update lastRunAtByPhone (where inFlightByPhone and lastRunAtByPhone
are used) so stale phone entries are removed; make TTL and interval configurable
constants and reference lastRunAtByPhone and inFlightByPhone in the
implementation.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/services/reload_baileys.ts`: - Around line 35-71: Indent the entire body of the try block one level deeper to improve readability: update the block that starts with try { ... } in the ReloadBaileys.run flow (including calls to this.getConfig, super.run, this.getClient, await config.getStore, usage of sessionStore.isStatusConnecting/isStatusRestartRequired/isStatusOnline/isStatusStandBy, currentClient.disconnect, the second this.getClient call, logger.info and the finally ReloadBaileys.inFlightByPhone.delete(phone)) so all statements inside try are consistently nested under try; ensure closing braces and the finally remain aligned correctly with the try. - Around line 10-11: The Map lastRunAtByPhone grows without bounds; add pruning to avoid stale entries by implementing a cleanup policy: either cap size with an LRU eviction or periodically remove entries older than a configurable TTL. Concretely, add a method (e.g., pruneLastRunAtEntries) on the same class that iterates lastRunAtByPhone and deletes keys with timestamp < Date.now() - TTL, and invoke it regularly (setInterval) or call it opportunistically from the code paths that update lastRunAtByPhone (where inFlightByPhone and lastRunAtByPhone are used) so stale phone entries are removed; make TTL and interval configurable constants and reference lastRunAtByPhone and inFlightByPhone in the implementation.src/services/reload_baileys.ts (2)
35-71: Indentation insidetryblock is inconsistent.Lines 36–69 (the body of the
try) are at the same indent level astryitself. This harms readability. Consider indenting the body one level deeper.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/services/reload_baileys.ts` around lines 35 - 71, Indent the entire body of the try block one level deeper to improve readability: update the block that starts with try { ... } in the ReloadBaileys.run flow (including calls to this.getConfig, super.run, this.getClient, await config.getStore, usage of sessionStore.isStatusConnecting/isStatusRestartRequired/isStatusOnline/isStatusStandBy, currentClient.disconnect, the second this.getClient call, logger.info and the finally ReloadBaileys.inFlightByPhone.delete(phone)) so all statements inside try are consistently nested under try; ensure closing braces and the finally remain aligned correctly with the try.
10-11:lastRunAtByPhoneentries are never pruned.The
Mapgrows by one entry per unique phone and is never cleaned up. For long-running instances handling a large number of phones this could accumulate stale entries. Not urgent given the small per-entry footprint, but worth a periodic cleanup or a boundedMap/LRU if the phone population is large.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/services/reload_baileys.ts` around lines 10 - 11, The Map lastRunAtByPhone grows without bounds; add pruning to avoid stale entries by implementing a cleanup policy: either cap size with an LRU eviction or periodically remove entries older than a configurable TTL. Concretely, add a method (e.g., pruneLastRunAtEntries) on the same class that iterates lastRunAtByPhone and deletes keys with timestamp < Date.now() - TTL, and invoke it regularly (setInterval) or call it opportunistically from the code paths that update lastRunAtByPhone (where inFlightByPhone and lastRunAtByPhone are used) so stale phone entries are removed; make TTL and interval configurable constants and reference lastRunAtByPhone and inFlightByPhone in the implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/controllers/registration_controller.ts`:
- Around line 42-44: The deregister endpoint currently calls
this.logout.run(phone, { force: true }) without error handling so any exception
becomes an unhandled rejection; wrap that call in a try/catch (mirroring the
register handler) inside the deregister function in registration_controller.ts,
catch errors from this.logout.run, log the error (use the controller's logger)
and return an appropriate HTTP error response (e.g., res.status(500).json({
error: 'Failed to deregister' }) or pass the error to next(err)) so failures in
socket disconnect/cleanup are handled gracefully.
In `@src/services/logout_baileys.ts`:
- Line 42: The log incorrectly always says "forced logout" regardless of why the
socket logout path ran; update the logger.warn call in logout_baileys.ts (the
code that uses logger.warn(error, 'Ignore error on forced logout for %s',
phone)) to conditionally choose the message based on the force flag (or
shouldTrySocketLogout) — e.g., log "Ignore error on forced logout for %s" when
force is true, otherwise log "Ignore error on socket/logout for online session
%s" (or similar) so the message accurately reflects the cause and include phone
and error as before.
- Around line 30-43: The logout flow currently calls this.getClient(...) when no
cached client exists, which may create a new connection just to logout; change
the logic so you only attempt a socket logout if a cached client exists (e.g.,
check clients.has(phone) or clients.get(phone) first) and skip calling
this.getClient(...) when there's no cached client; still run the session cleanup
afterwards (sessionStore methods) and keep using client?.logout() when a cached
client is found; update the shouldTrySocketLogout branch to guard the
this.getClient(...) call accordingly.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/services/logout_baileys.ts`: - Line 42: The log incorrectly always says "forced logout" regardless of why the socket logout path ran; update the logger.warn call in logout_baileys.ts (the code that uses logger.warn(error, 'Ignore error on forced logout for %s', phone)) to conditionally choose the message based on the force flag (or shouldTrySocketLogout) — e.g., log "Ignore error on forced logout for %s" when force is true, otherwise log "Ignore error on socket/logout for online session %s" (or similar) so the message accurately reflects the cause and include phone and error as before. - Around line 30-43: The logout flow currently calls this.getClient(...) when no cached client exists, which may create a new connection just to logout; change the logic so you only attempt a socket logout if a cached client exists (e.g., check clients.has(phone) or clients.get(phone) first) and skip calling this.getClient(...) when there's no cached client; still run the session cleanup afterwards (sessionStore methods) and keep using client?.logout() when a cached client is found; update the shouldTrySocketLogout branch to guard the this.getClient(...) call accordingly.src/services/logout_baileys.ts (2)
42-42: Log message says "forced logout" but this path also runs for non-forced online sessions.When
shouldTrySocketLogoutis true because the session is online (not becauseforceis true), the warning still reads "forced logout." Consider adjusting the message for accuracy.Proposed fix
- logger.warn(error, 'Ignore error on forced logout for %s', phone) + logger.warn(error, 'Ignore error on logout for %s (force=%s)', phone, force)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/logout_baileys.ts` at line 42, The log incorrectly always says "forced logout" regardless of why the socket logout path ran; update the logger.warn call in logout_baileys.ts (the code that uses logger.warn(error, 'Ignore error on forced logout for %s', phone)) to conditionally choose the message based on the force flag (or shouldTrySocketLogout) — e.g., log "Ignore error on forced logout for %s" when force is true, otherwise log "Ignore error on socket/logout for online session %s" (or similar) so the message accurately reflects the cause and include phone and error as before.
30-43: Skip socket logout attempt when no cached client exists to avoid unnecessary connection.In lines 34-39, if a client doesn't exist in cache, calling
this.getClient()instantiates a new client and triggersconnect()(which establishes a WhatsApp WebSocket connection), only to immediately logout. This wastes resources for forced logouts where the connection doesn't already exist. Since the session cleanup at lines 45-51 always runs regardless, the socket logout is optional.Consider skipping the logout attempt when there's no existing cached client:
Proposed fix
if (shouldTrySocketLogout) { try { - const client = clients.get(phone) || (await this.getClient({ - phone, - listener: this.listener, - getConfig: this.getConfig, - onNewLogin: this.onNewLogin, - })) + const client = clients.get(phone) + if (!client) { + logger.debug('No existing client for %s, skipping socket logout', phone) + } await client?.logout() } catch (error) { logger.warn(error, 'Ignore error on forced logout for %s', phone) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/logout_baileys.ts` around lines 30 - 43, The logout flow currently calls this.getClient(...) when no cached client exists, which may create a new connection just to logout; change the logic so you only attempt a socket logout if a cached client exists (e.g., check clients.has(phone) or clients.get(phone) first) and skip calling this.getClient(...) when there's no cached client; still run the session cleanup afterwards (sessionStore methods) and keep using client?.logout() when a cached client is found; update the shouldTrySocketLogout branch to guard the this.getClient(...) call accordingly.
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.
fix(reload): prevenir reload duplicado/destrutivo durante transições de conexão
Problema
Solução
ReloadBaileys:
ReloadAmqp:
defaults:
Resultado
Summary by CodeRabbit
Configuration
New Features
Improvements