fix(unique): T11 review round 2 - replication caller, stale doc, setRemove, mirror-unhealthy guard

Four findings from spec review, all addressed:

1. applyDualWriteMirror has a third caller: DatabaseService's replicated-entry
   apply path, which runs after loadDocument() already put the row into
   MemoryStore (loadDocument bypasses the persist callback to avoid
   re-replicating). A UniqueViolation there was previously only caught by the
   function-wide generic catch, leaving MemoryStore ahead of LMDB with
   mirror_healthy_ still true - no signal. Fixed by deliberately flipping
   health/drift at that call site instead of undoing loadDocument: there is no
   "reject the whole operation" available on a replication-apply path (the
   origin already committed, and may not even share this node's unique-field
   config), so MemoryStore keeps the row and the mirror is marked unhealthy on
   purpose, restoring the pre-T11 swallow-and-flip outcome for this one caller.

2. document_store_lmdb.hpp's header comment on set_unique_fields still said
   "UNREACHABLE BY DESIGN... no RPC or config key exposes it" - stale after
   T11 activated it. Rewritten to describe the three real call sites and the
   wire/config surface; the original "why this was hard" paragraph kept as
   history.

3. setRemove was exempted from undo/catch with the reasoning "removal-only
   writes add no postings," which only covers the members array field, not
   _version/_updated_at (also bumped by setRemove, also index-declarable).
   Fixed rather than re-documented: setRemove now snapshots and undoes via
   mirrorDocOrUndo like setAdd's update branch, and SetRemove's gRPC handler
   gained the same UniqueViolation -> ALREADY_EXISTS catch every other write
   handler has.

4. Report's undo-path section overstated the version-history imprecision
   ("a version that was never committed") - corrected: saveToHistory runs
   with the pre-write document, so the entry records a real prior state, not
   a phantom. The actual cost is a duplicate version number and one wasted
   max_versions slot, not fabricated history.

Also added the cheap guard from review: CreateIndex refuses a unique
declaration outright while the mirror is unhealthy, since find_duplicate_values
only consults LMDB and MemoryStore can legitimately be ahead of it then.

service/src/database_grpc_impl.cpp

@@ -1590,9 +1590,18 @@ grpc::Status DatabaseGrpcImpl::SetRemove(
     if (store_.pressure() == MemoryPressure::Emergency) {
         return memoryEmergencyStatus("SetRemove", store_);
     }
-    bool removed = store_.setRemove(request->collection(), request->set_id(), request->member());
-    response->set_removed(removed);
-    return grpc::Status::OK;
+    // v2.11.0 T11 — setRemove bumps _version/_updated_at, which are
+    // index-declarable metadata fields, so it can raise UniqueViolation the
+    // same as SetAdd can. Same treatment.
+    try {
+        bool removed = store_.setRemove(request->collection(), request->set_id(), request->member());
+        response->set_removed(removed);
+        return grpc::Status::OK;
+    } catch (const smartbotic::db::storage::UniqueViolation& e) {
+        return grpc::Status(grpc::StatusCode::ALREADY_EXISTS, e.what());
+    } catch (const std::exception& e) {
+        return grpc::Status(grpc::StatusCode::INTERNAL, e.what());
+    }
 }
 
 grpc::Status DatabaseGrpcImpl::SetMembers(
@@ -3235,6 +3244,19 @@ grpc::Status DatabaseGrpcImpl::CreateIndex(
         response->set_error("cannot index a masked field");
         return grpc::Status::OK;
     }
+    // v2.11.0 T11 — find_duplicate_values (below) reads the LMDB index only.
+    // While the mirror is unhealthy, MemoryStore can legitimately be ahead of
+    // LMDB (that's the whole point of the fallback), so the refusal check
+    // could miss duplicates that exist in memory and accept a constraint
+    // that is already false. Refuse outright rather than declare a unique
+    // constraint the data may not actually satisfy.
+    if (request->unique() && !service_.mirrorHealthy()) {
+        response->set_success(false);
+        response->set_error("cannot declare a unique constraint while the LMDB mirror is "
+                            "unhealthy: the duplicate check only consults LMDB, and "
+                            "MemoryStore may be ahead of it right now");
+        return grpc::Status::OK;
+    }
 
     try {
         const auto rc = smartbotic::database::resolveCollection(request->collection());

service/src/database_service.cpp

@@ -1328,9 +1328,46 @@ void DatabaseService::applyReplicatedEntry(const databasepb::ReplicationEntry& e
                                 entry.collection());
                     if (auto* ds = projects_->getOrCreate(pc.project)) {
                         std::optional<Document> opt_doc(doc);
-                        smartbotic::db::storage::applyDualWriteMirror(
-                            ds, mirror_healthy_, mirror_drift_count_,
-                            pc.collection, doc.id, opt_doc, EventType::INSERT);
+                        try {
+                            smartbotic::db::storage::applyDualWriteMirror(
+                                ds, mirror_healthy_, mirror_drift_count_,
+                                pc.collection, doc.id, opt_doc, EventType::INSERT);
+                        } catch (const smartbotic::db::storage::UniqueViolation& e) {
+                            // v2.11.0 T11 finding 1 — deliberate choice, not an
+                            // accident of the generic catch below. By this
+                            // point store_->loadDocument() above has ALREADY
+                            // put the row into MemoryStore (loadDocument
+                            // bypasses the persist callback specifically to
+                            // avoid re-replicating), so unlike every
+                            // client-facing write path there is no "reject the
+                            // whole operation" available: the origin node
+                            // already committed this write and every other
+                            // follower is expected to converge to it too.
+                            // Undoing loadDocument here would silently diverge
+                            // this follower's data from the rest of the
+                            // cluster over a constraint that may not even
+                            // exist on the origin (this follower could have
+                            // declared the unique field locally, after the
+                            // fact — replication carries no guarantee the
+                            // origin shares this node's config). So
+                            // MemoryStore keeps the row — it is the true
+                            // record of what replication decided — and the
+                            // mirror is marked unhealthy ON PURPOSE: this is
+                            // exactly the pre-T11 swallow-and-flip outcome,
+                            // chosen deliberately for this one caller so an
+                            // operator sees the drift (and LMDB-first reads
+                            // fall back to MemoryStore, which has the answer)
+                            // rather than nothing signalling a permanent gap
+                            // between the two stores.
+                            spdlog::error(
+                                "v2.11 replication: unique constraint violated applying "
+                                "{}/{}: {} - MemoryStore holds the row, LMDB does not; "
+                                "marking the mirror unhealthy so reads fall back instead "
+                                "of silently disagreeing with MemoryStore",
+                                entry.collection(), doc.id, e.what());
+                            mirror_healthy_.store(false, std::memory_order_release);
+                            mirror_drift_count_.fetch_add(1, std::memory_order_relaxed);
+                        }
                     }
                 }
 

service/src/memory_store.cpp

@@ -1443,6 +1443,14 @@ bool MemoryStore::setRemove(const std::string& collection, const std::string& se
         return false;
     }
 
+    // v2.11.0 T11 — snapshot for undo. setRemove only shrinks the "members"
+    // array itself, which cannot add an index posting - but it also bumps
+    // version/updatedAt, and those ARE index-declarable metadata fields. A
+    // unique constraint on _version or _updated_at makes to_add non-empty
+    // here too, so this path can raise UniqueViolation same as any other
+    // UPDATE mirror call, and needs the same undo as setAdd's update branch.
+    const Document original = it->second;
+
     saveToHistory(*coll, it->second);
     tree["members"] = newMembers;
     it->second.set_data(tree);
@@ -1454,7 +1462,9 @@ bool MemoryStore::setRemove(const std::string& collection, const std::string& se
     Document updated = it->second;
 
     // v2.0 dual-write under lock (srem-style: trim members, update doc)
-    mirrorWriteToDocStore(collection, setId, updated, EventType::UPDATE);
+    mirrorDocOrUndo(collection, setId, updated, EventType::UPDATE, [&]() {
+        it->second = original;
+    });
 
     lock.unlock();
 

service/src/storage/document_store_lmdb.hpp

@@ -89,24 +89,33 @@ public:
                             std::vector<std::string> fields);
     std::vector<std::string> indexed_fields(std::string_view collection);
 
-    // ⚠ v2.10.0 — UNREACHABLE BY DESIGN, pending write-path work. Nothing calls
-    // set_unique_fields, so the enforcement below never runs, and no RPC or config
-    // key exposes it. Kept because the mechanism is correct in isolation and is
-    // the base for finishing the feature.
+    // v2.11.0 T11 — ACTIVE. Called from three places: DatabaseGrpcImpl::CreateIndex
+    // and ::DropIndex (per-call, gated on a duplicate-value refusal — see
+    // find_duplicate_values below), and DatabaseService::applyIndexDeclarations
+    // at boot (re-arming from CollectionCfg::uniqueFields, persisted in
+    // `_collection_meta`). Exposed on the wire via CreateIndexRequest.unique and
+    // reported by ListIndexes.
     //
-    // Why it cannot be turned on yet: the check throws from put(), which runs
-    // inside applyDualWriteMirror - and that catches every exception, logs it,
-    // bumps mirror drift and flips mirror_healthy_. So a rejection would be
-    // swallowed (the row still lands in MemoryStore, unenforced) AND would send
-    // every read in the process to MemoryStore, which is the v2.8.1 fault. Worse,
-    // MemoryStore mutates BEFORE the mirror runs, so a clean rejection needs the
-    // in-memory write rolled back.
+    // Why this took its own task to turn on: the check throws UniqueViolation
+    // from put(), which runs inside applyDualWriteMirror - and that used to catch
+    // every exception, log it, bump mirror drift and flip mirror_healthy_. A
+    // rejection would have been swallowed (the row still lands in MemoryStore,
+    // unenforced) AND would have sent every read in the process to MemoryStore,
+    // the v2.8.1 fault. Worse, MemoryStore mutates BEFORE the mirror runs, so a
+    // clean rejection needed the in-memory write rolled back.
     //
-    // Finishing it means either propagating UniqueViolation through the mirror
-    // without touching health, plus rollback, or moving enforcement ahead of the
-    // MemoryStore mutation under the same collection lock. Both touch the write
-    // path that produced the v2.4.3, v2.4.4 and v2.8.0 incidents, so it wants its
-    // own change. See docs/ROADMAP.md.
+    // The fix (T11): applyDualWriteMirror catches UniqueViolation separately and
+    // rethrows it untouched - health/drift stay exactly as they were - and every
+    // MemoryStore write path that can raise it (insert/update/upsert/
+    // updateIfVersion/patchDocument/setAdd/restoreToVersion) undoes its own
+    // mutation via mirrorDocOrUndo() before letting it propagate. The gRPC
+    // handlers map it to ALREADY_EXISTS. See dual_write_mirror.hpp and
+    // memory_store.cpp's mirrorDocOrUndo for the mechanics, and
+    // database_service.cpp's replicated-entry apply path for the one caller of
+    // applyDualWriteMirror that deliberately keeps the old swallow-and-flip
+    // behaviour (loadDocument there already mutated MemoryStore before the
+    // mirror runs, and there is no "reject the whole operation" available on a
+    // replication-apply path).
     void set_unique_fields(std::string_view collection,
                           std::vector<std::string> fields);
     std::vector<std::string> unique_fields(std::string_view collection);