Conversation
…broken clones Context: production hosts ~70 actively-pushed external repos; the data dir grew to ~76 GB against ~2.3 GB of database. Root cause: the update path in repocloner is fetch(Depth:1, Force) + hard reset, go-git writes ONE NEW PACKFILE per fetch, the reset makes the previously fetched tree unreachable, and nothing ever runs gc (go-git has none; the distroless runtime has no git binary to shell out to). So every push of every repo permanently added a pack, forever. Full brief with measurements: loadtests/GIT_STORAGE_CONTEXT.md (local, gitignored). Changes in repocloner.CloneOrFetch, reuse path restructured into cloneFresh/updateExisting/reclone: 1. Packfile budget (maxFetchPacks=20): before reusing a checkout, count .git/objects/pack/*.pack; at/over budget, discard the directory and shallow-clone fresh. A fresh clone is one pack; real git auto-gcs at 50. This both bounds future growth and automatically reclaims the existing bloat in production: each over-budget repo re-clones on its next webhook/poll fetch. After a re-clone PrevIndexedSHA is unreachable, so Changes=nil and the caller lands in reconcile mode (hash-gated, cheap). 2. Self-healing reuse path: failures rooted in on-disk state (PlainOpen, pre-fetch Head — the signature of a SIGKILL-mid-clone half-write that previously wedged the repo in a forever-failing error loop, remote URL mismatch after a github_url change, post-fetch ref resolve, worktree, reset) are wrapped with an errLocalState sentinel and recovered by nuke + re-clone. Fetch/transport errors deliberately stay fatal-but- preserving: a network blip must not cost a healthy clone and force a reindex. A cancelled context also never triggers the nuke. 3. Result.RecloneReason (informational) + a log line in repojobs.handleClone so operators can see why a fetch turned into a full clone. 4. maintenance.DirSizeBytes now skips unreadable subtrees and keeps counting instead of returning (0,false) on any walk error — previously one bad directory made the whole "Cloned repositories" row vanish from the Resources screen. (0,false) is still returned for a missing/unreadable root (keeps "unreadable" vs "empty" distinguishable) and on context cancellation (partial numbers from an aborted request are not cached). Not addressed here (possible follow-ups): a maintenance category for bloat inside live checkouts (the packfile budget makes it mostly redundant for actively-pushed repos, but idle bloated repos only shrink on their next fetch); per-branch checkout duplication (no shared object store across branches of the same repo); accounting for a moved CIX_REPOS_DIR stranding the old tree. Tests: 4 new repocloner tests (half-written clone self-heals; changed remote URL re-clones; packfile budget re-clones and resets the count; fetch failure against a dead upstream preserves the local clone) + 4 new DirSizeBytes tests (sum, missing root, unreadable subtree partial, cancelled context). Full server suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The voyage provider had no tokenizer, so it guessed token cost as len(bytes)/2
and lived with the consequences: batches sized against a guess, a TPM throttle
metering phantom tokens, and over-long inputs cut into byte windows whose
vectors were then averaged. This adds a pure-Go, count-only implementation of
the model's actual pipeline and wires it in behind an interface, so the
guessing path becomes the fallback rather than the only option.
WHAT THE HEURISTIC ACTUALLY COST, measured against Voyage's own
usage.total_tokens (412 single-input requests) and against the HuggingFace
Rust tokenizer as oracle (20k real chunks from a 45-repo corpus):
- overestimates by 1.94x on average, so the TPM token bucket subtracts
roughly twice the tokens actually spent. At the observed indexing rate
(128 chunks/s, mean 221 tokens/chunk) real usage was ~1.7M TPM while the
limiter believed 3.3M and throttled against a 3M budget — the run was
self-limited to about half the tier's real capacity.
- AND still undercounts 0.5% of chunks, worst case -41%, which is the
direction that ships an over-limit POST. The safety the constant was
chosen for is not actually delivered.
- the "~1.4 bytes/token worst case" in the old comment is optimistic;
measured worst is ~1.18.
WHY A HAND-ROLLED TOKENIZER RATHER THAN A LIBRARY. Both pure-Go candidates
were evaluated against the same oracle on the same corpus:
- sugarme/tokenizer cannot load this tokenizer.json at all. It panics in
regexp.MustCompile: the Qwen2 pre-tokenizer pattern contains \s+(?!\S),
and Go's RE2 has no lookahead.
- the ollama tree's pure-Go tokenizer knows about that and rewrites the
pattern, but its compensation only covers space indentation. Tabs take a
different branch, so it mismatches HF on 8.19% of real chunks (30.2% of
chunks containing a tab), 1471 of them undercounts, worst -16.7% on
tab-indented JSON. It also skips the declared NFC normalizer, so
decomposed input costs an extra token.
bpecount instead implements the seven alternation branches by hand with Perl
leftmost-first semantics, which is what makes the whitespace precedence come
out right. Validated at 0 mismatches against the HF oracle across 70,412
inputs (412 Voyage-verified, 20k real chunks, 50k fuzz). It needs no new
module — golang.org/x/text was already an indirect dependency — and counts
27.5k chunks/s single-threaded, which is orders of magnitude above any
indexing rate on the 2-vCPU box this targets.
Only the merges array is read; the vocabulary is not needed for a count. New
pre-tokens are merged through a linked list plus a rank heap rather than the
naive pair rescan, because minified and generated files are real: a 30 KB run
of one character took 9s the naive way and takes ~16ms this way.
WHAT CHANGES FOR CALLERS. tokenizer.Budget is one interface, not a mandatory
counter plus an optional splitter, so the chunker takes a single constructor
argument. ExactCounts() is how a caller learns whether it may size to the
limit or must keep the old margin. The doc comment records the cost asymmetry
that makes two methods worth having: both do the same single pass over the
same memo, but CountTokens allocates nothing and runs per chunk (1.9M times on
the reference corpus) while SplitPoints builds an offsets slice and runs only
when an input does not fit (5 times on that same corpus).
SplitPoints is exact, not iterative. Because Split runs with Isolated
behaviour and BPE is applied per pre-token, merges never cross a pre-token
boundary, so token counts are additive over pre-tokens and a single
left-to-right pass yields cut points whose pieces provably sum to the whole.
Only a single pre-token larger than the budget needs a search, and it gets a
binary search on bytes inside that one pre-token.
Batch cap goes 80K -> 115K when counts are exact: the 40K of headroom existed
to absorb the heuristic's undercount against Voyage's 120K hard limit, not to
absorb anything on Voyage's side. An operator's explicit MaxTokensPerRequest
still wins over both.
MEASURED EFFECT, so nobody expects the wrong thing. Simulating both packers
over 200k real chunks: 1724 requests today vs 1568 with exact counts — 1.10x,
not the 2x the 1.94x inflation suggests. Batches are bound by the 128-input
cap of voyage-code-*, not by tokens (mean chunk is 221 tokens, so 128 inputs
is ~28K against an 80K cap). The throughput win is in the TPM throttle, not in
packing; the packing win is real but small.
DELIBERATELY NOT IN THIS CHANGE. The chunker does not yet consume Budget —
splitOversizeInput still byte-windows on the provider side, and only the
provider-side counting is switched over. That integration is the follow-up
this interface exists for, and it is where averaging over byte windows finally
goes away. On the reference corpus 5 chunks in 1.9M exceeded the old 30 KB
threshold, so the damage is bounded and self-healing: those files re-embed
correctly the next time they change, and can be found with
SELECT file_path FROM vector_contents WHERE LENGTH(content) > 30000.
TESTING. The golden counts need the real 7 MB tokenizer.json, which is not in
the repo, so those tests skip on a clean checkout — CI coverage comes from a
synthetic merge table that exercises the splitter, the merge loop and the
additivity property SplitPoints depends on. Packaging that file (embed vs
fetch-and-cache; only ~3.5 MB of it is load-bearing) is still open, which is
why the path is an operator-set config field for now and an unreadable path
degrades to the estimate with a warning rather than failing to start.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The chunker's size limit was 1500*3 bytes — a token target expressed in the only unit it had. That ratio holds for dense ASCII code and misses everywhere else: a comment block in Cyrillic or CJK costs two to three bytes per character, so a byte-capped chunk carries a third of the tokens intended, while minified JavaScript packs several times more. The chunker now asks the active embedding provider what a chunk actually costs, when that provider can answer. Wiring: embeddings.Service.TokenBudget() hands out the live provider when it implements tokenizer.Budget, nil otherwise. The indexer asks PER FILE rather than caching it, because a provider swap between files is legitimate while mixing two models' limits inside one file's chunk set is not. CIX_MAX_CHUNK_TOKENS finally does something: it was parsed into config and only ever used to size llama's context. The bound is applied ABOVE the chunking paths, not inside the tree-sitter one. That placement is the point: minified files and files with no grammar fall through to chunkFallback's sliding window, and those are precisely the inputs that blow the model's input limit. A budget that only covered the tree-sitter path would have missed exactly the cases it exists for — the first draft did sit inside that path, and the two tests that now cover minified JS and a grammarless file both failed against it. With a budget in hand the byte cap is deliberately raised out of the way (innerMax = MaxInt32) so semantic units arrive whole and are cut once, in tokens. Without one, nothing changes: nil budget, or a provider that reports ExactCounts() == false, keeps the byte path byte for byte. An estimating provider is routed to the byte path on purpose — its numbers are the same guess, and dressing them as a token budget would hide that from the caller. splitChunkTokens also closes a hole the byte splitter had. That loop requires len(currentLines) > 1, so a single line longer than the cap was never split: on the 45-repo reference corpus that produced a 65 KB chunk, which the voyage provider then cut into byte windows and averaged the vectors of — a vector representing neither half, with nothing in the logs to say so. A line that cannot fit alone is now cut on real token boundaries from Budget.SplitPoints, and the tail seeds the next chunk so short following lines can still join it. Attribution rule is unchanged: only the first piece keeps SymbolName and ChunkType, the rest become `block`, so one long function does not produce N symbol rows all claiming to be it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e counts The first token splitter counted each line, accumulated, and re-joined the lines it had collected. Joining reinserts the newlines, and a newline plus the next line's indentation is its own pre-token, so the joined text costs more than the sum the loop was tracking. On real files a 1500-token budget produced chunks of up to 1546 tokens — verified by counting the largest chunks of two freshly reindexed repos with the model's own tokenizer. Cut positions now come from Budget.SplitPoints over the whole content, which is exact by construction, and the pieces are SUBSTRINGS of that content rather than reassembled text. The class of error disappears instead of being compensated for. Two consequences worth spelling out, both learned from tests that failed: Snapping a cut back to a line start (so a piece never begins mid-line and its recorded line range does not lie) shrinks the piece BEFORE the cut and grows the one after it. Reusing the remaining offsets SplitPoints had returned would hand the next piece the tokens this one gave up and push it over budget. So each piece recomputes its cut from the actual new start; the loop is not an inefficiency, it is the correctness. The content-preservation invariant is asserted on splitChunkTokens directly rather than through ChunkFileTokens, because the sliding-window fallback deliberately overlaps its windows for recall — whole-pipeline output is not expected to concatenate back to the source, and a test that assumed otherwise was wrong about the pipeline, not about the splitter. Re-verified on the file that produced the reference corpus's 65 KB chunk: 48 pieces, largest exactly 1500 tokens, none over. The byte path leaves that file as 6 chunks whose largest is 65,553 tokens — twice voyage-code-3's 32K context, which with truncation enabled means the tail was silently dropped before embedding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n a real corpus Three bugs shipped into this branch before being caught, and the reason each one survived is the same: the only thing verifying the token layer was my own model of it. The tokenizer next door had zero bugs across 70,412 inputs because it could be checked against HuggingFace's implementation and against Voyage's billing. Chunk splitting has no such oracle — where to cut a chunk is our decision, not a spec — so the oracle has to be built out of invariants and inputs nobody wrote for the test. fakeBudget was the direct cause of the worst one. It counted whitespace-separated words and let newlines be free, which made the sum of per-line counts equal the count of the joined text — exactly the assumption the implementation got wrong. The double agreed with the bug, the tests passed, and real files came out 3% over budget. It now charges a token per newline, like the real tokenizer does, so summing parts no longer equals the whole unless the pieces are substrings — which is the property the splitter must have. The corpus tests run the chunker over real files from the local fixture: 396 files across 45 repositories, 5,199 chunks, largest exactly 1,500 against a 1,500 budget; 132 files large enough to need splitting, all reconstructing byte for byte with correct line numbers. Those files are what found the original bugs — a 65 KB single-line Zig literal, minified JavaScript that has no grammar and so takes the fallback path — and hand-written cases had not. The fixture is tens of gigabytes and is not in the repository, so the tests skip unless CIX_TEST_CORPUS_DIR and CIX_TEST_TOKENIZER are set. A clean checkout and CI stay green; anyone with a fixture gets the coverage. The file header says how to point them at one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cing budget-reclone Replaces the maxFetchPacks nuke-and-reclone bound (previous commit) with the approach validated by the PoC on branch poc/gc-compaction (server/cmd/gc-poc, run against all 45 loadtests fixtures — spring-boot, grafana, etc — with byte-level identity vs canonical full clones, git fsck --strict, a 12-round leak test, and O() scaling measurements; see that branch's commit messages for the numbers). Two production changes: 1. Tags:NoTags on both clone and fetch (repocloner.go). go-git's clone default is AllTags, and on a shallow clone every tag arrives as a FULL tree snapshot: real checkouts measured 2.6-5x the worktree on day zero (spring-boot: 391 tags -> 102MB store / 39MB worktree; worst fixture: 1240 tags, 503MB / 278MB). cix indexes exactly one branch and never reads tags. This kills the second bloat driver at the source. 2. In-place compaction (compact.go), run from CloneOrFetch after every successful update when needsCompaction() says so: packfileCount >= compactPackThreshold (4) — the fetch+reset path persists one snapshot-sized pack per fetch and go-git has no gc — OR tag refs present. The tag trigger IS the upgrade migration: the first ordinary update of every pre-NoTags checkout drops refs/tags/*, collapses the store to one branch-snapshot pack and rewrites .git/shallow, with no separate migration code. Runs on the NoChanges path too, so cleanup does not wait for the repo's next commit. Mechanism: reachability walk from non-tag refs + explicitly protected commits (PrevIndexedSHA — the base of the next incremental tree-diff, unreferenced once the branch moves, kept alive across compactions), honouring .git/shallow graft points exactly like git; encode the set into one pack via storer.PackfileWriter (window 0 — measured -17% size for 2.9x CPU with deltas, not worth it); delete old packs only after the new pack is durable (crash mid-compaction leaves extra packs, never a broken store); drop all loose objects; keep only still-present shallow entries. The walker is go-git's objectWalker with three fixes real repos hit immediately: shallow grafts terminate parent walks, submodule gitlinks are skipped, symlink-reached blobs are leaves. Costs (measured): linear time ~0.2-1.4ms CPU per object + ~0.2s/GB emitted; heap ~3x the uncompressed snapshot (go-git's encoder materialises content) — package-level compactMu serialises compactions across concurrent clone jobs so those peaks never stack on the 8GB prod hosts. Failed compaction falls back to nuke+reclone (store state unknown; shallow reclone is always correct). Result gains Compaction *CompactStats; repojobs logs it (bytes before/ after, packs deleted, tag refs dropped, duration) next to the existing RecloneReason log. Tests: fresh clone carries zero tag refs; the upgrade scenario end-to-end (legacy AllTags clone + 2 legacy fetch cycles -> first new-code update compacts: 2 tag refs dropped, packs 3+ -> 1, objects dir shrinks, the SAME update still returns the v4->v5 incremental ChangeSet, a later update still diffs from the protected-but-unreferenced indexed_sha, and a no-op cycle stays quiet); pack count stays <= threshold across 8 push/fetch cycles with at least one compaction. Budget-reclone test removed with the mechanism. Full server suite + vet green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mits Nine findings from review, grouped by what they were actually about. CORRECTNESS — offsets into the wrong string (blocking). SplitPoints normalised its input and returned offsets into that NFC copy while callers slice the original. NFC changes lengths in both directions: decomposed input shrinks (e + U+0301 becomes one code point) so cuts landed mid-rune, and the composition exclusions at U+0958..U+095F DEcompose under NFC, making the normalised form longer, so an offset could exceed len(raw) and panic the indexer on the slice expression. Reproduced both ways before fixing. Input that is already NFC — nearly all source — takes a fast path where the two coincide; anything else is cut one piece at a time, each cut mapped back to a raw offset on a normalisation boundary, rounding down so the budget survives the mapping. Every previous fixture was ASCII, which is why nothing caught it. CORRECTNESS — an over-budget pre-token left its tail uncounted (blocking in spirit; latent for today's only caller). After splitInside the accumulator was reset to zero although the tail after the last inner cut opens the next piece, so following pre-tokens could stack a full budget on top of it and produce pieces of nearly twice the budget. The chunker escaped it by consuming only the first cut and recomputing, but the doc invites consuming the whole slice. CORRECTNESS — an unvalidated tokenizer could be confidently wrong (blocking). LoadBytes checked only model.type == "BPE" while the Qwen2 pre-tokenizer regex and the NFC normalizer are hardcoded here. A GPT-2 or o200k tokenizer.json parses fine, declares BPE, and would have produced plausible wrong counts with ExactCounts() reporting true — which then sizes chunks and packs batches. The declared normalizer and pre-tokenizer are now compared against what this package implements, and a mismatch fails the load so the caller stays on its estimate, which is wrong but knows it. CORRECTNESS — MaxInputTokens was a constant 32000, but the provider's own model enum offers voyage-code-2 at 16K. It is a per-model table now, with unknown models falling back to the smaller window: undershooting costs a needless split, overshooting costs silently truncated input. CORRECTNESS — token-sized chunks reopened the averaging path. The chunker lifts the byte cap when it has a budget, but the provider still cut any input over 30 KB into byte windows and AVERAGED their vectors — the invisible quality loss this branch exists to remove. At CIX_MAX_CHUNK_TOKENS=20000, legal and well inside the window, that would have been routine. With a tokenizer the provider now asks the real question (does this exceed the model's context) and cuts on token boundaries; byte windows remain only where there is no tokenizer and therefore no better answer. CORRECTNESS — the fallback path was still byte-biased. innerMax reached only the tree-sitter path, so files with no grammar kept the fixed 4000-byte sliding window, and boundTokens could not repair it: it splits chunks that are too big and cannot grow ones that are too small. Multi-byte text — which correlates with "no grammar" more than one would like — produced chunks worth a third of the budget on exactly the path the description claimed was covered. ROBUSTNESS — Service.TokenBudget was the only Service method without the nil/disabled guard, and a typed-nil *Service satisfies the capability interface the indexer asserts on, so the first indexed file would panic in RLock. The indexer's inline anonymous assertion is now the named TokenBudgetSource, hoisted out of the per-file loop (the per-file CALL stays — a provider swap between files is legitimate): renaming TokenBudget is now a compile error rather than a silent return to byte chunking. EFFICIENCY — splitChunkTokens called SplitPoints per piece and used only the first cut, rescanning the whole remainder each time: on the 66 KB single-line case that is ~990 suffix scans instead of ~44. The cut list is now reused while it stays valid and recomputed only when snapping to a line start actually moves a boundary — which on single-line content never happens. boundTokens also skips tokenising entirely when len(content) <= budget: a byte-level BPE token cannot cover less than a byte, so that comparison PROVES the chunk fits, and most chunks are small. Corpus property tests dropped from 5.4s/7.6s to 3.8s/2.2s. EFFICIENCY — the contraction branch lowercased the entire remaining suffix for every apostrophe, though only two bytes can match. A 512 KB file with 5,000 quotes moved over a gigabyte through the allocator on the indexing hot path. Also: the dead estimating branch of Provider.SplitPoints (unreachable, and it looped forever when the budget was smaller than one leading multi-byte rune) is gone; the batch log reported the Config cap while packing used the Provider cap; CIX_MAX_CHUNK_TOKENS's default now comes from chunker.DefaultMaxChunkTokens instead of a second copy of 1500; and the /loadtests/ gitignore entry ships here rather than sitting in a working tree, since committed tests reference that path. Tests: NFD and composition-exclusion cases (both previously absent — every fixture was ASCII), a foreign-pipeline rejection case, an uncounted-tail case, a fallback-fills-the-budget case, and TestSplitPointsOversizePreToken fixed to actually enter splitInside — "aB3" repeated pre-tokenizes into 2-byte pieces that never exceed any budget, so the path it named was never executed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…byte runs Two blockers from the second review, both confirmed before fixing. DEADLOCK. splitInside binary-searched over byte offsets and aligned its midpoint to a rune start by DECREMENTING. When alignment pulled the midpoint below lo, the next lo = mid+1 did not advance, and the (lo, hi) pair repeated forever. Any run of multi-byte runes long enough to exceed the budget as one pre-token reached it — a box-drawing comment separator does, and so does an arrow run or a run of combining marks. The worker did not error or slow down, it stopped. Reproduced at budgets 5 and 50 on four inputs, including already-NFC ones on the fast path, so this was not confined to the new denormalised route. The search now runs over rune-start INDICES enumerated once, so every candidate is a valid boundary by construction and no alignment step exists to misbehave. A single rune that exceeds the budget is emitted rather than refused: not advancing is the failure being removed. All four inputs are tests now, at both budgets, asserting termination AND that no piece is cut mid-rune. Strictly this loop predates the previous commit, but the denormalised path added routes into it. UNREACHABLE FALLBACK. chunkFallbackTokens was dead code. chunkWithTreesitter never returns an error — all six of its decline paths called chunkFallback themselves and returned the byte windows as SUCCESS — so the caller's fallback branch could not run, and no-grammar, minified, parse-failure and empty-AST files kept getting 4000-byte windows regardless of any token budget. That is the finding the previous commit claimed to fix. Those six sites now return errUseFallback and the caller decides, because the caller is the one that knows whether a budget is in play. ChunkFile's byte path is unchanged: it already mapped err to chunkFallback. The test that was supposed to catch this passed by arithmetic. At budget 200 a 4000-byte window of the fixture is ~358 fake tokens, so boundTokens split every window into 200+158 and both halves cleared half-budget without the fallback being token-aware at all. Raised to 800, where a raw window is BELOW half the budget: verified it now fails against the old routing (7 of 8 chunks under half budget) and passes against the new. Also from the review: the split log reported max_input_bytes even when the split was token-bounded; the provider-level token-split branch had no direct test (added, covering both pass-through of a large-but-legal input and cutting one past the window); a doc comment still named the old lowercase constant. The token fallback drops the byte window's 500-byte overlap. That is deliberate and now documented at the site: overlap has to be expressed in tokens to coexist with a token budget — size pieces at budget minus overlap, then extend each start back into its predecessor — which is a change worth making on its own rather than smuggling into a correctness fix. The tree-sitter path has never overlapped, so the two paths are now consistent rather than the fallback being worse than its neighbours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat: size chunks in tokens — exact counting via the model's own BPE tokenizer
Review (adversarial, 8 CONFIRMED + 2 PLAUSIBLE) accepted in full; every
finding fixed, plus the runner-ups. Mapping:
1. Failed compaction no longer nukes the checkout. Compaction moved out of
CloneOrFetch entirely into exported MaybeCompact; repojobs calls it after
the clone section, logs a Warn on error and moves on — the store is
exactly what the update left (valid), and the trigger re-fires next
cycle. Nuke+reclone is reserved for the errLocalState taxonomy.
2. No more global-mutex-inside-repo-lock convoy. MaybeCompact acquires the
global compaction gate FIRST, then takes the per-repo write lock (passed
in by repojobs as a closure) only around the store mutation. A job queued
behind another repo's compaction now holds no locks at all, so readers
of its repo proceed; upgrade-day fleets serialise on the gate without
stalling each other's reads.
3. NoChanges no longer trusts HEAD: updateExisting performs the hard reset
on the NoChanges path too (a no-op write on a clean tree), repairing the
torn-worktree state a crash mid-reset leaves behind (go-git writes HEAD
before touching files). Test: NoChangesStillRepairsWorktree.
4. Crash-safe trigger ordering: tag refs are now dropped AFTER the new pack
is durable and BEFORE old packs are deleted (a ref never dangles over a
missing object). The remaining window — tags dropped, old packs still
present — is re-armed by a new needsCompaction backstop: store >= 2x
worktree (>=2 packs, 1MB floor). Test: RatioBackstopRearms simulates
exactly the crashed state.
5. compactCheckout takes ctx: checked between phases and every 1024 objects
inside the walk; cancellation aborts cleanly before any deletion.
MaybeCompact also re-checks after waiting on the gate. Test:
CancelledContext.
6. Pending index target protected: Result gains PrevHeadSHA (the on-disk
HEAD before the fetch — the TargetSHA of a possibly still-queued index
job) and repojobs passes {IndexedSHA, PrevHeadSHA} to the protect set.
Test: ProtectsPendingIndexTarget reproduces the two-cycle race.
7. Protect probe distinguishes not-found from store errors:
errors.Is(ErrObjectNotFound) -> skip; anything else -> loud failure.
Same fix applied to the walker's parent probe.
8. errLocalState gets one retry before the nuke: transient EMFILE/EIO-class
failures heal on the second attempt, a genuinely broken checkout fails
identically and still recloses. (Existing half-written-clone test covers
the broken path through the retry.)
9. DirSizeBytes undercounts are now visible: dirSizeDetail returns a
skipped-entry count, DiskUsage gains partial:true (openapi.yaml +
openapi-gen regenerated; maintenance.Usage serialises straight to the
wire) and computeUsage logs a Warn naming the disk and skipped count.
10. Reachability walk converted from recursion to an iterative worklist —
stack depth no longer scales with commit-chain length, so a full-history
clone seeded into the repos dir cannot blow the goroutine stack.
Runner-ups: the seven errLocalState wraps use double-%w so the cause chain
survives errors.Is/As; refs are read in ONE pass (tags collected and roots
seeded together); needsCompaction's ratio walk is gated on pack count >= 2
so the steady state never pays it.
Not taken (documented deliberately): clone-into-temp+rename for reclone —
reclone is now confined to genuinely-unusable-state paths where the old
checkout has no value; the review marked it optional.
Full server suite, vet, and openapi-gen sync green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(server): compact repo checkouts in place (NoTags + reachability repack), self-heal broken clones
Workspace search over the 45-repo load-test fixture took 23.9 s at the median
on a 14-core Mac with NVMe and 36 GB of RAM. Production is an e2-standard-2
(2 vCPU, 8 GB, network PD) behind a Cloudflare tunnel whose 100 s edge timeout
would fire before the answer.
The cause was not the fan-out logic, it was the volume. A 2048-dim float32
embedding is 8192 bytes, past SQLite's 8157-byte local-payload limit on our
8 KiB pages, so every `vectors` row spilled into its own overflow page.
Measured with dbstat on the fixture: 238,727 leaf + 1,909,447 overflow pages
for 1,909,447 vectors = 9,216 bytes read per vector, 17.6 GB per workspace
query. The schema comment still described the 768-dim case ("the scan reads two
rows per page and never follows an overflow chain"), which was true of every
model the store originally shipped with.
Fix: `vectors_q8`, the same vectors at one byte per component plus a per-vector
scale, is what a search now scans. It takes a shortlist, and the shortlist is
rescored against the float32 originals in `vectors`, which stay authoritative
and untouched. Measured on the same fixture: 636,483 leaf pages, zero overflow,
2,731 bytes per vector, 5.2 GB per workspace query — 3.4x less. The file grew
20 GB -> 26.35 GB (+31%).
Why rescoring rather than trusting int8. On 60k vectors of the fixture's
largest collection (ziglang/zig, voyage-code-3 @2048) against 50 REAL
query-side embeddings, recall of the exact float32 top-K:
shortlist k=10 k=20
20 0.998 0.994
40 0.998 0.999
60 1.000 1.000
200 1.000 1.000
int8 alone gives 0.994 at both k. The quantisation misorders near-ties, it does
not lose the documents, so re-reading a few dozen exact vectors recovers all of
them — hence a floor of 64 and 4x the limit above it. An earlier version of
this experiment drew its queries FROM the corpus; a corpus vector is an exact
member of the set being searched, its neighbours are far away, and it made
rescoring look worthless (0.990 either way). Query-side embeddings are the
regime that decides.
Latency, A/B on the same machine and the same warm page cache, back to back,
by flipping CIX_VECTOR_SCAN_QUANT (10 queries, one repeat each):
float32 scan int8 + rescore
single project 15,899 ms 1,422 ms p50
23,642 ms 2,093 ms p95
workspace (45) 23,879 ms 10,544 ms p50
33,366 ms 25,186 ms p95
The single-project scan improves more than the 3.4x byte reduction because
5.2 GB fits this machine's page cache and 17.6 GB does not. The workspace
number improves less, so something other than the dense scan now dominates the
fan-out — but what, exactly, is not established. BM25 is the obvious suspect
(`chunksfts.SearchProject` matches `chunks_fts` across the WHOLE server and
filters by project afterwards, once per repo), and measured through the
server's own driver it costs 326-542 ms per repo on this fixture, which does
not account for 10 s. The next step is per-phase timing inside the handler
rather than another guess.
A note for whoever measures next: the same BM25 query timed through Python's
system sqlite3 on this Mac takes 18 s, repeatably, against 380 ms through
modernc.org/sqlite. Any conclusion about FTS5 cost drawn with a different
SQLite build than the server's is worthless.
End-to-end check that the approximation is invisible: 20 queries, top-20, on
the full 346k-vector zig collection, captured with the compact scan off and
then on. 20/20 byte-identical, including the scores.
Mechanics:
- Scores returned to callers are always the exact cosine, never the int8
estimate. This is load-bearing beyond cosmetics: min_score thresholds on
it, the workspace fan-out min-max normalises across projects with it, and
hybrid search blends it with BM25 — an approximate score would move results
BETWEEN projects in a way no single-project test would catch.
- Candidates are keyed by doc_id, not rowid. `vectors` has a composite
PRIMARY KEY so its rowid is implicit, and SQLite only promises to preserve
implicit rowids across a VACUUM for tables with an INTEGER PRIMARY KEY.
Survivable while the rowid never leaves one query; fatal once a second
table keys off it.
- Readiness is a q8_state row, not a COUNT. Collections created by this code
are marked at creation (empty, therefore complete) and every upsert
maintains both tables in one transaction. Collections written by an older
binary have no flag, keep using the float32 scan, and are converted by a
background pass at open — largest first, 2000-row transactions, 50% duty
cycle, free-space checked up front. The flag is written in the same
transaction as the batch that proves it, so a kill leaves a collection
unmarked and still correct, never marked and incomplete. On the fixture the
backfill converted 1,909,447 vectors in 245 s.
- vectors_q8 carries `language` because that is the only filter any caller
produces (fetchVectorResults, from the `languages` parameter). Any other
`where` key falls back to the float32 scan, which has every column.
`{"language": ""}` is a filter, not the absence of one — chromem compared
metadata["language"] to "" — and the test for that fails against the
obvious `if language != ""` version.
- CIX_VECTOR_SCAN_QUANT=false opts out: the copy is a quarter of the float32
bytes on top of an already large store. Turning it off also withdraws the
completion flag from anything written while off, so turning it back on
rebuilds rather than trusting a stale copy.
- Deletes reach the copy in both directions, and delete-by-file runs BEFORE
the vectors delete because its subquery reads file_path from `vectors`. An
orphaned q8 row is a document the scan keeps shortlisting and the rescore
can no longer score: it vanishes from results with nothing logged.
- The legacy chromem import still writes float32 only; it creates its
collection with raw SQL so nothing marks it complete, and the backfill that
runs right after picks it up.
Also here, because it is the same query path and it was free: the stale-FTS
probe in workspace search used `SELECT COUNT(*) ... LIMIT 1` per repo, which
walks every matching index entry to answer a yes/no question (the LIMIT bounds
the result rows of an aggregate that always returns one). Measured on the
fixture's 1.95M-row chunks_meta across 46 projects: 53.2 ms as COUNT, 0.2 ms as
EXISTS. It runs serially, before the fan-out, on every workspace query.
Tests. The layout ones assert PAGES, not milliseconds, so they mean the same
thing in CI, on a laptop and on the production box — multiply by that machine's
read throughput and you have its latency. TestScanPackingEfficiency also fails
on the 1024-dim case as float32 (8192 bytes read to obtain 4096: one row per
leaf page, half of it air), which was reachable by an operator halving
output_dimension to save time and getting half the vector quality for 89% of
the I/O. The behaviour tests use a corpus with deliberate near-duplicate
clusters, because random unit vectors in 2048 dimensions are nearly orthogonal
and have no near-ties for a quantiser to confuse — real code corpora are the
opposite. TestScanQuantOffThenOn covers the toggle that would otherwise leave a
collection marked complete and missing every row written while it was off.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s, toggle staleness, exactness claims Addresses the review of 838c923. Findings 1-10 plus the minors; nothing was waved through, and two of them were only provable by writing a test that fails against the old code first. ## The backfill was racy, and the comment claiming otherwise was false (1) backfillBatch reads a batch in one implicit transaction, quantises it in Go, and writes it in another. It holds no lock across the gap — deliberately, since the thing most likely to want the write lock is the file watcher reindexing a file somebody just saved — and on the fixture that gap is open across 245 s of live server. Two things went wrong in it, neither of which surfaces as an error: - a doc deleted in the gap had its compact row REINSERTED, and nothing could ever remove it again: DeleteByFile finds doc_ids through `vectors`, where the row no longer is. Every later scan shortlists the orphan and every rescore drops it, so the query silently returns fewer results. - a doc re-embedded in the gap had its fresh compact row OVERWRITTEN by this batch's quantisation of the embedding it had just replaced. The document then ranks by a vector it no longer has. Both are now closed at the statement level rather than by locking: WHERE EXISTS (do not resurrect) and ON CONFLICT DO NOTHING (never be the later writer — the backfill only fills gaps). The empty-batch completion no longer claims to be "in the same transaction as the query that proved it", which was literally untrue; completeness rests on those two clauses, and the comment now says so. TestBackfillSurvivesConcurrentWrites runs the backfill against a churning collection and asserts invariants rather than an interleaving, so it can only fail for a real reason. TestBackfillNeverResurrectsOrOverwrites pins the two clauses deterministically. Both fail against a plain upsert — verified by reverting the SQL and re-running. ## One failed collection stopped the other forty-two (2) backfillQ8 returned on the first per-collection error. The realistic cause is a collection deleted mid-walk (admin project delete, orphan sweep) failing the next insert's foreign key — after which every remaining collection stayed on the float32 scan until somebody restarted the server, with one warn line as the only trace. Now logged per collection and skipped; the completion line reports how many failed. ## A legacy import could hide documents behind a live flag (3) The import writes `vectors` directly and creates its collection with INSERT OR IGNORE, which was justified as "nothing marks it complete". True only when the collection is NEW. An operator who indexed a project live (ensureCollection flags it at creation) and then pointed CIX_CHROMA_PERSIST_DIR at a legacy tree reaches the other case — migration_state is keyed on the legacy collection name and has never seen it — and the imported docs get no compact rows inside a collection whose flag says it is complete. The backfill skips flagged collections, so those documents would never be searchable on the fast path. The import now withdraws the flag unconditionally. ## Indexing must not fail over a performance hint (4) ensureCollection returned the error from markCollectionQ8Ready, which opens its own transaction and can lose a race for the write lock. That aborted a whole UpsertChunks over a row whose absence costs nothing but a slower scan — and which self-heals, because the backfill sets it at the next open. Logged now. ## The two scan paths disagreed on a zero query (5) quantizeInt8 returns scale 0 for an all-zero vector and scanQ8 short-circuited to empty, while the float32 path scores every row 0 and fills the heap. Same broken query, different answers depending on whether the collection had been converted — and in a workspace fan-out, both at once. The short-circuit is gone: a zero query now scores everything 0 on both paths, which is what quantizeInt8's own comment always claimed. ## "Exact" was overclaiming (6) The docs said results and scores "stay exact". Scores do, by construction — the rescore computes them from the float32 vectors. The SET does not, in the worst case: the shortlist is a fixed width and topK rejects boundary ties strictly, so a collection holding more than `shortlist` documents within one quantisation step of each other truncates the tie in scan order, and the rescore cannot recover a document it never received. Measured error is zero on every corpus tried; that is now what the documentation says, with the boundary case named in q8Shortlist next to the fix that would close it. ## Performance and duplication (7-10) - doc_id was scanned into a fresh Go string on every row of both scan loops — ~1.9M allocations per workspace query on the fixture, and the CIX_VECTOR_SCAN_QUANT=false path paid it too, so the opt-out did not actually restore pre-change behaviour. Both loops now read it as RawBytes and materialise the string only for a row that enters the heap. - The backfill paginated `vectors` in doc_id order. `vectors` is a rowid table whose composite primary key is a separate index, so that scattered ~9 kB row lookups across the collection's whole rowid span — the same 1.8x that made scanSQL pick idx_vec_coll in the first place. Now keyset-paginated on rowid through idx_vec_coll. - rescore duplicated scan's streaming loop line for line, putting the float32 decode protocol in three places; both now call streamExact. The IN-list batching duplicated hydrate's; both now call docIDInList. The scan-slot select was pasted twice; acquireScanSlot. - Options.ScanQuant's comment described a design that was not built ("writes maintain it either way"). Rewritten to match: the flag governs the whole lifecycle. ## Minors q8_state's comment claimed a dimension column that does not exist. The row-size expression was pasted three times; sizeExprQ8. q8Ready cached negatives it never used; positives only, presence is the answer. clearQ8Ready ran per 500-chunk batch; hoisted to once per UpsertChunks, and moved BEFORE the first write so a crash mid-run cannot leave the flag set over a half-written collection. Writes with the copy switched off now DELETE the compact rows they touch — without that, a doc re-embedded while off kept its stale compact row, and the backfill's new DO NOTHING would have sealed it in on re-enable. Filter fallback to the exact scan logs at debug, and TestQ8FilterableCoversEveryFilter fails when a new filter column appears without a decision about it. layout_test claimed scanTable was derived from the SQL; it now is, via an assertion. Not done, with reasons: quantizeInt8's per-component division stays a division — precomputing the reciprocal changes stored values for a loop that runs once per chunk at index time, and the end-to-end identity check below is worth more than the microseconds. The maintenance q8 aggregate still walks leaf pages; it sits behind the maintenance service's TTL cache, and the cheap fix if that changes is recording the total at completion, noted at the constant. Verified on the 45-repo fixture, not only in unit tests: one collection's compact copy was wiped, the server rebuilt it (54,063 vectors, 8 s, zero orphans in the whole database), and 20 queries x top-20 came back identical both to the pre-review compact scan and to the exact float32 scan — 20/20, scores included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
perf(vectorstore): scan an int8 copy and rescore the shortlist exactly
Stage 1 of the three-stage search-perf plan. It builds nothing faster; it exists so stages 2 and 3 are not guesses. The previous round of work budgeted a 10.5 s workspace query by measuring the dense scan, the BM25 query and the fan-out's parallel speedup separately and multiplying. The budget closed, which is not the same as being right, and two optimisations were about to be built on it. One of the two multiplicands turned out to be off by 18x. What lands: - searchtimings.go: a searchPhases accumulator. Serial phases (embed, stale-FTS probe, fan-out wall, fuse) are plain durations; the two per-project phases keep a SUM and a MAX behind atomics, because the sum is the work the query did and the max is what the user waited for, and under sublinear parallelism those are different questions. Reporting either one alone hides which. - The fan-out records dense and BM25 per project (workspacesearch.go). - projects_scanned / projects_returned, because their ratio is the premise of stage 3: the fan-out does full work on every project and then thresholds the answer down. Always collected, conditionally reported. The measurement costs a handful of time.Now() calls and two atomics against a query that reads gigabytes, so there is no reason to gate the collection. Where it goes is gated twice, and the two gates answer different questions: - the LOG line fires only above slowWorkspaceQuery (2 s). The server already writes one http_request line per request carrying the wall time, so a second line on every workspace query would be noise added to catch the rare slow one. A threshold keeps the property a ?debug flag cannot have: nobody needs to have switched anything on before the slow query happened. Two seconds is not "wrong" for a fan-out over every project in a workspace — it is the point past which the breakdown is worth storing, and low enough that a regression on a small workspace still trips it. - the RESPONSE object is attached only for ?timings=true, documented as WorkspaceSearchTimings in doc/openapi.yaml. In a response this is a debugging aid, not API surface: every existing caller (CLI, MCP tools, dashboard) gets byte-identical responses to before. One deliberate omission versus the spec in the plan: no hydrate_ms. Chunk payloads are hydrated inside VectorStore.Search and chunksfts.SearchProject, so hydration is not separable from out here; it is inside dense_sum_ms/bm25_sum_ms and addDense says so. Everything after fuse is in-memory slicing. Measured on the fixture, wall minus the four serial phases is ~19 ms of 9,911 ms, so nothing material is unaccounted for. What it measured, on the 45-repo fixture (1.9M chunks, voyage-code-3 @2048, 14-core Mac, int8 scan on), 10 queries against the 43-project workspace, via loadtests/bench/phases.py — medians: wall 9,911 ms | fan-out 9,663 | BM25 sum 93,345 (max 3,210) dense sum 26,476 (max 2,428) | embed 218 | stale-FTS 11 | fuse 0 Dense is a constant; BM25 is the variable and wall tracks it — 78% of the fan-out's work at the median, ranging 15,026-183,177 ms with the number and length of query terms rather than with repo size, because MATCH is evaluated over the whole server's chunks_fts and filtered by project afterwards, once per repo. Two estimates it disproved: - a repo's BM25 measures 326-542 ms standalone (loadtests/bench/ftstest, through the server's own driver) but up to 5,767 ms inside the fan-out. 43 concurrent FTS queries against one index degrade each other by roughly an order of magnitude. Stage 2 removes 42/43 of that work AND the contention, so it is worth more than it looked, not less. - the stale-FTS probe costs 11 ms through the server, not the 0.2 ms a Python-side measurement suggested. Tests assert shape and gating, never wall-clock values. Every field present when asked for; no timings block at all when not asked for, or on a response that ran no search (zeroes would read as "instant"); the counters matching the fan-out actually performed; max <= sum per phase, which is what catches a sum and a max wired to the wrong accumulator; and both sides of the log threshold, from one captured logger, because a test that only proves silence would still pass if the line were deleted. slowWorkspaceQuery is a var rather than a const purely so that test can cross the threshold without sleeping; nothing at runtime writes it. Each gate was mutation-checked: removing the opt-in, removing the threshold, and deleting the log line each fail the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…early returns, F3 wall coverage F1 (the one that mattered): projects_returned reported the count the caller was SHOWN, not the count that survived the relevance threshold. It was taken from projectPayloads, which is already truncated to top_projects (default 10, clamped 1..50). So on the 43-project fixture the scanned:returned ratio read 43:10 whether ten repos were relevant or forty, and it moved when a client passed a different top_projects — a request parameter, not a measurement of discarded work. That ratio is the stated premise of stage 3, so the metric two optimisations were going to rest on was measuring a UI cap. Now returned is len(surviving), and the panel count keeps its own field, projects_in_panel, because "what did the caller get" is a real but different question. F2: both early returns (no visible members, no indexed projects) passed a literal nil and never entered the reporter, so the log threshold and the response opt-in — documented as independent gates — were both off together on those paths. The query embedding has already been paid for by then (218 ms median on the fixture), and a hung embedding provider is exactly what the slow-query line is for. They now report with requested=false: no timings in the response, because nothing was searched, but a slow one still writes its line. F3: wall_ms was documented as "the whole handler" but started after requireWorkspaceVisible and the parameter clamps, and the unnamed remainder silently absorbed the membership SQL and access.AccessibleProjectHostPaths — the one pre-fan-out step that grows with how many projects the caller can see rather than with the workspace, and the one the fixture cannot exercise because an admin (and AuthDisabled) skips the ACL branch entirely. started now sits on the handler's first line, and the resolve phase gets its own resolve_ms. The spec now also states what the remainder is rather than implying there is none. Also from the review, documentation-only: - dense_sum_ms/bm25_sum_ms include projects whose query failed. Keeping them is correct — the time was spent, and excluding it would put the sums permanently below the wall time they exist to explain — but a slow failure can own the max, so both the spec and addDense now say so. - the comment on slowWorkspaceQuery now names the condition its test-only mutability depends on: nothing in this package calls t.Parallel(), and whoever adds the first parallel test here has to move the threshold onto Deps first or hit a data race under -race with a non-obvious cause. Two new tests, both mutation-checked against the bugs they describe: restoring the panel-capped counter fails the F1 test (reports 10, wants 14), and restoring the nil early return fails the F2 test. Not covered: an unrecorded resolve_ms still passes, because a shape test cannot tell an unset duration from a fast one. go test ./... green (46 packages), go test -race on the httpapi package green, make openapi-check in sync, go vet and gofmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up nit from the PR #264 review. reportSearchTimings takes the three project counters as consecutive ints — scanned, returned, panel — which is the signature where a transposition compiles, produces plausible numbers, and stays invisible until someone reasons from the scanned:returned ratio. That ratio is stage 3's premise, and getting it silently wrong is the exact failure F1 already was once. The chained inequality is the only relationship that holds unconditionally: the panel is a cap on what survived, and what survived is a subset of what was searched. Asserted from both tests that read timings, via a shared helper, so it applies to any future one too. Mutation-checked: swapping returned and panel at the call site trips both the F1 test and the new invariant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
perf(httpapi): measure workspace search per phase, inside the handler
Stage 2 of the search-perf plan, and the change stage 1's timings pointed at: BM25 was 78-80% of the workspace fan-out's work. The reason is structural. FTS5 drives the query: it evaluates MATCH over the WHOLE chunks_fts table — every project on the server — joins each hit to chunks_meta, and only then discards the rows belonging to other projects. So a per-project BM25 query costs about the same whichever project it names, and a 43-project workspace paid for the same global match 43 times. On top of that the 43 queries contended over one index: a repo's BM25 measured 326-542 ms standalone but up to 7,000 ms inside the fan-out. chunksfts.SearchProjects replaces them with one statement that ranks within each project via ROW_NUMBER() OVER (PARTITION BY project_path ORDER BY bm, rowid) and keeps each project's top rows. The IN list is batched at 500 paths to stay under SQLite's 999-variable ceiling, the same batch size the vector store already uses. In the handler the query runs in the fan-out's errgroup alongside the dense scans rather than before them, so nothing serialises; fusion moves out of the per-project goroutines because it now needs both sides, and it was under a millisecond across the whole fan-out anyway. Measured on the 45-repo fixture (1.9M chunks, 43-project workspace, 10 queries, medians). Both builds were run back to back against the same already-warm page cache — a process restart does not evict it — with one warm-up pass discarded each time, because a first comparison across a cold restart credited this change with twice the improvement it earned: phase develop stage 2 wall 10,235 4,650 2.2x fan-out 9,961 4,376 2.3x BM25 111,210 4,375 25.4x (summed over 43 -> one query) dense sum 15,166 11,136 1.4x dense max 2,318 1,123 2.1x The dense rows are the ones worth pausing on: nothing in the dense path changed. Removing 43 concurrent FTS queries gave the vector scans back the CPU and I/O they were contending for, which is worth 1.4x on the work and 2.1x on the project anyone actually waits for. BM25 is no longer the dominant term: at 4,375 ms it now sits level with the fan-out's own wall time, so the single FTS query IS the critical path. Whatever comes next should start there rather than from the old 78-80% figure. Correctness, on the fixture, 50 queries, full response captured per query: the BM25 signal is IDENTICAL in all 500 panel rows, and the project panel order is identical for all 50 queries. Dense scores wobble by <=0.0015 in a few percent of rows — but the same binary compared against ITSELF wobbles at least as much (30 rows vs 38), so that is a pre-existing property of the fixture, not this change. Its cause is not established; single-project search repeats bit-identically, and three consecutive workspace queries repeat bit-identically, so it correlates with machine load rather than with the query. Both orderings are (bm ASC, rowid ASC). The rowid is defensive rather than a fix: bm25 ties are the norm in a trigram index — 14 of 16 hits in the package's own test corpus share a score — and SQLite happens to return tied rows in rowid order for both the LIMIT and the window form today, so they agree without being told to. That is unspecified sorter behaviour, and naming the tiebreak makes the agreement a property of the queries instead of a coincidence. bm25_sum_ms and bm25_max_ms collapse into bm25_ms. The split existed to separate "work done" from "waited for" across N queries; with one query they are the same number, and keeping both would imply a fan-out that no longer happens. The blast radius grew and the tests say so: BM25 used to fail per project, and now one failing query costs every project its sparse signal at once. The fallback is the one a pre-FTS install already lives with — dense-only results, no failed_repos, no 500 — and TestWorkspaceSearch_SurvivesBM25Failure drops chunks_fts outright to prove it. Tests, each mutation-checked against the bug it describes: - SearchProjects matches SearchProject per project, same hits, same order, same scores, over four queries x two limits x four projects on a tie-heavy corpus; - the IN list does not prefix-match (project paths routinely share prefixes: "local:host:/x" vs "local:host:/x/y"); - the map survives the batch boundary (searchProjectsBatch + 7 projects); - BM25 hits stay in their own project end-to-end through the handler; - a total BM25 failure still returns dense results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by clicking the fixture dashboard: "google authentication login
form" took 18.6 s, against a 4.6 s median. The slow-query log said the
whole 18.6 s was the single BM25 statement, so the timings from stage 1
pointed straight at it.
The cause was not the partitioning, it was what the CTE carried. Selecting
file_path, content and the rest inside `hits` 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, because that is
how FTS5 evaluates MATCH. That query matched 263,515 rows to return 2,300.
Ranking on (project_path, rowid, bm) alone and joining chunks_meta and
chunks_fts back for the survivors costs one rowid round-trip per returned
row and nothing per discarded row.
Measured with loadtests/bench/ftsab (the server's own driver, serial, so
the old shape's sum is not hidden by the fan-out's concurrency):
query match set 46 queries payload rank
in CTE first
google authentication login form 263,515 26,085 15,070 2,798
parse JWT token and validate sig. 623,913 27,587 29,566 5,492
rate limiter middleware 166,347 16,904 7,692 993
websocket upgrade handshake 21,049 3,505 1,011 156
graceful shutdown on SIGTERM 14,987 3,244 854 424
The JWT row is the one that matters: with a 624k-row match set the query
as shipped was SLOWER than the 46 per-project queries it replaced. The
gain scaled inversely with the match set — exactly backwards — and the
ten-query bench set hid it because the old shape ran concurrently in the
fan-out while these numbers are serial.
End to end on the fixture, same warm cache, 10 queries, medians:
phase develop prev commit this commit vs develop
wall 10,235 4,650 3,048 3.4x
BM25 111,210 4,375 2,782 40.0x
dense sum 15,166 11,136 11,649 1.3x
dense max 2,318 1,123 1,138 2.0x
And the query that started this: 18,623 ms -> 2,711 ms.
No test guards this. It is a property of the query plan, not of the
result, and the equivalence tests pass against both forms — they did, and
that is the point: correctness tests cannot see this class of bug. What
guards it is loadtests/bench/ftsab, which times all three shapes against
the real corpus, and the comment on the query saying why the obvious form
is wrong.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…invariant N1, the one worth doing: 6a47df7's commit message named loadtests/bench/ftsab as the guard against the payload-in-CTE regression returning. /loadtests/ is gitignored, so that guard exists on one machine and the reference is worse than none in a repo whose commit messages are written for the next agent. The in-repo guard asserts the query PLAN, because that is the only place this bug lives: every equivalence test in the package passes against the slow form too — verified, not assumed — since both forms return the same rows. FTS5 reports a rowid lookup as "0:=" and a MATCH scan as "0:M..."; the rank-first form does both, the payload-in-CTE form only scans. workspaceRankQuery is extracted so the test builds the string production builds rather than a copy free to drift. Its mutation check lives in the tree rather than in a shell history: TestExplainDistinguishesTheTwoQueryShapes builds the slow form and asserts the plan assertion REJECTS it. Without that, a change in how SQLite reports plans could quietly turn the guard into a tautology. Confirmed by mutation: moving the payload back into the CTE fails the plan test while the equivalence tests still pass. Also, and where the reasoning differs from the review: - F2, bm25_ms had no invariant left after sum/max collapsed. Added bm25_ms <= fanout_ms. Stated plainly in the comment is what it does NOT catch: a dropped assignment, since 0 <= fanout holds. "> 0" is not available because an in-memory corpus rounds to 0 ms, and a flaky guard is worse than an honest partial one. - F3, the batch loop hand-copied scanHit's eleven lines and lost the sign-flip comment. Both paths now share scanRankedHit, which takes an optional leading project_path. This mattered more than it looks: the equivalence test compares the two paths against EACH OTHER, so a mistake made symmetrically in both would have passed. - F4, the mutex around bm25ByProject guarded nothing — single writer, readers after g.Wait(). Dropped, with a comment saying why, because a lock that protects nothing reads like protection to whoever next needs those hits inside the fan-out. - F5, denseHits[i] is released as the fusion loop consumes it. Fusion used to free its inputs per goroutine; without this, every project's dense hits, BM25 hits and fused copies stay live at once. - F7, placeholders' n<=0 branch returned "NULL", which matches nothing and is indistinguishable from "nothing matched". Removed: IN () is a syntax error, which is loud and points at the wrong caller. - F9, the searchPhases header still said "the fan-out phases keep a SUM and a MAX" after this PR left only dense with that shape. F6/F8 — collapsing SearchProject into SearchProjects([]string{p}) — NOT done, deliberately. It would make TestSearchProjects_MatchesPerProjectQueries compare a function to itself, and that test is the only independent check that the partitioned ranking matches the known-good per-project one. The per-project BM25 signal feeds project candidacy, so a divergence re-ranks the projects panel with no error and no failed_repos. Structural agreement is worth less here than an oracle. SearchProject's doc comment now says it has no production caller, why it is kept, and not to delete it as unused — which is the real fix for F8. go test ./... green (46 packages), -race green on both changed packages, go vet and gofmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by chasing "why is the answer different each time". Not caused by this branch — main has it too — but it is the larger half of the answer, so it lands here rather than waiting. fuseRRF built its output slice by ranging over a map. Go randomises map iteration deliberately, and sort.SliceStable then preserved that randomness for every pair of chunks with equal RRF. Equal RRF is not a corner case: a chunk found only by dense at rank r and a chunk found only by BM25 at the same rank r score identically by construction, which happens in most queries. The symptom was invisible from the projects panel — project scores do not depend on chunk order — so the panel looked stable while the chunk list underneath it moved. On the fixture, the same query on the same process and binary returned a different chunk at rank 0 between consecutive calls. Sorting by (rrf desc, chunk key asc) gives a total order. Measured on the 43-project fixture, two full 50-query sweeps of one build against itself: chunk lists differing 25/50 -> 5/50 The five that remain are not ours. The provider returns a different vector for a byte-identical request often enough to matter: logging the exact request body alongside a checksum of the vector it produced, over two sweeps, 4 of 50 queries got two distinct vectors from identical bodies (sha of the marshalled request equal, sha of the float32 vector not). When a query drifts it drifts in ALL ten panel projects at once, which is the signature of the query vector moving rather than of any per-collection scan. dense_score shifts by <=0.002 and occasionally flips a rank. Nothing in cix can make that deterministic; a query-embedding cache keyed on the text would, and would cut provider spend too, but that is a separate change with its own trade-offs. The test asserts across 20 repeats, because with N tied entries a single run has a 1/N! chance of looking ordered by accident. It also pins that RRF still dominates the key: a chunk present in both lists outranks single-list chunks whatever its key sorts like. Mutation-checked — restoring the SliceStable-without-tiebreak form fails it on run 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the rounded one Third review pass. N2 is the same defect 202d979 fixed, one level up. The panel sorted on ProjectScore — the round4 copy that ships in the JSON — and then truncated to top_projects on the next line. Rounding manufactures ties: 0.71234 and 0.71236 both become 0.7123, and the tie then decided which of two repos the caller saw AT ALL, resolved by whatever order the projects arrived in, which is workspace membership order (added_at DESC). The panel was a function of insertion history. Now sortPanel orders on the raw candidacy with ProjectPath as the tiebreak, so it is a total order and a function of the query. Extracted into its own function so the property is testable on a constructed slice: through the handler it would depend on added_at timestamps a test cannot control, and that test would be flaky rather than wrong. Two tests, both mutation-checked. The rounding one uses a 5e-6 gap — below round4's resolution — with the input in reverse order and the STRONGER project named last alphabetically, which is what lets it tell "sorted on the raw value" apart from "fell back to the path tiebreak": both alternatives would put "aaa" first. Restoring the rounded sort fails it; removing only the path tiebreak fails the other one. Writing this the obvious way first also re-broke F1: truncating `surviving` in place capped projects_returned at top_projects again, exactly the bug fixed in d511513. TestWorkspaceSearch_ReturnedCountIgnoresThePanelCap caught it immediately. `panel` now reslices instead, and the comment says why. That also removed the set-membership filter that rebuilt the panel for the interleave — `panel` already is that set, in that order. N3: fuseRRF's tiebreak called key() on both sides of every comparison, rebuilding a four-part concatenation that had already been computed as the map key and thrown away. At ~100 chunks per project that is tens of thousands of throwaway strings per query, on the path this branch exists to speed up. The key is now carried on the entry. The expression itself also existed twice — fuseRRF's `key` and interleaveByRank's `dedupKey` — and since 202d979 it decides ORDER, not just identity, so the two drifting apart would make fusion and dedup disagree with no error. One package-level chunkKey now. N4: scanRankedHit built an 8-element []any and then prepended to it, allocating and copying a second slice for every row of the workspace query — ~2,150 rows on the fixture. Built once with the right capacity. Also corrects a comment I wrote in c495f0b: denseHits[i] = nil does not avoid keeping "three sets of chunk payloads for the whole workspace" alive. fuseRRF returns the UNION of both lists, so the Content strings stay reachable through results[i].FusedChunks either way. What it frees is ~50 payload structs per project — a few KB, not the chunk text. The line is still right; the claim was inflated. go test ./... green (46 packages), -race green on both changed packages, make openapi-check in sync, go vet and gofmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…under it Fourth review pass, comment only. "the projects panel sees every surviving project" was written when payloads were built for everything and truncated afterwards. 7e03041 moved the truncation above the loop, so the sentence now describes the opposite of the code three lines below it. Worth a commit of its own because of WHICH reader it misleads: this is the comment someone consults when reasoning about projects_in_panel versus projects_returned, and telling them those are the same number is exactly how d511513 gets re-broken. It has already been re-broken once, while refactoring these very lines — so the replacement says so, and says which of the two must never be capped. The num_hits half of the original sentence was correct and is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
perf(chunksfts): rank the whole workspace in one FTS5 query
… 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>
perf(chunksfts): rank the workspace with a bounded heap, not a window function
…e comments TWO UNRELATED THINGS, BOTH SMALL, BOTH USER-VISIBLE OR READER-VISIBLE. SEARCH FIRED WHILE TYPING /search debounced the input into the URL after 250ms idle, and a change to the URL is what runs a search. That is the usual pattern and it is wrong here: a semantic search embeds the query through the configured provider, so every pause while typing spent a real API call and a full fan-out to answer a half-written question. "retry with exponential backoff" typed at a normal pace fires on "retry", "retry with", "retry with expo" — three searches nobody asked for and one they did. On a metered provider that is money; on a local sidecar it is a queue of pointless work in front of the query the user meant. Typing now changes local state and nothing else. The URL — and therefore the search — moves only on submit. SearchBar already had the onSubmit path; only the debounce had to go. The empty state says "press Enter to search" instead of implying results appear on their own. Verified through the real component in devmock, which boots the app with a mock fetch and no login: typing 31 characters one at a time issues ZERO search requests, and submitting issues exactly one, with no navigation. A note on how that was verified, because the first attempt was worthless: driving Enter through the browser-automation key API produced a page "reload" that looked like a regression. It was not — a keydown listener on the input recorded NOTHING, so those key events never reached the page and that test asserted nothing at all. The real check goes through form.requestSubmit(), which is the exact path a keypress takes. THREE STALE COMMENTS IN workspacesearch.go All three are from #265, all three describe code that commit changed: - projectHits' doc said the two sides "are fused inside the goroutine". They are not — fuseRRF runs in the serial loop after g.Wait(), and the comment above that loop says so in as many words. The struct doc contradicted a comment 650 lines below it. - the handler doc said "each project runs two queries in parallel: dense and sparse". There is one BM25 query for the whole workspace now, which is what #265 was. - BM25Signal's doc explained its normalization but never said it is computed on the RAW, unfused list while FusedChunks beside it is post-RRF. That asymmetry decides whether a panel reorder means what it appears to mean, and the one place a reader would look for it did not mention it. Same class as 7cc70a2 and as two commits in #266: the code moved and the comment above it did not. go test ./... green, go vet clean, gofmt clean on the touched files. Dashboard built with `npm run build` (tsc -b + vite); dashboard build is not on PR CI, so it was validated locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix(dashboard): search on Enter, not while typing; repoint three stale comments
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotion of
develop→mainfor the server/v0.14.0 release. Tags are cut onmain, so this is the gate.Why 0.14.0 and not 0.13.1
vectors_q8is a new on-disk table with a background backfill, and the workspace BM25 path was rewritten. Behaviour is identical and the upgrade is safe by construction — a collection with noq8_staterow stays on the float32 scan until the backfill completes and records it — but a store-format addition is not a patch release.What is in it
Search performance — the headline. Workspace search on real data is 2.7–4.2x faster.
write a unit test for the parserwhere is the HTTP router registeredparse JWT token and validate signaturegraceful shutdown on SIGTERM?timings=trueon the response. This is what showed the next two were aimed correctly.Search output is now reproducible.
fuseRRFranged over a map with no tiebreak and the projects panel sorted on a rounded score — both pre-existing, both fixed in #265. Measured on live data: the old build returns up to 3 different orderings in 6 runs of the same query; the new one returns one, every time.Also in: #260 (in-place repo compaction — the 76 GB git bloat fix), #262 (exact token counting via the model's own BPE tokenizer), plus site, macOS and docs work (#250–#258).
Verification
Beyond CI, the develop build was run against a clone of a real 48-project, 392k-chunk install with local (deterministic) embeddings, alongside the current release serving the same data:
vectors.dbgrew 2.2 → 2.52 GB (+15%);ERRORlines through boot, backfill and query.Security: what this PR DOES check, and what it does not
Runs on this PR (the Security workflow fires on
pull_request: branches: [main], which is what this is):govulncheckon the server module,govulncheckon the CLI, and Trivy at HIGH,CRITICAL — butscan-type: fs, a filesystem scan of the repo. That covers our Go dependencies and anything checked in.Not run: the image scan. The GPU host that builds and scans the CUDA image is unreachable, so the documented gate — no NEW HIGH/CRITICAL versus the prod tag — was skipped. Trivy
fsdoes not see it: OS packages in the runtime layer and thecurl-edcloudflared/ngrokbinaries are invisible to both it and Dependabot. That blindspot is on record (#194) and it is exactly the surface left unverified here.Bounding the risk honestly:
server/go.modis unchanged sinceserver/v0.13.0, andgovulncheckon this PR covers it anyway.server/DockerfileandDockerfile.cudaare unchanged, socloudflared(2026.7.3),ngrokand the digest-pinnedllama.cpplayers are the same content the v0.13.0 image shipped — and that image passed a gate.gcr.io/distroless/cc-debian13:nonrootandnvidia/cuda:12.8.1-base-ubuntu24.04are tags, not digests, so this build pulls whatever they point at today. Distroless rebuilds normally move forward onto patches, which makes a rebuild more likely to reduce CVEs than add them — but that is a tendency, not a guarantee, and it is precisely what the scan exists to confirm.cloudflaredis2026.7.3against2026.8.2upstream. Deliberately not bumped: bumping without a scan trades a surface that was verified for one that is not. Bump it and run the full gate when the host is back — that is the first thing to do, not an optional follow-up.After merge
git tag -a server/v0.14.0 <main-sha> && git push origin server/v0.14.0release-server.ymlbuilds CPU multi-arch + CUDA, pushes:v0.14.0,:latest,:v0.14.0-cu128,:cu128, and runs the Hub prune.SERVER_VERSIONinsite/src/shared/versions.json develop → PR to main.:v0.14.0-cu128before deploying, and bumpcloudflared.