fix(storage_engine): never reissue revisions across restarts so a stale expect_revision is refused - #390

🔧 Fix applied.
5 issues (4 warnings, 1 info) still open:

• ⚠️ src/storage_engine/revisions.rs:101 - A restore from backup can still reissue revisions, so a bare stale expect_revision can pass on a different history. The bound is read only from the data directory, and a restored directory carries an older bound or none. (1) Offline backup copies metadata/revisions.json as it was at backup time (src/storage/backup/tree.rs:27 skips only LOCK and the manifest). Sequence: back up at bound 2^20; the engine restarts and issues 2^20+1 onwards; a client observes 2^20+5; restore the backup; the restored engine continues at 2^20 and reissues 2^20+5 for a rolled-back history. (2) The online export (src/storage/backup/export.rs:141-143) writes persist/ but no revisions.json, so every restored export starts at UNRECORDED_FLOOR (2^40). Two restores of an export, or a restore of a directory that was itself upgraded from an earlier version, collide from 2^40+1. This path was equally open before the change and the issue names crash/restart only, so it is not a regression. The remedy needs authorization rather than the defect: closing it means extending the change to the backup/restore path (for example, excluding or raising the bound on restore, or requiring expect_epoch after one).
• ⚠️ src/storage_engine/revisions.rs:88 - When the durable write of the bound fails, the revision is issued anyway. reserve_through logs revision_reservation_failed and next() still returns a revision above the bound on disk. Sequence: save_json_atomic fails (ENOSPC, EIO, or a permission error on metadata/) when the counter crosses a block; commits that still succeed return revisions above the recorded bound to clients; the engine crashes; the restarted engine continues at the old bound and reissues those revisions. That is the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 failure again, limited to the window in which the write is failing. Each revision in that window also retries the temp-file write and fsync under the global mutex. The remedy needs authorization because it changes behavior: next_revision() is infallible and is called from snapshot construction, so failing closed means refusing the commit or aborting the process.
• ⚠️ src/storage_engine/revisions.rs:82 - Unrequired component: the branch in reserve_through that drops a reservation when its metadata/ directory no longer exists (commented "A removed data directory (a test's)"). No intent requirement needs it. It exists so that leaked test engines do not recreate deleted temp directories. In production it fails open: an open engine whose metadata/ is missing at that instant is removed from the list for good, Counter.reserved returns to u64::MAX, and its bound is never raised again without any log line. Recommend removing the branch. Dead engines are already pruned through the Weak upgrade.
• ⚠️ src/storage_engine/residency.rs:406 - Unrequired component, added by fix round 1: the reserve_ahead()? gate in activate(). activate is reached through kg_handle, which every read uses, so this gate refuses reads as well as writes. Sequence: the disk fills (or metadata/ becomes unwritable) while the counter is within HEADROOM of the bound; a client queries a dormant knowledge graph; kg_handle -> activate -> reserve_ahead returns the write error and the query fails. Before this round that read succeeded. The operator's decision asked only that "the commit that needs the new block must fail"; the gates in lock_kg (residency.rs:339) and create_graph (mod.rs:405) already cover every commit. Without the gate an activation still cannot issue a revision above the bound, because next_revision() panics rather than return one. Recommend removing the gate in activate. This needs a decision because it trades read availability during a storage fault against an activation failing by panic in the far corner where 2^19 revisions are used with no write in between.
• ℹ️ src/storage_engine/revisions.rs:86 - One revision-consuming path does not pass reserve_ahead, so the claim on next_revision that "Writes are refused long before that" (revisions.rs:58-59) does not hold for it. Fix round 1 gated lock_kg, activate and create_graph and left this sibling behind. WriteProgram::view (src/storage_engine/write_program.rs:254) builds a new snapshot, and so takes a revision, for each fact statement that reads state after a rule change. It is called while staging (src/protocol/handler/write_run.rs:290), before the commit reaches lock_kg; staging_base (src/storage_engine/mod.rs:834) uses kg_handle only. Sequence: the bound cannot be written and the counter is in the headroom zone; clients retry programs that change a rule and then read; each retry consumes revisions and its commit is refused; after 2^19 such revisions the next staging view panics in the handler task instead of returning an error. No revision above the bound is returned, so the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 invariant holds; the cost is a panic where an error was intended. Smallest fix: call revisions::reserve_ahead()? in staging_base, which only write programs use.

🔧 Fix applied.
5 issues (4 warnings, 1 info) still open:

• ⚠️ src/storage_engine/revisions.rs:101 - A restore from backup can still reissue revisions, so a bare stale expect_revision can pass on a different history. The bound is read only from the data directory, and a restored directory carries an older bound or none. (1) Offline backup copies metadata/revisions.json as it was at backup time (src/storage/backup/tree.rs:27 skips only LOCK and the manifest). Sequence: back up at bound 2^20; the engine restarts and issues 2^20+1 onwards; a client observes 2^20+5; restore the backup; the restored engine continues at 2^20 and reissues 2^20+5 for a rolled-back history. (2) The online export (src/storage/backup/export.rs:141-143) writes persist/ but no revisions.json, so every restored export starts at UNRECORDED_FLOOR (2^40). Two restores of an export, or a restore of a directory that was itself upgraded from an earlier version, collide from 2^40+1. This path was equally open before the change and the issue names crash/restart only, so it is not a regression. The remedy needs authorization rather than the defect: closing it means extending the change to the backup/restore path (for example, excluding or raising the bound on restore, or requiring expect_epoch after one).
• ⚠️ src/storage_engine/revisions.rs:88 - When the durable write of the bound fails, the revision is issued anyway. reserve_through logs revision_reservation_failed and next() still returns a revision above the bound on disk. Sequence: save_json_atomic fails (ENOSPC, EIO, or a permission error on metadata/) when the counter crosses a block; commits that still succeed return revisions above the recorded bound to clients; the engine crashes; the restarted engine continues at the old bound and reissues those revisions. That is the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 failure again, limited to the window in which the write is failing. Each revision in that window also retries the temp-file write and fsync under the global mutex. The remedy needs authorization because it changes behavior: next_revision() is infallible and is called from snapshot construction, so failing closed means refusing the commit or aborting the process.
• ⚠️ src/storage_engine/revisions.rs:82 - Unrequired component: the branch in reserve_through that drops a reservation when its metadata/ directory no longer exists (commented "A removed data directory (a test's)"). No intent requirement needs it. It exists so that leaked test engines do not recreate deleted temp directories. In production it fails open: an open engine whose metadata/ is missing at that instant is removed from the list for good, Counter.reserved returns to u64::MAX, and its bound is never raised again without any log line. Recommend removing the branch. Dead engines are already pruned through the Weak upgrade.
• ℹ️ src/storage_engine/revisions.rs:86 - One revision-consuming path does not pass reserve_ahead, so the claim on next_revision that "Writes are refused long before that" (revisions.rs:58-59) does not hold for it. Fix round 1 gated lock_kg, activate and create_graph and left this sibling behind. WriteProgram::view (src/storage_engine/write_program.rs:254) builds a new snapshot, and so takes a revision, for each fact statement that reads state after a rule change. It is called while staging (src/protocol/handler/write_run.rs:290), before the commit reaches lock_kg; staging_base (src/storage_engine/mod.rs:834) uses kg_handle only. Sequence: the bound cannot be written and the counter is in the headroom zone; clients retry programs that change a rule and then read; each retry consumes revisions and its commit is refused; after 2^19 such revisions the next staging view panics in the handler task instead of returning an error. No revision above the bound is returned, so the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 invariant holds; the cost is a panic where an error was intended. Smallest fix: call revisions::reserve_ahead()? in staging_base, which only write programs use.
• ⚠️ src/storage_engine/revisions.rs:120 - Unrequired component, added by fix round 2: the branch in reserve_through that drops a reservation and swallows its write error when the engine closed during the write (if open.strong_count() == 0 { return false; }, with the restructuring around it at lines 113-127). No intent requirement needs it, and the operator's round 2 decision asked only for removing the activate gate and adding the staging_base gate ("Nothing more"). A production process has one engine, so the branch can only fire at shutdown, where the alternative is one in-flight commit returning an error. Its practical use is parallel tests sharing the static counter, where one test's engine and temp directory go away while another test's commit raises bounds. That is the same role as the test-only prune branch removed in round 1. I found no wrong result from it: an engine with no strong reference issues no more revisions, so ignoring its failed write cannot let a revision pass a bound. No test covers it. Recommend reverting reserve_through to its round 1 form. This needs a decision because removal may bring back a cross-test race that the fixer presumably added it for.

🔧 Fix applied.
9 issues (1 error, 6 warnings, 2 infos) still open:

• ⚠️ src/storage_engine/revisions.rs:101 - A restore from backup can still reissue revisions, so a bare stale expect_revision can pass on a different history. The bound is read only from the data directory, and a restored directory carries an older bound or none. (1) Offline backup copies metadata/revisions.json as it was at backup time (src/storage/backup/tree.rs:27 skips only LOCK and the manifest). Sequence: back up at bound 2^20; the engine restarts and issues 2^20+1 onwards; a client observes 2^20+5; restore the backup; the restored engine continues at 2^20 and reissues 2^20+5 for a rolled-back history. (2) The online export (src/storage/backup/export.rs:141-143) writes persist/ but no revisions.json, so every restored export starts at UNRECORDED_FLOOR (2^40). Two restores of an export, or a restore of a directory that was itself upgraded from an earlier version, collide from 2^40+1. This path was equally open before the change and the issue names crash/restart only, so it is not a regression. The remedy needs authorization rather than the defect: closing it means extending the change to the backup/restore path (for example, excluding or raising the bound on restore, or requiring expect_epoch after one).
• ⚠️ src/storage_engine/revisions.rs:88 - When the durable write of the bound fails, the revision is issued anyway. reserve_through logs revision_reservation_failed and next() still returns a revision above the bound on disk. Sequence: save_json_atomic fails (ENOSPC, EIO, or a permission error on metadata/) when the counter crosses a block; commits that still succeed return revisions above the recorded bound to clients; the engine crashes; the restarted engine continues at the old bound and reissues those revisions. That is the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 failure again, limited to the window in which the write is failing. Each revision in that window also retries the temp-file write and fsync under the global mutex. The remedy needs authorization because it changes behavior: next_revision() is infallible and is called from snapshot construction, so failing closed means refusing the commit or aborting the process.
• ⚠️ src/storage_engine/revisions.rs:82 - Unrequired component: the branch in reserve_through that drops a reservation when its metadata/ directory no longer exists (commented "A removed data directory (a test's)"). No intent requirement needs it. It exists so that leaked test engines do not recreate deleted temp directories. In production it fails open: an open engine whose metadata/ is missing at that instant is removed from the list for good, Counter.reserved returns to u64::MAX, and its bound is never raised again without any log line. Recommend removing the branch. Dead engines are already pruned through the Weak upgrade.
• ℹ️ src/storage_engine/revisions.rs:86 - One revision-consuming path does not pass reserve_ahead, so the claim on next_revision that "Writes are refused long before that" (revisions.rs:58-59) does not hold for it. Fix round 1 gated lock_kg, activate and create_graph and left this sibling behind. WriteProgram::view (src/storage_engine/write_program.rs:254) builds a new snapshot, and so takes a revision, for each fact statement that reads state after a rule change. It is called while staging (src/protocol/handler/write_run.rs:290), before the commit reaches lock_kg; staging_base (src/storage_engine/mod.rs:834) uses kg_handle only. Sequence: the bound cannot be written and the counter is in the headroom zone; clients retry programs that change a rule and then read; each retry consumes revisions and its commit is refused; after 2^19 such revisions the next staging view panics in the handler task instead of returning an error. No revision above the bound is returned, so the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 invariant holds; the cost is a panic where an error was intended. Smallest fix: call revisions::reserve_ahead()? in staging_base, which only write programs use.
• ⚠️ src/storage_engine/revisions.rs:120 - Unrequired component, added by fix round 2: the branch in reserve_through that drops a reservation and swallows its write error when the engine closed during the write (if open.strong_count() == 0 { return false; }, with the restructuring around it at lines 113-127). No intent requirement needs it, and the operator's round 2 decision asked only for removing the activate gate and adding the staging_base gate ("Nothing more"). A production process has one engine, so the branch can only fire at shutdown, where the alternative is one in-flight commit returning an error. Its practical use is parallel tests sharing the static counter, where one test's engine and temp directory go away while another test's commit raises bounds. That is the same role as the test-only prune branch removed in round 1. I found no wrong result from it: an engine with no strong reference issues no more revisions, so ignoring its failed write cannot let a revision pass a bound. No test covers it. Recommend reverting reserve_through to its round 1 form. This needs a decision because removal may bring back a cross-test race that the fixer presumably added it for.
• 🚨 tests/scenarios/restart.rs:34 - The rebase brought S12 (tests/scenarios/restart.rs) onto this branch. It still wraps the bare pre-crash expect_revision case in the KnownDefect STALE_REVISION_ACCEPTED (Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380, judged at line 190), and its module docs say revisions restart with the engine and that the bare claim is accepted today. This change fixes Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380, so that expected failure now passes, and the scenario suite fails a run on an unexpected pass. The intent requires the bare pre-crash case in S12 as a required assertion. Fix: remove the KnownDefect and assert the refusal directly as a required pass in S12; correct the S12 module docs and any TESTING.md known-defect listing for Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 to match (a restarted engine continues above every revision of earlier runs, so the bare pre-crash revision is refused as predating the knowledge graph in this run); check that the S12 step that advances the new run until it has issued the pre-crash revision still terminates and still means something now that the new run starts above it, and simplify it if not. S12 then covers what tests/scenarios/stream.rs::a_bare_expect_revision_from_before_a_restart_is_refused was added for before S12 existed on main: remove that duplicate scenario and its mentions in the stream.rs module docs and TESTING.md.
• ⚠️ tests/scenarios/restart.rs:176 - Fix round 3 introduced this: the required S12 bare assertion no longer distinguishes the fix from the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 bug. The round replaced the writes.iter().cycle() loop, which advanced the new run until it had issued claimed_at, with exactly two writes (line 104). The assertion refused(reply, "precondition_failed", "revision") matches both refusal messages in src/storage_engine/precondition.rs:65-75: Unissued ("revision N has not been issued") and Predates ("revision N predates"). Sequence with the fix reverted: the restarted run counts from 1 and after two writes is still below claimed_at (the old loop had to cycle to reach it), so the bare claim is refused as Unissued and S12 passes with the bug present. The intent requires the bare pre-crash case as a required assertion, and the test-quality rule requires a regression test that fails before the fix. Smallest fix, test only: assert the property the fix provides, for example snapshot > claimed_at after the resubscribe at line 70 (the strategy's original ask that the first post-restart revision exceeds the last pre-crash one), and match "predates" instead of "revision" at line 176.
• ⚠️ src/storage_engine/revisions.rs:109 - Fix round 3 reverted the closed-engine branch as decided but left the race it covered open; its commit title says "cross-test race unresolved". The operator's decision placed that fix in the tests. Sequence in the lib test binary, where all engines share the static REVISIONS: test A reopens a data directory, so open (line 144) raises last to that directory's recorded bound, which is above every other open engine's last + HEADROOM. The next write in any test then runs reserve_through and rewrites metadata/revisions.json for every live engine. If test B drops its engine and TempDir between the upgrade() at line 110 and the write at line 115, save_json_atomic either recreates B's removed directory through create_dir_all (a leaked temp directory) or fails against the concurrent removal. In the failing case the unrelated test's write returns an error from reserve_ahead, or panics in next() at line 86. The result is an intermittent failure in tests that have no defect. Production has one engine and is not affected. The remedy needs a decision on scope, not on the defect: serialising or isolating every engine test that shares the counter is a broad test change, and the fixer did not attempt it.
• ℹ️ gui/lib/docs-bundle.ts:3920 - The tracked, generated docs bundle still carries the old websocket-api text "Revisions restart with the engine, so a revision from another run fails the precondition". The change rewrote that sentence in docs/content/docs/guides/websocket-api.mdx:198, and earlier commits that touched that page (for example 9aa2275) regenerated the bundle in the same commit. Regenerate it with node gui/scripts/bundle-docs.mjs so the in-app docs match the new contract.

🔧 Fix applied.
2 issues (1 warning, 1 info) still open:

• ⚠️ src/storage_engine/revisions.rs:101 - A restore from backup can still reissue revisions, so a bare stale expect_revision can pass on a different history. The bound is read only from the data directory, and a restored directory carries an older bound or none. (1) Offline backup copies metadata/revisions.json as it was at backup time (src/storage/backup/tree.rs:27 skips only LOCK and the manifest). Sequence: back up at bound 2^20; the engine restarts and issues 2^20+1 onwards; a client observes 2^20+5; restore the backup; the restored engine continues at 2^20 and reissues 2^20+5 for a rolled-back history. (2) The online export (src/storage/backup/export.rs:141-143) writes persist/ but no revisions.json, so every restored export starts at UNRECORDED_FLOOR (2^40). Two restores of an export, or a restore of a directory that was itself upgraded from an earlier version, collide from 2^40+1. This path was equally open before the change and the issue names crash/restart only, so it is not a regression. The remedy needs authorization rather than the defect: closing it means extending the change to the backup/restore path (for example, excluding or raising the bound on restore, or requiring expect_epoch after one).

• ℹ️ src/storage_engine/revisions_tests.rs:131 - Fix round 4 added an_engine_closing_while_bounds_are_raised_fails_no_write for the restored closed-engine branch (revisions.rs:118-126). The branch only covers a bound write that fails. If the spawned thread drops the engine and its TempDir after upgrade() but before save_json_atomic reaches create_dir_all (src/storage/metadata.rs:66), the write succeeds and recreates the removed directory, leaving a stray temp directory. The test passes either way, because it does not assert on the directory. The operator's "no directory is recreated" is asserted only in the deterministic test at line 110, which exercises the existing Weak prune and not the new branch. No wrong revision results: the engine has closed, and production has one engine. Recorded as a known limit of the branch; no change recommended.

• ⚠️ src/storage_engine/revisions.rs:23 - The durable bound lives at metadata/revisions.json, a path a knowledge graph drop can delete, after which a restart can reissue revisions (the Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380 failure). Traced from source, not executed. validate_kg_name (src/naming.rs:17) accepts metadata as a knowledge graph name and reserves no names. A knowledge graph's directory is data_dir.join(name) (src/storage_engine/mod.rs:446), and dropping it runs cleanup_dropped_kg -> delete_kg_files, which calls remove_dir_all(data_dir/<name>) (mod.rs:575, mod.rs:166). Sequence: (1) a data directory upgraded from an earlier version opens with no record, so it continues at UNRECORDED_FLOOR (2^40) and issues 2^40+1 onwards; a client observes 2^40+5. (2) A knowledge graph named metadata is created and dropped, which removes metadata/ including revisions.json. The running engine rewrites the file only at the next block raise, up to 2^20 revisions later. (3) The engine restarts before that raise. open finds no record and persist/ present, so it starts at 2^40 again (revisions.rs:152) and reissues 2^40+5 on a different history. A bare expect_revision at 2^40+5 can then pass. A directory that was never upgraded needs the drop-and-restart to happen twice. The name collision with metadata/ existed before this change (the drop also removes the knowledge graph list and tombstones, which are re-saved); this change adds a file there that is not re-saved and whose loss fails open. The remedy needs a decision, not the defect: either reserve metadata and persist as knowledge graph names (a behavior change outside Stale expect_revision may pass after restart: revisions restart, bare revision without epoch is not refused #380), or keep the bound at a path no knowledge graph name can collide with, for example revisions.json directly under the data directory (names allow only [a-z0-9_], so a dotted file name cannot collide), which also touches docs/content/docs/guides/backup.mdx:30.

• ⚠️ src/storage_engine/revisions.rs:124 - Component no longer needed by any real engine: the closed-engine branch in reserve_through (if open.strong_count() == 0 { return false; }, lines 120-126). This is a new situation, not a repeat of closed-mid-reservation-branch-unrequested: the operator requested the branch in round 4 to cover the cross-test race, and the later CI fix (419473c) closed that race a second way. OpenReservation::drop now calls Counter::close, which takes the reservations lock, and the Arc is released only after close returns. reserve_through holds that lock for the whole raise, so a real engine's strong count cannot reach 0 while its bound is being written. Only the Counter-level tests that drop a bare Arc reach the branch (an_engine_closing_while_bounds_are_raised_fails_no_write, revisions_tests.rs:131). The fixer's commit message records the same conflict and left the branch as decided. I found no wrong result from keeping it. Recommend removing the branch and that one test, and keeping close and its test (revisions_tests.rs:160) as the single mechanism. This needs a decision because it reverses a recorded operator instruction.

🔧 Fix applied.
✅ Re-checked - no issues remain.

✅ No issues found.

• Live validation: ✅ go - 9 of 9 scenarios driven live against the product

• cargo test --test scenarios restart:: -- --nocapture (S12 against a real engine process; 1 passed)
• node drive.mjs target/debug/inputlayer-server <tmp> "target 8c82c38b": five restarts (kill -9 and SIGTERM), bare and epoch-pinned stale expect_revision, in-run happy paths, second knowledge graph, missing and corrupt metadata/revisions.json (18 of 18 passed)
• SKIP_FIX_ONLY=1 node drive.mjs <base build>/inputlayer-server <tmp> "base 306df026": same driver on the base commit (6 of 15 failed; the bare stale write was accepted and applied)
• node drive-upgrade.mjs <base binary> <target binary> <tmp>: data directory written by the pre-fix engine, opened by the fixed engine (4 of 4 passed)
• node drive-bound.mjs target/debug/inputlayer-server <tmp>: pumped to revision 524268, chmod 555 metadata/, observed refusal, restored permissions (7 of 7 passed)
• Base binary built from git archive 306df026 in a temp directory with its own CARGO_TARGET_DIR, removed afterwards

✅ No issues found.

• Live validation: ✅ go - 8 of 8 scenarios driven live against the product

• cargo test --test scenarios s12_restart -- --nocapture (S12 against the real server binary, 1 passed)
• node drive.mjs target/debug/inputlayer-server <tmpdir>: real server over ws://127.0.0.1:<port>/ws, five runs separated by SIGKILL, 20/20 checks passed
• Manual check in the driver: revisions.json sits directly under the data directory and its reserved value is at or above every issued revision (1048576, then 2097152, then 3145728)
• Manual check in the driver: ?claim(O, I) after the refused writes returns only the committed rows

✅ No issues found.

✅ No issues found.

✅ No issues found.

✅ No issues found.

✅ No issues found.

✅ No issues found.