Skip to content

fix: keep undici terminated exceptions non-fatal#100098

Closed
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/100094-undici-terminated-classifier
Closed

fix: keep undici terminated exceptions non-fatal#100098
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/100094-undici-terminated-classifier

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #100094.

Bare undici response-body aborts can surface as TypeError("terminated"). The shared uncaught-exception classifier already handles several transient network shapes, but that bare undici shape was still routed through the fatal uncaught-exception path.

Why This Change Was Made

This adds a narrow match for an actual TypeError instance whose normalized message is exactly terminated, inside the existing benign network uncaught-exception classifier. The existing error graph traversal covers wrapped causes, while the exact type/message guard keeps generic Error("terminated") and broader "terminated ..." messages on the fatal path.

User Impact

Gateway processes can continue after this specific transient undici abort instead of exiting or restarting. Other uncaught exceptions keep the existing fatal behavior.

Evidence

  • node scripts/run-vitest.mjs src/infra/unhandled-rejections.test.ts src/infra/unhandled-rejections.fatal-detection.test.ts
  • git diff --check
  • Local autoreview: clean, no accepted/actionable findings

AI assistance: drafted and validated by Codex in the OpenClaw OSS sweep.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 5:34 PM ET / 21:34 UTC.

Summary
The PR adds exact TypeError("terminated") handling to the benign uncaught network exception classifier and adds regression coverage for direct and wrapped terminated errors.

PR surface: Source +10, Tests +8. Total +18 across 2 files.

Reproducibility: yes. for the source-level path: current main routes uncaught exceptions through isBenignUncaughtExceptionError, and the current classifier does not accept bare TypeError("terminated"). I did not reproduce the reporter's long-running live gateway crash locally.

Review metrics: 1 noteworthy metric.

  • Benign uncaught exception shapes: 1 added, 0 removed. The diff changes process-level fatality policy for one exact dependency error shape, which maintainers should notice before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #100094
Summary: This PR is the candidate fix for the linked gateway TypeError("terminated") crash; older and adjacent terminated-error items cover related but not identical surfaces.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted real behavior proof showing the patched gateway keeps running after the exact TypeError("terminated") path, using logs, terminal output, or a live validation transcript.
  • Move the PR out of draft once the contributor considers it ready for maintainer review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides focused test commands and diff checking, but no redacted live gateway run, logs, terminal output, or other after-fix real behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] No redacted real gateway run, logs, terminal output, or validation transcript yet shows the patched uncaught TypeError("terminated") path continuing instead of exiting.
  • [P1] The patch deliberately makes exact process-level TypeError("terminated") non-fatal, so maintainers may want an undici stack or cause guard before accepting that availability tradeoff.

Maintainer options:

  1. Require real gateway proof before merge (recommended)
    Ask for a redacted terminal transcript, logs, or live run showing the patched gateway handles TypeError("terminated") as non-fatal in a real setup.
  2. Tighten the classifier if provenance matters
    If exact type/message matching is too broad, require undici stack or cause evidence while preserving coverage for the reported crash shape.
  3. Accept the exact-shape availability tradeoff
    Maintainers may intentionally accept the current exact TypeError("terminated") policy because the dependency source and reports point to undici response-body aborts.

Next step before merge

  • [P1] The remaining action is contributor real-behavior proof plus maintainer review of the exact non-fatal error-shape policy, not an automated repair.

Security
Cleared: The diff touches only classifier logic and tests, with no dependency, CI, secret, permission, or code-download surface added.

Review details

Best possible solution:

Land the narrow shared classifier fix after real behavior proof, while treating the linked issue's wedged-but-alive observations as separate follow-up diagnostics if they continue.

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

Yes for the source-level path: current main routes uncaught exceptions through isBenignUncaughtExceptionError, and the current classifier does not accept bare TypeError("terminated"). I did not reproduce the reporter's long-running live gateway crash locally.

Is this the best way to solve the issue?

Yes, with a proof gap: the shared benign uncaught-exception classifier is the narrow owner boundary because both entrypoints use it before exiting. Provider- or channel-specific catches would not cover escaped process-level exceptions, though maintainers can still choose a stricter undici provenance guard.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 27d6c8816721.

Label changes

Label justifications:

  • P1: The PR targets a current stable gateway crash that can interrupt real multi-channel agent work and lose in-flight activity.
  • merge-risk: 🚨 availability: The diff changes process-level uncaught-exception handling so an exact TypeError("terminated") no longer exits the gateway.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides focused test commands and diff checking, but no redacted live gateway run, logs, terminal output, or other after-fix real behavior proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +10, Tests +8. Total +18 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 0 +10
Tests 1 8 0 +8
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 18 0 +18

What I checked:

  • Current main fatal path: Current main installs uncaught-exception handlers that continue only when isBenignUncaughtExceptionError accepts the error; otherwise the process restores terminal state and exits. (src/index.ts:95, 27d6c8816721)
  • Current main classifier gap: Current main's benign uncaught network classifier checks selected codes and exact benign messages, but has no direct TypeError("terminated") branch. (src/infra/unhandled-rejections.ts:423, 27d6c8816721)
  • Patch implementation: The PR head adds UNDICI_TERMINATED_TYPE_ERROR_MESSAGE and returns true only for candidate instanceof TypeError whose normalized message is exactly terminated. (src/infra/unhandled-rejections.ts:424, b98c8c8c03b5)
  • Patch tests: The PR head adds positive coverage for direct and wrapped TypeError("terminated"), plus negative coverage for plain Error("terminated") and longer TypeError messages. (src/infra/unhandled-rejections.test.ts:367, b98c8c8c03b5)
  • Latest release still has the gap: The v2026.6.11 tag has the same fatal entrypoint and benign classifier shape without a terminated branch, matching the linked issue's stable-release report. (src/infra/unhandled-rejections.ts:442, e085fa1a3ffd)
  • Dependency contract: OpenClaw pins undici 8.5.0, and the undici 8.5.0 tarball source errors an aborted readable response body with new TypeError('terminated', { cause }) in onAborted. (package.json:2021, 27d6c8816721)

Likely related people:

  • joshavant: Authored the merged undici HTTP/2 teardown handling in the same process-level transient/benign classifier family. (role: adjacent area contributor; confidence: high; commits: d0f22ccf9723; files: src/infra/unhandled-rejections.ts, src/infra/unhandled-rejections.test.ts)
  • HemantSudarshan: Authored recent transient polling socket handling in src/infra/unhandled-rejections.ts, including benign uncaught network exception coverage. (role: recent classifier contributor; confidence: high; commits: db6951088a19; files: src/infra/unhandled-rejections.ts, src/infra/unhandled-rejections.test.ts)
  • akrimm702: Added the adjacent ws pre-handshake benign uncaught-exception classifier and tests near the changed logic. (role: recent benign-classifier contributor; confidence: medium; commits: f327073fb3f2; files: src/infra/unhandled-rejections.ts, src/infra/unhandled-rejections.test.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 (1 earlier review cycle)
  • reviewed 2026-07-04T21:27:42.969Z sha b98c8c8 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 4, 2026
@steipete steipete self-assigned this Jul 5, 2026
@steipete steipete mentioned this pull request Jul 5, 2026
Merged
@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
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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

2 participants

@harjothkhara @steipete