fix: handle Windows file-lock errors in test cleanup by claudlos · Pull Request #6 · MemPalace/mempalace

claudlos · 2026-04-07T06:05:41Z

What does this PR do?

Fixes PermissionError: [WinError 32] on Windows when running pytest tests/ -v. Two tests (test_convo_mining, test_project_mining) fail during temp directory cleanup because ChromaDB's PersistentClient holds file locks on HNSW data files (data_level0.bin, etc.) even after all assertions pass.

The fix:

Delete ChromaDB client/collection references and call gc.collect() before shutil.rmtree, releasing most file handles
Use shutil.rmtree(onerror=...) to gracefully handle any files still locked by the process, letting the OS clean temp dirs on its own schedule
Remove unused sys imports flagged by ruff

All actual test logic is unchanged — this only affects cleanup.

How to test

# On Windows (Python 3.9+)
pip install -e ".[dev]"
pytest tests/ -v

Before this fix: 2 FAILED, 7 passed (PermissionError on rmtree)
After this fix: 9 passed

Also works on Linux/macOS (gc.collect + onerror are no-ops when there are no file locks).

Checklist

Tests pass (python -m pytest tests/ -v)
No hardcoded paths
Linter passes (ruff check .)

ChromaDB's PersistentClient holds file locks on HNSW data files (data_level0.bin etc.) even after assertions pass. On Windows this causes shutil.rmtree to raise PermissionError during temp dir cleanup. Fix: - Delete ChromaDB client/collection references and gc.collect() before rmtree, releasing most handles. - Use shutil.rmtree(onerror=...) to gracefully skip any files still locked, letting the OS clean temp dirs on its own schedule. - Remove unused sys imports flagged by ruff. Tested on Windows 11 + Python 3.13 — all 9 tests now pass clean.

bensig · 2026-04-07T06:29:20Z

Hey, thanks for the PR! Tests pass which is great, but CI is failing on ruff format --check . — the two test files you touched need reformatting:

Would reformat: tests/test_convo_miner.py
Would reformat: tests/test_miner.py

Should just be a quick ruff format tests/ and push. Let me know if you have any questions.

- Add _force_rmtree() with gc.collect + onerror handler to all test files using tempfile.mkdtemp (test_convo_miner, test_miner, test_config) - Fix ruff format violations on test_convo_miner.py and test_miner.py - Fix test_env_override to save/restore old env var instead of deleting it - Use Path(tempfile.gettempdir()) instead of hardcoded /tmp for convomem_cache benchmark cache dir - All checks pass: ruff format --check + ruff check

Cherry-picked from upstream PR MemPalace#6 (claudlos). - Fix PermissionError on Windows from ChromaDB HNSW file locks - Add _force_rmtree() with gc.collect() + onerror handler - Fix hardcoded /tmp paths to use tempfile.gettempdir() - Tested on Windows 11 + Python 3.13 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

bensig · 2026-04-07T20:58:37Z

This has been open since day 1 and conflicts with main now. The Windows file-lock approach is addressed in the test refactoring from other contributors. Closing — thanks for the early contribution!

…comparison, tests Fixes all 10 issues from PR MemPalace#208 review: HIGH PRIORITY: - MemPalace#1: encode_aaak() now uses real Dialect.compress() for fair comparison - MemPalace#2: Removed aggressive stopword mappings (a, in, it, is, so, etc.) Added _STOP_WORDS set to filter function words before prime mapping - MemPalace#3: Added _NON_ENTITY_WORDS set (days, months, common sentence-starters) First-word-of-sentence only skipped if also a non-entity word MEDIUM PRIORITY: - MemPalace#4: Tokens now emitted in document order — entities and keywords interleaved based on position, preserving agent/patient distinction - MemPalace#5: Fixed compare_compression return type: dict[str, int | float] - MemPalace#6: Added 33 tests across 11 test classes covering encoding, entities, stopwords, word order, decode, validation, compression, and primes - MemPalace#7: Validator now checks structural syntax ([X]+[Y]+... format), balanced brackets, and empty qualifiers LOW PRIORITY: - MemPalace#8: Removed ambiguous mappings (like, right, close, sound) - MemPalace#9: Added integration documentation in class docstring - MemPalace#10: compression_ratio returns 0.0 for empty encodings All 134 tests pass (33 new + 101 existing). Zero regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@igorls

Port of upstream a4149ab (by @igorls, findings MemPalace#6, MemPalace#11, MemPalace#13) adapted to the PG+pgvector backend. 1. MCP/diary path ID is now hash(content), not content[:200]+timestamp. Same content → same deterministic ID. Eliminates TOCTOU races and stops identical content from piling up as distinct duplicate rows. Matches upstream's intent (findings MemPalace#6 TOCTOU, MemPalace#13 non-deterministic IDs). 2. INSERT ... ON CONFLICT (id) DO NOTHING → ON CONFLICT (id) DO UPDATE. Re-mining a modified file previously got the new content silently dropped because the slot ID matched an existing row (upstream finding MemPalace#11 HIGH — "add ignores updates"). True upsert: content, embedding, metadata, and filed_at are all refreshed. 3. add_drawer now always returns the drawer_id (both insert and update paths). The old "return None on conflict" signal implicitly encoded the stagnation bug and is no longer meaningful. Updated tests/test_db.py::test_mine_drawer_reupsert_by_source to assert the new upsert semantics instead of the stagnation assertion it replaced, and added test_mcp_add_drawer_is_idempotent_on_same_content to cover the deterministic-content-ID path. The mtime-based file_already_mined half of bf88daa is in a follow-up commit. Co-Authored-By: Igor Lins e Silva <4753812+igorls@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@igorls

Addresses review items MemPalace#5 and MemPalace#6 from @igorls: 1. Extract core sync logic from cmd_sync (~200 lines) into mempalace/sync.py as sync_palace(...) returning a SyncReport dataclass. cmd_sync is now a thin CLI wrapper. Makes sync callable from MCP tools, tests, and future change-detection features (PII Guard, KG sync). 2. Replace direct chromadb.PersistentClient calls in _force_clean and cmd_sync with ChromaBackend.get_collection. All storage access now goes through the backend abstraction. _force_clean is also now a thin wrapper around sync.force_clean. 3. Document mempalace_sync_status in website/reference/mcp-tools.md so it passes test_no_undocumented_tools. Also ran ruff format with CI-pinned 0.4.x. All 956 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…ut, tests Addresses the six Copilot review comments on the initial commit. 1) MemPalace#6 (critical) — mcp_server.py `_get_collection` bypassed ChromaBackend The MCP server creates its palace collection directly via `chromadb.PersistentClient.get_or_create_collection` in `_get_collection`, not through `ChromaBackend.get_collection`. That path was missing the `hnsw:num_threads=1` metadata, so the primary crash surface for MemPalace#974 and MemPalace#965 was untouched by the original patch. Fixed by passing `hnsw:num_threads=1` at the mcp_server create site too. Documented in a code comment that the setting is only honored at creation time — existing palaces created before this fix still need a `mempalace nuke` + re-mine to gain the protection. 2) MemPalace#3 — mine_global_lock over-serialized mines across unrelated palaces Replaced the single global lock file `mine_global.lock` with a per-palace lock keyed by `sha256(os.path.abspath(palace_path))` (`mine_palace_<hash>.lock`). Mines against the same palace still collapse to a single runner (the correctness boundary), but mines against *different* palaces are now free to run in parallel. `mine_global_lock` is kept as a backward-compatible alias for `mine_palace_lock` so any external callers that imported the previous name keep working. 3) #1 — hook_precompact swallowed OSError but not subprocess.TimeoutExpired `subprocess.run(..., timeout=60)` raises `TimeoutExpired` on slow palaces. The previous `except OSError` clause didn't catch it, so the hook could raise and fail to emit any JSON decision — leaving the harness without a block/passthrough signal. Fixed by catching `(OSError, subprocess.TimeoutExpired)` together and always falling through to the block decision so the hook reliably emits a response. 4) MemPalace#2 + MemPalace#4 — tests - tests/test_hooks_cli.py: added `test_precompact_first_two_attempts_block`, `test_precompact_passes_through_after_cap`, and `test_precompact_counter_is_per_session` to lock in the MemPalace#955 deadlock fix. - tests/test_palace_locks.py (new): covers `mine_palace_lock` single-acquire, reuse-after-release, cross-process serialization on the same palace, non-interference across different palaces, path normalization, and the `mine_global_lock` back-compat alias. 5) MemPalace#5 — known limitation, documented but not auto-fixed Copilot suggested detecting collections missing `hnsw:num_threads=1` and calling `collection.modify(metadata=...)` to retrofit existing palaces. Verified against chromadb 1.5.7: `modify(metadata=...)` replaces metadata rather than merging, and re-passing `hnsw:space="cosine"` then raises `ValueError: Changing the distance function of a collection once it is created is not supported currently.` The HNSW runtime configuration (`configuration_json`) also does not expose `num_threads` in chromadb 1.5.x, so the flag appears to be read only at creation time. Rather than paper over the limitation with a best-effort `modify` that silently drops `hnsw:space`, documented in the mcp_server comment that pre-existing palaces need a `mempalace nuke` + re-mine to gain the protection. Fresh palaces are always protected. Testing - pytest tests/test_palace_locks.py tests/test_hooks_cli.py tests/test_backends.py tests/test_cli.py → **98 passed, 0 failed**. - Runtime validation with two concurrent `mempalace mine` calls: - Different palaces → both complete in parallel ✓ - Same palace → one completes, the other exits with "another `mine` is already running against <palace> — exiting cleanly." ✓

…ut, tests Addresses the six Copilot review comments on the initial commit. 1) MemPalace#6 (critical) — mcp_server.py `_get_collection` bypassed ChromaBackend The MCP server creates its palace collection directly via `chromadb.PersistentClient.get_or_create_collection` in `_get_collection`, not through `ChromaBackend.get_collection`. That path was missing the `hnsw:num_threads=1` metadata, so the primary crash surface for MemPalace#974 and MemPalace#965 was untouched by the original patch. Fixed by passing `hnsw:num_threads=1` at the mcp_server create site too. Documented in a code comment that the setting is only honored at creation time — existing palaces created before this fix still need a `mempalace nuke` + re-mine to gain the protection. 2) MemPalace#3 — mine_global_lock over-serialized mines across unrelated palaces Replaced the single global lock file `mine_global.lock` with a per-palace lock keyed by `sha256(os.path.abspath(palace_path))` (`mine_palace_<hash>.lock`). Mines against the same palace still collapse to a single runner (the correctness boundary), but mines against *different* palaces are now free to run in parallel. `mine_global_lock` is kept as a backward-compatible alias for `mine_palace_lock` so any external callers that imported the previous name keep working. 3) MemPalace#1 — hook_precompact swallowed OSError but not subprocess.TimeoutExpired `subprocess.run(..., timeout=60)` raises `TimeoutExpired` on slow palaces. The previous `except OSError` clause didn't catch it, so the hook could raise and fail to emit any JSON decision — leaving the harness without a block/passthrough signal. Fixed by catching `(OSError, subprocess.TimeoutExpired)` together and always falling through to the block decision so the hook reliably emits a response. 4) MemPalace#2 + MemPalace#4 — tests - tests/test_hooks_cli.py: added `test_precompact_first_two_attempts_block`, `test_precompact_passes_through_after_cap`, and `test_precompact_counter_is_per_session` to lock in the MemPalace#955 deadlock fix. - tests/test_palace_locks.py (new): covers `mine_palace_lock` single-acquire, reuse-after-release, cross-process serialization on the same palace, non-interference across different palaces, path normalization, and the `mine_global_lock` back-compat alias. 5) MemPalace#5 — known limitation, documented but not auto-fixed Copilot suggested detecting collections missing `hnsw:num_threads=1` and calling `collection.modify(metadata=...)` to retrofit existing palaces. Verified against chromadb 1.5.7: `modify(metadata=...)` replaces metadata rather than merging, and re-passing `hnsw:space="cosine"` then raises `ValueError: Changing the distance function of a collection once it is created is not supported currently.` The HNSW runtime configuration (`configuration_json`) also does not expose `num_threads` in chromadb 1.5.x, so the flag appears to be read only at creation time. Rather than paper over the limitation with a best-effort `modify` that silently drops `hnsw:space`, documented in the mcp_server comment that pre-existing palaces need a `mempalace nuke` + re-mine to gain the protection. Fresh palaces are always protected. Testing - pytest tests/test_palace_locks.py tests/test_hooks_cli.py tests/test_backends.py tests/test_cli.py → **98 passed, 0 failed**. - Runtime validation with two concurrent `mempalace mine` calls: - Different palaces → both complete in parallel ✓ - Same palace → one completes, the other exits with "another `mine` is already running against <palace> — exiting cleanly." ✓

Adds _try_gemini_json parser to normalize.py for three layouts: 1. Gemini API contents format (~/.gemini/sessions/*.json): {"contents": [{"role": "user", "parts": [{"text": "..."}]}, ...]} 2. Messages-wrapper variant: {"messages": [{"role": "user", ...}, {"role": "model", ...}]} 3. Flat top-level list with role="model". This complements the existing _try_gemini_jsonl parser (which handles ~/.gemini/tmp/<hash>/chats/session-*.jsonl with session_metadata sentinel) — JSONL covers Gemini CLI runtime sessions, JSON covers exported / Studio-saved transcripts. ## Review feedback addressed (PR MemPalace#204) bgauryy review: - MemPalace#1 Parser-precedence bug: _try_gemini_json runs *before* _try_claude_ai_json so the {"messages":[..., role=model, ...]} layout is no longer silently claimed by the Claude parser. The Gemini parser's has_model_role guard prevents false-positives against Claude / ChatGPT data. - MemPalace#2 Layout 2a coverage: TestGeminiJson.test_messages_wrapper_format + test_messages_wrapper_does_not_get_claimed_by_claude pin the fix in place. - MemPalace#3 Test conflicts with current main: rebased onto develop; tests restructured into TestGeminiJson class. - MemPalace#4 tempfile/os.unlink → pytest tmp_path everywhere. - MemPalace#5 elif not text → else (the elif branch was dead). - MemPalace#6 Module docstring updated to mention Google AI Studio. Tests: 9 new cases in TestGeminiJson covering all three layouts, multi-part text joining, non-text part skipping, has_model_role disambiguation, dispatch-chain regression for review MemPalace#1.

Capture the operator decisions made on proposals (accept/reject/merge/ review) into an append-only JSONL ledger at ~/.claude/memory-miner/decisions.jsonl. This is the data-capture foundation for Phase 2 self-tuning — it only CAPTURES the signal; the tuning algorithm is deliberately not built (tuning with zero data is premature). tuning.py: - record_decision(id, action, type=, score=, meta=, ts=, ledger_path=) append-only; no wall-clock stamped unless ts is passed (determinism). - load_decisions / decided_ids so future runs/emit can skip already- decided proposals. - reject(id, **meta) convenience. All writes are best-effort and never raise. accept.py: - on a successful create -> record 'accepted'; on merge -> record 'merged'. Logging is fully isolated/best-effort so a logging failure can never break a successful accept. Existing accept behavior and return shape are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Restore real source_excerpt provenance and add a deterministic time-sensitivity tag so human reviewers can verify a fact's origin and know which proposals to re-verify before accepting. - proposal.py: add time_sensitive: bool field (source_excerpt already existed); include both in from_dict round-trip; validate as optional typed fields in validate_proposal. - distill.py: replace the body-echo _excerpt stub with a real transcript-snippet finder (highest word-overlap passage, already secret-scrubbed); add is_time_sensitive heuristic (project facts with PR/issue numbers, branch/worktree refs, in-progress status words, or concrete dates -> True; durable user/feedback/reference -> False); wire time_sensitive into proposal construction. - emit.py: render a truncated "> excerpt:" block and a time-sensitive marker for stale proposals in proposals.md. - tests/test_provenance.py: excerpt provenance + scrub safety + md render; staleness heuristic (PR MemPalace#6 open -> True, durable -> False); schema + jsonl round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…at) (MemPalace#6) Adds two new ChromaDB-compatible embedding backends so users with a separate GPU box (DGX Spark, dedicated workstation, etc.) can offload embedding work without installing CUDA wheels on the MemPalace machine. * `OllamaEmbeddingFunction` (`embedding_ollama.py`) — wraps Ollama's `/api/embed` (batch) with `/api/embeddings` (per-input) fallback for older Ollama versions. Stdlib urllib only, no new deps. * `OpenAICompatEmbeddingFunction` (`embedding_openai.py`) — wraps any `/v1/embeddings` endpoint (vLLM, LM Studio, Together, OpenAI itself). Defensive index-sort on response, optional bearer auth. * Provider router in `embedding.py` selects ONNX (default, unchanged) vs Ollama vs OpenAI-compat by `MempalaceConfig.embedding_provider`. Each provider has its own `name()` so a config swap surfaces as an EF identity error rather than silent vector corruption. * New config keys with env var fallbacks: embedding_provider (MEMPALACE_EMBEDDING_PROVIDER) embedding_model (MEMPALACE_EMBEDDING_MODEL) embedding_endpoint (MEMPALACE_EMBEDDING_ENDPOINT) Measured on a DGX Spark (GB10) with nomic-embed-text-v1.5, batch=1024: Ollama /api/embed ~300 tok/s (single inference slot, NUM_PARALLEL does not apply to embed path on 0.23.2) vLLM /v1/embeddings ~11,400 tok/s (continuous batching, real GPU sat) 53 new tests; ruff clean. Switching providers is destructive (vector dimension changes invalidate existing collections) — wipe and re-mine. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

Per-project files mempalace writes into consumer repos during mining. Started appearing during the Spark-era remote embedding work (PR MemPalace#6). Issue MemPalace#185. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Design doc for a phased rollout of producer/consumer parallelism across the mining pipeline. vLLM on the DGX Spark saturates at ~11,400 tok/s but a serial mine leaves it idle most of the wallclock. Phase 0 (this PR): plan doc + LLM config-persistence mirror (env / file keys matching the embedding mirror PR MemPalace#6 shipped). Phase 1: parallel miner.py + shared mempalace/parallel.py harness. Phase 2: verify on the Spark (GPU util / throughput / re-mine idempotency). Phase 3: convo_miner, llm_refine, closet_llm + retire the HTTP-keepalive shim. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

… persistence Remote LLM support already worked via the existing `--llm-endpoint` / `--llm-provider` flags on `init`, but unlike embeddings (PR MemPalace#6), there was no `~/.mempalace/config.json` or env-var mirror — so pointing LLM refinement at a Spark vLLM container required passing the flags on every invocation. Adds four properties to MempalaceConfig mirroring the embedding_provider shape exactly: * `llm_provider` — env `MEMPALACE_LLM_PROVIDER` → config → "ollama" * `llm_model` — env `MEMPALACE_LLM_MODEL` → config → "gemma4:e4b" * `llm_endpoint` — env `MEMPALACE_LLM_ENDPOINT` → config → None (provider default) * `llm_api_key` — env `MEMPALACE_LLM_API_KEY` (env-only, never persisted) Defaults match historical CLI defaults so this is non-breaking. Argparse flags drop their hardcoded "ollama" / "gemma4:e4b" defaults and fall through to the new config layer when unset; --help text now documents the precedence. Foundation for the Phase 3 work in docs/design/embrace-parallelism.md (parallelizing llm_refine + closet_llm against a remote LLM endpoint). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

igorls mentioned this pull request Apr 7, 2026

fix: use upsert and deterministic IDs to prevent data stagnation #140

Merged

bensig closed this Apr 7, 2026

cktang88 mentioned this pull request Apr 7, 2026

Bugs and improvements #159

Closed

bgauryy mentioned this pull request Apr 8, 2026

feat: vocabulary map for query expansion (closes #283) #311

Closed

armujahid mentioned this pull request Apr 9, 2026

Bundle hook scripts in pip package #265

Open

5 tasks

jphein mentioned this pull request Apr 12, 2026

fix(kg): Prevent edge duplication in Knowledge Graph #655

Closed

rusel95 mentioned this pull request Apr 13, 2026

feat: add sync command and --force flag for incremental re-mining #251

Closed

18 tasks

felipetruman mentioned this pull request Apr 17, 2026

fix: HNSW graph corruption, PreCompact deadlock, mine fan-out (closes #974, #965, #955) #976

Merged

5 tasks

bensig mentioned this pull request Apr 19, 2026

fix: set hnsw:num_threads to 1 for thread-safe HNSW inserts #991

Closed

leopechnicki mentioned this pull request Apr 21, 2026

security: fix 7 audit findings (MCP server, hooks, CI) leopechnicki/mempalace#1

Closed

igorls mentioned this pull request Apr 27, 2026

fix(hooks): always mine the active transcript as convos, additive to MEMPAL_DIR #1231

Merged

6 tasks

xg-gh-25 mentioned this pull request May 22, 2026

HNSW corruption from concurrent ChromaDB clients (MCP server + hook subprocesses + auto-ingest mine) within single Claude Code session #1581

Open

Navigation Menu

Uh oh!

fix: handle Windows file-lock errors in test cleanup - #6

fix: handle Windows file-lock errors in test cleanup#6
claudlos wants to merge 2 commits into
MemPalace:mainfrom
claudlos:fix/windows-test-cleanup

claudlos commented Apr 7, 2026

Uh oh!

bensig commented Apr 7, 2026

Uh oh!

bensig commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

claudlos commented Apr 7, 2026

What does this PR do?

How to test

Checklist

Uh oh!

bensig commented Apr 7, 2026

Uh oh!

bensig commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants