Skip to content

chat: embedding collection names are not collision-safe, and the run collection has no model guard #447

Description

@amd-vivekag

Raised twice by Copilot on #422 (suppressed both times, at src/aorta/chat/rag/embeddings/base.py:64). Filing rather than fixing in #422, because the fix renames every collection and that is a format decision worth its own review — see the timing note at the bottom.

The problem

build_collection_name slugifies the model id and truncates it:

def build_collection_name(prefix: str, model: str) -> str:
    return (prefix + model_slug(model))[:MAX_COLLECTION_NAME].rstrip("_")

model_slug maps every run of non-alphanumerics to _, so it is not injective, and the 63-character cap makes it less so:

  • foo/bar and foo-bar both become foo_bar.
  • Two long model ids sharing their first ~47 characters after the prefix produce the same name.

A collection name is the identity that separates one model's vector space from another's. Two models that collide here, and happen to share a dimension, read each other's vectors.

Why the source collection is mostly fine and the run collection is not

The source collection is covered: the manifest records embedding_model, and manifest.validate() refuses on a mismatch before the first query. A collision is caught there.

The run-artifact collection has no manifest. rag/runs.py::_get_store opens run_collection_name() and queries it if it exists — nothing compares the model that wrote those vectors against the one now querying. So after a model switch whose slug collides at an equal dimension, search_run_artifacts answers from the previous model's vectors, silently. That is the failure mode Decision 20a exists to prevent, reached by the one door that has no check on it.

Two things to decide

  1. Make the name injective. Include a stable digest of the full model id within the 63-character budget, e.g. prefix + slug[:budget] + "_" + sha256(model)[:10].
  2. Give the run collection a model guard of its own. The registry table already carries a row per collection; recording the effective model id there and refusing on mismatch would close the hole independently of naming, and would also cover any future collection that is not manifest-backed.

(1) alone narrows the trigger to a hash collision. (2) alone closes the actual silent-read. They are complementary and (2) is the load-bearing one.

Timing

(1) renames every collection, which invalidates any index built before it — the manifest check turns that into a clear refusal telling the user to rebuild, so it is not a silent break, but it is a break. aorta chat has not shipped yet, so right now there are no published assets and no installed users to invalidate. This is the cheapest it will ever be. If #422's reviewers would rather take it before merge than after release, that is a reasonable call and I am happy to do it there instead.

Blast radius

  • 2 production callers: embeddings/fastembed_bge.py:264, embeddings/remote_api.py:46.
  • Hard-coded expected names in tests/chat/test_embeddings_factory.py, test_fastembed_provider.py, test_index_fetch.py, test_index_manifest.py, test_remote_auth.py.
  • No workflow or script hard-codes a collection name.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions