## Data Safety

#### Migration restore_from_backup overwrites live DB while pool holds open connections
- **Difficulty:** medium
- **Location:** `src/store/backup.rs:171-180` (called from `src/store/migrations.rs:106-128`)
- **Description:** When a migration step fails, `migrate()` calls `restore_from_backup(db_path, bak)` which invokes `copy_triplet` → `copy_file_atomic` → `crate::fs::atomic_replace` to rename the backup over the live `db_path`. But the SQLite `pool` from `migrate()`'s caller is STILL holding open file descriptors against the old inode. After the atomic_replace, `db_path`'s inode is the backup's; existing connections in the pool see the *unlinked old* inode, while any subsequent open via the same path sees the new one. Worse, the loop then copies `-wal` and `-shm` over what the pool's open connections believe are *their* sidecars — but those copies land on the new inode while the pool's mapped sidecars (mmap'd) belong to the old inode. The result is silent two-state divergence: in-process queries can read stale rows from the old WAL while readers from new processes see the restored DB. SQLite's documented restore pattern requires closing all connections first (or using the online backup API). Tests in `src/store/migrations.rs:tests` use a fresh temp DB and a single transaction, so the divergence never surfaces. In production a daemon already has a long-lived `Store::open(...)` that owns the pool; if a fresh CLI invocation triggers a migration and fails on a buggy step, the daemon then serves queries from a phantom inode.
- **Suggested fix:** Drop / close the pool before calling `restore_from_backup` (e.g. take `pool` by value, `.close().await`, then run the restore, then re-open). At minimum, `PRAGMA wal_checkpoint(TRUNCATE)` and force a connection close on every pooled connection before the file replace; document that callers must hold no other Store handles to `db_path` during the restore.

#### `stream_summary_writer` bypasses `WRITE_LOCK` — concurrent writer can collide with reindex
- **Difficulty:** medium
- **Location:** `src/store/chunks/crud.rs:504-545`
- **Description:** Every other write path in `Store<ReadWrite>` acquires the in-process `WRITE_LOCK` mutex via `begin_write()`. `stream_summary_writer` instead executes `INSERT OR IGNORE INTO llm_summaries ...` directly against `&self.pool` from a captured `Arc<SqlitePool>` callback that fires from LLM provider streaming threads. Two concrete races:
  1. A background `cqs llm summary` (or `--improve-docs`) batch is streaming results while the user runs `cqs index` on the same project. The streaming write and `upsert_chunks_and_calls` both contend for SQLite's exclusive lock without the in-process serialization that `WRITE_LOCK` provides. With WAL mode and `busy_timeout=5s` either side can SQLITE_BUSY and abort.
  2. Multiple in-flight LLM streams (Haiku + doc-comments + hyde concurrently) each fire INSERT OR IGNORE per item; without `begin_write()`, sqlx auto-wraps each statement in its own implicit transaction and commits it individually. That's 1 fsync per row instead of one per batch — already a perf bug, but the data-safety angle is that if the process is killed mid-stream, the partial writes are visible to readers immediately (no transactional grouping).
- **Suggested fix:** Fold the streamed `INSERT OR IGNORE` into a path that goes through `begin_write()` (or a shared queue drained by a single writer thread). For the hot streaming path, a `Mutex<Vec<(id, text)>>` collected for ~200ms then flushed under `begin_write()` keeps the perf cost low while restoring the invariant that all index.db writes serialize through `WRITE_LOCK`.

#### Chunk content change does not invalidate `umap_x` / `umap_y` — cluster view serves stale positions
- **Difficulty:** easy
- **Location:** `src/store/chunks/async_helpers.rs:339-362` (UPSERT) + `src/cli/commands/index/umap.rs:38-228`
- **Description:** v22 added nullable `umap_x`/`umap_y` columns; `cqs index --umap` runs a UMAP projection over current embeddings and writes coords back via `update_umap_coords_batch`. The `cqs serve` cluster view at `src/serve/data.rs:920-1003` reads these coords directly. Problem: the chunk UPSERT in `batch_insert_chunks` lists every column it overwrites on conflict — embedding, embedding_base, parser_version, etc. — but it does NOT touch `umap_x` or `umap_y`. So when content changes (`WHERE chunks.content_hash != excluded.content_hash`), the embedding gets refreshed but the UMAP coords stay frozen. The cluster view then displays the chunk at a position computed from the old embedding, potentially landing it in a wrong cluster (e.g. function rewritten end-to-end keeps its old coords until the user remembers to run `cqs index --umap`). Worse, `cqs serve` has no way to surface the staleness — the `umap_x IS NOT NULL` filter only catches the all-NULL case. Memory rule: invalidation counters / staleness must be enforced at the schema layer; relying on the user to re-run `--umap` is exactly the call-site instrumentation pattern the project explicitly rejected.
- **Suggested fix:** Add `umap_x = NULL, umap_y = NULL` to the ON CONFLICT UPDATE clause when content_hash differs. The cluster view's `IS NOT NULL` filter then correctly reports "needs reprojection." Optionally add a metadata `umap_generation` counter that bumps on chunks delete/insert (mirroring `splade_generation`) so a future `cqs serve` warning can fire when generation > umap_generation_at_projection.

#### `slot_remove` race: read active_slot → list_slots → remove_dir_all is TOCTOU on concurrent promote
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/slot.rs:299-350`
- **Description:** `slot_remove` reads `active_slot` (line 312), reads `list_slots` (line 313), then `fs::remove_dir_all(&dir)` (line 335). Between any two of those steps a concurrent `cqs slot promote` (or another `slot remove`) can mutate `.cqs/active_slot`. Trigger sequence:
  1. Process A enters `slot_remove("foo", force=false)`. `foo` is NOT the active slot per its read at line 312 — read says active is "default".
  2. Process B runs `cqs slot promote foo`, atomically rewriting `active_slot` to "foo".
  3. Process A proceeds past the active-slot guard (line 316) since its snapshot says active="default", and runs `fs::remove_dir_all(slot_dir(... "foo"))`.
  4. The system is now pointing `active_slot` at a slot directory that no longer exists. Subsequent commands hit `SlotError::Empty("foo")` until the user manually runs `cqs slot promote default`.
  Same race with two concurrent `slot_remove` calls competing to remove the only remaining slot. There is no file lock around `.cqs/slots/` lifecycle — `slot_dir` operations rely on plain filesystem semantics.
- **Suggested fix:** Take an exclusive lock on a `.cqs/slots.lock` file at the top of every `slot_*` operation (mirroring the `notes.toml.lock` pattern in `src/note.rs:209-228`). Hold it for the entire read-validate-mutate sequence. Same pattern in `slot_promote` and `slot_create`.

#### Slot legacy migration moves live `index.db-wal` / `-shm` instead of checkpointing first
- **Difficulty:** medium
- **Location:** `src/slot/mod.rs:511-624`
- **Description:** `migrate_legacy_index_to_default_slot` runs idempotently on every `Store::open`. If a legacy `.cqs/index.db` is present and `.cqs/slots/` is absent, it moves `index.db`, `index.db-wal`, and `index.db-shm` (alongside HNSW + SPLADE files) into `.cqs/slots/default/`. But `index.db-wal` may contain uncommitted pages from a *prior* daemon run that crashed before checkpointing. SQLite recovers WAL-mode databases by replaying the WAL on next open — but only if the WAL sits next to the DB on the same inode lineage. After the migration moves all three files atomically the WAL replay still works, but if the moves are NON-atomic (cross-device fallback at `move_file:631-637` does `fs::copy + fs::remove_file`), an interrupt between copying `index.db` and copying `index.db-wal` leaves the new `slots/default/index.db` without its WAL. SQLite reopens that DB and silently truncates / discards uncommitted WAL pages — data loss for any writes that were in flight when the crash occurred.
- **Suggested fix:** Before the migration, open the legacy `index.db` once with `PRAGMA wal_checkpoint(TRUNCATE)` so the WAL is drained into the main file and the sidecars are empty/absent. Only then move files. This makes the multi-file move's failure modes recoverable: the worst case is a partially-moved index.db, which on restart is detected by the legacy path still existing.

#### `model_fingerprint` fallback uses Unix timestamp — every restart misses cache, breaks cross-slot copy invariant
- **Difficulty:** medium
- **Location:** `src/embedder/mod.rs:435-465`
- **Description:** `model_fingerprint()` is the cache key for the cross-slot embeddings cache (per memory: "cross-slot copy by content_hash before A/B reindex saves ~$1-5 in API spend"). The fingerprint is normally a blake3 of the ONNX file. But four error branches fall back to `format!("{}:{}", self.model_config.repo, ts)` where `ts = SystemTime::now()`. Every process restart that hits a fallback writes cache rows under a NEW timestamp, and subsequent reads with a different timestamp miss them. Worse, the cache `(content_hash, model_fingerprint)` PRIMARY KEY treats different timestamps as different models — cross-slot copy by `content_hash` would silently match WRONG embeddings if two slots happened to use the timestamp fallback at different moments (the timestamp fallback for both slots gives different fingerprints, so the cross-slot copy queries would miss the cache entirely; even more concerning is that the fingerprint is used as cache identity across writes, so every fallback embedding becomes orphan, accumulating). The fingerprint is also used in PRAGMA-style metadata records — a stale fingerprint stored in `metadata.embedding_model_fp` will never match a cache write made under the current timestamp.
- **Suggested fix:** Make the fallback deterministic: `format!("{}:fallback:size={}", repo, file_size_or_zero)` with NO time component. A fallback fingerprint that's stable across restarts is strictly better than a "unique" fallback that fragments the cache. Log loudly at `warn!` so users notice the missing-file path was taken; failing the embedder open is a defensible alternative.

#### `write_slot_model` and `write_active_slot` skip parent-dir fsync after rename
- **Difficulty:** easy
- **Location:** `src/slot/mod.rs:237-277` (`write_slot_model`), `src/slot/mod.rs:363-406` (`write_active_slot`)
- **Description:** Both functions write to a temp file, fsync the file (only `write_active_slot` does this — `write_slot_model` only `f.sync_all()`s; both do), then `fs::rename`. Neither fsyncs the parent directory after the rename. On power loss between rename and the next inode/dirent flush, the rename can be lost (returning the user to the previous active_slot or a missing slot.toml). `src/note.rs:304` and `src/audit.rs:149` correctly use `crate::fs::atomic_replace`, which fsyncs both file AND parent dir. The slot writers are the odd ones out — same code shape, weaker guarantees. For `active_slot`, this matters because a `cqs slot promote foo` followed by a power cut can crash the system into seeing the OLD slot active even though `cqs slot promote` returned success — the user re-runs commands assuming the new slot, gets stale results.
- **Suggested fix:** Replace the bespoke temp+rename in `write_slot_model` and `write_active_slot` with a call to `crate::fs::atomic_replace` (the helper already exists for `notes.toml` and `audit-mode.json`). Removes ~20 lines from each function and gives durable rename semantics.

#### Daemon serializes ALL queries through one `Mutex<BatchContext>` — a slow query (LLM batch fetch, large gather) blocks every other reader
- **Difficulty:** medium
- **Location:** `src/cli/watch.rs:1775` (mutex setup), `src/cli/watch.rs:1853-1858` (per-connection thread that takes the mutex)
- **Description:** `cqs watch --serve` wraps the BatchContext in `Arc<Mutex<...>>` and per-connection threads acquire it for the entire `handle_socket_client` dispatch (line 1856 → `handle_socket_client(stream, &ctx_clone)`). All `cqs <cmd>` invocations from the user's shell — search, callers, scout, gather, even `notes list` — block on this single mutex. A slow path (e.g. `cqs llm summary --batch ...` triggering a Claude Batch poll, or a `cqs gather` BFS over a hostile-shape graph that takes 30+ seconds) blocks every other CLI invocation including `cqs --version` if it goes through the daemon. From a data-safety angle the issue is that with the mutex held, any background job that needs to *write* (like `stream_summary_writer` callbacks fired from a parallel LLM thread inside the BatchContext) waits behind reads. With `WRITE_LOCK` held inside the mutex while the daemon's outer mutex is also waiting, a deadlock surface emerges: thread A holds outer mutex + WRITE_LOCK, thread B (LLM stream) wants WRITE_LOCK but can't proceed; if thread A's transaction is waiting on a rayon pool that thread B's host thread serves, A and B both stall.
- **Suggested fix:** Two options that match the project's conventions:
  1. Replace the outer `Mutex<BatchContext>` with `RwLock<BatchContext>`; reader paths take `read()`, the few mutator paths (sweep_idle_sessions, reload notes) take `write()`. Lets concurrent reads parallelize.
  2. Push the mutex inside `BatchContext` to per-resource locks (one for sessions, one for notes cache, etc.) and reach in only for the specific field a query needs.
  Either way, audit `stream_summary_writer` carefully — it must NOT be reachable from inside the daemon mutex without going through the `WRITE_LOCK` discipline.

#### `embedding_cache` schema is identical for both `embedding` and `embedding_base` columns — no separation by purpose
- **Difficulty:** medium
- **Location:** `src/cache.rs:159-171` (schema), `src/store/chunks/async_helpers.rs:319` (writes)
- **Description:** The cache stores `(content_hash, model_fingerprint) → embedding`. v18 added `embedding_base` (raw NL embedding before enrichment) but the cache schema does not record WHICH of the two dual-index columns the cached blob represents. The cache is read at the top of every embed batch (in `Embedder::embed_batch_with_cache` or similar) — if the lookup is for "embedding" but the row was written for "embedding_base" (or vice versa), the wrong vector is returned. In practice today both columns are seeded identically on the initial insert (line 319), so it works; but PR #1040's enrichment pass overwrites `embedding` only, leaving `embedding_base` intact. The next reindex hits the cache by content_hash + fingerprint and gets back... whichever row was written last. There's no `purpose` discriminator. If a future change ever caches the post-enrichment embedding, the cache becomes non-deterministic between purposes.
- **Suggested fix:** Add a `purpose TEXT NOT NULL DEFAULT 'embedding'` column to the cache schema, include it in the PRIMARY KEY and all reads/writes. Costs one migration and one extra bind per query; eliminates the implicit assumption that the same content_hash + fingerprint can only have one meaning.

#### `update_umap_coords_batch` uses TEMP TABLE shared across concurrent calls — DELETE may clear another session's data mid-flight
- **Difficulty:** easy
- **Location:** `src/store/chunks/crud.rs:392-450`
- **Description:** Inside the write transaction, the function does `CREATE TEMP TABLE IF NOT EXISTS _update_umap (...)` then `DELETE FROM _update_umap` (line 401-403). TEMP tables in SQLite are *connection-scoped*, not transaction-scoped: they persist across statements on the same connection. Because the sqlx pool may hand out the same connection to a future `update_umap_coords_batch` call, the second call's `DELETE FROM _update_umap` sees rows from the first call's TEMP table (or the table simply persists across multiple invocations until the connection is dropped). Today this is masked because `WRITE_LOCK` serializes calls. But if a concurrent error path leaves the table populated (e.g. `INSERT INTO _update_umap` succeeds, then the `UPDATE chunks ... FROM _update_umap` fails and the transaction rolls back), the CREATE IF NOT EXISTS sees the leftover, and DELETE clears it — but a DROP TABLE IF EXISTS at the end of the function (line 435-437) is INSIDE the transaction that was just rolled back, so the leftover survives. The next call's INSERT-then-UPDATE then operates on the right shape but the TEMP TABLE is now persistently dirty.
- **Suggested fix:** Either (a) DROP the TEMP TABLE before CREATE so each call starts clean, regardless of prior state; or (b) use a uniquely-named TEMP TABLE (suffix with random u64) and DROP at end-of-function via a Drop guard so failures still clean up. Option (a) is simpler; SQLite TEMP DROP is cheap and the error path is the rare case.

#### Cache `evict()` size-then-DELETE is in a transaction but the per-row `write_batch()` it competes with is NOT under the same lock
- **Difficulty:** medium
- **Location:** `src/cache.rs:408-460` (evict tx with `evict_lock`), `src/cache.rs:354-398` (`write_batch` tx, no `evict_lock`)
- **Description:** The DS2-5 fix moved the `(SELECT size, AVG, DELETE)` triplet inside one transaction with an in-process `evict_lock` mutex serializing concurrent evicts. Good. But `write_batch` runs in its own transaction without acquiring `evict_lock`. Under WAL mode the evict's BEGIN takes a snapshot at evict-start; a concurrent `write_batch` can commit AFTER the SELECT-size step but BEFORE the DELETE step. The evict's DELETE then deletes some rows just inserted, with no signal to the writer. From the writer's perspective, the embedding write succeeded; from a cross-session read it's a cache miss, and the next embed pass repeats the (potentially expensive) embedding computation. Not corruption — but a confusing perf footgun.
- **Suggested fix:** Hold `evict_lock` (or the Tokio equivalent) across writes too — every cache mutation goes through one of the two paths, both acquire the same mutex. Costs a per-batch lock acquire (cheap) and eliminates the silent re-embed loop.
