Procházet zdrojové kódy

fix(audit): never open a second MDB_env in-process; count via LMDB everywhere

Two fixes, one shipped-broken, one incomplete.

1. auditSubdbPlacement() called the path-taking audit(), which opens its
   own MDB_env on a path the server already has open. LMDB coordinates
   readers with POSIX record locks, and POSIX locks are per-process: closing
   ANY descriptor on a file drops every lock the process holds on it. So the
   audit's mdb_env_close() tore down the locks belonging to the service's own
   env, and every subsequent mdb_txn_begin returned EINVAL for the life of
   the process. This took all LMDB reads down for ~90s on the first 2.4.4
   boot in production; rolled back to 2.4.3 to restore service.

   audit_env(MDB_env*) borrows an already-open env and opens only a read txn.
   The server uses it; the path-taking overload is now documented as CLI-only
   and both share audit_in_txn().

2. GetCollectionInfo still reported MemoryStore's documentCount. That is the
   surface `smartbotic-db-cli count` actually uses - it calls
   GetCollectionInfo, not the Count RPC - which is how the divergence stayed
   hidden after the Count fix. Now LMDB-first behind the same gate. sizeBytes
   deliberately stays a MemoryStore estimate: it measures resident footprint,
   a different quantity, and is what the eviction knobs act on.

Verified against a copy of the production dataset on a staging port: repeated
LMDB reads clean, zero EINVAL, and Count RPC / GetCollectionInfo / find all
agree at 70 where they previously read 47 / 47 / 69.
fszontagh před 1 měsícem
rodič
revize
635a617806

+ 24 - 0
service/src/database_grpc_impl.cpp

@@ -1198,6 +1198,30 @@ grpc::Status DatabaseGrpcImpl::GetCollectionInfo(
         return grpc::Status::OK;
     }
 
+    // v2.4.4 — documentCount from LMDB, same gate as Count/Get/Find. This is
+    // the third surface that reported a MemoryStore-only count while reads
+    // were served from LMDB (`smartbotic-db-cli count` goes through here, not
+    // through the Count RPC, which is how the divergence stayed hidden).
+    //
+    // Only documentCount is corrected. sizeBytes stays a MemoryStore estimate
+    // — it measures resident footprint, which is a genuinely different
+    // quantity from on-disk size and is what the eviction knobs act on.
+    const std::string& coll = request->name();
+    if (!coll.empty() && coll[0] != '_'
+        && service_.mirrorHealthy() && service_.mirrorDriftCount() == 0) {
+        try {
+            const auto rc = smartbotic::database::resolveCollection(coll);
+            if (auto* ds = service_.docStore(rc.project)) {
+                info->documentCount = ds->count(rc.collection);
+            }
+        } catch (const std::exception& e) {
+            // Advisory correction only — fall back to the MemoryStore figure
+            // rather than failing an informational RPC.
+            spdlog::warn("v2.4.4 GetCollectionInfo LMDB count failed coll={}: {}",
+                         coll, e.what());
+        }
+    }
+
     *response->mutable_info() = toProtoCollInfo(*info);
     response->set_found(true);
 

+ 7 - 3
service/src/database_service.cpp

@@ -314,9 +314,13 @@ void DatabaseService::auditSubdbPlacement() {
         auto h = projects_->getHandle(name);
         if (!h.env) continue;
         try {
-            // audit() opens the env read-only on its own handle, so it does
-            // not contend with the service's write path.
-            const auto rep = smartbotic::db::storage::audit(h.env->path(), name);
+            // audit_env(), NOT audit(). The path-taking overload opens a
+            // second MDB_env on the same file; closing it would drop the
+            // POSIX record locks this process already holds for `h.env`
+            // (POSIX locks are per-process, and closing any fd on a file
+            // releases all of them), leaving every later mdb_txn_begin
+            // failing EINVAL. Borrow the open handle instead.
+            const auto rep = smartbotic::db::storage::audit_env(h.env->raw(), name);
             if (rep.misplaced.empty()) {
                 spdlog::debug("placement audit: project '{}' consistent ({} rows)",
                               name, rep.rows_scanned);

+ 41 - 7
service/src/storage/subdb_placement.cpp

@@ -134,13 +134,13 @@ struct EnvHandle {
 
 }  // namespace
 
-AuditReport audit(const std::string& env_path, const std::string& project_in) {
-    AuditReport rep;
-    rep.project = project_in.empty() ? infer_project(env_path) : project_in;
+namespace {
 
-    EnvHandle eh(env_path, /*readonly=*/true);
-    MDB_txn* txn = nullptr;
-    ck(mdb_txn_begin(eh.env, nullptr, MDB_RDONLY, &txn), "txn_begin (audit)");
+// Shared body. Runs inside a read txn the caller owns; touches no descriptor
+// and no env lifetime, so it is safe both in-process and standalone.
+AuditReport audit_in_txn(MDB_txn* txn, const std::string& project) {
+    AuditReport rep;
+    rep.project = project;
 
     const auto all = list_subdbs(txn);
 
@@ -206,10 +206,44 @@ AuditReport audit(const std::string& env_path, const std::string& project_in) {
         m.home_occupied = (it != ids_by_subdb.end()) && it->second.count(m.id) > 0;
     }
 
-    mdb_txn_abort(txn);
     return rep;
 }
 
+}  // namespace
+
+AuditReport audit_env(MDB_env* env, const std::string& project) {
+    if (!env) return {};
+    MDB_txn* txn = nullptr;
+    ck(mdb_txn_begin(env, nullptr, MDB_RDONLY, &txn), "txn_begin (audit_env)");
+    try {
+        AuditReport rep = audit_in_txn(txn, project);
+        mdb_txn_abort(txn);
+        return rep;
+    } catch (...) {
+        mdb_txn_abort(txn);
+        throw;
+    }
+}
+
+AuditReport audit(const std::string& env_path, const std::string& project_in) {
+    const std::string project =
+        project_in.empty() ? infer_project(env_path) : project_in;
+
+    // Opens its own env — CLI only. See the header for why the server must
+    // never take this path.
+    EnvHandle eh(env_path, /*readonly=*/true);
+    MDB_txn* txn = nullptr;
+    ck(mdb_txn_begin(eh.env, nullptr, MDB_RDONLY, &txn), "txn_begin (audit)");
+    try {
+        AuditReport rep = audit_in_txn(txn, project);
+        mdb_txn_abort(txn);
+        return rep;
+    } catch (...) {
+        mdb_txn_abort(txn);
+        throw;
+    }
+}
+
 RepairResult repair(const std::string& env_path,
                     const AuditReport& report,
                     bool stamp_identity) {

+ 23 - 3
service/src/storage/subdb_placement.hpp

@@ -29,6 +29,9 @@
 #include <string>
 #include <vector>
 
+// Forward declaration — keep <lmdb.h> out of this header.
+struct MDB_env;
+
 namespace smartbotic::db::storage {
 
 // One row sitting in a sub-db other than the one it declares.
@@ -49,11 +52,28 @@ struct AuditReport {
     std::vector<std::string> unstamped;     // sub-dbs lacking an identity sentinel
 };
 
-// Inspect `env_path` read-only. `project` de-qualifies declared names of the
-// form "<project>:<collection>"; pass empty to infer it from the path
-// (.../projects/<name>/env).
+// Inspect `env_path` read-only by OPENING ITS OWN MDB_env.
+//
+// ONLY safe from a process that has no other handle on this env — i.e. the
+// CLI. Never call this from the server.
+//
+// Why: LMDB coordinates readers with POSIX record locks (fcntl), and POSIX
+// locks are per-process, not per-descriptor. Closing ANY descriptor on a file
+// drops EVERY lock the process holds on it. So a second MDB_env opened and
+// closed inside the server tears down the locks belonging to the server's own
+// env, and every subsequent mdb_txn_begin fails EINVAL for the life of the
+// process. This is not theoretical: it took LMDB reads down in production for
+// ~90s on the first 2.4.4 boot. Use audit_env() in-process instead.
+//
+// `project` de-qualifies declared names of the form "<project>:<collection>";
+// pass empty to infer it from the path (.../projects/<name>/env).
 AuditReport audit(const std::string& env_path, const std::string& project);
 
+// Same inspection against an ALREADY-OPEN env. This is the in-process form and
+// the one the server must use — it borrows the caller's MDB_env and opens only
+// a read txn, so it never touches the descriptor or the process's locks.
+AuditReport audit_env(MDB_env* env, const std::string& project);
+
 struct RepairResult {
     uint64_t moved = 0;        // relocated to their declared home
     uint64_t quarantined = 0;  // home occupied; parked in _orphans_<subdb>