fix(outbound): hold active-delivery claim so reconnect drain skips live sends by neeravmakwana · Pull Request #70428 · openclaw/openclaw

neeravmakwana · 2026-04-23T01:37:19Z

Summary

Fixes #70386. Reconnect drain was re-driving the same queue entry while the
original deliverOutboundPayloads was still mid-send, producing 7-12x
outbound duplication on WhatsApp cron sends whenever the 30-minute
inbound-silence watchdog fired during a delivery window.

Root cause

drainPendingDeliveries is intentionally allowed to replay fresh entries
(retryCount === 0 && lastAttemptAt === undefined) to preserve crash-replay.
That contract was safe while the reconnect drain previously required a
specific "No active ... listener" lastError on the entry; once the drain
selector was widened to match any pending entry for the reconnecting account,
nothing prevented it from firing against an entry whose original
deliverOutboundPayloads caller was still executing. The live delivery path
persisted via enqueueDelivery but never held an in-memory claim on
queueId during the send, so a concurrent reconnect passed
claimRecoveryEntry, passed isEntryEligibleForRecoveryRetry (fresh entries
are drain-eligible by design), and invoked deliver(...) a second (or Nth)
time — once per reconnect that occurred inside the live send window.

Cron run records showed only one delivered: true per scheduled run because
the drain's deliver() enqueues and acks a new entry path, leaving no
visible audit in the scheduler.

Fix

Claim queueId against the existing entriesInProgress set immediately
after enqueueDelivery, and release in the finally block that wraps
ack/fail in deliverOutboundPayloads. Drain already consults the same set
via claimRecoveryEntry and skips claimed ids with an
"already being recovered" log, so no drain-side logic change is needed.

Two new thin exports on delivery-queue.ts:

tryClaimActiveDelivery(entryId) — returns false if already claimed.
releaseActiveDelivery(entryId) — pair in finally.

Why the fix is safe

The claim is a process-local Set. A crashed owner leaves no claim
behind, so recoverPendingDeliveries on the next startup reclaims any
orphaned entries exactly as before — crash replay for fresh entries is
preserved intentionally.
Drain already had the skip path ("already being recovered") for
in-progress recovery callers; this reuses it and adds a second legitimate
owner: the live delivery path.
No queue file format change, no migration, no persistence change.
The shared concurrency guard claimRecoveryEntry / releaseRecoveryEntry
is unchanged; the new helpers are a stable public wrapper over the same
set so other callers (live sends) can participate without importing a
private symbol.
Regression test covers exactly the race: claim the entry (simulating a
live send in flight), run the drain, assert deliver is not called and
the "already being recovered" skip log is emitted; after release, a
follow-up drain delivers exactly once.

Security / runtime controls unchanged

No prompt-derived policy surface is involved. Concurrency is enforced
structurally via the in-memory claim set, not by message text, metadata,
or any LLM-facing behavior.
No config defaults, no help metadata, no generated artifacts change.
Plugin SDK public surface (src/plugin-sdk/infra-runtime.ts) selectively
re-exports only drainPendingDeliveries; the two new helpers are
deliberately not re-exposed there, so third-party plugin contracts and
docs/.generated/plugin-sdk-api-baseline.* are intentionally untouched.
Outbound delivery adapters, channel plugins, session/policy keys,
transcript mirroring, and best-effort / abort behavior are all
unmodified — the claim wraps the existing ack/fail flow in a finally
without changing any of its branches.

Tests

Added regression test: src/infra/outbound/delivery-queue.reconnect-drain.test.ts
→ "skips entries that an in-flight live delivery has actively claimed".

Exact commands run locally:

pnpm test src/infra/outbound/delivery-queue.reconnect-drain.test.ts
— 14 passed (includes new case).
pnpm test src/infra/outbound — 452 passed across 35 files.
pnpm test src/gateway/server-restart-sentinel.test.ts src/gateway/server-runtime-services.test.ts src/plugin-sdk/infra-runtime.test.ts — the other consumers of the
./delivery-queue.js mock, 27 passed.
pnpm test src/cron/isolated-agent — cron delivery-dispatch consumers of
deliverOutboundPayloads, 222 passed across 20 files.
pnpm check:changed — scoped typecheck/lint/guards green.
pnpm tsgo — core prod typecheck green.

The pnpm tsgo:prod failures on unrelated qa-lab, qqbot, and
tokenjuice extensions reproduce on origin/main at the same SHA and are
not introduced by this change. Same for the pre-existing
plugin-sdk-api-baseline hash drift — this PR does not add any public
plugin-sdk surface.

Checklist

Noted as AI-assisted
Fully tested (new regression test plus full outbound/cron/infra lanes)
Understand what the code does
Will resolve/reply to bot review conversations after addressing them

Made with Cursor

greptile-apps · 2026-04-23T01:40:12Z

Greptile Summary

This PR fixes a 7-12x outbound duplication bug on WhatsApp cron sends by adding an in-memory active-delivery claim (tryClaimActiveDelivery/releaseActiveDelivery) in deliverOutboundPayloads immediately after enqueueDelivery, so a concurrent reconnect drain's claimRecoveryEntry call finds the entry already held and skips it. The fix is minimal and targeted — it reuses the existing entriesInProgress set and the drain's existing "already being recovered" skip path, with no queue-format or persistence changes. The regression test and claim/release logic are both correct.

Confidence Score: 5/5

Safe to merge — the fix is correctly scoped, the release is properly guarded in finally, crash-replay semantics are preserved, and the regression test validates the primary scenario end-to-end.

All findings are P2 or lower. The residual narrow race (the window between enqueueDelivery completing and tryClaimActiveDelivery being called) is effectively reduced to a single synchronous step with no intervening awaits, making it practically irreproducible compared to the original multi-minute window. Cleanup logic (guard with heldActiveClaim in finally) is correct.

No files require special attention.

_{Reviews (1): Last reviewed commit: "fix(outbound): hold active-delivery clai..." | Re-trigger Greptile}

neeravmakwana · 2026-04-23T01:45:04Z

Addressed both bot reviews. Pushed 9f30ceb.

Response to Aisle (Medium, CWE-362)

This finding is correct. The scenario is:

Live path awaits enqueueDelivery(...), the queue file is written.
Before the live path's continuation resumes, a concurrent reconnect/startup drain runs loadPendingDeliveries → synchronously calls claimRecoveryEntry(entryId) and wins.
Live path resumes, tryClaimActiveDelivery(queueId) returns false.
Previous code still fell through to deliverOutboundPayloadsCore and then ack/fail → duplicate outbound message + racing queue cleanup with the drain.

Fix in src/infra/outbound/deliver.ts:

const heldActiveClaim = queueId ? tryClaimActiveDelivery(queueId) : false;
if (queueId && !heldActiveClaim) {
  return [];
}

Rationale for the specific choice of "return [] and leave the queue entry alone":

The drain path that won the claim already owns delivery and ack/fail for this queueId, so letting it complete is exactly the contract we want.
Not calling ackDelivery / failDelivery here is intentional and required — touching the queue after losing the claim would race the drain's cleanup.
Not calling releaseActiveDelivery here is also intentional — we never acquired the claim in this branch, so there is nothing to release. The finally branch is already guarded with if (queueId && heldActiveClaim) so this is safe.
Returning [] is consistent with the existing shape of deliverOutboundPayloads; callers already handle empty result arrays (e.g. fully-sanitized-to-empty payload lists go down the same path).

New regression test in src/infra/outbound/deliver.test.ts named "bails out without sending when a concurrent drain already claimed the queue entry" forces tryClaimActiveDelivery to return false and asserts that:

sendMatrix is never called.
ackDelivery is never called.
failDelivery is never called.
releaseActiveDelivery is never called.
The result is [].

I also reset the two new mocks in the beforeEach (tryClaimActiveDelivery.mockReturnValue(true), releaseActiveDelivery.mockClear()) so the default state across the rest of the suite keeps matching production (claim always succeeds when the set is empty) and state does not leak between tests.

Response to Greptile (5/5)

Confirming the read. The claim/release is guarded in finally with heldActiveClaim so we never release a claim we did not acquire, and the process-local Set keeps crash-replay for orphaned entries working unchanged. With this follow-up, the residual "another in-process owner won the race" path is now also handled safely instead of relying on it being practically hard to hit.

Tests run

pnpm test src/infra/outbound/deliver.test.ts src/infra/outbound/delivery-queue.reconnect-drain.test.ts — 55/55 pass (includes the two new regression cases).
pnpm test src/infra/outbound — 453/453 across 35 files.
pnpm test src/cron/isolated-agent — 222/222 across 20 files.
pnpm test src/gateway/server-restart-sentinel.test.ts src/plugin-sdk/infra-runtime.test.ts — 24/24 (the other consumers of the ./delivery-queue.js mock).
pnpm check:changed — scoped typecheck, lint, guards, import cycles green; core-tests lane green.

Scope guardrails

No change to drainPendingDeliveries, claimRecoveryEntry, or any persisted queue format.
No change to plugin-sdk public surface; infra-runtime.ts still re-exports only drainPendingDeliveries.
No config, docs, or generated baseline artifacts touched.
No change to policy, prompt handling, or any security surface beyond the runtime concurrency guard described above.

aisle-research-bot · 2026-04-23T02:09:45Z

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

#	Severity	Title
1	🟡 Medium	Process-local delivery claim held across potentially unbounded network send operations (recovery/drain DoS)

1. 🟡 Process-local delivery claim held across potentially unbounded network send operations (recovery/drain DoS)

Property	Value
Severity	Medium
CWE	CWE-400
Location	`src/infra/outbound/deliver.ts:668-680`

Description

deliverOutboundPayloads now wraps the entire live delivery + queue cleanup flow in withActiveDeliveryClaim(queueId, ...). This claim is stored in a process-local entriesInProgress set and is also used by reconnect/startup drains.

Because the claim is held while deliverOutboundPayloadsWithQueueCleanup() awaits outbound adapter sends (e.g., handler.sendText, handler.sendPayload, handler.sendMedia), any adapter call that hangs indefinitely (network stall, missing request timeout, missing abort propagation) will prevent the finally block in withActiveDeliveryClaim from running.

Impact:

The queue entry remains permanently “in progress” in this process.
Reconnect/startup drains that see the same queue entry will log it as already being recovered and skip it indefinitely.
The on-disk queue can grow without recovery, creating a denial-of-service condition.

Vulnerable flow (claim spans awaits to network-dependent sends):

const claimResult = await withActiveDeliveryClaim(queueId, () =>
  deliverOutboundPayloadsWithQueueCleanup(params, queueId),
);

Inside deliverOutboundPayloadsWithQueueCleanup/deliverOutboundPayloadsCore, multiple awaits call outbound adapters (not shown here) without any enforced timeout, and the adapter API does not visibly take an AbortSignal for cancellation.

Recommendation

Ensure the active-delivery claim cannot be held indefinitely by unbounded network operations.

Options (combine as appropriate):

Enforce a hard timeout around the claimed section and fail/abort the delivery when exceeded:

import { withTimeout } from "./timeouts"; // example helper

const claimResult = await withActiveDeliveryClaim(queueId, () =>
  withTimeout(
    deliverOutboundPayloadsWithQueueCleanup(params, queueId),
    cfg.outboundSendTimeoutMs ?? 30_000,
  ),
);

Propagate cancellation by making outbound adapter methods accept an AbortSignal and passing params.abortSignal through; then ensure each adapter uses it (e.g., fetch(..., { signal })) and/or implements its own request timeout.
As a defensive fallback, consider a lease/TTL-based claim rather than an unbounded in-memory set entry, so recovery can re-attempt after the lease expires.

These changes ensure finally executes in bounded time and drains cannot be blocked forever by a hung send.

Analyzed PR: #70428 at commit b71f2f1

_{Last updated on: 2026-04-23T02:22:54Z}

…ve sends Reconnect drain (drainPendingDeliveries) matches fresh pending entries by design to preserve crash-replay, but the live delivery path in deliverOutboundPayloads held no in-memory claim while the send was running. A reconnect firing mid-send therefore re-drove the same queue entry and produced duplicate outbound messages (e.g. WhatsApp cron sends going out 7-12x when the 30-minute inbound-silence watchdog fired during delivery). Claim the queueId against the existing entriesInProgress set right after enqueueDelivery and release it in the finally branch around ack/fail. Drain already skips claimed ids via claimRecoveryEntry, so no drain-side change is needed. The claim is process-local on purpose: a crashed owner leaves no claim behind, so startup recovery still reclaims orphaned entries. Fixes openclaw#70386. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw#70428. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on #70428. Made-with: Cursor

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b71f2f110d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-23T02:25:11Z

+  if (claimResult.status === "claimed-by-other-owner") {
+    return [];


Avoid returning empty success when queue claim is already held

When withActiveDeliveryClaim returns claimed-by-other-owner, this path returns [] as if delivery simply produced no results. That creates a regression for callers that treat empty results as failure (for example src/gateway/server-methods/send.ts throws on no result), so in the enqueue/claim race with reconnect drain a message can be delivered by the drain while the original API call still fails, prompting client retries and possible duplicate user-visible sends. This should return a distinct state/error or synchronize with the active owner instead of an empty successful result.

Useful? React with 👍 / 👎.

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw#70428. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw/openclaw#70428. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw#70428. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw/openclaw#70428. Made-with: Cursor

If a reconnect/startup drain observes the newly enqueued queue entry and calls claimRecoveryEntry before the live delivery path reaches tryClaimActiveDelivery, tryClaimActiveDelivery returns false. Previously the live path still proceeded to deliverOutboundPayloadsCore and then ack/fail, which would race the drain's own delivery and ack/fail for the same entry id and produce duplicate outbound messages. Treat a failed claim acquisition as "another in-process owner is already handling this queue entry" and bail out with an empty result array, leaving the queue entry in place for the drain to deliver and clean up. This closes the narrow residual race called out by the Aisle security review on openclaw#70428. Made-with: Cursor

openclaw-barnacle Bot added the size: S label Apr 23, 2026

neeravmakwana and others added 3 commits April 23, 2026 03:15

fix(outbound): centralize active delivery claims

b71f2f1

steipete force-pushed the fix/whatsapp-reconnect-drain-duplicate-sends branch from da9e138 to b71f2f1 Compare April 23, 2026 02:20

steipete merged commit aa27a94 into openclaw:main Apr 23, 2026
72 checks passed

chatgpt-codex-connector Bot reviewed Apr 23, 2026

View reviewed changes

clawsweeper Bot mentioned this pull request May 11, 2026

Gateway replays stale queued messages after restart #55564

Closed

clawsweeper Bot mentioned this pull request Jul 2, 2026

WhatsApp: add configurable send retry for transient network errors #54183

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(outbound): hold active-delivery claim so reconnect drain skips live sends#70428

fix(outbound): hold active-delivery claim so reconnect drain skips live sends#70428
steipete merged 3 commits into
openclaw:mainfrom
neeravmakwana:fix/whatsapp-reconnect-drain-duplicate-sends

neeravmakwana commented Apr 23, 2026

Uh oh!

greptile-apps Bot commented Apr 23, 2026

Uh oh!

neeravmakwana commented Apr 23, 2026

Uh oh!

aisle-research-bot Bot commented Apr 23, 2026 •

edited

Loading

Description

Recommendation

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if (claimResult.status === "claimed-by-other-owner") {
		return [];

Uh oh!

Uh oh!

Conversation

neeravmakwana commented Apr 23, 2026

Summary

Root cause

Fix

Why the fix is safe

Security / runtime controls unchanged

Tests

Checklist

Uh oh!

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

neeravmakwana commented Apr 23, 2026

Response to Aisle (Medium, CWE-362)

Response to Greptile (5/5)

Tests run

Scope guardrails

Uh oh!

aisle-research-bot Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

Description

Recommendation

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aisle-research-bot Bot commented Apr 23, 2026 •

edited

Loading