fix(lmdb): cache the dbi only after commit - aborted writes poisoned the collection

The v2.4.3 EINVAL bug in a third failure mode, found live in production rather
than by a test.

open_for_write() cached the MDB_dbi immediately after mdb_dbi_open, before the
caller committed. LMDB keeps a handle private to the opening transaction until
it COMMITS and closes it if that transaction aborts. So any write that threw
after the handle was cached - a failed mdb_put, a sentinel mismatch, a WriteTxn
destructing uncommitted - left a closed handle in the cache, and every later
operation on that collection failed EINVAL for the life of the process.

Observed on 2.7.1 in production: find on smartbotic-automation:workflows failed
with "LMDB cursor_open: Invalid argument" on every attempt while every other
collection was fine. Six errors in twelve minutes, workflow listing broken for
users, nothing surfacing it but the service log. A restart cleared it, because
handles live in the env's shared table.

This is also the likely cause of the only production SIGSEGV on record: a live
PatchDocument -> mirrorWriteToDocStore -> mdb_put writing through a handle in
that state. Which means that crash is NOT a 2.8.0 regression - the defect was
live on 2.7.1, and an upgrade's queued-write burst is simply what trips it.

Fix: open_for_write no longer caches. Callers call cacheCommittedDbi() after
wtxn.commit() succeeds. Once committed, LMDB promotes the handle into the env's
shared table where it stays valid for the life of the env - which is what makes
caching safe at that point and only at that point.

Test: test_aborted_write_does_not_poison_the_collection induces the abort
honestly with a key past LMDB's 511-byte limit, then asserts a later write,
scan, count and get on the same collection all still work. Before the fix three
of those failed, including cursor_open - the exact production symptom.

v2.4.3 fixed read-path caching; v2.4.4 added the identity sentinel. Neither
addressed caching before commit.

ctest 18/18, namespacing 31/31, policy enforcement 19/19, views and TLS/auth
green.

service/src/storage/document_store_lmdb.cpp

@@ -227,13 +227,34 @@ unsigned int LmdbDocumentStore::open_for_write(WriteTxn& wtxn,
     // pre-date v2.4.4 acquire a sentinel on their next write without a
     // migration pass. Idempotent.
     write_subdb_identity(wtxn, raw_dbi, collection);
-    {
-        std::lock_guard<std::mutex> lock(cache_mutex_);
-        dbi_cache_[key] = raw_dbi;
-    }
+
+    // v2.8.0 — deliberately NOT cached here.
+    //
+    // LMDB keeps a handle private to the opening transaction until it COMMITS,
+    // and CLOSES it if that transaction aborts. Caching at this point meant any
+    // write that threw afterwards (a failed mdb_put, a sentinel mismatch, a
+    // WriteTxn destructing uncommitted) left a CLOSED handle in the cache, and
+    // every later operation on that collection failed EINVAL for the rest of the
+    // process's life.
+    //
+    // That is the v2.4.3 bug in a third failure mode, and it was live in
+    // production on 2.7.1: `find` on smartbotic-automation:workflows failed with
+    // "LMDB cursor_open: Invalid argument" on every attempt while every other
+    // collection was fine, and only a restart cleared it.
+    //
+    // Callers cache via cacheCommittedDbi() AFTER their commit succeeds. Once
+    // committed, LMDB promotes the handle into the env's shared table and it
+    // stays valid for the life of the env - which is what makes caching safe at
+    // that point and only at that point.
     return raw_dbi;
 }
 
+void LmdbDocumentStore::cacheCommittedDbi(std::string_view collection,
+                                           unsigned int dbi) {
+    std::lock_guard<std::mutex> lock(cache_mutex_);
+    dbi_cache_[std::string(collection)] = dbi;
+}
+
 std::optional<unsigned int>
 LmdbDocumentStore::try_open_for_read(ReadTxn& rtxn,
                                       std::string_view collection) {
@@ -276,6 +297,7 @@ void LmdbDocumentStore::put(std::string_view collection,
     MDB_val v = to_val(payload);
     mdb_check(mdb_put(wtxn.raw(), dbi, &k, &v, 0), "put");
     wtxn.commit();
+    cacheCommittedDbi(collection, dbi);
 }
 
 std::optional<smartbotic::database::Document>
@@ -309,10 +331,12 @@ bool LmdbDocumentStore::del(std::string_view collection, std::string_view id) {
     int rc = mdb_del(wtxn.raw(), dbi, &k, nullptr);
     if (rc == MDB_NOTFOUND) {
         wtxn.commit();
+        cacheCommittedDbi(collection, dbi);
         return false;
     }
     if (rc != MDB_SUCCESS) throw_mdb(rc, "del");
     wtxn.commit();
+    cacheCommittedDbi(collection, dbi);
     return true;
 }
 
@@ -535,6 +559,7 @@ void LmdbDocumentStore::put_vector(std::string_view collection,
               const_cast<void*>(static_cast<const void*>(vec.data()))};
     mdb_check(mdb_put(wtxn.raw(), dbi, &k, &v, 0), "put_vector");
     wtxn.commit();
+    cacheCommittedDbi(subdb, dbi);
 }
 
 bool LmdbDocumentStore::del_vector(std::string_view collection, std::string_view id) {
@@ -553,6 +578,7 @@ bool LmdbDocumentStore::del_vector(std::string_view collection, std::string_view
     if (rc == MDB_NOTFOUND) return false;
     if (rc != MDB_SUCCESS) throw_mdb(rc, "del_vector");
     wtxn.commit();
+    cacheCommittedDbi(subdb, dbi);
     return true;
 }
 

service/src/storage/document_store_lmdb.hpp

@@ -85,6 +85,13 @@ private:
     // it already exists. Returns nullopt if the sub-db doesn't exist —
     // because read txns can't MDB_CREATE.
     unsigned int open_for_write(class WriteTxn& wtxn, std::string_view collection);
+
+    // v2.8.0 — record a handle in the cache, to be called ONLY after the
+    // transaction that opened it has committed. LMDB closes a handle whose
+    // opening transaction aborts, so caching any earlier leaves a closed handle
+    // behind and every later operation on that collection fails EINVAL for the
+    // life of the process.
+    void cacheCommittedDbi(std::string_view collection, unsigned int dbi);
     std::optional<unsigned int> try_open_for_read(class ReadTxn& rtxn,
                                                    std::string_view collection);
 };

tests/test_subdb_identity.cpp

@@ -353,6 +353,70 @@ void test_scan_fast_path_matches_general_path() {
     check(z.total_matched == 25, "limit=0 still reports the true total");
 }
 
+// v2.8.0 — a WRITE that aborts must not poison the collection.
+//
+// This is the v2.4.3 EINVAL bug in a third failure mode, observed live in
+// production on 2.7.1: `find` on smartbotic-automation:workflows failed with
+// "LMDB cursor_open: Invalid argument" on every attempt while every other
+// collection was fine, and a restart was the only cure.
+//
+// Cause: open_for_write() cached the MDB_dbi immediately after mdb_dbi_open,
+// BEFORE the caller committed. LMDB keeps a handle private to the opening
+// transaction until it commits and CLOSES it if that transaction aborts - so any
+// write that threw after the handle was cached (a failed mdb_put, a sentinel
+// mismatch, a WriteTxn destructing uncommitted) left a closed handle in the
+// cache, and every later operation on that collection failed EINVAL for the rest
+// of the process's life.
+//
+// The abort is induced honestly here, with a key past LMDB's 511-byte limit, so
+// the test exercises the same path a real failed write takes.
+void test_aborted_write_does_not_poison_the_collection() {
+    TmpEnv t("abortpoison");
+    LmdbDocumentStore store(t.env);
+
+    // Force a write that opens the sub-db and then fails: an oversized key makes
+    // mdb_put return MDB_BAD_VALSIZE, which throws, so the WriteTxn aborts.
+    const std::string huge_id(600, 'k');
+    bool threw = false;
+    try {
+        store.put("poisoned", huge_id, make_doc(huge_id, "poisoned"));
+    } catch (const std::exception&) {
+        threw = true;
+    }
+    check(threw, "an oversized key really does fail the write");
+
+    // The collection must still be usable. Before the fix, every one of these
+    // failed with EINVAL because the cache held a handle LMDB had closed.
+    bool ok_put = true;
+    try {
+        store.put("poisoned", "good", make_doc("good", "poisoned"));
+    } catch (const std::exception&) {
+        ok_put = false;
+    }
+    check(ok_put, "a later WRITE to the same collection still works");
+
+    bool ok_read = true;
+    try {
+        smartbotic::database::Query q;
+        q.limit = 10;
+        auto res = store.scan("poisoned", q);
+        check(res.documents.size() == 1, "and the document written after the abort is there");
+    } catch (const std::exception&) {
+        ok_read = false;
+    }
+    check(ok_read, "a later SCAN of the same collection still works (cursor_open)");
+
+    bool ok_count = true;
+    try {
+        check(store.count("poisoned") == 1, "count is right after the abort");
+    } catch (const std::exception&) {
+        ok_count = false;
+    }
+    check(ok_count, "and count() does not throw");
+
+    check(store.get("poisoned", "good").has_value(), "get() works after the abort");
+}
+
 }  // namespace
 
 int main() {
@@ -365,6 +429,7 @@ int main() {
     test_vector_subdb_sentinel();
     test_scan_limit_zero_reports_total();
     test_scan_fast_path_matches_general_path();
+    test_aborted_write_does_not_poison_the_collection();
 
     std::cout << "passed: " << g_pass << ", failed: " << g_fail << "\n";
     return g_fail == 0 ? 0 : 1;