Forráskód Böngészése

docs(plan): pre-flight fixes before execution

Three corrections found scanning the plan against the tree it will run in.

1. Global Constraint against opening a second MDB_env on an already-open
path. Commit 8cd73fb landed after the plan was written and records that
doing so took every LMDB read down for ~90s in production - POSIX locks
are per-process, so closing any fd drops them all. RelationIndex already
borrows an LmdbEnv&, but an implementer could reasonably add a
path-taking constructor without knowing why not to.

2. Tasks 6 and 8 mandated tests that were not red-green cycles - one
asserted only declaration facts already covered elsewhere, the other was
annotated "Expected: PASS already". Both now genuinely fail before the
code exists: Task 6 drives RelationEngine against a real index, Task 8
spans a document sub-db and an index sub-db in one transaction, which
cannot compile without the txn overloads it is meant to prove.

3. Test scripts derive ROOT from their own location instead of hardcoding
/data/smartbotic-database, so a run from a worktree tests that worktree's
binaries rather than the main checkout's.
fszontagh 1 hónapja
szülő
commit
fa99931de5
1 módosított fájl, 224 hozzáadás és 28 törlés
  1. 224 28
      docs/superpowers/plans/2026-08-04-relations-v2.5.0.md

+ 224 - 28
docs/superpowers/plans/2026-08-04-relations-v2.5.0.md

@@ -18,6 +18,8 @@
 - **Every new sub-db carries the v2.4.4 identity sentinel.** `write_subdb_identity` on first write-open, `verify_subdb_identity` on cached-handle reuse, and `is_identity_key` skipped in every scan and count.
 - **Cross-project relations are rejected.** No LMDB transaction spans two envs.
 - **References name `_id` only.** No candidate keys, no `ON UPDATE`.
+- **NEVER open a second `MDB_env` on a path this process already has open.** LMDB coordinates readers with POSIX record locks, and POSIX locks are per-process, not per-descriptor: closing ANY fd on the file releases every lock the process holds on it. The first v2.4.4 build opened a second env for its placement audit and closed it, which destroyed the service's own lock state and returned `EINVAL` from every `mdb_txn_begin` for the life of the process - all LMDB reads were down for ~90 seconds in production. `RelationIndex` therefore takes an `LmdbEnv&` and borrows the project's existing env. Do NOT add a path-taking constructor, and do not open an env anywhere outside the existing registry.
+- **Test scripts derive their own root.** `tests/load_test/*.sh` must compute `ROOT="$(cd "$(dirname "$0")/../.." && pwd)"` rather than hardcoding `/data/smartbotic-database`, so a script run from a git worktree builds and tests that worktree's binaries instead of silently exercising the main checkout's.
 - **Build:** `cmake -B build -G Ninja && cmake --build build -j$(nproc)`
 - **Test:** `cd build/tests && ctest --output-on-failure`
 - **Tests use real check functions, never bare `assert()`** in new files. (`-UNDEBUG` is set on test targets as of v2.4.3, but write checks that fail loudly regardless.)
@@ -1675,30 +1677,195 @@ unrelated writes is one cache lookup."
 
 - [ ] **Step 1: Write the failing test**
 
-Extend `tests/test_relation_manager.cpp` with a restrict test. Add before `main`:
+Create `tests/test_relation_engine.cpp`. This drives the real engine against a
+real index, so it genuinely fails before the engine exists:
 
 ```cpp
-void test_restrict_blocks_when_children_exist() {
-    // Engine-level check without a server: a relation with children present
-    // must refuse, and the message must name the relation and the count so an
-    // operator learns what to do next from the error itself.
+// RelationEngine against a real LmdbEnv and a real reverse index.
+//
+// restrict must refuse while children exist, and the refusal must name the
+// relation, the count and a sample of ids - an operator should learn what to
+// do next from the message rather than from the source.
+#include "memory_store.hpp"
+#include "relations/relation_engine.hpp"
+#include "relations/relation_manager.hpp"
+#include "storage/lmdb_env.hpp"
+#include "storage/relation_index.hpp"
+
+#include <filesystem>
+#include <iostream>
+#include <string>
+#include <unistd.h>
+
+namespace fs = std::filesystem;
+
+using smartbotic::database::MemoryStore;
+using smartbotic::database::RelationEngine;
+using smartbotic::database::RelationInfo;
+using smartbotic::database::RelationManager;
+using smartbotic::db::storage::LmdbEnv;
+using smartbotic::db::storage::LmdbEnvOpts;
+using smartbotic::db::storage::RelationIndex;
+
+namespace {
+
+int failures = 0;
+
+void check(bool cond, const std::string& msg) {
+    std::cout << (cond ? "  PASS  " : "  FAIL  ") << msg << "\n";
+    if (!cond) ++failures;
+}
+
+std::string tmpdir(const char* tag) {
+    static int n = 0;
+    std::string p = "/tmp/sbdb-relengine-" + std::to_string(::getpid()) + "-" +
+                    std::to_string(n++) + "-" + tag;
+    std::error_code ec;
+    fs::remove_all(p, ec);
+    return p;
+}
+
+MemoryStore::Config testConfig() {
+    MemoryStore::Config cfg;
+    cfg.nodeId = "test";
+    cfg.maxMemoryBytes = 64ULL * 1024 * 1024;
+    return cfg;
+}
+
+// The engine resolves a project name to an env through a callback so it never
+// opens one itself - see the Global Constraint on second MDB_env instances.
+LmdbEnv* envResolver(void* ctx, const std::string&) {
+    return static_cast<LmdbEnv*>(ctx);
+}
+
+RelationInfo restrictRelation() {
+    RelationInfo r;
+    r.name = "default:exec_wf";
+    r.child = "default:executions";
+    r.childField = "workflow_id";
+    r.parent = "default:workflows";
+    r.onDelete = "restrict";
+    return r;
+}
+
+void test_restrict_allows_when_no_children() {
+    const std::string path = tmpdir("no-children");
+    LmdbEnv env(LmdbEnvOpts{path, 64ULL << 20, 256, 126, false});
     MemoryStore store(testConfig());
     RelationManager mgr(store);
     std::string err;
-    check(mgr.createRelation(makeRelation(), err), "relation created");
+    mgr.createRelation(restrictRelation(), err);
 
-    auto rel = mgr.getRelation("default:executions_workflow");
-    check(rel->onDelete == "restrict", "policy is restrict");
-    check(rel->parent == "default:workflows", "parent is qualified");
+    RelationEngine engine(mgr, &envResolver, &env);
+    std::string relErr;
+    check(engine.checkRestrict("default:workflows", "wf-1", relErr),
+          "restrict allows the delete when no children reference the parent");
+    check(relErr.empty(), "no error message when allowed");
+
+    std::error_code ec;
+    fs::remove_all(path, ec);
+}
+
+void test_restrict_blocks_and_explains() {
+    const std::string path = tmpdir("blocks");
+    LmdbEnv env(LmdbEnvOpts{path, 64ULL << 20, 256, 126, false});
+    MemoryStore store(testConfig());
+    RelationManager mgr(store);
+    std::string err;
+    mgr.createRelation(restrictRelation(), err);
+
+    RelationIndex idx(env, "exec_wf");
+    idx.add("wf-1", "ex-1");
+    idx.add("wf-1", "ex-2");
+
+    RelationEngine engine(mgr, &envResolver, &env);
+    std::string relErr;
+    check(!engine.checkRestrict("default:workflows", "wf-1", relErr),
+          "restrict blocks the delete while children exist");
+    check(relErr.find("default:exec_wf") != std::string::npos,
+          "the message names the relation");
+    check(relErr.find("2") != std::string::npos,
+          "the message reports the child count");
+    check(relErr.find("ex-1") != std::string::npos,
+          "the message samples a blocking child id");
+
+    std::error_code ec;
+    fs::remove_all(path, ec);
+}
+
+void test_describe_delete_reports_impact() {
+    const std::string path = tmpdir("describe");
+    LmdbEnv env(LmdbEnvOpts{path, 64ULL << 20, 256, 126, false});
+    MemoryStore store(testConfig());
+    RelationManager mgr(store);
+    std::string err;
+    mgr.createRelation(restrictRelation(), err);
+
+    RelationIndex idx(env, "exec_wf");
+    idx.add("wf-1", "ex-1");
+
+    RelationEngine engine(mgr, &envResolver, &env);
+    auto impacts = engine.describeDelete("default:workflows", "wf-1");
+    check(impacts.size() == 1, "one relation applies");
+    check(impacts[0].relation == "default:exec_wf", "impact names the relation");
+    check(impacts[0].policy == "restrict", "impact reports the policy");
+    check(impacts[0].childCount == 1, "impact counts the children");
+    check(impacts[0].blocks, "impact says it blocks");
+    check(impacts[0].sampleChildIds.size() == 1, "impact samples the child id");
+
+    std::error_code ec;
+    fs::remove_all(path, ec);
+}
+
+void test_no_action_never_blocks() {
+    const std::string path = tmpdir("no-action");
+    LmdbEnv env(LmdbEnvOpts{path, 64ULL << 20, 256, 126, false});
+    MemoryStore store(testConfig());
+    RelationManager mgr(store);
+    RelationInfo r = restrictRelation();
+    r.onDelete = "no_action";
+    std::string err;
+    mgr.createRelation(r, err);
+
+    RelationIndex idx(env, "exec_wf");
+    idx.add("wf-1", "ex-1");
+
+    RelationEngine engine(mgr, &envResolver, &env);
+    std::string relErr;
+    check(engine.checkRestrict("default:workflows", "wf-1", relErr),
+          "no_action never blocks even with children present");
+
+    std::error_code ec;
+    fs::remove_all(path, ec);
+}
+
+}  // namespace
+
+int main() {
+    test_restrict_allows_when_no_children();
+    test_restrict_blocks_and_explains();
+    test_describe_delete_reports_impact();
+    test_no_action_never_blocks();
+
+    std::cout << (failures ? "\nFAILED: " + std::to_string(failures) + " check(s)\n"
+                           : "\ntest_relation_engine: all passed\n");
+    return failures ? 1 : 0;
 }
 ```
 
-and call it from `main`. The full behavioral coverage lands in the e2e test (Task 10); this keeps the unit suite honest about the declaration surface.
+Add a `test_relation_engine` target to `tests/CMakeLists.txt` with the same
+source list as `test_relation_manager` plus
+`../service/src/relations/relation_engine.cpp`,
+`../service/src/relations/reference_extract.cpp`,
+`../service/src/storage/relation_index.cpp`, `../service/src/storage/lmdb_env.cpp`,
+`../service/src/storage/lmdb_txn.cpp`, `../service/src/storage/subdb_identity.cpp`,
+linking `${LMDB_LIBRARY}` and including `${LMDB_INCLUDE_DIR}`. Add it to the
+`-UNDEBUG` foreach and register `add_test(NAME relation_engine COMMAND test_relation_engine)`.
 
 - [ ] **Step 2: Run test to verify it fails**
 
-Run: `cmake --build build -j$(nproc) --target test_relation_manager && ./build/tests/test_relation_manager`
-Expected: PASS (this step is a guard, not a red test - the engine below is covered end-to-end in Task 10).
+Run: `cmake -B build -G Ninja && cmake --build build -j$(nproc) --target test_relation_engine`
+Expected: FAIL to compile - `relations/relation_engine.hpp` does not exist.
 
 - [ ] **Step 3: Write the engine**
 
@@ -2159,41 +2326,67 @@ This is the Phase B core. It touches the same write path that produced the v2.4.
 
 - [ ] **Step 1: Write the failing test**
 
-Add to `tests/test_relation_index.cpp`, before `main`:
+Add to `tests/test_document_store.cpp`, before `main`. This spans a DOCUMENT
+sub-db and an INDEX sub-db in one transaction, which is the property cascade
+depends on and which cannot compile until the txn-accepting store overloads
+exist:
 
 ```cpp
-void test_index_edges_move_atomically_with_documents() {
-    // One WriteTxn spanning two sub-dbs. Either both changes land or neither
-    // does - this is the property cascade depends on.
-    TmpEnv t("atomic");
-    RelationIndex idx(t.env, "r1");
+// v2.5.0 - one WriteTxn spanning a document sub-db and a relation index
+// sub-db. Either both changes land or neither does. Cascade is built on this:
+// a child document and the index edge describing it must never disagree.
+void test_document_and_index_move_in_one_transaction() {
+    TmpEnv t("txn-span");
+    LmdbDocumentStore store(t.env);
+    smartbotic::db::storage::RelationIndex idx(t.env, "r1");
+
+    store.put("executions", "ex-1", make_doc("ex-1", "executions",
+                                             nlohmann::json{{"workflow_id", "wf-1"}}));
     idx.add("wf-1", "ex-1");
-    idx.add("wf-1", "ex-2");
+    check(store.get("executions", "ex-1").has_value(), "seed document present");
+    check(idx.count_children("wf-1") == 1, "seed index edge present");
 
+    // Aborted: neither the document nor the edge may change.
     {
         smartbotic::db::storage::WriteTxn txn(t.env);
+        store.del(txn, "executions", "ex-1");
         idx.remove(txn, "wf-1", "ex-1");
-        idx.remove(txn, "wf-1", "ex-2");
-        // Dropped without commit.
+        // Dropped without commit - WriteTxn's destructor aborts.
     }
-    check(idx.count_children("wf-1") == 2, "aborted txn leaves the index untouched");
+    check(store.get("executions", "ex-1").has_value(),
+          "aborted txn leaves the document in place");
+    check(idx.count_children("wf-1") == 1,
+          "aborted txn leaves the index edge in place");
 
+    // Committed: both must land together.
     {
         smartbotic::db::storage::WriteTxn txn(t.env);
+        store.del(txn, "executions", "ex-1");
         idx.remove(txn, "wf-1", "ex-1");
-        idx.remove(txn, "wf-1", "ex-2");
         txn.commit();
     }
-    check(idx.count_children("wf-1") == 0, "committed txn applies every edge");
+    check(!store.get("executions", "ex-1").has_value(),
+          "committed txn removed the document");
+    check(idx.count_children("wf-1") == 0,
+          "committed txn removed the index edge");
 }
 ```
 
-Call it from `main`, and add `#include "storage/lmdb_txn.hpp"` at the top.
+Call it from `main`, and add these includes at the top of the file:
+
+```cpp
+#include "storage/lmdb_txn.hpp"
+#include "storage/relation_index.hpp"
+```
+
+Add `../service/src/storage/relation_index.cpp` and
+`../service/src/storage/subdb_identity.cpp` to `test_document_store`'s source
+list in `tests/CMakeLists.txt`.
 
 - [ ] **Step 2: Run test to verify it fails**
 
-Run: `cmake --build build -j$(nproc) --target test_relation_index && ./build/tests/test_relation_index`
-Expected: PASS already, because `RelationIndex` gained txn overloads in Task 2. If it fails, the txn overloads are wrong and must be fixed before proceeding.
+Run: `cmake -B build -G Ninja && cmake --build build -j$(nproc) --target test_document_store`
+Expected: FAIL to compile - `LmdbDocumentStore` has no `del(WriteTxn&, ...)` overload.
 
 - [ ] **Step 3: Add txn-accepting store overloads**
 
@@ -2742,7 +2935,10 @@ Create `tests/load_test/test_relations_e2e.sh`, modelled on `tests/load_test/tes
 set -euo pipefail
 
 cd "$(dirname "$0")"
-ROOT=/data/smartbotic-database
+# Derive the repo root from this script's own location, so running from a git
+# worktree builds and tests THAT worktree's binaries rather than silently
+# exercising the main checkout's.
+ROOT="$(cd "$(dirname "$0")/../.." && pwd)"
 DIR=/tmp/sbdb-relations-e2e
 PORT=9081