Put the snippet-backfill ANN tests on their own embedding axis (#645) - #747
Merged
Conversation
CI failed on "a system-scope canonical gets a lead too, not a bare title" at its retrievability precondition - the ANN read returned no row for an article the test had just inserted - and passed on rerun. That is the failure data_case.ex already documents. The sandbox rolls each test back, a rolled-back INSERT leaves a dead entry in the shared pgvector HNSW graph, and a live row that links only to dead neighbours becomes UNREACHABLE, so the read returns nothing while a count on the same connection shows the row present. It is reproduced deterministically in embeddings/hnsw_dead_entry_recall_test.exs. The file put itself in the way of it. Every vector in it was the literal [1.0 | zeros] - dimension 0, shared by all ten of its tests and by any peer reaching for the obvious vector - while DataCase.setup already assigns each test its own sparse axis and exposes it as test_vec/2 precisely so tests do not share a region of the graph with each other's corpses. The literal opted out of the protection the harness provides. Now it uses the axis. WHY NOT THE VACUUM TAG. @moduletag :vacuum_vector_indexes is the other documented repair and it works, but it is not free, and the number is worth recording because it is invisible when you measure one file: full suite, no tag 37.9s full suite, tag on this ONE module 56.2s (+50%) full suite, tag on 24 ANN-reading modules 744s (18.6x) The per-call cost is ~16ms on an idle machine. The real cost is that VACUUM contends with every other async worker, so it is superlinear in the number of vacuuming modules and it taxes the WHOLE suite, not the module that opts in. The axis costs nothing: 37.97s against a 37.85s baseline, 7827 tests green. So the ordering is: put ANN assertions on their own axis, and reach for the vacuum only where they genuinely cannot be. That reasoning is recorded next to the helper it replaced, with the measurements, so the next person does not re-derive it from a red CI run.
The previous commit fixed the correct thing for a reason the KB explicitly lists as a DEAD THEORY. Correcting it before it lands, because a wrong mechanism in a comment is read as settled by whoever hits this next. I wrote that a rolled-back INSERT leaves a dead entry in the HNSW graph and the live row becomes unreachable. KB c6fd1e5d - which opens "read this whole article before touching the flake, it has been misdiagnosed twice and each misdiagnosis cost multiple reverted fixes" - disproves that by probe: pgvector returns the live row past 2000 rolled-back distance-0 ties, because it skips invisible tuples and keeps scanning. The mechanism that DOES fit this file is the distance-0 clique. All ten of its tests shared the literal [1.0 | zeros], so their rows are exact ties, and under concurrent load the graph walk cannot navigate that clique and non-deterministically misses exact matches. That is KB 4e5ccb17 and 0bdadd52, and it is the same reason DataCase's default MockEmbeddingClient stub keys deterministic_embedding/1 on tenant_id instead of returning a constant. The fix does not change - test_vec/2 is right either way, and it is right for the better-supported reason. What changes is the explanation, which now cites the articles and warns the next reader off the dead theory rather than restating it. Also worth recording, since I did it in the wrong order: that article's prescribed response to this failure shape is two checks then a rerun - does the diff touch the READ path (vector_search, heavy_read, embeddings, system_config_read_path), and does the file pass 5x locally. The diff that hit this was a route-index change containing no read-path file at all, which leg 1 settles in one command. I ran the reruns and the local passes, then reasoned about mechanism from the data_case docstring alone instead of searching for the article that already had it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
CI failed on
SnippetBackfillTest, "a system-scope canonical gets a lead too, not a baretitle", at its retrievability precondition — the ANN read returned no row for an article
the test had just inserted — and passed on rerun. It surfaced on an unrelated PR (#746, a
route-index change), which is how this class of flake wastes time: it reads as "your change
broke something".
Mechanism — the distance-0 clique
All ten tests in this file shared the literal
[1.0 | zeros]for both the stored embeddingsand the query. Rows at an identical vector are exact ties, and under concurrent full-suite
load the HNSW graph walk cannot navigate that clique — it non-deterministically misses exact
matches. That is KB
4e5ccb17and0bdadd52("the fix is per-tenant mock embeddings, NOTraising ef_search"), and it is the same reason
DataCase's defaultMockEmbeddingClientstub keys
deterministic_embedding/1ontenant_idrather than returning a constant.Correction, made before merge. My first commit on this branch explained it as a
rolled-back INSERT leaving a dead graph entry that makes the live row unreachable. KB
c6fd1e5dlists that as a dead theory, disproved by probe: pgvector returns the liverow past 2000 rolled-back distance-0 ties, because it skips invisible tuples and keeps
scanning. That article opens by saying this family has been misdiagnosed twice and each
misdiagnosis cost multiple reverted fixes — so the second commit replaces the explanation
and warns the next reader off it. The fix itself is unchanged:
test_vec/2is correcteither way, and now correct for the better-supported reason.
The fix
DataCase.setupalready assigns each test its own sparse axis and exposes it astest_vec/2, precisely so tests don't collide in the shared index. The literal opted out ofit. It now uses the axis.
Why not
@moduletag :vacuum_vector_indexesThe other documented repair. It works and it is not free — the cost is invisible if you
measure one file:
Per-call cost is ~16ms on an idle machine. The real cost is
VACUUMcontending with everyother async worker, so it's superlinear in the number of vacuuming modules and taxes the
whole suite, not the module that opts in. The axis is free: 37.97s against a 37.85s
baseline, 7827 tests green.
Scope — what I checked and did not change
test_vec/2.MockEmbeddingClientstub is tenant-keyed and tenants are per-test, soeverything reaching the index through
Memory.remember/2or article ingest is alreadyseparated.
embeddings_test.exsis the one confirmed other instance (vec(dim) = sin(i/dim), denseand identical across tests). It is its own module with its own dimension-migration
assertions; it gets its own branch rather than being bundled here.
Verification
Full gate green on both commits: 7827 tests, 0 failures (86 excluded), credo
--strictclean, dialyzer clean. Timing measured three ways above, A/B'd under identical conditions.
Review
Reviewed inline, then corrected against the KB — which is what caught the wrong mechanism.
The change is one helper in one test file, established by measurement rather than argument.