fix(skip-flags): --skip-upload must reuse the corpus, not destroy it (#238)

[fcostaoliveira]

The bug

--skip-upload exists to reuse an already-populated corpus. Two separate paths destroyed it instead.

Path 1 — configure(). With --skip-vector-index also set, experiment.rs still routed through Engine::configure(), destructive on 14 of the 15 engines (only Vertex is not). The flag pair meaning "do not upload, do not build an index, just use what is there" deleted the corpus, then benchmarked the empty index, printed a QPS number and exited 0.

Path 2 — engine.delete(). Cleanup runs at the end of every successful experiment unless --keep-data, which defaults to false. So the plainest form of the flag reused a corpus, measured it, and then deleted it. Found in review; both original regression tests passed --keep-data, so neither covered it.

Empirical matrix (established live)

--skip-vector-index is wired for redis / valkey / mongodb only (experiment.rs:274); every other engine's config is skipped with a warning before configure() is reached, so path 1 is reachable on exactly those three — and all three destroy. Path 2 applies to every engine.

engine	path 1 (--skip-upload --skip-vector-index)	path 2 (--skip-upload, no --keep-data)
redis	DESTROYS — FT.DROPINDEX … DD	DESTROYS — DBSIZE 400 → 0
valkey	DESTROYS — FT.DROPINDEX + SCAN/UNLINK	DESTROYS (same delete())
mongodb	DESTROYS — collection.drop(), unbounded: unlike redis/valkey's namespace-scoped DD/UNLINK it removes the whole benchmark collection	DESTROYS
qdrant	not reached (config skipped)	DESTROYS — points_count 100 → collection MISSING
vectorsets	not reached	DESTROYS — VCARD idx 100 → 0
chroma, es, opensearch, pgvector, weaviate, milvus, turbopuffer, kividb, dragonfly	not reached (configure() is destructive for all of them anyway)	DESTROYS
vertex	not reached	non-destructive configure(); delete() guarded by reuse_index_ids()

A third shape survives and the new guard is what catches it: if phase 1 was a normal upload, phase 2 destroys nothing — it publishes a QPS number from an empty <engine>-no-vector index while the real corpus sits untouched under the original config name.

Semantics

--skip-upload = "the server already holds the corpus I want; do not create, drop, recreate or otherwise modify it."

1. configure() never runs on that path, in any flag combination. The removed arm was unnecessary anyway: --skip-vector-index rewrites the config name to <engine>-no-vector, so the prior --skip-vector-index --keep-data upload already left the schema-only index the second phase needs.

2. Cleanup never runs either. keep_data is forced on for --skip-upload.

3. The corpus is verified before anything is measured. Engine::corpus_row_count() reads the count off the live server and the runner compares it with the corpus measured on disk:

   • fewer rows than the corpus holds, including zero → hard error;
   • more → warning;
   • an estimate that comes up short → warning only (see pgvector below);
   • a probe failure → a hard error that says probe failure, never "the corpus is empty";
   • no count implemented → a printed note that the reuse went unverified.

   Implemented for redis, valkey, dragonfly, kividb, vectorsets, qdrant, elasticsearch, opensearch, pgvector, mongodb. Chroma, Milvus and Weaviate all expose a count we have not wired up (Wire corpus_row_count() for Chroma, Milvus and Weaviate #285); only Turbopuffer and Vertex are genuinely uncountable.

Proof — server-side row counts, both binaries

Binaries confirmed distinct by a strings probe for a symbol added here (did not create this corpus: absent in pre, present in post). Digests are recorded but are not the load-bearing evidence — a debug binary embeds its build path, so they cannot falsify anything on their own.

Path 2, three engines with different code paths (pre = 0a1bb3f, this branch's first commit; post = HEAD):

                              BEFORE            AFTER (pre)       AFTER (post)
qdrant      points_count        100          collection MISSING       100
vectorsets  VCARD idx           100                 0                 100
redis       DBSIZE              400                 0                 400

All three pre-fix runs printed Cleanup (deleting index and data) and exited 0; the qdrant one printed Reuse check — server holds 100 of 100 expected rows first, then deleted the collection it had just certified. Post-fix all three print Keep data (--skip-upload did not create this corpus, so it does not delete it).

Path 1, vs master (6f8b814) — redis FT.INFO num_docs 400 → 0 while printing QPS: 825.2; mongodb countDocuments 400 → 0 while printing 118.2/38.6 QPS; valkey DBSIZE 400 → 0. All 400 → 400 on this branch.

Probe failures no longer fabricate an empty corpus (corpus intact in all three):

redis   FT.INFO ACL-denied, real num_docs=400
        → "could not read the server-side corpus size … (NoPerm: User lim has no
           permissions to run the 'FT.INFO' command). This is a PROBE failure, not a
           verdict on the corpus — the data may well be intact."   corpus after: 400
qdrant  dead gRPC port, real points_count=100  → same shape, names the connect error
mongo   dead port                              → same shape, names the topology error

A wrong vector_count no longer disables the guard (server holding 0 of 100, datasets.json under-declaring 50):

pre    Reuse check — SKIPPED (dataset does not declare … a corpus size)
       → QPS: 3171.7, Recall: 0.0000    exit 0, result file written
post   WARNING: dataset 'd238' declares vector_count 50 but its corpus on disk holds 100.
       The --skip-upload reuse check uses the MEASURED 100; fix vector_count …
       Experiment failed: … holds 0 of the 100 rows …    exit 1, no file

The verdict now reaches the artifact (2-config sweep, e-m16 amputated to 50, e-m32 intact at 100):

run	rc	search files	outcome
default (--exit-on-error true)	1	0	m32's correct measurement is lost too
--exit-on-error false	0	1	m32 recall 1.0 published; summary rejected_experiments names e-m16 — the remedy the error message and README now teach
--allow-partial-corpus	0	2	m16's wrong 0.42 published, tagged waived_by_allow_partial_corpus: true

Every result file carries params.corpus_reuse — e.g. {"status":"verified","expected_rows":100,"actual_rows":100,"actual_is_estimate":false,"waived_by_allow_partial_corpus":false}.

Engine state that configure() used to set

Making "configure never runs here" a documented contract required auditing what depends on it:

• pgvector — distance_op was empty on this path, splicing as embedding $1 and failing every query after a green reuse check. Now re-derived in search() (pure function of the dataset). hnsw_ops_class is not read in search — that half of the reported claim did not hold.
• turbopuffer — distance_metric defaulted to cosine_distance: an l2/dot dataset measured under the wrong metric, no error, plausible file. Re-derived.
• milvus — metric_type empty. Re-derived.
• chroma — collection_id comes from a server response, so it is not derivable; now resolved by name.
• All five Redis-wire engines — commandstats_baseline: None means "RESETSTAT ran, counters are zero". With configure() skipped, nothing reset them, so check_commandstats compared this run against zero while the counters held every failure since server start — a hard error on a run in which nothing failed. A commandstats_primed flag disambiguates and search() primes exactly once. (This also restores the CONFIG RESETSTAT that master's --skip-vector-index path performed.)
• vertex — already self-heals in search(); untouched. The claim that it is inoperable did not hold.
• mongodb — load_schema_types in search() is kept as defensive symmetry with redis/valkey, and the comment now says so. It is not load-bearing for a pure search: the filter builders type literals off the JSON value and never read the cache.

pgvector's count is an estimate on purpose — measured, not asserted

The original finding was measurement integrity: count(*) primed the page cache immediately before the search phase, converting a cold-cache run into a warm one. "Estimates only warn" addresses the abort behaviour and not that, so here is the cache half, measured on a cold 1M-row / 768-dim table built exactly like this engine's (SERIAL PK + vector NOT NULL, STORAGE PLAIN, never VACUUMed; 3906 MB, 500000 relpages), postgres restarted between runs:

	heap blocks touched	cache primed	cold wall clock
SELECT count(*) FROM items	500,000	3906 MB (the whole heap)	1.58 s
ANALYZE items	30,001	234 MB	0.84 s

EXPLAIN (ANALYZE, BUFFERS) confirms the plan: Parallel Seq Scan on items … Buffers: shared hit=96 read=499904.

Is ANALYZE cheap on a 1M-row table? Yes, and — more importantly — it is bounded. ANALYZE samples 300 × default_statistics_target = 30,000 rows regardless of table size, so its footprint is constant while count(*) grows linearly with the corpus. At 1536 dims (~7.8 GB) count(*) primes all 7.8 GB; ANALYZE still touches ~30,000 blocks.

But bounded is not free, and the body should not claim otherwise: ANALYZE still pulls ~234 MB — about 6% of this heap — into cache before the search phase, and it writes statistics. The perturbation is capped, not eliminated. That is stated in the shipped code comment and the README rather than left as a favourable silence.

Live behaviour: Reuse check — server holds ≈100 of 100 expected rows, then a clean search (which failed on every query before the distance_op fix).

Merged with #263's start gate, and exercised rather than assumed

This branch is merged (not rebased — it is published, and rebasing it once already produced a non-fast-forward that must never be resolved by force-push) with master 01145d4. #263 replaced the fixed-count Barrier in all 14 engine search harnesses with start_gate, touching the same worker-spawn and error-propagation paths this branch adds corpus_row_count() and a pre-search hard error to. The merge was textually conflict-free — which is exactly the case where a clean merge that does not compile is the risk — so it was verified, not assumed:

• Both sides survive and compile. src/start_gate.rs 994 lines, 14 engines routing through it, 11 corpus_row_count impls, 8 prime_commandstats_if_needed call sites, the keep_data forcing intact. cargo build --bins --tests clean.
• The reuse error still reaches exit(1). fix(harness): thread-spawn failure and worker panics deadlocked the search start barrier #263 converted an in-search return Err into fatal_search_error + break 'search_phase so pending_saves is flushed first. My guard fires before the search phase and returns through ? directly, so that restructuring does not intercept it. Live on the merged binary, against a half-deleted Qdrant collection: EXIT CODE = 1.
• No partial or empty artifact for --plot to chart. Same run: 0 search result files, 0 summary files, 0 chart lines, and fix(harness): thread-spawn failure and worker panics deadlocked the search start barrier #263's wrote N completed search point(s) message correctly does not fire (pending_saves is empty because nothing had run yet).
• The complement works too — guard passes → fix(harness): thread-spawn failure and worker panics deadlocked the search start barrier #263's gate runs a real search → Reuse check — server holds 100 of 100 expected rows, three reps, and with no --keep-data the collection still holds 100 afterwards, with params.corpus_reuse {"status":"verified", …} in the artifact.
• INV-4 / INV-4b / INV-4c pass. In all 8 search entry points across the five Redis-wire engines, prime_commandstats_if_needed()? sits immediately after the signature — before every dispatch and before every WorkerPool::new — so its early return cannot strand a gate (INV-4c's concern). No Barrier was added.

Tests

8 live integration tests across 5 engines, all verified RED. Against master (6f8b814):

valkey   --skip-upload --skip-vector-index unlinked the corpus it was told to reuse
         (400 -> 0 keys) — issue #238
mongodb  --skip-upload --skip-vector-index dropped the collection it was told to
         reuse (400 -> 0 docs) — issue #238
redis    destroyed the indexed corpus (400 -> 0 docs)  [first commit]

Against this branch's first commit 0a1bb3f (i.e. the review round's own regressions):

redis    --skip-upload without --keep-data deleted the corpus it reused  left: -1  right: 400
redis    recorded verdict: null   left: Null  right: "short"     (nothing reached the artifact)
redis    recorded verdict: null   left: Null  right: "surplus"
qdrant   --skip-upload without --keep-data deleted the collection it reused  left: 0  right: 400
kividb   KiviDB must report a count (hnsw_live_count), not degrade to a note

Mutation probe (both directions). With the guard mutated to fire only for configs whose name contains redis:

config named cfg238short   → test FAILED   (mutant killed)
config named redis-238-short → test ok     (mutant survives)

Engine::name() returns the config name, so the rename is what makes the test discriminating.

A note on KiviDB: its HNSW graph is independent of the hashes — UNLINKing 45 of 100 documents leaves hnsw_live_count at 100 and recall at 1.0000 (measured), so the count is right and there is nothing to catch. The test amputates with FT.DROPINDEX instead, which is what actually removes the searchable corpus.

Gates, on the tree merged with current master 01145d4 (#263's start gate):

• cargo test --lib --bins: 872 passed, 0 failed across all six binaries (126 + 0 + 0 + 0 + 2 + 744). Master baseline is 860; +12 is the new unit tests.
• cargo clippy --all-targets -- -D warnings and cargo fmt --check clean.
• tests/overhead_invariants.rs 9/9, including inv4_no_fixed_count_start_barrier, inv4_search_harnesses_route_through_the_start_gate and inv4_bare_start_gate_users_hold_an_abort_guard. integration_cli 4/4.
• Suites, on dedicated containers: integration_redis 38/38, integration_valkey 26/26, integration_qdrant 27/27, integration_mongodb 19/19, integration_kividb 20/20 (test_kividb_flat_algorithm_works filtered — it hangs >40 min on this box, uses no --skip-upload, and hangs on master too).
• integration_redis and integration_qdrant gain the REDIS_TEST_PORT / QDRANT_TEST_{REST,GRPC}_PORT overrides the valkey/kividb/mongodb suites already had, so a session can test its own container rather than flushing a shared one.

Correction to my earlier body: I previously reported integration_opensearch failing 16/16 and attributed it to container contention. The suite passes 16/16 on master and on this branch; the failure was disk pressure — the shared cluster had crossed the 90% watermark, so index creation returned index_create_block_exception: blocked by [FORBIDDEN/10/cluster create-index blocked (api)], tripping assert!(resp.status().is_success()) everywhere. Not this change, but the stated cause was wrong. Also corrected: the claim that Valkey Search rejects a vector-less FT.CREATE — it does not on the image docker-compose.test.yml pins (valkey 9.1.1); the real result is stronger, master destroys 400 keys while printing a QPS line and exiting 0.

On recall as a detector: the mean_recall: 1.0 I quoted for a half-deleted Qdrant collection is deletion-pattern-dependent, not a property. random-100 has 9 queries with one ground-truth neighbour each in ids 0-8, so deleting the upper half removes no ground-truth point → 1.0, and deleting the lower half → 0.0. Recall is a coin flip on which rows went; a server-side count is not. The qualification is now in the code comments and the test doc, and the new Qdrant test deletes the upper half deliberately.

Follow-ups filed, deliberately not fixed here

#279 (cardinality is not identity — a sibling config's corpus certifies as this one's on the 6 single-object engines), #280 (--skip-if-exists is a no-op without --skip-upload), #281 (--reset-between-configs on a run that never uploads), #282 (--update-search-ratio mutates the reused corpus by design), #283 (--skip-vector-index collapses every config into <type>-no-vector, defeating #151-4; the collision guard runs on pre-rewrite names), #284 (ES/OS _count is refresh-scoped), #285 (wire the count for Chroma/Milvus/Weaviate).

Also out of scope: #251 filter builders, #246 force-merge, #263 harness start gate, #264 results-JSON config recording (the reuse verdict should join its provenance block once both land); Valkey Search's own --skip-vector-index behaviour; the hardcoded VectorSets idx key (#236).

One new per-experiment cost, only on the --skip-upload path: measuring the corpus is a header read for npy/hdf5, but a full-file line count for the two jsonl datasets — cheap at their size, noted in the code.

Closes #238

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

[github-actions]

Docker Build Validation

Platforms tested:

• linux/amd64 (built and tested)
• linux/arm64 (build validated)

Tests performed:

• --help command
• --describe datasets command
• Integration test (random-100 dataset + Redis)
• Multi-platform build validation (amd64 + arm64)

[github-actions]

Docker Build Validation

Platforms tested:

• linux/amd64 (built and tested)
• linux/arm64 (build validated)

Tests performed:

• --help command
• --describe datasets command
• Integration test (random-100 dataset + Redis)
• Multi-platform build validation (amd64 + arm64)

[github-actions]

Docker Build Validation

Platforms tested:

• linux/amd64 (built and tested)
• linux/arm64 (build validated)

Tests performed:

• --help command
• --describe datasets command
• Integration test (random-100 dataset + Redis)
• Multi-platform build validation (amd64 + arm64)

[github-actions]

Docker Build Validation

Platforms tested:

• linux/amd64 (built and tested)
• linux/arm64 (build validated)

Tests performed:

• --help command
• --describe datasets command
• Integration test (random-100 dataset + Redis)
• Multi-platform build validation (amd64 + arm64)