Skip to content

fix(whatsapp-gateway): snapshot sock at sendOrEdit entry (supersedes #4759)#4851

Merged
houko merged 3 commits into
mainfrom
fix/whatsapp-gateway-sendorEdit-race
May 10, 2026
Merged

fix(whatsapp-gateway): snapshot sock at sendOrEdit entry (supersedes #4759)#4851
houko merged 3 commits into
mainfrom
fix/whatsapp-gateway-sendorEdit-race

Conversation

@houko

@houko houko commented May 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Picks up #4759's resilience pass verbatim (cherry-picked from f-liva with attribution preserved on commit 1) and closes the residual sendOrEdit race that the prior round flagged.

What lands here vs #4759

The streaming onProgress callback already snapshots const localSock = sock to guard against cleanupSocket() nulling the global mid-await. But sendOrEdit reverted 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 null at the sendMessage call, or send through a half-closed socket once cleanupSocket has fired).

This PR snapshots sock once at sendOrEdit entry, fails fast (returning the existing streamMsgKey so the user still sees the last visible chunk on WhatsApp), and uses the snapshot for both the edit branch and the new-message branch.

const sendOrEdit = async (jid, finalText) => {
  const s = sock;
  if (!s) {
    console.warn(JSON.stringify({
      event: 'send_message_outbound',
      kind: 'sock_unavailable_at_send', jid, had_stream: Boolean(streamMsgKey),
    }));
    return streamMsgKey || null;
  }
  // ... rest uses `s` instead of `sock`
};

Stand-down on the other prior-review items

After re-reading the PR HEAD (not just the diff slice):

  • shuttingDown declaration + gracefulShutdown toggle 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 = 0 IS reset on the connection === 'open' branch (lines 1535 and 1566). No leak.

So the only blocker from the prior round is the sendOrEdit snapshot, which this PR fixes.

Verification

  • node --test packages/whatsapp-gateway/index.test.js132 pass / 2 fail.
  • The 2 failing tests (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 without git stash. They appear to be a pre-existing mock-server flake (ECONNRESET) inherited from the cherry-picked PR base, unrelated to the sendOrEdit snapshot 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.
  • Live WhatsApp soak (human-only, requires a paired session) to confirm sock_unavailable_at_send log fires under reconnect storms.

Federico Liva and others added 2 commits May 10, 2026 23:32
…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.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added area/sdk JavaScript and Python SDKs size/L 250-999 lines changed labels May 10, 2026
… 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.
@houko
houko merged commit c14cbbe into main May 10, 2026
15 checks passed
@houko
houko deleted the fix/whatsapp-gateway-sendorEdit-race branch May 10, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sdk JavaScript and Python SDKs size/L 250-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant