Fix: SIGABRT crash on app exit in production builds

[malibio]

Closes #884

[malibio]

Pragmatic Code Review: PR #885 - Fix SIGABRT crash on app exit

Overall Assessment

Verdict: Approve. This PR is a clear net positive. It addresses a real production crash (SIGABRT on macOS app exit) through three well-reasoned, complementary fixes that align precisely with the root cause analysis in issue #884. The implementation is clean, well-documented, and uses established patterns (RAII, CancellationToken, tokio::select!). No critical issues found.

---

Requirements Validation (Issue #884)

Acceptance Criterion	Status
panic = "abort" changed to panic = "unwind" in release profile	Met (Cargo.toml:36)
Background tasks receive shutdown signal before Tokio runtime drops	Met (CancellationToken plumbed to MCP server and domain event forwarder)
Global LLAMA_BACKEND explicitly cleaned up before process exit	Met (Mutex<Option<LlamaBackend>> with release_llama_backend())
Production build exits cleanly without SIGABRT	Requires manual verification
Dev mode continues to work correctly	Requires manual verification
No new test failures introduced	Requires CI verification

---

Findings

No Critical Issues

No blocking issues were identified.

Suggested Improvements

1. [Improvement] BackendGuard holds Mutex lock during model loading and context creation

• Files: packages/nlp-engine/src/embedding.rs:61-97, embedding.rs:311-322
• The BackendGuard RAII type holds the MutexGuard for the entire duration of model loading (LlamaModel::load_from_file) and context creation (model.new_context). Model loading involves disk I/O and GPU shader compilation -- potentially seconds of wall time. During this period, any concurrent call to get_or_init_backend() (e.g., from another thread) will block.
• The previous OnceLock approach allowed lock-free reads after initialization. Now, every embedding operation that needs a context recreation acquires the global backend lock.
• Practical impact for this project: Low. The EmbeddingService already serializes access through its own Mutex<Option<LlamaState>>, so contention on the backend lock is unlikely in practice. The model is loaded once at startup and the context is reused (Issue Optimize embedding processor performance - reuse LlamaContext #776). This is an acceptable tradeoff for explicit shutdown cleanup.
• Principle: Be aware that if the architecture ever moves to multiple independent EmbeddingService instances (e.g., different models), this global lock could become a bottleneck. The current design is fine for the single-model architecture.

2. [Improvement] No timeout on graceful shutdown

• File: packages/desktop-app/src-tauri/src/lib.rs:356-389
• The shutdown sequence cancels the token and then immediately calls release_gpu_resources. The background tasks are signaled to stop, but there is no await or timeout to confirm they have actually stopped before GPU resources are released. If a background task is in the middle of a database operation or network call when the token is cancelled, the tokio::select! branch may not fire instantly.
• Practical impact: This is mitigated by the fact that release_gpu_resources only touches the EmbeddingService state (a separate Mutex from the backend tasks), so there is no direct data race. The tasks will be cleaned up when the Tokio runtime drops. However, the issue description (Fix 2) mentioned "Optionally add a brief timeout to allow tasks to finish cleanup."
• Recommendation: Consider adding a small sleep (e.g., 50-100ms) after cancel() and before release_gpu_resources() in the CloseRequested handler to give tasks a chance to exit their loops cleanly. This is a nice-to-have, not a blocker.

3. [Improvement] ShutdownToken inner field is pub

• File: packages/desktop-app/src-tauri/src/lib.rs:172
• pub struct ShutdownToken(pub tokio_util::sync::CancellationToken) -- the inner CancellationToken is public. While this works, it means any code with access to Tauri state could cancel the token or create child tokens without going through a controlled API.
• Recommendation: Consider making the inner field private and providing specific methods (e.g., child_token(), cancel()). This is minor given the pre-release state of the project.

Nitpicks

Nit: packages/nlp-engine/src/embedding.rs:129 -- The explicit drop(backend) is redundant since guard.take() already moves the value out, and it would be dropped at the end of the if let block anyway. Not harmful, but slightly misleading since it suggests drop does something special here.

Nit: packages/desktop-app/src-tauri/src/lib.rs:152-158 -- Two TODO comments were removed from the MCP server error/panic handlers. While the issue mentions this is acceptable cleanup, these were reasonable future considerations (emitting Tauri events on MCP failure, automatic restart). If these are intentionally descoped, the removal is fine. Otherwise, they could be tracked as separate issues.

---

Architecture Assessment

The three-layer defense approach is well-designed:

1. panic = "unwind" -- The broadest fix. Prevents any panic during shutdown from immediately aborting. This alone likely resolves the crash, but is the least surgical solution.

2. CancellationToken -- The most architecturally sound fix. Uses tokio_util::sync::CancellationToken with child tokens, which is the idiomatic Tokio pattern for cooperative task cancellation. The token is managed via Tauri state (ShutdownToken), making it accessible to both the setup phase and the event handler. The use of child_token() for each background task is correct -- it prevents one task's cancellation from affecting others.

3. Mutex<Option<LlamaBackend>> -- The most targeted fix for the Metal/GPU crash. Replacing OnceLock with Mutex<Option<>> is the simplest pattern that enables explicit cleanup. The BackendGuard RAII wrapper is a clean abstraction that preserves the ergonomics of the old &'static LlamaBackend reference while allowing ownership transfer via take().

The cleanup ordering (contexts first, then backend) is correctly enforced through the sequential calls in release_gpu_resources. The redundant cancel() calls across CloseRequested, ExitRequested, and Exit events properly handle the unreliable macOS event delivery noted in Tauri issue #9198.

Security Assessment

No security concerns. The unsafe code in LlamaState (lifetime transmute at line 226) is pre-existing and not modified by this PR. The new release_llama_backend function uses safe Rust exclusively. The unwrap_or_else(|p| p.into_inner()) pattern for poisoned Mutex recovery is appropriate for shutdown scenarios where the alternative is crashing anyway.

Testing Assessment

The PR does not add new unit tests. The nature of this fix (process shutdown behavior, GPU resource cleanup, macOS-specific crash) makes it fundamentally difficult to unit test -- the crash only manifests in release builds during __cxa_finalize_ranges. The issue's testing approach (manual verification via cargo tauri build + macOS Console.app) is appropriate. The existing embedding tests (test_blob_conversion, test_service_creation, etc.) continue to validate the EmbeddingService API surface, which has not changed.

---

Summary: This is a well-executed fix for a real production crash. The three-layer approach provides defense in depth, the code is clean and well-documented, and the architectural decisions are sound. Approve with minor suggestions noted above.

[malibio]

Approving. This is a well-executed, three-layer fix for the SIGABRT crash on macOS app exit. The CancellationToken shutdown signaling, Mutex<Option> for explicit cleanup, and panic=unwind change are all sound and complementary. Minor improvement suggestions noted in the PR comment (shutdown timeout, pub field encapsulation) are non-blocking. Net positive change.

[malibio]

Review Recommendations Addressed

✅ Addressed (3)

1. Redundant drop(backend) — Replaced with guard.take().is_some() check; value drops naturally at scope end
2. ShutdownToken inner field is pub — Made private, added cancel() and child_token() methods. All call sites updated to use the API instead of .0. access
3. No timeout between shutdown signal and GPU cleanup — Added 50ms thread::sleep after cancel() in CloseRequested handler to let background tasks exit before GPU resource release

⏭️ Skipped (2)

4. BackendGuard holds Mutex during model loading — Reviewer explicitly noted this is acceptable for the current single-model architecture. No change needed.
5. Removed TODO comments — Intentionally descoped. These were speculative future work, not actionable items for this fix.

Summary

• ✅ Addressed: 3 recommendations
• ⏭️ Skipped: 2 recommendations (with justification)
• 📝 Commits: 1
• 🧪 Tests: 783 Rust passed, quality checks clean