test(local-ai): serialize Ollama env mutations#1656
Conversation
📝 WalkthroughWalkthroughThis PR introduces a centralized test-only helper function ChangesTest synchronization via centralized local-AI guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/local_ai/mod.rs (1)
7-12: ⚡ Quick winMove test guard implementation out of
mod.rsto keep it export-focused.Line 8 adds operational logic in a domain
mod.rs. Prefer re-exporting from a sibling test-support module and keeping this file as declarations/re-exports only.Proposed refactor (module stays export-focused)
#[cfg(test)] -pub(crate) fn local_ai_test_guard() -> std::sync::MutexGuard<'static, ()> { - LOCAL_AI_TEST_MUTEX - .lock() - .unwrap_or_else(|p| p.into_inner()) -} +mod test_support; +#[cfg(test)] +pub(crate) use test_support::local_ai_test_guard;As per coding guidelines:
src/openhuman/*/mod.rs: Keep domainmod.rsfiles light and export-focused; place operational code in sibling files (e.g.,ops.rs,store.rs,schedule.rs,types.rs) and re-export the public API frommod.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/local_ai/mod.rs` around lines 7 - 12, The function local_ai_test_guard and its use of LOCAL_AI_TEST_MUTEX implement operational test logic inside mod.rs; move that implementation into a sibling module (e.g., test_support.rs or ops.rs) where you define pub(crate) fn local_ai_test_guard() -> std::sync::MutexGuard<'static, ()> and the LOCAL_AI_TEST_MUTEX there, then in mod.rs replace the implementation with a simple re-export (pub(crate) use test_support::local_ai_test_guard;) so mod.rs remains export-focused and the operational code lives in the sibling module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/local_ai/mod.rs`:
- Around line 7-12: The function local_ai_test_guard and its use of
LOCAL_AI_TEST_MUTEX implement operational test logic inside mod.rs; move that
implementation into a sibling module (e.g., test_support.rs or ops.rs) where you
define pub(crate) fn local_ai_test_guard() -> std::sync::MutexGuard<'static, ()>
and the LOCAL_AI_TEST_MUTEX there, then in mod.rs replace the implementation
with a simple re-export (pub(crate) use test_support::local_ai_test_guard;) so
mod.rs remains export-focused and the operational code lives in the sibling
module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 434a8e87-7543-47f8-8f2e-777c522a7edd
📒 Files selected for processing (8)
src/openhuman/local_ai/install.rssrc/openhuman/local_ai/mod.rssrc/openhuman/local_ai/ollama_api.rssrc/openhuman/local_ai/service/ollama_admin_tests.rssrc/openhuman/local_ai/service/public_infer_tests.rssrc/openhuman/local_ai/service/vision_embed.rssrc/openhuman/memory/store/factories.rssrc/openhuman/voice/postprocess.rs
Summary
local_ai_test_guard()wrapper around the existing local-AI test mutexOPENHUMAN_OLLAMA_BASE_URL,OLLAMA_BIN, and local PATH lookupssrc/openhuman/local_ai/README.mdWhy
While validating #1589 locally, the Rust coverage path exposed a flaky race where one test could mutate or clear Ollama process-global env vars while another local-AI test was reading them. This keeps the existing test-only isolation model but applies it consistently across the affected modules. No production behavior changes.
Test plan
cargo fmt --checkCARGO_INCREMENTAL=0 cargo test -p openhuman openhuman::local_ai -- --nocaptureCARGO_INCREMENTAL=0 cargo test -p openhuman openhuman::memory::store::factories::tests -- --nocapturesudo -n docker run ... ghcr.io/tinyhumansai/openhuman_ci:rust-1.93.0 ... cargo llvm-cov -p openhuman --lcov --output-path /tmp/lcov-core.infocargo checkSummary by CodeRabbit