perf(test): cut round-trips in HNSW vector-index integration tests#1523
Merged
Conversation
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi,
heskew and
kriszyp
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request optimizes the HNSW vector-index data-integrity integration tests by consolidating the schema deployment and worker restarts into a single shared setup, and parallelizing read-only vector searches. The review feedback recommends limiting the concurrency of these parallelized HTTP requests by processing them in smaller chunks to prevent potential socket exhaustion or test flakiness on slow CI environments.
Contributor
|
Reviewed; no blockers found. |
dawsontoth
force-pushed
the
claude/perf-vector-tests-main
branch
from
39c3a2e to
efad325
Compare
The vector-index-integrity suite is a major Windows CI hotspot (~29 min for the suite). Most of the cost is ~100–200 sequential, individually-committed HTTP round-trips per test — tiny data (DIMS=8, N≤50), so it's round-trip + per-write fsync overhead, not vector math. Apply correctness-neutral reductions: - reindex backfill: bulk-insert the N pre-index seed records in one request instead of N sequential POSTs. There is no HNSW index during insert, so per-record ordering is irrelevant; the backfill builds the index afterward. - update-churn & reindex: run the independent read-back verification searches concurrently (Promise.all) instead of serializing N round-trips. Left untouched (semantically order-sensitive): the entry-point insert/delete sequence and the churn update sequence. Note: validated on CI, not locally — restart_service is flaky in a local multi-loopback setup (readiness probes time out at 60s), which both fails and dominates local timings; the churn test (the one that runs cleanly locally) still passes with the parallelized read-back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each test in this suite deployed its own component and restarted the HTTP workers; the restart is the dominant per-test cost on slow runners (a test doing 3 inserts still took ~265s on Windows — almost entirely restart/setup). Deploy all four tables in one component with a single restart in `before`, and drop the per-test add_component/set_component_file/restart. Restarts: ~5 -> 2 (the shared one, plus the single restart `reindex` needs for its mid-test index-add); add_component: 4 -> 1. Tables keep distinct names/databases so the tests stay isolated despite sharing one component. ReindexTable is deployed without an index initially; the reindex test redeploys the full schema with it indexed (a no-op re-assert for the other three, which have already run — reindex is last) to trigger backfill. CI-validated (local restart_service is flaky in a multi-loopback setup). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…il workers settle
The reindex test restarts HTTP workers to add the HNSW index, then backfill
settles per-worker. The single-shot readiness poll confirmed only one worker was
ready, so firing all N read-back searches at once fanned them across workers —
some still reloading the index-adding schema ('embedding is not indexed') —
producing spurious misses (saw 7/40 > 4 allowed on Linux v24 5/6).
- Add mapConcurrent(): bounded-concurrency fan-out (cap 10 in flight) so a large
burst can't exhaust sockets on slow runners, while staying parallel.
- reindex: collapse the readiness gate + recall assertion into one converging
poll — retry the full bounded read-back until misses <= allowed (or 60s), so
per-worker backfill settling can't cause a spurious failure. A real recall
regression still never converges and fails.
- update-churn: use mapConcurrent for the read-back (no behavior change; bounds
the fan-out per review feedback).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/perf-vector-tests-main
branch
from
efad325 to
dea55fb
Compare
kriszyp
approved these changes
Jun 29, 2026
kriszyp
left a comment
kriszyp
left a comment
Member
There was a problem hiding this comment.
Approving — test-only change cutting round-trips in the HNSW vector-index integration tests; no assertions weakened and build/search are still genuinely exercised. CI green. Thanks @dawsontoth!
— 🤖 KrAIs (Kris's review assistant)
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.
Splits the HNSW vector-index test perf work out of #1505 (which merged into a stacked branch) into its own PR targeting
main.Why
integrationTests/server/vector-index-integrity.test.tsis a top Windows CI hotspot — theHNSW vector-index data-integritysuite ran ~29 min on Windows 5/6 (1763s). The data is tiny (DIMS=8, N≤50); the cost is per-test worker restarts plus ~100–200 sequential, individually-committed HTTP round-trips per test — round-trip + per-write fsync overhead, not vector math.Measured result (CI, Windows shard 5/6)
All four vector tests pass on v24/v26/Windows. The dominant per-test cost was the per-test worker restart; the residual ~422s is the single shared
beforedeploy+restart.Changes (correctness-neutral)
before, dropping the per-testadd_component/set_component_file/restart. Restarts: ~5 → 2;add_component: 4 → 1. Tables keep distinct names/databases so tests stay isolated.insertop. There's no HNSW index during these inserts (the index is added afterward), so per-record ordering is irrelevant; the backfill builds the index from the stored rows.update-churnandreindexverification loops run their N independent QUERY searches withPromise.allinstead of serializing.Deliberately left sequential (order-sensitive): the delete-entry-point insert/delete sequence and the churn update sequence.
Validation
CI-validated, not locally measured —
restart_serviceis flaky in a local multi-loopback setup (per-test readiness probes time out at 60s, an artifact that doesn't exist on CI where these tests pass). Theupdate-churntest — the one that runs cleanly locally — still passes with the parallelized read-back.🤖 Generated with Claude Code