Skip to content

Close the pty controller fd on dispose - #8

Merged
tomlm merged 2 commits into
tomlm:mainfrom
JohnCampionJr:fix/close-controller-fd
Aug 22, 2026
Merged

Close the pty controller fd on dispose#8
tomlm merged 2 commits into
tomlm:mainfrom
JohnCampionJr:fix/close-controller-fd

Conversation

@JohnCampionJr

Copy link
Copy Markdown
Collaborator

Every pseudoterminal leaks its controller file descriptor for the life of the process.

PtyStream wraps the fd with ownsHandle: false, which is correctReaderStream and WriterStream are two streams over the same fd, and letting either own it would make disposing both a double close.

The consequence is that disposing them closes neither, and nothing else closes it either: pty_close exists in the native shim and is declared in both Mac and Linux NativeMethods, and has no callers anywhere in the library.

How it shows up

As pty_spawn failing after enough terminals have come and gone — which reads as a system limit rather than as a leak, and reads that way most convincingly in exactly the long-lived process that leaks the most.

Found by moving a test suite into a single process, where several tests each opening a batch of ptys ran back to back. Each test alone had always been fine.

On the ordering

Streams, then kill, then close. The streams don't own the fd; the kill wants the process still addressable; closing the controller last means nothing reads from a descriptor that has gone. TryClose never throws, matching TryKill beside it — a cleanup path isn't a place to discover a new failure.


Companion to #7 (forkpty thread safety) but independent of it — either can go in alone.

Every pseudoterminal leaked its controller file descriptor for the life of the
process.

PtyStream wraps the fd with ownsHandle: false, which is correct -- ReaderStream
and WriterStream are two streams over the SAME fd, and letting either own it
would make disposing both a double close. The consequence was that disposing
them closed neither, and nothing else closed it either: pty_close exists in the
native shim and is declared in both Mac and Linux NativeMethods, and had no
callers anywhere in the library.

It surfaces as pty_spawn failing after enough terminals have come and gone,
which reads as a system limit rather than as a leak -- and reads that way most
convincingly in exactly the long-lived process that leaks the most.

Found by moving a test suite into a single process, where several tests each
opening a batch of ptys ran back to back. Each test alone had always been fine.

Ordering is streams, then kill, then close: the streams do not own the fd, the
kill wants the process still addressable, and closing the controller last means
nothing reads from a descriptor that has gone. TryClose never throws, matching
TryKill beside it -- a cleanup path is not a place to discover a new failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Synchronize disposal or atomically invalidate the descriptor before closing to prevent unsafe double closes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR closes leaked Unix PTY controller file descriptors during disposal on macOS and Linux.

Changes:

  • Adds disposal-time controller descriptor cleanup.
  • Preserves cleanup ordering: streams, process termination, then descriptor closure.
  • Implements platform-specific close operations.
File summaries
File Summary
src/Porta.Pty/Unix/PtyConnection.cs Adds controller descriptor cleanup; disposal is not synchronized, risking a double close and incorrect closure of a reused descriptor.
src/Porta.Pty/Mac/PtyConnection.cs Implements macOS descriptor closing.
src/Porta.Pty/Linux/PtyConnection.cs Implements Linux descriptor closing.
Review details

Suppressed comments (2)

src/Porta.Pty/Unix/PtyConnection.cs:95

  • This resource-lifetime fix has no regression test: the existing tests dispose connections incidentally via using, but none repeatedly spawn and dispose PTYs in one process or assert that the controller descriptor is reclaimed. Add a Unix-focused stress/regression test that exercises repeated disposal and detects descriptor exhaustion or an unbounded descriptor count, so this leak cannot be reintroduced unnoticed.
            this.TryClose();

src/Porta.Pty/Unix/PtyConnection.cs:95

  • If either FileStream.Dispose() throws while flushing the writer (for example after the child has already closed the slave), control exits Dispose before reaching this line. Because isDisposed was already set, a later Dispose returns immediately, leaving the controller fd leaked. Put the stream disposal in a try/finally whose cleanup always runs TryKill and TryClose (and ensure both streams are attempted).
            this.TryClose();
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

{
try
{
this.Close(this.controller);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe should be fixed

@tomlm
tomlm merged commit 88fda68 into tomlm:main Aug 22, 2026
3 checks passed
JohnCampionJr added a commit to JohnCampionJr/Porta.Pty that referenced this pull request Aug 22, 2026
… found it all

Answers #33. Everything here came out of driving this library hard from a
desktop app — terminal panes and long-running children, many at once — so each
piece is a measurement rather than a preference.

OUT-OF-BAND ConPTY. Microsoft ships ConPTY out of band as
Microsoft.Windows.Console.ConPTY (conpty.dll + OpenConsole.exe), the same
implementation Windows Terminal carries. Both are wired up behind
PORTAPTY_CONPTY so the choice can be measured rather than argued; out-of-band is
the default with an automatic fallback to in-box when conpty.dll is not beside
the assembly, so a consumer who has not referenced the package cannot break.

It appeared for a long time to cost ~3.0 SECONDS per pseudoconsole. It does not,
and the reason is worth the docs page: ConPTY asks the terminal what it is
(Primary Device Attributes) and blocks three seconds waiting for a reply that a
read-only consumer never sends. Answering it up front took first-output latency
from [3016,3012,3011,3013,3019]ms to [15,9,9,8,8]ms. Out-of-band then measures
9ms per pseudoconsole against in-box's 13ms.

Two things that made the A/B lie before it told the truth, both in docs: recent
Windows 11 ships its own System32\conpty.dll, so an unqualified DllImport
resolves the OS copy and the 'out-of-band' arm is quietly in-box; and the only
direct evidence of which implementation is live is a process census, because
in-box spawns conhost.exe per pseudoconsole and out-of-band spawns
OpenConsole.exe.

CsWin32 REPLACES Vanara. The point is the runtime dependency: Vanara ships an
assembly every consumer then carries, for about twenty entry points. CsWin32 is
a build-time generator with PrivateAssets=all, so it contributes nothing at
runtime and nothing to a consumer's graph, and the generated interop is trimming
and AOT friendly. The idea is Sylinko's, from their fork; this is an independent
implementation of it.

WINDOWS JOB-OBJECT RACE. CreateProcessW was not given CREATE_SUSPENDED, so a
child could run — and exit — before AssignProcessToJobObject reached it, failing
with 'Failed to assign process to job object'. Created suspended, assigned, then
resumed.

net10.0 and MSTest on the Microsoft Testing Platform. MTP reports crashes and
hangs instead of absorbing them, which matters here because almost everything
interesting is threads and process lifetime — a swallowed hang looks exactly
like a pass.

ConcurrentSpawnTests is the harness the rest came from: 24 concurrent spawns,
20 samples per cell, minimal and realistic shells. It is what showed the reader
strategy dominating everything else (137ms to first output on a dedicated thread
against 7546ms pooled) and what surfaced forkpty's thread-unsafety on macOS as
5/24 failures rather than as an occasional mystery.

Overlaps tomlm#7, tomlm#8 and tomlm#9 — those are the same fixes against the pre-CsWin32 tree.
Merge them first and I will rebase, or take this and close them, whichever you
prefer.

23 tests, 0 failed on macOS (3 Windows-only skipped).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@JohnCampionJr
JohnCampionJr deleted the fix/close-controller-fd branch August 22, 2026 19:06
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.

3 participants