[malibio]

Closes #3683

Summary

• The failed-load flag is process-global beside the missing and in-flight flags; embedding_model_unavailable() returns FAILED_PRECONDITION for it, so SearchNodes and FindSkillGuidance stop saying the model is loading.
• Removed the injected SharedContext::model_load_failed, the EmbeddingsServiceImpl::new parameter and every hand-built SharedContext field (net -32 lines).
• Added a daemon test that reaches the failed-load state with a non-model file and asserts the status through NodeService search. The docs section lists the failed-load status.

Test plan

• bun run test:changed
• no_model_search:: covers missing, failed and loading

🤖 Generated with Claude Code

[malibio]

Code Review: PR 3686 (issue 3683)

Verdict: APPROVE (posted as a comment, since the PR is self-authored). No blockers.

Requirements

• Failed load returns non-retryable status on SearchNodes, FindSkillGuidance and EmbeddingsService: MET. All go through embedding_model_unavailable(), which returns FAILED_PRECONDITION "failed to load".
• In-flight load still Unavailable/retry, missing file still no-model message: MET. The failed check runs first, but the two flags are mutually exclusive (a load only spawns when a file exists).
• SharedContext::model_load_failed and the EmbeddingsServiceImpl flag removed: MET. Grep shows no remaining references; all 14 hand-built contexts updated.
• Daemon test reaching failed-load through NodeService search: MET (no_model_search.rs).
• Docs line in daemon-cli-lifecycle.md: MET (docs db81586).
• Flag reset on every build_shared_services: MET.

Findings

• Nit: assembly.rs build_shared_services (clear of SHARED_MODEL_LOAD_FAILED). A load task from an earlier call that finishes after a later call's clear could set the flag stale. This matches the existing SHARED_MODEL_MISSING and IN_FLIGHT semantics, and the process builds shared services once, so no change is needed.
• Nit: embeddings_service.rs. The removed unit test (unavailable_reports_failed_precondition_once_load_failed) is covered by the new integration test. That test only asserts through NodeService search, not the EmbeddingsService RPCs, but both share the single function, so coverage is adequate.
• Nit: no_model_search.rs. std::env::set_var and the process-global flags rely on nextest per-process isolation. This is the same as the existing test, and the comment there explains it. The new test lacks that comment but it is not needed.
• No issue numbers in comments or docs, no lint suppression, no leftover unused imports (AtomicBool is still used in assembly.rs).

Recommendation

Approve and proceed through address-review, then merge.

[malibio]

Code Review: PR 3686 (issue 3683)

Verdict: APPROVE (posted as a comment, since the PR is self-authored). No blockers.

Requirements

• Failed load returns non-retryable status on SearchNodes, FindSkillGuidance and EmbeddingsService: MET. All go through embedding_model_unavailable(), which returns FAILED_PRECONDITION "failed to load".
• In-flight load still Unavailable/retry, missing file still no-model message: MET. The failed check runs first, but the two flags are mutually exclusive (a load only spawns when a file exists).
• SharedContext::model_load_failed and the EmbeddingsServiceImpl flag removed: MET. Grep shows no remaining references; all 14 hand-built contexts updated.
• Daemon test reaching failed-load through NodeService search: MET (no_model_search.rs).
• Docs line in daemon-cli-lifecycle.md: MET (docs db81586).
• Flag reset on every build_shared_services: MET.

Findings

• Nit: assembly.rs build_shared_services (clear of SHARED_MODEL_LOAD_FAILED). A load task from an earlier call that finishes after a later call's clear could set the flag stale. This matches the existing SHARED_MODEL_MISSING and IN_FLIGHT semantics, and the process builds shared services once, so no change is needed.
• Nit: embeddings_service.rs. The removed unit test (unavailable_reports_failed_precondition_once_load_failed) is covered by the new integration test. That test only asserts through NodeService search, not the EmbeddingsService RPCs, but both share the single function, so coverage is adequate.
• Nit: no_model_search.rs. std::env::set_var and the process-global flags rely on nextest per-process isolation. This is the same as the existing test, and the comment there explains it. The new test lacks that comment but it is not needed.
• No issue numbers in comments or docs, no lint suppression, no leftover unused imports (AtomicBool is still used in assembly.rs).

Recommendation

Approve and proceed through address-review, then merge.

[malibio]

Address review

The review was APPROVE with three nits and no change requested. Skipped all three:

• Stale load task setting the flag after a later build_shared_services clears it: matches the existing missing and in-flight flags, and shared services are built once per process. Revisit if a daemon ever rebuilds them in-process.
• Removed EmbeddingsService unit test: both services answer through the one shared function, which the integration test covers.
• set_var relying on nextest's process-per-test: the sibling test already documents it.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: No code changed after the review.