Retrieval scored every re-summarised node on its first summary - #532
Merged
Conversation
ContextQuerySummaryVectors asked load_context_summaries for Some(1) and took the front of the range. The summary series is keyed by context_timeline_key, which is timestamp_ms * FANOUT + disambiguator and so ascends with time, meaning the front of the range is the OLDEST summary in the window, not the newest. ContextUpsertSummary keys the series by valid_from_ms * FANOUT + level, so a node re-summarised at a later time accumulates an entry rather than replacing one. Every node that had been re-summarised was therefore scored on a superseded embedding -- and a stale vector is the same width as a current one, so it yields an ordinary cosine with no error, no decode failure and nothing in the logs. load_newest_context_summary walks the same range backwards and returns the first entry satisfying valid_from_ms <= as_of_ms, decoding one record in the ordinary case. load_latest_context_summary was already correct but decodes the node's whole window, which is wrong for a path that runs once per candidate. load_context_summaries keeps its take-from-the-front semantics for ContextQuerySummaries, where a limit on a windowed listing should mean that. Also adds a counting global allocator behind the alloc-probe feature, off by default. Process RSS conflates allocated-and-held, freed-but-retained and non-heap memory -- 71 percent of the proxy's resident memory measured as allocator retention -- so a request-level memory change cannot be read off RSS. Counting the calls is how this bug was found: per additional retrieve candidate the cost is 32.5 allocations, split 12.0 for fetching whole nodes against 19.0 for the summary-vector pass, and the call returning the least data costing the most is what prompted reading it. Feature-gated rather than always-on in tests, and that is not incidental: two atomics on every allocation in the process turned socket-bound proxy and raft tests red in a full single-threaded run -- tests that pass individually, as a group, and in a run without it. An instrument that changes the outcome of the suite verifying the change is worse than no instrument. The gate then creates its own trap, since with the feature off every counter reads zero and the harness would print a table of zeros that reads as 'this path allocates nothing', so it allocates something known first and asserts the counter moved, and the probe's self-tests are gated too rather than asserting a property the default build deliberately does not have. Mutation-tested. Dropping the .rev() reproduces the original bug and fails. Unbounding the range and dropping the valid_from_ms filter each PASS alone, because as_of_ms is enforced twice over; removing both fails.
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
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.
Retrieval scored every re-summarised node on its first summary vector, not its current one.
The defect
ContextQuerySummaryVectorsasked for one summary and took it from the front of the range:load_context_summarieswalksseries.range(0..context_timeline_end(as_of_ms)).take(limit), andthe series is keyed by
which ascends with time. So the first entry in the range is the oldest summary in the window,
not the newest. The arm's own comment above the call said "Only the newest summary at or before
as_of_msis consulted", and the module comment on the neighbouring range helper says "oldestfirst" — the two disagreed, and the code followed the second.
ContextUpsertSummarykeys the series byvalid_from_ms * FANOUT + level, so a node re-summarisedat a later time accumulates a new entry rather than replacing the old one. Every node that has
ever been re-summarised was therefore scored on a superseded embedding.
Why nothing surfaced it
A stale vector is the same width as a current one, so it produces a perfectly ordinary cosine.
There is no length mismatch, no decode failure, no log line — the only symptom is that retrieval
quality drifts toward each node's first-ever summary as the corpus matures, which looks like the
model being mediocre rather than like a bug.
The fix
load_newest_context_summarywalks the same range backwards and returns the first entry thatsatisfies
valid_from_ms <= as_of_ms. In the ordinary case that decodes exactly one record — thesame cost as before, with the right answer.
load_latest_context_summaryalready returned the correct summary, but it does so by decoding thenode's whole window and taking a
max_by_key, so it was not the right thing to call from a paththat runs once per candidate.
load_context_summarieskeeps its existing take-from-the-frontsemantics, because
ContextQuerySummariesis a windowed listing where that is what a limit shouldmean.
Tests
a_resummarised_node_scores_on_its_newest_vector_not_its_firstwrites two versions of one node'ssummary with deliberately opposite vectors, so choosing the wrong one cannot be mistaken for a
rounding difference. It asserts two things that pull against each other:
as_of_msdecorativeBoth were checked against deliberately broken code, because a test that passes on correct code has
not yet been shown able to fail:
.rev()dropped — the original oldest-first behaviour0..u64::MAX), alonevalid_from_ms <= as_of_msfilter dropped, aloneThe two middle rows are worth keeping rather than tidying away. Neither fails alone because
as_of_msis enforced twice over: walking backwards, the range bound excludes a not-yet-validentry, and so does the filter, so removing either leaves the other doing the job. Only removing
both leaves the constraint unenforced, and then the assertion fires.
That is a fact about the code, not a weakness in the test — but it is only knowable by running the
mutations. Taken one at a time they would have read as "this assertion guards nothing", which is
exactly the wrong conclusion to draw about a redundant safety property.
How it was found, and the instrument that found it
This also adds a counting global allocator, because attributing it is what surfaced the bug. Per
additional retrieve candidate the cost is 32.5 allocations, split:
ContextNodeper candidate)The call that returns the least data — just
(node_hash, vector)— was costing the most, which iswhat made me read the arm.
RSS cannot substitute for this: it conflates allocated-and-held, freed-but-retained and non-heap
memory, and 71% of this system's proxy resident memory measured as allocator retention rather than
live data. Counts also do not move with machine load, which matters on a host that sits between
load 5 and 30.
It is behind the
alloc-probefeature and off by default, which is not incidental. Installedunconditionally in test builds, a wrapper adding two atomics to every allocation in the process
turned socket-bound proxy and raft tests red in a full single-threaded run — tests that pass
individually, as a group, and in a run without it. An instrument that changes the outcome of the
suite verifying the change is worse than no instrument.
The gate then creates its own trap: with the feature off every counter reads zero, and the harness
would print a tidy table of zeros that looks exactly like "this path allocates nothing". So the
harness allocates something known first and asserts the counter moved, and the probe's own
self-tests are gated on the feature too rather than asserting a property the default build
deliberately does not have.