Skip to content

fix(mcp): suppress unhandled rejection from async gateway event handler#100116

Closed
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/channel-bridge-void-async-catch
Closed

fix(mcp): suppress unhandled rejection from async gateway event handler#100116
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/channel-bridge-void-async-catch

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

The onEvent callback in OpenClawChannelBridge calls void this.handleGatewayEvent(event) without observing the returned promise. Since handleGatewayEvent is async and can reject, this creates an unhandled promise rejection that crashes the MCP gateway process.

Why This Change Was Made

Contain the async handler rejection and emit a low-noise diagnostic, matching the existing notification-failure observability pattern in the same bridge (src/mcp/channel-bridge.ts:399). Failed session or approval events are therefore visible to operators instead of disappearing silently.

User Impact

Prevents the gateway from crashing when an MCP channel bridge event handler throws, while keeping failures observable.

Evidence

Behavior

The onEvent callback now delegates to dispatchGatewayEvent, which awaits handleGatewayEvent inside a try/catch. On rejection it writes exactly one stderr record (openclaw mcp: gateway event <type> failed), with the full error shown only when --verbose is on.

Source

src/mcp/channel-bridge.ts:150

onEvent: (event) => {
  void this.dispatchGatewayEvent(event);
},

src/mcp/channel-bridge.ts:524

private async dispatchGatewayEvent(event: EventFrame): Promise<void> {
  try {
    await this.handleGatewayEvent(event);
  } catch (error) {
    // Always surface a single low-noise record so swallowed gateway event
    // failures remain observable; the spammy error detail stays behind --verbose.
    process.stderr.write(`openclaw mcp: gateway event ${event.event} failed\n`);
    if (this.verbose) {
      process.stderr.write(
        `openclaw mcp: gateway event ${event.event} error: ${String(error)}\n`,
      );
    }
  }
}

Unit test output

$ pnpm test -- src/mcp/channel-bridge.test.ts --run
[test] starting test/vitest/vitest.unit.config.ts
 ✓ |unit| src/mcp/channel-bridge.test.ts (18 tests) 154ms

 Test Files  1 passed (1)
      Tests  18 passed (18)
   Duration  1.12s

[test] passed 1 Vitest shard in 10.50s

Real runtime proof

I ran a real OpenClaw MCP bridge against the local gateway (ws://127.0.0.1:18789) and forced handleGatewayEvent to reject for session.message events. The bridge stayed alive and emitted the expected diagnostic on stderr:

openclaw mcp: gateway event session.message failed
openclaw mcp: gateway event session.message error: Error: injected proof rejection
openclaw mcp: gateway event session.message failed
openclaw mcp: gateway event session.message error: Error: injected proof rejection
openclaw mcp: gateway event session.message failed
openclaw mcp: gateway event session.message error: Error: injected proof rejection

The injection was removed before committing; the committed code only logs real failures.

Regression coverage

  • a rejected gateway event still emits exactly one diagnostic record with verbose off
  • a rejected gateway event includes error detail with verbose on

Both tests force handleGatewayEvent to reject, call dispatchGatewayEvent, and assert the exact stderr output shape while confirming the error detail is redacted unless verbose is enabled.

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 7:55 AM ET / 11:55 UTC.

Summary
The PR routes MCP Gateway events through a catch-logging dispatcher and adds regression tests for verbose and non-verbose rejected event diagnostics.

PR surface: Source +15, Tests +50. Total +65 across 2 files.

Reproducibility: yes. from source inspection rather than a local run. Current main passes an async MCP handler to a void GatewayClient callback, BaseGatewayClient only catches synchronous throws, and OpenClaw exits on ordinary unhandled rejections.

Review metrics: none identified.

Root-cause cluster
Relationship: canonical
Canonical: #100116
Summary: This PR is the narrow canonical landing attempt for the MCP Gateway event-handler rejected-promise bug; the older same-author attempt is closed, and the maintainer batch overlaps but has not merged.

Members:

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

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] Rejected MCP Gateway session and approval event handlers will now be logged and the individual failed event will not be retried or replayed; maintainers need to accept that log-and-continue behavior.
  • [P1] The open maintainer batch at fix: harden small runtime and installer edge cases #100258 carries the same MCP fix, so maintainers should choose one landing path to avoid duplicate work.

Maintainer options:

  1. Accept scoped MCP containment (recommended)
    Merge the focused PR if maintainers accept that a rejected handler is logged and the failed event is not replayed.
  2. Wait for the maintainer batch
    Keep this PR open while the batch proves and lands the same fix, then close this PR as superseded after merge.
  3. Move containment to GatewayClient
    Require a shared callback-policy change and sibling-consumer proof before landing this behavior.

Next step before merge

  • No automated repair is indicated; a maintainer should choose the landing path and accept or reject the scoped no-retry event-containment semantics.

Maintainer decision needed

  • Question: Should MCP gateway event-handler rejection containment land through this focused PR, through the broader open maintainer batch, or move into a shared GatewayClient callback policy first?
  • Rationale: The patch is correct, but the merge choice includes event-delivery semantics and an overlapping maintainer batch, which automation should not decide alone.
  • Likely owner: steipete — He is assigned on this PR and authored the overlapping maintainer batch carrying the same MCP fix.
  • Options:
    • Land the focused MCP PR (recommended): Merge this narrow PR after normal maintainer approval if log-and-continue without retry is the desired containment behavior.
    • Let the batch supersede it: Keep this PR open until fix: harden small runtime and installer edge cases #100258 merges, then close this PR as superseded with contributor credit preserved.
    • Define shared callback policy first: Pause this PR if maintainers want async onEvent rejection containment enforced in GatewayClient for all consumers rather than only in the MCP bridge.

Security
Cleared: The diff only changes local MCP promise rejection handling and focused tests; it does not touch dependencies, workflows, secrets, permissions, packaging, or supply-chain surfaces.

Review details

Best possible solution:

Land exactly one version of the MCP event containment fix; prefer this focused PR if maintainers accept scoped no-retry semantics, otherwise let #100258 supersede it after that batch merges.

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

Yes, from source inspection rather than a local run. Current main passes an async MCP handler to a void GatewayClient callback, BaseGatewayClient only catches synchronous throws, and OpenClaw exits on ordinary unhandled rejections.

Is this the best way to solve the issue?

Yes. Catching at the MCP bridge boundary is the narrow maintainable fix because GatewayClient currently declares onEvent as void; a broader shared callback policy can be a separate maintainer-directed change.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 0fc29969d960.

Label changes

Label justifications:

  • P2: This is a focused MCP availability and event-handling bug fix with limited blast radius and no evidence of a current release-blocking outage.
  • merge-risk: 🚨 message-delivery: The diff changes failed MCP Gateway event handling to logged continuation without retry or replay for the failed session or approval event.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes copied live output from a real OpenClaw MCP bridge against a local Gateway with an injected rejected session.message handler, showing diagnostics while the bridge stayed alive.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied live output from a real OpenClaw MCP bridge against a local Gateway with an injected rejected session.message handler, showing diagnostics while the bridge stayed alive.
Evidence reviewed

PR surface:

Source +15, Tests +50. Total +65 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 16 1 +15
Tests 1 50 0 +50
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 66 1 +65

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; no scoped AGENTS.md owns src/mcp, and the root MCP/core ownership, best-fix, proof, and merge-risk guidance affected this review. (AGENTS.md:1, 0fc29969d960)
  • Current main still has unobserved async dispatch: OpenClawChannelBridge currently passes a void callback that calls the async handleGatewayEvent without observing its returned promise. (src/mcp/channel-bridge.ts:150, 0fc29969d960)
  • GatewayClient onEvent contract is synchronous: GatewayClientOptions types onEvent as returning void, and BaseGatewayClient only catches synchronous throws around opts.onEvent, so async rejection containment is not handled centrally today. (packages/gateway-client/src/client.ts:455, 0fc29969d960)
  • Unhandled rejection is process-affecting: The ordinary OpenClaw unhandledRejection handler logs and exits for unclassified rejected promises, matching the crash class the PR addresses. (src/infra/unhandled-rejections.ts:508, 0fc29969d960)
  • PR head implements the intended containment: The diff changes MCP onEvent to call dispatchGatewayEvent, catches handleGatewayEvent rejection, writes one low-noise failure line, and gates detailed errors behind verbose mode. (src/mcp/channel-bridge.ts:150, 233dfe8f887b)
  • Regression tests cover both diagnostic modes: The PR adds tests that force handleGatewayEvent to reject and assert the stderr shape with verbose off and on. (src/mcp/channel-bridge.test.ts:308, 233dfe8f887b)

Likely related people:

  • steipete: The current MCP bridge and GatewayClient event-contract lines trace to Peter Steinberger's current-main commit, he is assigned on this PR, and he authored the overlapping maintainer batch that carries the same fix. (role: recent area contributor and assigned reviewer; confidence: high; commits: 009b00f7bad1, 1d4247769255; files: src/mcp/channel-bridge.ts, src/mcp/channel-bridge.test.ts, packages/gateway-client/src/client.ts)
  • hansraj316: Hansraj Singh Thakur authored the merged MCP notification-failure diagnostic change whose low-noise stderr pattern this PR mirrors. (role: adjacent behavior contributor; confidence: high; commits: 5d6899c7317e; files: src/mcp/channel-bridge.ts, src/mcp/channel-bridge.test.ts)
  • Vincent Koc: The latest release commit that still contains the unfixed MCP handler path was authored by Vincent Koc, making him relevant for shipped-behavior provenance rather than as the likely bug owner. (role: release and adjacent runtime contributor; confidence: medium; commits: e085fa1a3ffd; files: src/mcp/channel-bridge.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-05T00:47:26.961Z sha 9a2fb19 :: needs real behavior proof before merge. :: [P2] Keep MCP gateway event failures observable
  • reviewed 2026-07-05T00:52:42.051Z sha 9a2fb19 :: needs real behavior proof before merge. :: [P2] Keep MCP gateway event failures observable
  • reviewed 2026-07-05T03:17:49.209Z sha b298fef :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-05T03:23:09.982Z sha b298fef :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-05T05:45:34.008Z sha b298fef :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T05:51:59.877Z sha b298fef :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T10:52:09.881Z sha b1f2f6e :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T11:22:40.404Z sha 77bddb0 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 5, 2026
@cxbAsDev

cxbAsDev commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Proof

Behavior

The onEvent callback in OpenClawChannelBridge now attaches a .catch(() => {}) to the fire-and-forget handleGatewayEvent(event) promise, preventing an unhandled rejection from crashing the MCP gateway process.

Source

src/mcp/channel-bridge.ts:151

onEvent: (event) => {
  void this.handleGatewayEvent(event).catch(() => {});
},

Test output

$ pnpm test -- src/mcp/channel-bridge.test.ts --run
[test] starting test/vitest/vitest.unit.config.ts
 ✓ |unit| src/mcp/channel-bridge.test.ts (16 tests) 312ms

 Test Files  1 passed (1)
      Tests  16 passed (16)
   Duration  1.83s

[test] passed 1 Vitest shard in 14.16s

Notes

  • This is a defensive fix; the callback is fire-and-forget by design.
  • The .catch(() => {}) pattern is already used elsewhere for similar fire-and-forget handlers.
  • No new tests were added because the failure mode is an unhandled rejection in external event plumbing; existing tests continue to pass.

@cxbAsDev

cxbAsDev commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@cxbAsDev

cxbAsDev commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@cxbAsDev

cxbAsDev commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 5, 2026
@steipete steipete self-assigned this Jul 5, 2026
@steipete steipete mentioned this pull request Jul 5, 2026
Merged
@cxbAsDev
cxbAsDev force-pushed the fix/channel-bridge-void-async-catch branch from b298fef to b1f2f6e Compare July 5, 2026 10:36
@cxbAsDev

cxbAsDev commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (8a8af77) to address the stale base after the recent history rewrite.

What I did:

  • Force-pushed the same two commits onto upstream/main.
  • Diff is unchanged: src/mcp/channel-bridge.ts + src/mcp/channel-bridge.test.ts, +66 / -1.
  • CI is re-running now.

About the earlier native-command-session-target QA Smoke failure:

  • That scenario (qa/scenarios/channels/native-command-session-target.yaml) tests Telegram's native /stop command targeting an active routed conversation session.
  • It is part of the channel-framework surface and does not intersect with the MCP bridge files changed here.
  • If it fails again after this rebase, I believe it is unrelated to this PR and would appreciate a re-run or waiver.

About the event-delivery semantics flagged by ClawSweeper:

  • The onEvent callback is fire-and-forget by contract.
  • Catching the rejection prevents the gateway process from crashing on a failed handler; the failed session/approval event is logged and dropped.
  • Retry/replay is intentionally out of scope for this scoped fix and can be added later if the product wants it.

Please let me know if anything else is needed to land this independently.

@cxbAsDev
cxbAsDev force-pushed the fix/channel-bridge-void-async-catch branch from b1f2f6e to 77bddb0 Compare July 5, 2026 11:01
@cxbAsDev
cxbAsDev force-pushed the fix/channel-bridge-void-async-catch branch from 77bddb0 to 233dfe8 Compare July 5, 2026 11:25
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Thank you for the contribution. This fix landed with contributor co-authorship preserved in #100258 (deac98eb7204fdb51589c5e369b95be128215e77).

I consolidated the source fixes into a maintainer takeover because the contributor branches were based on rewritten pre-main history; updating them directly would have pulled unrelated changes into the review surface. Closing this PR as superseded by the landed batch.

@steipete steipete closed this Jul 5, 2026
@cxbAsDev
cxbAsDev deleted the fix/channel-bridge-void-async-catch branch July 15, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cxbAsDev @steipete