kshitijk4poor
112baae665
Reviewer findings from the formal /simplify-code pass, all verified: 1. Scanner blind spots (HIGH): five more locked pure readers were invisible to the v1 gate — get_compression_fallback_streak and get_compression_ineffective_count hid behind `conn = self._conn` aliasing; list_gateway_sessions, find_session_by_origin and search_sessions hid behind SQL held in variables/f-strings (the scanner required unknown == 0 to flag). All five converted to _read_ctx(); the scanner now (a) tracks self._conn aliases and (b) flags lock blocks with NO proven write instead of silently skipping unprovable SQL. Sabotage self-check extended to pin all three detection classes (literal, alias, variable-SQL) plus a mixed variable-SQL writer that must NOT fire. 2. get_meta reverted to the writer lock: its inline comment (present on main) documents a real read-your-writes dependency — fts_rebuild_step reads rebuild progress that a pooled WAL reader cannot see mid-transaction. The blanket conversion had overridden a documented design decision; it is now the single justified _ALLOWED_LOCKED_READERS entry, replacing the dead _enter_fts_fail_open entry (whose lock block counts 3 writes and never needed allowlisting). Strengthened scanner on pre-conversion main: 43 violations (39 pure-read + 4 no-proven-write). This branch: zero. Suites: gate 2/2; tests/test_hermes_state.py + tests/state/ 331 passed (same 3 pre-existing FTS-rebuild reds as clean main); 433 passed across the 12 consumer suites of the five newly-converted methods (compression anti-thrash, session search, status, scheduler).
238 lines
9.8 KiB
Python
238 lines
9.8 KiB
Python
"""Pattern-C gate: pure-read SessionDB methods must not take the writer lock.
|
|
|
|
The gateway shares ONE SessionDB across every agent. ``self._lock`` guards
|
|
the single writer connection — any read-only query executed under it
|
|
convoys every concurrent turn's persistence behind that reader (Pattern C
|
|
of the 2026-08 perf triage; #90734 shipped the unlocked-reader subset,
|
|
this gate covers the locked-reader subset).
|
|
|
|
``_read_ctx()`` exists precisely for reads: WAL reader from a bounded
|
|
pool, no lock, with a byte-identical fallback to the locked writer when
|
|
WAL is off. Reads have no reason to hold the writer lock.
|
|
|
|
The gate parses ``hermes_state.py`` with ``ast`` and flags any method
|
|
that (a) opens ``with self._lock:`` and (b) runs ONLY read statements
|
|
(SELECT/PRAGMA-read) on ``self._conn`` inside it — i.e. a pure reader
|
|
convoying on the writer lock. Methods that write under the lock are the
|
|
lock's legitimate users and pass. New violations fail with the method
|
|
name and the fix (route through ``_read_ctx()``).
|
|
|
|
Deliberately NOT flagged:
|
|
- methods that INSERT/UPDATE/DELETE/REPLACE under the lock (writers);
|
|
- read-modify-write methods (the read is ordered against its own write);
|
|
- ``_read_ctx``'s own writer-fallback (``yield self._conn`` — no execute);
|
|
- SELECTs on ``conn``/other objects (already pooled readers).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import ast
|
|
import re
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
_STATE_PY = Path(__file__).resolve().parents[2] / "hermes_state.py"
|
|
|
|
_WRITE_RE = re.compile(
|
|
r"^\s*(INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|ALTER|VACUUM|BEGIN|COMMIT|ANALYZE)\b",
|
|
re.IGNORECASE,
|
|
)
|
|
# PRAGMA is read-only EXCEPT the checkpoint/optimize family, which mutates
|
|
# the database file and legitimately belongs on the writer connection.
|
|
_PRAGMA_WRITE_RE = re.compile(
|
|
r"^\s*PRAGMA\s+(wal_checkpoint|optimize|incremental_vacuum|integrity_check)",
|
|
re.IGNORECASE,
|
|
)
|
|
_READ_RE = re.compile(r"^\s*(SELECT|PRAGMA)\b", re.IGNORECASE)
|
|
|
|
# Methods allowed to keep a pure-read body under the writer lock, each with
|
|
# the reason. Keep this list SHRINKING — never add to it without the same
|
|
# scrutiny a new blocking call would get.
|
|
_ALLOWED_LOCKED_READERS: dict[str, str] = {
|
|
# get_meta stays on the writer lock BY DESIGN (see its inline comment):
|
|
# fts_rebuild_step reads rebuild progress before entering a write
|
|
# transaction, and a pooled WAL reader sees only committed data — the
|
|
# writer's own just-staged meta updates would be invisible to it.
|
|
"get_meta": "read-your-writes: rebuild progress read before write txn",
|
|
}
|
|
|
|
|
|
def _first_sql_text(call: ast.Call) -> str | None:
|
|
"""Best-effort SQL text from an execute()'s first argument."""
|
|
if not call.args:
|
|
return None
|
|
arg = call.args[0]
|
|
text = None
|
|
if isinstance(arg, ast.Constant) and isinstance(arg.value, str):
|
|
text = arg.value
|
|
elif isinstance(arg, ast.JoinedStr):
|
|
parts = [
|
|
v.value for v in arg.values
|
|
if isinstance(v, ast.Constant) and isinstance(v.value, str)
|
|
]
|
|
text = "".join(parts)
|
|
if not text or not text.strip():
|
|
return None
|
|
return text.strip()
|
|
|
|
|
|
def _is_self_conn_execute(call: ast.Call, aliases: set[str]) -> bool:
|
|
"""Match ``self._conn.execute*`` and ``<alias>.execute*`` where the
|
|
alias was bound from ``self._conn`` (``conn = self._conn``)."""
|
|
f = call.func
|
|
if not (
|
|
isinstance(f, ast.Attribute)
|
|
and f.attr in ("execute", "executemany", "executescript")
|
|
):
|
|
return False
|
|
target = f.value
|
|
if (
|
|
isinstance(target, ast.Attribute)
|
|
and target.attr == "_conn"
|
|
and isinstance(target.value, ast.Name)
|
|
and target.value.id == "self"
|
|
):
|
|
return True
|
|
return isinstance(target, ast.Name) and target.id in aliases
|
|
|
|
|
|
def _collect_conn_aliases(method: ast.AST) -> set[str]:
|
|
"""Names bound from ``self._conn`` anywhere in the method body."""
|
|
aliases: set[str] = set()
|
|
for node in ast.walk(method):
|
|
if isinstance(node, ast.Assign) and isinstance(node.value, ast.Attribute):
|
|
v = node.value
|
|
if (
|
|
v.attr == "_conn"
|
|
and isinstance(v.value, ast.Name)
|
|
and v.value.id == "self"
|
|
):
|
|
for t in node.targets:
|
|
if isinstance(t, ast.Name):
|
|
aliases.add(t.id)
|
|
return aliases
|
|
|
|
|
|
def _is_self_lock_with(item: ast.withitem) -> bool:
|
|
ctx = item.context_expr
|
|
return (
|
|
isinstance(ctx, ast.Attribute)
|
|
and ctx.attr == "_lock"
|
|
and isinstance(ctx.value, ast.Name)
|
|
and ctx.value.id == "self"
|
|
)
|
|
|
|
|
|
def _scan_locked_readers(state_py: "Path | None" = None) -> list[str]:
|
|
target = state_py if state_py is not None else _STATE_PY
|
|
tree = ast.parse(target.read_text(encoding="utf-8"))
|
|
violations: list[str] = []
|
|
|
|
session_db = None
|
|
for node in tree.body:
|
|
if isinstance(node, ast.ClassDef) and node.name == "SessionDB":
|
|
session_db = node
|
|
break
|
|
assert session_db is not None, "SessionDB class not found"
|
|
|
|
for method in session_db.body:
|
|
if not isinstance(method, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
|
continue
|
|
aliases = _collect_conn_aliases(method)
|
|
for node in ast.walk(method):
|
|
if not isinstance(node, ast.With):
|
|
continue
|
|
if not any(_is_self_lock_with(i) for i in node.items):
|
|
continue
|
|
reads, writes, unknown = 0, 0, 0
|
|
for inner in ast.walk(node):
|
|
if isinstance(inner, ast.Call) and _is_self_conn_execute(inner, aliases):
|
|
word_full = _first_sql_text(inner)
|
|
word = word_full.split(None, 1)[0].upper() if word_full else None
|
|
if word is None:
|
|
# SQL held in a variable or built f-string: the
|
|
# scanner cannot prove it reads. A lock block whose
|
|
# ONLY statements are unprovable is still flagged
|
|
# below — writers name their verbs in literals
|
|
# throughout this file, so opacity correlates with
|
|
# composed SELECTs, and silently skipping these is
|
|
# how 5 readers hid from the first version of this
|
|
# gate.
|
|
unknown += 1
|
|
elif _PRAGMA_WRITE_RE.match(word_full or ""):
|
|
writes += 1
|
|
elif _WRITE_RE.match(word):
|
|
writes += 1
|
|
elif _READ_RE.match(word):
|
|
reads += 1
|
|
else:
|
|
unknown += 1
|
|
# Method calls under the lock may write internally
|
|
# (e.g. self._execute_write, cursor ops) — treat any
|
|
# self.<something>() as potentially writing.
|
|
elif isinstance(inner, ast.Call):
|
|
f = inner.func
|
|
if (
|
|
isinstance(f, ast.Attribute)
|
|
and isinstance(f.value, ast.Name)
|
|
and f.value.id == "self"
|
|
and (
|
|
"write" in f.attr
|
|
or "commit" in f.attr
|
|
or f.attr.startswith(("set_", "record_", "insert_",
|
|
"update_", "delete_", "clear_"))
|
|
)
|
|
):
|
|
writes += 1
|
|
if writes == 0 and (reads > 0 or unknown > 0):
|
|
if method.name not in _ALLOWED_LOCKED_READERS:
|
|
kind = "pure-read" if unknown == 0 else "no-proven-write"
|
|
violations.append(
|
|
f"{method.name} (line {node.lineno}): {kind} "
|
|
f"body under `with self._lock:` — route through "
|
|
f"_read_ctx() instead (or add a justified "
|
|
f"allowlist entry)"
|
|
)
|
|
return violations
|
|
|
|
|
|
class TestNoPureReadersUnderWriterLock:
|
|
def test_no_locked_pure_readers(self):
|
|
violations = _scan_locked_readers()
|
|
assert violations == [], (
|
|
"Pure-read SessionDB methods holding the writer lock "
|
|
"(Pattern C — every concurrent turn's persistence convoys "
|
|
"behind these reads):\n " + "\n ".join(violations)
|
|
)
|
|
|
|
def test_gate_detects_a_locked_reader(self, tmp_path):
|
|
"""Sabotage self-check: the scanner must flag a synthetic violation."""
|
|
sabotage = (
|
|
"class SessionDB:\n"
|
|
" def innocent_writer(self):\n"
|
|
" with self._lock:\n"
|
|
" self._conn.execute(\"UPDATE t SET x = 1\")\n"
|
|
" def guilty_reader(self):\n"
|
|
" with self._lock:\n"
|
|
" return self._conn.execute(\"SELECT 1\").fetchone()\n"
|
|
" def guilty_alias_reader(self):\n"
|
|
" with self._lock:\n"
|
|
" conn = self._conn\n"
|
|
" return conn.execute(\"SELECT 2\").fetchone()\n"
|
|
" def guilty_variable_sql(self, query):\n"
|
|
" with self._lock:\n"
|
|
" return self._conn.execute(query).fetchall()\n"
|
|
" def innocent_variable_writer(self, query):\n"
|
|
" with self._lock:\n"
|
|
" self._conn.execute(query)\n"
|
|
" self._conn.execute(\"UPDATE t SET x = 2\")\n"
|
|
)
|
|
p = tmp_path / "fake_state.py"
|
|
p.write_text(sabotage, encoding="utf-8")
|
|
violations = _scan_locked_readers(p)
|
|
flagged = {v.split(" ")[0] for v in violations}
|
|
assert flagged == {
|
|
"guilty_reader", "guilty_alias_reader", "guilty_variable_sql"
|
|
}, violations
|