Skip to content

Fix SherpaOnnx finalizer crash with DllNotFoundException#745

Merged
steipete merged 4 commits into
openclaw:mainfrom
williamwwm:main
Jun 15, 2026
Merged

Fix SherpaOnnx finalizer crash with DllNotFoundException#745
steipete merged 4 commits into
openclaw:mainfrom
williamwwm:main

Conversation

@williamwwm

Copy link
Copy Markdown
Contributor

Suppress fatal crashes caused by SherpaOnnx.OfflineTts finalizer calling SherpaOnnxDestroyOfflineTts when native DLL is unavailable.

Changes:

  • PiperTextToSpeechClient: gracefully handle DllNotFoundException in constructor (TTS disabled gracefully), add GC.SuppressFinalize after dispose to prevent finalizer from calling native code during GC
  • App.xaml.cs: improved OnDomainUnhandledException logging for SherpaOnnx native DLL errors
  • Crash prevention relies on exe.config legacyUnhandledExceptionPolicy and GC.SuppressFinalize (IsTerminating is readonly in .NET 10)

Fixes repeated crashes: System.DllNotFoundException: Unable to load DLL 'sherpa-onnx-c-api' at SherpaOnnx.OfflineTts.Finalize()

Suppress fatal crashes caused by SherpaOnnx.OfflineTts finalizer
calling SherpaOnnxDestroyOfflineTts when native DLL is unavailable.

Changes:
- PiperTextToSpeechClient: gracefully handle DllNotFoundException
  in constructor (TTS disabled gracefully), add GC.SuppressFinalize
  after dispose to prevent finalizer from calling native code during GC
- App.xaml.cs: improved OnDomainUnhandledException logging for
  SherpaOnnx native DLL errors
- Crash prevention relies on exe.config legacyUnhandledExceptionPolicy
  and GC.SuppressFinalize (IsTerminating is readonly in .NET 10)

Fixes repeated crashes: System.DllNotFoundException: Unable to load
DLL 'sherpa-onnx-c-api' at SherpaOnnx.OfflineTts.Finalize()
# Conflicts:
#	src/OpenClaw.Tray.WinUI/App.xaml.cs
@clawsweeper

clawsweeper Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 7:22 AM ET / 11:22 UTC.

Summary
The PR preloads the Sherpa native library before constructing OfflineTts, suppresses OfflineTts finalization before best-effort disposal, and adds source-contract tests for that ordering.

Reproducibility: yes. for a source-level path: the related crash report captured DllNotFoundException during OfflineTts finalization, and current main still constructs the finalizable wrapper before any explicit native-load check. I did not reproduce the live Windows crash in this read-only review.

Review metrics: 1 noteworthy metric.

  • Changed Surface: 2 files changed, 75 additions, 0 deletions. The PR is narrow, but one changed file is a native loading/finalizer path that ordinary source-contract tests do not prove at runtime.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
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 Windows runtime proof for the Piper native-load/finalizer failure path, such as tray logs, Event Viewer output, terminal output, or a recording of Piper preview/tts.speak failing without a crash.
  • [P1] Report the required ./build.ps1, shared test, and tray test results for the final head.
  • Update the PR body so it matches the final two-file diff and remove stale App.xaml.cs/constructor-catch claims.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No PR body or comment evidence shows the changed behavior in a real Windows setup after the fix; the contributor should add redacted logs, terminal output, screenshot, or video proof and update the PR body to trigger re-review.

Risk before merge

  • [P1] No after-fix proof shows a real Windows tray/Piper preview or tts.speak path with sherpa-onnx-c-api missing or unloadable failing without a tray crash.
  • [P1] The final head has no reported build or test checks, and the new tests assert source text rather than exercising the native loading/finalizer behavior.
  • [P1] The PR body still describes earlier App.xaml.cs logging and constructor catch behavior that is not present in the final combined diff.

Maintainer options:

  1. Require Windows Native Proof (recommended)
    Ask for redacted terminal, log, screenshot, or recording evidence that the packaged tray/Piper path fails gracefully without crash when Sherpa native loading is broken, plus the required validation results.
  2. Accept Source-Review Risk
    Maintainers can merge the narrow native-loader/finalizer hardening based on source review while owning the remaining Windows runtime uncertainty.
  3. Pause If Packaging Fix Is Enough
    If PR Fix tray crash-loop: source x64 VC++ runtime from VS install at publish time #713 is considered sufficient for the real-world crash, pause this mitigation until a remaining live repro is shown.

Next step before merge

  • [P1] Human review is needed because the blocker is missing contributor runtime proof and validation evidence, not a narrow code repair ClawSweeper can safely generate.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds a constrained native library load and finalizer-order change without new dependencies, scripts, permissions, or secrets handling.

Review details

Best possible solution:

Merge a narrow Piper finalizer mitigation only after redacted Windows runtime proof and the repository-required build/shared/tray validation are available for the final head.

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

Yes for a source-level path: the related crash report captured DllNotFoundException during OfflineTts finalization, and current main still constructs the finalizable wrapper before any explicit native-load check. I did not reproduce the live Windows crash in this read-only review.

Is this the best way to solve the issue?

Mostly yes: preloading the native library before wrapper construction and suppressing finalization before best-effort cleanup is a narrow mitigation. The solution still needs real Windows proof because the added tests only inspect source ordering.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P1: This PR targets a fatal tray crash path in Piper/Sherpa TTS, but current main already contains the broader packaging crash fix so it is urgent hardening rather than a P0 emergency.
  • add merge-risk: 🚨 availability: The diff changes Windows native library loading and finalization behavior for Piper TTS, and green source tests alone would not prove the tray no longer crashes or loses TTS availability in the packaged runtime.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No PR body or comment evidence shows the changed behavior in a real Windows setup after the fix; the contributor should add redacted logs, terminal output, screenshot, or video proof and update the PR body to trigger re-review.
  • remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P1: This PR targets a fatal tray crash path in Piper/Sherpa TTS, but current main already contains the broader packaging crash fix so it is urgent hardening rather than a P0 emergency.
  • merge-risk: 🚨 availability: The diff changes Windows native library loading and finalization behavior for Piper TTS, and green source tests alone would not prove the tray no longer crashes or loses TTS availability in the packaged runtime.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab 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: No PR body or comment evidence shows the changed behavior in a real Windows setup after the fix; the contributor should add redacted logs, terminal output, screenshot, or video proof and update the PR body to trigger re-review.
Evidence reviewed

What I checked:

Likely related people:

  • shanselman: Merged and co-authored the recent PR that fixed the Sherpa/ONNX VC runtime crash-loop packaging root cause. (role: recent crash-fix merger and area contributor; confidence: high; commits: 8bf605cdbd32, 4ff5166d165f, 4eb848b09bba; files: scripts/Test-ReleaseNativeDependencies.ps1, src/Directory.Build.targets, tests/OpenClaw.Tray.Tests/ReleaseSigningWorkflowTests.cs)
  • RBrid: Prior review evidence on the related crash report traced the Sherpa package reference and OfflineTts wrapper to this contributor's merged TTS work. (role: introduced related Sherpa/Piper TTS native dependency; confidence: medium; files: src/OpenClaw.Tray.WinUI/OpenClaw.Tray.WinUI.csproj, src/OpenClaw.Tray.WinUI/Services/TextToSpeech/PiperTextToSpeechClient.cs, src/OpenClaw.Shared/Audio/PiperVoiceManager.cs)
  • Ranjesh: Recently refactored brittle tray tests and added the shared repository-root helper that this PR's new source-contract test uses. (role: recent adjacent test contributor; confidence: medium; commits: d4284d43fb6a; files: tests/OpenClaw.Tray.Tests/TestRepositoryPaths.cs, tests/OpenClaw.Tray.Tests/ReleaseSigningWorkflowTests.cs, tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj)
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.

@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 12, 2026
This was referenced Jun 15, 2026
steipete and others added 2 commits June 15, 2026 07:07
Preload the Sherpa native library before constructing its finalizable wrapper and suppress finalization before best-effort cleanup.

Co-authored-by: suportewmit-cmyk <suportewmit@gmail.com>
@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. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 15, 2026
@steipete steipete merged commit aff45d0 into openclaw:main Jun 15, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P1 Urgent regression or broken agent/channel workflow affecting real users now. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants