Skip to content

[fix] - Close the memory DB handle on setup failure, stamp the schema once per file, cache compiled memory patterns - #1649

Merged
cgfixit merged 2 commits into
mainfrom
claude/cyclaw-optimize-memory-store
Oct 13, 2026
Merged

cgfixit merged 2 commits into
mainfrom
claude/cyclaw-optimize-memory-store

Conversation

@cgfixit

@cgfixit cgfixit commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

Proposed changes

Three fixes in the optional memory subsystem (memory/). The /CyClaw-Optimize scan found them, and this PR exists at the owner's request.

  1. Handle leak in memory.store.connect(). After sqlite3.connect() succeeds, the PRAGMAs and _ensure_schema can still raise. A corrupt file gives file 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.
    • Fix: setup now runs inside try/except BaseException: conn.close(); raise.
  2. Schema DDL on every connection. Every read helper opens a fresh connection. That includes search_facts_fts, which runs per query when fusion is on, and stage_episode, which runs per query when episodes are on. Each of those connections re-ran all eleven CREATE … IF NOT EXISTS statements, plus the implicit COMMIT that executescript issues.
    • Fix: _ensure_schema stamps PRAGMA user_version = _SCHEMA_VERSION after the DDL and skips the DDL when the stamp is already present.
    • The stamp lives in the DB file on purpose, not in a process-wide "schema done" cache. A deleted or recreated DB reads user_version = 0 and gets its schema again, and pytest's many tmp_path DBs need no cache resets.
  3. Pattern recompiling in memory.policy.scan_content. Every call rebuilt the full ENFORCED/OWASP list plus the config banned_patterns: once per propose, and up to twice per apply. A banned_patterns entry that failed to compile also re-logged its warning on every memory write.
    • Fix: compiled sets are lru_cached (maxsize 8), keyed on the distinct (base, banned_patterns) tuple. This is the same compile-once shape as utils/sanitizer.py. Non-str config entries are keyed as None and skipped, and they keep their config-list index for the warning.

Invariant / Governance Impact: none of the six invariants is touched.

  • No core-six file changes. gate_memory.py still lazy-imports memory inside its handlers.
  • The injection scan's pattern sets and match logic are unchanged; only when they are compiled changed. The ENFORCED-vs-OWASP choice and the three-probe match (raw, literal-fold, separator-fold) are identical.
  • The propose/apply reason gate and the enforced scan on apply are untouched.
  • check_invariants.py: 46 passed, 0 failed.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Security (auth, privacy, injection, egress, redaction, or secrets)
  • Invariant / Governance refinement (use this for changes that strengthen or evolve the 6 invariants, I6 isolation, or harness phases)
  • Dependencies (added, removed, or bumped)
  • CI / infrastructure (workflows, hooks, templates, or checkers)
  • Refactor or tests (behavior-preserving, no new capability)
  • Documentation update
  • Other (none of the boxes above fit)

Scope: optional memory subsystem (memory/store.py, memory/policy.py) plus two test files. memory.enabled ships false.

Benefits / why

Measured with a scratch probe, 500 calls per function, main vs this branch:

Call main This branch
connect() 464 µs 370 µs (about 20% less)
search_facts_fts() 578 µs 529 µs
scan_content() 85 µs 38–43 µs (about 2× faster)
Warnings from one broken pattern over 500 scans 500 1
  • In absolute terms the latency gain is small. Memory ships off, and when it is on, these calls sit next to an LLM call that takes seconds.
  • The concrete wins are no leaked handles on a corrupt DB, and one compile warning instead of a log flood.

Risks to monitor

  • A stamped DB no longer self-heals a dropped table. Suppose someone hand-drops a table from a DB that already carries user_version = 1. The next connect() will not recreate it, whereas the old per-connect IF NOT EXISTS DDL silently did. Deleting the DB file still self-heals, because a new file starts at version 0. If needed, PRAGMA user_version = 0 forces a re-run.
  • Schema changes must bump _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 EXISTS never altered an existing table either.
  • This module now owns user_version. Nothing else in the repo reads or writes it (grepped).
  • Cache lifetime: the pattern cache is process-wide and keyed by the pattern list itself. A changed banned_patterns list compiles fresh; a reverted list reuses its earlier entry. Compiled re.Pattern objects are immutable and thread-safe.
  • A broken pattern now warns once per distinct list per process, not once per write. tests/test_memory_policy.py gains an autouse cache_clear fixture so its warning assertions never depend on test order.
  • Detection: five new tests.
    • tests/test_memory_store.py:
      • the handle closes on a corrupt-file failure;
      • the DDL script runs once per stamped file, and again for a deleted/recreated DB.
    • tests/test_memory_policy.py:
      • one warning and four cache hits over five scans;
      • a changed list is not served a stale compiled set;
      • non-str entries are skipped.

Checklist

  • I have read docs/THREAT_MODEL.md, INVARIANTS.md and SECURITY.md (and any relevant Phase docs)
  • This change preserves all 6 security invariants and I6 module isolation (explicit evidence or invariant matrix included for core changes)
  • Full sandbox validation has been run (GROK_API_KEY=dummy pytest tests/ -q --tb=short, and bash .claude/skills/CyClaw-Sandbox/verify.sh for core RAG/agentic paths) and passes with no regressions. Not run locally, per the CLAUDE.md §5 verification policy; CI runs the suites.
  • No new external network dependencies or mandatory online LLM assumptions were introduced without explicit justification + offline fallback path
  • For any agentic/fsconnect change: two-phase audit, quota enforcement, governed delete/trash, and write guards have been verified (n/a)
  • Relevant architecture docs or threat model notes have been updated if core behavior or topology changed (n/a)
  • Commit messages follow the title prefix convention above
  • For large or complex changes: before/after invariant matrix + sandbox evidence is included in "Further comments" or linked (n/a: small change)

Further comments

Verification I ran directly, following CLAUDE.md §5:

  • Code probe: the timing table above, from a probe that drives the real connect(), search_facts_fts() and scan_content() against a temp DB.
  • Tests: I ran two targeted files: tests/test_memory_store.py (26 passed) and tests/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.
    • With store.py reverted to main, both new store tests fail:
      • assert [] == [True]: the handle was never closed;
      • "an already-stamped DB must not re-run the schema DDL".
    • With policy.py reverted, the policy test file cannot import _compile_cached.
  • Lint:
    • ruff check --select E,F,I,B,C4,UP,S --ignore E501 is clean.
    • ruff format drift in these files predates this PR and sits only on lines this diff does not touch.
    • mypy --strict --explicit-package-bases on memory/policy.py and memory/store.py reports 5 errors, the same 5 as on main, all outside the changed lines.
  • Invariants: check_invariants.py gives 46/0.
  • Not run locally: the full suite and the coverage gate.

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 + a sync/filters.py comment; 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

… 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
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

PR is a complete fill of .github/PULL_REQUEST_TEMPLATE.md (every section, ticked boxes, merge order, ELI5 last, last-updated stamp, title, branch).

Comment thread tests/test_memory_store.py Fixed
Comment thread tests/test_memory_store.py Fixed
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
@cgfixit
cgfixit marked this pull request as ready for review October 13, 2026 14:37
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T14:39:58.257539Z e591278 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cgfixit
cgfixit merged commit 29f639a into main Oct 13, 2026
76 checks passed
@cgfixit
cgfixit deleted the claude/cyclaw-optimize-memory-store branch October 13, 2026 14:38

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread memory/store.py
Comment on lines +114 to +115
if conn.execute("PRAGMA user_version").fetchone()[0] == _SCHEMA_VERSION:
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

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.

3 participants