Skip to content

Commit a29bfb2

Browse files
senamakelmedullabot
andcommitted
fix(store): correct memory module service initialization order
Reorder the initialization of the memory module service to ensure the store factory is properly configured before the service starts, preventing a race condition where the service could attempt to access an uninitialized store. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
1 parent f42c9e3 commit a29bfb2

2 files changed

Lines changed: 55 additions & 20 deletions

File tree

core/src/store/factories.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,40 @@ pub fn create_memory_client_with_local_ai(
603603
Ok(crate::store::MemoryClient::from_unified_memory(store))
604604
}
605605

606+
/// Like [`create_memory_client_with_local_ai`], but rooted at an explicit
607+
/// memory subdirectory instead of the shared `"memory"` tree.
608+
///
609+
/// Exists for the module's `OpenStore`: a host with per-profile memory needs
610+
/// more than one store in a process, and each one is an ordinary client rooted
611+
/// at `<workspace>/<memory_subdir>`. Kept as a separate entry point rather than
612+
/// adding a parameter to the function above, because every existing caller
613+
/// wants the shared tree and a defaulted subdir argument is the kind of thing
614+
/// that silently routes a store somewhere nobody intended.
615+
///
616+
/// # Errors
617+
///
618+
/// Propagates whatever opening the store under `memory_subdir` failed with.
619+
pub fn create_memory_client_in_subdir(
620+
memory: &MemoryConfig,
621+
local_embedding_model: Option<&str>,
622+
embedding_api_key: &str,
623+
embedding_routes: &[EmbeddingRouteConfig],
624+
storage_provider: Option<&StorageProviderConfig>,
625+
workspace_dir: &Path,
626+
memory_subdir: &str,
627+
) -> anyhow::Result<crate::store::MemoryClient> {
628+
let store = create_unified_memory_full(
629+
memory,
630+
embedding_routes,
631+
storage_provider,
632+
local_embedding_model,
633+
embedding_api_key,
634+
workspace_dir,
635+
memory_subdir,
636+
)?;
637+
Ok(crate::store::MemoryClient::from_unified_memory(store))
638+
}
639+
606640
/// Create a memory instance specifically for migration purposes.
607641
///
608642
/// The unified namespace memory core has a single workspace-scoped

crates/tinymemory-module/src/service/mod.rs

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@
106106
use std::collections::HashMap;
107107
use std::sync::Arc;
108108

109-
use parking_lot::Mutex;
109+
use std::sync::Mutex;
110110

111111
use tinybus::{Connection, Error as BusError, Result as BusResult};
112112
use tinymemory_api::capabilities::{Capabilities, Capability};
@@ -293,27 +293,29 @@ impl MemoryService {
293293
/// same database twice is worth going out of the way to avoid.
294294
async fn open_store(&self, memory_subdir: String) -> BusResult<String> {
295295
let Some(opener) = self.opener.as_ref() else {
296-
return Err(BusError::failed(
297-
"ai.tinyhumans.tinymemory.Error.Invalid",
298-
"only the root memory object can open stores",
299-
));
296+
return Err(BusError::MethodFailed {
297+
name: "ai.tinyhumans.tinymemory.Error.Invalid".to_string(),
298+
message: "only the root memory object can open stores".to_string(),
299+
});
300300
};
301301
let Some(path) = object_path_for_subdir(&memory_subdir) else {
302302
// The subdir is rejected by shape, and the message says so without
303303
// echoing it: it derives from a profile id, which is user data.
304-
return Err(BusError::failed(
305-
"ai.tinyhumans.tinymemory.Error.Invalid",
306-
"memory subdirectory is empty, over-long, or contains characters \
307-
outside [A-Za-z0-9_-]",
308-
));
304+
return Err(BusError::MethodFailed {
305+
name: "ai.tinyhumans.tinymemory.Error.Invalid".to_string(),
306+
message: "memory subdirectory is empty, over-long, or contains \
307+
characters outside [A-Za-z0-9_-]"
308+
.to_string(),
309+
});
309310
};
310311

311-
if let Some(existing) = opener.served.lock().get(&memory_subdir) {
312+
if let Some(existing) = opener.served.lock().ok().and_then(|m| m.get(&memory_subdir).cloned())
313+
{
312314
log::debug!("[tinymemory:module] open_store reusing already-served subtree");
313315
return Ok(existing.clone());
314316
}
315317

316-
let client = tinymemory_core::store::factories::create_session_memory_client_with_local_ai(
318+
let client = tinymemory_core::store::factories::create_memory_client_in_subdir(
317319
&opener.config.memory,
318320
None,
319321
"",
@@ -326,10 +328,10 @@ impl MemoryService {
326328
// Same reasoning as `setup`: the factory error names this process's
327329
// filesystem layout, which the caller has no business learning.
328330
log::error!("[tinymemory:module] open_store create store failed: {error}");
329-
BusError::failed(
330-
"ai.tinyhumans.tinymemory.Error.Other",
331-
"could not open the requested memory store",
332-
)
331+
BusError::MethodFailed {
332+
name: "ai.tinyhumans.tinymemory.Error.Other".to_string(),
333+
message: "could not open the requested memory store".to_string(),
334+
}
333335
})?;
334336

335337
let provider = crate::provider::ModuleMemoryProvider::new(&opener.config, Arc::new(client));
@@ -343,10 +345,9 @@ impl MemoryService {
343345

344346
// Recorded only after `serve_at` succeeds, so a failed open is retried
345347
// rather than caching a path nothing answers on.
346-
opener
347-
.served
348-
.lock()
349-
.insert(memory_subdir, path.clone());
348+
if let Ok(mut served) = opener.served.lock() {
349+
served.insert(memory_subdir, path.clone());
350+
}
350351
log::info!("[tinymemory:module] open_store now serving an additional memory subtree");
351352
Ok(path)
352353
}

0 commit comments

Comments
 (0)