Skip to content

feat(sdk): add PTY (interactive terminal) support to Go SDK#815

Open
chaojixinren wants to merge 1 commit into
TencentCloud:masterfrom
chaojixinren:feat/go-sdk-pty
Open

feat(sdk): add PTY (interactive terminal) support to Go SDK#815
chaojixinren wants to merge 1 commit into
TencentCloud:masterfrom
chaojixinren:feat/go-sdk-pty

Conversation

@chaojixinren

Copy link
Copy Markdown
Contributor

Summary

Adds PTY (pseudo-terminal) support to the Go SDK (sdk/go), closing the
cross-SDK parity gap called out in #811. Go users can now run interactive
programs that need a real TTY — shells/REPLs, full-screen tools (vim,
top), agent-driven terminals — mirroring the Python (sandbox.pty,
#579) and Node (sandbox.pty, #792) surface.

What's added

sandbox.Pty() namespace over envd's Connect-JSON RPC:

  • Streaming RPCsCreate (/process.Process/Start, runs /bin/bash -i -l) and Connect (/process.Process/Connect, reattach by PID).
  • Selector RPCsKill (SendSignal + SIGNAL_SIGKILL),
    SendStdin (SendInput), Resize (Update).
  • PtyHandle — streams raw output via Output() (channel) or
    Wait(onData); exposes PID() / ExitCode() / ErrorMessage();
    Disconnect() to detach without killing; and per-handle
    Kill/SendStdin/Resize.

Behavior matches Python/Node:

  • Seeds TERM=xterm-256color, LANG/LC_ALL=C.UTF-8 (caller-overridable).
  • Kill returns false (not an error) on Connect not_found / HTTP 404.
  • timeout is sent to envd as Connect-Timeout-Ms and enforced
    client-side as an idle abort that resets on every frame (default 60s).
  • Wait surfaces envd's end-event error (e.g. signal: killed) and parses
    exit codes from exitCode/exit_code/status.

Testing

  • Unit (sdk/go/pty_test.go, httptest-mocked envd): create/connect,
    kill (success + 404 + Connect not_found body + error), sendStdin,
    resize, wait error propagation, handle→PID delegation, disconnect, and
    idle timeout. gofmt/go vet clean; go test -race ./... green.
  • Live E2E against a real CubeSandbox: create → echo roundtrip →
    resize verified via stty size readback (40 120) → disconnect +
    reattach-by-PID → kill (Wait reports signal: killed) → second kill
    reports not-found. All pass.

Scope

Changes are confined to sdk/go/pty.go, sdk/go/pty_test.go, and the PTY
section of sdk/go/README.md. No shared code is touched — the PTY reuses
the existing newEnvdRequest/Connect-framing helpers.

Closes #811

@chaojixinren chaojixinren requested a review from tinklone as a code owner July 8, 2026 10:38
Comment thread sdk/go/pty.go Outdated
Comment thread sdk/go/pty.go
Comment thread sdk/go/pty.go Outdated
Comment thread sdk/go/pty.go
Comment thread sdk/go/pty.go
Comment thread sdk/go/pty.go
Comment thread sdk/go/README.md Outdated
@cubesandboxbot

cubesandboxbot Bot commented Jul 8, 2026

Copy link
Copy Markdown

PTY Go SDK — Code Review

Overview

A well-structured, idiomatic Go PTY implementation that mirrors the Python/Node SDK surfaces. Seven inline comments have been posted for specific issues; this comment summarizes all findings and adds context.


Inline Comments Summary

# Severity File Issue
1 High README.md:92 Primary code example uses both Output() and Wait(), violating its own documented rule at line 128 — output chunks are split unpredictably between the two consumers.
2 High pty.go:500 ptyStreamControl.reset() stops + re-allocates a time.AfterFunc per frame. Use Timer.Reset() to reuse instead.
3 High pty.go:489 Race between reset() and onFire() — timer fires between Stop() (returns false) and the new AfterFunc. Old callback then spuriously cancels the stream.
4 Medium pty.go:266 h.body double-closed from both Disconnect() and readLoop() defer. Safe today with *http.Response.Body but not guaranteed by io.ReadCloser.
5 Medium pty.go:321 TOCTOU race — idleFired() checked outside h.mu lock. Descriptive timeout error can be replaced by raw I/O error.
6 Medium pty.go:550 exitCodeFromStatus() completely untested; -? in regex accepts negative exit codes (Unix codes are 0–255).
7 Low pty.go:581 Compressed frame, end-stream-with-error, and base64-decode-failure error paths untested.

Additional Findings (not inlined)

Test Coverage Gaps (Medium)

  • exitCodeFromStatus (pty.go:558–576): The regex-based exit-code parsing — mirroring Python/Node fallback logic — has zero test coverage. Every test end event uses the exitCode JSON field, so the status-based fallback in recordEnd is exercised only via dead code. Add a table-driven unit test covering all branches.
  • End events with status field (not exitCode): No test verifies the fallback path in recordEnd (pty.go:309–310) or signal-to-128+N conversion.
  • Output() channel (pty.go:187): The primary streaming API is untested — no test ranges over Output() directly.
  • SendStdin/Resize error responses: Only HTTP 200 paths are tested.
  • Connect error responses: No equivalent of TestPtyStartHTTPError.
  • Context cancellation: No test cancels the caller's context.Context and verifies the result.
  • handle.Kill/handle.Resize delegation: Only SendStdin delegation is verified by the delegation test.

Performance (Medium)

  • Output channel buffer (pty.go:396, cap=64): For bursty output (e.g., find / -name '*.go'), a buffer of 64 can cause TCP-level backpressure when the consumer is slower. Consider raising to 256–512.
  • Per-frame allocations: Every data frame incurs three heap allocations — readConnectEnvelope payload buffer, json.Unmarshal of the processStartResponse struct tree, and base64.DecodeString. The envelope buffer could use sync.Pool as a practical optimization.

Minor

  • Nil-guard boilerplate (pty.go:78,115,129,139,151): Could be extracted into a requireSandbox() error helper.
  • io.ReadAll error path (pty.go:433): Error body read in unary is unbounded — use io.LimitReader (e.g., 64 KiB) as defense-in-depth.
  • PtyHandle resource leak (pty.go:162): If a caller abandons a PtyHandle without calling Wait/Disconnect, the readLoop goroutine and HTTP connection leak.
  • ExitCode() docs (README.md:125): Boolean return "not yet known" is implied but should be explicit: "exit code (0, false if not yet known)".
  • PtyCreateOptions.User/Cwd (README.md:117): Documented in Go doc comments but absent from the README table.

Positive Highlights

  • ptyStreamControl cleanly separates timeout, disconnect, and frame-receipt concerns.
  • openStream correctly assembles Connect envelopes and cleans up on every error path — no resource leaks traced.
  • sync.Once in Disconnect prevents double-cancel of the context.
  • isConnectNotFound correctly handles both HTTP 404 and the Connect {"code":"not_found"} JSON body.
  • Test suite is well-structured with t.Helper(), table-driven subtests for Kill, and clean defer server.Close().
  • Go vet and -race clean; live E2E tests pass.

chaojixinren added a commit to chaojixinren/CubeSandbox that referenced this pull request Jul 8, 2026
newEnvdRequest — the shared request builder for every envd data-plane
RPC (commands, files, filesystem, and the PTY added in TencentCloud#815) — hardcoded
the Jupyter port (49999) as the virtual host prefix. CubeProxy routes by
that prefix, so these requests reached the Jupyter kernel gateway, which
does not serve /process.Process/*, /files, or /filesystem.Filesystem/*,
yielding a non-2xx error (HTTP 404 or 502, depending on the template's
Jupyter upstream) against real deployments. Only RunCode (/execute)
worked, since /execute genuinely lives on the Jupyter port.

Add a dedicated EnvdPort (49983) constant and use it in newEnvdRequest,
matching the Python (ENVD_PORT) and Node SDKs; RunCode's /execute stays
on JupyterPort. Update the affected unit-test host assertions and the
README proxy example.

Verified against a live CubeSandbox: /files read/write and
filesystem.Filesystem/* now succeed on 49983 (previously non-2xx on
49999).

Closes TencentCloud#816

Signed-off-by: chaojixinren <chaoji_xinren@163.com>
Add sandbox.Pty() mirroring the Python/Node pty API: Create/Connect
streaming RPCs plus Kill/SendStdin/Resize selector RPCs over envd's
Connect-JSON protocol. PtyHandle streams raw output, exposes Wait for
the exit code (surfacing envd end errors such as "signal: killed"),
and supports Disconnect to detach and later reattach via Connect.
Includes a client-side idle timeout, exit-code-from-status parsing,
and httptest-based unit tests.

Closes TencentCloud#811

Signed-off-by: chaojixinren <chaoji_xinren@163.com>
chaojixinren added a commit to chaojixinren/CubeSandbox that referenced this pull request Jul 8, 2026
newEnvdRequest — the shared request builder for every envd data-plane
RPC (commands, files, filesystem, and the PTY added in TencentCloud#815) — hardcoded
the Jupyter port (49999) as the virtual host prefix. CubeProxy routes by
that prefix, so these requests reached the Jupyter kernel gateway, which
does not serve /process.Process/*, /files, or /filesystem.Filesystem/*,
yielding a non-2xx error (HTTP 404 or 502, depending on the template's
Jupyter upstream) against real deployments. Only RunCode (/execute)
worked, since /execute genuinely lives on the Jupyter port.

Add a dedicated EnvdPort (49983) constant and use it in newEnvdRequest,
matching the Python (ENVD_PORT) and Node SDKs; RunCode's /execute stays
on JupyterPort. Update the affected unit-test host assertions and the
README proxy example.

Verified against a live CubeSandbox: /files read/write and
filesystem.Filesystem/* now succeed on 49983 (previously non-2xx on
49999).

Closes TencentCloud#816

Signed-off-by: chaojixinren <chaoji_xinren@163.com>
ls-ggg pushed a commit that referenced this pull request Jul 9, 2026
newEnvdRequest — the shared request builder for every envd data-plane
RPC (commands, files, filesystem, and the PTY added in #815) — hardcoded
the Jupyter port (49999) as the virtual host prefix. CubeProxy routes by
that prefix, so these requests reached the Jupyter kernel gateway, which
does not serve /process.Process/*, /files, or /filesystem.Filesystem/*,
yielding a non-2xx error (HTTP 404 or 502, depending on the template's
Jupyter upstream) against real deployments. Only RunCode (/execute)
worked, since /execute genuinely lives on the Jupyter port.

Add a dedicated EnvdPort (49983) constant and use it in newEnvdRequest,
matching the Python (ENVD_PORT) and Node SDKs; RunCode's /execute stays
on JupyterPort. Update the affected unit-test host assertions and the
README proxy example.

Verified against a live CubeSandbox: /files read/write and
filesystem.Filesystem/* now succeed on 49983 (previously non-2xx on
49999).

Closes #816

Signed-off-by: chaojixinren <chaoji_xinren@163.com>
@ls-ggg ls-ggg added the test-needed Awaiting test info label Jul 9, 2026
@ls-ggg

ls-ggg commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pr,Could you add some test results here?

@chaojixinren

chaojixinren commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Here are the actual test results, run on the rebased branch.@ls-ggg

Base branch: latest master, which now includes the #821 envd-port fix.

Unit tests — go test -race ./... (sdk/go)

gofmt -l is clean, and go vet ./... produces no output.

PTY-related test results:

=== RUN   TestPtyKill
--- PASS: TestPtyKill (0.01s)
    --- PASS: TestPtyKill/success (0.00s)
    --- PASS: TestPtyKill/http_404_is_not_found (0.00s)
    --- PASS: TestPtyKill/connect_not_found_body_is_not_found (0.00s)
    --- PASS: TestPtyKill/other_error_propagates (0.00s)
--- PASS: TestPtySendStdin (0.00s)
--- PASS: TestPtyResize (0.00s)
--- PASS: TestPtyHandleDelegatesToHandlePID (0.00s)
--- PASS: TestPtyIdleTimeout (0.15s)
--- PASS: TestPtyDisconnectKeepsStreamOpen (0.00s)
--- PASS: TestExitCodeFromStatus (0.00s)
    --- PASS: TestExitCodeFromStatus/empty (0.00s)
    --- PASS: TestExitCodeFromStatus/exit_status (0.00s)
    --- PASS: TestExitCodeFromStatus/exited_with_code (0.00s)
    --- PASS: TestExitCodeFromStatus/signal (0.00s)
    --- PASS: TestExitCodeFromStatus/terminated_by_signal (0.00s)
    --- PASS: TestExitCodeFromStatus/plain_exited (0.00s)
    --- PASS: TestExitCodeFromStatus/negative_parity (0.00s)
    --- PASS: TestExitCodeFromStatus/unrecognized (0.00s)
--- PASS: TestPtyRecordEndFallbacks (0.01s)
    --- PASS: TestPtyRecordEndFallbacks/status_string (0.00s)
    --- PASS: TestPtyRecordEndFallbacks/exited_flag_only (0.00s)
--- PASS: TestPtyOutputChannelStreaming (0.00s)
--- PASS: TestPtyCompressedFrameError (0.00s)
--- PASS: TestPtyEndStreamTrailerError (0.00s)
--- PASS: TestPtyBase64DecodeError (0.00s)
--- PASS: TestPtyStartStreamClosedBeforeStart (0.00s)
--- PASS: TestPtyConnectHTTPError (0.00s)
--- PASS: TestPtySendStdinAndResizeErrors (0.00s)
--- PASS: TestPtyHandleKillResizeDelegation (0.00s)
--- PASS: TestPtyContextCancellation (0.00s)
PASS
ok      github.com/tencentcloud/CubeSandbox/sdk/go    1.265s

Live E2E — full PTY lifecycle against a real CubeSandbox

Covered lifecycle:

create → echo roundtrip → resize verified via stty size readback → detach → reattach by PID → kill → second kill returns not-found

=== RUN   TestIntegrationPtyLifecycle
    PTY started, pid=11
    echo roundtrip OK (saw __MARK__42__)
    resize verified: stty size -> "40 120"
    disconnected from pid=11 (process still alive)
    reattached to pid=11
    reattached.Wait -> code=-1 err=PTY exited with error: signal: killed exitKnown=true endErr="signal: killed"
    second Kill correctly reported not-found (false)
--- PASS: TestIntegrationPtyLifecycle (1.04s)
PASS
ok      github.com/tencentcloud/CubeSandbox/sdk/go    1.051s

Test environment:

api=127.0.0.1:13000
proxy=127.0.0.1:11080
domain=cube.app

@ls-ggg

ls-ggg commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

LGTM @tinklone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-needed Awaiting test info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Add PTY support to Go SDK

3 participants