diff --git a/CHANGELOG/v4.md b/CHANGELOG/v4.md index f963f2ac3..7634bc4cb 100644 --- a/CHANGELOG/v4.md +++ b/CHANGELOG/v4.md @@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Two shipped diagnostics opened the live store read-write, so running one could flip a user's locks ([#1328](https://github.com/robotrocketscience/aelfrice/issues/1328)).** `MemoryStore(path)` is a *write* open: it runs the DDL battery, pending migrations, the `schema_meta` seed, `_resolve_local_scope_id` (which mints and persists a federation identity on a store that has none), eleven guarded one-shot passes, and — since [#1314](https://github.com/robotrocketscience/aelfrice/issues/1314) — `sweep_expired_locks`. `benchmarks/r3_idf_clip_bound.py` documents `--store .git/aelfrice/memory.db` and `benchmarks/temporal_spine_shadow.py` points `--db` at the same file, so an operator following either usage line mutated the corpus the script existed to measure. Reproduced on a copy of the live store: planting one expired time-boxed lock and then doing nothing but constructing a `MemoryStore` and closing it flipped `lock_level` from `user` to `none`, wrote a sweep marker, and changed the file bytes — with no analysis code having run. New `MemoryStore(..., read_only=True)` opens `mode=ro` and skips the entire open-time write window, so the engine refuses writes rather than the caller having to remember not to make them; the previous guarantee was a sentence in a benchmark docstring and it did not hold. `temporal_spine_shadow` keeps a write handle under `--backfill`, which is a deliberate write — the rule is *open for write only when writes were asked for*, not *never construct a store*. A test enumerates `benchmarks/*.py` from the directory and parses each with `ast`, failing on any `MemoryStore(...)` call without `read_only=` in a module that names a live-store path, because the failure mode is a **new** benchmark reaching for the convenient call — three did so independently. `benchmarks/consolidate_blocking_recall.py` was fixed the same way under review of [#1316](https://github.com/robotrocketscience/aelfrice/issues/1316). - **The consolidation audit reported zero duplicates exactly when the duplicate family was largest ([#1312](https://github.com/robotrocketscience/aelfrice/issues/1312)).** Candidate blocking skipped any 4-gram above a `df` cap, justified by the claim that genuine near-duplicates "share many rarer shingles as well, so they survive the cap through those." That is false for a *homogeneous* family: every member shares every shingle, so all of them sit at `df = K` and none is rarer. Past the cap the whole family lost every posting — at the shipped cap of 32, a 32-member family priced **31** removable and a 33-member family priced **0**. Beliefs the cap leaves with **no posting at all** are now rescued onto their shared shingles instead, so a homogeneous family is its own bucket at any size. The rescue is a *fallback*, not a replacement, and that distinction is the fix: an intermediate revision blocked every belief on its own rarest shingles, which closes the homogeneous cliff and opens a heterogeneous one — two near-duplicates whose minimum `df` differs never share a bucket, so the family shatters into components too small to report and the audit again reads **0** on a large family. That variant silently dropped 490 beliefs and 81 whole clusters the cap had found, including a 46-member clique in which all 1,035 pairs satisfy the shipped predicate. As a fallback the candidate set is a strict superset of the cap's, verified on the development store at 0 pairs, 0 clusters and 0 beliefs lost (`benchmarks/consolidate_blocking_recall.py`, which ships so the claim is re-derivable). On that store the largest cluster was being reported as 90 and is really **165**, 434 beliefs enter a cluster that previously entered none, and the published figure moves from 2.23% to **3.19% (1,424 of 44,594)** — the direction held, the number did not. What the fallback does *not* close is an over-cap family whose members each carry some low-`df` shingle to post to instead; measured against an uncapped pass that is 31 beliefs of 44,594, so the cap is kept as a tradeoff rather than removed. `n_beliefs_rescued` is reported, so the fallback is never silent, and the candidate budget now counts pairs *attempted* rather than distinct pairs kept — `candidates` is a set, so the old guard could never fire on the very boilerplate shape it was documented to bound. Also fixed alongside: `n_would_remove` counted user-locked members that `aelf retire` refuses without `--force`, and the O(k^2) medoid stage is now bounded by `MEDOID_SAMPLE_CAP` (74s → 46s on the development store, identical counts). `--consolidate-max-shingle-df` is replaced by `--consolidate-max-pairs`; the audit surface shipped unreleased, so no released flag changes. - **Opening a store while another process opened the same store could raise `database schema has changed` ([#1310](https://github.com/robotrocketscience/aelfrice/issues/1310)).** SQLite raises `SQLITE_SCHEMA` when the schema cookie moves between a statement's prepare and its step, which is exactly what two processes running the open-time `CREATE TABLE IF NOT EXISTS` battery against one `.git/aelfrice/memory.db` do to each other. `busy_timeout=5000` does not cover it — that pragma retries `database is locked`, a different error — so the constructor failed outright and took a required CI check down at random. The whole open-time window (the stale-`ingest_log` probe, `_SCHEMA`, `_MIGRATIONS`, the post-migration indexes, the `schema_meta` seed reads and writes, and the local scope-id resolve) now runs under one bounded re-prepare retry rather than only the DDL loop the traceback named: the reads are exposed to the same race, and a per-call-site patch would leave gaps as the constructor grows. The window is idempotent by construction (`IF NOT EXISTS` / `OR IGNORE` / marker-gated), so re-running it is a no-op. Only `schema has changed` is retried — every other `OperationalError` still propagates, and a persistently changing schema fails after a bounded number of attempts instead of spinning. Contrary to the issue's reasoning, the `_MIGRATIONS` catch three lines below did **not** already tolerate this class: it admits only `duplicate column name` and `no such column`, both of which are re-raised for anything else. - **`[relationship_detector]` was half-honoured at ingest: the flag took effect, the thresholds did not ([#1299](https://github.com/robotrocketscience/aelfrice/issues/1299)).** `ingest.py` resolved `auto_detect` from `.aelfrice.toml` and then called `write_semantic_edges(store, new_belief_ids=...)` with no threshold arguments, so `jaccard_min` / `confidence_min` / `max_candidate_pairs` reached the read-only `aelf doctor --relationships` audits and were silently ignored on the one path that actually mutates the graph. Adjacent keys in the same section, opposite reach, no trace on stderr — and the asymmetry ran the risky direction, with the audit tunable and the writer pinned at the module defaults. Ingest now threads the resolved config through. The three keys are parsed from the **same** `.aelfrice.toml` read that resolves `auto_detect` (new `resolve_ingest_relationship_config`), so the per-turn config-probe count is unchanged — measured 11 probes with no config file and 4 with one four directories up, before and after — rather than adding a second filesystem walk to a hot path ([#1289](https://github.com/robotrocketscience/aelfrice/issues/1289)/[#1298](https://github.com/robotrocketscience/aelfrice/issues/1298)). That measurement is env-unset; the env var is the other half of the precedence and it moves the count, so it is stated separately. `AELFRICE_AUTO_RELATIONSHIPS=0` decides the question without needing any threshold, and the flag-only resolver short-circuited on env before touching the filesystem — so that install paid **0** probes per turn and still does, the resolver returning before the walk rather than reading a config whose only consumer will not run. With the var set truthy the walk is real and new (0 -> 11 on a deep tree): the thresholds are then actually used, which is the point of the fix, so that one is a cost and not a regression. Precedence for `auto_detect` is unchanged (env > TOML > default-off), and default-off means a fresh install is byte-identical. `residual_overlap_min` and `max_edges_per_belief` still have no TOML key at all; `docs/user/CONFIG.md` now documents the section with per-key reach so which is which is readable. diff --git a/benchmarks/r3_idf_clip_bound.py b/benchmarks/r3_idf_clip_bound.py index 66a6571da..f09fc20a6 100644 --- a/benchmarks/r3_idf_clip_bound.py +++ b/benchmarks/r3_idf_clip_bound.py @@ -359,7 +359,11 @@ def main(argv: list[str] | None = None) -> int: print("no user-turn prompts in the audit files", file=sys.stderr) return 2 - store = MemoryStore(str(args.store)) + # #1328: read-only. This script's own usage line points `--store` at + # `.git/aelfrice/memory.db`, and a bare open runs migrations plus the + # #1314 lock-expiry sweep — measuring a store is not a reason to + # mutate it. + store = MemoryStore(str(args.store), read_only=True) try: index = BM25Index.build(store) finally: diff --git a/benchmarks/temporal_spine_shadow.py b/benchmarks/temporal_spine_shadow.py index 5fdc13b93..483a9afff 100644 --- a/benchmarks/temporal_spine_shadow.py +++ b/benchmarks/temporal_spine_shadow.py @@ -422,7 +422,12 @@ def main(argv: Sequence[str] | None = None) -> int: ap.add_argument("--out", default="/tmp/temporal_spine_shadow.json") args = ap.parse_args(argv) - store = MemoryStore(args.db) + # #1328: read-only unless `--backfill` was asked for. `--backfill` + # is the one path here that legitimately writes, so it is also the + # only one that gets a write handle; every other invocation reads a + # store it must not change, and the shipped comment above points + # `--db` at the live file. + store = MemoryStore(args.db, read_only=not args.backfill) if args.backfill: report = backfill_temporal_spine(store) print(f"[{HARNESS_NAME}] backfill: {report}", file=sys.stderr) diff --git a/src/aelfrice/store.py b/src/aelfrice/store.py index 4041dcb41..6f125918d 100644 --- a/src/aelfrice/store.py +++ b/src/aelfrice/store.py @@ -1283,7 +1283,36 @@ def hash_query(query: str) -> str: class MemoryStore: """SQLite store. Pass `:memory:` for tests, a path otherwise.""" - def __init__(self, path: str, *, project_context_default: str = "") -> None: + def __init__( + self, + path: str, + *, + project_context_default: str = "", + read_only: bool = False, + ) -> None: + """Open the store. `read_only=True` opens a diagnostic-safe handle. + + #1328. A bare open is a **write**: it runs the DDL battery, any + pending migrations, the `schema_meta` seed, `_resolve_local_scope_id` + (which generates and persists an id on a store that has none), and — + since #1314 — `sweep_expired_locks`, which flips a user's expired + locks to unlocked. Two shipped benchmarks pointed their `--store` + default at the live `.git/aelfrice/memory.db` and therefore mutated + the corpus they existed to measure; the sweep was observed changing + `lock_level` on a real belief with no analysis code having run. + + `read_only=True` opens the file `mode=ro` and skips the entire + open-time write window. SQLite then refuses writes at the engine + level, so the guarantee does not rest on the caller's discipline — + which is the point, since the previous guarantee was a sentence in a + docstring and it did not hold. + + The schema is taken as found: no migration runs, so a store written + by an older binary is read at whatever shape it has. That is correct + for a diagnostic (it should observe the store, not upgrade it) and + wrong for anything that needs the current schema, which is why this + is opt-in rather than the default. + """ # #970: repo identity stamped on new project-scope, non-user-locked # beliefs whose project_context is ''. Empty (the default) disables # stamping and the backfill — direct callers that open a store @@ -1308,18 +1337,29 @@ def __init__(self, path: str, *, project_context_default: str = "") -> None: # #1176: (store_generation, active_belief_count) memo for the # fan-effect lane. `None` = not yet computed. self._active_count_memo: tuple[int, int] | None = None - self._conn: sqlite3.Connection = sqlite3.connect(path) + self._read_only: bool = read_only + if read_only and path != ":memory:": + # `mode=ro` requires the file to exist; a missing path raises + # here rather than silently creating an empty store, which is + # the right failure for a diagnostic pointed at the wrong file. + self._conn: sqlite3.Connection = sqlite3.connect( + f"file:{path}?mode=ro", uri=True + ) + else: + self._conn = sqlite3.connect(path) self._conn.row_factory = sqlite3.Row # WAL only meaningful on-disk; harmless on :memory:. + # The documented WAL pairing: fsync on checkpoint, not on every + # commit. Durability window is the WAL — an app crash loses + # nothing; an OS crash can lose the tail of the WAL, which for a + # memory store is re-derivable (ingest_log is append-only and + # re-ingest is idempotent). Measured ~2x cheaper per commit than + # the FULL default (#1135). Both are writes, so a read-only + # handle skips them rather than relying on the except below. try: - self._conn.execute("PRAGMA journal_mode=WAL") - # The documented WAL pairing: fsync on checkpoint, not on - # every commit. Durability window is the WAL — an app crash - # loses nothing; an OS crash can lose the tail of the WAL, - # which for a memory store is re-derivable (ingest_log is - # append-only and re-ingest is idempotent). Measured ~2x - # cheaper per commit than the FULL default (#1135). - self._conn.execute("PRAGMA synchronous=NORMAL") + if not read_only: + self._conn.execute("PRAGMA journal_mode=WAL") + self._conn.execute("PRAGMA synchronous=NORMAL") except sqlite3.DatabaseError: pass # Block up to 5s waiting for a write lock instead of failing @@ -1337,9 +1377,16 @@ def __init__(self, path: str, *, project_context_default: str = "") -> None: # prepare and step. The window is idempotent by construction # (IF NOT EXISTS / OR IGNORE / marker-gated), so re-running it # is safe. See `_retry_on_schema_change`. - self._local_scope_id: str = _retry_on_schema_change( - self._apply_open_schema - ) + if read_only: + # #1328: no DDL, no migrations, no seed, no sweep. The scope id + # is read if present and left absent otherwise — generating one + # is a write, and a diagnostic has no business minting the + # federation identity of the store it is inspecting. + self._local_scope_id = self._read_only_scope_id() + else: + self._local_scope_id: str = _retry_on_schema_change( + self._apply_open_schema + ) # #1161: every one-shot below runs through `_run_guarded_migration` # so a raising pass cannot make the store unopenable. Ordering is # unchanged and the guard does not alter the success path — see @@ -1669,8 +1716,18 @@ def _run_guarded_migration(self, pass_: Callable[[], object]) -> bool: methods themselves still raise, so a test or repair tool that calls one directly sees the exception unchanged. - Returns True if the pass completed, False if it raised. - """ + Returns True if the pass completed *or was skipped*, False only + if it raised. The skip case is the #1328 read-only handle below: + there is no third return value, and True is the fail-safe of the + two, because False is consumed as "record a failure marker" — + which is itself a write. + """ + if self._read_only: + # #1328: every pass here writes, and a read-only handle exists + # precisely so a diagnostic cannot. Gated once here rather than + # at each of the eleven call sites, so a twelfth added later is + # covered without anyone remembering to cover it. + return True name = getattr(pass_, "__name__", repr(pass_)) key = f"{SCHEMA_META_MIGRATION_FAILED_PREFIX}{name}" try: @@ -1748,6 +1805,24 @@ def local_scope_id(self) -> str: """ return self._local_scope_id + def _read_only_scope_id(self) -> str: + """The persisted scope id, or `""` if the store has none (#1328). + + `_resolve_local_scope_id` mints and persists an id when the key is + absent, which is a write. On a read-only handle the honest answer + for a store that has never had one is "none" — the value is only + consumed by the write paths (`_bump_belief_version` and friends), + and those cannot run here anyway. + + Tolerates a store whose `schema_meta` table does not exist yet, + because a read-only open runs no DDL and must not assume the + current schema. + """ + try: + return self.get_schema_meta(SCHEMA_META_LOCAL_SCOPE_ID) or "" + except sqlite3.DatabaseError: + return "" + def _resolve_local_scope_id(self) -> str: """Read the persisted scope id, generating one on first open.""" existing = self.get_schema_meta(SCHEMA_META_LOCAL_SCOPE_ID) diff --git a/tests/test_readonly_diagnostics_1328.py b/tests/test_readonly_diagnostics_1328.py new file mode 100644 index 000000000..6fea29356 --- /dev/null +++ b/tests/test_readonly_diagnostics_1328.py @@ -0,0 +1,263 @@ +"""#1328: a diagnostic must not mutate the store it measures. + +`MemoryStore(path)` is a **write** open. It runs the DDL battery, any +pending migrations, the `schema_meta` seed, `_resolve_local_scope_id` +(which mints and persists an id on a store that has none) and — since +#1314 — `sweep_expired_locks`, which flips expired user locks to unlocked. +Two shipped benchmarks pointed their store argument at the live +`.git/aelfrice/memory.db` in their own usage text and opened it read-write, +so running them changed the corpus they existed to measure. + +The arms here are deliberately of two kinds. The behavioural ones prove +`read_only=True` actually prevents the mutation — including the specific +one observed, a lock being swept away. The static one enumerates +`benchmarks/` from the directory rather than from a literal list, because +the failure mode is a *new* benchmark reaching for the convenient call; a +list would pass forever while the directory grew around it. +""" +from __future__ import annotations + +import ast +import sqlite3 +from datetime import datetime, timedelta, timezone +from pathlib import Path + +import pytest + +from aelfrice.models import ( + BELIEF_FACTUAL, + LOCK_USER, + Belief, +) +from aelfrice.store import MemoryStore + +_BENCHMARKS = Path(__file__).resolve().parents[1] / "benchmarks" + + +def _store_with_an_expired_lock(path: Path) -> str: + """Build a store holding one time-boxed lock whose window has closed.""" + past = (datetime.now(timezone.utc) - timedelta(days=2)).isoformat() + store = MemoryStore(str(path)) + try: + store.insert_belief( + Belief( + id="expired_lock", + content="the release key rotates at the end of the quarter", + content_hash="h_expired_lock", + alpha=1.0, + beta=1.0, + type=BELIEF_FACTUAL, + lock_level=LOCK_USER, + locked_at=past, + created_at=past, + last_retrieved_at=None, + lock_expires_at=past, + ) + ) + finally: + store.close() + return "expired_lock" + + +class TestReadOnlyOpenDoesNotMutate: + def test_a_read_only_open_does_not_sweep_an_expired_lock( + self, tmp_path: Path + ) -> None: + """The exact mutation observed on the live store. + + Distinguishing: the sibling test below opens the same store + read-write and asserts the lock *is* swept, so this cannot pass + because the fixture failed to arm. + """ + db = tmp_path / "ro.db" + bid = _store_with_an_expired_lock(db) + + store = MemoryStore(str(db), read_only=True) + try: + assert store.get_belief(bid).lock_level == LOCK_USER + finally: + store.close() + + con = sqlite3.connect(str(db)) + try: + level = con.execute( + "SELECT lock_level FROM beliefs WHERE id = ?", (bid,) + ).fetchone()[0] + finally: + con.close() + assert level == LOCK_USER, ( + "a read-only open swept the lock: the write window is not gated" + ) + + def test_a_write_open_does_sweep_it(self, tmp_path: Path) -> None: + """The control. Without it the arm above passes on a broken + fixture that never armed an expiring lock in the first place.""" + db = tmp_path / "rw.db" + bid = _store_with_an_expired_lock(db) + store = MemoryStore(str(db)) + try: + assert store.get_belief(bid).lock_level != LOCK_USER + finally: + store.close() + + def test_a_read_only_open_leaves_the_file_bytes_alone( + self, tmp_path: Path + ) -> None: + """Stronger than "no logical change": no bytes move at all. + + Migrations, the generation seed and the scope-id mint would each + show up here even when they change nothing a query can see. + """ + db = tmp_path / "bytes.db" + _store_with_an_expired_lock(db) + # Settle the WAL so the comparison is against a quiescent file. + con = sqlite3.connect(str(db)) + con.execute("PRAGMA wal_checkpoint(TRUNCATE)") + con.close() + before = db.read_bytes() + + store = MemoryStore(str(db), read_only=True) + try: + store.list_beliefs_for_indexing() + finally: + store.close() + + assert db.read_bytes() == before + + def test_writes_are_refused_by_the_engine_not_by_convention( + self, tmp_path: Path + ) -> None: + """`mode=ro` is what makes this a guarantee. + + Gating the open-time passes stops the *known* writes. Only the + engine stops the ones nobody thought of, which is the difference + between this and the docstring that was there before. + """ + db = tmp_path / "refuse.db" + _store_with_an_expired_lock(db) + store = MemoryStore(str(db), read_only=True) + try: + with pytest.raises(sqlite3.OperationalError, match="readonly"): + store._conn.execute( # noqa: SLF001 - asserting the handle + "UPDATE beliefs SET lock_level = 'none'" + ) + finally: + store.close() + + def test_a_missing_file_raises_rather_than_being_created( + self, tmp_path: Path + ) -> None: + """A diagnostic pointed at the wrong path should say so. + + The read-write default creates an empty store, which reads as "the + corpus is empty" rather than as "that is not the corpus". + """ + with pytest.raises(sqlite3.OperationalError): + MemoryStore(str(tmp_path / "does-not-exist.db"), read_only=True) + + +class TestBenchmarksDoNotOpenTheLiveStoreForWrite: + """Static guard, enumerated from the directory. + + A literal list of the two offending files would pass forever while + `benchmarks/` grew around it, and growth is the failure mode: three + separate benchmarks reached for `MemoryStore(path)` independently. + """ + + @staticmethod + def _live_store_paths(module: Path) -> bool: + text = module.read_text(encoding="utf-8", errors="replace") + return "aelfrice/memory.db" in text or "AELFRICE_DB" in text + + @staticmethod + def _bare_memorystore_calls(module: Path) -> list[int]: + """Line numbers of `MemoryStore(...)` calls with no `read_only=`.""" + try: + tree = ast.parse(module.read_text(encoding="utf-8")) + except SyntaxError: # pragma: no cover - a broken benchmark + return [] + bare: list[int] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + fn = node.func + name = ( + fn.id if isinstance(fn, ast.Name) + else fn.attr if isinstance(fn, ast.Attribute) + else None + ) + if name != "MemoryStore": + continue + if not any(k.arg == "read_only" for k in node.keywords): + bare.append(node.lineno) + return bare + + def test_no_benchmark_opens_a_named_live_store_read_write(self) -> None: + offenders: list[str] = [] + for module in sorted(_BENCHMARKS.glob("*.py")): + if not self._live_store_paths(module): + continue + for line in self._bare_memorystore_calls(module): + offenders.append(f"{module.name}:{line}") + assert offenders == [], ( + "these benchmarks name a live store path and open it without " + f"read_only=: {offenders}. Pass read_only=True, or " + "read_only= if the script has a " + "deliberate write mode." + ) + + def test_the_guard_can_actually_fire(self, tmp_path: Path) -> None: + """The guard above is a no-op if its detector is broken. + + Feeds it a synthetic offender and asserts it is caught, so a + refactor that silently stops matching `MemoryStore(` fails here + instead of turning the real check green. + """ + offender = tmp_path / "b.py" + offender.write_text( + "from aelfrice.store import MemoryStore\n" + "s = MemoryStore('.git/aelfrice/memory.db')\n" + ) + assert self._live_store_paths(offender) + assert self._bare_memorystore_calls(offender) == [2] + + clean = tmp_path / "c.py" + clean.write_text( + "from aelfrice.store import MemoryStore\n" + "s = MemoryStore('.git/aelfrice/memory.db', read_only=True)\n" + ) + assert self._bare_memorystore_calls(clean) == [] + + +def test_a_read_only_open_logs_no_migration_failures( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """The gate in `_run_guarded_migration` earns its keep here. + + `mode=ro` already refuses the writes, so removing that gate does not + break correctness — every pass simply raises and the guard swallows + it. What it does break is quiet: eleven passes would raise, each + logging at ERROR and each attempting to write a `migration_failed:` + marker that also fails. A diagnostic that prints eleven store errors + every run is one people stop reading. + + This is the arm that fails when the gate is removed; without it the + gate is untested and the next refactor drops it as dead weight. + """ + import logging + + db = tmp_path / "quiet.db" + _store_with_an_expired_lock(db) + + with caplog.at_level(logging.ERROR, logger="aelfrice"): + store = MemoryStore(str(db), read_only=True) + try: + store.list_beliefs_for_indexing() + finally: + store.close() + + failures = [r for r in caplog.records if "migration" in r.getMessage()] + assert failures == [], ( + "a read-only open logged migration failures: the guarded-migration " + f"gate is not firing — {[r.getMessage()[:80] for r in failures]}" + )