Skip to content

docs(investigate): root-cause metadata reshape bug (#32) - #140

Merged
jphein merged 1 commit into
mainfrom
investigate/metadata-reshape-32
May 23, 2026
Merged

docs(investigate): root-cause metadata reshape bug (#32)#140
jphein merged 1 commit into
mainfrom
investigate/metadata-reshape-32

Conversation

@jphein

@jphein jphein commented May 23, 2026

Copy link
Copy Markdown
Collaborator

Fixes investigation portion of #32.

TL;DR

The chokepoint sanitizer
ChromaCollection._sanitize_metadatas_for_chromadb
(mempalace/backends/chroma.py:1094-1111) returns a new outer list but
aliases each non-empty caller dict by reference. The
comprehension [m if (...) else sentinel for m in metadatas]
returns m itself whenever the guard passes, so the sanitized list
still points at the caller's dict. Any in-place mutation between
sanitize and chromadb's validate_metadata empties the dict from
under the validator's feet — which matches the failure mode reported
at ~120K of a 151K rebuild.

The same aliasing pattern appears in repair.py:_extract_drawers and
repair.py:_rebuild_one_collection.

What this PR contains

  • docs/investigations/metadata-reshape-root-cause.md — full
    walkthrough of the chromadb internal flow (Collection.add
    normalize_insert_record_setvalidate_metadatas
    validate_metadata), confirmation that chromadb does not reshape
    metadata in our code path, the aliasing diagnosis with empirical
    reproduction, candidate mutators at scale, and a minimal-fix
    proposal.

  • tests/test_metadata_reshape_bug.py — 9 tests. Two of them
    (test_sanitizer_inner_dicts_alias_caller_dicts and
    test_sanitized_dict_empty_after_caller_clear_passes_into_chromadb)
    reproduce the failure deterministically on a small palace, against
    the real chromadb 1.5.9 validator. The remaining 7 establish the
    upstream contract ({} rejected, None accepted) and verify the
    current sanitizer behaviour the fix must preserve.

This is investigation-only. The actual fix (change mdict(m)
in three places) is recorded in the investigation doc and proposed as
a follow-up PR so review can focus on the diagnosis first.

Verification

$ python -m pytest tests/test_metadata_reshape_bug.py -v
====================== 9 passed in 1.47s =======================

$ python -m pytest tests/ -x -q --ignore=tests/benchmarks
====================== 2606 passed, 35 skipped =================

Architectural follow-ups

Recorded at the end of the investigation doc:

  1. Land the non-aliasing fix in a separate small PR.
  2. After that ships and a real rebuild confirms the fix, drop the
    redundant repair-layer sanitizers and keep the chokepoint as the
    sole owner of the "no empty dict reaches chromadb" contract — the
    chokepoint is the structurally-correct layer for this invariant.

🤖 Generated with Claude Code

The chokepoint sanitizer in `ChromaCollection._sanitize_metadatas_for_chromadb`
returns a new outer list but **aliases** each non-empty caller dict by
reference. The comprehension `[m if (...) else sentinel for m in metadatas]`
returns `m` itself whenever the guard passes, so the sanitized list still
points at the caller's dicts. Any in-place mutation that empties one of
those dicts between sanitize and chromadb's `validate_metadata` slips
through unchecked — which is exactly the failure mode reported at
~120K/151K rebuilds.

The same aliasing pattern appears in `repair.py:_extract_drawers`
and `repair.py:_rebuild_one_collection`.

Reproduction: tests/test_metadata_reshape_bug.py — 9 tests, including
the regression demonstrating that post-sanitize mutation triggers the
exact `ValueError: Expected metadata to be a non-empty dict in add`
reported in #32.

Proposed minimal fix (separate PR, recorded in the investigation doc):
change `m` → `dict(m)` in three places. ~0.15s amortized cost on a
151K-drawer rebuild; eliminates the failure mode regardless of which
specific mutator triggered it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 23, 2026 00:58
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jphein
jphein merged commit c108c2f into main May 23, 2026
8 of 9 checks passed
@jphein
jphein deleted the investigate/metadata-reshape-32 branch May 23, 2026 01:02
jphein added a commit that referenced this pull request May 23, 2026
PR #140 (metadata reshape investigation) added 9 reproduction tests.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants