Skip to content

fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other - #278

Merged
fcostaoliveira merged 4 commits into
masterfrom
fix/vectorsets-index-namespacing-236
Aug 8, 2026
Merged

fix(vectorsets): derive the vector-set key per config so concurrent runs stop clobbering each other#278
fcostaoliveira merged 4 commits into
masterfrom
fix/vectorsets-index-namespacing-236

Conversation

@fcostaoliveira

@fcostaoliveira fcostaoliveira commented Aug 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

The bug

vectorsets.rs hardcoded the key idx at every site. It never got the #151-4 /
PR #153 per-config namespacing that redis / valkey / dragonfly / kividb have, and
because there was no derived name, there was nothing for the startup collision
guard to detect either.

This is worse than the Redis-family version. VectorSets' whole corpus is one
key, and configure() opened with DEL idx — so starting config B did not
interleave with config A's data, it deleted config A's corpus outright and
rebuilt the same key with its own.

Scope of the fix, precisely. derive_index_name keys off engine_config.name
alone, so this fixes the two-configs case. One config across two datasets,
and two concurrent runs of the same config, still share a key — identical to the
Redis family's design, so not a regression, but not covered either.

And even for two configs it is data isolation, not measurement isolation:
configure() calls server-global CONFIG RESETSTAT, so two concurrent runs no
longer destroy each other's corpus but do still corrupt each other's commandstats
baseline — blinding the failed-call guard that catches a silently-failing
VADD/VSIM. Concurrency was impossible before this PR and is newly reachable, so
the caveat is newly relevant; it is documented at the derivation site. The
used_memory half of that problem is fixed below.

The fix

key: derive_index_name("VECTORSETS_INDEX_NAME", "idx", &engine_config.name)

Same helper, same <base>:<sanitized-config-name> shape, same
VECTORSETS_INDEX_NAME_EXACT escape hatch, same defaulting as the four siblings.
No derive_key_prefix analogue — a vector set is a single key, not an index over
a keyspace of doc hashes, so this name is the namespace.

All eleven sites that named the key moved, since a half-migration is worse than
none: configure (DEL), upload→vadd_batch (VADD), search (VSIM, query
and the priming query), search_mixed (VSIM + vadd_single's VADD),
delete (DEL), get_memory_usage + server_metadata (VINFO), and
corpus_row_count (VCARD). vectorsets.rs no longer contains the string "idx"
anywhere except the default-base argument.

What the test suite actually guards, which is less than eleven. A mutation
campaign over this branch points each site back at idx one at a time. Five
sites are killed; six have no guard at all — vadd_single, delete,
configure's DEL, and both VINFO sites (the priming VSIM is a legitimate
equivalent mutant, its result is let _ =-discarded). Two of those six are
demonstrably silent-wrong when mutated: pointing vadd_single at idx leaves
VCARD idx = 399 while the searched corpus receives zero updates and
mixed_benchmark stays green; pointing delete at idx leaks all 7 per-config
keys with the suite still 12/12. And search/search_mixed are killed only
by pre-existing recall tests, not by anything this PR added. The fix is correct
at all eleven sites — this is a statement about coverage, not about the code.
The vadd_single case is a pre-existing metric-validity bug (update_count
counts client-side attempts, never the server's acceptance into the searched
corpus); filed as #293, not fixed here.

experiment::run's collision guard now maps "vectorsets" => "VECTORSETS_INDEX_NAME".

Two things the merge with #271 forced, and one it created

corpus_row_count — a silent-wrong born from a clean merge. #271 landed
Engine::corpus_row_count() for vectorsets as VCARD idx, hardcoded, with a doc
comment reading "The key is the hardcoded idx (issue #236)" — written before
this PR existed. Git conflicts only on the import line. Resolved naively you get
writes to idx:<config> and a reuse probe reading idx. Reproduced live on the
merge, seeding the legacy key with exactly the expected 400 rows:

Experiment stage: Reuse check — server holds 400 of 400 expected rows
→ QPS: 1924.5, Recall: 0.0000, Precision@returned: 0.0000, MRR: 0.0000
exit 0

Fixed in the merge commit, with a comment naming the hazard. #271 shipped
corpus_row_count tests for redis/kividb/mongodb/qdrant/valkey but not
vectorsets, which is why CI could not see this — that test is added here (VREM
half the corpus, assert holds 200 of the 400 rows).

get_memory_usage — a metric that was right, made wrong by coexistence. It
returns server-wide used_memory. That was correct per-config before this PR:
the hardcoded key plus the destructive DEL guaranteed exactly one resident
corpus. Now every config after the first reports the sum. VINFO has no memory
field, but the corpus is one key, so MEMORY USAGE <key> attributes it exactly —
published as index_memory_bytes, the same field the four FT engines already use.

A alone:            index_memory_bytes=287,740  used_memory=1,985,664
B, with A resident: index_memory_bytes=287,740  used_memory=2,319,864
MEMORY USAGE idx:vectorsets-mem-a=287,740  idx:vectorsets-mem-b=287,740

Joining the family's guard — nothing new added, because #271 already gives it.
VectorSets has no ensure_index_exists analogue, and VSIM on a missing key
returns an empty array with rc=0, so failed_queries stays 0 and the empty search
returns fast. Measured on this tree (400 vectors, ef=400 parallel=1, same
server, key deleted then --skip-upload --allow-partial-corpus):

FULL   VCARD=400 rps=637.55  recall=1.0  failed_queries=0
EMPTY  VCARD=0   rps=2679.26 recall=0.0  failed_queries=0

4.2× faster for measuring nothing. With corpus_row_count reading the derived
key, --skip-upload now routes through check_corpus_reuse_precondition →
Short → hard error, so that run is rejected instead of published. A
VCARD-in-search gate would be a second, redundant mechanism on the same path.

The guarantee is conditional, and the README now says so. When the expected row
count cannot be determined (sparse/h5-multi layouts, unresolvable dataset path),
the verdict is Unverifiable — prints SKIPPED, records
params.corpus_reuse.status = "unverified", and lets the run proceed. Inherited
from #238/#271, filed separately as #290; scoped here, not fixed here.

Migration

An idx key from a pre-#236 binary is not found and not deleted. Accepted and
now loud: the reuse check rejects the run rather than measuring nothing. DEL idx
to reclaim the memory and re-upload. Note it also keeps inflating used_memory in
every later result file until deleted.

Verification — server-side, not recall

Two configs back-to-back on one live redis:8.8.0. All assertions are
VCARD/VDIM/EXISTS read off the server; a recall assertion cannot catch this,
because two same-shaped corpora produce a plausible recall whichever one answered.

RED on master (test file only, src/ reverted):

#236 read-back after config A: VCARD idx:vectorsets-iso-a=0 (VDIM 0) | legacy VCARD idx=400 (VDIM 8)
#236 read-back after config B: VCARD idx:vectorsets-iso-a=0 (VDIM 0), VCARD idx:vectorsets-iso-b=0 (VDIM 0) | legacy VCARD idx=400 (VDIM 16)

panicked: assertion `left == right` failed: config A must upload into its own key 'idx:vectorsets-iso-a' (#236)
  left: 0
 right: 400

One key. After A it holds 400 8-d vectors; after B the same key holds 400 16-d
vectors. A's corpus is gone.

GREEN on this branch (verbatim):

#236 read-back after config A: VCARD idx:vectorsets-iso-a=400 (VDIM 8) | legacy VCARD idx=0 (VDIM 0)
#236 read-back after config B: VCARD idx:vectorsets-iso-a=400 (VDIM 8), VCARD idx:vectorsets-iso-b=400 (VDIM 16) | legacy VCARD idx=0 (VDIM 0)

(EXISTS idx == 0 is asserted separately.)

The load-bearing assertion is card_a_after_a == n_a — config A's count
snapshotted before B runs. The two fixtures upload at different
dimensionalities (8 and 16) and the VDIM assertions check them, but that is
belt-and-braces, not what makes the test work: rebuilding both fixtures at dim 8
and relaxing both VDIM assertions leaves master RED with an identical
left: 0 / right: 400. Earlier revisions of this description claimed the differing
dims were what made a surviving key attributable; they are not.

Five new integration tests plus two index_naming unit tests. Where the teeth
actually are, since an earlier version of this body got it wrong: the migration
test runs over a private base, so its exact-count seeding is not what catches a
wrong-key probe. Counterfactual with corpus_row_count patched back to
.arg("idx"), live:

test_vectorsets_corpus_row_count_tracks_the_live_key        ... FAILED  (holds 200 of the 400 rows)
test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus ... FAILED  (the POSITIVE CONTROL)
10 passed; 2 failed

Both catch it; the seeded-negative half does not. Corrected in the test's doc
comment.

Two of the new tests are the unique killer of their mutant:
exact_pin_drops_the_config_suffix for removing the _EXACT hatch (which had zero
coverage anywhere in the repo before this PR), and the startup-rejection test for
deleting the "vectorsets" => "VECTORSETS_INDEX_NAME" guard row. The memory floor
is the sole assertion that kills index_memory_bytes = used_memory, panicking with
server-wide used_memory (2312344) should dwarf this config's own corpus (2312344). Honest counterweights: the 10× scaling assertion added in the last
revision kills no mutant the suite did not already kill — the constant-1 case dies
one assertion earlier — so it is defence-in-depth against a server-side property
that cannot be expressed as a source mutation, not a new killer; and
vectorsets_configs_derive_distinct_keys is redundant with the pre-existing
derive_index_name_appends_sanitized_config and uses a fake env name, so the
literal format!("idx:{name}") in the integration tests is what actually pins the
wire key.

Side effect: the integration suite is parallel-safe

Every test wrote to the same idx and each configure() wiped its neighbours
mid-flight, which is why CI pins --test-threads=1. At default test threads on
master this is a race, not a number — 11 runs at fd92603, dedicated
container, FLUSHALL between:

master (fd92603, 7 tests) frequency
0 passed / 7 failed 3/11
1 passed / 6 failed 7/11
2 passed / 5 failed 1/11

So 0–2 of 7 pass depending on interleaving. This branch: 12/12 at default threads
×3, and 12/12 at --test-threads=1, with KEYS * empty after every run — the
order-independence is real, not a lucky interleaving. Master carrying this same
test file scores 0/12 at default threads.

The --test-threads=1 flag in .github/workflows/ci.yml is left alone — still
correct, just no longer load-bearing.

Docs

README.md asserted the opposite of the code in four places, all now fixed:
the "Per-config index isolation" heading + knob list; the --skip-upload
destructive-configure list (DEL <key>); the #230 datetime section; and the
"cardinality is not identity" paragraph, which still said "only the four
Redis-wire engines" and listed VectorSets among the one-object-per-server engines
— the exact twin of the engine/mod.rs trait doc, fixed in Rust and initially
missed in Markdown.

Also: the #230 section was chronologically impossible (it claimed a pre-#230
corpus is silently found, while the next paragraph said pre-#236 corpora are
rejected — and pre-#230 ⊂ pre-#236, since 66fcaaa predates this branch). Now
scoped to the only two things that reach it: VECTORSETS_INDEX_NAME_EXACT=1 at
base idx, or RENAME idx idx:<config>.

index_memory_bytes was documented nowhere outside code comments and the section
that now owns it still said FT.INFO; there is now a table giving both memory
fields, their sources and their scopes, and noting that a pre-#236 used_memory
and a current one are not comparable quantities.

experiment.rs's two stale comments (VCARD idx at :734, DEL idx at :797) are
fixed — comment-only; re-checked that #264 still merges clean over them.

Merge state

git merge-tree against #264 (feat/record-effective-config-212): zero conflict
markers. #264 reroutes the *_INDEX_NAME reads through
effective_config::env_or/env_flag while my exact_pin_drops_the_config_suffix
drives them via std::env::set_var. Verified rather than assumed — performed
the merge in a scratch worktree and ran it live: 5/5 index_naming unit tests,
12/12 integration tests, 931 unit tests total.

Gates

Baseline: master fd92603 = 872.

  • cargo test --lib --bins — 874 passed, 0 failed (lib 126,
    vector-db-benchmark 746, generate-dataset 2, three bench_* 0). +2 = the new
    index_naming tests; the 5 new integration tests are docker-gated.
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo fmt --check — clean
  • cargo test --test integration_vectorsets — 12/12 at default threads ×3, and
    12/12 at --test-threads=1
  • cargo test --test overhead_invariants — 9/9

Deliberately out of scope

Closes #236

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

@fcostaoliveira @claude
fix(vectorsets): derive the vector-set key per config so concurrent r…
23207c4 
…uns stop clobbering each other

VectorSets never got the #151-4 per-config namespacing that the rest of the
Redis-wire family did. Every config addressed the literal key `idx`, and
`configure()` opened with `DEL idx`, so starting a second config did not merely
interleave with the first — it deleted the first's entire corpus and rebuilt the
same key with its own. Two configs, two datasets, or two concurrent runs on one
server were mutually destructive, and nothing named the collision so nothing
could detect it.

The key is now `derive_index_name("VECTORSETS_INDEX_NAME", "idx", &config.name)`
— the same helper, the same `<base>:<sanitized-config-name>` shape, the same
`_EXACT` escape hatch and the same defaulting as redis/valkey/dragonfly/kividb.
A vector set is one key rather than an index over a keyspace, so there is no
`derive_key_prefix` analogue: this name IS the namespace.

Every site that named the key moved with it: VADD (batch and the mixed-workload
single update), VSIM (search, the mixed path, and the connection-priming query),
the `DEL` in both `configure()` and `delete()`, and the `VINFO` in
`get_memory_usage()` and `server_metadata()`. `vectorsets.rs` no longer contains
the string "idx" anywhere except the default-base argument.

`experiment::run`'s startup collision guard now knows "vectorsets", so an
exact-pinned sweep of >1 config is rejected before any data is written instead of
silently re-collapsing into one shared key.

Migration: a vector set left behind by an older run under the bare `idx` key is
not found and not deleted. That is accepted — it is a regenerable benchmark
corpus, and `--skip-upload` reuse across the boundary would in any case be
reading a corpus the run cannot attribute. Delete the stale `idx` key by hand.

Verification (live redis:8.8.0, two configs back-to-back on one server, dims 8
and 16 so a surviving key is attributable by content, all read back server-side
with VCARD/VDIM/EXISTS rather than by recall):

  before (master)
    after config A: VCARD idx:vectorsets-iso-a=0  (VDIM 0) | VCARD idx=400 (VDIM 8)
    after config B: VCARD idx:vectorsets-iso-a=0  (VDIM 0),
                    VCARD idx:vectorsets-iso-b=0  (VDIM 0) | VCARD idx=400 (VDIM 16)
      -> one shared key, last writer wins; A's 8-d corpus is gone.

  after (this branch)
    after config A: VCARD idx:vectorsets-iso-a=400 (VDIM 8)  | VCARD idx=0
    after config B: VCARD idx:vectorsets-iso-a=400 (VDIM 8),
                    VCARD idx:vectorsets-iso-b=400 (VDIM 16) | EXISTS idx=0

As a side effect the integration suite is now parallel-safe: at default test
threads it went from 0/8 passing to 9/9.

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

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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)

fcostaoliveira and others added 2 commits August 8, 2026 18:18
@fcostaoliveira @claude
Merge origin/master (#271) into #236, threading the derived key throu…
37e73da 
…gh corpus_row_count

The textual conflict was one import line. The real conflict was semantic and the
merge tools could not see it: #271 added `Engine::corpus_row_count()` to
`vectorsets.rs` as `VCARD idx` — the literal key — while this branch was moving
every other site to the per-config `idx:<config>`. Merged naively, the corpus
verification `--skip-upload` depends on would count a DIFFERENT key from the one
the upload and the search use.

That is not theoretical. Reproduced live on the merge, seeding the legacy `idx`
with exactly the expected 400 rows:

  Experiment stage: Reuse check — server holds 400 of 400 expected rows
  → QPS: 1924.5, Recall: 0.0000, Precision@returned: 0.0000, MRR: 0.0000
  exit 0

The guard passed on the wrong key, the search ran against an empty per-config
key, and a recall-0.0 result file was published under a config name that claims
otherwise — a brand-new silent-wrong result created by the merge itself, in the
exact area both PRs are about. `corpus_row_count` now probes `self.config.key`,
with a comment naming the hazard so a future merge does not re-introduce it.

`vectorsets.rs` again contains the string "idx" exactly once: the default-base
argument to `derive_index_name`.

REVIEWER FINDING (ensure_index_exists): not fixed by adding a second mechanism,
because #271 already shipped the right one. VectorSets has no `ensure_index_exists`
analogue and `VSIM` on a missing key returns an empty array with no error, so
"search succeeded" is not evidence. But `--skip-upload` now runs
`check_corpus_reuse_precondition` → `corpus_row_count` → `VCARD <key>`, and 0 rows
against a declared 400 is classified `Short` → hard error:

  --skip-upload: the corpus you asked to reuse is empty or missing — config
  'vectorsets-migrate' holds 0 of the 400 rows dataset 'vs-migrate' declares.

So the README's promise that "--skip-upload against a missing/mismatched index now
hard-errors instead of silently writing a recall 0.0 file" is now TRUE for the
fifth member of the family, via the mechanism the other four also route through
for their row counts. A `VCARD`-based check inside `vectorsets.rs` would have been
a second, redundant gate.

New integration test `test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus`
pins exactly this. It seeds the legacy un-suffixed key with EXACTLY the expected
row count, which is what gives it teeth: a `corpus_row_count` reading the old key
sees 400 of 400, returns Ok, and the run proceeds — the RED transcript above. It
carries a positive control (upload, then `--skip-upload` against the config's OWN
corpus must succeed) so it cannot pass by rejecting everything, and it runs over a
private base (`idx236mig`) rather than the global `idx`, because
`test_vectorsets_two_configs_do_not_clobber_each_other` asserts `idx` is never
written and two tests mutating it would race at default test threads — the very
property #236 restored.

Docs: README's "Per-config index isolation" section now names VectorSets in the
heading, lists `VECTORSETS_INDEX_NAME` with the other four `*_INDEX_NAME` knobs and
their `_EXACT` rule, describes the single-key shape (no doc-key prefix), gives the
pre-#236 cut-off alongside the pre-#151-4 one, and explains why the hard-error
promise holds for an engine whose search cannot fail. The `--skip-upload` section's
destructive-configure list now reads `DEL <key>` rather than `DEL idx`.

Gates on the merge commit (new baseline: master fd92603 = 872):
  cargo test --lib --bins  → 874 passed, 0 failed
    lib 126 | vector-db-benchmark 746 | generate-dataset 2 | bench_hdf5/jsonl/npy 0
    (+2 = the two new index_naming tests)
  cargo clippy --all-targets -- -D warnings  → clean
  cargo fmt --check                          → clean
  cargo test --test integration_vectorsets   → 10/10 at DEFAULT test threads (live redis:8.8.0)
  cargo test --test overhead_invariants      → 9/9

Two-config coexistence re-verified on the merged tree, server-side:
  after A: VCARD idx:vectorsets-iso-a=400 (VDIM 8)  | legacy VCARD idx=0
  after B: VCARD idx:vectorsets-iso-a=400 (VDIM 8),
           VCARD idx:vectorsets-iso-b=400 (VDIM 16) | EXISTS idx=0

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fcostaoliveira @claude
fix(vectorsets): attribute memory per config, and finish the #271 doc…
4969ea5 
…/test debt

Follow-up to the merge commit, closing the three reviewer blockers.

BLOCKER 2 — get_memory_usage over-reported once configs could coexist.

It returns server-wide `INFO memory:used_memory`. That was CORRECT as a per-config
figure before this PR: the hardcoded key plus a destructive `DEL` in `configure()`
guaranteed exactly one resident corpus, so the server total WAS this config's cost.
Making configs coexist silently turned it into the sum over all of them — 2× for
two configs, N× for an N-config sweep, and the new coexistence test is exactly that
scenario. A metric that was right became wrong as a side effect of a correctness
fix, which is the worst way to lose one.

`VINFO` carries no memory field (verified live), but the corpus is a single key, so
`MEMORY USAGE <key>` attributes it exactly. Published as `index_memory_bytes`, the
same field name redis/valkey/dragonfly/kividb already use for their per-index
`FT.INFO` figure, so no consumer needs a special case. `used_memory` keeps its
shape and meaning as the global secondary, with the caveat spelled out in a comment
mirroring redis.rs's.

Measured on the merged tree, two 400-doc configs on one server:

  A alone:            index_memory_bytes=287,740  used_memory=1,985,664
  B, with A resident: index_memory_bytes=287,740  used_memory=2,319,864
  MEMORY USAGE idx:vectorsets-mem-a=287,740  idx:vectorsets-mem-b=287,740

The per-config figure is flat and matches the key; the global one grows. New test
`test_vectorsets_index_memory_is_per_config_not_server_wide` pins it and is RED
without the fix (`index_memory_bytes` absent → -1 for both configs).

Its final assertion deliberately does NOT check that `used_memory` grew between the
two runs: that is global state any concurrent test moves either way, and it flaked
1-in-3 in the full suite. It asserts a floor instead (`used_memory > 2 ×
index_memory_bytes`) — same point, and other corpora landing only make it safer.

BLOCKER 1 leftovers — the stale docs and the missing test.

`corpus_row_count`'s doc comment still said "The key is the hardcoded `idx` (issue
#236): this count therefore cannot distinguish two configs sharing one server", and
`engine/mod.rs`'s trait doc still listed VectorSets among the engines with "exactly
ONE such object per server, shared by every config". Both were written before this
PR and both now assert the opposite of the code. Rewritten: VectorSets is the fifth
per-config member, and its count is what makes the `--skip-upload` promise true for
an engine whose search cannot fail.

`test_vectorsets_corpus_row_count_tracks_the_live_key` is the family test #271
shipped for kividb/mongodb/qdrant/valkey and not for vectorsets — which is why CI
could not see the hardcoded probe. Same shape: upload, `VREM` half the corpus behind
the tool's back, assert the run hard-errors with "holds 200 of the 400 rows". A
constant, or a read of the old shared key, fails it.

BLOCKER 3 — no new mechanism added, because #271 already provides one.

With `corpus_row_count` reading the derived key, `--skip-upload` against a
pre-#236 corpus classifies `Short` and hard-errors before anything is measured. An
`ensure_index_exists` analogue would be a second, redundant gate on the same path.
Covered by `test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus` (added in
the merge commit), which seeds the legacy key with EXACTLY the expected row count
so a probe on the wrong key would certify 400-of-400 and let the run publish
recall 0.0.

Docs — the third stale README claim.

The "#230 datetime corpora" section still read "`VCARD idx`" and "the engine uses a
single hardcoded key (`idx`, issue #236), there is not even a name change for the
tool to detect. A stale corpus of the right size is found, the run succeeds, and
only the recall number is wrong." Every clause was falsified. It now separates the
two cut-offs: a pre-#230 corpus IS found (right size, wrong encoding — the one
remaining silent case), while a pre-#236 corpus is NOT found and is rejected, with
the reason spelled out: `VSIM` on a missing key returns an empty array with rc=0,
so `failed_queries` stays 0 and the empty search is FAST — recall 0.0 at inflated
QPS is worse than a plain zero.

Gates (baseline: master fd92603 = 872):
  cargo test --lib --bins → 874 passed, 0 failed
    lib 126 | vector-db-benchmark 746 | generate-dataset 2 | bench_hdf5/jsonl/npy 0
  cargo clippy --all-targets -- -D warnings → clean
  cargo fmt --check                        → clean
  cargo test --test integration_vectorsets → 12/12 at DEFAULT test threads,
    three consecutive runs, no flake (master, measured the same way in a clean
    worktree three times: 1 passed / 6 failed)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fcostaoliveira fcostaoliveira mentioned this pull request Aug 8, 2026
Closed
@fcostaoliveira @claude
docs(vectorsets): fix the fourth stale README claim, scope three over…
b6755ee 
…claims, and correct this PR's own test-teeth story

Claim accuracy only — no behaviour change. Every item below is a place where the
prose asserted more than the code delivers, including two I wrote myself.

BLOCKER 1 — the fourth stale README place, twin of one I had already fixed.

README's "cardinality is not identity" paragraph still read: "Only the FOUR
Redis-wire engines address a per-config index and keyspace … MongoDB, pgvector,
Elasticsearch/OpenSearch, Qdrant, Milvus AND VECTORSETS each have ONE corpus
object per server, shared by every config and every dataset — so a full-size
corpus uploaded by a sibling config … passes."

That is verbatim the assertion I rewrote in `engine/mod.rs`'s trait doc. I fixed
the Rust twin and missed the Markdown one, then claimed in the PR body that all
three stale places were fixed; there were four. Post-PR it was false three ways:
VectorSets is per-config, a sibling's corpus does NOT pass (this PR's own
`test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus` proves it), and
"four" is "five". It also contradicted my new text 50 lines below, which a reader
reaches second.

Rewritten to five/ten, with the limit that actually survives on the per-config
engines spelled out: a sibling CONFIG no longer passes, but the same config
re-run against a DIFFERENT DATASET of equal size still does, because the key
derives from the config name alone.

BLOCKER 2 — the #230 section was chronologically impossible.

It said a pre-#230 corpus "is found, the run succeeds, and only the recall number
is wrong … the one failure mode left here", while the next paragraph said a
pre-#236 corpus is not found and is rejected. `git merge-base --is-ancestor
66fcaaa fd92603` is true: #230's fix predates this branch, so pre-#230 ⊂ pre-#236
and the two halves are mutually exclusive. After this PR the "right size, wrong
encoding, silently found" case is unreachable by any released binary.

Now scoped: the wrong-encoding case is reachable only by pinning
`VECTORSETS_INDEX_NAME_EXACT=1` at the base `idx`, or by `RENAME idx idx:<config>`
— both named, and both explicitly flagged as not migration paths this README
recommends.

S1 — this PR's own account of where its test's teeth are was wrong.

Three places (the merge commit message, the test's doc comment, the PR body)
credited "seeds the legacy key with EXACTLY the expected row count" as what makes
the migration test sharp — the idea being that a probe hardcoded to `idx` would
read 400 of 400 and wave the run through. It would not. That test runs over a
PRIVATE base (`idx236mig`, chosen so it cannot race the coexistence test over the
global `idx`), so a hardcoded probe reads an empty `idx`, gets 0 of 400, and
hard-errors — passing the negative half for the wrong reason.

Verified by counterfactual on this tree, `corpus_row_count` patched back to
`.arg("idx")`, live, `--test-threads=1`:

  test_vectorsets_corpus_row_count_tracks_the_live_key        ... FAILED  (:869)
  test_vectorsets_skip_upload_hard_errors_on_a_pre_236_corpus ... FAILED  (:777)
  10 passed; 2 failed

`:869` is `holds 200 of the 400 rows` — a direct wrong-key detector. `:777` is the
POSITIVE CONTROL ("--skip-upload against this config's OWN corpus must succeed"),
not any seeded-negative assertion. So the merge hazard is caught by CI, by both
tests, but not by the mechanism I named. The doc comment now says so explicitly,
including why the seeding still earns its place (it makes the scenario realistic —
rejected even with a full-size corpus sitting on the server — rather than merely
absent).

S3 — the headline new metric was undocumented, under a heading that contradicted it.

The "Per-config index isolation" heading gained VectorSets, but the line under it
still said memory "is reported per-index via FT.INFO", which VectorSets does not
use; and `index_memory_bytes` appeared nowhere in any .md. Added a table giving
both fields, their sources (`FT.INFO` vs `MEMORY USAGE <key>`) and their scopes,
plus the two consequences a reader needs: `used_memory` from a pre-#236 run and
from a current sweep are not comparable quantities, and an orphaned legacy `idx`
keeps inflating `used_memory` in every later result file until it is deleted.

S4 — `experiment.rs`: both stale comments fixed after all.

`:734`'s historical measurement record (`VCARD idx`) and `:797`'s destructive-call
list (`DEL idx`) are now `VCARD idx:<config>` and `DEL <key>`, matching the README
line I had already updated. Comment-only, two words; #264 merges clean over them
(re-checked with `git merge-tree`). My earlier disclosure said `:796` — the `DEL
idx` line is `:797`.

Test: the memory test now asserts SCALING, not just presence.

Every previous assertion was satisfied by a fixed per-key overhead figure, which is
the plausible way `MEMORY USAGE` could be meaningless (a module type with no
`mem_usage` callback reports bare key overhead). It now compares the 400-element
corpus against a 1-element vector set of the same dimensionality created on the
same server, and requires >10×.

Artifact for a claim that had none.

The README asserted "measured 437 → 3413 QPS" for the empty-corpus case with no
transcript in the PR. Re-measured on this tree, 400-vector corpus, ef=400
parallel=1, same server, via `--skip-upload --allow-partial-corpus` after deleting
the key:

  FULL   VCARD=400 rps=637.55  recall=1.0  failed_queries=0
  EMPTY  VCARD=0   rps=2679.26 recall=0.0  failed_queries=0

4.2× faster for measuring nothing, `failed_queries: 0` in both. README now carries
these numbers instead of the unsourced pair.

Master baseline is a RACE, not a number.

I reported "1 passed / 6 failed, three times". Re-measured 11 times at default
threads, dedicated container, FLUSHALL between runs, at `fd92603`:

  0 passed / 7 failed   ×3
  1 passed / 6 failed   ×7
  2 passed / 5 failed   ×1

So it is 0–2 of 7 depending on interleaving. `mixed_benchmark` is the most frequent
survivor but not the only one (`datetime_keyword_schema` also survived once), so
"the one that survives" was wrong twice over. This branch: 12/12, three consecutive
runs, no variance.

Gates (baseline: master fd92603 = 872):
  cargo test --lib --bins → 874 passed, 0 failed
  cargo clippy --all-targets -- -D warnings → clean
  cargo fmt --check → clean
  cargo test --test integration_vectorsets → 12/12 ×3 at default threads
  cargo test --test overhead_invariants → 9/9

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 8, 2026
Closed
Closed
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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)

@fcostaoliveira fcostaoliveira mentioned this pull request Aug 8, 2026
Open
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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)

1 similar comment
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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)

@fcostaoliveira
fcostaoliveira merged commit cff7e3a into master Aug 8, 2026
18 checks passed
@fcostaoliveira fcostaoliveira mentioned this pull request Aug 8, 2026
Merged
fcostaoliveira added a commit that referenced this pull request Aug 8, 2026
@fcostaoliveira
Merge origin/master (#278) into feat/record-effective-config-212
c0e9e2a 
Merged, not rebased: the branch is published and the standing rule here is to
add a commit rather than force-push.
fcostaoliveira added a commit that referenced this pull request Aug 8, 2026
@fcostaoliveira
test: serialize #278's index-naming tests against the process-wide re…
bacc2c5 
…corder

#278's two new `index_naming` tests resolve through the recorder
(`derive_index_name` -> `env_or` / `env_flag`) without holding `test_lock`, so
an observation can land between another test's `begin_experiment` and its
`snapshot()`. Six consecutive green suites did not disprove it — the window is a
few instructions wide — and "measured green" is precisely the evidence that was
wrong about this test last time.

Closed two ways rather than one:

* those tests now take `effective_config::test_lock()`, which any test driving
  the global recorder must;
* `begin_experiment_clears_the_previous_experiments_accumulation` now asserts
  the keys it SEEDED are gone rather than asserting total emptiness, so a
  concurrent writer cannot fail it. Equally strong against the mutation, since
  deleting `reset()` leaves exactly those keys behind — re-verified 5/5 red
  (3/3 alone, 1/1 in-suite, 1/1 `--test-threads=1`). Total clearing stays
  covered deterministically by `begin_clears_every_accumulating_field`, which
  owns a private `Recorder`.

These changes were written before the merge commit and lost to a
`git checkout HEAD -- src/` used to measure the master baseline; recommitted
here rather than amending, since the branch is published.

Refs #212
@fcostaoliveira fcostaoliveira mentioned this pull request Aug 8, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

VectorSets uses a hardcoded idx key — it never got the #151-4 per-config namespacing, so concurrent runs clobber each other

1 participant

@fcostaoliveira