Repository navigation
[fix] - Close the memory DB handle on setup failure, stamp the schema once per file, cache compiled memory patterns - #1649
Conversation
… once per file, cache compiled memory patterns memory.store.connect() leaked the SQLite handle when a PRAGMA or the schema step raised (corrupt or locked DB), and re-ran all eleven CREATE ... IF NOT EXISTS statements on every call even though every read helper opens a fresh connection. connect() now closes on any setup failure, and _ensure_schema skips the DDL when PRAGMA user_version already carries _SCHEMA_VERSION -- the stamp lives in the DB file, so a deleted/recreated DB gets its schema again with no process-wide cache to go stale. memory.policy recompiled every pattern on each scan_content call and re-logged a broken banned_patterns entry's warning on every memory write. Compiled sets are now lru_cached per distinct (base, banned_patterns) pair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJXTdWRbDpkynPYbpL2pxd
|
PR is a complete fill of |
CodeQL flagged the two new tests for importing memory.store with both 'import' and 'import from'. Take _SCHEMA_VERSION from the existing from-import and patch sqlite3.connect directly (memory.store.sqlite3 is the same module). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJXTdWRbDpkynPYbpL2pxd
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e591278633
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if conn.execute("PRAGMA user_version").fetchone()[0] == _SCHEMA_VERSION: | ||
| return |
There was a problem hiding this comment.
Reject newer schemas instead of downgrading them
When an operator rolls back to this version after a later release has stamped the database with a higher user_version, the equality check treats that newer schema as needing initialization, runs the older DDL, and then overwrites its version with 1. This destroys the migration marker and can cause a later upgrade to rerun migrations against an already-newer schema. Read the version once and reject versions greater than _SCHEMA_VERSION rather than passing them through the initialization path.
Useful? React with 👍 / 👎.
Proposed changes
Three fixes in the optional memory subsystem (
memory/). The/CyClaw-Optimizescan found them, and this PR exists at the owner's request.memory.store.connect(). Aftersqlite3.connect()succeeds, the PRAGMAs and_ensure_schemacan still raise. A corrupt file givesfile is not a database, and a locked DB can fail at the WAL switch. In either case the open connection was never closed, so each failing request leaked one SQLite handle until GC.try/except BaseException: conn.close(); raise.search_facts_fts, which runs per query when fusion is on, andstage_episode, which runs per query when episodes are on. Each of those connections re-ran all elevenCREATE … IF NOT EXISTSstatements, plus the implicit COMMIT thatexecutescriptissues._ensure_schemastampsPRAGMA user_version = _SCHEMA_VERSIONafter the DDL and skips the DDL when the stamp is already present.user_version = 0and gets its schema again, and pytest's manytmp_pathDBs need no cache resets.memory.policy.scan_content. Every call rebuilt the full ENFORCED/OWASP list plus the configbanned_patterns: once per propose, and up to twice per apply. Abanned_patternsentry that failed to compile also re-logged its warning on every memory write.lru_cached (maxsize 8), keyed on the distinct(base, banned_patterns)tuple. This is the same compile-once shape asutils/sanitizer.py. Non-str config entries are keyed asNoneand skipped, and they keep their config-list index for the warning.Invariant / Governance Impact: none of the six invariants is touched.
gate_memory.pystill lazy-importsmemoryinside its handlers.reasongate and the enforced scan on apply are untouched.check_invariants.py: 46 passed, 0 failed.Types of changes
Scope: optional memory subsystem (
memory/store.py,memory/policy.py) plus two test files.memory.enabledshipsfalse.Benefits / why
Measured with a scratch probe, 500 calls per function,
mainvs this branch:mainconnect()search_facts_fts()scan_content()Risks to monitor
user_version = 1. The nextconnect()will not recreate it, whereas the old per-connectIF NOT EXISTSDDL silently did. Deleting the DB file still self-heals, because a new file starts at version 0. If needed,PRAGMA user_version = 0forces a re-run._SCHEMA_VERSION. The code comment says so, and bumping it is the only way an existing DB picks up new DDL. That is no worse than today:CREATE TABLE IF NOT EXISTSnever altered an existing table either.user_version. Nothing else in the repo reads or writes it (grepped).banned_patternslist compiles fresh; a reverted list reuses its earlier entry. Compiledre.Patternobjects are immutable and thread-safe.tests/test_memory_policy.pygains an autousecache_clearfixture so its warning assertions never depend on test order.tests/test_memory_store.py:tests/test_memory_policy.py:Checklist
docs/THREAT_MODEL.md,INVARIANTS.mdandSECURITY.md(and any relevant Phase docs)GROK_API_KEY=dummy pytest tests/ -q --tb=short, andbash .claude/skills/CyClaw-Sandbox/verify.shfor core RAG/agentic paths) and passes with no regressions. Not run locally, per the CLAUDE.md §5 verification policy; CI runs the suites.Further comments
Verification I ran directly, following CLAUDE.md §5:
connect(),search_facts_fts()andscan_content()against a temp DB.tests/test_memory_store.py(26 passed) andtests/test_memory_policy.py(12 passed). That is one more than §5's one-file guideline; the change spans two modules, each with its own test file.store.pyreverted tomain, both new store tests fail:assert [] == [True]: the handle was never closed;policy.pyreverted, the policy test file cannot import_compile_cached.ruff check --select E,F,I,B,C4,UP,S --ignore E501is clean.ruff formatdrift in these files predates this PR and sits only on lines this diff does not touch.mypy --strict --explicit-package-basesonmemory/policy.pyandmemory/store.pyreports 5 errors, the same 5 as onmain, all outside the changed lines.check_invariants.pygives 46/0.Suggested merge order of open PRs
Trial merges verified (
git merge --no-ff, all three branches in sequence onto a throwaway checkout) against main @ 6396152 on 2026-10-13. The three PRs change disjoint sets of files.#1647 · Blocked ⛔ (draft, CI pending)
Impact:
agentic/harness_optimizer/only; independent.Action: Merge first (lowest number). Trial merge clean vs main.
#1648 · Blocked ⛔ (draft, CI pending)
Impact:
agentic/fsconnect/indexer.py+ async/filters.pycomment; independent.Action: Merge second; no shared files.
#1649 (this PR) · Blocked ⛔ (draft, CI pending)
Impact:
memory/store.py,memory/policy.py+ their tests; independent.Action: Merge third; no shared files with #1647 or #1648.
ELI5
The memory store is a small database CyClaw can use to remember facts. Every time CyClaw looked something up, it opened the database and re-ran the full "build all the tables if they're missing" checklist, even though the tables were built long ago. Now the database carries a sticky note (
PRAGMA user_version) that says "tables built, version 1". If the note is there, CyClaw skips the checklist. A brand-new or replaced database file has no note, so it still gets built.There are two smaller fixes. If the database file was broken,
connect()gave up but forgot to close what it had opened; it now always closes it. And the injection-pattern list used for memory writes was re-compiled from scratch on every write, repeating any "this pattern is broken" warning each time. It is now compiled once per distinct list and kept.What could still go wrong: if someone hand-deletes one table from a database that already has the sticky note, CyClaw will no longer quietly rebuild it. Deleting the whole file still works. Anyone who changes the table definitions must bump
_SCHEMA_VERSION. The speedups are real but small, tens of microseconds; the bigger wins are no leaked handles and no log flood. I ran two targeted test files, not the full suite; CI runs the rest.🤖 Generated with Claude Code
https://claude.ai/code/session_01BJXTdWRbDpkynPYbpL2pxd
Generated by Claude Code
Last updated: 2026-10-13 09:44 ET