fix(miner): close four re-mine safety gaps in process_file - #2088
1 commit merged into
Conversation
#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).
Adapts the portable half of upstream PR MemPalace#2088 (four re-mine safety gaps in process_file) to the PostgreSQL backend. Upstream needs a chunk_total completion marker and abort-on-failed-purge because ChromaDB commits batch by batch; here one PalaceDB.replace_file_drawers transaction makes the whole file atomic instead: stale-row purge and every insert commit together, so a mine killed mid-file leaves the previous state intact, a failed purge aborts the attempt, and a shrunk or empty-yield file stops serving its old chunks. source_mtime is captured with the read, not a later re-stat (the PR's TOCTOU gap: an append landing between read and re-stat was stamped as mined and silently skipped forever). file_already_mined now requires every drawer in the file's mining scope to carry the current mtime instead of trusting one arbitrary row, so partial legacy states re-mine and self-clean. Purge and freshness check are scoped by (ingest_mode, extract_mode) so convo extract modes cannot delete or invalidate each other's drawers — the over-match class of upstream PR MemPalace#2089. Co-authored-by: KeilerHirsch <KeilerHirsch@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ration Extends the atomic-replace port (upstream PR MemPalace#2088) to the convo path and closes the fork-side analog of upstream PR MemPalace#2089 (purge deleting rows another pass owns): - mine_convos files each transcript through one replace_file_drawers transaction scoped to (convos, extract_mode), so re-chunked exchanges can no longer leave orphaned tail rows and a mine killed mid-file leaves the previous state intact. The freshness check is scoped the same way, so exchange-mode and general-mode drawers for one transcript track their own staleness. - source_mtime is captured before normalize() reads the file — Claude Code session logs are appended to while being mined, and a later re-stat stamps the drawers as covering a tail that was never chunked (PR MemPalace#2088's TOCTOU gap). - register_empty_file now takes that caller-captured mtime and a purge_stale flag: genuinely-empty content purges the scope's stale drawers in the same transaction as the sentinel upsert, while the transient normalize()-failure path keeps registering the sentinel without touching mined data. Co-authored-by: KeilerHirsch <KeilerHirsch@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fatkobra
left a comment
There was a problem hiding this comment.
Blocking: the new completion marker covers all drawer batches, but not the
closet phase that follows them.
By the time purge_file_closets() and upsert_closet_lines() run, every
drawer already carries the current source_mtime and the complete
chunk_total. file_already_mined() will therefore return True on the
next run.
This leaves two partial-completion cases that are not retried:
purge_file_closets()still catches every exception and logs only at
debug level. If the purge fails, stale higher-numbered closets can survive
while the new closets are written.- If
upsert_closet_lines()raises after the drawer batches complete, the
next mine sees the drawers as complete and skips the source instead of
retrying the closet rebuild.
Please make the completion contract cover the entire per-file replacement,
not only the drawer upserts. For example, record final completion only after
the closet purge/rebuild succeeds, or otherwise ensure that a closet-stage
failure causes the next freshness check to return False.
Regression coverage should separately simulate:
- a failed closet purge followed by another mining run;
- a failed closet upsert followed by another mining run;
and verify that the second run retries the source rather than treating it as
fully complete.
|
Hey @KeilerHirsch, one more thing about the
Dedup is the path I ran, but |
83d8cbd
Stack the post-3.7.0 hang and silent-skip fixes for a fast patch release: - Keep MemPalace#2223 (non-regular file hang) and MemPalace#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 (MemPalace#2122, MemPalace#2151). - Install SIGTERM/SIGHUP handlers in mcp_server.main so atexit can release the palace writer lease (MemPalace#2205). - Adapt non-regular-file tests to the (content, mtime) read return type.
* fix(mcp_server): reset chromadb System cache on staleness reconnect (MemPalace#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 MemPalace#1963. * fix(chroma): reset chromadb System cache in ChromaBackend._client() on 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 MemPalace#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 (MemPalace#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 MemPalace#2028. * test(chroma): reformat test_backends.py to satisfy ruff format 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 * test(mcp_server): re-acquire closets handle after delete_by_source The MemPalace#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 * fix(miner): close four re-mine safety gaps in process_file MemPalace#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. MemPalace#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. MemPalace#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. MemPalace#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). * docs(changelog): tighten 3.7.0 notes to match prior release style Rewrite the 3.7.0 section as short, scannable bullets like 3.6.0 — bold lead, one or two sentences per item, thematically grouped fixes — instead of multi-paragraph issue writeups. * fix(tests): harden hybrid search against empty Windows Chroma reads Windows CI intermittently returns zero hybrid hits right after a fast seed write (same class as "Nothing found on disk" on tiny collections). Close the palace client after seeding so the next open re-reads flushed segments, retry search once if empty, and assert non-empty results with a clear message instead of IndexError. * fix(ingest): never block on a non-regular file (MemPalace#2221) `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. * docs(changelog): note the non-regular-file ingest hang (MemPalace#2221) * fix: 3.7.1 critical patch — re-mine honesty + SIGTERM lock release Stack the post-3.7.0 hang and silent-skip fixes for a fast patch release: - Keep MemPalace#2223 (non-regular file hang) and MemPalace#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 (MemPalace#2122, MemPalace#2151). - Install SIGTERM/SIGHUP handlers in mcp_server.main so atexit can release the palace writer lease (MemPalace#2205). - Adapt non-regular-file tests to the (content, mtime) read return type. * fix(convo): stamp chunk_total and clean partial multi-batch mines (MemPalace#2183) 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 * test(repair): release SharedSystemClient after seeding for Windows rename 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. * chore(release): 3.7.1 Bump package, plugins, lock, OpenClaw skill, and README badge to 3.7.1. Fold Unreleased integrity notes into the 3.7.1 changelog: FIFO ingest hang, project and convo re-mine completeness, chromadb System-cache rewind, and SIGTERM/SIGHUP lease release. * fix(release): preserve fork behavior after upstream sync Keep normalized and subject-routed ingestion compatible with the 3.7.1 re-mine safeguards. Distinguish local Chroma writes from peer changes so cache refreshes release stale clients without invalidating live handles. * fix(ci): record HTTP status before response delivery * fix(ci): normalize SDK HTTP status values * test(ci): make conversation fixtures portable --------- Co-authored-by: Cristian Deheleanu <160292664+colorpanda82@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: KeilerHirsch <KeilerHirsch@users.noreply.github.com> Co-authored-by: Igor Lins e Silva <4753812+igorls@users.noreply.github.com> Co-authored-by: Michael Valentsev <michael@valentsev.ru>
Problem
Four related gaps in
miner.py::process_file's re-mine path, all found in the same audit pass:1. No completion marker across multi-batch upserts.
process_filedeletes all existing drawers forsource_file, then re-inserts new chunks in batches ofDRAWER_UPSERT_BATCH_SIZE. Every chunk in a pass carries the samesource_mtime. If the process is killed after batch 1 commits but before a later batch, the palace ends up with only the first N batches, and old closets left dangling. On the nextmempalace minerun,file_already_mined(check_mtime=True)finds the surviving batch's drawers with asource_mtimethat still matches the file's on-disk mtime (the file itself was never touched) and reports the file as fully mined — the missing chunks become permanently unreachable.2.
source_mtimecaptured after content is read, not at read time (TOCTOU). The file is read once (_read_text_no_follow), thensource_mtimeis captured later via a separateos.path.getmtime()call — after chunking, room detection, and acquiring the mine lock. If the file is modified in that window (the exact scenario the codebase's own docs describe for an actively-appended Claude Code session log), the storedsource_mtimereflects content that was never actually chunked. The next mine sees the current on-disk mtime match the stored value and treats the file as current — the appended tail is silently, permanently skipped.3. Failed drawer purge swallowed to a debug log.
collection.delete(where={"source_file": source_file})is wrapped inexcept Exception: logger.debug(...), and execution continues unconditionally into the upsert loop. A transient delete failure (lock contention, a corrupted segment) then leaves old and new drawers coexisting — orphaned tail entries if the new set is smaller, silently overwritten rows if it's larger — with no operator-visible signal.4. Old-drawer delete runs before the new chunk count is known. The delete at the top of the locked section runs unconditionally; the closet purge+rebuild only runs when
drawers_added > 0. If every chunk from a re-mine ends up filtered belowmin_chunk_sizeafter boundary-splitting, the old drawers are gone but the old closets survive, now pointing at deleted drawer IDs.Fix
chunk_total(the number of chunks this pass expects to write).file_already_minedverifies a matching-stored_mtimegroup's drawer count reacheschunk_totalbefore reportingTrue. A drawer with nochunk_total(legacy rows, or the single-shotadd_drawer()helper, which has no partial-batch risk) is trusted as before — this stays backward compatible._read_text_no_follownow returns(content, mtime)from the samefstat()call that validates the file.process_filethreads that single value through for the storedsource_mtimeinstead of a later re-stat.return 0, room, None) and prints a stderr warning, instead of proceeding. The old drawers' stored mtime is untouched, so the next mine still sees a mismatch and retries.purge_file_closetsnow runs whenever the delete-and-rebuild cycle does, regardless of the new chunk count. Only the closet rebuild itself stays conditional ondrawers_added > 0.Tests
Five new tests in
tests/test_miner.py(one direct unit test offile_already_mined's new completeness check, four exercisingprocess_filevia aFakeColtest double), plus the existing_metadata/_build_drawer_metadatacoverage. Each new test was confirmed to fail against the pre-fix code (verified viagit stashof the source files only) and pass after the fix. Full existing suite acrosstest_miner.py,test_convo_miner*.py,test_palace.py,test_hallways.py,test_format_miner.py, andtest_miner_fts5_validation.pystill passes — no regressions.