Skip to content

Survive a refused new tab instead of exiting the terminal - #124

Open
lexasoft123 wants to merge 1 commit into
pg83:masterfrom
lexasoft123:fix-tab-spawn-failure
Open

lexasoft123 wants to merge 1 commit into
pg83:masterfrom
lexasoft123:fix-tab-spawn-failure

Conversation

@lexasoft123

Copy link
Copy Markdown
Contributor

The problem

Holding Cmd+T makes Shitty exit: every tab and the window vanish, with no crash report.

Nothing caps the tab count, and each tab costs a pty and a forked shell. When the system refuses one (out of file descriptors, ptys or processes), Pty::spawn calls sysError(), which calls exit(1). launchd gives GUI apps a 256 descriptor soft limit and a tab costs about one, so key repeat reaches it in well under a minute.

Reproduced through the real Pty::spawn (test mode's NEW_SESSION uses a fake pty and does not exercise it) with a 128-descriptor limit; tab 124 printed

Error: can't open slave pty: open(): Too many open files (errno=24)

and the process exited with status 1. That this is the reported crash is an inference from the symptoms (no crash report, needs sustained key repeat); I don't have a stack for it.

The fix

  • Pty::spawn's pty-open steps and fork now throw through raiseError() (the recoverable path newSession() already cleans up for) instead of exiting, and close the master descriptor the failed step still held, so a refused attempt leaks nothing.
  • The new-tab action catches it, prints Warning: cannot open a new tab: ... to stderr, and leaves every existing tab alone.
  • A failure of the very first tab at startup still ends the run, with the same Error: ... line the top-level handler printed before.

sysError() is unchanged and still used for the genuinely fatal drain-thread setup.

Test

Pty::RefusedSpawnThrowsAndLeaksNothing runs the real Pty under a tight RLIMIT_NOFILE. Spawning must be refused with an exception, and a second round must open at least as many terminals as the first (a master left open by a refused attempt would shrink it). Against the old pty.cpp it kills the test binary with the error above; with the fix it passes. The Error: F_DUPFD_CLOEXEC lines in its output are forked children reporting on their own stderr at the starved boundary.

  • The full unit suite passes through ./build's own runner, and tst/test_tabs.py passes (26/26).
  • Not verified: the Cmd+T catch in session.cpp was read and compiled but not exercised, since test mode substitutes its own pty and I couldn't synthesize key events in a real window.

Not in this PR

No tab cap, and no raising the descriptor soft limit at startup. Once the limit is hit, other things that need a descriptor can still fail, so raising RLIMIT_NOFILE at startup is probably worth a follow-up.

🤖 Generated with Claude Code

Opening a tab that the system refuses - out of file descriptors, ptys or
processes - called sysError(), which exits the whole process: every tab
and the window vanish, with no crash report. Nothing caps the tab count,
so holding Cmd+T is enough: key repeat opens tabs until the 256
descriptor soft limit launchd gives GUI apps runs out (about one
descriptor per tab), and the terminal is gone.

The pty-open steps and fork in Pty::spawn now throw through raiseError()
instead, the recoverable-error path newSession() already cleans up for,
and release the master descriptor the failed step still held so a
refused attempt leaks nothing. The new-tab action catches it, warns on
stderr and leaves every existing tab alone. A failure of the very first
tab at startup still ends the run, with the same "Error: ..." message the
top-level handler printed before.

RefusedSpawnThrowsAndLeaksNothing runs the real Pty under a tight
RLIMIT_NOFILE: spawning must be refused with an exception, and a second
round must open as many terminals as the first. Against the old
pty.cpp it kills the test binary with "Error: can't open slave pty:
open(): Too many open files".

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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