Repository navigation
Conversation
hnswEfSearch raised small candidate scans to 200 but never capped large ones. pgvector rejects an hnsw.ef_search above 1000, so set_config failed and the whole candidate scan errored. Watch Tonight's genre filter asks for five times its pool (240 x 5 = 1200 at the default limit), so its taste-profile candidates were dropped, and For You cluster rows hit the same error once their limit passed 1000. Clamp ef_search to 1000. With relaxed_order iterative scans the index keeps returning rows past ef_search, so a larger LIMIT still reads past 1000 index entries. The new DB contract calls FindTasteProfileCandidates with a genre and a pool of 240 on connections that have loaded pgvector, as a reused pooled connection has. It fails on main with SQLSTATE 22023 and passes here.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRecommendation candidate scans now cap ChangesRecommendation candidate search
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The ef_search cap and its boundary tests present no identified merge-blocking issue; merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Problem
Related issue: N/A
Validation tasks: changes #1145 C1, which generates recommendations and hasn't been run yet.
Recommendation candidate scans fail outright when they ask pgvector for more than 1,000 candidates.
hnswEfSearchraises small scans to anhnsw.ef_searchof 200 but never caps large ones, and pgvector rejects anything above 1,000:set_configfails, so the whole candidate query never runs. Watch Tonight's discover with a genre filter asks for five times its pool, which is 240 × 5 = 1,200 at the default limit (watch_tonight_cards.go). The handler logs a warning and drops its taste-profile candidates, so the cards fall back to the other sources. For You cluster rows hit the same error once their limit passes 1,000.This clamps
hnsw.ef_searchto 1,000.#1966 (draft) contains the same clamp, as part of a much larger change. This is the one-line fix with a regression test, so main stops failing now. If this merges first, #1966 can take main's version on rebase.
Approach
hnswEfSearchreturnsmin(max(limit, 200), 1000). Withhnsw.iterative_scan = relaxed_order, which every caller sets, the index keeps returning rows pastef_search. I checked that locally: withef_search1000 andLIMIT 1200, the HNSW index scan read 2,105 index entries rather than stopping at 1,000.Validation
TestFindTasteProfileCandidatesWithGenresAboveEfSearchMaximumDBcallsFindTasteProfileCandidateswith a genre and a pool of 240, on connections that have already loaded pgvector, as a reused pooled connection has. On a connection that hasn't, PostgreSQL accepts the value, then only warns when pgvector loads and falls back to the defaultef_searchof 40, which the clamp also avoids. Onmainthe test fails with the SQLSTATE 22023 error above. On this branch it passes and returns the seeded candidates. Added toscripts/ci/db-contracts.txt.TestHNSWEfSearchStaysWithinPgvectorRange(renamed fromTestHNSWEfSearchUsesCandidateLimitFloor) adds 1000, 1001, 1200 and 2000.go test ./internal/recommendations/against a migrated PostgreSQL 18 database with pgvector 0.8.7 passes.make lint-changedreports 0 issues.Benchmarks
Not applicable: this turns an error into a working query and changes no query plan.
Evidence
The same
set_configcall against my server's database (read-only transaction, pgvector 0.8.2, after loading the extension in the session):My server's retained logs (since its last restart on 7 October) don't contain this error. So I can't show a before/after of a real Watch Tonight response. What users see when it does happen is a genre-filtered Watch Tonight without its taste-profile picks. The after evidence is the regression test above. Its raw output:
Tests on main and on this branch
Risks
ef_searchof 1,000 and rely on the iterative scan for the rest. They previously failed.Checklist
AI Disclosure
AI-assisted with Claude Opus. I directed the task and designed the work.