Open
Conversation
📝 WalkthroughWalkthroughAdds a new 'reloading' session status, updates status checks to use unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReloadService
participant SessionStore
participant BaileysClient
Client->>ReloadService: request reload(phone)
ReloadService->>SessionStore: getStatus(phone)
SessionStore-->>ReloadService: currentStatus
alt currentStatus == "reloading"
ReloadService->>ReloadService: log "already reloading" and return
else
ReloadService->>SessionStore: setStatus(phone, "reloading")
ReloadService->>BaileysClient: if active (online/connecting/standby) -> disconnect()
ReloadService->>BaileysClient: initialize/reconnect client
BaileysClient-->>ReloadService: ready/connected
ReloadService->>SessionStore: setStatus(phone, "online" | appropriate)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/reload_baileys.ts (1)
23-58:⚠️ Potential issue | 🔴 CriticalIf an error is thrown after setting
'reloading', the session is permanently stuck.There is no
try/finally(or equivalent) to reset the status away from'reloading'on failure. Any exception thrown between line 36 and line 50 (e.g.,getClientfails,disconnectfails,super.runfails) will leave the session stuck in'reloading'forever, causing the guard on line 32 to block all future reload attempts for that phone.🐛 Proposed fix: wrap the reload body in try/finally
await sessionStore.setStatus(phone, 'reloading') + try { const currentClient = await this.getClient({ phone, listener: this.listener, getConfig: this.getConfig, onNewLogin: this.onNewLogin, }) const currentStatus = await sessionStore.getStatus(phone) if (['online', 'standby', 'connecting'].includes(currentStatus)) { logger.warn('Reload disconnect session %s!', phone) await currentClient?.disconnect() } await super.run(phone) await sessionStore.setStatus(phone, 'online') // to clear standby await sessionStore.setStatus(phone, 'disconnected') await this.getClient({ phone, listener: this.listener, getConfig: this.getConfig, onNewLogin: this.onNewLogin, }) logger.info('Reloaded session %s!', phone) + } catch (error) { + logger.error(error, 'Error reloading session %s', phone) + await sessionStore.setStatus(phone, 'disconnected') + throw error + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reload_baileys.ts` around lines 23 - 58, The reload flow in run(...) sets sessionStore.setStatus(phone, 'reloading') but never guarantees it will be cleared on error; wrap the critical reload sequence (calls to getClient, sessionStore.getStatus checks, currentClient.disconnect, super.run and the subsequent client creation) in a try/finally so that sessionStore.setStatus(phone, ...) is always reset (either back to the prior status or to a safe state like 'disconnected' or 'standby') in the finally block; update references in the finally to use the same sessionStore and phone variables and ensure any awaited cleanup (e.g., clearing 'reloading' or restoring previous status) occurs there so an exception does not permanently leave the session in 'reloading'.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/reload_baileys.ts`:
- Around line 32-36: The TOCTOU occurs because run(phone) calls
sessionStore.isStatusReloading and then sessionStore.setStatus in separate async
Redis trips (SessionStoreRedis), allowing races; change the logic to perform an
atomic check-and-set instead of two calls — either add/use a new atomic
sessionStore method (e.g., setStatusIfNotReloading or acquireReloadLock(phone))
that uses Redis SET key value NX (or a Lua script) to set the "reloading" marker
only if absent (optionally with TTL) and return success/failure, and replace the
isStatusReloading+setStatus pair in run(phone) with that single atomic call;
alternatively, if single-process, implement a per-phone in-memory lock/semaphore
and use that in run(phone) to serialize concurrent reloads.
- Around line 49-50: Replace the fragile side-effect hack of calling
sessionStore.setStatus(phone, 'online') to clear the connect count: remove that
call and invoke the explicit API sessionStore.clearConnectCount(phone) (or the
appropriate clearConnectCount method on SessionStore) before or instead of
setting sessionStore.setStatus(phone, 'disconnected'); keep the final
sessionStore.setStatus(phone, 'disconnected') but use clearConnectCount(phone)
to express intent and avoid triggering online-status side effects.
- Around line 23-58: The reload flow in run(...) sets
sessionStore.setStatus(phone, 'reloading') but never guarantees it will be
cleared on error; wrap the critical reload sequence (calls to getClient,
sessionStore.getStatus checks, currentClient.disconnect, super.run and the
subsequent client creation) in a try/finally so that
sessionStore.setStatus(phone, ...) is always reset (either back to the prior
status or to a safe state like 'disconnected' or 'standby') in the finally
block; update references in the finally to use the same sessionStore and phone
variables and ensure any awaited cleanup (e.g., clearing 'reloading' or
restoring previous status) occurs there so an exception does not permanently
leave the session in 'reloading'.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reload_baileys.ts`: - Around line 49-50: Replace the fragile side-effect hack of calling sessionStore.setStatus(phone, 'online') to clear the connect count: remove that call and invoke the explicit API sessionStore.clearConnectCount(phone) (or the appropriate clearConnectCount method on SessionStore) before or instead of setting sessionStore.setStatus(phone, 'disconnected'); keep the final sessionStore.setStatus(phone, 'disconnected') but use clearConnectCount(phone) to express intent and avoid triggering online-status side effects.src/services/reload_baileys.ts (1)
49-50: Setting status to'online'only to immediately override it with'disconnected'is a fragile side-effect hack.This relies on the implicit behavior that
setStatus('online')clears the connect count in the Redis store. A dedicatedclearConnectCountcall (already available onSessionStore) would express the intent directly and wouldn't risk triggering other side effects tied to the'online'status (e.g., listeners, logging).♻️ Suggested change
- await sessionStore.setStatus(phone, 'online') // to clear standby - await sessionStore.setStatus(phone, 'disconnected') + await sessionStore.clearConnectCount(phone) + await sessionStore.setStatus(phone, 'disconnected')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reload_baileys.ts` around lines 49 - 50, Replace the fragile side-effect hack of calling sessionStore.setStatus(phone, 'online') to clear the connect count: remove that call and invoke the explicit API sessionStore.clearConnectCount(phone) (or the appropriate clearConnectCount method on SessionStore) before or instead of setting sessionStore.setStatus(phone, 'disconnected'); keep the final sessionStore.setStatus(phone, 'disconnected') but use clearConnectCount(phone) to express intent and avoid triggering online-status side effects.
src/services/reload_baileys.ts
Outdated
Comment on lines
32
to
36
There was a problem hiding this comment.
TOCTOU race between the reloading check and the status set.
With the Redis-backed SessionStoreRedis, the isStatusReloading (line 32) and setStatus (line 36) are two separate async Redis round-trips. Two concurrent run(phone) calls can both observe isStatusReloading === false before either sets the status, defeating the guard. Consider using an atomic Redis operation (e.g., SET key value NX or a Lua script) to make the check-and-set atomic, or use a per-phone in-memory lock/semaphore as a simpler alternative if single-process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/reload_baileys.ts` around lines 32 - 36, The TOCTOU occurs
because run(phone) calls sessionStore.isStatusReloading and then
sessionStore.setStatus in separate async Redis trips (SessionStoreRedis),
allowing races; change the logic to perform an atomic check-and-set instead of
two calls — either add/use a new atomic sessionStore method (e.g.,
setStatusIfNotReloading or acquireReloadLock(phone)) that uses Redis SET key
value NX (or a Lua script) to set the "reloading" marker only if absent
(optionally with TTL) and return success/failure, and replace the
isStatusReloading+setStatus pair in run(phone) with that single atomic call;
alternatively, if single-process, implement a per-phone in-memory lock/semaphore
and use that in run(phone) to serialize concurrent reloads.
clairton
force-pushed
the
fix/duplicate-reloading
branch
from
a85abb1 to
f858074
Compare
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/services/reload_baileys.ts`:
- Around line 30-37: The reload flow currently sets
sessionStore.setStatus(phone, 'reloading') then proceeds and may throw, leaving
the status stuck; update the logic in the reload function (the block that calls
config.getStore, sessionStore.getStatus, sessionStore.setStatus and then calls
getClient, disconnect, super.run) to wrap the remainder of the reload body in a
try/finally so that sessionStore.setStatus(phone, <previousStatus or null/''>)
is always called in the finally block to clear/reset the status even if
getClient, disconnect, super.run (or any other operation) throws; preserve
retrieval of the prior status from sessionStore.getStatus(phone) before setting
'reloading' and restore that value (or a cleared state) in the finally.
Comment on lines
+30
to
+37
| const store = await config.getStore(phone, config) | ||
| const { sessionStore } = store | ||
| const currentStatus = await sessionStore.getStatus(phone) | ||
| if (currentStatus == 'reloading') { | ||
| logger.info('Already reloading session %s!', phone) | ||
| return | ||
| } | ||
| await sessionStore.setStatus(phone, 'reloading') |
There was a problem hiding this comment.
Status stuck at 'reloading' if run() throws after line 37.
If any operation after setStatus(phone, 'reloading') throws (e.g., getClient, disconnect, super.run), the status is never reset and all future reloads for this phone will be permanently blocked by the early-return guard on line 33. Wrap the reload body in a try/finally to ensure the status is cleared on failure.
Proposed fix
await sessionStore.setStatus(phone, 'reloading')
+ try {
const currentClient = await this.getClient({
phone,
listener: this.listener,
getConfig: this.getConfig,
onNewLogin: this.onNewLogin,
})
if (['online', 'standby', 'connecting'].includes(currentStatus)) {
logger.warn('Reload disconnect session %s!', phone)
await currentClient?.disconnect()
}
await super.run(phone)
await sessionStore.setStatus(phone, 'online') // to clear standby
await sessionStore.setStatus(phone, 'disconnected')
await this.getClient({
phone,
listener: this.listener,
getConfig: this.getConfig,
onNewLogin: this.onNewLogin,
})
logger.info('Reloaded session %s!', phone)
+ } catch (e) {
+ logger.error(e, 'Error reloading session %s', phone)
+ await sessionStore.setStatus(phone, 'offline')
+ throw e
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const store = await config.getStore(phone, config) | |
| const { sessionStore } = store | |
| const currentStatus = await sessionStore.getStatus(phone) | |
| if (currentStatus == 'reloading') { | |
| logger.info('Already reloading session %s!', phone) | |
| return | |
| } | |
| await sessionStore.setStatus(phone, 'reloading') | |
| const store = await config.getStore(phone, config) | |
| const { sessionStore } = store | |
| const currentStatus = await sessionStore.getStatus(phone) | |
| if (currentStatus == 'reloading') { | |
| logger.info('Already reloading session %s!', phone) | |
| return | |
| } | |
| await sessionStore.setStatus(phone, 'reloading') | |
| try { | |
| const currentClient = await this.getClient({ | |
| phone, | |
| listener: this.listener, | |
| getConfig: this.getConfig, | |
| onNewLogin: this.onNewLogin, | |
| }) | |
| if (['online', 'standby', 'connecting'].includes(currentStatus)) { | |
| logger.warn('Reload disconnect session %s!', phone) | |
| await currentClient?.disconnect() | |
| } | |
| await super.run(phone) | |
| await sessionStore.setStatus(phone, 'online') // to clear standby | |
| await sessionStore.setStatus(phone, 'disconnected') | |
| await this.getClient({ | |
| phone, | |
| listener: this.listener, | |
| getConfig: this.getConfig, | |
| onNewLogin: this.onNewLogin, | |
| }) | |
| logger.info('Reloaded session %s!', phone) | |
| } catch (e) { | |
| logger.error(e, 'Error reloading session %s', phone) | |
| await sessionStore.setStatus(phone, 'offline') | |
| throw e | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/reload_baileys.ts` around lines 30 - 37, The reload flow
currently sets sessionStore.setStatus(phone, 'reloading') then proceeds and may
throw, leaving the status stuck; update the logic in the reload function (the
block that calls config.getStore, sessionStore.getStatus, sessionStore.setStatus
and then calls getClient, disconnect, super.run) to wrap the remainder of the
reload body in a try/finally so that sessionStore.setStatus(phone,
<previousStatus or null/''>) is always called in the finally block to
clear/reset the status even if getClient, disconnect, super.run (or any other
operation) throws; preserve retrieval of the prior status from
sessionStore.getStatus(phone) before setting 'reloading' and restore that value
(or a cleared state) in the finally.
clairton
force-pushed
the
fix/duplicate-reloading
branch
from
f858074 to
0af8f80
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/services/reload_baileys.ts`:
- Around line 37-57: The reload sequence can leave the session stuck at
'reloading' if any of sessionStore.setStatus, this.getClient,
currentClient.disconnect, super.run, or the subsequent getClient calls throw;
wrap the core reload logic in a try/finally so you always clear/reset status:
set sessionStore.setStatus(phone,'reloading') before the try, perform
getClient/disconnect/super.run/second getClient inside the try, and in the
finally ensure you call sessionStore.setStatus(phone,'online') (or original
state) and then sessionStore.setStatus(phone,'disconnected') as necessary so the
existing early-return that checks status will not permanently block future
reloads. Ensure references to currentClient?.disconnect and this.getClient
remain unchanged when moving into the try block.
- Around line 32-37: The current guard in run(phone) uses two separate async
calls sessionStore.getStatus(phone) and sessionStore.setStatus(phone,
'reloading'), which allows a TOCTOU race when run(phone) is invoked
concurrently; replace this with an atomic check-and-set using the sessionStore's
Redis backend (e.g., use Redis SET key value NX with an expiration or a small
Lua script that performs GET and SET atomically) so only the first caller
succeeds in setting 'reloading' and others see that and exit; modify run(phone)
to call a new atomic method on sessionStore (e.g., trySetStatusIfNotExists /
setStatusIfCurrent) or add an atomic Lua helper and use it instead of
getStatus+setStatus.
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.
Summary by CodeRabbit
Bug Fixes
Chores