Kaynağa Gözat

fix(relations): T12 review round 2 - vacuous crash test, replication gap, grandchild restrict, timestamp precision

Addresses 2 Critical + 4 Important findings from spec review:

- C1: the crash-recovery test's assertions were vacuous - children were
  seeded via store.put()/mstore.loadDocument(), neither of which writes to
  the WAL, so the WAL held only the cascade's DELETE entries and recovery
  into a fresh store trivially "converged" regardless of whether
  writeCascadeWal() did anything. Fixed by seeding through
  PersistenceManager::logInsert() (a real WAL write) so recovery must
  genuinely apply INSERT-then-DELETE. Confirmed by reverting the fix and
  observing 8 assertions fail, then restoring it.
- C2: cascade mutations never queued replication or published Subscribe
  events (both live inside MemoryStore's persistCallback_, which the
  cascade bypasses to avoid double WAL-logging). Fixed by extracting
  DatabaseService::notifyReplicationAndEvents() out of that callback and
  having executeCascade() call it explicitly once per mutation, mirroring
  how v2.3.1 fixed replicated-entry apply the same way.
- I2: cascade wasn't recursive and could silently destroy a
  restrict-protected grandchild with canDelete() never seeing it. Fixed by
  refusing the whole cascade (new CascadeBlocked exception, mapped to
  FAILED_PRECONDITION) when a child slated for deletion is itself
  protected - not recursive cascading, which would need a much larger
  change to the WAL-first/single-transaction shape.
- I3: cascade-mutated documents were stamped in milliseconds regardless of
  the collection's configured precision, silently wrong on the ns-default
  (since v2.2.0). Fixed by threading CollectionConfigManager through
  planCascade() and stamping per the same rule MemoryStore::currentTimeFor
  uses.
- I4: a failure after the WAL fsync wasn't documented as possibly already
  durable. Documented in relation_cascade.hpp and surfaced in the gRPC
  error text; no compensating WAL entries (erasing a durable entry is
  riskier than leaving it, since replay is idempotent).
- M7: relation_enforcement.{hpp,cpp} comments no longer claim cascade/
  set_null are still permissive placeholders.

New test: test_cascade_refuses_when_grandchild_is_restrict_protected.
test_relation_enforcement: 90 -> 116 assertions.
fszontagh 1 ay önce
ebeveyn
işleme
748e1d25ac

+ 47 - 5
service/src/database_grpc_impl.cpp

@@ -859,15 +859,57 @@ grpc::Status DatabaseGrpcImpl::Delete(
                         }
                         if (hasDestructive) {
                             bool parentExisted = false;
+                            // v2.11.0 T12 review (C2) — drive replication +
+                            // Subscribe events explicitly, once per mutation,
+                            // right after that mutation's MemoryStore apply.
+                            // See relation_cascade.hpp's file header for why
+                            // executeCascade() cannot go back through the
+                            // ordinary persistCallback_ path itself.
+                            auto notify = [this](const std::string& coll, const std::string& id,
+                                                 const std::optional<smartbotic::database::Document>& doc,
+                                                 smartbotic::database::EventType eventType) {
+                                service_.notifyReplicationAndEvents(coll, id, doc, eventType);
+                            };
                             try {
                                 parentExisted = smartbotic::database::executeCascade(
-                                    relation_manager_, *lmdb, persistence_, store_,
-                                    request->collection(), request->id());
+                                    relation_manager_, *lmdb, persistence_, store_, config_manager_,
+                                    request->collection(), request->id(), notify);
+                            } catch (const smartbotic::database::CascadeBlocked& e) {
+                                // v2.11.0 T12 review (I2) — a grandchild is
+                                // protected by its own restrict relation.
+                                // Nothing was written anywhere (this is thrown
+                                // from planCascade(), before writeCascadeWal()
+                                // ever runs) — same refusal shape restrict
+                                // itself uses.
+                                return grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what());
                             } catch (const std::exception& e) {
-                                spdlog::error("relations: cascade delete of '{}/{}' failed: {}",
-                                             request->collection(), request->id(), e.what());
+                                // v2.11.0 T12 review (I4) — WAL-first means a
+                                // failure here can occur AFTER the WAL entries
+                                // for this cascade were already durably
+                                // fsynced (writeCascadeWal() ran inside
+                                // executeCascade() before commitCascadeLmdb()/
+                                // applyCascadeToMemory(), either of which could
+                                // be what actually threw). This is NOT
+                                // necessarily a no-op failure: the mutation may
+                                // already be committed to the WAL and WILL be
+                                // applied on the next restart's replay even
+                                // though this call reports failure now, and
+                                // neither store reflects it yet in the
+                                // meantime. There is no compensating "un-write"
+                                // of the WAL entry — replaying it again is
+                                // safe, erasing it is not. Check server logs
+                                // and, if in doubt, the current document state
+                                // before retrying.
+                                spdlog::error(
+                                    "relations: cascade delete of '{}/{}' failed - the WAL entries for "
+                                    "this cascade may already be durable and will apply on next "
+                                    "restart even though this call is reporting failure: {}",
+                                    request->collection(), request->id(), e.what());
                                 return grpc::Status(grpc::StatusCode::INTERNAL,
-                                    std::string("cascade delete failed: ") + e.what());
+                                    "cascade delete failed after some or all of its WAL entries may "
+                                    "already be durable - it may still apply on the next restart even "
+                                    "though this call failed; check server logs and current state "
+                                    "before retrying: " + std::string(e.what()));
                             }
                             // executeCascade already deleted the parent (and its
                             // vector, if any) atomically with every child

+ 63 - 53
service/src/database_service.cpp

@@ -1152,59 +1152,10 @@ void DatabaseService::setupComponents() {
         // race window where readers could see a doc in MemoryStore but not
         // yet in LMDB. The callback now does only WAL + replication + events.
 
-        // Queue for replication broadcast
-        if (replication_ && config_.replicationEnabled) {
-            databasepb::ReplicationEntry entry;
-            entry.set_collection(collection);
-            entry.set_document_id(id);
-            // v1.8.0 — stamp our nodeId so peers can attribute the write to
-            // its actual origin. Pre-v1.8 broadcasts left this empty, which
-            // is now treated by receivers as "legacy / unknown origin".
-            entry.set_node_id(config_.nodeId);
-            entry.set_global_timestamp(
-                std::chrono::duration_cast<std::chrono::milliseconds>(
-                    std::chrono::system_clock::now().time_since_epoch()
-                ).count());
-
-            switch (eventType) {
-                case EventType::INSERT:
-                    entry.set_op(databasepb::OP_INSERT);
-                    // Send the full Document envelope (id, collection, data,
-                    // version, timestamps, encryption state). The follower's
-                    // applyReplicatedEntry uses Document::fromJson() which
-                    // expects this envelope — sending only doc->data silently
-                    // dropped every user field on the other side.
-                    if (doc) entry.set_data(doc->toJson().dump());
-                    break;
-                case EventType::UPDATE:
-                    entry.set_op(databasepb::OP_UPDATE);
-                    if (doc) entry.set_data(doc->toJson().dump());
-                    break;
-                case EventType::DELETE:
-                    entry.set_op(databasepb::OP_DELETE);
-                    break;
-                default:
-                    break;
-            }
-
-            replication_->queueForReplication(entry);
-        }
-
-        // Publish event
-        if (events_) {
-            DatabaseEvent event;
-            event.type = eventType;
-            event.collection = collection;
-            event.documentId = id;
-            event.timestamp = std::chrono::duration_cast<std::chrono::milliseconds>(
-                std::chrono::system_clock::now().time_since_epoch()
-            ).count();
-            event.nodeId = config_.nodeId;
-            if (doc) {
-                event.data = doc->data();
-            }
-            events_->publish(event);
-        }
+        // v2.11.0 T12 review (C2) — replication + events extracted into
+        // notifyReplicationAndEvents() so relations/relation_cascade.cpp can
+        // drive them explicitly too. See that method's doc comment.
+        notifyReplicationAndEvents(collection, id, doc, eventType);
     });
 
     // Set up document load callback for LRU eviction recovery.
@@ -1282,6 +1233,65 @@ void DatabaseService::setupComponents() {
     );
 }
 
+void DatabaseService::notifyReplicationAndEvents(const std::string& collection,
+                                                  const std::string& id,
+                                                  const std::optional<Document>& doc,
+                                                  EventType eventType) {
+    // Queue for replication broadcast
+    if (replication_ && config_.replicationEnabled) {
+        databasepb::ReplicationEntry entry;
+        entry.set_collection(collection);
+        entry.set_document_id(id);
+        // v1.8.0 — stamp our nodeId so peers can attribute the write to
+        // its actual origin. Pre-v1.8 broadcasts left this empty, which
+        // is now treated by receivers as "legacy / unknown origin".
+        entry.set_node_id(config_.nodeId);
+        entry.set_global_timestamp(
+            std::chrono::duration_cast<std::chrono::milliseconds>(
+                std::chrono::system_clock::now().time_since_epoch()
+            ).count());
+
+        switch (eventType) {
+            case EventType::INSERT:
+                entry.set_op(databasepb::OP_INSERT);
+                // Send the full Document envelope (id, collection, data,
+                // version, timestamps, encryption state). The follower's
+                // applyReplicatedEntry uses Document::fromJson() which
+                // expects this envelope — sending only doc->data silently
+                // dropped every user field on the other side.
+                if (doc) entry.set_data(doc->toJson().dump());
+                break;
+            case EventType::UPDATE:
+                entry.set_op(databasepb::OP_UPDATE);
+                if (doc) entry.set_data(doc->toJson().dump());
+                break;
+            case EventType::DELETE:
+                entry.set_op(databasepb::OP_DELETE);
+                break;
+            default:
+                break;
+        }
+
+        replication_->queueForReplication(entry);
+    }
+
+    // Publish event
+    if (events_) {
+        DatabaseEvent event;
+        event.type = eventType;
+        event.collection = collection;
+        event.documentId = id;
+        event.timestamp = std::chrono::duration_cast<std::chrono::milliseconds>(
+            std::chrono::system_clock::now().time_since_epoch()
+        ).count();
+        event.nodeId = config_.nodeId;
+        if (doc) {
+            event.data = doc->data();
+        }
+        events_->publish(event);
+    }
+}
+
 void DatabaseService::applyReplicatedEntry(const databasepb::ReplicationEntry& entry) {
     try {
         // v1.8.0 — also append the replicated entry to the local WAL, tagged

+ 21 - 0
service/src/database_service.hpp

@@ -291,6 +291,27 @@ public:
         return mirror_drift_count_.load(std::memory_order_relaxed);
     }
 
+    // v2.11.0 T12 review (C2) — replication queueing + Subscribe event
+    // publication, extracted out of the MemoryStore persistCallback_
+    // lambda (setupComponents()) so a caller that deliberately bypasses
+    // that callback can still drive both explicitly, in the same shape
+    // the callback itself uses. This is what a cascade's `executeCascade`
+    // (relations/relation_cascade.cpp) calls once per child mutation and
+    // once for the parent delete, AFTER MemoryStore has been updated —
+    // mirroring how v2.3.1 fixed replicated-entry apply by explicitly
+    // driving `applyDualWriteMirror` rather than relying on a callback
+    // that had already been bypassed for the same "don't double up on
+    // WAL/mirror" reason. `collection` is the QUALIFIED name (what
+    // `ReplicationEntry`/`DatabaseEvent` both expect); `doc` is null for a
+    // delete. Does NOT touch WAL or the LMDB mirror — those are the
+    // caller's job, done separately, and calling this a second time for
+    // the same mutation would double-queue replication and double-publish
+    // the event, so callers must call it exactly once per mutation.
+    void notifyReplicationAndEvents(const std::string& collection,
+                                    const std::string& id,
+                                    const std::optional<Document>& doc,
+                                    EventType eventType);
+
     // v2.3 Stage F — project CRUD entry points (delegate to registry).
     // The registry is the source of truth for listing / creating / dropping.
     // CreateProject is idempotent. DropProject refuses "default". The

+ 64 - 9
service/src/relations/relation_cascade.cpp

@@ -6,22 +6,43 @@
 
 #include <spdlog/spdlog.h>
 
+#include "../config/collection_config_manager.hpp"
 #include "../memory_store.hpp"
 #include "../persistence/persistence_manager.hpp"
 #include "../project_addressing.hpp"
 #include "../storage/document_store_lmdb.hpp"
 #include "../storage/filter_eval.hpp"
 #include "../storage/lmdb_txn.hpp"
+#include "relation_enforcement.hpp"
 
 namespace smartbotic::database {
 
 namespace {
 
-uint64_t nowMs() {
+// v2.11.0 T12 review (I3) — mirrors MemoryStore::currentTimeFor() exactly
+// (memory_store.cpp): "ms" (the fallback) unless the collection's
+// CollectionCfg declares "ns". planCascade() only has an
+// LmdbDocumentStore&, not a MemoryStore&, so it cannot call
+// currentTimeFor() directly (private, and the wrong layer to reach across
+// to for one timestamp) — hence the duplication here rather than a shared
+// call. Getting this wrong is not a rounding error: resolveFilterValue()
+// returns Document::updatedAt raw, with no unit normalisation on any read/
+// sort/filter path (the 10^15-magnitude heuristic exists in exactly two
+// places — eviction's hot-write floor and migrateCollectionTimestamps'
+// idempotency guard — neither of which is a general-purpose read path). A
+// millisecond stamp on an "ns"-precision collection (the default since
+// v2.2.0) sorts as if written in 1970 and never matches an
+// `_updated_at > <ns-value>` filter.
+uint64_t nowForCollection(CollectionConfigManager& configManager,
+                          const std::string& qualifiedCollection) {
+    const auto cfg = configManager.configFor(qualifiedCollection);
+    const auto now = std::chrono::system_clock::now().time_since_epoch();
+    if (cfg.timestampPrecision == "ns") {
+        return static_cast<uint64_t>(
+            std::chrono::duration_cast<std::chrono::nanoseconds>(now).count());
+    }
     return static_cast<uint64_t>(
-        std::chrono::duration_cast<std::chrono::milliseconds>(
-            std::chrono::system_clock::now().time_since_epoch())
-            .count());
+        std::chrono::duration_cast<std::chrono::milliseconds>(now).count());
 }
 
 std::vector<std::string> splitDotPath(const std::string& path) {
@@ -111,6 +132,7 @@ struct WorkingChild {
 
 CascadePlan planCascade(RelationManager& relations,
                         smartbotic::db::storage::LmdbDocumentStore& store,
+                        CollectionConfigManager& configManager,
                         const std::string& qualifiedParentCollection,
                         const std::string& parentId) {
     CascadePlan plan;
@@ -182,6 +204,25 @@ CascadePlan planCascade(RelationManager& relations,
                 pullArrayIdInPlace(w.doc, r.childField, parentId);
             } else if (val->is_string() && val->get<std::string>() == parentId) {
                 if (r.onDelete == OnDelete::Cascade) {
+                    // v2.11.0 T12 review (I2) — this child is about to be
+                    // DELETED. Before committing to that, check whether IT
+                    // is itself protected by a restrict relation (a
+                    // grandchild of the delete being planned) — otherwise
+                    // this cascade would silently destroy a row that
+                    // relation exists to protect, with canDelete() never
+                    // having seen it (canDelete() only ever evaluates the
+                    // ORIGINAL parent/id). See the .hpp file header's "NOT
+                    // RECURSIVE" section for why this refuses rather than
+                    // cascading further, and why it is unconditional
+                    // (not gated by the child's own relationsEnforced).
+                    auto blocks = findRelationBlocks(relations, store, w.collectionQualified, w.childId);
+                    if (!blocks.empty()) {
+                        throw CascadeBlocked(
+                            "cascade delete of '" + qualifiedParentCollection + "/" + parentId +
+                            "' would delete '" + w.collectionQualified + "/" + w.childId +
+                            "', which is itself protected: " +
+                            formatRelationBlockError(w.collectionQualified, w.childId, blocks));
+                    }
                     w.deleted = true;
                     continue;   // don't touch w.doc further — it's being deleted
                 }
@@ -190,7 +231,7 @@ CascadePlan planCascade(RelationManager& relations,
                 continue;   // no longer actually references parentId — race, skip
             }
             w.doc.version += 1;
-            w.doc.updatedAt = nowMs();
+            w.doc.updatedAt = nowForCollection(configManager, w.collectionQualified);
         }
     }
 
@@ -263,27 +304,41 @@ bool commitCascadeLmdb(smartbotic::db::storage::LmdbDocumentStore& store,
 void applyCascadeToMemory(MemoryStore& memStore,
                           const std::string& qualifiedParentCollection,
                           const std::string& parentId,
-                          const CascadePlan& plan) {
+                          const CascadePlan& plan,
+                          bool parentExisted,
+                          const CascadeNotifyFn& notify) {
     for (const auto& m : plan.mutations) {
         if (m.kind == CascadeMutation::Kind::DeleteChild) {
             memStore.unloadDocument(m.childCollectionQualified, m.childId);
+            if (notify) notify(m.childCollectionQualified, m.childId, std::nullopt, EventType::DELETE);
         } else {
             memStore.loadDocumentWithHistory(m.childCollectionQualified, *m.updatedDoc);
+            if (notify) notify(m.childCollectionQualified, m.childId, m.updatedDoc, EventType::UPDATE);
         }
     }
     memStore.unloadDocument(qualifiedParentCollection, parentId);
+    // Gated on the LMDB-confirmed parentExisted, not MemoryStore's own
+    // return — see the .hpp file header. MemoryStore may not have had the
+    // parent cached (evicted) even though LMDB genuinely deleted it, or
+    // vice versa; LMDB is the source of truth for "did this really happen".
+    if (notify && parentExisted) {
+        notify(qualifiedParentCollection, parentId, std::nullopt, EventType::DELETE);
+    }
 }
 
 bool executeCascade(RelationManager& relations,
                     smartbotic::db::storage::LmdbDocumentStore& store,
                     PersistenceManager& persistence,
                     MemoryStore& memStore,
+                    CollectionConfigManager& configManager,
                     const std::string& qualifiedParentCollection,
-                    const std::string& parentId) {
-    const CascadePlan plan = planCascade(relations, store, qualifiedParentCollection, parentId);
+                    const std::string& parentId,
+                    const CascadeNotifyFn& notify) {
+    const CascadePlan plan = planCascade(relations, store, configManager,
+                                         qualifiedParentCollection, parentId);
     writeCascadeWal(persistence, qualifiedParentCollection, parentId, plan);
     const bool parentExisted = commitCascadeLmdb(store, qualifiedParentCollection, parentId, plan);
-    applyCascadeToMemory(memStore, qualifiedParentCollection, parentId, plan);
+    applyCascadeToMemory(memStore, qualifiedParentCollection, parentId, plan, parentExisted, notify);
     return parentExisted;
 }
 

+ 160 - 21
service/src/relations/relation_cascade.hpp

@@ -36,8 +36,61 @@
 // exact sequence, and tests/test_relation_enforcement.cpp for the crash
 // simulation that pins this.
 //
+// ⚠ WHAT "WAL-BEFORE-LMDB" DOES NOT FIX (review finding I1): MemoryStore
+// recovers correctly from the WAL alone, as above. LMDB itself does NOT
+// converge for every mutation kind after a crash in the WAL→commit window.
+// DELETE replay goes through MemoryStore::remove(), which still mirrors to
+// LMDB (memory_store.cpp), so a crashed-and-recovered DeleteChild/parent
+// delete eventually reaches LMDB too, on the next boot's replay. UPDATE
+// replay (a SetNull/array-pull mutation) goes through
+// loadDocumentWithHistory(), which mirrors NOTHING — and the one-time
+// backfillIntoDocStore() sync is short-circuited by the schema_version=2
+// marker on any already-migrated install, so it will not run again. A
+// crash in that window therefore leaves LMDB permanently serving the
+// PRE-cascade child (parent id still in the array, or the field still
+// non-null) — reads are LMDB-first, so this is user-visible, not just an
+// internal inconsistency — plus a live reverse-index posting pointing at a
+// now-deleted parent, invisible until that child is next rewritten through
+// the ordinary write path (which re-mirrors it). This is a real, currently
+// UNCLOSED gap — recorded here rather than fixed because closing it needs
+// either extending loadDocumentWithHistory() to mirror (a MemoryStore
+// change with implications far beyond this module) or a dedicated
+// re-mirror pass keyed off the mutations WAL replay just applied, and both
+// are more than this task's remaining budget covers. Flagged for whoever
+// picks this up next; do not assume LMDB is exempt from the crash window
+// just because MemoryStore is.
+//
+// ⚠ NOT RECURSIVE, AND WHAT THAT MEANS FOR `restrict` (review finding I2):
+// planCascade() only ever looks at relations whose parent is the document
+// actually being deleted. A child this cascade deletes is never itself
+// re-planned as a parent — grandchildren under a Cascade/SetNull relation
+// declared on that child are left dangling (the same class of leftover
+// `relations check`, Task 7, exists to find — not new, and not silently
+// worse than what no_action already leaves behind everywhere else). The
+// dangerous case is different: if a grandchild relation declares
+// `restrict`, deleting the top-level parent would otherwise destroy a
+// row that relation exists to protect, and `RelationEnforcer::canDelete()`
+// never sees it — it only ever evaluates the ORIGINAL parent/id. planCascade()
+// closes that specific hole (not the general dangling-grandchild one): for
+// every child this plan would actually DELETE (never for one it only
+// updates — that child survives), it calls findRelationBlocks() against
+// that child exactly as if IT were being deleted, and throws CascadeBlocked
+// (caught by the Delete handler, mapped to the same FAILED_PRECONDITION
+// restrict itself uses) if anything blocks. I chose refuse-the-whole-
+// cascade over recursive cascading because recursion changes the shape of
+// this entire module — WAL-first and one-atomic-txn both need to extend to
+// an unbounded graph walk rather than one level, which is a much larger
+// change than this finding's fix budget covers — and because refusing is
+// the conservative, safety-preserving choice: a cascade that would bypass
+// a restrict guard is now loudly rejected rather than silently destroying
+// the row that guard exists to protect. This check runs unconditionally,
+// NOT gated by the grandchild collection's own relationsEnforced — being
+// more cautious than strictly required here is deliberate.
+//
 // Sequence, in full (mirrors the design doc):
-//   1. Resolve children through the reverse index — planCascade().
+//   1. Resolve children through the reverse index — planCascade(). Also
+//      where the restrict/grandchild check above runs, and where
+//      CascadeBlocked can be thrown.
 //   2. restrict still blocks upstream (relation_enforcement.hpp) — this
 //      module is never reached if that already refused the delete.
 //   3. WAL-log the parent delete + every child mutation, fsync —
@@ -46,21 +99,52 @@
 //      index (automatic — see maintainRelations in document_store_lmdb.cpp),
 //      delete the parent, commit — commitCascadeLmdb().
 //   5. Apply the same mutations to MemoryStore, taking each affected
-//      collection's lock in turn — applyCascadeToMemory().
+//      collection's lock in turn — applyCascadeToMemory(). Also where the
+//      optional replication/Subscribe-event notification (see below) fires,
+//      once per mutation, after that mutation's MemoryStore apply.
 // executeCascade() runs 3 → 4 → 5 in order; it is what DatabaseGrpcImpl::
 // Delete calls once RelationEnforcer::canDelete() has already permitted
 // the delete.
 //
-// Scope note: this module does NOT queue replication or publish Subscribe
-// events for the child mutations or the parent delete — see the Task 12
-// report for why (the ordinary per-doc write path gets both for free from
-// MemoryStore's persistCallback_, which this module deliberately bypasses
-// to avoid double-logging to WAL; reaching replication/events without that
-// callback needs its own follow-up, out of this task's scope).
-
+// ⚠ FAILURE AFTER THE WAL COMMIT (review finding I4): steps 3-5 are not
+// itself transactional as a WHOLE — only step 4 (the LMDB txn) is
+// all-or-nothing. If commitCascadeLmdb() or applyCascadeToMemory() throws
+// AFTER writeCascadeWal() has already fsynced, the mutation is already
+// DURABLE (the next boot's WAL replay will apply it) even though the
+// caller receives an error and neither store reflects it yet. This is a
+// direct, unavoidable consequence of WAL-first: making the WAL entry
+// contingent on the LMDB commit succeeding would reopen exactly the
+// resurrection window this module exists to close. DatabaseGrpcImpl::Delete
+// says so explicitly in the error text it returns rather than leaving the
+// caller to assume nothing happened; there is no compensating "un-write"
+// of the WAL entry, because replaying it again is safe (idempotent) but
+// erasing it is not (a second failure between erasure and re-fsync would
+// then silently lose the cascade for real).
+//
+// Replication + Subscribe events (review finding C2): the ordinary
+// per-document write path gets both for free because
+// MemoryStore::remove()/update() call emitPersist() -> persistCallback_ ->
+// DatabaseService's lambda, which does WAL + replication + events in one
+// block. executeCascade() cannot reuse that callback directly — it already
+// does its own WAL logging (step 3) and its own LMDB mirroring (step 4),
+// so going back through the ordinary MemoryStore write path in step 5
+// would re-log every mutation to WAL a second time and re-attempt the LMDB
+// mirror a second time, undermining the "WAL and the LMDB commit each
+// happen exactly once, in that order" property this whole module exists to
+// establish. Instead, DatabaseService::notifyReplicationAndEvents() (the
+// replication+events TAIL of that same lambda, extracted so it can be
+// called on its own) is passed in as `notify` and invoked once per
+// mutation, right after that mutation's MemoryStore apply — mirroring how
+// v2.3.1 fixed replicated-entry apply by explicitly driving
+// applyDualWriteMirror rather than relying on a callback that had already
+// been bypassed for the same reason. `notify` is optional (nullptr skips
+// it) so the unit tests in tests/test_relation_enforcement.cpp, which have
+// no DatabaseService, keep working unchanged.
 #pragma once
 
+#include <functional>
 #include <optional>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -75,6 +159,17 @@ namespace smartbotic::database {
 
 class PersistenceManager;
 class MemoryStore;
+class CollectionConfigManager;
+
+// Thrown by planCascade() when proceeding would silently destroy a row
+// protected by its own `restrict` relation (a grandchild of the delete
+// being planned) — see the file header's "NOT RECURSIVE" section.
+// DatabaseGrpcImpl::Delete catches this and maps it to FAILED_PRECONDITION,
+// the same status code a direct restrict refusal uses.
+class CascadeBlocked : public std::runtime_error {
+public:
+    explicit CascadeBlocked(std::string msg) : std::runtime_error(std::move(msg)) {}
+};
 
 // One resolved child-side mutation a cascade/set_null relation requires.
 struct CascadeMutation {
@@ -92,11 +187,33 @@ struct CascadePlan {
     bool parentHadVector = false;
 };
 
-// Step 1 — read-only. Resolves every Cascade/SetNull relation whose parent
-// is qualifiedParentCollection, walks the reverse index for parentId under
-// each (unbounded — a cascade must see every child, never a truncated
-// sample), reads the current child documents, and decides Delete vs
-// Update per the array rule in the file header. Mutates nothing.
+// Called once per mutation (child), and once more for the parent, from
+// applyCascadeToMemory() — AFTER that specific document's MemoryStore
+// apply — to drive replication + Subscribe events explicitly. See the file
+// header's "Replication + Subscribe events" section. `doc` is nullopt for
+// a delete. Typically bound to
+// DatabaseService::notifyReplicationAndEvents(); nullptr/empty skips
+// notification entirely (what every unit test in
+// tests/test_relation_enforcement.cpp does, having no DatabaseService).
+using CascadeNotifyFn = std::function<void(const std::string& qualifiedCollection,
+                                           const std::string& id,
+                                           const std::optional<Document>& doc,
+                                           EventType eventType)>;
+
+// Step 1 — read-only except for the throw path. Resolves every
+// Cascade/SetNull relation whose parent is qualifiedParentCollection,
+// walks the reverse index for parentId under each (unbounded — a cascade
+// must see every child, never a truncated sample), reads the current child
+// documents, and decides Delete vs Update per the array rule in the file
+// header. For every child this plan would DELETE, also checks whether that
+// child is itself protected by a `restrict` relation (see the file
+// header's "NOT RECURSIVE" section) and throws CascadeBlocked if so —
+// the only way this function mutates anything is by NOT returning.
+//
+// `configManager` supplies the per-collection timestamp_precision each
+// mutated child is stamped with (see the file header — mirrors
+// MemoryStore::currentTimeFor() exactly, since planCascade() only has an
+// LmdbDocumentStore&, not a MemoryStore&, to ask directly).
 //
 // If the SAME child is targeted by two different relations from this
 // parent (e.g. two fields on one child collection both referencing it),
@@ -106,6 +223,7 @@ struct CascadePlan {
 // the Task 12 report for the reasoning.
 CascadePlan planCascade(RelationManager& relations,
                         smartbotic::db::storage::LmdbDocumentStore& store,
+                        CollectionConfigManager& configManager,
                         const std::string& qualifiedParentCollection,
                         const std::string& parentId);
 
@@ -127,6 +245,12 @@ void writeCascadeWal(PersistenceManager& persistence,
 // children were mutated — a cascade run against an already-gone parent id
 // still cleans up any dangling children the reverse index still names, the
 // same repair `relations check` (Task 7) exists to find).
+//
+// ⚠ See the file header's "LMDB does not converge" note (I1): this step's
+// own commit is atomic, but if it or applyCascadeToMemory() never runs at
+// all (the process crashed between writeCascadeWal()'s fsync and here),
+// LMDB stays pre-cascade for UpdateChild mutations specifically until that
+// row is next rewritten through the ordinary write path.
 bool commitCascadeLmdb(smartbotic::db::storage::LmdbDocumentStore& store,
                        const std::string& qualifiedParentCollection,
                        const std::string& parentId,
@@ -134,24 +258,39 @@ bool commitCascadeLmdb(smartbotic::db::storage::LmdbDocumentStore& store,
 
 // Step 5 — apply the same mutations to MemoryStore, taking each affected
 // collection's lock IN TURN (one call per document, not one lock spanning
-// the whole cascade). Memory-only, no WAL/mirror/callback side effects —
-// see MemoryStore::unloadDocument()'s header comment for why.
+// the whole cascade). Memory-only, no WAL/mirror side effects — see
+// MemoryStore::unloadDocument()'s header comment for why. If `notify` is
+// set, it is called once per mutation (and once for the parent, gated on
+// `parentExisted` — commitCascadeLmdb()'s return, the LMDB-confirmed
+// answer, NOT whatever MemoryStore's own cache happened to hold, which may
+// have evicted the parent independently) immediately after that document's
+// MemoryStore apply — see the file header's "Replication + Subscribe
+// events" section. Every CHILD mutation notifies unconditionally: each one
+// was already confirmed real by planCascade()'s LMDB read and committed by
+// commitCascadeLmdb(), independent of whether THIS node's MemoryStore
+// cache happened to hold that child.
 void applyCascadeToMemory(MemoryStore& store,
                           const std::string& qualifiedParentCollection,
                           const std::string& parentId,
-                          const CascadePlan& plan);
+                          const CascadePlan& plan,
+                          bool parentExisted,
+                          const CascadeNotifyFn& notify = nullptr);
 
 // Orchestrates steps 3 → 4 → 5, in order, with nothing in between. What
 // DatabaseGrpcImpl::Delete calls once RelationEnforcer::canDelete() (step
 // 2 — restrict) has already permitted the delete; this function does not
-// re-check restrict and must not be reached if that refused. Returns
-// commitCascadeLmdb()'s result — whether the parent document itself was
-// present and removed — for the handler's DeleteResponse.deleted field.
+// re-check restrict and must not be reached if that refused. May throw
+// CascadeBlocked from the planning step (see above) before anything is
+// written anywhere. Returns commitCascadeLmdb()'s result — whether the
+// parent document itself was present and removed — for the handler's
+// DeleteResponse.deleted field.
 bool executeCascade(RelationManager& relations,
                     smartbotic::db::storage::LmdbDocumentStore& store,
                     PersistenceManager& persistence,
                     MemoryStore& memStore,
+                    CollectionConfigManager& configManager,
                     const std::string& qualifiedParentCollection,
-                    const std::string& parentId);
+                    const std::string& parentId,
+                    const CascadeNotifyFn& notify = nullptr);
 
 } // namespace smartbotic::database

+ 7 - 3
service/src/relations/relation_enforcement.cpp

@@ -58,9 +58,13 @@ std::vector<RelationBlock> findRelationBlocks(
     std::vector<RelationBlock> out;
 
     for (const auto& r : relations.relationsWithParent(qualifiedParentCollection)) {
-        // Only restrict blocks. no_action is the documented escape hatch;
-        // cascade/set_null are not yet destructive (Task 12) and behave as
-        // permissive until then - see the file header.
+        // Only restrict blocks here. no_action is the documented escape
+        // hatch. cascade/set_null (v2.11.0 T12+) ARE destructive, but that
+        // destruction is handled entirely by relations/relation_cascade.cpp,
+        // AFTER this function has permitted the delete - so they still fall
+        // through this filter unblocked, on purpose, not because they are
+        // still permissive placeholders. See relation_cascade.hpp's file
+        // header for what actually happens to them.
         if (r.onDelete != OnDelete::Restrict) continue;
 
         const auto lookup = lookupRelationCounts(store, r, parentId);

+ 11 - 6
service/src/relations/relation_enforcement.hpp

@@ -8,12 +8,17 @@
 // gate and before store_.remove(); Task 9 exercises it over the real gRPC
 // boundary.
 //
-// Scope: only `restrict` blocks. `no_action` permits the delete and leaves
-// the reference dangling - the documented escape hatch. A relation
-// declaring `cascade` or `set_null` ALSO permits for now (behaves exactly
-// like no_action) because the destructive write-path work those policies
-// need is Task 12's, not this one's - do not read the permissive behaviour
-// here as their final semantics.
+// Scope: only `restrict` blocks HERE. `no_action` permits the delete and
+// leaves the reference dangling - the documented escape hatch. A relation
+// declaring `cascade` or `set_null` also falls through this filter
+// unblocked, but as of v2.11.0 T12 that is NOT permissive placeholder
+// behaviour - those policies are genuinely destructive, just handled by a
+// separate module (relations/relation_cascade.cpp), called by the Delete
+// handler AFTER this check has permitted the delete. Do not read "not
+// blocked here" as "does nothing" - see relation_cascade.hpp's file header
+// for what a cascade/set_null delete actually does, including the one
+// place it can still refuse: a grandchild protected by its own `restrict`
+// relation.
 
 #pragma once
 

+ 177 - 7
tests/test_relation_enforcement.cpp

@@ -22,6 +22,7 @@
 
 #include <nlohmann/json.hpp>
 
+#include "config/collection_config_manager.hpp"
 #include "document.hpp"
 #include "memory_store.hpp"
 #include "persistence/persistence_manager.hpp"
@@ -33,8 +34,10 @@
 
 namespace fs = std::filesystem;
 
+using smartbotic::database::CascadeBlocked;
 using smartbotic::database::CascadeMutation;
 using smartbotic::database::CascadePlan;
+using smartbotic::database::CollectionConfigManager;
 using smartbotic::database::Document;
 using smartbotic::database::MemoryStore;
 using smartbotic::database::OnDelete;
@@ -528,6 +531,8 @@ void test_cascade_deletes_scalar_children_wal_first() {
     mstore.start();
     RelationManager rm(mstore);
     rm.loadFromStore();
+    CollectionConfigManager cfgManager(mstore);
+    cfgManager.loadFromStore();
 
     TmpEnv t("rel-cascade-scalar");
     LmdbDocumentStore store(t.env);
@@ -563,9 +568,37 @@ void test_cascade_deletes_scalar_children_wal_first() {
 
     check(store.relation_index_child_count("exec_wf", "wf-1") == 3, "three children indexed");
 
-    const bool parentExisted = executeCascade(rm, store, p.pm, mstore, "default:workflows", "wf-1");
+    // v2.11.0 T12 review (C2) - a notify callback wired the way
+    // DatabaseGrpcImpl::Delete wires DatabaseService::notifyReplicationAndEvents,
+    // recorded here instead of actually queuing replication/publishing events
+    // (this test has no DatabaseService). Confirms executeCascade() actually
+    // drives it once per mutation plus once for the parent, with the right
+    // event type each time - the wiring C2 added, not just that it compiles.
+    struct Notification {
+        std::string collection, id;
+        bool hadDoc;
+        smartbotic::database::EventType eventType;
+    };
+    std::vector<Notification> notifications;
+    auto notify = [&](const std::string& coll, const std::string& id,
+                      const std::optional<Document>& doc,
+                      smartbotic::database::EventType et) {
+        notifications.push_back({coll, id, doc.has_value(), et});
+    };
+
+    const bool parentExisted = executeCascade(rm, store, p.pm, mstore, cfgManager,
+                                              "default:workflows", "wf-1", notify);
     check(parentExisted, "the parent document was present and removed");
 
+    check(notifications.size() == 4, "notified for e1, e2, e3 and the parent - nothing missed, nothing extra");
+    int deleteNotifications = 0;
+    for (const auto& n : notifications) {
+        check(!n.hadDoc, "every notification here is a delete - no doc payload");
+        check(n.eventType == smartbotic::database::EventType::DELETE, "every notification is a DELETE");
+        if (n.eventType == smartbotic::database::EventType::DELETE) ++deleteNotifications;
+    }
+    check(deleteNotifications == 4, "all four notifications are DELETE (3 children + parent)");
+
     // LMDB: children gone, index gone.
     check(!store.get("executions", "e1").has_value(), "e1 gone from LMDB");
     check(!store.get("executions", "e2").has_value(), "e2 gone from LMDB");
@@ -606,6 +639,8 @@ void test_array_reference_pulls_id_and_keeps_document() {
     mstore.start();
     RelationManager rm(mstore);
     rm.loadFromStore();
+    CollectionConfigManager cfgManager(mstore);
+    cfgManager.loadFromStore();
 
     TmpEnv t("rel-cascade-array");
     LmdbDocumentStore store(t.env);
@@ -638,9 +673,35 @@ void test_array_reference_pulls_id_and_keeps_document() {
     std::string mgrErr;
     check(rm.createRelation(rel, mgrErr), "declared the cascade relation on an array field");
 
-    const bool parentExisted = executeCascade(rm, store, p.pm, mstore, "default:credentials", "c1");
+    // v2.11.0 T12 review (C2) - same notify-recording as the scalar test,
+    // to confirm the UPDATE (not DELETE) path also notifies correctly with
+    // the post-mutation document attached.
+    struct Notification {
+        std::string collection, id;
+        bool hadDoc;
+        smartbotic::database::EventType eventType;
+    };
+    std::vector<Notification> notifications;
+    auto notify = [&](const std::string& coll, const std::string& id,
+                      const std::optional<Document>& doc,
+                      smartbotic::database::EventType et) {
+        notifications.push_back({coll, id, doc.has_value(), et});
+    };
+
+    const bool parentExisted = executeCascade(rm, store, p.pm, mstore, cfgManager,
+                                              "default:credentials", "c1", notify);
     check(parentExisted, "the credential document was present and removed");
 
+    check(notifications.size() == 2, "notified for the node update and the parent delete");
+    check(notifications[0].collection == "default:nodes" && notifications[0].id == "n1" &&
+          notifications[0].hadDoc &&
+          notifications[0].eventType == smartbotic::database::EventType::UPDATE,
+          "node notification is an UPDATE carrying the mutated doc");
+    check(notifications[1].collection == "default:credentials" && notifications[1].id == "c1" &&
+          !notifications[1].hadDoc &&
+          notifications[1].eventType == smartbotic::database::EventType::DELETE,
+          "parent notification is a DELETE with no doc payload");
+
     // The node survives, in BOTH stores, with c1 pulled and the other two ids intact.
     auto lmdbNode = store.get("nodes", "n1");
     check(lmdbNode.has_value(), "node n1 still exists in LMDB - not deleted");
@@ -708,6 +769,8 @@ void test_crash_between_wal_and_commit_recovers() {
     mstore.start();
     RelationManager rm(mstore);
     rm.loadFromStore();
+    CollectionConfigManager cfgManager(mstore);
+    cfgManager.loadFromStore();
 
     TmpEnv t("rel-cascade-crash");
     LmdbDocumentStore store(t.env);
@@ -716,10 +779,20 @@ void test_crash_between_wal_and_commit_recovers() {
     TmpPersistence p("rel-cascade-crash-wal");
     check(p.pm.start(), "persistence manager started");
 
+    // v2.11.0 T12 review (C1) - seed through the REAL WAL via
+    // p.pm.logInsert(), not just store.put()/mstore.loadDocument() (neither
+    // of which writes to the WAL). Without this the WAL below would contain
+    // ONLY the cascade's DELETE entries, freshStore would start from
+    // nothing, and "!freshStore.get(...).has_value()" would be true whether
+    // or not writeCascadeWal() wrote anything at all - a vacuous assertion
+    // (caught in review; confirmed by actually reverting this fix and
+    // re-running, see the Task 12 report). Seeding via logInsert makes the
+    // WAL read INSERT-then-DELETE per document, so recovery only ends up
+    // with nothing there if the DELETE entries genuinely got applied.
     auto putBoth = [&](const std::string& id, const nlohmann::json& data) {
         Document d; d.id = id; d.collection = "executions"; d.set_data(data);
         store.put("executions", id, d);
-        mstore.loadDocument("default:executions", d);
+        p.pm.logInsert("default:executions", d);
     };
     putBoth("e1", {{"workflowId", "wf-1"}});
     putBoth("e2", {{"workflowId", "wf-1"}});
@@ -728,7 +801,7 @@ void test_crash_between_wal_and_commit_recovers() {
         Document parent; parent.id = "wf-1"; parent.collection = "workflows";
         parent.set_data({{"name", "example"}});
         store.put("workflows", "wf-1", parent);
-        mstore.loadDocument("default:workflows", parent);
+        p.pm.logInsert("default:workflows", parent);
     }
 
     RelationInfo rel;
@@ -742,7 +815,7 @@ void test_crash_between_wal_and_commit_recovers() {
 
     // Steps 1+3 only - simulating a crash immediately after flushWal()
     // returns, before commitCascadeLmdb()/applyCascadeToMemory() ever run.
-    CascadePlan plan = planCascade(rm, store, "default:workflows", "wf-1");
+    CascadePlan plan = planCascade(rm, store, cfgManager, "default:workflows", "wf-1");
     check(plan.mutations.size() == 2, "planned both children");
     writeCascadeWal(p.pm, "default:workflows", "wf-1", plan);
     p.pm.stop();   // closes the WAL file, like a process exiting
@@ -757,6 +830,15 @@ void test_crash_between_wal_and_commit_recovers() {
     check(store.get("workflows", "wf-1").has_value(),
           "LMDB was never committed - the parent is still there");
 
+    // The WAL now genuinely contains 3 INSERTs (e1, e2, parent) followed by
+    // 3 DELETEs (e1, e2, parent) from writeCascadeWal() - read directly,
+    // the same way test_cascade_deletes_scalar_children_wal_first does,
+    // rather than only inferred from the recovered store's absence.
+    auto rawEntries = replayRawWal(p.path);
+    check(walHasDelete(rawEntries, "default:executions", "e1"), "WAL has DELETE for e1");
+    check(walHasDelete(rawEntries, "default:executions", "e2"), "WAL has DELETE for e2");
+    check(walHasDelete(rawEntries, "default:workflows", "wf-1"), "WAL has DELETE for the parent");
+
     // "Restart": fresh MemoryStore, fresh PersistenceManager over the SAME
     // dataDir, recover().
     MemoryStore freshStore(MemoryStore::Config{});
@@ -767,9 +849,13 @@ void test_crash_between_wal_and_commit_recovers() {
     auto outcome = pm2.recover(freshStore);
     check(outcome.kind != smartbotic::database::RecoveryOutcome::Kind::Failed,
           "recovery did not fail");
-    check(outcome.walEntriesReplayed >= 3,
-          "replayed at least the parent delete + two child deletes");
+    check(outcome.walEntriesReplayed >= 6,
+          "replayed the 3 inserts AND the 3 deletes (parent + two children)");
 
+    // Load-bearing: with the C1 fix, this is only possible because the WAL
+    // held both the INSERT and the DELETE for each id, and recovery applied
+    // both in order. Without the DELETE entries (the bug this test exists
+    // to catch), these would all be PRESENT after recovery instead.
     check(!freshStore.get("default:executions", "e1").has_value(),
           "recovery converges: e1 is gone from the recovered MemoryStore");
     check(!freshStore.get("default:executions", "e2").has_value(),
@@ -790,6 +876,89 @@ void test_crash_between_wal_and_commit_recovers() {
     mstore.stop();
 }
 
+// v2.11.0 T12 review (I2) - a cascade must refuse rather than silently
+// destroy a grandchild protected by its own `restrict` relation.
+// workflows --cascade--> executions --restrict--> logs: deleting the
+// workflow would, without this check, delete e1 out from under a log entry
+// that names it, with canDelete() never having evaluated e1 at all (it only
+// ever evaluates the ORIGINAL parent, wf-1). Would fail if regressed: a
+// version of planCascade() without the grandchild check would throw
+// nothing here and instead silently proceed to delete e1.
+void test_cascade_refuses_when_grandchild_is_restrict_protected() {
+    MemoryStore mstore(MemoryStore::Config{});
+    mstore.start();
+    RelationManager rm(mstore);
+    rm.loadFromStore();
+    CollectionConfigManager cfgManager(mstore);
+    cfgManager.loadFromStore();
+
+    TmpEnv t("rel-cascade-grandchild");
+    LmdbDocumentStore store(t.env);
+    store.set_relations("executions", {{"exec_wf", "workflowId"}});
+    store.set_relations("logs", {{"log_exec", "executionId"}});
+
+    TmpPersistence p("rel-cascade-grandchild-wal");
+    check(p.pm.start(), "persistence manager started");
+
+    {
+        Document parent; parent.id = "wf-1"; parent.collection = "workflows";
+        parent.set_data({{"name", "example"}});
+        store.put("workflows", "wf-1", parent);
+    }
+    {
+        Document exec; exec.id = "e1"; exec.collection = "executions";
+        exec.set_data({{"workflowId", "wf-1"}});
+        store.put("executions", "e1", exec);
+    }
+    {
+        Document log; log.id = "log1"; log.collection = "logs";
+        log.set_data({{"executionId", "e1"}});
+        store.put("logs", "log1", log);
+    }
+
+    std::string mgrErr;
+    RelationInfo cascadeRel;
+    cascadeRel.name = "default:exec_wf";
+    cascadeRel.child = "default:executions";
+    cascadeRel.childField = "workflowId";
+    cascadeRel.parent = "default:workflows";
+    cascadeRel.onDelete = OnDelete::Cascade;
+    check(rm.createRelation(cascadeRel, mgrErr), "declared the cascade relation");
+
+    RelationInfo restrictRel;
+    restrictRel.name = "default:log_exec";
+    restrictRel.child = "default:logs";
+    restrictRel.childField = "executionId";
+    restrictRel.parent = "default:executions";
+    restrictRel.onDelete = OnDelete::Restrict;
+    check(rm.createRelation(restrictRel, mgrErr), "declared the grandchild restrict relation");
+
+    bool threw = false;
+    std::string thrownMessage;
+    try {
+        executeCascade(rm, store, p.pm, mstore, cfgManager, "default:workflows", "wf-1");
+    } catch (const CascadeBlocked& e) {
+        threw = true;
+        thrownMessage = e.what();
+    }
+    check(threw, "cascade refuses rather than silently deleting the restrict-protected grandchild");
+    check(thrownMessage.find("e1") != std::string::npos, "names the blocked child");
+    check(thrownMessage.find("log_exec") != std::string::npos || thrownMessage.find("logs") != std::string::npos,
+          "names the blocking relation or collection");
+
+    // Nothing was mutated anywhere - the throw happens inside planCascade(),
+    // before writeCascadeWal() ever runs.
+    check(store.get("workflows", "wf-1").has_value(), "parent untouched");
+    check(store.get("executions", "e1").has_value(), "the protected child untouched");
+    check(store.get("logs", "log1").has_value(), "the grandchild untouched");
+
+    p.pm.stop();
+    auto entries = replayRawWal(p.path);
+    check(entries.empty(), "nothing was ever written to the WAL - the refusal happened before step 3");
+
+    mstore.stop();
+}
+
 }  // namespace
 
 int main() {
@@ -807,6 +976,7 @@ int main() {
     test_cascade_deletes_scalar_children_wal_first();
     test_array_reference_pulls_id_and_keeps_document();
     test_crash_between_wal_and_commit_recovers();
+    test_cascade_refuses_when_grandchild_is_restrict_protected();
 
     std::cout << "passed: " << g_pass << ", failed: " << g_fail << "\n";
     return g_fail == 0 ? 0 : 1;