llm-multiverse/implementation-plans/issue-114.md

# Implementation Plan — Issue #114: Tech debt: minor findings from issue #28 review

## Metadata

| Field | Value |
|---|---|
| Issue | [#114](https://git.shahondin1624.de/llm-multiverse/llm-multiverse/issues/114) |
| Title | Tech debt: minor findings from issue #28 review |
| Milestone | Phase 4: Memory Service |
| Labels | `tech-debt` |
| Status | `PLANNED` |
| Language | Rust |
| Related Plans | [issue-028.md](issue-028.md), [issue-033.md](issue-033.md) |
| Blocked by | — |

## Acceptance Criteria

- [ ] `EMBEDDING_DIM` constant and `CREATE_EMBEDDINGS` DDL cannot drift out of sync
- [ ] `with_connection()` does not block the tokio runtime when called from async handlers
- [ ] Unused dependencies removed from `Cargo.toml`

## Architecture Analysis

### Service Context
- All changes are internal to `services/memory`.
- No proto or gRPC API changes required.

### Existing Patterns
- `services/memory/src/db/schema.rs` — DDL constants and `EMBEDDING_DIM`
- `services/memory/src/db/mod.rs` — `DuckDbManager` with `std::sync::Mutex`
- `services/memory/Cargo.toml` — dependency list

## Implementation Steps

### 1. Sync `EMBEDDING_DIM` with DDL (schema.rs)

**Option A (recommended): Generate DDL dynamically via `format!`**

Replace the static `CREATE_EMBEDDINGS` constant with a function that builds the DDL string using `EMBEDDING_DIM`:

```rust
fn create_embeddings_ddl() -> String {
    format!(
        "CREATE TABLE IF NOT EXISTS embeddings (
            memory_id VARCHAR NOT NULL REFERENCES memories(id),
            embedding_type VARCHAR NOT NULL,
            vector FLOAT[{EMBEDDING_DIM}] NOT NULL,
            PRIMARY KEY (memory_id, embedding_type)
        )"
    )
}
```

Update `apply_v1()` to call `create_embeddings_ddl()` instead of referencing the `CREATE_EMBEDDINGS` constant.

**Option B (alternative): Add a compile-time assertion**

Keep the static string but add a `const_assert!` (via `static_assertions` crate) or a unit test that parses the DDL and verifies the dimension matches `EMBEDDING_DIM`. This is simpler but less robust.

**Decision:** Go with Option A. It is straightforward, requires no extra crates, and eliminates the drift risk entirely.

### 2. Avoid blocking tokio with `std::sync::Mutex` (mod.rs)

DuckDB's `Connection` is `!Sync`, so `tokio::sync::Mutex` alone would not help (the connection cannot be held across `.await` points anyway). The correct fix is to use `tokio::task::spawn_blocking` at the call sites.

**Changes:**

- Keep `DuckDbManager` using `std::sync::Mutex` internally (this is correct for `!Sync` types).
- Add a new async helper method on `DuckDbManager`:

```rust
/// Execute a closure with the connection on a blocking thread,
/// avoiding stalling the tokio runtime.
pub async fn with_connection_blocking<F, T>(&self, f: F) -> Result<T, DbError>
where
    F: FnOnce(&Connection) -> Result<T, DbError> + Send + 'static,
    T: Send + 'static,
{
    // self needs to be behind Arc for this to work
    // Acquire lock inside spawn_blocking
}
```

Since `DuckDbManager` is already shared via `Arc<DuckDbManager>`, wrap the call in `spawn_blocking`:

- `DuckDbManager` must be `Send + Sync` (it already is because `Mutex<Connection>` is `Sync`).
- Clone the `Arc` and move it into the `spawn_blocking` closure, then call `with_connection` inside.
- Update gRPC handler call sites (in `services/memory/src/service.rs`) to use the async variant.

Retain the synchronous `with_connection()` for use in tests and non-async contexts.

### 3. Remove unused `uuid` dependency (Cargo.toml)

- Remove `uuid = { version = "1", features = ["v4"] }` from `[dependencies]`.
- `chrono` is actively used (timestamp parsing in retrieval and provenance modules), so it stays.

### 4. Tests

- **Item 1:** Existing `test_insert_embedding` and `test_vector_similarity_query` in `schema.rs` already exercise DDL creation with `EMBEDDING_DIM`. Verify they still pass after switching to the function.
- **Item 2:** Add a `#[tokio::test]` that calls `with_connection_blocking` to confirm it runs without blocking. Existing sync tests for `with_connection` remain unchanged.
- **Item 3:** Run `cargo build` — if `uuid` is used anywhere, the build will fail, catching any oversight.

## Files to Create/Modify

| File | Action | Purpose |
|---|---|---|
| `services/memory/src/db/schema.rs` | Modify | Replace `CREATE_EMBEDDINGS` constant with `create_embeddings_ddl()` function |
| `services/memory/src/db/mod.rs` | Modify | Add `with_connection_blocking` async method |
| `services/memory/src/service.rs` | Modify | Switch gRPC handlers from `with_connection` to `with_connection_blocking` |
| `services/memory/Cargo.toml` | Modify | Remove `uuid` dependency |

## Risks and Edge Cases

- **`spawn_blocking` lifetime requirements:** The closure passed to `spawn_blocking` must be `'static`. This means data borrowed from the request must be cloned/owned before entering the closure. gRPC handlers already receive owned types, so this should be straightforward.
- **`format!`-based DDL:** The generated string is created at runtime rather than compile time. This is negligible overhead since it only runs once during migration.

## Deviation Log

_(Filled during implementation if deviations from plan occur)_

| Deviation | Reason |
|---|---|