fix(miner): harden Windows mine against ONNX bad_alloc + silent partial exits (#1296)#1402
Merged
Merged
Conversation
…al exits Three small changes that together address the failure modes in #1296: 1. Add pnpm-lock.yaml and yarn.lock to SKIP_FILENAMES, mirroring the existing package-lock.json rule. A 24K-line pnpm-lock.yaml produced ~1124 chunks in one batch and tripped onnxruntime bad_alloc on Windows; pnpm/yarn lockfiles are no more useful to mine than npm's. 2. Skip any file that produces more than MAX_CHUNKS_PER_FILE (500) chunks, with a clear log line. Catches the broader class — generated CSV/JSON, build artifacts, etc. — that the named-file SKIP list will never fully cover. The cap is conservative (500 chunks * 800 chars ≈ 400 KB of source) so legitimate hand-written content still mines. 3. Print a partial-progress summary on any exception in _mine_impl, not just KeyboardInterrupt, then re-raise. Without this, an arbitrary exception (ONNX bad_alloc, chromadb HNSW error, OS fault) propagates silently — the operator sees only the last progress line and assumes the mine succeeded. The new path mirrors the KeyboardInterrupt summary (files_processed, drawers_filed, last_file) plus the exception type and message, then re-raises so the original traceback surfaces and the exit code is non-zero. Tests cover: SKIP_FILENAMES contents, the chunk-cap path returning (0, room) with no upserts, and the new mine-aborted summary surfacing both the partial counters and the exception class.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the project mining pipeline against Windows-specific failure modes reported in #1296 by reducing pathological per-file ingestion size and ensuring partial-progress summaries are printed when mining aborts unexpectedly.
Changes:
- Extend
SKIP_FILENAMESto exclude additional JS lockfiles (pnpm-lock.yaml,yarn.lock). - Add
MAX_CHUNKS_PER_FILEto skip files that generate an excessive number of chunks. - Print an abort summary for non-
KeyboardInterruptexceptions (then re-raise), and add tests covering the new behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mempalace/miner.py |
Adds lockfile skips, a per-file chunk cap, and an exception abort summary during mining. |
tests/test_miner.py |
Adds tests to pin the new skip list entries, chunk-cap behavior, and exception-summary behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
835
to
+839
| chunks = chunk_text(content, source_file) | ||
|
|
||
| if len(chunks) > MAX_CHUNKS_PER_FILE: | ||
| print( | ||
| f" ! [skip] {filepath.name[:50]:50} produced {len(chunks)} chunks " |
Comment on lines
+1194
to
+1197
| print(f" files_processed: {files_processed}/{len(files)}") | ||
| print(f" drawers_filed: {total_drawers}") | ||
| print(f" last_file: {last_file or '<none>'}") | ||
| print(f" error: {type(exc).__name__}: {exc}") |
Comment on lines
+713
to
+718
| def test_process_file_skips_when_chunks_exceed_max(tmp_path, monkeypatch): | ||
| """A file producing more than MAX_CHUNKS_PER_FILE chunks must be skipped | ||
| with a clear message and zero upserts. Generated artifacts (CSVs, lock | ||
| files not in SKIP_FILENAMES) hit this — the cap is what prevents ONNX | ||
| bad_alloc on Windows when the embedder is asked to swallow thousands of | ||
| chunks in one batch (#1296).""" |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Three small changes that together address the failure modes in #1296.
Summary
pnpm-lock.yamlandyarn.lockadded toSKIP_FILENAMES— mirrors the existingpackage-lock.jsonrule. A 24K-linepnpm-lock.yamlproduced ~1124 chunks in one batch and trippedonnxruntimebad_allocon Windows in the original report.MAX_CHUNKS_PER_FILE = 500cap — any file producing more than the cap is skipped with a clear log line. Catches the broader class (generated CSV/JSON, build artifacts) that a named-file skip list will never fully cover. Conservative budget: 500 chunks × 800 chars ≈ 400 KB of source, so legitimate hand-written content still mines._mine_implnow prints a summary on any exception, not justKeyboardInterrupt— without this, an arbitrary exception (ONNXbad_alloc, chromadb HNSW error, OS fault) propagated silently and the operator saw only the last progress line, assuming the mine succeeded (mine fails on Windows: HNSW index corruption + ONNX bad allocation on large files #1296 Failure 2). The new path mirrors theKeyboardInterruptsummary plus the exception class + message, then re-raises so the original traceback surfaces and the exit code is non-zero.Tests
Three additions in
tests/test_miner.py:test_skip_filenames_includes_lockfiles— pin the constanttest_process_file_skips_when_chunks_exceed_max—process_filereturns(0, room)with no upserts when chunks exceed the captest_mine_arbitrary_exception_prints_summary_and_reraises—RuntimeErrormid-mine surfaces the summary banner with files_processed / drawers_filed / last_file / exception class, then re-raisesLocal: 1596 tests pass, ruff lint + format clean against the 0.4.x CI pin.
Out of scope
mempalace repair --mode from-sqlite, just merged) and the preflight quarantine in fix: harden Chroma repair preflight and rollback recovery #1285 (also merged).Closes #1296