From 8f58229cd18769ade676d1dd4f6a9047f4889107 Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 06:24:59 -0400 Subject: [PATCH 1/6] design(add_relationship): atomic collapse (PR 2 of 2) design doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse do_add_relationship/7's five transactions into one for TOCTOU isolation. Grep-confirmed the four phase helpers are single-use → convert in place; do_class_of/1 has a public caller → add class_of_in_txn/1 twin. Spends PR 1's tier-1 primitives. Behaviour-preserving; +2 error-atom tests (source/target_has_no_class), no atomicity/race test. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- .../designs/atomic-add-relationship-design.md | 184 ++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 docs/designs/atomic-add-relationship-design.md diff --git a/docs/designs/atomic-add-relationship-design.md b/docs/designs/atomic-add-relationship-design.md new file mode 100644 index 0000000..fa59b7e --- /dev/null +++ b/docs/designs/atomic-add-relationship-design.md @@ -0,0 +1,184 @@ + + +# Atomic `add_relationship` — Design + +**Status:** Approved (design) — not yet planned/implemented +**Date:** 2026-06-21 +**Author:** David W. Thomas (with Claude) +**Slice:** Atomic `add_relationship`, PR 2 of 2 (the collapse) + +## Background + +PR 1 (`ad030f6`, `docs/designs/atomic-add-relationship-primitives-design.md`) +added the tier-1 in-transaction read primitives to `graphdb_class` +(`default_template_in_txn/1`, `get_template_in_txn/1`, +`class_in_ancestry_in_txn/2`) without touching any existing path. This PR +spends them: it collapses `graphdb_instance:do_add_relationship/7`'s five +separate transactions into one. + +The three-tier transaction seam (PR #41 / PR #43, +`docs/designs/transaction-seam-retrofit-design.md`): + +- **Tier 1** — in-transaction primitives: bare mnesia ops, signal failure via + `mnesia:abort/1`, never open their own transaction, so they compose. +- **Tier 2** — single-op public API: owns exactly one transaction via + `graphdb_mgr:transaction/1`. +- **Tier 3** — batch/composite: wraps one transaction, calls tier-1 primitives + directly. + +`add_relationship` is a tier-2 operation that today is *five* transactions +instead of one. This PR makes it own exactly one. + +## The honest reframe (carried from PR 1) + +`do_add_relationship/7` runs four read-only phases, each in its own +transaction or gen_server call, then writes in a fifth: + +1. `validate_arc_endpoints` — read the four endpoint nodes (own txn) +2. `resolve_arc_classes` — `do_class_of/1` ×2 (two txns) +3. `resolve_template` — `graphdb_class:default_template/1` (gen_server txn) +4. `validate_template_scope` — `graphdb_class:get_template/1` + + `class_in_ancestry/2` ×2 (gen_server reads) +5. `write_connection_arcs` — the two directed rows (fifth txn) + +**Only phase 5 writes.** Phases 1–4 are read-only, so a failure in any of +them never reaches the write — **there is no partial-write bug today.** Both +rows already write together in one transaction. + +Collapsing therefore does not fix a bug. It buys exactly one thing: + +- **TOCTOU isolation** — validation and the write share one consistent + snapshot, closing the window where another process retires an endpoint, + deletes a class, or changes a template *between* validation and write. + +TOCTOU isolation is protection against a *race*. A race has no deterministic +CT test (see Testing). The behaviour every test can observe is unchanged. + +## Goal + +Make `do_add_relationship/7` own exactly one transaction, preserving every +externally observable behaviour, and give the two currently-uncovered error +atoms (`source_has_no_class`, `target_has_no_class`) test coverage. + +## Refactor shape + +A grep over `apps/graphdb/src/` confirms the four phase helpers +(`validate_arc_endpoints`, `resolve_arc_classes`, `resolve_template`, +`validate_template_scope`) are each **single-use** — referenced only at their +definition and the one call site in `do_add_relationship/7`. That settles the +shape: + +| Helper | Treatment | +| --------------------------------- | ------------------------------------------------------------------------------------------------------- | +| `validate_arc_endpoints/6` | Convert in place to in-txn: drop the `graphdb_mgr:transaction` wrapper, lift its already-abort-based body | +| `resolve_arc_classes/2` | Convert in place: use `class_of_in_txn/1`; `not_found` arms become `mnesia:abort/1` | +| `resolve_template/2` | Convert in place: `default` arm uses `default_template_in_txn/1`, aborts on `not_found` | +| `validate_template_scope/3` | Convert in place: use `get_template_in_txn/1` + `class_in_ancestry_in_txn/2`, abort on failure | +| `do_class_of/1` | **Add, don't rewrap** — see below | + +`do_class_of/1` is the one exception. It has a public caller — +`handle_call({class_of, Nref}, …)` at line 425 — so it cannot be converted in +place. Add a new private `class_of_in_txn/1` (bare +`mnesia:index_read(relationships, InstanceNref, #relationship.source_nref)` + +`lists:search` for `?ARC_INST_TO_CLASS`, returning `{ok, ClassNref} | +not_found`); leave `do_class_of/1` untouched. This mirrors PR 1's +add-don't-rewrap decision and the same rationale: the gen_server path keeps +its own transaction for its own caller. + +Phases 3 and 4 already have their primitives from PR 1 +(`default_template_in_txn/1`, `get_template_in_txn/1`, +`class_in_ancestry_in_txn/2`). No new `graphdb_class` work in this PR. + +## The collapsed flow + +`do_add_relationship/7`: + +1. Allocate the relationship-id pair **up-front**, outside the transaction: + `{Id1, Id2} = rel_id_server:get_id_pair()`. Allocation is a gen_server call + and must never run inside an mnesia transaction fun (it would risk blocking, + and a transaction retry would re-call it). This is the + allocate-outside-transaction doctrine already followed by the B4 connection + firing path. A validation abort now orphans one id pair — harmless and + accepted. +2. Run one `graphdb_mgr:transaction/1` fun, in this order: + - `validate_arc_endpoints` (in-txn) — reads four nodes, aborts on any + endpoint/kind/retired/target-kind violation + - `resolve_arc_classes` (in-txn) — `class_of_in_txn/1` ×2, aborts + `{source_has_no_class, _}` / `{target_has_no_class, _}` + - `resolve_template` (in-txn) — yields `TemplateNref`, aborts + `no_default_template` + - `validate_template_scope` (in-txn) — aborts `{invalid_template, …}` / + `{template_class_not_in_ancestry, …}` + - build the two rows with the pre-allocated `{Id1, Id2}` and resolved + `TemplateNref`, then `mnesia:write` both +3. Map the result: `{ok, ok}` → `ok`; `{error, Reason}` → `{error, Reason}`. + +The public contract (`add_relationship/4,5,6` → `ok | {error, term()}`) is +unchanged. + +### `build_connection_rows` split (DRY) + +`build_connection_rows/6` currently allocates the id pair *and* builds the +rows. Split it so allocation is the caller's choice: + +- `build_connection_rows/7({Id1, Id2}, S, C, T, R, TemplateNref, AVPSpec)` — + pure builder, no allocation. The id pair rides as a single tuple argument so + the arity is `/7` (the existing `/6` plus one tuple arg). +- `build_connection_rows/6` — allocates `{Id1, Id2}` via + `rel_id_server:get_id_pair()` then delegates to `/7`. Unchanged for its + callers. + +The B4 callers (`build_connection_rows/6` at line 767, `write_connection_arcs` +at line 1392) are untouched. The collapsed `do_add_relationship` calls the pure +builder inside its transaction with the up-front-allocated pair. + +`write_connection_arcs/6` stays — it is still used by the B4 auto post-commit +connection pass (line 864). It is simply no longer on the `add_relationship` +path. + +## Behaviour preservation + +Same discipline as the PR #43 transaction-seam retrofit: + +- **Phase order is preserved** inside the single fun, so an input violating + multiple constraints reports the same first error it does today. +- `graphdb_mgr:transaction/1` maps `{aborted, Reason}` → `{error, Reason}` + byte-identically to the current inline mapping, so an in-fun + `mnesia:abort(Reason)` surfaces as the same `{error, Reason}`. +- `validate_arc_endpoints`' body is already abort-based, so every endpoint + atom (`source_not_found`, `target_not_found`, `characterization_not_found`, + `reciprocal_not_found`, `endpoint_retired`, `*_not_an_attribute`, + `target_kind_mismatch`) is preserved free when the body is lifted. +- `source_has_no_class` / `target_has_no_class` convert from `{error, _}` + return values to `mnesia:abort/1` with byte-identical Reason terms. +- The nested Reason inside `{invalid_template, TemplateNref, Reason}` is + preserved: `get_template_in_txn` returns the same inner `{error, + not_a_template | not_found}` as the gen_server `get_template`. + +## Testing + +The collapse buys TOCTOU isolation — a race — which has **no deterministic CT +test**. There is no partial-write state to expose and both rows already write +in one transaction. An "atomicity test" here would either be flaky or assert +nothing. **It will not be written.** + +The test deliverable is exactly: + +- **Two new tests** in `graphdb_instance_SUITE` for the previously-uncovered + error atoms: `add_relationship` with a source that has no class → + `{error, {source_has_no_class, SourceNref}}`; with a target that has no + class → `{error, {target_has_no_class, TargetNref}}`. +- **The entire existing `add_relationship` suite passing unchanged** — this is + the behaviour-preservation proof. No existing test is modified. + +## Non-goals + +- `mutate([Mutation])` — the tier-3 batch entry point. Separate, later slice; + reuses the same primitives this PR exercises. +- Converging the default-template name-search duplication noted in PR 1's + design (the gen_server `do_find_template_by_name` vs `default_template_in_txn` + copies). Out of scope; deliberately not converged. +- Any change to `graphdb_class` — its primitives are complete as of PR 1. From 278e2ebbf8d4de8763801ed2a8e7d4912f8da183 Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 06:45:46 -0400 Subject: [PATCH 2/6] plan: atomic add_relationship collapse (PR 2 of 2) 3 TDD tasks: (1) characterization tests for source/target_has_no_class; (2) collapse do_add_relationship/7 into one transaction (in-txn phase helpers + class_of_in_txn/1 + build_connection_rows split); (3) docs incl. deferred default-template name-search convergence task. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- .../2026-06-21-atomic-add-relationship.md | 481 ++++++++++++++++++ 1 file changed, 481 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-21-atomic-add-relationship.md diff --git a/docs/superpowers/plans/2026-06-21-atomic-add-relationship.md b/docs/superpowers/plans/2026-06-21-atomic-add-relationship.md new file mode 100644 index 0000000..2113d7d --- /dev/null +++ b/docs/superpowers/plans/2026-06-21-atomic-add-relationship.md @@ -0,0 +1,481 @@ +# Atomic `add_relationship` Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Collapse `graphdb_instance:do_add_relationship/7`'s five separate transactions into one, spending PR 1's tier-1 primitives, while preserving every observable behaviour. + +**Architecture:** Convert the four single-use phase helpers (`validate_arc_endpoints`, `resolve_arc_classes`, `resolve_template`, `validate_template_scope`) in place to in-transaction helpers that signal failure via `mnesia:abort/1`; add a private `class_of_in_txn/1` twin (the gen-server `do_class_of/1` keeps its own txn for its public caller); split `build_connection_rows` so the rel-id pair is allocated up-front outside the transaction; rewrite `do_add_relationship/7` to run validate → resolve classes → resolve template → validate scope → write inside one `graphdb_mgr:transaction/1` fun. + +**Tech Stack:** Erlang/OTP 28.5, rebar3 3.27 (repo-local `./rebar3`), Mnesia, Common Test. + +## Global Constraints + +- Design contract: `docs/designs/atomic-add-relationship-design.md`. Read it first. +- **Behaviour-preserving.** No existing test is modified. The entire existing `add_relationship` suite must pass unchanged — that is the behaviour-preservation proof. +- **No atomicity/race test.** The collapse buys TOCTOU isolation (a race); it has no deterministic CT test and none will be written. New tests are exactly the two error-atom characterization tests. +- **Byte-identical Reason terms.** `source_has_no_class`/`target_has_no_class` convert from `{error, _}` returns to `mnesia:abort/1` with the same Reason terms: `{source_has_no_class, SourceNref}`, `{target_has_no_class, TargetNref}`. +- **Phase order preserved** inside the single fun: validate endpoints → resolve classes → resolve template → validate scope → write. An input violating multiple constraints must report the same first error. +- **Allocate the rel-id pair OUTSIDE the transaction.** `rel_id_server:get_id_pair()` is a gen_server call and must never run inside an mnesia transaction fun. A validation abort orphans one id pair — accepted under the allocate-outside-transaction doctrine. +- **`graphdb_mgr:transaction/1` wrap shape:** the fun returns `ok` on success → `{ok, ok}` → map to `ok`; an abort → `{error, Reason}` → return verbatim. Public contract `add_relationship/4,5,6 -> ok | {error, term()}` is unchanged. +- **Hard tabs.** `apps/graphdb/src/graphdb_instance.erl` and `apps/graphdb/test/graphdb_instance_SUITE.erl` use hard tabs for indentation. Match them. +- **Macros already in scope** in `graphdb_instance.erl`: `?ARC_INST_TO_CLASS` (instance→class membership char), `?ARC_TEMPLATE` (Template AVP attr 31). No new includes. +- **Test runner:** single suite — `scripts/test-ct-parallel.sh instance`; full — `make test-ct-parallel` then `./rebar3 eunit`. Invoke `./rebar3` directly (PATH/kerl preset; no `source ~/.bashrc` prefix). + +--- + +### Task 1: Characterization tests for the two uncovered class-resolution atoms + +Lock the `source_has_no_class` / `target_has_no_class` contract with tests **before** the refactor. Both atoms are already returned by the current code, so these tests pass immediately against `HEAD`; they then form part of the green net the collapse must preserve. + +**Files:** +- Modify (test): `apps/graphdb/test/graphdb_instance_SUITE.erl` + +**Interfaces:** +- Consumes: `graphdb_class:create_class/2`, `graphdb_instance:create_instance/3`, `graphdb_attr:create_relationship_attribute_pair/3`, `graphdb_instance:add_relationship/4`. +- Produces: two new CT cases — `add_relationship_rejects_source_has_no_class/1`, `add_relationship_rejects_target_has_no_class/1`. + +**Why a class node serves as the "node with no class":** `validate_arc_endpoints` does **not** check the source's/target's `kind`; it only requires the node to exist, be un-retired, and (for the target) match the characterization's `target_kind` AVP. A freshly-created class node satisfies that, yet has no instance→class (`?ARC_INST_TO_CLASS`) outgoing arc, so `do_class_of` on it returns `not_found`. For the target case, a characterization whose `target_kind = class` lets a class node pass endpoint validation as the target. + +- [ ] **Step 1: Add the two cases to the `-export` list and the test group** + +In the `-export([...])` block near the other `add_relationship_*` entries (around line 74-91), add: + +```erlang + add_relationship_rejects_source_has_no_class/1, + add_relationship_rejects_target_has_no_class/1, +``` + +In `groups()` where the `add_relationship_*` cases are listed (around line 219-236), add the two names alongside the existing `add_relationship_rejects_*` cases: + +```erlang + add_relationship_rejects_source_has_no_class, + add_relationship_rejects_target_has_no_class, +``` + +- [ ] **Step 2: Write the two test functions** + +Add after `add_relationship_rejects_target_kind_mismatch/1` (around line 862), matching the file's hard-tab indentation and comment style: + +```erlang +%%----------------------------------------------------------------------------- +%% source that exists and passes endpoint validation but has no instance->class +%% membership arc is rejected. A class node is such a node: validate_arc_endpoints +%% does not constrain the source's kind, and a class has no ?ARC_INST_TO_CLASS arc. +%%----------------------------------------------------------------------------- +add_relationship_rejects_source_has_no_class(_Config) -> + {ok, ClassNref} = graphdb_class:create_class("Thing", 3), + {ok, B, _} = graphdb_instance:create_instance("B", ClassNref, 5), + {ok, {Char, Recip}} = + graphdb_attr:create_relationship_attribute_pair("Knows", "KnownBy", instance), + ?assertEqual({error, {source_has_no_class, ClassNref}}, + graphdb_instance:add_relationship(ClassNref, Char, B, Recip)). + +%%----------------------------------------------------------------------------- +%% target that exists and passes endpoint validation but has no instance->class +%% membership arc is rejected. Char's target_kind=class lets a class node pass +%% endpoint validation as the target; the class has no ?ARC_INST_TO_CLASS arc. +%%----------------------------------------------------------------------------- +add_relationship_rejects_target_has_no_class(_Config) -> + {ok, ClassNref} = graphdb_class:create_class("Thing", 3), + {ok, A, _} = graphdb_instance:create_instance("A", ClassNref, 5), + {ok, {Char, Recip}} = + graphdb_attr:create_relationship_attribute_pair("Has", "HeldBy", class), + ?assertEqual({error, {target_has_no_class, ClassNref}}, + graphdb_instance:add_relationship(A, Char, ClassNref, Recip)). +``` + +- [ ] **Step 3: Run the two new cases — verify they PASS against current code** + +Run: `scripts/test-ct-parallel.sh instance` + +Expected: PASS. The two new cases pass against `HEAD` (the atoms are already returned by today's `resolve_arc_classes`). This confirms the contract the refactor must preserve. If either FAILS, the test setup is wrong (e.g. endpoint validation rejected the class node first) — fix the test, do not touch source. + +- [ ] **Step 4: Commit** + +```bash +git add apps/graphdb/test/graphdb_instance_SUITE.erl +git commit -m "test(add_relationship): cover source/target_has_no_class atoms + +Characterization tests for the two previously-uncovered class-resolution +error atoms, locked in before the atomic-collapse refactor. + +Co-Authored-By: Claude Opus 4.8 +Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF" +``` + +--- + +### Task 2: Collapse `do_add_relationship/7` into a single transaction + +Convert the four phase helpers in place to in-txn helpers, add `class_of_in_txn/1`, split `build_connection_rows`, and rewrite `do_add_relationship/7`. All tests (existing + Task 1's two new ones) stay green. + +**Files:** +- Modify: `apps/graphdb/src/graphdb_instance.erl` + +**Interfaces:** +- Consumes (from PR 1, `graphdb_class`): `default_template_in_txn/1 -> {ok, Nref} | not_found`; `get_template_in_txn/1 -> {ok, #node{}} | {error, not_a_template | not_found}`; `class_in_ancestry_in_txn/2 -> boolean()`. +- Consumes (existing, unchanged): `head_parent/1`, `check_target_kind/3`, `first_retired/2`, `rel_id_server:get_id_pair/0`, `graphdb_mgr:transaction/1`. +- Produces (private, this module): `class_of_in_txn/1 -> {ok, ClassNref} | not_found`; `validate_arc_endpoints_in_txn/6 -> ok` (aborts on violation); `resolve_arc_classes_in_txn/2 -> {SourceClass, TargetClass}` (aborts); `resolve_template_in_txn/2 -> TemplateNref` (aborts); `validate_template_scope_in_txn/3 -> ok` (aborts); `build_connection_rows/7({Id1,Id2}, S, C, T, R, TemplateNref, AVPSpec) -> [{relationships, #relationship{}}]`. +- Unchanged for B4 callers: `build_connection_rows/6` (now allocates then delegates to `/7`), `write_connection_arcs/6`, `do_class_of/1`. + +- [ ] **Step 1: Run the full suite first — confirm the green baseline** + +Run: `scripts/test-ct-parallel.sh instance` + +Expected: PASS (includes Task 1's two new cases). This is the baseline the refactor must keep green. Note the case count. + +- [ ] **Step 2: Add `class_of_in_txn/1` next to `do_class_of/1`** + +`do_class_of/1` lives around line 1482 and has a public caller (`handle_call({class_of, Nref}, …)` ~line 425) — leave it untouched. Add the in-txn twin immediately after `do_class_of/1`: + +```erlang +%%----------------------------------------------------------------------------- +%% class_of_in_txn(InstanceNref) -> {ok, ClassNref} | not_found +%% +%% Tier-1 in-transaction twin of do_class_of/1. Assumes it runs inside an +%% active mnesia activity; uses a bare index_read. do_class_of/1 keeps its +%% own transaction for its public class_of caller. +%%----------------------------------------------------------------------------- +class_of_in_txn(InstanceNref) -> + Rels = mnesia:index_read(relationships, InstanceNref, + #relationship.source_nref), + case lists:search( + fun(R) -> + R#relationship.characterization =:= ?ARC_INST_TO_CLASS + end, Rels) of + {value, #relationship{target_nref = ClassNref}} -> {ok, ClassNref}; + false -> not_found + end. +``` + +- [ ] **Step 3: Replace `validate_arc_endpoints/6` with `validate_arc_endpoints_in_txn/6`** + +Replace the whole `validate_arc_endpoints/6` definition (its head comment may stay or be updated; the body is the change) — drop the `F = fun() ... end` wrapper and the trailing `graphdb_mgr:transaction(F)` case; the body becomes the function body directly, returning `ok` on success and aborting otherwise: + +```erlang +%%----------------------------------------------------------------------------- +%% validate_arc_endpoints_in_txn(Source, Char, Target, Reciprocal, TkAttr, +%% RetAttr) -> ok (aborts the enclosing transaction on any violation) +%% +%% In-transaction endpoint validation. Assumes it runs inside an active mnesia +%% activity; reads the four nodes with bare mnesia:read and signals every +%% violation via mnesia:abort/1 (same Reason terms as the prior own-txn form). +%%----------------------------------------------------------------------------- +validate_arc_endpoints_in_txn(SourceNref, CharNref, TargetNref, ReciprocalNref, + TkAttr, RetAttr) -> + Source = mnesia:read(nodes, SourceNref), + Target = mnesia:read(nodes, TargetNref), + Char = mnesia:read(nodes, CharNref), + Recip = mnesia:read(nodes, ReciprocalNref), + case {Source, Target, Char, Recip} of + {[], _, _, _} -> + mnesia:abort({source_not_found, SourceNref}); + {_, [], _, _} -> + mnesia:abort({target_not_found, TargetNref}); + {_, _, [], _} -> + mnesia:abort({characterization_not_found, CharNref}); + {_, _, _, []} -> + mnesia:abort({reciprocal_not_found, ReciprocalNref}); + {[#node{attribute_value_pairs = SAVPs}], + [#node{kind = TKind, attribute_value_pairs = TAVPs}], + [#node{kind = CKind, attribute_value_pairs = CAVPs} = CharNode], + [#node{kind = RKind, attribute_value_pairs = RAVPs}]} -> + case first_retired([{SourceNref, SAVPs}, {TargetNref, TAVPs}, + {CharNref, CAVPs}, {ReciprocalNref, RAVPs}], + RetAttr) of + {retired, RNref} -> + mnesia:abort({endpoint_retired, RNref}); + none -> + case {CKind, RKind} of + {attribute, attribute} -> + case check_target_kind(CharNode, TKind, TkAttr) of + ok -> ok; + {error, Reason} -> mnesia:abort(Reason) + end; + {attribute, _} -> + mnesia:abort({reciprocal_not_an_attribute, + ReciprocalNref, RKind}); + {_, _} -> + mnesia:abort({characterization_not_an_attribute, + CharNref, CKind}) + end + end + end. +``` + +Leave `first_retired/2`, `is_retired/2`, `check_target_kind/3`, and `find_avp_value/2` exactly as they are. + +- [ ] **Step 4: Replace `resolve_arc_classes/2` with `resolve_arc_classes_in_txn/2`** + +Replace the whole `resolve_arc_classes/2` definition with: + +```erlang +%%----------------------------------------------------------------------------- +%% resolve_arc_classes_in_txn(SourceNref, TargetNref) -> +%% {SourceClass, TargetClass} (aborts on a missing class) +%% +%% In-transaction class resolution. class_of_in_txn returns only {ok,_} | +%% not_found inside a txn (a read error aborts the txn directly), so the +%% no-class arms abort with the same Reason terms the prior form returned. +%%----------------------------------------------------------------------------- +resolve_arc_classes_in_txn(SourceNref, TargetNref) -> + SourceClass = case class_of_in_txn(SourceNref) of + {ok, SC} -> SC; + not_found -> mnesia:abort({source_has_no_class, SourceNref}) + end, + TargetClass = case class_of_in_txn(TargetNref) of + {ok, TC} -> TC; + not_found -> mnesia:abort({target_has_no_class, TargetNref}) + end, + {SourceClass, TargetClass}. +``` + +- [ ] **Step 5: Replace `resolve_template/2` with `resolve_template_in_txn/2`** + +Replace both clauses of `resolve_template/2` with: + +```erlang +%%----------------------------------------------------------------------------- +%% resolve_template_in_txn(TemplateSpec, SourceClass) -> TemplateNref +%% (aborts no_default_template when `default' is requested but absent) +%%----------------------------------------------------------------------------- +resolve_template_in_txn(default, SourceClass) -> + case graphdb_class:default_template_in_txn(SourceClass) of + {ok, Nref} -> Nref; + not_found -> mnesia:abort(no_default_template) + end; +resolve_template_in_txn(TemplateNref, _SourceClass) + when is_integer(TemplateNref) -> + TemplateNref. +``` + +- [ ] **Step 6: Replace `validate_template_scope/3` with `validate_template_scope_in_txn/3`** + +Replace the whole `validate_template_scope/3` definition with: + +```erlang +%%----------------------------------------------------------------------------- +%% validate_template_scope_in_txn(TemplateNref, SourceClass, TargetClass) -> ok +%% (aborts invalid_template / template_class_not_in_ancestry) +%% +%% Confirms TemplateNref resolves to a kind=template node whose parent class is +%% in SourceClass's or TargetClass's taxonomic ancestry. The nested Reason in +%% {invalid_template, _, Reason} is byte-identical to the gen-server get_template +%% form: get_template_in_txn returns the same {error, not_a_template|not_found}. +%%----------------------------------------------------------------------------- +validate_template_scope_in_txn(TemplateNref, SourceClass, TargetClass) -> + case graphdb_class:get_template_in_txn(TemplateNref) of + {ok, #node{parents = TmplParents}} -> + TmplClass = head_parent(TmplParents), + InSource = graphdb_class:class_in_ancestry_in_txn(TmplClass, + SourceClass), + InTarget = graphdb_class:class_in_ancestry_in_txn(TmplClass, + TargetClass), + case InSource orelse InTarget of + true -> ok; + false -> mnesia:abort({template_class_not_in_ancestry, + TemplateNref, TmplClass, SourceClass, TargetClass}) + end; + {error, Reason} -> + mnesia:abort({invalid_template, TemplateNref, Reason}) + end. +``` + +- [ ] **Step 7: Split `build_connection_rows` into `/6` (allocates) + `/7` (pure)** + +Replace the existing `build_connection_rows/6` definition (around line 1362) with these two definitions. The `/6` form now only allocates and delegates; the `/7` form is the pure builder taking a pre-allocated id pair: + +```erlang +build_connection_rows(SourceNref, CharNref, TargetNref, ReciprocalNref, + TemplateNref, AVPSpec) -> + IdPair = rel_id_server:get_id_pair(), + build_connection_rows(IdPair, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, AVPSpec). + +%% build_connection_rows({Id1, Id2}, S, C, T, R, TemplateNref, {FwdAVPs,RevAVPs}) +%% -> [{relationships, #relationship{}}] +%% +%% Pure builder: no allocation. The caller supplies the rel-id pair (allocated +%% up-front, outside any transaction) so the rows can be built inside a caller's +%% transaction. Template AVP rides index 0 of each direction. +build_connection_rows({Id1, Id2}, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, {FwdAVPs, RevAVPs}) -> + TemplateAVP = #{attribute => ?ARC_TEMPLATE, value => TemplateNref}, + Fwd = #relationship{ + id = Id1, kind = connection, + source_nref = SourceNref, + characterization = CharNref, + target_nref = TargetNref, + reciprocal = ReciprocalNref, + avps = [TemplateAVP | FwdAVPs] + }, + Rev = #relationship{ + id = Id2, kind = connection, + source_nref = TargetNref, + characterization = ReciprocalNref, + target_nref = SourceNref, + reciprocal = CharNref, + avps = [TemplateAVP | RevAVPs] + }, + [{relationships, Fwd}, {relationships, Rev}]. +``` + +Leave the existing `build_connection_rows/6` head comment (the one describing the no-write contract) in place above the `/6` clause, or fold it into these comments — do not duplicate it. Leave `write_connection_arcs/6` unchanged: it still calls `build_connection_rows/6` and is still used by the B4 auto post-commit pass. + +- [ ] **Step 8: Rewrite `do_add_relationship/7` as one transaction** + +Replace the whole `do_add_relationship/7` definition (around line 1188) with: + +```erlang +do_add_relationship(SourceNref, CharNref, TargetNref, ReciprocalNref, + TemplateSpec, AVPSpec, State) -> + TkAttr = State#state.target_kind_avp_nref, + RetAttr = State#state.retired_nref, + %% Allocate the rel-id pair up-front, OUTSIDE the transaction: get_id_pair + %% is a gen_server call and must never run inside an mnesia fun. A + %% validation abort below orphans this pair -- harmless (allocate-outside- + %% transaction doctrine). + IdPair = rel_id_server:get_id_pair(), + Txn = fun() -> + ok = validate_arc_endpoints_in_txn(SourceNref, CharNref, TargetNref, + ReciprocalNref, TkAttr, RetAttr), + {SourceClass, TargetClass} = + resolve_arc_classes_in_txn(SourceNref, TargetNref), + TemplateNref = resolve_template_in_txn(TemplateSpec, SourceClass), + ok = validate_template_scope_in_txn(TemplateNref, SourceClass, + TargetClass), + Rows = build_connection_rows(IdPair, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, AVPSpec), + lists:foreach(fun({Tab, Rec}) -> ok = mnesia:write(Tab, Rec, write) end, + Rows) + end, + case graphdb_mgr:transaction(Txn) of + {ok, ok} -> ok; + {error, _} = Err -> Err + end. +``` + +Update the `do_add_relationship/7` head comment so it describes the single-transaction flow (validate → resolve classes → resolve template → scope → write, all in one txn; id pair allocated up-front). + +- [ ] **Step 9: Compile — verify clean, zero warnings** + +Run: `./rebar3 compile` + +Expected: clean compile, **zero warnings**. In particular, no "function … is unused" warnings (the four old phase helpers were renamed, not duplicated; `class_of_in_txn/1` is used by `resolve_arc_classes_in_txn/2`). If a warning appears, fix the cause before proceeding. + +- [ ] **Step 10: Run the instance suite — verify all green** + +Run: `scripts/test-ct-parallel.sh instance` + +Expected: PASS, same case count as Step 1's baseline. Every existing `add_relationship_*` case plus Task 1's two new cases pass unchanged. This is the behaviour-preservation proof. + +- [ ] **Step 11: Run the full CT + EUnit suites — verify no cross-suite regression** + +`write_connection_arcs/6` and the B4 connection-firing path call `build_connection_rows`; confirm nothing regressed. + +Run: `make test-ct-parallel` then `./rebar3 eunit` + +Expected: all green, zero warnings. + +- [ ] **Step 12: Commit** + +```bash +git add apps/graphdb/src/graphdb_instance.erl +git commit -m "feat(add_relationship): collapse into a single transaction + +Validate endpoints, resolve class/template scope, and write the two +directed rows in one graphdb_mgr:transaction/1 (TOCTOU-isolated). Convert +the four single-use phase helpers in place to in-txn (abort-based) form; +add private class_of_in_txn/1 (do_class_of/1 keeps its own txn for its +public caller); split build_connection_rows into /6 (allocates) + /7 +(pure) so the rel-id pair is allocated up-front outside the txn. Spends +PR 1's tier-1 primitives. Behaviour-preserving. + +Co-Authored-By: Claude Opus 4.8 +Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF" +``` + +--- + +### Task 3: Documentation + +Mark the follow-up implemented, record the deferred convergence task, and note the single-transaction behaviour in the worker doc. + +**Files:** +- Modify: `TASKS.md` +- Modify: `apps/graphdb/CLAUDE.md` + +- [ ] **Step 1: Update the `add_relationship` follow-up bullet in `TASKS.md`** + +Replace the **Atomic `add_relationship`** bullet (around line 136-144) with the implemented form: + +```markdown +- **Atomic `add_relationship`** — IMPLEMENTED. `do_add_relationship/7`'s five + separate transactions (validate endpoints → resolve classes → resolve + template → validate scope → write) are collapsed into one + `graphdb_mgr:transaction/1` (TOCTOU isolation). The four single-use phase + helpers were converted in place to in-txn (abort-based) form; a private + `class_of_in_txn/1` was added (`do_class_of/1` keeps its own txn for its + public caller); `build_connection_rows` was split into `/6` (allocates) + + `/7` (pure) so the rel-id pair is allocated up-front outside the + transaction. Behaviour-preserving; existing `add_relationship` suite + unchanged, +2 new instance CT cases (`source_has_no_class` / + `target_has_no_class`). Design + `docs/designs/atomic-add-relationship-design.md`; plan + `docs/superpowers/plans/2026-06-21-atomic-add-relationship.md`. +``` + +- [ ] **Step 2: Add the deferred convergence task to `TASKS.md`** + +Immediately after the **Batch `mutate([Mutation])`** bullet (around line 145), add a new tracked follow-up: + +```markdown +- **Converge default-template name search** — `graphdb_class` carries two + copies of the default-template name-search walk: the gen-server + `do_find_template_by_name/2` (own txn) and the tier-1 + `default_template_in_txn/1` (PR 1). The gen-server `default_template/1` path + is already transactional, so `do_default_template/1` could be rewritten to + wrap a txn and call `default_template_in_txn/1`, removing the duplication. + Deliberately deferred (the duplication is sanctioned project precedent); + a future cleanup, not blocking anything. +``` + +- [ ] **Step 3: Note single-transaction behaviour in `apps/graphdb/CLAUDE.md`** + +Find the `add_relationship/4` bullet in the `graphdb_instance` API section: + +```markdown +- `add_relationship/4` (source_nref, characterization_nref, target_nref, reciprocal_nref) — writes two directed rows atomically; IDs allocated via `get_nref()` +``` + +Replace it with: + +```markdown +- `add_relationship/4,5,6` (source_nref, characterization_nref, target_nref, reciprocal_nref [, template_nref [, {FwdAVPs, RevAVPs}]]) — validates endpoints, resolves source/target class and template scope, and writes the two directed `kind=connection` rows in a **single** `graphdb_mgr:transaction/1` (TOCTOU-isolated). The rel-id pair is allocated up-front (outside the transaction) via `rel_id_server:get_id_pair/0`. `/4` uses the source class's default template; `/5` takes an explicit template nref; `/6` adds per-direction AVPs. +``` + +- [ ] **Step 4: Confirm `docs/Architecture.md` needs no change** + +The public contract (signatures and return shapes of `add_relationship/4,5,6`) is unchanged; the collapse is an internal transaction-count change already covered by the documented three-tier seam. **Do not edit `docs/Architecture.md`.** + +- [ ] **Step 5: Commit** + +```bash +git add TASKS.md apps/graphdb/CLAUDE.md +git commit -m "docs(add_relationship): record atomic collapse + deferred convergence + +Mark the Atomic add_relationship follow-up implemented; add the deferred +default-template name-search convergence task; note add_relationship's +single-transaction behaviour in the graphdb worker doc. + +Co-Authored-By: Claude Opus 4.8 +Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF" +``` + +--- + +## Notes for the executor + +- The collapse fixes no bug (only the final phase writes today). It buys TOCTOU isolation, a race with no deterministic test. **Do not write an atomicity test.** The behaviour-preservation proof is the existing suite passing unchanged plus Task 1's two characterization tests. +- The four converted phase helpers are single-use (grep-confirmed: each appears only at its definition and the one call site in `do_add_relationship/7`), so converting them in place leaves no dead code and no second caller to break. `do_class_of/1` is the sole exception — it has a public `handle_call` caller — hence the add-don't-rewrap twin. +- Reason-term fidelity is the one correctness risk: keep the abort terms byte-identical to the prior returns, and preserve phase order. From 106b872b78a9fa623bfb7f7843f004d23ca31917 Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 06:50:14 -0400 Subject: [PATCH 3/6] test(add_relationship): cover source/target_has_no_class atoms Characterization tests for the two previously-uncovered class-resolution error atoms, locked in before the atomic-collapse refactor. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- apps/graphdb/test/graphdb_instance_SUITE.erl | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/apps/graphdb/test/graphdb_instance_SUITE.erl b/apps/graphdb/test/graphdb_instance_SUITE.erl index c03dc3b..68f8f3d 100644 --- a/apps/graphdb/test/graphdb_instance_SUITE.erl +++ b/apps/graphdb/test/graphdb_instance_SUITE.erl @@ -85,6 +85,8 @@ add_relationship_rejects_non_attribute_char/1, add_relationship_rejects_non_attribute_reciprocal/1, add_relationship_rejects_target_kind_mismatch/1, + add_relationship_rejects_source_has_no_class/1, + add_relationship_rejects_target_has_no_class/1, add_relationship_stamps_user_avps/1, add_relationship_avps_are_per_direction/1, add_relationship_default_avps_empty/1, @@ -230,6 +232,8 @@ groups() -> add_relationship_rejects_non_attribute_char, add_relationship_rejects_non_attribute_reciprocal, add_relationship_rejects_target_kind_mismatch, + add_relationship_rejects_source_has_no_class, + add_relationship_rejects_target_has_no_class, add_relationship_stamps_user_avps, add_relationship_avps_are_per_direction, add_relationship_default_avps_empty, @@ -861,6 +865,32 @@ add_relationship_rejects_target_kind_mismatch(_Config) -> ?assertEqual({error, {target_kind_mismatch, class, instance}}, graphdb_instance:add_relationship(A, Char, B, Recip)). +%%----------------------------------------------------------------------------- +%% source that exists and passes endpoint validation but has no instance->class +%% membership arc is rejected. A class node is such a node: validate_arc_endpoints +%% does not constrain the source's kind, and a class has no ?ARC_INST_TO_CLASS arc. +%%----------------------------------------------------------------------------- +add_relationship_rejects_source_has_no_class(_Config) -> + {ok, ClassNref} = graphdb_class:create_class("Thing", 3), + {ok, B, _} = graphdb_instance:create_instance("B", ClassNref, 5), + {ok, {Char, Recip}} = + graphdb_attr:create_relationship_attribute_pair("Knows", "KnownBy", instance), + ?assertEqual({error, {source_has_no_class, ClassNref}}, + graphdb_instance:add_relationship(ClassNref, Char, B, Recip)). + +%%----------------------------------------------------------------------------- +%% target that exists and passes endpoint validation but has no instance->class +%% membership arc is rejected. Char's target_kind=class lets a class node pass +%% endpoint validation as the target; the class has no ?ARC_INST_TO_CLASS arc. +%%----------------------------------------------------------------------------- +add_relationship_rejects_target_has_no_class(_Config) -> + {ok, ClassNref} = graphdb_class:create_class("Thing", 3), + {ok, A, _} = graphdb_instance:create_instance("A", ClassNref, 5), + {ok, {Char, Recip}} = + graphdb_attr:create_relationship_attribute_pair("Has", "HeldBy", class), + ?assertEqual({error, {target_has_no_class, ClassNref}}, + graphdb_instance:add_relationship(A, Char, ClassNref, Recip)). + %%----------------------------------------------------------------------------- %% /6 stamps user AVPs on both connection rows alongside the From 0ad8471bdf8e8e0dd7f8dd6f86c4c5b6e843b0a4 Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 06:58:40 -0400 Subject: [PATCH 4/6] feat(add_relationship): collapse into a single transaction Validate endpoints, resolve class/template scope, and write the two directed rows in one graphdb_mgr:transaction/1 (TOCTOU-isolated). Convert the four single-use phase helpers in place to in-txn (abort-based) form; add private class_of_in_txn/1 (do_class_of/1 keeps its own txn for its public caller); split build_connection_rows into /6 (allocates) + /7 (pure) so the rel-id pair is allocated up-front outside the txn. Spends PR 1's tier-1 primitives. Behaviour-preserving. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- apps/graphdb/src/graphdb_instance.erl | 265 ++++++++++++++------------ 1 file changed, 146 insertions(+), 119 deletions(-) diff --git a/apps/graphdb/src/graphdb_instance.erl b/apps/graphdb/src/graphdb_instance.erl index a34a376..c5174eb 100644 --- a/apps/graphdb/src/graphdb_instance.erl +++ b/apps/graphdb/src/graphdb_instance.erl @@ -1176,101 +1176,91 @@ do_validate_parent(ParentNref, RetAttr) -> %%----------------------------------------------------------------------------- %% do_add_relationship(SourceNref, CharNref, TargetNref, ReciprocalNref, -%% TemplateSpec, State) -> ok | {error, term()} +%% TemplateSpec, AVPSpec, State) -> ok | {error, term()} %% -%% TemplateSpec is either the atom `default` (look up source's class -%% default template) or an integer template nref. Arc validation -%% (existence + arc-label kind + target_kind agreement) runs first, -%% then class lookup, template resolution, scope check, and the -%% two-row write of the connection arcs with the Template AVP stamped -%% on each. +%% Validates endpoints, resolves class membership and template scope, then +%% writes the two directed connection rows -- all in one graphdb_mgr:transaction/1 +%% (TOCTOU-isolated). The rel-id pair is allocated up-front, outside the +%% transaction: get_id_pair is a gen_server call and must never run inside an +%% mnesia fun. A validation abort orphans that pair -- harmless (allocate- +%% outside-transaction doctrine). Phase order: validate endpoints -> +%% resolve classes -> resolve template -> validate scope -> write. %%----------------------------------------------------------------------------- do_add_relationship(SourceNref, CharNref, TargetNref, ReciprocalNref, TemplateSpec, AVPSpec, State) -> - TkAttr = State#state.target_kind_avp_nref, - case validate_arc_endpoints(SourceNref, CharNref, TargetNref, - ReciprocalNref, TkAttr, State#state.retired_nref) of - ok -> - case resolve_arc_classes(SourceNref, TargetNref) of - {ok, SourceClass, TargetClass} -> - case resolve_template(TemplateSpec, SourceClass) of - {ok, TemplateNref} -> - case validate_template_scope(TemplateNref, - SourceClass, TargetClass) of - ok -> - write_connection_arcs(SourceNref, - CharNref, TargetNref, - ReciprocalNref, TemplateNref, - AVPSpec); - {error, _} = Err -> Err - end; - {error, _} = Err -> Err - end; - {error, _} = Err -> Err - end; - {error, _} = Err -> - Err + TkAttr = State#state.target_kind_avp_nref, + RetAttr = State#state.retired_nref, + %% Allocate the rel-id pair up-front, OUTSIDE the transaction: get_id_pair + %% is a gen_server call and must never run inside an mnesia fun. A + %% validation abort below orphans this pair -- harmless (allocate-outside- + %% transaction doctrine). + IdPair = rel_id_server:get_id_pair(), + Txn = fun() -> + ok = validate_arc_endpoints_in_txn(SourceNref, CharNref, TargetNref, + ReciprocalNref, TkAttr, RetAttr), + {SourceClass, TargetClass} = + resolve_arc_classes_in_txn(SourceNref, TargetNref), + TemplateNref = resolve_template_in_txn(TemplateSpec, SourceClass), + ok = validate_template_scope_in_txn(TemplateNref, SourceClass, + TargetClass), + Rows = build_connection_rows(IdPair, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, AVPSpec), + lists:foreach(fun({Tab, Rec}) -> ok = mnesia:write(Tab, Rec, write) end, + Rows) + end, + case graphdb_mgr:transaction(Txn) of + {ok, ok} -> ok; + {error, _} = Err -> Err end. %%----------------------------------------------------------------------------- -%% validate_arc_endpoints(Source, Char, Target, Reciprocal, TkAttr, RetAttr) -> -%% ok | {error, term()} +%% validate_arc_endpoints_in_txn(Source, Char, Target, Reciprocal, TkAttr, +%% RetAttr) -> ok (aborts the enclosing transaction on any violation) %% -%% Arc validation. Reads all four nodes inside one mnesia transaction -%% and rejects: -%% - missing source / target / characterization / reciprocal -%% - any endpoint that is retired (retired => true AVP under RetAttr); -%% reports the first retired nref as {endpoint_retired, Nref} -%% - characterization or reciprocal that is not kind=attribute -%% - target whose kind disagrees with the characterization's -%% `target_kind` AVP (the value stored under attribute=TkAttr) +%% In-transaction endpoint validation. Assumes it runs inside an active mnesia +%% activity; reads the four nodes with bare mnesia:read and signals every +%% violation via mnesia:abort/1 (same Reason terms as the prior own-txn form). %%----------------------------------------------------------------------------- -validate_arc_endpoints(SourceNref, CharNref, TargetNref, ReciprocalNref, +validate_arc_endpoints_in_txn(SourceNref, CharNref, TargetNref, ReciprocalNref, TkAttr, RetAttr) -> - F = fun() -> - Source = mnesia:read(nodes, SourceNref), - Target = mnesia:read(nodes, TargetNref), - Char = mnesia:read(nodes, CharNref), - Recip = mnesia:read(nodes, ReciprocalNref), - case {Source, Target, Char, Recip} of - {[], _, _, _} -> - mnesia:abort({source_not_found, SourceNref}); - {_, [], _, _} -> - mnesia:abort({target_not_found, TargetNref}); - {_, _, [], _} -> - mnesia:abort({characterization_not_found, CharNref}); - {_, _, _, []} -> - mnesia:abort({reciprocal_not_found, ReciprocalNref}); - {[#node{attribute_value_pairs = SAVPs}], - [#node{kind = TKind, attribute_value_pairs = TAVPs}], - [#node{kind = CKind, attribute_value_pairs = CAVPs} = CharNode], - [#node{kind = RKind, attribute_value_pairs = RAVPs}]} -> - case first_retired([{SourceNref, SAVPs}, {TargetNref, TAVPs}, - {CharNref, CAVPs}, {ReciprocalNref, RAVPs}], - RetAttr) of - {retired, RNref} -> - mnesia:abort({endpoint_retired, RNref}); - none -> - case {CKind, RKind} of - {attribute, attribute} -> - case check_target_kind(CharNode, TKind, TkAttr) of - ok -> ok; - {error, Reason} -> mnesia:abort(Reason) - end; - {attribute, _} -> - mnesia:abort({reciprocal_not_an_attribute, - ReciprocalNref, RKind}); - {_, _} -> - mnesia:abort({characterization_not_an_attribute, - CharNref, CKind}) - end - end - end - end, - case graphdb_mgr:transaction(F) of - {ok, ok} -> ok; - {error, _} = Err -> Err + Source = mnesia:read(nodes, SourceNref), + Target = mnesia:read(nodes, TargetNref), + Char = mnesia:read(nodes, CharNref), + Recip = mnesia:read(nodes, ReciprocalNref), + case {Source, Target, Char, Recip} of + {[], _, _, _} -> + mnesia:abort({source_not_found, SourceNref}); + {_, [], _, _} -> + mnesia:abort({target_not_found, TargetNref}); + {_, _, [], _} -> + mnesia:abort({characterization_not_found, CharNref}); + {_, _, _, []} -> + mnesia:abort({reciprocal_not_found, ReciprocalNref}); + {[#node{attribute_value_pairs = SAVPs}], + [#node{kind = TKind, attribute_value_pairs = TAVPs}], + [#node{kind = CKind, attribute_value_pairs = CAVPs} = CharNode], + [#node{kind = RKind, attribute_value_pairs = RAVPs}]} -> + case first_retired([{SourceNref, SAVPs}, {TargetNref, TAVPs}, + {CharNref, CAVPs}, {ReciprocalNref, RAVPs}], + RetAttr) of + {retired, RNref} -> + mnesia:abort({endpoint_retired, RNref}); + none -> + case {CKind, RKind} of + {attribute, attribute} -> + case check_target_kind(CharNode, TKind, TkAttr) of + ok -> ok; + {error, Reason} -> mnesia:abort(Reason) + end; + {attribute, _} -> + mnesia:abort({reciprocal_not_an_attribute, + ReciprocalNref, RKind}); + {_, _} -> + mnesia:abort({characterization_not_an_attribute, + CharNref, CKind}) + end + end end. %% first_retired([{Nref, AVPs}], RetAttr) -> {retired, Nref} | none @@ -1297,56 +1287,63 @@ check_target_kind(#node{attribute_value_pairs = AVPs}, ActualKind, TkAttr) -> %%----------------------------------------------------------------------------- -%% resolve_arc_classes(SourceNref, TargetNref) -> -%% {ok, SourceClass, TargetClass} | {error, term()} +%% resolve_arc_classes_in_txn(SourceNref, TargetNref) -> +%% {SourceClass, TargetClass} (aborts on a missing class) +%% +%% In-transaction class resolution. class_of_in_txn returns only {ok,_} | +%% not_found inside a txn (a read error aborts the txn directly), so the +%% no-class arms abort with the same Reason terms the prior form returned. %%----------------------------------------------------------------------------- -resolve_arc_classes(SourceNref, TargetNref) -> - case do_class_of(SourceNref) of - {ok, SourceClass} -> - case do_class_of(TargetNref) of - {ok, TargetClass} -> {ok, SourceClass, TargetClass}; - not_found -> {error, {target_has_no_class, TargetNref}}; - {error, _} = Err -> Err - end; - not_found -> {error, {source_has_no_class, SourceNref}}; - {error, _} = Err -> Err - end. +resolve_arc_classes_in_txn(SourceNref, TargetNref) -> + SourceClass = case class_of_in_txn(SourceNref) of + {ok, SC} -> SC; + not_found -> mnesia:abort({source_has_no_class, SourceNref}) + end, + TargetClass = case class_of_in_txn(TargetNref) of + {ok, TC} -> TC; + not_found -> mnesia:abort({target_has_no_class, TargetNref}) + end, + {SourceClass, TargetClass}. %%----------------------------------------------------------------------------- -%% resolve_template(TemplateSpec, SourceClass) -> -%% {ok, TemplateNref} | {error, term()} +%% resolve_template_in_txn(TemplateSpec, SourceClass) -> TemplateNref +%% (aborts no_default_template when `default' is requested but absent) %%----------------------------------------------------------------------------- -resolve_template(default, SourceClass) -> - case graphdb_class:default_template(SourceClass) of - {ok, Nref} -> {ok, Nref}; - not_found -> {error, no_default_template}; - {error, _} = Err -> Err +resolve_template_in_txn(default, SourceClass) -> + case graphdb_class:default_template_in_txn(SourceClass) of + {ok, Nref} -> Nref; + not_found -> mnesia:abort(no_default_template) end; -resolve_template(TemplateNref, _SourceClass) when is_integer(TemplateNref) -> - {ok, TemplateNref}. +resolve_template_in_txn(TemplateNref, _SourceClass) + when is_integer(TemplateNref) -> + TemplateNref. %%----------------------------------------------------------------------------- -%% validate_template_scope(TemplateNref, SourceClass, TargetClass) -> -%% ok | {error, term()} +%% validate_template_scope_in_txn(TemplateNref, SourceClass, TargetClass) -> ok +%% (aborts invalid_template / template_class_not_in_ancestry) %% -%% Confirms TemplateNref resolves to a kind=template node whose parent -%% class is in SourceClass's or TargetClass's taxonomic ancestry. +%% Confirms TemplateNref resolves to a kind=template node whose parent class is +%% in SourceClass's or TargetClass's taxonomic ancestry. The nested Reason in +%% {invalid_template, _, Reason} is byte-identical to the gen-server get_template +%% form: get_template_in_txn returns the same {error, not_a_template|not_found}. %%----------------------------------------------------------------------------- -validate_template_scope(TemplateNref, SourceClass, TargetClass) -> - case graphdb_class:get_template(TemplateNref) of +validate_template_scope_in_txn(TemplateNref, SourceClass, TargetClass) -> + case graphdb_class:get_template_in_txn(TemplateNref) of {ok, #node{parents = TmplParents}} -> TmplClass = head_parent(TmplParents), - InSource = graphdb_class:class_in_ancestry(TmplClass, SourceClass), - InTarget = graphdb_class:class_in_ancestry(TmplClass, TargetClass), + InSource = graphdb_class:class_in_ancestry_in_txn(TmplClass, + SourceClass), + InTarget = graphdb_class:class_in_ancestry_in_txn(TmplClass, + TargetClass), case InSource orelse InTarget of true -> ok; - false -> {error, {template_class_not_in_ancestry, - TemplateNref, TmplClass, SourceClass, TargetClass}} + false -> mnesia:abort({template_class_not_in_ancestry, + TemplateNref, TmplClass, SourceClass, TargetClass}) end; {error, Reason} -> - {error, {invalid_template, TemplateNref, Reason}} + mnesia:abort({invalid_template, TemplateNref, Reason}) end. @@ -1360,8 +1357,19 @@ validate_template_scope(TemplateNref, SourceClass, TargetClass) -> %% composition root txn; auto connections are written post-commit). %%----------------------------------------------------------------------------- build_connection_rows(SourceNref, CharNref, TargetNref, ReciprocalNref, - TemplateNref, {FwdAVPs, RevAVPs}) -> - {Id1, Id2} = rel_id_server:get_id_pair(), + TemplateNref, AVPSpec) -> + IdPair = rel_id_server:get_id_pair(), + build_connection_rows(IdPair, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, AVPSpec). + +%% build_connection_rows({Id1, Id2}, S, C, T, R, TemplateNref, {FwdAVPs,RevAVPs}) +%% -> [{relationships, #relationship{}}] +%% +%% Pure builder: no allocation. The caller supplies the rel-id pair (allocated +%% up-front, outside any transaction) so the rows can be built inside a caller's +%% transaction. Template AVP rides index 0 of each direction. +build_connection_rows({Id1, Id2}, SourceNref, CharNref, TargetNref, + ReciprocalNref, TemplateNref, {FwdAVPs, RevAVPs}) -> TemplateAVP = #{attribute => ?ARC_TEMPLATE, value => TemplateNref}, Fwd = #relationship{ id = Id1, kind = connection, @@ -1496,6 +1504,25 @@ do_class_of(InstanceNref) -> end. +%%----------------------------------------------------------------------------- +%% class_of_in_txn(InstanceNref) -> {ok, ClassNref} | not_found +%% +%% Tier-1 in-transaction twin of do_class_of/1. Assumes it runs inside an +%% active mnesia activity; uses a bare index_read. do_class_of/1 keeps its +%% own transaction for its public class_of caller. +%%----------------------------------------------------------------------------- +class_of_in_txn(InstanceNref) -> + Rels = mnesia:index_read(relationships, InstanceNref, + #relationship.source_nref), + case lists:search( + fun(R) -> + R#relationship.characterization =:= ?ARC_INST_TO_CLASS + end, Rels) of + {value, #relationship{target_nref = ClassNref}} -> {ok, ClassNref}; + false -> not_found + end. + + %%----------------------------------------------------------------------------- %% do_get_instance(Nref) -> %% {ok, #node{}} | {error, not_found | not_an_instance | term()} From 3846cf44ee1252ee595046d567cb541a0d6e30bb Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 07:04:00 -0400 Subject: [PATCH 5/6] docs(add_relationship): record atomic collapse + deferred convergence Mark the Atomic add_relationship follow-up implemented; add the deferred default-template name-search convergence task; note add_relationship's single-transaction behaviour in the graphdb worker doc. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- TASKS.md | 30 +++++++++++++++++++++--------- apps/graphdb/CLAUDE.md | 2 +- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/TASKS.md b/TASKS.md index 3750d23..613354f 100644 --- a/TASKS.md +++ b/TASKS.md @@ -133,16 +133,28 @@ Tracked follow-ups (not in the seam spec): instance CT cases (`characterization_not_found`/`reciprocal_not_found` arms). Design `docs/designs/transaction-seam-retrofit-design.md`; plan `docs/superpowers/plans/2026-06-20-transaction-seam-retrofit.md`. -- **Atomic `add_relationship`** — collapse its four separate transactions - (validate → resolve classes → resolve template → write) into one. The - prerequisite tier-1 `graphdb_class` read primitives - (`get_template_in_txn/1`, `class_in_ancestry_in_txn/2`, - `default_template_in_txn/1`) have landed (PR 1, - `docs/designs/atomic-add-relationship-primitives-design.md`). PR 2 swaps - `add_relationship` onto them, converts the `source_has_no_class` / - `target_has_no_class` arms to `mnesia:abort/1`, and allocates the rel-id pair - up-front. Sequence with / before `mutate/1`, which wants those primitives too. +- **Atomic `add_relationship`** — IMPLEMENTED. `do_add_relationship/7`'s five + separate transactions (validate endpoints → resolve classes → resolve + template → validate scope → write) are collapsed into one + `graphdb_mgr:transaction/1` (TOCTOU isolation). The four single-use phase + helpers were converted in place to in-txn (abort-based) form; a private + `class_of_in_txn/1` was added (`do_class_of/1` keeps its own txn for its + public caller); `build_connection_rows` was split into `/6` (allocates) + + `/7` (pure) so the rel-id pair is allocated up-front outside the + transaction. Behaviour-preserving; existing `add_relationship` suite + unchanged, +2 new instance CT cases (`source_has_no_class` / + `target_has_no_class`). Design + `docs/designs/atomic-add-relationship-design.md`; plan + `docs/superpowers/plans/2026-06-21-atomic-add-relationship.md`. - **Batch `mutate([Mutation])`** — the tier-3 entry point. +- **Converge default-template name search** — `graphdb_class` carries two + copies of the default-template name-search walk: the gen-server + `do_find_template_by_name/2` (own txn) and the tier-1 + `default_template_in_txn/1` (PR 1). The gen-server `default_template/1` path + is already transactional, so `do_default_template/1` could be rewritten to + wrap a txn and call `default_template_in_txn/1`, removing the duplication. + Deliberately deferred (the duplication is sanctioned project precedent); + a future cleanup, not blocking anything. ### Node deletion (slice A) — IMPLEMENTED diff --git a/apps/graphdb/CLAUDE.md b/apps/graphdb/CLAUDE.md index eb14d77..94653c5 100644 --- a/apps/graphdb/CLAUDE.md +++ b/apps/graphdb/CLAUDE.md @@ -260,7 +260,7 @@ Manages the "is a" hierarchy of class nodes in the ontology. Creates and manages instance nodes in the project (instance space). - `create_instance/3,4,5` (name, class_nref, compositional_parent_nref [, connection_resolver [, conflict_resolver]]) — atomically writes the node record AND the instance→class membership relationship pair (arc labels nref=29 and nref=30), then fires composition rules (F4 B2). Returns `{ok, Nref, Report}` on success or `{error, Reason, Report}` on rule-firing failure; pre-plan validation errors (unknown class, non-instantiable class, etc.) return `{error, Reason}` (2-tuple). Rejects a class marked non-instantiable with `{error, {class_not_instantiable, ClassNref}}` (L9). Propose-mode composition rules surface as `proposed` outcomes in the report (B3); nothing is materialised for them. `/4` threads a connection **resolver** (`fun((ConnContext) -> {connect, [Target]} | defer end`): the RESOLVE step fires effective ConnectionRules (F4 B4) — `mandatory` connections to existing targets land in the root transaction, `auto` post-commit, `defer`/`propose` are reported only; targets are validated (exists, instance, instance-of target_class-or-subclass). `/3` uses the built-in `report_only` (defer-all) connection resolver, so connection rules surface as `required`/`not_connected`/`proposed` outcomes and nothing is connected. `/5` threads a B5 **conflict resolver** (`fun((#{kind, rules, class_nref}) -> [Pair])`); `/3` and `/4` inject the built-in `graphdb_rules:default_conflict_resolver/0`, which shadows conflicting inherited rules (nearest-level winner by mode priority), merges multiplicity (nearest Min, greatest Max), and demotes both-real-template losers to `propose` (F4 B5). -- `add_relationship/4` (source_nref, characterization_nref, target_nref, reciprocal_nref) — writes two directed rows atomically; IDs allocated via `get_nref()` +- `add_relationship/4,5,6` (source_nref, characterization_nref, target_nref, reciprocal_nref [, template_nref [, {FwdAVPs, RevAVPs}]]) — validates endpoints, resolves source/target class and template scope, and writes the two directed `kind=connection` rows in a **single** `graphdb_mgr:transaction/1` (TOCTOU-isolated). The rel-id pair is allocated up-front (outside the transaction) via `rel_id_server:get_id_pair/0`. `/4` uses the source class's default template; `/5` takes an explicit template nref; `/6` adds per-direction AVPs. - `add_class_membership/2` (instance_nref, class_nref) — adds a membership arc pair; also rejects a non-instantiable class target with `{error, {class_not_instantiable, ClassNref}}` (L9) - `get_instance/1`, `children/1`, `compositional_ancestors/1`, `resolve_value/2` From 16ef16f5ec82dd8ca667cbb15519cd3340c85c32 Mon Sep 17 00:00:00 2001 From: "David W. Thomas" Date: Sun, 21 Jun 2026 07:08:46 -0400 Subject: [PATCH 6/6] docs(add_relationship): tighten convergence-bullet wording Final-review nit: do_default_template/1 (not default_template/1) is what already wraps its own transaction. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EWukKCbrN8GybaScJGU2kF --- TASKS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/TASKS.md b/TASKS.md index 613354f..803165f 100644 --- a/TASKS.md +++ b/TASKS.md @@ -150,9 +150,9 @@ Tracked follow-ups (not in the seam spec): - **Converge default-template name search** — `graphdb_class` carries two copies of the default-template name-search walk: the gen-server `do_find_template_by_name/2` (own txn) and the tier-1 - `default_template_in_txn/1` (PR 1). The gen-server `default_template/1` path - is already transactional, so `do_default_template/1` could be rewritten to - wrap a txn and call `default_template_in_txn/1`, removing the duplication. + `default_template_in_txn/1` (PR 1). `do_default_template/1` already wraps its + own transaction, so it could be rewritten to call `default_template_in_txn/1` + inside that txn, removing the duplication. Deliberately deferred (the duplication is sanctioned project precedent); a future cleanup, not blocking anything.