From dd4a21a61fa028a943c097d8a0adfe6c5f4519b2 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Wed, 2 Sep 2026 15:48:43 +0300 Subject: [PATCH] fix(terminals): a link click no longer leaks a spinning thread per click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit helm reported every ghostty action as unhandled, and ghostty reads that as its own to perform. So a ⌘-click on a link in a pane opened twice: once through helm, and once through a `/usr/bin/open` ghostty spawned itself. That second open never stopped running. Ghostty reads the child's stderr on a detached thread whose loop only breaks on end-of-stream, and after the child exits the reader returns empty slices instead — so the thread never leaves the loop, never reaps the child, and logs `open stderr=` at about 9k lines a second forever. Measured on a helm up nine days: seven such threads, seven unreaped children, 398% CPU at the time and 587 CPU-hours burnt, which averages three full cores continuously since 25 August. Only a restart clears it, and every click adds another one. It also meant TerminalURLPolicy decided nothing. helm drops any scheme outside its allowlist, and ghostty opened it anyway. The fix reports the action as handled when a delegate actually took the URL, so the fallback never runs. Ownership is claimed for open_url alone, and only on the main thread where the answer can be given synchronously — every other action keeps ghostty's own default, since no other fallback was measured and claiming one would suppress it blind. It lands in the vendored wrapper because that is where the callback is, and it is a backport: upstream fixed it identically in 1.5.2, so the code is byte-identical to theirs and a repin retires the patch rather than needing a rebase. Ghostty's own stderr loop is still unfixed upstream (ghostty-org/ghostty#13480, closed unmerged), so not triggering it is the fix available to us. Also fixes the patch script, which could not clone at all: upstream has moved past the branch that held the pinned commit, so a fresh clone no longer contains it and the checkout died with "unable to read tree" in every new worktree. It now fetches the SHA explicitly. --- Patches/libghostty-spm-open-url-handled.patch | 68 ++++++++++++++++ Sources/Helm/Terminals/TerminalSession.swift | 8 ++ .../TerminalOpenURLOwnershipTests.swift | 81 +++++++++++++++++++ docs/VENDORED.md | 72 +++++++++++++++++ scripts/patch-libghostty.sh | 8 ++ 5 files changed, 237 insertions(+) create mode 100644 Patches/libghostty-spm-open-url-handled.patch create mode 100644 Tests/HelmTests/Terminals/TerminalOpenURLOwnershipTests.swift diff --git a/Patches/libghostty-spm-open-url-handled.patch b/Patches/libghostty-spm-open-url-handled.patch new file mode 100644 index 0000000..7b854cd --- /dev/null +++ b/Patches/libghostty-spm-open-url-handled.patch @@ -0,0 +1,68 @@ +From c950adc65394f3dafd240ffa7e7ba445439b3ef7 Mon Sep 17 00:00:00 2001 +From: Rasmus Widing +Date: Wed, 2 Sep 2026 15:34:50 +0300 +Subject: [PATCH] fix(callbacks): report a handled open_url as handled +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Every action reported `false`, which ghostty reads as "the apprt did not +handle this". For `open_url` its default is `internal_os.open` +(Surface.zig `openUrl`), so a ⌘-click on a link in a pane opened twice: +once through the host's delegate, and once through a `/usr/bin/open` +that ghostty spawned itself. + +The second open is not merely redundant. Ghostty reads the child's +stderr on a detached thread whose loop breaks only on `EndOfStream`, and +after `/usr/bin/open` exits the reader returns zero-length slices +forever instead — so the thread never leaves the loop, never reaches +`exe.wait()`, and spins logging `open stderr=` at roughly 9k lines a +second (ghostty-org/ghostty#13480, closed unmerged). Measured on a helm +that had been up nine days: seven such threads, seven unreaped children, +398% CPU, and 586 CPU-hours burnt. It never recovers — only a restart +clears it, and each click adds another. + +It also meant the host's URL policy decided nothing. helm drops any +scheme outside its allowlist, and ghostty opened it regardless. + +Reporting ownership needs the answer synchronously, so the main-thread +case is split out: both `open_url` emitters run on the main thread, and +an action arriving off it keeps the old async dispatch and the old +`false`. Ownership is claimed only for `open_url`, and only when a +delegate actually took it — every other action keeps ghostty's default. + +This is a backport. Upstream fixed it identically in 1.5.2; the code +here is byte-identical to that so it retires cleanly on a repin. +--- + .../Controller/TerminalController+Callbacks.swift | 13 ++++++++++--- + 1 file changed, 10 insertions(+), 3 deletions(-) + +diff --git a/Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift b/Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift +index 76632aa..cb66dec 100644 +--- a/Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift ++++ b/Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift +@@ -36,11 +36,18 @@ private enum TerminalCallbacks { + let bridge = Unmanaged + .fromOpaque(bridgePtr) + .takeUnretainedValue() +- terminalRunOnMain { ++ guard Thread.isMainThread else { ++ terminalRunOnMain { bridge.handleAction(action) } ++ return false ++ } ++ return MainActor.assumeIsolated { + bridge.handleAction(action) ++ // Core spawns /usr/bin/open for an open_url reported unhandled, ++ // so a host delegate that took the URL is reported as handling ++ // it. Both open_url emitters run on the main thread. ++ return action.tag == GHOSTTY_ACTION_OPEN_URL ++ && bridge.delegate is any TerminalSurfaceOpenURLDelegate + } +- +- return false + } + + static func closeSurface( +-- +2.49.0 + diff --git a/Sources/Helm/Terminals/TerminalSession.swift b/Sources/Helm/Terminals/TerminalSession.swift index b6b60af..da4f8f1 100644 --- a/Sources/Helm/Terminals/TerminalSession.swift +++ b/Sources/Helm/Terminals/TerminalSession.swift @@ -623,6 +623,14 @@ extension TerminalSession: TerminalSurfaceLifecycleDelegate, /// the whole security story: terminal content is untrusted, so anything /// but http/https/file/mailto is dropped silently. /// + /// **That sentence is only true because the action callback reports this action as + /// handled**, which it did not until `Patches/libghostty-spm-open-url-handled.patch`. + /// ghostty reads an unhandled `open_url` as its own to perform and falls back to + /// `internal_os.open` (`Surface.zig:4415`), so every URL dropped below was opened by + /// ghostty anyway — the allowlist decided nothing, and the fallback also leaked a + /// spinning thread and an unreaped child per click. `docs/VENDORED.md` has the + /// measurement; `TerminalOpenURLOwnershipTests` pins the conformance it turns on. + /// /// It used to point out of the app: every link went to `NSWorkspace`, so a rendered /// report tabbed you into a browser — the trip helm exists to absorb. A link to /// something the canvas renders now opens **in helm** instead. diff --git a/Tests/HelmTests/Terminals/TerminalOpenURLOwnershipTests.swift b/Tests/HelmTests/Terminals/TerminalOpenURLOwnershipTests.swift new file mode 100644 index 0000000..64b74be --- /dev/null +++ b/Tests/HelmTests/Terminals/TerminalOpenURLOwnershipTests.swift @@ -0,0 +1,81 @@ +import GhosttyKit +import GhosttyTerminal +import XCTest + +@testable import Helm + +/// A ⌘-click on a link used to open it twice, and the second open never stopped running. +/// +/// **The rule: an action helm's delegate took is reported to ghostty as taken.** +/// +/// ghostty reads a `false` from the action callback as *the apprt did not handle this* and +/// runs its own default. For `open_url` that default is `internal_os.open` +/// (`Surface.zig:4415`), which spawns `/usr/bin/open` and reads its stderr on a detached +/// thread. That thread's loop breaks only on `EndOfStream`, and after the child exits the +/// reader returns zero-length slices instead — so it never leaves the loop, never reaches +/// `exe.wait()`, and spins logging `open stderr=` at roughly 9k lines a second +/// (ghostty-org/ghostty#13480, closed unmerged). Measured on a helm up for nine days: seven +/// such threads, seven unreaped children, 398% CPU, 586 CPU-hours. Only a restart clears it, +/// and every click adds another one. +/// +/// The fix is one expression in the vendored wrapper's action callback +/// (`Patches/libghostty-spm-open-url-handled.patch`), and it claims ownership only when a +/// delegate actually took the URL: +/// +/// action.tag == GHOSTTY_ACTION_OPEN_URL +/// && bridge.delegate is any TerminalSurfaceOpenURLDelegate +/// +/// **So the conformance below is load-bearing, which is the whole reason this file exists.** +/// `scripts/patch-libghostty.sh` greps for a marker symbol, which proves the patch was +/// *applied* and says nothing about whether its condition can still be met. Drop +/// `TerminalSurfaceOpenURLDelegate` from `TerminalSession` and nothing fails to compile: +/// the callback simply reports `false` again, ghostty resumes opening every link itself, and +/// the leak is back with no diagnostic anywhere. That is a silent regression a marker grep +/// cannot see, so it is pinned here instead. +/// +/// What is deliberately *not* here: the callback itself. `TerminalCallbackBridge` is internal +/// to `GhosttyTerminal` and the callback needs live `ghostty_app_t`/`ghostty_surface_t` +/// pointers, so the predicate cannot be executed from this side. +/// +/// **The live half is NOT covered here and was not run.** Proving the leak is gone needs a +/// real link ⌘-clicked in a real pane, and a ⌘-click needs a display and an Accessibility +/// grant no agent has (AGENTS.md). The check is: ⌘-click a link, then +/// `ps -M -p ` for a new thread pinned near 57%, and +/// `ps -Ao pid,ppid,stat | awk '$2== && $3 ~ /Z/'` for a new zombie. Neither should +/// appear, and on a build without this patch both do. +final class TerminalOpenURLOwnershipTests: XCTestCase { + /// The condition the patch tests at runtime, asserted against the type helm actually + /// installs as the surface delegate. + func testTerminalSessionTakesOpenURLSoGhosttyDoesNotOpenItToo() { + XCTAssertTrue( + (TerminalSession.self as Any.Type) is (any TerminalSurfaceOpenURLDelegate.Type), + "TerminalSession must conform to TerminalSurfaceOpenURLDelegate: the vendored callback claims open_url only when it does, and without it ghostty falls back to /usr/bin/open and leaks a spinning thread per ⌘-click" + ) + } + + /// The other half of the same sentence, and the reason a bare `true` would be wrong: + /// ownership is claimed for `open_url` alone. Every other action keeps ghostty's default, + /// because no other fallback was measured and claiming one would suppress it blind. + func testOwnershipIsClaimedForOpenURLAndNothingElse() { + XCTAssertNotEqual( + GHOSTTY_ACTION_OPEN_URL, + GHOSTTY_ACTION_SET_TITLE, + "the patch keys on this tag; if the constants ever collapse the condition stops discriminating" + ) + } + + /// Taking the action is not the same as opening the URL, and the difference is the point + /// of the allowlist. helm drops a scheme outside it and *still* owns the action — before + /// the patch ghostty opened the dropped URL anyway, which made `TerminalURLPolicy` + /// advisory rather than the security boundary `TerminalSession` documents it as. + func testADroppedSchemeIsStillHelmsDecisionToMake() { + XCTAssertNil( + TerminalURLPolicy.validated("javascript:alert(1)"), + "the policy must still refuse this" + ) + XCTAssertTrue( + (TerminalSession.self as Any.Type) is (any TerminalSurfaceOpenURLDelegate.Type), + "and refusing it must not read to ghostty as unhandled, or the fallback opens what the policy just dropped" + ) + } +} diff --git a/docs/VENDORED.md b/docs/VENDORED.md index 8e1caad..c58144d 100644 --- a/docs/VENDORED.md +++ b/docs/VENDORED.md @@ -134,10 +134,23 @@ clones and patches it. revision `b0930320739324886590e865d571eb5dd7073912` — the same exact pin docs/SPIKE.md records. The patch does not touch the binary target, so the `GhosttyKit.xcframework.zip` URL and checksum are upstream's, unchanged. + + **The script fetches that SHA explicitly, and has to.** A clone carries only what + its branches reach, and upstream has since moved past the branch that held + `b093032` — so a fresh clone does not contain the pinned commit at all. The + checkout after it died with `fatal: unable to read tree (b093032…)` on every new + worktree while an older `vendor/`, cloned before the prune, kept verifying fine. + The object is still served, it is just no longer advertised by any ref, so + `git fetch origin ` is what keeps a pin-by-SHA actually reachable. Found by + running the script in a fresh worktree, where it had been failing silently: the + invocation was piped into `tail`, and a pipeline reports the *last* command's + status, so `set -e` inside the script could not surface it. AGENTS.md documents + that shape for `log show`; it bites here identically. - **Patches**, applied in this order by `scripts/patch-libghostty.sh` (both are `git format-patch` output; applied with `git am`): 1. `Patches/libghostty-spm-multi-surface-wakeup.patch` — the multi-surface wakeup fix. 2. `Patches/libghostty-spm-clipboard-destination.patch` — the clipboard-destination fix (#297). + 3. `Patches/libghostty-spm-open-url-handled.patch` — report a handled `open_url` as handled. **Their diffstats are deliberately not restated here.** `git format-patch` already writes one into each patch file, so a copy in this document is a second spelling of a number nothing @@ -237,6 +250,59 @@ owns — and are tracked separately. this decision from helm's own gate — a marker grep proves a patch was applied and says nothing about what it decides. +## Patch 3 — report a handled `open_url` as handled + +**What it fixes.** `TerminalCallbacks.action` returned `false` for every action, and ghostty +reads `false` as *the apprt did not handle this*. For `open_url` its default is +`internal_os.open` (`Surface.zig:4415`, the only call site), so a ⌘-click on a link in a pane +opened twice: once through helm's delegate, and once through a `/usr/bin/open` ghostty spawned +itself. + +**The second open never stopped running.** Ghostty reads that child's stderr on a detached +thread whose loop breaks only on `EndOfStream`; after `/usr/bin/open` exits the reader returns +zero-length slices instead, so the thread never leaves the loop, never reaches `exe.wait()`, +and logs `open stderr=` at roughly 9k lines a second forever. Upstream ghostty knows +(ghostty-org/ghostty#13480) and the fix there is closed unmerged, so this is not something a +newer XCFramework fixes. + +Measured on the operator's helm, up nine days, 2026-09-02: + +``` +7 spinning threads, ~57% of a core each 398% process CPU +7 unreaped `/usr/bin/open` children 586 CPU-hours burnt +93% of each thread's samples in os_log logd 43%, analyticsd 49% +``` + +`sample` put every one of those threads in `zig_os_log_with_type` with **zero** I/O syscalls, +hitting `__FIREHOSE_CLIENT_THROTTLED_DUE_TO_HEAVY_LOGGING__`. Their seven TIDs matched the +seven zombies one-for-one, and the four oldest zombies matched the four threads with the +largest accumulated CPU. It never recovers: only a restart clears it, and each ⌘-click adds +another. + +**It also made `TerminalURLPolicy` advisory.** helm drops any scheme outside its allowlist, +and `TerminalSession` calls that allowlist "the whole security story" — but dropping it +returned `false` like everything else, so ghostty opened the dropped URL anyway. + +The patch claims ownership for `open_url`, and only when a delegate actually took it. The +answer is needed synchronously, so the main-thread case is split out; both `open_url` emitters +run on the main thread, and an action arriving off it keeps the old async dispatch and the old +`false`. No other action's return value changes — no other fallback was measured, and claiming +one would suppress it blind. + +**Why helm needs it.** helm hosts long-lived agents in its panes and cannot restart to shed a +leak without killing them. + +**This one is a backport, which is why it is written to disappear.** Upstream fixed it +identically in 1.5.2; the code here is byte-identical to upstream's, so a repin to ≥1.5.2 +retires this patch on its own rather than needing a rebase. +**What is not proved by any gate**: that a ⌘-click no longer leaks. That needs a display and +an Accessibility grant, so it is the operator's check — ⌘-click a link, then look for a new +~57% thread in `ps -M -p ` and a new zombie child. Neither should appear. +`Tests/HelmTests/Terminals/TerminalOpenURLOwnershipTests.swift` pins the conformance the +condition depends on — drop `TerminalSurfaceOpenURLDelegate` from `TerminalSession` and +nothing fails to compile, the callback just reports `false` again and the leak returns with no +diagnostic anywhere. + ## Where the pin lives, and how it retires **Where the pin lives now.** A local path dependency is not recorded in @@ -257,6 +323,12 @@ local pin puts it back into `Package.resolved` where the rest of the deps are. They retire independently: upstream taking one and not the other leaves `Patches/` holding whichever is left, and the script's array is already per-patch. +**Patch 3 is already retired upstream, and that is an argument for repinning rather than a +reason to do it today.** libghostty-spm 1.5.2 carries the same fix, so a repin drops that +patch outright. It also moves the ghostty XCFramework several versions, which is a change with +its own blast radius and its own gate run — worth doing deliberately, not as a side effect of +a leak fix. + Until one of those happens the branch does not build from a clean clone without running `scripts/patch-libghostty.sh` first. Verify the patches with: diff --git a/scripts/patch-libghostty.sh b/scripts/patch-libghostty.sh index d3772e9..6ee9c61 100755 --- a/scripts/patch-libghostty.sh +++ b/scripts/patch-libghostty.sh @@ -27,6 +27,7 @@ base_tag=1.3.1 patches=( "libghostty-spm-multi-surface-wakeup.patch|wakeupSubscribers|Sources/GhosttyTerminal/Controller/TerminalController.swift" "libghostty-spm-clipboard-destination.patch|TerminalClipboardDestination|Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift" + "libghostty-spm-open-url-handled.patch|TerminalSurfaceOpenURLDelegate|Sources/GhosttyTerminal/Controller/TerminalController+Callbacks.swift" ) for entry in "${patches[@]}"; do @@ -96,6 +97,13 @@ if [ -d "$vendor" ]; then fi git clone https://github.com/Lakr233/libghostty-spm.git "$vendor" +# A clone carries only what its branches reach, and upstream has since moved on +# from the branch that once held $base — so a fresh clone does not contain the +# pinned commit at all and the checkout below died with "unable to read tree" on +# every new worktree, while an existing vendor/ from before the prune kept +# working. Fetching the SHA directly is what makes a pin by SHA stay reachable; +# the object is still served, it is just no longer advertised by any ref. +git -C "$vendor" fetch --no-tags origin "$base" git -C "$vendor" checkout -b helm/patches "$base" for entry in "${patches[@]}"; do git -C "$vendor" am "$root/Patches/${entry%%|*}"