Add EmbeddingsService gRPC: proto, daemon handler, Tauri proxy (closes #1135)

[malibio]

Summary

• Defines EmbeddingsService in the nodespace proto with 7 RPCs (GetEmbeddingStatus, SearchSemantic, RegenerateEmbedding, QueueEmbedding, TriggerBatchEmbed, GetStaleCount, BatchQueueEmbeddings)
• Implements the tonic handler in nodespaced wrapping NodeEmbeddingService and EmbeddingProcessor
• Replaces all AppServices direct calls in commands/embeddings.rs with thin gRPC proxy calls via GrpcClient
• GPU drain protocol (release_gpu_context) handled on daemon shutdown; EmbeddingState struct removed from AppServices

Test plan

• cargo build --bin nodespaced passes
• bun run test:all passes
• bun run quality:fix passes
• Semantic search end-to-end: Tauri UI → gRPC → nodespaced → response

🤖 Generated with Claude Code

[malibio]

Code Review Summary

This PR successfully delivers the EmbeddingsService gRPC migration described in #1135. The architecture is sound: the proto definition is clean, the tonic handler follows the established NodeServiceImpl pattern, and the Tauri command layer has been reduced to genuine thin proxies. The GPU drain protocol is correctly centralized in the daemon. The change is a clear net improvement in code organization and satisfies the stated goal (embeddings work when the UI window is closed).

Several issues need attention before merge, ranging from a hard startup failure to numeric type problems.

---

Findings

Critical Issues

[Critical/Blocker] build_services crashes the daemon if the NLP model is absent
/packages/daemon/src/main.rs lines 193–203

build_embedding_state is documented as non-fatal and returns None when the model is absent, but the None arm of the immediately following match returns a hard Err, aborting the daemon entirely. The original code comment in main.rs even said "Embedding service is wired by a follow-up issue; returns Unavailable until provided." The current behavior means a fresh install without a model file gets a crash-at-launch instead of a gracefully degraded daemon. The contract build_embedding_state advertises (None = model absent, carry on) is violated one call-site away.

Either:

1. Make the EmbeddingsServiceImpl field in ServiceBundle an Option and register the server conditionally (matching what GrpcClient::start already does in the Tauri path), or
2. Make build_embedding_state fatal (returning Result) and document that the model is a hard requirement for the standalone daemon.

Option 1 is more consistent with the Tauri-side behavior.

[Critical/Blocker] switch_database leaves the in-process gRPC server pointing at stale services
/packages/desktop-app/src-tauri/src/commands/settings.rs lines 179–237

switch_database_services calls AppServices::switch_database with the new embedding_service but never restarts or updates the GrpcClient. The GrpcClient managed state holds its own EmbeddingsServiceImpl backed by the old Arc<NodeEmbeddingService> and Arc<EmbeddingProcessor>. After a database switch, every search_roots, generate_root_embedding, etc. call proxies to the old database. This is a data-correctness bug: the user selects a new database but embedding queries still hit the previous one.

The fix is to either restart GrpcClient::start with the new service instances and app.manage the new client, or replace the in-process gRPC server's backing services (requires a shared Arc<Mutex<Option<EmbeddingsServiceImpl>>> swap). Since app.manage cannot be called twice on the same type, the simplest solution is to wrap GrpcClient in an Arc<RwLock<>> and expose an update_services method that swaps the inner server and reconnects.

---

Suggested Improvements

[Improvement] get_embedding_status and get_stale_count are identical — DRY violation
/packages/daemon/src/services/embeddings_service.rs lines 45–61 and 159–172

Both RPCs call store().get_stale_embedding_root_ids(None, 0, EmbeddingConfig::default().max_retries).await?.len() as i32 with identical logic. Extract a private async fn stale_count(&self) -> Result<i32, Status> helper and call it from both. This also avoids instantiating EmbeddingConfig::default() twice per invocation.

[Improvement] get_stale_count and stale_count in get_embedding_status truncate silently on overflow
/packages/daemon/src/services/embeddings_service.rs lines 55 and 169

usize as i32 wraps around at 2,147,483,648 items — unlikely but the prior PR (#1148) already addressed the analogous issue in ListSessionsResponse.count with a saturating cast. Apply the same treatment here: i32::try_from(count).unwrap_or(i32::MAX). The proto field types (int32) were addressed in the AgentSession PR; raising them to int64 here would be more correct but is a proto-breaking change that should be a deliberate decision.

[Improvement] node_from_proto silently drops nodes with unparseable timestamps
/packages/desktop-app/src-tauri/src/commands/embeddings.rs lines 33–50

created_at.parse().ok() and modified_at.parse().ok() return None on parse failure, causing filter_map in search_roots to silently exclude those nodes from results. A log warning at tracing::warn! would make the failure visible without changing the user-facing behavior. The existing pattern in node_service.rs where proto→model conversions happen in the daemon uses Default fallbacks for optional fields but does not silently drop entire records.

[Improvement] on_root_idle always returns true
/packages/desktop-app/src-tauri/src/commands/embeddings.rs lines 169–183

The old implementation returned true if queuing succeeded and could have been false if the node was already fresh (the original queued regardless, so the bool was always true there too). Now the return is unconditionally true even before the gRPC call succeeds, which means callers cannot distinguish "queued" from "embeddings unavailable". The frontend contract here may be intentional ("fire and forget"), but if the return type is bool it should mean something — consider returning Result<(), CommandError> like the other trigger commands, or documenting clearly that the bool is always true and exists only for frontend compatibility.

[Improvement] build_embedding_state in main.rs hardcodes the model filename
/packages/daemon/src/main.rs line 227

The filename nomic-embed-text-v1.5.Q8_0.gguf appears verbatim here but is already defined as EMBEDDING_MODEL_FILENAME in packages/desktop-app/src-tauri/src/constants.rs. The daemon does not depend on the desktop-app crate, so the constant cannot be shared directly — but it should at minimum live in nodespace-core or nodespace-nlp-engine as a shared constant rather than a string literal in the daemon binary. This is a cross-package consistency concern for the next refactor.

---

Nitpicks

Nit: queue_embedding and regenerate_embedding in embeddings_service.rs (lines 127–145 and 106–124) are semantically identical — both call queue_for_embedding. The distinction between "queue" and "regenerate" exists at the RPC level but is collapsed in the handler. A comment explaining the intent ("regenerate forces a fresh embedding even if one exists; queue only marks as stale") would clarify the future divergence point.

Nit: SearchSemanticRequest.threshold uses 0.0 as a sentinel for "use server default" (documented in the proto comment). Using a float sentinel for optionality is error-prone because a caller legitimately wanting a 0.0 threshold cannot express it. optional float threshold = 2 (proto3 optional syntax) would be cleaner, though it requires a proto-breaking change and updated generated code. Flag for future proto iteration.

Nit: GrpcClient::start in grpc_client.rs (lines 105–109) creates the EmbeddingsServiceClient based on embedding_service.is_some(). The zip on line 78–82 already established embeddings_impl based on the conjunction of embedding_service and processor. The client-side optionality check would be more precisely embeddings_impl.is_some() — as written, if processor is None but embedding_service is Some, the server won't register the service but the client will, leading to a client that thinks the service is available when it is not.

---

Requirements Validation

Acceptance Criterion	Status
EmbeddingsService proto defined and compiled via tonic-build	Satisfied
nodespaced handler implemented; GPU drain protocol handled on daemon shutdown	Satisfied — but see Critical #1 (hard-crash when model absent)
commands/embeddings.rs calls nodespaced via gRPC; no direct packages/core imports remain	Satisfied
Semantic search works end-to-end	Satisfied (in-process path; see Critical #2 for database-switch case)
Embedding generation continues when Tauri window is closed	Satisfied by daemon ownership
EmbeddingState removed from AppServices	Satisfied
cargo build succeeds	Satisfied per commit message
bun run test:all passes	Satisfied per commit message
bun run quality:fix passes	Satisfied per commit message

The two Critical issues mean the change is a net improvement in the happy path but introduces regressions in model-absent and database-switch scenarios. Recommend fixing both before merge.

[malibio]

Review Findings Addressed (commit f53e25b)

Critical Fixes

Critical #1 — Daemon crash when NLP model absent (packages/daemon/src/main.rs)

• ServiceBundle.embeddings_service_grpc changed from EmbeddingsServiceImpl to Option<EmbeddingsServiceImpl>
• build_services() now uses .map() instead of the hard Err(...) when embedding state is None
• Both serve_headless and serve_grpc conditionally register EmbeddingsServiceServer only when the impl is Some
• Daemon starts fully without semantic search when model is absent, matching the Tauri app's tiered-init pattern

Critical #2 — switch_database leaving gRPC server stale (packages/desktop-app/src-tauri/src/services/grpc_client.rs, commands/settings.rs)

• GrpcClient refactored to hold Arc<RwLock<GrpcClientInner>> for interior mutability
• Added restart(node_service, embedding_service, processor) async method that starts a new in-process server and atomically swaps the inner channels via write lock
• switch_database_services now calls grpc_client.restart() after swapping AppServices, so all subsequent RPCs target the new database
• All command callers (nodes.rs, collections.rs, schemas.rs, embeddings.rs) updated to .await the now-async client() / embeddings_client() accessors — no command signature changes required

Suggested Improvements

DRY: stale count logic (packages/daemon/src/services/embeddings_service.rs)

• Extracted stale_count_inner() private async method shared by get_embedding_status and get_stale_count
• Saturating cast: len() as i32 replaced with i32::try_from(ids.len()).unwrap_or(i32::MAX)

Silent node drop warning (packages/desktop-app/src-tauri/src/commands/embeddings.rs)

• node_from_proto now emits tracing::warn! with node_id and raw timestamp string when a node is silently dropped due to unparseable timestamps

on_root_idle return value (packages/desktop-app/src-tauri/src/commands/embeddings.rs)

• Added comment explaining why Ok(true) is always correct (failure path returns Err via ? above)

embeddings_client guard (packages/desktop-app/src-tauri/src/services/grpc_client.rs)

• has_embeddings bool computed from embeddings_impl.is_some() before the impl is moved into the spawn closure; EmbeddingsServiceClient creation is now gated on whether the server actually registered the service

Skipped (as instructed)

• Model filename constant deduplication — cross-crate refactor, scope creep
• Proto optional float threshold — separate proto change
• queue_embedding vs regenerate_embedding comment — minor

Build + Test Results

cargo build -p nodespace-daemon -p nodespace-app  ✅  Finished dev in 19.63s
bun run test                                        ✅  128 files, 3918 tests passed