fix(ingest): never block on a non-regular file (#2221) - #2223
Conversation
`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. Write side: `split_file` synthesises its own output filenames and writes them into the source directory, so the discovery gate above cannot vet them. A pre-existing pipe at one of those names blocked the write with the new SKIP line for that same pipe already on screen; it is gated too. Two gates needed a second pass. In `project_scanner`, `is_file()` sat outside the handler each parser already provides, so a directory that is readable but not searchable turned "no manifest name" into a traceback out of `init`; both are back inside a handler. And `sweep_directory` skipped anything it could not `stat`, which quietly covered a dangling symlink and a file unlinked mid-walk — real errors that `sweep` books and exits 2 on. A non-regular type stays a benign SKIP; a failed stat is booked as before. `normalize._read_transcript_file` was wrapping its two refusals a second time, so the path appeared twice in the message. They are prefix-free now and the wrapper composes them once. tests/test_non_regular_file_guards.py covers all of it under a SIGALRM deadline, so a regression turns the file red instead of hanging pytest. The deadline raises a BaseException subclass: `except OSError` guards every site here and TimeoutError *is* an OSError, which cost four vacuous passes before the switch. The permission tests skip as root, whose CAP_DAC_OVERRIDE ignores the missing `x` bit they depend on.
0f3f0c6 to
47d6841
Compare
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.
|
Closing this: the #2221 work reached The revision that landed predates this branch's last pass, so three guards and their tests did not come along with it:
I opened #2244 with exactly that remainder — 6 files, +146/−10 on top of |
* 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>
What does this PR do?
Fixes #2221. A named pipe in a mined directory wedged
mempalace mineforever;mine --mode convos,sweep,init,compressandsplitblocked the same way. All ten scenarios in the issue hang ondevelopunder a 180 s cap and complete here.Two shapes, neither of which asks what the file is.
Open first, check the type second.
scan_projectacceptsnotes.mdon its suffix (miner.py:1683) andstatnever blocks on a pipe, soprocess_filereachesminer.py:66:convo_miner.py:191,normalize.py:128andrepair.py:68carry the identical dead guard.exists()is not a type check. It is true for a pipe, andmempalace.yaml,.gitignore,entities.jsonand a gradlesettings.gradleall reach anopenbehind one.The change
O_NONBLOCKon those four opens, so theS_ISREGrefusal already written there can run — the verdict is the file mode, not an errno, so a pipe with a live writer is refused too. Everywhere else a type gate: the walks drop non-regular entries with aSKIP: <name> (not a regular file)line beside the existing symlink and size lines, and fixed-name reads useis_file().scan_projectandscan_convosalready stat for the size limit, so onestat()now answers both questions.Two traps:
EAGAINwhere a blocking open waits outlease-break-timeand succeeds, silently dropping a file the old code read. The retry re-checks the type first, and the kernel grants leases on regular files only (F_SETLEASEon a pipe failsEINVAL), so it is authorised by the type, not the errno.Path.is_file()raisesPermissionErroron a directory withoutx(through 3.13), so it belongs inside whatevertrytheopensat in — including eachproject_scannerparser, which swallows its ownOSError. Andsweep_directoryskips a non-regular type as a benignSKIPbut still books a failedstat: a dangling symlink is a real error, andsweepstill exits 2 on it.Audit
Every
os.open,open,read_text,read_bytesand parser entry inmempalace/was traced to where its path comes from and probed with a realmkfifoon both commits. Nineteen sites block ondevelop; none block here. Two are hardening rather than live hangs, and I would rather say so than overclaim:repair's helper sits behindcontains_palace_database(cli.py:1866) andrepair.py:1692,hook_shellbehind[ -f ]inmempal_save_hook.sh:228.detect_entitiesis the chokepoint for all four callers ofscan_for_detection. The one site no discovery gate could reach is a write:split_filesynthesises its own output names, so a pipe at one of them blocked the write with the newSKIP:line for that pipe already on screen.format_miner.extract_textis left alone — immune by ordering rather than by type, which deserves its own change; the ordering is pinned by a test here. Known limit: the stat-then-open gates stay TOCTOU-racy, unlike_read_text_no_follow, which re-checks the type on the descriptor.How to test
develop: printsFiles: 2, filesreal.md, then nothing —exit=124. Here:SKIP: notes.md (not a regular file),Files: 1,Drawers filed: 2,exit=0, andsearchafterwards returnsreal.md.The exit statuses are the easiest thing here to get wrong:
Tests
tests/test_non_regular_file_guards.py, 42 tests, each under aSIGALRMdeadline so a regression turns the file red instead of hanging pytest — the deadline raises aBaseExceptionsubclass, becauseexcept OSErrorguards every site here andTimeoutErroris anOSError. Every production hunk was reverted on its own in an isolated copy; two are not covered by a unit test and I would rather name them than round the count up — theexcept OSErrorarounddetect_rooms_localincmd_init, and the legacymempal.yamlname inload_config. 36 of the 42 are POSIX-only (Windows has no FIFO in the filesystem namespace), and three more skip as root, matchingtests/test_backups.py.The suite's only failures here are the palace-lock family under full-suite load, and they are load artefacts rather than anything this branch does: the same commit produced one failure on a quiet machine and twelve on a busy one, and every one of them passes when the file is run on its own. CI is the arbiter. One unrelated fix rides along on a line this PR had to touch:
normalize._read_transcript_filewas wrapping its two refusals a second time, printing the path twice.Checklist
python -m pytest tests/ -v)ruff check ., andruff format --check ., at the ruff 0.16.1 pinned in CI)