Bläddra i källkod

release(v2.2.1): three bug fixes from shadowman code review

The code-reviewer agent found three real issues in the v2.0→v2.2.0
commit range. All fixed; full suite (12/12) green.

## Fix #1 (critical) — updateIfVersion vector handling

`MemoryStore::updateIfVersion` did not call `extractVector` /
`storeVector` / `mirrorVectorToDocStore` like every other write path.
Consequence: an optimistic-lock update on a vector collection updated
the LMDB doc body but left both the in-memory `coll.vectors` map AND
the LMDB `_vectors_<collection>` sub-db stale. `SimilaritySearch`
returned an old score for the doc; `Get` returned the new body. The
v2.0 substrate made the divergence observable across handlers; v1.x
hid it because both stores were wrong in the same way.

Now matches the shape of `update()` — extract, then either storeVector
+ mirrorVectorToDocStore(UPDATE) when non-empty, or removeVector +
mirrorVectorToDocStore(DELETE) when empty.

## Fix #2 (high) — backfill schema-version marker

`DatabaseService::backfillIntoDocStore` walked every MemoryStore doc
into LMDB at boot but never set the `_meta.schema_version=2` marker
that `migration_complete()` checks. Every restart re-backfilled the
entire dataset. On Zoe (~1.85M docs) this was minutes of pre-READY
blocking per restart, including crash-recovery restarts where the
mirror was already healthy.

New: `smartbotic::db::storage::mark_migration_complete(env)` is exposed
in the public header (delegates to the existing anon-namespace
`mark_complete` helper). The backfill calls it on the success path
(`total_failures == 0`); the failure path explicitly does NOT mark, so
the next boot retries.

## Fix #3 (medium) — SimilaritySearch error classification

When `scan_vectors` saw a dimension mismatch in the first iteration
AND then encountered an unrelated throw (e.g. `bad_alloc` from the
top-K heap), the catch block returned `INTERNAL` masking the real
INVALID_ARGUMENT cause. Hoisted `dim_mismatch` + `mismatch_msg` to the
try-block scope and check them in the catch — INVALID_ARGUMENT wins
when both signals are present, since the dim mismatch is the operator-
actionable root cause.

## Tests

Full suite stays at 12/12. No new test exercises the updateIfVersion
vector path explicitly yet — that's a follow-up.
fszontagh 2 månader sedan
förälder
incheckning
b9a2fa14b7

+ 1 - 0
CLAUDE.md

@@ -41,6 +41,7 @@ cmake --build build -j$(nproc) && ./build/tests/test_vector_storage
 - **Auto-backup before deb upgrade** — `apt upgrade smartbotic-database` runs a `preinst` hook that snapshots `/var/lib/smartbotic-database/` to `/var/backups/smartbotic-database/pre-upgrade-<ts>-from-<version>/` via `cp -a` after the old `prerm` stops the service. Retention `3`, 1.2× free-space precheck on the backup volume (aborts the upgrade if short). Opt out with `SMARTBOTIC_DB_SKIP_BACKUP=1 apt upgrade`. Tunables: `SMARTBOTIC_DB_BACKUP_RETENTION`, `SMARTBOTIC_DB_BACKUP_ROOT`.
 - **yyjson hot-path parsing (v1.10.0+)** — every JSON parse on WAL replay, snapshot deserialize, history reads, gRPC request bodies, and replication apply goes through `smartbotic::db::parse_to_nlohmann` (yyjson-backed) instead of `nlohmann::json::parse`. Measured 2.80× speedup on Zoe-shape corpora (136ms→49ms on 10k docs). In-memory representation stays `nlohmann::json` until Phase B. Wire and on-disk JSON bytes unchanged. New runtime dep: `libyyjson0`. Bench: `./build/tests/bench_json_parse <corpus.jsonl>`.
 - **Binary-backed Document storage (v1.11.0+)** — `Document::data` is now a `smartbotic::db::doc_binary::Doc` (yyjson `mut_doc` wrapper) with a lazy `nlohmann::json` view cache. New accessor surface: `doc.field(name)` is the O(field-count) fast-path that reads one top-level field without materialising the full tree (the dominant read pattern), `doc.data()` is the lazy full-tree accessor, `doc.set_data(j)` re-encodes from an nlohmann tree. Wire and on-disk JSON bytes unchanged. **Honest note:** per-doc heap footprint is ~1.5% smaller than v1.10 (1618→1594 bytes on Zoe-shape) — the win is field-access CPU + Phase C scaffolding, not RSS. See `docs/superpowers/specs/2026-05-15-binary-doc-format-decision.md` post-implementation amendment for the bench numbers. Bench: `./build/tests/bench_doc_memory --mode=<nlohmann|binary> [N]`.
+- **v2.2.1 bug fixes from shadowman code review.** Three issues found by a code-review pass on the v2.0→v2.2.0 commit range, all now fixed: (1) `MemoryStore::updateIfVersion` was missing `extractVector` + `storeVector` + `mirrorVectorToDocStore` calls — optimistic-lock updates on vector collections left the vector mirror stale, so `SimilaritySearch` returned old vector scores while `Get` returned new doc bodies; (2) `DatabaseService::backfillIntoDocStore` walked every doc into LMDB but never set `_meta.schema_version=2`, so every boot re-backfilled the entire dataset (Zoe-scale ~1.85M docs = minutes of pre-READY blocking on every restart); (3) `SimilaritySearch` LMDB handler returned `INTERNAL` instead of `INVALID_ARGUMENT` when an unrelated throw (e.g. bad_alloc) fired after a dimension-mismatch was already observed in the scan. Exposes `smartbotic::db::storage::mark_migration_complete(env)` for the backfill marker. Tests 12/12 green.
 - **v2.2.0 — default timestamp precision flipped to `"ns"`.** BREAKING for fresh collections. `CollectionCfg::timestampPrecision` default went from `"ms"` to `"ns"` so rapid-write collections (chat messages, metrics events, tool-call audits) get nanosecond stamps and strict ordering without needing an explicit `configureCollection()` call. Closes shadowman audit B5. Operators who need ms can still set it explicitly. Existing collections that already have a stored config record keep their value — only collections created post-upgrade WITHOUT an explicit `configureCollection()` inherit the new default. Mixed-precision rows in the same collection are read-correct via the 10^15 magnitude auto-detect; operators wanting clean rows should run `migrateCollectionTimestamps`. **`update()` docstring** updated to clarify: it OVERWRITES the doc; use `patch()` for partial mutations. Reflects shadowman audit B6.
 - **v2.1.1 cleanup — no defensive fallback on LMDB throw.** Removed the `catch → fall back to MemoryStore` paths in Exists / Get / Find / SimilaritySearch. A live LMDB exception now returns gRPC `INTERNAL` to the client instead of silently being masked by MemoryStore. The gate-closed fallback (`mirror_healthy_=false`, system collection, env unopened) stays — that's correctness, since MemoryStore can legitimately be ahead of LMDB when a mirror write has failed. **Answers shadowman audit F3:** `usedWalFallback` / `walMatchCount` / `memoryMatchCount` / `memorySearchMicros` / `walSearchMicros` are guaranteed zero on the LMDB-served path. Any non-zero value is a real signal — it means the read fell back to MemoryStore (mirror unhealthy). Operators can treat `walMatchCount > 0` as a paging/SLA alert.
 - **v2.1 audit answers (shadowman alignment).** F1 — v1.x auto-migration: stays in-binary through v2.4. Extracts to separate `smartbotic-database-migrate-v1` deb in v2.5 once every production host has booted v2.4 ≥ once. v2.x is the project's terminal arc, no v3 planned. F2 — `setReadOnly` / `lock()` / `unlock()` are dual-audience: operator-side via `smartbotic-db-cli unlock` and client-side via `Client::lock()` for self-defensive migrations. F3 — see above. F4 — `migrateCollectionTimestamps` cost benchmark deferred until shadowman starts D.1 (we haven't seen real traffic on the path yet); back-of-envelope ~1ms per 1k docs on Zoe-shape data, online-safe up to ~1M docs.

+ 1 - 1
VERSION

@@ -1 +1 @@
-2.2.0
+2.2.1

+ 15 - 3
service/src/database_grpc_impl.cpp

@@ -859,6 +859,11 @@ grpc::Status DatabaseGrpcImpl::SimilaritySearch(
         if (!collection.empty() && collection[0] != '_'
             && service_.mirrorHealthy() && service_.mirrorDriftCount() == 0) {
             if (auto* ds = service_.docStore()) {
+                // Hoisted to try-block scope so the catch block can prefer
+                // INVALID_ARGUMENT over INTERNAL when a dim mismatch was
+                // observed before some unrelated throw fired.
+                bool dim_mismatch = false;
+                std::string mismatch_msg;
                 try {
                     // Pre-compute query norm once outside the iteration.
                     const float queryNorm =
@@ -878,8 +883,6 @@ grpc::Status DatabaseGrpcImpl::SimilaritySearch(
                         std::priority_queue<Scored, std::vector<Scored>,
                                             decltype(cmp)> heap(cmp);
                         size_t expectedDim = queryVec.size();
-                        bool dim_mismatch = false;
-                        std::string mismatch_msg;
 
                         ds->scan_vectors(collection,
                             [&](std::string_view id, const float* data,
@@ -952,7 +955,16 @@ grpc::Status DatabaseGrpcImpl::SimilaritySearch(
                     // INVALID_ARGUMENT.
                     throw;
                 } catch (const std::exception& e) {
-                    // v2.1.1 — no defensive fallback on throw; surface INTERNAL.
+                    // v2.2.1 — if the scan saw a dimension mismatch BEFORE
+                    // the throw fired (e.g. bad_alloc on the heap), the
+                    // root cause is the caller's wrong-dimension query, not
+                    // the storage error. Prefer INVALID_ARGUMENT over the
+                    // less-actionable INTERNAL so operators see the real
+                    // cause.
+                    if (dim_mismatch) {
+                        return grpc::Status(grpc::StatusCode::INVALID_ARGUMENT,
+                                            mismatch_msg);
+                    }
                     spdlog::error("v2.1 SimilaritySearch LMDB scan failed coll={}: {}",
                                   collection, e.what());
                     return grpc::Status(grpc::StatusCode::INTERNAL, e.what());

+ 17 - 3
service/src/database_service.cpp

@@ -275,11 +275,25 @@ bool DatabaseService::backfillIntoDocStore() {
     const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
         std::chrono::steady_clock::now() - t0).count();
     if (total_failures == 0) {
-        spdlog::info("v2.0 backfill: complete — {} docs across {} collections in {} ms",
-                     total_docs, collections.size(), elapsed);
+        // v2.2.1 — stamp _meta.schema_version=2 so subsequent boots
+        // short-circuit at migration_complete() and skip the re-backfill.
+        // Pre-2.2.1 the marker was only set by the offline migrate_v1_to_v2
+        // tool, so every restart re-walked the entire MemoryStore.
+        try {
+            smartbotic::db::storage::mark_migration_complete(*env_);
+            spdlog::info("v2.0 backfill: complete — {} docs across {} collections "
+                         "in {} ms (schema_version=2 marker set)",
+                         total_docs, collections.size(), elapsed);
+        } catch (const std::exception& e) {
+            spdlog::warn("v2.0 backfill: completed {} docs in {} ms but the "
+                         "schema_version marker write failed: {} — next boot "
+                         "will re-backfill",
+                         total_docs, elapsed, e.what());
+        }
     } else {
         spdlog::error("v2.0 backfill: completed with {} failures out of {} docs "
-                      "in {} ms — mirror_healthy_=false, reads must NOT flip to LMDB",
+                      "in {} ms — mirror_healthy_=false, reads must NOT flip to LMDB "
+                      "and schema_version marker NOT set (next boot retries)",
                       total_failures, total_docs, elapsed);
     }
     return true;

+ 22 - 1
service/src/memory_store.cpp

@@ -717,6 +717,14 @@ bool MemoryStore::updateIfVersion(const std::string& collection, const std::stri
     updated.updatedAt = currentTimeFor(collection);
     updated.nodeId = config_.nodeId;
 
+    // v2.2.1 — extract _vector + sync coll.vectors (and the LMDB vector
+    // sub-db). Pre-2.2.1 updateIfVersion left _vector embedded in
+    // doc.data() and never touched coll.vectors, which silently broke
+    // SimilaritySearch after every optimistic-lock update on a vector
+    // collection (the v2.0 mirror made the divergence observable
+    // across handlers).
+    auto vec = extractVector(*coll, updated);
+
     // Add to new expiration index
     if (updated.expiresAt > 0) {
         addToExpirationIndex(*coll, id, updated.expiresAt);
@@ -726,11 +734,24 @@ bool MemoryStore::updateIfVersion(const std::string& collection, const std::stri
     it->second = updated;
     coll->updatedAt = updated.updatedAt;
 
+    // Update vector (replace or remove if no new vector provided) — match
+    // the semantics of update().
+    if (!vec.empty()) {
+        storeVector(*coll, id, vec);
+    } else {
+        removeVector(*coll, id);
+    }
+
     // Track memory change (new size)
     uint64_t newSize = estimateDocumentSize(updated);
 
-    // v2.0 dual-write under lock
+    // v2.0 dual-write under lock — both doc body and vector.
     mirrorWriteToDocStore(collection, id, updated, EventType::UPDATE);
+    if (!vec.empty()) {
+        mirrorVectorToDocStore(collection, id, &vec, EventType::UPDATE);
+    } else {
+        mirrorVectorToDocStore(collection, id, nullptr, EventType::DELETE);
+    }
 
     lock.unlock();
 

+ 4 - 0
service/src/storage/migrate_v1_to_v2.cpp

@@ -356,6 +356,10 @@ bool migration_complete(LmdbEnv& env) {
     }
 }
 
+void mark_migration_complete(LmdbEnv& env) {
+    mark_complete(env);  // delegates to the anonymous-namespace helper
+}
+
 bool migration_in_progress(LmdbEnv& env) {
     try {
         auto v = read_meta_key(env, kKeyInProgress);

+ 8 - 0
service/src/storage/migrate_v1_to_v2.hpp

@@ -78,6 +78,14 @@ bool migration_complete(LmdbEnv& env);
 // partial state cleanly via LMDB put-overwrite semantics).
 bool migration_in_progress(LmdbEnv& env);
 
+// v2.2.1 — write the schema_version=2 + completion-timestamp markers and
+// clear any in-progress flag in a single atomic txn. Exposed for the
+// runtime `DatabaseService::backfillIntoDocStore` path: a successful
+// backfill leaves the env in the same observable state as a successful
+// migration, so subsequent boots short-circuit at `migration_complete()`.
+// Idempotent — safe to call after every backfill.
+void mark_migration_complete(LmdbEnv& env);
+
 // -------------------------------------------------------------------------
 // Task 3.2 — auto-detect-on-boot helper.
 // -------------------------------------------------------------------------