[malibio]

Summary

• Root cause: When macOS terminates the app via NSApplication terminate: (Cmd+Q after long session, Dock quit, system shutdown), the exit() path bypasses Tauri's RunEvent handlers entirely. C++ static destructors (__cxa_finalize_ranges) then destroy the Metal device while our LlamaModel still holds Metal residency sets, triggering ggml_metal_rsets_free assertion failure (SIGABRT).
• Fix: Register a libc::atexit handler when the embedding model is first loaded. atexit handlers run before __cxa_finalize_ranges, so we drop all LlamaState instances (freeing Metal residency sets) and the LlamaBackend before C++ static destructors run.
• Architecture: Each EmbeddingService registers its Arc<Mutex<Option<LlamaState>>> in a global LLAMA_STATES vec. The atexit handler iterates and clears them all. Graceful shutdown still works (its .take() runs first; atexit's .take() returns None).

Test plan

• cargo check — compiles clean
• cargo clippy — no warnings
• bun run rust:test — 788 passed, 0 failed
• Atexit handler confirmed working in test output: [atexit] LlamaState dropped x3 + [atexit] LlamaBackend dropped
• Manual: launch app, wait for NLP model load, Cmd+Q — no crash (exit code 0)

Closes #905

🤖 Generated with Claude Code

[malibio]

Code Review Summary

Verdict: APPROVE (posted as comment — GitHub blocks self-approval)

This is a well-targeted, narrowly scoped fix for a real crash (SIGABRT in ggml_metal_rsets_free) that occurs when NSApplication terminate: bypasses Tauri's RunEvent handlers and the exit() path goes straight to C++ static destructors. The solution — registering a libc::atexit handler that drops Metal-holding state before __cxa_finalize_ranges runs — is the correct approach for this class of problem.

The change is a clear net positive: it fixes a crash with minimal surface area, introduces no regressions to graceful shutdown, and the code is well-documented.

---

Findings

No Critical Issues

None. The architecture is sound and the implementation is correct.

---

Suggested Improvements

1. [Improvement] Unbounded growth of LLAMA_STATES registry
packages/nlp-engine/src/embedding.rs, lines 98-103:

register_state_for_cleanup pushes a new Arc clone every time initialize() is called. If an EmbeddingService is shutdown and re-initialized (e.g., during database hot-swap via app_services.rs:214), the old Arc stays in the vec (its inner Option is None, so the atexit handler harmlessly .take()s None). Over many hot-swaps this leaks Arc entries.

For a desktop app with infrequent hot-swaps this is negligible, but a one-line cleanup would make it bulletproof:

fn register_state_for_cleanup(state: &Arc<Mutex<Option<LlamaState>>>) {
    let mut states = LLAMA_STATES.lock().unwrap_or_else(|p| p.into_inner());
    // Prune already-cleared entries from previous shutdown cycles
    states.retain(|s| s.lock().unwrap_or_else(|p| p.into_inner()).is_some());
    states.push(Arc::clone(state));
}

Not a blocker -- the current code is correct, just not frugal with stale entries.

2. [Improvement] Registration order: register_state_for_cleanup before register_atexit_handler
packages/nlp-engine/src/embedding.rs, lines 390-391:

Currently the state is registered first (line 390), then the atexit handler is registered (line 391). If -- in a truly pathological race -- exit() were called between these two lines, the state would be registered but no atexit handler would exist to clean it up. Reversing the order (register handler first, then state) closes this theoretical window:

register_atexit_handler();
register_state_for_cleanup(&self.state);

Extremely unlikely in practice, but trivial to fix and makes the ordering obviously correct.

---

Nitpicks

Nit: packages/nlp-engine/src/embedding.rs, lines 80, 88 -- The eprintln! calls in the atexit handler are appropriate for a last-resort cleanup path (tracing may be torn down), so this is fine. Just noting for future readers: these are intentionally eprintln! not tracing::info!.

Nit: packages/nlp-engine/Cargo.toml, line 30 -- libc is added as an unconditional dependency but only used under #[cfg(feature = "embedding-service")]. Consider making it optional and gating it on the feature:

libc = { version = "0.2", optional = true }

[features]
embedding-service = ["llama-cpp-2", "libc"]

This avoids pulling libc when the crate is compiled without embedding-service. Minor -- libc is lightweight and likely already in the dep tree.

---

Summary Table

Category	Status
Architecture	Clean -- atexit is the correct mechanism for this problem
Correctness	Correct -- idempotent with graceful shutdown path
Security	N/A
Maintainability	Good -- excellent comments explaining the "why"
Testing	Verified via rust tests (788 passed) and manual Cmd+Q
Performance	No impact -- atexit registration is O(1), cleanup is O(n) at exit
Dependencies	libc is standard, well-maintained, MIT licensed

[malibio]

Review Feedback Addressed

✅ Addressed (2/3 recommendations)

1. 🟢 Stale entry pruning — Added states.retain(|s| s.lock()...is_some()) in register_state_for_cleanup() to prune already-taken states, preventing unbounded growth from repeated initialize() calls.

2. 🟢 Registration order — Swapped register_atexit_handler() before register_state_for_cleanup() in initialize() so the atexit handler is always registered before any state is tracked.

⏭️ Skipped (1/3)

3. 🟢 Conditional libc dependency — libc is already transitively included via llama-cpp-2, and the #[cfg(feature = "embedding-service")] guards on all atexit code already prevent it from compiling when disabled. Adding conditional dependency gating provides no practical benefit.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Both changes are trivial — a one-line retain() call and a line swap. No logic, architecture, or behavior changes. All quality checks pass.