Sfoglia il codice sorgente

fix(storage): apply round-1 findings to the vector overloads too

put_vector(WriteTxn&, ...) and del_vector(WriteTxn&, ...) still had the
same two defects fixed on the document path in the previous commit:

- put_vector recorded its handle in to_cache only after mdb_put
  succeeded, so a throw from mdb_put on a brand-new vector sub-db (e.g.
  an oversized key) left that handle out of to_cache. Moved the
  emplace_back to immediately after open_for_write, matching put/del.

- del_vector's existence probe used cachedDbi() alone, which only sees
  already-committed sub-dbs - so a vector sub-db opened by an earlier
  put_vector(wtxn, ...) call sharing the same to_cache, within the same
  still-uncommitted transaction, would read as nonexistent and the
  delete would silently no-op. Added the same to_cache fallback del()
  got, compared against the vector sub-db's own name so a collection or
  index handle in the same to_cache can't be mistaken for it.

Both were reachable under the Task 12 cascade design (one shared
to_cache across put/del/put_vector/del_vector for a parent and its
children in one transaction), dormant only because nothing but the
no-txn wrappers calls these overloads yet.

Audited all six to_cache.emplace_back(...) call sites in the file; the
two inside maintainIndexes/maintainRelations were already correct
(immediate, pre-existing from the original to_cache pattern). No further
late-recording sites found.

Test: test_subdb_identity gains
test_txn_accepting_vector_writes_are_atomic, mirroring the document
test - an aborted put_vector leaves no vector behind (checked against a
genuinely warmed cache, not a cold-cache false negative), an oversized
key proves the handle is recorded before the failing mdb_put, and a
same-transaction put_vector + del_vector pair actually deletes rather
than no-op'ing.
fszontagh 1 mese fa
parent
commit
1c8ba09915
2 ha cambiato i file con 126 aggiunte e 3 eliminazioni
  1. 21 3
      service/src/storage/document_store_lmdb.cpp
  2. 105 0
      tests/test_subdb_identity.cpp

+ 21 - 3
service/src/storage/document_store_lmdb.cpp

@@ -2448,11 +2448,14 @@ void LmdbDocumentStore::put_vector(WriteTxn& wtxn,
     if (vec.empty()) return;  // mirror the migration tool's no-op semantics
     const std::string subdb = vector_subdb_name(collection);
     unsigned int dbi = open_for_write(wtxn, subdb);
+    // Recorded immediately, not after mdb_put succeeds - see put(WriteTxn&,
+    // ...)'s comment. A throw from mdb_put on a brand-new vector sub-db must
+    // not leave its handle out of to_cache.
+    to_cache.emplace_back(subdb, dbi);
     MDB_val k = to_val(id);
     MDB_val v{vec.size() * sizeof(float),
               const_cast<void*>(static_cast<const void*>(vec.data()))};
     mdb_check(mdb_put(wtxn.raw(), dbi, &k, &v, 0), "put_vector");
-    to_cache.emplace_back(subdb, dbi);
 }
 
 bool LmdbDocumentStore::del_vector(std::string_view collection, std::string_view id) {
@@ -2473,11 +2476,26 @@ bool LmdbDocumentStore::del_vector(WriteTxn& wtxn,
                                     std::string_view id,
                                     std::vector<std::pair<std::string, unsigned int>>& to_cache) {
     const std::string subdb = vector_subdb_name(collection);
-    if (!cachedDbi(subdb)) return false;
+    // Same same-transaction gap as del(WriteTxn&, ...): cachedDbi() only
+    // sees already-committed sub-dbs, so a vector sub-db opened by an
+    // earlier put_vector(wtxn, ...) call sharing this to_cache would not be
+    // visible there yet. Check to_cache too - by construction it already
+    // holds any handle opened earlier in this transaction (see the
+    // emplace_back in put_vector(WriteTxn&, ...) above). Compared against
+    // the VECTOR sub-db's own name, not the collection's, so a collection or
+    // index handle recorded earlier in the same to_cache cannot be mistaken
+    // for it.
+    const bool known_this_txn = std::any_of(
+        to_cache.begin(), to_cache.end(),
+        [&](const auto& p) { return p.first == subdb; });
+    if (!cachedDbi(subdb) && !known_this_txn) return false;
     unsigned int dbi = open_for_write(wtxn, subdb);
+    // Recorded immediately, matching put_vector(WriteTxn&, ...) and
+    // del(WriteTxn&, ...) - a handle from a transaction that later aborts is
+    // harmless, since the caller only applies to_cache after its own commit.
+    to_cache.emplace_back(subdb, dbi);
     MDB_val k = to_val(id);
     int rc = mdb_del(wtxn.raw(), dbi, &k, nullptr);
-    to_cache.emplace_back(subdb, dbi);
     if (rc == MDB_NOTFOUND) return false;
     if (rc != MDB_SUCCESS) throw_mdb(rc, "del_vector");
     return true;

+ 105 - 0
tests/test_subdb_identity.cpp

@@ -1877,6 +1877,110 @@ void test_txn_accepting_writes_are_atomic_across_collections() {
           "confirming the earlier miss was purely a cold cache");
 }
 
+// v2.11.0 T10 review round 2 — the vector equivalents of the two defects
+// fixed on the document path (put/del) were still present on put_vector/
+// del_vector: late handle-recording (Finding 2) and a same-transaction
+// existence-probe gap (Finding 3). Both are reachable under the Task 12
+// cascade design documented in the task report, which shares ONE to_cache
+// across put/del/put_vector/del_vector for a parent and its children in a
+// single transaction — dormant only because nothing but the self-contained
+// no-txn wrappers calls the txn-accepting vector overloads yet.
+void test_txn_accepting_vector_writes_are_atomic() {
+    TmpEnv t("txn-atomic-vec");
+    LmdbDocumentStore store(t.env);
+
+    // --- Abort path: an aborted put_vector must leave no vector behind.
+    // Proven the same non-vacuous way as the document test: get_vector()
+    // on a never-committed sub-db returns nullopt purely from a cold cache
+    // (try_open_for_read is cache-only), so the follow-up write under the
+    // SAME collection is what makes the absence-of-'a1' check real - if the
+    // aborted key had survived, get_vector("vecsA", "a1") would find it via
+    // the now-warm cache, not report absent. ---
+    {
+        WriteTxn wtxn(t.env);
+        std::vector<std::pair<std::string, unsigned int>> to_cache;
+        store.put_vector(wtxn, "vecsA", "a1", {1.0f, 2.0f, 3.0f}, to_cache);
+        wtxn.abort();
+    }
+    bool ok_put = true;
+    try {
+        store.put_vector("vecsA", "a2", {9.0f, 9.0f, 9.0f});  // warms the cache for real
+    } catch (const std::exception&) {
+        ok_put = false;
+    }
+    check(ok_put, "a later put_vector on 'vecsA' after the abort still works - "
+                  "not poisoned by the aborted transaction");
+    check(!store.get_vector("vecsA", "a1").has_value(),
+          "aborted txn: 'a1' does not exist, checked against a genuinely "
+          "warm cache (via 'a2'), not a cold-cache false negative");
+    auto v2 = store.get_vector("vecsA", "a2");
+    check(v2.has_value() && v2->size() == 3 && (*v2)[0] == 9.0f,
+          "the post-abort vector write is readable");
+
+    // --- Finding 2 (put_vector): the handle must be recorded in to_cache
+    // BEFORE mdb_put runs, not after - so a throw from mdb_put does not
+    // silently drop the collection's own handle from what the caller was
+    // told to cache. Reproduced with the same oversized-key technique as
+    // test_aborted_write_does_not_poison_the_collection: a key past LMDB's
+    // 511-byte limit makes mdb_put fail MDB_BAD_VALSIZE. ---
+    {
+        WriteTxn wtxn(t.env);
+        std::vector<std::pair<std::string, unsigned int>> to_cache;
+        const std::string huge_id(600, 'v');
+        bool threw = false;
+        try {
+            store.put_vector(wtxn, "vecsB", huge_id, {1.0f}, to_cache);
+        } catch (const std::exception&) {
+            threw = true;
+        }
+        check(threw, "an oversized key really does fail put_vector's mdb_put");
+        check(!to_cache.empty(),
+              "put_vector(wtxn,...) recorded the sub-db handle BEFORE the "
+              "failing mdb_put ran, not after - so to_cache is not silently "
+              "missing it");
+        wtxn.abort();
+    }
+    bool ok_put_b = true;
+    try {
+        store.put_vector("vecsB", "b1", {4.0f, 5.0f});
+    } catch (const std::exception&) {
+        ok_put_b = false;
+    }
+    check(ok_put_b, "'vecsB' is not poisoned by the failed put_vector either");
+    check(store.get_vector("vecsB", "b1").has_value(),
+          "and the post-failure vector write is readable");
+
+    // --- Finding 3 (del_vector): a vector sub-db opened by put_vector(wtxn,
+    // ...) earlier in the SAME still-uncommitted transaction must be
+    // deletable by del_vector(wtxn, ...) sharing that to_cache - cachedDbi()
+    // alone only sees already-committed sub-dbs, so without the to_cache
+    // fallback this would silently no-op instead of deleting. ---
+    bool existed = false;
+    {
+        WriteTxn wtxn(t.env);
+        std::vector<std::pair<std::string, unsigned int>> to_cache;
+        store.put_vector(wtxn, "vecsC", "c1", {7.0f, 7.0f}, to_cache);
+        existed = store.del_vector(wtxn, "vecsC", "c1", to_cache);
+        wtxn.commit();
+        for (const auto& [sub, d] : to_cache) {
+            // Stand-in for the caller's post-commit caching step - not
+            // strictly needed for this assertion (get_vector below would
+            // read as absent for a deleted key from a cold cache too, same
+            // as a genuinely-deleted one), but exercised for parity with
+            // how a real caller (Task 12) must use this.
+            (void)sub;
+            (void)d;
+        }
+    }
+    check(existed,
+          "del_vector(wtxn, ...) found and deleted 'c1' within the SAME "
+          "transaction that created it, rather than treating the "
+          "not-yet-committed sub-db as nonexistent and no-op'ing");
+    store.prime_dbi_cache();
+    check(!store.get_vector("vecsC", "c1").has_value(),
+          "'c1' is genuinely gone after the commit, not merely unreported");
+}
+
 }  // namespace
 
 int main() {
@@ -1891,6 +1995,7 @@ int main() {
     test_scan_fast_path_matches_general_path();
     test_aborted_write_does_not_poison_the_collection();
     test_txn_accepting_writes_are_atomic_across_collections();
+    test_txn_accepting_vector_writes_are_atomic();
     test_filtered_scan_operator_matrix();
     test_concurrent_reads_do_not_rebind_cached_handles();
     test_existing_collection_readable_without_writing_first();