Răsfoiți Sursa

fix(storage): del(WriteTxn&, ...) also recorded its handle too late

The round-2 audit claimed all six to_cache.emplace_back(...) sites
recorded immediately after open_for_write. That was wrong: del's own
collection handle was recorded at the end of the function, after
mdb_get and the maintainIndexes/maintainRelations calls - all of which
can throw, including UniqueViolation. Same shape as Finding 2, one
function over, missed because the audit was written from memory of
round 1's fix rather than by re-reading del's actual line order.

Harmless today (the only caller's WriteTxn aborts unwritten on any
throw regardless), but live the moment a cascade shares one to_cache
across calls, catches a per-op exception, and commits the rest of the
transaction - the design Task 12 builds on.

Moved the emplace_back to immediately after open_for_write, matching
the other five sites. No new test: the oversized-key technique used for
put/put_vector doesn't reach del's throwable statements (mdb_get has no
key-size limit; UniqueViolation needs a constructed conflict), and
rigging that cleanly is more machinery than this mechanical reorder
warrants - noted in the report rather than skipped silently.
fszontagh 1 lună în urmă
părinte
comite
71b7a053de
1 a modificat fișierele cu 12 adăugiri și 3 ștergeri
  1. 12 3
      service/src/storage/document_store_lmdb.cpp

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

@@ -1493,6 +1493,16 @@ bool LmdbDocumentStore::del(WriteTxn& wtxn,
     if (!cachedDbi(collection) && !known_this_txn) return false;
 
     unsigned int dbi = open_for_write(wtxn, collection);
+    // Recorded immediately, not after mdb_del - see put(WriteTxn&, ...)'s
+    // comment. mdb_get (below) and maintainIndexes/maintainRelations can all
+    // throw (including UniqueViolation), and a throw between here and the
+    // old late emplace_back left the collection's own handle out of
+    // to_cache while any index/relation handles opened along the way were
+    // in it. Harmless for the no-txn wrapper above (its WriteTxn just aborts
+    // unwritten either way) but exactly Finding 2's shape one function over,
+    // and it becomes live the moment a cascade shares to_cache across calls,
+    // catches per-op exceptions, and commits the rest of the transaction.
+    to_cache.emplace_back(std::string(collection), dbi);
     MDB_val k = to_val(id);
 
     // Remove index entries before the row goes, while its stored bytes are
@@ -1515,9 +1525,8 @@ bool LmdbDocumentStore::del(WriteTxn& wtxn,
     }
 
     int rc = mdb_del(wtxn.raw(), dbi, &k, nullptr);
-    // NOT cached here - the caller caches every entry in to_cache (this one
-    // included) only after ITS commit succeeds.
-    to_cache.emplace_back(std::string(collection), dbi);
+    // NOT cached here - the caller caches every entry in to_cache (recorded
+    // above) only after ITS commit succeeds.
     if (rc == MDB_NOTFOUND) return false;
     if (rc != MDB_SUCCESS) throw_mdb(rc, "del");
     return true;