feat(backend): PTY-attach terminal streaming over /mux WebSocket#50
Conversation
Add internal/terminal: a transport-agnostic feature package that attaches a PTY to a session's tmux pane and multiplexes the byte stream to WebSocket clients, plus a session-state channel fed by the CDC broadcaster. The PTY is reached through a small PTYSource interface (satisfied by the tmux runtime adapter) and spawned via an injectable spawnFunc, so fan-out, the 50KB replay ring, and re-attach resilience all test without a real process, tmux, or network. A tmux-guarded integration test exercises the real creack/pty path end-to-end. Raw PTY bytes never touch the CDC change_log; only the sessions channel is CDC-fed. Windows PTY spawning is stubbed pending a ConPTY path.
Add the /mux route: httpd performs the WebSocket upgrade (coder/websocket) and adapts the connection to terminal.wsConn via wsjson, then hands it to terminal.Manager.Serve. httpd owns only the upgrade and transport adaptation; all stream logic stays in internal/terminal. The route is mounted outside the per-request Timeout middleware (the connection is long-lived) and is omitted entirely when no manager is wired, so the daemon degrades to no terminal surface rather than failing. New/ NewRouter take the manager; main.go passes nil until commit 3 wires it. mux_test.go drives the real upgrade + wsjson + Serve + creack/pty path with a throwaway shell command, so it needs no tmux.
Construct the tmux runtime and a terminal.Manager fed by the CDC broadcaster, and hand it to httpd.New so the /mux WebSocket surface goes live. httpd.New now runs after the CDC substrate so the broadcaster exists when the manager subscribes; the listener still binds before running.json is written, preserving fail-fast on port conflict. The manager is closed on shutdown alongside the CDC pipeline and lifecycle stack.
87ae6ac to
4d90d59
Compare
Greptile SummaryThis PR ports the terminal-streaming surface from the legacy TypeScript/Node stack to Go, landing as a single multiplexed
Confidence Score: 5/5Safe to merge. The two concurrency defects identified in previous rounds — replay/fanout duplicate delivery and the subscribe-to-assign stale-entry — are correctly closed by the deliver atomic method and the exitFired guard. No new correctness issues were found. The central locking invariant (deliver serialises ring-append and fanout under s.mu; subscribe snapshots and replays under the same lock) is sound and well-tested including a 400-iteration race-cover test. The exitFired guard handles the late-exit window correctly. PTY teardown is idempotent via sync.Once. Daemon shutdown ordering (defer termMgr.Close() after HTTP server drains) is correct. The two suggestions are style/robustness improvements, not correctness blockers. No files require special attention. The two inline suggestions on session.go (clear s.pty after close) and manager.go (collapse the double-lock in handleSubscribe) are low-risk improvements worth considering before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as xterm.js (Browser)
participant HTTP as httpd.muxHandler
participant Mgr as terminal.Manager.Serve
participant Sess as terminal.session.run
participant PTY as creack/pty (tmux attach)
participant Ring as ringBuffer (50 KB)
participant CDC as cdc.Broadcaster
Client->>HTTP: WS Upgrade /mux
HTTP->>Mgr: Serve(ctx, coderConn)
Mgr->>Mgr: spawn writeLoop + heartbeatLoop
Client->>Mgr: "{ch:terminal, type:open, id:t1}"
Mgr->>Sess: openSession(t1) → session.run(m.ctx)
Sess->>PTY: defaultSpawn(argv) via spawnFunc seam
Mgr->>Client: "{ch:terminal, type:opened}"
Mgr->>Sess: subscribe(onData, onExit) [replay under s.mu]
loop PTY output
PTY-->>Sess: Read(buf)
Sess->>Ring: deliver(chunk) [append + fanout under s.mu]
Ring-->>Mgr: onData(chunk) → enqueue base64 frame
Mgr-->>Client: "{ch:terminal, type:data, data:base64}"
end
Client->>Mgr: "{ch:terminal, type:data, data:base64 keystrokes}"
Mgr->>Sess: session.write(raw)
Sess->>PTY: Write(raw)
Client->>Mgr: "{ch:subscribe}"
Mgr->>CDC: events.Subscribe(onEvent)
CDC-->>Mgr: onEvent(cdc.Event)
Mgr-->>Client: "{ch:sessions, type:snapshot, session:{...}}"
PTY-->>Sess: EOF / error → copyOut returns
Sess->>Sess: shouldReattach? [IsAlive check + backoff]
alt pane dead
Sess->>Mgr: markExited → onExit callbacks
Mgr->>Client: "{ch:terminal, type:exited}"
else session alive
Sess->>PTY: "re-attach (bounded to maxReattach=5)"
end
Reviews (5): Last reviewed commit: "fix(terminal): guard subscribe-to-assign..." | Re-trigger Greptile |
…n lock A new subscriber could receive the same PTY bytes twice: the ring snapshot was taken under s.mu but replayed after unlock, while copyOut appended to the ring and fanned out as two separate lock acquisitions. A chunk appended before the snapshot could then be fanned out after the replay, delivering it in both. The same gap let an exited frame overtake the replay. Make append+fanout one atomic step under s.mu (deliver) and replay the snapshot before releasing the lock in subscribe, so the two critical sections fully serialize and each chunk reaches a subscriber exactly once. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lock The session run loop closes the PTY after copyOut returns, and session.close (via Manager.Close) closes the same PTY again. creackPTY.Close called cmd.Wait each time, and a second concurrent Wait on the same process blocks forever, so daemon shutdown deadlocked whenever a terminal was still attached. fakePTY is idempotent via sync.Once, so the unit suite never exercised this; a real tmux attach surfaced it. Guard close+kill+wait with a sync.Once so Wait runs exactly once. Add a regression test that double-closes a real PTY under a watchdog. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
# Conflicts: # backend/internal/httpd/router.go
Opening a terminal whose session has exited left c.terms[id] set to a no-op (already-exited path) or to a never-cleared unsubscribe (exit after open), so the open guard silently dropped every later open for that id on the connection until close/reconnect. Clients also saw exited/data before the opened ack. Ack opened before subscribe so it always precedes replay/data/exited; have subscribe report whether the pane was already terminal and skip registering in that case; and clear the connection entry from the exit callback for panes that exit after open. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The exit callback enqueued the exited frame before deleting c.terms[id], so a client reopening on receipt of exited could hit the open guard while the entry was still set and have its open dropped. Delete first so the cleared entry is visible by the time the client sees exited. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A session can exit and run onExit (which deletes c.terms[id]) in the gap between subscribe returning exited=false and openTerminal assigning c.terms[id]. The delete is a no-op there since the key isn't set yet, so the later assign resurrects a stale entry for a dead pane, trapping every future open for that id on the connection. Re-apply the delete after the assign when onExit fired in the window, tracked by a c.mu-guarded flag. Add a stress regression test that races the exit against the assign. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The shutdown endpoint test was authored against the pre-rebase httpd.New(cfg, log) signature. After rebasing onto main, the terminal manager (from #50) made termMgr a required third arg. Pass nil — the test exercises /shutdown, not /mux, so the terminal surface stays off. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes #51
Summary
Ports the terminal-streaming surface from the legacy TypeScript repo to the Go backend. In the old stack a separate Node WebSocket process was required because Next.js cannot hijack the HTTP connection for an upgrade. Go's net/http upgrades in-process, so this lands as a single multiplexed
/muxendpoint owned by the daemon, with the stream logic in a dedicated feature package.The change is split into three reviewable commits: the feature package with its tests, the httpd transport that mounts it, and the daemon wiring.
What changed
New
internal/terminalfeature package (owns the workflow):protocol.go: the channel-tagged JSON wire protocol. Channels areterminal,subscribe,sessions, andsystem. Terminal payloads are base64 because raw PTY bytes are not guaranteed valid UTF-8.session.go: a single attached PTY session. Defines thePTYSourceseam (supplies the attach command and liveness) and theptyProcess/spawnFuncseam that lets fan-out, ring replay, and re-attach be tested without a real PTY, tmux, or network. Includes bounded re-attach with backoff for resilience across transient tmux drops.ring.go: a 50KB ring buffer that replays recent output to a client on subscribe so a reconnecting terminal is not blank.manager.go: theManagerthat accepts connections, multiplexes channels, runs the write loop and heartbeat, and bridges the CDC broadcaster onto thesessionschannel. Defines itsEventSourceandwsConninterfaces next to where they are consumed.pty_unix.go/pty_windows.go: the real spawn via creack/pty on Unix; a clear not-supported error on Windows for now.internal/httpdtransport (owns only upgrade and decoding):mux.go: mounts/mux, performs the WebSocket upgrade, sets a 1MiB read limit, and adapts coder/websocket to the manager'swsConn. It does not reach into the terminal internals. When the manager is nil the route is simply not mounted.server.go/router.go: thread the optional*terminal.ManagerthroughNewandNewRouter.Daemon wiring (
main.go):PTYSource, builds the terminal manager fed by the CDC broadcaster, and passes it intohttpd.New. Raw PTY bytes never flow through the CDCchange_log; only session-state events reach thesessionschannel via the broadcaster.How the flow looks
A client opens a terminal channel; the session attaches to the tmux pane, streams PTY bytes out base64-encoded, and replays the ring buffer to late subscribers. In parallel the
sessionschannel forwards CDC session-state events from the broadcaster so the UI can refresh without polling. If the pane drops, the session re-attaches with bounded backoff; on clean exit it reportsexitedand stops.Testing
spawnFuncseam keeps the bulk of the suite hermetic: ring replay, fan-out, re-attach, protocol wire shape, and manager multiplexing all run without a real PTY or network.mux_test.goexercises the genuine upgrade plus wsjson plus Serve plus creack/pty path against a throwaway shell.session_integration_test.godrives a real tmux pane (TestSessionStreamsRealTmuxPane), guarded so it skips where tmux is absent.go test -race ./...;go build ./...andgo vet ./...are clean.Notes
This was rebased onto the current main, which carries the trigger-driven CDC refactor. The
sessionschannel payload was updated to match the newcdc.Eventshape (typedTypeenum,ProjectIDadded,revisiondropped).