store: idempotent forward migrations — survive concurrent connect()

`arborist analyze` crashed during orientation with
`sqlite3.OperationalError: table fork_score_branches already exists`:
check-sqlite_master-then-CREATE forward migrations let two connect()
calls racing a fresh shard both pass the probe and both issue CREATE.

- CREATE TABLE/INDEX inside the _migrate_* helpers now IF NOT EXISTS
  (probe stays as the fast-path skip).
- _migrate_audit_mode's ALTER ADD COLUMN routes through
  _add_column_if_missing (PRAGMA fast-path + duplicate-column catch).
- connect() raises busy_timeout=5000 for the one migration pass so a
  racing connect() waits on a peer's _rebuild_* write txn.
- test_store_migration_concurrency.py: source-level "all CREATE is
  IF NOT EXISTS" pin, busy_timeout assertion, 8-thread connect smoke.
- test_store_migration_memoization.py: _MigrationProbeCounter.NAMES
  was missing two migrations; synced + de-hardcoded counts.
This commit is contained in:
russell@unturf.com 2026-05-11 07:28:35 -04:00
parent e894634406
commit 71c98487b7
No known key found for this signature in database
3 changed files with 245 additions and 72 deletions

View file

@ -0,0 +1,128 @@
"""Regression: forward-migration DDL must be idempotent (concurrency-safe).
Surfaced 2026-05-11 during orientation. Two ``arborist`` invocations a
few seconds apart hit shards 000-003 right after #000012 (Phase 1c)
landed — before the new ``fork_score_branches`` table had been committed
on those shards. ``arborist analyze`` crashed::
sqlite3.OperationalError: table fork_score_branches already exists
Root cause: ``store.connect()`` runs check-``sqlite_master``-then-``CREATE``
forward migrations. Two connections both pass the existence probe (table
absent), both issue ``CREATE TABLE`` — the loser raises. The probe stays
as the common-case fast-path skip, but the DDL it guards must itself be
``CREATE TABLE/INDEX IF NOT EXISTS`` so the TOCTOU loser no-ops.
The race window (probe → create within one ``connect()``) is microseconds
and not reliably reproducible from threads under the GIL, so this test
pins the invariant at the source level: every ``CREATE TABLE`` /
``CREATE INDEX`` inside a ``_migrate_*`` helper uses ``IF NOT EXISTS``.
Any future sidecar migration that forgets trips this immediately.
"""
from __future__ import annotations
import inspect
import re
import threading
from pathlib import Path
import pytest
from arborist import store
from arborist.store import _clear_migration_cache, connect
# Every forward-migration helper that issues CREATE statements (the
# CHECK-constraint table-rebuild helpers are exempt — they intentionally
# CREATE a fresh `providence_cache_new` and rename).
_MIGRATION_HELPERS = (
store._migrate_audit_mode,
store._migrate_mesh_peer_chains,
store._migrate_document_http_meta,
store._migrate_selfmodel_tables,
store._migrate_capital_ledger,
store._migrate_memory_root,
store._migrate_adapter_loss_reports,
store._migrate_controller_events,
store._migrate_fork_score_branches,
)
_BARE_CREATE = re.compile(
r"CREATE\s+(?:UNIQUE\s+)?(?:TABLE|INDEX)\s+(?!IF\s+NOT\s+EXISTS)",
re.IGNORECASE,
)
@pytest.mark.parametrize("helper", _MIGRATION_HELPERS, ids=lambda h: h.__name__)
def test_forward_migration_ddl_uses_if_not_exists(helper) -> None:
src = inspect.getsource(helper)
offenders = _BARE_CREATE.findall(src)
assert not offenders, (
f"{helper.__name__} issues CREATE without IF NOT EXISTS — a "
f"concurrent connect() loser will raise 'already exists': {offenders}"
)
def test_migrating_connect_raises_busy_timeout(tmp_path: Path) -> None:
"""The one migration pass raises ``busy_timeout`` so a ``connect()``
racing the same fresh shard waits on a peer's migration write txn
instead of failing fast with ``database is locked``."""
_clear_migration_cache()
db = tmp_path / "bt.db"
conn = connect(db)
try:
(busy_timeout,) = conn.execute("PRAGMA busy_timeout").fetchone()
assert busy_timeout == 5000
finally:
conn.close()
def test_concurrent_connect_fresh_shard_smoke(tmp_path: Path) -> None:
"""Best-effort behavioural smoke: N barrier-synced ``connect()`` calls
on a brand-new shard (per-process migration memo is lock-free, so the
losers run the migration block too) must not raise, and every sidecar
table must land exactly once. Does not guarantee hitting the TOCTOU
window — the source-level test above is the hard regression pin.
"""
_clear_migration_cache()
db = tmp_path / "race.db"
n = 8
barrier = threading.Barrier(n)
errors: list[BaseException] = []
lock = threading.Lock()
def worker() -> None:
try:
barrier.wait(timeout=15)
connect(db).close()
except BaseException as exc: # noqa: BLE001 — surface it
with lock:
errors.append(exc)
threads = [threading.Thread(target=worker) for _ in range(n)]
for t in threads:
t.start()
for t in threads:
t.join(timeout=30)
assert not errors, f"connect() raced on a fresh shard: {errors!r}"
conn = connect(db)
try:
names = {
row[0]
for row in conn.execute(
"SELECT name FROM sqlite_master WHERE type='table'"
).fetchall()
}
finally:
conn.close()
for table in (
"fork_score_branches",
"controller_events",
"memory_records",
"capital_ledger",
"selfmodel_records",
):
assert table in names, f"{table} missing after concurrent connect()"

View file

@ -1,10 +1,10 @@
"""Regression tests for ticket #000026 Phase 1.
Per-process migration memoization. ``arborist.store.connect()`` used
to run all 7 forward-migration probes on every open — 588 redundant
probe sequences per typical query. With memoization, the migration
block runs once per (physical file, process) and is skipped on every
subsequent open of the same shard.
to run every forward-migration probe on every open — hundreds of
redundant probe sequences per typical query. With memoization, the
migration block runs once per (physical file, process) and is skipped
on every subsequent open of the same shard.
Tests pin:
@ -38,6 +38,9 @@ class _MigrationProbeCounter:
counted versions.
"""
# Mirrors the forward-migration sequence in ``store.connect()`` — keep
# in sync when a migration is added (otherwise its memoization goes
# unverified, and the count assertions below silently under-count).
NAMES = (
"_migrate_audit_mode",
"_migrate_mesh_peer_chains",
@ -46,6 +49,8 @@ class _MigrationProbeCounter:
"_migrate_capital_ledger",
"_migrate_memory_root",
"_migrate_adapter_loss_reports",
"_migrate_controller_events",
"_migrate_fork_score_branches",
)
def __init__(self, monkeypatch):
@ -64,7 +69,7 @@ class _MigrationProbeCounter:
return sum(self.counts.values())
def test_first_connect_runs_all_seven_migrations(tmp_path: Path, monkeypatch):
def test_first_connect_runs_all_forward_migrations(tmp_path: Path, monkeypatch):
_clear_migration_cache()
counter = _MigrationProbeCounter(monkeypatch)
db = tmp_path / "first.db"
@ -72,8 +77,8 @@ def test_first_connect_runs_all_seven_migrations(tmp_path: Path, monkeypatch):
conn = connect(db)
conn.close()
# All seven probes ran exactly once.
assert counter.total == 7
# Every probe ran exactly once.
assert counter.total == len(counter.NAMES)
for name in counter.NAMES:
assert counter.counts[name] == 1
@ -112,6 +117,8 @@ def test_second_connect_schema_is_intact(tmp_path: Path):
"capital_ledger", # _migrate_capital_ledger
"memory_records", # _migrate_memory_root
"adapter_loss_reports", # _migrate_adapter_loss_reports
"controller_events", # _migrate_controller_events
"fork_score_branches", # _migrate_fork_score_branches
):
row = conn.execute(
"SELECT name FROM sqlite_master WHERE type='table' AND name=?",
@ -137,7 +144,7 @@ def test_clear_migration_cache_forces_reprobe(tmp_path: Path, monkeypatch):
# After clearing — full re-probe.
_clear_migration_cache()
connect(db).close()
assert counter.total == 7
assert counter.total == len(counter.NAMES)
def test_invalidate_migration_cache_after_file_replacement(
@ -169,7 +176,7 @@ def test_invalidate_migration_cache_after_file_replacement(
counter = _MigrationProbeCounter(monkeypatch)
conn = connect(db)
conn.close()
assert counter.total == 7
assert counter.total == len(counter.NAMES)
def test_distinct_paths_each_get_one_probe(tmp_path: Path, monkeypatch):
@ -182,8 +189,8 @@ def test_distinct_paths_each_get_one_probe(tmp_path: Path, monkeypatch):
counter = _MigrationProbeCounter(monkeypatch)
connect(db_a).close()
connect(db_b).close()
# Two paths × seven migrations = 14.
assert counter.total == 14
# Two paths × the full migration sequence.
assert counter.total == 2 * len(counter.NAMES)
# Re-open both — cache hit on each, zero new probes.
before = counter.total