App crashes on restart: ggml Metal residency set assertion failure

[malibio]

Closes #896

[malibio]

Pragmatic Code Review -- PR #897

Summary

Verdict: Approve (posted as comment due to self-review restriction). This is a clean, well-scoped bugfix that mirrors the existing graceful shutdown pattern (from the quit/close path in lib.rs) into the restart path. The change is minimal, correct, and directly addresses the SIGABRT crash caused by Metal residency sets still being active when std::process::exit() runs.

Requirements Validation (Issue #896)

Acceptance Criterion	Status
restart_app gracefully shuts down embedding engine before restarting	Addressed
No SIGABRT / assertion failure on restart	Addressed
No crash report generated on restart	Follows from above
Normal quit (#885) also addressed if feasible	Already fixed in quit path; commit message notes this

Findings

No Critical Issues

The implementation is sound. The cleanup sequence in restart_app exactly mirrors the proven pattern from CloseRequested/ExitRequested handlers in lib.rs (lines 426-438, 439-447):

1. Cancel ShutdownToken (signal background tasks)
2. 50ms thread::sleep (let tasks exit loops)
3. release_gpu_resources() (release Metal/GPU state)
4. Then proceed (restart or exit)

The visibility change from fn to pub(crate) fn on release_gpu_resources is the correct scope -- it stays internal to the crate while allowing cross-module access from commands/settings.rs.

[Improvement] Extract shared shutdown helper (DRY)

settings.rs:134-146 / lib.rs:433-437: The same three-step shutdown dance (cancel token + 50ms sleep + release GPU) is now duplicated across two call sites. Consider extracting into a shared helper:

// lib.rs
pub(crate) fn graceful_shutdown(app: &AppHandle, shutdown_token: &ShutdownToken) {
    shutdown_token.cancel();
    std::thread::sleep(std::time::Duration::from_millis(50));
    release_gpu_resources(app);
}

This would make future changes to the shutdown sequence (adjusting timeout, adding cleanup steps) less error-prone. However, given that issue #894 (AppServices container with tiered shutdown) is already the planned comprehensive fix, this is acceptable to defer.

[Nit] Redundant inner import

settings.rs:132: use tauri::Manager; inside the function body is redundant -- Manager is already imported at module scope on line 7 (use tauri::{AppHandle, Manager};). Removing the inner import would clean up the function slightly.

Positive Observations

• The commit message is excellent -- explains root cause, fix mechanics, and relationship to #885.
• The doc comment on restart_app explaining the "why" behind the cleanup is valuable for future maintainers.
• The fix is appropriately scoped as a tactical patch; the strategic fix (#894) is tracked separately.

---

Reviewed by senior-architect-reviewer agent using the Pragmatic Quality framework.

[malibio]

Review Recommendations Addressed

✅ Addressed (2/2)

1. [Improvement] Extract shared shutdown helper (DRY)

• Created graceful_shutdown() in lib.rs that encapsulates: cancel token → 50ms pause → release GPU
• CloseRequested, ExitRequested, and restart_app all now call this single helper
• Bonus: ExitRequested now gets the 50ms pause it was previously missing

2. [Nit] Redundant inner use tauri::Manager

• Removed the inner import from restart_app — Manager is already imported at module scope (line 7)

⏭️ Skipped: 0

Summary

• 📝 1 commit: aaddf68e
• 🧪 Tests: 788 Rust passed, 3586 frontend passed (matches baseline)
• ✅ Quality checks: clean (clippy, fmt, eslint, svelte-check)

🤖 Generated with Claude Code