Skip to content

Fix rel_id_server counter seeding: three defects, one path #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
david-w-t wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix rel_id_server counter seeding: three defects, one path #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 35 additions & 13 deletions TASKS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
111 changes: 86 additions & 25 deletions apps/graphdb/src/rel_id_server.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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]),
Expand All @@ -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.


Expand All @@ -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.

Expand All @@ -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}.
128 changes: 126 additions & 2 deletions apps/graphdb/test/rel_id_server_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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], [
Expand All @@ -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
]}].


Expand All @@ -82,26 +92,74 @@ 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])),
TmpDir = filename:join([OrigCwd, "_build", "test", "ct_scratch",
?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),
delete_dir_recursive(TmpDir),
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,
Expand Down Expand Up @@ -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().
Loading
Loading