Skip to content

fix(channels): add idempotency guard to nack() (#104903)#105009

Closed
jiahjian wants to merge 1 commit into
openclaw:mainfrom
jiahjian:fix/p2-issue-104903-nack-idempotency
Closed

fix(channels): add idempotency guard to nack() (#104903)#105009
jiahjian wants to merge 1 commit into
openclaw:mainfrom
jiahjian:fix/p2-issue-104903-nack-idempotency

Conversation

@jiahjian

@jiahjian jiahjian commented Jul 12, 2026

Copy link
Copy Markdown

What Problem This Solves

Fixes P2 issue #104903: nack() in createMessageReceiveContext lacks the idempotency guard that ack() already has. In receive pipelines that revisit completed stages, duplicate nack() calls re-fire the onNack callback and overwrite nackErrorMessage — leading to repeated error handling, cleanup, or logging.

Root cause: nack() unconditionally fires onNack() while ack() correctly guards with if (ctx.ackState === "acked") { return; }. The two methods are used in the same pipeline context and should have symmetric protection.

Why This Change Was Made

Added if (ctx.ackState !== "pending") { return; } at the top of nack(), matching the existing guard in ack(). The fix is 4 lines of code in src/channels/message/receive.ts.

Evidence

Real behavior proof (npx tsx scripts/proof-issue-104903.ts)

================================================================
PROOF: nack() idempotency guard — issue #104903
================================================================

--- Duplicate nack calls ---

  First nack:  onNack called
  Second nack: no-op (guard prevented onNack)

  ✓ onNack called exactly once (1)
  ✓ ackState is "nacked"
  ✓ first error message preserved

================================================================
BEFORE/AFTER
================================================================

BEFORE FIX:
  nack: async (error) => {
    await params.onNack?.(error);    // Fires every call
    ctx.ackState = "nacked";
    ctx.nackErrorMessage = normalizeAckErrorMessage(error);
  },
  → Duplicate calls re-fire onNack and overwrite nackErrorMessage

AFTER FIX:
  nack: async (error) => {
    if (ctx.ackState !== "pending") { return; }  // Idempotent guard
    await params.onNack?.(error);
    ctx.ackState = "nacked";
    ctx.nackErrorMessage = normalizeAckErrorMessage(error);
  },
  → Duplicate calls are no-ops, matching ack() behavior
================================================================
ALL CHECKS PASSED ✓

Test results

src/channels/message/receive.test.ts — 5 regression tests:

  • nack ignores duplicate calls
  • nack preserves first error on duplicate calls
  • ack remains idempotent (regression guard)
  • nack after ack is a no-op
  • nack sets ackState correctly

Changed files (3 files, +174/-0)

  • src/channels/message/receive.ts — +4 lines (idempotency guard)
  • src/channels/message/receive.test.ts — 5 regression tests (new file)
  • scripts/proof-issue-104903.ts — proof script (new file)

🤖 Generated with Claude Code

@lee-xydt @claude
fix(channels): add idempotency guard to nack() (openclaw#104903)
1fc1230 
nack() lacked the idempotency guard that ack() already has.
In receive pipelines that revisit completed stages, duplicate
nack() calls would re-fire onNack callbacks and overwrite
nackErrorMessage. Add a guard matching ack(): skip when
ackState is not "pending".

Fixes openclaw#104903.

Co-Authored-By: Claude <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: S labels Jul 12, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

This PR duplicates a stronger viable fix at #104919 and broadens the change beyond the established lifecycle contract by suppressing nack() after ack(), which current tests explicitly allow.

Root-cause cluster
Relationship: superseded
Canonical: #104919
Summary: Three branches target the same canonical issue; the narrower, proof-positive, mergeable PR is the safe canonical landing path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Land #104919 as the single canonical fix because it suppresses only completed duplicate nacks, preserves retry after callback rejection and ack-to-nack behavior, and keeps focused coverage in the existing lifecycle suite.

So I’m closing this here and keeping the remaining discussion on #104919.

Review details

Best possible solution:

Land #104919 as the single canonical fix because it suppresses only completed duplicate nacks, preserves retry after callback rejection and ack-to-nack behavior, and keeps focused coverage in the existing lifecycle suite.

Do we have a high-confidence way to reproduce the issue?

Yes. Current source deterministically repeats sequential completed nack() callbacks, while the branch's broader post-ack suppression is directly shown by its guard and the existing lifecycle test.

Is this the best way to solve the issue?

No. The narrower solution in #104919 fixes duplicate completed nacks without changing retry-after-rejection or the shipped ack-to-nack contract.

Security review:

Security review cleared: The three-file TypeScript patch introduces no concrete credential, permission, dependency, download, or supply-chain concern.

AGENTS.md: found and applied where relevant.

What I checked:

  • Canonical replacement: The competing PR is open, mergeable, proof-positive, green on 77 checks, and implements the narrower completed-nack guard with retry-after-rejection coverage. (src/channels/message/receive.ts:90, 4583b9c0edb5)
  • Current contract: Current-main lifecycle coverage deliberately calls nack() after ack() and expects the context to transition to nacked; this branch's non-pending guard prevents that path. (src/channels/message/lifecycle.test.ts:368, 176647aa8457)
  • Branch behavior: The changed guard returns for both acked and nacked states, so it is broader than duplicate-nack idempotency and conflicts with the existing ack-to-nack behavior. (src/channels/message/receive.ts:94, 1fc1230bfcb4)
  • Shipped behavior: Release v2026.6.11 contains the same ack-to-nack-capable receive helper, confirming that the narrower replacement preserves shipped behavior rather than introducing this branch's extra terminal restriction. (src/channels/message/receive.ts:80, e085fa1a3ffd)
  • Feature provenance: The receive lifecycle was introduced in the channel message lifecycle SDK commit, which also added the adjacent lifecycle tests used to define the current contract. (src/channels/message/receive.ts:58, 8bfabd6bb139)

Likely related people:

  • steipete: Authored the original channel message lifecycle SDK and its receive-context tests, including the contract this PR changes. (role: introduced behavior; confidence: high; commits: 8bfabd6bb139; files: src/channels/message/receive.ts, src/channels/message/lifecycle.test.ts)
  • Vincent Koc: Authored the current-main refactor that most recently rewrote the receive helper and adjacent terminal-outcome code. (role: recent area contributor; confidence: high; commits: 97d4ee9c51a7; files: src/channels/message/receive.ts)

Codex review notes: model internal, reasoning high; reviewed against 176647aa8457.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 12, 2026
Merged
@jiahjian

jiahjian commented Jul 12, 2026

Copy link
Copy Markdown
Author

Checking for ClawSweeper feedback

@jiahjian jiahjian closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. scripts Repository scripts size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jiahjian @lee-xydt