fix(exec): preserve output after stdin EOF and verify stream completion - #3359
varshaprasad96 wants to merge 2 commits into
Conversation
gmenher
left a comment
There was a problem hiding this comment.
Nice work on this @varsha.
I left one inline comment about possible I/O starvation in the input-prioritized select. I think this PR would also need a rebase onto current main after the recent changes.
Would it make sense to add a test for sustained simultaneous stdin plus stdout/stderr while the request stream remains open, to exercise scheduling fairness, and another SDK-to-live-gateway integration test covering input EOF, output draining, and final transport status through the public helpers? I think those could give this a bit more robustness and coverage.
| tokio::pin!(input, output); | ||
| let exchange = async { | ||
| tokio::select! { | ||
| biased; |
There was a problem hiding this comment.
I think this could cause starvation, no? Since the input branch is biased, a client that keeps sending stdin or resize messages could keep winning the select and prevent output from being polled.
That could leave stdout/stderr undrained and eventually block on SSH flow control. Would it make sense to remove the bias here, or add some fairness between the two directions?
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
Thanks for the review. I’ve addressed the feedback:
The focused tests, server suite, Go/TypeScript CI, and live E2E checks pass. The duplex regression bounds outstanding input to 256 KiB; it does not yet establish liveness under simultaneous transport-window saturation. |
c47949d to
c696725
Compare
Summary
Interactive exec now treats request EOF as the end of stdin and resize input while continuing to drain stdout/stderr and report the command's exit. Go and TypeScript helpers distinguish an observed process exit from successful final gRPC completion, retaining the exit code if the stream subsequently fails.
Related Issue
Partially addresses #3056. This is the interactive-exec and SDK portion; it does not close the issue. Relay half-close negotiation, typed relay aborts, reconnect policy, process-group termination, and general stream limits remain separate work.
Changes
Close(). Add optionalInteractiveSessionControlcapabilities and helpers for graceful input closure and cancellation.Testing
mise run pre-commitmise run go:ci, including race tests.mise run sdk:ts:ci: 117 tests passed, plus type-checking, lint, code generation, and build.mise run e2e:pythonwith-k interactive_exec: 2 passed, including the new request-EOF regression.mise run e2e:docker: conformance passed and 120 Rust tests passed on the full rerun. Two--no-keeplifecycle tests initially failed during provisioning; both the focused retry and full rerun passed without code changes.mise run ci: attempted, but the starting commit's existing macOS gateway-config test expects/bin/true. Upstream PR fix(ci): restore mise run ci on macOS #3294 fixes that test-path issue.The Docker E2E run covers the unchanged gateway/supervisor implementation and predates the subsequent SDK compatibility fixes; those fixes were validated with SDK CI.
Checklist