Skip to content

fix: make knowledge_graph.py SQLite connections exception-safe#44

Closed
adv3nt3 wants to merge 1 commit intoMemPalace:mainfrom
adv3nt3:fix/knowledge-graph-connection-safety
Closed

fix: make knowledge_graph.py SQLite connections exception-safe#44
adv3nt3 wants to merge 1 commit intoMemPalace:mainfrom
adv3nt3:fix/knowledge-graph-connection-safety

Conversation

@adv3nt3
Copy link
Copy Markdown
Contributor

@adv3nt3 adv3nt3 commented Apr 7, 2026

Summary

  • Wrap all 8 methods in KnowledgeGraph that open SQLite connections with contextlib.closing() to guarantee conn.close() is called even if execute()/commit()/fetchall() raises. Previously, connections would leak on exception.
  • Add transaction context manager (with conn:) to write methods (add_entity, add_triple, invalidate) for auto-commit on success and auto-rollback on exception.
  • Read methods (query_entity, query_relationship, timeline, stats) use closing() only.
  • _init_db keeps manual commit() since executescript has its own transaction semantics.
  • Simplifies the add_triple early-return path — no more redundant manual conn.close().

Test plan

  • ruff check mempalace/knowledge_graph.py passes clean
  • ruff format --check mempalace/knowledge_graph.py already formatted
  • python3 -m py_compile mempalace/knowledge_graph.py compiles OK
  • Pattern validated against CPython sqlite3 docs via Context7
  • Pyright reports 0 new diagnostics (10 pre-existing Optional type warnings unchanged)

@adv3nt3 adv3nt3 force-pushed the fix/knowledge-graph-connection-safety branch from 5dbe6ce to dc11fbc Compare April 7, 2026 21:32
@adv3nt3
Copy link
Copy Markdown
Contributor Author

adv3nt3 commented Apr 7, 2026

The CI failure is not from this PR — both failing tests are in test_dialect.py, unrelated to knowledge_graph.py:

  1. TestCompressionStats::test_stats — expects stats["ratio"] key that was removed when PR fix: honest AAAK stats — word-based token estimator, lossy labels #147 changed the compression stats API
  2. TestCompressionStats::test_count_tokens — expects the old len(text)//3 heuristic, but PR fix: honest AAAK stats — word-based token estimator, lossy labels #147 switched to word-based token counting

This is a pre-existing test-vs-code mismatch on main. PR #150 ("test(dialect): update assertions for new honest-stats API") is the fix for exactly these failures.

All 97 other tests pass, including the knowledge graph tests that exercise the code this PR changes.

Wrap all 8 methods that open SQLite connections with
contextlib.closing() to guarantee conn.close() is called even if
execute()/commit()/fetchall() raises. Previously, connections would
leak on exception since close() was called manually after operations.

Add transaction context manager (with conn:) to write methods
(add_entity, add_triple, invalidate) for auto-commit on success and
auto-rollback on exception. Read methods use closing() only.
_init_db keeps manual commit since executescript has its own
transaction semantics.
@adv3nt3 adv3nt3 force-pushed the fix/knowledge-graph-connection-safety branch from dc11fbc to 40a979a Compare April 7, 2026 23:29
@adv3nt3
Copy link
Copy Markdown
Contributor Author

adv3nt3 commented Apr 9, 2026

@bensig @milla-jovovich Closing this, the upstream changes to knowledge_graph.py solve both problems this PR addressed:

  1. Connection leaks — now handled by a persistent self._connection with an explicit close() method, instead of per-call connections that could leak on exception
  2. Transaction safetywith conn: blocks are now used for write operations, providing auto-commit/rollback

The persistent connection + sqlite3.Row approach is cleaner than the contextlib.closing() pattern this PR used. No need to merge this anymore.

@adv3nt3 adv3nt3 closed this Apr 9, 2026
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.

1 participant