mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
J. Scott Miller
3c5160dd66
## Summary Fixes the `TestRun/RPTY` flake tracked in PLAT-116 (`timeout waiting for read to finish`). `rptyConn.Close` sends `Ctrl+C` to interrupt the command, then waits up to 30s for the read to finish. The read only unblocks once the server closes the reconnecting PTY stream, which depends on the agent terminating the command under test (a `dd` reading stdin) and tearing down the backend. When the server-side teardown does not complete within 30s, `Close` returned a hard error and failed the run. Logs from the March 2026 failure confirm the agent used the `screen` backend (`backend_type=screen`) and show no session teardown activity at all after `Ctrl+C`; the interrupt chain stalled rather than merely running slowly. The previously deferred `c.conn.Close()` ran only *after* the wait gave up, so nothing actively unblocked the read within the window. ## Changes - `conn.go`: graceful close is now best-effort. After the grace period, `Close` actively force-closes the underlying connection to unblock the read, waits a bounded `forceCloseReadTimeout` (5s) for the read to drain rather than blocking indefinitely, and returns a distinguishable sentinel `errRPTYGracefulCloseTimeout`. The same force-close path is used when the `Ctrl+C` write fails. Timeouts are fields on `rptyConn` so tests can shrink them deterministically. - `run.go`: treats `errRPTYGracefulCloseTimeout` as non-fatal (logged as a warning) so the run no longer fails when the connection was closed, just not gracefully. Any other close error still fails the run, preserving signal for a genuine regression. - `conn_internal_test.go`: new unit tests covering the graceful, forced-close, stuck-read-after-close, and double-close paths using a stub connection. ## Testing - `go test ./scaletest/workspacetraffic/ -run TestRPTYConnClose -race -count=10` passes. - `go test ./scaletest/workspacetraffic/ -run TestRun/RPTY` passes. - `golangci-lint run ./scaletest/workspacetraffic/` clean; `gofmt`/emdash clean. <details> <summary>Root-cause analysis and lifecycle notes</summary> The client conn is bound to `context.Background()`, so the test context cannot unblock the read; only an actual websocket close can. The coderd proxy bridges client and agent with `agentssh.Bicopy`, which propagates closes promptly, so the stall is not there. On the agent side both backends do eventually close the connection after the command exits: - **buffered**: output reader hits EOF on command exit and closes active conns in-process (one goroutine handoff). - **screen**: a longer chain (`Ctrl+C` -> screen client PTY -> daemon -> inner PTY -> SIGINT -> `dd` exit -> session teardown -> `screen -x` client exit -> agent output reader EOF -> conn close), involving extra OS processes. The backend is auto-selected (`screen` if present on Linux, else `buffered`) and the test does not pin it, so behavior depends on the runner image. Logs from the March 2026 failure (run 23322663002) confirm `backend_type=screen` and show no `unable to read pty output` or session-quit activity between the attach and the moment the client gave up 30s later, meaning `dd` never exited in response to `Ctrl+C` within the window. The stall is in delivery or signal handling inside the screen path, not a slow process exit. No agent-side logic bug was identified from the logs, which is why the fix makes graceful close best-effort rather than asserting a fixed deadline. Possible follow-ups (not in this PR): pin the test to a deterministic backend, and/or log the agent's chosen `backend_type` in test output to aid future diagnosis. </details> --- This PR was generated with assistance from Coder Agents.