fix(store): retry the open-time schema window on SQLITE_SCHEMA

[robotrocketscience]

Closes #1310.

MemoryStore.__init__ executes its whole schema battery — four DDL loops plus a handful of reads and writes — with bare conn.execute calls. SQLite raises SQLITE_SCHEMA ("database schema has changed") when another process commits a schema change between prepare and step, so a concurrent open fails at random. It has been failing a required CI check.

What changed

Two module-level helpers in store.py:

• _execute_reprepare(conn, stmt, attempts=3) — re-prepares a single statement against the new schema cookie. Any OperationalError that is not the cookie race re-raises immediately and stays loud.
• _retry_on_schema_change(op, attempts=3) — retries a whole callable on the same condition, bounded so a persistently-changing schema fails rather than spins.

The open-time schema battery moved verbatim into a new _apply_open_schema(), which is invoked through the window-level retry.

Scope is wider than the issue's suggested fix, and deliberately so

The issue proposed a 2-line try/except around the _SCHEMA loop. That is not sufficient: after such a patch, five more bare self._conn.execute calls remain in the same constructor (the schema_meta probe, the store_generation insert, the schema_meta read, and _resolve_local_scope_id). Reads are exposed to SQLITE_SCHEMA too — which is the issue's own reasoning — and a per-call-site patch leaves new gaps as the constructor grows.

This is measured, not asserted. Dropping the window wrapper while keeping per-statement retries fails test_open_survives_injected_schema_change[generation_read] and [scope_id_write], while [schema_ddl] still passes — i.e. the whole-window wrapper covers exactly the sites a loop-only patch would miss.

Correction to the issue's rationale

The issue states the existing _MIGRATIONS try/except already tolerates this class. It does not: it admits only "duplicate column name" and "no such column".

What the tests prove, and what they do not

Real-concurrency reproduction was attempted and did not fire — 0 of 180 rounds locally. The suite therefore uses deterministic fault injection: a connection whose execute() raises OperationalError("database schema has changed") on the Nth call and then succeeds. The module docstring and the commit message both say plainly that these tests prove the retry is wired, not that the CI flake is eliminated. That distinction is the honest claim and should not be softened in review.

Four mutations, each verified to go red (file staged before mutating, restored after):

mutation	result
drop the window wrapper, keep per-statement retry	2 failed, 7 passed
return op() at the top of _retry_on_schema_change	6 failed, 3 passed
widen the catch to every OperationalError	2 failed, 7 passed
off-by-one on the attempt bound (i == attempts)	2 failed, 7 passed

Reviewer's checklist

1. Confirm the move is verbatim. ~60 lines relocated into _apply_open_schema(); the only intended deltas are three self._conn.execute(stmt) → _execute_reprepare(...), two comments, and a trailing return.
2. One deliberate reorder: self._invalidation_callbacks = [] now initialises before the schema window rather than after self._commit(). Argued inert — nothing in the window registers or fires a callback, and _commit does not call _fire_invalidation — but it is the single ordering change in the diff.
3. Retrying the window assumes it is idempotent. True today by construction (IF NOT EXISTS / OR IGNORE / OR REPLACE / marker-gated), and the docstring states the contract — but nothing enforces it, so a future non-idempotent statement added there would be re-run on retry.
4. Retry budgets nest: 3 statement attempts inside 3 window attempts = up to 9 executions of one DDL statement in the pathological case. Bounded, and asserted by test_open_gives_up_on_a_persistently_changing_schema.
5. The trigger is a string match on "schema has changed". CPython's sqlite3 does not surface SQLITE_SCHEMA distinctly through this path, so the message is the available signal; a future wording change would silently disable the retry.

Verification

• tests/test_store_schema_race.py → 9 passed.
• Store/migration/concurrency/federation/scope selection → 597 passed, 22 skipped.
• doctor/uninstall/hook/retrieval/cli selection → 1,737 passed, 4 skipped.
• scripts/check_migration_policy.py → "OK: no new migration entries".
• vulture src --min-confidence 80 → only the pre-existing ingest.py:113 hit.
• CHANGELOG is a single inserted line under [Unreleased] ### Fixed — insert-only.

Not done

No retry added to _run_guarded_migration's one-shot passes: they already catch Exception and degrade, so SQLITE_SCHEMA there cannot make a store unopenable. It would be recorded as a failed migration — worth a follow-up, out of scope here.

Summary by Sourcery

Add bounded retries for store open-time schema operations to handle SQLITE_SCHEMA races when multiple processes open the same store concurrently.

Bug Fixes:

• Ensure MemoryStore construction tolerates transient database schema has changed errors during open-time schema setup while still surfacing other OperationalError conditions.

Enhancements:

• Factor the open-time DDL, seed, and backfill logic into an idempotent _apply_open_schema helper and centralize schema-change retry behaviour in _retry_on_schema_change and _execute_reprepare.
• Initialize invalidation callbacks before the schema application window to keep ordering consistent with future retries.

Documentation:

• Document the SQLITE_SCHEMA open-time race and its fix in the v4 CHANGELOG.

Tests:

• Add targeted tests that inject SQLITE_SCHEMA and other OperationalError failures to validate retry wiring, idempotence assumptions, and bounded behaviour on persistently changing schemas.

Summary by CodeRabbit

• Bug Fixes

  • Improved resilience when opening stores during concurrent SQLite schema changes.
  • Automatically retries transient schema-change errors while preserving failures for other operational errors.
  • Limits retries and reports an error when schema changes persist.

• Tests

  • Added coverage for retry behavior, statement re-execution, error propagation, and persistent schema changes.

• Documentation

  • Added an unreleased changelog entry describing the improved initialization retry behavior.

[sourcery-ai]

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

📝 Walkthrough

Walkthrough

SQLite schema-change errors during MemoryStore initialization now use bounded retries. The retry covers schema setup, migrations, seeding, backfills, commit, and scope resolution. Tests verify re-preparation, error propagation, usability, and retry limits.

Changes

Store initialization schema retries

Layer / File(s)	Summary
Schema-change retry infrastructure src/aelfrice/store.py	Adds bounded retries for SQLite schema has changed errors and re-prepares failed statements. Other operational errors still propagate.
MemoryStore open-schema integration src/aelfrice/store.py	Moves initialization work into _apply_open_schema and applies retries across the complete schema setup window.
Race-condition validation and release notes tests/test_store_schema_race.py, CHANGELOG/v4.md	Adds fault-injection tests for retry behavior, persistent failures, non-schema errors, and successful store opening. Documents the change.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MemoryStore
  participant RetryWrapper
  participant SQLite
  participant TestHarness
  TestHarness->>MemoryStore: initialize store
  MemoryStore->>RetryWrapper: run _apply_open_schema
  RetryWrapper->>SQLite: execute schema operation
  SQLite-->>RetryWrapper: schema has changed
  RetryWrapper->>SQLite: re-prepare and retry
  SQLite-->>MemoryStore: initialization result
  MemoryStore-->>TestHarness: usable store or bounded error

Loading

Possibly related PRs

• robotrocketscience/aelfrice#1140: Both changes modify MemoryStore initialization and schema setup in src/aelfrice/store.py.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the store fix for open-time SQLITE_SCHEMA errors.
Description check	✅ Passed	The description covers the change, linked issue, verification, tests, reviewer notes, and known limitations.
Linked Issues check	✅ Passed	The implementation addresses #1310 with bounded retries, selective error handling, full open-window coverage, and distinguishing tests.
Out of Scope Changes check	✅ Passed	The changes remain within store retry logic, related tests, and the corresponding Unreleased changelog entry.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-1310-schema-race

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR-size soft cap

This PR is over the advisory size threshold:

• 471 changed lines (limit: 200)
• 3 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

[sourcery-ai]

Reviewer's Guide

Adds targeted retry handling for SQLITE_SCHEMA during store open by introducing helpers that re-run open-time schema operations when SQLite reports "database schema has changed", relocating the schema battery into an idempotent helper and covering both DDL and reads under a bounded retry window, with focused tests and changelog update.

Sequence diagram for store open-time schema retry on SQLITE_SCHEMA

sequenceDiagram
    participant MemoryStore__init__
    participant retry_on_schema_change as _retry_on_schema_change
    participant apply_open_schema as _apply_open_schema
    participant execute_reprepare as _execute_reprepare
    participant sqlite3_connection as sqlite3_Connection

    MemoryStore__init__->>retry_on_schema_change: _retry_on_schema_change(self._apply_open_schema)
    loop attempts <= _SCHEMA_RETRY_ATTEMPTS
        retry_on_schema_change->>apply_open_schema: call
        apply_open_schema->>sqlite3_connection: _drop_stale_ingest_log
        apply_open_schema->>execute_reprepare: _execute_reprepare(self._conn, stmt)
        execute_reprepare->>retry_on_schema_change: _retry_on_schema_change(lambda: conn.execute(stmt))
        retry_on_schema_change->>sqlite3_connection: conn.execute(stmt)
        alt [schema has changed]
            sqlite3_connection-->>retry_on_schema_change: OperationalError("database schema has changed")
            retry_on_schema_change-->>retry_on_schema_change: retry (re-prepare statement/window)
        else [other OperationalError or attempts exhausted]
            sqlite3_connection-->>retry_on_schema_change: OperationalError
            retry_on_schema_change-->>MemoryStore__init__: raise
        end
        apply_open_schema->>sqlite3_connection: self._commit()
        apply_open_schema->>MemoryStore__init__: return _resolve_local_scope_id()
    end
    MemoryStore__init__-->>MemoryStore__init__: self._local_scope_id = scope_id

Loading

File-Level Changes

Change	Details	Files
Introduce bounded retry helpers that detect SQLITE_SCHEMA by message and re-run either a callable or a single statement against the updated schema cookie.	Define constants for the schema-change message and default retry attempts. Add a generic _retry_on_schema_change helper that retries only on the "schema has changed" OperationalError and is bounded by an attempt counter. Add _execute_reprepare to re-run parameterless statements with the retry helper, propagating non-schema OperationalError values unchanged.	src/aelfrice/store.py
Refactor MemoryStore.init schema setup into an idempotent window function and execute it under the schema-change retry helper, including a small reordering of one field initialisation.	Extract the open-time DDL + seed + backfill sequence into a new _apply_open_schema method that returns the resolved local scope id. Replace direct self._conn.execute calls in schema-related loops with _execute_reprepare where appropriate while preserving the existing migration error filtering for duplicate/no-such-column cases. Call _apply_open_schema via _retry_on_schema_change from MemoryStore.init, initialising _invalidation_callbacks before the schema window and wiring _local_scope_id to the helper’s return value.	src/aelfrice/store.py
Add deterministic fault-injection tests that validate retry behaviour for both helpers and the MemoryStore open-time window under SQLITE_SCHEMA and other OperationalError scenarios.	Introduce a _FaultingConnection wrapper and a _faulting_connect context manager to inject controlled OperationalError instances on matching SQL statements. Test that _retry_on_schema_change retries once for SQLITE_SCHEMA, propagates other OperationalError values, and respects the attempt bound. Test that _execute_reprepare successfully re-runs a CREATE TABLE statement after a single injected schema-change error. Add parametrized tests that ensure MemoryStore.init survives injected schema-change errors at key points in the open-time window, still raises on non-schema errors, and gives up after bounded attempts when the schema keeps changing.	tests/test_store_schema_race.py
Document the fix and its scope in the v4 changelog, noting that only schema-change errors are retried and that the whole open-time window is idempotently re-run.	Add a detailed Fixed entry describing the SQLITE_SCHEMA race on concurrent store open and explaining the new whole-window bounded retry approach. Clarify that only the "schema has changed" error is retried and that other OperationalError cases continue to propagate. Note that the migrations block’s existing error filtering did not previously cover the schema-change case.	CHANGELOG/v4.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#1310	Ensure MemoryStore.init tolerates concurrent schema changes (SQLITE_SCHEMA / 'database schema has changed') during the open-time schema battery, retrying only this specific error without swallowing genuinely malformed statements or other OperationalErrors.	✅
#1310	Add tests that (a) fail without the fix by injecting schema-change errors into the open-time window and (b) verify that malformed or non-schema statements still raise rather than being retried into silence, with bounded retry behaviour.	✅
#1310	Clarify and document whether _drop_stale_ingest_log (the ingest_log probe/drop immediately above the schema loops) has the same exposure to concurrent schema changes during store open.	✅

Possibly linked issues

• fix(store): concurrent MemoryStore init races on the _SCHEMA loop — 'database schema has changed' fails a required check at random #1310: PR implements windowed and per-statement SQLITE_SCHEMA retries for MemoryStore open-time schema work, fixing the race.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[robotrocketscience]

[claim:review:Setr:2026-08-04T05:02:22Z]

[robotrocketscience]

[claim:review:Kulili:2026-08-04T05:06:32Z]

[robotrocketscience]

[release:review:Kulili:2026-08-04T05:06:37Z]

[robotrocketscience]

Review — no defects found; rebased onto main (7f1a9301)

I worked the reviewer's checklist in the body rather than around it, and independently checked the one thing the PR itself cannot: whether the string the retry keys on is the string SQLite actually produced.

The trigger string is verified against the real failure, not against the injected one. This is the load-bearing question — the tests inject OperationalError("database schema has changed"), so they would pass identically if the production message were worded differently and the retry never fired. The observed CI traceback on #1310 (pytest (3.12), job 91337037667) reads sqlite3.OperationalError: database schema has changed verbatim, so _SCHEMA_CHANGED_MSG = "schema has changed" matches the message that actually took the check down. Checklist item 5 stands as a real fragility, but it is not currently mis-keyed.

I also tried to produce a natural SQLITE_SCHEMA to double-check, and reproduced your negative result from the other direction: on SQLite 3.50.4, 200 consecutive cookie bumps between prepare and execute were re-prepared transparently by sqlite3_prepare_v2 every time, and a mid-scan cursor returned its remaining 499 rows without raising. That is why it is 0/180 rather than a flaw in how you tried — the error only escapes when re-preparation itself cannot succeed. It also means fault injection genuinely is the only deterministic arm available here, and the docstring's refusal to claim more than "the retry is wired" is the correct claim, not excessive hedging.

Checklist item 1 — the move is verbatim, checked mechanically rather than by eye. I extracted both blocks (main's __init__ window vs the PR's _apply_open_schema), stripped comments and blank lines, normalised _execute_reprepare(self._conn, stmt) back to self._conn.execute(stmt) and the trailing return back to the assignment, and diffed. 43 lines vs 42, and the only residual difference is the _invalidation_callbacks line leaving the block — i.e. exactly the delta you declared in item 2 and nothing else.

Checklist item 2 — the reorder is inert, and provably so in the safe direction. _fire_invalidation is reached only from _commit_mutation and the transaction() exit, neither of which the window touches; _commit is a bare conn.commit(). And the argument is one-sided: moving the initialiser earlier can only convert a hypothetical AttributeError into a no-op over an empty list. Since the old ordering never crashed, nothing in the window fires it.

Checklist item 3 — I audited the window's idempotency statement by statement rather than taking the docstring's word. All 12 _MIGRATIONS are ALTER TABLE ... ADD/DROP COLUMN, so a second pass raises duplicate column name / no such column into the existing catch. All 8 _POST_MIGRATION_INDEXES are CREATE INDEX IF NOT EXISTS. Both _BACKFILL_STATEMENTS are UPDATE ... WHERE origin = 'unknown' setting origin to a non-unknown value, so they are naturally self-idempotent in addition to being marker-gated. _drop_stale_ingest_log is gated on column-set mismatch and a zero row count. _resolve_local_scope_id is read-first — and worth noting explicitly, because it is the one place a non-idempotent retry would have been serious: a retry that re-ran it could in principle have persisted a second scope id and broken federation identity. It cannot, because a pass that fails before set_schema_meta commits leaves nothing for the next pass to read, so exactly one id is ever written.

Your framing that nothing enforces the contract is the right residual worry, and it is the one thing here that could bite later. The retry is invisible at the point where someone would add a statement.

Checklist item 4 — the 3×3 = 9 bound is asserted, and test_open_gives_up_on_a_persistently_changing_schema is the arm that keeps it from being unbounded.

Mutations reproduced independently, not read off your table. return op() at the top of _retry_on_schema_change → 6 failed, 3 passed. Widening the catch to every OperationalError → 2 failed, 7 passed, and specifically the two that assert a non-schema error stays loud. Both match what you reported.

The correction to the issue's rationale is right. The _MIGRATIONS catch admits only duplicate column name and no such column; the issue's claim that it already tolerates this class is wrong, and a fix built on that premise would have left the _SCHEMA loop as the only patched site. Widening scope past what the issue proposed was the correct call here, and the [generation_read] / [scope_id_write] arms are the evidence rather than the assertion.

What I changed

Rebase only — the branch was behind main after #1322 and #1323 landed. store.py auto-merged (#1322 touches next_exploration_fire_idx, a different method; I confirmed its SELECT MAX(fire_idx) clamp is intact on the rebased tree). CHANGELOG/v4.md conflicted where both PRs inserted at the head of ### Fixed; resolved insert-only, keeping both bullets with no reordering of the other 73 — the section goes 74 → 75 entries, so nothing was dropped in the resolution.

Verification

Full suite on the rebased tree: 7004 passed, 69 skipped, 71 xfailed. The two ..._when_fastmcp_missing failures are a local dependency-state artifact of the mcp bump and are green in CI. tests/test_store_schema_race.py → 9 passed. Discretion grep on added lines vs main: clean. All three commits signed.

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_store_schema_race.py (1)

258-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tighten the bound to exact equality.

The count here is deterministic. The first _SCHEMA statement fails 3 times inside _execute_reprepare, the outer _retry_on_schema_change retries the window 3 times, so the injection fires exactly 9 times.

<= 9 also passes when retry coverage regresses. Removing the outer window retry gives 3 firings. Removing _execute_reprepare from the DDL loop also gives 3 firings. Lowering _SCHEMA_RETRY_ATTEMPTS to 2 gives 4. All three satisfy <= 9, so this test would stay green against the composition it exists to pin. Every sibling test in this file asserts an exact call count.

== 9 keeps the hang guard and adds a guard on both retry layers.

♻️ Proposed change to pin the exact count

     # 3 outer window attempts x 3 statement-level attempts, not unbounded.
-    assert made[0].fired <= 9
+    assert made[0].fired == 9, "both retry layers must be in the path"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_store_schema_race.py` around lines 258 - 259, Update the assertion
for the schema injection counter in the race test to require exactly 9 firings
instead of allowing any value up to 9, preserving the existing hang guard while
validating both retry layers.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_store_schema_race.py`:
- Around line 258-259: Update the assertion for the schema injection counter in
the race test to require exactly 9 firings instead of allowing any value up to
9, preserving the existing hang guard while validating both retry layers.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c688963-21ac-4ffc-97db-fbb95844474b

📥 Commits

Reviewing files that changed from the base of the PR and between b2873e5 and 7f1a930.

📒 Files selected for processing (3)

• CHANGELOG/v4.md
• src/aelfrice/store.py
• tests/test_store_schema_race.py

[github-actions]

merge-train: merged 7f1a930 → main via FF push.

[robotrocketscience]

[release:review:Setr:2026-08-04T05:13:30Z]