feat(worker-proxy): relay extension RPC streams - #11959
feat(worker-proxy): relay extension RPC streams#11959Jacob Viau (jviau) wants to merge 7 commits into
Conversation
Jacob Viau (jviau)
added
the
compute-separation
ab3e546 to
7217b43
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The current stream/session lifecycle and gRPC error-surfacing behavior has correctness/operational gaps that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the WorkerProxy-side transport for multiplexed Extension RPC, including a runtime-facing gRPC service that attaches a single physical stream to a coordinator and supports worker-originated logical calls with timeout propagation and byte-credit flow control.
Changes:
- Introduces
ExtensionRpcStreamCoordinator/ExtensionRpcStream/ExtensionCallto manage a single physical stream, logical-call routing, negotiation gating, and outbound/inbound message piping. - Adds
ExtensionRpcRelaygRPC service and registers it inWorkerProxyApplication. - Adds
ExtensionRpcTransportTestsvalidating negotiation, call opening, credit-window blocking, and session closure behavior.
File summaries
| File | Description |
|---|---|
| test/Functions.WorkerProxy.Tests/ExtensionRpcTransportTests.cs | Adds unit tests for coordinator negotiation, call routing, flow control, and relay shutdown behavior. |
| src/Functions.WorkerProxy/WorkerProxyApplication.cs | Registers the extension RPC coordinator/relay and maps the new gRPC service endpoint. |
| src/Functions.WorkerProxy/NumberExtensions.cs | Adds invariant numeric formatting helpers used for protocol IDs and timeout formatting. |
| src/Functions.WorkerProxy/ExtensionRpcStreamLease.cs | Adds an async-disposable lease to manage coordinator registration/unregistration of the physical stream. |
| src/Functions.WorkerProxy/ExtensionRpcStreamCoordinator.cs | Implements single-stream coordination, negotiation gating, call opening, and grpc-timeout propagation/formatting. |
| src/Functions.WorkerProxy/ExtensionRpcStream.cs | Implements the physical stream abstraction, outbound queueing (Hello), inbound lifecycle handling, and logical call registration. |
| src/Functions.WorkerProxy/ExtensionRpcRelay.cs | Implements the runtime-facing ExtensionRpc gRPC service and relays inbound/outbound stream messages. |
| src/Functions.WorkerProxy/ExtensionRpcCreditWindow.cs | Implements byte-credit flow-control window used by logical calls. |
| src/Functions.WorkerProxy/ExtensionCall.cs | Implements a logical call with inbound buffering and request credit reservation for Data messages. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7217b43 to
5ac0c61
Compare
5ac0c61 to
7e2043a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues (timeout exception semantics and call lifecycle cleanup) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A call deadline provided via start.Timeout is not enforced while waiting for stream negotiation, which can cause OpenExtensionCallAsync to block past (or indefinitely beyond) the intended timeout.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
a647ae5 to
3ab7660
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues in the new relay and tests (gRPC status code semantics, exception masking in cleanup, and flaky/misleading timeout assertions) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The negotiation state in ExtensionRpcStream is accessed across threads without synchronization, which can cause stale reads and incorrect readiness behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Functions.WorkerProxy/ExtensionRpc/ExtensionRpcStream.cs:30
_readyis written from the inbound relay reader task and read from other threads (e.g., coordinator/open-call paths) without any synchronization. This can lead to stale reads (e.g.,IsReadynever observing negotiation completion) or a spurious "not ready" failure even afterReadyis received. Make_readyvolatile or useVolatile.Read/Writeto establish the required memory barriers for cross-thread visibility.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The current call-open/write paths don’t reliably observe physical-stream cancellation, which can cause hangs or incorrect behavior during disconnect/reconnect scenarios.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Functions.WorkerProxy/ExtensionRpc/ExtensionCall.cs:85
- Writes can block indefinitely if the physical stream ends while waiting for flow-control credits or while queueing to the outbound channel, because this method only observes the caller-provided cancellationToken and does not also observe the stream’s cancellation token. This can hang callers that use CancellationToken.None (or a token unrelated to the stream) during disconnect/reconnect scenarios.
src/Functions.WorkerProxy/ExtensionRpc/ExtensionRpcStreamCoordinator.cs:112 - OpenExtensionCallAsync intends to retry when the transport ends (it catches OperationCanceledException when stream.CancellationToken is canceled), but the call into stream.OpenExtensionCallAsync only uses the caller’s cancellationToken. This means stream disconnects may not cancel the open/write path promptly (and the OCE retry path may never trigger), allowing calls to be opened/writes to be queued against a dead stream until it’s fully closed.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There is a build-breaking missing namespace import for ToStringInvariant() usage in ExtensionRpcStreamCoordinator.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Move the Extension RPC relay types into a dedicated folder and namespace.
926a2b0 to
70f6e84
Compare
Issue describing the changes in this PR
N/A. Preceding stack layer: #11958.
This layer adds only the WorkerProxy multiplexed extension-RPC transport and runtime-facing ExtensionRpc stream service. It ports physical-stream negotiation and lifecycle, logical-call routing, timeout propagation, byte-credit flow control, reconnect/session closure, and relay registration. It does not add extension gRPC ingress, telemetry, or host-side dispatch/catalog behavior.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.mdAdditional information
Validation:
dotnet test test\Functions.WorkerProxy.Tests\Functions.WorkerProxy.Tests.csproj --no-restore --verbosity minimaldotnet build src\Functions.WorkerProxy\Functions.WorkerProxy.csproj --no-restore --configuration Release --verbosity minimal