xds: Fix half-close race condition in ext-proc client interceptor#12888
Merged
kannanjgithub merged 2 commits intoJul 1, 2026
grpc:mastergrpc/grpc-java:masterfrom
kannanjgithub:ext-proc-client-fix-testskannanjgithub/grpc-java:ext-proc-client-fix-testsCopy head branch name to clipboard
Merged
xds: Fix half-close race condition in ext-proc client interceptor#12888kannanjgithub merged 2 commits intogrpc:mastergrpc/grpc-java:masterfrom kannanjgithub:ext-proc-client-fix-testskannanjgithub/grpc-java:ext-proc-client-fix-testsCopy head branch name to clipboard
kannanjgithub merged 2 commits into
grpc:mastergrpc/grpc-java:masterfrom
kannanjgithub:ext-proc-client-fix-testskannanjgithub/grpc-java:ext-proc-client-fix-testsCopy head branch name to clipboard
Conversation
Resolve flakiness in ExternalProcessorClientInterceptorTest's
givenFailureModeAllowTrue_whenExtProcStreamFails_thenCallFailsOpen test by
eliminating two subtle race conditions between the test thread and the
asynchronous mock sidecar thread.
1. First Race (Premature Body Send): The mock sidecar failed asynchronously on
headers, racing with the client thread sending the body message. If the
message was sent first, fail-open was disabled due to data loss. Fixed by
intercepting the data plane server to block the client thread using a
headersReceivedLatch until the data plane call has been activated.
2. Second Race (Incorrect Frame Order): When the client thread woke up from
headersReceivedLatch, it immediately sent message and half-close. However,
the sidecar's async error thread was still busy executing activateCall() and
had not yet set passThroughMode to true. This caused the message to be
buffered while half-close bypassed the buffer, violating the gRPC frame
order (sending message after half-close) and resulting in dropped messages.
Fixed by:
- Configuring the ext-proc channel to use a single-thread executor instead
of directExecutor().
- Synchronizing the client thread using a no-op task on the single-thread
executor, guaranteeing the onError callback task has fully completed
executing handleFailOpen() and passThroughMode is true before sending
the message.
kannanjgithub
force-pushed
the
ext-proc-client-fix-tests
branch
from
July 1, 2026 07:29
a3bb441 to
8730fa9
Compare
sauravzg
approved these changes
Jul 1, 2026
| proxyCall.cancel("Cleanup", null); | ||
| channelManager.close(); | ||
| } finally { | ||
| extProcChannelExecutor.shutdown(); |
Contributor
There was a problem hiding this comment.
nit: Is this enough or should we also await thread termination?
Contributor
Author
There was a problem hiding this comment.
In the previous workaround, we created a thread pool using Executors.newSingleThreadExecutor() specifically for that test, which meant we had to clean it up using .shutdown().
In the new redesigned test, we do not instantiate any thread pools or ExecutorService objects. Instead, the channel uses directExecutor() (which executes tasks synchronously on the calling thread), so there is no executor resources or threads to shut down.
|
|
||
| ManagedChannel dataPlaneChannel = grpcCleanup.register( | ||
| InProcessChannelBuilder.forName(dataPlaneServerName).directExecutor().build()); | ||
| final CountDownLatch closedLatch = new CountDownLatch(1); |
Contributor
There was a problem hiding this comment.
Is this dead code? Nothing seems to be waiting for closedLatch
Contributor
Author
There was a problem hiding this comment.
I have updated the test to use closedLatch and assert that the client-side call completes successfully with an OK status.
Contributor
|
The second race sounds like a code bug rather than a test bug. If the client sends "message" followed by "half close". I'd expect either both to be buffered or both to be pass throughed. Or at least pending message to be drained before the half close is allowed to passthrough. |
Contributor
Author
Agree, good catch. Fixed. |
Resolve a protocol violation bug in ExternalProcessorClientInterceptor and fix a related data race in its fail-open unit tests: 1. Interceptor Bug Fix (Incorrect Frame Order): When the external processor stream fails (extProcStreamState is FAILED/completed) but the interceptor has not yet transitioned to pass-through mode, a call to sendMessage() is buffered, but a call to halfClose() immediately executed super.halfClose() because it only checked stream completion. This sent the halfClose frame to the backend server before the buffered message was drained, violating the gRPC protocol and causing the backend to drop the message. Fixed by modifying halfClose() to only execute immediately if passThroughMode is true. If passThroughMode is false and the stream is completed/failed, halfClose is buffered (pendingHalfClose = true) and deferred until the draining task drains the buffered messages and triggers it. 2. Test Fix (Deterministic Race Simulation): Redesigned givenFailureModeAllowTrue_whenExtProcStreamFails_thenCallFailsOpen in ExternalProcessorClientInterceptorTest to deterministically simulate the interleaving that caused the bug. Used a custom ServerInterceptor on the backend server to suspend the async onError thread inside activateLine() (midway through fail-open processing) while the client main thread calls sendMessage() and halfClose(). This forces the race condition and verifies that the message is no longer dropped.
kannanjgithub
force-pushed
the
ext-proc-client-fix-tests
branch
from
July 1, 2026 15:35
f2c0c02 to
2739ee2
Compare
sauravzg
approved these changes
Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve a protocol violation bug in ExternalProcessorClientInterceptor
and fix a related data race in its fail-open unit tests:
Interceptor Bug Fix (Incorrect Frame Order):
When the external processor stream fails (extProcStreamState is FAILED/completed)
but the interceptor has not yet transitioned to pass-through mode, a call to
sendMessage() is buffered, but a call to halfClose() immediately executed
super.halfClose() because it only checked stream completion. This sent the
halfClose frame to the backend server before the buffered message was drained,
violating the gRPC protocol and causing the backend to drop the message.
Fixed by modifying halfClose() to only execute immediately if passThroughMode
is true. If passThroughMode is false and the stream is completed/failed,
halfClose is buffered (pendingHalfClose = true) and deferred until the draining
task drains the buffered messages and triggers it.
Test Fix (Deterministic Race Simulation):
Redesigned givenFailureModeAllowTrue_whenExtProcStreamFails_thenCallFailsOpen
in ExternalProcessorClientInterceptorTest to deterministically simulate the
interleaving that caused the bug. Used a custom ServerInterceptor on the
backend server to suspend the async onError thread inside activateLine()
(midway through fail-open processing) while the client main thread calls
sendMessage() and halfClose(). This forces the race condition and verifies
that the message is no longer dropped.