PR #70142: fix(gateway): re-check chatAbortControllers after attachment parse (protect user abort pathway)

• Repository: openclaw/openclaw
• Author: Feelw00
• State: open | merged: False
• Link: https://github.com/openclaw/openclaw/pull/70142

Description (problem / solution / changelog)

Summary

• The attachment branch of chat.send has two real I/O awaits (resolveGatewayModelSupportsImages, parseMessageWithAttachments) between the pre-parse chatAbortControllers.get guard and the post-parse .set. A same-idempotencyKey retry that arrives during that window passes the initial guard and overwrites the original caller's AbortController entry — subsequent user-initiated chat.abort hits the orphan controller and fails to stop the real run.
• Add a post-parse re-check so the documented same-idempotencyKey contract (docs/web/control-ui.md#L131-L132, docs/concepts/architecture.md#L95-L96) holds on the attachment branch as it already does on the no-attachment branch (which has no await between get and set and is atomic under single-thread semantics).
• Regression test included: two concurrent same-idempotencyKey attachment sends must produce exactly one started ack and one in_flight ack, and the map entry must belong to the winner.

Change Type

• Bug fix
• New feature
• Refactor required for the fix
• Chore / tooling only

Scope

• src/gateway/server-methods/chat.ts (+20 lines, attachment branch only)
• src/gateway/server-methods/chat.directive-tags.test.ts (+150 lines — a parseMessageWithAttachments wrapper mock and a new race describe block)

The new test lives in chat.directive-tags.test.ts to reuse the existing mock harness. Happy to split into chat.attachment-race.test.ts in a follow-up if preferred.

Linked Issue

Closes #70139

Root Cause

1. The chat.send handler in chat.ts passes the pre-parse chatAbortControllers.get(clientRunId) guard.
2. When normalizedAttachments.length > 0, it awaits resolveGatewayModelSupportsImages and then parseMessageWithAttachments — this is the race window.
3. A second caller with the same idempotencyKey arriving in that window also passes the guard.
4. Both callers reach chatAbortControllers.set (~L1960) and the second one overwrites the first caller's AbortController entry.
5. The real run keeps executing under the first controller, but the map now points at the orphan second controller — a later chat.abort RPC aborts the orphan and the real run keeps running.

claimInboundDedupe (auto-reply/reply/inbound-dedupe.ts:94-112) already blocks the downstream agent-pipeline duplicate, so the LLM is only called once. The remaining harms are gateway-layer:

• 🔴 User abort misdirection — user-visible reliability regression
• Duplicate {status: "started"} ack sent to the client
• Duplicate persistChatSendImages / saveMediaBuffer writes (offloadedRefs duplicated)
• context.dedupe double-write on .then/.catch

Regression Test Plan

A new describe block chat.send attachment branch — chatAbortControllers atomicity in chat.directive-tags.test.ts:

• parseMessageWithAttachments is wrapped via a hoisted attachmentParseHooks.pending deferred-promise hook (non-race tests are unaffected — when pending === null the wrapper delegates to the real function via vi.importActual).
• Fire two concurrent same-idempotencyKey attachment sends with chatHandlers["chat.send"] against a shared context.
• Wait until callCount >= 2 so both callers have entered the race window.
• Release the deferred parse and await both settled.
• Assertions: startedAcks.length === 1 and inFlightAcks.length === 1 and chatAbortControllers.has(idem) === true.

Baseline proof:

• Before fix (revert chat.ts only, keep the test): both callers receive started → expect(startedAcks).toHaveLength(1) fails.
• After fix: passes.

Security Impact

• No new permissions, secrets, or network calls.
• No data-scope changes.
• No-attachment branch is unchanged (still atomic).
• When the re-check fires, the second caller receives the exact same payload / cached: true metadata as the pre-parse guard's {status: "in_flight"} response.

Repro + Verification

Environment

• openclaw upstream/main at 45ca6566 (rebased)
• Node 22.18.0, pnpm 10.33.0

Steps

1. git checkout fix/chat-send-attachment-race-recheck
2. pnpm install --frozen-lockfile
3. pnpm exec vitest run --config test/vitest/vitest.gateway.config.ts src/gateway/server-methods/chat.directive-tags.test.ts

Expected

• After fix: 54/54 pass.
• Before fix (revert only chat.ts): 1 failed in the attachment race describe.

Actual

54 passed (54).

Evidence

Failing test before (chat.ts reverted, test kept)

× chat.send attachment branch — chatAbortControllers atomicity > returns in_flight for a same-idempotencyKey retry that arrives during attachment parsing
  AssertionError: expected startedAcks length 1 but got 2

Passing test after

 Test Files  1 passed (1)
      Tests  54 passed (54)
   Duration  945ms

Full gateway config

pnpm exec vitest run --config test/vitest/vitest.gateway.config.ts → 2577/2579 passed. The two pre-existing failures are in server.plugin-http-auth.test.ts (Mattermost slash callback routes) and reproduce on upstream/main without this PR — unrelated.

Build + Check

• pnpm build ✅
• pnpm check ✅

Human Verification

1. Worktree-level before/after bisection confirmed (1 failed vs 54 passed).
2. No-attachment branch structure unchanged (no await between get and set; still atomic).
3. Rationale for Option B (post-parse re-check): stays XS, keeps the no-attachment sibling unchanged, touches neither server-maintenance.ts nor chat-abort.ts.

Review Conversations

• Ready for Greptile / Codex / maintainer review.
• Different axis from PR #69208 (duplicate transcript/replay umbrella) — this PR is a pre-spawn gateway-layer guard; the umbrella is post-persist.

Compatibility / Migration

• No API contract changes — if anything, this PR makes the documented {status: "in_flight"} contract hold on the attachment branch too.
• No wire-format changes.
• No migration required.
• No performance impact on the no-attachment chat.send path (that branch is not modified).

Risks and Mitigations

Related

• Related: #69208 (duplicate transcript/replay umbrella — different axis)
• Complementary to: #68801 (agentRunStarts TTL sweep — downstream safety belt)
• Adjacent: #69747 (surface chat.send lifecycle errors — adjacent lines, semantically orthogonal; trivial rebase if either lands first)

[AI-assisted] — tested via the regression harness above; reviewed through the openclaw-audit pre-PR 5-agent cross-review protocol.

Changed files

• src/gateway/server-methods/chat.directive-tags.test.ts (modified, +178/-1)
• src/gateway/server-methods/chat.ts (modified, +44/-2)