## Data Safety

#### DS2-1: `prune_missing` reads origin list OUTSIDE write transaction — same TOCTOU that P2 #32 closed for `prune_all`
- **Difficulty:** easy
- **Location:** `src/store/chunks/staleness.rs:76-90`
- **Description:** The previous audit closed the Phase 1 / Phase 2 race in `prune_all` by moving the `SELECT DISTINCT origin FROM chunks` inside the `begin_write()` transaction. `prune_missing` still does the reverse: it fetches `rows` from `&self.pool` at line 76, computes `missing` from that snapshot, and only *then* calls `begin_write()` at line 102. During the gap `cqs watch` (which only holds `try_acquire_index_lock`, not the DB `WRITE_LOCK`) can call `upsert_chunks_and_calls` for a freshly-added file. That file is missing from the caller-supplied `existing_files` snapshot but present in `chunks`; the Phase 2 DELETE will wipe the just-added rows. `prune_missing` is called by both the daemon startup GC (`cli/watch.rs:944`) and the periodic GC (`cli/watch.rs:1026`), so the window is reachable every time either fires.
- **Suggested fix:** Move the `SELECT DISTINCT origin FROM chunks` call inside the `begin_write()` transaction (swap the order at line 75 and 102). Mirror the `prune_all` layout exactly — take the write lock first, then fetch distinct origins from `&mut *tx` so the DELETE operates on the same snapshot the read observed.

#### DS2-2: `prune_gitignored` reads origin list OUTSIDE write transaction — same class as DS2-1
- **Difficulty:** easy
- **Location:** `src/store/chunks/staleness.rs:333-337`
- **Description:** `prune_gitignored` (called on every daemon startup via `run_daemon_startup_gc` and every 30 min via `run_daemon_periodic_gc`) runs `SELECT DISTINCT origin FROM chunks` on `&self.pool` outside a transaction (line 336), then later opens `begin_write` at line 374 and does batched DELETEs. If `cqs watch`'s reindex commits between the two phases for a newly-gitignored path (rare but possible via staged .gitignore edits), the delete set reflects the stale Phase-1 snapshot and a just-written chunk survives the prune — or, more consequentially, a chunk whose matcher result changed in the gap gets deleted even though its `chunks` row was refreshed in the interim. The function comment at line 331 even acknowledges "Rust-side filter, outside tx so the matcher walk doesn't hold the write lock" as if the hazard were a deliberate trade — but the matcher walk is microseconds on ~10k origins, not a meaningful lock hold.
- **Suggested fix:** Open the `begin_write` transaction first and issue the `SELECT DISTINCT origin` against `&mut *tx` (as `prune_all` now does). The matcher walk then proceeds inside the tx — trivial cost compared to the DELETE phase that already runs there.

#### DS2-3: `set_metadata_opt` and `touch_updated_at` bypass `WRITE_LOCK` — torn-write race against `begin_write()` transactions
- **Difficulty:** easy
- **Location:** `src/store/metadata.rs:409-418` (`touch_updated_at`), `:452-475` (`set_metadata_opt`)
- **Description:** `set_hnsw_dirty` at line 436-449 correctly uses `begin_write()` (added by DS-V1.25-3) to serialize against every other in-process writer. The sibling functions `touch_updated_at` and `set_metadata_opt` still execute raw pool writes: `sqlx::query(...).execute(&self.pool)` with no `WRITE_LOCK` guard. Two in-process writers — e.g. the daemon's watch loop calling `touch_updated_at()` at `cli/watch.rs:2590` and the pipeline path simultaneously running `upsert_chunks_and_calls` — can both have deferred transactions open, and SQLite returns `SQLITE_BUSY` when one of them tries to upgrade to exclusive. `WRITE_LOCK` was added specifically to prevent this — `set_metadata_opt` is called by pending-batch ID setters (`set_pending_batch_id`, `set_pending_doc_batch_id`, `set_pending_hyde_batch_id`) used by long-running LLM batch polling, which are exactly the paths where a concurrent reindex is likely.
- **Suggested fix:** Rewrap both in `let (_guard, mut tx) = self.begin_write().await?` + execute against `&mut *tx` + `tx.commit()`. Same pattern as `set_hnsw_dirty`. The write is a one-statement UPSERT/DELETE, so the transactional overhead is negligible, and the invariant "every in-process mutation takes WRITE_LOCK" becomes structural instead of "call the right setter."

#### DS2-4: Phantom-chunks DELETE is in a separate transaction from the chunk upsert that precedes it — mid-batch crash leaves the index inconsistent with disk
- **Difficulty:** medium
- **Location:** `src/cli/watch.rs:2568-2579`
- **Description:** For each file in the watch cycle, the loop runs `store.upsert_chunks_and_calls(pairs, ...)?` followed by `store.delete_phantom_chunks(file, &live_ids)?` — each in its own independent write transaction. The source code comment at line 2574-2577 explicitly acknowledges "Ideally this would share a transaction with `upsert_chunks_and_calls`, but both methods manage their own internal transactions. A crash between the two leaves phantoms that get cleaned on the next reindex." This is hand-waved as tolerable because the next reindex will clean it, but there's a worse class of failure mode: if the daemon crashes between the upsert (which marks `hnsw_dirty=1` at `watch.rs:2179`) and any subsequent HNSW rebuild — a long window on large batches — the HNSW has been flagged dirty but phantoms still exist and will match search queries against IDs that are about to be deleted. The `hnsw_dirty` flag is only cleared after a full rebuild following the final file; if the process is SIGKILLed mid-batch, we serve queries against a half-pruned index for the next daemon boot until the first event triggers a rebuild.
- **Suggested fix:** Extend `upsert_chunks_and_calls` to accept a `live_ids: Option<&[&str]>` argument that, when present, performs the phantom delete inside the same tx — mirroring how `prune_all` does four logical operations in one transaction. Alternative: add a single `upsert_file_chunks(file, chunks_with_embeddings, calls, live_ids)` that wraps the whole per-file delta in one BEGIN/COMMIT, and let `cli/watch.rs:2538-2580` call it per file instead of the two-step dance.

#### DS2-5: `EmbeddingCache::evict` is TOCTOU — SELECT size / AVG runs on pool, DELETE runs on pool — concurrent writers invalidate the eviction decision
- **Difficulty:** easy
- **Location:** `src/cache.rs:352-400`
- **Description:** `evict()` queries `SELECT SUM(LENGTH) + COUNT*200 FROM embedding_cache`, then `SELECT AVG(LENGTH + 200)`, then `DELETE WHERE rowid IN (SELECT ... LIMIT ?)` — all three on `&self.pool`, each in its own implicit transaction. Between the size measurement and the DELETE, a concurrent `write_batch` (which uses `pool.begin()` with no shared lock; the cache has its own pool and no cross-call WRITE_LOCK equivalent) could add 10+ MB of entries; the computed `entries_to_delete` is then wrong, and we evict based on a stale snapshot. Under the daemon's periodic-evict-while-writing pattern this is the hot path: evict runs alongside `write_batch` in the same tokio runtime. Worse, two `evict()` calls from different threads can both decide to delete from the same `LIMIT ?1` prefix — the `ORDER BY created_at ASC` makes the deletes overlap and each caller's `rows_affected()` count is larger than the per-call contribution. `QueryCache::evict` at `:1103-1147` has the identical shape and the identical bug.
- **Suggested fix:** Wrap all three SELECTs + the DELETE in a single `pool.begin()` transaction so the size measurement, AVG computation, and DELETE see one consistent snapshot. Add an in-process `Mutex<()>` guard (either on the `EmbeddingCache` struct or a process-global like SQLite's `WRITE_LOCK`) so two evicts can't race at all. Since `evict()` is idempotent to some extent, this is low-risk — but the mis-accounting currently shows up as "evicted more than needed" ratchets during parallel reindex loops.

#### DS2-6: HNSW save's `.bak` rename sequence writes `.bak` without fsync before `atomic_replace` — power cut between step-3 rename and step-4 atomic_replace can lose the directory entry
- **Difficulty:** medium
- **Location:** `src/hnsw/persist.rs:414-426` (backup rename) + `:429-461` (atomic_replace)
- **Description:** Save sequence: (1) build temp_dir with new files + checksums; (2) acquire lock; (3) for each ext, `std::fs::rename(final, bak)` at line 418 — plain `rename`, no fsync of the parent dir after; (4) for each ext, `atomic_replace(temp, final)` which does fsync the parent. Between step 3 and step 4, the parent directory's rename-to-`.bak` entries are NOT fsynced. On a power cut with ext4's default `data=ordered` we're likely fine because journal ordering commits the rename entry before subsequent writes — but the exact guarantee depends on the filesystem. If step 4 fails after step 3 completed, rollback path at line 465-487 tries `std::fs::rename(bak, final)` to restore — again no fsync on the restore rename, and no fsync of the parent dir after the rollback completes. A second power cut during rollback can leave the index with missing files even though the .bak existed on disk. The intermediate window is small but real: a multi-file rollback (graph + data + ids + checksum) takes multiple rename syscalls.
- **Suggested fix:** After the back-up loop at line 418-425 but before the atomic_replace loop, open the parent `dir` and call `sync_all()` once. Same at the end of the rollback restore loop at line 471-483. A single fsync amortized across 4 renames is negligible overhead compared to the HNSW save's cost (which is dominated by graph+data writes, hundreds of MB). The `atomic_replace` helper already does this for its own pass; the backup pre-pass just needs the same treatment.

#### DS2-7: HNSW incremental insert + save is not atomic against the `hnsw_dirty` metadata flag — crash leaves `dirty=1` permanently even though the on-disk file was fully written
- **Difficulty:** medium
- **Location:** `src/cli/watch.rs:2326-2338` (incremental save + clear-dirty), `:2250-2257` (full rebuild + clear-dirty)
- **Description:** Both the incremental and the full-rebuild branches clear `hnsw_dirty` only AFTER `index.save(...)` succeeds. If the save itself succeeds but `set_hnsw_dirty(Enriched, false)` fails (transient SQLITE_BUSY, daemon read-only mode racing with a manual `index --force`, etc.) we log a warning and continue. Now the on-disk HNSW is actually fresh but the metadata says "dirty," so every subsequent reopen of the Store at `src/cli/store.rs:289` will see `is_hnsw_dirty() == true` and rebuild from scratch — paying 10s-1m of full-graph-rebuild cost per daemon restart. More importantly, in the reverse ordering (`watch.rs:2179-2186`) the flag is set to `dirty=1` before the chunks write, then the write happens, then we try to clear. If the process is SIGKILLed between the chunks commit and the HNSW save, the next daemon load sees dirty + stale HNSW and correctly rebuilds — that path is safe. It's the "save succeeded but clear-dirty failed" path that is unrecoverable without a manual reindex.
- **Suggested fix:** The clear-dirty is a single metadata UPDATE; if it returns an error, retry once (with `begin_write` retry semantics) before giving up. Alternatively, expose `set_hnsw_dirty_after_save(...)` that reads the on-disk checksum and only reports "dirty" if checksum mismatches actual file — moving the invariant from metadata-must-agree-with-filesystem to filesystem-is-source-of-truth. Simpler fix: tie the clear-dirty to a blake3 of the saved files; on reopen, if the blake3 matches the stored value, treat as clean regardless of the dirty flag. That turns the dirty flag into an optimization rather than a correctness requirement.

#### DS2-8: `v18→v19` migration drops orphan sparse_vectors rows WITHOUT updating schema_version in the same transaction — rollback on error leaves half-migrated state (prior to P1 #16 UPSERT fix)
- **Difficulty:** medium (but may already be mitigated by backup-restore path)
- **Location:** `src/store/migrations.rs:478-562` (`migrate_v18_to_v19`) + `:197-203` (UPSERT of schema_version)
- **Description:** The `migrate_v18_to_v19` function does (in order inside the tx): CREATE sparse_vectors_v19, INSERT...SELECT from sparse_vectors INNER JOIN chunks (drops orphans), DROP INDEX idx_sparse_token, DROP TABLE sparse_vectors, ALTER TABLE RENAME to sparse_vectors, CREATE INDEX idx_sparse_token, bump splade_generation. Then at `run_migration_tx` line 197-203 the caller UPSERTs schema_version. These are all in one tx, so SQLite rollback-on-error is safe per-SQLite. But the v18→v19 migration is destructive: it PERMANENTLY drops orphan sparse_vectors rows as part of the copy (the INNER JOIN filter at line 508). If the migration fails AFTER the INSERT step (say, during the ALTER TABLE RENAME or the CREATE INDEX), the rollback correctly reverts everything — but the filesystem backup at `src/store/backup.rs:107` taken before the migration is the only recovery path. If that backup step itself silently failed (because `CQS_MIGRATE_REQUIRE_BACKUP` is not set, which is the default), the log says "Migration backup failed; proceeding without snapshot" and then the migration proceeds. A subsequent non-transactional error — e.g. a commit-time IO failure that SQLite can't fully reverse — leaves the user with a partially-migrated DB and no backup to restore from. The happy-path case is fine; the no-backup + non-transactional-commit-failure case is catastrophic.
- **Suggested fix:** Flip the default: `CQS_MIGRATE_REQUIRE_BACKUP` should default to `1`, not `0`. The current default ("proceed without snapshot") is the correct choice only for users on filesystem-full scenarios; for everyone else, a backup failure is a signal to abort before doing anything destructive. Alternative: add a pre-migration `SELECT COUNT(*) FROM sparse_vectors` vs `SELECT COUNT(*) FROM sparse_vectors_v19` assertion that refuses to DROP the old table if the INSERT filter removed rows we weren't expecting to lose. Log a `tracing::error!` and require `CQS_ALLOW_ORPHAN_DROP=1` env to proceed.

#### DS2-9: `upsert_sparse_vectors` drops the secondary `idx_sparse_token` index INSIDE the write tx — a panic after the DROP but before the CREATE leaves the index without the token_id B-tree, killing SPLADE search on reopen
- **Difficulty:** medium
- **Location:** `src/store/sparse.rs:113-117` (DROP) + `:193-196` (CREATE INDEX at the end)
- **Description:** The bulk-load pattern DROP INDEX → INSERT in batches → CREATE INDEX is standard SQLite optimization, and because it's all inside one `begin_write()` transaction, SQLite's rollback semantics correctly revert both the DROP and any INSERT on error. HOWEVER: `sqlx::Transaction` holds an `&mut SqliteConnection`. The inner loop at line 162-177 does `qb.build().execute(&mut *tx).await?` — a network or I/O error on any batch returns early, the `?` propagates, and `tx` is dropped implicitly which triggers a rollback ONLY if the connection is still alive. Under panic unwind (e.g. a downstream `split_at_unchecked` OOB panic when weight is NaN), the rollback also fires via `Drop`. What does NOT fire a rollback: if `upsert_sparse_vectors` itself is canceled asynchronously (e.g. the tokio runtime dropped by CTRL-C while this function is mid-transaction under `rt.block_on`). `block_on` is synchronous, so `Drop::drop(tx)` runs, and SQLite should roll back — so this is actually safe in principle. But the failure mode I'm worried about: if the process is SIGKILLed while the tx is open, SQLite's WAL recovery on next open will see the uncommitted INSERTs as unreachable in the WAL and discard them — the DDL (DROP INDEX / CREATE INDEX) that SQLite treats as a schema-altering operation likewise rolls back via WAL. Net: safe under kill-9. The real concern is more subtle: if an admin `kill -9`s the process after the transaction committed BUT BEFORE the CREATE INDEX statement fired (impossible in practice because they're both in the same tx, but I'm exhaustively exercising the concern) — OK, this case cannot happen. The legitimate residual issue: if the bulk insert fails and returns early, the transaction rolls back, but `bump_splade_generation_tx` at line 202 was also rolled back. Net: no generation bump means the on-disk `splade.index.bin` from a prior run is still trusted, while `sparse_vectors` rows were never added. Readers see stale SPLADE results until the next successful upsert bumps the counter.
- **Suggested fix:** On `upsert_sparse_vectors` error, call `bump_splade_generation` (a standalone function, NOT inside any tx) to force invalidation of the on-disk file that could now be out-of-step with a rollback. This converts "rollback + stale on-disk file" into "rollback + regenerated on-disk file," which is the correct invalidation direction. Add the cleanup to the error path of the function itself, outside `self.rt.block_on(async { ... })`.

#### DS2-10: `as_millis() as i64` mtime cast silently truncates — if a user's system clock jumps ~292 million years forward or if a pathological filesystem returns mtime beyond `i64::MAX` milliseconds, comparisons against stored_mtime wrap silently and flag fresh files as stale
- **Difficulty:** easy
- **Location:** `src/cli/watch.rs:2556`, `src/lib.rs:454`, `src/store/chunks/staleness.rs:511,601,674,834,879,1332,1395`, `src/store/notes.rs:150,354`
- **Description:** `Duration::as_millis()` returns `u128`. The cast `as i64` silently wraps when the value exceeds `i64::MAX` (year 2262-ish) — for realistic wall clock this is never a concern. The real concern is pathological mtime values: a filesystem bug (ZFS snapshot with a bad atime, a clock-jumped VM that briefly set mtime in the far future, WSL 9P's notorious mtime clamping) can return mtimes that survive `duration_since(UNIX_EPOCH)` as a positive `Duration` but overflow `i64::MAX` on the cast. The store compares these as `i64` in `needs_reindex`, `list_stale_files`, `check_origins_stale`, `notes_need_reindex`. A silently-wrapped negative `i64` compares as LESS than any legitimate stored_mtime, which means: (a) `needs_reindex` returns `Some(wrapped_neg_mtime)` because `stored_mtime >= current` is false, triggering a wasteful reindex — benign but noisy; (b) `list_stale_files` puts the file in `stale` because `current > stored` — also benign; (c) `notes_need_reindex` matches the else arm and returns a wrapped `current_mtime` that gets WRITTEN into the `notes.file_mtime` column as a negative i64, permanently polluting the DB until manual cleanup.
- **Suggested fix:** Replace the 13 sites with a shared helper `fn duration_to_mtime_millis(d: Duration) -> i64 { i64::try_from(d.as_millis()).unwrap_or(i64::MAX) }`. Saturating semantics match the "future time is still fresh" invariant, and the one-site-changes-all-callers pattern prevents drift. Defensive but cheap; also makes the cast intent explicit.

Summary: 10 actionable findings covering schema migration risk (DS2-8), TOCTOU in GC paths (DS2-1, DS2-2), torn writes from WRITE_LOCK bypass (DS2-3), atomicity gaps across multi-step watch reindex (DS2-4), cache evict race (DS2-5), HNSW parent-dir fsync gaps (DS2-6), HNSW/SQLite dirty-flag state drift (DS2-7), SPLADE generation bump rollback (DS2-9), and mtime cast truncation (DS2-10). The SPLADE/HNSW/chunks atomicity invariant is strong where the prior audits fixed it (`prune_all`, `set_hnsw_dirty`, `upsert_sparse_vectors`) but the fixes didn't propagate uniformly — two sibling prune functions and two metadata setters still run outside the `WRITE_LOCK` discipline.
