fix: close WebSocket after failed notification ack#2623
Open
hliebovkyrylo wants to merge 2 commits into
Open
Conversation
Contributor
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change modifies error handling in the ChangesMessage Acknowledgement Error Handling
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Socket/messages-recv.ts`:
- Line 1583: Remove the trailing semicolon from the ws.close() call to comply
with the project's no-semicolon coding guideline; locate the ws.close invocation
in src/Socket/messages-recv.ts (the ws.close line) and change it to the
semicolon-free form used across the codebase so formatting matches Prettier
rules.
- Around line 1581-1584: handleNotification currently always calls
sendMessageAck(node) in a finally and unconditionally calls ws.close() in the
catch, which causes unnecessary reconnections for transient transport errors;
change the catch to inspect the failure and only close the socket for
unrecoverable errors or auth/QR-related node types: in the
sendMessageAck(node).catch block (referencing sendMessageAck,
handleNotification, sendNode/sendRawMessage and ws), detect Boom('Connection
Closed')/transport-closed conditions or check ws.isOpen and if the error is a
transport-closed/transient error (or ackErr indicates connection already closed)
just log and return, otherwise for unrecoverable errors or for node.nodeType
values that require re-auth (e.g., auth/qr-related types such as
link_code_companion_reg/account_sync) call ws.close(); ensure you use
Boom.isBoom or error.output.statusCode (or error.message) to distinguish error
kinds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3439322-4773-40e3-a25a-6bdfdd24cac9
📒 Files selected for processing (1)
src/Socket/messages-recv.ts
Comment on lines
+1581
to
+1584
| await sendMessageAck(node).catch(ackErr => { | ||
| logger.error({ ackErr }, 'failed to ack notification') | ||
| ws.close(); | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Examine notification types and their handling to assess whether
# all notification ack failures warrant forced WebSocket closure.
# Find all notification type handling in processNotification
rg -n -A2 -B2 "case '.*':" src/Socket/messages-recv.ts | rg -A2 -B2 "nodeType|node.attrs.type"
# Look for connection state or auth state checks in notification handling
rg -n "authState.creds.registered|connection.*===|isOnline" src/Socket/messages-recv.tsRepository: WhiskeySockets/Baileys
Length of output: 452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact code around the reported lines
echo "=== Around lines 1560-1605 ==="
sed -n '1540,1620p' src/Socket/messages-recv.ts | nl -ba
# 2) Locate the ack call and its catch/log message
echo
echo "=== Locations of sendMessageAck(node) in messages-recv.ts ==="
rg -n "sendMessageAck\\(node\\)" src/Socket/messages-recv.ts
echo
echo "=== Locations of 'failed to ack notification' ==="
rg -n "failed to ack notification" src/Socket/messages-recv.ts
# 3) Identify surrounding notification processing function/context
echo
echo "=== Find nearby function names around the ack catch ==="
# Use the first matching line number from earlier snippet if possible:
ACK_LINE=$(rg -n "failed to ack notification" src/Socket/messages-recv.ts | head -n1 | cut -d: -f1 || true)
if [ -n "${ACK_LINE}" ]; then
START=$((ACK_LINE-120))
END=$((ACK_LINE+120))
sed -n "${START},${END}p" src/Socket/messages-recv.ts | nl -ba
fi
# 4) Enumerate what notification types/classes reach this ack path
echo
echo "=== Search for nodeType switch cases near the ack context ==="
# Just pull a broader region that includes the earlier notification switch (from initial results)
sed -n '980,1250p' src/Socket/messages-recv.ts | nl -ba
# 5) Quick scan for any auth/connection checks in the same local area
echo
echo "=== Any authState/connection checks between 1500-1700 ==="
sed -n '1500,1700p' src/Socket/messages-recv.ts | rg -n "authState\\.creds\\.registered|connection\\s*===|connection\\b|isOnline"Repository: WhiskeySockets/Baileys
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/messages-recv.ts"
echo "=== Line numbers for 'failed to ack notification' ==="
rg -n "failed to ack notification" "$file" || true
echo
echo "=== Inspect around the first 'failed to ack notification' occurrence ==="
line=$(rg -n "failed to ack notification" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${line}" ]; then
start=$((line-80))
end=$((line+80))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d\t%s\n", NR, $0}' "$file"
fi
echo
echo "=== All sendMessageAck(node) occurrences ==="
rg -n "sendMessageAck\\(node\\)" "$file" || true
echo
echo "=== Nearby context for each sendMessageAck(node) ==="
rg -n "sendMessageAck\\(node\\)" "$file" | while IFS=: read -r l _; do
start=$((l-40))
end=$((l+40))
echo "--- around line $l ---"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d\t%s\n", NR, $0}' "$file"
done
echo
echo "=== Look for any auth/connection gating near 1500-1700 ==="
awk 'NR>=1450 && NR<=1750 {print}' "$file" | rg -n "authState\\.creds\\.registered|connection\\s*===|connection\\b|isOnline" || true
echo
echo "=== Find notification handler switch/cases around 1000-1250 (includes earlier newsletter case) ==="
sed -n '950,1250p' "$file" | rg -n "switch \\(|case '.*'|nodeType|node\\.attrs\\.type|processNotification" || trueRepository: WhiskeySockets/Baileys
Length of output: 35180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/messages-recv.ts"
echo "=== Call sites of handleNotification(node) ==="
rg -n "handleNotification\\(node\\)|handleNotification\\(" "$file"
echo
echo "=== Inspect processNotification() switch body (nodeType cases) ==="
sed -n '70,320p' "$file" | nl -ba 2>/dev/null || sed -n '70,320p' "$file"
echo
echo "=== Find and inspect sendMessageAck definition ==="
rg -n "sendMessageAck\\b" "$file"
line=$(rg -n "sendMessageAck\\b" "$file" | head -n1 | cut -d: -f1 || true)
# Try to find the definition by searching keywords near it
defline=$(rg -n "const sendMessageAck|function sendMessageAck|async function sendMessageAck" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${defline}" ]; then
start=$((defline-40))
end=$((defline+140))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d\t%s\n", NR, $0}' "$file"
else
echo "sendMessageAck definition not found via simple pattern"
fi
echo
echo "=== Inspect around sendMessageAck usage with ackErr closing ws ==="
sed -n '1530,1605p' "$file"Repository: WhiskeySockets/Baileys
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/messages-recv.ts"
echo "=== All references to handleNotification in the file ==="
rg -n "handleNotification" "$file" || true
echo
echo "=== Inspect around each handleNotification reference ==="
rg -n "handleNotification" "$file" | while IFS=: read -r l _; do
start=$((l-60))
end=$((l+60))
echo "--- context around line $l ---"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d\t%s\n", NR, $0}' "$file"
echo
done
echo "=== Inspect sendMessageAck definition/implementation ==="
rg -n "sendMessageAck\\b" "$file" || true
defline=$(rg -n "(const|function|async function) sendMessageAck\\b|sendMessageAck\\s*=\\s*async|sendMessageAck\\s*=\\s*\\(" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${defline}" ]; then
start=$((defline-80))
end=$((defline+200))
echo "--- context around sendMessageAck near line $defline ---"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d\t%s\n", NR, $0}' "$file"
fiRepository: WhiskeySockets/Baileys
Length of output: 25616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/messages-recv.ts"
echo "=== buildAckStanza occurrences ==="
rg -n "buildAckStanza" "$file" || true
echo
echo "=== Inspect definition of buildAckStanza (and surrounding) ==="
defline=$(rg -n "(const|function) buildAckStanza\\b|async function buildAckStanza\\b|buildAckStanza\\s*=\\s*async" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${defline}" ]; then
start=$((defline-80)); end=$((defline+160))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {print NR "\t" $0}' "$file"
fi
echo
echo "=== sendNode occurrences ==="
rg -n "\\bsendNode\\b" "$file" || true
echo
echo "=== Inspect definition/implementation of sendNode (surrounding) ==="
senddef=$(rg -n "(const|function) sendNode\\b|async function sendNode\\b|sendNode\\s*=\\s*async" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${senddef}" ]; then
start=$((senddef-80)); end=$((senddef+200))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {print NR "\t" $0}' "$file"
fi
echo
echo "=== Where sendMessageAck is used with close() elsewhere? ==="
rg -n "ws\\.close\\(" "$file" || trueRepository: WhiskeySockets/Baileys
Length of output: 556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/messages-recv.ts"
echo "=== imports section (first ~140 lines) ==="
sed -n '1,140p' "$file"
echo
echo "=== Locate where sendNode is imported from ==="
rg -n "sendNode\\s*,|from '.*sendNode|import\\s*\\{[^}]*sendNode" "$file" || true
echo
echo "=== Open sendNode source file (via import path) ==="
# Extract likely module specifier for sendNode from the first match
imp=$(rg -n "sendNode" "$file" | head -n1 | sed -E "s/.*from '([^']+)'.*/\\1/")
echo "import module candidate: ${imp:-<none>}"
# Try common patterns: resolve by searching for "export.*sendNode" within repo
echo
echo "=== Search for sendNode definition/export in repo ==="
rg -n "export (async )?function sendNode\\b|export \\{[^}]*sendNode|const sendNode\\s*=|async function sendNode\\b|function sendNode\\b" src -g'*.ts' || true
echo
echo "=== Inspect buildAckStanza helper file ==="
# We know path from earlier import: src/Utils/stanza-ack
stfile="src/Utils/stanza-ack.ts"
if [ -f "$stfile" ]; then
sed -n '1,220p' "$stfile"
else
echo "File not found: $stfile"
rg -n "buildAckStanza" src/Utils -g'*.ts' || true
fi
echo
echo "=== Search for ws.close() logic related to send failures around sendNode/sendMessageAck ==="
rg -n "ws\\.close\\(|close\\(\\)" src/Socket -g'*.ts' | head -n 50 || trueRepository: WhiskeySockets/Baileys
Length of output: 5433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/Socket/socket.ts"
echo "=== Inspect sendNode implementation around src/Socket/socket.ts:153 ==="
sed -n '120,210p' "$file" | awk '{print NR "\t" $0}'
echo
echo "=== Search for ws.send / error handling in sendNode path ==="
rg -n "sendNode\\b|ws\\.send\\(|socket\\.send\\(|await ws\\.send|await query\\(" "$file" -S || true
echo
echo "=== Inspect websocket close behavior in src/Socket/Client/websocket.ts ==="
wfile="src/Socket/Client/websocket.ts"
sed -n '1,120p' "$wfile" | awk '{print NR "\t" $0}'
echo
echo "=== Inspect ws.close() call site around src/Socket/socket.ts:641 ==="
line=$(rg -n "ws\\.close\\(" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${line}" ]; then
start=$((line-40)); end=$((line+80))
sed -n "${start},${end}p" "$file" | awk '{print NR "\t" $0}'
fiRepository: WhiskeySockets/Baileys
Length of output: 8938
Make WebSocket close on notification ACK failure conditional
handleNotificationalways callssendMessageAck(node)in afinally, and anysendMessageAckerror unconditionally logs and callsws.close()(no gating bynodeType—processNotificationhandles multiple types such asnewsletter,encrypt,devices,account_sync,link_code_companion_reg,privacy_token—and no inspection ofackErr/connection state).sendNode/sendRawMessagecan fail simply because the transport is already closed (it throwsBoom('Connection Closed')when!ws.isOpen), so forcing a close on every transient ack/send failure risks reconnection churn; gatews.close()on unrecoverable transport errors (and/or restrict forced close to the auth/QR-relevant notification types).
await sendMessageAck(node).catch(ackErr => {
logger.error({ ackErr }, 'failed to ack notification')
ws.close();
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Socket/messages-recv.ts` around lines 1581 - 1584, handleNotification
currently always calls sendMessageAck(node) in a finally and unconditionally
calls ws.close() in the catch, which causes unnecessary reconnections for
transient transport errors; change the catch to inspect the failure and only
close the socket for unrecoverable errors or auth/QR-related node types: in the
sendMessageAck(node).catch block (referencing sendMessageAck,
handleNotification, sendNode/sendRawMessage and ws), detect Boom('Connection
Closed')/transport-closed conditions or check ws.isOpen and if the error is a
transport-closed/transient error (or ackErr indicates connection already closed)
just log and return, otherwise for unrecoverable errors or for node.nodeType
values that require re-auth (e.g., auth/qr-related types such as
link_code_companion_reg/account_sync) call ws.close(); ensure you use
Boom.isBoom or error.output.statusCode (or error.message) to distinguish error
kinds.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
2 participants
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
When
sendMessageAckfails duringhandleNotification, the error issilently swallowed via
.catch()and the socket remains open indefinitely.This causes the connection to hang — no
connection.updateevent is emitted,so the application has no way to detect the failure and reconnect.
Symptoms:
failed to ack notificationlogged with emptyackErr: {}Fix
Call
ws.close()after logging the error to trigger the normalreconnection flow via
connection.update.Reproduction
failed to ack notificationis loggedSummary by cubic
Close the WebSocket when
sendMessageAckfails so the connection doesn’t hang and the app can auto-reconnect viaconnection.update, fixing QR scans stuck on “Couldn't connect device.” Also remove a stray semicolon per our style guide.Written for commit e8b66b6. Summary will update on new commits.
Summary by CodeRabbit