Repository navigation
fix(pty): reap unattached PTY sessions to prevent orphan process leaks - #62342
JohnnyWilson16 wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes #50699 as described. I ran the issue's repro (50 PTY sessions whose connect-token is refused) against opencode serve from source. On the base, all 50 shells and sessions were still there after 60s. With this PR, all 50 were gone. The PTY session tests and tsc pass.
Public API impact:
- New environment variable
OPENCODE_PTY_UNATTACHED_TIMEOUT(default60 seconds). It is undocumented and its value isn't validated. - Behaviour change for
POST /api/pty: sessions that aren't attached within the grace period are now killed and apty.deletedevent is published. API/SDK clients that create a PTY and only drive it withwrite, or attach later than 60s, will lose it. In-repo clients (the app and the TUI) use persistent PTYs, so they aren't affected. Existing callers that attach promptly keep working.
Concerns (details inline):
- Only sessions that never attach are reaped. A client that attaches and then drops (tab closed, network loss) still leaves the shell running forever.
- A malformed env value silently turns the reaper off.
- The tests depend on real-time sleeps and on changing
process.env.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
Thanks for the update. This fixes #50699. I ran the issue's repro against opencode serve from source: on the base, all 50 shells from refused connect-tokens were still running after 60s. With this commit, all 50 shells and sessions were gone within 60s. The PTY session tests (12/12, stable over 4 runs), the server's PTY socket tests and tsc for packages/core pass.
My earlier concerns are resolved:
- The
OPENCODE_PTY_UNATTACHED_TIMEOUTenv var is gone. The timeout is now a layer option, and invalid values fall back to the default. - Reaper failures are logged instead of silently dropped.
- The timer restarts when the last subscriber detaches and is cancelled when a client re-attaches.
- The tests use
TestClockinstead of real sleeps and no longer changeprocess.env.
Public API impact: no new env vars, config keys, or HTTP routes. Pty.configured, Pty.layer and Pty.Options are new internal exports in @opencode/core. One behaviour change on /api/pty: a session with no attached client for 60s (never attached, or since its last detach) is now killed and a pty.deleted event is published. API/SDK clients that drive a PTY only through write, or reconnect after more than 60s, will lose the session. This is intended, but it's a change for those callers.
No blocking concerns.
benjaminkincses
left a comment
benjaminkincses
left a comment
There was a problem hiding this comment.
one nit on the reaper error path, rest looks right.
|
Thanks. I checked both in |
There was a problem hiding this comment.
The new commit only stops the reaper from logging an error when its timer is cancelled on attach or teardown; real failures are still logged. That looks right. The PTY session tests (12/12) and tsc for packages/core pass at this commit. This commit doesn't change the public API, and I have no new concerns.
Issue for this PR
Closes #50699
Type of change
What does this PR do?
When a client creates a PTY session via
/api/ptybut fails to attach (e.g., connect-token rejected by origin policy, early tab close) or drops connection while running, the underlying shell process runs indefinitely with no subscriber.This change:
Pty.configured({ timeout?: Duration.Input })(with a 60-second default) to configure or inject the timeout cleanly without public environment variables.Effect.catchCauseandErrorSummarylogging.session.process.kill()) and removes the session when the grace timer expires with no active subscribers.How did you verify your code works?
Added deterministic unit and integration tests using
TestClockinpackages/core/test/pty/pty-session.test.ts:reaps unattached sessions after the grace period— verifies unattached session is reaped after grace window.does not reap sessions while attached— verifies sessions with active attachments are not reaped.reaps session after grace period when last subscriber detaches— verifies reaper restarts upon client disconnect/detach.cancels reaper if client re-attaches before grace period expires— verifies re-attaching before timeout resets grace window.reaps unattached session using configured timeout— verifies custom timeout injection viaPty.configured({ timeout: "10 seconds" }).Ran verification commands locally:
cd packages/core && bun test test/pty/(24 pass, 2 skip)cd packages/server && bun test test/pty-socket.test.ts(4 pass)cd packages/core && bun run typecheck(0 errors)bun x oxlint packages/core/src/pty.ts packages/core/test/pty/pty-session.test.ts(0 errors)Checklist
Added by the opencode bot (AI-generated).
Related: #48691, #51828
Public API: /api/pty sessions with no attached client for 60s (never attached, or since the last detach) are now removed (behaviour change for API clients that never attach or reconnect late)