fix: batch upserts in miner to prevent ChromaDB 1.5.x compaction crashes - #796
fix: batch upserts in miner to prevent ChromaDB 1.5.x compaction crashes#796IzmanIzy wants to merge 1 commit into
Conversation
|
This addresses the same WAL pressure issue we tackled in #629 — worth noting that PR also adds bulk mtime pre-fetch ( |
1 similar comment
|
This addresses the same WAL pressure issue we tackled in #629 — worth noting that PR also adds bulk mtime pre-fetch ( |
igorls
added
area/mining
Adapts the approach from @IzmanIzy's PR MemPalace#796 to current develop, which diverged after that PR was opened — ``add_drawer`` now populates additional metadata fields (``normalize_version``, ``hall``, ``entities``, ``source_mtime``) that a verbatim rebase of MemPalace#796 would have regressed. Changes: - Extract ``_build_drawer_payload()`` helper — single source of truth for drawer ID + metadata construction, shared by ``add_drawer`` (single drawer, kept as a thin wrapper for backwards compat with tests) and ``process_file`` (batched per-file path). - ``process_file`` now accumulates all chunks from one file into batch lists and issues a single ``collection.upsert()`` call with them all, instead of one upsert per chunk. Preserves every metadata field ``add_drawer`` sets, including the ones added after MemPalace#796 was opened. - ``mine()`` adds a periodic checkpoint every 200 files that releases and re-acquires the collection (and closets collection), letting the Rust compactor flush buffered WAL entries between batches. - ``mine()`` releases both collection references at the end of mining so the final WAL entries flush cleanly before the process exits. ## Problem A typical project mine issues thousands of individual ``col.upsert()`` calls. ChromaDB 1.5.x's Rust compactor runs concurrently with writes and falls behind under this pattern, surfacing as: - ``Segfault (exit 139)`` — the compactor corrupts the metadata segment during concurrent individual writes - ``chromadb.errors.InternalError: Error in compaction: Failed to apply logs to the metadata segment`` - Later reads crash with ``mismatched types; Rust type u64 (as SQL type INTEGER) is not compatible with SQL type BLOB`` — the compactor left half-migrated rows with ``seq_id`` values still stored as BLOB instead of decoded to INTEGER ## Scope ``tests/test_miner.py::test_add_drawer`` and ``tests/test_hall_detection.py::test_add_drawer_includes_hall`` both call ``miner.add_drawer()`` directly and expect ``True`` + a single drawer with full metadata. Both pass with the backwards-compat wrapper. Related: MemPalace#796 (original PR, now has develop conflicts), MemPalace#899 (MCP server library staleness — the other vector that creates compaction crashes upstream of this fix). Co-Authored-By: IzmanIzy <noreply@github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi @IzmanIzy — I hit the exact compaction crash this PR fixes while mining into an existing palace that had accumulated BLOB-typed I wanted to flag that this PR has a non-trivial rebase conflict against current
The I rebased your approach onto current Commit: messelink/mempalace@9cc1365 Test I ranA delete-and-remine of a 20-file subtree (mix of markdown docs and chat transcripts) into an existing ~14.8k-drawer palace. Result: 1,324 drawers filed in a single end-to-end run without any compaction errors. The largest single-file batch was 618 drawers in one Previously (without the batching), the same mine crashed with How to proceedTotally your call — I don't want to step on your PR. Three options:
Let me know which you prefer. Happy to wait a few days for a reply before doing option 3. cc @milla-jovovich for awareness — this PR becomes unblocked with the rebase. |
|
hey @IzmanIzy — this conflicts with develop now. pls rebase and we can merge. thanks! |
|
@IzmanIzy pls check conflict |
|
@bensig — just flagging that my comment above (03:26 UTC) has the rebased version ready if it helps move this along. Branch: |
f15bffd to
9cc1365
Compare
|
Rebased onto current @bensig good to go. |
Adapts the approach from @IzmanIzy's PR MemPalace#796 to current develop, which diverged after that PR was opened — ``add_drawer`` now populates additional metadata fields (``normalize_version``, ``hall``, ``entities``, ``source_mtime``) that a verbatim rebase of MemPalace#796 would have regressed. Changes: - Extract ``_build_drawer_payload()`` helper — single source of truth for drawer ID + metadata construction, shared by ``add_drawer`` (single drawer, kept as a thin wrapper for backwards compat with tests) and ``process_file`` (batched per-file path). - ``process_file`` now accumulates all chunks from one file into batch lists and issues a single ``collection.upsert()`` call with them all, instead of one upsert per chunk. Preserves every metadata field ``add_drawer`` sets, including the ones added after MemPalace#796 was opened. - ``mine()`` adds a periodic checkpoint every 200 files that releases and re-acquires the collection (and closets collection), letting the Rust compactor flush buffered WAL entries between batches. - ``mine()`` releases both collection references at the end of mining so the final WAL entries flush cleanly before the process exits. ## Problem A typical project mine issues thousands of individual ``col.upsert()`` calls. ChromaDB 1.5.x's Rust compactor runs concurrently with writes and falls behind under this pattern, surfacing as: - ``Segfault (exit 139)`` — the compactor corrupts the metadata segment during concurrent individual writes - ``chromadb.errors.InternalError: Error in compaction: Failed to apply logs to the metadata segment`` - Later reads crash with ``mismatched types; Rust type u64 (as SQL type INTEGER) is not compatible with SQL type BLOB`` — the compactor left half-migrated rows with ``seq_id`` values still stored as BLOB instead of decoded to INTEGER ## Scope ``tests/test_miner.py::test_add_drawer`` and ``tests/test_hall_detection.py::test_add_drawer_includes_hall`` both call ``miner.add_drawer()`` directly and expect ``True`` + a single drawer with full metadata. Both pass with the backwards-compat wrapper. Related: MemPalace#796 (original PR, now has develop conflicts), MemPalace#899 (MCP server library staleness — the other vector that creates compaction crashes upstream of this fix). Co-Authored-By: IzmanIzy <noreply@github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lace#796) Pre-Phase-5 each chunk of a file was upserted one-at-a-time via add_drawer in a per-chunk loop. On the local (PersistentClient) backend this was merely wasteful; after Phase 5 introduced the HTTP client mode, it became one network round-trip per chunk. Mining a 250-chunk file over the QNAP stack paid 250 RTTs. Collapse that into a single batched call path: - palace.py: add upsert_in_batches(collection, ids, docs, metas) with a DRAWER_UPSERT_BATCH_SIZE = 100 cap (tuned conservatively — bigger batches give diminishing returns and risk embedder working-set growth on local mode). - miner.py: extract _build_drawer_record() from add_drawer so process_file can build all records up front, then upsert in ⌈N/100⌉ calls. add_drawer stays as a thin single-chunk compat shim for existing callers/tests. - convo_miner.py: same pattern in _file_chunks_locked. Behavior change worth flagging: the old convo_miner had a per-chunk `except "already exists"` swallow. Upsert is idempotent by contract so that branch was dead — batching naturally removes it. If ChromaDB ever starts surfacing "already exists" from upsert (it shouldn't), we'd now fail the whole batch instead of silently skipping. New fake-collection tests assert the batching contract without needing a live embedder: - test_upsert_in_batches_splits_into_fixed_size_calls: 250 items → 3 calls of 100/100/50, ids preserved in order - test_process_file_batches_upserts_for_large_file: end-to-end via process_file, asserts ⌈N/100⌉ upsert calls with unique ids Not a crash fix (the notes' stale framing); a round-trip cost fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@IzmanIzy heads-up — looks like the failure modes this PR was preventing have been addressed in other paths since your rebase. As of current
The one piece not in develop is the 200-file periodic collection release/re-acquire from this PR. If you've seen crashes that only the checkpoint specifically catches, worth flagging here. Otherwise this PR may be safe to close. |
|
Thanks! Per-file upsert batching already landed on develop (DRAWER_UPSERT_BATCH_SIZE), which covers the 1.5.x compaction crash; the remaining 200-file checkpoint here is speculative. Closing as superseded — please reopen if you still hit the crash on current develop. |
5 participants
Summary
collection.upsert()call instead of upserting each chunk individually. This reduces WAL write pressure that causes the Rust compactor in ChromaDB >= 1.5 to crash.Problem
When mining projects with 100+ files, the miner issues thousands of individual upserts. On ChromaDB 1.5.x this causes:
InternalError: Failed to apply logs to the metadata segment— WAL entries accumulate faster than compaction can process themBoth errors are intermittent and depend on project size, making them hard to reproduce in small test suites but consistent on real-world knowledge bases (300+ files).
Root Cause
ChromaDB's Rust compactor (introduced in 1.5.x) runs in a background thread. Individual upserts create one WAL entry each, and 2000+ entries in rapid succession overwhelm the compactor's ability to merge them atomically. The previous code already had a comment about hnswlib's thread-unsafe
updatePointpath causing segfaults on macOS ARM — this is the same class of bug on the compaction side, now affecting all platforms.Testing
pytest tests/ -k "mine"— 37 passed, 0 failed)ruff check) and format (ruff format --check) passTest plan
ruff check .passesruff format --check .passespytest tests/ -v --ignore=tests/benchmarks -k "mine"— 37 passed🤖 Generated with Claude Code