[pull] develop from MemPalace:develop - #22
Merged
Conversation
…2002) _get_client() detects a peer writer's inode/mtime change and rebuilds the client via ChromaBackend.make_client(), but chromadb caches its System (and the live in-memory HNSW segment) keyed by path. The rebuilt client is handed back the same stale segment, which on its next _persist() overwrites the on-disk index, destroying records other writers had already indexed. Observed in a live multi-writer palace: the persisted index count went backwards (4 to 3). Call the existing _force_chroma_cache_reset() on the staleness path, before make_client(), so chromadb rebuilds the segment from the on-disk state. The call is guarded by the existing inode_changed/mtime_changed check, so it has no effect on first-open. Adds test_get_client_resets_chroma_system_cache_on_reconnect, which asserts the reset runs before make_client on an mtime reconnect (fails without the fix). Refs #1963.
…n inode/mtime reopen _client() reconstructs PersistentClient on an inode/mtime change but did not drop chromadb's process-global SharedSystemClient cache first, so the rebuilt client reused the stale path-keyed System (and its in-memory HNSW segment) and could persist an outdated index over on-disk changes -- the same class as #2002, reached via _client() instead of _get_client. Add SharedSystemClient.clear_system_cache() to the external-change branch of _client(), mirroring mcp_server._force_chroma_cache_reset (#2026) and repair._close_chroma_handles. Backend-level regression test asserts the reset fires on the change reopen, strictly before the reconstruct, and not on first open (chroma-core/chroma#2536, #5843). Fixes #2028.
Two monkeypatch.setattr calls were wrapped across lines that fit within the line length; ruff format --check flagged them. Formatter-only, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg6g5efZ1rNbBTHqGz2Tjw
The #2002 staleness reconnect makes _get_client() call _force_chroma_cache_reset(), which clears chromadb's path-keyed SharedSystemClient cache. Two TestDeleteBySource tests grabbed a closets collection handle *before* calling tool_delete_by_source and then asserted on it afterwards, by which point the reset had dropped the Rust binding underneath the handle (AttributeError: 'RustBindingsAPI' object has no attribute 'bindings'). Re-acquire the closets collection after the tool call in both tests. Production callers already re-acquire fresh handles per call, so this is a test-lifetime issue, not a regression in the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg6g5efZ1rNbBTHqGz2Tjw
#21 (CRITICAL, data-loss): multi-batch re-mine had no completion marker. A mid-file crash after batch 1 committed but before a later batch left permanently silent partial data -- the surviving drawers shared the file's unchanged on-disk mtime, so file_already_mined() treated the file as fully mined forever. Every chunk now carries chunk_total, and file_already_mined() verifies a matching-mtime group's drawer count reaches chunk_total before reporting True. Drawers with no chunk_total (legacy rows, single-shot add_drawer()) are trusted as before. #22 (HIGH, correctness/TOCTOU): source_mtime was captured via a fresh os.path.getmtime() well after content was read, chunked, and room-detected. A file appended to in that window got its new drawers stamped with an mtime that already matched the (now newer) on-disk state, so the appended tail was silently, permanently skipped on every future mine. _read_text_no_follow now returns (content, mtime) from the same fstat() that validates the file; process_file threads that single value through instead of re-stating. #23 (HIGH, silent-failure): a failed stale-drawer purge was swallowed to a debug log and mining proceeded anyway, silently producing duplicate or orphaned drawers. A purge failure now aborts this file's mine attempt (old drawers' stored mtime is untouched, so the next mine still sees a mismatch and retries) and prints a visible warning, matching every other degraded path in this module. #24 (LOW, data-loss): the old-drawer delete ran unconditionally, but the closet purge+rebuild only ran when drawers_added > 0 -- a file whose chunks all landed below min_chunk_size after boundary-splitting lost its drawers but kept stale closets pointing at now-deleted IDs. purge_file_closets now runs whenever the delete-and-rebuild cycle does, regardless of the new chunk count; only the rebuild itself stays conditional. 169 tests pass across test_miner.py/test_convo_miner*.py/test_palace.py/ test_hallways.py/test_format_miner.py/test_miner_fts5_validation.py, no regressions. Full suite: 1 unrelated pre-existing flake in test_mcp_server.py (module-global peer-writer-lock state leaking across test files in full-suite ordering -- passes standalone and as a full file; the diff here never touches mcp_server.py).
`os.walk` and `glob` list a FIFO, a socket and a device node as ordinary filenames, and MemPalace decides what to read from the suffix. Opening a FIFO for reading parks in the kernel until a writer appears, so a named pipe called `notes.md` in a mined directory wedged `mempalace mine` forever — no output, no error, no progress. `mine --mode convos`, `sweep`, `init`, `compress` and `split` blocked the same way. Two shapes are at fault. Four helpers already refused non-regular files with `fstat` + `S_ISREG`, but the check sat *after* a blocking `os.open`, so it could never run. Adding `O_NONBLOCK` to those opens makes the existing type check reachable: the open returns immediately and the file mode decides, with no errno guesswork. A FIFO that does have a live writer is refused just the same. Linux open(2) states the flag has no effect on regular files; the one exception is a write lease, where a non-blocking open fails EAGAIN instead of waiting out lease-break-time. Leases are granted on regular files only, so that branch re-checks the type and then opens without the flag rather than silently dropping a file that used to be mined. The rest guard with `exists()`, which is true for a pipe, and then open anyway. Those become type checks: a discovery walk drops non-regular entries before any reader sees them, and a fixed-name read decides with `is_file()` instead. `scan_project` and `scan_convos` already stat every candidate for the size limit, so the type check costs no extra syscall. Where the gate replaced an `open` that sat inside a `try`, it goes in the same `try`: `is_file()` raises `PermissionError` on a directory without `x`, which that handler already absorbed. O_NONBLOCK: miner._read_text_no_follow, convo_miner._is_regular_source_file, normalize._read_transcript_file, repair._open_regular_file_no_follow. Type gate: miner.scan_project, miner.load_config, convo_miner.scan_convos, sweeper.parse_claude_jsonl, sweeper.sweep_directory, entity_detector.detect_entities, cli._gather_origin_samples, cli._ensure_mempalace_files_gitignored, cli.cmd_compress, cli.cmd_init, project_scanner._collect_manifest_names, project_scanner._parse_gradle_root_project_name, room_detector_local.detect_rooms_local, llm_refine.collect_corpus_text, split_mega_files.main, hook_shell.count_human_messages.
Stack the post-3.7.0 hang and silent-skip fixes for a fast patch release: - Keep #2223 (non-regular file hang) and #2088 (chunk_total / same-fstat mtime / purge abort / closet purge) as the base. - On multi-batch upsert failure, delete partial drawers and closets for that source before re-raising so the next mine retries (#2122, #2151). - Install SIGTERM/SIGHUP handlers in mcp_server.main so atexit can release the palace writer lease (#2205). - Adapt non-regular-file tests to the (content, mtime) read return type.
) Port project-miner re-mine honesty to conversation ingest so an interrupted transcript mine cannot permanently skip missing exchanges: - stamp chunk_total on every convo drawer in the pass - delete partial drawers for the source/extract_mode on upsert failure - teach prefetch_mined_set the same completeness rule as file_already_mined
…name In-place rebuild tests archive the palace directory after _seed_palace. backend.close() alone left chromadb's path-keyed System holding files open on Windows (WinError 5), so rebuild_from_sqlite aborted before the mocked upsert path and test_rebuild_from_sqlite_raises_on_upsert_failure never raised RebuildPartialError. Clear the shared cache and GC after close.
fix: 3.7.1 critical patch — hang, re-mine honesty, stale Chroma cache
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )