john-mlika opened a new issue, #16419:
URL: https://github.com/apache/lucene/issues/16419
### Description
On `branch_10x`, `KnnVectorsReader#finishMerge()` is dead code: nothing in
the branch calls it.
The revert half of the merge read-advice machinery is disconnected, so every
merge of a vector
format that propagates `getMergeInstance()` — plain float/HNSW fields via
`Lucene99FlatVectorsReader`/`Lucene99HnswVectorsReader` — flips the
**shared, search-visible** raw
`.vec` `IndexInput` to `DataAccessHint.SEQUENTIAL`
(`Lucene99FlatVectorsReader#getMergeInstance` →
`vectorData.updateIOContext(...)`, which mutates and returns `this`) and
**nothing ever flips it
back**. The advice sticks until the segment reader is closed.
This is not a bug on `main`.
### How the branch got here
- #14977 (2025-09-17) fixed exactly this problem on `main` — its own
rationale was *"ReadAdvice not
being reset properly"* — by moving the cascade initiation into
`SegmentMerger#cleanupMerge()`,
invoked from both IndexWriter call sites inside `finally` blocks (on main:
`IndexWriter.java:3544-3545` addIndexes path, `:5356-5357` merge path;
`SegmentMerger.java:331-337`
iterates `mergeState.knnVectorsReaders` calling `finishMerge()`). #14977
was never backported to
`branch_10x`.
- Commit `d1c4db43036` ("Prevent writing vectors twice during merging HNSW
graphs", 2026-04-28 — the
`branch_10x` backport of #15732) **deleted the previous caller**: the
private
`finishMerge(MergeState)` helper and its `reader.finishMerge()` loop in
`KnnVectorsWriter#merge()`,
without the #14977 replacement existing on the branch. It is an ancestor
of the
`releases/lucene/10.5.0` tag, so 10.5.0 shipped with it.
- Net: since `d1c4db43036`, `branch_10x` has the advice-flip side
(`getMergeInstance`) fully wired
and the advice-revert side (`finishMerge`) unreachable. Verified against
the released
`lucene-core-10.5.0.jar`: a jar-wide search finds `finishMerge` only on
`KnnVectorsReader` and its
implementors (`Lucene99FlatVectorsReader`, `Lucene99HnswVectorsReader`,
`PerFieldKnnVectorsFormat$FieldsReader`) — no call site.
### Impact
For any 10.5.x deployment using mmap and float vector fields (the advice
machinery is a no-op on
Directory implementations that ignore read advice):
- **During a merge** (intended, same as main): concurrent searches on the
merging segments read raw
vectors with SEQUENTIAL advice — readahead-friendly, random-access-hostile.
- **After the merge** (the regression): the flip is never reverted, so
SEQUENTIAL advice persists on
the *pooled* segment readers that searches share, for as long as those
readers live — until the
source segments are dropped *and* every NRT reader referencing them cycles
out. For merges that
fail or abort, and for segments that survive as sources of aborted merges,
the mis-advice persists
until segment close.
- Downstream, Elasticsearch 9.x ships Lucene 10.5.0, so mmap-served float
vector fields there
inherit this today.
One related note: quantized readers are currently shielded from this by a
separate gap — they don't
override `getMergeInstance` at all, so the flip never reaches them. I have a
change ready that closes
that gap; it seems worth resolving this issue on `branch_10x` before that
propagation is backported,
otherwise quantized fields would join the regression.
### Proposed fix
Backport #14977 to `branch_10x` (restore `SegmentMerger#cleanupMerge()` plus
the two
`finally`-covered IndexWriter invocations), or fold an equivalent
restoration into the next vector
backport touching the area. By #14977's own framing this is a bug fix, so it
seems to qualify for a
10.5.x patch release independently of any optimization work.
One reason this hasn't shown up in CI: the test framework's own invariant is
weaker on this branch.
`AssertingKnnVectorsFormat` closes with
`assert finishMergeCount.get() <= 0 || mergeInstanceCount.get() ==
finishMergeCount.get();` on
`branch_10x`, whereas `main` tightened it to a strict `assert
mergeInstanceCount.get() ==
finishMergeCount.get();` in the very commit that fixed this (`085a0ae054b`,
#14977). So on
`branch_10x` the `AssertingCodec` cannot catch the regression either.
### Reproducing it
I wrote a detector (`TestMergeReadAdviceRevert`, identical source on both
branches) that indexes two
segments, holds an NRT reader open across `forceMerge(1)`, and records every
`updateIOContext` per
file through a recording `FilterDirectory`. It asserts that each `.vec`
switched to SEQUENTIAL during
the merge is switched back before its input closes. On `branch_10x` it fails:
```
The following .vec files were switched to DataAccessHint.SEQUENTIAL for the
merge and were
never switched back (KnnVectorsReader#finishMerge() was never invoked):
_0_Lucene99HnswVectorsFormat_0.vec [ADVICE:SEQUENTIAL]
_1_Lucene99HnswVectorsFormat_0.vec [ADVICE:SEQUENTIAL]
```
One flip, no revert, and both source inputs are still open (pinned by the
NRT reader) when the
assertion runs. The same test passes on `main`.
Three constraints matter, or the detector silently false-passes:
- the field must be a **plain float HNSW** field — on `branch_10x` the
quantized readers don't
override `getMergeInstance` at all, so a quantized detector fails for the
wrong reason;
- an **NRT reader must be held open across the merge** — a merge-created
pooled reader is opened
with `IOContext.merge(...)`, whose `withHints` is a no-op, so the flip
never becomes observable;
- the recording `IndexInput` must explicitly delegate `updateIOContext`,
`clone()` and both `slice`
overloads (`FilterIndexInput` forwards none of them).
Happy to open the backport PR and contribute the test if it would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
