From 5ef96f024c5a764fed3971c429bab214c1428bdb Mon Sep 17 00:00:00 2001 From: hotragn Date: Tue, 18 Aug 2026 22:12:18 -0400 Subject: [PATCH] refactor(index): close IndexStore connections deterministically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IndexStore` is the only store that does not close its SQLite connections. All eight of its methods use `with connect_index(path) as connection:`, and `sqlite3.Connection.__exit__` scopes a *transaction* — it commits or rolls back and returns, leaving the connection open for the interpreter to reclaim. Every other store routes through `codealmanac.database.open_local_database`, which closes in a `finally` and carries a comment explaining why it exists: sqlite3.Connection's own context manager scopes a transaction and never closes; long agent runs record hundreds of events and leaked descriptors until sqlite3.connect failed with "unable to open database file". `IndexStore` bypasses that helper, so it never got the fix. This matters more here than elsewhere because every query command reindexes implicitly, making this the hot path rather than a rare one. Adds `open_index`, which nests `with connection:` inside a `try/finally` that closes. Nesting rather than replacing is deliberate: none of the eight callers commits explicitly, so they depend entirely on `__exit__` for durability. Dropping it for a plain close would silently stop persisting the index. Commit-on-success and rollback-on-error are therefore unchanged; the only difference is that the handle is released when the block exits. `connect_index` is kept as-is, both because `open_index` builds on it and because `tests/test_architecture.py` pins its presence in this module. The regression test mirrors the existing `test_store_connections_close_after_ every_use` for `RepositoryStore`, and fails on the previous code. Scope note: I could not reproduce descriptor exhaustion from this path, and psutil's open-file reporting on Windows was not reliable enough to measure it, so this is deterministic resource hygiene and consistency with the pattern the repo already chose everywhere else — not a fix for an observed crash. --- src/codealmanac/services/index/schema.py | 17 +++++++++++ src/codealmanac/services/index/store.py | 18 ++++++------ tests/test_database.py | 36 ++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/codealmanac/services/index/schema.py b/src/codealmanac/services/index/schema.py index 448cce49..7d6c9c27 100644 --- a/src/codealmanac/services/index/schema.py +++ b/src/codealmanac/services/index/schema.py @@ -1,3 +1,5 @@ +from collections.abc import Iterator +from contextlib import contextmanager from pathlib import Path from codealmanac.database import ( @@ -130,3 +132,18 @@ def connect_index(path: Path) -> SQLiteConnection: connection = connect_sqlite(path) apply_migrations(connection, INDEX_MIGRATIONS) return connection + + +@contextmanager +def open_index(path: Path) -> Iterator[SQLiteConnection]: + # `with connection:` scopes a transaction and never closes, so callers using + # it alone left the handle for the interpreter to reclaim instead of closing + # it deterministically. `open_local_database` exists for the same reason and + # records where that ended up once already. Nesting `with connection:` keeps + # the commit-on-success and rollback-on-error behaviour exactly as it was. + connection = connect_index(path) + try: + with connection: + yield connection + finally: + connection.close() diff --git a/src/codealmanac/services/index/store.py b/src/codealmanac/services/index/store.py index e015d105..716a7d30 100644 --- a/src/codealmanac/services/index/store.py +++ b/src/codealmanac/services/index/store.py @@ -15,7 +15,7 @@ stored_signature, ) from codealmanac.services.index.requests import SearchIndexRequest -from codealmanac.services.index.schema import connect_index, index_db_path +from codealmanac.services.index.schema import index_db_path, open_index from codealmanac.services.index.sources import load_index_sources from codealmanac.services.index.views import ( build_health_report, @@ -33,7 +33,7 @@ def refresh(self, almanac_path: Path, runtime_path: Path) -> IndexRefreshResult: require_initialized_almanac_root(almanac_path) sources = load_index_sources(almanac_path) db_path = index_db_path(runtime_path) - with connect_index(db_path) as connection: + with open_index(db_path) as connection: if stored_signature(connection) == sources.signature: return IndexRefreshResult( changed=0, @@ -55,7 +55,7 @@ def rebuild(self, almanac_path: Path, runtime_path: Path) -> IndexRefreshResult: require_initialized_almanac_root(almanac_path) sources = load_index_sources(almanac_path) db_path = index_db_path(runtime_path) - with connect_index(db_path) as connection: + with open_index(db_path) as connection: replace_documents(connection, sources) return IndexRefreshResult( changed=len(sources.documents), @@ -72,12 +72,12 @@ def search( request: SearchIndexRequest, ) -> tuple[SearchPageResult, ...]: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return search_pages(connection, request) def counts(self, almanac_path: Path, runtime_path: Path) -> IndexCounts: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return index_counts(connection) def get_page( @@ -87,7 +87,7 @@ def get_page( slug: str, ) -> PageView | None: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return get_page_view(connection, slug) def list_topics( @@ -96,7 +96,7 @@ def list_topics( runtime_path: Path, ) -> tuple[TopicSummary, ...]: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return list_topic_summaries(connection) def get_topic( @@ -107,7 +107,7 @@ def get_topic( include_descendants: bool, ) -> TopicDetail | None: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return get_topic_detail(connection, slug, include_descendants) def health_report( @@ -118,7 +118,7 @@ def health_report( registered_wikis: set[str], ) -> HealthReport: require_initialized_almanac_root(almanac_path) - with connect_index(index_db_path(runtime_path)) as connection: + with open_index(index_db_path(runtime_path)) as connection: return build_health_report( connection, repository_root, diff --git a/tests/test_database.py b/tests/test_database.py index 61f73711..77733cef 100644 --- a/tests/test_database.py +++ b/tests/test_database.py @@ -95,3 +95,39 @@ def tracking_connect(path: Path) -> sqlite3.Connection: for connection in opened: with pytest.raises(sqlite3.ProgrammingError): connection.execute("SELECT 1") + + +def test_index_store_connections_close_after_every_use(tmp_path: Path, monkeypatch): + from codealmanac.services.index import schema + from codealmanac.services.index.store import IndexStore + + almanac_path = tmp_path / "repo" / "almanac" + almanac_path.mkdir(parents=True) + (almanac_path / "README.md").write_text( + "---\ntopics: [concepts]\n---\n# Wiki\n\nRoot page.\n", + encoding="utf-8", + ) + (almanac_path / "topics.yaml").write_text("topics: []\n", encoding="utf-8") + runtime_path = tmp_path / "runtime" + + opened: list[sqlite3.Connection] = [] + real_connect = schema.connect_sqlite + + def tracking_connect(path: Path) -> sqlite3.Connection: + connection = real_connect(path) + opened.append(connection) + return connection + + monkeypatch.setattr(schema, "connect_sqlite", tracking_connect) + store = IndexStore() + + for _ in range(50): + store.refresh(almanac_path, runtime_path) + + # Every query command reindexes implicitly, so this is the hot path rather + # than a rare one. IndexStore was the only store still relying on sqlite3's + # transaction-only context manager instead of closing. + assert len(opened) == 50 + for connection in opened: + with pytest.raises(sqlite3.ProgrammingError): + connection.execute("SELECT 1")