From d6579d73455e25be9bbe5e2d607ef8a8873d7fc7 Mon Sep 17 00:00:00 2001 From: Christian Boos Date: Thu, 25 Jun 2026 10:52:16 +0200 Subject: [PATCH 1/5] perf(cache): set PRAGMA synchronous=NORMAL on cache connections Paired with the existing WAL journal mode, synchronous=NORMAL skips an fsync on every commit while preserving durability across application crashes (only a power/OS crash can lose the last committed transaction). The SQLite cache is fully regenerable from the JSONL source, so that residual risk is acceptable. Measured: ~6x faster on a commit-bound microbenchmark (300 commits: 363ms -> 57ms). NOTE the real-build impact is modest: a full single-project build issues only ~47 commits (~169ms total fsync), so this saves ~145ms. The dominant cache overhead is connection churn (~189 sqlite3.connect calls per build), addressed separately. Adds a test pinning synchronous=NORMAL alongside the WAL-mode test. Co-Authored-By: Claude Opus 4.8 (1M context) --- claude_code_log/cache.py | 7 +++++++ test/test_cache_sqlite_integrity.py | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/claude_code_log/cache.py b/claude_code_log/cache.py index 62f9d250..dc0dc6a4 100644 --- a/claude_code_log/cache.py +++ b/claude_code_log/cache.py @@ -318,6 +318,13 @@ def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: conn.row_factory = sqlite3.Row conn.execute("PRAGMA foreign_keys = ON") conn.execute("PRAGMA journal_mode = WAL") + # synchronous=NORMAL is the recommended pairing for WAL: it keeps + # durability across application crashes (only a power/OS crash can lose + # the last committed transaction) while skipping an fsync on every + # commit. The cache is fully regenerable from the JSONL source, so that + # residual risk is acceptable, and on Windows the per-commit fsync was + # measured as ~two-thirds of cache-build time. + conn.execute("PRAGMA synchronous = NORMAL") try: yield conn finally: diff --git a/test/test_cache_sqlite_integrity.py b/test/test_cache_sqlite_integrity.py index bc1c3e43..2424d980 100644 --- a/test/test_cache_sqlite_integrity.py +++ b/test/test_cache_sqlite_integrity.py @@ -635,6 +635,20 @@ def test_wal_journal_mode_enabled(self, cache_manager): row = conn.execute("PRAGMA journal_mode").fetchone() assert row[0] == "wal" + def test_synchronous_normal(self, cache_manager): + """Connections run with synchronous=NORMAL (1). + + Paired with WAL this skips an fsync on every commit while keeping + durability across application crashes (only a power/OS crash can lose + the last committed transaction). The cache is fully regenerable from + the JSONL source, so that residual risk is acceptable and gives a large + speedup on the per-commit fsync-bound build path (Windows especially). + """ + with cache_manager._get_connection() as conn: + row = conn.execute("PRAGMA synchronous").fetchone() + # 0=OFF, 1=NORMAL, 2=FULL, 3=EXTRA + assert row[0] == 1, f"expected synchronous=NORMAL (1), got {row[0]}" + class TestConcurrentAccess: """Tests for concurrent database access.""" From 790c55e5cebd081892b9b7610122a543727cd509 Mon Sep 17 00:00:00 2001 From: Christian Boos Date: Thu, 25 Jun 2026 11:21:43 +0200 Subject: [PATCH 2/5] perf(cache): reuse one connection per build via CacheManager.batch() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full single-project build issued ~189 sqlite3.connect() calls — every CacheManager method (reads included) opened its own connection. That connection churn, not fsync, is the dominant cache-build cost (~3.9ms/open). Add an opt-in batch() context manager that opens one shared connection, has _get_connection() reuse it, and closes it on scope exit (including on exception, via finally). Wrap the converter's build hotspots in batch(): - ensure_fresh_cache: invalidation reads + populate (load + session/ aggregate writes) share one connection. - load_directory_transcripts: the per-file load loop. - _generate_individual_session_files: the per-session staleness checks, html_cache writes, and per-session renderer cache reads. Nesting is a no-op reuse (inner batch leaves the outer connection's lifecycle to the outermost scope), so these compose safely — the process-hierarchy path drives per-project builds through convert_jsonl_to and inherits the reuse. Result: connection opens for a single-project build drop ~189 -> ~14 (93%). Outside a batch the default is unchanged (connection-per-call), which keeps the Windows-safe no-lingering-handle behaviour: batch() closes the shared connection before any temp-dir teardown / clear_cache rmtree (WinError 32). Integrity tests pin reuse, normal+exception close, nesting, write-persistence, and the rmtree-after-build case. Co-Authored-By: Claude Opus 4.8 (1M context) --- claude_code_log/cache.py | 64 +++++++- claude_code_log/converter.py | 243 +++++++++++++++------------- test/test_cache_sqlite_integrity.py | 121 ++++++++++++++ 3 files changed, 312 insertions(+), 116 deletions(-) diff --git a/claude_code_log/cache.py b/claude_code_log/cache.py index dc0dc6a4..1d1299b1 100644 --- a/claude_code_log/cache.py +++ b/claude_code_log/cache.py @@ -309,12 +309,14 @@ def __init__( # Initialise database and ensure project exists self._init_database() self._project_id: Optional[int] = None + # When inside a batch() scope this holds the single shared connection + # reused by every _get_connection() call; None means the default + # open-a-connection-per-call behaviour. + self._shared_conn: Optional[sqlite3.Connection] = None self._ensure_project_exists() - @contextmanager - def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: - """Get a database connection with proper settings.""" - conn = sqlite3.connect(self.db_path, timeout=30.0) + def _configure_connection(self, conn: sqlite3.Connection) -> None: + """Apply the standard pragmas/row factory to a fresh connection.""" conn.row_factory = sqlite3.Row conn.execute("PRAGMA foreign_keys = ON") conn.execute("PRAGMA journal_mode = WAL") @@ -322,14 +324,64 @@ def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: # durability across application crashes (only a power/OS crash can lose # the last committed transaction) while skipping an fsync on every # commit. The cache is fully regenerable from the JSONL source, so that - # residual risk is acceptable, and on Windows the per-commit fsync was - # measured as ~two-thirds of cache-build time. + # residual risk is acceptable. conn.execute("PRAGMA synchronous = NORMAL") + + @contextmanager + def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: + """Get a database connection with proper settings. + + Inside a ``batch()`` scope this yields the shared connection without + closing it (the batch owns its lifecycle). Otherwise it opens a fresh + connection and closes it on exit — the default, Windows-safe behaviour + (no lingering file handle on the .db/.db-wal/.db-shm files). + """ + if self._shared_conn is not None: + yield self._shared_conn + return + + conn = sqlite3.connect(self.db_path, timeout=30.0) + self._configure_connection(conn) try: yield conn finally: conn.close() + @contextmanager + def batch(self) -> Generator[None, None, None]: + """Reuse a single connection for every operation within the scope. + + A full project build issues ~190 ``_get_connection`` calls; opening a + fresh SQLite connection each time dominates cache-build cost. Wrapping + the build in ``with cache_manager.batch():`` collapses those to one + connection. + + Lifecycle guarantees (the risky part — see the integrity tests): + - The shared connection is **always closed on scope exit**, including + when the body raises (the ``finally``). This releases the file lock + on the .db/.db-wal/.db-shm files *before* any caller tears down a + TemporaryDirectory or runs ``clear_cache``/rmtree — critical on + Windows, which refuses to delete open files (WinError 32). + - Outside a batch, behaviour is unchanged (connection-per-call). + - Nesting is a no-op: an inner ``batch()`` reuses the outer connection + and does NOT close it, so the converter loop can't double-open or + close a connection still in use by an enclosing scope. + """ + if self._shared_conn is not None: + # Already batching — reuse the existing shared connection and leave + # its lifecycle to the outermost batch(). + yield + return + + conn = sqlite3.connect(self.db_path, timeout=30.0) + self._configure_connection(conn) + self._shared_conn = conn + try: + yield + finally: + self._shared_conn = None + conn.close() + def _init_database(self) -> None: """Create schema if needed using migration runner.""" # Run any pending migrations diff --git a/claude_code_log/converter.py b/claude_code_log/converter.py index 18b39ea0..99ea2e41 100644 --- a/claude_code_log/converter.py +++ b/claude_code_log/converter.py @@ -835,11 +835,19 @@ def load_directory_transcripts( f for f in directory_path.glob("*.jsonl") if not f.name.startswith("agent-") ] - for jsonl_file in jsonl_files: - messages = load_transcript( - jsonl_file, cache_manager, from_date, to_date, silent - ) - all_messages.extend(messages) + # Reuse one connection across all per-file cache reads/writes in this load + # pass. Nested under an outer batch() (e.g. ensure_fresh_cache) this is a + # no-op reuse; called standalone (Phase 2 reload) it opens and closes its + # own shared connection. nullcontext keeps the no-cache path unchanged. + load_batch = ( + cache_manager.batch() if cache_manager is not None else contextlib.nullcontext() + ) + with load_batch: + for jsonl_file in jsonl_files: + messages = load_transcript( + jsonl_file, cache_manager, from_date, to_date, silent + ) + all_messages.extend(messages) # Parent agent entries and assign synthetic session IDs so they # form separate DAG-lines spliced at their anchor points. @@ -2022,33 +2030,39 @@ def ensure_fresh_cache( if not session_jsonl_files: return False - # Get cached project data - cached_project_data = cache_manager.get_cached_project_data() - - # Check various invalidation conditions - modified_files = cache_manager.get_modified_files(session_jsonl_files) - needs_update = ( - cached_project_data is None - or from_date is not None - or to_date is not None - or bool(modified_files) # Session files changed - or ( - cached_project_data.total_message_count == 0 and session_jsonl_files - ) # Stale cache - ) + # Reuse one connection for the invalidation reads AND the whole populate + # pass (per-file load + save + the session/aggregate writes) instead of + # opening one per call. batch() closes the shared connection on scope exit + # (incl. on exception), so the cache files are unlocked before any caller + # tears down a temp dir. + with cache_manager.batch(): + # Get cached project data + cached_project_data = cache_manager.get_cached_project_data() + + # Check various invalidation conditions + modified_files = cache_manager.get_modified_files(session_jsonl_files) + needs_update = ( + cached_project_data is None + or from_date is not None + or to_date is not None + or bool(modified_files) # Session files changed + or ( + cached_project_data.total_message_count == 0 and session_jsonl_files + ) # Stale cache + ) - if not needs_update: - return False # Cache is already fresh + if not needs_update: + return False # Cache is already fresh - # Load and process messages to populate cache - if not silent: - print(f"Updating cache for {project_dir.name}...") - messages, _tree = load_directory_transcripts( - project_dir, cache_manager, from_date, to_date, silent - ) + # Load and process messages to populate cache + if not silent: + print(f"Updating cache for {project_dir.name}...") + messages, _tree = load_directory_transcripts( + project_dir, cache_manager, from_date, to_date, silent + ) - # Update cache with fresh data - _update_cache_with_session_data(cache_manager, messages) + # Update cache with fresh data + _update_cache_with_session_data(cache_manager, messages) return True @@ -2228,93 +2242,102 @@ def _generate_individual_session_files( ) regenerated_count = 0 - # Generate HTML file for each session - for session_id in session_ids: - # Create session-specific title using cache data if available - session_title = build_session_title( - project_title, - session_id, - session_data.get(session_id), - ) - - # Add date range if specified - if from_date or to_date: - date_range_parts: list[str] = [] - if from_date: - date_range_parts.append(f"from {from_date}") - if to_date: - date_range_parts.append(f"to {to_date}") - date_range_str = " ".join(date_range_parts) - session_title += f" ({date_range_str})" - - # Check if session file needs regeneration - session_file_name = f"session-{session_id}{suffix}.{ext}" - session_file_path = output_dir / session_file_name - - # Use incremental regeneration: check per-session staleness via html_cache - if cache_manager is not None and format == "html": - is_stale, _reason = cache_manager.is_html_stale( - session_file_name, session_id - ) - should_regenerate_session = ( - is_stale - or renderer.is_outdated(session_file_path) - or from_date is not None - or to_date is not None - or not session_file_path.exists() - ) - else: - # Fallback without cache or non-HTML formats - should_regenerate_session = ( - renderer.is_outdated(session_file_path) - or from_date is not None - or to_date is not None - or not session_file_path.exists() - or cache_was_updated - ) - - if should_regenerate_session: - # Generate session content. Under `--combined no` the - # combined file is never written, so the per-session - # back-link would 404 — suppress it. - session_content = renderer.generate_session( - messages, + # Reuse one connection for every per-session staleness check + html_cache + # write, plus the per-session cache reads inside renderer.generate_session. + # Without this each session reopens the DB several times. nullcontext keeps + # the no-cache path unchanged; nested under an outer batch it's a no-op + # reuse, and the shared connection is closed on scope exit. + session_batch = ( + cache_manager.batch() if cache_manager is not None else contextlib.nullcontext() + ) + with session_batch: + # Generate HTML file for each session + for session_id in session_ids: + # Create session-specific title using cache data if available + session_title = build_session_title( + project_title, session_id, - session_title, - cache_manager, - output_dir, - session_tree=session_tree, - suppress_combined_link=not write_combined, - ) - assert session_content is not None - # Write session file - # See issue #139: errors="replace" for lone-surrogate safety. - session_file_path.write_text( - session_content, encoding="utf-8", errors="replace" + session_data.get(session_id), ) - regenerated_count += 1 - # Update html_cache to track this generation (HTML only) + # Add date range if specified + if from_date or to_date: + date_range_parts: list[str] = [] + if from_date: + date_range_parts.append(f"from {from_date}") + if to_date: + date_range_parts.append(f"to {to_date}") + date_range_str = " ".join(date_range_parts) + session_title += f" ({date_range_str})" + + # Check if session file needs regeneration + session_file_name = f"session-{session_id}{suffix}.{ext}" + session_file_path = output_dir / session_file_name + + # Use incremental regeneration: check per-session staleness via html_cache if cache_manager is not None and format == "html": - # Use message count from cache (pre-deduplication) to match - # the count used in is_html_stale() - if session_id in session_data: - session_message_count = session_data[session_id].message_count - else: - # Fallback: count from messages list (less accurate due to dedup) - session_message_count = sum( - 1 - for m in messages - if hasattr(m, "sessionId") - and getattr(m, "sessionId") == session_id + is_stale, _reason = cache_manager.is_html_stale( + session_file_name, session_id + ) + should_regenerate_session = ( + is_stale + or renderer.is_outdated(session_file_path) + or from_date is not None + or to_date is not None + or not session_file_path.exists() + ) + else: + # Fallback without cache or non-HTML formats + should_regenerate_session = ( + renderer.is_outdated(session_file_path) + or from_date is not None + or to_date is not None + or not session_file_path.exists() + or cache_was_updated + ) + + if should_regenerate_session: + # Generate session content. Under `--combined no` the + # combined file is never written, so the per-session + # back-link would 404 — suppress it. + session_content = renderer.generate_session( + messages, + session_id, + session_title, + cache_manager, + output_dir, + session_tree=session_tree, + suppress_combined_link=not write_combined, + ) + assert session_content is not None + # Write session file + # See issue #139: errors="replace" for lone-surrogate safety. + session_file_path.write_text( + session_content, encoding="utf-8", errors="replace" + ) + regenerated_count += 1 + + # Update html_cache to track this generation (HTML only) + if cache_manager is not None and format == "html": + # Use message count from cache (pre-deduplication) to match + # the count used in is_html_stale() + if session_id in session_data: + session_message_count = session_data[session_id].message_count + else: + # Fallback: count from messages list (less accurate due to dedup) + session_message_count = sum( + 1 + for m in messages + if hasattr(m, "sessionId") + and getattr(m, "sessionId") == session_id + ) + cache_manager.update_html_cache( + session_file_name, session_id, session_message_count ) - cache_manager.update_html_cache( - session_file_name, session_id, session_message_count + elif not silent: + print( + f"Session file {session_file_path.name} is current, skipping regeneration" ) - elif not silent: - print( - f"Session file {session_file_path.name} is current, skipping regeneration" - ) return regenerated_count diff --git a/test/test_cache_sqlite_integrity.py b/test/test_cache_sqlite_integrity.py index 2424d980..a58e3a30 100644 --- a/test/test_cache_sqlite_integrity.py +++ b/test/test_cache_sqlite_integrity.py @@ -2,6 +2,7 @@ """Comprehensive SQL-level integrity tests for SQLite cache.""" import json +import shutil import sqlite3 import threading import time @@ -701,6 +702,126 @@ def read_data(): assert all(results), "Not all reads succeeded" +class TestBatchConnectionReuse: + """Tests for the batch() connection-reuse context manager. + + batch() collapses the ~190 per-call connection opens of a full build + into one shared connection. The lifecycle guarantees are the risky part + (Windows file locks), so they are pinned here explicitly. + """ + + def _entry(self, session_id: str = "session-1") -> UserTranscriptEntry: + return UserTranscriptEntry( + type="user", + uuid="user-1", + timestamp="2024-01-01T10:00:00Z", + sessionId=session_id, + version="1.0.0", + parentUuid=None, + isSidechain=False, + userType="external", + cwd="/test", + message=UserMessageModel( + role="user", content=[TextContent(type="text", text="Test")] + ), + ) + + def test_default_opens_fresh_connection_per_call(self, cache_manager): + """Outside a batch the behaviour is unchanged: every _get_connection + opens (and closes) its own connection.""" + assert cache_manager._shared_conn is None + with cache_manager._get_connection() as c1: + pass + with cache_manager._get_connection() as c2: + pass + assert c1 is not c2 + # Each was closed on exit (default path). + with pytest.raises(sqlite3.ProgrammingError): + c1.execute("SELECT 1") + assert cache_manager._shared_conn is None + + def test_batch_reuses_single_connection(self, cache_manager): + """Inside a batch every _get_connection yields the same connection.""" + with cache_manager.batch(): + shared = cache_manager._shared_conn + assert shared is not None + with cache_manager._get_connection() as c1: + pass + with cache_manager._get_connection() as c2: + pass + assert c1 is shared and c2 is shared + # Reused connections are NOT closed when the inner `with` exits. + c1.execute("SELECT 1") + # Closed and cleared once the batch exits. + assert cache_manager._shared_conn is None + with pytest.raises(sqlite3.ProgrammingError): + shared.execute("SELECT 1") + + def test_batch_closes_connection_on_normal_exit_allows_rmtree(self, tmp_path): + """After a batch completes, the cache files are unlocked so the + containing directory can be removed — the Windows WinError 32 guard.""" + cache_dir = tmp_path / "proj" + cache_dir.mkdir() + db_path = tmp_path / "cache.db" + cm = CacheManager(cache_dir, "1.0.0", db_path=db_path) + + entry = self._entry() + jsonl = cache_dir / "s.jsonl" + jsonl.write_text(json.dumps(entry.model_dump()), encoding="utf-8") + with cm.batch(): + cm.save_cached_entries(jsonl, [entry]) + + # No lingering open handle: deleting the tree (and the db/-wal/-shm + # files) must not raise. On Windows an open connection here would + # raise PermissionError (WinError 32). + shutil.rmtree(tmp_path) + assert not tmp_path.exists() + + def test_batch_closes_connection_on_exception(self, cache_manager): + """A build that raises inside the batch must still release the shared + connection (finally), or a crashed build would leak the file lock.""" + captured = {} + with pytest.raises(ValueError): + with cache_manager.batch(): + captured["conn"] = cache_manager._shared_conn + raise ValueError("boom") + assert cache_manager._shared_conn is None + with pytest.raises(sqlite3.ProgrammingError): + captured["conn"].execute("SELECT 1") + + def test_nested_batch_is_noop_reuse(self, cache_manager): + """A nested batch reuses the outer connection and does NOT close it on + inner exit, so the converter loop can't double-open or close a + connection still in use by an enclosing scope.""" + with cache_manager.batch(): + outer = cache_manager._shared_conn + assert outer is not None + with cache_manager.batch(): + assert cache_manager._shared_conn is outer + # Inner exit must leave the outer connection open and active. + assert cache_manager._shared_conn is outer + outer.execute("SELECT 1") + # Only the outermost batch closes it. + assert cache_manager._shared_conn is None + with pytest.raises(sqlite3.ProgrammingError): + outer.execute("SELECT 1") + + def test_writes_within_batch_persist(self, tmp_path): + """Data written through the shared connection is committed and visible + after the batch — reuse must not drop the writes.""" + cache_dir = tmp_path / "proj" + cache_dir.mkdir() + cm = CacheManager(cache_dir, "1.0.0", db_path=tmp_path / "cache.db") + entry = self._entry() + jsonl = cache_dir / "s.jsonl" + jsonl.write_text(json.dumps(entry.model_dump()), encoding="utf-8") + with cm.batch(): + cm.save_cached_entries(jsonl, [entry]) + loaded = cm.load_cached_entries(jsonl) + assert loaded is not None + assert len(loaded) == 1 + + class TestLargeDatasetPerformance: """Tests for performance with large datasets.""" From aa081fbb064805fc4edf1aad93727cf04a5472d0 Mon Sep 17 00:00:00 2001 From: Christian Boos Date: Thu, 25 Jun 2026 11:29:14 +0200 Subject: [PATCH 3/5] docs(dev): document cache connection model (WAL/NORMAL + batch reuse) Record the as-built connection lifecycle in the cache deep-dive: connection-per-call default (Windows-safe, no lingering handle), synchronous=NORMAL durability tradeoff, and the batch() connection reuse the converter uses for build hotspots. Co-Authored-By: Claude Opus 4.8 (1M context) --- dev-docs/application_model.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dev-docs/application_model.md b/dev-docs/application_model.md index ef0f51c6..da5292a1 100644 --- a/dev-docs/application_model.md +++ b/dev-docs/application_model.md @@ -125,6 +125,16 @@ cache row, the session is reparsed. The schema-version row also invalidates the entire HTML cache when migrations bump the version, since rendered output may have changed even when source data hasn't. +Connections run in WAL mode with `synchronous=NORMAL` (durable across +app crashes; only a power/OS crash can lose the last commit — fine for a +regenerable cache). By default `_get_connection()` opens and closes a +connection per call, so no file handle lingers to block temp-dir cleanup +on Windows. A build issues ~190 such opens, which dominates cache-build +cost, so the converter wraps its hotspots (`ensure_fresh_cache`, the +per-file load loop, per-session generation) in `CacheManager.batch()`: +one shared connection reused for the scope and closed on exit (including +on exception). `batch()` nesting is a no-op reuse, so the wraps compose. + For the operations / recovery side (archived sessions, manual deletion, `cleanupPeriodDays`), see [`docs/restoring-archived-sessions.md`](../docs/restoring-archived-sessions.md). From d4871816271e43db17ecaa47e23fb493aea81a2c Mon Sep 17 00:00:00 2001 From: Christian Boos Date: Thu, 25 Jun 2026 11:43:39 +0200 Subject: [PATCH 4/5] perf(cache): init _shared_conn before _init_database (review nit) Harden the field ordering so _shared_conn always exists before any _get_connection() could run, even if migrations are ever routed through it. Currently safe (migrations use the standalone run_migrations), but this removes a latent AttributeError footgun. Per monk review. Co-Authored-By: Claude Opus 4.8 (1M context) --- claude_code_log/cache.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/claude_code_log/cache.py b/claude_code_log/cache.py index 1d1299b1..77508d55 100644 --- a/claude_code_log/cache.py +++ b/claude_code_log/cache.py @@ -306,13 +306,16 @@ def __init__( else: self.db_path = get_cache_db_path(project_path.parent) - # Initialise database and ensure project exists - self._init_database() - self._project_id: Optional[int] = None # When inside a batch() scope this holds the single shared connection # reused by every _get_connection() call; None means the default - # open-a-connection-per-call behaviour. + # open-a-connection-per-call behaviour. Set before _init_database() so + # the field always exists before any _get_connection() could run, even + # if migrations are ever routed through it. self._shared_conn: Optional[sqlite3.Connection] = None + + # Initialise database and ensure project exists + self._init_database() + self._project_id: Optional[int] = None self._ensure_project_exists() def _configure_connection(self, conn: sqlite3.Connection) -> None: From 51adead2886a08240407d1fb757bd107993a544c Mon Sep 17 00:00:00 2001 From: Christian Boos Date: Mon, 29 Jun 2026 23:04:01 +0200 Subject: [PATCH 5/5] fix(cache): close connection if PRAGMA setup fails _get_connection() and batch() opened the SQLite handle before applying pragmas, so a failure in _configure_connection() would leak the connection and could lock the .db/.db-wal/.db-shm files. Add _open_configured_connection(), which closes the handle before re-raising if setup fails, and use it in both paths. batch() now only publishes to _shared_conn after setup succeeds, so a failed configure never leaves a half-initialised handle for reuse. Add a test asserting the connection is closed (and _shared_conn stays None) when _configure_connection raises, in both paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- claude_code_log/cache.py | 25 ++++++++++++--- test/test_cache_sqlite_integrity.py | 49 +++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/claude_code_log/cache.py b/claude_code_log/cache.py index 77508d55..c865d922 100644 --- a/claude_code_log/cache.py +++ b/claude_code_log/cache.py @@ -330,6 +330,22 @@ def _configure_connection(self, conn: sqlite3.Connection) -> None: # residual risk is acceptable. conn.execute("PRAGMA synchronous = NORMAL") + def _open_configured_connection(self) -> sqlite3.Connection: + """Open a connection and apply pragmas, closing it if setup fails. + + If a PRAGMA in ``_configure_connection`` raises, the just-opened + handle is closed before re-raising so it can't leak and lock the + .db/.db-wal/.db-shm files — the exact failure mode the connection + lifecycle elsewhere is careful to avoid (Windows WinError 32). + """ + conn = sqlite3.connect(self.db_path, timeout=30.0) + try: + self._configure_connection(conn) + except BaseException: + conn.close() + raise + return conn + @contextmanager def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: """Get a database connection with proper settings. @@ -343,8 +359,7 @@ def _get_connection(self) -> Generator[sqlite3.Connection, None, None]: yield self._shared_conn return - conn = sqlite3.connect(self.db_path, timeout=30.0) - self._configure_connection(conn) + conn = self._open_configured_connection() try: yield conn finally: @@ -376,8 +391,10 @@ def batch(self) -> Generator[None, None, None]: yield return - conn = sqlite3.connect(self.db_path, timeout=30.0) - self._configure_connection(conn) + # Open+configure first; only publish to _shared_conn once setup has + # succeeded, so a failed PRAGMA never leaves a half-initialised (and + # now-closed) handle assigned for other calls to reuse. + conn = self._open_configured_connection() self._shared_conn = conn try: yield diff --git a/test/test_cache_sqlite_integrity.py b/test/test_cache_sqlite_integrity.py index a58e3a30..c1961a47 100644 --- a/test/test_cache_sqlite_integrity.py +++ b/test/test_cache_sqlite_integrity.py @@ -821,6 +821,55 @@ def test_writes_within_batch_persist(self, tmp_path): assert loaded is not None assert len(loaded) == 1 + def test_failed_configure_closes_connection(self, tmp_path, monkeypatch): + """If a PRAGMA fails during connection setup, the just-opened handle + must be closed (not leaked), in BOTH the per-call and batch() paths — + otherwise the failed connection would lock the cache files. batch() + must also not publish a half-initialised handle to _shared_conn. + """ + import claude_code_log.cache as cachemod + + cache_dir = tmp_path / "proj" + cache_dir.mkdir() + cm = CacheManager(cache_dir, "1.0.0", db_path=tmp_path / "cache.db") + + # Track every connection opened after setup so we can assert it closed. + opened: list[sqlite3.Connection] = [] + real_connect = cachemod.sqlite3.connect + + def tracking_connect(*args, **kwargs): + conn = real_connect(*args, **kwargs) + opened.append(conn) + return conn + + def boom(_conn): + raise RuntimeError("pragma boom") + + monkeypatch.setattr(cachemod.sqlite3, "connect", tracking_connect) + monkeypatch.setattr(cm, "_configure_connection", boom) + + # Per-call path: configure fails -> connection opened then closed. + with pytest.raises(RuntimeError): + with cm._get_connection(): + pass + assert opened, "expected a connection to have been opened" + with pytest.raises(sqlite3.ProgrammingError): + opened[-1].execute("SELECT 1") + + # batch() path: configure fails -> connection closed AND _shared_conn + # never left pointing at the dead handle. + with pytest.raises(RuntimeError): + with cm.batch(): + pass + assert cm._shared_conn is None + with pytest.raises(sqlite3.ProgrammingError): + opened[-1].execute("SELECT 1") + + # No leaked OS handle: the cache dir is removable (Windows WinError 32). + monkeypatch.undo() + shutil.rmtree(tmp_path) + assert not tmp_path.exists() + class TestLargeDatasetPerformance: """Tests for performance with large datasets."""