Skip to content

Prevent EventStore read verbs from creating events.db-wal/events.db-shm sidecars - #80

Merged
SollanSystems merged 3 commits into
mainfrom
copilot/fix-eventstore-read-verbs
Jul 25, 2026
Merged

SollanSystems merged 3 commits into
mainfrom
copilot/fix-eventstore-read-verbs

Conversation

Copilot AI commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

status, replay, and doctor’s EventStore consistency reads were logically read-only but still reopening SQLite in a way that recreated WAL sidecars, breaking tree-byte-identity checks on store-backed workspaces. This change aligns runtime read semantics with the existing WAL-aware projection path so read verbs remain non-mutating at the filesystem level on clean stores.

  • Runtime read URI selection (WAL-aware)

    • Added a shared runtime helper to select read URI mode based on sidecar presence.
    • Uses mode=ro&immutable=1 when no events.db-wal exists (prevents sidecar creation).
    • Falls back to mode=ro when events.db-wal exists (preserves crash-left WAL replay behavior).
  • Apply fix across runtime read surfaces

    • Updated both run-id discovery and event-row reads in loop/runtime.py to use the same WAL-aware query logic.
    • This covers status, replay, and doctor’s event consistency path (which composes those runtime reads).
  • Regression coverage for sidecar cleanliness

    • Strengthened status/replay no-write test to detect newly created files by snapshotting .loop file set before/after reads.
    • Added doctor-focused regression asserting no events.db-wal/events.db-shm are left behind after EventStore-backed doctor reads.
def _readonly_query(path: Path) -> str:
    return "mode=ro" if (path.parent / (path.name + "-wal")).exists() else "mode=ro&immutable=1"

conn = sqlite3.connect(f"{path.absolute().as_uri()}?{_readonly_query(path)}", uri=True)

Copilot AI changed the title [WIP] Fix EventStore read verbs to remove sidecar files Prevent EventStore read verbs from creating events.db-wal/events.db-shm sidecars Jul 21, 2026
Copilot AI requested a review from SollanSystems July 21, 2026 01:40
@SollanSystems
SollanSystems marked this pull request as ready for review July 22, 2026 14:21
Copilot AI review requested due to automatic review settings July 22, 2026 14:21

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 160b331e71

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread loop/runtime.py

def _readonly_query(path: Path) -> str:
"""Avoid creating sidecars on clean stores while preserving crash-left WAL reads."""
return "mode=ro" if (path.parent / (path.name + "-wal")).exists() else "mode=ro&immutable=1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Recheck the WAL before trusting an immutable snapshot

When a writer starts an append after this exists() check returns false but before sqlite3.connect() opens the database, the URI remains immutable=1; SQLite then ignores the newly created WAL and its committed frames. Running status, replay, or doctor concurrently with an active loop can consequently report a stale event count or a false state divergence, so the WAL selection needs an atomic/retry check around opening the immutable connection.

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request updates Loop’s read-only runtime/doctor surfaces to open the SQLite EventStore in a way that avoids creating events.db-wal/events.db-shm sidecars on clean stores, preserving filesystem immutability for status, replay, and doctor’s consistency checks.

Changes:

  • Added a WAL-aware _readonly_query() helper in loop/runtime.py to choose mode=ro&immutable=1 when no WAL sidecar exists, otherwise fall back to mode=ro.
  • Updated runtime read paths (run-id discovery and event-row reads) to use the same WAL-aware read-only URI selection.
  • Strengthened regression tests to detect newly created .loop files after status/replay and to assert doctor reads do not leave WAL/SHM sidecars behind.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
scripts/test_loop_cli_status_replay.py Makes the no-write regression test detect newly created .loop files by re-snapshotting the directory after status/replay.
scripts/test_doctor_eventstore.py Adds a doctor regression asserting EventStore-backed reads do not leave WAL/SHM sidecars behind.
loop/runtime.py Introduces WAL-aware read-only SQLite URI selection and applies it to runtime EventStore reads.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@earfman

earfman commented Jul 22, 2026

Copy link
Copy Markdown

Heads up — separate from the WAL/immutable race the Codex review flagged, I think there's a second issue: this closes the sidecar leak only on clean stores, and the new test may be masking the other half.

The immutable=1 branch is solid — on a clean store, reads leave zero sidecars. But when a crash leaves an events.db-wal, _readonly_query falls back to plain mode=ro, and SQLite then creates an events.db-shm to read the WAL — so a read still leaves a sidecar in exactly the "preserving crash-left WAL reads" case the docstring calls out.

Reproduced (crash-left state on disk = events.db + events.db-wal, no -shm):

query chosen by _readonly_query: ?mode=ro
files before read: ['events.db', 'events.db-wal']
files after  read: ['events.db', 'events.db-shm', 'events.db-wal']   # -shm created by a READ

The added test test_..._reads_do_not_leave_wal_or_shm_sidecars builds only a clean store, so it exercises the immutable=1 branch and passes green — the -wal-present branch (the harder one) isn't covered, which is where the leak remains.

A few directions that would actually close it: checkpoint-on-open under a controlled recovery; validate the WAL then reopen with immutable=1; or read from a temp copy. A regression test that seeds a leftover events.db-wal before the read would lock it in.

Happy to share the exact repro script if useful.

— flagged via Coretexa (adversarial PR verification)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EventStore read verbs leave events.db-wal/-shm sidecars behind (tree-byte-identity broken for read-only ops)

4 participants