diff --git a/TASKS.md b/TASKS.md index 9d68746..fbfee0f 100644 --- a/TASKS.md +++ b/TASKS.md @@ -570,19 +570,41 @@ still open: backfilling `target_kind` onto bootstrap arc labels 21–30, which would let `graphdb_instance:check_target_kind/3` drop its permissive legacy arm. -**Open defect (Important, pre-existing, unrelated to SP2) — -`rel_id_server:seed_from_mnesia/0` calls a nonexistent function.** -`rel_id_server.erl:207` calls `mnesia:dirty_foldl/3`, which does not exist -in OTP 28 (should be `mnesia:foldl/3`). Invisible to the compiler; caught -by the `xref` gate added alongside SP2 and currently suppressed by a -single-MFA `xref_ignores` entry in `rebar.config` — **remove that entry -when this is fixed.** `initialize/0` calls it only when the DETS `counter` -key is absent, so a normal restart with an intact DETS file never hits it. -The real failure mode: if the DETS file is lost while the Mnesia -`relationships` table survives (restore, data-dir move, partial recovery), -the counter restarts at 1 and hands out ids colliding with existing primary -keys, and `mnesia:write` then **silently overwrites existing relationship -rows**. +**Defect (Important, pre-existing, unrelated to SP2) — +`rel_id_server` counter seeding — IMPLEMENTED (2026-08-09).** Filed as a +one-line typo; it was three defects stacked in the same path, and the typo +was the least of them. + +1. `seed_from_mnesia/0` called `mnesia:dirty_foldl/3`, which does not exist + in OTP 28 (the real function is `mnesia:foldl/3`, and it must run inside + a transaction). Invisible to the compiler. +2. Its blanket `catch _:_ -> 1` swallowed the resulting `undef` and + returned **1** — precisely the corrupting value, so the failure was + silent by construction. Now 1 is returned only for a definite + `{no_exists, relationships}`; anything else logs and exits + `rel_id_server_seed`. A "cannot determine the high-water mark" answer + must never default to the value that collides. +3. **The ordering defect, which the filed report missed and which the other + two fixes do not close.** Seeding ran from `init/1`, where the table is + structurally unreadable: `rel_id_server` must start *before* + `graphdb_mgr` (`graphdb_bootstrap` consumes ids from `get_id_pair/0`), + but the `relationships` table is not created — and mnesia is not even + started — until `graphdb_bootstrap:ensure_mnesia/0` runs *inside* + `graphdb_mgr:init/1`. An eager seed therefore always read "no rows" and + landed on 1 regardless. Seeding is now **lazy**, on the first + `get_id`/`get_id_pair` call, by which point bootstrap has run and the + table is loaded. + +Failure mode closed: DETS file lost while the Mnesia `relationships` table +survives (restore, data-dir move, partial recovery) — the counter no longer +restarts at 1 handing out ids that collide with live primary keys, which +`mnesia:write` would then **silently overwrite**. + +The `xref_ignores` entry in `rebar.config` that suppressed this is +**removed**; the gate now runs with an empty ignore list. Regression +coverage in `rel_id_server_SUITE` group `seeding` (4 cases); the suite also +now starts mnesia per case, since starting the server with no mnesia at all +— as it used to — is not a state the system can be in. **Open design question — `TheKnowledgeNetwork.md` §3 says identity is uniform; SP2 makes that false at the physical layer.** The canonical spec diff --git a/apps/graphdb/src/rel_id_server.erl b/apps/graphdb/src/rel_id_server.erl index 126c810..dfb6ead 100644 --- a/apps/graphdb/src/rel_id_server.erl +++ b/apps/graphdb/src/rel_id_server.erl @@ -17,6 +17,20 @@ %% Rev A Date: 2026-05-19 Author: David W. Thomas %% %%--------------------------------------------------------------------- +%% Rev PA2 Date: 2026-08-09 Author: David W. Thomas +%% Counter seeding repaired -- three defects, all in the same path: +%% 1. seed_from_mnesia/0 called mnesia:dirty_foldl/3, which does not +%% exist. Now mnesia:foldl/3 inside a transaction. +%% 2. Its blanket `catch _:_ -> 1' turned the resulting undef -- and +%% every other failure -- into a seed of 1, the one value that +%% collides with live rows and makes mnesia:write silently overwrite +%% them. Now 1 is returned only for a definite "table does not +%% exist"; anything else logs and exits. +%% 3. Seeding ran from init/1, where the relationships table cannot be +%% read yet (see counter/0), so it could never observe existing rows +%% no matter how (1) and (2) were fixed. Now seeded lazily on first +%% id request. +%%--------------------------------------------------------------------- -module(rel_id_server). -behaviour(gen_server). @@ -109,7 +123,8 @@ get_id_pair() -> %%----------------------------------------------------------------------------- %% init([]) -> {ok, State} %% -%% Opens the DETS file for this rel_id_server instance. +%% Opens the DETS file for this rel_id_server instance. Does NOT seed the +%% counter -- see counter/0 for why that cannot happen here. %%----------------------------------------------------------------------------- init([]) -> open("rel_id_server.dets"), @@ -173,10 +188,6 @@ code_change(_OldVsn, State, _Extra) -> open(File) -> case dets:open_file(?MODULE, [{file, File}]) of {ok, ?MODULE} -> - case dets:member(?MODULE, counter) of - false -> initialize(); - true -> void - end, true; {error, Reason} -> logger:error("cannot open rel_id_server dets table: ~p", [Reason]), @@ -185,32 +196,82 @@ open(File) -> %%----------------------------------------------------------------------------- -%% initialize() -> ok +%% counter() -> integer() +%% +%% Returns the current counter value, seeding it on first use. +%% +%% Seeding is deliberately LAZY -- it cannot be done in init/1. Startup +%% ordering makes the table unreadable at that point, and circularly so: %% -%% Seeds the DETS counter from the maximum existing relationship ID in Mnesia, -%% or 1 if Mnesia is unavailable or the relationships table is empty. +%% * rel_id_server must start BEFORE graphdb_mgr, because +%% graphdb_bootstrap consumes ids from get_id_pair/0 while loading the +%% scaffold; and +%% * the relationships table does not exist until graphdb_bootstrap +%% creates it, which happens inside graphdb_mgr:init/1 -- and mnesia +%% itself is not even running until graphdb_bootstrap:ensure_mnesia/0 +%% starts it there. +%% +%% So at init/1 the answer is never knowable: an eager seed always reads +%% "no rows" and lands on 1, which is exactly the value that collides with +%% live rows when the DETS file was lost but Mnesia survived. By first +%% get_id/get_id_pair call, bootstrap has run, mnesia is up, and the table +%% is loaded -- so the high-water mark is real. %%----------------------------------------------------------------------------- -initialize() -> - StartId = seed_from_mnesia(), - dets:insert(?MODULE, {counter, StartId}), - ok. +counter() -> + case dets:lookup(?MODULE, counter) of + [{counter, N}] -> + N; + [] -> + Seed = seed_from_mnesia(), + ok = dets:insert(?MODULE, {counter, Seed}), + Seed + end. %%----------------------------------------------------------------------------- -%% seed_from_mnesia() -> integer() +%% seed_from_mnesia() -> integer() | exit(rel_id_server_seed) +%% +%% Returns the id the counter should start at: one past the highest id +%% already present in the Mnesia relationships table. +%% +%% Called only from counter/0, i.e. once, on the first id request after +%% the DETS counter key is found absent. Two states reach it: +%% +%% * Genuine first boot. graphdb_bootstrap has created the relationships +%% table and is loading the scaffold, so the table exists and is empty; +%% the fold returns 0 and the counter starts at 1. (A caller reaching +%% here before the table exists reads {no_exists, ...} and also gets 1, +%% which is equally correct -- no table means no rows.) +%% +%% * DETS file lost while Mnesia survived -- restore, data-dir move, or +%% partial recovery. The table is loaded and populated, and the +%% counter MUST resume above its highest id. Starting at 1 here hands +%% out ids that collide with live primary keys, and mnesia:write then +%% SILENTLY OVERWRITES existing rows. %% -%% Scans the Mnesia relationships table for the maximum existing ID. -%% Returns max(1, Max + 1) on success, 1 if Mnesia is unavailable. +%% Because that second case is silent data loss, any outcome that is not a +%% definite answer is fatal rather than defaulted. In particular a blanket +%% `catch _:_ -> 1' is not safe here -- 1 is precisely the corrupting +%% value, so swallowing an error produces the worst possible guess. %%----------------------------------------------------------------------------- seed_from_mnesia() -> - try - Max = mnesia:dirty_foldl( - fun(Rec, Acc) -> max(element(2, Rec), Acc) end, - 0, - relationships), - max(1, Max + 1) - catch - _:_ -> 1 + %% element(2, Rec) is #relationship.id, the table's primary key; this + %% module deliberately carries no #relationship{} copy of its own. + Fold = fun(Rec, Acc) -> max(element(2, Rec), Acc) end, + Read = fun() -> mnesia:foldl(Fold, 0, relationships) end, + case mnesia:transaction(Read) of + {atomic, Max} when is_integer(Max) -> + max(1, Max + 1); + {aborted, {no_exists, relationships}} -> + 1; + Other -> + logger:error( + "rel_id_server: cannot determine the highest existing " + "relationship id (~p) -- refusing to seed the counter, " + "because restarting at 1 would hand out ids that collide " + "with live rows and silently overwrite them", + [Other]), + exit(rel_id_server_seed) end. @@ -220,7 +281,7 @@ seed_from_mnesia() -> %% Reads the current counter, increments it in DETS, returns the old value. %%----------------------------------------------------------------------------- do_get_id() -> - [{counter, N}] = dets:lookup(?MODULE, counter), + N = counter(), ok = dets:insert(?MODULE, {counter, N + 1}), N. @@ -231,6 +292,6 @@ do_get_id() -> %% Allocates two consecutive IDs atomically for a reciprocal arc pair. %%----------------------------------------------------------------------------- do_get_id_pair() -> - [{counter, N}] = dets:lookup(?MODULE, counter), + N = counter(), ok = dets:insert(?MODULE, {counter, N + 2}), {N, N + 1}. diff --git a/apps/graphdb/test/rel_id_server_SUITE.erl b/apps/graphdb/test/rel_id_server_SUITE.erl index 23f8176..948cab5 100644 --- a/apps/graphdb/test/rel_id_server_SUITE.erl +++ b/apps/graphdb/test/rel_id_server_SUITE.erl @@ -48,13 +48,17 @@ persists_counter_across_restart/1, get_id_pair_returns_integers/1, get_id_pair_are_consecutive/1, - get_id_pair_no_overlap_with_get_id/1 + get_id_pair_no_overlap_with_get_id/1, + first_boot_seeds_at_one/1, + seed_is_deferred_until_first_use/1, + seeds_above_existing_relationship_ids/1, + refuses_to_seed_when_max_id_undeterminable/1 ]). suite() -> [{timetrap, {seconds, 30}}]. all() -> - [{group, counter}]. + [{group, counter}, {group, seeding}]. groups() -> [{counter, [sequence], [ @@ -65,6 +69,12 @@ groups() -> get_id_pair_returns_integers, get_id_pair_are_consecutive, get_id_pair_no_overlap_with_get_id + ]}, + {seeding, [sequence], [ + first_boot_seeds_at_one, + seed_is_deferred_until_first_use, + seeds_above_existing_relationship_ids, + refuses_to_seed_when_max_id_undeterminable ]}]. @@ -82,6 +92,12 @@ end_per_suite(_Config) -> %%--------------------------------------------------------------------- %% Per-testcase setup/teardown %%--------------------------------------------------------------------- +%% Mnesia is started (schema-less, so ram-only) before rel_id_server for +%% every case. The real deployment always has mnesia up -- it is an +%% application dependency of graphdb -- and seed_from_mnesia/0 now treats +%% an unreachable mnesia as fatal rather than defaulting the counter to 1. +%% Starting the server with no mnesia at all, as this suite used to, is +%% not a state the system can actually be in. init_per_testcase(_TC, Config) -> OrigCwd = proplists:get_value(orig_cwd, Config), Unique = integer_to_list(erlang:unique_integer([positive, monotonic])), @@ -89,12 +105,14 @@ init_per_testcase(_TC, Config) -> ?DIR_PREFIX ++ Unique]), ok = filelib:ensure_dir(filename:join(TmpDir, "x")), ok = file:set_cwd(TmpDir), + ok = mnesia:start(), {ok, _} = rel_id_server:start_link(), [{tmp_dir, TmpDir} | Config]. end_per_testcase(_TC, Config) -> catch gen_server:stop(rel_id_server), catch dets:close(rel_id_server), + stop_mnesia(), OrigCwd = proplists:get_value(orig_cwd, Config), ok = file:set_cwd(OrigCwd), TmpDir = proplists:get_value(tmp_dir, Config), @@ -102,6 +120,46 @@ end_per_testcase(_TC, Config) -> ok. +%%--------------------------------------------------------------------- +%% Helpers +%%--------------------------------------------------------------------- + +%% mnesia:stop/0 is asynchronous; wait for it so the next case starts +%% from a known state and the temp dir can be removed safely. +stop_mnesia() -> + mnesia:stop(), + wait_until(fun() -> mnesia:system_info(is_running) =:= no end, 50). + +wait_until(_Pred, 0) -> + ct:fail(timeout_waiting_for_condition); +wait_until(Pred, N) -> + case Pred() of + true -> ok; + false -> timer:sleep(20), wait_until(Pred, N - 1) + end. + +%% Stop the server and delete its DETS file, so the next start_link/0 has +%% no counter key and must re-run seed_from_mnesia/0. +wipe_counter(Config) -> + catch gen_server:stop(rel_id_server), + catch dets:close(rel_id_server), + TmpDir = proplists:get_value(tmp_dir, Config), + ok = file:delete(filename:join(TmpDir, "rel_id_server.dets")). + +%% The subset of the real relationships table this suite needs: same +%% record name and attribute order, ram_copies so nothing touches disk. +create_relationships_table() -> + {atomic, ok} = mnesia:create_table(relationships, + [{record_name, relationship}, + {attributes, [id, kind, source_nref, characterization, + target_nref, reciprocal, avps]}]), + ok. + +write_relationship(Id) -> + ok = mnesia:dirty_write(relationships, + {relationship, Id, connection, 1, 21, 2, 22, []}). + + delete_dir_recursive(Dir) -> IsAbsolute = filename:pathtype(Dir) =:= absolute, HasScratch = string:find(Dir, ?SCRATCH_SENTINEL) =/= nomatch, @@ -167,3 +225,69 @@ get_id_pair_no_overlap_with_get_id(_Config) -> {_A, B} = rel_id_server:get_id_pair(), Next = rel_id_server:get_id(), ?assertEqual(B + 1, Next). + + +%%===================================================================== +%% Counter Seeding Tests +%% +%% seed_from_mnesia/0 runs only when the DETS counter key is absent. +%% These cases cover its three outcomes. +%%===================================================================== + +%% Genuine first boot: rel_id_server starts before graphdb_mgr, so the +%% relationships table does not exist yet. No rows can exist, so 1 is the +%% correct seed. +first_boot_seeds_at_one(Config) -> + %% Assert absence via system_info/1, not table_info(_, size): the + %% latter returns 0 for a table that does not exist, which is + %% indistinguishable from one that exists and is empty. + ?assertNot(lists:member(relationships, mnesia:system_info(tables))), + wipe_counter(Config), + {ok, _} = rel_id_server:start_link(), + ?assertEqual(1, rel_id_server:get_id()). + +%% The counter is seeded on first use, not in init/1. It has to be: under +%% graphdb_sup, rel_id_server starts before graphdb_mgr, and it is +%% graphdb_mgr:init/1 that runs graphdb_bootstrap -- which starts mnesia +%% and creates the relationships table. At init/1 there is nothing to read. +seed_is_deferred_until_first_use(Config) -> + wipe_counter(Config), + {ok, _} = rel_id_server:start_link(), + ?assertEqual([], dets:lookup(rel_id_server, counter)), + _ = rel_id_server:get_id(), + ?assertMatch([{counter, _}], dets:lookup(rel_id_server, counter)). + +%% Regression: DETS lost while Mnesia survived (restore, data-dir move, +%% partial recovery). The counter MUST resume above the highest existing +%% id -- seeding at 1 would hand out ids that collide with live primary +%% keys, and mnesia:write would then silently overwrite those rows. +%% +%% Staged in the real startup order: the server starts while the +%% relationships table still does not exist, the table then appears +%% already populated, and only then is an id requested. That ordering is +%% the point -- it is why seeding cannot happen in init/1. +seeds_above_existing_relationship_ids(Config) -> + wipe_counter(Config), + {ok, _} = rel_id_server:start_link(), + ok = create_relationships_table(), + %% Written out of order; 4242 is the high-water mark, not the last write. + lists:foreach(fun write_relationship/1, [7, 4242, 41]), + FirstId = rel_id_server:get_id(), + ?assertEqual(4243, FirstId), + %% The point of the assertion above: no id collides with a live row. + ?assertEqual([], [I || I <- [FirstId, rel_id_server:get_id()], + mnesia:dirty_read(relationships, I) =/= []]). + +%% If the highest existing id cannot be determined, the server must fail +%% loudly rather than default the counter to 1 -- 1 is precisely the value +%% that corrupts, so a silent fallback is the worst available guess. +refuses_to_seed_when_max_id_undeterminable(Config) -> + wipe_counter(Config), + stop_mnesia(), + {ok, _} = rel_id_server:start_link(), + process_flag(trap_exit, true), + ?assertExit({rel_id_server_seed, _}, rel_id_server:get_id()), + receive {'EXIT', _Pid, rel_id_server_seed} -> ok after 100 -> ok end, + process_flag(trap_exit, false), + %% Restore mnesia so end_per_testcase tears down from a known state. + ok = mnesia:start(). diff --git a/docs/Architecture.md b/docs/Architecture.md index a91bdef..327e9f6 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -494,6 +494,25 @@ spillover path (raise floor and continue via get_nref) is not yet wired to the loader. With `?NREF_START = 1 000 000` and `?LABEL_START = 10 001` the permanent tier has roughly 990 000 free slots — spill-over is not expected. +### Environment arc-row IDs (`rel_id_server`) + +Relationship row IDs come from `rel_id_server`, a DETS-backed counter kept +deliberately separate from the nref space so arc rows do not consume +graph-visible integers. Its counter is seeded **lazily, on the first id +request** — not in `init/1`. That is forced by startup ordering, and the +constraint is circular: `rel_id_server` must start before `graphdb_mgr` +(because `graphdb_bootstrap` draws ids from it while loading the scaffold), +yet the `relationships` table does not exist — and mnesia is not running — +until `graphdb_bootstrap:ensure_mnesia/0` runs inside `graphdb_mgr:init/1`. +Seeding at `init/1` could therefore never observe existing rows. + +The seed is one past the highest existing id, read via `mnesia:foldl/3` in a +transaction. A seed of 1 is used only for a definite "table does not exist"; +any other unreadable outcome exits `rel_id_server_seed` rather than +defaulting, because 1 is exactly the value that collides with live primary +keys when the DETS file was lost but Mnesia survived — and `mnesia:write` +would then silently overwrite those rows. + ### Project allocators Per-project; start at **1**; no bootstrap floor. Implemented (SP2) as an diff --git a/rebar.config b/rebar.config index 4b96745..a88028f 100644 --- a/rebar.config +++ b/rebar.config @@ -24,15 +24,11 @@ %% output. {xref_checks, [undefined_function_calls]}. -%% rel_id_server:seed_from_mnesia/0 calls mnesia:dirty_foldl/3, which does -%% not exist (real function is mnesia:foldl/3). Pre-existing, unrelated to -%% Fixes 1/2, out of scope for this wave -- see wave-a report. Silently -%% caught by seed_from_mnesia/0's own `catch _:_ -> 1`, so today this is a -%% silent-fallback bug (rel_id_server always reseeds at 1 on restart -%% instead of the true max), not a crash. Ignored here by exact MFA so the -%% gate stays strict for everything else; do not widen this list without -%% the same scrutiny. -{xref_ignores, [{rel_id_server, seed_from_mnesia, 0}]}. +%% No xref_ignores. The list previously held one entry -- +%% {rel_id_server, seed_from_mnesia, 0}, for its mnesia:dirty_foldl/3 call +%% -- which was fixed rather than suppressed; an empty gate is the point. +%% Do not add an entry without the same scrutiny the check itself got: +%% suppressing an undefined_function_calls hit hides a runtime `undef'. {relx, [ {release, {seerstone, "0.1.0"},