fix(exec): preserve output after stdin EOF and verify stream completion - #3359
varshaprasad96 wants to merge 4 commits into
Conversation
gmenher
left a comment
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.
|
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
BlockedThanks @varshaprasad96. I checked your note that you rebased onto Gator is blocked by merge conflicts, so the independent code review cannot start on a clean current patch yet. Next action: please rebase or merge the latest Gator metadata
|
c696725 to
610dc25
Compare
|
/ok to test 610dc25 |
|
Label |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96. I checked your update about removing input priority, adding cooperative yielding, covering sustained bidirectional and resize traffic, adding the live TypeScript SDK path, and rebasing. The current head is mergeable, the independent review found no blocking issues, and the documented interactive-exec lifecycle is covered by the implementation and tests.
Action required: a maintainer must open the current-head Branch E2E Checks run and choose Re-run all jobs so the newly applied test:e2e label actually dispatches the required suite.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Non-blocking suggestions:
- None.
Gator metadata
- Validation: Project-valid as the focused interactive-exec and Go/TypeScript SDK portion of issue #3056.
- Docs: Fern user documentation, SDK documentation, and gateway architecture documentation are updated.
- Checks: DCO is green; required branch checks are not dispatched for the current head yet.
- E2E:
test:e2eis applied, and the E2E Label Help workflow requires Re-run all jobs before the suite starts. - Head SHA:
610dc2589157a2e90b0afae558f26a0fb0c97851 - Base SHA:
29e89a2f2289ad538c195e136baaaac2a92a3a2e - Merge base SHA:
29e89a2f2289ad538c195e136baaaac2a92a3a2e - Patch ID:
38c1e776eb1d67fcefcf101d3117040b57cd7fb1 - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
drew
added
gator:watch-pipeline
|
Rebasing and pushing again, to retrigger the tests |
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
610dc25 to
752063d
Compare
|
/ok to test 752063d |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96. I checked your update that you rebased again to retrigger tests. The author-only range-diff preserves the previously reviewed implementation, and the independent follow-up review found no blocking issues on the current head.
Action required: a maintainer must approve the current-head Trivy Changes workflow run so that required scan can execute. Branch Checks, Helm Lint, and the required E2E workflow are already dispatched.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Gator metadata
- Validation: Project-valid as the focused interactive-exec and Go/TypeScript SDK portion of issue #3056.
- Docs: Fern user documentation, SDK documentation, and gateway architecture documentation are updated.
- Checks: Branch Checks and Helm Lint are running; Trivy Changes is awaiting maintainer approval.
- E2E:
test:e2eis applied and the current-head Branch E2E Checks workflow is running. - Head SHA:
752063d60a5b4e2e105b6e0605c94becb2e8ecc4 - Base SHA:
cbf026366dcc8983ba959ab788e236a3ac38141f - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
15924f786c2ca18d2b2185ccccccad53ba2ae19d - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
610dc2589157a2e90b0afae558f26a0fb0c97851 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
drew
added
gator:blocked
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
/ok to test 30917ea |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96. I checked the latest test-only update, which replaces a multi-megabyte deep-equality assertion with native Buffer equality while retaining both length and byte-for-byte verification. The independent follow-up review found no blocking issues, and there are no carried findings.
Action required: the current-head Branch Checks, Helm Lint, Trivy Changes, and E2E workflows must actually dispatch. Gator has posted /ok to test for this head; if GitHub still leaves the runs awaiting approval, a maintainer must approve them.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Gator metadata
- Validation: Project-valid as the focused interactive-exec and Go/TypeScript SDK portion of issue #3056.
- Docs: Fern user documentation, SDK documentation, and gateway architecture documentation remain updated; this delta is test-only.
- Checks: DCO is green; required current-head checks are pending but their workflows are not yet queued.
- E2E:
test:e2eis applied, and/ok to testhas been posted for the current head; dispatch is not yet confirmed. - Head SHA:
30917ea32636183e53c19cb360f474d8c241f4ef - Base SHA:
cbf026366dcc8983ba959ab788e236a3ac38141f - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
7dbd92d2434de56008d4d39936ec07c338e84e1f - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
752063d60a5b4e2e105b6e0605c94becb2e8ecc4 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
/ok to test 91cf06e |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @varshaprasad96. I checked the latest test-only update, which switches the EOF regression to the workspace-scoped sandbox selector. The required critical-only follow-up review found no newly introduced Critical defect, and there are no carried findings.
Action required: a maintainer must approve the current-head Trivy Changes workflow run. Branch Checks, Helm Lint, and the required E2E workflow are already dispatched.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Gator metadata
- Validation: Project-valid as the focused interactive-exec and Go/TypeScript SDK portion of issue #3056.
- Docs: Fern user documentation, SDK documentation, and gateway architecture documentation remain updated; this delta is test-only.
- Checks: DCO is green; Branch Checks and Helm Lint are queued or running; Trivy Changes is awaiting maintainer approval.
- E2E:
test:e2eis applied, and the current-head Branch E2E Checks workflow is running. - Head SHA:
91cf06ef2b1309cc546de0fbd98c69a66ded109c - Base SHA:
cbf026366dcc8983ba959ab788e236a3ac38141f - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
26148cead33212b7b517e042674c033ba22984c7 - Gator payload:
9 - Review mode:
critical_only - Previous reviewed SHA:
30917ea32636183e53c19cb360f474d8c241f4ef - Review budget exhausted: yes
- Maintainer decision required: no — no unresolved obligations, qualifying scope growth, or proposed Warning remains.
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
3 participants
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