AkurAI-Build

public
Latest change 2a1193541cf008c406be9f347e44150a58a66130 - docs: preserve the architecture review, backlog, and schema policy by Olafur Bui
# Architect Review 4/10 — Data Layer & SQLCipher (bunfork)

Scope: src/db.rs (1434 lines), migrations/001_init.sql, src/main.rs, src/server.rs handlers. Verified with `cargo test db::` — 14/14 pass, including encrypted backup round-trip, restore preservation, hard-link rejection, and path-replacement detection.

## What is solid (evidence)
- **Handler discipline**: vector upsert/search/delete all run rusqlite via `tokio::task::spawn_blocking` (server.rs:911, 948, 968).
- **Backup/restore safety is exemplary**: descriptor pinning via dev/ino (`ensure_path_matches_file`, db.rs:875), `O_NOFOLLOW`+`O_CLOEXEC` opens (db.rs:961), nlink==1 checks, hard-link publish then verify (db.rs:311), WAL checkpoint + journal DELETE + sidecar checks before restore (db.rs:483–503), staged copy verified before atomic rename.
- **Migration ledger**: full SQL stored per version, exact prefix verification, IMMEDIATE transaction (db.rs:115–140, 710–735); schema validation compares normalized `sqlite_schema` SQL and rejects unexpected objects (db.rs:739–790).
- **Key handling**: `Zeroizing` buffers, raw `sqlite3_key` (db.rs:640), `cipher_memory_security ON`, constant-time compare (db.rs:389), hardened DbConfig/limits (db.rs:583–630).
- **Tenant scoping**: composite PK `(tenant_id, model, id)`, all queries parameterized with both scope columns; server pins tenant/model from CLI.

## Prioritized findings

1. **P1 — Readiness probe blocks the tokio runtime.** `native_ready` (server.rs:763–765) calls `database.check_ready()` (db.rs:145–148) directly in async context. It acquires the shared `std::sync::Mutex<Connection>`; a long search (up to 10M multiply-adds) or a concurrent CLI backup holding BEGIN IMMEDIATE (5s busy_timeout, db.rs:582) can pin a worker thread for seconds. Wrap in `spawn_blocking` like the other handlers.

2. **P1 — Search budget cliff is reachable via normal writes.** `search_vectors` hard-errors when candidate count exceeds `work_limit` (db.rs:207–222): 10,000 vectors at ≤1000 dims, only ~2,441 at 4096 dims. `upsert_vector` enforces no per-(tenant,model,dims) count cap, so a tenant can write past the budget and permanently break search (persistent 4xx/5xx) with no recovery path except deletes. Enforce the cap at upsert time, or make it operationally visible.

3. **P2 — Single connection serializes everything despite WAL.** One `Arc<Mutex<Connection>>` (db.rs:52–55) plus `Semaphore::new(1)` search gate (server.rs:229) means one operation at a time. WAL is enabled but unused for concurrency. Fine at current scale (the "ponytail" comment shows intent, db.rs:212), but a small read-only pool for search is the obvious next step; record the trigger metric.

4. **P2 — `user_version` correctness depends on migration authors.** `migrate()` doesn't set `user_version`; 001_init.sql:1–2 sets it via PRAGMA, and `validate_application_schema` requires `user_version == MIGRATIONS.len()` (db.rs:748–752). A future migration lacking the PRAGMA bump bricks `validate_schema` post-migrate. Set `application_id`/`user_version` programmatically at the end of `migrate()`.

5. **P2 — Zeroization gaps in `load_secret` (db.rs:414–433).** The untrimmed file buffer (`value`, up to 1024 bytes) is dropped unzeroized after `.trim().to_owned()` (db.rs:425); the `env::var` fallback String likewise. Read into a `Zeroizing<String>`/buffer instead.

6. **P2 — Backup cost multiplies full-file scans.** `backup()` runs `validate_application_schema` (quick_check + cipher_integrity_check, db.rs:679–703) on live, temp, and destination — 4–5 full-DB decrypt+HMAC passes per backup (db.rs:284–324); `open_existing` also quick_checks at every server start. Acceptable now; O(N) startup/backup latency cliff for large DBs — consider making post-publish re-verification optional.

7. **P2 — Unbounded blocking-pool queueing on writes.** upsert/delete `spawn_blocking` without a gate (server.rs:911, 968); slow clients can stack up to tokio's 512 blocking threads all parked on one mutex. Authenticated-only mitigates; a write semaphore like `search_gate` would bound it.

8. **Nit — dead check & poison behavior.** `ensure!(candidate_count <= work_limit)` at db.rs:243–246 duplicates the pre-loop check (db.rs:216) inside the same snapshot transaction — unreachable. Poisoned mutex (db.rs:367–371) leaves the server permanently degraded; acceptable since readiness flags it, but document that expectation.

## Verdict
Strong, defensively engineered data layer; no blockers. Fix #1 and decide policy for #2; the rest are hardening/scale follow-ups.