fix(whatsapp-gateway): snapshot sock at sendOrEdit entry (supersedes #4759)#4851
Merged
Conversation
…ty, sweep race Hardens the gateway against operational issues observed on long-running deployments. Bundled changes: **Connection resilience** - HEARTBEAT_MS raised from 180s → 300s. Baileys' executeInitQueries and fetchProps legitimately exceed 60s after long sessions; 180s burned the gateway on every reconnect. - scheduleReconnect() helper. The previous setTimeout(()=>startConnection().catch(...)) inside the close-handler stranded the gateway when startConnection threw before the new sock's connection.update listener was installed: no future event source meant no future retry. The helper self-reschedules on retry-throw so the in-process recovery loop stays alive end-to-end. - Quiet link-preview-js warnings. Advisory-only, fill stderr. - connection.update handler consolidated. Multiple registrations were stomping each other on hot-reload. **Dedup + retransmit** - dedupTracker window 60s → 600s. The 1min window was too tight for retransmits arriving after a Signal session-recovery round-trip. - Two-phase API: wasProcessed(id) / markProcessed(id) so retransmits of decrypt-failed messageIds reach the recovery handler instead of being dedup-skipped. **Crash safety** - WAL checkpoint + db.close() on graceful shutdown — prevents 'database is malformed' after SIGTERM. - Persist cached agent UUID across restarts (was re-resolved from API on every boot, which failed when the API was slow to come up). **Outbound delivery** - sendOrEdit rewritten with try/catch + bounded STREAM_EDIT_MAX_FAILURES backoff. After 3 consecutive edit failures the streaming path bails and the final delivery handles full text in one send. - Snapshot sock at callback entry as localSock — guards against the global sock being nulled out by a concurrent reconnect between existence check and sendMessage await. - Quoted-reply prefix now covers all media types (was text-only). - Structured send_message_outbound log lines (stream_first, edit_final, new_message) for prod observability. **Catch-up sweep race + shutdown drainage** - processing_since SQLite column on the message store. Two concurrent sweep cycles (boot + watchdog re-arm) were racing each other. - catchUpInterval + dbCleanupInterval handles captured at module scope so gracefulShutdown can clearInterval them alongside heartbeatInterval / gapDetectionTimer. Without this, runCatchUpSweep / runDbCleanup ticks could resume after db.close() and write to a closed handle. - runCatchUpSweep checks shuttingDown at entry AND between iterations, so a paused inter-iteration setTimeout doesn't resume into dbIncrRetryOrFail / dbSaveMessage post-shutdown. - Inter-iteration setTimeout is unref()'d so the event loop exits ~CATCHUP_INTER_DELAY_MS sooner per pending iteration during shutdown. **Health probe** - New scripts/health-check.sh for container probe and ops dashboards. Verification: - node --check: passes - node --test: 88 unit tests pass (dedup 8, echo 15, group-roster 5, identity 41, lid-cache 14, session-key 6) - Live deploy tested on the fork's NAS for 48h — no false-positive heartbeats, no double-deliveries. (cherry picked from commit 56ca169)
…al-delivery race One fix on top of #4759 (cherry-picked from f-liva): the streaming `onProgress` callback already snapshots `const localSock = sock` to guard against `cleanupSocket()` nulling the global mid-await, but `sendOrEdit` reverted to the global. The final delivery edit / send runs AFTER the LLM stream completes — that's a much wider window for a concurrent reconnect than per-chunk progress, and the symptoms are the same race: a "Cannot read properties of null" when `sock` is nulled between the check and the `sendMessage`, or a send through a half-closed socket. Snapshot once at entry, fail fast (returning the existing `streamMsgKey` so the user still sees the last visible chunk on WhatsApp), use the snapshot for both the edit branch and the new-message branch. Worth noting from the prior round of review (now stand down): - `shuttingDown` declaration / `gracefulShutdown` toggle ARE in the file (lines 3651 / 3660 on the PR head); the diff just didn't surface them. No fix needed. - `reconnectAttempts = 0` IS reset on the `connection === 'open'` branch (lines 1535 / 1566 on the PR head). No fix needed. Closes #4759.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… polish Three small follow-ups from review on #4851: 1. **sendOrEdit new-message branch try/catch.** The edit branch already swallows mid-await throws (stale streamMsgKey, sock half-closing between snapshot and sendMessage) and returns the streamed key as "last visible chunk is final". The new-message branch left the throw uncaught, so the same race in a fallback delivery would crash the message handler. Mirror the edit branch: try/catch, log `kind: 'new_message_failed'`, return null. Caller already handles a null return (same shape as the entry-snapshot bail-out). 2. **health-check.sh portability comment.** Top-of-file note that the script is Linux + PM2 only — uses GNU `stat -c %s` and `nslookup -timeout=3`, won't run on a macOS dev host. Saves the next reader 5 minutes of confusion when a quick local sanity-run silently does the wrong thing. 3. **pm2_restart faithful exit code.** `pm2 ... | while read` lets the `while`/`done` exit status (always 0) mask pm2's real status. Capture `${PIPESTATUS[0]}` so the function's boolean signal isn't a lie. `set -o pipefail` is already on, so this just makes the pre-existing intent explicit. Today's caller does `|| true`; this prevents a future caller from acting on a fake success. Verification: - node --check packages/whatsapp-gateway/index.js (clean) - bash -n packages/whatsapp-gateway/scripts/health-check.sh (clean) - node --test packages/whatsapp-gateway/index.test.js → 132/134 pass — the same 2 pre-existing flakes (`onOwnerNotice` ECONNRESET and `LIBREFANG_DISPATCH_LOG=off` forward_dispatch) the prior commit identified, both in HTTP/forward harness layers unrelated to the sendOrEdit closure or the bash script.
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
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
Picks up #4759's resilience pass verbatim (cherry-picked from f-liva with attribution preserved on commit 1) and closes the residual
sendOrEditrace that the prior round flagged.What lands here vs #4759
The streaming
onProgresscallback already snapshotsconst localSock = sockto guard againstcleanupSocket()nulling the global mid-await. ButsendOrEditreverted to the global — and the final delivery edit/send runs AFTER the LLM stream completes, which is a much wider window for a concurrent reconnect than per-chunk progress. Same race, same symptoms (Cannot read properties of nullat thesendMessagecall, or send through a half-closed socket once cleanupSocket has fired).This PR snapshots
sockonce at sendOrEdit entry, fails fast (returning the existingstreamMsgKeyso the user still sees the last visible chunk on WhatsApp), and uses the snapshot for both the edit branch and the new-message branch.Stand-down on the other prior-review items
After re-reading the PR HEAD (not just the diff slice):
shuttingDowndeclaration +gracefulShutdowntoggle ARE present (lines 3651 and 3660 on the PR head). The diff didn't surface them but they exist; the catch-up sweep's bail-out is real.reconnectAttempts = 0IS reset on theconnection === 'open'branch (lines 1535 and 1566). No leak.So the only blocker from the prior round is the
sendOrEditsnapshot, which this PR fixes.Verification
node --test packages/whatsapp-gateway/index.test.js— 132 pass / 2 fail.forwardToLibreFang surfaces owner_notice via onOwnerNotice callback,LIBREFANG_DISPATCH_LOG=off silences forward_dispatch but HTTP still fires) fail identically with and without my edit — confirmed by running with and withoutgit stash. They appear to be a pre-existing mock-server flake (ECONNRESET) inherited from the cherry-picked PR base, unrelated to thesendOrEditsnapshot in any way (my change touches a closure inside the message handler, not the HTTP layer).If those 2 are tracked as a known flake or shipped a follow-up to address the mock harness, please point me at it; otherwise happy to triage separately.
Refs
Closes #4759.
Test plan
node --test packages/whatsapp-gateway/index.test.js— 132/132 of the unrelated suites pass; my edit doesn't regress anything.sock_unavailable_at_sendlog fires under reconnect storms.