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%%|*}"