[malibio]

Closes #1328

[malibio]

Code Review — PR #1335 (issue #1328)

Review Type: Initial review · Rust-only change (2 files) · race-condition fix for embedding-service wiring

Requirements Check

Acceptance Criterion	Status
Executor can never be wired with a stale or None embedding service when one is/becomes available	✅
Startup race (chat model loaded before embeddings) closed — no engine swap needed	✅
search_semantic + skill-based select_tools work when chat model loaded first	✅
No remaining construction site can pass embedding_service: None when the service exists	✅
bun run quality:fix passes	✅ (compiles clean)
Tests pass (test:all)	✅ (all Rust crates pass; one frontend perf benchmark flaked under parallel contention, passes in isolation — unrelated to this Rust-only change)

Verification performed

• Confirmed embedding_svc_state in main.rs:297 is the SAME Arc<RwLock<..>> passed to LocalAgentServiceImpl::new and cloned into the bg loader (main.rs:303,307); loader publishes None→Some at main.rs:425. The shared-handle claim holds end-to-end.
• All 4 GraphToolExecutor construction sites (daemon build_noop_service, replace_engine, + 2 tests) now pass the shared handle. Old new / new_with_optional_services constructors deleted with zero dangling references — clean greenfield removal, no compat cruft.
• cargo build -p nodespace-agent -p nodespace-daemon — clean.
• embedding_handle_is_shared_not_snapshotted test — passes.

Findings

🟢 Locking is correct — no guard held across .await

In select_tools (tools.rs:1422), exec_search_skills (tools.rs:1164), and the embedding_service() accessor (tools.rs:1377), the pattern self.embedding_service.read().await.clone() drops the RwLockReadGuard at the end of the scrutinee expression and carries only the cloned Arc across the subsequent find_skills().await. No read lock is held across an await point, so there is no deadlock or writer-starvation risk against the bg loader's write().await. Confirmed against find_skills's &Arc<NodeEmbeddingService> signature.

🟢 build_workspace_context path also reads live (consistency, no defect)

local_agent_service.rs:334 independently snapshots the service per-turn via self.inner.embedding_service.read().await.clone() before each turn's context build, so that path is also race-free (reads at turn time, not construction time). It deliberately stays a per-turn Option snapshot rather than adopting the shared handle — acceptable because a single turn's context is a point-in-time read. Noting only so a future reader doesn't mistake it for a missed site.

🟢 Test asserts the structural guarantee well

embedding_handle_is_shared_not_snapshotted can't build a real NodeEmbeddingService, so it asserts Arc::ptr_eq(&handle, &executor.embedding_service) plus the live-empty error paths through both the accessor and the search_skills tool. This is the right altitude for a structural invariant — a None→Some write through the shared handle is observed by construction. A full None→Some-then-succeeds integration test would need an NLP engine and belongs in a higher tier.

🟢 Documentation is intent-focused

The SharedEmbeddingService type doc and the rewritten replace_engine comment explain why (the startup race, no engine-swap) rather than restating mechanics. Good.

Nit

• 🟢 tools.rs:1164 / tools.rs:1422 share the identical match self.embedding_service.read().await.clone() { Some.. None.. } preamble with slightly different fallbacks (error result vs. full tool list). Not worth a helper given the divergent None-branches, but if a third call site appears, consider an async fn current_embedding(&self) -> Option<Arc<..>> to centralize the read.

Recommendation

APPROVE — The change is a clean net improvement that directly closes the documented startup race. The snapshot→shared-handle refactor is the correct design (SRP: the executor reads current state instead of owning a stale copy), locking is sound, dead constructors are fully removed per greenfield rules, and the new test pins the invariant. No blocking or improvement-level issues found.

🤖 Generated with Claude Code

[malibio]

Reviewed via /pragmatic-code-review — APPROVE. Race fix is correct, locking is sound (no guard held across await), dead constructors fully removed, new test pins the shared-handle invariant. Full findings posted as a PR comment.