From bc733bef2e335b9cd79fa56b103d04144754e84c Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sat, 15 Aug 2026 14:42:23 +0530 Subject: [PATCH] docs: add Phase 1 SQLite hardening spec (implemented via PRs #47-51) --- plans/PHASE1-SQLITE-HARDENING.md | 224 +++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 plans/PHASE1-SQLITE-HARDENING.md diff --git a/plans/PHASE1-SQLITE-HARDENING.md b/plans/PHASE1-SQLITE-HARDENING.md new file mode 100644 index 0000000..14fc28d --- /dev/null +++ b/plans/PHASE1-SQLITE-HARDENING.md @@ -0,0 +1,224 @@ +# Feature Specification: Phase 1 SQLite Hardening + +**Status:** Implemented — merged via five PRs (#47–#51), all CI green +**Author:** Patel230 +**Date:** 2026-08-15 +**Repos affected:** `GrayCodeAI/yaad` (via hawk submodule `external/yaad`) + +## Problem Statement + +Yaad's persistence layer (`storage/`) is a single SQLite database (`~/.yaad/data/yaad.db`) holding agent memory. The audit identified five hardening gaps: + +1. **No cross-process lock** — two yaad processes can open the same DB concurrently; WAL serializes writes but nothing prevents a second process from starting. +2. **No schema versioning** — `createTables()` is one monolithic DDL blob; schema evolution has to be hand-stitched and is not repeatable. +3. **Minimal PRAGMA tuning** — only `journal_mode(WAL)`, `foreign_keys(ON)`, `busy_timeout` are set (storage/sqlite.go:259). No checkpoint tuning, mmap, cache sizing, or `PRAGMA optimize`; zero observability into storage behaviour. +4. **No vector index** — similarity search over embeddings is a linear scan via `AllEmbeddings`/cosine in Go (storage/vectors.go). +5. **No backup** — no way to take a consistent snapshot of the memory DB. + +## Proposed Solution + +Five independently reviewable branches off `main`, merged strictly in order: + +| Order | Branch | PR | Content | +|---|---|---|---| +| 1 | `feat/storage-process-lock` | #47 | flock(2) cross-process lock | +| 2 | `feat/storage-migrations` | #48 | versioned, transactional schema migrations | +| 3 | `feat/storage-pragma-metrics` | #49 | PRAGMA tuning + Prometheus metrics | +| 4 | `feat/storage-hnsw` | #50 | HNSW approximate nearest-neighbor index | +| 5 | `feat/storage-backup` | #51 | `VACUUM INTO` online backup | + +All changes are additive; no existing behaviour changes for current DB files (migrations bring forward-only changes via `CREATE … IF NOT EXISTS` / `ALTER` with defaults). + +## Design Details + +### 1. Process lock (`feat/storage-process-lock`, PR #47) + +**New files (platform-split — CI builds windows/amd64 and `unix.Flock` +does not exist on Windows):** +- `storage/lock_unix.go` (`//go:build !windows`) — `ProcessLock` struct; + `AcquireProcessLock(dbPath)` opens `dbPath+.lock` (0600) and takes a + non-blocking `unix.Flock` `LOCK_EX|LOCK_NB` (golang.org/x/sys). `Release()` + unlocks, closes, best-effort removes the lock file. In-memory DSNs return + a no-op lock. +- `storage/lock_windows.go` (`//go:build windows`) — same API backed by + `windows.LockFileEx` with `LOCKFILE_EXCLUSIVE_LOCK|LOCKFILE_FAIL_IMMEDIATELY` + for equivalent non-blocking exclusive semantics; `UnlockFileEx` best-effort + in `Release()`. + +**Modified files:** +- `storage/sqlite.go` + - `Store` struct (:231): add `processLock *ProcessLock` + - `NewStore()` (:240): acquire lock right after directory creation (:251), release on any later error path + - `Close()` (:305): release the lock + +**Acceptance criteria:** +- Second `NewStore` on the same path fails fast with `"database locked by another yaad process"`; first store unaffected. +- Lock is released on `Close()` — a fresh `NewStore` after `Close()` succeeds. +- `:memory:` DSNs never touch the lock path. +- No new goroutines, no polling — pure flock(2). + +**Tests:** `TestProcessLock` in `storage/sqlite_test.go` (open store A → expect error from store B → close A → reopen succeeds). +**Gate:** `GOWORK=off go test ./storage/... -run TestProcessLock -v` + +### 2. Migrations (`feat/storage-migrations`, PR #48) + +**New file:** +- `storage/migrate.go` — + - `type Migration struct { Version int; Up func(*sql.Tx) error }` + - `var migrations []Migration` (ordered, idempotent): + | v | change | + |---|--------| + | 1 | full base `schema` DDL | + | 2 | `idx_nodes_type_project_scope` composite index | + | 3 | `node_metadata` table + `idx_node_metadata_key_value` | + | 4 | `embeddings_hnsw` table + `idx_embeddings_hnsw_model` | + | 5 | `nodes.encrypted` / `nodes.encryption_key_version` columns | + - `(s *Store) Migrate(ctx)` — reads `MAX(version)` from `schema_version` (0 if absent), runs pending migrations in one transaction, records each applied version. + +**Modified files:** +- `storage/sqlite.go` + - schema version table added to base DDL in `createTables()` (:362): `CREATE TABLE IF NOT EXISTS schema_version (version INTEGER PRIMARY KEY, applied_at DATETIME DEFAULT CURRENT_TIMESTAMP)` + - `NewStore()` (:282): call `s.Migrate(ctx)` after `createTables()`, before `newStmtCache` + +**Acceptance criteria:** +- Fresh DB: all 5 migrations applied exactly once; re-open is a no-op. +- Existing pre-migration DB: opens cleanly; pending migrations applied in order inside a tx (atomic — partial failure leaves version unchanged). +- `PRAGMA integrity_check` clean after migration. + +**Tests:** migration idempotency + partial-failure rollback in `storage/sqlite_test.go` (`TestMigrations*`). +**Gate:** `GOWORK=off go test ./storage/... -run TestMigration -v` + +### 3. PRAGMA tuning + metrics (`feat/storage-pragma-metrics`, PR #49) + +**New files:** +- `storage/metrics.go` — promauto-registered instruments (`yaad_storage_ops_total{op,result}`, `yaad_storage_query_duration_seconds{op}`, plus WAL/db size gauges); one `observe()` helper wrapping hot paths. +- `applyPragmas(db *sql.DB) error` — executed once per connection via `db.Conn(ctx)` so pooled conns inherit the settings: + + | PRAGMA | Value | Why | + |---|---|---| + | `synchronous` | `NORMAL` | safe under WAL, ~2x write throughput | + | `wal_autocheckpoint` | `1000` (pages) | prevent unbounded WAL growth | + | `temp_store` | `MEMORY` | temp tables/sorts off disk | + | `mmap_size` | `268435456` (256 MiB) | read-heavy workload | + | `cache_size` | `-32768` (32 MiB) | larger page cache | + | `page_size` | `4096` | matches default; explicit for determinism | + | `recursive_triggers` | `ON` | cascade correctness | + | `foreign_keys` | `ON` | (also set via DSN; belt-and-braces) | + | `optimize` | run on `Close()` | lets SQLite update planner stats | + +**Modified files:** +- `storage/sqlite.go` + - DSN (:259): keep existing three pragmas (they only stick via DSN on modernc.org/sqlite for all pooled connections) + - `NewStore()`: after `PingContext` (:264), call `applyPragmas(db)` + - `Close()` (:305): `PRAGMA optimize` before closing +- `go.mod`: add `github.com/prometheus/client_golang` (already fetched via `go get`) +- Hot paths (sqlite.go/sqlite_nodes.go/sqlite_edges.go read/write helpers): wrap with metrics observe + +**Acceptance criteria:** +- `PRAGMA synchronous` reports `1` (NORMAL) on every pooled connection (`SELECT` via fresh connections from the 5-conn pool). +- `yaad_storage_ops_total` increments per operation; histogram observes latency. +- No behaviour change for callers; metrics are read-only observability. + +**Tests:** `TestPragmas` (assert pragmas on multiple pooled conns), `TestMetricsRecorded` (run ops, gather from prometheus registry, assert counters > 0). +**Gate:** `GOWORK=off go test ./storage/... -run 'TestPragmas|TestMetrics' -v` + +### 4. HNSW vector index (`feat/storage-hnsw`, PR #50) + +**New file:** +- `storage/hnsw.go` — + - `HNSWIndex{vectors, graph map[string]map[int][]string, maxLevel, entryPoint, efConstruction=200, m=16, efSearch=50, rng}` + - `Build(ctx, s, model)` — loads `AllEmbeddings(ctx, model)`, inserts in sorted ID order (deterministic build), persists graph via `persist()` into `embeddings_hnsw` (upsert) + - `insert()` — greedy entry-point descent from top level, heap-based `searchLayer` (min-heap candidates / max-heap results), neighbor pruning to `m` + - `Search(query, k, ef)` — level descent then layer-0 beam search, returns IDs + cosine scores descending + - `cosineSim(a, b []float32)` shared with vectors.go semantics + +**Modified files:** +- `storage/interface.go`: `BuildHNSWIndex(ctx, model) error`, `SearchHNSW(ctx, model, query, k, ef) ([]string, []float32, error)` added to `Storage` +- `storage/sqlite.go`: + - `Store` struct (:231): `hnswMu sync.Mutex`, `hnswIndexes map[string]*HNSWIndex` (per-model — vectors from different embedding models occupy incompatible spaces) + - `createTables()` (:473, after `embeddings`): `embeddings_hnsw` table (`node_id PK`, `vector BLOB`, `model TEXT`, `neighbors TEXT` JSON, `updated_at`) +- `storage/vectors.go`: `Store.BuildHNSWIndex` / `Store.SearchHNSW` (build-on-demand under `hnswMu`) +- `storage/sqlite_tx.go`: both methods return `errors.New("hnsw … not supported inside a transaction")` — index lives outside txs and is rebuilt on demand +- `storage/mock.go`, `storage/interface_test.go` (mockStorage), `engine/engine_mock_storage_test.go` (engine mockStorage): implement the two new methods + +**Acceptance criteria:** +- `SearchHNSW` returns exact-match vector first, scores in descending order, `len(ids)==k` when corpus ≥ k. +- Per-model isolation: building model B doesn't disturb model A's results. +- Transaction rejection: `SearchHNSW` inside `WithTx` returns a clear unsupported error. +- Deterministic within a process (sorted insertion order + seeded rng). + +**Tests:** `TestHNSWIndex` in `storage/sqlite_test.go` (10-node corpus, angle-distinct vectors — NOT collinear, since cosine ties break ranking; exact-match query; second model build-on-demand). +**Gate:** `GOWORK=off go test ./storage/... -run TestHNSWIndex -v` + +### 5. Backup (`feat/storage-backup`, PR #51) + +**New file:** +- `storage/backup.go` — `Store.Backup(ctx, backupPath)`: + 1. Validate path non-empty; `os.MkdirAll(dir, 0o700)` + 2. `VACUUM INTO ?` to a temp file (`backupPath..tmp`) via `s.withTimeout` + 3. `Chmod` temp to 0600, then `os.Rename` into place (atomic) + 4. On any error: remove the temp file + + `VACUUM INTO` reads the live DB through WAL without blocking writers beyond a moment and emits a compact standalone file — no need to copy `-wal`/`-shm`. + +**Modified files:** +- `storage/interface.go`: `Backup(ctx context.Context, backupPath string) error` added to `Storage` (documented: atomic temp+rename, owner-only) +- `storage/sqlite_tx.go`: `txStore.Backup` returns `"backup is not supported inside a transaction"` +- `storage/mock.go`: `MockStorage.Backup` writes an empty file at the path (honours injected error) +- `storage/interface_test.go`, `engine/engine_mock_storage_test.go`: no-op `Backup` on both test mocks + +**Acceptance criteria:** +- Backup file exists, non-empty, permissions exactly 0600. +- Backup is a standalone consistent DB: `NewStore(backupPath)` opens it and reads seeded nodes back. +- Empty path → error; no partial file left on failure. +- Rejected inside `WithTx`. + +**Tests:** `TestBackup`, `TestBackupEmptyPath`, `TestBackupInsideTx` in `storage/backup_test.go`. +**Gate:** `GOWORK=off go test ./storage/... -run TestBackup -v` + +## Implementation Plan + +### Phase 1 (this spec) — DONE, merged +- [x] `feat/storage-process-lock` — PR #47 (`b30fa11`) +- [x] `feat/storage-migrations` — PR #48 (`58d3eef`) +- [x] `feat/storage-pragma-metrics` — PR #49 (`09ccc71`) +- [x] `feat/storage-hnsw` — PR #50 (`6c03254`) +- [x] `feat/storage-backup` — PR #51 (`dfedbce`) + +### Phase 2 (future, out of scope here) +- Incremental HNSW updates on `SaveEmbedding`/`DeleteEmbedding` (currently full rebuild) +- Persist-and-restore of the HNSW graph across restarts (read `embeddings_hnsw`) +- Backup rotation / scheduled backups from the daemon +- Apply encryption columns (migration v5) with a real key provider + +## Testing Strategy + +Per-branch gates above, plus after each merge: + +```bash +# Yaad (submodule — parent go.work must be detached) +GOWORK=off gofmt -l . # must print nothing +GOWORK=off go vet ./... +GOWORK=off go test ./... # full suite, ~30s + +# Hawk parent — must still build after storage/ interface changes +make build # from hawk repo root +``` + +All five branches individually pass the full `GOWORK=off go test ./...` suite (verified 2026-08-15). + +## Risks & Mitigations + +| Risk | Impact | Mitigation | +|------|--------|------------| +| Merge-order conflicts: later branches each carried a copy of the lock files (`lock_unix.go`/`lock_windows.go`, split for Windows CI) before #47 landed; `sqlite.go`/`sqlite_test.go` touched by several branches | med | Merged strictly #47→#48→#49→#50→#51; resolved by keeping the union (all use `IF NOT EXISTS` / additive edits) | +| flock unavailable on some platforms (e.g. NFS mount) | low | `AcquireProcessLock` returns wrapped error; host can fall back to advisory lock later | +| `VACUUM INTO` requires temp space ≈ DB size | low | Documented; backup path is caller-controlled | +| HNSW full rebuild cost grows with corpus | med | efConstruction/m constants tuned; Phase 2 adds incremental updates | +| `modernc.org/sqlite` DSN pragma quirks | low | Per-connection pragmas applied via `db.Conn(ctx)` in `applyPragmas`; DSN kept for pool-wide settings | + +## References + +- PRs: [#47](https://github.com/GrayCodeAI/yaad/pull/47) · [#48](https://github.com/GrayCodeAI/yaad/pull/48) · [#49](https://github.com/GrayCodeAI/yaad/pull/49) · [#50](https://github.com/GrayCodeAI/yaad/pull/50) · [#51](https://github.com/GrayCodeAI/yaad/pull/51) +- SQLite docs: [WAL](https://www.sqlite.org/wal.html), [VACUUM INTO](https://www.sqlite.org/lang_vacuum.html), [PRAGMA optimize](https://www.sqlite.org/pragma.html#pragma_optimize) +- HNSW paper: Malkov & Yashunin, arXiv:1603.09320