chore(tests): wrap sqlite3 connections in contextlib.closing#1430
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the test suite against leaked sqlite3 connections by wrapping connection lifetimes with contextlib.closing(...), ensuring connections are closed even when assertions fail—reducing ResourceWarning: unclosed database noise and potential SQLite lock-related CI hangs.
Changes:
- Wrap multiple
sqlite3.connect(...)usages inwith closing(...)blocks across affected tests. - Deduplicate in-test
import sqlite3statements intests/test_sources.pyinto a single top-level import. - Apply the same connection cleanup approach to a helper in
tests/test_repair.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/test_sources.py |
Adds top-level sqlite3/closing imports and wraps KnowledgeGraph schema/provenance sqlite reads/writes with closing(...). |
tests/test_repair.py |
Imports closing and wraps the _seed_poisoned_max_seq_id sqlite setup in closing(...) for failure-safe cleanup. |
tests/test_backends.py |
Imports closing and wraps sqlite setup/verification connections in blob seq_id repair tests to prevent leaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors several test files, including tests/test_backends.py, tests/test_repair.py, and tests/test_sources.py, to improve database connection management. It replaces manual sqlite3 connection closing with the contextlib.closing context manager to ensure resources are properly released even if exceptions occur. Additionally, redundant local imports of sqlite3 were moved to the module level in tests/test_sources.py. I have no feedback to provide.
4 tasks
Several tests opened sqlite3 connections without try/finally or context-manager cleanup, relying on a flat conn.close() after the work. Any assertion failure or exception between connect and close leaked the connection until GC, producing ``ResourceWarning: unclosed database`` in CI logs. On Python 3.13 / macOS the ResourceWarning isn't just noise: a leaked connection can hold a SQLite advisory lock long enough for later test setup to block on it, which appears to be the cause of recent intermittent CI hangs on those two runners. Wrap each affected ``conn = sqlite3.connect(...)`` block in ``contextlib.closing(...)`` so cleanup runs on the failure path too. Mirrors the try/finally pattern already used in production code (searcher.py, repair.py, backends/chroma.py). No behavior change — same operations, same assertions, just deterministic cleanup. All 162 affected tests pass locally.
3cf555a to
df5ca11
Compare
This was referenced May 10, 2026
Merged
Merged
arnoldwender
pushed a commit
to arnoldwender/mempalace
that referenced
this pull request
May 10, 2026
Bumps version 3.3.4 → 3.3.5 across pyproject.toml, version.py, plugin manifests, README badge, and uv.lock. Flips CHANGELOG.md from ``[3.3.5] — unreleased`` to ``[3.3.5] — 2026-05-09`` and adds entries for the four PRs that landed after the bug-fix block was authored: - Bug Fixes: MemPalace#1396 (tool_search retry on transient HNSW flush) - Documentation: MemPalace#1385 (CONTRIBUTING git-identity guidance, closes MemPalace#1317) - Internal: MemPalace#1431 (test multiprocessing fork → spawn) - Internal: MemPalace#1430 (test sqlite connection lifecycle via contextlib.closing) The four open issues remaining on the v3.3.5 milestone (MemPalace#1266, MemPalace#1253, MemPalace#1092, MemPalace#1082) have been moved to v3.4 — they form the concurrent-writer / HNSW corruption cluster that needs deeper work than this cycle could absorb.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several tests open
sqlite3connections without try/finally or context-manager cleanup, relying on a flatconn.close()after the work. Any assertion failure or exception betweenconnectandcloseleaks the connection until GC, producingResourceWarning: unclosed databasein CI logs.On Python 3.13 / macOS the warning isn't just noise: a leaked connection can hold a SQLite advisory lock long enough for later test setup to block on it, which appears to be the cause of recent intermittent CI hangs on those two runners specifically (e.g. the 33-minute pytest hang on #1396's first attempt).
Change
Wrap each naked
conn = sqlite3.connect(...)block incontextlib.closing(...)so cleanup runs on the failure path too. Mirrors thetry/finallypattern already used in production code (searcher.py,repair.py,backends/chroma.py).Files touched:
tests/test_backends.py— 12 sites in the_fix_blob_seq_idstest clustertests/test_sources.py— 4 sites in the KnowledgeGraph schema/migration tests; also dedups threeimport sqlite3calls inside test bodies into one top-level importtests/test_repair.py— 1 site in_seed_poisoned_max_seq_idSkipped:
test_hnsw_capacity.py:50/365/403andtest_repair.py:1362already usetry/finally— left as-istest_cli.pyandtest_repair.pyone-linersqlite3.connect(...).close()patterns are already safe (single expression, no operations between)Out of scope (worth a follow-up)
KnowledgeGraph(...)instances in test bodies (test_sources.py,test_fact_checker.py,test_knowledge_graph_extra.py) hold their own sqlite connection onself._connection. Most already usetry/finally: kg.close(), but a couple don't. Separate cleanup pass.Test plan
uv run pytest tests/test_backends.py tests/test_sources.py tests/test_repair.py -x --no-cov -q→ 162 passedruff check(pinnedruff>=0.4.0,<0.5) → cleanruff format --check(pinnedruff>=0.4.0,<0.5) → clean