Skip to content

fix(session): release the native PTY when the process exits - #69

Open
dhaern wants to merge 1 commit into
shekohex:mainfrom
dhaern:omos/fix-pty-release-on-exit
Open

dhaern wants to merge 1 commit into
shekohex:mainfrom
dhaern:omos/fix-pty-release-on-exit

Conversation

@dhaern

@dhaern dhaern commented Oct 5, 2026

Copy link
Copy Markdown

Summary

A PTY whose process exits on its own keeps its native handles open. Every exited session leaks four file descriptors and two threads, and clearing the session afterwards does not release them. This PR releases the native PTY from the exit handler.

In plain terms: when a command started through the plugin finishes by itself, the plugin used to leave some operating system resources open for it until OpenCode shut down. After many short commands those leftovers pile up. Now the plugin lets them go as soon as the command ends. Nothing changes in what users see.

Cause

bun-pty 0.4.10 closes the native handle (bun_pty_close) only inside Terminal.kill(). When the child exits by itself, the read loop reports the exit but never reaches kill(), so the handle stays open. SessionLifecycleManager called kill() only when the plugin itself ended the process.

Changes

  • onExit calls session.process.kill() after it records the status, exit code, signal and exitAt. Terminal.kill() fires onExit again synchronously, so the handler returns early when exitAt is already set. That keeps the real exit code and a single exit callback.
  • The timeout path reuses kill() instead of repeating its body, and clearAllSessions loses its one-line private wrapper.

Validation

Six PTYs running true, counting /proc/self/fd and /proc/self/task entries of the plugin process:

fds before fds after exit threads before threads after exit
main 8 32 7 19
this PR 8 8 7 8

The new test starts a real PTY (sh -c 'exit 3'). On main it fails because kill is never called. Here it passes with one kill call, status exited, exit code 3 and one exit callback. Removing the exitAt guard makes it fail with two kill calls.

bun test over test/*.test.ts, without the live and npm-pack suites, gives 185 passing. bun run typecheck, bun run lint and bunx biome format . are clean. I did not run the Playwright e2e suite locally.

Diff

Production: -8 lines (+13/-21), all in src/plugin/pty/session-lifecycle.ts.
Tests: +21 lines, one new file (test/session-lifecycle.test.ts).

The native PTY is only closed inside Terminal.kill(), so a process that
exits on its own leaked file descriptors and threads (about +4 fds and +2
threads per PTY). Release it from the exit handler, guarded by exitAt
because releasing can emit onExit again synchronously.

Also simplify the timeout kill path and inline clearAllSessions.
@dhaern dhaern changed the title fix(pty): release the native PTY when the process exits fix(session): release the native PTY when the process exits Oct 5, 2026
@dhaern

dhaern commented Oct 5, 2026

Copy link
Copy Markdown
Author

Thanks for re-running the CI, @shekohex. The only red check is test (test): 191 tests pass and 1 fails, WebSocket Functionality > should demonstrate WebSocket subscription logic works correctly in test/websocket.test.ts, which times out at its 500 ms limit. This PR only changes session-lifecycle.ts and its own test file, so it doesn't go near that test.

I don't think this change causes it. The same commit passes every check in my fork (test (test) passed in all 7 runs I did on it), and main at b6c2067 passed all 7 as well. There the test took 51 to 187 ms on main and 52 to 202 ms on this branch. Locally, 100 runs of each took 98 to 134 ms on main and 100 to 133 ms here, with no failures. If I cap the CPU to about a third of a core, it fails on both: 35 of 40 runs each.

So it looks like a slow runner pushing that test past 500 ms, and a re-run should clear it. If it keeps showing up, I can send a separate small PR that raises that timeout instead of mixing it into this one.

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.

1 participant