AppServices container for hot-swappable database connections

[malibio]

Summary

• Introduces AppServices container with Arc<RwLock<>> interior mutability, replacing write-once app.manage() state for all runtime services
• All 13 command files updated to use State<'_, AppServices> with async accessor pattern (services.node_service().await?)
• Database switching now hot-swaps services at runtime via drain-then-replace protocol — no app restart required
• Frontend updated to handle database-changed events without restart dialog
• Documents embedding sync boundaries in sync-protocol.md (local commit before sync, subtree atomic sync units)

Closes #894

Test plan

• All 3586 frontend tests pass
• All Rust tests pass
• cargo clippy clean (zero warnings)
• svelte-check clean
• Manual test: app starts, initializes database, all commands work
• Manual test: select new database from settings, services hot-swap without restart
• Manual test: MCP server restarts on database switch
• Manual test: embedding commands return clear error if model fails to load

🤖 Generated with Claude Code

[malibio]

Code Review: PR #899 -- AppServices Container for Hot-Swappable Database Connections

Reviewer: Pragmatic Code Review (Claude Opus 4.6)
Branch: feature/issue-894-app-services-container
Issue: #894

---

Overall Assessment

This is a well-executed structural refactor that introduces a centralized AppServices container with Arc<RwLock<>> interior mutability, replacing Tauri's write-once app.manage() pattern for runtime services. The change is a clear net positive: it unlocks hot-swappable database connections, removes the forced app restart on database changes, and establishes a cleaner service access pattern across all 13 command files.

The code is consistent, the drain-then-replace protocol in switch_database() is thoughtfully ordered, and the frontend changes correctly remove the restart confirmation dialogs.

---

Requirements Validation (Issue #894 Acceptance Criteria)

Criterion	Status	Notes
AppServices container with Arc<RwLock<Option<ActiveServices>>>	PASS	app_services.rs:42-46
Single app.manage() call in lib.rs::setup()	PASS	lib.rs:276
All 40+ Tauri commands updated to use AppServices	PASS	All 13 command files updated
Graceful "no database" errors when inner is None	PASS	NOT_INITIALIZED error code returned
switch_database() with drain-then-replace	PASS	app_services.rs:173-232
Per-session CancellationToken	PASS	Child token from ShutdownToken
GPU resource cleanup in switch protocol	PASS	Ordered drop: processor -> pause -> release GPU
Tiered initialization (NodeService works if embeddings fail)	PARTIAL	See finding below
select_new_database updated for hot-swap	PASS	settings.rs:101-136
requires_restart removed from PendingDatabaseChange	PASS	Replaced with DatabaseSwitchResult
MCP server restarts on database switch	PASS	settings.rs:272-280
Domain event forwarder restarts	PASS	settings.rs:282-293
Frontend database-changed event	PASS	settings.rs:130, app-shell.svelte:297-299
Existing tests pass	PASS (per PR description)	3586 frontend + Rust tests

---

Findings

[Improvement] Tiered initialization not implemented in init_services() (db.rs)

File: /packages/desktop-app/src-tauri/src/commands/db.rs, lines 103-109
Issue: The acceptance criteria require tiered initialization where NodeService (Tier 1) works even if embedding model loading fails (Tier 2). The current init_services() still treats NLP engine initialization as a hard failure -- if EmbeddingService::new() or nlp_engine.initialize() fails, the entire init_services() returns Err and the app has no database at all.

Contrast this with switch_database_services() in settings.rs (lines 218-243) which correctly handles embedding failure gracefully by setting embedding_state = None.

Recommendation: Apply the same match pattern from switch_database_services() to init_services() so that Tier 1 (SurrealStore + NodeService) is populated in AppServices even when Tier 2 (embeddings) fails. This was explicitly called out in the issue:

If Tier 2 fails, Tier 1 services are still available. Embedding commands return a clear "embedding service unavailable" error.

This is the only acceptance criterion not fully met.

---

[Improvement] Duplicated service initialization logic between init_services() and switch_database_services()

Files: /packages/desktop-app/src-tauri/src/commands/db.rs (lines 59-181) and /packages/desktop-app/src-tauri/src/commands/settings.rs (lines 177-297)

The service creation sequence (SurrealStore -> NodeService -> EmbeddingService -> EmbeddingProcessor -> wire waker -> create session token -> populate container -> start background tasks) is duplicated between init_services() and switch_database_services(). These two functions do nearly identical work with slightly different error handling.

Recommendation: Consider extracting the shared service-creation logic into a helper (e.g., create_active_services(app, config) -> Result<(Arc<SurrealStore>, Arc<NodeService>, Option<EmbeddingState>), String>) and having both init_services and switch_database_services call it. This would:

1. Fix the tiered initialization gap automatically (one code path to maintain)
2. Reduce risk of the two paths diverging as the codebase evolves
3. Follow DRY principle

This is not a blocker -- the duplication is understandable for a first pass -- but it is a real maintenance hazard given that these paths will evolve together.

---

[Improvement] MCP server not restarted when embeddings are unavailable during hot-swap

File: /packages/desktop-app/src-tauri/src/commands/settings.rs, lines 271-281

if let Some(emb_svc) = embedding_service_arc {
    if let Err(e) = crate::initialize_mcp_server(
        app.clone(),
        node_service_arc.clone(),
        emb_svc,
        new_session_token.clone(),
    ) {

MCP server is only restarted if embeddings are available (if let Some(emb_svc)). If the NLP model fails to load during a switch, the MCP server silently stops and is never restarted. MCP should still be able to serve node queries even without embedding capabilities.

Recommendation: Refactor initialize_mcp_server() to accept Option<Arc<NodeEmbeddingService>> so MCP can operate in a degraded mode without embeddings, or at minimum add a tracing::warn! when MCP is skipped so the user is informed.

---

[Nit] Redundant error mapping in schemas.rs

File: /packages/desktop-app/src-tauri/src/commands/schemas.rs, lines 47-52 and 87-92

let service = services.node_service().await.map_err(|e| CommandError {
    message: e.message,
    code: e.code,
    details: e.details,
})?;

node_service() already returns Result<_, CommandError>. This .map_err() reconstructs an identical CommandError from itself. All other command files simply use .await? directly.

Recommendation: Replace with let service = services.node_service().await?; for consistency.

---

[Nit] database-changed event listener in app-shell.svelte only logs

File: /packages/desktop-app/src/lib/components/layout/app-shell.svelte, lines 297-299

unlistenDatabaseChanged = listen('database-changed', (event) => {
    log.info('Database changed to:', event.payload);
});

The comment says "Frontend views will refresh automatically via domain event forwarder restart" -- this is reasonable if the domain event forwarder triggers a full UI refresh. However, it might be worth adding a TODO noting that explicit view refresh logic may be needed if the automatic refresh proves insufficient during manual testing.

---

[Nit] DatabaseSwitchResult.success is always true

File: /packages/desktop-app/src-tauri/src/commands/settings.rs, lines 132-135

The success field is always set to true because any failure path returns Err(...). The frontend checks result.success but this will never be false. Consider removing the field and relying on the Result's Ok/Err semantics, or documenting that it exists for future partial-success scenarios.

---

Positive Observations

1. Clean accessor pattern. The services.node_service().await? pattern is ergonomic and consistent across all command files. The read-lock path is fast for the common case.

2. Ordered GPU cleanup. The switch_database() method in app_services.rs correctly drops the processor Arc before releasing the GPU context, with appropriate pauses. This directly addresses the Metal SIGABRT crash documented in App crashes on restart: ggml Metal residency set assertion failure #897.

3. Graceful shutdown integration. The release_gpu_resources() method and the block_on call in lib.rs for sync shutdown code is a pragmatic approach.

4. Frontend simplification. Removing the restart confirmation dialogs is a clear UX improvement. The settings UI correctly reloads after hot-swap.

5. Documentation. The sync-protocol.md additions about embedding sync boundaries are well-written and cover important edge cases.

6. Comment cleanup. Removing verbose doc comments that restated obvious parameter types reduces noise without losing important context.

---

Final Recommendation

APPROVE -- with a strong suggestion to address the tiered initialization gap in init_services() before or shortly after merge, as it is explicitly called out in the acceptance criteria. The duplicated initialization logic and MCP-without-embeddings issues are worth tracking as fast follow-ups but are not blockers for this PR.

The change is a definitive improvement to the codebase: it removes a fundamental architectural limitation (write-once state), establishes a clean service access pattern, and enables hot-swappable database connections. Net positive.

[malibio]

Pragmatic Code Review complete. Recommendation: APPROVE with suggestions. See detailed review comment above. The only acceptance criterion not fully met is tiered initialization in init_services() (NodeService should work even if embedding model loading fails).

[malibio]

Review Findings Addressed (df5f293)

All three important findings from the code review have been addressed in a single commit:

1. Tiered init inconsistency (FIXED)

init_services() in db.rs now handles NLP failure gracefully — sets embedding_state = None and logs a warning instead of killing the entire init. This matches the pattern already used by switch_database_services().

2. Duplicated service creation logic (FIXED)

Extracted create_service_bundle() helper in db.rs that both init_services() and switch_database_services() call. Both paths now share identical tiered-init logic, preventing divergence.

3. MCP server skipped without embeddings (FIXED)

Made embedding_service optional (Option<Arc<NodeEmbeddingService>>) throughout the MCP stack:

• McpServices.embedding_service → Option<Arc<...>>
• McpServerService::new() → accepts optional embedding service
• handle_initialize() → warmup is best-effort when embeddings available
• handle_tools_call() → search_semantic returns graceful error when embeddings unavailable
• initialize_mcp_server() in lib.rs → accepts Option<Arc<...>>
• MCP server now always starts after DB init/switch, serving node CRUD even without embeddings

Skipped (per review)

• Finding 4 (redundant map_err in schemas.rs) — correctly maps between two distinct CommandError types
• Finding 5 (DatabaseSwitchResult.success always true) — harmless for frontend consumption

Verification

• cargo clippy --all-targets — clean (0 warnings)
• bun run quality:fix — clean
• bun run rust:test — 788 tests passed
• All dev-tools binaries updated (dev-mcp, dev-proxy)