fix(opensearch): force-merge to one SEARCHABLE segment, matching elasticsearch (#210) - #246
Conversation
…ticsearch `elasticsearch.rs` force-merges with `max_num_segments(1)`; `opensearch.rs` force-merged with no segment target at all, leaving whatever Lucene's merge policy happened to settle on. A segment is one HNSW graph, and a k-NN query searches every segment of every shard and merges the per-segment result lists, so segment count moves BOTH recall and latency. Measured on glove-25-angular (1,183,514 docs, 1 shard) the un-pinned merge left 85 searchable segments. Pinning the merge alone is not enough on OpenSearch, and this is the part that is easy to ship broken. `_forcemerge` defaults to `flush=true`, which commits the merged segment, but on OpenSearch the commit only refreshes Lucene's INTERNAL reader — the searcher that answers queries keeps its handle on the pre-merge segments, and `refresh_interval: -1` (set during upload) means no periodic refresh will ever swap it. Same 150-doc probe on both engines, right after `_forcemerge?max_num_segments=1` and before any refresh: Elasticsearch 9.4.3 -> 1 segment, 1 searchable (merge visible) OpenSearch 3.7.0 -> 4 segments, 3 searchable (merge invisible) So the merge is followed by an explicit refresh. Without it the merge is a 20-minute, 2x-disk no-op that changes nothing a query sees. Also added, because pinning the merge makes it far longer: * Its own request timeout (`OPENSEARCH_FORCE_MERGE_TIMEOUT`, default max(OPENSEARCH_TIMEOUT, 1h)). The measured merge took 1077-1312 s — well past the 300 s client-wide `OPENSEARCH_TIMEOUT`, so sharing that bound would abort every attempt client-side while the merge kept running server-side and burn the whole #208 retry budget failing a merge that was always going to succeed. * A post-merge cluster-health wait, mirroring `elasticsearch.rs`, scoped to our index rather than the whole cluster so an unrelated system index stuck at yellow on a managed domain cannot fail a finished ingest. The health API answers HTTP 200 with `"timed_out": true` when the status was not reached, so the body is checked, not just the status. The #208 `retry_index_op` / `index_maintenance_retryable` behaviour is kept unchanged. This changes published OpenSearch numbers. Any existing OpenSearch baseline needs re-running. Closes #210
Docker Build ValidationPlatforms tested:
Tests performed:
|
…e retries Review follow-ups on #246. The new integration test was a coin flip. `searchable == 1` is the right invariant, but the PRE-merge segment count is not a property of the fixture — it is however many Lucene indexing buffers happen to be live at the single end-of-upload refresh, and 400 docs at dim 8 never fill one. Measured with a probe mirroring the engine's upload shape: 8 workers give 2-6 segments, 2 workers give 2, and 1 worker gives exactly 1 — and at 1, a bare `_forcemerge` is a no-op that also leaves 1, so the assertion holds on unfixed code. A 2-vCPU CI runner reaches that state, so the guard would have gone green on ~half of all reverts. Fixed with a conclusiveness check on `merges.total_docs`, which is exact here: the whole corpus when the merge had >1 segment to collapse, 0 when it had one. It runs AFTER the segment assertion, so that on genuinely unfixed code the accurate diagnosis fires rather than the guard. Verified 8 reps each: master FAILS 8/8 on the segment assertion, this branch passes 8/8, and forcing a single-segment upload produces "INCONCLUSIVE, not a pass". Three factual corrections, all verified against the live containers: * `_cluster/health` on a missed status returns HTTP 408, not 200, on BOTH OpenSearch 3.7.0 and Elasticsearch 9.4.3. `cluster_health_settled` was already correct (408 is outside 200..300) but the stated reason was wrong; the `timed_out` body check stays as defence in depth for the 2xx-carrying variant. * The recall mechanism is not "each segment contributes ef candidates" — Lucene passes each segment the global k. It is that each segment holds 1/N of the corpus, so a fixed ef explores a far larger FRACTION of a small graph. * CI cost: the force-merge change itself costs ~0.13 s across all 15 pre-existing tests; the suite's 15.12 -> 17.60 s is dominated by the new 16th test (~2.35 s). Three defects that would have reached managed domains: * `OPENSEARCH_FORCE_MERGE_TIMEOUT=0` resolved to Duration::ZERO, and reqwest checks the deadline before sending, so every attempt failed instantly and discarded a completed ingest. 0 now means unlimited, as the convention implies. * Force merge had no wall-clock budget. `retry_index_op` retries every transport error, so a 3600 s per-attempt bound meant an ~11 h worst case for one call. `RetryPolicy` gained an opt-in budget (other index ops unchanged, still count-only); force merge defaults to 2x its per-attempt timeout. `OPENSEARCH_FORCE_MERGE_BUDGET` overrides, 0 = unlimited. * `wait_for_cluster_health` retried everything. On AWS OpenSearch with fine-grained access control `cluster:monitor/health` is routinely denied, and the wait is the tail of force_merge(), so a 403 burned 11 attempts (~14.5 min) and then failed a COMPLETED ingest. 401/403 now warn and continue — the merge and refresh that determine the measured state have already succeeded — while 408 and the back-pressure statuses retry and anything else fails loudly. Also: malformed OPENSEARCH_FORCE_MERGE_TIMEOUT/BUDGET values now fail loudly instead of silently reinstating the default, matching parse_number_of_shards in this file; and upload() no longer refreshes before force_merge(), which was billed to OpenSearch's index_time while elasticsearch.rs pays nothing equivalent (~1 s on 1.18M docs) and is redundant now that force_merge refreshes on exit.
…emerge-segment-parity # Conflicts: # tests/integration_opensearch.rs
…ds a knob Surfaced by merging #246 into master, and by neither side alone. `every_shipped_config_knob_is_read_by_its_engine` decides "the engine reads this knob" with a whole-token text search over the engine's source. Master added KNOWN_UNREAD ("opensearch", "connection_params.request_timeout") for issue #245 — true there, because master's opensearch.rs contains no such token. #246 added three occurrences of `request_timeout` to opensearch.rs: reqwest's `.request_timeout(..)` builder on the force-merge and cluster-health requests, plus a local binding. Also true there, because that branch has no such entry. Merged, the token search matches, the guard concludes the knob is now read, and its anti-rot check demands the KNOWN_UNREAD entry be deleted as paid-off debt. Deleting it would have recorded #245 as fixed while the shipped `request_timeout: 10000` still has no effect — the silent-wrong-config failure this guard exists to catch, produced by the guard itself. The values behind all three occurrences come from OPENSEARCH_FORCE_MERGE_TIMEOUT / OPENSEARCH_TIMEOUT. None consults connection_params. Fixed structurally rather than with an exemption list. `knob_is_read` now ignores occurrences of the form `.leaf(` — a chained builder setter that merely shares a name with the knob — and counts every other occurrence, so field access, bare identifiers and `.setter(cfg.leaf)` forwarding all still read as reads. The local in force_merge is renamed `merge_deadline`, which is what it is; a local named after the transport setter is indistinguishable from a config read to any textual guard. The asymmetry is deliberate and documented at the function: discounting a real read costs a loud, self-explaining failure, while accepting a fake read costs silence. When the two are in tension, err toward loud. An earlier revision of this fix used a TOKEN_COLLISIONS allowlist. The structural rule subsumes it and needs no per-collision bookkeeping, so the list is gone. Guard behaviour is pinned by new assertions in `guard_detects_a_knob_the_engine_does_not_read`: a chained `.request_timeout(merge_deadline)` is not a read, while `cp.request_timeout.unwrap_or(300)` and `.request_timeout(cfg.request_timeout)` both are. * cargo test --lib --bins — 698 passed, 0 failed * cargo clippy --all-targets -- -D warnings — clean * cargo fmt --check — clean Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ailing ingests Re-applies the engine half of the #246 review follow-ups. It was validated alongside 27f72d4 but never reached master: my own timing/flakiness helper scripts ended with `git checkout HEAD -- engine/opensearch.rs` to undo their temporary revert, and HEAD was the previous commit, so the edits were discarded. The scripts now restore from a saved copy instead. * `OPENSEARCH_FORCE_MERGE_TIMEOUT=0` resolved to Duration::ZERO, and reqwest checks the deadline before sending, so all 11 attempts failed instantly and then hard-errored — discarding a completed ingest. `0` now means unlimited. * Force merge had no wall-clock budget. `retry_index_op` retries every transport error, so a 3600 s per-attempt bound meant an ~11 h worst case for one call. `RetryPolicy` gained an opt-in `budget` (the other three index ops are unchanged, still count-only); force merge defaults to 2x its per-attempt timeout. `OPENSEARCH_FORCE_MERGE_BUDGET` overrides it, 0 = unlimited. * `wait_for_cluster_health` retried everything. On AWS OpenSearch with fine-grained access control `cluster:monitor/health` is routinely denied even when the index actions are granted, and the wait is the tail expression of force_merge(), so a 403 burned all 11 attempts (~14.5 min) and then failed a COMPLETED multi-hour ingest — on exactly the managed domains the index-scoping was added to protect. 401/403 now warn and continue (the merge and refresh that determine the measured state have already succeeded); 408 and the back-pressure statuses retry; anything else fails loudly with the body attached. * Malformed OPENSEARCH_FORCE_MERGE_TIMEOUT/BUDGET values fail loudly instead of silently reinstating the default, matching parse_number_of_shards. Corrections verified against the live containers: * `_cluster/health` reports a missed status as HTTP 408, not 200, on BOTH OpenSearch 3.7.0 and Elasticsearch 9.4.3. `cluster_health_settled` was already correct (408 is outside 200..300) but the stated reason was wrong. The `timed_out` body check stays as defence in depth for the 2xx-carrying variant. * The recall mechanism is not "each segment contributes ef candidates" — Lucene passes each segment the global k. It is that each segment holds 1/N of the corpus, so a fixed ef explores a far larger FRACTION of a small graph. Reconciles the upload() comment with #248, resolving a conflict against the concurrent edit on this branch. That edit kept the pre-merge refresh, arguing #248 gave Elasticsearch one too and the engines were therefore symmetric. That misses the refresh force_merge() now performs on its way out: keeping a pre-merge refresh as well bills OpenSearch TWO refreshes against Elasticsearch's one, re-opening the asymmetry rather than closing it. Each engine now spends exactly one refresh in its timed index phase, placed where the server requires it — Elasticsearch's force merge swaps the external searcher onto the merged segment and OpenSearch's does not (ES 9.4.3: 1 segment / 1 searchable straight after the merge; OpenSearch 3.7.0: 4 / 3 with the merged segment at "search": false). The concurrent edit's TOKEN_COLLISIONS mechanism is kept as-is; it already handles the `.request_timeout(..)` builder-vs-knob name collision, so an alternative fix in contains_token was dropped rather than duplicating it.
…current edit Two agents landed the same rename on this branch. My re-application of the engine changes was based on the tree before 5705e89 and reverted its version, so this restores it, keeping that commit's comment. Also drops 'binds a local of the same name' from the TOKEN_COLLISIONS note: with the rename in place the only remaining occurrences of the token in opensearch.rs are the two transport builder calls, so the entry should describe just those.
Merged
|
Correction to my previous comment — the "Still open" item was wrongI claimed I grepped a stale working tree — my local checkout still carried a superseded edit So Everything else in that comment stands, and re-verified at the current head:
|
…oo (#239 review) Follow-up to the review of #250. A. The "glob order is not stable across platforms" claim was FALSE. glob 0.3.3 documents and implements alphabetical ordering (lib.rs:163), so the shadowing was deterministic, not flaky — `vectorsets-rs-NOQUANT.json` sorts after `vectorsets-NOQUANT.json` and won on every platform, every time. The error message, the doc comments and the justification for `paths.sort()` now say what is true: a duplicate resolves by an accident of filename ordering rather than by anyone's choice. B. A config file that fails to load was only a warning, on the theory that it surfaces as "no engines match". That only holds for exact single-name selection. Under a wildcard — and `--engines` DEFAULTS to `*` — the sweep just gets smaller and exits 0: Warning: qdrant-on-disk.json does not parse ... Found 1 datasets, 52 engines # 56 without the typo summary.rs picks best-QPS among the points that ran and plot.rs charts them, so the published peak and Pareto frontier are quietly truncated. The loader now returns the skipped files, a run REFUSES to start when the list is non-empty, `--allow-partial-configs` opts in, and the opt-in path records `skipped_config_files` in every summary JSON — mirroring `uncalibrated_configs` from #217, which decided this exact question. `--engines-file` was already strict; the two policies for one failure mode are now one. `--describe engines` stays tolerant and lists the offenders first: its job is to diagnose the directory. C. `read_dataset_configs` had the identical bare-insert bug, and worse: a dataset name selects a corpus AND its ground truth. A duplicate appended to datasets.json passed the whole suite. It is now the same hard error, with a `shipped_datasets_have_no_duplicate_names` guard. E. Message quality: every collision is reported in ONE error (two duplicates cost one fix-rerun cycle, not two); intra-file duplicates name `entry N of <file>` and drop the filename-ordering clause that does not apply to them; "first defined in"/"redefined in" became neutral "defined in"/"also defined in", because the order is alphabetical and telling someone their brand-new `aaa-mine.json` is the original is backwards. README documents both rules. Deliberately NOT touched: engine/opensearch.rs (PR #246 owns that file) and `glob(...).flatten()`, which cannot partially shrink the set — glob reports a directory-level error, giving zero configs, and zero is already a hard error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 similar comment
Docker Build ValidationPlatforms tested:
Tests performed:
|
…iring not just the predicates Review follow-ups on #246. ## Restores the structural knob-guard rule (A) Reverts the TOKEN_COLLISIONS allowlist back to 5705e89's `knob_is_read`, which does not treat a chained `.leaf(` builder setter as evidence that an engine reads a config knob of that name. Mutation testing decided this: the allowlist had two silent-green holes where the structural rule has none. mutation allowlist restored rule delete both .request_timeout(..) calls loud loud delete the KNOWN_UNREAD entry only loud loud delete BOTH entries SILENT loud engine genuinely reads the knob, both kept SILENT loud Both holes re-verified by mutation against the restored code. The first is the worse one: the allowlist's own error message ("TOKEN_COLLISIONS lists X but KNOWN_UNREAD does not") steers the reader into deleting the other entry, landing exactly in the silent-green state with #245's debt untracked. The second is deeper — `collides` short-circuits `knob_is_read` unconditionally, so once #245 is actually fixed the anti-rot branch can never fire for that knob. ## Fail-early env validation (B) Both new knobs were parsed for the first time INSIDE force_merge, i.e. after the whole upload, so a malformed OPENSEARCH_FORCE_MERGE_BUDGET=2h — precisely the value parse_env_secs exists to reject loudly — would discard a multi-hour ingest at the last step. Resolved in `new()` now and stored as Durations, matching parse_number_of_shards, which resolves there for the same reason. Also removes two env::var reads from inside the timed index window. ## Wiring tests (C) Five mutations broke the wiring while leaving every predicate correct, and all five passed the suite. Fixed by: * `retry_index_op_stops_at_the_wall_clock_budget_not_just_the_attempt_count` — drives an always-failing `send` closure with a 150 ms budget and a 1,000,000 attempt ceiling, so only the budget can end it. Its sibling pins that an unbudgeted policy still stops on the attempt count, so a budget cannot leak into create/delete/refresh. * `classify_health_response` extracted so the ORDER of the health checks is pinned. Swapping the authorization and retry checks flips "continue the run" into "abort a completed multi-hour ingest" with every predicate test still green; it was the only path returning Ok(()) without a settled cluster and had zero coverage. * `retry_policy_budget_defaults_off_and_is_opt_in` was a tautology — it built a RetryPolicy literal and asserted the field it had just set. It now reads `index_op_policy()`, the actual source of the default it claims to pin. * `resolve_force_merge_budget` is a free function like its sibling, with coverage for the knob itself (default tracks the per-attempt deadline; explicit value wins both ways; 0 = unlimited). ## Code defects (D) * The comment claiming both engines "do an explicit refresh() before force-merging" was false at head — a survivor of the reverted concurrent edit, contradicting the correct comment 320 lines below. OpenSearch refreshes AFTER. * `is_authorization_denied` was the only status-only classifier in this file. OpenSearch ships `cluster_block_exception` as HTTP 403 too, and that is transient — a disk-watermark block clears itself. It now inspects the body, and `cluster_health_retryable` delegates to `index_maintenance_retryable` so a 403 block is retried exactly as its three siblings retry it. * A `timed_out` body is now classified Retry whatever status carried it, so the 2xx-carrying variant no longer falls through to a hard failure. Also: the "~14.5 min" cost of a 403 burn was inflated ~4x (a 403 returns immediately, so only the backoff counts, ~1.7-3.5 min), and "10 attempts" is corrected to 11 (10 retries) where it described the same budget. README documents both new knobs; they were otherwise undocumented, and the table implied force-merge was bounded only by OPENSEARCH_INDEX_OP_MAX_RETRIES.
Correction to my merge comment aboveThat comment said the builder-setter collision was "fixed structurally — It is true again as of I was wrong to revert it, and mutation testing is what settled it. Against the
Both holes were re-verified as closed against the restored code before pushing. The first hole is worse than a plain gap: the allowlist's error message on the The second hole is structural: My original reasoning — a missed read is loud, a false read is silent, so err |
…nt hanging instead of failing Two fixes found by running the review's wiring mutations rather than assuming they would die. `retry_index_op_stops_at_the_wall_clock_budget_not_just_the_attempt_count` used `max_retries: 1_000_000` so that only the budget could end the loop. With the budget mutated off that does not fail the test — it runs 1M attempts against a capped backoff, i.e. hours, which in CI is a timeout rather than a diagnosis. A mutation must fail FAST. Lowered to 60 retries: a correct run still stops on the 150 ms budget after a handful of attempts, and a broken one ends in ~4 s with the assertion that the error names the wall-clock budget. Added `merge_bounds_are_resolved_when_the_engine_is_constructed`, which had no coverage at all: it drives the env -> `new()` -> stored `Duration` path directly, pinning that the 300 s client timeout does not bound a merge, that the budget tracks the deadline rather than being a constant, that 0 means unlimited on both knobs rather than "expire immediately", and — the point of resolving in `new()` — that a malformed `OPENSEARCH_FORCE_MERGE_BUDGET=2h` fails at construction, before there is an ingest to discard. Mutation results on this tree (`cargo test --bins`, each mutation applied alone): M1 retry_index_op never enforces the budget KILLED M2 auth check unwired from the health decision KILLED M3 force merge sends self.timeout, not the deadline SURVIVES M4 force-merge budget computed then discarded SURVIVES M5 resolve_force_merge_budget ignores its env knob KILLED M3 and M4 are single-argument substitutions at the one call site that builds the request and the retry policy. The values are pinned as computed and stored, and each predicate is pinned as correct, but nothing observes which value is actually handed to `.request_timeout(..)` / `retry_index_op(..)`; extracting a helper only moves the mutation to the helper's call site. Closing them needs an end-to-end run whose merge exceeds a deliberately tiny deadline — too slow for the 400-doc per-PR suite. Filed as #272 rather than claimed as covered.
Wiring mutation results (
|
| # | mutation | result | killed by |
|---|---|---|---|
| M1 | retry_index_op never enforces the budget |
KILLED | retry_index_op_stops_at_the_wall_clock_budget_not_just_the_attempt_count |
| M2 | auth check unwired from the health decision | KILLED | health_verdicts_compose_so_a_denial_never_aborts_a_finished_ingest |
| M3 | force merge sends self.timeout, not merge_deadline |
SURVIVES | — |
| M4 | force-merge budget computed then discarded | SURVIVES | — |
| M5 | resolve_force_merge_budget ignores its env knob |
KILLED | force_merge_budget_defaults_to_two_attempts_and_honours_its_knob |
Running the mutations found a defect in my own test
M1's test originally used max_retries: 1_000_000, on the reasoning that only the
budget could then end the loop. With the budget mutated off that does not fail
the test — it hangs: a million attempts against a capped backoff is hours, which
in CI is a timeout, not a diagnosis. A mutation has to fail fast. Lowered to 60
retries: a correct run stops on the 150 ms budget after a handful of attempts, a
broken one ends in ~4 s failing the assertion that the error names the budget.
This is exactly why the sweep was worth running rather than assumed.
Why M3 and M4 survive, and what would kill them
Both are single-argument substitutions at the one call site that builds the
request and the retry policy:
.request_timeout(merge_deadline) // M3 -> self.timeout
index_op_policy().with_budget(self.force_merge_budget) // M4 -> index_op_policy()The new merge_bounds_are_resolved_when_the_engine_is_constructed pins that the
right values are computed and stored, and the predicate tests pin that each value
is correct — but nothing observes which value reaches the call. Extracting a
force_merge_bounds() helper does not close it; the mutation just moves to the
helper's call site.
Only an end-to-end run does: a corpus large enough that the merge reliably exceeds
a deliberately tiny OPENSEARCH_FORCE_MERGE_TIMEOUT=1, then asserting the run
fails. Correctly wired it times out; mutated it sends the 300 s client-wide
timeout, the merge succeeds, and the run passes. That needs a fixture far bigger
than the 400-doc per-PR suite, so it is filed as #272 rather than bolted on here.
Also added
merge_bounds_are_resolved_when_the_engine_is_constructed covers the fail-early
fix, which had no test: env → new() → stored Duration, including that
OPENSEARCH_FORCE_MERGE_BUDGET=2h fails at construction rather than after the
upload it would otherwise discard.
Gates: cargo test --lib --bins 807 passed, 0 failed, 0 ignored
(98 + 0 + 0 + 0 + 2 + 707 across the six binaries);
cargo clippy --all-targets -- -D warnings clean; cargo fmt --check clean.
|
Heads-up from #264 (provenance recording for #212), which is in flight and will interact with this PR in two small places. 1. 2. That is the guard working as designed rather than an accident: today, a run with
|
Docker Build ValidationPlatforms tested:
Tests performed:
|
… actually used The results JSON carried the configuration *name* and nothing else about how the run was set up: no `collection_params`, no `upload_params`, none of the ~104 environment knobs the engines read, and not even `--host`. Two runs of one committed configuration could differ by `OPENSEARCH_SEARCH_RETRY_BUDGET_MS`, or run against two different servers, report materially different `p99_time`/`rps`/`upload_time`, and produce byte-identical `params` blocks. Adds a per-experiment recorder (`effective_config`) that captures values **at the point they are resolved**, emitted as `engine_params` in the `*-search-*` / `*-upload-*` files (under `params`) and in `*-summary.json` (top level): declared collection_params / upload_params as the FILE spells them effective what the run used, after env + defaults + in-code pinning env which variables were CONSULTED, and what was there invocation --host, --skip-upload and the other measuring flags overridden declared value + used value + why they differ ignored_declared_keys known-unread knobs, marked non-exhaustive phase which phase's resolved state this block describes Recording the declared config would reproduce the failure this repo keeps hitting, so the emphasis is on divergence: * `env_parsed` / `env_or` / `env_flag` / `env_opt` / `env_present` resolve AND record in one step, so the artifact value is by construction the value the caller received. Text that does not parse, or a flag value that is not recognised, leaves the run on its default and lands in `overridden` with both sides. `VALKEY_PROTOCOL=" resp3 "` selects RESP2 and now says so. * Only variables the run actually read appear; one that was read and unset appears as `null` with its defaulted result. * Elasticsearch pins `number_of_shards` in code, and `config::KNOWN_UNREAD` is the CI-asserted inventory of knobs shipped configs declare and engines ignore. Both now reach the artifact, which used to say nothing while the guard said "documented debt (#245)". * `declared` is the raw configuration-file JSON (`EngineConfig::raw`), not a serde round-trip: the round-trip injected nulls, normalised `m`/`ef_construct` to `M`/`EF_CONSTRUCTION`, and silently dropped `index_options.type` and `index_options.confidence_interval`. * Credentials are recorded as *set* and never disclosed. Redaction lives in `Recorder::record_effective`, one choke point, and `snapshot()` asserts over every map that no credential-named key is published in the clear — a leak was otherwise one `env_or` -> `env_parsed::<String>` edit away, with tests green. Three guards keep it from decaying, each asserted to bite: 1. no `std::env::var` in production code outside the recorder (a raw read is invisible to the artifact, and an early return past a recorded read makes the file affirmatively deny a knob that was consulted); 2. `KNOWN_UNRECORDED` lists every remaining raw-text `env_var` site with a reason, asserted in BOTH directions so migrating one forces deleting its row; 3. `experiment::run` still begins a recording before building the engine. `metrics_schema_version` is deliberately NOT bumped: it versions the metric definitions, which did not move. `engine_params.schema_version` is the marker for this block, and it is emitted in all three file kinds. Engine edits are the mechanical `std::env::var` -> recorder swap plus 14 value-transforming knobs moved onto recording helpers. #246's `parse_env_secs` is routed through the recorder too, so `OPENSEARCH_FORCE_MERGE_*` is recorded. Recording the resolved INDEX params (`m`, `ef_construction`, the qdrant HNSW diff) is deliberately excluded and tracked in #273: doing it correctly requires reading them back off the server after `configure()`, because a `--skip-upload` run or a shared ES/OS index makes the process's intent diverge from what the server has, and a field named `effective` asserting a value the server does not have is worse than recording nothing. Closes #212 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 participant
Closes #210.
elasticsearch.rsforce-merges withmax_num_segments(1).opensearch.rsdidnot, so an ES-vs-OS chart compared one HNSW graph against however many Lucene's
merge policy happened to leave. Every existing OpenSearch result is now stale.
Recall goes down, throughput goes up. Do not put an old OpenSearch number
next to a new one, or next to another engine.
What master actually did
Measured directly against a live container — master's force merge was not merely
under-specified, it was a no-op:
merges.total_docsmax_num_segments)max_num_segments(1)alone is not enough on OpenSearch_forcemergedefaults toflush=true, which commits the merged segment — but onOpenSearch that commit only refreshes Lucene's internal reader. The searcher
answering queries keeps the pre-merge segments, and
refresh_interval: -1meansno periodic refresh will ever swap it.
Identical 150-doc, 3-segment probe against both containers in
tests/docker-compose.test.yml, inspected after the merge and before anyrefresh:
_statsquantifies it: the merge bumps ES'srefresh.external_totalby 1 andOpenSearch's by 0. Reproduced three times in review, including on a clean
container. So this PR mirrors
elasticsearch.rs's effect, not just its call:the merge is followed by an explicit refresh. Without it the merge is a
20-minute, 2x-disk no-op that changes nothing a query sees.
Refresh accounting. Both engines disable periodic refresh during upload
(
refresh_interval: -1, #248) and each makes exactly one explicit_refreshcall in the timed index phase — ES before its merge, OpenSearch after, because
only ES's merge reopens the searcher. That is why the pre-merge refresh is
removed here rather than kept:
force_mergenow refreshes on exit, so keepingboth would give OpenSearch two explicit refreshes to ES's one. The external
refresh counts are still not equal — ES's merge performs one implicitly, so ES
bills two and OpenSearch one. Impact is ~0.1% of the index phase and both engines
end in an identical state; the tidy-up is #259.
Measurement: one unreplicated run, no artifact retained
I ran
glove-25-angular(1.18M × 25d, 1 shard) before/after on the samecorpus in the same window. Treat the following as a single observation, not a
benchmark result. The run artifacts were not preserved and it has not been
repeated; a filesystem sweep during review found no surviving output.
Conditions were bad: load average 32–118 on 14 cores, the root filesystem at
99–100% (which had tripped OpenSearch's flood-stage
cluster.blocks.create_indexand required disabling the disk watermark on the test container), and the tool's
own
CLIENT LIKELY SATURATEDguard fired. Absolute QPS and latency figures aretherefore omitted from this PR entirely — an earlier revision tabulated them
while also declaring them unpublishable, which invites exactly the copy-paste it
warned against.
What is load-independent, and what the change is actually about:
Recall is the trustworthy column: it came out identical to four decimal places at
parallel=1andparallel=100at everyef, which is what a deterministic,load-independent measurement looks like. Throughput and p50 improved at every
point and p95/p99 at five of six — at
parallel=100, ef=512p95 and p99 gotworse — but all of that was measured under a fired saturation guard and I draw
no conclusion from it. No iso-recall or trade-off-curve claim is made: that
would rest on a ~25% gap between numbers I have just called unusable.
Review independently confirmed the recall shape at 200k × 25d against exact numpy
ground truth: 0.9992 → 0.9892 at ef=128, converging by ef=512.
Why recall moves. Not because each segment gets its own
efbudget — Lucenepasses each segment the global
k. Each segment holds 1/N of the corpus, so afixed
efexplores a far larger fraction of a small graph than a big one.Multi-shard.
max_num_segmentsis per shard, so "85 → 1" is the 1-shardfigure;
opensearch-5-shard.jsonends at 1 segment per shard, 5 total. Reviewverified per-shard symmetry on both engines at 5 shards. The recall deltas above
are not transferable to the 5-shard sweep, where the corpus is already 5
graphs regardless of merging.
Force-merge duration and CI cost
The index phase (refresh + merge) went from ~15 s to ~1,077–1,312 s on 1.18M docs
— same single unreplicated run, same caveats. Unpinned the merge was a no-op;
pinned it rewrites the whole corpus and needs ~2x the disk transiently.
FORCE_MERGE_TIMEOUT_SECS = 3600is a safe bound above that observation, nota value calibrated to it.
CI cost, attributed rather than lumped (2 reps each, quiet box):
The force-merge change costs ~0.13 s across all 15 tests; the suite grows
+2.5 s, and ~2.35 s of that is the new test, which is a full binary run. The
two scale differently: adding a test is a one-off, the merge cost scales with
corpus size.
Two new bounds, because a one-hour per-attempt timeout is not a bound
index_maintenance_retryableand the retry classification from #208 areunchanged.
retry_index_opitself gained one thing: an opt-in wall-clockbudget.
OPENSEARCH_FORCE_MERGE_TIMEOUT(seconds, defaultmax(OPENSEARCH_TIMEOUT, 1h),0= unlimited).OPENSEARCH_TIMEOUT(300 s) isthe client-wide transport timeout, sized for queries and bulk. Sharing it would
abort every attempt client-side while the merge kept running server-side and
burn the whole retry budget on a merge that was always going to succeed. As
first written in this PR,
0resolved toDuration::ZERO, which reqwest treatsas an already-expired deadline — every attempt failing before a byte left the
process, then hard-erroring and discarding the ingest.
OPENSEARCH_FORCE_MERGE_BUDGET(seconds, default 2x the per-attemptdeadline,
0= unlimited).retry_index_opretries every transport error, so a3600 s per-attempt bound over 11 attempts (10 retries) was an ~11 h worst case.
The search path has carried a wall-clock budget for exactly this reason; the
index path did not. Only force merge opts in — create/delete/refresh stay
count-only.
Both are resolved in
OpenSearchEngine::new, not insideforce_merge. Parsingthem at the call site meant a malformed
OPENSEARCH_FORCE_MERGE_BUDGET=2h— thevery value the loud parser exists to reject — would discard a completed
multi-hour ingest at the last step.
parse_number_of_shardsresolves innew()for the same reason. It also keeps two
env::varreads out of the timed window.Retrying is cheap when it happens, now measured rather than asserted: client
aborted at 3.0 s, the server kept merging 108 s (
force_mergepoolactive=1across 36 samples), the re-issued request returned in 0.0 s. Thepool is
fixed, size=1, queue_size=-1on both engines, so a retry queues ratherthan being rejected.
The cluster-health wait
Added, mirroring
elasticsearch.rs, so the search phase starts from a settledcluster on both engines. Deviation: scoped to our index rather than the whole
cluster, because a managed domain's system indices can sit permanently unassigned
and pin cluster-wide health at yellow forever, failing a finished ingest over an
index the benchmark never touches.
Three things review corrected or hardened:
"timed_out": trueon a missedstatus — verified on OpenSearch 3.7.0 and Elasticsearch 9.4.3.
cluster_health_settledwas already right (408 is outside200..300) but thestated reason was wrong, and the 2xx-carrying variant is kept only as defence in
depth.
elasticsearch.rs's status-only check is fine in practice; that is nolonger offered as a parity argument.
control
cluster:monitor/healthis routinely denied even when index actions aregranted. The wait is the tail of
force_merge(), so a denial would have thrownaway a completed ingest over a monitoring permission — a new failure mode
introduced by adding the wait, on exactly the domains the index-scoping protects.
Cost of that mistake was backoff-bounded (~1.7–3.5 min), not the ~14.5 min an
earlier revision claimed: a 403 returns immediately, so only the sleeps count.
cluster_block_exceptionis not a denial. OpenSearch shipsdisk-watermark blocks as 403, and they clear themselves.
is_authorization_deniedinspects the body, matching its three sibling predicates in this file rather than
being the only status-only one.
Tests
cargo test --lib --bins— 806 passed, 0 failed, 0 ignored across all sixbinaries (98 + 0 + 0 + 0 + 2 + 706).
cargo clippy --all-targets -- -D warningsand
cargo fmt --checkclean. Livecargo test --test integration_opensearch --release -- --test-threads=1againstopensearchproject/opensearch:3.7.0— 17 passed, 0 failed.The integration test was a coin flip; it is now conclusive-or-loud
searchable == 1is the right invariant, but the pre-merge segment count is not aproperty of the fixture — it is however many Lucene indexing buffers happen to be
live at the single end-of-upload refresh, and 400 docs at dim 8 never fill one:
merges.total_docsAt 1 segment a bare
_forcemergeis a no-op that also leaves 1, so the assertionholds on unfixed code — reachable on a 2-vCPU runner. The test now checks
merges.total_docs, which is exact here, after the segment assertion so thatgenuinely unfixed code reports the accurate diagnosis. Reverting only
engine/opensearch.rsto master: FAILED 8/8, then 6/6 post-merge, always onthe segment assertion; this branch passes 8/8 then 6/6. Forcing a single-segment
upload yields
INCONCLUSIVE, not a pass: … merges rewrote only 0 of 400 docs.Wiring, not just predicates
Review found five mutations that broke the wiring while leaving every predicate
correct, and all five passed the suite. Each is now killed — per-mutation results
are in a comment below.
The two structural changes that did it:
classify_health_responseextracts thehealth decision so the order of its checks is pinned (swapping the auth and
retry checks flips "continue the run" into "abort a finished ingest"), and
retry_index_opis driven directly by a test with an always-failingsendand a150 ms budget.
retry_policy_budget_defaults_off_and_is_opt_inpreviously built aRetryPolicyliteral and asserted the field it had just set; it now readsindex_op_policy().Scope — what this diff actually touches
src/bin/vector_db_benchmark/engine/opensearch.rs— the change and its unit tests.tests/integration_opensearch.rs— one new test.src/bin/vector_db_benchmark/config.rs— not incidental, and it affects all 15engines:
knob_is_readno longer treats a chained.leaf(builder setter asevidence that an engine reads a config knob of that name. Without it, this PR's
.request_timeout(..)transport calls made the shipped-config guard concludeOpenSearch now reads
connection_params.request_timeoutand demand itsKNOWN_UNREADentry be deleted — retiring issue Elasticsearch/OpenSearch: connection_params.request_timeout is declared on 14 shipped configs and read by nothing #245's tracked debt as paid.An allowlist alternative was tried and reverted: mutation testing showed two
silent-green holes (deleting both entries; the engine genuinely reading the
knob), and its error message steered you into the first. The structural rule
fails loudly on both — re-verified by mutation on the restored code.
README.md— documents the two new env knobs, which were otherwise undocumented.The bulk-upload retry path (#209) and every other engine are untouched.
Follow-ups filed
elasticsearch.rsstill has the exact failure loop this fixes forOpenSearch: 30 retries, no wall-clock budget, no request-timeout override. Plus
ES 9.x's
indices.merge.disk.watermark.highhanging force merges indefinitelyabove 95% disk (a 150-doc merge sat 6.6 min), which also makes ES-side timings
from this PR's 99–100%-disk window suspect.
refresh()is redundant; dropping it is the real symmetryfix for the external-refresh count.
.parse().ok(), includingOPENSEARCH_TIMEOUT, which feeds this PR's deadline:OPENSEARCH_TIMEOUT=7200ssilently becomes 300 and halves the merge deadline.A note on "previously"
Several statements in earlier revisions of this description compared against
this PR's own first commit, not against master, which overstated the change's
apparent value. Behaviour described as changed is changed relative to master
unless the text says "as first written in this PR".
🤖 Generated with Claude Code