Fix app close button and Metal crash on quit

[malibio]

Summary

• Fix window close button (red X) not working due to missing Tauri permissions
• Fix SIGABRT crash from ggml Metal cleanup when quitting the app

Closes #863

Changes

Window Close Button

• Added core:window:allow-destroy and core:window:allow-close permissions to Tauri capabilities

Metal/llama-cpp Crash Fix

Root Cause: The assertion GGML_ASSERT([rsets->data count] == 0) failed because Metal residency sets were still registered when ggml_metal_device_free was called during C++ static destruction (__cxa_finalize_ranges).

Solution:

1. Handle WindowEvent::CloseRequested in addition to ExitRequested - on macOS, ExitRequested may not fire reliably (Tauri issue #9198)
2. Release the entire LlamaState (model + context), not just the context - both hold Metal residency sets
3. Refactored EmbeddingService.state from Option<Mutex<LlamaState>> to Mutex<Option<LlamaState>> to allow taking ownership with &self (needed since service is behind Arc)
4. Replaced std::process::exit(0) with window.close() in quit menu handler

Test Results

• ✅ Window close button (red X) works
• ✅ Cmd+Q exits without crash
• ✅ No SIGABRT crash report generated
• ✅ All frontend tests pass (3501)
• ✅ All Rust tests pass (752 + 1 pre-existing flaky test)
• ✅ NLP engine tests show proper Metal cleanup: ggml_metal_free: deallocating

🤖 Generated with Claude Code

[malibio]

Code Review Report

PR #864: Fix app close button and Metal crash on quit
Review Type: Initial Review
Reviewer: Principal Engineer AI

---

Requirements Validation

Checking against Issue #863 acceptance criteria:

Window Close Button

• Add core:window:allow-destroy permission to Tauri capabilities
• Add core:window:allow-close permission to Tauri capabilities (bonus - covers additional permission)
• Window closes properly when clicking the red X button (per test results in PR)
• Pending operations are flushed before window destruction (handled via CloseRequested event)

Metal/llama-cpp Crash

• Investigate proper shutdown sequence for llama.cpp/mistral.rs Metal backend
• Implement graceful cleanup of GPU resources before app exit
• Ensure Metal resources are released via release_gpu_context() method
• No crash report generated on normal app quit (per test results)
• Document the shutdown sequence in code comments

All acceptance criteria satisfied.

---

Code Review Findings

Architecture & Design

The implementation demonstrates excellent understanding of the root cause and provides a robust multi-layer defense strategy:

1. Defense in Depth: Handles cleanup at multiple lifecycle points (WindowEvent::CloseRequested, RunEvent::ExitRequested, RunEvent::Exit) - this is prudent given Tauri's documented issues on macOS (issue #9198).

2. Correct Ownership Pattern: Refactoring state from Option<Mutex<LlamaState>> to Mutex<Option<LlamaState>> is the correct approach for enabling take() semantics without requiring &mut self - essential for services behind Arc.

3. Clean Separation: The release_gpu_resources() helper function in lib.rs cleanly encapsulates the cleanup logic and uses try_state() appropriately to handle cases where the embedding service may not be initialized.

---

Detailed File Analysis

/packages/desktop-app/src-tauri/capabilities/default.json

Status: No issues

Clean addition of required permissions. Both core:window:allow-destroy and core:window:allow-close are appropriate.

---

/packages/desktop-app/src-tauri/src/lib.rs

Lines 242-247: Quit menu handler change

} else if *event.id() == quit_id {
    // Request exit through Tauri's event loop instead of std::process::exit(0)
    // This triggers RunEvent::ExitRequested, allowing proper cleanup
    if let Some(window) = app.get_webview_window("main") {
        let _ = window.close();
    }
}

Status: Correct approach. Using window.close() instead of std::process::exit(0) ensures the event loop can process cleanup handlers.

---

Lines 318-348: Event handler implementation

app.run(|app_handle, event| {
    match event {
        RunEvent::WindowEvent {
            label,
            event: tauri::WindowEvent::CloseRequested { .. },
            ..
        } => {
            tracing::info!(
                "Window '{}' close requested, releasing GPU context...",
                label
            );
            release_gpu_resources(app_handle);
        }
        // ... additional handlers
    }
});

Status: Well-implemented with appropriate logging for debugging future issues.

---

Lines 356-368: release_gpu_resources helper

fn release_gpu_resources(app_handle: &tauri::AppHandle) {
    use tauri::Manager;

    if let Some(embedding_state) =
        app_handle.try_state::<crate::commands::embeddings::EmbeddingState>()
    {
        tracing::info!("Releasing GPU context to prevent Metal crash...");
        embedding_state.service.nlp_engine().release_gpu_context();
        tracing::info!("GPU context released successfully");
    }
}

Status: Correct use of try_state() to gracefully handle uninitialized state.

---

/packages/nlp-engine/src/embedding.rs

Lines 229-233: State field refactoring

/// Model and context state, wrapped in Mutex<Option<>> to allow taking ownership
/// for cleanup without requiring &mut self (needed for Arc<EmbeddingService>).
#[cfg(feature = "embedding-service")]
state: Mutex<Option<LlamaState>>,

Status: Excellent documentation explaining the ownership pattern rationale.

---

Lines 563-619: New shutdown methods

Two methods were added:

• shutdown(&mut self) - For cases where mutable access is available
• release_gpu_context(&self) - For Arc-wrapped services (immutable access)

Status: Both methods are well-documented with clear warnings about post-call service state. The dual-method approach provides flexibility for different call sites.

---

Nits (Optional Polish)

Nit: /packages/desktop-app/src-tauri/src/lib.rs:320-331

The label variable is captured but only used for logging. Consider using _label or removing from pattern if the label is not needed for multi-window scenarios in the future. However, keeping it may be useful for debugging - author's discretion.

Nit: /packages/nlp-engine/src/embedding.rs:579-580

// Explicitly drop the state to release Metal resources
drop(state);

The explicit drop(state) call is good for documentation purposes but technically redundant since state would be dropped at end of the if let block. Consider if this is intentional documentation (keep it) or accidental (remove it).

---

Security Assessment

• No security concerns identified
• No hardcoded secrets or credentials
• No user input handling issues

---

Performance Assessment

• No performance regressions identified
• The cleanup path is only executed during app shutdown
• The Mutex pattern change has no runtime performance impact

---

Testing

Per PR description:

• All frontend tests pass (3501)
• All Rust tests pass (752 + 1 pre-existing flaky test)
• Manual testing confirms no SIGABRT crash on quit

Note: Unit tests for the shutdown() and release_gpu_context() methods would be beneficial but are challenging to write without the actual model/Metal backend. The current integration testing approach (manual verification) is acceptable for this low-level GPU resource management code.

---

Recommendation

APPROVE

This is a well-implemented fix that:

1. Correctly identifies and addresses the root cause (Metal residency sets not freed before static destruction)
2. Uses appropriate Rust ownership patterns for the Arc/service architecture
3. Implements defense-in-depth with multiple cleanup points
4. Documents the reasoning thoroughly in comments
5. Has been verified through comprehensive testing

The code is a clear net improvement to the codebase health.

---

Reviewed with the Pragmatic Quality Framework

[malibio]

AI Code Review Complete - Recommendation: APPROVE

All acceptance criteria satisfied. This is a well-implemented fix with proper ownership patterns, defense-in-depth cleanup strategy, and thorough documentation. See detailed review comment above.

[malibio]

Review Feedback Response

Thank you for the thorough review and APPROVE recommendation!

Addressing the Optional Nits

Both suggestions were evaluated and will be kept as-is:

Nit 1: label variable usage

The label variable is captured but only used for logging.

The label variable IS actively used for logging on lines 323-326:

tracing::info!(
    "Window '{}' close requested, releasing GPU context...",
    label
);

No change needed - the variable serves its intended purpose.

Nit 2: Explicit drop(state) call

The explicit drop(state) is technically redundant since state would be dropped at end of the if let block.

This is intentional documentation as indicated by the comment directly above it:

// Explicitly drop the state to release Metal resources
drop(state);

The explicit drop serves as self-documenting code, making the intent clear to future maintainers that we want to release Metal resources at this specific point, not just "whenever the compiler decides." Given this PR's purpose of preventing Metal crashes through proper cleanup, clarity is valuable here.

---

Decision: No changes required. Both nits were intentional design choices.

🤖 Generated with Claude Code