Skip to content

[Bugfix] Implement skipsDeserialization() in RestoringTransportResponseHandler#6154

Merged
DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
finnegancarroll:fix-stream-transport-deserialization
May 20, 2026
Merged

[Bugfix] Implement skipsDeserialization() in RestoringTransportResponseHandler#6154
DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
finnegancarroll:fix-stream-transport-deserialization

Conversation

@finnegancarroll

@finnegancarroll finnegancarroll commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Description

The security interceptor's RestoringTransportResponseHandler wraps the inner transport response handler but does not forward the skipsDeserialization() method which was added in a recent upstream core change.

This results in a mismatch between the client and server configurations, the client sending payloads over the stream has skipsDeserialization() = true while the response handling side sees skipsDeserialization() = false due to the default value in the RestoringTransportResponseHandler interface, and unknowingly opens the VectorStreamInput.forByteSerialized stream input to handle the response.

This causes the Arrow Flight stream transport responses to throw the following exception:

[2026-05-20T05:42:17,029][INFO ][o.o.a.e.QueryScheduler   ][4778b9b750ef6b705f2b6beedbfde8a9] [QueryScheduler] ExecutionGraph built:
ExecutionGraph[queryId=48f9634d-76b6-4075-9794-4f2faac14225, stages=1, leaves=1]
  Stage 0 [ShardFragmentStageExecution] state=CREATED

[2026-05-20T05:42:17,034][ERROR][o.o.a.f.t.FlightClientChannel][4778b9b750ef6b705f2b6beedbfde8a9] Exception while handling stream response
org.opensearch.transport.stream.StreamException: no such index [test-composite]
        at org.opensearch.arrow.flight.transport.FlightErrorMapper.fromFlightException(FlightErrorMapper.java:65)
        at org.opensearch.arrow.flight.transport.FlightTransportResponse.lambda$openAndPrefetchAsync$0(FlightTransportResponse.java:104)
        at java.base/java.lang.VirtualThread.run(VirtualThread.java:456)
[2026-05-20T05:42:17,034][ERROR][o.o.s.p.r.RestPPLQueryAction][4778b9b750ef6b705f2b6beedbfde8a9] Error happened during query handling (status INTERNAL_SERVER_ERROR)
java.lang.RuntimeException: Stage 0 failed
        at org.opensearch.analytics.exec.stage.ShardFragmentStageExecution$1.onFailure(ShardFragmentStageExecution.java:134)
        at org.opensearch.analytics.exec.AnalyticsSearchTransportService$1.handleException(AnalyticsSearchTransportService.java:174)
        at org.opensearch.transport.TransportService$9.handleException(TransportService.java:1831)
        at org.opensearch.security.transport.SecurityInterceptor$RestoringTransportResponseHandler.handleException(SecurityInterceptor.java:450)
        at org.opensearch.transport.TransportService$ContextRestoreResponseHandler.handleException(TransportService.java:1607)
        at org.opensearch.arrow.flight.transport.MetricsTrackingResponseHandler.handleException(MetricsTrackingResponseHandler.java:55)
        at org.opensearch.arrow.flight.transport.FlightClientChannel.safeHandleException(FlightClientChannel.java:344)
        at org.opensearch.arrow.flight.transport.FlightClientChannel.notifyHandlerOfException(FlightClientChannel.java:336)
        at org.opensearch.arrow.flight.transport.FlightClientChannel.handleStreamException(FlightClientChannel.java:311)
        at org.opensearch.arrow.flight.transport.FlightClientChannel.lambda$openStreamAndInvokeHandler$1(FlightClientChannel.java:273)
        at java.base/java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:884)
        at java.base/java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:862)
        at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:531)
        at java.base/java.util.concurrent.CompletableFuture.completeExceptionally(CompletableFuture.java:2221)
        at org.opensearch.arrow.flight.transport.FlightTransportResponse.lambda$openAndPrefetchAsync$0(FlightTransportResponse.java:104)
        at java.base/java.lang.VirtualThread.run(VirtualThread.java:456)
Caused by: org.opensearch.transport.stream.StreamException: no such index [test-composite]
        at org.opensearch.arrow.flight.transport.FlightErrorMapper.fromFlightException(FlightErrorMapper.java:65)
        ... 2 more

Issues Resolved

N/A

Testing

N/A

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 2de4fad)

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

Null SecurityInterceptor

The SecurityInterceptor is instantiated with all null dependencies except the last two parameters. If the RestoringTransportResponseHandler or any method it calls attempts to access these null fields (e.g., threadPool for getThreadContext()), a NullPointerException will occur. While the current test only verifies delegation of interface methods without triggering context restoration, this creates a fragile test that may break if the implementation changes to use any of these dependencies during construction or method calls.

SecurityInterceptor interceptor = new SecurityInterceptor(
    null,
    null,
    null,
    null,
    null,
    null,
    null,
    null,
    null,
    null,
    () -> false,
    null
);
return interceptor.new RestoringTransportResponseHandler<>(innerHandler, restorableContext);

@finnegancarroll
finnegancarroll force-pushed the fix-stream-transport-deserialization branch from 0f991d3 to aea5480 Compare May 20, 2026 15:43
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit aea5480

@cwperks

cwperks commented May 20, 2026

Copy link
Copy Markdown
Member

TY @finnegancarroll, do you think we should consider adding a test like https://github.com/opensearch-project/security/blob/main/src/test/java/org/opensearch/security/filter/DelegatingRestHandlerTests.java to ensure that all default methods from the interface are properly overridden?

inner transport response handler but does not forward the
skipsDeserialization() method. This causes the Arrow Flight stream
transport to fall back to byte serialization for responses that should
use zero-copy Arrow vector transfer.

Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
@finnegancarroll
finnegancarroll force-pushed the fix-stream-transport-deserialization branch from aea5480 to 7ad6d7c Compare May 20, 2026 16:28
@finnegancarroll

Copy link
Copy Markdown
Collaborator Author

good idea @cwperks , added a unit test to compare the wrapped / non wrapped interfaces & output.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 7ad6d7c

Signed-off-by: Finn Carroll <carrofin@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2de4fad

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.01%. Comparing base (09de157) to head (2de4fad).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6154      +/-   ##
==========================================
+ Coverage   74.97%   75.01%   +0.03%     
==========================================
  Files         453      452       -1     
  Lines       29112    29107       -5     
  Branches     4386     4382       -4     
==========================================
+ Hits        21828    21835       +7     
+ Misses       5257     5245      -12     
  Partials     2027     2027              
Files with missing lines Coverage Δ
...search/security/transport/SecurityInterceptor.java 79.23% <100.00%> (+0.11%) ⬆️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@DarshitChanpura
DarshitChanpura merged commit 324a038 into opensearch-project:main May 20, 2026
113 of 114 checks passed
terryquigleysas pushed a commit to terryquigleysas/security that referenced this pull request May 21, 2026
…seHandler (opensearch-project#6154)

Signed-off-by: Finn Carroll <carrofin@amazon.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
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.

3 participants