Skip to content

fix: stop queries from mutating the in-memory alias index - #349

Open
Kevin Turcios (KRRT7) wants to merge 4 commits into
microsoft:mainfrom
KRRT7:fix-alias-index-mutation
Open

Kevin Turcios (KRRT7) wants to merge 4 commits into
microsoft:mainfrom
KRRT7:fix-alias-index-mutation

Conversation

@KRRT7

Copy link
Copy Markdown
Contributor

Summary

  • TermToRelatedTermsMap (in-memory alias index) returned and stored its own Term objects. Search preparation lowercases and reweights related terms in place, and dedupe_related_terms raises a related term shared by several search terms to its maximum weight, so running a query permanently changed stored alias weights and text.
    • Example: aliases car → auto (0.5) and vehicle → auto (0.9); after an OR query for car and vehicle, lookup_term("car") returned auto (0.9).
  • Copy terms in lookup_term and add_related_term, matching the SQLite alias index, which already builds fresh Term objects.

Validation

  • New tests, run against both the memory and SQLite backends: mutating a looked-up term, mutating the caller's term after add_related_term, and running resolve_related_terms twice must leave the aliases unchanged. The memory cases fail without the fix.
  • make format check test

Kevin Turcios (KRRT7) and others added 2 commits October 18, 2026 06:05
TermToRelatedTermsMap returned and stored its own Term objects. Search
preparation lowercases and reweights related terms in place, and
dedupe_related_terms raises shared related terms to their maximum weight,
so running a query permanently changed stored alias weights and text.
Copy terms on lookup and on add, matching the SQLite alias index.

Co-authored-by: wtfyd-agent[bot] <328989958+wtfyd-agent[bot]@users.noreply.github.com>

@bmerkle Bernhard Merkle (bmerkle) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM — verified locally.

  • Confirmed the root cause: lookup_term returned the stored Term objects, and search preparation lowercases text (search.py:552), resets weights (search.py:541), and dedupe_related_terms raises shared terms to their max weight (collections.py:599), all in place, so queries permanently rewrote the alias map.
  • Copy-on-add plus copy-on-lookup closes both aliasing directions and brings the memory backend in line with SQLite, which already builds fresh Terms.
  • The 3 new memory-backend tests fail against main's related_terms_index.py and pass with the fix; full make (format, pyright, 817 tests, build) is green on aec1bd5.

Optional nit: if Term ever gains fields, both Term(term.text, term.weight) copies would silently drop them; dataclasses.replace(term) would be future-proof. Not blocking.

@KRRT7

Copy link
Copy Markdown
Contributor Author

Thanks! Took the nit: both copies now use dataclasses.replace(...) (b0c3e3c), and merged the latest main. make format check test is green.

This branch has not been deployed

No deployments
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