Skip to content

fix: set hnsw:num_threads to 1 for thread-safe HNSW inserts - #991

Closed
bobo-xxx wants to merge 1 commit into
MemPalace:developfrom
bobo-xxx:clawoss/fix/hnsw-num-threads
Closed

fix: set hnsw:num_threads to 1 for thread-safe HNSW inserts#991
bobo-xxx wants to merge 1 commit into
MemPalace:developfrom
bobo-xxx:clawoss/fix/hnsw-num-threads

Conversation

@bobo-xxx

Copy link
Copy Markdown
Contributor

Fixes SIGSEGV in HNSW parallel inserts by limiting HNSW to single-threaded inserts.

Summary

When multiple threads insert in parallel to an HNSW index, the Rust compactor crashes with SIGSEGV. This fix adds hnsw:num_threads: 1 to the collection metadata, ensuring thread-safe HNSW operations.

Changes

  • mempalace/backends/chroma.py: Added "hnsw:num_threads": "1" to metadata in:
    • get_or_create_collection() - line 129
    • create_collection() - line 150

Testing

This fix prevents the SIGSEGV crash when running parallel insert workloads against the HNSW index.

Copilot AI review requested due to automatic review settings April 18, 2026 07:48
@bobo-xxx
bobo-xxx requested a review from bensig as a code owner April 18, 2026 07:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to prevent SIGSEGVs in ChromaDB HNSW parallel insert workloads by forcing HNSW operations to run single-threaded via collection metadata.

Changes:

  • Set hnsw:num_threads to 1 in collection metadata for get_or_create_collection(...).
  • Set hnsw:num_threads to 1 in collection metadata for create_collection(...).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if create:
collection = client.get_or_create_collection(
collection_name, metadata={"hnsw:space": "cosine"}
collection_name, metadata={"hnsw:space": "cosine", "hnsw:num_threads": "1"}

Copilot AI Apr 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior adds hnsw:num_threads to the collection metadata, but the existing backend tests only assert hnsw:space. Please add/extend a test (e.g. in tests/test_backends.py) to assert the created collection has hnsw:num_threads set as expected so regressions are caught.

Copilot uses AI. Check for mistakes.
Comment on lines 149 to 151
collection = self._client(palace_path).create_collection(
collection_name, metadata={"hnsw:space": hnsw_space}
collection_name, metadata={"hnsw:space": hnsw_space, "hnsw:num_threads": "1"}
)

Copilot AI Apr 18, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collection metadata dict is now duplicated between get_collection(...create=True) and create_collection(). To avoid these drifting (especially if more HNSW knobs are added later), consider centralizing the default collection metadata in a small helper/constant and reuse it in both places.

Copilot uses AI. Check for mistakes.
@mvalentsev

Copy link
Copy Markdown
Contributor

Same hnsw:num_threads=1 change on the same file is already in #976 (felipetruman, 2026-04-17, closes #974 / #965 / #955). #976 pins the flag at three sites (get_or_create_collection, create_collection, mcp_server._get_collection) and adds a best-effort retrofit _pin_hnsw_threads for pre-existing palaces via collection.modify(configuration=UpdateCollectionConfiguration(hnsw=UpdateHNSWConfiguration(num_threads=1))). Third Copilot review round has been addressed.

Separately, #346 (yoshi280) attributes the sparse link_lists.bin bloat in #965 to persistDirty() seek drift on the default batch_size=100 / sync_threshold=1000, and raises both thresholds on collection creation. Different root cause from the thread race, orthogonal mitigation. Both paths could land together to cover the full failure surface.

This PR's chroma.py hunk is a narrow subset of #976.

@bensig

bensig commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Code review

Found 2 issues:

  1. hnsw:num_threads is set to the string "1", but ChromaDB validates it as int. Collection creation raises on both supported chromadb versions, so this change turns a SIGSEGV into an immediate hard failure on every fresh palace. Empirically reproduced: chromadb 0.6.3 → ValueError: Invalid value for HNSW parameter: hnsw:num_threads = 1; chromadb 1.5.8 → InvalidArgumentError: Failed to parse hnsw parameters from segment metadata. Issue SIGSEGV in HNSW parallel inserts — missing hnsw:num_threads on collection creation #974's own diff uses int 1, and the parallel PR fix: HNSW graph corruption, PreCompact deadlock, mine fan-out (closes #974, #965, #955) #976 uses int 1 at the same sites. Must be 1, not "1".

if create:
collection = client.get_or_create_collection(
collection_name, metadata={"hnsw:space": "cosine", "hnsw:num_threads": "1"}
)
else:
collection = client.get_collection(collection_name)

def create_collection(
self, palace_path: str, collection_name: str, hnsw_space: str = "cosine"
) -> "ChromaCollection":
"""Create (not get-or-create) *collection_name* with cosine HNSW space."""
collection = self._client(palace_path).create_collection(
collection_name, metadata={"hnsw:space": hnsw_space, "hnsw:num_threads": "1"}
)
return ChromaCollection(collection)

  1. The primary crash surface from SIGSEGV in HNSW parallel inserts — missing hnsw:num_threads on collection creation #974 is the MCP server's _get_collection in mempalace/mcp_server.py, which bypasses ChromaBackend and calls client.get_or_create_collection directly with only {"hnsw:space": "cosine"} — it is untouched by this PR. A user running the MCP server will still hit the SIGSEGV even after merging, because every MCP read/write path goes through this call. PR fix: HNSW graph corruption, PreCompact deadlock, mine fan-out (closes #974, #965, #955) #976 pins the flag here as well (Copilot review round 1, item fix: handle Windows file-lock errors in test cleanup #6) after the same hole was flagged; this PR leaves it open, so it does not actually close SIGSEGV in HNSW parallel inserts — missing hnsw:num_threads on collection creation #974 for the reporter's reproducer (MCP add_drawer / auto-mining).

def _get_collection(create=False):
"""Return the ChromaDB collection, caching the client between calls."""
global _collection_cache, _metadata_cache, _metadata_cache_time
try:
client = _get_client()
if create:
_collection_cache = ChromaCollection(
client.get_or_create_collection(
_config.collection_name, metadata={"hnsw:space": "cosine"}
)
)
_metadata_cache = None
_metadata_cache_time = 0
elif _collection_cache is None:
_collection_cache = ChromaCollection(client.get_collection(_config.collection_name))
_metadata_cache = None
_metadata_cache_time = 0
return _collection_cache
except Exception:
return None

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@igorls

igorls commented Apr 25, 2026

Copy link
Copy Markdown
Member

Closing as superseded by #976, which just merged into develop and includes the same hnsw:num_threads=1 change at both get_or_create_collection() and create_collection() call sites in mempalace/backends/chroma.py — plus a _pin_hnsw_threads() retrofit that re-applies the pin on existing legacy collections that were created before the fix landed.

Thanks for filing this — your PR was the clearest articulation of the SIGSEGV root cause, and the diff in #976 is a strict superset of yours. If you have palaces created on earlier MemPalace versions, the retrofit will pick them up automatically on the next get_collection call.

@igorls igorls closed this Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants