fix(retrieval): widen exclusion until the search is exhausted, and say when it isn't (#1205) - #1206
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdjusts supersession exclusion retrieval so widening continues until search exhaustion instead of stopping after a fixed number of rounds, adds explicit stderr signalling when the widening cap binds, raises the cap, and adds tests plus changelog notes to pin the new behavior and guard against regressions. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Kulili:2026-07-30T19:27:08Z] |
|
Reviewed against the branch, not the description. Verified:
On the substance: raising a cap from 3 to 8 would normally just move the cliff, and the reason it doesn't here is One thing to know, not a defect in this PR: the Nothing for you to do here; do not add it to this PR's scope. I am fixing it separately so it does not reach the release notes twice. Approving. |
|
[release:review:Kulili:2026-07-30T19:29:29Z] |
|
[claim:review:Gylf:2026-07-30T19:34:00Z] |
…y when it isn't The three-round cap was the normal termination, not a backstop. At l1_limit=50 the rounds reach 200, so a query whose 200 strongest matches are all retired returned an empty pack while current ones sat below it: superseded/matching available returned (before) after 150/300 150 50 50 200/300 100 0 50 250/300 50 0 50 That is the starvation #1187 set out to remove, moved out 4x rather than removed — my bound, introduced while fixing the original. Worse, it truncated silently. Nothing distinguished 'the store holds two survivors' from 'I stopped after three rounds with a hundred unread', which is the silent-cap failure #1160 is open about, and which I argued against on #1184 before shipping it a PR later. The cap is now high enough that binding means something pathological, and binding traces to stderr naming the limit reached and how many survivors were found. The comment's 'strictly more than the pre-#1187 behaviour' claim is corrected: at 200/300 both returned 0, so it was 'at least as much'. Closes #1205.
65a1ade to
a8e0f93
Compare
|
merge-train: merged a8e0f93 → |
|
[release:review:Gylf:2026-07-30T19:48:15Z] |
Closes #1205. A defect in my own #1191 fix, found while auditing this session's changes after #1202 turned up a different one in the same batch.
The defect
#1187 fixed the exclusion arm shrinking the pack, by widening the candidate fetch and retrying. I bounded that at three rounds. Three rounds made the bound the normal termination rather than a backstop:
At
l1_limit=50the rounds reach 200. A query whose 200 strongest matches are all retired returns nothing, while a hundred current, matching beliefs sit below the widest fetch attempted. The starvation #1187 set out to remove was moved out 4×, not removed.The worse half
It truncated silently. Nothing distinguished "the store holds two survivors" from "I stopped after three rounds with a hundred unread." That is the silent-cap shape #1160 is open about — and I argued against exactly it on #1184, declining a
heapq.nlargestsuggestion partly because its magic cutoff dropped candidates with no counter and no log line, before shipping the same thing one PR later.The code comment asserted the wrong thing too:
At 200/300 it returned 0, and pre-#1187 also returned 0. "At least as much" holds; "strictly more" does not. Corrected.
What ships
Widening continues until the search is genuinely exhausted (
len(rows) < limit). The round cap stays, raised to 8, but is now a backstop: reaching it means something pathological, and reaching it traces to stderr naming the limit reached and the survivor count, so a short pack is never mistaken for an empty store.Verification
min(l1_limit, available)rather than zero. Reverting the cap to 3 fails the 200 and 250 cases.aelf eval --jsonbyte-identical to the pinned baseline (the ci: trigger eval-calibration on the whole package (#1160) #1195 gate bindssrc/aelfrice/**).Recorded for the bench, not fixed here
The exclusion arm costs more queries than demote on almost any store containing retired beliefs at all: with 2 of 300 retired at
l1_limit=50it already widens once (48 survivors < 50 requested), measuring 3.4 ms against demote's 1.7 ms on the same store. That asymmetry is inherent to filter-then-refetch and should be known before arm latency is compared, since it is the same class of confound as the pack-size difference #1191 removed.Pushing exclusion into SQL (
WHERE id NOT IN (...)) would remove the refetch entirely and is the better end state, but it needs the superseded set before the candidate query rather than after — a different shape from what both arms share today. Left for its own issue if the bench picks exclusion; noted in #1205's out-of-scope section.Summary by Sourcery
Prevent supersession exclusion from silently truncating retrieval results when retired beliefs dominate a query, and surface a diagnostic when the widening cap binds.
Bug Fixes:
Enhancements:
Documentation:
Tests: