Review

Resilience pass is well-motivated and most of the engineering is solid. CI's Rust lanes correctly skip (no Rust touched). Two blocking concerns and a few medium/low items below.

⚠️ Blocking: dedup window 60s → 600s, but the two-phase API the description promises is not in the diff

The PR description says:

Two-phase API: wasProcessed(id) / markProcessed(id) so retransmits of decrypt-failed messageIds can reach the session-recovery handler

But lib/dedup-tracker.js is not in the changed files — only index.js, package.json, and health-check.sh. The diff at index.js:~908 retains the existing comment that warns against exactly this regression:

// `assertSessions` to recover the Signal session. A mark-on-sight dedup
// blocks the retransmit and strands the sender — 2026-04-16 outage, see
// lib/dedup-tracker.js docstring.
const dedupTracker = createDedupTracker({ windowMs: 600_000 });

If dedup-tracker.js is still mark-on-sight, multiplying the window by 10× widens the 2026-04-16-style outage by the same factor — decrypt-failure retransmits get silently swallowed for 10 minutes instead of 1.

Please clarify before merge:

1. Is the two-phase wasProcessed / markProcessed API already in lib/dedup-tracker.js from a prior PR? If so, please point at the file/commit so reviewers can verify, and reword the description to "leverage existing two-phase API".
2. Or did the dedup-tracker.js change get dropped? In that case, please revert the window bump and resubmit alongside the tracker change.

⚠️ Blocking: link-preview-js added with no require / import in the diff

package.json adds "link-preview-js": "^3.0.0", but no usage appears anywhere in index.js. The PR description mentions "Quiet link-preview-js advisory warnings" — likely Baileys' optional peer-dep emits warnings when it's missing. If that's the actual reason, please say so explicitly in the PR body (e.g., "added as a direct dep solely to silence Baileys' optional-peer warning; no code path requires it"). Otherwise this looks like an unused dep.

Medium

1. Asymmetric process-error handlers. unhandledRejection logs and continues; uncaughtException exits. The justification ("most unhandled rejections in this codebase are recoverable") is empirical, not audited. A single rejection may leave a half-completed transaction or a half-closed socket; continuing can amplify the failure. Node 15+ defaults to crash for a reason. Suggest at minimum adding a counter: if N rejections fire within K minutes, process.exit(1) and let PM2 restart.

2. gracefulShutdown doesn't drain in-flight writes or stop module-level timers before db.close() (index.js:~3573). heartbeatInterval, gapDetectionTimer, and the runCatchUpSweep interval are still scheduled when the close runs; they fire after db.close() and write to a closed handle. The new unhandledRejection handler catches and ignores them, but the cleanup window is racy. Recommend clearInterval for all module-scope intervals + await cleanupSocket() before the WAL checkpoint.

3. MEDIA_PIPELINE_TIMEOUT_MS setTimeout is never clearTimeout'd on success (index.js:~1777):

   await Promise.race([
     processMediaMessage(...),
     new Promise((_, reject) =>
       setTimeout(() => reject(new Error('media_pipeline_timeout')), MEDIA_PIPELINE_TIMEOUT_MS),
     ),
   ]);

   When processMediaMessage resolves first, the 90s timer keeps the event loop alive. N concurrent media messages leave N zombie timers. Capture the handle and clearTimeout in a .finally.

Low / nits

1. dbGetUnprocessed has no SQL LIMIT — the prepared statement loads every matching row, then the caller does .slice(0, CATCHUP_BATCH_SIZE). With a 1000-row backlog this is wasteful. Add LIMIT ? to the prepared statement and pass CATCHUP_BATCH_SIZE through.

2. dbClearProcessing only runs on the failure path. Success relies on processed = 1 filtering the row out via the WHERE processed = 0 clause. Logic is correct, but processing_since lingers as dead data. Optional: clear it inside whatever function flips processed = 1.

3. HEARTBEAT_MS 180s → 300s is a coarse knob. The real differentiator is "init phase" vs "steady state". Current change is pragmatic and worth shipping, but consider a follow-up issue: switch back to ~180s once connection === 'open' has been observed for a stable window. Otherwise real network hangs now take 5min instead of 3min to detect.

4. agent_id.json write isn't atomic. writeFileSync + SIGKILL mid-write → corrupt JSON next boot. Read-side parse-failure path falls back to resolveAgentId(), so it's self-healing, but the standard pattern is writeFileSync(tmp); renameSync(tmp, dst). Optional cleanup.

5. read_counter in health-check.sh is fragile against non-numeric content. failures=$(($(read_counter) + 1)) blows up under set -e if the file ever contains stray whitespace. Add [[ "$v" =~ ^[0-9]+$ ]] || v=0 inside the function.

6. PR size is borderline. "Interlocking" rationale is defensible — heartbeat / dedup window / sweep race do interact — but the dedup window bump alone (with its dedup-tracker.js companion) deserves to land separately given its blast radius. For next time.

Strengths

• processing_since lease design is clean. Claim → process → success (natural expiry via processed = 1) / failure (explicit release). The (processing_since IS NULL OR processing_since < ?) clause expresses lease expiry concisely; the lease bound covers crashed-handler recovery.
• gapDetectionTimer lifted to module scope correctly fixes the duplicate-fire bug from accumulating connection.update listeners across reconnects.
• localSock snapshot in onProgress correctly closes the await-vs-null-out race.
• WAL checkpoint + db.close() on SIGTERM is the right pattern for SQLite WAL durability.
• Media-pipeline cap with graceful degrade to text-only forwarding prevents head-of-line blocking and keeps user-visible behaviour reasonable.
• Structured outbound logs (event=send_message_outbound, kind=stream_first|edit_final|new_message) make ledger reconciliation tractable.
• Health probe distinguishes DNS blackout from process failure — exactly right; PM2-restarting through a network outage just floods logs.

Verdict

Request changes — the dedup window bump and the link-preview-js dep both need explicit clarification before merge. Once those are addressed, the medium items are mergeable as-is or with small follow-ups. Direction is sound; the engineering quality is high.

Review feedback addressed (force-push)

Thanks @houko — pushed a revision addressing the review.

Blocking

• ✅ Two-phase dedup API clarified: `lib/dedup-tracker.js` (`wasProcessed`/`markProcessed`) is upstream, not introduced by this PR. The window bump from 60s → 600s leverages the existing two-phase API; my original PR description was misleading. Updated. Bump is safe because retransmits of decrypt-failed messageIds reach the recovery handler (the tracker only gates on the `markProcessed` side).
• ✅ `link-preview-js` rationale: Baileys' optional peer dep emits a missing-module warning on every connect. Added as a direct dep solely to silence that — no code path requires it. Updated PR description.

Medium addressed

• ✅ `unhandledRejection` burst counter: rolling 5-rejection / 5-min window now triggers `process.exit(1)` so PM2 restarts on genuine broken state. Single rejections still log + continue. Sync-invariant noted in code (no `await` allowed in the handler).
• ✅ `gracefulShutdown` ordering: timers (`heartbeatInterval` / `gapDetectionTimer`) cleared BEFORE the DB close path; `cleanupSocket` now awaited inside `server.close` callback so Baileys writes finish before WAL checkpoint.
• ✅ Media-pipeline `clearTimeout`: handle captured + cleared in `finally` so the 90s zombie timer no longer keeps the event loop alive.

Low addressed

• ✅ `dbGetUnprocessed` pushes `LIMIT` to SQL: was load-all + JS `.slice`. Sweep now drains exactly `CATCHUP_BATCH_SIZE` rows from SQLite. New `UNPROCESSED_QUERY_DEFAULT_LIMIT` named constant for the debug endpoint.
• ✅ `agent_id.json` atomic write: tmp + rename + unlink-on-fail.
• ✅ `health-check.sh` numeric guard: `read_counter` rejects non-numeric content so `set -e` arithmetic stops crashing on corrupt counter files.

Skipped (per pass-1 review)

• `HEARTBEAT_MS` 180→300s steady-state knob: kept as flat bump for now; init-vs-steady tuning is a follow-up.
• Test coverage for new behaviors: not added in this PR (the gateway has no unit-test scaffolding for module-scope process handlers / shutdown sequencing). Worth a follow-up.

CI lights should re-run on the new HEAD.

Re-review on 13b49f9

Most of the houko review is addressed cleanly — the two-phase wasProcessed / markProcessed API is wired correctly (index.js:1609 read-side, :1638 mark-after-decrypt), the unhandledRejection 5-rejection / 5-min sliding window is sync-only and exits on threshold (:28-49), media-pipeline clearTimeout is in finally (:1825-1842), dbGetUnprocessed SQL LIMIT is pushed into the prepared statement (:216-223), and agent_id.json writes are tmp+rename+unlink-on-fail (:505-520). The link-preview-js peer-dep silencing rationale also holds — there's no require of it in the diff. Health-check read_counter numeric guard is in place at health-check.sh:73.

Hard blockers

1. PR is mergeable=dirty — needs rebase against current main.

2. Reconnect path can permanently strand the gateway.

index.js:1508-1519 schedules a reconnect via setTimeout(() => startConnection().catch(err => console.warn(...))). The companion catch at :2392-2399 makes startConnection throw on internal failure (dynamic import / useMultiFileAuthState / fetchLatestBaileysVersion). If the throw lands before sock.ev.on('connection.update', ...) is installed at :1453, no future connection.update close event source exists. The inline comment ("the catch keeps the next reconnect tick scheduled — we'll retry on the next connection.update close") is wrong: there is no listener to fire. A single transient fetchLatestBaileysVersion failure mid-reconnect leaves sock=null, no listener, no scheduled retry.

The 30s health-check + PM2 restart eventually rescues, so this isn't an outright break — it's a degraded recovery path that bypasses the in-process resilience the comment promises. Fix: re-schedule another setTimeout(() => startConnection().catch(...), nextDelay) inside the .catch, or set a sentinel that the heartbeat / outer driver re-arms.

Partial

3. gracefulShutdown only clears heartbeatInterval and gapDetectionTimer (index.js:3606-3613), but setInterval(runCatchUpSweep, ...) (:3159) and setInterval(runDbCleanup, ...) (:3177) are still running. Worse: a runCatchUpSweep already in-flight can be paused at await new Promise(r => setTimeout(r, 750)) (:3154) when db.close() runs in the server.close callback, then resume and write to a closed handle. The 10s force-exit timer mostly hides this, but the houko-flagged "drains in-flight writes before WAL checkpoint" claim is not actually met for the sweep path. Either clear both intervals at the top of gracefulShutdown, or set a shutdown sentinel that the sweep's inter-iteration setTimeout checks before continuing.

Minor

• processing_since not cleared on success — dbMarkProcessed(msg.id, 1) at :188-190 only flips processed; the lease column lingers as dead data. Houko flagged this as Optional and you skipped it — fine, but worth a one-liner in the PR body so the next reader doesn't think it's a bug.
• /messages/unprocessed debug endpoint default at :3471 calls dbGetUnprocessed(Date.now()), which returns rows currently held by the live handler (claim within last few ms). Behavior is correct (debug should show everything) but the JSDoc at :286-291 reads as if "in-flight excluded" is the always-true contract. Doc/code mismatch only.
• unref() on the sweep inter-iteration setTimeout at :3154 would let the event loop exit ~750ms sooner during shutdown per pending iteration. Minor.

Verdict

Rebase + fix the reconnect-stranding .catch. The sweep-shutdown race is real but mitigated by the 10s force-exit timer. Direction is sound.

Re-review feedback addressed (rebase + force-push)

Thanks @houko — pushed a rebase onto current upstream/main plus the re-review fixes on 13b49f9e.

Hard blocker — mergeable=dirty

✅ Rebased onto current main. Single-conflict in index.js was the streaming-callback prelude: main added the STREAM_TO_CHANNEL config gate; this PR added the streamEditFailures budget + localSock snapshot. Resolved by combining: gate first, then budget, then snapshot.

Hard blocker — reconnect can permanently strand the gateway

✅ scheduleReconnect() helper with self-rescheduling .catch. The previous shape — setTimeout(()=>startConnection().catch(warn)) inside the close-handler — left the gateway stranded if startConnection threw before sock.ev.on('connection.update', ...) was installed: no future event source meant no future retry, only the 30s health-check + PM2 restart could rescue.

The new helper:

1. Increments reconnectAttempts, computes backoff, updates status line.
2. setTimeout(() => startConnection().catch(err => { warn; scheduleReconnect(); })).
3. The .catch arms a fresh scheduleReconnect() call, so a retry-throw before the listener install simply queues the next backoff tick. The in-process recovery loop stays alive without depending on the close-event listener that the failed attempt never managed to install.

The close-handler now calls scheduleReconnect() instead of inlining the same logic.

Partial — gracefulShutdown drainage

✅ catchUpInterval + dbCleanupInterval captured at module scope, cleared in gracefulShutdown alongside the existing heartbeatInterval / gapDetectionTimer clears.

✅ runCatchUpSweep checks shuttingDown at function entry AND between iterations: a sweep paused mid-loop at the inter-iteration setTimeout no longer resumes into dbIncrRetryOrFail / dbSaveMessage after db.close() runs in the server.close callback.

✅ Inter-iteration setTimeout unref()'d — lets the event loop exit ~CATCHUP_INTER_DELAY_MS sooner per pending iteration during shutdown.

The houko-flagged "drains in-flight writes before WAL checkpoint" claim now actually holds: sweep bails on shutdown, intervals are cleared, then cleanupSocket + wal_checkpoint(TRUNCATE) + db.close() run in sequence.

Defensibly deferred (mentioned for transparency)

• processing_since not cleared on success — non-blocking per your earlier verdict; cosmetic dead column entry. The SQL doesn't depend on it being null after success. Will fold into the integration-test PR if that lands.
• /messages/unprocessed debug endpoint default — JSDoc/code mismatch is doc-only; debug behaviour is intentional.

Verification

• node --check index.js — passes.
• node --test per file: dedup 8, echo 15, group-roster 5, identity 41, lid-cache 14, session-key 6 → 88 unit tests pass, 0 fail.

PR diff vs upstream/main is now a single commit: index.js (+551/−57 with all the resilience changes plus the re-review fixes), package.json (+1), scripts/health-check.sh (+205 new file). Single-commit shape preserved for clean squash-merge.

Re-review on 56ca169. Most of the prior round is addressed cleanly. One residual issue worth a fix before merge, plus one verification ask.

1. sendOrEdit reverted to global sock — same race the PR fixes elsewhere.
The streaming onProgress correctly snapshots const localSock = sock at callback entry so a concurrent reconnect can't null it out mid-await. But sendOrEdit calls sock.sendMessage(...) directly on the global. The final-delivery edit path runs after the LLM stream completes — that's a much wider window for cleanupSocket() to null sock than the per-chunk progress callback. Snapshot once at sendOrEdit entry: const s = sock; if (!s) return null; then use s throughout. Otherwise the localSock fix is half-done.

2. Verification ask: shuttingDown ownership.
runCatchUpSweep reads shuttingDown at entry and between iterations, but the diff doesn't show its declaration or where gracefulShutdown flips it to true. If it lives outside the diff, please point at the line so reviewers can confirm the sweep actually bails. If gracefulShutdown doesn't set it, the bail-out is dead code and the sweep can still write to a closed db handle.

3. Nit (non-blocking): reconnect attempt counter reset.
scheduleReconnect() increments reconnectAttempts indefinitely. I don't see a reset on the connection === 'open' branch in the diff. If it's not reset elsewhere, after a few hours of intermittent network the next legitimate reconnect waits at the cap delay even from a fully-recovered state. Worth a one-liner: reconnectAttempts = 0 on open.

Direction is sound; #1 is the only thing I'd block on.

Closing — fully superseded by sibling PRs

Reopened to address the three blockers from the 2026-05-10 re-review, but after rebasing onto current main the resilience changes contribute zero unique content: all three items have already landed via sibling PRs.

Verification against b8dedd61 (current main tip)

1. sendOrEdit snapshots global sock at entry
   Addressed by fix(whatsapp-gateway): snapshot sock at sendOrEdit entry (supersedes #4759) #4851 (c14cbbea) — explicit "supersedes fix(whatsapp-gateway): resilience pass — heartbeat, dedup, crash-safety, sweep race #4759" in the title. Same shape as the re-review asked for: const s = sock; if (!s) return null; ... s.sendMessage(...).

2. shuttingDown ownership
   index.js:3691 declares let shuttingDown = false;, gracefulShutdown flips it at :3700, and runCatchUpSweep reads it at :3166 + :3180. Live, not dead code.

3. reconnectAttempts = 0 on connection === 'open'
   Reset at two sites: :1535 and :1566. Backoff cap no longer pins after recovery.

git diff HEAD upstream/main on the rebased PR returns one trivial line; git log upstream/main..HEAD is empty. No diff to push.

Closing again — thanks for the patient reviews. Houko's pattern of carving the most contentious bit into its own follow-up PR (#4851) is the cleaner path forward for next time.