Skip to content

perf(chunksfts): rank the workspace with a bounded heap, not a window function - #266

Merged
dvcdsys merged 5 commits into
developfrom
perf/workspace-bm25-bounded-heap
Aug 21, 2026
Merged

perf(chunksfts): rank the workspace with a bounded heap, not a window function#266
dvcdsys merged 5 commits into
developfrom
perf/workspace-bm25-bounded-heap

Conversation

@dvcdsys

@dvcdsys dvcdsys commented Aug 21, 2026

Copy link
Copy Markdown
Owner

What

Moves the per-project top-N trim of the workspace BM25 query out of SQLite and
into a bounded heap in Go. The scan streams (project_path, rowid, bm25) with
no ORDER BY; the payload is fetched by rowid for the survivors, as before.

Results are identical, not close — same rows, same order, same
(score, rowid) tiebreak. TestSearchProjects_MatchesPerProjectQueries is the
oracle: it compares the workspace path against the per-project statement.

Why this and not what the plan said

The plan's next stage was to prune the dense fan-out, because 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, so the
whole dense fan-out lands in ~1,450 ms. BM25 is one serial query at 1–6 s.
Timed from inside the handler on the 43-project fixture, fanout_ms equals
bm25_ms to within 2 ms in eight of ten queries. Pruning dense would have
removed work nobody was waiting for.

Inside the BM25 query, the MATCH is not the cost either. Walking the
statement up one addition at a time, six-term query:

step ms
MATCH only (posting-list merge) 186
+ bm25() per matched row 1,889
+ join chunks_meta by rowid 352
+ project_path IN (46) 487
production shape (window + payload) 2,726

The posting merge is 7%. The rest is bm25() over the whole match set plus
ROW_NUMBER() sorting that same set to keep fifty rows per project.

Match sets are large because the tokenizer is trigram, so terms match
substrings. Share of the 1.95M-chunk corpus: test 38%, for 33%, the 31%,
and 28%, are 16% (parse, shared, compare), fault 10.5% — 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.

Measured

Back to back on the same warm page cache, medians of five passes, bm25_ms:

query develop this
write a unit test for the parser 7,747 5,988
type inference for generics 5,908 4,962
mock an HTTP client in tests 4,602 3,727
parse JWT token and validate signature 3,867 2,883
median of those seven 4,471 3,782 (1.18x)
median of the standard ten 1,917 1,740 (1.10x)

Standalone, outside the server, the same substitution is 1.7x (2,466 →
1,455 on a 624k match set; 5,216 → 3,014 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 (cold measures the
same as warm), 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 — modernc's own
allocator arena is the untested suspect. Recorded as a lead, 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.

Correctness

50 fixture queries, full workspace response captured per query:

  • BM25 signal bit-identical in all 500 panel rows;
  • panel order 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

The suite grew across four review passes; two of the additions exist because a
test that looked like protection turned out not to be.

  • TestSearchProjects_MatchesPerProjectQueries — the oracle. It runs the
    workspace path and the per-project statement over the same corpus and
    compares rank by rank, so "identical results" is checked rather than argued.
    Ties are the norm in a trigram index, which is what makes it catch a wrong
    tiebreak.

  • TestTopHits_MatchesAFullSort — property test for the heap against a
    full sort: shuffled input, deliberately many tied scores, limits 1, 3 and 50,
    20 seeds.

  • TestCollectHits and TestFetchPayload_SpansTheBatchBoundary — the
    two paths splitting one statement into two created. A chunk can now be
    deleted between the ranking scan and the payload fetch; TestCollectHits
    drives that seam with a payload map that deliberately omits rows. The payload
    batching loop is the one that always runs in production (43 projects x 50
    hits = five batches) and nothing reached it, because
    TestSearchProjects_SpansTheBatchBoundary seeds one hit per project and the
    two batch constants are both 500.

  • Two plan guards, each with a companion that proves it can fail.
    TestSearchProjects_ScanDoesNotSortTheMatchSet pins that the scan sorts
    nothing; TestSearchProjects_FetchesPayloadByRowid pins that the payload
    fetch is a plain rowid lookup, comparing the whole FTS5 idxStr rather
    than a prefix of it.

    That last detail is the one worth reading. The first version of that guard
    asserted the plan contains 0:= and does not contain 0:M — and a payload
    fetch that also ran a MATCH reports 0:=M3, which contains the first and not
    the second. It passed on exactly the merge it existed to catch. Found in
    review, not by the suite. TestExplainRejectsThePayloadShapes and
    TestExplainRejectsTheSortingForms are the companions that would have said
    so, and they build the shapes the guards must reject rather than asserting
    the guards are green.

Mutation-checked, 15 mutations, each applied alone to a freshly restored
tree.
14 fail the suite by name: dropping the rowid tiebreak, never evicting,
sorting output on score alone, losing the bm25 sign flip, stopping the heap's
sift-down after one level, inverting the sift-up comparison, keeping
perProject + 1, always assigning dst[pp], turning a vanished row into an
empty Hit, processing only the first payload batch, clearing dst at
function entry, breaking the payload join so it is no longer a rowid lookup,
merging a MATCH back into payloadQuery, and leaking a project prefix via
substr(project_path, 1, 4) IN (...).

One mutation survives and is declared: removing the sort.Slice(rids, ...).
That sort exists so the payload IN-lists do not come out of Go's map iteration
order — it has no observable effect on results, which is exactly why deleting
it passes.

go test ./... green, go vet ./... clean, gofmt clean on the touched
files. Seven files elsewhere in the tree fail gofmt; they fail on develop
too and none is touched here.

Not in this PR

  • Dropping high-frequency terms — closed by decision, not deferred. The
    prize was real: removing and from a six-term query is 3.9x on the BM25
    statement. It is closed because document frequency does not distinguish a
    stopword from the point of the query.
    Over the 50 fixture queries, 28 have
    a top term above 10% of the corpus, and in nine of those the term is the
    subject — test 38.1% ("write a unit test for the parser"), string 21.4%,
    struct 15.9%, tar 10.2% (drop it and "tar archive extraction" becomes
    "archive extraction"). Meanwhile and at 27.9% and string at 21.4% have
    comparable df and opposite value. A perfect, free df oracle would still
    delete the subject in about a third of the cases where it fires; deciding
    which terms matter is a learned-ranking problem, not a corpus measurement.
    Cost was never the blocker and is fully measured
    (fts5vocab as a df source is refuted — exact but a scan, 71 ms against
    25 ms for the same count via MATCH).
  • Stage 3 as originally specified (centroid pruning). Dropped. Its
    over-scan premise was a counter bug (real over-scan 1.3x, not 4.3x, fixed in
    perf(httpapi): measure workspace search per phase, inside the handler #264), and the work-vs-latency point above kills what was left.
  • The unexplained in-server gap. The same query costs ~1,460 ms standalone
    and ~2,880 ms inside the server. Ruled out: contention with the dense scans,
    SQLite's per-connection page cache, GC pressure. A fresh process converges
    toward the standalone number only on its third pass. That is worth about 2x —
    more than this PR — and it is the next thing to look at.

The bench harness behind these numbers is not in this repository —
/loadtests/ is gitignored, corpus and tools alike. The workspaceScanQuery
doc comment says how to recreate it.

dvcdsys and others added 5 commits August 21, 2026 01:11
… function

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 <noreply@anthropic.com>
… the comment it outdated

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 <noreply@anthropic.com>
…nshadow rows

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 <noreply@anthropic.com>
…e bound-variable fact

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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@dvcdsys
dvcdsys merged commit 9904eee into develop Aug 21, 2026
1 check passed
@dvcdsys
dvcdsys deleted the perf/workspace-bm25-bounded-heap branch August 21, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant