mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
* feat: LRU embedding cache for workspace search (#165) Add CachedEmbeddingProvider that wraps any EmbeddingProvider with an in-memory LRU cache keyed by SHA-256(model_name + text). This avoids redundant HTTP calls when the same text is embedded multiple times (common during reindexing and repeated searches). - Cache uses HashMap + last_accessed tracking with manual LRU eviction (same pattern as llm::response_cache::CachedProvider) - Lock is never held during HTTP calls to prevent blocking - embed_batch() partitions into hits/misses and only fetches misses - Default 10,000 entries (~58 MB for 1536-dim vectors) - Configurable via EMBEDDING_CACHE_SIZE env var - Workspace.with_embeddings() auto-wraps; with_embeddings_uncached() available for tests Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address review comments on embedding cache - Validate embed_batch return count matches expected miss count - Replace unwrap_or_default() with proper error propagation - Fix batch eviction: run final eviction pass after insert to enforce cap - Fix test: use different-length inputs to verify ordering correctness - Reject EMBEDDING_CACHE_SIZE=0 in config validation (minimum is 1) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: replace .expect() with proper error handling in embed_batch The all-cache-hits early-return path used .expect("all cache hits") which violates the project convention of no .unwrap()/.expect() in production code. Replaced with the same ok_or_else pattern used in the normal path. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: clarify memory sizing docs and use saturating_add for eviction - Update memory comments in embedding_cache.rs, config/embeddings.rs, and workspace/mod.rs to note the ~58 MB figure is payload-only (actual memory is higher due to HashMap/key/allocation overhead) - Use saturating_add(1) instead of + 1 for eviction threshold to prevent overflow if max_entries is usize::MAX Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address Copilot review on embedding cache - Avoid double-clone per miss in embed_batch: move embedding into results, clone only for the cache entry - Evict per-insert instead of after all inserts to keep peak memory bounded during large batches - Clamp max_entries to at least 1 in constructor to prevent unexpected eviction behavior when set to 0 via the public API Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: reduce embedding_cache module visibility to private Types are already re-exported via `pub use`, so the module itself doesn't need to be public. Reduces unnecessary API surface. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address serrrfirat review feedback on embedding cache - Add TODO comment for O(n) LRU eviction scalability - Add thundering herd note at lock release in embed() - Warn when cache max_entries exceeds 100k - Use with_embeddings_uncached() in integration test - Add tests: error_does_not_pollute_cache, embed_batch_empty_input - Update README with cache-aware with_embeddings() docs Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: prevent u32 wrapping in FailThenSucceedMock failure counter fetch_sub(1) wraps to u32::MAX when called past zero, silently breaking the mock for 3+ calls. Switch to load-then-store to avoid the wrapping bug in both embed() and embed_batch(). Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address Copilot and serrrfirat review findings on embedding cache - Switch tokio::sync::Mutex to std::sync::Mutex (lock never held across .await — cheaper synchronous lock) - Extract DEFAULT_EMBEDDING_CACHE_SIZE constant to avoid 10_000 duplication between EmbeddingCacheConfig and EmbeddingsConfig Co-Authored-By: Claude Opus 4.6 <[email protected]> * test: add all-misses batch test for embedding cache Adds embed_batch_all_misses test covering the case where every text in a batch is a cache miss — fulfilling the commitment from serrrfirat's review. Co-Authored-By: Claude Opus 4.6 <[email protected]> * chore: trigger CI re-check after rebase * fix: use raw [u8;32] cache keys and pre-allocate HashMap capacity Address Copilot review findings: - cache_key() now returns [u8; 32] instead of hex String, avoiding a 64-byte allocation per lookup - HashMap::with_capacity(max_entries) avoids incremental reallocation - Fix pre-existing staging compilation error in cli/routines.rs (missing max_tool_rounds/use_tools fields) [skip-regression-check] * fix: make cache accessors sync and update doc for [u8;32] keys Address Copilot review: - len(), is_empty(), clear() are now sync since they only take a std::sync::Mutex lock with no .await points - Update cache_size doc comment to reflect [u8;32] keys instead of String keys [skip-regression-check] * fix: remove clone_on_copy for [u8; 32] cache keys [skip-regression-check] * ci: add safety comments to test code for no-panics check The CI no-panics grep check cannot distinguish test code inside src/ files from production code. Add // safety: test annotations to .unwrap(), .expect(), and assert!() calls in #[cfg(test)] modules. * fix: correct cache doc and demote hit/miss logs to trace - Fix misleading "String keys" in memory comment (cache uses [u8; 32]) - Demote per-request hit/miss logs from debug to trace to reduce noise on hot paths (batch summary stays at trace too) * docs: add missing Arc import in workspace README example * perf: batch eviction in embed_batch to avoid O(n×m) cost Replace per-insert evict_lru call with a single evict_k_oldest pass that computes eviction count upfront and removes the k oldest entries in one O(n) scan. Avoids O(n×m) HashMap iterations while holding the mutex during batch inserts. * fix: cap batch cache inserts at max_entries and use O(n) selection - evict_k_oldest now uses select_nth_unstable_by_key for O(n) average partial selection instead of O(n log n) full sort - embed_batch caps cached entries at max_entries when misses exceed capacity, preventing the cache from growing unbounded - Added test: batch_exceeding_capacity_respects_max_entries * fix: flatten test assert for fmt compatibility Shorten assert message to fit single line so cargo fmt doesn't split the safety annotation onto a separate line. * fix: address review feedback and improve embedding cache (takeover #235) - Fix merge conflict: add missing allow_always field in PendingApproval - Thread EmbeddingCacheConfig through CLI memory commands so they respect EMBEDDING_CACHE_SIZE instead of silently using default (fixes #235 review) - Cap HashMap pre-allocation at min(max_entries, 1024) to avoid upfront memory waste at large cache sizes - Fix FailThenSucceedMock race: replace load+store with atomic fetch_update - Remove noisy '// safety: test' comments (40+ lines of diff noise) - Fix collapsed lines from comment removal - Simplify redundant Ok(...collect()?) to just collect() Co-Authored-By: ztsalexey <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(embedding-cache): skip eviction on concurrent duplicate insert When the lock is released for the HTTP call, another caller may insert the same key. Re-check under lock and just update the existing entry without evicting, avoiding unnecessary cache churn under concurrency. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: ztsalexey <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: ztsalexey <[email protected]>
344 lines
12 KiB
Rust
344 lines
12 KiB
Rust
use std::sync::Arc;
|
||
|
||
use secrecy::{ExposeSecret, SecretString};
|
||
|
||
use crate::config::helpers::{optional_env, parse_bool_env, parse_optional_env};
|
||
use crate::error::ConfigError;
|
||
use crate::llm::SessionManager;
|
||
use crate::settings::Settings;
|
||
use crate::workspace::EmbeddingProvider;
|
||
|
||
/// Default maximum number of cached embeddings.
|
||
pub const DEFAULT_EMBEDDING_CACHE_SIZE: usize = 10_000;
|
||
|
||
/// Embeddings provider configuration.
|
||
#[derive(Debug, Clone)]
|
||
pub struct EmbeddingsConfig {
|
||
/// Whether embeddings are enabled.
|
||
pub enabled: bool,
|
||
/// Provider to use: "openai", "nearai", or "ollama"
|
||
pub provider: String,
|
||
/// OpenAI API key (for OpenAI provider).
|
||
pub openai_api_key: Option<SecretString>,
|
||
/// Model to use for embeddings.
|
||
pub model: String,
|
||
/// Ollama base URL (for Ollama provider). Defaults to http://localhost:11434.
|
||
pub ollama_base_url: String,
|
||
/// Embedding vector dimension. Inferred from the model name when not set explicitly.
|
||
pub dimension: usize,
|
||
/// Custom base URL for OpenAI-compatible embedding providers.
|
||
/// When set, overrides the default `https://api.openai.com`.
|
||
pub openai_base_url: Option<String>,
|
||
/// Maximum entries in the embedding LRU cache (default 10,000).
|
||
///
|
||
/// Approximate raw embedding payload: `cache_size × dimension × 4 bytes`.
|
||
/// 10,000 × 1536 floats ≈ 58 MB (payload only; actual memory is higher
|
||
/// due to HashMap buckets, per-entry Vec/timestamp overhead).
|
||
pub cache_size: usize,
|
||
}
|
||
|
||
impl Default for EmbeddingsConfig {
|
||
fn default() -> Self {
|
||
let model = "text-embedding-3-small".to_string();
|
||
let dimension = default_dimension_for_model(&model);
|
||
Self {
|
||
enabled: false,
|
||
provider: "openai".to_string(),
|
||
openai_api_key: None,
|
||
model,
|
||
ollama_base_url: "http://localhost:11434".to_string(),
|
||
dimension,
|
||
openai_base_url: None,
|
||
cache_size: DEFAULT_EMBEDDING_CACHE_SIZE,
|
||
}
|
||
}
|
||
}
|
||
|
||
/// Infer the embedding dimension from a well-known model name.
|
||
///
|
||
/// Falls back to 1536 (OpenAI text-embedding-3-small default) for unknown models.
|
||
fn default_dimension_for_model(model: &str) -> usize {
|
||
match model {
|
||
"text-embedding-3-small" => 1536,
|
||
"text-embedding-3-large" => 3072,
|
||
"text-embedding-ada-002" => 1536,
|
||
"nomic-embed-text" => 768,
|
||
"mxbai-embed-large" => 1024,
|
||
"all-minilm" => 384,
|
||
_ => 1536,
|
||
}
|
||
}
|
||
|
||
impl EmbeddingsConfig {
|
||
pub(crate) fn resolve(settings: &Settings) -> Result<Self, ConfigError> {
|
||
let openai_api_key = optional_env("OPENAI_API_KEY")?.map(SecretString::from);
|
||
|
||
let provider = optional_env("EMBEDDING_PROVIDER")?
|
||
.unwrap_or_else(|| settings.embeddings.provider.clone());
|
||
|
||
let model =
|
||
optional_env("EMBEDDING_MODEL")?.unwrap_or_else(|| settings.embeddings.model.clone());
|
||
|
||
let ollama_base_url = optional_env("OLLAMA_BASE_URL")?
|
||
.or_else(|| settings.ollama_base_url.clone())
|
||
.unwrap_or_else(|| "http://localhost:11434".to_string());
|
||
|
||
let dimension =
|
||
parse_optional_env("EMBEDDING_DIMENSION", default_dimension_for_model(&model))?;
|
||
|
||
let enabled = parse_bool_env("EMBEDDING_ENABLED", settings.embeddings.enabled)?;
|
||
|
||
let openai_base_url = optional_env("EMBEDDING_BASE_URL")?;
|
||
|
||
let cache_size = parse_optional_env("EMBEDDING_CACHE_SIZE", DEFAULT_EMBEDDING_CACHE_SIZE)?;
|
||
|
||
if cache_size == 0 {
|
||
return Err(ConfigError::InvalidValue {
|
||
key: "EMBEDDING_CACHE_SIZE".to_string(),
|
||
message: "must be at least 1".to_string(),
|
||
});
|
||
}
|
||
|
||
Ok(Self {
|
||
enabled,
|
||
provider,
|
||
openai_api_key,
|
||
model,
|
||
ollama_base_url,
|
||
dimension,
|
||
openai_base_url,
|
||
cache_size,
|
||
})
|
||
}
|
||
|
||
/// Get the OpenAI API key if configured.
|
||
pub fn openai_api_key(&self) -> Option<&str> {
|
||
self.openai_api_key.as_ref().map(|s| s.expose_secret())
|
||
}
|
||
|
||
/// Create the appropriate embedding provider based on configuration.
|
||
///
|
||
/// Returns `None` if embeddings are disabled or the required credentials
|
||
/// are missing. The `nearai_base_url` and `session` are needed only for
|
||
/// the NEAR AI provider but must be passed unconditionally.
|
||
pub fn create_provider(
|
||
&self,
|
||
nearai_base_url: &str,
|
||
session: Arc<SessionManager>,
|
||
) -> Option<Arc<dyn EmbeddingProvider>> {
|
||
if !self.enabled {
|
||
tracing::debug!("Embeddings disabled (set EMBEDDING_ENABLED=true to enable)");
|
||
return None;
|
||
}
|
||
|
||
match self.provider.as_str() {
|
||
"nearai" => {
|
||
tracing::debug!(
|
||
"Embeddings enabled via NEAR AI (model: {}, dim: {})",
|
||
self.model,
|
||
self.dimension,
|
||
);
|
||
Some(Arc::new(
|
||
crate::workspace::NearAiEmbeddings::new(nearai_base_url, session)
|
||
.with_model(&self.model, self.dimension),
|
||
))
|
||
}
|
||
"ollama" => {
|
||
tracing::debug!(
|
||
"Embeddings enabled via Ollama (model: {}, url: {}, dim: {})",
|
||
self.model,
|
||
self.ollama_base_url,
|
||
self.dimension,
|
||
);
|
||
Some(Arc::new(
|
||
crate::workspace::OllamaEmbeddings::new(&self.ollama_base_url)
|
||
.with_model(&self.model, self.dimension),
|
||
))
|
||
}
|
||
_ => {
|
||
if let Some(api_key) = self.openai_api_key() {
|
||
let mut provider = crate::workspace::OpenAiEmbeddings::with_model(
|
||
api_key,
|
||
&self.model,
|
||
self.dimension,
|
||
);
|
||
if let Some(ref base_url) = self.openai_base_url {
|
||
tracing::debug!(
|
||
"Embeddings enabled via OpenAI (model: {}, base_url: {}, dim: {})",
|
||
self.model,
|
||
base_url,
|
||
self.dimension,
|
||
);
|
||
provider = provider.with_base_url(base_url);
|
||
} else {
|
||
tracing::debug!(
|
||
"Embeddings enabled via OpenAI (model: {}, dim: {})",
|
||
self.model,
|
||
self.dimension,
|
||
);
|
||
}
|
||
Some(Arc::new(provider))
|
||
} else {
|
||
tracing::warn!("Embeddings configured but OPENAI_API_KEY not set");
|
||
None
|
||
}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
|
||
#[cfg(test)]
|
||
mod tests {
|
||
use super::*;
|
||
use crate::config::helpers::ENV_MUTEX;
|
||
use crate::settings::{EmbeddingsSettings, Settings};
|
||
use crate::testing::credentials::*;
|
||
|
||
/// Clear all embedding-related env vars.
|
||
fn clear_embedding_env() {
|
||
// SAFETY: Only called under ENV_MUTEX in tests.
|
||
unsafe {
|
||
std::env::remove_var("EMBEDDING_ENABLED");
|
||
std::env::remove_var("EMBEDDING_PROVIDER");
|
||
std::env::remove_var("EMBEDDING_MODEL");
|
||
std::env::remove_var("OPENAI_API_KEY");
|
||
std::env::remove_var("EMBEDDING_BASE_URL");
|
||
std::env::remove_var("EMBEDDING_CACHE_SIZE");
|
||
}
|
||
}
|
||
|
||
#[test]
|
||
fn embeddings_disabled_not_overridden_by_openai_key() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
// SAFETY: Under ENV_MUTEX, no concurrent env access.
|
||
unsafe {
|
||
std::env::set_var("OPENAI_API_KEY", TEST_OPENAI_API_KEY_ISSUE_129);
|
||
}
|
||
|
||
let settings = Settings {
|
||
embeddings: EmbeddingsSettings {
|
||
enabled: false,
|
||
..Default::default()
|
||
},
|
||
..Default::default()
|
||
};
|
||
|
||
let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
|
||
assert!(
|
||
!config.enabled,
|
||
"embeddings should remain disabled when settings.embeddings.enabled=false, \
|
||
even when OPENAI_API_KEY is set (issue #129)"
|
||
);
|
||
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::remove_var("OPENAI_API_KEY");
|
||
}
|
||
}
|
||
|
||
#[test]
|
||
fn embeddings_enabled_from_settings() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
|
||
let settings = Settings {
|
||
embeddings: EmbeddingsSettings {
|
||
enabled: true,
|
||
..Default::default()
|
||
},
|
||
..Default::default()
|
||
};
|
||
|
||
let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
|
||
assert!(
|
||
config.enabled,
|
||
"embeddings should be enabled when settings say so"
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn embeddings_env_override_takes_precedence() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::set_var("EMBEDDING_ENABLED", "true");
|
||
}
|
||
|
||
let settings = Settings {
|
||
embeddings: EmbeddingsSettings {
|
||
enabled: false,
|
||
..Default::default()
|
||
},
|
||
..Default::default()
|
||
};
|
||
|
||
let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
|
||
assert!(
|
||
config.enabled,
|
||
"EMBEDDING_ENABLED=true env var should override settings"
|
||
);
|
||
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::remove_var("EMBEDDING_ENABLED");
|
||
}
|
||
}
|
||
|
||
#[test]
|
||
fn embedding_base_url_parsed_from_env() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
|
||
// SAFETY: Under ENV_MUTEX, no concurrent env access.
|
||
unsafe {
|
||
std::env::set_var("EMBEDDING_BASE_URL", "https://custom.example.com");
|
||
}
|
||
|
||
let settings = Settings::default();
|
||
let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
|
||
assert_eq!(
|
||
config.openai_base_url.as_deref(),
|
||
Some("https://custom.example.com")
|
||
);
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::remove_var("EMBEDDING_BASE_URL");
|
||
}
|
||
}
|
||
|
||
#[test]
|
||
fn embedding_base_url_defaults_to_none() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
|
||
let settings = Settings::default();
|
||
let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
|
||
assert!(
|
||
config.openai_base_url.is_none(),
|
||
"openai_base_url should be None when EMBEDDING_BASE_URL is not set"
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn cache_size_zero_rejected() {
|
||
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
|
||
clear_embedding_env();
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::set_var("EMBEDDING_CACHE_SIZE", "0");
|
||
}
|
||
|
||
let settings = Settings::default();
|
||
let result = EmbeddingsConfig::resolve(&settings);
|
||
assert!(result.is_err(), "cache_size=0 should be rejected");
|
||
let err = result.unwrap_err().to_string();
|
||
assert!(err.contains("at least 1"), "should mention minimum: {err}");
|
||
// SAFETY: Under ENV_MUTEX.
|
||
unsafe {
|
||
std::env::remove_var("EMBEDDING_CACHE_SIZE");
|
||
}
|
||
}
|
||
}
|