Skip to content

Commit 2dc78b2

Browse files
henrypark133claude
andauthored
feat(db): add per-user CachedSettingsStore decorator (#2425)
* feat(db): add per-user CachedSettingsStore decorator SettingsStore methods hit the database on every call. The v2 engine path (effect_adapter) and the dispatcher's per-turn tool permission loading both called get_all_settings() without caching, adding unnecessary DB round-trips on every agentic loop iteration. Add a write-through CachedSettingsStore decorator that caches get_all_settings() results per user_id. Write operations (set_setting, delete_setting, set_all_settings) delegate to the inner store then invalidate that user's cache entry. The write lock is held across DB loads to prevent stale-data races from concurrent invalidations. Wire the cache into TenantScope via a new settings_store field on AgentDeps, so all settings reads in the agent loop go through the cache. Remove the per-turn cached_tool_permissions Mutex hack from ChatDelegate that was working around the missing cache layer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review feedback - Store Arc<HashMap> in cache instead of bare HashMap to avoid cloning the full settings map on every cache hit. get_setting/has_settings now only clone the single requested value or check emptiness through the Arc. - Route get_setting_with_admin_fallback() through self.settings() instead of self.inner so both the per-user and admin lookups go through the cache. - Update settings section comment to accurately describe which methods delegate through settings(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address all PR review feedback - Use `crate::db::` imports instead of `super::` (convention fix) - Add `wrap()` factory fn to CachedSettingsStore, simplify app.rs construction - Store `Arc<HashMap>` in cache to avoid full map clones on hits - Expose `invalidate_user()` and `flush()` public methods - Wire `flush()` into SIGHUP handler via concrete `settings_cache` on AppComponents - Wire `settings_store` into GatewayState and route all settings handlers through it so web UI writes invalidate the cache (critical fix) - Route `get_setting_with_admin_fallback()` through `self.settings()` - Add error-path test (FailingStore mock, cache not poisoned on error) - Add concurrent-access test (8 concurrent readers, inner store hit once) - Add TenantScope caller-level test (read/write through cache wiring) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reuse resolve_settings_store() in settings_tools_set_handler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address all review feedback on CachedSettingsStore - Add TTL (300s) and max-entries cap (1000) to bound cache staleness and memory growth. Entries expire after TTL; cache clears when cap exceeded. - Route admin tool_policy GET/PUT through resolve_settings_store() so writes invalidate the __admin__ cache entry. - Route settings_export_handler and settings_tools_list_handler through resolve_settings_store() (were bypassing cache on reads). - Wire invalidate_user() into users_delete_handler and users_suspend_handler so deleted/suspended users' settings are evicted. - Replace GatewayState.settings_store (trait object) with settings_cache (concrete CachedSettingsStore) — single field for both trait dispatch and cache management, no desync risk. - Add settings_override to ExtensionManager with with_settings_store() builder. All settings reads/writes in ExtensionManager now route through the cached store when available. - Make ExtensionManager::settings_store() pub(crate); update AuthManager to call it instead of database(), closing the auth descriptor cache bypass. - Remove unused wrap() method; merge redundant invalidate/invalidate_user. - Add tracing::debug on SIGHUP cache flush. - Expand module docs with design assumptions, known bypass paths, TTL and eviction semantics. - Add tests: expired_entry_triggers_reload, fresh_entry_does_not_reload, max_entries_cap_triggers_eviction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: collapse nested if into filter to satisfy clippy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 82d341d commit 2dc78b2

27 files changed

Lines changed: 969 additions & 146 deletions

src/agent/agent_loop.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,9 @@ pub struct AgentDeps {
196196
/// Resolved durable owner scope for the instance.
197197
pub owner_id: String,
198198
pub store: Option<Arc<dyn Database>>,
199+
/// Cached settings store. When set, `TenantScope` routes settings reads
200+
/// through this cache instead of hitting the raw `Database` directly.
201+
pub settings_store: Option<Arc<dyn crate::db::SettingsStore + Send + Sync>>,
199202
pub llm: Arc<dyn LlmProvider>,
200203
/// Cheap/fast LLM for lightweight tasks (heartbeat, routing, evaluation).
201204
/// Falls back to the main `llm` if None.
@@ -482,10 +485,13 @@ impl Agent {
482485
let user_id = identity.owner_id.as_str();
483486
let rate = self.deps.tenant_rates.get_or_create(user_id).await;
484487

485-
let store =
486-
self.deps.store.as_ref().map(|db| {
487-
crate::tenant::TenantScope::with_identity(identity.clone(), Arc::clone(db))
488-
});
488+
let store = self.deps.store.as_ref().map(|db| {
489+
let scope = crate::tenant::TenantScope::with_identity(identity.clone(), Arc::clone(db));
490+
match &self.deps.settings_store {
491+
Some(ss) => scope.with_settings_store(Arc::clone(ss)),
492+
None => scope,
493+
}
494+
});
489495

490496
// Reuse the owner workspace if user matches, otherwise create per-user.
491497
// Per-user workspaces are seeded on first creation so they get identity

src/agent/dispatcher.rs

Lines changed: 22 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,6 @@ impl Agent {
261261
force_text_at,
262262
user_tz,
263263
turn_usage: std::sync::Mutex::new(TurnUsageSummary::default()),
264-
cached_tool_permissions: std::sync::Mutex::new(None),
265264
cached_admin_tool_policy: tokio::sync::OnceCell::new(),
266265
};
267266

@@ -377,8 +376,6 @@ struct ChatDelegate<'a> {
377376
force_text_at: usize,
378377
user_tz: chrono_tz::Tz,
379378
turn_usage: std::sync::Mutex<TurnUsageSummary>,
380-
cached_tool_permissions:
381-
std::sync::Mutex<Option<std::collections::HashMap<String, PermissionState>>>,
382379
cached_admin_tool_policy: crate::tools::permissions::AdminToolPolicyCache,
383380
}
384381

@@ -478,48 +475,25 @@ impl<'a> LoopDelegate for ChatDelegate<'a> {
478475
// AlwaysAllow tools are pre-approved in session so the approval
479476
// flow is skipped — unless the tool declares ApprovalRequirement::Always,
480477
// which is an unbypassable hard floor.
481-
let tool_permissions = {
482-
// Check the cache first (brief lock, no await while held).
483-
let cached = {
484-
let cache = self
485-
.cached_tool_permissions
486-
.lock()
487-
.unwrap_or_else(|e| e.into_inner());
488-
cache.clone()
489-
};
490-
if let Some(perms) = cached {
491-
perms
492-
} else {
493-
// Cache miss — load from DB (async).
494-
let perms = if let Some(store) = self.tenant.store() {
495-
match store.get_all_settings().await {
496-
Ok(db_map) => {
497-
crate::settings::Settings::from_db_map(&db_map).tool_permissions
498-
}
499-
Err(e) => {
500-
tracing::warn!(
501-
"Failed to load tool permissions, keeping existing session state: {}",
502-
e
503-
);
504-
// Fail closed: preserve the previously filtered available_tools
505-
// rather than publishing the unfiltered tool list, which could
506-
// re-expose tools explicitly marked Disabled.
507-
return None;
508-
}
509-
}
510-
} else {
511-
std::collections::HashMap::new()
512-
};
513-
// Store in cache for subsequent iterations.
514-
{
515-
let mut cache = self
516-
.cached_tool_permissions
517-
.lock()
518-
.unwrap_or_else(|e| e.into_inner());
519-
*cache = Some(perms.clone());
478+
//
479+
// The SettingsStore is wrapped in CachedSettingsStore so repeated
480+
// calls within the same session are cheap (in-memory lookup).
481+
let tool_permissions = if let Some(store) = self.tenant.store() {
482+
match store.get_all_settings().await {
483+
Ok(db_map) => crate::settings::Settings::from_db_map(&db_map).tool_permissions,
484+
Err(e) => {
485+
tracing::warn!(
486+
"Failed to load tool permissions, keeping existing session state: {}",
487+
e
488+
);
489+
// Fail closed: preserve the previously filtered available_tools
490+
// rather than publishing the unfiltered tool list, which could
491+
// re-expose tools explicitly marked Disabled.
492+
return None;
520493
}
521-
perms
522494
}
495+
} else {
496+
std::collections::HashMap::new()
523497
};
524498

525499
// Filter tool definitions and collect AlwaysAllow names for session
@@ -2011,6 +1985,7 @@ mod tests {
20111985
let deps = AgentDeps {
20121986
owner_id: "default".to_string(),
20131987
store: None,
1988+
settings_store: None,
20141989
llm: Arc::new(StaticLlmProvider),
20151990
cheap_llm: None,
20161991
safety: Arc::new(SafetyLayer::new(&SafetyConfig {
@@ -2320,6 +2295,7 @@ mod tests {
23202295
let deps = AgentDeps {
23212296
owner_id: "default".to_string(),
23222297
store: None,
2298+
settings_store: None,
23232299
llm: Arc::new(AuthThenApprovalProvider),
23242300
cheap_llm: None,
23252301
safety: Arc::new(SafetyLayer::new(&SafetyConfig {
@@ -3300,6 +3276,7 @@ mod tests {
33003276
let deps = AgentDeps {
33013277
owner_id: "default".to_string(),
33023278
store: None,
3279+
settings_store: None,
33033280
llm,
33043281
cheap_llm: None,
33053282
safety: Arc::new(SafetyLayer::new(&SafetyConfig {
@@ -3448,6 +3425,7 @@ mod tests {
34483425
let deps = AgentDeps {
34493426
owner_id: "default".to_string(),
34503427
store: Some(db),
3428+
settings_store: None,
34513429
llm: llm as Arc<dyn LlmProvider>,
34523430
cheap_llm: None,
34533431
safety: Arc::new(SafetyLayer::new(&SafetyConfig {
@@ -3578,6 +3556,7 @@ mod tests {
35783556
let deps = AgentDeps {
35793557
owner_id: "default".to_string(),
35803558
store: None,
3559+
settings_store: None,
35813560
llm,
35823561
cheap_llm: None,
35833562
safety: Arc::new(SafetyLayer::new(&SafetyConfig {

src/agent/thread_ops.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2569,6 +2569,7 @@ mod tests {
25692569
let deps = crate::agent::AgentDeps {
25702570
owner_id: "default".to_string(),
25712571
store: None,
2572+
settings_store: None,
25722573
llm: Arc::new(StaticLlmProvider),
25732574
cheap_llm: None,
25742575
safety: Arc::new(ironclaw_safety::SafetyLayer::new(
@@ -3137,6 +3138,7 @@ mod tests {
31373138
let deps = AgentDeps {
31383139
owner_id: "default".to_string(),
31393140
store: None,
3141+
settings_store: None,
31403142
llm: Arc::new(StubLlm::default()),
31413143
cheap_llm: None,
31423144
safety: Arc::new(SafetyLayer::new(&SafetyConfig {

src/app.rs

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ pub struct AppComponents {
4949
/// runtime settings writes flow through the workspace and pick up schema
5050
/// validation.
5151
pub settings_store: Option<Arc<dyn crate::db::SettingsStore + Send + Sync>>,
52+
/// Concrete cache handle for `flush()` / `invalidate_user()`.
53+
/// Same instance backing `settings_store` when a cache is active.
54+
pub settings_cache: Option<Arc<crate::db::cached_settings::CachedSettingsStore>>,
5255
pub extension_manager: Option<Arc<ExtensionManager>>,
5356
pub mcp_session_manager: Arc<McpSessionManager>,
5457
pub mcp_process_manager: Arc<McpProcessManager>,
@@ -823,7 +826,7 @@ impl AppBuilder {
823826
Arc::new(InMemorySecretsStore::new(crypto))
824827
};
825828
let extension_manager = {
826-
let manager = Arc::new(ExtensionManager::new(
829+
let mut em = ExtensionManager::new(
827830
Arc::clone(&mcp_session_manager),
828831
Arc::clone(&mcp_process_manager),
829832
ext_secrets,
@@ -836,7 +839,11 @@ impl AppBuilder {
836839
self.config.owner_id.clone(),
837840
self.db.clone(),
838841
catalog_entries.clone(),
839-
));
842+
);
843+
if let Some(ref ss) = settings_store_override {
844+
em = em.with_settings_store(Arc::clone(ss));
845+
}
846+
let manager = Arc::new(em);
840847
tools.register_extension_tools(Arc::clone(&manager));
841848

842849
// Register permission management tool and upgrade tool_list with
@@ -949,22 +956,30 @@ impl AppBuilder {
949956
// `upgrade_tool_list`) can be wired with the adapter from the start.
950957
// The same adapter instance is then exposed on `AppComponents.settings_store`
951958
// and reused by main.rs (e.g. for the SIGHUP reload handler).
952-
let settings_store: Option<Arc<dyn crate::db::SettingsStore + Send + Sync>> =
953-
match (&workspace, &self.db) {
954-
(Some(ws), Some(db)) => {
955-
let adapter = Arc::new(crate::workspace::WorkspaceSettingsAdapter::new(
956-
Arc::clone(ws),
957-
Arc::clone(db),
958-
));
959-
if let Err(e) = adapter.ensure_system_config().await {
960-
tracing::debug!(
961-
"WorkspaceSettingsAdapter eager seed failed (lazy seed will retry): {e}"
962-
);
963-
}
964-
Some(adapter as Arc<dyn crate::db::SettingsStore + Send + Sync>)
959+
let (settings_store, settings_cache): (
960+
Option<Arc<dyn crate::db::SettingsStore + Send + Sync>>,
961+
Option<Arc<crate::db::cached_settings::CachedSettingsStore>>,
962+
) = match (&workspace, &self.db) {
963+
(Some(ws), Some(db)) => {
964+
let adapter = Arc::new(crate::workspace::WorkspaceSettingsAdapter::new(
965+
Arc::clone(ws),
966+
Arc::clone(db),
967+
));
968+
if let Err(e) = adapter.ensure_system_config().await {
969+
tracing::debug!(
970+
"WorkspaceSettingsAdapter eager seed failed (lazy seed will retry): {e}"
971+
);
965972
}
966-
_ => None,
967-
};
973+
let cached = Arc::new(crate::db::cached_settings::CachedSettingsStore::new(
974+
adapter as Arc<dyn crate::db::SettingsStore + Send + Sync>,
975+
));
976+
(
977+
Some(Arc::clone(&cached) as Arc<dyn crate::db::SettingsStore + Send + Sync>),
978+
Some(cached),
979+
)
980+
}
981+
_ => (None, None),
982+
};
968983

969984
let (
970985
mcp_session_manager,
@@ -1099,6 +1114,7 @@ impl AppBuilder {
10991114
embeddings,
11001115
workspace,
11011116
settings_store,
1117+
settings_cache,
11021118
extension_manager,
11031119
mcp_session_manager,
11041120
mcp_process_manager,

src/bridge/auth_manager.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,9 @@ impl AuthManager {
123123
.map(|db| db.as_ref() as &dyn crate::db::SettingsStore)
124124
})
125125
.or_else(|| {
126-
self.extension_manager.as_ref().and_then(|manager| {
127-
manager
128-
.database()
129-
.map(|db| db.as_ref() as &dyn crate::db::SettingsStore)
130-
})
126+
self.extension_manager
127+
.as_ref()
128+
.and_then(|manager| manager.settings_store())
131129
})
132130
}
133131

src/bridge/router.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4308,6 +4308,7 @@ mod tests {
43084308
let deps = crate::agent::AgentDeps {
43094309
owner_id: "default".to_string(),
43104310
store: None,
4311+
settings_store: None,
43114312
llm: Arc::new(StaticLlmProvider),
43124313
cheap_llm: None,
43134314
safety: Arc::new(ironclaw_safety::SafetyLayer::new(
@@ -4930,6 +4931,7 @@ mod tests {
49304931
let deps = AgentDeps {
49314932
owner_id: "default".to_string(),
49324933
store: None,
4934+
settings_store: None,
49334935
llm: Arc::new(StubLlm::default()),
49344936
cheap_llm: None,
49354937
safety: Arc::new(SafetyLayer::new(&SafetyConfig {

0 commit comments

Comments
 (0)