fix(federation): open peers with mode=ro so a live WAL is honoured (#1198)

[robotrocketscience]

Closes #1198.

federation.open_peer_connection opened every peer with
file:...?mode=ro&immutable=1. immutable=1 promises SQLite the file cannot
change, and SQLite acts on that promise by skipping locking and ignoring the
write-ahead log entirely. Every aelfrice store runs in WAL mode, and a store
held open by a running hook — the normal deployment shape — keeps recent
commits in memory.db-wal until something checkpoints.

Reproduced

Same file, same instant, 30 committed beliefs, writer still open:

wal size: 20632 bytes (uncheckpointed)
immutable=1 peer sees: ERROR: no such table: beliefs
plain mode=ro sees:    30 of 30
read-only guarantee:   held (attempt to write a readonly database)

The WAL held the schema as well as the rows, so the peer saw nothing. Where
the schema has been checkpointed but recent rows have not, the failure is
quieter and worse: a smaller row count with no error, so federated retrieval
silently omits beliefs that are committed and durable. Same class as #1173 — a
live WAL means the main database file is not the data.

The read-only-media case cuts the other way

The issue flagged this as needing a deliberate decision rather than a later
discovery. It is not hypothetical, and it argues against an unconditional
drop. Measured on a read-only directory:

peer state (read-only dir)	mode=ro	immutable=1
clean, fully checkpointed	fails — attempt to write a readonly database	reads 30/30
WAL present, no -shm	fails	fails (no such table)

A WAL-mode database needs to create a -shm file even when fully
checkpointed, so plain mode=ro fails outright on read-only media. Dropping
immutable=1 unconditionally would have turned those peers from working into
silently unreachable — MemoryStore._peer_conn swallows open failures and
demotes the peer, so it would have failed quietly.

So: mode=ro is the primary form, immutable=1 the fallback. On a genuinely
read-only medium the immutable promise is truthful and is the only way to read
the peer at all. The fallback runs only after an honest read has already
failed, so it cannot mask the live-WAL case above.

One non-obvious mechanic

sqlite3.connect on a URI is lazy: it returns a handle without touching the
file, and a read-only medium only errors at the first statement. A
try/except around connect() alone is dead code — verified. The open now
issues a PRAGMA schema_version to force the pager (and the -shm attempt) to
resolve where the fallback can act on it, instead of at an arbitrary later
query.

Acceptance criteria

• Drop immutable=1; keep mode=ro — done, with the fallback above
• Test committing to a WAL-mode peer without checkpointing, asserting the
  peer sees every committed row
• Read-only guarantee unchanged — asserted against a live-WAL peer
• No other reader opens a store with immutable=1 — git grep immutable
  confirms this was the only site

Verification

• Three new tests in tests/test_federation_loader.py. Reverting the
  implementation to immutable=1 fails two of them
  (no such table: beliefs); the third is a regression guard for the
  read-only-media fallback, which the old code trivially had.
• The fallback test asserts its own precondition and skips rather than
  passing vacuously if the directory mode is not enforced (i.e. running as
  root), so it cannot silently become a no-op in CI.
• Full suite: 6259 passed, 69 skipped, 71 xfailed, 4 xpassed.

Follow-up worth considering (not in this PR)

aelf health reports peers as reachable/unreachable only. A peer opened via
the immutable fallback is reachable but reads only checkpointed state, which is
worth surfacing there. That needs a PeerDep field and a health-output change,
so it is outside these acceptance criteria.

Summary by Sourcery

Adjust peer federation to respect SQLite WAL while preserving read-only guarantees and support for read-only media.

Bug Fixes:

• Ensure federated peer connections see committed-but-uncheckpointed rows in WAL-mode stores instead of ignoring the WAL.
• Maintain a strict read-only guarantee on federated peer connections even when a peer has a live WAL present.
• Allow federated peers on genuinely read-only media to remain readable via an immutable fallback when plain read-only mode fails.

Enhancements:

• Document open_peer_connection behaviour and WAL/immutable fallback semantics in the architecture overview and changelog.

Documentation:

• Update architecture documentation to describe WAL-aware peer opening with an immutable=1 fallback for read-only media.

Tests:

• Add federation loader tests covering visibility of uncheckpointed WAL rows, enforcement of read-only behaviour against a live WAL, and immutable fallback operation on read-only directories.

[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]

Warning

Review limit reached

@robotrocketscience, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 46 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 247ce91d-10d5-4b7e-a72b-70a9d377f5d5

📥 Commits

Reviewing files that changed from the base of the PR and between 4f5f866 and 2f589f8.

📒 Files selected for processing (4)

• CHANGELOG/v4.md
• docs/concepts/ARCHITECTURE.md
• src/aelfrice/federation.py
• tests/test_federation_loader.py

---

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.}

[sourcery-ai]

Reviewer's Guide

Federated peer SQLite connections are changed to open in plain read-only mode that honours WAL, with an explicit immutable fallback for genuine read-only media, plus targeted tests and docs/changelog updates to capture the behavior and regression being fixed.

File-Level Changes

Change	Details	Files
Peer DB connections now use mode=ro as the primary form, with an immutable=1 fallback only when a read-only medium prevents WAL-aware access.	Changed open_peer_connection to connect via file:...?...mode=ro instead of mode=ro&immutable=1 as the default. Added a forced PRAGMA schema_version on the initial connection to trigger any read-only filesystem errors at open time. Implemented a try/except around the pragma to close the failing mode=ro connection and reopen using mode=ro&immutable=1 on read-only media. Left the connection in row-factory sqlite3.Row form as before.	src/aelfrice/federation.py
Federation behavior around WAL, read-only guarantees, and read-only media is now covered by explicit tests to prevent regressions.	Added _wal_peer helper to construct a WAL-mode peer with committed rows and a still-open writer. Added a test asserting that open_peer_connection on a peer with an uncheckpointed WAL sees all committed rows. Added a test asserting that peer connections remain read-only (writes raise OperationalError with a readonly message) even against a live WAL. Added a test asserting that open_peer_connection successfully falls back via immutable=1 when the peer directory is made read-only, including a root-detection probe that skips when directory mode is not enforced.	tests/test_federation_loader.py
Architecture docs and changelog now describe the new WAL-honouring federation open strategy and the regression fixed in #1198.	Updated ARCHITECTURE.md to describe open_peer_connection as opening peers in mode=ro with an immutable=1 fallback for read-only media, explicitly noting WAL honoring. Added a detailed changelog entry in v4.md explaining the previous immutable=1 behavior, its interaction with WAL, and the new mode=ro-primary, immutable-fallback strategy.	docs/concepts/ARCHITECTURE.md CHANGELOG/v4.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#1198	Change federation.open_peer_connection so peer SQLite databases are opened in a way that honours a live WAL (dropping immutable=1 as the primary mode) and update any related documentation, ensuring no other peer readers use immutable=1.	✅
#1198	Add tests that (a) commit to a WAL-mode peer without checkpointing and assert the peer connection sees all committed rows, and (b) confirm the read-only guarantee remains unchanged when opening peers.	✅
#1198	Confirm behavior on read-only media and that any remaining use of immutable=1 is deliberate (not causing the live-WAL under-read bug).	✅

Possibly linked issues

• #Umbrella: Umbrella issue flags immutable=1 WAL bug; PR changes open_peer_connection to mode=ro with tested immutable fallback.

---

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.

[github-actions]

PR-size soft cap

This PR is over the advisory size threshold:

• 213 changed lines (limit: 200)
• 4 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.

[robotrocketscience]

[claim:review:Setr:2026-07-30T17:29:48Z]

[robotrocketscience]

Good fix, right diagnosis — two things before merge, one substantive.

The core reasoning is correct and the measurements on both sides of the read-only-media question are the best part of this PR. The issue asked for a deliberate decision rather than a later discovery, and it got one backed by a table showing that an unconditional drop would have turned read-only peers from working into silently unreachable. That is the right way to resolve a "which default" question.

Verified

• Restoring immutable=1 fails test_peer_connection_reads_an_uncheckpointed_wal and test_peer_connection_is_read_only_against_a_live_wal. Load-bearing.
• AC4 confirmed independently: git grep immutable=1 -- src/ returns nothing outside federation.py.
• The root-probe in the fallback test — asserting its own precondition and skipping rather than passing vacuously — is exactly right. A read-only-media test that silently no-ops under root is worse than no test.
• Same defect family as fix(lifecycle): aelf uninstall --purge and --archive leave the entire belief corpus on disk #1173: a live WAL means the main database file is not the data. Worth the cross-reference the PR draws.

---

1. The fallback triggers on any OperationalError

The docstring's safety argument is:

The fallback is reached only once an honest read has already failed, so it cannot mask the live-WAL case.

That holds only when the failure is specifically SQLITE_READONLY. The code catches bare sqlite3.OperationalError, which also covers lock contention (database is locked), unable to open database file, and transient filesystem errors. Any of those routes to immutable=1 — and an immutable handle is WAL-blind for its whole lifetime, which is precisely the bug this PR fixes. _peer_conn swallows failures and the handle is cached, so a peer downgraded this way reads only checkpointed state until the process restarts, with no error and no log line.

Being straight about the evidence: I could not construct a lock-contention case in-process to demonstrate it — WAL readers don't block on writers, which is most of why this is unlikely rather than likely. This is a finding from reading the bare except, not a reproduction. But the fix is one line and it makes the docstring's claim true rather than nearly-true:

except sqlite3.OperationalError as exc:
    if "readonly" not in str(exc):
        raise            # not read-only media; do not hand back a WAL-blind handle
    conn.close()
    conn = sqlite3.connect(f"file:{resolved}?mode=ro&immutable=1", uri=True)

Given the whole point of the change is "never silently read stale peer state", the fallback should be reachable only by the one condition that justifies it.

2. The laziness claim is too strong as written

sqlite3.connect on a URI is lazy — it returns a handle without touching the file [...] A try/except around connect() alone is dead code — verified.

Measured:

sqlite3.connect("file:/does/not/exist?mode=ro", uri=True)
  -> sqlite3.OperationalError: unable to open database file    (at connect(), not at first statement)

So connect() does raise, for a missing file. The claim is true for the case that actually matters — -shm creation on read-only media defers to the first statement, which is why the PRAGMA schema_version is needed and is a good call — but the docstring states it as a general property of URI connections, and someone will later rely on that generality. Narrow it to the case you measured.

This is a small thing, but it's the third instance today of a docstring on this umbrella asserting slightly more than the code does, so it seems worth naming rather than letting through.

3. CodeQL: empty except in the root probe

tests/test_federation_loader.py:253 — except sqlite3.OperationalError: pass with no inline comment. The surrounding comment explains the probe, but CodeQL wants the swallow itself annotated, and it's blocking as an unresolved thread. One line inside the except saying "the plain form is blocked as expected — this is the branch we want" clears it. Keep the probe; only the comment is missing.

---

Agreed on the follow-up

aelf health reporting peers as reachable/unreachable is now a two-state answer to a three-state question: a peer opened via the immutable fallback is reachable and reads only checkpointed state. Filing that separately is right — it needs a PeerDep field and a health-output change. If item 1 above lands, that third state becomes exactly "read-only media", which makes it easier to describe than it would be today.

---

Also needs a rebase — main is now 9606628c. Happy to re-review promptly; apart from item 1 this is ready.

[robotrocketscience]

[release:review:Setr:2026-07-30T17:32:52Z]

[robotrocketscience]

[claim:review:Setr:2026-07-30T18:07:47Z]

[robotrocketscience]

Rebased onto 7d4915c5 and addressed all three review items. Now f934f591, FF on main, 3 signed commits, discretion clean, 20 federation tests green.

1. The fallback is now narrowed to SQLITE_READONLY. Any other OperationalError propagates and the caller demotes the peer. That makes the docstring's safety argument true rather than nearly-true — the fallback is reached after an honest read has failed for the one reason that justifies it, not for any reason at all.

Pinned by test_a_non_readonly_error_does_not_fall_back_to_immutable, which fakes a locked first statement and asserts no immutable=1 URI is ever opened. Widening the catch back to bare OperationalError fails it.

I could not construct a real lock case in-process — WAL readers don't block on writers, which is most of why this was unlikely rather than likely — so the test fakes the error rather than provoking it. Worth saying plainly: it pins the branch, not the scenario.

2. Corrected the laziness claim. sqlite3.connect on a URI defers the -shm attempt for a file that exists on read-only media, which is the case that matters and why the schema_version pragma is needed. But a missing file raises unable to open database file from connect itself:

sqlite3.connect("file:/does/not/exist?mode=ro", uri=True)
  -> sqlite3.OperationalError: unable to open database file

So "a try/except around connect() alone is dead code" was too strong as a general statement. Docstring and changelog now say what was actually measured.

3. CodeQL — annotated the root-probe's except ... : pass. The probe design stays: skipping rather than passing vacuously under root is right, only the comment was missing.

Labelling once CI settles.

[robotrocketscience]

Rebased onto 4f5f866d, now 2f589f8f.

The earlier pytest (3.13) red was a runner flake, not this change: it timed out test_main_skips_auto_install_for_setup and test_setup_writes_stamp_at_installed_version at the 5s global cap, both subprocess-driven setup tests in files this PR does not touch. Locally they run in 1.03s and 0.40s. The rebase re-triggers the matrix; if it reds again on the same two, that is worth a separate issue rather than a retry loop — those two sitting near the cap on CI hardware is the same class as the perf-gate problem #1197 just fixed.

[robotrocketscience]

[release:review:Setr:2026-07-30T18:25:23Z]

[github-actions]

merge-train: merged 2f589f8 → main via FF push.