Skip to content

fix(exec): preserve output after stdin EOF and verify stream completion - #3359

Open
varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:feat/3056-stream-lifecycle-implementation/varshaprasad96
Open

varshaprasad96 wants to merge 2 commits into
NVIDIA:mainfrom
varshaprasad96:feat/3056-stream-lifecycle-implementation/varshaprasad96

Conversation

@varshaprasad96

Copy link
Copy Markdown
Contributor

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

  • Keep SSH input/output futures owned by the exec operation, propagate input failures, send only SSH EOF on normal request closure, and bound proxy/SSH cleanup.
  • Preserve Go's original session interface and cancellation-style Close(). Add optional InteractiveSessionControl capabilities and helpers for graceful input closure and cancellation.
  • Preserve TypeScript's original session interface and eager command startup. Expose extended lifecycle controls and use a bounded, cancellation-aware output queue; report success only after final transport status.
  • Add SSH bridge, SDK lifecycle/compatibility, and Docker-backed EOF-draining regressions. Update architecture and SDK/user documentation.

Testing

  • mise run pre-commit
  • Server library suite: 1,521 passed, 7 ignored; focused interactive-exec rerun: 16 passed.
  • mise run go:ci, including race tests.
  • mise run sdk:ts:ci: 117 tests passed, plus type-checking, lint, code generation, and build.
  • Focused mise run e2e:python with -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-keep lifecycle tests initially failed during provisioning; both the focused retry and full rerun passed without code changes.
  • Unit and E2E tests added/updated.
  • 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

  • Follows Conventional Commits.
  • Commits are signed off (DCO).
  • Architecture and relevant user/SDK documentation updated.
  • Related skill maintenance reviewed; no companion skill changes required.

@copy-pr-bot

copy-pr-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@gmenher gmenher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@varshaprasad96

varshaprasad96 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I’ve addressed the feedback:

  • Removed input priority from the duplex loop and added cooperative yielding so continuously ready input cannot starve output. Cancellation retains priority.
  • Added regression tests for sustained bidirectional traffic and continuously ready resize requests.
  • Added a live TypeScript SDK E2E test covering stdin closure, output draining, and final stream completion.
  • Rebased onto main.

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.

@varshaprasad96
varshaprasad96 force-pushed the feat/3056-stream-lifecycle-implementation/varshaprasad96 branch from c47949d to c696725 Compare September 17, 2026 20:27
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.

2 participants