perf(sessions): open the session-listing projection read-only (#5455)
read_importable_agent_session_rows() is a pure read, but it opened a read-write sqlite connection on the live multi-GB WAL state.db and re-ran a defensive CREATE INDEX self-heal on every sidebar build. Holding a write-capable handle while the agent streams into the same DB adds needless checkpoint/lock surface. Open the DB read-only (file:...?mode=ro) for the projection, and self-heal a missing idx_messages_session through a separate short-lived writable connection only. With the index present (the normal case) the read path performs zero writes; when it is missing the self-heal still runs and rows still come back. Behavior-neutral: identical rows when the index exists; self-heal preserved when missing; read-only-fs degradation to the pre-aggregated path unchanged. Tests: tests/test_issue5455_listing_readonly_connection.py (read-only open, no writable connection when index present, self-heal when missing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
312d3faba4
commit
855b7f1ea4
2 files changed
+129
-10
No files matched your search
+24
-10
@@ -453,7 +453,16 @@ def read_importable_agent_session_rows(
|
||||
return []
|
||||
|
||||
log = log or logger
|
||||
with closing(sqlite3.connect(str(db_path))) as conn:
|
||||
# Open read-only for this projection/listing path: it is a pure read, and
|
||||
# holding a write-capable handle on the live (multi-GB, WAL) state.db while
|
||||
# the agent streams into it adds needless checkpoint/lock surface (#5455).
|
||||
# The defensive index self-heal below still runs, but through a separate
|
||||
# short-lived writable connection on the rare missing-index path only.
|
||||
try:
|
||||
conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True)
|
||||
except sqlite3.Error:
|
||||
conn = sqlite3.connect(str(db_path))
|
||||
with closing(conn):
|
||||
conn.row_factory = sqlite3.Row
|
||||
cur = conn.cursor()
|
||||
|
||||
@@ -511,16 +520,21 @@ def read_importable_agent_session_rows(
|
||||
messages_index_present = any(str(row[1]) == "idx_messages_session" for row in cur.fetchall())
|
||||
except sqlite3.Error:
|
||||
messages_index_present = False
|
||||
try:
|
||||
if not messages_index_present:
|
||||
cur.execute(
|
||||
"CREATE INDEX IF NOT EXISTS idx_messages_session "
|
||||
"ON messages(session_id, timestamp)"
|
||||
)
|
||||
conn.commit()
|
||||
if not messages_index_present:
|
||||
# Self-heal via a separate writable connection so the common
|
||||
# (index-present) path keeps its read-only handle. On a truly
|
||||
# read-only/locked db this fails and we degrade to the
|
||||
# pre-aggregated cron-only path below, exactly as before.
|
||||
try:
|
||||
with closing(sqlite3.connect(str(db_path))) as _heal:
|
||||
_heal.execute(
|
||||
"CREATE INDEX IF NOT EXISTS idx_messages_session "
|
||||
"ON messages(session_id, timestamp)"
|
||||
)
|
||||
_heal.commit()
|
||||
messages_index_present = True
|
||||
except sqlite3.Error:
|
||||
pass # read-only db / locked / older schema — degrade gracefully
|
||||
except sqlite3.Error:
|
||||
pass # read-only db / locked / older schema — degrade gracefully
|
||||
|
||||
if use_messages_join:
|
||||
actual_count_expr = f"COUNT(m.{count_col})"
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
"""Regression tests for #5455 — the session-listing projection reads read-only.
|
||||
|
||||
``read_importable_agent_session_rows()`` is a pure read, but it used to open a
|
||||
read-WRITE ``sqlite3`` connection on the live (multi-GB, WAL) ``state.db`` and
|
||||
re-run a defensive ``CREATE INDEX`` self-heal on every sidebar build. Holding a
|
||||
write-capable handle while the agent streams into the same DB adds needless
|
||||
checkpoint/lock surface.
|
||||
|
||||
The listing path now opens the DB read-only (``file:...?mode=ro``) and only
|
||||
self-heals a missing ``idx_messages_session`` through a SEPARATE short-lived
|
||||
writable connection. With the index present (the normal case) the read path
|
||||
performs zero writes; when the index is missing the self-heal still runs and
|
||||
rows still come back.
|
||||
"""
|
||||
import sqlite3
|
||||
|
||||
import api.agent_sessions as agent_sessions
|
||||
from api.agent_sessions import read_importable_agent_session_rows
|
||||
|
||||
|
||||
def _make_db(path, *, with_index=True):
|
||||
conn = sqlite3.connect(str(path))
|
||||
conn.execute(
|
||||
"""
|
||||
CREATE TABLE sessions (
|
||||
id TEXT PRIMARY KEY, title TEXT, model TEXT, message_count INTEGER,
|
||||
started_at REAL, source TEXT, session_source TEXT
|
||||
)
|
||||
"""
|
||||
)
|
||||
conn.execute(
|
||||
"INSERT INTO sessions (id, title, model, message_count, started_at, source, session_source) "
|
||||
"VALUES (?,?,?,?,?,?,?)",
|
||||
("cli-1", "Hello", "gpt", 2, 1000.0, "cli", "cli"),
|
||||
)
|
||||
conn.execute(
|
||||
"CREATE TABLE messages (id INTEGER PRIMARY KEY, session_id TEXT, role TEXT, timestamp REAL)"
|
||||
)
|
||||
conn.executemany(
|
||||
"INSERT INTO messages (session_id, role, timestamp) VALUES (?,?,?)",
|
||||
[("cli-1", "user", 1001.0), ("cli-1", "assistant", 1002.0)],
|
||||
)
|
||||
if with_index:
|
||||
conn.execute("CREATE INDEX idx_messages_session ON messages(session_id, timestamp)")
|
||||
conn.commit()
|
||||
conn.close()
|
||||
|
||||
|
||||
def _record_connects(monkeypatch):
|
||||
"""Wrap agent_sessions.sqlite3.connect and record how each conn was opened."""
|
||||
real_connect = sqlite3.connect
|
||||
calls = []
|
||||
|
||||
def spy(target, *args, **kwargs):
|
||||
calls.append({"target": str(target), "uri": bool(kwargs.get("uri"))})
|
||||
return real_connect(target, *args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(agent_sessions.sqlite3, "connect", spy)
|
||||
return calls
|
||||
|
||||
|
||||
def test_listing_opens_read_only_and_returns_rows(tmp_path, monkeypatch):
|
||||
db = tmp_path / "state.db"
|
||||
_make_db(db, with_index=True)
|
||||
calls = _record_connects(monkeypatch)
|
||||
|
||||
out = read_importable_agent_session_rows(db, exclude_sources=None)
|
||||
|
||||
assert "cli-1" in {r["id"] for r in out}
|
||||
# The read path is opened read-only via a file: URI.
|
||||
assert calls, "expected at least one sqlite connection"
|
||||
assert calls[0]["uri"] is True
|
||||
assert "mode=ro" in calls[0]["target"]
|
||||
|
||||
|
||||
def test_index_present_performs_no_writable_connection(tmp_path, monkeypatch):
|
||||
db = tmp_path / "state.db"
|
||||
_make_db(db, with_index=True)
|
||||
calls = _record_connects(monkeypatch)
|
||||
|
||||
read_importable_agent_session_rows(db, exclude_sources=None)
|
||||
|
||||
# With the index already present, no self-heal write connection is opened:
|
||||
# every connection is the read-only URI form.
|
||||
assert all(c["uri"] and "mode=ro" in c["target"] for c in calls), calls
|
||||
|
||||
|
||||
def test_missing_index_self_heals_via_separate_connection(tmp_path, monkeypatch):
|
||||
db = tmp_path / "state.db"
|
||||
_make_db(db, with_index=False)
|
||||
calls = _record_connects(monkeypatch)
|
||||
|
||||
out = read_importable_agent_session_rows(db, exclude_sources=None)
|
||||
|
||||
# Rows still come back...
|
||||
assert "cli-1" in {r["id"] for r in out}
|
||||
# ...and the self-heal ran through a separate writable (non-ro) connection.
|
||||
assert any((not c["uri"]) and "mode=ro" not in c["target"] for c in calls), calls
|
||||
# The index now exists on disk.
|
||||
verify = sqlite3.connect(str(db))
|
||||
try:
|
||||
names = {row[1] for row in verify.execute("PRAGMA index_list(messages)")}
|
||||
finally:
|
||||
verify.close()
|
||||
assert "idx_messages_session" in names
|
||||
Reference in new issue
Block a user