Skip to content

Fix flaky BWC knn query serialization by gating on stream version - #3423

Open
vlmkoa wants to merge 1 commit into
opensearch-project:mainfrom
vlmkoa:fix-issue-1622
Open

Fix flaky BWC knn query serialization by gating on stream version#3423
vlmkoa wants to merge 1 commit into
opensearch-project:mainfrom
vlmkoa:fix-issue-1622

Conversation

@vlmkoa

@vlmkoa vlmkoa commented Jul 12, 2026

Copy link
Copy Markdown

What does this PR do?

Gates KNNQueryBuilder's optional-field serialization on the transport stream version
(StreamInput#getVersion() / StreamOutput#getVersion()) instead of the cluster
minimum version, so the writing and reading nodes always agree on the wire format during
a rolling upgrade. Also fixes the two internal delegations to MethodParametersParser
in KNNQueryBuilderParser, which were hardcoding the cluster-version predicate instead
of forwarding the one passed in.

Why was this PR needed?

During a rolling upgrade, cluster state propagates asynchronously, so two communicating
nodes can transiently hold different views of KNNClusterUtil#getClusterMinVersion().
The writer and reader then disagree on whether a given optional field is present on the
wire, desyncing the byte stream. A later readBoolean() in SearchSourceBuilder lands
on a stray byte, producing the flaky unexpected byte [0x05] 500 error reported in #1622.

The fix uses the transport stream version instead, which is negotiated per connection and
is always identical on both ends — matching the pattern already used by RescoreParser
in the same package.

What are the relevant issue numbers?

Closes #1622

Does this PR meet the acceptance criteria?

  • Tests added for new/changed behavior (KNNQueryBuilderRollingUpgradeSerializationTests)
  • All tests passing
  • Follows project style guide
  • No breaking changes introduced
  • Changelog entry added

Gate optional-field serialization on the transport
  stream version instead of the cluster minimum version, so writer and reader always agree during rolling upgrades and the byte stream no longer
  desyncs (unexpected byte [0x05]). Matches the existing RescoreParser pattern. Fixes opensearch-project#1622

Signed-off-by: Ryan <minhkhoavole@gmail.com>
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Accidentally removed changelog entry

The existing bug-fix entry "Check to see if Lucene's search budget has exhausted when deciding to exact search in MOS [#3354]" was removed from the Bug Fixes section. This appears to be an unintended deletion unrelated to this PR's purpose and should be restored alongside the new entry.

### Bug Fixes
* Fix flaky BWC serialization failure for knn queries during rolling upgrades by gating optional fields on the transport stream version instead of cluster minimum version [#1622](https://github.com/opensearch-project/k-NN/issues/1622)
* Turn off ACORN for MOS to match default Lucene HNSW behavior [#3346](https://github.com/opensearch-project/k-NN/pull/3346)
* Preserve mixed-case derived source vector field names and add backward-compatible field resolution for previously lowercased segment metadata [#3313](https://github.com/opensearch-project/k-NN/pull/3313)
* Fix rescore flag not propagating over transport layer in multi-node clusters [#3343](https://github.com/opensearch-project/k-NN/pull/3343)
* Integrated proper ef_search functionality into MOS and Lucene with oversample_factor [#3331](https://github.com/opensearch-project/k-NN/pull/3331)
* Fix skip warm up in old indices when MOS is enabled [#3344](https://github.com/opensearch-project/k-NN/pull/3344)

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Do not silently drop user-provided field

This XContent parser still gates the ignore_unmapped field on the cluster-min
version. Since this parses user-supplied JSON (not transport stream), it silently
drops the flag while the cluster is mid-upgrade, causing inconsistent query
semantics. Either always accept the flag or document/behave-consistently, but do not
conditionally ignore a user-provided value based on transient cluster state.

src/main/java/org/opensearch/knn/index/query/parser/KNNQueryBuilderParser.java [76-80]

-internalParser.declareBoolean((b, v) -> {
-    if (isClusterOnOrAfterMinRequiredVersion("ignore_unmapped")) {
-        b.ignoreUnmapped(v);
-    }
-}, IGNORE_UNMAPPED_FIELD);
+internalParser.declareBoolean(KNNQueryBuilder.Builder::ignoreUnmapped, IGNORE_UNMAPPED_FIELD);
Suggestion importance[1-10]: 4

__

Why: The suggestion identifies a legitimate concern about ignore_unmapped being conditionally parsed based on cluster min version, but this behavior is pre-existing and outside the scope of the PR which specifically addresses transport-stream serialization. Removing the version gate could also break BWC parsing semantics intentionally introduced originally.

Low

@vlmkoa

vlmkoa commented Jul 12, 2026

Copy link
Copy Markdown
Author

Hi @jmazanec15. I have investigated the issue. The root cause is that the optional field serialization of KNNQueryBuilder is controlled by the cluster minimum version rather than the transport stream version. I fixed it by let StreamInput#getVersion()/StreamOutput#getVersion() instead, matching the pattern that RescoreParser uses. I also added a regression test that isolates the divergence by holding the stream version fixed while varying the mocked cluster-min view.
Would appreciate a review when you have time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BWC tests failing for org.opensearch.knn.bwc.IndexingIT.testKNNDefaultIndexSettings

2 participants