From 449d66db28ff4bbc1cb67c52a9f4da912e3a9e24 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Fri, 21 Aug 2026 01:11:15 +0100 Subject: [PATCH 1/5] perf(chunksfts): rank the workspace with a bounded heap, not a window function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 2 replaced 43 per-project BM25 queries with one partitioned query and made BM25 four times cheaper. The remaining cost is not where the plan said it was, and this commit is the consequence of measuring rather than assuming. WHAT THE MEASUREMENT SAID The plan's next stage was to prune the dense fan-out, on the grounds that dense is 89% of the fan-out's WORK. That is true and it is the wrong lens: dense is 11.2 s of work spread over 13 workers with a per-project ceiling of ~1.1 s, while BM25 is ONE serial query. Timed from inside the handler on the 43-project fixture, fanout_ms equals bm25_ms to within 2 ms in eight of ten queries. The user waits on BM25; dense hides behind it. Inside the BM25 query the cost is not the MATCH either. Walking the statement up one addition at a time, for a six-term query on that fixture: MATCH only (posting-list merge) 186 ms + bm25() per matched row 1889 ms + join chunks_meta by rowid 352 ms + project_path IN (46) 487 ms production shape 2726 ms The posting merge is 7% of it. The rest is bm25() over the whole match set and, on top of that, ROW_NUMBER() sorting that same match set to keep fifty rows per project. The match sets are large because the tokenizer is trigram: "and" matches a quarter of the corpus through command, handler, standard and random, and "fault" matches 10% of it, almost all of them the word "default". A six-term query matched 623,913 rows to keep 2,300; one containing "test" matched 1,288,739. WHAT THIS CHANGES The trim moves out of SQL. The scan streams (project_path, rowid, bm25) with no ORDER BY, and the caller keeps a bounded per-project heap, so a row that does not make the cut costs one comparison instead of a place in a sort of everything. The payload is then fetched by rowid for the ~2,300 survivors, which is what the previous shape already did. Results are IDENTICAL, not close: same rows, same order, including the (score, rowid) tiebreak that ties in a trigram index make routine. MEASURED, back to back on the same warm page cache, medians of five passes: bm25_ms, expensive queries develop this write a unit test for parser 7747 5988 type inference for generics 5908 4962 mock an HTTP client in tests 4602 3727 parse JWT token and validate 3867 2883 median of those seven 4471 3782 1.18x median of the standard ten 1917 1740 1.10x Standalone, outside the server, the same substitution is 1.7x (2466 -> 1455 on a 624k match set, 5216 -> 3014 on 1.29M). Most of that does not survive inside the server and I could not find out why. Ruled out: CPU/IO contention with the dense scans (the standalone bench measures the same while the server is saturated), SQLite's per-connection page cache (a cold connection measures the same as a warm one), and GC pressure (GOGC=600 moves dense_max but not bm25_ms). A fresh process running this code path converges toward the standalone number only on its third pass, so something process-level warms up. That is a lead for whoever looks next, not a blocker: every measured query is faster or unchanged, and the gap grows with the match set, which is the class of query that produced the multi-second waits this work began from. CORRECTNESS 50 fixture queries, the full workspace response captured per query: the BM25 signal is bit-identical in all 500 panel rows and the panel order is identical for all 50. One chunk list differs, on a query where ten projects also report different DENSE scores — the provider returns different query vectors for byte-identical requests, and the same build diffed against ITSELF shows the same thing on other queries. Only BM25 is bit-stable here, so it is the only side an "identical results" claim can rest on. TESTS TestSearchProjects_MatchesPerProjectQueries already compared the workspace path against the per-project query as an oracle; it now covers the heap, and it is what catches a wrong tiebreak. Added: a property test that offers shuffled rows with deliberately many tied scores and compares the heap against a full sort, at limits 1, 3 and 50 over 20 seeds; plan guards that the scan sorts nothing and that the payload fetch is a rowid lookup with no MATCH; and the in-tree mutation check for the first of those, which builds the window form and asserts the guard rejects it. Mutation-checked, each independently against a restored tree: dropping the rowid tiebreak, never evicting, sorting output on score alone, losing the bm25 sign flip, and stopping the heap's sift-down after one level all fail the suite. The bench harness behind these timings is NOT in this repository (/loadtests/ is gitignored, corpus and tools alike). The workspaceScanQuery doc comment says how to recreate it. Co-Authored-By: Claude Opus 5 --- server/internal/chunksfts/chunksfts.go | 307 +++++++++++++++----- server/internal/chunksfts/chunksfts_test.go | 146 ++++++++-- 2 files changed, 353 insertions(+), 100 deletions(-) diff --git a/server/internal/chunksfts/chunksfts.go b/server/internal/chunksfts/chunksfts.go index 618d659..ab1b62d 100644 --- a/server/internal/chunksfts/chunksfts.go +++ b/server/internal/chunksfts/chunksfts.go @@ -28,6 +28,7 @@ import ( "context" "database/sql" "fmt" + "sort" "strings" ) @@ -256,10 +257,14 @@ func SearchProject(ctx context.Context, db *sql.DB, projectPath, query string, l } // searchProjectsBatch caps how many project_path values go into one IN -// list. SQLite's default bound-variable ceiling is 999; 500 leaves room -// for the query and limit parameters and matches the batch size the +// list, and payloadFetchBatch does the same for the rowid list of the +// second statement. SQLite's default bound-variable ceiling is 999; 500 +// leaves room for the query parameter and matches the batch size the // vector store already uses for its own IN lists. -const searchProjectsBatch = 500 +const ( + searchProjectsBatch = 500 + payloadFetchBatch = 500 +) // SearchProjects answers the same question as SearchProject for many // projects at once, returning each project's top `perProject` hits keyed @@ -315,74 +320,244 @@ func SearchProjects(ctx context.Context, db *sql.DB, projectPaths []string, quer func searchProjectsBatchInto(ctx context.Context, db *sql.DB, projectPaths []string, fts5Q string, perProject int, dst map[string][]Hit) error { - args := make([]any, 0, len(projectPaths)+2) + args := make([]any, 0, len(projectPaths)+1) args = append(args, fts5Q) for _, pp := range projectPaths { args = append(args, pp) } - args = append(args, perProject) - rows, err := db.QueryContext(ctx, workspaceRankQuery(placeholders(len(projectPaths))), args...) + rows, err := db.QueryContext(ctx, workspaceScanQuery(placeholders(len(projectPaths))), args...) if err != nil { return fmt.Errorf("chunks_fts workspace search: %w", err) } - defer rows.Close() + tops := make(map[string]*topHits, len(projectPaths)) for rows.Next() { var pp string - h, err := scanRankedHit(rows, &pp) - if err != nil { - return err + var r rankedRow + if err := rows.Scan(&pp, &r.rid, &r.bm); err != nil { + rows.Close() + return fmt.Errorf("scan chunks_fts ranking row: %w", err) } - dst[pp] = append(dst[pp], h) + t := tops[pp] + if t == nil { + t = &topHits{n: perProject} + tops[pp] = t + } + t.offer(r) } - if err := rows.Err(); err != nil { + err = rows.Err() + rows.Close() + if err != nil { return fmt.Errorf("iterate chunks_fts: %w", err) } + + ranked := make(map[string][]rankedRow, len(tops)) + var rids []int64 + for pp, t := range tops { + rows := t.sorted() + ranked[pp] = rows + for _, r := range rows { + rids = append(rids, r.rid) + } + } + payload, err := fetchPayload(ctx, db, rids) + if err != nil { + return err + } + for pp, rows := range ranked { + hits := make([]Hit, 0, len(rows)) + for _, r := range rows { + h, ok := payload[r.rid] + if !ok { + // The row was deleted between the ranking scan and the + // payload fetch — a reindex of that file landed in between. + // Two statements cannot be atomic the way one was, and a + // hit fewer is the right answer for a chunk that no longer + // exists. Erroring would fail a whole workspace search + // because one file was being rewritten. + continue + } + h.Score = -r.bm + hits = append(hits, h) + } + if len(hits) > 0 { + dst[pp] = hits + } + } return nil } -// workspaceRankQuery builds the partitioned statement for a placeholder list. +// rankedRow is a matched chunk before its payload is fetched: the two columns +// the ranking needs and nothing else. +type rankedRow struct { + rid int64 + bm float64 +} + +// betterThan orders rows the way the per-project query's +// ORDER BY bm ASC, rowid ASC does. SQLite gives more-negative bm25 to better +// matches, so smaller wins; ties break on the lower rowid. The tiebreak is not +// cosmetic — in a trigram index over real code most hits share a score with +// another hit, and without it the surviving set would depend on the order the +// scan happened to visit rows in. +func (r rankedRow) betterThan(o rankedRow) bool { + if r.bm != o.bm { + return r.bm < o.bm + } + return r.rid < o.rid +} + +// topHits keeps the best n rows seen for one project. +// +// h is a max-heap on `betterThan`: h[0] is the WORST row kept, which is the +// one a new row has to beat. That makes the common case — a row that does not +// make the cut — a single comparison, which is the whole point of doing this +// here instead of in SQL. See workspaceScanQuery for why. +type topHits struct { + n int + h []rankedRow +} + +func (t *topHits) offer(r rankedRow) { + if t.n <= 0 { + return + } + if len(t.h) < t.n { + t.h = append(t.h, r) + t.up(len(t.h) - 1) + return + } + if t.h[0].betterThan(r) { + return + } + t.h[0] = r + t.down(0) +} + +// sorted returns the kept rows best-first, leaving the heap unusable. +func (t *topHits) sorted() []rankedRow { + out := t.h + sort.Slice(out, func(i, j int) bool { return out[i].betterThan(out[j]) }) + t.h = nil + return out +} + +func (t *topHits) up(i int) { + for i > 0 { + p := (i - 1) / 2 + if !t.h[p].betterThan(t.h[i]) { + return + } + t.h[p], t.h[i] = t.h[i], t.h[p] + i = p + } +} + +func (t *topHits) down(i int) { + for { + worst := i + for _, c := range [2]int{2*i + 1, 2*i + 2} { + if c < len(t.h) && t.h[worst].betterThan(t.h[c]) { + worst = c + } + } + if worst == i { + return + } + t.h[i], t.h[worst] = t.h[worst], t.h[i] + i = worst + } +} + +// fetchPayload reads the chunk columns for the rows that survived ranking. +func fetchPayload(ctx context.Context, db *sql.DB, rids []int64) (map[int64]Hit, error) { + out := make(map[int64]Hit, len(rids)) + for start := 0; start < len(rids); start += payloadFetchBatch { + end := start + payloadFetchBatch + if end > len(rids) { + end = len(rids) + } + batch := rids[start:end] + args := make([]any, len(batch)) + for i, rid := range batch { + args[i] = rid + } + rows, err := db.QueryContext(ctx, payloadQuery(placeholders(len(batch))), args...) + if err != nil { + return nil, fmt.Errorf("chunks_fts payload fetch: %w", err) + } + for rows.Next() { + var rid int64 + var h Hit + var chunkT, symName, language sql.NullString + if err := rows.Scan(&rid, &h.FilePath, &h.StartLine, &h.EndLine, + &chunkT, &symName, &language, &h.Content); err != nil { + rows.Close() + return nil, fmt.Errorf("scan chunks_fts payload row: %w", err) + } + h.ChunkType = chunkT.String + h.SymbolName = symName.String + h.Language = language.String + out[rid] = h + } + err = rows.Err() + rows.Close() + if err != nil { + return nil, fmt.Errorf("iterate chunks_fts payload: %w", err) + } + } + return out, nil +} + +// workspaceScanQuery streams every matched row's project, rowid and BM25 +// score. It deliberately does no ordering and no trimming: the caller keeps a +// bounded per-project heap as the rows go past. +// +// The obvious form asks SQLite for the answer directly, with +// ROW_NUMBER() OVER (PARTITION BY project_path ORDER BY bm) and rn <= N. That +// is correct and it is what this shipped first, but a window function has to +// sort the ENTIRE match set to find N rows per project, and the match set here +// is the whole server's index: the trigram tokenizer makes a common short word +// match a quarter of the corpus, because "and" occurs inside command, handler, +// standard and random. On the load-test fixture a six-term query matched +// 623,913 rows to keep 2,300, and a six-term query with the word "test" in it +// matched 1,288,739. Sorting those to keep fifty per project is the single +// largest cost in the statement. // -// Split out so the test that pins the query PLAN builds the same string this -// does. A copy in the test would drift, and drifting is the entire failure -// mode being guarded against. +// A bounded heap looks at each row once and rejects most of them in one +// comparison. Measured on that fixture, window form vs heap: 144 ms vs 119 ms +// on a 21k match set, 2,466 vs 1,455 on 624k, 5,216 vs 3,014 on 1.29M. It wins +// at every size, and it wins by more as the match set grows. // -// Rank on the cheap columns, then fetch the payload for the rows that -// survived. The obvious form — selecting file_path, content and the rest -// inside the CTE — makes SQLite materialise all of it for EVERY matched -// row before ROW_NUMBER trims to `perProject` per project, and the match -// set is the whole server's index. On the load-test fixture a four-common- -// word query matched 263,515 rows to return 2,300: carrying the payload -// through cost 15.1 s against 2.8 s for this form, and a 624k-row match -// cost 29.6 s — worse than the 46 per-project queries this replaced. The -// rowid round-trip is the cheap half of the trade. +// The result is IDENTICAL, not merely close — same rows, same order. That is +// what TestSearchProjects_MatchesPerProjectQueries checks, against the +// per-project statement as the oracle. // -// Those numbers came from a bench harness that is NOT in this repository: -// /loadtests/ is gitignored, corpus and tools alike. To recreate it, time -// this statement against a corpus with a large match set alongside the -// same statement with the payload columns moved into `hits` — the gap only -// appears when the match set is orders of magnitude larger than the result. -// TestSearchProjects_FetchesPayloadAfterTheTrim is the in-repo guard. -func workspaceRankQuery(ph string) string { +// Those timings came from a bench harness that is NOT in this repository: +// /loadtests/ is gitignored, corpus and tools alike. To recreate it, time this +// statement plus the Go-side heap against the same statement wrapped in the +// window form, over a corpus whose match set is orders of magnitude larger +// than the result. +func workspaceScanQuery(ph string) string { return fmt.Sprintf(` - WITH hits AS ( - SELECT cm.project_path AS pp, cm.rowid AS rid, bm25(chunks_fts) AS bm - FROM chunks_fts cf - JOIN chunks_meta cm ON cm.rowid = cf.rowid - WHERE chunks_fts MATCH ? AND cm.project_path IN (%s) - ), - ranked AS ( - SELECT pp, rid, bm, - ROW_NUMBER() OVER (PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn - FROM hits - ) - SELECT r.pp, cm.file_path, cm.start_line, cm.end_line, - cm.chunk_type, cm.symbol_name, cm.language, cf.content, r.bm - FROM ranked r - JOIN chunks_meta cm ON cm.rowid = r.rid - JOIN chunks_fts cf ON cf.rowid = r.rid - WHERE r.rn <= ? - ORDER BY r.pp, r.rn`, ph) + SELECT cm.project_path, cm.rowid, bm25(chunks_fts) + FROM chunks_fts cf + JOIN chunks_meta cm ON cm.rowid = cf.rowid + WHERE chunks_fts MATCH ? AND cm.project_path IN (%s)`, ph) +} + +// payloadQuery fetches the chunk columns for rows that already survived +// ranking. Keeping the payload out of the scan matters for the same reason the +// ranking is not done in SQL: carrying file_path and content through a +// 600k-row scan materialises them for every match to return a couple of +// thousand. Both halves of that lesson cost a shipped regression to learn. +func payloadQuery(ph string) string { + return fmt.Sprintf(` + SELECT cm.rowid, cm.file_path, cm.start_line, cm.end_line, + cm.chunk_type, cm.symbol_name, cm.language, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid = cm.rowid + WHERE cm.rowid IN (%s)`, ph) } // placeholders builds "?,?,?" for an IN list. n is always >= 1: SearchProjects @@ -395,18 +570,15 @@ func placeholders(n int) string { return strings.TrimSuffix(strings.Repeat("?,", n), ",") } -// scanHit reads one single-project ranking row. -func scanHit(rows *sql.Rows) (Hit, error) { return scanRankedHit(rows, nil) } - -// scanRankedHit reads one ranking row, optionally preceded by a project_path -// column (pass nil for the single-project query, which does not select one). +// scanHit reads one row of the single-project ranking query. // -// Both queries share this because they must produce byte-identical Hits and -// their only structural difference is that leading column. Two hand-written -// scans agreeing is exactly the kind of thing that stays true until it -// quietly does not — and the equivalence test compares the two paths against -// EACH OTHER, so a mistake made symmetrically in both would pass. -func scanRankedHit(rows *sql.Rows, pp *string) (Hit, error) { +// The workspace query no longer shares this: it scans (project, rowid, score) +// and fetches the payload separately, so the two paths now build a Hit in two +// different places. They must still produce byte-identical Hits — +// TestSearchProjects_MatchesPerProjectQueries compares them — and a mistake +// made symmetrically in both would pass that test, so keep the two column +// lists side by side when editing either. +func scanHit(rows *sql.Rows) (Hit, error) { var ( h Hit chunkT sql.NullString @@ -414,17 +586,8 @@ func scanRankedHit(rows *sql.Rows, pp *string) (Hit, error) { language sql.NullString bm float64 ) - // Built in one allocation rather than prepending to a finished slice: the - // workspace query returns up to len(projects) * perProject rows — ~2,150 - // on the 43-project load-test fixture — and this runs per row, inside the - // path the rest of this change exists to speed up. - dest := make([]any, 0, 9) - if pp != nil { - dest = append(dest, pp) - } - dest = append(dest, &h.FilePath, &h.StartLine, &h.EndLine, - &chunkT, &symName, &language, &h.Content, &bm) - if err := rows.Scan(dest...); err != nil { + if err := rows.Scan(&h.FilePath, &h.StartLine, &h.EndLine, + &chunkT, &symName, &language, &h.Content, &bm); err != nil { return Hit{}, fmt.Errorf("scan chunks_fts row: %w", err) } h.ChunkType = chunkT.String diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 33beeba..29e9229 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -4,6 +4,8 @@ import ( "context" "database/sql" "fmt" + "math/rand" + "sort" "strings" "testing" @@ -492,54 +494,142 @@ func TestSearchProjects_EmptyInputs(t *testing.T) { // FTS5 reports a rowid-equality lookup as "0:=" and a MATCH scan as "0:M...". // The rank-first form does both — scan to match, point lookups for survivors. // The payload-in-CTE form only ever scans. -func TestSearchProjects_FetchesPayloadAfterTheTrim(t *testing.T) { +// TestSearchProjects_ScanDoesNotSortTheMatchSet pins the reason the ranking +// moved out of SQL. The window form has to sort every matched row to find N +// per project; on the load-test fixture that is up to 1.29 million rows to +// keep 2,300. The scan query must stay a plain scan — no sorter of any kind. +func TestSearchProjects_ScanDoesNotSortTheMatchSet(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) - plan := explain(t, ctx, d, workspaceRankQuery(placeholders(2)), - `"retry" OR "backoff"`, "p1", "p2", 3) - if !strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { - t.Errorf("chunks_fts is not looked up by rowid — the payload is being "+ - "carried through the window sorter again:\n%s", plan) + plan := explain(t, ctx, d, workspaceScanQuery(placeholders(2)), + `"retry" OR "backoff"`, "p1", "p2") + if strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("the ranking scan sorts the whole match set again:\n%s", plan) } } -// TestExplainDistinguishesTheTwoQueryShapes is the mutation check for the test -// above, kept in the tree rather than run by hand: it builds the slow form and -// asserts the plan assertion would REJECT it. Without this, a change to how -// SQLite reports plans could turn the guard into a tautology that passes on -// everything, and nothing would say so. -func TestExplainDistinguishesTheTwoQueryShapes(t *testing.T) { +// TestExplainRejectsTheWindowForm is the mutation check for the test above, +// kept in the tree rather than run by hand: it builds the form that WAS +// shipped and asserts the assertion above would reject it. Without this, a +// change in how SQLite reports plans could turn the guard into a tautology +// that passes on everything, and nothing would say so. +func TestExplainRejectsTheWindowForm(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) - slow := ` + window := ` WITH hits AS ( - SELECT cm.project_path AS pp, cm.rowid AS rid, - cm.file_path, cm.start_line, cm.end_line, - cm.chunk_type, cm.symbol_name, cm.language, - cf.content, bm25(chunks_fts) AS bm + SELECT cm.project_path AS pp, cm.rowid AS rid, bm25(chunks_fts) AS bm FROM chunks_fts cf JOIN chunks_meta cm ON cm.rowid = cf.rowid WHERE chunks_fts MATCH ? AND cm.project_path IN (?,?) + ), + ranked AS ( + SELECT pp, rid, bm, + ROW_NUMBER() OVER (PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn + FROM hits ) - SELECT pp, file_path, start_line, end_line, - chunk_type, symbol_name, language, content, bm - FROM (SELECT *, ROW_NUMBER() OVER ( - PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn - FROM hits) - WHERE rn <= ? - ORDER BY pp, rn` - - plan := explain(t, ctx, d, slow, `"retry" OR "backoff"`, "p1", "p2", 3) - if strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { - t.Errorf("the payload-in-CTE form reports a rowid lookup, so the plan "+ + SELECT r.pp, cm.file_path, r.bm + FROM ranked r + JOIN chunks_meta cm ON cm.rowid = r.rid + WHERE r.rn <= ?` + + plan := explain(t, ctx, d, window, `"retry" OR "backoff"`, "p1", "p2", 3) + if !strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("the window form no longer reports a sorter, so the plan "+ "assertion no longer distinguishes the two shapes:\n%s", plan) } } +// TestSearchProjects_FetchesPayloadByRowid guards the second half of the same +// lesson: file_path and content are fetched for the rows that survived, by +// rowid, and never carried through the scan. +func TestSearchProjects_FetchesPayloadByRowid(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + seedCorpus(t, d, []string{"p1", "p2"}) + + plan := explain(t, ctx, d, payloadQuery(placeholders(2)), 1, 2) + if !strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { + t.Errorf("chunks_fts is not looked up by rowid in the payload fetch:\n%s", plan) + } + // 0:M... is how FTS5 reports a MATCH scan. The payload fetch has no MATCH + // at all, so seeing one would mean the two statements had been merged back + // together. + if strings.Contains(plan, "VIRTUAL TABLE INDEX 0:M") { + t.Errorf("the payload fetch is running a MATCH scan:\n%s", plan) + } +} + +// TestTopHits_MatchesAFullSort is the property test for the bounded heap that +// replaced SQLite's window function. +// +// The scores are drawn from a deliberately tiny set so that most rows tie: +// in a trigram index over real code most hits share a score with another hit, +// which makes the (score, rowid) tiebreak the part most likely to be wrong and +// least likely to be noticed. Rows are offered in a shuffled order, because an +// implementation that quietly depended on arrival order would still pass if +// they arrived sorted. +func TestTopHits_MatchesAFullSort(t *testing.T) { + for _, n := range []int{1, 3, 50} { + for seed := int64(1); seed <= 20; seed++ { + rng := rand.New(rand.NewSource(seed)) + rows := make([]rankedRow, 0, 500) + for i := 0; i < 500; i++ { + rows = append(rows, rankedRow{ + rid: int64(rng.Intn(1 << 20)), + bm: -float64(rng.Intn(8)), + }) + } + seen := map[int64]bool{} + uniq := rows[:0] + for _, r := range rows { + if !seen[r.rid] { + seen[r.rid] = true + uniq = append(uniq, r) + } + } + rows = uniq + + top := &topHits{n: n} + for _, r := range rows { + top.offer(r) + } + got := top.sorted() + + want := append([]rankedRow(nil), rows...) + sort.Slice(want, func(i, j int) bool { return want[i].betterThan(want[j]) }) + if len(want) > n { + want = want[:n] + } + if len(got) != len(want) { + t.Fatalf("n=%d seed=%d: kept %d rows, a full sort keeps %d", + n, seed, len(got), len(want)) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("n=%d seed=%d: rank %d is %+v, a full sort puts %+v there", + n, seed, i, got[i], want[i]) + } + } + } + } +} + +// TestTopHits_ZeroLimitKeepsNothing pins the guard in offer. perProject is +// clamped to a positive number by SearchProjects, so this is about the heap +// being safe on its own terms rather than about a reachable call. +func TestTopHits_ZeroLimitKeepsNothing(t *testing.T) { + top := &topHits{n: 0} + top.offer(rankedRow{rid: 1, bm: -9}) + if got := top.sorted(); len(got) != 0 { + t.Errorf("n=0 kept %d rows", len(got)) + } +} + func explain(t *testing.T, ctx context.Context, d *sql.DB, query string, args ...any) string { t.Helper() rows, err := d.QueryContext(ctx, "EXPLAIN QUERY PLAN "+query, args...) From 956d6576795de570d1fe752f342dcc404a1417d3 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Fri, 21 Aug 2026 12:02:18 +0100 Subject: [PATCH 2/5] fix(chunksfts): cover the two paths the statement split created, drop the comment it outdated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #266 found four things, all confirmed here before being acted on. Three were holes the change opened and nothing held; the fourth was a comment that survived the code it describes. THE TWO UNTESTED PATHS Both were verified as real by mutating the tree and watching the suite pass: - fetchPayload's batching loop never ran twice. TestSearchProjects_SpansTheBatch- Boundary cannot reach it, and the reason is a coincidence of searchProjectsBatch and payloadFetchBatch both being 500: that test seeds one hit per project, so the rowid list is at most 500 long and the loop body runs exactly once however many projects it is given. Production is 43 projects x 50 hits — five batches. The path that always runs in production was the one nothing covered. Making fetchPayload `break` after the first batch passed the whole suite. - collectHits' vanished-row handling had no test at all, which is the ONE genuinely new behaviour in #266: ranking and payload fetch are two statements now, so a chunk can be deleted between them. Two mutations passed the suite — turning the dropped row into an empty Hit carrying a real BM25 score, and deleting the `len(hits) > 0` guard so a project whose every survivor vanished becomes present-with-an-empty-slice, which contradicts this package's own documented contract. Racing a real delete against a live query is not worth building. The assembly moved into collectHits(ranked, payload, dst) instead, and TestCollectHits drives it with a payload map that deliberately omits rows — which is exactly the state that race produces. All three mutations now fail the suite, named: TestFetchPayload_SpansTheBatchBoundary, TestCollectHits/one_row_vanished, TestCollectHits/every_row_vanished. WHY THE SPLIT IS SAFE, WRITTEN DOWN The old comment said why a MISSING row is acceptable and never said why a WRONG row is impossible — and that second fact is the whole reason splitting the statement is safe. chunks_meta.rowid is INTEGER PRIMARY KEY AUTOINCREMENT (internal/db/schema.go:430), so SQLite never re-issues a rowid after a delete; a row can only go missing, never come back pointing at a different chunk. That guarantee lives in another package and is one schema edit away from silently becoming false, at which point a reindex could hand project B's chunk back under project A's score with no error and nothing in failed_repos. collectHits' doc comment now says so, and says what to do if it ever changes. THE OUTDATED COMMENT SearchProjects' doc still explained that "the window function does the partitioning" and that "both forms order by (bm ASC, rowid ASC)" — of a window form that #266 deleted. A reader following it to find where the workspace path orders ties landed on workspaceScanQuery, which has neither ORDER BY nor rowid; the tiebreak now lives in rankedRow.betterThan. The substance was right and only the artifact was wrong, so it is repointed rather than removed. This is the same drift 7cc70a2 fixed one commit earlier in workspacesearch.go. A dead 14-line comment block for the deleted TestSearchProjects_FetchesPayload- AfterTheTrim was also still sitting above TestSearchProjects_ScanDoesNotSortThe- MatchSet, describing a different guard. Its 0:= reasoning already exists, correctly, on TestSearchProjects_FetchesPayloadByRowid. Deleted. ONE NIT TAKEN rids was built by ranging a map, so the payload IN-lists — and the batch boundaries — differed between two runs of the same query. Results did not (payload is keyed by rowid, each project is assembled in rank order), but a statement whose bound parameters come out of Go's map iteration cannot be compared plan-to-plan between runs, which is the first thing anyone timing this will want to do. Now sorted. Ascending rowids also probe both B-trees in order rather than at random. NOT measured as a speedup, and deliberately not tested: removing the sort passes the suite, because the change has no observable effect on results. The reason to do it is the determinism. VERIFIED - go test ./... green, go vet clean, gofmt clean on the touched files. Three files elsewhere in the tree fail gofmt; they fail on develop too and are not touched here. - The five mutations #266 was checked against still fail after the refactor — the sign flip in particular now lives in collectHits. - 50 fixture queries recaptured and diffed against both the pre-review build and develop: BM25 signal bit-identical in all 500 panel rows both ways, panel order identical 50/50 both ways. Dense scores differ on 6 and 8 of 50 queries respectively and every chunk-list difference falls on a query where dense also moved — the provider returns different vectors for byte-identical requests, and the same build diffed against itself shows it too. BM25 is the only bit-stable side and it is the only side this code touches. Co-Authored-By: Claude Opus 5 --- server/internal/chunksfts/chunksfts.go | 67 +++++++++---- server/internal/chunksfts/chunksfts_test.go | 106 +++++++++++++++++--- 2 files changed, 140 insertions(+), 33 deletions(-) diff --git a/server/internal/chunksfts/chunksfts.go b/server/internal/chunksfts/chunksfts.go index ab1b62d..1384f68 100644 --- a/server/internal/chunksfts/chunksfts.go +++ b/server/internal/chunksfts/chunksfts.go @@ -282,19 +282,22 @@ const ( // fan-out's total work, and each query slowed from ~400 ms standalone to // as much as 7 s when 43 of them ran against the index at once. // -// The window function does the partitioning SQLite would otherwise make -// us do with N queries: rank within each project, keep the top rows of -// each. +// The per-project trim is a bounded heap in Go, not a window function in +// SQL: a window has to sort the ENTIRE match set to find N rows per +// project, and the match set here is the whole server's index. See +// workspaceScanQuery for why that is the dominant cost and what it +// measured. // -// Both forms order by (bm ASC, rowid ASC). The rowid is defensive, not a -// fix for an observed bug: bm25 ties are the norm rather than the -// exception in a trigram index over real code — in this package's own -// test corpus 14 of 16 hits share a score with another hit — and today -// SQLite happens to return tied rows in rowid order for both the LIMIT -// and the window form, so they agree without being told to. That is -// unspecified behaviour of the sorter. Naming the tiebreak makes the -// agreement a property of the queries instead of a coincidence that a -// future planner is free to break. +// Both paths order by (bm ASC, rowid ASC) — the per-project query in its +// ORDER BY, the workspace path in rankedRow.betterThan. The rowid is +// defensive, not a fix for an observed bug: bm25 ties are the norm rather +// than the exception in a trigram index over real code — in this package's +// own test corpus 14 of 16 hits share a score with another hit — and today +// SQLite happens to return tied rows in rowid order for the LIMIT form, so +// the two would agree without being told to. That is unspecified behaviour +// of the sorter. Naming the tiebreak on BOTH sides makes the agreement a +// property of the code instead of a coincidence a future planner is free +// to break. func SearchProjects(ctx context.Context, db *sql.DB, projectPaths []string, query string, perProject int) (map[string][]Hit, error) { if perProject <= 0 { perProject = 20 @@ -360,21 +363,48 @@ func searchProjectsBatchInto(ctx context.Context, db *sql.DB, projectPaths []str rids = append(rids, r.rid) } } + // Sorted because rids was built by ranging a map, so without this the IN + // lists — and therefore the batch boundaries — differ between two runs of + // the same query. The results do not (payload is keyed by rowid and each + // project is assembled in rank order), but a statement whose bound + // parameters come out of Go's map iteration cannot be compared plan-to-plan + // between runs, which is exactly what someone timing this will want to do. + // Ascending rowids also probe both B-trees in order rather than at random. + // Not measured as a speedup; the reason to do it is the determinism. + sort.Slice(rids, func(i, j int) bool { return rids[i] < rids[j] }) + payload, err := fetchPayload(ctx, db, rids) if err != nil { return err } + collectHits(ranked, payload, dst) + return nil +} + +// collectHits pairs each project's ranked rows with the payload fetched for +// them, and writes the projects that still have hits into dst. +// +// A row whose chunk vanished between the ranking scan and the payload fetch is +// dropped. Two statements cannot be atomic the way the one they replaced was, +// and a hit fewer is the right answer for a chunk that no longer exists; +// erroring would fail a whole workspace search because one file happened to be +// getting reindexed. A project that loses ALL of its survivors that way is left +// out of dst entirely, because this package's contract is that a project with +// no match is absent rather than present with an empty slice. +// +// What makes the split safe is not in this package: a rowid can only go +// MISSING, never come back pointing at a different chunk. chunks_meta.rowid is +// INTEGER PRIMARY KEY AUTOINCREMENT (internal/db/schema.go), so SQLite never +// re-issues a rowid after a delete. Drop the AUTOINCREMENT and a reindex could +// hand project B's chunk back under project A's score, with no error and +// nothing in failed_repos — at which point this needs cm.project_path in the +// payload SELECT and a check against the ranked row. +func collectHits(ranked map[string][]rankedRow, payload map[int64]Hit, dst map[string][]Hit) { for pp, rows := range ranked { hits := make([]Hit, 0, len(rows)) for _, r := range rows { h, ok := payload[r.rid] if !ok { - // The row was deleted between the ranking scan and the - // payload fetch — a reindex of that file landed in between. - // Two statements cannot be atomic the way one was, and a - // hit fewer is the right answer for a chunk that no longer - // exists. Erroring would fail a whole workspace search - // because one file was being rewritten. continue } h.Score = -r.bm @@ -384,7 +414,6 @@ func searchProjectsBatchInto(ctx context.Context, db *sql.DB, projectPaths []str dst[pp] = hits } } - return nil } // rankedRow is a matched chunk before its payload is fetched: the two columns diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 29e9229..2e027b6 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -480,20 +480,6 @@ func TestSearchProjects_EmptyInputs(t *testing.T) { } } -// TestSearchProjects_FetchesPayloadAfterTheTrim pins the query PLAN, which is -// the only place this bug can live: every equivalence test in this file passes -// against the slow form too, because the two forms return the same rows. -// -// Carrying file_path/content through the CTE makes SQLite materialise them for -// every globally-matched row before ROW_NUMBER trims — and FTS5 evaluates MATCH -// over the whole server's index, so the match set has nothing to do with how -// many rows come back. Measured on the load-test corpus: 15.1 s against 2.8 s -// on a 263k-row match, and 29.6 s on a 624k-row one, which was slower than the -// per-project queries the partitioned form replaced. -// -// FTS5 reports a rowid-equality lookup as "0:=" and a MATCH scan as "0:M...". -// The rank-first form does both — scan to match, point lookups for survivors. -// The payload-in-CTE form only ever scans. // TestSearchProjects_ScanDoesNotSortTheMatchSet pins the reason the ranking // moved out of SQL. The window form has to sort every matched row to find N // per project; on the load-test fixture that is up to 1.29 million rows to @@ -564,6 +550,98 @@ func TestSearchProjects_FetchesPayloadByRowid(t *testing.T) { } } +// TestFetchPayload_SpansTheBatchBoundary covers the rowid IN-list batching. +// +// TestSearchProjects_SpansTheBatchBoundary cannot reach it, and the reason is a +// coincidence of the two constants being equal: that test seeds one hit per +// project, so the rowid list is at most searchProjectsBatch long and the +// payload loop runs exactly once however many projects there are. Production is +// 43 projects x 50 hits = five batches, so without this the path that always +// runs in production would be the one nothing covers. +func TestFetchPayload_SpansTheBatchBoundary(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + + const n = payloadFetchBatch + 7 + chunks := make([]Chunk, 0, n) + for i := 0; i < n; i++ { + chunks = append(chunks, Chunk{ + Content: "func retryWithBackoff() {}", + FilePath: "a.go", + StartLine: 1 + i*10, EndLine: 5 + i*10, + Language: "go", + }) + } + upsert(t, d, "proj", "a.go", chunks) + + got, err := SearchProjects(ctx, d, []string{"proj"}, "retry", n) + if err != nil { + t.Fatalf("SearchProjects: %v", err) + } + if len(got["proj"]) != n { + t.Errorf("got %d hits, want %d — a payload batch was dropped", + len(got["proj"]), n) + } +} + +// TestCollectHits covers what splitting one statement into two actually +// changed: a chunk can disappear between the ranking scan and the payload +// fetch. Racing a real delete against a live query is not worth building, so +// the seam is tested directly — a payload map with rows deliberately left out +// is exactly the state that race produces. +func TestCollectHits(t *testing.T) { + rows := []rankedRow{{rid: 7, bm: -9}, {rid: 8, bm: -5}, {rid: 9, bm: -1}} + full := map[int64]Hit{ + 7: {FilePath: "a.go"}, 8: {FilePath: "b.go"}, 9: {FilePath: "c.go"}, + } + + t.Run("all present", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, full, dst) + got := dst["p"] + if len(got) != 3 { + t.Fatalf("got %d hits, want 3", len(got)) + } + for i, want := range []struct { + file string + score float64 + }{{"a.go", 9}, {"b.go", 5}, {"c.go", 1}} { + if got[i].FilePath != want.file || got[i].Score != want.score { + t.Errorf("rank %d: got %s/%v, want %s/%v", + i, got[i].FilePath, got[i].Score, want.file, want.score) + } + } + }) + + t.Run("one row vanished", func(t *testing.T) { + partial := map[int64]Hit{7: full[7], 9: full[9]} + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, partial, dst) + got := dst["p"] + if len(got) != 2 { + t.Fatalf("got %d hits, want 2", len(got)) + } + if got[0].FilePath != "a.go" || got[1].FilePath != "c.go" { + t.Errorf("got %s,%s — the surviving rows lost their rank order", + got[0].FilePath, got[1].FilePath) + } + if got[0].Score != 9 || got[1].Score != 1 { + t.Errorf("got scores %v,%v — a dropped row shifted the scores", + got[0].Score, got[1].Score) + } + }) + + t.Run("every row vanished", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, map[int64]Hit{}, dst) + if _, present := dst["p"]; present { + t.Errorf("a project whose every survivor vanished is present with "+ + "%d hits; this package's contract is that it is absent", + len(dst["p"])) + } + }) +} + // TestTopHits_MatchesAFullSort is the property test for the bounded heap that // replaced SQLite's window function. // From 46713ff12b7219dda26736666e3a4394e495db34 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Fri, 21 Aug 2026 12:34:18 +0100 Subject: [PATCH 3/5] docs(chunksfts): correct the gofmt count, repoint one test comment, unshadow rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass 2 on #266: ship-it verdict with three nits. All three confirmed here first. CORRECTION TO 956d657's COMMIT MESSAGE That message says "Three files elsewhere in the tree fail gofmt". It is SEVEN. The substance holds — the set is identical at this HEAD and at origin/develop, and none of the seven is touched by this PR — but the number does not reproduce, and in this repo a commit message is a report for whoever comes next, so the wrong number is the part that costs someone else time. It came from running `gofmt -l internal/ | head -3`: the `head -3` truncated the list and `internal/` excluded bench/. The real set is bench/bench_eval_retrieval.go internal/callgraph/eval/eval_test.go internal/secrets/secrets.go internal/tunnels/ngrok.go internal/workspaceprojects/workspaceprojects.go internal/workspaceprojects/workspaceprojects_test.go internal/workspaces/workspaces.go Not amended into 956d657 on purpose: that commit is the reviewed head, and force-pushing over it would invalidate a review that names the OID. ONE MORE COMMENT THAT NO LONGER MATCHED ITS CODE TestSearchProjects_SpansTheBatchBoundary's doc named "a batch that overwrote instead of appending" as the failure mode it guards. Since the previous commit the implementation deliberately does NOT append — collectHits assigns dst[pp] = hits, which is safe because the batching slices projectPaths into disjoint batches, so each project is written exactly once. The test still guards something real, so only the phrasing is repointed: a batch that replaced the MAP rather than adding to it, or that dropped its last slice. Verified by mutation rather than by reading — clearing dst at the top of searchProjectsBatchInto fails the suite on that test by name. The same comment now also says what the test does NOT reach: with one hit per project the rowid list is exactly searchProjectsBatch long, so fetchPayload's loop runs once. That is the coincidence that hid the batching hole pass 1 found, and it is worth stating next to the test that looks like it covers it. SHADOWING `rows := t.sorted()` shadowed the *sql.Rows twenty lines above it. The outer rows is closed by then and vet is happy, so this is only a hazard for the next edit: anyone adding a Close() or Err() inside that loop gets confusion at best. Renamed to `ordered`. VERIFIED go test ./... green, go vet ./... clean on the whole module, gofmt clean on the touched files. Post-rename regression check on the mutation battery: the sign flip, never-evict and replace-dst mutations all still fail the suite, named. Co-Authored-By: Claude Opus 5 --- server/internal/chunksfts/chunksfts.go | 6 +++--- server/internal/chunksfts/chunksfts_test.go | 12 ++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/server/internal/chunksfts/chunksfts.go b/server/internal/chunksfts/chunksfts.go index 1384f68..2de73dd 100644 --- a/server/internal/chunksfts/chunksfts.go +++ b/server/internal/chunksfts/chunksfts.go @@ -357,9 +357,9 @@ func searchProjectsBatchInto(ctx context.Context, db *sql.DB, projectPaths []str ranked := make(map[string][]rankedRow, len(tops)) var rids []int64 for pp, t := range tops { - rows := t.sorted() - ranked[pp] = rows - for _, r := range rows { + ordered := t.sorted() + ranked[pp] = ordered + for _, r := range ordered { rids = append(rids, r.rid) } } diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 2e027b6..5dcf266 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -433,10 +433,14 @@ func TestSearchProjects_DoesNotPrefixMatchProjectPaths(t *testing.T) { } } -// TestSearchProjects_SpansTheBatchBoundary checks the IN-list batching. The -// map is filled across several statements, so a batch that overwrote instead -// of appending, or that dropped its last slice, would only show up above the -// batch size. +// TestSearchProjects_SpansTheBatchBoundary checks the project IN-list batching. +// dst is filled across several statements and each project is written to +// exactly once — a batch that replaced the map instead of adding to it, or that +// dropped its last slice, would only show up above the batch size. +// +// It does NOT reach the rowid batching inside fetchPayload; one hit per project +// keeps that list at exactly searchProjectsBatch. See +// TestFetchPayload_SpansTheBatchBoundary. func TestSearchProjects_SpansTheBatchBoundary(t *testing.T) { d := openTestDB(t) ctx := context.Background() From 9087ca492e422f11188ebeee6599e5021fd2f0b8 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Fri, 21 Aug 2026 14:13:45 +0100 Subject: [PATCH 4/5] test(chunksfts): the payload plan guard could not fail; fix it and the bound-variable fact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass 3 on #266 found a test that reads as protection and is not. Every claim below was reproduced here before being acted on. THE GUARD THAT COULD NOT FIRE TestSearchProjects_FetchesPayloadByRowid asserted two substrings of the FTS5 plan: that it contains "VIRTUAL TABLE INDEX 0:=" and that it does NOT contain "VIRTUAL TABLE INDEX 0:M". The second assertion is unreachable. FTS5 packs its plan into one idxStr. "0:=" is a bare rowid lookup; a MATCH adds "M" plus the matched column. A payload fetch that ALSO ran a MATCH reports "0:=M3" — which still contains "0:=" and does not contain "0:M", because the M is now preceded by "=". Both assertions are satisfied by exactly the merge the test exists to catch. Reproduced: adding `AND chunks_fts MATCH 'retry'` to payloadQuery, arity unchanged, the suite reported ok and that test PASSed. The mechanical lesson is worth more than the fix. "0:M" was borrowed from the scan query, which reports "0:M3" because it has no rowid constraint. Add one and FTS5 records "=" ahead of the M, so the M moves and the prefix stops matching. The string was not wrong; it was a PREFIX of a packed field whose earlier characters vary. A substring assertion over planner output is safe when it matches a complete token whose variable part comes AFTER it — "USE TEMP B-TREE FOR X" varies in X — and unsafe when it matches a prefix of a packed field. FTS5's idxStr is documented as an internal encoding, which is the marker for the second class. Fixed by comparing the WHOLE idxStr: every FTS5 index in the payload plan must be exactly "0:=". ftsIndexes() extracts them. TestExplainRejectsThePayloadShapes is the companion that would have caught this: it builds the merged form and a join FTS5 cannot serve by rowid, and asserts the guard rejects both. Verified: the merged-MATCH mutation is now KILLED by TestSearchProjects_FetchesPayloadByRowid by name, and making ftsIndexes return nothing — the way to make the new guard vacuous — is killed by both the guard and its companion, so the replacement is not vacuous either. THE COMPANION THAT GUARDED THE WRONG THING TestExplainRejectsTheWindowForm only built the window form, which this PR DELETED. The regression far more likely to happen is someone adding ORDER BY back to the scan, and nothing proved the guard would catch that. Now table-driven as TestExplainRejectsTheSortingForms over both shapes. Verified: appending `ORDER BY bm25(chunks_fts)` to workspaceScanQuery is killed by TestSearchProjects_ScanDoesNotSortTheMatchSet by name. The "TEMP B-TREE" assertion itself is NOT brittle the way the idxStr one was — review measured every plausible way of putting a sort back (ORDER BY, ORDER BY with LIMIT, GROUP BY, SELECT DISTINCT) and all report "USE TEMP B-TREE FOR ...". Kept as is. A WRONG FACT IN THE CONSTANTS COMMENT It said SQLite's bound-variable ceiling is 999 and that 500 "leaves room for the query parameter". Measured through this driver: `rowid IN (...)` takes 32,766 placeholders and fails at 32,767. SQLite raised SQLITE_MAX_VARIABLE_NUMBER from 999 to 32,766 in 3.32.0 and modernc tracks a recent upstream, so the real headroom is 32,266, not 4. The headroom half of the justification was wrong, and it was wrong in the one place someone would look before deciding whether 500 could be raised. The half that survives is the real reason: 500 is hydrateBatch (internal/vectorstore/search.go:442), so both IN-list batchers use one number. The constant is unchanged — 43 x 50 = 2,150 rowids in five statements is nothing against a multi-second BM25 scan, so there is nothing to gain by tuning it. TWO CORRECTIONS INHERITED FROM THE REVIEW LOG, CONFIRMED HERE - The "IN -> LIKE" mutation quoted in earlier review passes proved nothing: it is a row-value misuse, SQLite errors, and the suite dies on a query error rather than on prefix leakage. The honest form keeps arity and stays valid SQL — `substr(cm.project_path, 1, 4) IN (%s)`, which really does leak proj-extended into proj. Ran it: killed by TestSearchProjects_DoesNotPrefixMatchProjectPaths by name. - "heap keeps perProject+1" is killed by TestSearchProjects_MatchesPerProjectQueries, not by TestTopHits_MatchesAFullSort as an earlier log said. That is the better answer: the property test constructs &topHits{n: n} directly and never sees how searchProjectsBatchInto picks n. SCOPE No production behaviour changes. The only non-test edit is a comment; the diff over chunksfts.go contains no non-comment lines. The fixture was not re-measured for that reason. go test ./... green, go vet ./... clean, gofmt clean on the touched files. Co-Authored-By: Claude Opus 5 --- server/internal/chunksfts/chunksfts.go | 19 +- server/internal/chunksfts/chunksfts_test.go | 204 ++++++++++---------- 2 files changed, 116 insertions(+), 107 deletions(-) diff --git a/server/internal/chunksfts/chunksfts.go b/server/internal/chunksfts/chunksfts.go index 2de73dd..cf662f0 100644 --- a/server/internal/chunksfts/chunksfts.go +++ b/server/internal/chunksfts/chunksfts.go @@ -256,11 +256,20 @@ func SearchProject(ctx context.Context, db *sql.DB, projectPath, query string, l return out, nil } -// searchProjectsBatch caps how many project_path values go into one IN -// list, and payloadFetchBatch does the same for the rowid list of the -// second statement. SQLite's default bound-variable ceiling is 999; 500 -// leaves room for the query parameter and matches the batch size the -// vector store already uses for its own IN lists. +// searchProjectsBatch caps how many project_path values go into one IN list, +// and payloadFetchBatch does the same for the rowid list of the second +// statement. +// +// 500 is NOT a headroom number. Measured through this driver, `rowid IN (...)` +// takes 32,766 placeholders and fails at 32,767 — SQLite raised +// SQLITE_MAX_VARIABLE_NUMBER from 999 to 32,766 in 3.32.0 and modernc tracks a +// recent upstream, so the ceiling is two orders of magnitude away and neither +// constant is anywhere near it. The reason for 500 is consistency: it is +// hydrateBatch (internal/vectorstore/search.go), the batch size the vector +// store already uses for its own IN lists, and one number for both is worth +// more than a tuned one for each. Raising it would want a measurement, and +// 43 x 50 = 2,150 rowids in five statements is nothing against a multi-second +// BM25 scan, so there is nothing to gain by measuring. const ( searchProjectsBatch = 500 payloadFetchBatch = 500 diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 5dcf266..5b1a19e 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -500,12 +500,16 @@ func TestSearchProjects_ScanDoesNotSortTheMatchSet(t *testing.T) { } } -// TestExplainRejectsTheWindowForm is the mutation check for the test above, -// kept in the tree rather than run by hand: it builds the form that WAS -// shipped and asserts the assertion above would reject it. Without this, a -// change in how SQLite reports plans could turn the guard into a tautology -// that passes on everything, and nothing would say so. -func TestExplainRejectsTheWindowForm(t *testing.T) { +// TestExplainRejectsTheSortingForms is the mutation check for the test above, +// kept in the tree rather than run by hand: it builds forms that DO sort the +// match set and asserts the assertion above would reject each one. Without +// this, a change in how SQLite reports plans could turn the guard into a +// tautology that passes on everything, and nothing would say so. +// +// Two shapes, not one. The window form is what this PR deleted; a plain +// ORDER BY added back to the scan is the regression far more likely to +// actually happen, and a guard is worth exactly what it rejects. +func TestExplainRejectsTheSortingForms(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) @@ -527,123 +531,119 @@ func TestExplainRejectsTheWindowForm(t *testing.T) { JOIN chunks_meta cm ON cm.rowid = r.rid WHERE r.rn <= ?` - plan := explain(t, ctx, d, window, `"retry" OR "backoff"`, "p1", "p2", 3) - if !strings.Contains(plan, "TEMP B-TREE") { - t.Errorf("the window form no longer reports a sorter, so the plan "+ - "assertion no longer distinguishes the two shapes:\n%s", plan) + for _, tc := range []struct { + name string + query string + args []any + }{ + { + name: "the window form this replaced", + query: window, + args: []any{`"retry" OR "backoff"`, "p1", "p2", 3}, + }, + { + name: "an ORDER BY added back to the scan", + query: workspaceScanQuery(placeholders(2)) + "\n ORDER BY bm25(chunks_fts)", + args: []any{`"retry" OR "backoff"`, "p1", "p2"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + plan := explain(t, ctx, d, tc.query, tc.args...) + if !strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("this form no longer reports a sorter, so the plan "+ + "assertion no longer distinguishes it:\n%s", plan) + } + }) } } // TestSearchProjects_FetchesPayloadByRowid guards the second half of the same // lesson: file_path and content are fetched for the rows that survived, by // rowid, and never carried through the scan. +// +// The assertion is on the WHOLE FTS5 idxStr, not a prefix of it. FTS5 packs its +// plan into one string: "0:=" is a bare rowid lookup, and a MATCH adds an "M" +// plus the matched column, so a payload fetch that ALSO matched reports +// "0:=M3" — which still contains "0:=" and does not contain "0:M". The first +// version of this test asserted on those two prefixes and therefore passed on +// exactly the merge it existed to catch. Found in review of #266, not by the +// suite, which is the whole argument for the companion test below. func TestSearchProjects_FetchesPayloadByRowid(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) - plan := explain(t, ctx, d, payloadQuery(placeholders(2)), 1, 2) - if !strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { - t.Errorf("chunks_fts is not looked up by rowid in the payload fetch:\n%s", plan) + idxs := ftsIndexes(explain(t, ctx, d, payloadQuery(placeholders(2)), 1, 2)) + if len(idxs) == 0 { + t.Fatal("the payload fetch does not touch chunks_fts at all") } - // 0:M... is how FTS5 reports a MATCH scan. The payload fetch has no MATCH - // at all, so seeing one would mean the two statements had been merged back - // together. - if strings.Contains(plan, "VIRTUAL TABLE INDEX 0:M") { - t.Errorf("the payload fetch is running a MATCH scan:\n%s", plan) + for _, idx := range idxs { + if idx != "0:=" { + t.Errorf(`chunks_fts is not a plain rowid lookup in the payload `+ + `fetch: idxStr %q ("=" is the rowid constraint; an "M" means a `+ + `MATCH crept back in)`, idx) + } } } -// TestFetchPayload_SpansTheBatchBoundary covers the rowid IN-list batching. +// ftsIndexes returns every FTS5 idxStr in a query plan, whole. The planner +// prints it as "... VIRTUAL TABLE INDEX " at the end of the line. +func ftsIndexes(plan string) []string { + var out []string + for _, line := range strings.Split(plan, "\n") { + if _, idx, ok := strings.Cut(line, "VIRTUAL TABLE INDEX "); ok { + out = append(out, strings.TrimSpace(idx)) + } + } + return out +} + +// TestExplainRejectsThePayloadShapes is the mutation check for the test above, +// kept in the tree rather than run by hand: it builds the shapes that test +// exists to reject and asserts it would reject them. // -// TestSearchProjects_SpansTheBatchBoundary cannot reach it, and the reason is a -// coincidence of the two constants being equal: that test seeds one hit per -// project, so the rowid list is at most searchProjectsBatch long and the -// payload loop runs exactly once however many projects there are. Production is -// 43 projects x 50 hits = five batches, so without this the path that always -// runs in production would be the one nothing covers. -func TestFetchPayload_SpansTheBatchBoundary(t *testing.T) { +// The MATCH case is not hypothetical. The prefix-matching version of the guard +// let it straight through, and nothing in the suite said so. +func TestExplainRejectsThePayloadShapes(t *testing.T) { d := openTestDB(t) ctx := context.Background() + seedCorpus(t, d, []string{"p1", "p2"}) - const n = payloadFetchBatch + 7 - chunks := make([]Chunk, 0, n) - for i := 0; i < n; i++ { - chunks = append(chunks, Chunk{ - Content: "func retryWithBackoff() {}", - FilePath: "a.go", - StartLine: 1 + i*10, EndLine: 5 + i*10, - Language: "go", + for _, tc := range []struct { + name string + query string + args []any + }{ + { + name: "the two statements merged back together", + query: ` + SELECT cm.rowid, cm.file_path, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid = cm.rowid + WHERE chunks_fts MATCH ? AND cm.rowid IN (?,?)`, + args: []any{`"retry" OR "backoff"`, 1, 2}, + }, + { + name: "a join FTS5 cannot serve by rowid", + query: ` + SELECT cm.rowid, cm.file_path, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid + 0 = cm.rowid + WHERE cm.rowid IN (?,?)`, + args: []any{1, 2}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + plan := explain(t, ctx, d, tc.query, tc.args...) + for _, idx := range ftsIndexes(plan) { + if idx != "0:=" { + return + } + } + t.Errorf("this shape reports a plain rowid lookup, so the payload "+ + "guard no longer distinguishes it:\n%s", plan) }) } - upsert(t, d, "proj", "a.go", chunks) - - got, err := SearchProjects(ctx, d, []string{"proj"}, "retry", n) - if err != nil { - t.Fatalf("SearchProjects: %v", err) - } - if len(got["proj"]) != n { - t.Errorf("got %d hits, want %d — a payload batch was dropped", - len(got["proj"]), n) - } -} - -// TestCollectHits covers what splitting one statement into two actually -// changed: a chunk can disappear between the ranking scan and the payload -// fetch. Racing a real delete against a live query is not worth building, so -// the seam is tested directly — a payload map with rows deliberately left out -// is exactly the state that race produces. -func TestCollectHits(t *testing.T) { - rows := []rankedRow{{rid: 7, bm: -9}, {rid: 8, bm: -5}, {rid: 9, bm: -1}} - full := map[int64]Hit{ - 7: {FilePath: "a.go"}, 8: {FilePath: "b.go"}, 9: {FilePath: "c.go"}, - } - - t.Run("all present", func(t *testing.T) { - dst := map[string][]Hit{} - collectHits(map[string][]rankedRow{"p": rows}, full, dst) - got := dst["p"] - if len(got) != 3 { - t.Fatalf("got %d hits, want 3", len(got)) - } - for i, want := range []struct { - file string - score float64 - }{{"a.go", 9}, {"b.go", 5}, {"c.go", 1}} { - if got[i].FilePath != want.file || got[i].Score != want.score { - t.Errorf("rank %d: got %s/%v, want %s/%v", - i, got[i].FilePath, got[i].Score, want.file, want.score) - } - } - }) - - t.Run("one row vanished", func(t *testing.T) { - partial := map[int64]Hit{7: full[7], 9: full[9]} - dst := map[string][]Hit{} - collectHits(map[string][]rankedRow{"p": rows}, partial, dst) - got := dst["p"] - if len(got) != 2 { - t.Fatalf("got %d hits, want 2", len(got)) - } - if got[0].FilePath != "a.go" || got[1].FilePath != "c.go" { - t.Errorf("got %s,%s — the surviving rows lost their rank order", - got[0].FilePath, got[1].FilePath) - } - if got[0].Score != 9 || got[1].Score != 1 { - t.Errorf("got scores %v,%v — a dropped row shifted the scores", - got[0].Score, got[1].Score) - } - }) - - t.Run("every row vanished", func(t *testing.T) { - dst := map[string][]Hit{} - collectHits(map[string][]rankedRow{"p": rows}, map[int64]Hit{}, dst) - if _, present := dst["p"]; present { - t.Errorf("a project whose every survivor vanished is present with "+ - "%d hits; this package's contract is that it is absent", - len(dst["p"])) - } - }) } // TestTopHits_MatchesAFullSort is the property test for the bounded heap that From 61436ad7b93dd8b745de5a6259a64327a7d9dee6 Mon Sep 17 00:00:00 2001 From: dvcdsys Date: Fri, 21 Aug 2026 14:20:42 +0100 Subject: [PATCH 5/5] test(chunksfts): restore the two tests 9087ca4 deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass 4 caught that 9087ca4 removed TestFetchPayload_SpansTheBatchBoundary and TestCollectHits — the two tests 956d657 added to close pass-1's findings 3 and 4. Confirmed here: test function count went 21 -> 20 across that commit while collectHits stayed in production at chunksfts.go:411 with nothing exercising it. Measured rather than inferred. The three mutations those tests were written to kill all walked through 9087ca4: always assign dst[pp] killed at 46713ff -> SURVIVED at 9087ca4 payload miss -> empty Hit killed at 46713ff -> SURVIVED at 9087ca4 payload fetch: first batch killed at 46713ff -> SURVIVED at 9087ca4 Coverage was back to its pre-956d657 state: the payload batching loop that always runs in production uncovered again, and so was the vanished-row handling, which is the only genuinely new behaviour in this PR. HOW IT HAPPENED, because the mechanism generalises 956d657 inserted both tests immediately BEFORE the anchor comment "// TestTopHits_MatchesAFullSort is the property test". 9087ca4 then replaced a region delimited by index("// TestSearchProjects_FetchesPayloadByRowid guards") and index(that same anchor) — so the two tests sat inside the replaced span and went out with it. Editing by string-delimited region is fine for a region you just read; it is not fine for one that a previous edit has since grown. Nothing detected it. The suite was green, because deleting a test never fails a suite. vet and gofmt were clean, because the file was still valid Go. 9087ca4's own message says "No production behaviour changes ... the only non-test edit is a comment", which was true and beside the point: the loss was entirely in the test file. I ran a mutation battery for that commit, but only the mutations relevant to what I was changing, so the three that regressed were never re-checked. The instrument that would have caught it costs one command: diff the list of test function names against the previous head. A commit that touches only tests is exactly the commit where the test inventory is the only thing that can see what happened. Doing that from here on any test-only edit. RESTORED Both functions come back verbatim from 46713ff — verified byte-identical to that head, not retyped — and all three mutations are killed again by name: TestCollectHits/every_row_vanished, TestCollectHits/one_row_vanished, TestFetchPayload_SpansTheBatchBoundary. The dangling cross-reference at the end of TestSearchProjects_SpansTheBatchBoundary's comment, which pointed at a test that did not exist at 9087ca4, is correct again as a result. ALSO: the companion subtest no longer derives its query from production TestExplainRejectsTheSortingForms' second subtest built its query as workspaceScanQuery(...) + " ORDER BY ...". When the scan itself was mutated to sort, the concatenation produced two ORDER BY clauses, the SQL went invalid, and explain's t.Fatalf fired — so the companion failed for a reason unrelated to what it asserts. The companion is a claim about how SQLite REPORTS a sort, not about production code, so it should not touch production code. Now a literal. Verified: with the scan mutated to sort, only TestSearchProjects_ScanDoesNotSortTheMatchSet fails; the companion stays green, which is what a companion should do. go test ./... green, go vet ./... clean, gofmt clean on the touched files. Co-Authored-By: Claude Opus 5 --- server/internal/chunksfts/chunksfts_test.go | 108 +++++++++++++++++++- 1 file changed, 105 insertions(+), 3 deletions(-) diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 5b1a19e..e55a1b4 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -542,9 +542,19 @@ func TestExplainRejectsTheSortingForms(t *testing.T) { args: []any{`"retry" OR "backoff"`, "p1", "p2", 3}, }, { - name: "an ORDER BY added back to the scan", - query: workspaceScanQuery(placeholders(2)) + "\n ORDER BY bm25(chunks_fts)", - args: []any{`"retry" OR "backoff"`, "p1", "p2"}, + // Written out rather than derived from workspaceScanQuery: this + // subtest is a claim about how SQLite REPORTS a sort, not about + // production code, and appending to the real statement made it + // fail for the wrong reason whenever that statement was itself + // mutated to sort. + name: "an ORDER BY added back to the scan", + query: ` + SELECT cm.project_path, cm.rowid, bm25(chunks_fts) + FROM chunks_fts cf + JOIN chunks_meta cm ON cm.rowid = cf.rowid + WHERE chunks_fts MATCH ? AND cm.project_path IN (?,?) + ORDER BY bm25(chunks_fts)`, + args: []any{`"retry" OR "backoff"`, "p1", "p2"}, }, } { t.Run(tc.name, func(t *testing.T) { @@ -646,6 +656,98 @@ func TestExplainRejectsThePayloadShapes(t *testing.T) { } } +// TestFetchPayload_SpansTheBatchBoundary covers the rowid IN-list batching. +// +// TestSearchProjects_SpansTheBatchBoundary cannot reach it, and the reason is a +// coincidence of the two constants being equal: that test seeds one hit per +// project, so the rowid list is at most searchProjectsBatch long and the +// payload loop runs exactly once however many projects there are. Production is +// 43 projects x 50 hits = five batches, so without this the path that always +// runs in production would be the one nothing covers. +func TestFetchPayload_SpansTheBatchBoundary(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + + const n = payloadFetchBatch + 7 + chunks := make([]Chunk, 0, n) + for i := 0; i < n; i++ { + chunks = append(chunks, Chunk{ + Content: "func retryWithBackoff() {}", + FilePath: "a.go", + StartLine: 1 + i*10, EndLine: 5 + i*10, + Language: "go", + }) + } + upsert(t, d, "proj", "a.go", chunks) + + got, err := SearchProjects(ctx, d, []string{"proj"}, "retry", n) + if err != nil { + t.Fatalf("SearchProjects: %v", err) + } + if len(got["proj"]) != n { + t.Errorf("got %d hits, want %d — a payload batch was dropped", + len(got["proj"]), n) + } +} + +// TestCollectHits covers what splitting one statement into two actually +// changed: a chunk can disappear between the ranking scan and the payload +// fetch. Racing a real delete against a live query is not worth building, so +// the seam is tested directly — a payload map with rows deliberately left out +// is exactly the state that race produces. +func TestCollectHits(t *testing.T) { + rows := []rankedRow{{rid: 7, bm: -9}, {rid: 8, bm: -5}, {rid: 9, bm: -1}} + full := map[int64]Hit{ + 7: {FilePath: "a.go"}, 8: {FilePath: "b.go"}, 9: {FilePath: "c.go"}, + } + + t.Run("all present", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, full, dst) + got := dst["p"] + if len(got) != 3 { + t.Fatalf("got %d hits, want 3", len(got)) + } + for i, want := range []struct { + file string + score float64 + }{{"a.go", 9}, {"b.go", 5}, {"c.go", 1}} { + if got[i].FilePath != want.file || got[i].Score != want.score { + t.Errorf("rank %d: got %s/%v, want %s/%v", + i, got[i].FilePath, got[i].Score, want.file, want.score) + } + } + }) + + t.Run("one row vanished", func(t *testing.T) { + partial := map[int64]Hit{7: full[7], 9: full[9]} + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, partial, dst) + got := dst["p"] + if len(got) != 2 { + t.Fatalf("got %d hits, want 2", len(got)) + } + if got[0].FilePath != "a.go" || got[1].FilePath != "c.go" { + t.Errorf("got %s,%s — the surviving rows lost their rank order", + got[0].FilePath, got[1].FilePath) + } + if got[0].Score != 9 || got[1].Score != 1 { + t.Errorf("got scores %v,%v — a dropped row shifted the scores", + got[0].Score, got[1].Score) + } + }) + + t.Run("every row vanished", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, map[int64]Hit{}, dst) + if _, present := dst["p"]; present { + t.Errorf("a project whose every survivor vanished is present with "+ + "%d hits; this package's contract is that it is absent", + len(dst["p"])) + } + }) +} + // TestTopHits_MatchesAFullSort is the property test for the bounded heap that // replaced SQLite's window function. //