fix: catch unhandled promise rejections in fire-and-forget async calls#102185
Open
zhangLei99586 wants to merge 1 commit into
Open
fix: catch unhandled promise rejections in fire-and-forget async calls#102185zhangLei99586 wants to merge 1 commit into
zhangLei99586 wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
channel: telegram
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 9:11 PM ET / 01:11 UTC. Summary PR surface: Source +4. Total +4 across 4 files. Reproducibility: yes. for the remaining blocker by source inspection: the PR catches inside Telegram remove while the shared helper catches remove and calls onError. I did not run a live failure-path probe, and the contributor has not provided after-fix proof for the unhandled-rejection claim. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Merge a narrow version that preserves the shared ack-removal onError/logAckFailure path, keeps only defensive catches that do not hide existing diagnostics, and includes redacted live output, terminal output, logs, or a recording proving rejected background work is handled. Do we have a high-confidence way to reproduce the issue? Yes for the remaining blocker by source inspection: the PR catches inside Telegram remove while the shared helper catches remove and calls onError. I did not run a live failure-path probe, and the contributor has not provided after-fix proof for the unhandled-rejection claim. Is this the best way to solve the issue? No. The Feishu catches are a plausible defensive mitigation, but the Telegram change should let the shared helper catch removal failures or explicitly route them through the same onError/logAckFailure path. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6630138598b3. Label changesLabel justifications:
Evidence reviewedPR surface: Source +4. Total +4 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (4 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
removed
the
triage: needs-pr-context
zhangLei99586
force-pushed
the
fix/unhandled-promise-rejections-fw-20260708
branch
from
July 8, 2026 13:45
7a79f54 to
65d4bb3
Compare
openclaw-barnacle
Bot
added
channel: nostr
clawsweeper
Bot
removed
rating: 🧂 unranked krab
zhangLei99586
force-pushed
the
fix/unhandled-promise-rejections-fw-20260708
branch
from
July 8, 2026 13:49
65d4bb3 to
a9f355b
Compare
openclaw-barnacle
Bot
added
size: XS
and removed
channel: nostr
clawsweeper
Bot
added
rating: 🧂 unranked krab
Adds .catch() handlers to five async call sites where Promise rejections were silently discarded via `void`, creating unhandled rejection risks: - gmail-ops.ts: startGmailWatch renew timer callback - feishu/monitor.bot-identity.ts: retryBotIdentityProbe fire-and-forget - feishu/comment-handler.ts: cleanupTypingReaction fire-and-forget - feishu/comment-dispatcher.ts: typingReaction.cleanup fire-and-forget - telegram/bot-message-dispatch.ts: reactionApi remove promise chain Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
zhangLei99586
force-pushed
the
fix/unhandled-promise-rejections-fw-20260708
branch
from
July 9, 2026 00:59
a9f355b to
3c58abd
Compare
9 tasks
|
This pull request has been automatically marked as stale due to inactivity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Fire-and-forget async calls with
voidkeyword silently discard Promise rejections. If the underlying async operation throws (network error, timeout, API failure), the rejected promise becomes an unhandled rejection, which Node.js may terminate the process on in future versions.Five affected call sites were identified across core and extension code.
Summary
void, creating unhandled rejection risks in production runtime..catch()handlers to each call site.Changes
src/hooks/gmail-ops.tsvoid startGmailWatch()→.catch()with error loggingextensions/feishu/src/monitor.bot-identity.tsvoid retryBotIdentityProbe()→.catch()with error loggingextensions/feishu/src/comment-handler.tsvoid cleanupTypingReaction()→.catch(() => undefined)extensions/feishu/src/comment-dispatcher.tsvoid typingReaction.cleanup()→.catch(() => undefined)extensions/telegram/src/bot-message-dispatch.ts.then(() => {})without.catch()→ added.catch(() => undefined)Evidence
Each change follows the same pattern: replace
void asyncCall()withvoid asyncCall().catch(handler)or add.catch()to an existing.then()chain. Changes are purely defensive — success path behavior is unchanged.src/hooks/gmail-ops.ts- adds error logging viadefaultRuntime.error()extensions/feishu/src/monitor.bot-identity.ts- adds error logging vialog.warn()extensions/feishu/src/comment-handler.ts- silently swallows (cleanup, non-critical)extensions/feishu/src/comment-dispatcher.ts- silently swallows (cleanup, non-critical)extensions/telegram/src/bot-message-dispatch.ts- silently swallows (best-effort)Test Plan
🤖 Generated with Claude Code