diff --git a/TODOS.md b/TODOS.md index 6d61e928a..97aad27dc 100644 --- a/TODOS.md +++ b/TODOS.md @@ -9,8 +9,49 @@ ## P2 — perf, correctness, and the next VCS phase +### Retire the attach-only `workroom-session` shim (macapp) — issue #154 follow-up + +**What:** Delete the remaining `macapp/WorkroomSession/` client, its pinned v2.0.0 binary fixture +(`macapp/WorkroomAppTests/Fixtures/workroom-session-v2.0.0`, 464KB), `SessionShimCompatibilityTests`, +and the freeze on `WorkroomSessionProtocol` — once no user can still be running a v2.0.0 daemon. + +**Why:** The shim is deliberately temporary. It exists only to reattach terminals held by a daemon +process started by an app version older than the agent. That population shrinks with every reboot, +every logout, and every closed pane, and eventually reaches zero — at which point the client, its +fixture and its tests are pure carrying cost. + +**Pros:** Removes the last of two pty client implementations, drops a binary from git, and frees +`WorkroomSessionProtocol` to change again (it is frozen today because its only real peer — a shipped +v2.0.0 binary — can never be recompiled to match). + +**Cons:** Nothing signals when the population hits zero. It is a judgement call on elapsed time and +release adoption, so this will sit open for a while. Deleting it early strands exactly the users it +was added for. + +**How to start:** Check whether any reattach-from-old-daemon has been reported since the shim landed. +Then delete the directory, the fixture, the compatibility tests, and the freeze comment together — +they are one unit, and leaving the fixture behind without its tests is worse than leaving all of it. + +**Depends on / blocked by:** The shim landing first, then elapsed time plus release adoption. No hard +gate. + +**Priority:** P2, not before 2027. + ### `workroom-session` daemon: the findings the `/review` pass didn't fix (macapp) — persist-sessions follow-up +> **RETIRED.** The daemon this section is about no longer exists in the tree: `SessionDaemon.swift` +> and `SessionPTY.swift` were deleted when `workroom-session` became an attach-only client (issue +> #154). The `SessionDaemonEndToEndTests.test*` methods cited below as red/green evidence went with +> it, so those citations now point at git history rather than at runnable tests — read them as a +> record of what was fixed, not as coverage that still runs. `git log -- macapp/WorkroomSession/` +> has the originals. +> +> **The P0 architectural note below is NOT closed by that removal, and it would be wrong to read it +> that way.** Deleting the daemon's source from this build does not stop the daemons already running +> on users' machines: those were started by an app at or before v2.0.0, they keep their original +> `LOCAL_PEERCRED`-only trust boundary, and reaching them is precisely what the retained attach +> client is for. The note stands, unchanged, until the last such process is gone. + **What:** The `workroom-session` daemon (persisted ordinary terminals across quit, `b28e9134`) went through a two-round `/review` (checklist + specialist dispatch, then a full Claude+Codex adversarial pass). The three highest-value findings from round two were fixed directly (fallback-socket path diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 7ffcaa887..f7cae45bd 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -651,11 +651,16 @@ exists, but run both. buffer rather than porting it — see Phase 3's scrollback bullet) and the framing; Swift keeps only what a client needs to speak the wire (`SessionFrame`, `SessionBytes`, `SessionIdentifier`), so `SessionReplayBuffer`'s Swift copy retires with the daemon rather than living on in parallel. - **Two consequences for the deletion:** `macapp/WorkroomSession/` goes in full — daemon, `SessionPTY`, - `SessionAttachClient.swift` and `main.swift` alike, since `wr-agent attach` replaces the relay — - while `macapp/WorkroomSessionProtocol/` survives in slimmed form for the Swift client. And - `SessionDaemonEndToEndTests.swift` (7 tests) plus `SessionDaemonHarness.swift` drive the Swift - daemon binary directly, so they must be **rewritten against the agent**, not expected to pass. + **Two consequences for the deletion**, and 2026-09-15 corrected the first one: `macapp/WorkroomSession/` + was to go in full — daemon, `SessionPTY`, `SessionAttachClient.swift` and `main.swift` alike, + since `wr-agent attach` replaces the relay. It does replace it **for sessions the agent owns**, + which is not the same claim: a pty a pre-v2.0.0 daemon is holding can only be reached by the + Swift relay, so `SessionAttachClient.swift` and `main.swift` stay and the daemon half goes. See + "The deletion was release-gated, and that was wrong". `macapp/WorkroomSessionProtocol/` survives + as predicted, now frozen while the shim ships. And `SessionDaemonEndToEndTests.swift` (7 tests) + plus `SessionDaemonHarness.swift` drove the Swift daemon binary directly, so they were deleted + rather than rewritten — the agent's equivalents are Rust tests, and the one piece worth keeping + (the version handshake) moved to `SessionShimCompatibilityTests`. - New crate `wr-agent` beside `wr-vcs-core`. A **versioned** multiplex envelope carrying stream id, service kind, and payload — versioned from the first commit; see the Distribution Plan's resume-policy item for what makes the versioning @@ -1350,9 +1355,10 @@ established either way. ## Phase 1 Results -Landed on `master` 2026-09-12/13 across PRs #181, #182 and #183. Phase 1 is **complete except its -last bullet** — deleting `macapp/WorkroomSession/` — which is gated on a release, not on code; see -"The deletion is release-gated" below. +Landed on `master` 2026-09-12/13 across PRs #181, #182 and #183, and **completed** 2026-09-15 by +PR #192. The last bullet was deleting `macapp/WorkroomSession/`. It was not deleted — it was split, +because the release gate this doc wrote for it turns out not to protect anyone. See "The deletion +was release-gated, and that was wrong" below, then Outstanding item 1 for what shipped instead. ### What shipped @@ -1419,7 +1425,12 @@ code immediately: Both verified by removal. A chosen split point tests the boundary its author thought of; these were at the boundaries they did not. -### The deletion is release-gated +### The deletion was release-gated, and that was wrong + +> **SUPERSEDED 2026-09-15, kept because the error is the useful part.** Everything below is sound +> about *exposure* — no shipped release contained the agent — and wrong about what a soak buys. A +> release cycle validates the replacement; it does not drain anyone. The reasoning that replaced it +> is Outstanding item 1. **No shipped release contains the agent at all.** `v2.0.0` was tagged 2026-09-09; the drain landed 2026-09-12. Every user in the field is on the Swift daemon exclusively, so deleting @@ -1430,9 +1441,11 @@ The deletion should wait until the agent has shipped and soaked for at least one drain is designed to complete on its own — the daemon idles out once its last session ends — so nothing needs to be done to make it happen, only to confirm it has. -`SessionDaemonEndToEndTests.swift` and `SessionDaemonHarness.swift` therefore stay for now: the -daemon still ships, and they are live coverage of shipping code. They go together with -`macapp/WorkroomSession/` in the deletion change. +`SessionDaemonEndToEndTests.swift` and `SessionDaemonHarness.swift` were said here to stay "for +now", as live coverage of shipping code. **Both are deleted** (PR #192): they drive a daemon this +binary can no longer start, so they covered nothing that still exists. The one thing in them worth +keeping — the version handshake, the only test anywhere that exercised it — was ported into +`SessionShimCompatibilityTests`, and now runs against the real v2.0.0 binary instead of a fake. **What replaced them is narrower than "rewritten against the agent", deliberately.** Three of those seven tests cover agent behaviour that Rust tests cover more cheaply and can run on Linux; three @@ -1445,11 +1458,95 @@ disagreement passes every test on either side alone while presenting as an empty ### Outstanding -1. **Delete `macapp/WorkroomSession/`** once the agent has shipped and soaked — see above. +1. **~~Delete `macapp/WorkroomSession/`~~ — DONE DIFFERENTLY, 2026-09-15.** The plan above said to + delete the directory once the agent had soaked for a release cycle. That reasoning does not + survive contact: **a soak drains nobody.** Draining completes only for panes a user actually + closes, so someone who skips the intermediate release — or simply holds a long-lived pane through + the update — still meets a build whose bundle has no `workroom-session` while their old daemon is + still holding their shell. Traced: `resolveOwner` answers `.swiftDaemon`, `binaryPath` is nil, + `attachCommand` is nil, and `GhosttySurfaceView` silently opens a plain shell while the real work + keeps running in an orphaned daemon that now never exits. + + So the directory was **split, not deleted**: the half that RUNS terminals is gone + (`SessionDaemon.swift`, `SessionPTY.swift`, the client's own `launchDaemon` spawn, the `daemon` + subcommand), and the half that CONNECTS to them stays. The result can attach to a daemon an app + at or before v2.0.0 left running, and can never be one. It self-obsoletes; retiring it is now a + TODO with its criteria written down rather than a deletion waiting on a calendar. + + **Compatibility is measured, not assumed.** `git log v2.0.0..master -- macapp/WorkroomSessionProtocol/` + is empty, and the shipped v2.0.0 `workroom-session` (464KB, sha256 `6f75e6f7…`, lifted unmodified + from `workroom-macos-app_2.0.0.dmg`) is pinned at + `macapp/WorkroomAppTests/Fixtures/workroom-session-v2.0.0`. `SessionShimCompatibilityTests` runs + that binary as a daemon, has it create a session, and asserts THIS build's client reattaches and + receives the replay the old daemon produced. Verified live before any deletion, then made + repeatable. A fake built from our own `WorkroomSessionProtocol` was considered and rejected: it + reads the same source the client does, so a protocol edit moves both sides together and the test + stays green through exactly the break it exists to catch. + + **`WorkroomSessionProtocol` is frozen** while the shim ships, because its only real peer can never + be recompiled to match a change. + + Three routing bugs the removal would otherwise have introduced, all fixed in the same change: + `preferred()` returning the daemon as a fallback (it now returns nil — there is nowhere to fall + back to); that Optional collapsing the probe cache's nil sentinel, turning a once-per-launch 2s + main-actor probe into one per access (~16s of frozen app on an eight-pane restore); and the + availability gate being asked at **two** layers, so `TerminalPersistentSessionPolicy` discarded a + restored pane's session id before anything could ask who owned it — one layer above the guard + that was supposed to fix this. + + **What this does NOT do.** It does not restore the rollback target: an agent that breaks in the + field now degrades to plain shells rather than falling back to the daemon. And it does not stop + the old daemon creating a session on attach for an id it does not hold + (`SessionDaemon.handleAttach`, in the shipped binary we cannot change), so a reattach can still + silently become a fresh shell — detection for that is outstanding, see item 4. 2. **A Linux `wr-agent` as a build artifact.** The cross-build recipe works and the Linux test suite runs in a container (`vcs/scripts/test-linux.sh`), but nothing publishes the ELFs yet. Phase 3 needs them; Phase 1 does not. 3. **OQ21, where the `HostDriver` lives**, is untouched and remains open. +4. **~~Two items from the 2026-09-15 eng review~~ — BOTH DONE, 2026-09-15.** + + (a) **A failed attach now becomes a shell, not a dead pane.** `SessionBackendProbe` runs + `wr-agent protocol` and parses a version, which proves the binary starts and prints — not that it + can serve. A stale socket, a failed negotiation, a crash between the two all pass it. And nothing + downstream could recover: the app picks session-or-plain-shell before forking, and + `wait_after_command` is false, so a relay that exits leaves a pane with no shell in it. So the + relay itself execs the user's shell on every pre-attach failure (`fall_back_to_shell`), printing + one line saying persistence is gone — a terminal that silently stopped surviving quit is worse + than one that admits it. `RawMode::restore()` had to become explicit: `exec` runs no destructors, + and a shell handed a raw tty has no line editing, echo or signal keys. + + (b) **A lost session is no longer replaced by an invented one.** The shipped daemon's + `handleAttach` creates for an id it does not hold, so a reattach to a session whose shell had + exited silently forked a new one — a fresh prompt where a build was running, indistinguishable + from success. `confirmBeforeAttach` asks once more immediately before attaching and refuses when + the daemon disclaims it, which **prevents** the substitution rather than detecting it: the daemon + never receives the request that would make it create. `.unreachable` still attaches, deliberately + — it means we could not ask, and a daemon too wedged to answer is also too wedged to fork. + + Residual, and not closable from this side of the socket: the daemon can answer `.owned` and lose + the session before the attach lands. +5. **Rolling BACK to v2.0.0 strands agent-held sessions, and re-updating does not recover them.** + This one is release notes, not code: nothing a newer build writes changes what an older one does + on relaunch. + + Traced. A v2.0.0 app knows nothing about `wr-agent`. On relaunch it starts its own daemon, and + for each restored pane it attaches by session id at its own socket. `SessionDaemon.handleAttach` + creates for an id it does not hold, so the user gets a fresh shell in every pane — not an error. + The agent's sessions are not destroyed: `wr-agent` outlives the app and the shells keep running, + reachable with `wr-agent` directly. + + **Updating forward again does not give them back**, which is the part worth saying out loud. + `resolveOwner` asks the daemon first and `owner(preferred:daemon:)` answers `.swiftDaemon` for + any `.owned` — the agent is preferred only when the daemon is `.unreachable`. After a rollback + both helpers hold a session under the same id, the daemon claims it, and the daemon wins. The + user lands back in the rollback-era shell. + + Preferring the agent unconditionally is not the fix: that is the same coin flip in the other + direction, and it breaks the case this whole shim exists for, where the daemon legitimately holds + the only copy of a user's work. The honest answer is the release note. + + **What the notes must say:** rolling back to v2.0.0 replaces the contents of open terminals with + fresh shells, and updating again will not bring them back. Close what matters first. ## Open Questions @@ -1598,10 +1695,18 @@ disagreement passes every test on either side alone while presenting as an empty 13. **Can the wakefulness service actually defeat an idle timer?** **ANSWERED — Phase 0 item 5.** Yes, and more cheaply than assumed: both boxd timers measure *network* idle (the CLI help says so), a CPU-busy job with no network I/O was hibernated mid-work with a 193 s clock gap, and the in-VM CLI setting its own `auto-hibernate.timeout` to 0 stopped it recurring. The process survived the hibernate, so the live-process gate is not binding here. The agnosticism decision bets the headline capability on it. Phase 0 item 5 settles it. A bad answer does not reopen the provider choice — it turns `keepAwake` back into a capability gate, which is a worse product. -14. **What is the local rollback?** Phases 1-2 replace shipped session code for every *local* user - and the Nightly flag protecting remote does not protect that; the Distribution Plan names - keeping the Swift daemon selectable for one release as the mitigation, but the rollback trigger - and the dual-run period are undecided. +14. ~~**What is the local rollback?**~~ **ANSWERED 2026-09-15 — there is none, and the question was + aimed at the wrong risk.** The proposed mitigation was keeping the Swift daemon selectable for + one release. It was never built, and the question it was meant to answer ("what if the agent is + bad?") is not the one that strands users: **the app updating at all** is, because a v2.0.0 + daemon holding a live shell cannot hand it over and cannot be restarted by a build that no + longer contains it. That is not a rollback problem, so a rollback would not have fixed it. + + What shipped instead is forward compatibility: an attach-only client that can reach a daemon an + older app left running. A bad agent now degrades to plain shells rather than falling back to the + daemon — accepted deliberately, since that fallback was what let both helpers create a session + for the same id. Rolling back to v2.0.0 remains possible and is lossy in a way the release notes + have to state; see Outstanding item 5. 15. **What is the `maxLifetime` floor?** It is one of three hard gates (see the Goal), so it needs a number. A working day is the obvious candidate, but a workroom is supposed to outlive a working day — which suggests @@ -1678,26 +1783,31 @@ disagreement passes every test on either side alone while presenting as an empty with what the same commands report on the box itself — including a push, a pull-rebase, and a commit, since VCS writes are half the surface. - PR and CI badges resolve for a remote workroom rather than failing `launchFailed`. -- Local workrooms run on the same agent, with the Swift daemon still **selectable** for one release - as the rollback named in the Distribution Plan (a third state on the existing - `backgroundSessions` preference, - `DefaultsKeys.swift:76`). Deletion of `SessionDaemon`, `SessionPTY` and their helpers is a - criterion for the release *after* that, not for Phase 1. When it lands, `macapp/WorkroomSession/` - goes entirely — `SessionAttachClient.swift` included, since `wr-agent attach` replaces it — while - `macapp/WorkroomSessionProtocol/` remains in slimmed form as the Swift client's wire codec. - **Note the tension this accepts:** Approach A was chosen partly because it "never has - two code paths", and a selectable daemon is two code paths for one release. That is a deliberate, - time-boxed exception for rollback safety, not a reversal of the unify decision — and it is - narrower than Approach B's rejected version, which would have run two paths indefinitely with the - remote one diverging. +- ~~Local workrooms run on the same agent, with the Swift daemon still **selectable** for one + release as the rollback.~~ **RESTATED 2026-09-15 — the selectable daemon was never built and + cannot be.** `backgroundSessions` is still a plain `Bool` (`DefaultsKeys.swift:78`); no third + state exists. The criterion as written also had the deletion backwards: `SessionDaemon`, + `SessionPTY` and the `daemon` subcommand are **already gone**, while `SessionAttachClient.swift` + — which this bullet said would go with them — is precisely the part that **stays**, because + attaching is the only thing that can reach a pty an older app left running. + `macapp/WorkroomSessionProtocol/` does remain as the wire codec, as written, and is frozen. + + The criterion is therefore: local workrooms run on the agent, and a user holding a terminal from + a pre-v2.0.0 daemon keeps it. What replaced the rollback is asymmetric by construction — forward + compatibility with a daemon we can no longer build, not the ability to go back to it. Outstanding + item 5 records what rolling back actually costs. + + **The tension this bullet accepted is resolved, not deferred:** two code paths for one release + was the price of a selectable daemon. What ships is one path for new sessions (the agent) plus a + read-only bridge to the old one, which is not the dual-run Approach A was chosen to avoid. - ~~The ported replay buffer matches the Swift original on the generated/fuzzed corpus.~~ **Restated — there is no ported replay buffer.** `libghostty-vt` holds the terminal state, so the criterion is: on the generated/fuzzed corpus, a snapshot round-trips (grid, styles, cursor, modes, continuation), and a client attaching mid-stream renders the same screen as one that watched from byte zero — **including under an alternate-screen program**, which the Swift - original could not do at all. Every session and store test passes **except** - `SessionDaemonEndToEndTests` (7 tests) and `SessionDaemonHarness`, which drive the deleted binary - and are rewritten against the agent rather than expected to pass. + original could not do at all. The exception this named — `SessionDaemonEndToEndTests` (7 tests) + and `SessionDaemonHarness`, which drove the deleted binary — is closed: both files are gone, and + the suite passes with no session exception (2662 tests, 1 unrelated skip). - **The far-side terminal state survives a provider stop-and-reboot**: reopen after one, and the alternate-screen program's screen is still there (the process is not — that is the live-process suspend gate, not a bug). This criterion did not previously exist because nothing could satisfy @@ -1762,9 +1872,12 @@ disagreement passes every test on either side alone while presenting as an empty session code with a fresh Rust implementation for **every local user**, unflagged, before any remote code ships. The differential harness proves replay parity on a byte corpus; it proves nothing about pty ownership, exec-failure detection, or the alt-screen repaint on real hardware - with real TUIs. Mitigation: keep the Swift daemon **selectable** for at least one release (the - existing `backgroundSessions` preference is the natural place for a third state), dual-run them - during development, and write down the rollback before Phase 1 lands. Tracked as open question 14. + with real TUIs. The mitigation proposed here — keep the Swift daemon **selectable** for a release + via a third state on `backgroundSessions` — **was not built** (see open question 14). The concern + itself stands, and was partly borne out: three routing bugs and two create-on-attach hazards were + found in exactly this seam, none of them by the differential harness. What guards it now is the + attach-only client plus a pinned v2.0.0 binary the suite drives on every run — which tests the + boundary a selectable daemon would only have provided an escape from. ## Next Steps @@ -1860,7 +1973,8 @@ Carried forward, unresolved by design: justifies porting a working daemon ahead of the feature. Approach C remains a legitimate fallback, and the differential-harness argument that dismissed it cuts both ways. - **The local switchover is riskier than the remote feature** and the Nightly flag does not - cover it. Open question 14. + cover it. Open question 14, now answered — and the concern was right about the seam while wrong + about the remedy: the bugs were real and none of them was a rollback away from being survivable. - **The base machine is real scope creep** — a fourth product object introduced by a provider capability. Justified, named, still unpriced. - Round 2 has not re-reviewed the fixes made in response to it. diff --git a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift index 5c5151592..0b063641a 100644 --- a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift +++ b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift @@ -1,6 +1,7 @@ import AppKit import Defaults import GhosttyKit +import WorkroomSessionProtocol import os /// One terminal surface: an `NSView` that hosts a `ghostty_surface_t` (Metal-rendered by libghostty @@ -70,6 +71,11 @@ final class GhosttySurfaceView: NSView { /// Daemon session this surface attaches to. When set and the helper is available, `createSurface` /// launches `workroom-session attach` instead of a login shell. var persistentSessionID: UUID? + /// Whether `persistentSessionID` was carried over from a previous launch rather than minted for + /// this pane. Only a restored id can name a session that has since died, and only a restored id + /// is re-checked before attaching — asking about a fresh one would refuse every new pane, since + /// no helper holds an id that has never existed. + var persistentSessionIsRestored = false /// Project / workroom / tab / title metadata sent on attach. var sessionMetadata: [(key: String, value: String)] = [] @@ -402,7 +408,13 @@ final class GhosttySurfaceView: NSView { envVarCount = 0 } - /// Configure libghostty to spawn `workroom-session attach` for a background session. + /// Configure libghostty to spawn a session helper for a background session. + /// + /// Returns whether it set `config.command` — not strictly "a session was attached". The two + /// differ in one case: a session the daemon has already lost, where this sets a command that + /// tells the user so and then becomes their shell. The caller only uses the result to decide + /// whether to install a run command, and a pane with a persistent session is never a run-command + /// pane (`TerminalPersistentSessionPolicy` excludes them), so the distinction costs nothing. @discardableResult private func applyPersistentSession( to config: inout ghostty_surface_config_s, @@ -410,20 +422,52 @@ final class GhosttySurfaceView: NSView { ) -> Bool { // No session was requested for this pane — not a failure, nothing to log. guard let persistentSessionID else { return false } + + // Ask the daemon once more, before committing to an attach, whether it still holds this. + // The shipped daemon CREATES a session on attach for an id it does not hold, so a cached + // ownership answer that has outlived its session turns a reattach into a brand new shell with + // nothing to mark it as one. See `confirmBeforeAttach`. + if PersistentSessionService.shared.confirmBeforeAttach( + sessionID: persistentSessionID, wasRestored: persistentSessionIsRestored) == .gone, + let noticePointer = strdup(Self.lostSessionCommand()) + { + Self.sessionLogger.error( + """ + persistent session \(persistentSessionID.uuidString, privacy: .public) is gone; \ + opening a shell with a notice rather than letting the daemon create a new session + """) + surfaceCStrings.append(noticePointer) + config.command = UnsafePointer(noticePointer) + config.wait_after_command = false + // This branch returns before `launchEnvironment`, so without this the notice pane would be a + // second-class terminal: no `GHOSTTY_RESOURCES_DIR` means no shell integration, which means + // no OSC 133 and no OSC 7 — the title latches to whatever the prompt last set and the footer + // never learns the directory. The agent's own fallback keeps them by routing through + // `shell::invocation`; losing a session should not also cost you a working title bar. + if let resources = GhosttyResources.bundledURL?.path { + environment.append(("GHOSTTY_RESOURCES_DIR", resources)) + } + return true + } + // Deliberately NOT gated on `PersistentSessionService.isAvailable`, and + // `PersistentSessionAttachGateTests` fails if that is ever reintroduced. `isAvailable` answers + // for the backend a NEW session would go to, so it goes false whenever the agent is unhealthy + // — which is precisely when a session the retired Swift daemon still holds most needs reaching. + // Gating here rejected those panes before `attachCommand` could route them, defeating the + // attach-only client in its own use case. `attachCommand(forSession:)` already answers nil when + // the owning backend has no binary, so the global check bought nothing on this path. guard - PersistentSessionService.shared.isAvailable, let attach = PersistentSessionService.shared.attachCommand( forSession: persistentSessionID), let attachPointer = strdup(attach) else { // A pane that expected a persisted session fell back to a plain login shell. Whatever the - // daemon-side session was doing is now orphaned/unreachable from this pane — worth a log + // helper-side session was doing is now orphaned/unreachable from this pane — worth a log // line since the resulting terminal otherwise looks identical to a normal fresh shell. Self.sessionLogger.error( """ persistent session \(persistentSessionID.uuidString, privacy: .public) unavailable \ - (helper isAvailable=\(PersistentSessionService.shared.isAvailable, privacy: .public)); \ - falling back to a plain shell + (no owning helper resolved); falling back to a plain shell """) return false } @@ -438,6 +482,43 @@ final class GhosttySurfaceView: NSView { return true } + /// A command that says the previous session is gone, then becomes the user's shell. + /// + /// **Why a printed line and not silence.** Refusing the attach already prevents the wrong + /// outcome: the daemon does not get asked, so it cannot fork a replacement shell and pass it off + /// as the user's. But the pane still comes back showing a fresh prompt, which is exactly what a + /// successfully restored session looks like — and the thing that was running there is gone. A + /// terminal that quietly swaps a finished build for an empty prompt is the failure worth paying + /// a line of text to avoid. + /// + /// `exec` so the shell IS the pane's process: its exit status, title and signals behave the way + /// they would in any other terminal, rather than being wrapped by something the user did not ask + /// for. The agent's `fall_back_to_shell` shares that idea and little else — it builds its argv + /// through `shell::invocation`, strips `WORKROOM_SESSION_*` and sets a marker, none of which + /// apply here: this branch returns before `launchEnvironment`, so the pane never had those + /// variables to strip. + /// `SHELL`, or the default when it is unset OR EMPTY. + /// + /// `??` only defaults on absent, so `SHELL=""` produced `exec '' -l` — exit 127, and with + /// `wait_after_command` false that is the dead pane this whole function exists to prevent. The + /// Rust side has always filtered empties (`serve.rs`'s `from_vars`); these two are now in step. + static func resolvedShell( + environment: [String: String] = ProcessInfo.processInfo.environment + ) -> String { + guard let shell = environment["SHELL"], !shell.isEmpty else { + return SessionShellIntegration.defaultShell + } + return shell + } + + static func lostSessionCommand( + shell: String = GhosttySurfaceView.resolvedShell() + ) -> String { + let notice = "The terminal that was running here has ended, so this is a new shell." + let script = "printf '%s\\n' \(shellQuoted(notice)); exec \(shellQuoted(shell)) -l" + return "/bin/sh -c \(shellQuoted(script))" + } + func reattachPersistentSession() { guard persistentSessionID != nil, !isTornDown else { return } destroySurface() @@ -1741,7 +1822,7 @@ extension GhosttySurfaceView { /// POSIX single-quote a path so spaces and shell metacharacters are taken literally by the shell /// that receives the inserted text. - private static func shellQuoted(_ path: String) -> String { + static func shellQuoted(_ path: String) -> String { "'" + path.replacingOccurrences(of: "'", with: "'\\''") + "'" } } diff --git a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift index 37178807a..982592ab0 100644 --- a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift +++ b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift @@ -21,6 +21,14 @@ import WorkroomSessionProtocol protocol SessionControlPlane { func list() -> [SessionDescriptor] func info(identifier: SessionIdentifier) -> SessionDescriptor? + /// Whether this helper holds the session, **distinguishing "it said no" from "it never + /// answered"** — which `info` cannot, because it returns nil for both. + /// + /// On the protocol rather than on one client because BOTH helpers create-on-attach: the daemon's + /// `handleAttach` ends in `create(request:connection:)` and the agent's is guarded only by + /// `sessions.contains(id)` (`serve.rs`). Asking only the daemon closed the substitution hole on + /// the backend being retired and left it open on the one that owns every new session. + func ownership(identifier: SessionIdentifier) -> SessionOwnership func kill(identifier: SessionIdentifier) -> Bool func killAll() -> Bool } @@ -53,6 +61,19 @@ struct AgentControlClient: SessionControlPlane { list().first { $0.identifier == identifier } } + /// `list()` collapses "no reply" into an empty array, which is why this cannot be written on top + /// of it: an agent that is wedged and an agent holding nothing would be indistinguishable, and + /// answering `.notOwned` for the first would throw away a live session. Going through `transact` + /// directly keeps nil meaning "nothing readable came back". + func ownership(identifier: SessionIdentifier) -> SessionOwnership { + let sessions = transact(SessionFrame(kind: .list)) { frame -> [SessionDescriptor]? in + guard frame.kind == .sessions else { return nil } + return try? SessionDescriptor.decodeList(frame.payload) + } + guard let sessions else { return .unreachable } + return sessions.contains { $0.identifier == identifier } ? .owned : .notOwned + } + func kill(identifier: SessionIdentifier) -> Bool { transact( SessionFrame(kind: .kill, payload: SessionIdentifierPayload.encode(identifier)) diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift index ff9b7634d..b5450d7c4 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift @@ -18,23 +18,118 @@ final class PersistentSessionService { /// One socket path per backend. They deliberately differ, so these must never be conflated — /// the daemon's sessions and the agent's are reached through different files. private var resolvedSocketPaths: [SessionBackend: String] = [:] - /// Where new sessions go. Cached because resolving it runs the agent to check it works, and - /// that answer does not change within a launch. + /// Where new sessions go, once resolved. Cached because resolving it RUNS the agent to check it + /// works, and that answer does not change within a launch. + /// + /// **Nil has two meanings here and `lastProbeAt` is what separates them.** `preferred()` returns + /// an Optional, so nil is a legitimate answer ("no backend can take a new session"), not merely + /// "not asked yet". Using nil for both — as this did while `preferred()` was non-Optional — + /// silently un-caches exactly the case that most needs caching: the probe is a `Process()` with a + /// 2-second deadline run SYNCHRONOUSLY ON THE MAIN ACTOR during terminal creation + /// (`SessionBackendProbe.runProtocolCommand`), so a hung agent costs 2s per access, and `backend` + /// is read once or more per pane. An eight-pane restore would freeze for ~16s. private var cachedPreferred: SessionBackend? + /// When the last probe ran, or nil if it never has. A SUCCESSFUL answer is cached for the launch + /// — the agent does not become unavailable once it has answered — but a nil answer is retried + /// after `probeRetryInterval`. + /// + /// Latching nil forever was worse than what it replaced. Before this change a failed probe cached + /// `.swiftDaemon`, and the daemon could still be STARTED, so a transient failure degraded to + /// daemon-backed persistence; now it degrades to no persistence at all, for the rest of the + /// launch, with no way back short of quitting. The probe is a `Process()` with a 2-second + /// watchdog, so load, resource exhaustion or a slow first exec of a freshly-updated binary are + /// all enough to trip it once. Re-probing on a cooldown keeps the fix for the real problem — the + /// per-access re-probe that froze the main actor for ~2s a time — while leaving a way out. + private var lastProbeAt: Double? + /// One background re-probe at a time; a burst of panes must not spawn one each. + private var isReprobing = false + /// When a helper last failed to ANSWER, per backend. + /// + /// `ownership` has its own 2-second deadline and, unlike the agent probe, it is asked once per + /// SESSION with no shared answer — so a helper that accepts and then stalls cost N x 2s for N + /// panes, serially, on the main actor. That is the same arithmetic the probe cooldown exists to + /// stop, applied to the query the probe cooldown does not cover. A helper that could not answer + /// a moment ago is not worth asking again for every remaining pane in the window. + private var lastUnreachableAt: [SessionBackend: Double] = [:] + /// Long enough that a burst of pane creation pays one probe, short enough that a user who waits + /// a moment and opens another terminal gets another chance. + static let probeRetryInterval: Double = 30 /// Which helper owns each session, resolved once. See `backend(forSession:)` for why one answer /// per session rather than one per call — a pane asks twice and the two must agree. private var owners: [UUID: SessionBackend] = [:] - private var descriptors: [UUID: SessionDescriptor] = [:] - private init() {} + /// How the agent's health is measured. Injected so a test can drive the unhealthy path without + /// a real `wr-agent` to break. + private let probe: (SessionBackend) -> SessionBackendAvailability + /// What the shipped daemon says about a session, or nil to ask a real one over its socket. + /// Injected for the same reason, and separately: the two answers combine, and the case that + /// matters most (unhealthy agent, daemon-owned session) needs both driven at once. + private let ownershipOverride: ((UUID) -> SessionOwnership)? + /// Monotonic seconds, for the probe cooldown. `systemUptime` rather than `Date()` so a clock + /// adjustment cannot make the cooldown never expire. + private let now: () -> Double + + private init() { + self.probe = { SessionBackendProbe.probe($0) } + self.ownershipOverride = nil + self.now = { ProcessInfo.processInfo.systemUptime } + } + + /// Test seam. `shared` never uses it; every other behaviour is identical. + init( + probe: @escaping (SessionBackend) -> SessionBackendAvailability, + ownership: @escaping (UUID) -> SessionOwnership, + now: @escaping () -> Double = { ProcessInfo.processInfo.systemUptime } + ) { + self.probe = probe + self.ownershipOverride = ownership + self.now = now + } - /// Where a NEW session would be created. Existing sessions are resolved individually — see - /// `backend(forSession:)`, which is what makes the migration invisible. - var backend: SessionBackend { + /// Where a NEW session would be created, or **nil when nowhere can take one**. Existing sessions + /// are resolved individually — see `backend(forSession:)`. + /// **The first probe is synchronous; every retry is not.** That split is the whole design. + /// + /// The probe is a `Process()` with a 2-second watchdog, and this runs on the main actor during + /// terminal creation. Making the FIRST one async would cost the common case its persistence — + /// the healthy agent answers in milliseconds, and a pane that opened before the answer arrived + /// would be a plain shell for no reason. Making the RETRIES synchronous costs a broken agent a + /// >=2s frozen main actor once per cooldown window, for the life of the launch, which is Sentry's + /// AppHang threshold in a repo that has spent this month fixing AppHangs. + /// + /// So: answer nil immediately and re-probe in the background. The worst case degrades from "the + /// app freezes every 30 seconds" to "this pane has no persistence, the next one does". + var backend: SessionBackend? { if let cachedPreferred { return cachedPreferred } - let resolved = SessionBackend.preferred() - cachedPreferred = resolved - return resolved + let currentTime = now() + guard let lastProbeAt else { + self.lastProbeAt = currentTime + cachedPreferred = SessionBackend.preferred(probe: probe) + return cachedPreferred + } + guard currentTime - lastProbeAt >= Self.probeRetryInterval else { return nil } + self.lastProbeAt = currentTime + reprobeInBackground() + return nil + } + + /// Re-probe off the main actor and publish the answer back onto it. + /// + /// `probe` is called on a detached task, so a hung agent burns its 2 seconds somewhere nobody is + /// waiting. Only a SUCCESS is written back: a second nil would just re-arm the same cooldown that + /// `backend` already set before starting this. + private func reprobeInBackground() { + guard !isReprobing else { return } + isReprobing = true + let probe = self.probe + Task.detached(priority: .utility) { + let resolved = SessionBackend.preferred(probe: probe) + await MainActor.run { [weak self] in + guard let self else { return } + self.isReprobing = false + if let resolved { self.cachedPreferred = resolved } + } + } } /// Which helper owns an EXISTING session, or where a new one should go. @@ -63,26 +158,23 @@ final class PersistentSessionService { return resolved } + /// The ambiguity branch this used to carry is gone, because the state it guarded is now + /// unreachable. It fired when new sessions were routed at the DAEMON — the old fallback for a + /// failed agent probe — and answered nil rather than send `workroom-session attach` at an id the + /// daemon had never held, which the daemon creates rather than refuses. `preferred()` no longer + /// names the daemon under any condition, so a failed probe now yields nil directly and the same + /// safety falls out of `owner(preferred:daemon:)` with nothing special to say. private func resolveOwner(_ sessionID: UUID) -> SessionBackend? { - let preferred = backend let daemon = daemonOwnership(sessionID) - if case .owned = daemon { return .swiftDaemon } - guard preferred == .swiftDaemon else { return Self.owner(preferred: preferred, daemon: daemon) } - - // New sessions go to the DAEMON, which means the agent probe failed this launch. That does not - // mean no agent sessions exist: an agent from an EARLIER launch can still be running and still - // holding ptys, reachable through its socket even though this launch could not start one. The - // daemon disclaiming the session is therefore not evidence that the daemon owns it, and - // answering `.swiftDaemon` here sends `workroom-session attach` at an id the daemon has never - // held — which it creates, forking a duplicate shell and orphaning the real one. - // - // A live agent socket is enough to make that ambiguous, so say so. A definitive answer would - // need an ownership query on `SessionControlPlane`, which only the daemon side has; adding one - // to the agent is the right fix and is more than this belongs in. - guard existingSocketPath(for: .rustAgent) != nil else { - return Self.owner(preferred: preferred, daemon: daemon) + // **A wedged retired daemon must not veto a session the live agent holds.** `.unreachable` + // resolves to nil, and nil blocks `endSession` — which `reap` gates deleting a workroom + // directory on (issue #7) — so a v2.0.0 daemon that accepts and then stalls made healthy + // AGENT sessions unkillable and their workrooms undeletable. The daemon's opinion only needs + // to win for ids the agent does not hold, so ask the agent before accepting the veto. + if case .unreachable = daemon, case .owned = ownership(of: sessionID, in: .rustAgent) { + return .rustAgent } - return nil + return Self.owner(preferred: backend, daemon: daemon) } /// Which helper a session belongs to, given where new sessions go and what the daemon said — @@ -103,7 +195,11 @@ final class PersistentSessionService { /// /// `nonisolated` because it touches no state — the probe is the caller's job, and a pure rule /// should not need the main actor to evaluate. - nonisolated static func owner(preferred: SessionBackend, daemon: SessionOwnership) + /// `preferred` is itself Optional now: nil means no backend can take a NEW session. It only + /// reaches the answer through `.notOwned`, so a daemon that CLAIMS the session still resolves to + /// the daemon even when nothing else can run — which is the whole point of keeping the attach + /// client, and the case an earlier draft of this change broke. + nonisolated static func owner(preferred: SessionBackend?, daemon: SessionOwnership) -> SessionBackend? { switch daemon { @@ -119,11 +215,47 @@ final class PersistentSessionService { /// steady state once the migration has drained, and it must not push every session at a daemon /// that is not running. private func daemonOwnership(_ sessionID: UUID) -> SessionOwnership { - guard - let identifier = SessionIdentifier(uuidString: sessionID.uuidString), - let socketPath = existingSocketPath(for: .swiftDaemon) - else { return .notOwned } - return PersistentSessionControlClient(socketPath: socketPath).ownership(identifier: identifier) + ownership(of: sessionID, in: .swiftDaemon) + } + + /// What a specific helper says about a session. + /// + /// No socket file at all is `.notOwned` rather than `.unreachable`, for both backends and for the + /// same reason: a helper that is not running holds nothing. Its sessions are its children and + /// died with it. + private func ownership(of sessionID: UUID, in backend: SessionBackend) -> SessionOwnership { + // A helper that just failed to answer is not asked again for every remaining pane. The answer + // is the same `.unreachable` it would have given, arrived at without a second 2-second wait — + // which is what a window of panes opening against one wedged daemon would otherwise cost, + // serially, on the main actor. + // + // **Above the test override on purpose.** The override replaces the round trip, not the policy + // about how often to make one; with it on top, no test could reach this cooldown at all, and + // the first version of it shipped with nothing measuring it. + let currentTime = now() + if let failedAt = lastUnreachableAt[backend], + currentTime - failedAt < Self.probeRetryInterval + { + return .unreachable + } + + let answer: SessionOwnership + if let ownershipOverride { + answer = ownershipOverride(sessionID) + } else { + guard + let identifier = SessionIdentifier(uuidString: sessionID.uuidString), + let socketPath = existingSocketPath(for: backend) + else { return .notOwned } + answer = controlPlane(socketPath: socketPath, backend: backend) + .ownership(identifier: identifier) + } + // Only the failure is recorded. Clearing this on a real answer looks like the matching half + // and is not: `now()` is monotonic, so an entry can only be READ while it is still inside the + // cooldown — and a helper that answered had already outlived it. The clearing branch could + // never change what the next caller sees. + if case .unreachable = answer { lastUnreachableAt[backend] = currentTime } + return answer } func socketPath(for backend: SessionBackend) -> String? { @@ -138,7 +270,7 @@ final class PersistentSessionService { } } - var socketPath: String? { socketPath(for: backend) } + var socketPath: String? { backend.flatMap { socketPath(for: $0) } } func existingSocketPath(for backend: SessionBackend) -> String? { let candidates = [ @@ -148,21 +280,82 @@ final class PersistentSessionService { return candidates.compactMap { $0 }.first { FileManager.default.fileExists(atPath: $0) } } - var existingSocketPath: String? { existingSocketPath(for: backend) } - func binaryPath(for backend: SessionBackend) -> String? { PersistentSessionPaths.binaryURL(for: backend)?.path } - var binaryPath: String? { binaryPath(for: backend) } + var binaryPath: String? { backend.flatMap { binaryPath(for: $0) } } + /// Whether a NEW session could be created. **Not whether an existing one can be reached** — that + /// is `attachCommand(forSession:)`, which routes per session. The distinction is the bug this + /// change was written around twice: gating a RESTORED pane on this answer discards its id before + /// anything asks who owns it (`TerminalPersistentSessionPolicy`), and gating the attach itself on + /// it strands every daemon-owned pane whenever the agent is unhealthy. var isAvailable: Bool { socketPath != nil && binaryPath != nil } + /// Whether a session is still there to attach TO, asked immediately before attaching. + enum DaemonSessionState: Equatable { + case attachable + /// The daemon answered, and it does not hold this session. Attaching would make it CREATE one. + case gone + } + + /// Re-ask the retired daemon whether it still holds this session, right before we attach to it. + /// + /// **The failure this prevents is the one that looks like success.** `SessionDaemon.handleAttach` + /// — in the shipped v2.0.0 binary, which cannot be changed — ends in + /// `create(request:connection:)` for an id it does not hold. So when a session's shell has exited + /// but this launch still has `.swiftDaemon` cached for it, attaching does not fail: the daemon + /// silently forks a brand new shell and hands it over. The user sees a fresh prompt where their + /// build was running, with nothing to distinguish it from a successful reattach. + /// + /// Asking again is what closes it. Ownership is otherwise resolved once per session and cached + /// for the launch (a pane asks twice and the two answers must agree), and that cache is correct + /// for routing — but it long outlives the session it describes. + /// + /// **`.unreachable` proceeds, deliberately.** It means we could not ask, not that the session is + /// gone, and the substitution needs the daemon to be RESPONSIVE enough to answer "not mine" and + /// then create. A daemon too wedged to reply is also too wedged to fork anything, so attaching + /// cannot produce the wrong result — and refusing would throw away a session that is probably + /// still there. + /// + /// What it does not close: the daemon can still answer `.owned` here and lose the session before + /// the attach lands. That race is narrow and cannot be closed from this side of the socket. + func confirmBeforeAttach(sessionID: UUID, wasRestored: Bool) -> DaemonSessionState { + // A freshly minted id has never existed anywhere, so create-on-attach is the WANTED behaviour + // and asking would refuse every new pane. Only an id carried over from a previous launch can + // name a session that has since died. + guard wasRestored else { return .attachable } + guard let owner = backend(forSession: sessionID) else { return .attachable } + switch ownership(of: sessionID, in: owner) { + case .owned, .unreachable: + return .attachable + case .notOwned: + // The cached answer described a session that no longer exists. Drop it so a later reattach + // resolves afresh rather than walking back into this. + owners.removeValue(forKey: sessionID) + logger.error( + """ + session \(sessionID.uuidString, privacy: .public) is no longer held by the daemon; \ + not attaching, because the shipped daemon would create a new shell instead + """) + return .gone + } + } + /// The command libghostty forks for this session, from the helper that owns it. + /// + /// Requires the same three things `launchEnvironment` does — owner, binary AND socket — because + /// the two are consumed as a pair and disagreeing is worse than either refusing. It used to check + /// only owner and binary, so an unresolvable socket path produced a non-nil command beside an + /// empty environment: `workroom-session attach` then started with no `WORKROOM_SESSION_ID` or + /// `_SOCKET` and exited 2 immediately, and because the plain-shell fallback is decided BEFORE the + /// command is spawned, the pane died rather than degrading. Refusing here degrades properly. func attachCommand(forSession sessionID: UUID) -> String? { // Nil owner ⇒ nil command ⇒ the caller opens a plain shell rather than guessing a helper. guard let owner = backend(forSession: sessionID), - let path = binaryPath(for: owner) + let path = binaryPath(for: owner), + socketPath(for: owner) != nil else { return nil } return path.replacingOccurrences(of: " ", with: "\\ ") + " attach" } @@ -178,15 +371,19 @@ final class PersistentSessionService { ) -> [(key: String, value: String)] { // Socket and binary must both come from the helper that owns THIS session, or the relay is // pointed at one implementation while being told to use the other's socket. + // `WORKROOM_SESSION_BINARY` used to be set here. Its only reader was the attach client's + // `executablePath()`, which existed only to re-exec itself as `workroom-session daemon` when no + // daemon answered — a path this build no longer has. The Rust agent's env contract never read + // it (`WORKROOM_SESSION_{ID,SOCKET,SHELL,CWD,COMMAND,RESOURCES}` only), so it was left written + // by the app and read by nothing. guard let backend = backend(forSession: sessionID), let socketPath = socketPath(for: backend), - let binaryPath = binaryPath(for: backend) + binaryPath(for: backend) != nil else { return [] } var entries: [(key: String, value: String)] = [ ("WORKROOM_SESSION_ID", sessionID.uuidString), ("WORKROOM_SESSION_SOCKET", socketPath), - ("WORKROOM_SESSION_BINARY", binaryPath), ("WORKROOM_SESSION_SHELL", shell), ("WORKROOM_SESSION_CWD", workingDirectory), ("WORKROOM_SESSION_COMMAND", ""), @@ -267,7 +464,6 @@ final class PersistentSessionService { @discardableResult func endSession(sessionID: UUID) async -> Bool { - descriptors.removeValue(forKey: sessionID) // An UNKNOWN owner must report failure, not success. `reap` gates deleting the workroom's // directory on this (issue #7), so answering "killed" for a session we could not even route to // would delete a worktree out from under a live shell. A malformed id or a missing socket is @@ -291,7 +487,7 @@ final class PersistentSessionService { return true } let client = controlPlane(socketPath: socketPath, backend: owner) - // Resolved above, via `controlPlane(forSession:)`, then forgotten: this session is over, and + // Resolved above, via `backend(forSession:)`, then forgotten: this session is over, and // holding its owner would outlive the thing it describes. defer { owners.removeValue(forKey: sessionID) } let killed = await Task.detached(priority: .utility) { client.kill(identifier: identifier) } @@ -317,7 +513,6 @@ final class PersistentSessionService { /// looking at it right now. Pass an empty set (the default) to kill everything. func endAllSessions(excluding attachedSessionIDs: Set = []) async { guard !attachedSessionIDs.isEmpty else { - descriptors.removeAll() // Owners go with them: these session ids are dead, and a tab reopened later reuses its // persisted id (`TerminalSessions.assignedSessionID`) — a surviving entry would pin it to // the helper that held the session just killed. diff --git a/macapp/WorkroomApp/Core/Session/SessionBackend.swift b/macapp/WorkroomApp/Core/Session/SessionBackend.swift index e4d6d7581..c54acebea 100644 --- a/macapp/WorkroomApp/Core/Session/SessionBackend.swift +++ b/macapp/WorkroomApp/Core/Session/SessionBackend.swift @@ -1,37 +1,35 @@ import Foundation -/// Which process owns a persistent terminal session: the shipped Swift daemon, or the Rust agent -/// that replaces it (issue #154, `docs/designs/remote-workrooms.md`). +/// Which process owns a persistent terminal session: the Rust agent that owns every new one, or +/// the retired Swift daemon that may still be holding sessions from an older build (issue #154, +/// `docs/designs/remote-workrooms.md`). /// /// **This is resolved per SESSION, not per app, and never by the user.** The two are separate pty /// owners with separate sockets and cannot hand sessions to each other, so any app-wide switch -/// would strand whatever terminals were running at the moment it flipped. Instead the migration -/// drains: a session the daemon already owns stays with the daemon until it closes on its own, and -/// every new session goes to the agent. Nothing is taken away, so nothing is noticed, and the -/// daemon idles out and exits once its last session ends. +/// would strand whatever terminals were running at the moment it flipped. Instead: +/// +/// - **Every new session goes to the agent.** `preferred()` names it, or names nothing at all. +/// - **The daemon is reachable but no longer runnable.** `workroom-session` still ships, but only +/// its attach client: the `daemon` subcommand is gone, so nothing in this build can start one. +/// The daemons it attaches to were started by an app at or before v2.0.0 and are still running on +/// the user's machine. They exit on their own once their last session closes. +/// +/// **There is no fallback between them.** An agent that cannot run used to send new sessions to the +/// daemon; it now sends them nowhere, because a daemon this build cannot start is not a fallback — +/// routing at it would hand libghostty an attach command for a session nobody holds. `preferred()` +/// returns nil instead and the pane opens a plain shell: a working terminal without persistence, +/// rather than a dead one. /// -/// That is also what makes an automatic fallback safe here. Falling back would be dangerous if -/// both could own one session — two pty owners racing is the thing "unify" exists to prevent — but -/// the sockets are distinct and ownership is per session, so a session lives in exactly one of -/// them by construction. /// Not `Defaults.Serializable`: nothing stores this. Which backend owns a session is a fact about /// the machine's live processes, answered by asking them, and a stored value would go stale the /// moment a daemon exited or an upgrade landed. enum SessionBackend: String, CaseIterable, Sendable { - /// `workroom-session`, shipped and incident-hardened. The default, and the rollback target. + /// `workroom-session`, now an attach-only client. Never the destination for a NEW session; only + /// ever resolved for a session a pre-existing daemon says it already holds. case swiftDaemon = "swift" - /// `wr-agent`, the unified local+remote agent. + /// `wr-agent`, the unified local+remote agent. Where every new session goes. case rustAgent = "rust" - static let `default`: SessionBackend = .swiftDaemon - - var label: String { - switch self { - case .swiftDaemon: return "Swift daemon" - case .rustAgent: return "Rust agent" - } - } - /// The helper this backend forks. Distinct binaries, so neither can be mistaken for the other. var binaryName: String { switch self { @@ -40,10 +38,10 @@ enum SessionBackend: String, CaseIterable, Sendable { } } - /// Distinct socket file names, which is the property that makes a mid-flight switch safe: the + /// Distinct socket file names, which is the property that makes the two safe to hold at once: the /// two backends can never bind the same socket, so there is never a moment with two pty owners - /// racing for one session. It also means each backend's sessions survive independently while - /// the other is selected, instead of being silently adopted by a process that cannot own them. + /// racing for one session. It also means a daemon's sessions survive independently while the + /// agent owns everything new, instead of being silently adopted by a process that cannot own them. var socketFileName: String { switch self { case .swiftDaemon: return "session.sock" @@ -51,20 +49,26 @@ enum SessionBackend: String, CaseIterable, Sendable { } } - /// The backend a NEW session should be created in. + /// The backend a NEW session should be created in, or **nil when none can take one**. /// - /// The agent, unless it cannot run — see `SessionBackendProbe`. A build where the agent is - /// missing or broken keeps working on the daemon rather than failing to open a terminal. + /// Nil is a real answer, not an error: this build can start an agent and nothing else, so an + /// agent that fails its probe leaves nowhere for a new session to go. The caller opens a plain + /// shell. See `SessionBackendProbe` for what "cannot run" is measured by, and note it is a + /// liveness check rather than a guarantee — an agent that answers `protocol` can still fail at + /// attach, which `GhosttySurfaceView` handles separately. static func preferred( probe: (SessionBackend) -> SessionBackendAvailability = { SessionBackendProbe.probe($0) } - ) -> SessionBackend { - probe(.rustAgent).isReady ? .rustAgent : .swiftDaemon + ) -> SessionBackend? { + probe(.rustAgent).isReady ? .rustAgent : nil } } -/// Why a backend cannot be used right now. Distinguishing these matters: "not in this build" is a -/// packaging fact the user can do nothing about, while "did not respond" is a health failure that -/// is exactly the signal a rollback decision needs. +/// Why a backend cannot be used right now. +/// +/// The distinction used to be justified by a rollback decision; there is no rollback left to make +/// (see the type doc above). It survives because the two cases are diagnosed differently in a bug +/// report — "not in this build" is a packaging fault, "did not respond" is a runtime one — and +/// because only the second is worth re-probing, which is what `probeRetryInterval` does. enum SessionBackendAvailability: Equatable, Sendable { case ready(version: String) case notBundled @@ -74,12 +78,4 @@ enum SessionBackendAvailability: Equatable, Sendable { if case .ready = self { return true } return false } - - var summary: String { - switch self { - case .ready(let version): return "Ready — \(version)" - case .notBundled: return "Not included in this build" - case .unhealthy(let reason): return "Not responding — \(reason)" - } - } } diff --git a/macapp/WorkroomApp/Core/Session/TerminalPersistentSessionPolicy.swift b/macapp/WorkroomApp/Core/Session/TerminalPersistentSessionPolicy.swift index 6c1dfdddf..20a1a561b 100644 --- a/macapp/WorkroomApp/Core/Session/TerminalPersistentSessionPolicy.swift +++ b/macapp/WorkroomApp/Core/Session/TerminalPersistentSessionPolicy.swift @@ -5,12 +5,25 @@ enum TerminalPersistentSessionPolicy { /// Quick terminals aren't excluded here because they never reach this policy: /// `QuickTerminalController` builds its own `GhosttySurfaceView` in a bare `NSWindow`, bypassing /// `TerminalSessions` entirely, so they get no session ID by construction. + /// + /// **`isAvailable` gates only a NEW id.** It answers for the backend a new session would be + /// created in, so it goes false whenever that backend cannot run — and a pane RESTORED from a + /// previous launch already has an id naming a session some helper may still be holding. Running + /// that id through the same gate threw it away before anything could ask who owned it, which is + /// precisely the case the attach-only shim exists for: an unhealthy agent stranded every session + /// the Swift daemon was still holding. An id the pane already has is a fact about the past, not a + /// request for a new resource. + /// + /// Keeping the id when nothing can attach to it is harmless: `attachCommand(forSession:)` returns + /// nil for an unresolved owner and `GhosttySurfaceView` opens a plain shell, which is the same + /// outcome as having no id at all. static func usesPersistentSession( preferenceEnabled: Bool = Defaults[.backgroundSessions], isAvailable: Bool, isRunCommand: Bool, + hasExistingSession: Bool, isFixture: Bool = UITestFixture.isActive ) -> Bool { - preferenceEnabled && isAvailable && !isRunCommand && !isFixture + preferenceEnabled && (isAvailable || hasExistingSession) && !isRunCommand && !isFixture } } diff --git a/macapp/WorkroomApp/Core/Session/TerminalSessionAttachment.swift b/macapp/WorkroomApp/Core/Session/TerminalSessionAttachment.swift index 6a7c9a904..3c42b2192 100644 --- a/macapp/WorkroomApp/Core/Session/TerminalSessionAttachment.swift +++ b/macapp/WorkroomApp/Core/Session/TerminalSessionAttachment.swift @@ -19,6 +19,9 @@ enum TerminalSessionAttachment { } state.sessionID = sessionID state.view.persistentSessionID = sessionID + // Always restored: this path exists to point a pane at a session that already exists, chosen + // by the user from the detached-sessions list. + state.view.persistentSessionIsRestored = true state.view.reattachPersistentSession() tab.content = .terminal(state) sessions.replace(tab, for: target) diff --git a/macapp/WorkroomApp/Core/TerminalSessions.swift b/macapp/WorkroomApp/Core/TerminalSessions.swift index b7bc1e31d..460a7ff1f 100644 --- a/macapp/WorkroomApp/Core/TerminalSessions.swift +++ b/macapp/WorkroomApp/Core/TerminalSessions.swift @@ -2011,6 +2011,9 @@ final class TerminalSessions: ObservableObject { let view = makeView(target, cwd, command) let assignedSessionID = assignedSessionID(persisted: sessionID, isRunCommand: command != nil) view.persistentSessionID = assignedSessionID + // `sessionID` non-nil means this pane is being rebuilt from a restore payload, so its id names + // a session that may no longer exist. A fresh pane's id was minted a line ago. + view.persistentSessionIsRestored = sessionID != nil view.sessionMetadata = [ (SessionMetadataKey.project, projectPath(from: target.id) ?? target.path), (SessionMetadataKey.workroom, target.id), @@ -2118,7 +2121,8 @@ final class TerminalSessions: ObservableObject { private func assignedSessionID(persisted: UUID?, isRunCommand: Bool) -> UUID? { let policy = TerminalPersistentSessionPolicy.usesPersistentSession( isAvailable: PersistentSessionService.shared.isAvailable, - isRunCommand: isRunCommand) + isRunCommand: isRunCommand, + hasExistingSession: persisted != nil) guard policy else { return nil } return persisted ?? UUID() } diff --git a/macapp/WorkroomAppTests/Fixtures/workroom-session-v2.0.0 b/macapp/WorkroomAppTests/Fixtures/workroom-session-v2.0.0 new file mode 100755 index 000000000..3bf7dd5cb Binary files /dev/null and b/macapp/WorkroomAppTests/Fixtures/workroom-session-v2.0.0 differ diff --git a/macapp/WorkroomAppTests/Session/AgentControlPlaneTests.swift b/macapp/WorkroomAppTests/Session/AgentControlPlaneTests.swift index b45c835ff..8f23c8ecb 100644 --- a/macapp/WorkroomAppTests/Session/AgentControlPlaneTests.swift +++ b/macapp/WorkroomAppTests/Session/AgentControlPlaneTests.swift @@ -88,7 +88,7 @@ final class AgentControlPlaneTests: XCTestCase { defer { try? FileManager.default.removeItem(at: directory) } let socketPath = directory.appendingPathComponent("b.sock").path - let listener = try Self.listen(at: socketPath) + let listener = try UnixSocketListener.listen(at: socketPath) defer { close(listener) } let greeter = Thread { let accepted = accept(listener, nil, nil) @@ -110,29 +110,4 @@ final class AgentControlPlaneTests: XCTestCase { "a banner must be recognised as not-an-agent, not waited out") } - private static func listen(at socketPath: String) throws -> Int32 { - let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) - guard descriptor >= 0 else { throw XCTSkip("socket() failed") } - var address = sockaddr_un() - address.sun_family = sa_family_t(AF_UNIX) - let pathBytes = Array(socketPath.utf8) - guard pathBytes.count < MemoryLayout.size(ofValue: address.sun_path) else { - close(descriptor) - throw NSError(domain: "AgentControlPlaneTests", code: 1) - } - withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: pathBytes) } - let size = socklen_t(MemoryLayout.size) - let bound = withUnsafePointer(to: &address) { pointer in - pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { - Darwin.bind(descriptor, $0, size) - } - } - guard bound == 0, Darwin.listen(descriptor, 1) == 0 else { - close(descriptor) - throw NSError( - domain: "AgentControlPlaneTests", code: 2, - userInfo: [NSLocalizedDescriptionKey: "bind/listen failed: \(errno)"]) - } - return descriptor - } } diff --git a/macapp/WorkroomAppTests/Session/PersistentSessionAttachGateTests.swift b/macapp/WorkroomAppTests/Session/PersistentSessionAttachGateTests.swift new file mode 100644 index 000000000..289b7eb89 --- /dev/null +++ b/macapp/WorkroomAppTests/Session/PersistentSessionAttachGateTests.swift @@ -0,0 +1,180 @@ +import Foundation +import XCTest + +@testable import Workroom + +/// Thrown after an `XCTFail` so the test stops without the failure reading as a skip. The source +/// file is located by `#filePath` and therefore always exists: a symbol that cannot be found is +/// the test's subject moving, not a missing dependency. +private enum GateTestError: Error { + case symbolMissing +} + +/// The one thing about `GhosttySurfaceView.applyPersistentSession` that a behavioural test cannot +/// reach: that it does **not** consult the global `PersistentSessionService.isAvailable` before +/// attaching. +/// +/// **Why this is a source-parsing test.** The obvious version drives the service and asserts +/// `attachCommand(forSession:)` is non-nil while `isAvailable` is false — which is true whatever +/// the view does, so reintroducing the guard leaves it green. That was measured, not assumed: +/// putting the guard back and re-running `PersistentSessionRoutingTests` produced 3 passes. A test +/// that cannot fail for the reason it was written is worse than no test, and this repo has paid for +/// that twice already (see the commit "cover the wiring, not just the writer" and the "verify the +/// premise" rule in `macapp/CLAUDE.md`). +/// +/// `applyPersistentSession` is private, builds a `ghostty_surface_config_s`, and lives on an +/// `NSView` subclass that XCUITest cannot query, so the call site itself is the only observable. +/// `DefaultsIsolationTests.testEveryShippedKeyDeclaresTheAppSuite` already parses source to enforce +/// a rule the type system cannot, which is the same trade: a cheap, blunt check that actually fires. +/// +/// What it costs: it is sensitive to how the guard is spelled, not to what it means. A future +/// author could route the same value through a differently-named helper and slip past. It catches +/// the realistic regression — someone "restoring" the availability check — and nothing subtler. +final class PersistentSessionAttachGateTests: XCTestCase { + /// Walks up from this file rather than reading a bundle resource: the app sources are not in the + /// test bundle, and `#filePath` is stable across every configuration this suite runs in. + private static var surfaceViewSource: String { + get throws { + let url = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() // Session/ + .deletingLastPathComponent() // WorkroomAppTests/ + .deletingLastPathComponent() // macapp/ + .appendingPathComponent("WorkroomApp/Core/GhosttySurfaceView.swift") + return try String(contentsOf: url, encoding: .utf8) + } + } + + /// The body of `applyPersistentSession`, from its declaration to the next declaration at the same + /// indentation. + /// + /// Matching `"\n private func"` was wrong: the next declaration is `func + /// reattachPersistentSession()`, which is not private, so the extracted "body" ran on past the end + /// of the function it was supposed to bound. Matching any `func` at two-space indentation is what + /// the comment always claimed. + private static func applyPersistentSessionBody(in source: String) throws -> String { + guard let start = source.range(of: "private func applyPersistentSession") else { + XCTFail("applyPersistentSession has been renamed; update this test deliberately") + throw GateTestError.symbolMissing + } + let rest = source[start.upperBound...] + guard + let end = rest.range( + of: "\n (?:private |fileprivate |internal )?(?:static )?func ", + options: .regularExpression) + else { return String(rest) } + return String(rest[rest.startIndex..", $ARGV[0]) or exit 1; - print F $$; - close F; - while (1) { sleep 1 } - ' "$1" & - exec cat - """ - try script.write(to: scriptFile, atomically: true, encoding: .utf8) - try FileManager.default.setAttributes( - [.posixPermissions: 0o755], ofItemAtPath: scriptFile.path) - - let identifier = SessionIdentifier(UUID()) - var client = try SessionTestClient.connect(socketPath: harness.socketPath) - let attach = SessionAttachRequest( - identifier: identifier, - columns: 80, - rows: 24, - workingDirectory: workDirectory.path, - command: "/bin/sh \(scriptFile.path) \(pidFile.path)", - shell: "/bin/sh", - resourcesDirectory: "", - environment: [ - SessionEnvironmentEntry(key: "TERM", value: "dumb"), - SessionEnvironmentEntry(key: "PATH", value: "/bin:/usr/bin"), - ]) - try client.send(SessionFrame(kind: .attach, payload: attach.encoded())) - _ = try client.wait(for: .attached, timeout: 3) - - let startDeadline = Date().addingTimeInterval(3) - var grandchildPID: pid_t = 0 - while Date() < startDeadline { - if let contents = try? String(contentsOf: pidFile, encoding: .utf8), - let pid = pid_t(contents.trimmingCharacters(in: .whitespacesAndNewlines)) - { - grandchildPID = pid - break - } - usleep(20_000) - } - XCTAssertGreaterThan(grandchildPID, 0, "the setsid'd grandchild never started") - XCTAssertEqual(kill(grandchildPID, 0), 0, "the grandchild should be alive before killAll") - - var killer = try SessionTestClient.connect(socketPath: harness.socketPath) - try killer.send(SessionFrame(kind: .killAll)) - _ = try killer.wait(for: .acknowledged, timeout: 3) - killer.closeConnection() - - let goneDeadline = Date().addingTimeInterval(3) - var stillAlive = true - while Date() < goneDeadline { - if kill(grandchildPID, 0) != 0 { - stillAlive = false - break - } - usleep(20_000) - } - XCTAssertFalse(stillAlive, "a setsid'd grandchild must not survive killAll") - - client.closeConnection() - } - - func testVersionMismatchIsRefused() throws { - let harness = try SessionDaemonHarness.start() - defer { harness.stop() } - - var client = try SessionTestClient.connect(socketPath: harness.socketPath) - let identifier = SessionIdentifier(UUID()) - let request = SessionAttachRequest( - version: SessionProtocolVersion.current + 1, - identifier: identifier, - columns: 80, - rows: 24, - workingDirectory: "/", - command: "", - shell: "/bin/sh", - resourcesDirectory: "", - environment: []) - try client.send(SessionFrame(kind: .attach, payload: request.encoded())) - let failure = try client.wait(for: .failure, timeout: 2) - let message = try SessionTextPayload.decode(failure.payload) - XCTAssertTrue(message.contains("different version"), message) - client.closeConnection() - } -} - -/// Minimal unix-socket client for protocol tests. -private struct SessionTestClient { - let descriptor: Int32 - var decoder = SessionFrameDecoder() - - static func connect(socketPath: String) throws -> SessionTestClient { - let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) - guard descriptor >= 0 else { throw XCTSkip("socket() failed") } - var address = sockaddr_un() - address.sun_family = sa_family_t(AF_UNIX) - let path = Array(socketPath.utf8CString) - guard path.count < MemoryLayout.size(ofValue: address.sun_path) else { - close(descriptor) - throw NSError(domain: "SessionTestClient", code: 1) - } - withUnsafeMutablePointer(to: &address.sun_path) { pointer in - pointer.withMemoryRebound(to: CChar.self, capacity: path.count) { dest in - for (i, byte) in path.enumerated() { dest[i] = byte } - } - } - address.sun_len = UInt8(MemoryLayout.size) - let result = withUnsafePointer(to: &address) { pointer in - pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { casted in - Darwin.connect(descriptor, casted, socklen_t(MemoryLayout.size)) - } - } - guard result == 0 else { - close(descriptor) - throw NSError( - domain: "SessionTestClient", code: 2, - userInfo: [NSLocalizedDescriptionKey: "connect failed: \(errno)"]) - } - let flags = fcntl(descriptor, F_GETFL, 0) - _ = fcntl(descriptor, F_SETFL, flags | O_NONBLOCK) - return SessionTestClient(descriptor: descriptor) - } - - mutating func send(_ frame: SessionFrame) throws { - var bytes = frame.encoded() - var offset = 0 - while offset < bytes.count { - let written = bytes.withUnsafeBytes { pointer -> Int in - Darwin.write(descriptor, pointer.baseAddress!.advanced(by: offset), bytes.count - offset) - } - if written > 0 { - offset += written - continue - } - if errno == EAGAIN || errno == EINTR { - var pollfd = pollfd(fd: descriptor, events: Int16(POLLOUT), revents: 0) - _ = poll(&pollfd, 1, 1000) - continue - } - throw NSError(domain: "SessionTestClient", code: 3) - } - } - - mutating func wait(for kind: SessionFrameKind, timeout: TimeInterval) throws -> SessionFrame { - let deadline = Date().addingTimeInterval(timeout) - while Date() < deadline { - if let frame = try decoder.next() { - if frame.kind == kind { return frame } - continue - } - var pollfd = pollfd(fd: descriptor, events: Int16(POLLIN), revents: 0) - let remaining = deadline.timeIntervalSinceNow - let ready = poll(&pollfd, 1, Int32(max(remaining, 0) * 1000)) - if ready <= 0 { continue } - var buffer = [UInt8](repeating: 0, count: 64 * 1024) - let capacity = buffer.count - let count = buffer.withUnsafeMutableBytes { pointer in - Darwin.read(descriptor, pointer.baseAddress, capacity) - } - if count > 0 { - decoder.push(Array(buffer.prefix(count))) - } else if count == 0 { - throw NSError( - domain: "SessionTestClient", code: 4, - userInfo: [NSLocalizedDescriptionKey: "eof waiting for \(kind)"]) - } - } - throw NSError( - domain: "SessionTestClient", code: 5, - userInfo: [NSLocalizedDescriptionKey: "timeout waiting for \(kind)"]) - } - - func closeConnection() { - Darwin.close(descriptor) - } -} diff --git a/macapp/WorkroomAppTests/Session/SessionDaemonHarness.swift b/macapp/WorkroomAppTests/Session/SessionDaemonHarness.swift deleted file mode 100644 index 9ea6062c4..000000000 --- a/macapp/WorkroomAppTests/Session/SessionDaemonHarness.swift +++ /dev/null @@ -1,83 +0,0 @@ -import Foundation - -@testable import WorkroomSessionProtocol - -/// Launches a throwaway `workroom-session daemon` against a temp socket. -final class SessionDaemonHarness { - let socketPath: String - private let process: Process - private let directory: URL - - static func start() throws -> SessionDaemonHarness { - // sun_path is 104 bytes; NSTemporaryDirectory() + a UUID overflows it. - let directory = URL( - fileURLWithPath: "/tmp/wr-\(UUID().uuidString.prefix(8))", isDirectory: true) - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - let socketPath = directory.appendingPathComponent("s.sock").path - let binary = try binaryURL() - let process = Process() - process.executableURL = binary - process.arguments = ["daemon", "--socket", socketPath, "--idle-timeout", "5000"] - process.standardOutput = FileHandle.nullDevice - let errURL = directory.appendingPathComponent("daemon.err") - FileManager.default.createFile(atPath: errURL.path, contents: nil) - process.standardError = FileHandle(forWritingAtPath: errURL.path) - try process.run() - - let deadline = Date().addingTimeInterval(3) - while Date() < deadline { - if FileManager.default.fileExists(atPath: socketPath) { break } - if !process.isRunning { break } - Thread.sleep(forTimeInterval: 0.02) - } - guard FileManager.default.fileExists(atPath: socketPath) else { - let err = (try? String(contentsOf: errURL, encoding: .utf8)) ?? "" - let status = process.isRunning ? "running" : "exited \(process.terminationStatus)" - process.terminate() - throw NSError( - domain: "SessionDaemonHarness", code: 1, - userInfo: [ - NSLocalizedDescriptionKey: - "daemon socket never appeared at \(socketPath) (\(status), binary=\(binary.path), stderr=\(err))" - ]) - } - return SessionDaemonHarness(socketPath: socketPath, process: process, directory: directory) - } - - private init(socketPath: String, process: Process, directory: URL) { - self.socketPath = socketPath - self.process = process - self.directory = directory - } - - func stop() { - process.terminate() - process.waitUntilExit() - try? FileManager.default.removeItem(at: directory) - } - - static func binaryURL() throws -> URL { - let names = ["BUILT_PRODUCTS_DIR", "TARGET_BUILD_DIR"] - for name in names { - if let dir = ProcessInfo.processInfo.environment[name] { - let url = URL(fileURLWithPath: dir).appendingPathComponent("workroom-session") - if FileManager.default.isExecutableFile(atPath: url.path) { return url } - } - } - // Logic-test host: the helper sits next to xctest in BUILT_PRODUCTS_DIR, which - // XCTest does not always export. Walk up from this test bundle. - let embedded = Bundle.main.bundleURL.appendingPathComponent("Contents/MacOS/workroom-session") - if FileManager.default.isExecutableFile(atPath: embedded.path) { - return embedded - } - var candidate = Bundle(for: SessionDaemonHarness.self).bundleURL - for _ in 0..<6 { - let url = candidate.appendingPathComponent("workroom-session") - if FileManager.default.isExecutableFile(atPath: url.path) { return url } - candidate.deleteLastPathComponent() - } - throw NSError( - domain: "SessionDaemonHarness", code: 2, - userInfo: [NSLocalizedDescriptionKey: "workroom-session binary not found"]) - } -} diff --git a/macapp/WorkroomAppTests/Session/SessionOwnershipTests.swift b/macapp/WorkroomAppTests/Session/SessionOwnershipTests.swift index 121d62f27..714191581 100644 --- a/macapp/WorkroomAppTests/Session/SessionOwnershipTests.swift +++ b/macapp/WorkroomAppTests/Session/SessionOwnershipTests.swift @@ -43,7 +43,7 @@ final class SessionOwnershipTests: XCTestCase { /// two calls needs two of them, and reusing the path fails to bind with `EADDRINUSE`. private func fakeDaemon(reply: [SessionDescriptor]?) throws -> String { let socketPath = directory.appendingPathComponent("d\(servers.count).sock").path - let listener = try Self.listen(at: socketPath) + let listener = try UnixSocketListener.listen(at: socketPath) servers.append(listener) Thread.detachNewThread { let accepted = accept(listener, nil, nil) @@ -121,7 +121,7 @@ final class SessionOwnershipTests: XCTestCase { /// stays, nothing answers. func testAStaleSocketFileIsNotOwnedRatherThanUnreachable() throws { let socketPath = directory.appendingPathComponent("stale.sock").path - let listener = try Self.listen(at: socketPath) + let listener = try UnixSocketListener.listen(at: socketPath) close(listener) XCTAssertTrue( FileManager.default.fileExists(atPath: socketPath), @@ -145,48 +145,20 @@ final class SessionOwnershipTests: XCTestCase { } } - private static func listen(at socketPath: String) throws -> Int32 { - let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) - guard descriptor >= 0 else { - throw NSError(domain: "test", code: 1, userInfo: [NSLocalizedDescriptionKey: "socket()"]) - } - var address = sockaddr_un() - address.sun_family = sa_family_t(AF_UNIX) - let pathBytes = Array(socketPath.utf8) - guard pathBytes.count < MemoryLayout.size(ofValue: address.sun_path) else { - throw NSError( - domain: "test", code: 2, - userInfo: [NSLocalizedDescriptionKey: "socket path too long: \(socketPath)"]) - } - withUnsafeMutableBytes(of: &address.sun_path) { pointer in - pointer.withMemoryRebound(to: CChar.self) { dest in - for (index, byte) in pathBytes.enumerated() { dest[index] = CChar(bitPattern: byte) } - } - } - address.sun_len = UInt8(MemoryLayout.size) - let bound = withUnsafePointer(to: &address) { pointer in - pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { casted in - Darwin.bind(descriptor, casted, socklen_t(MemoryLayout.size)) - } - } - guard bound == 0, Darwin.listen(descriptor, 1) == 0 else { - close(descriptor) - throw NSError( - domain: "test", code: 3, - userInfo: [NSLocalizedDescriptionKey: "bind/listen failed: \(errno)"]) - } - return descriptor - } } /// The routing rule itself, exhaustively. /// -/// Six combinations, all of them asserted, because the interesting one is a deliberate asymmetry -/// that reads like a bug unless you know why: an unanswered probe resolves to the DAEMON even -/// though the daemon is the backend being migrated away from. +/// `preferred` is Optional now — nil means "no backend can take a NEW session", which is what a +/// failed agent probe produces since the daemon stopped being a fallback. That makes the nil row of +/// each case the one worth reading. final class SessionOwnerRuleTests: XCTestCase { - func testADaemonThatOwnsTheSessionKeepsIt() { - for preferred in SessionBackend.allCases { + /// The property the whole attach-only shim rests on, and the one an earlier draft of the change + /// broke: a daemon that CLAIMS the session still wins even when nothing can take a new one. If + /// this ever returns nil for `preferred: nil`, a user with an unhealthy agent loses every + /// terminal an older build left running. + func testADaemonThatOwnsTheSessionKeepsItEvenWithNowhereForNewOnes() { + for preferred in [SessionBackend?.none, .rustAgent, .swiftDaemon] { XCTAssertEqual( PersistentSessionService.owner(preferred: preferred, daemon: .owned), .swiftDaemon, "the daemon holds that pty and cannot hand it over, whatever new sessions do") @@ -194,11 +166,16 @@ final class SessionOwnerRuleTests: XCTestCase { } /// The drain. Without this the migration never happens — every session would stay on the daemon. + /// + /// REGRESSION: the `preferred: .swiftDaemon` row is gone. `preferred()` can no longer name the + /// daemon under any condition, so that input is unreachable and asserting on it pinned behaviour + /// that does not exist. The nil row replaces it, and is the live case. func testADefinitiveNoSendsTheSessionWhereNewOnesGo() { XCTAssertEqual( PersistentSessionService.owner(preferred: .rustAgent, daemon: .notOwned), .rustAgent) - XCTAssertEqual( - PersistentSessionService.owner(preferred: .swiftDaemon, daemon: .notOwned), .swiftDaemon) + XCTAssertNil( + PersistentSessionService.owner(preferred: nil, daemon: .notOwned), + "nothing can take a new session, and the daemon disclaimed this one — so there is nowhere") } /// An unanswered probe resolves to NEITHER helper. @@ -211,10 +188,616 @@ final class SessionOwnerRuleTests: XCTestCase { /// Nil means the pane opens a plain shell — visible and recoverable — instead of duplicating a /// pty, which is neither. func testAnUnansweredProbeResolvesToNeitherHelper() { - for preferred in SessionBackend.allCases { + for preferred in [SessionBackend?.none, .rustAgent, .swiftDaemon] { XCTAssertNil( PersistentSessionService.owner(preferred: preferred, daemon: .unreachable), "there is no safe guess: both helpers create-on-attach, so either one forks a duplicate") } } } + +/// The wiring, not just the rule. +/// +/// `owner(preferred:daemon:)` above is a pure function, and a test of it cannot tell whether +/// anything CALLS it correctly — the failure this repo has already paid for twice (see the commit +/// "cover the wiring, not just the writer"). These drive the real service through its injected +/// seams and assert on `attachCommand`, which is what `GhosttySurfaceView` actually consults. +final class PersistentSessionRoutingTests: XCTestCase { + /// The case the attach-only shim exists for, end to end through the service: the agent is + /// installed but broken, and a session the retired daemon still holds must STILL produce an + /// attach command. + /// + /// **This is NOT a control for the view's guard, and an earlier version of this comment claimed + /// it was.** Measured: restoring `PersistentSessionService.shared.isAvailable` to the guard in + /// `GhosttySurfaceView.applyPersistentSession` leaves all three tests here green, because they + /// drive the service directly and never reach the view. What they prove is the service-level + /// contract — that the state is reachable and answers correctly. The call site is pinned + /// separately by `PersistentSessionAttachGateTests`. + @MainActor + func testAnUnhealthyAgentStillReachesADaemonOwnedSession() { + let service = PersistentSessionService( + probe: { _ in .unhealthy(reason: "exited 127") }, + ownership: { _ in .owned }) + + XCTAssertNil(service.backend, "nothing can take a NEW session when the agent is unhealthy") + XCTAssertFalse(service.isAvailable, "the global gate is false, which is why it must not gate") + + let sessionID = UUID() + XCTAssertEqual( + service.backend(forSession: sessionID), .swiftDaemon, + "a daemon that claims the session owns it regardless of where new sessions go") + let command = service.attachCommand(forSession: sessionID) + XCTAssertNotNil( + command, + """ + an unhealthy agent stranded a session the daemon still holds. This is the whole reason the \ + attach client was kept — see docs/designs/remote-workrooms.md. + """) + XCTAssertTrue( + command?.hasSuffix(" attach") == true && command?.contains("workroom-session") == true, + "expected the attach-only client, got \(command ?? "nil")") + } + + /// And the other half: with nowhere for a new session and no daemon claiming this one, the + /// service says so rather than guessing, so the pane opens a plain shell. + @MainActor + func testAnUnhealthyAgentWithNoDaemonResolvesToNothing() { + let service = PersistentSessionService( + probe: { _ in .unhealthy(reason: "exited 127") }, + ownership: { _ in .notOwned }) + + XCTAssertNil(service.backend(forSession: UUID())) + XCTAssertNil(service.attachCommand(forSession: UUID())) + } + + /// The probe runs ONCE even when its answer is nil. + /// + /// `cachedPreferred` uses nil for the cached value, so before `preferredResolved` existed the + /// "no backend" answer was indistinguishable from "not asked yet" and re-probed on every access. + /// The real probe is a `Process()` with a 2-second deadline on the main actor, so an eight-pane + /// restore against a hung agent froze the app for ~16s. Counting calls is the only way to see it: + /// the returned value is identical either way. + @MainActor + func testTheAgentProbeRunsOnceEvenWhenItAnswersNothing() { + let probeCount = Counter() + let service = PersistentSessionService( + probe: { _ in + probeCount.increment() + return .unhealthy(reason: "hung") + }, + ownership: { _ in .notOwned }) + + for _ in 0..<5 { + _ = service.backend + _ = service.isAvailable + _ = service.socketPath + _ = service.binaryPath + } + + XCTAssertEqual( + probeCount.value, 1, + "a nil answer must cache; re-probing costs 2s of frozen main actor per access") + } + + /// …but not forever. A nil answer is retried after a cooldown. + /// + /// Latching it for the launch was worse than the behaviour it replaced: before this change a + /// failed probe fell back to the daemon, which could still be started, so a transient failure + /// degraded to daemon-backed persistence. Now it degrades to none at all, and the probe is a + /// 2-second-watchdogged `Process()` that a loaded machine or a slow first exec of a + /// freshly-updated binary is enough to trip once. Without a way back, that user has no persistent + /// terminals until they quit the app. + @MainActor + func testANilAnswerIsRetriedAfterTheCooldown() async { + let probeCount = Counter() + var clock = 1000.0 + let service = PersistentSessionService( + probe: { _ in + probeCount.increment() + return probeCount.value == 1 + ? .unhealthy(reason: "transient") : .ready(version: "protocol 1") + }, + ownership: { _ in .notOwned }, + now: { clock }) + + XCTAssertNil(service.backend) + XCTAssertEqual(probeCount.value, 1) + + clock += PersistentSessionService.probeRetryInterval - 1 + XCTAssertNil(service.backend, "still inside the cooldown") + XCTAssertEqual(probeCount.value, 1, "the cooldown is what keeps the main actor free") + + // Past the cooldown the agent gets another chance — but the caller does not wait for it. The + // retry is the whole reason this cannot be synchronous: the first probe is paid once at launch, + // whereas a retry recurs every 30s for as long as the agent stays unhealthy, and each one would + // freeze the main actor for the watchdog's 2 seconds. So the call that ARMS the retry still + // answers nil, and the answer arrives on a later read. + clock += 2 + XCTAssertNil(service.backend, "the retry is dispatched, not awaited") + + let deadline = Date().addingTimeInterval(5) + while service.backend == nil && Date() < deadline { + await Task.yield() + } + + XCTAssertEqual( + service.backend, .rustAgent, "the background answer lands and is cached from then on") + XCTAssertEqual(probeCount.value, 2) + } + + /// One retry in flight at a time. + /// + /// A same-turn burst of panes is already handled by the cooldown — they all read the same + /// `lastProbeAt`. What this covers is the case the cooldown cannot: a probe that is SLOW. The + /// watchdog gives it 2 seconds, the cooldown lapses at 30, so a helper wedged long enough would + /// otherwise accumulate one live `Process()` per retry with nothing to stop them piling up. + @MainActor + func testARetryStillInFlightIsNotJoinedByAnother() async { + let probeCount = Counter() + let released = DispatchSemaphore(value: 0) + var clock = 1000.0 + let service = PersistentSessionService( + probe: { _ in + probeCount.increment() + // Only the retries block. The first probe is synchronous, so waiting there would freeze + // the main actor this whole change exists to keep free. + if probeCount.value == 1 { return .unhealthy(reason: "transient") } + released.wait() + return .ready(version: "protocol 1") + }, + ownership: { _ in .notOwned }, + now: { clock }) + + XCTAssertNil(service.backend) + XCTAssertEqual(probeCount.value, 1) + + clock += PersistentSessionService.probeRetryInterval + 1 + XCTAssertNil(service.backend, "arms a retry, which has not answered yet") + + // Wait for that retry to actually be running: if the next read arrived first it would be + // arming the only background probe, and the guard would never come into it. + let startedAt = Date() + while probeCount.value < 2 && Date().timeIntervalSince(startedAt) < 5 { + await Task.yield() + } + XCTAssertEqual(probeCount.value, 2, "the retry reached the probe") + + clock += PersistentSessionService.probeRetryInterval + 1 + XCTAssertNil(service.backend, "the cooldown has lapsed again while the probe is still out") + + released.signal() + let deadline = Date().addingTimeInterval(5) + while service.backend == nil && Date() < deadline { + await Task.yield() + } + + XCTAssertEqual(service.backend, .rustAgent) + XCTAssertEqual( + probeCount.value, 2, "a second read past the cooldown must not start a second probe") + } + + /// **A wedged helper costs the window one timeout, not one per pane.** + /// + /// An unanswered ownership probe is a 2-second wait, and it is paid on the main actor during + /// terminal creation. A workroom restoring six panes against a v2.0.0 daemon that accepts and + /// then stalls used to pay it six times, serially: twelve seconds of frozen app, well past the + /// threshold that files an AppHang, for six copies of one answer. + /// + /// Each resolve asks twice — the daemon, then the agent, because a wedged daemon must not veto a + /// session the live agent holds — so the first session costs two probes and the rest cost none. + @MainActor + func testAWedgedHelperIsAskedOncePerCooldownNotOncePerPane() { + let probes = Counter() + var clock = 1000.0 + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in + probes.increment() + return .unreachable + }, + now: { clock }) + + XCTAssertNil(service.backend(forSession: UUID()), "an unanswered probe resolves to neither") + XCTAssertEqual(probes.value, 2, "the daemon, then the agent") + + for _ in 0..<5 { XCTAssertNil(service.backend(forSession: UUID())) } + XCTAssertEqual( + probes.value, 2, + """ + five more panes each waited out a helper that had just been proved silent. That is ~2s of \ + frozen main actor per pane, during terminal restore, when the answer was already known. + """) + + clock += PersistentSessionService.probeRetryInterval + 1 + XCTAssertNil(service.backend(forSession: UUID())) + XCTAssertEqual( + probes.value, 4, + "past the cooldown the helper is asked for real again; a wedge is not permanent") + } + + /// A successful answer is cached for good — no cooldown, no re-probe. An agent that has answered + /// does not stop existing, and re-running it would reintroduce the cost the cache exists to avoid. + @MainActor + func testAHealthyAnswerIsNeverReprobed() { + let probeCount = Counter() + var clock = 1000.0 + let service = PersistentSessionService( + probe: { _ in + probeCount.increment() + return .ready(version: "protocol 1") + }, + ownership: { _ in .notOwned }, + now: { clock }) + + XCTAssertEqual(service.backend, .rustAgent) + clock += PersistentSessionService.probeRetryInterval * 10 + XCTAssertEqual(service.backend, .rustAgent) + XCTAssertEqual(probeCount.value, 1) + } +} + +/// **`attachCommand` and `launchEnvironment` are consumed as a pair.** +/// +/// A pane asks twice: once for the binary to fork, once for the environment that binary reads. They +/// are separate calls, and nothing in either signature says they must agree — but if they disagree +/// the pane runs one helper pointed at the other's socket. The Swift client would bind the agent's +/// socket, which takes the path a live agent's sessions are reached through and serves none of +/// them. `macapp/CLAUDE.md` calls this load-bearing; until now nothing tested it. +final class PersistentSessionPairTests: XCTestCase { + /// A session the daemon holds: the attach-only client, on the daemon's socket. + @MainActor + func testADaemonOwnedSessionGetsTheDaemonsBinaryAndTheDaemonsSocket() { + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in .owned }) + let sessionID = UUID() + + let command = service.attachCommand(forSession: sessionID) + let environment = Dictionary( + uniqueKeysWithValues: service.launchEnvironment( + sessionID: sessionID, workingDirectory: NSTemporaryDirectory())) + + XCTAssertEqual(service.backend(forSession: sessionID), .swiftDaemon) + XCTAssertEqual(command?.contains("workroom-session"), true, "got \(command ?? "nil")") + XCTAssertEqual( + environment["WORKROOM_SESSION_SOCKET"], service.socketPath(for: .swiftDaemon), + "the daemon's client was handed a socket that is not the daemon's") + XCTAssertEqual(environment["WORKROOM_SESSION_ID"], sessionID.uuidString) + } + + /// And a session nothing claims goes to the agent, on the agent's socket. + @MainActor + func testAnAgentOwnedSessionGetsTheAgentsBinaryAndTheAgentsSocket() { + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in .notOwned }) + let sessionID = UUID() + + let command = service.attachCommand(forSession: sessionID) + let environment = Dictionary( + uniqueKeysWithValues: service.launchEnvironment( + sessionID: sessionID, workingDirectory: NSTemporaryDirectory())) + + XCTAssertEqual(service.backend(forSession: sessionID), .rustAgent) + XCTAssertEqual(command?.contains("wr-agent"), true, "got \(command ?? "nil")") + XCTAssertEqual( + environment["WORKROOM_SESSION_SOCKET"], service.socketPath(for: .rustAgent), + "the agent was handed a socket that is not the agent's") + } + + /// **The one that would actually break: ownership that answers differently each time.** + /// + /// A daemon on a loaded machine answers one probe and misses the next — a dropped answer is a + /// timeout, not an error, which is why `.unreachable` exists as its own case. The two calls a + /// pane makes are milliseconds apart but they are not atomic, so without the per-session cache + /// the first sees `.owned` and the second sees whatever the second probe returned. The pane then + /// forks `workroom-session` with the agent's socket in its environment. + /// + /// One probe per session is therefore not an optimisation. It is what makes the pair coherent. + @MainActor + func testAFlappingOwnershipProbeCannotProduceAMismatchedPair() { + let answers = Counter() + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + // Owned, then not owned, then owned… the worst case, not a random one. + ownership: { _ in + answers.increment() + return answers.value.isMultiple(of: 2) ? .notOwned : .owned + }) + let sessionID = UUID() + + let command = service.attachCommand(forSession: sessionID) + let environment = Dictionary( + uniqueKeysWithValues: service.launchEnvironment( + sessionID: sessionID, workingDirectory: NSTemporaryDirectory())) + + XCTAssertEqual( + answers.value, 1, "the session was resolved twice; the two answers need not agree") + + let socket = environment["WORKROOM_SESSION_SOCKET"] + let usesDaemonBinary = command?.contains("workroom-session") == true + XCTAssertEqual( + socket, service.socketPath(for: usesDaemonBinary ? .swiftDaemon : .rustAgent), + """ + the pane was told to run \(command ?? "nil") against \(socket ?? "nil"). One helper, the \ + other's socket: whichever binds first takes the path the other's sessions live on. + """) + } + + /// Neither, or both — never one. + /// + /// `attachCommand` returning non-nil beside an empty environment is the shape that already + /// shipped once: the relay started with no `WORKROOM_SESSION_ID`, exited 2, and because the + /// plain-shell fallback is chosen BEFORE the command is spawned, the pane died instead of + /// degrading. + @MainActor + func testAnUnresolvableSessionRefusesOnBothSidesAtOnce() { + let service = PersistentSessionService( + probe: { _ in .unhealthy(reason: "exited 127") }, + ownership: { _ in .notOwned }) + let sessionID = UUID() + + XCTAssertNil(service.attachCommand(forSession: sessionID)) + XCTAssertTrue( + service.launchEnvironment(sessionID: sessionID, workingDirectory: NSTemporaryDirectory()) + .isEmpty, + "a nil command beside a populated environment is a pane that dies rather than degrades") + } +} + +/// The substitution the shipped daemon performs on an id it does not hold, and the re-check that +/// stops us walking into it. +/// +/// `SessionDaemon.handleAttach` — in the v2.0.0 binary, which cannot be changed — ends in +/// `create(request:connection:)` rather than refusing. So attaching to a session whose shell has +/// exited does not fail: it silently forks a new one. The user gets a fresh prompt where their +/// build was, and nothing distinguishes it from a successful reattach. +final class DaemonSessionSubstitutionTests: XCTestCase { + /// The ordinary case: the daemon still has it, so nothing changes. + @MainActor + func testAStillHeldSessionIsAttachable() { + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in .owned }) + let sessionID = UUID() + XCTAssertEqual(service.backend(forSession: sessionID), .swiftDaemon) + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .attachable) + } + + /// The bug. The owner is resolved and cached while the daemon holds the session; the shell then + /// exits; a later reattach must NOT proceed on the cached answer. + @MainActor + func testASessionTheDaemonHasLostIsNotAttachedTo() { + let answers = OwnershipScript([.owned, .notOwned]) + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in answers.next() }) + let sessionID = UUID() + + // Resolved and cached while it was still held — this is what makes the cache stale later. + XCTAssertEqual(service.backend(forSession: sessionID), .swiftDaemon) + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .gone, + """ + attaching here would ask the v2.0.0 daemon for a session it no longer holds, and it creates \ + one rather than refusing — the user's running work is replaced by an empty shell that looks \ + exactly like a successful reattach. + """) + } + + /// And the stale answer is dropped, so a later reattach resolves afresh instead of arriving back + /// at the same wrong conclusion. + @MainActor + func testALostSessionDropsItsCachedOwner() { + let answers = OwnershipScript([.owned, .notOwned, .notOwned]) + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in answers.next() }) + let sessionID = UUID() + XCTAssertEqual(service.backend(forSession: sessionID), .swiftDaemon) + XCTAssertEqual(service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .gone) + + XCTAssertEqual( + service.backend(forSession: sessionID), .rustAgent, + "the cached daemon answer must be gone, so the session re-resolves to where new ones go") + } + + /// **A daemon that cannot answer still gets attached to.** `.unreachable` means we could not ask, + /// not that the session is gone — and the substitution needs the daemon responsive enough to + /// answer "not mine" and then fork. One too wedged to reply cannot produce the wrong result, so + /// refusing here would throw away a session that is probably still running. + @MainActor + func testAnUnreachableDaemonIsStillAttachedTo() { + let answers = OwnershipScript([.owned, .unreachable]) + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in answers.next() }) + let sessionID = UUID() + XCTAssertEqual(service.backend(forSession: sessionID), .swiftDaemon) + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .attachable) + } + + /// **A freshly minted id is never challenged.** It has never existed anywhere, so create-on-attach + /// is the wanted behaviour — asking would refuse every new pane. This is the guard that makes the + /// check safe to apply to BOTH backends rather than only the retired one. + @MainActor + func testAFreshSessionIsNeverChallenged() { + let probes = Counter() + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in + probes.increment() + return .notOwned + }) + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: UUID(), wasRestored: false), .attachable, + "a pane opening a brand new session must not be refused because nobody holds its id yet") + XCTAssertEqual(probes.value, 0, "a fresh id is not worth a round trip to anyone") + } + + /// The agent is re-checked too, now. It creates on first attach exactly as the daemon does + /// (`serve.rs`: guarded only by `sessions.contains(id)`), so checking only the retired backend + /// closed the hole on the one being drained and left it open on the one that owns every new + /// session — and once the drain completes, on the only one left. + @MainActor + func testAnAgentOwnedSessionIsAlsoChallenged() { + let answers = OwnershipScript([.notOwned, .notOwned]) + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in answers.next() }) + let sessionID = UUID() + XCTAssertEqual(service.backend(forSession: sessionID), .rustAgent) + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .gone, + """ + an agent that disclaims a restored id would CREATE a new session on attach, exactly as the \ + daemon does + """) + } + + /// REGRESSION, and the shape of the hole this replaced. + /// + /// This test used to assert that an agent-owned session is NOT re-checked — "the agent refuses an + /// id it does not hold, so there is no substitution to prevent". That premise was false: + /// `serve.rs` creates on first attach guarded only by `sessions.contains(id)`, exactly as the + /// daemon does. Checking only the retired backend closed the hole on the one being drained and + /// left it open on the one that owns every new session — and, once the drain finishes, on the + /// only one left. The inverted assertion lives in `testAnAgentOwnedSessionIsAlsoChallenged`; what + /// remains here is the cost question, which the answer above must not regress. + @MainActor + func testAChallengeCostsOneRoundTripAtMost() { + let probes = Counter() + let service = PersistentSessionService( + probe: { _ in .ready(version: "protocol 1") }, + ownership: { _ in + probes.increment() + return .owned + }) + let sessionID = UUID() + _ = service.backend(forSession: sessionID) + let afterResolve = probes.value + + XCTAssertEqual( + service.confirmBeforeAttach(sessionID: sessionID, wasRestored: true), .attachable) + XCTAssertEqual( + probes.value, afterResolve + 1, + "confirming must cost exactly one ask — it runs on the main actor during pane creation") + } +} + +/// The notice a pane shows when its session is gone. +final class LostSessionCommandTests: XCTestCase { + /// It has to actually run. A malformed command string is a pane that opens to nothing, which is + /// worse than the silent fresh prompt this replaces. + func testTheNoticeCommandRunsAndPrintsThenBecomesTheShell() throws { + let command = GhosttySurfaceView.lostSessionCommand(shell: "/bin/sh") + let process = Process() + process.executableURL = URL(fileURLWithPath: "/bin/sh") + process.arguments = ["-c", command] + let pipe = Pipe() + let input = Pipe() + process.standardOutput = pipe + process.standardError = pipe + process.standardInput = input + try process.run() + + // Typed AT the pane, not redirected from `/dev/null`. The earlier version ended the shell with + // EOF and could only see the notice — which `printf` alone would satisfy, leaving a pane that + // explains itself and then dies. Feeding it a script proves there is a live shell on the other + // side to read it, which is the half of the name this test was not testing. + input.fileHandleForWriting.write(Data("echo ARGV0=$0\nexit 3\n".utf8)) + try input.fileHandleForWriting.close() + let data = pipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + + let output = String(decoding: data, as: UTF8.self) + XCTAssertTrue( + output.contains("has ended, so this is a new shell"), + "the pane would come back with no explanation. got: \(output)") + XCTAssertTrue( + output.contains("ARGV0=/bin/sh"), + """ + nothing read what was typed, so the notice printed into a pane with no shell behind it — \ + `wait_after_command` is false, so that pane is dead, not idle. got: \(output) + """) + XCTAssertEqual( + process.terminationStatus, 3, + """ + the exit status came from somewhere other than the shell we typed `exit 3` at. `exec` is what \ + makes this one process rather than a wrapper holding a child, and a wrapper would report its \ + own status here. got: \(output) + """) + } + + /// A shell path carrying shell metacharacters must be taken literally. `SHELL` is + /// environment-supplied, so it is not ours to trust, and this string is handed to `/bin/sh -c`. + /// + /// Asserted by RUNNING it and checking the side effect did not happen. The first version of this + /// test looked for the injected text as a substring of the command and failed — correctly quoted + /// output still contains it, inside quotes, which is the whole point. A substring check cannot + /// tell "quoted" from "escaped"; only execution can. + func testAnAwkwardShellPathIsQuotedNotInterpreted() throws { + let marker = URL(fileURLWithPath: "/tmp/wr-quoting-\(UUID().uuidString.prefix(8))") + let hostile = "/nonexistent/sh'; touch \(marker.path); '" + let command = GhosttySurfaceView.lostSessionCommand(shell: hostile) + + let process = Process() + process.executableURL = URL(fileURLWithPath: "/bin/sh") + process.arguments = ["-c", "\(command) < /dev/null"] + process.standardOutput = FileHandle.nullDevice + process.standardError = FileHandle.nullDevice + try process.run() + process.waitUntilExit() + + let escaped = FileManager.default.fileExists(atPath: marker.path) + try? FileManager.default.removeItem(at: marker) + XCTAssertFalse( + escaped, + "a shell path from the environment broke out of its quoting and ran: \(command)") + } +} + +/// Answers a scripted sequence of ownership results, then repeats the last one. +private final class OwnershipScript: @unchecked Sendable { + private let lock = NSLock() + private var answers: [SessionOwnership] + private var index = 0 + + init(_ answers: [SessionOwnership]) { + self.answers = answers + } + + func next() -> SessionOwnership { + lock.lock() + defer { lock.unlock() } + let answer = answers[min(index, answers.count - 1)] + index += 1 + return answer + } +} + +/// A plain counter rather than a captured `var`: the closure is `@escaping` and stored on the +/// service, so it cannot capture a local mutable. +private final class Counter: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + func increment() { + lock.lock() + count += 1 + lock.unlock() + } + + var value: Int { + lock.lock() + defer { lock.unlock() } + return count + } +} diff --git a/macapp/WorkroomAppTests/Session/SessionReplayBufferTests.swift b/macapp/WorkroomAppTests/Session/SessionReplayBufferTests.swift deleted file mode 100644 index 7cb438f05..000000000 --- a/macapp/WorkroomAppTests/Session/SessionReplayBufferTests.swift +++ /dev/null @@ -1,126 +0,0 @@ -import XCTest - -@testable import WorkroomSessionProtocol - -final class SessionReplayBufferTests: XCTestCase { - func testWrapsAndKeepsNewestBytes() { - var buffer = SessionReplayBuffer(capacity: 8) - buffer.append(Array("abcdefgh".utf8)) - buffer.append(Array("ij".utf8)) - XCTAssertEqual(String(bytes: buffer.bytes, encoding: .utf8), "cdefghij") - } - - func testReplaySkipsAlternateScreen() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append(Array("before".utf8)) - buffer.append([0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x39, 0x68]) // enter alt - buffer.append(Array("hidden".utf8)) - XCTAssertTrue(buffer.isAlternateScreenActive) - XCTAssertEqual(buffer.replayBytes, []) - buffer.append([0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x39, 0x6C]) // leave alt - buffer.append(Array("after".utf8)) - XCTAssertFalse(buffer.isAlternateScreenActive) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "after") - } - - func testRemoveAllClearsState() { - var buffer = SessionReplayBuffer(capacity: 16) - buffer.append(Array("hello".utf8)) - buffer.removeAll() - XCTAssertTrue(buffer.isEmpty) - XCTAssertEqual(buffer.replayBytes, []) - } - - func testReplayStripsColorQueryOSC() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append(Array("before".utf8)) - buffer.append([0x1B, 0x5D] + Array("11;?".utf8) + [0x07]) // OSC 11 query (BEL) - buffer.append(Array("after".utf8)) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "beforeafter") - } - - func testReplayStripsColorQueryOSCWithSTTerminator() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append([0x1B, 0x5D] + Array("10;?".utf8) + [0x1B, 0x5C]) // OSC 10 query (ST) - buffer.append(Array("after".utf8)) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "after") - } - - func testReplayKeepsColorSetOSC() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append([0x1B, 0x5D] + Array("11;rgb:1d1d/1d1d/1f1f".utf8) + [0x07]) // OSC 11 SET - let replayed = buffer.replayBytes - XCTAssertEqual(String(bytes: replayed, encoding: .utf8), "\u{1B}]11;rgb:1d1d/1d1d/1f1f\u{07}") - } - - func testReplayKeepsTitleEndingInQuestionMark() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append([0x1B, 0x5D] + Array("2;Continue?".utf8) + [0x07]) // OSC 2 title, not a query - let replayed = buffer.replayBytes - XCTAssertEqual(String(bytes: replayed, encoding: .utf8), "\u{1B}]2;Continue?\u{07}") - } - - func testReplayStripsDeviceAttributesAndStatusReportCSI() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append(Array("before".utf8) + [0x1B, 0x5B, 0x36, 0x6E]) // CSI 6n (cursor position) - buffer.append([0x1B, 0x5B, 0x3E, 0x63]) // CSI >c (device attributes) - buffer.append(Array("after".utf8)) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "beforeafter") - } - - func testReplayStripsKittyKeyboardQuery() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append(Array("before".utf8) + [0x1B, 0x5B, 0x3F, 0x75]) // CSI ?u (kitty flags query) - buffer.append(Array("after".utf8)) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "beforeafter") - } - - func testReplayStripsClipboardQueryOSC52() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append(Array("before".utf8)) - buffer.append([0x1B, 0x5D] + Array("52;c;?".utf8) + [0x07]) // OSC 52 clipboard read query - buffer.append(Array("after".utf8)) - XCTAssertEqual(String(bytes: buffer.replayBytes, encoding: .utf8), "beforeafter") - } - - func testReplayKeepsClipboardWriteOSC52() { - var buffer = SessionReplayBuffer(capacity: 256) - // A real OSC 52 write payload is base64 — never ends in a bare `?` — so it must survive replay. - buffer.append([0x1B, 0x5D] + Array("52;c;aGVsbG8=".utf8) + [0x07]) - let replayed = buffer.replayBytes - XCTAssertEqual(String(bytes: replayed, encoding: .utf8), "\u{1B}]52;c;aGVsbG8=\u{07}") - } - - func testReplayKeepsKittyKeyboardSetPushPop() { - var buffer = SessionReplayBuffer(capacity: 256) - buffer.append([0x1B, 0x5B, 0x3E, 0x35, 0x75]) // CSI >5u (push flags=5) — not a query - buffer.append([0x1B, 0x5B, 0x3D, 0x35, 0x3B, 0x31, 0x75]) // CSI =5;1u (set) — not a query - buffer.append([0x1B, 0x5B, 0x3C, 0x75]) // CSI 5u\u{1B}[=5;1u\u{1B}[ String? { - invocation.environment.first { $0.key == key }?.value - } -} diff --git a/macapp/WorkroomAppTests/Session/SessionShimCompatibilityTests.swift b/macapp/WorkroomAppTests/Session/SessionShimCompatibilityTests.swift new file mode 100644 index 000000000..a55d1a832 --- /dev/null +++ b/macapp/WorkroomAppTests/Session/SessionShimCompatibilityTests.swift @@ -0,0 +1,892 @@ +import Darwin +import Foundation +import WorkroomSessionProtocol +import XCTest + +@testable import Workroom + +/// The attach-only `workroom-session` against the daemon it actually has to talk to: the one +/// shipped in **v2.0.0**, pinned as a binary fixture. +/// +/// **Why a pinned binary and not a fake.** The obvious cheap version of this test is a stub that +/// binds a socket and answers with `SessionFrame`s. It proves nothing. A fake built from +/// `WorkroomSessionProtocol` reads the same source the client does, so a change to that module +/// moves both sides together and the test stays green through exactly the break it was written to +/// catch. The only peer whose behaviour cannot drift with our source tree is a binary that was +/// compiled before it: `Fixtures/workroom-session-v2.0.0`, lifted unmodified from +/// `workroom-macos-app_2.0.0.dmg` (sha256 6f75e6f7…), which is what users in the field are running. +/// +/// This is the whole justification for keeping the client half of `macapp/WorkroomSession/` at all. +/// If these fail, a user who updates while holding a terminal from an older build loses it — see +/// `docs/designs/remote-workrooms.md`. **`WorkroomSessionProtocol` is frozen while the shim ships** +/// precisely because this peer can never be recompiled to match a change. + +/// Thrown after an `XCTFail` so the test stops without the failure being reported as a skip. +private enum CompatibilityFixtureError: Error { + case unusable +} + +final class SessionShimCompatibilityTests: XCTestCase { + private var directory: URL! + private var daemon: Process? + + override func setUpWithError() throws { + try super.setUpWithError() + // sun_path is 104 bytes; the per-test temporary directory blows that on its own. + directory = URL(fileURLWithPath: "/tmp/wr-compat-\(UUID().uuidString.prefix(8))") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + } + + override func tearDownWithError() throws { + if let daemon, daemon.isRunning { + daemon.terminate() + daemon.waitUntilExit() + } + daemon = nil + try? FileManager.default.removeItem(at: directory) + try super.tearDownWithError() + } + + /// The property the shim exists for: a session created by the v2.0.0 daemon, whose creating + /// client has since gone away, is reattachable by THIS build — and the reattaching client is + /// handed the output the shell produced before it existed. + /// + /// Asserting on the replayed marker rather than on "no error" is deliberate. A client that + /// connects, fails the handshake and exits quietly would pass a liveness check; only bytes that + /// originated in the old daemon's pty prove the session was really reached. + func testReattachesASessionTheShippedDaemonHolds() throws { + let socketPath = try startShippedDaemon() + let sessionID = SessionIdentifier(UUID()) + let marker = "SHIM-COMPAT-\(UUID().uuidString.prefix(8))" + let sentinel = directory.appendingPathComponent("printed").path + + // Created by the SHIPPED client, then detached: `/dev/null` on stdin ends it immediately while + // the daemon keeps the pty, which is exactly the state a user's machine is left in when they + // quit an older build of the app. + let creator = try run( + Self.shippedBinaryURL, arguments: ["attach"], + environment: attachEnvironment( + sessionID: sessionID, socketPath: socketPath, + command: "sh -c 'echo \(marker); touch \(sentinel); sleep 120'")) + creator.waitUntilExit() + + // Wait for the marker to be firmly in the PAST before attaching. Without this the new client + // could be receiving it as live output and the test would pass either way — it would prove the + // session exists, not that its history was replayed, which is the property a user notices. + let deadline = Date().addingTimeInterval(10) + while Date() < deadline, !FileManager.default.fileExists(atPath: sentinel) { + Thread.sleep(forTimeInterval: 0.05) + } + XCTAssertTrue( + FileManager.default.fileExists(atPath: sentinel), + "the v2.0.0 daemon never ran the session's command; nothing to replay") + + let output = try attachWithCurrentBuild(sessionID: sessionID, socketPath: socketPath) + + XCTAssertTrue( + output.contains(marker), + """ + the attach-only client did not receive the v2.0.0 daemon's REPLAY for a session it holds — \ + the marker was already printed before this client connected. A user updating with a terminal \ + open would come back to a blank pane. Got: \(output.suffix(400)) + """) + } + + /// **Typing.** The reattach test above proves bytes flow from the daemon to the pane; this one + /// proves they flow the other way, which is the half a user notices within a second of coming + /// back to a restored terminal. + /// + /// The observable is a file, not the echo. Echo would also appear if the client merely rendered + /// what was typed locally without it ever reaching the session, which is exactly the failure + /// worth catching. + func testTypingInTheReattachedPaneReachesTheV2Session() throws { + let socketPath = try startShippedDaemon() + let session = try startShellSession(on: socketPath) + let sentinel = directory.appendingPathComponent("typed").path + + let attached = try attachInteractively( + sessionID: session.identifier, socketPath: socketPath) + defer { attached.terminate() } + XCTAssertTrue( + attached.text().contains(session.marker), + "this client got a session the daemon made for it, not the one already running") + attached.type("touch \(sentinel)\n") + + XCTAssertTrue( + waitForFile(at: sentinel), + """ + keystrokes did not reach a session the v2.0.0 daemon holds. The pane repaints and then \ + ignores the user. got: \(attached.text().suffix(400)) + """) + } + + /// **Ending.** The session's shell exits, and the pane must go with it carrying the same status. + /// + /// Everything else in this file is about a relay that must NOT exit (`wait_after_command` is + /// false, so exiting leaves a dead pane). This is the one case where exiting is correct, and the + /// two are one `if` apart in the same function — so the ordinary end of a terminal gets a test + /// beside the failures. + func testTheV2SessionsExitStatusBecomesTheClientsExitStatus() throws { + let socketPath = try startShippedDaemon() + let session = try startShellSession(on: socketPath) + + let attached = try attachInteractively( + sessionID: session.identifier, socketPath: socketPath) + defer { attached.terminate() } + XCTAssertTrue( + attached.text().contains(session.marker), + "this client got a session the daemon made for it, not the one already running") + attached.type("exit 7\n") + + XCTAssertTrue( + attached.waitUntilExit(within: 10), + "the client outlived the session's shell. got: \(attached.text().suffix(400))") + XCTAssertEqual( + attached.status(), 7, + """ + the session exited 7 and the pane reported \(attached.status()). A shell's exit status is \ + what a script wrapping the pane reads, and 0 for a failed command is worse than no status. + """) + XCTAssertFalse( + attached.text().contains("will not survive quitting"), + "a clean session exit took the fallback path, which is for a FAILED attach") + } + + /// **The helper dies mid-relay.** Not a failed attach — an attach that WORKED, and then the + /// socket under it went away. + /// + /// Found by review, and the branch had no coverage at all: `transportOutcome(isAttached:)` + /// returns `transportFailure` only after the `.attached` frame has landed, so nothing else in + /// this file can reach it. The review's proposed fix was to stop falling back here, on the + /// grounds that an active session should not be replaced by a fresh shell. The premise is right + /// and the conclusion is not: `wait_after_command` is false, so not falling back does not + /// preserve anything — it leaves a pane with no shell in it and no way to read the error. What + /// the post-attach case does need is its own message, because the pre-attach one says + /// persistence is gone and here the session may simply be out of reach. + func testTheHelperDyingMidRelayLeavesAWorkingShellNotADeadPane() throws { + // The pane under test is the thing on the other end of this pipe. If it dies, writing to it is + // EPIPE, and the default disposition would kill the test host instead of failing the test. + // + // Restored on the way out. The disposition is process-wide and `exec` preserves an IGNORED + // signal, so leaving it set would follow every later test and every process they spawn out of + // this one — a shell that ignores SIGPIPE does not end a pipeline when its reader closes. + let previousSIGPIPE = signal(SIGPIPE, SIG_IGN) + defer { signal(SIGPIPE, previousSIGPIPE) } + let socketPath = try startShippedDaemon() + let session = try startShellSession(on: socketPath) + let sentinel = directory.appendingPathComponent("after-death").path + + let attached = try attachInteractively( + sessionID: session.identifier, socketPath: socketPath) + defer { attached.terminate() } + XCTAssertTrue( + attached.text().contains(session.marker), "never reattached, so this proves nothing") + + // The relay is live and pumping. Take the helper out from under it. + daemon?.terminate() + daemon?.waitUntilExit() + + let deadline = Date().addingTimeInterval(10) + while Date() < deadline, !attached.text().contains("lost the connection") { + Thread.sleep(forTimeInterval: 0.05) + } + XCTAssertTrue( + attached.text().contains("lost the connection"), + "the pane was not told what happened. got: \(attached.text().suffix(400))") + XCTAssertFalse( + attached.text().contains("no session helper is listening"), + """ + a session that had already attached reported the pre-attach message, which tells the user \ + their session is gone when it may only be unreachable. + """) + + // The real assertion: there is a shell here, not a corpse. + attached.type("touch \(sentinel)\n") + XCTAssertTrue( + waitForFile(at: sentinel), + """ + the pane died with the helper. `wait_after_command` is false, so an exit here leaves nothing \ + to read the error in and nothing to type into. got: \(attached.text().suffix(400)) + """) + } + + /// **The control plane, not just the pty.** `list`, `info` and `kill` are how the app enumerates + /// and tears down sessions, and this build sends all three at a daemon it can never recompile. + /// + /// `ownership(identifier:)` is in here on purpose: it is the newest method on the protocol — + /// added because `list()` folds "no reply" into `[]` — and it is the one the reattach guard + /// consults before letting a pane attach. Its answer against the REAL v2.0.0 daemon is the thing + /// that decides whether a user's terminal is reconnected or replaced. + func testTheControlPlaneStillWorksAgainstTheShippedDaemon() throws { + let socketPath = try startShippedDaemon() + let session = try startShellSession(on: socketPath) + let client = PersistentSessionControlClient(socketPath: socketPath) + + let listed = client.list() + XCTAssertTrue( + listed.contains { $0.identifier == session.identifier }, + "this build's `list` could not see a session the v2.0.0 daemon is holding: \(listed)") + XCTAssertEqual(client.info(identifier: session.identifier)?.identifier, session.identifier) + guard case .owned = client.ownership(identifier: session.identifier) else { + return XCTFail("the daemon holds this session but did not claim it, so a reattach is refused") + } + + XCTAssertTrue( + client.kill(identifier: session.identifier), + "the v2.0.0 daemon refused this build's `kill`; closing a pane would leave the pty behind") + + // Killed, then gone: the daemon reaps asynchronously, so poll rather than assert on the next + // line and call a race a regression. + let deadline = Date().addingTimeInterval(5) + while Date() < deadline { + if !client.list().contains(where: { $0.identifier == session.identifier }) { break } + Thread.sleep(forTimeInterval: 0.05) + } + XCTAssertFalse( + client.list().contains { $0.identifier == session.identifier }, + "the session survived a kill this build acknowledged") + guard case .notOwned = client.ownership(identifier: session.identifier) else { + return XCTFail( + """ + a killed session still reads as owned, so a pane would attach to it — and the v2.0.0 \ + daemon creates on attach, which silently replaces the user's shell with a fresh one. + """) + } + } + + /// The version handshake, ported from the deleted `SessionDaemonEndToEndTests` — the only test + /// anywhere that exercised it, and the one most likely to be broken by an edit to + /// `WorkroomSessionProtocol` made for the app's own control client. + /// + /// Driven straight against the SHIPPED daemon rather than a rebuilt one, so it measures the + /// agreement that actually matters: what this tree encodes against what v2.0.0 decodes. + func testTheShippedDaemonRefusesAFutureProtocolVersion() throws { + let socketPath = try startShippedDaemon() + var client = try SessionTestClient.connect(socketPath: socketPath) + defer { client.closeConnection() } + + let request = SessionAttachRequest( + version: SessionProtocolVersion.current + 1, + identifier: SessionIdentifier(UUID()), + columns: 80, + rows: 24, + workingDirectory: "/", + command: "", + shell: "/bin/sh", + resourcesDirectory: "", + environment: []) + try client.send(SessionFrame(kind: .attach, payload: request.encoded())) + + let failure = try client.wait(for: .failure, timeout: 5) + let message = try SessionTextPayload.decode(failure.payload) + XCTAssertTrue(message.contains("different version"), message) + } + + /// The current version is accepted by the same peer. Without this, the test above passes just as + /// well when EVERY version is refused — which is what a real encoding break looks like. + func testTheShippedDaemonAcceptsTheCurrentProtocolVersion() throws { + let socketPath = try startShippedDaemon() + var client = try SessionTestClient.connect(socketPath: socketPath) + defer { client.closeConnection() } + + let request = SessionAttachRequest( + identifier: SessionIdentifier(UUID()), + columns: 80, + rows: 24, + workingDirectory: directory.path, + command: "sh -c 'sleep 30'", + shell: "/bin/sh", + resourcesDirectory: "", + environment: []) + try client.send(SessionFrame(kind: .attach, payload: request.encoded())) + + XCTAssertNoThrow( + try client.wait(for: .attached, timeout: 5), + "v2.0.0 refused a request encoded by this tree — WorkroomSessionProtocol has drifted") + } + + // MARK: - The attach-only client's own failure modes + + /// The retirement itself, asserted against the shipped binary rather than against the source. + /// + /// `workroom-session daemon` started a pty daemon in v2.0.0. This build must not: every new + /// session belongs to `wr-agent`, and a daemon started now would hold none of the sessions this + /// client exists to reach — it would bind the socket the OLD daemon's sessions live on and + /// answer for none of them. + func testTheDaemonSubcommandIsGone() throws { + let socketPath = directory.appendingPathComponent("never.sock").path + let (output, status) = try runCapturing( + arguments: ["daemon", "--socket", socketPath], environment: [:]) + + XCTAssertEqual(status, 2, "`daemon` must be rejected as an unknown command. got: \(output)") + XCTAssertTrue( + output.contains("unknown command"), "expected an unknown-command error: \(output)") + XCTAssertFalse( + FileManager.default.fileExists(atPath: socketPath), + """ + this build bound a daemon socket. The pty daemon is retired: a new one would take the socket \ + the sessions this client reattaches to are reached through, and hold none of them. + """) + + let (usage, usageStatus) = try runCapturing(arguments: [], environment: [:]) + XCTAssertEqual(usageStatus, 2) + XCTAssertFalse( + usage.contains("daemon"), + "the usage text still advertises a subcommand this binary no longer has: \(usage)") + } + + /// Nothing listening at all: the session the client was asked to reach is gone. + /// + /// **The elapsed bound is the real assertion.** `connect` used to spawn a daemon and wait for it + /// to bind — 3 cycles of 50 × 20ms, three times over, ~9.2s of a pane showing nothing before the + /// user was told anything. With no daemon to start there is nothing to wait for, and the budget + /// is ~0.8s. A regression that restores the wait cannot be seen any other way: the exit code and + /// the message are the same either way. + func testAttachWithNothingListeningFailsFastAndStartsNothing() throws { + let socketPath = directory.appendingPathComponent("absent.sock").path + let started = Date() + let (output, status) = try runCapturing( + arguments: ["attach"], + environment: attachEnvironment( + sessionID: SessionIdentifier(UUID()), socketPath: socketPath, command: "")) + let elapsed = Date().timeIntervalSince(started) + + // The client BECOMES a shell rather than exiting 92, so the status is the shell's. Asserting + // 92 here pinned the dead pane: `config.wait_after_command` is false, so a relay that exits + // leaves nothing to read the message in. What must still hold is that it says why first. + XCTAssertTrue( + output.contains("no session helper is listening"), + "the pane fell back with no explanation of why: \(output)") + XCTAssertTrue( + output.contains("will not survive quitting"), + "the user was not told persistence was lost: \(output)") + XCTAssertNotEqual( + status, 92, + "exiting 92 means the relay died instead of becoming a shell, which is the dead pane") + XCTAssertFalse( + FileManager.default.fileExists(atPath: socketPath), + "the client started a helper. It cannot serve any session that exists, and it takes the " + + "socket a real one would need.") + XCTAssertLessThan( + elapsed, 5, + """ + the client spent \(elapsed)s before giving up. The spawn-and-wait path is gone, so the \ + budget is ~0.8s; anything near 9s means it is back and the pane shows nothing meanwhile. + """) + } + + /// **A helper that accepts and then says nothing.** Wedged, not absent — and nothing bounded it: + /// `poll` blocks indefinitely once no settle check is pending, so the pane sat blank forever with + /// no message and no exit. The connect retries above bound only a REFUSED connect, which is a + /// different failure entirely. + /// + /// The peer is held open well past the 5s deadline on purpose. Closing sooner is an EOF, which + /// takes the retry path and reports "no session helper is listening" — so a short-sleeping fake + /// would make this test pass while measuring the wrong thing. + func testAWedgedHelperIsGivenUpOnRatherThanHangingThePane() throws { + let socketPath = directory.appendingPathComponent("wedged.sock").path + let listener = try UnixSocketListener.listen(at: socketPath) + Thread.detachNewThread { + let accepted = accept(listener, nil, nil) + guard accepted >= 0 else { return } + Thread.sleep(forTimeInterval: 12) + close(accepted) + } + defer { close(listener) } + + let process = Process() + process.executableURL = try Self.currentBinaryURL() + process.arguments = ["attach"] + process.environment = attachEnvironment( + sessionID: SessionIdentifier(UUID()), socketPath: socketPath, command: "") + let output = Pipe() + // Held open for the whole run: a client whose stdin is already at EOF takes a different exit + // out of the poll loop, and this test would never reach the deadline it exists to measure. + let input = Pipe() + process.standardOutput = output + process.standardError = output + process.standardInput = input + + // Collected incrementally rather than by reading to EOF. The client no longer EXITS on this + // path — it becomes a shell, which inherits the held-open stdin and waits forever — so the + // observable is the message arriving in time, not the process ending. Reading to EOF here is + // what made this test sit until its own watchdog fired. + let collected = NSMutableData() + let lock = NSLock() + let sawNotice = XCTestExpectation(description: "the client reports giving up") + output.fileHandleForReading.readabilityHandler = { handle in + let data = handle.availableData + guard !data.isEmpty else { return } + lock.lock() + collected.append(data) + let seen = String(decoding: collected as Data, as: UTF8.self) + lock.unlock() + if seen.contains("did not answer the attach request") { sawNotice.fulfill() } + } + + let started = Date() + try process.run() + // A regression here is an infinite wait, which would hang the whole suite rather than fail it. + let watchdog = DispatchWorkItem { if process.isRunning { process.terminate() } } + DispatchQueue.global().asyncAfter(deadline: .now() + 25, execute: watchdog) + let outcome = XCTWaiter().wait(for: [sawNotice], timeout: 20) + let elapsed = Date().timeIntervalSince(started) + + input.fileHandleForWriting.closeFile() + process.terminate() + process.waitUntilExit() + watchdog.cancel() + output.fileHandleForReading.readabilityHandler = nil + lock.lock() + let text = String(decoding: collected as Data, as: UTF8.self) + lock.unlock() + + XCTAssertEqual( + outcome, .completed, + """ + the client never gave up on a helper that accepted the connection and then went silent. The \ + pane shows nothing, forever, with no message and no exit. got: \(text) + """) + XCTAssertLessThan(elapsed, 20, "gave up, but not within its own deadline. got: \(text)") + XCTAssertTrue( + text.contains("will not survive quitting"), + """ + the client gave up and then exited instead of becoming a shell, which leaves the pane dead — \ + `wait_after_command` is false, so there is nothing left to read the message in. got: \(text) + """) + XCTAssertFalse( + text.contains("no session helper is listening"), + """ + a helper that ACCEPTED the connection was reported as absent. That message is the retry \ + path's, and retrying a slow peer makes it redo the introspection and replay each attempt \ + already paid for — it cannot converge. got: \(text) + """) + } + + // MARK: - Harness + + /// The pinned fixture, located from the source tree rather than the test bundle: an executable + /// copied as a bundle resource does not reliably keep its exec bit, and `#filePath` is stable for + /// every configuration this suite runs in. + private static let shippedBinaryURL: URL = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() // Session/ + .deletingLastPathComponent() // WorkroomAppTests/ + .appendingPathComponent("Fixtures/workroom-session-v2.0.0") + + /// This build's `workroom-session`, beside the test host in the products directory. + private static func currentBinaryURL() throws -> URL { + for key in ["BUILT_PRODUCTS_DIR", "TARGET_BUILD_DIR"] { + guard let directory = ProcessInfo.processInfo.environment[key] else { continue } + let url = URL(fileURLWithPath: directory).appendingPathComponent("workroom-session") + if FileManager.default.isExecutableFile(atPath: url.path) { return url } + } + let embedded = Bundle.main.bundleURL.appendingPathComponent( + "Contents/MacOS/workroom-session") + if FileManager.default.isExecutableFile(atPath: embedded.path) { return embedded } + var candidate = Bundle(for: SessionShimCompatibilityTests.self).bundleURL + for _ in 0..<6 { + let url = candidate.appendingPathComponent("workroom-session") + if FileManager.default.isExecutableFile(atPath: url.path) { return url } + candidate.deleteLastPathComponent() + } + // This build's own product, not an optional dependency: if it is missing the target did not + // build, which is a failure however the suite was invoked. + XCTFail("this build's workroom-session was not found beside the test bundle") + throw CompatibilityFixtureError.unusable + } + + /// **A missing fixture skips; a fixture that will not RUN fails.** + /// + /// The difference matters more than it looks. "Not checked out" is an environmental absence. "The + /// pinned binary is here and cannot start" — a broken signature, quarantine, a future macOS + /// refusing an old Mach-O — is this test's own subject failing, and reporting that as a skip would + /// let all three tests in this file stop running indefinitely while the suite stayed green. That + /// is precisely the wire-compatibility claim the whole change rests on, and `make app-test` does + /// not print a summary under Xcode 26, so nobody would notice. + private func startShippedDaemon() throws -> String { + guard FileManager.default.fileExists(atPath: Self.shippedBinaryURL.path) else { + throw XCTSkip("v2.0.0 fixture not present at \(Self.shippedBinaryURL.path)") + } + guard FileManager.default.isExecutableFile(atPath: Self.shippedBinaryURL.path) else { + XCTFail("the pinned v2.0.0 fixture is present but not executable — check its mode in git") + throw CompatibilityFixtureError.unusable + } + let socketPath = directory.appendingPathComponent("s.sock").path + daemon = try run( + Self.shippedBinaryURL, + arguments: ["daemon", "--socket", socketPath, "--idle-timeout", "30000"], + environment: [:]) + + let deadline = Date().addingTimeInterval(5) + while Date() < deadline { + if FileManager.default.fileExists(atPath: socketPath) { return socketPath } + if let daemon, !daemon.isRunning { break } + Thread.sleep(forTimeInterval: 0.02) + } + let status = daemon.map { $0.isRunning ? "still running" : "exited \($0.terminationStatus)" } + XCTFail( + """ + the pinned v2.0.0 daemon never bound \(socketPath) (\(status ?? "not started")). The fixture \ + is present, so this is the compatibility subject failing, not a missing test dependency — \ + do not downgrade it to a skip. + """) + throw CompatibilityFixtureError.unusable + } + + /// Runs THIS build's attach client against the shipped daemon, holding stdin open with a pipe so + /// the client stays attached long enough to be served, and returns everything it wrote. + private func attachWithCurrentBuild( + sessionID: SessionIdentifier, socketPath: String + ) throws -> String { + let process = Process() + process.executableURL = try Self.currentBinaryURL() + process.arguments = ["attach"] + process.environment = attachEnvironment( + sessionID: sessionID, socketPath: socketPath, command: "") + let output = Pipe() + let input = Pipe() + process.standardOutput = output + process.standardError = output + process.standardInput = input + try process.run() + + // Read on a background queue: the client streams until its stdin closes, and reading to EOF on + // this thread would deadlock against our own write end being open. + let collected = NSMutableData() + let lock = NSLock() + output.fileHandleForReading.readabilityHandler = { handle in + let data = handle.availableData + guard !data.isEmpty else { return } + lock.lock() + collected.append(data) + lock.unlock() + } + + // Wait for the reattach to actually produce something, then for it to stop producing, rather + // than for a fixed three seconds. A flat sleep is wrong in both directions: it is three seconds + // of test time on every run, and on a loaded machine it is still not enough — the daemon has to + // accept, look the session up and replay its buffer before a single byte is due. + let settled = Self.waitForOutputToSettle(collected, lock: lock) + XCTAssertTrue( + settled, "the shipped daemon sent nothing within \(Self.attachCeiling)s of the attach") + input.fileHandleForWriting.closeFile() + process.waitUntilExit() + output.fileHandleForReading.readabilityHandler = nil + + lock.lock() + defer { lock.unlock() } + return String(decoding: collected as Data, as: UTF8.self) + } + + /// How long a reattach gets before we call it a failure. Generous on purpose: this bounds a + /// process spawn, a connect and a replay on whatever machine CI gave us, and the poll below + /// returns as soon as the output settles, so the ceiling is only ever paid by a real failure. + private static let attachCeiling: TimeInterval = 10 + + /// True once the client has written something and then gone quiet for `quiet` seconds. + /// + /// Quiescence, not just non-empty: the replay arrives in several writes and asserting on the + /// first of them would read a prefix of the screen. + private static func waitForOutputToSettle( + _ collected: NSMutableData, lock: NSLock, quiet: TimeInterval = 0.25 + ) -> Bool { + let deadline = Date().addingTimeInterval(attachCeiling) + var lastLength = 0 + var lastChange = Date() + while Date() < deadline { + lock.lock() + let length = collected.length + lock.unlock() + if length != lastLength { + lastLength = length + lastChange = Date() + } else if length > 0, Date().timeIntervalSince(lastChange) >= quiet { + return true + } + Thread.sleep(forTimeInterval: 0.02) + } + return lastLength > 0 + } + + /// A session on the shipped daemon running a plain `/bin/sh` on its pty, created by the SHIPPED + /// client and then detached — the state a user's machine is in after they quit an older build. + private func startShellSession(on socketPath: String) throws -> ShellSession { + let sessionID = SessionIdentifier(UUID()) + let marker = "V2-SHELL-\(UUID().uuidString.prefix(8))" + let ready = directory.appendingPathComponent("ready-\(sessionID.uuidString.prefix(8))").path + let creator = try run( + Self.shippedBinaryURL, arguments: ["attach"], + environment: attachEnvironment( + sessionID: sessionID, socketPath: socketPath, + // Print, touch, then `exec sh`. The marker goes into the daemon's replay buffer, the + // sentinel proves the pty is up and running commands before a second client attaches, and + // the `exec` leaves that same pty owned by an ordinary shell to type at. + command: "sh -c 'echo \(marker); touch \(ready); exec /bin/sh'")) + creator.waitUntilExit() + XCTAssertTrue( + waitForFile(at: ready), + "the v2.0.0 daemon never started a shell for this session; there is nothing to type at") + return ShellSession(identifier: sessionID, marker: marker) + } + + /// A session the shipped daemon holds, plus the marker that proves a later attach REPLAYED it. + /// + /// The marker is not decoration. `SessionDaemon.handleAttach` creates a session for an id it does + /// not hold, so an attach that silently missed would still hand the test a working shell — one it + /// could type at and exit — and every assertion below it would pass against a pane the user's + /// work is not in. The replayed marker is the only thing that tells the two apart. + struct ShellSession { + let identifier: SessionIdentifier + let marker: String + } + + private func waitForFile(at path: String, within seconds: TimeInterval = 10) -> Bool { + let deadline = Date().addingTimeInterval(seconds) + while Date() < deadline { + if FileManager.default.fileExists(atPath: path) { return true } + Thread.sleep(forTimeInterval: 0.02) + } + return false + } + + /// This build's attach client, running, with its stdin held open so it can be typed at. + private func attachInteractively( + sessionID: SessionIdentifier, socketPath: String + ) throws -> AttachedClient { + let process = Process() + process.executableURL = try Self.currentBinaryURL() + process.arguments = ["attach"] + process.environment = attachEnvironment( + sessionID: sessionID, socketPath: socketPath, command: "") + let output = Pipe() + let input = Pipe() + process.standardOutput = output + process.standardError = output + process.standardInput = input + + let client = AttachedClient(process: process, input: input, output: output) + try process.run() + + // Type only once the session has repainted. Bytes written before the client finishes its + // handshake are relayed into a socket the daemon is not reading yet, and the test would be + // measuring the race rather than the feature. + XCTAssertTrue( + Self.waitForOutputToSettle(client.collected, lock: client.lock), + "the shipped daemon sent nothing within \(Self.attachCeiling)s of the attach") + return client + } + + /// Owns the pipes for the lifetime of one attached client, so a test can type at it, read what + /// came back, and clean it up without three locals and a `defer` each time. + final class AttachedClient { + let collected = NSMutableData() + let lock = NSLock() + private let process: Process + private let input: Pipe + private let output: Pipe + + init(process: Process, input: Pipe, output: Pipe) { + self.process = process + self.input = input + self.output = output + let collected = self.collected + let lock = self.lock + output.fileHandleForReading.readabilityHandler = { handle in + let data = handle.availableData + guard !data.isEmpty else { return } + lock.lock() + collected.append(data) + lock.unlock() + } + } + + /// Throwing, and SIGPIPE ignored by the caller: if the relay has exited, its end of this pipe + /// is closed, and the non-throwing `write(_:)` raises rather than returns. That took the whole + /// test host down with it — a regression showed up as a test that had VANISHED from the run + /// rather than one that failed, which is the least legible way for this to break. + @discardableResult + func type(_ text: String) -> Bool { + do { + try input.fileHandleForWriting.write(contentsOf: Data(text.utf8)) + return true + } catch { + return false + } + } + + func text() -> String { + lock.lock() + defer { lock.unlock() } + return String(decoding: collected as Data, as: UTF8.self) + } + + func waitUntilExit(within seconds: TimeInterval) -> Bool { + let deadline = Date().addingTimeInterval(seconds) + while Date() < deadline { + if !process.isRunning { return true } + Thread.sleep(forTimeInterval: 0.02) + } + return false + } + + func status() -> Int32 { + process.isRunning ? -1 : process.terminationStatus + } + + func terminate() { + output.fileHandleForReading.readabilityHandler = nil + if process.isRunning { process.terminate() } + try? input.fileHandleForWriting.close() + } + } + + private func attachEnvironment( + sessionID: SessionIdentifier, socketPath: String, command: String + ) -> [String: String] { + [ + "WORKROOM_SESSION_ID": sessionID.uuidString, + "WORKROOM_SESSION_SOCKET": socketPath, + "WORKROOM_SESSION_SHELL": "/bin/sh", + "WORKROOM_SESSION_CWD": directory.path, + "WORKROOM_SESSION_COMMAND": command, + ] + } + + @discardableResult + private func run( + _ url: URL, arguments: [String], environment: [String: String] + ) throws -> Process { + let process = Process() + process.executableURL = url + process.arguments = arguments + process.environment = environment + process.standardOutput = FileHandle.nullDevice + process.standardError = FileHandle.nullDevice + if environment["WORKROOM_SESSION_ID"] != nil { + process.standardInput = FileHandle.nullDevice + } + try process.run() + return process + } + + /// Runs this build's `workroom-session` to completion and returns everything it wrote. + /// + /// `readDataToEndOfFile` rather than a readability handler: `Process` closes the parent's copy of + /// the child-side pipe ends after spawning, so the read end reaches EOF when the child exits. + /// (`attachWithCurrentBuild` uses a handler because it has to close stdin at a chosen moment + /// while output is still streaming — a different problem.) The watchdog is what keeps a hang + /// regression a FAILURE rather than a blocked read. + private func runCapturing( + arguments: [String], environment: [String: String] + ) throws -> (String, Int32) { + let process = Process() + process.executableURL = try Self.currentBinaryURL() + process.arguments = arguments + process.environment = environment + let output = Pipe() + process.standardOutput = output + process.standardError = output + process.standardInput = FileHandle.nullDevice + + try process.run() + let watchdog = DispatchWorkItem { if process.isRunning { process.terminate() } } + DispatchQueue.global().asyncAfter(deadline: .now() + 25, execute: watchdog) + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + watchdog.cancel() + return (String(decoding: data, as: UTF8.self), process.terminationStatus) + } +} + +/// Minimal unix-socket client for protocol tests, carried over from the deleted +/// `SessionDaemonEndToEndTests` — the frames it sends are what this tree encodes, which is the +/// half of the comparison under test. +private struct SessionTestClient { + let descriptor: Int32 + var decoder = SessionFrameDecoder() + + static func connect(socketPath: String) throws -> SessionTestClient { + let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) + guard descriptor >= 0 else { throw XCTSkip("socket() failed") } + var address = sockaddr_un() + address.sun_family = sa_family_t(AF_UNIX) + let path = Array(socketPath.utf8CString) + guard path.count < MemoryLayout.size(ofValue: address.sun_path) else { + close(descriptor) + throw NSError(domain: "SessionTestClient", code: 1) + } + withUnsafeMutablePointer(to: &address.sun_path) { pointer in + pointer.withMemoryRebound(to: CChar.self, capacity: path.count) { dest in + for (i, byte) in path.enumerated() { dest[i] = byte } + } + } + address.sun_len = UInt8(MemoryLayout.size) + let result = withUnsafePointer(to: &address) { pointer in + pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { casted in + Darwin.connect(descriptor, casted, socklen_t(MemoryLayout.size)) + } + } + guard result == 0 else { + close(descriptor) + throw NSError( + domain: "SessionTestClient", code: 2, + userInfo: [NSLocalizedDescriptionKey: "connect failed: \(errno)"]) + } + let flags = fcntl(descriptor, F_GETFL, 0) + _ = fcntl(descriptor, F_SETFL, flags | O_NONBLOCK) + return SessionTestClient(descriptor: descriptor) + } + + mutating func send(_ frame: SessionFrame) throws { + var bytes = frame.encoded() + var offset = 0 + while offset < bytes.count { + let written = bytes.withUnsafeBytes { pointer -> Int in + Darwin.write(descriptor, pointer.baseAddress!.advanced(by: offset), bytes.count - offset) + } + if written > 0 { + offset += written + continue + } + if errno == EAGAIN || errno == EINTR { + var pollfd = pollfd(fd: descriptor, events: Int16(POLLOUT), revents: 0) + _ = poll(&pollfd, 1, 1000) + continue + } + throw NSError(domain: "SessionTestClient", code: 3) + } + } + + mutating func wait(for kind: SessionFrameKind, timeout: TimeInterval) throws -> SessionFrame { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + if let frame = try decoder.next() { + if frame.kind == kind { return frame } + continue + } + var pollfd = pollfd(fd: descriptor, events: Int16(POLLIN), revents: 0) + let remaining = deadline.timeIntervalSinceNow + let ready = poll(&pollfd, 1, Int32(max(remaining, 0) * 1000)) + if ready <= 0 { continue } + var buffer = [UInt8](repeating: 0, count: 64 * 1024) + let capacity = buffer.count + let count = buffer.withUnsafeMutableBytes { pointer in + Darwin.read(descriptor, pointer.baseAddress, capacity) + } + if count > 0 { + decoder.push(Array(buffer.prefix(count))) + } else if count == 0 { + throw NSError( + domain: "SessionTestClient", code: 4, + userInfo: [NSLocalizedDescriptionKey: "eof waiting for \(kind)"]) + } + } + throw NSError( + domain: "SessionTestClient", code: 5, + userInfo: [NSLocalizedDescriptionKey: "timeout waiting for \(kind)"]) + } + + func closeConnection() { + Darwin.close(descriptor) + } +} diff --git a/macapp/WorkroomAppTests/Session/TerminalPersistentSessionPolicyTests.swift b/macapp/WorkroomAppTests/Session/TerminalPersistentSessionPolicyTests.swift index ecb82d9da..d5e0129a4 100644 --- a/macapp/WorkroomAppTests/Session/TerminalPersistentSessionPolicyTests.swift +++ b/macapp/WorkroomAppTests/Session/TerminalPersistentSessionPolicyTests.swift @@ -9,6 +9,7 @@ final class TerminalPersistentSessionPolicyTests: XCTestCase { preferenceEnabled: true, isAvailable: true, isRunCommand: false, + hasExistingSession: false, isFixture: false)) } @@ -18,24 +19,71 @@ final class TerminalPersistentSessionPolicyTests: XCTestCase { preferenceEnabled: false, isAvailable: true, isRunCommand: false, + hasExistingSession: false, + isFixture: false)) + } + + /// The user's preference beats a restored id. Turning background sessions off means off, even for + /// a pane that already has one. + func testOffPreferenceDisablesEvenWithAnExistingSession() { + XCTAssertFalse( + TerminalPersistentSessionPolicy.usesPersistentSession( + preferenceEnabled: false, + isAvailable: false, + isRunCommand: false, + hasExistingSession: true, isFixture: false)) } func testRunCommandAndFixtureAreExcluded() { XCTAssertFalse( TerminalPersistentSessionPolicy.usesPersistentSession( - preferenceEnabled: true, isAvailable: true, isRunCommand: true, isFixture: false)) + preferenceEnabled: true, isAvailable: true, isRunCommand: true, + hasExistingSession: false, isFixture: false)) XCTAssertFalse( TerminalPersistentSessionPolicy.usesPersistentSession( - preferenceEnabled: true, isAvailable: true, isRunCommand: false, isFixture: true)) + preferenceEnabled: true, isAvailable: true, isRunCommand: false, + hasExistingSession: false, isFixture: true)) } - func testUnavailableHelperFallsBack() { + /// No backend can take a NEW session, so a fresh pane gets none. + func testUnavailableHelperGivesANewPaneNoSession() { XCTAssertFalse( TerminalPersistentSessionPolicy.usesPersistentSession( preferenceEnabled: true, isAvailable: false, isRunCommand: false, + hasExistingSession: false, + isFixture: false)) + } + + /// REGRESSION, and the reason `hasExistingSession` exists. + /// + /// `isAvailable` answers for the backend a NEW session would go to. A RESTORED pane already has + /// an id naming a session some helper may still be holding, and running that id through the same + /// gate discarded it before anything could ask who owned it — so an unhealthy agent silently + /// stranded every session the Swift daemon was still holding, one layer above the fix that was + /// supposed to prevent exactly that. Keeping the id is safe: if nothing can attach to it, + /// `attachCommand` returns nil and the pane opens a plain shell anyway. + func testARestoredSessionSurvivesAnUnavailableHelper() { + XCTAssertTrue( + TerminalPersistentSessionPolicy.usesPersistentSession( + preferenceEnabled: true, + isAvailable: false, + isRunCommand: false, + hasExistingSession: true, + isFixture: false), + "a pane restoring a session it already had must keep its id so the owner can be resolved") + } + + /// A run command is still excluded even when restoring, so the bypass cannot widen past its case. + func testARestoredRunCommandIsStillExcluded() { + XCTAssertFalse( + TerminalPersistentSessionPolicy.usesPersistentSession( + preferenceEnabled: true, + isAvailable: false, + isRunCommand: true, + hasExistingSession: true, isFixture: false)) } } diff --git a/macapp/WorkroomAppTests/Session/UnixSocketListener.swift b/macapp/WorkroomAppTests/Session/UnixSocketListener.swift new file mode 100644 index 000000000..ad3349892 --- /dev/null +++ b/macapp/WorkroomAppTests/Session/UnixSocketListener.swift @@ -0,0 +1,64 @@ +import Darwin +import Foundation +import XCTest + +/// A bound, listening unix socket that nothing ever services. +/// +/// Three session test files needed this and each grew its own copy, which drifted: one forgot +/// `sun_len`, one wrote the path with `copyBytes` and one byte by byte, and each threw its own error +/// domain. They are the same socket, so they are one function now. +/// +/// The point of it is the case a client cannot tell apart from a healthy peer without waiting: the +/// `connect` succeeds, so there is something there, and then nothing answers. That is what a wedged +/// helper looks like from the app, and it is what every timeout in the session layer is for. +enum UnixSocketListener { + /// Returns the listening descriptor; the caller closes it and removes the path. + static func listen(at socketPath: String, file: StaticString = #filePath, line: UInt = #line) + throws -> Int32 + { + let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) + guard descriptor >= 0 else { throw Failure.socket(errno) } + + var address = sockaddr_un() + address.sun_family = sa_family_t(AF_UNIX) + let pathBytes = Array(socketPath.utf8) + guard pathBytes.count < MemoryLayout.size(ofValue: address.sun_path) else { + close(descriptor) + throw Failure.pathTooLong(socketPath) + } + withUnsafeMutableBytes(of: &address.sun_path) { pointer in + pointer.withMemoryRebound(to: CChar.self) { destination in + for (index, byte) in pathBytes.enumerated() { destination[index] = CChar(bitPattern: byte) } + } + } + // BSD sockets carry their own length. macOS tolerates a zero here, but every other consumer of + // this struct in the tree sets it, and a half-initialized sockaddr is not worth the saving. + address.sun_len = UInt8(MemoryLayout.size) + + let bound = withUnsafePointer(to: &address) { pointer in + pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { casted in + Darwin.bind(descriptor, casted, socklen_t(MemoryLayout.size)) + } + } + guard bound == 0, Darwin.listen(descriptor, 1) == 0 else { + let reason = errno + close(descriptor) + throw Failure.bind(reason) + } + return descriptor + } + + enum Failure: LocalizedError { + case socket(Int32) + case pathTooLong(String) + case bind(Int32) + + var errorDescription: String? { + switch self { + case .socket(let code): return "socket() failed: \(code)" + case .pathTooLong(let path): return "socket path too long for sun_path: \(path)" + case .bind(let code): return "bind/listen failed: \(code)" + } + } + } +} diff --git a/macapp/WorkroomAppTests/VCSRemoteIntegrationTests.swift b/macapp/WorkroomAppTests/VCSRemoteIntegrationTests.swift index 72181cf44..da8c91fab 100644 --- a/macapp/WorkroomAppTests/VCSRemoteIntegrationTests.swift +++ b/macapp/WorkroomAppTests/VCSRemoteIntegrationTests.swift @@ -389,7 +389,12 @@ final class VCSRemoteIntegrationTests: XCTestCase { return XCTFail("expected .locked, got \(failure)") } let located = try XCTUnwrap(file, "the path is in git's stderr, so it must be located") - XCTAssertEqual(located.path, lockPath) + // Resolved on both sides: the fixture builds its path from `NSTemporaryDirectory()` (`/var/…`) + // while git reports the one it actually opened (`/private/var/…`), and `/var` is a symlink to + // `/private/var`. Comparing the raw strings comes down to which side happened to resolve it. + XCTAssertEqual( + URL(fileURLWithPath: located.path).resolvingSymlinksInPath().path, + URL(fileURLWithPath: lockPath).resolvingSymlinksInPath().path) XCTAssertEqual(located.filename, "index.lock") // The consequence that matters: no Retry, because there is nothing a retry could achieve. diff --git a/macapp/WorkroomSession/SessionAttachClient.swift b/macapp/WorkroomSession/SessionAttachClient.swift index aacfe20d7..74d470e65 100644 --- a/macapp/WorkroomSession/SessionAttachClient.swift +++ b/macapp/WorkroomSession/SessionAttachClient.swift @@ -1,5 +1,4 @@ import Darwin -import MachO import WorkroomSessionProtocol enum SessionAttachExitCode { @@ -10,11 +9,39 @@ enum SessionAttachExitCode { } enum SessionAttachClient { - static let connectCycles = 3 - static let connectAttemptsPerCycle = 50 + /// How hard to retry a refused connect, and for how long to wait once connected. + /// + /// These used to be far larger (3 cycles of 50 × 20ms) because `connect` STARTED a daemon when + /// none answered and then had to wait for it to bind its socket. This build cannot start one — + /// the `daemon` subcommand is gone — so there is nothing to wait for that is not already there. + /// What remains covers a live helper that is momentarily slow to `accept`, which is the only real + /// wait left: 3 cycles of 10 x 20ms plus two 100ms backoffs, so ~0.7s of a refused connect rather + /// than ~9.2s. Note that is the CONNECT budget alone — the worst case a user can see is a peer + /// that refuses twice and then accepts and goes silent, which adds `attachHandshakeTimeoutSeconds` + /// for ~5.6s total. + static let connectAttempts = 10 static let connectRetryMicroseconds: useconds_t = 20000 - static let handshakeAttempts = 3 - static let handshakeRetryMicroseconds: useconds_t = 100_000 + /// How many times to re-open the connection. Named for the connect budget, not the + /// handshake: the handshake proper is deliberately NOT retried (see + /// `attachHandshakeTimeoutSeconds`), so the only thing that reaches these is a refused connect or + /// a transport that dropped before `.attached`. + static let connectCycles = 3 + static let connectCycleRetryMicroseconds: useconds_t = 100_000 + /// How long to wait, once CONNECTED, for the helper to answer the attach request (seconds). + /// + /// Shrinking the connect retries above bounds only a REFUSED connect. A helper that accepts and + /// then says nothing — a wedged one — is a different failure, and nothing bounded it: `poll` + /// blocks indefinitely once no settle check is pending, so the pane sat blank forever with no + /// message and no exit. + /// + /// **Generous, and deliberately not retried.** The v2.0.0 daemon answers an attach from a single + /// poll iteration that first runs process introspection (`foregroundProcessGroup`, + /// `executableName`, `workingDirectory`) and then enqueues the whole replay buffer, flushing only + /// once that returns. On a loaded machine that is slow rather than broken, so a tight deadline + /// plus a retry is the worst combination available: each attempt tears down the connection, makes + /// the peer redo the same introspection and replay, and hands the next attempt an even busier + /// daemon. It cannot converge. One long wait converges or it does not. + static let attachHandshakeTimeoutSeconds: Double = 5 static let inputBacklogLimit = 4 * 1024 * 1024 /// How long after attaching to re-check the terminal size once more (seconds). /// @@ -46,18 +73,6 @@ enum SessionAttachClient { case failed(Int32) } - private enum ConnectionOutcome { - case connected(Int32) - case retry - case failed(Int32) - } - - private enum DaemonLaunchOutcome { - case started - case retryableFailure - case fatalFailure - } - static func run(configuration: Configuration) -> Int32 { signal(SIGPIPE, SIG_IGN) @@ -72,32 +87,35 @@ enum SessionAttachClient { } } - for attempt in 0.. Outcome - { - let socket: Int32 - switch connect(socketPath: configuration.socketPath) { - case .connected(let descriptor): - socket = descriptor - case .retry: - return .retry - case .failed(let status): - return .failed(status) - } + private static func attach( + configuration: Configuration, signalPipe: SessionSignalPipe, connected: inout Bool + ) -> Outcome { + guard let socket = connect(socketPath: configuration.socketPath) else { return .retry } + connected = true defer { SessionIO.close(socket) } SessionIO.setNonBlocking(socket) @@ -122,7 +140,23 @@ enum SessionAttachClient { { var isAttached = false var settleDeadline: Double? + // Cleared the moment `.attached` lands. Until then it bounds the whole wait, checked at the top + // of every iteration rather than only on a poll timeout — a helper can keep this loop busy with + // traffic that never includes the attach reply. + var handshakeDeadline: Double? = + SessionClock.monotonicSeconds() + attachHandshakeTimeoutSeconds while true { + if let deadline = handshakeDeadline, SessionClock.monotonicSeconds() >= deadline { + // TERMINAL, not `.retry`. Retrying reconnects and re-sends the attach, which on a slow peer + // makes it redo the introspection and replay this attempt already paid for — three attempts + // against a progressively busier daemon, converging on nothing. It also let `run`'s final + // "no session helper is listening" overwrite this line on screen, which is false in this + // path: the helper was listening, we reached it three times. + report( + "connected to the session helper, but it did not answer the attach request within " + + "\(Int(attachHandshakeTimeoutSeconds))s — the session may still be running") + return .failed(SessionAttachExitCode.daemonUnavailable) + } guard connection.flush() else { return transportOutcome(isAttached: isAttached) } var descriptors = [ @@ -137,7 +171,9 @@ enum SessionAttachClient { descriptors.append(pollfd(fd: STDIN_FILENO, events: Int16(POLLIN), revents: 0)) } - let ready = poll(&descriptors, nfds_t(descriptors.count), pollTimeout(until: settleDeadline)) + let ready = poll( + &descriptors, nfds_t(descriptors.count), + pollTimeout(untilEarliestOf: settleDeadline, handshakeDeadline)) if ready == 0, let deadline = settleDeadline, SessionClock.monotonicSeconds() >= deadline { settleDeadline = nil sendResize(connection) @@ -160,18 +196,43 @@ enum SessionAttachClient { return outcome } } - // Arm the one-shot settle re-check the moment `.attached` lands, not before — there's - // nothing to settle until the daemon has actually accepted us. + // The attach reply has landed, so the handshake is no longer what we are waiting for. Past + // this point an idle connection is a healthy one and blocking indefinitely is correct. + // Cleared BEFORE the settle deadline is armed, so the two are never live at once. + if isAttached { handshakeDeadline = nil } + // Arm the settle re-check the moment `.attached` lands, not before — there's nothing to + // settle until the daemon has actually accepted us. + // + // NOT one-shot, despite how it reads, and this comment used to claim otherwise. Firing it + // sets `settleDeadline = nil` and `continue`s past this block; the next wake then finds + // `isAttached` with no deadline armed and starts another. So a resize trails every burst of + // output by ~300ms. Pre-existing and harmless — the v2.0.0 peer's `.resize` is a plain + // `SessionPTY.resize` with no forced redraw — but worth knowing when reading the timeouts. if isAttached, settleDeadline == nil { settleDeadline = SessionClock.monotonicSeconds() + settleCheckDelaySeconds } } } - /// `poll`'s timeout in milliseconds: bounded while a settle check is pending (so we wake up to - /// fire it even with no other activity), `-1` (block indefinitely) once there's nothing to wait - /// for — this is a single terminal session's I/O loop, not a busy-poll. - private static func pollTimeout(until deadline: Double?) -> Int32 { + /// `poll`'s timeout in milliseconds: the nearest pending deadline, or `-1` (block indefinitely) + /// when none is pending — this is a single terminal session's I/O loop, not a busy-poll. + /// + /// Two plain optionals rather than a variadic: this is called once per wake of the relay's + /// busiest loop, and a Swift variadic heap-allocates an Array — then `compactMap` allocates a + /// second — where the surrounding code allocates nothing per iteration it does not have to. + /// + /// Both are passed even though only one can be armed at a time (the handshake deadline is cleared + /// in the same block that arms the settle one), because the alternative is a caller that has to + /// know which is live. What enforces the handshake bound is the top-of-loop check, not this — + /// `poll` only has to wake up in time for it. + private static func pollTimeout(untilEarliestOf first: Double?, _ second: Double?) -> Int32 { + let deadline: Double? + switch (first, second) { + case (let a?, let b?): deadline = min(a, b) + case (let a?, nil): deadline = a + case (nil, let b?): deadline = b + case (nil, nil): deadline = nil + } guard let deadline else { return -1 } let remaining = deadline - SessionClock.monotonicSeconds() guard remaining > 0 else { return 0 } @@ -251,7 +312,7 @@ enum SessionAttachClient { private static func sendResize(_ connection: SessionConnection) { let size = SessionWindowSizePolicy.attachSize( - from: SessionPTY.windowSize(descriptor: STDIN_FILENO)) + from: SessionTerminalSize.of(descriptor: STDIN_FILENO)) guard SessionWindowSizePolicy.isUsable(columns: size.columns, rows: size.rows) else { return } connection.enqueue( SessionFrame( @@ -261,7 +322,7 @@ enum SessionAttachClient { private static func makeRequest(_ configuration: Configuration) -> SessionAttachRequest { let size = SessionWindowSizePolicy.attachSize( - from: SessionPTY.windowSize(descriptor: STDIN_FILENO)) + from: SessionTerminalSize.of(descriptor: STDIN_FILENO)) let environment = SessionProcessEnvironment.current() .filter { !$0.key.hasPrefix("WORKROOM_SESSION_") } .map { SessionEnvironmentEntry(key: $0.key, value: $0.value) } @@ -286,69 +347,18 @@ enum SessionAttachClient { return original } - private static func connect(socketPath: String) -> ConnectionOutcome { - for _ in 0.. DaemonLaunchOutcome { - guard let binaryPath = executablePath() else { - report("unable to locate the session daemon binary") - return .fatalFailure - } - let arguments = SessionCStringArray([binaryPath, "daemon", "--socket", socketPath]) - - var fileActions: posix_spawn_file_actions_t? - posix_spawn_file_actions_init(&fileActions) - defer { posix_spawn_file_actions_destroy(&fileActions) } - posix_spawn_file_actions_addopen(&fileActions, STDIN_FILENO, "/dev/null", O_RDWR, 0) - posix_spawn_file_actions_addopen(&fileActions, STDOUT_FILENO, "/dev/null", O_RDWR, 0) - posix_spawn_file_actions_addopen( - &fileActions, - STDERR_FILENO, - socketPath + ".log", - O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, - 0o600) - - var attributes: posix_spawnattr_t? - posix_spawnattr_init(&attributes) - defer { posix_spawnattr_destroy(&attributes) } - posix_spawnattr_setflags(&attributes, Int16(POSIX_SPAWN_SETSID)) - - var processID: pid_t = 0 - let result = posix_spawn( - &processID, binaryPath, &fileActions, &attributes, arguments.pointer, environ) - guard result != 0 else { return .started } - report("could not start the session daemon (\(result))") - return result == EAGAIN || result == EINTR ? .retryableFailure : .fatalFailure - } - - static func executablePath() -> String? { - if let provided = SessionProcessEnvironment.value("WORKROOM_SESSION_BINARY") { - return provided - } - var size = UInt32(PATH_MAX) - var buffer = [CChar](repeating: 0, count: Int(PATH_MAX)) - guard _NSGetExecutablePath(&buffer, &size) == 0 else { return nil } - return buffer.withUnsafeBufferPointer { pointer in - guard let base = pointer.baseAddress else { return nil } - return String(cString: base) + /// Connect to a helper that is ALREADY running, or give up. + /// + /// This used to `posix_spawn` `workroom-session daemon` when nothing answered, then wait for the + /// new process to bind. That is gone with the daemon itself: the only sessions this client can + /// reach are held by a daemon some OLDER build of the app started, and starting a fresh one would + /// serve no session that exists. What is left is a short retry for a live helper that is + /// momentarily slow to `accept`. + private static func connect(socketPath: String) -> Int32? { + for attempt in 0.. Never? { + // The terminal is in raw mode by now if the attach got that far, and `execve` runs no + // deferred blocks. A shell handed a raw tty has no line editing, no echo and no signal keys, + // which is a worse terminal than the dead pane this is preventing. + var restored = termios() + if tcgetattr(STDIN_FILENO, &restored) == 0 { + restored.c_lflag |= UInt(ECHO | ICANON | ISIG) + restored.c_iflag |= UInt(ICRNL) + restored.c_oflag |= UInt(OPOST) + tcsetattr(STDIN_FILENO, TCSANOW, &restored) + } + + SessionIO.writeAll( + STDERR_FILENO, + Array( + ("workroom-session: this terminal will not survive quitting Workroom\r\n").utf8)) + + // The same invocation the daemon would have run for this session, so the pane keeps its login + // profile and ghostty's shell integration (OSC 133/OSC 7) rather than getting a bare shell. + let shell = + configuration.shell.isEmpty + ? SessionShellIntegration.defaultShell : configuration.shell + // Basename without Foundation: this target imports Darwin only. + let name = shell.split(separator: "/").last.map(String.init) ?? shell + // A leading dash on argv[0] is the only thing that makes a login shell; there is no flag. + var argv: [String] = ["-\(name)"] + var program = shell + if !configuration.command.isEmpty { + program = "/bin/sh" + argv = ["sh", "-c", "exec \(configuration.command)"] + } + + // Requested directory, then home, then root — whichever `chdir` will actually accept. Checking + // `isDirectory` first is not the same test: a directory can exist and still refuse `chdir` for + // want of the execute bit. + for candidate in [ + configuration.workingDirectory, SessionProcessEnvironment.value("HOME") ?? "", "/", + ] where !candidate.isEmpty { + if chdir(candidate) == 0 { break } + } + + // Nothing downstream should believe it is inside a session that does not exist. + unsetenv("WORKROOM_SESSION_ID") + unsetenv("WORKROOM_SESSION_SOCKET") + setenv("WORKROOM_SESSION_FALLBACK", "1", 1) + + let argvPointers: [UnsafeMutablePointer?] = argv.map { strdup($0) } + [nil] + _ = argvPointers.withUnsafeBufferPointer { buffer in + execv(program, buffer.baseAddress!) + } + // Only reached when exec itself failed; the caller exits with the original status. + SessionIO.writeAll( + STDERR_FILENO, Array("workroom-session: could not start \(program)\r\n".utf8)) + return nil + } +} diff --git a/macapp/WorkroomSession/SessionConnection.swift b/macapp/WorkroomSession/SessionConnection.swift index e63bbd611..329d2b0ba 100644 --- a/macapp/WorkroomSession/SessionConnection.swift +++ b/macapp/WorkroomSession/SessionConnection.swift @@ -1,11 +1,15 @@ import Darwin import WorkroomSessionProtocol +/// One socket to a session helper, from the client side. +/// +/// `attachedSession` and `closesAfterFlush` used to live here and are gone: both were written and +/// read only by the daemon, which tracked which session a connection was serving and whether to +/// close it once its outbox drained. The attach client has exactly one connection and one session, +/// so it never needed either. final class SessionConnection { let descriptor: Int32 var decoder = SessionFrameDecoder() - var attachedSession: SessionIdentifier? - var closesAfterFlush = false private var outbox = SessionByteQueue() @@ -27,60 +31,3 @@ final class SessionConnection { return true } } - -final class PTYSession { - let identifier: SessionIdentifier - let processID: pid_t - let ttyDevice: UInt64 - let workingDirectory: String - - var masterDescriptor: Int32 - var replay: SessionReplayBuffer - var clientDescriptor: Int32? - var metadata: [SessionEnvironmentEntry] = [] - - private var pendingInput = SessionByteQueue() - - init( - identifier: SessionIdentifier, - process: SessionProcess, - workingDirectory: String, - replayCapacity: Int - ) { - self.identifier = identifier - processID = process.processID - ttyDevice = process.ttyDevice - masterDescriptor = process.masterDescriptor - self.workingDirectory = workingDirectory - replay = SessionReplayBuffer(capacity: replayCapacity) - } - - var hasPendingInput: Bool { pendingInput.hasPendingOutput } - - var descriptor: SessionDescriptor { - SessionDescriptor( - identifier: identifier, - shellProcessID: processID, - ttyDevice: ttyDevice, - workingDirectory: workingDirectory, - isAttached: clientDescriptor != nil, - metadata: metadata) - } - - func appendInput(_ bytes: [UInt8], limit: Int) { - guard pendingInput.pendingByteCount + bytes.count <= limit else { - SessionLog.write( - "dropped \(bytes.count) input byte(s): the session is not draining its terminal") - return - } - pendingInput.enqueue(bytes) - } - - /// A hard write failure clears the pending input outright, unlike `SessionConnection.flush` — - /// the pty itself (not this session's bookkeeping) is what's in an unknown state, so there's - /// nothing to usefully retry. - func flushInput() { - guard masterDescriptor >= 0 else { return } - if case .failed = pendingInput.drain(to: masterDescriptor) { pendingInput.clear() } - } -} diff --git a/macapp/WorkroomSession/SessionDaemon.swift b/macapp/WorkroomSession/SessionDaemon.swift deleted file mode 100644 index 5193a4059..000000000 --- a/macapp/WorkroomSession/SessionDaemon.swift +++ /dev/null @@ -1,724 +0,0 @@ -import Darwin -import WorkroomSessionProtocol - -enum SessionDaemonStart { - case running(SessionDaemon) - case lockHeld - case failed(String) -} - -final class SessionDaemon { - static let replayCapacity = 256 * 1024 - static let clientOutputLimit = 4 * 1024 * 1024 - /// Caps a single connection's outbound reply queue. `clientOutputLimit` already bounds output - /// frames sourced from a session's pty (the daemon stops reading the master once a client is - /// backed up past it), but protocol replies (`list`/`info`/`attach`/`kill`/`killAll`) are - /// generated straight from client requests and aren't covered by that guard — a same-UID client - /// that keeps sending requests without ever reading its socket can otherwise grow this daemon's - /// memory without bound. Sized above `clientOutputLimit` so a client that is legitimately - /// backed up on real terminal output isn't punished for it; a connection that exceeds this is - /// misbehaving, not merely slow, and gets dropped. - static let connectionOutboxLimit = 8 * 1024 * 1024 - static let sessionInputLimit = 1024 * 1024 - static let maximumConnections = 64 - static let maximumSessions = 256 - static let idleTimeoutMilliseconds: Int32 = 10000 - static let replayChunkSize = 32 * 1024 - /// How long to leave a reattached session's pty bumped before resizing it back and signaling — - /// long enough for the child to actually be scheduled and observe the transient size (see - /// `SessionPTY.bumpedRowCount`), short enough nobody notices. Deferred through the poll loop - /// rather than a blocking sleep: this daemon is single-threaded and serves every session across - /// every open project, so blocking here would freeze all of them for the duration, worst-case - /// stacking to N × this value when many persisted tabs reattach at once on relaunch. - static let redrawSettleSeconds: Double = 0.03 - - private let socketPath: String - private let idleTimeout: Int32 - private let listener: Int32 - private let lockDescriptor: Int32 - private let signalPipe: SessionSignalPipe - - private var sessions: [SessionIdentifier: PTYSession] = [:] - private var sessionsByMaster: [Int32: SessionIdentifier] = [:] - private var connections: [Int32: SessionConnection] = [:] - private var closingDescriptors: [Int32] = [] - /// Sessions bumped by `attach()`, awaiting their non-blocking resize-back-and-signal once - /// `redrawSettleSeconds` has elapsed. Keyed by identifier, not master fd, so a session that gets - /// killed and its fd reused before the deadline can't cause this to resize/signal the WRONG pty. - private var pendingRedraws: [SessionIdentifier: PendingRedraw] = [:] - - private struct PendingRedraw { - let deadline: Double - let columns: UInt16 - let rows: UInt16 - } - - static func start( - socketPath: String, - idleTimeoutMilliseconds: Int32 = SessionDaemon.idleTimeoutMilliseconds - ) -> SessionDaemonStart { - signal(SIGPIPE, SIG_IGN) - signal(SIGHUP, SIG_IGN) - - guard let lock = SessionSocket.acquireLock(path: socketPath + ".lock") else { - return .lockHeld - } - guard let signalPipe = SessionSignalPipe(signals: [SIGCHLD]) else { - SessionIO.close(lock) - return .failed("unable to create the signal pipe") - } - guard let listener = SessionSocket.makeListener(path: socketPath) else { - SessionIO.close(lock) - return .failed("unable to listen on \(socketPath)") - } - return .running( - SessionDaemon( - socketPath: socketPath, - idleTimeout: idleTimeoutMilliseconds, - listener: listener, - lockDescriptor: lock, - signalPipe: signalPipe)) - } - - private init( - socketPath: String, - idleTimeout: Int32, - listener: Int32, - lockDescriptor: Int32, - signalPipe: SessionSignalPipe - ) { - self.socketPath = socketPath - self.idleTimeout = idleTimeout - self.listener = listener - self.lockDescriptor = lockDescriptor - self.signalPipe = signalPipe - } - - func run() { - while true { - let isIdle = sessions.isEmpty && connections.isEmpty - var descriptors = buildPollDescriptors() - let ready = poll(&descriptors, nfds_t(descriptors.count), pollTimeout(isIdle: isIdle)) - if ready == 0 { - if processExpiredRedraws() { continue } - guard sessions.isEmpty, connections.isEmpty else { continue } - acceptConnections() - guard connections.isEmpty else { continue } - break - } - if ready < 0 { - switch errno { - case EINTR, EAGAIN, ENOMEM: - continue - default: - SessionLog.write("poll failed: \(String(cString: strerror(errno)))") - break - } - break - } - handle(descriptors) - processExpiredRedraws() - reapChildren() - flushConnections() - closeMarkedConnections() - } - shutdown() - } - - /// The nearest pending redraw's deadline overrides the ordinary idle/-1 timeout whenever one is - /// outstanding, so the loop wakes up to resize-back-and-signal even with no other activity — - /// without ever blocking inside a `usleep` the way the redraw used to. - private func pollTimeout(isIdle: Bool) -> Int32 { - guard let nearest = pendingRedraws.values.map(\.deadline).min() else { - return isIdle ? idleTimeout : -1 - } - let remainingMilliseconds = (nearest - SessionClock.monotonicSeconds()) * 1000 - return Int32(max(0, remainingMilliseconds.rounded(.up))) - } - - /// Resize each expired pending redraw back to its real size and signal, non-blockingly. Collects - /// expired keys into a copy first — mutating `pendingRedraws` while iterating it directly is - /// unsafe. A session that died before its deadline is silently dropped: nothing to resize. - @discardableResult - private func processExpiredRedraws() -> Bool { - guard !pendingRedraws.isEmpty else { return false } - let now = SessionClock.monotonicSeconds() - let expired = pendingRedraws.filter { $0.value.deadline <= now } - guard !expired.isEmpty else { return false } - for (identifier, pending) in expired { - pendingRedraws.removeValue(forKey: identifier) - guard let session = sessions[identifier] else { continue } - SessionPTY.resize( - masterDescriptor: session.masterDescriptor, columns: pending.columns, rows: pending.rows) - SessionPTY.requestRedraw( - masterDescriptor: session.masterDescriptor, fallbackProcessID: session.processID) - } - return true - } - - private func shutdown() { - for session in Array(sessions.values) { - closeMaster(session) - } - for descriptor in connections.keys { - SessionIO.close(descriptor) - } - SessionIO.close(listener) - unlink(socketPath) - // Deliberately NOT unlinking the lock file: closing this descriptor releases the flock on - // its inode, but the path stays put so the next daemon's `acquireLock` opens the SAME inode - // and re-flocks it. Unlinking here would leave a window where the path still exists but is - // unlocked — a racing daemon could open+flock that inode before we unlink it, and a THIRD - // daemon started after our unlink would then create a brand-new inode and also succeed, - // leaving two daemons simultaneously believing they're the sole instance. - SessionIO.close(lockDescriptor) - } - - private func buildPollDescriptors() -> [pollfd] { - var descriptors: [pollfd] = [ - pollfd(fd: listener, events: Int16(POLLIN), revents: 0), - pollfd(fd: signalPipe.readDescriptor, events: Int16(POLLIN), revents: 0), - ] - for connection in connections.values { - var events = Int32(POLLIN) - if connection.hasPendingOutput { events |= Int32(POLLOUT) } - descriptors.append(pollfd(fd: connection.descriptor, events: Int16(events), revents: 0)) - } - for session in sessions.values where session.masterDescriptor >= 0 { - var events: Int32 = 0 - if !isClientBackedUp(session) { events |= Int32(POLLIN) } - if session.hasPendingInput { events |= Int32(POLLOUT) } - guard events != 0 else { continue } - descriptors.append(pollfd(fd: session.masterDescriptor, events: Int16(events), revents: 0)) - } - return descriptors - } - - private func isClientBackedUp(_ session: PTYSession) -> Bool { - guard let descriptor = session.clientDescriptor, - let connection = connections[descriptor] - else { return false } - return connection.pendingByteCount >= Self.clientOutputLimit - } - - private func handle(_ descriptors: [pollfd]) { - for entry in descriptors where entry.revents != 0 { - if entry.fd == listener { - acceptConnections() - } else if entry.fd == signalPipe.readDescriptor { - signalPipe.drain() - } else if connections[entry.fd] != nil { - handleConnection(entry) - } else if let identifier = sessionsByMaster[entry.fd] { - handleMaster(entry, identifier: identifier) - } - } - } - - private func acceptConnections() { - while let descriptor = SessionSocket.accept(listener) { - guard connections.count < Self.maximumConnections else { - SessionIO.close(descriptor) - continue - } - guard SessionSocket.peerUserID(descriptor) == getuid() else { - SessionLog.write("rejected connection from another user") - SessionIO.close(descriptor) - continue - } - connections[descriptor] = SessionConnection(descriptor: descriptor) - } - } - - private func handleConnection(_ entry: pollfd) { - guard let connection = connections[entry.fd] else { return } - if entry.revents & Int16(POLLOUT) != 0, !connection.flush() { - markForClose(entry.fd) - return - } - guard entry.revents & Int16(POLLIN) != 0 || entry.revents & Int16(POLLHUP) != 0 else { return } - - var reachedEnd = false - readLoop: while true { - switch SessionIO.read(entry.fd) { - case .bytes(let bytes): - connection.decoder.push(bytes) - case .wouldBlock: - break readLoop - case .endOfFile, .failed: - reachedEnd = true - break readLoop - } - } - - while true { - do { - guard let frame = try connection.decoder.next() else { break } - handle(frame: frame, connection: connection) - } catch { - SessionLog.write("dropping connection with a malformed frame") - markForClose(entry.fd) - return - } - if connection.pendingByteCount > Self.connectionOutboxLimit { - SessionLog.write( - "dropping connection \(entry.fd): reply queue exceeded \(Self.connectionOutboxLimit) bytes" - ) - markForClose(entry.fd) - return - } - } - - if reachedEnd { markForClose(entry.fd) } - } - - private func handle(frame: SessionFrame, connection: SessionConnection) { - switch frame.kind { - case .attach: - handleAttach(payload: frame.payload, connection: connection) - case .input: - guard let identifier = connection.attachedSession, let session = sessions[identifier] else { - return - } - session.appendInput(frame.payload, limit: Self.sessionInputLimit) - session.flushInput() - case .resize: - guard let identifier = connection.attachedSession, - let session = sessions[identifier], - let size = try? SessionResizePayload.decode(frame.payload) - else { return } - guard SessionWindowSizePolicy.isUsable(columns: size.columns, rows: size.rows) else { return } - SessionPTY.resize( - masterDescriptor: session.masterDescriptor, columns: size.columns, rows: size.rows) - case .list: - enqueueSessionList(sessions.values.map(\.descriptor), connection: connection) - case .info: - guard let identifier = try? SessionIdentifierPayload.decode(frame.payload) else { return } - let matches = sessions[identifier].map { [$0.descriptor] } ?? [] - enqueueSessionList(matches, connection: connection) - case .kill: - let stopped: Bool - if let identifier = try? SessionIdentifierPayload.decode(frame.payload), - let session = sessions[identifier] - { - stopped = endSession(session, status: 0, force: true) - } else { - stopped = true - } - enqueueStopResult(stopped, connection: connection) - case .killAll: - enqueueStopResult(endSessions(Array(sessions.values), status: 0), connection: connection) - case .attached, .output, .exited, .failure, .sessions, .acknowledged: - break - } - } - - private func handleAttach(payload: [UInt8], connection: SessionConnection) { - guard let request = try? SessionAttachRequest.decode(payload) else { - connection.enqueue( - SessionFrame( - kind: .failure, payload: SessionTextPayload.encode("malformed attach request"))) - connection.closesAfterFlush = true - return - } - guard request.version == SessionProtocolVersion.current else { - connection.enqueue( - SessionFrame( - kind: .failure, - payload: SessionTextPayload.encode( - "this background session is owned by a different version of Workroom; stop it to start a new one" - ))) - connection.closesAfterFlush = true - return - } - guard connection.attachedSession == nil else { return } - - if let session = sessions[request.identifier] { - attach(session: session, request: request, connection: connection) - return - } - create(request: request, connection: connection) - } - - private func attach( - session: PTYSession, request: SessionAttachRequest, connection: SessionConnection - ) { - if let previous = session.clientDescriptor, previous != connection.descriptor { - connections[previous]?.attachedSession = nil - markForClose(previous) - } - session.clientDescriptor = connection.descriptor - session.metadata = request.metadata - connection.attachedSession = session.identifier - - if SessionWindowSizePolicy.isUsable(columns: request.columns, rows: request.rows) { - SessionPTY.resize( - masterDescriptor: session.masterDescriptor, - columns: request.columns, - rows: request.rows) - } - connection.enqueue( - SessionFrame( - kind: .attached, - payload: SessionAttachAccepted( - created: false, - shellProcessID: session.processID, - ttyDevice: session.ttyDevice - ).encoded())) - // Synthesize a fresh title (OSC 2) and pwd (OSC 7) from the pty's CURRENT foreground process - // before replaying anything. Both are normally set once by the shell's preexec/prompt hooks — - // one-shot escapes emitted on the PRIMARY screen, before the foreground program (typically) - // switches to the alternate screen — and `SessionReplayBuffer` drops its buffer the moment the - // alternate screen activates, so a reattaching client never sees them; the foreground program - // itself has no reason to re-emit either on the redraw `requestRedraw` forces. Excluding the - // shell's own pgid (`forkpty` makes an idle prompt's foreground group equal `session.processID`) - // keeps an idle reattached pane from showing a phantom "zsh"/"bash" title and stale pwd. - if let group = SessionPTY.foregroundProcessGroup(masterDescriptor: session.masterDescriptor), - group != session.processID - { - // Sanitized before ANY use below — both the log line and the OSC payloads. `name`/`cwd` come - // from OS process introspection (an exec'd argv[0], a directory's real name) and can legally - // contain any byte a filesystem path or exec argument allows, including a raw ESC or BEL. Left - // unsanitized, such a byte would prematurely terminate the OSC 2/7 sequence it's interpolated - // into (from the terminal's perspective) and let the remainder be reinterpreted as fresh, - // attacker-chosen escape/terminal input in the reattaching client. - let name = SessionPTY.executableName(processID: group).map(Self.sanitizedForEscapeSequence) - let cwd = SessionPTY.workingDirectory(processID: group).map(Self.sanitizedForEscapeSequence) - SessionLog.write( - "attach \(session.identifier.uuidString): foreground pgid=\(group) name=" - + "\(name ?? "?") cwd=\(cwd ?? "?")") - if let name, !name.isEmpty { - connection.enqueue( - SessionFrame(kind: .output, payload: Array("\u{1B}]2;\(name)\u{07}".utf8))) - } - if let cwd, !cwd.isEmpty { - // Ghostty's OSC 7 handler validates the host against the local machine's hostname before - // accepting the path (`internal_os.hostname.isLocal`) — except for the literal string - // "localhost", which it special-cases as always-local. A locally-forked pty is never - // remote, so that's the simpler, mismatch-proof choice over resolving the real hostname. - connection.enqueue( - SessionFrame( - kind: .output, payload: Array("\u{1B}]7;kitty-shell-cwd://localhost\(cwd)\u{07}".utf8))) - } - } else { - SessionLog.write( - "attach \(session.identifier.uuidString): foreground is the login shell " - + "(pid=\(session.processID)); skipping title/pwd synthesis") - } - enqueueReplay(session: session, connection: connection) - // Query the CURRENT size fresh rather than trusting `request.columns/rows` — those may have - // been rejected as unusable above (a transient tiny size from a not-yet-laid-out surface), in - // which case the redraw must still bump from/to whatever size the session's pty actually has. - // - // The bump happens now (cheap, a single ioctl); the resize-back-and-signal is DEFERRED to the - // poll loop (`processExpiredRedraws`), not done here inline with a blocking sleep — this daemon - // is single-threaded and serves every session across every open project, so sleeping here would - // freeze all of them for the duration, worst case stacking to N × the settle window when many - // persisted tabs reattach at once on relaunch. - if let current = SessionPTY.windowSize(descriptor: session.masterDescriptor) { - SessionPTY.resize( - masterDescriptor: session.masterDescriptor, columns: current.columns, - rows: SessionPTY.bumpedRowCount(current.rows)) - pendingRedraws[session.identifier] = PendingRedraw( - deadline: SessionClock.monotonicSeconds() + Self.redrawSettleSeconds, - columns: current.columns, rows: current.rows) - } - } - - /// `SessionDescriptor.encodeList`'s wire encoding has no cap of its own relative to - /// `SessionFrame.maximumPayloadSize` — a `list` reply grows with both session COUNT (up to - /// `maximumSessions`) and each session's client-supplied metadata, and unlike `.output` - /// (`enqueueReplay`, chunked across multiple frames) a `.sessions` reply is a single frame that - /// callers (`SessionControlClient.list`) expect to decode whole. Enqueueing an oversized one - /// wouldn't just be slow — every reader's decoder, on ANY connection, permanently fails the - /// instant it sees a declared length past `maximumPayloadSize` (`SessionFrameDecoder.next()` - /// sets `failed = true` and throws `frameTooLarge` from then on), poisoning that connection. - /// Refuse to build that frame at all; the daemon is otherwise healthy; a `.failure` reply lets - /// the client's existing "no answer" handling apply cleanly rather than needing a new one. - private func enqueueSessionList(_ descriptors: [SessionDescriptor], connection: SessionConnection) - { - let payload = SessionDescriptor.encodeList(descriptors) - guard payload.count <= SessionFrame.maximumPayloadSize else { - SessionLog.write( - "session list payload (\(payload.count) bytes) exceeds the frame maximum " - + "(\(SessionFrame.maximumPayloadSize)); refusing to send a malformed frame") - connection.enqueue( - SessionFrame( - kind: .failure, - payload: SessionTextPayload.encode("too many sessions to list in a single reply"))) - return - } - connection.enqueue(SessionFrame(kind: .sessions, payload: payload)) - } - - private func enqueueReplay(session: PTYSession, connection: SessionConnection) { - let bytes = session.replay.replayBytes - guard !bytes.isEmpty else { return } - SessionLog.write( - "attach \(session.identifier.uuidString): replaying \(bytes.count) bytes: " - + Self.escapedPreview(bytes)) - var index = 0 - while index < bytes.count { - let end = min(index + Self.replayChunkSize, bytes.count) - connection.enqueue(SessionFrame(kind: .output, payload: Array(bytes[index..= 0 else { return } - sessionsByMaster.removeValue(forKey: session.masterDescriptor) - SessionIO.close(session.masterDescriptor) - session.masterDescriptor = -1 - if signalProcess { - SessionPTY.terminate(processID: session.processID) - } - } - - @discardableResult - private func endSession( - _ session: PTYSession, status: Int32, force: Bool = false, signalProcess: Bool = true - ) -> Bool { - // On the force path, the process-tree snapshot that finds a setsid-detached descendant must be - // taken BEFORE anything can kill the session's shell out from under it — `closeMaster` - // unconditionally closes the pty's master descriptor, and closing a pty's last open master fd - // is itself a kernel-generated hangup (SIGHUP to the foreground process group) regardless of - // the `signalProcess` flag, which only gates our own EXPLICIT killpg call. So the snapshot - // (`terminationTargets`) runs first; closeMaster runs right after it, and that same fast kernel - // hangup is left to kill the shell in the common case, same as before this existed. Measured: - // making forceTerminate's own SIGTERM/SIGKILL loop the ONLY thing killing the shell (by moving - // closeMaster fully after it) made an ordinary, non-detached kill occasionally exceed the - // loop's retry budget under load and get reported as a failed stop — reordering back to this - // (snapshot first, then the fast hangup) is what resolved that without shrinking the snapshot's - // correctness window. - guard !force else { - let targets = SessionPTY.terminationTargets(for: [session.processID]) - closeMaster(session, signalProcess: false) - // The master is closed either way, so the daemon can never serve this session again - // regardless of whether forceTerminate reports a failure — finishSession must run - // unconditionally or the session zombifies: still in `sessions`, masterDescriptor == -1, - // reachable by list/info/attach but never sent `.exited`. - let failed = !SessionPTY.forceTerminate(targets: targets).isEmpty - if failed { - SessionLog.write("failed to stop session \(session.identifier.uuidString)") - } - finishSession(session, status: status) - return !failed - } - closeMaster(session, signalProcess: signalProcess) - finishSession(session, status: status) - return true - } - - private func endSessions(_ endingSessions: [PTYSession], status: Int32) -> Bool { - // Always force-path semantics here (there's no non-force `killAll`) — see endSession's doc - // comment for why the snapshot must be taken before closeMaster runs. - let targets = SessionPTY.terminationTargets(for: endingSessions.map(\.processID)) - for session in endingSessions { closeMaster(session, signalProcess: false) } - let failedProcessIDs = SessionPTY.forceTerminate(targets: targets) - // finishSession runs for every session regardless of forceTerminate's result — its master is - // already closed above either way, so leaving a "failed" session in `sessions` only zombifies - // it (see endSession's matching comment). - for session in endingSessions { - finishSession(session, status: status) - if failedProcessIDs.contains(session.processID) { - SessionLog.write("failed to stop session \(session.identifier.uuidString)") - } - } - return failedProcessIDs.isEmpty - } - - private func finishSession(_ session: PTYSession, status: Int32) { - sessions.removeValue(forKey: session.identifier) - guard let descriptor = session.clientDescriptor, let connection = connections[descriptor] - else { return } - connection.enqueue( - SessionFrame(kind: .exited, payload: SessionExitPayload.encode(status: status))) - connection.attachedSession = nil - connection.closesAfterFlush = true - session.clientDescriptor = nil - } - - private func enqueueStopResult(_ stopped: Bool, connection: SessionConnection) { - let frame = - stopped - ? SessionFrame(kind: .acknowledged) - : SessionFrame( - kind: .failure, payload: SessionTextPayload.encode("failed to stop one or more sessions")) - connection.enqueue(frame) - } - - private func reapChildren() { - while true { - var status: Int32 = 0 - let processID = waitpid(-1, &status, WNOHANG) - guard processID > 0 else { return } - guard let session = sessions.values.first(where: { $0.processID == processID }) else { - continue - } - // This waitpid call just reaped processID, freeing it back to the OS — see closeMaster's - // doc comment for why signaling it now would be signaling whatever the OS may have already - // recycled that number to, not this session's (already-dead) shell. - endSession(session, status: Self.exitCode(status), signalProcess: false) - } - } - - static func exitCode(_ status: Int32) -> Int32 { - let terminationSignal = status & 0o177 - if terminationSignal == 0 { return (status >> 8) & 0xFF } - if terminationSignal == 0o177 { return 0 } - return 128 + terminationSignal - } - - /// Renders bytes for a log line: printable ASCII passes through, `ESC` becomes `` (so - /// escape-sequence boundaries are visible at a glance), everything else becomes `\xNN`. Capped so - /// one attach with a large buffer can't blow up the log file. - private static func escapedPreview(_ bytes: [UInt8], limit: Int = 2048) -> String { - let hexDigits = Array("0123456789ABCDEF") - var result = "" - for byte in bytes.prefix(limit) { - switch byte { - case 0x1B: - result += "" - case 0x20...0x7E: - result.append(Character(UnicodeScalar(byte))) - default: - result += "\\x" - result.append(hexDigits[Int(byte >> 4)]) - result.append(hexDigits[Int(byte & 0x0F)]) - } - } - if bytes.count > limit { result += "…(+\(bytes.count - limit) more bytes)" } - return result - } - - /// Strips C0 controls (0x00–0x1F), DEL (0x7F), and C1 controls (0x80–0x9F) — the bytes an 8-bit - /// or 7-bit terminal could read as an escape/CSI/OSC introducer or terminator — from a value - /// before it's interpolated into an OSC payload or a log line. `name`/`cwd` come from OS process - /// introspection (`SessionPTY.executableName`/`workingDirectory`), not from this daemon's own - /// generated data, so they must be treated as untrusted content here. - private static func sanitizedForEscapeSequence(_ value: String) -> String { - String( - String.UnicodeScalarView( - value.unicodeScalars.filter { scalar in - scalar.value >= 0x20 && scalar.value != 0x7F - && !(0x80...0x9F).contains(scalar.value) - })) - } - - private func flushConnections() { - for connection in connections.values { - if !connection.flush() { - markForClose(connection.descriptor) - continue - } - if connection.closesAfterFlush, !connection.hasPendingOutput { - markForClose(connection.descriptor) - } - } - } - - private func markForClose(_ descriptor: Int32) { - guard !closingDescriptors.contains(descriptor) else { return } - closingDescriptors.append(descriptor) - } - - private func closeMarkedConnections() { - let descriptors = closingDescriptors - closingDescriptors.removeAll(keepingCapacity: true) - for descriptor in descriptors { - guard let connection = connections.removeValue(forKey: descriptor) else { continue } - if let identifier = connection.attachedSession, - let session = sessions[identifier], - session.clientDescriptor == descriptor - { - session.clientDescriptor = nil - } - SessionIO.close(descriptor) - } - } -} diff --git a/macapp/WorkroomSession/SessionPTY.swift b/macapp/WorkroomSession/SessionPTY.swift deleted file mode 100644 index b12fa06dd..000000000 --- a/macapp/WorkroomSession/SessionPTY.swift +++ /dev/null @@ -1,384 +0,0 @@ -import Darwin -import WorkroomSessionProtocol - -struct SessionProcess { - let masterDescriptor: Int32 - let processID: pid_t - let ttyDevice: UInt64 -} - -enum SessionPTY { - private static let resetSignals: [Int32] = [ - SIGPIPE, SIGCHLD, SIGHUP, SIGINT, SIGQUIT, SIGTERM, SIGTSTP, SIGTTIN, SIGTTOU, - ] - private static let terminationPollMicroseconds: useconds_t = 10000 - private static let gracefulTerminationAttempts = 20 - private static let forcedTerminationAttempts = 50 - - static func spawn( - invocation: SessionShellInvocation, - workingDirectory: String, - columns: UInt16, - rows: UInt16 - ) -> SessionProcess? { - let argumentList = SessionCStringArray(invocation.arguments) - let environmentList = SessionCStringArray( - invocation.environment.map { "\($0.key)=\($0.value)" }) - guard let executable = strdup(invocation.executable) else { return nil } - let directory = strdup(workingDirectory) - defer { - free(executable) - free(directory) - } - - // The classic self-pipe trick for synchronous fork/exec error reporting: both ends are - // close-on-exec, so a SUCCESSFUL execve closes the child's copy of the write end as part of - // the exec syscall itself, and the parent's blocking read below observes that as EOF. A - // FAILED execve never reaches that closure — the child is still running our Swift code, so it - // writes its errno first. Without this, `forkpty` returning a pid tells the parent only that - // fork succeeded, not that the child ever reached its shell; a bad shell path or resources - // directory would ack the attach as created, then close moments later with no explanation. - var execErrorPipe: [Int32] = [-1, -1] - guard pipe(&execErrorPipe) == 0 else { - SessionLog.write("pipe failed: \(String(cString: strerror(errno)))") - return nil - } - let execErrorReadDescriptor = execErrorPipe[0] - let execErrorWriteDescriptor = execErrorPipe[1] - SessionIO.setCloseOnExec(execErrorReadDescriptor) - SessionIO.setCloseOnExec(execErrorWriteDescriptor) - - var size = winsize( - ws_row: rows == 0 ? 24 : rows, - ws_col: columns == 0 ? 80 : columns, - ws_xpixel: 0, - ws_ypixel: 0) - var master: Int32 = -1 - var name = [CChar](repeating: 0, count: Int(PATH_MAX)) - - let processID = forkpty(&master, &name, nil, &size) - if processID < 0 { - SessionLog.write("forkpty failed: \(String(cString: strerror(errno)))") - SessionIO.close(execErrorReadDescriptor) - SessionIO.close(execErrorWriteDescriptor) - return nil - } - - if processID == 0 { - SessionIO.close(execErrorReadDescriptor) - var empty = sigset_t() - sigemptyset(&empty) - sigprocmask(SIG_SETMASK, &empty, nil) - for number in resetSignals { - signal(number, SIG_DFL) - } - if let directory, chdir(directory) != 0 { - if let home = getenv("HOME") { - _ = chdir(home) - } else { - _ = chdir("/") - } - } - execve(executable, argumentList.pointer, environmentList.pointer) - var failureErrno = errno - withUnsafeBytes(of: &failureErrno) { bytes in - _ = Darwin.write(execErrorWriteDescriptor, bytes.baseAddress, bytes.count) - } - _exit(127) - } - - // Parent must close its own copy of the write end before reading — otherwise the pipe never - // fully closes on a successful exec (the child's copy closes, but ours would still hold it - // open), and the read below would block forever instead of observing EOF. - SessionIO.close(execErrorWriteDescriptor) - var execFailureErrno: Int32 = 0 - let errorByteCount = withUnsafeMutableBytes(of: &execFailureErrno) { buffer -> Int in - var total = 0 - while total < buffer.count { - let result = Darwin.read( - execErrorReadDescriptor, buffer.baseAddress!.advanced(by: total), buffer.count - total) - if result > 0 { - total += result - continue - } - if result < 0, errno == EINTR { continue } - break - } - return total - } - SessionIO.close(execErrorReadDescriptor) - guard errorByteCount == 0 else { - SessionLog.write("execve failed: \(String(cString: strerror(execFailureErrno)))") - SessionIO.close(master) - return nil - } - - SessionIO.setNonBlocking(master) - SessionIO.setCloseOnExec(master) - - var status = stat() - let device: UInt64 = - stat(&name, &status) == 0 - ? UInt64(UInt32(bitPattern: status.st_rdev)) - : 0 - - return SessionProcess(masterDescriptor: master, processID: processID, ttyDevice: device) - } - - static func resize(masterDescriptor: Int32, columns: UInt16, rows: UInt16) { - var size = winsize( - ws_row: rows == 0 ? 24 : rows, - ws_col: columns == 0 ? 80 : columns, - ws_xpixel: 0, - ws_ypixel: 0) - _ = ioctl(masterDescriptor, TIOCSWINSZ, &size) - } - - static func windowSize(descriptor: Int32) -> (columns: UInt16, rows: UInt16)? { - var size = winsize() - guard ioctl(descriptor, TIOCGWINSZ, &size) == 0 else { return nil } - return (size.ws_col, size.ws_row) - } - - static func foregroundProcessGroup(masterDescriptor: Int32) -> pid_t? { - let group = tcgetpgrp(masterDescriptor) - return group > 0 ? group : nil - } - - /// The basename of `argv[0]` as the process was originally exec'd (e.g. `claude`, `vim`), or nil - /// if it can't be resolved. Deliberately NOT `proc_name`/`p_comm`: some CLIs (Claude Code among - /// them, observed in the wild) rename their own process title after launch — `p_comm` then reads - /// as their version string ("2.1.232"), not the command a user typed. `KERN_PROCARGS2` returns - /// the ORIGINAL exec-time argv snapshot, unaffected by any later self-renaming, and matches what - /// the shell's own preexec title-report already uses for a live (non-reattached) pane. - static func executableName(processID: pid_t) -> String? { - guard processID > 0 else { return nil } - var mib: [Int32] = [CTL_KERN, KERN_PROCARGS2, processID] - var size = 0 - guard sysctl(&mib, 3, nil, &size, nil, 0) == 0, size > 4 else { return nil } - var buffer = [UInt8](repeating: 0, count: size) - guard sysctl(&mib, 3, &buffer, &size, nil, 0) == 0, size > 4 else { return nil } - - let argc = buffer.withUnsafeBytes { $0.loadUnaligned(as: Int32.self) } - guard argc > 0 else { return nil } - - // Layout: argc, then the exec path (NUL-terminated), then NUL padding, then argv[0..argc-1] - // each NUL-terminated. Skip the exec path and padding to reach argv[0]. - var offset = 4 - while offset < size, buffer[offset] != 0 { offset += 1 } - while offset < size, buffer[offset] == 0 { offset += 1 } - guard offset < size else { return nil } - - let start = offset - while offset < size, buffer[offset] != 0 { offset += 1 } - guard offset > start else { return nil } - let argv0 = String(decoding: buffer[start.. String? { - var info = proc_vnodepathinfo() - let size = proc_pidinfo( - processID, PROC_PIDVNODEPATHINFO, 0, &info, Int32(MemoryLayout.size)) - guard size == Int32(MemoryLayout.size) else { return nil } - return withUnsafeBytes(of: &info.pvi_cdir.vip_path) { raw in - raw.baseAddress.map { String(cString: $0.assumingMemoryBound(to: CChar.self)) } - } - } - - /// Row count to bump a pty to, transiently, to force an observable size delta. - /// - /// A bare `SIGWINCH` is not enough to make a full-screen program repaint on reattach: the pty's - /// real size is already correct (it never changed while the session sat alive in the - /// background), so a program that checks "did the terminal size actually change" before - /// repainting — observed with Claude Code; Codex repaints regardless — silently ignores it, - /// leaving the new client showing stale primary-screen content underneath the (unpainted) - /// alternate screen until a genuine later resize forces a real delta. - /// - /// Bumping the row count down (or up, at the 1-row floor) then back — the same trick tmux/screen - /// use to force a client redraw on attach — guarantees the child observes an actual size - /// transition either way. Pure arithmetic only: the daemon owns *when* to resize back and signal - /// (`SessionDaemon`'s deferred redraw scheduling), since giving the child time to actually observe - /// the transient size before reverting it needs a real delay — and this single-threaded daemon - /// serves every session across every open project, so that delay must not block its poll loop. - static func bumpedRowCount(_ rows: UInt16) -> UInt16 { - rows > 1 ? rows - 1 : rows + 1 - } - - static func requestRedraw(masterDescriptor: Int32, fallbackProcessID: pid_t) { - let group = foregroundProcessGroup(masterDescriptor: masterDescriptor) ?? fallbackProcessID - guard group > 0 else { return } - _ = killpg(group, SIGWINCH) - } - - static func terminate(processID: pid_t) { - guard processID > 0 else { return } - if killpg(processID, SIGHUP) != 0 { - _ = kill(processID, SIGHUP) - } - _ = killpg(processID, SIGCONT) - } - - /// Every pid that must be signaled to end `processIDs`, captured by a parent-pid tree walk - /// BEFORE any signal goes out. A shell can `setsid()` a descendant into its own new session (a - /// deliberate double fork, rare but not impossible from inside a persisted shell) — at that - /// point it's escaped both `killpg` (process-group scoping) and the `getsid`-based matching in - /// `forceTerminate` (session scoping), since it is now the leader of a session nothing here is - /// tracking. Snapshotting by PARENT pid, once, up front, finds it anyway: whatever session it - /// has since made itself leader of, its ppid still chains back to a root here (unless an - /// intermediate ancestor has ALREADY exited and been reparented to launchd — a real but much - /// narrower race than the one this closes, and not one `pkill`-style tooling solves without pid - /// namespaces either). Callers that also close a session's pty master (itself a kernel-level - /// hangup that can kill the root out from under a later snapshot) must call this FIRST, before - /// that close, then pass the result to `forceTerminate(targets:)`. - static func terminationTargets(for processIDs: [pid_t]) -> Set { - let roots = Set(processIDs.filter { $0 > 0 }) - guard !roots.isEmpty else { return [] } - return roots.union(descendantProcessIDs(of: roots)) - } - - static func forceTerminate(targets: Set) -> Set { - guard !targets.isEmpty else { return [] } - signalSessions(targets, signal: SIGTERM) - let resistant = waitForSessionsExit(targets, attempts: gracefulTerminationAttempts) - guard !resistant.isEmpty else { return [] } - - signalSessions(resistant, signal: SIGKILL) - return waitForSessionsExit( - resistant, attempts: forcedTerminationAttempts, repeatedSignal: SIGKILL) - } - - private static func signalSessions(_ sessionIDs: Set, signal: Int32) { - guard let members = sessionProcessIDs(sessionIDs: sessionIDs) else { - for sessionID in sessionIDs { - _ = killpg(sessionID, signal) - _ = kill(sessionID, signal) - } - return - } - signalSessions(sessionIDs, members: members, signal: signal) - } - - private static func signalSessions( - _ sessionIDs: Set, members: [pid_t: [pid_t]], signal: Int32 - ) { - for sessionID in sessionIDs { - let memberProcessIDs = members[sessionID] ?? [] - if memberProcessIDs.isEmpty { - _ = kill(sessionID, signal) - continue - } - for memberProcessID in memberProcessIDs { - _ = kill(memberProcessID, signal) - } - } - } - - private static func waitForSessionsExit( - _ sessionIDs: Set, - attempts: Int, - repeatedSignal: Int32? = nil - ) -> Set { - var remaining = sessionIDs - for _ in 0.., members: [pid_t: [pid_t]]) - -> Set - { - sessionIDs.filter { processExists($0) || !(members[$0] ?? []).isEmpty } - } - - private static func reap(processIDs: Set) { - for processID in processIDs { - var status: Int32 = 0 - while waitpid(processID, &status, WNOHANG) < 0, errno == EINTR {} - } - } - - private static func processExists(_ processID: pid_t) -> Bool { - guard kill(processID, 0) != 0 else { return true } - return errno == EPERM - } - - private static func sessionProcessIDs(sessionIDs: Set) -> [pid_t: [pid_t]]? { - guard let records = snapshotProcesses() else { return nil } - var members: [pid_t: [pid_t]] = [:] - for record in records { - let processID = record.kp_proc.p_pid - guard processID > 0 else { continue } - let sessionID = getsid(processID) - guard sessionIDs.contains(sessionID) else { continue } - members[sessionID, default: []].append(processID) - } - return members - } - - /// Every transitive descendant of `roots`, found by walking parent-pid links rather than - /// session/group membership — the only way to still find a descendant that has `setsid()`'d - /// itself into its own session. Returns an empty set (never `nil`) on a scan failure: the caller - /// already falls back to signaling `roots` alone, exactly as it did before this existed. - private static func descendantProcessIDs(of roots: Set) -> Set { - guard let records = snapshotProcesses() else { return [] } - var childrenByParent: [pid_t: [pid_t]] = [:] - for record in records { - let processID = record.kp_proc.p_pid - guard processID > 0 else { continue } - childrenByParent[record.kp_eproc.e_ppid, default: []].append(processID) - } - - var descendants: Set = [] - var frontier = Array(roots) - while !frontier.isEmpty { - var next: [pid_t] = [] - for parent in frontier { - for child in childrenByParent[parent] ?? [] - where !descendants.contains(child) && !roots.contains(child) { - descendants.insert(child) - next.append(child) - } - } - frontier = next - } - return descendants - } - - private static func snapshotProcesses() -> [kinfo_proc]? { - let stride = MemoryLayout.stride - for _ in 0..<3 { - var name: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_ALL] - var size = 0 - guard sysctl(&name, 3, nil, &size, nil, 0) == 0, size > 0 else { return nil } - - var records = [kinfo_proc](repeating: kinfo_proc(), count: size / stride + 16) - size = records.count * stride - if sysctl(&name, 3, &records, &size, nil, 0) != 0 { - guard errno == ENOMEM else { return nil } - continue - } - return Array(records.prefix(size / stride)) - } - return nil - } -} diff --git a/macapp/WorkroomSession/SessionSocket.swift b/macapp/WorkroomSession/SessionSocket.swift index 6d703f3fd..572f4fd97 100644 --- a/macapp/WorkroomSession/SessionSocket.swift +++ b/macapp/WorkroomSession/SessionSocket.swift @@ -2,40 +2,6 @@ import Darwin import WorkroomSessionProtocol enum SessionSocket { - static let backlog: Int32 = 16 - - static func makeListener(path: String) -> Int32? { - guard let address = makeAddress(path: path) else { - SessionLog.write("socket path too long: \(path)") - return nil - } - unlink(path) - let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) - guard descriptor >= 0 else { return nil } - SessionIO.setCloseOnExec(descriptor) - - var storage = address - let previousMask = umask(0o077) - let bound = withUnsafePointer(to: &storage) { pointer in - pointer.withMemoryRebound(to: sockaddr.self, capacity: 1) { casted in - bind(descriptor, casted, socklen_t(MemoryLayout.size)) - } - } - umask(previousMask) - guard bound == 0 else { - SessionLog.write("bind failed: \(String(cString: strerror(errno)))") - SessionIO.close(descriptor) - return nil - } - chmod(path, 0o600) - guard listen(descriptor, backlog) == 0 else { - SessionLog.write("listen failed: \(String(cString: strerror(errno)))") - SessionIO.close(descriptor) - return nil - } - SessionIO.setNonBlocking(descriptor) - return descriptor - } static func connect(path: String, timeoutMilliseconds: Int32 = 1000) -> Int32? { guard let address = makeAddress(path: path) else { return nil } @@ -71,35 +37,6 @@ enum SessionSocket { return descriptor } - static func accept(_ listener: Int32) -> Int32? { - let descriptor = Darwin.accept(listener, nil, nil) - guard descriptor >= 0 else { return nil } - SessionIO.setCloseOnExec(descriptor) - SessionIO.setNonBlocking(descriptor) - return descriptor - } - - static func peerUserID(_ descriptor: Int32) -> uid_t? { - var credentials = xucred() - var length = socklen_t(MemoryLayout.size) - let result = withUnsafeMutablePointer(to: &credentials) { pointer in - getsockopt(descriptor, SOL_LOCAL, LOCAL_PEERCRED, pointer, &length) - } - guard result == 0, credentials.cr_version == XUCRED_VERSION else { return nil } - return credentials.cr_uid - } - - static func acquireLock(path: String) -> Int32? { - let descriptor = open(path, O_CREAT | O_RDWR, 0o600) - guard descriptor >= 0 else { return nil } - SessionIO.setCloseOnExec(descriptor) - guard flock(descriptor, LOCK_EX | LOCK_NB) == 0 else { - SessionIO.close(descriptor) - return nil - } - return descriptor - } - private static func makeAddress(path: String) -> sockaddr_un? { var address = sockaddr_un() address.sun_family = sa_family_t(AF_UNIX) diff --git a/macapp/WorkroomSession/SessionSupport.swift b/macapp/WorkroomSession/SessionSupport.swift index 873ae3f4b..3afc50fee 100644 --- a/macapp/WorkroomSession/SessionSupport.swift +++ b/macapp/WorkroomSession/SessionSupport.swift @@ -1,6 +1,20 @@ import Darwin import WorkroomSessionProtocol +/// The terminal geometry of a descriptor. +/// +/// All that survives of `SessionPTY`, which was deleted with the daemon: this is the one thing the +/// attach client needed from it, and the only part that was never about owning a pty. Everything +/// else there — `spawn`, `terminate`, the foreground-pgid and `/proc`-equivalent introspection — +/// existed to RUN sessions, which this build no longer does. +enum SessionTerminalSize { + static func of(descriptor: Int32) -> (columns: UInt16, rows: UInt16)? { + var size = winsize() + guard ioctl(descriptor, TIOCGWINSZ, &size) == 0 else { return nil } + return (size.ws_col, size.ws_row) + } +} + /// Monotonic clock, unaffected by wall-clock jumps (NTP, user changing the date) — used for /// short-lived deadlines (poll timeouts, settle windows) where a `Date()`-based one could stall /// or fire early. @@ -41,27 +55,6 @@ final class SessionSignalPipe { } } -final class SessionCStringArray { - private let storage: UnsafeMutablePointer?> - private let count: Int - - init(_ values: [String]) { - count = values.count - storage = UnsafeMutablePointer?>.allocate(capacity: count + 1) - for (index, value) in values.enumerated() { - storage[index] = strdup(value) - } - storage[count] = nil - } - - var pointer: UnsafeMutablePointer?> { storage } - - deinit { - for index in 0.. Never { exit(2) } +/// `daemon` is deliberately absent. This binary is an attach-only CLIENT: it connects to a daemon +/// an app at or before v2.0.0 left running and relays a session it already holds. It cannot start +/// one, and starting one would serve no session that exists — every new session goes to `wr-agent`. +/// See `docs/designs/remote-workrooms.md`. func usage() -> Never { fail( """ - usage: workroom-session daemon --socket - workroom-session attach + usage: workroom-session attach workroom-session list --socket workroom-session kill --socket --session workroom-session kill --socket --all @@ -59,28 +62,44 @@ let arguments = CommandLine.arguments guard arguments.count > 1 else { usage() } switch arguments[1] { -case "daemon": - guard let socketPath = argumentValue("--socket", in: arguments) else { - fail("usage: workroom-session daemon --socket ") - } - let idleTimeout = - argumentValue("--idle-timeout", in: arguments).flatMap(Int32.init) - ?? SessionDaemon.idleTimeoutMilliseconds - switch SessionDaemon.start(socketPath: socketPath, idleTimeoutMilliseconds: idleTimeout) { - case .running(let daemon): - daemon.run() - exit(0) - case .lockHeld: - exit(0) - case .failed(let message): - fail(message) - } - case "attach": + // Only a relay falls back. A hand-typed `attach` with no environment gets an error, exactly as + // `list` and `kill` do — opening a login shell inside the user's own shell would be absurd. guard let configuration = attachConfiguration() else { fail("workroom-session: WORKROOM_SESSION_ID and WORKROOM_SESSION_SOCKET are required") } - exit(SessionAttachClient.run(configuration: configuration)) + let status = SessionAttachClient.run(configuration: configuration) + // **The pane must not die because the helper did.** `config.wait_after_command` is false, so a + // relay that simply exits leaves a pane with no shell in it — nothing to read the error in, and + // nothing to type into. The Rust agent solved this with `fall_back_to_shell`; this client had no + // equivalent, so the very failure this branch introduced a bound for (a v2.0.0 daemon that + // accepts and then stalls) destroyed the pane rather than degrading it, while the identical + // failure against the agent degraded to a working shell. Two relays behind one feature must fail + // the same way. + // + // `finished` and `protocolFailure` never fall back: the first carries the shell's own exit + // status and must be reported verbatim, and the second means the peer is not a session helper at + // all. An earlier version of this comment claimed the list below was "only the pre-attach + // outcomes". It is not, and `transportFailure` is the exception — see below. + // `transportFailure` is the one that is NOT pre-attach: `transportOutcome(isAttached:)` returns + // it only once the `.attached` frame has landed, and answers `.retry` before that. It still falls + // back — exiting leaves a pane with nothing in it either way — but it cannot borrow the pre-attach + // message, which says persistence is gone. Here the session may well be alive and simply out of + // reach, so say that instead and point at the thing that recovers it. + if status == SessionAttachExitCode.transportFailure { + SessionIO.writeAll( + STDERR_FILENO, + Array( + ("workroom-session: lost the connection to the session helper. If it is still running, " + + "closing and reopening this terminal will reattach.\r\n").utf8)) + } + if status == SessionAttachExitCode.daemonUnavailable + || status == SessionAttachExitCode.transportFailure + || status == SessionAttachExitCode.startupFailure + { + SessionAttachFallback.becomeShell(configuration: configuration) + } + exit(status) case "list": guard let socketPath = argumentValue("--socket", in: arguments) else { diff --git a/macapp/WorkroomSessionProtocol/SessionMessages.swift b/macapp/WorkroomSessionProtocol/SessionMessages.swift index c5aa3bbae..2f3cb4d01 100644 --- a/macapp/WorkroomSessionProtocol/SessionMessages.swift +++ b/macapp/WorkroomSessionProtocol/SessionMessages.swift @@ -1,3 +1,13 @@ +/// **FROZEN while the attach-only client ships.** +/// +/// Its only real peer is a `workroom-session daemon` started by an app at or before v2.0.0, running +/// on a user's machine right now. That binary can never be recompiled to match a change here, so +/// anything this module encodes differently is a terminal the user silently cannot get back. +/// `SessionShimCompatibilityTests` drives the pinned v2.0.0 binary and will fail if this drifts; +/// treat that failure as the contract breaking, not the test being stale. +/// +/// The freeze lifts when `macapp/WorkroomSession/` is deleted — see the TODOS entry that names the +/// client, the fixture and its tests as one unit. public enum SessionProtocolVersion { public static let current: UInt16 = 1 } diff --git a/macapp/WorkroomSessionProtocol/SessionReplayBuffer.swift b/macapp/WorkroomSessionProtocol/SessionReplayBuffer.swift deleted file mode 100644 index 01bcc7b4d..000000000 --- a/macapp/WorkroomSessionProtocol/SessionReplayBuffer.swift +++ /dev/null @@ -1,457 +0,0 @@ -/// Ring buffer of recent PTY output, replayed to a client that reattaches. -/// -/// Drops contents while the alternate screen is active (full-screen programs get SIGWINCH -/// instead). On wrap, leading/trailing escape and UTF-8 fragments are trimmed so a mid-sequence -/// cut does not corrupt the terminal. -/// -/// `replayBytes` also strips terminal capability *queries* (color queries, clipboard reads, device -/// attributes, cursor position reports, keyboard-protocol flags) — see its doc comment for why a -/// query is never safe to replay. -/// -/// Inspired by Muxy's SessionReplayBuffer (MIT). -public struct SessionReplayBuffer: Sendable { - private static let alternateScreenEnterSequences: [[UInt8]] = [ - [0x1B, 0x5B, 0x3F, 0x34, 0x37, 0x68], - [0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x37, 0x68], - [0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x39, 0x68], - ] - - private static let alternateScreenLeaveSequences: [[UInt8]] = [ - [0x1B, 0x5B, 0x3F, 0x34, 0x37, 0x6C], - [0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x37, 0x6C], - [0x1B, 0x5B, 0x3F, 0x31, 0x30, 0x34, 0x39, 0x6C], - ] - - private static let screenControlTailLength = max( - alternateScreenEnterSequences.map(\.count).max() ?? 0, - alternateScreenLeaveSequences.map(\.count).max() ?? 0) - - public let capacity: Int - - private var storage: [UInt8] - private var start = 0 - private var count = 0 - private var hasDiscardedBytes = false - private var alternateScreenActive = false - private var screenControlTail: [UInt8] = [] - - public init(capacity: Int) { - self.capacity = max(capacity, 0) - storage = [UInt8](repeating: 0, count: self.capacity) - } - - public var isEmpty: Bool { count == 0 } - public var byteCount: Int { count } - public var isAlternateScreenActive: Bool { alternateScreenActive } - - public mutating func append(_ bytes: [UInt8]) { - bytes.withUnsafeBufferPointer { append($0) } - } - - public mutating func append(_ bytes: UnsafeBufferPointer) { - guard capacity > 0, !bytes.isEmpty else { return } - appendForReplay(Array(bytes)) - } - - /// Replayable bytes, with terminal capability *queries* stripped (color queries and clipboard - /// reads — OSC 4/10-19/21/52 with a `?` payload — plus CSI device-attributes/status-report - /// requests ending in `c`/`n`, and DECRQM request-mode queries ending in `$p`). - /// - /// A query is a request for the CURRENT client to answer right now — a shell theme querying the - /// background color, a program probing cursor position, or (OSC 52) a request for the clipboard, - /// which Ghostty answers with the real pasteboard contents and no user confirmation. The response - /// is never captured here (only PTY *output* is buffered; the client's reply is input, flowing the - /// other way), so the query byte sequence sits in the buffer having already been answered once, - /// live, by whichever client was attached at the time. Replaying it to a newly-attaching client - /// makes that client answer it AGAIN, as if it were a fresh request — and that answer lands on - /// whatever's now in the foreground with no reader expecting it (often an idle shell prompt, or - /// for OSC 52, silently handing that foreground process the clipboard), which echoes the - /// reply as visible garbage. Queries have no visual representation of their own, so dropping them - /// from replay never loses anything the user could actually see. - public var replayBytes: [UInt8] { - guard !alternateScreenActive else { return [] } - var output = bytes - var needsLeadingFragmentCleanup = false - if hasDiscardedBytes { - let start = safeReplayStart(in: output) - needsLeadingFragmentCleanup = start == output.startIndex - output = Array(output[start...]) - } - if needsLeadingFragmentCleanup { - let leading = leadingSafeIndex(in: output) - guard leading < output.count else { return [] } - output = Array(output[leading...]) - } - let trailing = trailingSafeEnd(in: output) - guard trailing > 0 else { return [] } - return strippingQueries(Array(output[.. 0 else { return [] } - var result = [UInt8]() - result.reserveCapacity(count) - if start + count <= capacity { - result.append(contentsOf: storage[start..<(start + count)]) - } else { - result.append(contentsOf: storage[start...]) - result.append(contentsOf: storage[..<(start + count - capacity)]) - } - return result - } - - public mutating func removeAll() { - start = 0 - count = 0 - hasDiscardedBytes = false - alternateScreenActive = false - screenControlTail = [] - } - - private mutating func appendForReplay(_ bytes: [UInt8]) { - let previousTail = screenControlTail - let combined = previousTail + bytes - let newByteOffset = previousTail.count - var index = 0 - var unhandledNewByteIndex = 0 - while index < combined.count { - if alternateScreenActive { - guard - let leave = nextAlternateScreenSequence(in: combined, from: index, entering: false) - else { - updateScreenControlTail(from: combined) - return - } - removeAll() - alternateScreenActive = false - unhandledNewByteIndex = max(unhandledNewByteIndex, max(0, leave.upperBound - newByteOffset)) - index = leave.upperBound - continue - } - - guard let next = nextAlternateScreenSequence(in: combined, from: index, entering: true) else { - if unhandledNewByteIndex < bytes.count { - appendStorage(Array(bytes[unhandledNewByteIndex...])) - } - updateScreenControlTail(from: combined) - return - } - removeAll() - alternateScreenActive = true - unhandledNewByteIndex = max(unhandledNewByteIndex, max(0, next.upperBound - newByteOffset)) - index = next.upperBound - } - if !alternateScreenActive, unhandledNewByteIndex < bytes.count { - appendStorage(Array(bytes[unhandledNewByteIndex...])) - } - updateScreenControlTail(from: combined) - } - - private mutating func appendStorage(_ bytes: [UInt8]) { - guard capacity > 0, !bytes.isEmpty else { return } - if bytes.count >= capacity { - let tail = bytes.suffix(capacity) - storage.replaceSubrange(0.. Int { - guard !bytes.isEmpty else { return 0 } - guard let newline = bytes.firstIndex(where: { $0 == 0x0A || $0 == 0x0D }) else { return 0 } - return bytes.index(after: newline) - } - - private func leadingSafeIndex(in bytes: [UInt8]) -> Int { - var index = 0 - while index < bytes.count { - let byte = bytes[index] - if byte >= 0x80, byte <= 0xBF { - index += 1 - continue - } - if byte == 0x5D { - guard isLikelyBareOSCBody(in: bytes, from: index) else { return index } - guard let end = oscTerminator(in: bytes, from: index + 1) else { return bytes.count } - index = end - continue - } - if byte == 0x5B { - guard isLikelyBareCSIFragment(in: bytes, from: index) else { return index } - guard let end = csiTerminator(in: bytes, from: index + 1) else { return bytes.count } - index = end - continue - } - return index - } - return index - } - - private func isLikelyBareOSCBody(in bytes: [UInt8], from index: Int) -> Bool { - var cursor = index + 1 - var sawDigit = false - while cursor < bytes.count, bytes[cursor] >= 0x30, bytes[cursor] <= 0x39 { - sawDigit = true - cursor += 1 - } - return sawDigit && cursor < bytes.count && bytes[cursor] == 0x3B - } - - private func isLikelyBareCSIFragment(in bytes: [UInt8], from index: Int) -> Bool { - let next = index + 1 - guard next < bytes.count else { return false } - return (bytes[next] >= 0x30 && bytes[next] <= 0x3F) - || (bytes[next] >= 0x20 && bytes[next] <= 0x2F) - } - - private func trailingSafeEnd(in bytes: [UInt8]) -> Int { - var index = 0 - while index < bytes.count { - guard bytes[index] == 0x1B else { - index += 1 - continue - } - guard let end = escapeTerminator(in: bytes, from: index) else { - return trailingUTF8SafeEnd(in: bytes, endingAt: index) - } - index = end - } - return trailingUTF8SafeEnd(in: bytes, endingAt: bytes.count) - } - - private func trailingUTF8SafeEnd(in bytes: [UInt8], endingAt end: Int) -> Int { - guard end > 0 else { return 0 } - var sequenceStart = end - 1 - var continuationCount = 0 - while sequenceStart > 0, isUTF8Continuation(bytes[sequenceStart]), continuationCount < 3 { - sequenceStart -= 1 - continuationCount += 1 - } - guard let expectedLength = utf8SequenceLength(startingWith: bytes[sequenceStart]) else { - return end - } - let actualLength = end - sequenceStart - guard actualLength < expectedLength else { return end } - guard bytes[(sequenceStart + 1).. Int? { - switch byte { - case 0x00...0x7F: 1 - case 0xC2...0xDF: 2 - case 0xE0...0xEF: 3 - case 0xF0...0xF4: 4 - default: nil - } - } - - private func isUTF8Continuation(_ byte: UInt8) -> Bool { - byte >= 0x80 && byte <= 0xBF - } - - private func escapeTerminator(in bytes: [UInt8], from index: Int) -> Int? { - let next = index + 1 - guard next < bytes.count else { return nil } - switch bytes[next] { - case 0x5D: - return oscTerminator(in: bytes, from: next + 1) - case 0x50, 0x58, 0x5E, 0x5F: - return stringControlTerminator(in: bytes, from: next + 1) - case 0x5B: - return csiTerminator(in: bytes, from: next + 1) - case 0x20...0x2F: - var cursor = next + 1 - while cursor < bytes.count { - if bytes[cursor] >= 0x30, bytes[cursor] <= 0x7E { - return cursor + 1 - } - cursor += 1 - } - return nil - default: - return next + 1 - } - } - - private func oscTerminator(in bytes: [UInt8], from index: Int) -> Int? { - var cursor = index - while cursor < bytes.count { - if bytes[cursor] == 0x07 { return cursor + 1 } - if bytes[cursor] == 0x1B, cursor + 1 < bytes.count, bytes[cursor + 1] == 0x5C { - return cursor + 2 - } - cursor += 1 - } - return nil - } - - private func stringControlTerminator(in bytes: [UInt8], from index: Int) -> Int? { - var cursor = index - while cursor + 1 < bytes.count { - if bytes[cursor] == 0x1B, bytes[cursor + 1] == 0x5C { return cursor + 2 } - cursor += 1 - } - return nil - } - - private func csiTerminator(in bytes: [UInt8], from index: Int) -> Int? { - var cursor = index - while cursor < bytes.count { - if bytes[cursor] >= 0x40, bytes[cursor] <= 0x7E { return cursor + 1 } - cursor += 1 - } - return nil - } - - /// OSC commands that are exclusively used to QUERY the client (never to set anything), signaled - /// by a `?` payload — color queries and their relatives, plus 52 (clipboard read). Title (0/2), - /// pwd (7), hyperlink (8), notification (9), and shell-integration (133) OSCs are deliberately - /// excluded: those are always sets, so a payload that happens to end in a literal `?` (e.g. a - /// title reading "Continue?") must survive replay untouched. 52 belongs here for a sharper reason - /// than the color queries: Ghostty's own OSC 52 read handler answers with the REAL system - /// clipboard and no user confirmation (`GhosttyRuntimeAdapter.readClipboard`), so a buffered query - /// replayed on reattach would silently hand the current clipboard contents to whatever's now in - /// the foreground. A legitimate OSC 52 WRITE payload is base64 and never ends in a bare `?`, so - /// including it here can't misfire on a set. - private static let queryOnlyOSCCommands: Set = [ - 4, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 21, 52, - ] - - private func oscCommandNumber(in bytes: [UInt8], from index: Int, to end: Int) -> Int? { - var cursor = index - var value = 0 - var sawDigit = false - while cursor < end, bytes[cursor] >= 0x30, bytes[cursor] <= 0x39 { - value = value * 10 + Int(bytes[cursor] - 0x30) - sawDigit = true - cursor += 1 - } - return sawDigit ? value : nil - } - - /// Whether the OSC sequence `bytes[start.. Bool { - guard let command = oscCommandNumber(in: bytes, from: start, to: end), - Self.queryOnlyOSCCommands.contains(command) - else { return false } - let payloadEnd: Int - if end >= 1, bytes[end - 1] == 0x07 { - payloadEnd = end - 1 - } else if end >= 2, bytes[end - 2] == 0x1B, bytes[end - 1] == 0x5C { - payloadEnd = end - 2 - } else { - payloadEnd = end - } - return payloadEnd > start && bytes[payloadEnd - 1] == 0x3F - } - - /// Whether the CSI sequence `bytes[start.. … u` (push), and `< u` (pop) are the - /// same final byte but are not queries, so `u` needs the parameter check the other finals don't. - /// `p` needs the `$` intermediate check so DECSTR soft-reset (`CSI ! p`) isn't mistaken for it. - private func isStatusQueryCSI(_ bytes: [UInt8], start: Int, end: Int) -> Bool { - guard end > start else { return false } - switch bytes[end - 1] { - case 0x63, 0x6E: return true - case 0x75: return end - start == 2 && bytes[start] == 0x3F - case 0x70: return end - start >= 2 && bytes[end - 2] == 0x24 - default: return false - } - } - - /// Drops OSC color queries and CSI device-attributes/status-report requests from `bytes`, - /// leaving everything else — including their surroundings — byte-for-byte intact. `replayBytes` - /// calls this last, after leading/trailing fragment trimming, so most input is a run of complete - /// sequences — but a mid-buffer fragment left by that trimming is still possible; an incomplete - /// `ESC …` here is passed through byte-by-byte rather than misidentified as a query. - private func strippingQueries(_ bytes: [UInt8]) -> [UInt8] { - var result = [UInt8]() - result.reserveCapacity(bytes.count) - var index = 0 - while index < bytes.count { - guard bytes[index] == 0x1B, index + 1 < bytes.count else { - result.append(bytes[index]) - index += 1 - continue - } - switch bytes[index + 1] { - case 0x5D: // ']' — OSC - guard let end = oscTerminator(in: bytes, from: index + 2) else { - result.append(bytes[index]) - index += 1 - continue - } - if !isQueryOnlyOSC(in: bytes, start: index + 2, end: end) { - result.append(contentsOf: bytes[index.. Range? - { - let sequences = - entering ? Self.alternateScreenEnterSequences : Self.alternateScreenLeaveSequences - var best: Range? - for sequence in sequences { - guard let range = firstRange(of: sequence, in: bytes, from: index) else { continue } - if best == nil || range.lowerBound < best!.lowerBound { - best = range - } - } - return best - } - - private func firstRange(of needle: [UInt8], in bytes: [UInt8], from index: Int) -> Range? { - guard !needle.isEmpty, index >= 0, bytes.count - index >= needle.count else { return nil } - var cursor = index - while cursor + needle.count <= bytes.count { - var matches = true - for offset in 0.. String? { - get { values[key] } - set { - guard let newValue else { - keys.removeAll { $0 == key } - values.removeValue(forKey: key) - return - } - if values[key] == nil { - keys.append(key) - } - values[key] = newValue - } - } - - public var entries: [SessionEnvironmentEntry] { - keys.compactMap { key in - guard let value = values[key] else { return nil } - return SessionEnvironmentEntry(key: key, value: value) - } - } -} - +/// The default login shell, shared by the app and the attach client. +/// +/// Everything else that lived here — `SessionShellInvocation`, `invocation(...)` and the +/// per-shell integration wiring — was deleted with the daemon: its sole caller was +/// `SessionPTY.spawn`, and the only process that still spawns a session shell is the Rust agent, +/// whose `wr_agent::shell::invocation` is the live twin. public enum SessionShellIntegration { public static let defaultShell = "/bin/zsh" - private static let posixShell = "/bin/sh" - private static let xdgFallbackDataDirectories = "/usr/local/share:/usr/share" - - public static func invocation( - command: String, - shell: String, - resourcesDirectory: String, - environment: [SessionEnvironmentEntry] - ) -> SessionShellInvocation { - let resolvedShell = shell.isEmpty ? defaultShell : shell - let name = shellName(resolvedShell) - var environment = SessionEnvironment(environment) - var arguments = ["-" + name] - - if !resourcesDirectory.isEmpty { - environment["GHOSTTY_RESOURCES_DIR"] = resourcesDirectory - let root = resourcesDirectory + "/shell-integration" - switch name { - case "zsh": - applyZshIntegration(root: root, environment: &environment) - case "bash": - applyBashIntegration(root: root, environment: &environment) - if command.isEmpty { - arguments.append("--posix") - } - case "fish", "elvish", "nu": - applyXDGIntegration(root: root, environment: &environment) - default: - break - } - } - - guard command.isEmpty else { - return SessionShellInvocation( - executable: posixShell, - arguments: [posixShell, "-c", "exec " + command], - environment: environment.entries) - } - - return SessionShellInvocation( - executable: resolvedShell, - arguments: arguments, - environment: environment.entries) - } - - public static func shellName(_ shell: String) -> String { - guard let separator = shell.lastIndex(of: "/") else { return shell } - return String(shell[shell.index(after: separator)...]) - } - - private static func applyZshIntegration(root: String, environment: inout SessionEnvironment) { - if let existing = environment["ZDOTDIR"] { - environment["GHOSTTY_ZSH_ZDOTDIR"] = existing - } - environment["ZDOTDIR"] = root + "/zsh" - } - - private static func applyBashIntegration(root: String, environment: inout SessionEnvironment) { - if let existing = environment["ENV"] { - environment["GHOSTTY_BASH_ENV"] = existing - } - environment["ENV"] = root + "/bash/ghostty.bash" - environment["GHOSTTY_BASH_INJECT"] = "1" - if environment["HISTFILE"] == nil, let home = environment["HOME"], !home.isEmpty { - environment["HISTFILE"] = home + "/.bash_history" - environment["GHOSTTY_BASH_UNEXPORT_HISTFILE"] = "1" - } - } - - private static func applyXDGIntegration(root: String, environment: inout SessionEnvironment) { - environment["GHOSTTY_SHELL_INTEGRATION_XDG_DIR"] = root - guard let existing = environment["XDG_DATA_DIRS"], !existing.isEmpty else { - environment["XDG_DATA_DIRS"] = root + ":" + xdgFallbackDataDirectories - return - } - guard !existing.split(separator: ":").contains(where: { $0 == root }) else { return } - environment["XDG_DATA_DIRS"] = root + ":" + existing - } } diff --git a/vcs/crates/wr-agent/src/main.rs b/vcs/crates/wr-agent/src/main.rs index f518b3999..175b92192 100644 --- a/vcs/crates/wr-agent/src/main.rs +++ b/vcs/crates/wr-agent/src/main.rs @@ -130,7 +130,151 @@ fn run_serve(socket: PathBuf, idle: Option) -> ExitCode { /// Exit code the app already knows: `SessionAttachClient.daemonUnavailable`. const DAEMON_UNAVAILABLE: u8 = 92; +/// Become an ordinary shell, because this relay could not deliver a persistent session. +/// +/// **Why the relay and not the app.** The app decides whether a pane gets a session before it +/// forks anything: `applyPersistentSession` either hands libghostty `wr-agent attach` as its +/// `config.command` or lets it open a plain shell, and by the time this process exists that choice +/// is spent. `config.wait_after_command` is false, so a relay that exits leaves a dead pane — no +/// shell, no prompt, nothing the user can type into. The only place left that can still turn a +/// failed attach into a working terminal is this process, by replacing itself with the shell the +/// pane would have run anyway. +/// +/// The app's own health check cannot prevent this. `SessionBackendProbe` runs `wr-agent protocol` +/// and parses a version; an agent that answers that and then fails to serve — a stale socket it +/// cannot bind, a version it cannot negotiate, a crash between the two — passes the probe and dies +/// here. "A broken agent still leaves you a working terminal" is only true if this function exists. +/// +/// **Persistence is lost, and that is said out loud.** The notice goes to stderr, which is the pane, +/// because a terminal that silently stopped surviving quit is worse than one that says so: the user +/// would find out by losing work. It is one line, printed once. +/// +/// Never returns on success — `exec` replaces this process, so the shell inherits the pty, the +/// window title, the exit status, everything. Returns only when `exec` itself fails. +fn fall_back_to_shell(request: &serve::AttachRequest, reason: &str) -> ExitCode { + use std::os::unix::process::CommandExt; + + let text = |value: &Option| { + value + .as_ref() + .map(|v| v.to_string_lossy().into_owned()) + .unwrap_or_default() + }; + + // The SAME invocation the agent would have exec'd in the pty, not a hand-rolled one. + // + // The first version of this ran `$SHELL -l`, which is wrong twice over. `-l` is not what makes + // a login shell — `argv[0]` prefixed with `-` is, and there is no flag for it (see shell.rs) — + // and it skipped ghostty's shell integration, so the pane lost OSC 133 and OSC 7: a title the + // user's prompt sets would latch and never clear, and the footer would never learn the working + // directory. `shell::invocation` knows all of that, per shell, and routes a run command through + // `/bin/sh -c "exec …"` rather than through a `$SHELL` that may not be POSIX at all. + let mut environment: Vec<(std::ffi::OsString, std::ffi::OsString)> = request + .env + .iter() + // Nothing downstream should believe it is inside a session that does not exist. + .filter(|(key, _)| !key.to_string_lossy().starts_with("WORKROOM_SESSION_")) + .cloned() + .collect(); + // A marker the app, a shell prompt, or a bug report can see. The notice below is one line into + // a login shell whose own startup may clear the screen (instant prompts, a `clear` in + // `.zprofile`), so it cannot be the only signal that persistence was lost. + environment.push(( + std::ffi::OsString::from("WORKROOM_SESSION_FALLBACK"), + std::ffi::OsString::from("1"), + )); + + // A shell with no slash would be PATH-searched by `exec` (which ends in `execvp`), while the + // agent's own pty spawn uses `execve` and does not search (`pty.rs`). Two paths documented as + // running "the SAME invocation" must not disagree about what a shell path means, so a + // slash-less value is treated as unusable rather than resolved differently here. + let requested_shell = text(&request.shell); + let shell = if requested_shell.contains('/') { + requested_shell + } else { + wr_agent::shell::DEFAULT_SHELL.to_string() + }; + + let invocation = wr_agent::shell::invocation( + &text(&request.command), + &shell, + &text(&request.resources), + &environment, + ); + + eprintln!("wr-agent: {reason}; this terminal will not survive quitting Workroom\r"); + + let mut command = std::process::Command::new(&invocation.program); + if let Some((argv0, rest)) = invocation.arguments.split_first() { + command.arg0(argv0); + command.args(rest); + } + command.env_clear(); + command.envs(invocation.environment.iter().map(|(k, v)| (k, v))); + // `chdir` happens INSIDE the exec, so a directory it cannot enter aborts the whole thing — + // producing the dead pane this function exists to prevent, and blaming the shell for it. A + // workroom's directory being deleted while a tab is open is a supported operation (`reap`), so + // this is reachable. + // + // **Tried in turn rather than pre-checked**, because the two are not the same test. An earlier + // version picked the first entry passing `is_dir()` and committed to it — but a directory can + // exist, pass `is_dir()`, and still refuse `chdir` for want of the execute bit, and then the + // exec fails with no second chance. `exec` only returns on failure, so the loop below IS the + // fallback chain the agent's own child uses (`pty.rs`: requested, then `$HOME`, then `/`). + for directory in candidate_directories(request.cwd.as_ref()) { + command.current_dir(&directory); + let error = command.exec(); + // Reached only when exec failed. If the working directory is why, the next candidate may + // work; if it is the shell itself, every candidate fails the same way and the loop ends. + if error.kind() != std::io::ErrorKind::NotFound + && error.kind() != std::io::ErrorKind::PermissionDenied + { + eprintln!( + "wr-agent: could not start {}: {error}\r", + invocation.program.to_string_lossy() + ); + return ExitCode::from(DAEMON_UNAVAILABLE); + } + } + eprintln!( + "wr-agent: could not start {} in any working directory\r", + invocation.program.to_string_lossy() + ); + ExitCode::from(DAEMON_UNAVAILABLE) +} + +/// The pane's directory, then the fallbacks, in the order they should be attempted. +/// +/// Empty entries are dropped; existence is NOT checked here, because `chdir` is the only authority +/// on whether a directory can be entered and it runs inside `exec`. +fn candidate_directories(requested: Option<&std::ffi::OsString>) -> Vec { + [ + requested.cloned().map(PathBuf::from), + std::env::var_os("HOME").map(PathBuf::from), + Some(PathBuf::from("/")), + ] + .into_iter() + .flatten() + .filter(|path| !path.as_os_str().is_empty()) + .collect() +} + fn run_attach(args: &[String]) -> ExitCode { + // **Only a relay falls back**, and this is read BEFORE anything can write to it. `attach` is + // also a documented subcommand someone can type, and answering a typo by silently opening a + // nested login shell inside their current one is absurd — the same reasoning `run_list` uses. + // The app always exports the session variables, so their total absence is the tell. + // + // Order is the whole correctness argument here. The `--session` flag below writes + // `WORKROOM_SESSION_ID` into this process's own environment, so reading the flag first let a + // hand-typed `attach --session ` forge the very evidence that classifies it as + // app-invoked — and the only regression test ran bare `attach` with no flags, so it could not + // see that. `WORKROOM_SESSION_CWD` is included because it is exported by + // `PersistentSessionService.launchEnvironment` and has no flag that can fake it. + let invoked_by_the_app = std::env::var_os("WORKROOM_SESSION_SOCKET").is_some() + || std::env::var_os("WORKROOM_SESSION_ID").is_some() + || std::env::var_os("WORKROOM_SESSION_CWD").is_some(); + // Flags win, environment is the fallback — and the environment alone has to be enough, because // `PersistentSessionService.attachCommand()` builds the command line as ` attach` with // no arguments at all. Everything the app wants to say, it says through the variables it @@ -139,18 +283,30 @@ fn run_attach(args: &[String]) -> ExitCode { // Safety: set before any thread is spawned, and only so the shared parser can read it. unsafe { std::env::set_var("WORKROOM_SESSION_ID", text) }; } + // Parsed BEFORE the socket is resolved, so a misconfigured invocation can still fall back to a + // shell: `fall_back_to_shell` reads the shell, cwd and resources the app exported, and those + // arrive in the same environment. + let mut request = serve::AttachRequest::from_env(); + + let give_up = |reason: &str| -> ExitCode { + if invoked_by_the_app { + fall_back_to_shell(&request, reason) + } else { + eprintln!("error: attach needs --socket and --session , or the"); + eprintln!(" WORKROOM_SESSION_* environment the app exports ({reason})"); + ExitCode::FAILURE + } + }; + let socket = flag(args, "--socket") .map(PathBuf::from) .or_else(serve::socket_from_env); let Some(socket) = socket else { - eprintln!("error: attach needs --socket or WORKROOM_SESSION_SOCKET"); - return ExitCode::FAILURE; + return give_up("no session socket was given"); }; - let mut request = serve::AttachRequest::from_env(); let Some(session) = request.id else { - eprintln!("error: attach needs --session or WORKROOM_SESSION_ID"); - return ExitCode::FAILURE; + return give_up("no session id was given"); }; // The pty's initial size comes from the terminal this relay was forked into, so a session is // created at the size it will actually be shown at rather than at 80x24 and then resized — @@ -159,14 +315,19 @@ fn run_attach(args: &[String]) -> ExitCode { request.columns = columns; request.rows = rows; - // Spawn-on-connect-failure, exactly as the Swift attach client does: the agent is started by - // whoever needs it first rather than by an installed service, so there is no install footprint. + // Spawn-on-connect-failure: the agent is started by whoever needs it first rather than by an + // installed service, so there is no install footprint. (The Swift attach client used to do the + // same; it no longer can — its `daemon` subcommand went with the daemon.) + // + // Every failure from here to the attach reply becomes a plain shell rather than a dead pane — + // see `fall_back_to_shell`. These are precisely the states an agent that PASSED the app's + // `wr-agent protocol` probe can still reach. let mut stream = match serve::connect(&socket) { Some(stream) => stream, None => { let binary = std::env::current_exe().unwrap_or_else(|_| PathBuf::from("wr-agent")); if serve::spawn_agent(&binary, &socket).is_err() { - return ExitCode::from(DAEMON_UNAVAILABLE); + return fall_back_to_shell(&request, "could not start the session agent"); } let deadline = std::time::Instant::now() + Duration::from_secs(5); loop { @@ -174,7 +335,10 @@ fn run_attach(args: &[String]) -> ExitCode { break stream; } if std::time::Instant::now() >= deadline { - return ExitCode::from(DAEMON_UNAVAILABLE); + return fall_back_to_shell( + &request, + "the session agent did not start within 5s", + ); } std::thread::sleep(Duration::from_millis(20)); } @@ -182,7 +346,10 @@ fn run_attach(args: &[String]) -> ExitCode { }; if handshake(&mut stream).is_err() { - return ExitCode::from(DAEMON_UNAVAILABLE); + return fall_back_to_shell( + &request, + "could not agree a protocol with the session agent", + ); } let attach = Frame::new(FrameKind::Attach, request.encode()); @@ -191,7 +358,10 @@ fn run_attach(args: &[String]) -> ExitCode { .write_all(&Envelope::new(Service::Terminal, 1, attach.encode()).encode()) .is_err() { - return ExitCode::from(DAEMON_UNAVAILABLE); + return fall_back_to_shell( + &request, + "the session agent closed before accepting the attach", + ); } // Before anything reads stdin: a relay that leaves its own tty cooked is not a relay. Held to @@ -247,6 +417,24 @@ fn run_attach(args: &[String]) -> ExitCode { // `ExitCode` is a byte; a shell status is already 0-255. return ExitCode::from(code.clamp(0, 255) as u8); } + // **Deliberately NOT a fallback**, and an earlier version of this made + // it one. That was wrong in the worst available direction. + // + // A `Failure` here does not mean the agent is broken — it is talking, + // and the session it is talking about is usually ALIVE. `RepaintFailed` + // (session.rs) says so outright: "the client could not take its repaint + // whole, so it was not attached at all… can retry from a clean + // terminal", and it only fires on reattach to a session that already + // has scrollback — precisely the pane with the user's work in it. A + // fatal pty read reports the same kind for an already-attached client. + // + // Exec'ing a shell on either is unrecoverable (exec is one-way, so the + // retry the agent is inviting can never happen) and, worse, it hands + // back a healthy-looking prompt for a session that is still running. + // That is the exact failure `confirmBeforeAttach` exists to prevent on + // the app side: the one that looks like success. A visible error and a + // non-zero exit is the honest answer, and it leaves the session where + // the user can still reach it from the detached-sessions list. FrameKind::Failure => { eprintln!("wr-agent: {}", String::from_utf8_lossy(&frame.payload)); return ExitCode::FAILURE; @@ -353,6 +541,9 @@ impl RawMode { } impl Drop for RawMode { + /// Was briefly split into a separate `restore()` so an `exec` could call it without a + /// destructor. Nothing execs from inside the relay loop any more — see the `FrameKind::Failure` + /// arm — so the split was generality for an unreachable state and the two lines live here again. fn drop(&mut self) { if let Some(original) = self.0.as_ref() { unsafe { libc::tcsetattr(libc::STDIN_FILENO, libc::TCSANOW, original) }; @@ -389,6 +580,9 @@ fn run_list(args: &[String]) -> ExitCode { eprintln!("error: no agent listening on {}", socket.display()); return ExitCode::from(DAEMON_UNAVAILABLE); }; + // No fallback here, deliberately: `list` is a diagnostic command run by a person at a prompt, + // not a relay libghostty forked into a pane. Exec'ing a shell would be absurd; an exit code is + // exactly what the caller wants. if handshake(&mut stream).is_err() { return ExitCode::from(DAEMON_UNAVAILABLE); } diff --git a/vcs/crates/wr-agent/src/serve.rs b/vcs/crates/wr-agent/src/serve.rs index d05b8abcf..b4eedc5f9 100644 --- a/vcs/crates/wr-agent/src/serve.rs +++ b/vcs/crates/wr-agent/src/serve.rs @@ -284,6 +284,21 @@ fn dispatch( // Not `` with no arguments: see `shell::invocation`. Spawning the shell // bare cost the login profile and ghostty's shell integration, which showed up in // the app as panes stuck on the title the user's own prompt sets. + // **The session's own variables do not reach the session's shell**, matching what + // the Swift attach client has always done (`SessionAttachClient.makeRequest` + // filters the same prefix before sending). Leaking them had two consequences, both + // real: `wr-agent attach` typed inside a pane picked up THAT pane's id and socket + // from its own environment and attached a second relay to the session it was + // running in, with the pty fanning output to both; and `run_attach`'s "was I + // invoked by the app" test — which reads exactly these variables — was true for + // anything the user typed in a pane, so the usage-instead-of-nested-shell guard + // could never fire there. + let child_environment: Vec<(OsString, OsString)> = request + .env + .iter() + .filter(|(key, _)| !key.to_string_lossy().starts_with("WORKROOM_SESSION_")) + .cloned() + .collect(); let invocation = shell::invocation( &request .command @@ -296,7 +311,7 @@ fn dispatch( .clone() .unwrap_or_default() .to_string_lossy(), - &request.env, + &child_environment, ); sessions .create(SessionSpec { diff --git a/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs b/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs new file mode 100644 index 000000000..45227d468 --- /dev/null +++ b/vcs/crates/wr-agent/tests/attach_falls_back_to_a_shell.rs @@ -0,0 +1,703 @@ +//! An agent that passes the app's health probe and then fails to serve must still leave the user a +//! working terminal. +//! +//! **Why this is not covered by the probe.** `SessionBackendProbe` (macapp) runs `wr-agent protocol` +//! and parses a version string. That answers "can this binary start and print", not "can it serve a +//! session": a stale socket it cannot bind, a peer it cannot negotiate with, a crash between the +//! two — all pass the probe and fail here. And by the time `wr-agent attach` runs, the app has +//! already chosen; `applyPersistentSession` set libghostty's `config.command`, and +//! `config.wait_after_command` is false, so a relay that exits leaves a pane with no shell in it at +//! all. Nothing the app can do at that point recovers it, which is why the relay execs a shell. +//! +//! The claim under test is the one the eng review said was not established: *if the agent breaks, +//! you still get a working terminal.* + +use std::io::Read; +use std::os::unix::net::UnixListener; +use std::path::PathBuf; +use std::process::{Command, Stdio}; + +fn agent_binary() -> PathBuf { + match std::env::var_os("WR_AGENT_BIN") { + Some(path) => PathBuf::from(path), + None => PathBuf::from(env!("CARGO_BIN_EXE_wr-agent")), + } +} + +/// A short directory: a unix socket address is capped at 104 bytes and cargo's target dir is long. +fn scratch(name: &str) -> PathBuf { + let dir = PathBuf::from(format!("/tmp/wr-fb-{}-{}", name, std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("scratch dir"); + dir +} + +/// Runs `wr-agent attach` with the app's environment contract and returns (stdout+stderr, code). +fn attach(env: &[(&str, &str)]) -> (String, Option) { + attach_inner(env, None) +} + +/// Same, but feeding the exec'd shell a script on stdin — the only way to observe the INTERACTIVE +/// branch, which takes no command argument by definition. +fn attach_with_stdin(env: &[(&str, &str)], stdin: &str) -> (String, Option) { + attach_inner(env, Some(stdin)) +} + +fn attach_inner(env: &[(&str, &str)], stdin: Option<&str>) -> (String, Option) { + let mut command = Command::new(agent_binary()); + command.arg("attach"); + command.env_clear(); + command.env("PATH", "/usr/bin:/bin"); + for (key, value) in env { + command.env(key, value); + } + command.stdin(if stdin.is_some() { + Stdio::piped() + } else { + Stdio::null() + }); + command.stdout(Stdio::piped()); + command.stderr(Stdio::piped()); + let mut child = command.spawn().expect("spawn attach"); + if let Some(script) = stdin { + use std::io::Write; + let mut handle = child.stdin.take().expect("stdin"); + let _ = handle.write_all(script.as_bytes()); + // Closing it is what ends the interactive shell; without this the test hangs. + drop(handle); + } + + let mut out = String::new(); + let mut err = String::new(); + child + .stdout + .take() + .expect("stdout") + .read_to_string(&mut out) + .expect("read stdout"); + child + .stderr + .take() + .expect("stderr") + .read_to_string(&mut err) + .expect("read stderr"); + let status = child.wait().expect("wait"); + (format!("{out}{err}"), status.code()) +} + +/// The realistic shape of "passed the probe, failed at attach": something IS listening on the +/// socket — so the client connects, exactly as it would to a healthy agent — and then the peer goes +/// away without negotiating. Before the fallback this exited 92 and the pane was dead. +#[test] +fn a_peer_that_cannot_negotiate_still_leaves_a_working_shell() { + let dir = scratch("negotiate"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + + // Accept one connection and drop it immediately: connect succeeds, the handshake cannot. + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + // A command, so this test has something to observe. The two branches are NOT identical — + // an earlier comment here claimed they were — so the interactive one, which is the only + // one the app can actually reach, is covered separately by + // `the_interactive_fallback_is_a_real_login_shell`. + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("FELL-BACK-TO-SHELL"), + "attach did not become a shell after the peer refused to negotiate; the pane would be \ + dead. got: {output:?}" + ); + assert_eq!( + code, + Some(0), + "the shell's own status must be the process's, so the pane behaves like a normal terminal" + ); + assert!( + output.contains("will not survive quitting"), + "the user must be told persistence was lost, or they find out by losing work. got: \ + {output:?}" + ); +} + +/// Nothing listening, and the spawned agent cannot start there. +/// +/// The guard below is load-bearing and was missing. The reason the agent fails is NOT "the +/// directory does not exist" — `serve.rs` does `create_dir_all(parent)` before binding. It fails +/// because `acquire_instance_lock` opens `.lock` first and that ENOENTs. Move the lock +/// below the mkdir, a plausible refactor, and a REAL agent binds and runs the command, so every +/// assertion below passes without the fallback ever executing. The notice is what proves which +/// path ran. +#[test] +fn an_agent_that_cannot_be_started_still_leaves_a_working_shell() { + let dir = scratch("nostart"); + let socket = dir.join("missing").join("a.sock"); + + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains("FELL-BACK-TO-SHELL"), + "attach did not become a shell when no agent could be started. got: {output:?}" + ); + assert_eq!(code, Some(0)); +} + +/// A malformed invocation is also a dead pane without the fallback. Note this needs the app to have +/// invoked us — `WORKROOM_SESSION_SOCKET` is set here — because a hand-run `attach` reports usage +/// instead; see `a_hand_run_attach_reports_usage_instead_of_opening_a_shell`. +#[test] +fn a_missing_session_id_still_leaves_a_working_shell() { + let dir = scratch("nosession"); + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_SOCKET", + dir.join("a.sock").to_str().unwrap(), + ), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains("FELL-BACK-TO-SHELL"), + "attach with no session id must open a shell, not exit. got: {output:?}" + ); + assert_eq!(code, Some(0)); +} + +/// The fallback runs in the directory the pane asked for. A shell that opens in the wrong place is +/// a visibly broken terminal even though it is a working one. +/// Same trap as the test below it: an absent socket in an existing directory spawns a REAL agent, +/// which also runs `pwd` in the right place, so this passed without the fallback ever running. +#[test] +fn the_fallback_shell_starts_in_the_requested_directory() { + let dir = scratch("cwd"); + let canonical = std::fs::canonicalize(&dir).expect("canonicalize"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, _) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", canonical.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "pwd"), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains(canonical.to_str().unwrap()), + "the fallback shell did not start in the pane's directory. got: {output:?}" + ); +} + +/// The session variables are stripped before exec, so nothing downstream — a shell hook, a nested +/// `wr-agent`, the app's own shell integration — believes it is inside a session that does not +/// exist. +/// +/// Must use the non-negotiating peer, not merely an absent socket. An absent socket in a directory +/// that EXISTS makes the client spawn a real agent, which serves the session perfectly well — the +/// first draft of this test did that, saw the variables, and blamed the fallback for a path it had +/// never reached. The fallback has to actually fire for this assertion to mean anything. +#[test] +fn the_fallback_shell_does_not_inherit_the_session_variables() { + let dir = scratch("env"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, _) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo id=[${WORKROOM_SESSION_ID:-unset}] sock=[${WORKROOM_SESSION_SOCKET:-unset}]", + ), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains("id=[unset]") && output.contains("sock=[unset]"), + "the fallback shell inherited session variables for a session it is not in. got: \ + {output:?}" + ); +} + +/// **The branch production actually takes.** Every other test here sets +/// `WORKROOM_SESSION_COMMAND`, because a command gives the test something to observe — but +/// `PersistentSessionService` hardcodes that variable to `""`, and a pane with a run command never +/// gets a persistent session at all. So the interactive branch is the ONLY one the app can reach, +/// and it was the one branch with no coverage. The two differ by more than an argument now: +/// `shell::invocation` gives the interactive case a `-`-prefixed `argv[0]` and the shell-integration +/// environment, and the command case a POSIX `/bin/sh -c "exec …"`. +/// +/// Asserted through `argv[0]`, which is what makes a login shell — there is no flag for it. +#[test] +fn the_interactive_fallback_is_a_real_login_shell() { + let dir = scratch("login"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + // `-c` on the exec'd shell would take the command branch, so the login shell is driven the way + // a pane drives it — over stdin — and asked to report its own argv[0]. + let (output, _) = attach_with_stdin( + &[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + // Empty, exactly as PersistentSessionService.launchEnvironment sets it. + ("WORKROOM_SESSION_COMMAND", ""), + ], + "printf 'argv0=[%s]\\n' \"$0\"; printf 'fallback=[%s]\\n' \"${WORKROOM_SESSION_FALLBACK:-unset}\"\n", + ); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains("argv0=[-sh]"), + "the fallback shell is not a LOGIN shell: argv[0] must be the shell's name prefixed with \ + a dash, which is the only thing that makes it one. got: {output:?}" + ); + assert!( + output.contains("fallback=[1]"), + "the exec'd shell must carry WORKROOM_SESSION_FALLBACK so the loss of persistence is \ + visible to more than one scrollable line. got: {output:?}" + ); +} + +/// A session variable must not survive into the fallback shell, and the marker must. +#[test] +fn the_fallback_shell_reports_no_session_but_does_report_the_fallback() { + let dir = scratch("marker"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, _) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo id=[${WORKROOM_SESSION_ID:-unset}] fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("id=[unset]") && output.contains("fb=[1]"), + "expected the session id stripped and the fallback marker set. got: {output:?}" + ); +} + +/// A pane whose directory has been deleted must still get a shell. +/// +/// `chdir` happens INSIDE `exec`, so a missing directory aborts it — and the relay then reported +/// "could not start /bin/zsh", blaming the shell, and exited 92: the dead pane the fallback exists +/// to prevent, in the one case where the user most needs it. Deleting a workroom while a tab is +/// open is a supported operation. +#[test] +fn a_deleted_working_directory_still_leaves_a_working_shell() { + let dir = scratch("gonecwd"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ( + "WORKROOM_SESSION_CWD", + "/tmp/wr-this-directory-does-not-exist", + ), + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("FELL-BACK-TO-SHELL"), + "a deleted working directory aborted the exec and produced a dead pane. got: {output:?}" + ); + assert_eq!(code, Some(0)); +} + +/// Typed at a prompt rather than forked into a pane, `attach` must report usage — not silently +/// open a nested login shell inside the user's current one. Same reasoning `list` uses. +#[test] +fn a_hand_run_attach_reports_usage_instead_of_opening_a_shell() { + let mut command = Command::new(agent_binary()); + command.arg("attach"); + command.env_clear(); + command.env("PATH", "/usr/bin:/bin"); + command.env("SHELL", "/bin/sh"); + command.stdin(Stdio::null()); + command.stdout(Stdio::piped()); + command.stderr(Stdio::piped()); + let output = command.output().expect("run attach"); + + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + combined.contains("attach needs"), + "a hand-run attach must explain itself. got: {combined:?}" + ); + assert!( + !combined.contains("will not survive quitting"), + "a hand-run attach must not open a fallback shell. got: {combined:?}" + ); + assert_ne!(output.status.code(), Some(0)); +} + +/// REGRESSION. A hand-typed `attach --session ` must ALSO report usage, not open a shell. +/// +/// The classification reads `WORKROOM_SESSION_ID` from the environment, and the `--session` flag +/// WRITES that variable into this process's own environment. Reading the flag first therefore let a +/// hand-typed invocation forge the evidence that classified it as app-invoked, and the only test +/// covering the classifier ran bare `attach` with no flags — so it could not see it. The fix is +/// ordering: the environment is read before any flag can write to it. +#[test] +fn a_hand_run_attach_with_a_session_flag_still_reports_usage() { + let dir = scratch("handsession"); + let mut command = Command::new(agent_binary()); + command.arg("attach"); + // `--session` ONLY, deliberately. With a socket as well this never reaches the classifier: it + // spawns a real agent, attaches, and sits on an interactive shell forever — and because the + // agent is setsid'd while inheriting the test's stdout pipe, `output()` then waits on an EOF + // that never comes. The classifier is reached exactly when something is missing, which is also + // the state a typo produces. + command + .arg("--session") + .arg("6B9B968D-0BD7-4172-850A-A373DA73BC70"); + command.env_clear(); + command.env("PATH", "/usr/bin:/bin"); + command.env("SHELL", "/bin/sh"); + command.stdin(Stdio::null()); + command.stdout(Stdio::piped()); + command.stderr(Stdio::piped()); + let output = command.output().expect("run attach"); + let _ = std::fs::remove_dir_all(&dir); + + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + !combined.contains("will not survive quitting"), + "a flag the user typed classified them as the app and opened a nested login shell inside \ + their own terminal. got: {combined:?}" + ); + assert!( + combined.contains("attach needs"), + "a hand-run attach must explain itself. got: {combined:?}" + ); + assert_ne!(output.status.code(), Some(0)); +} + +/// Negative control. Without this, every assertion above would pass just as well against a relay +/// that ALWAYS execs a shell and never attaches to anything — which would be a total regression +/// wearing the fallback's clothes. +/// +/// The discriminator is the session variable, not the notice string. Asserting only on the absence +/// of "will not survive quitting" leaned on a literal from the same commit; `WORKROOM_SESSION_ID` +/// is passed THROUGH by a real session and STRIPPED by the fallback, so it separates the two +/// regardless of what any message says. +#[test] +fn a_healthy_agent_is_not_replaced_by_a_shell() { + let dir = scratch("healthy"); + let socket = dir.join("a.sock"); + + let mut agent = Command::new(agent_binary()) + .arg("serve") + .arg("--socket") + .arg(&socket) + .arg("--idle-timeout") + .arg("30") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn agent"); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !socket.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + assert!(socket.exists(), "the agent never bound its socket"); + + let (output, _) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC71", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo RAN-IN-A-SESSION fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ]); + + let _ = agent.kill(); + let _ = agent.wait(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("RAN-IN-A-SESSION"), + "a healthy agent should have run the command in a session. got: {output:?}" + ); + assert!( + output.contains("fb=[unset]"), + "WORKROOM_SESSION_FALLBACK is set ONLY by fall_back_to_shell, so seeing it here means the \ + fallback fired against a HEALTHY agent — and every other assertion in this file would \ + pass against a relay that always execs. got: {output:?}" + ); + assert!( + !output.contains("will not survive quitting"), + "a healthy attach must not print the fallback notice. got: {output:?}" + ); +} + +/// **The line the fallback must NOT cross.** A `Failure` frame means the agent is TALKING, and the +/// session it is talking about is usually alive — `RepaintFailed` (session.rs) fires only on +/// reattach to a session that already has scrollback, which is precisely the pane holding the +/// user's work, and it explicitly invites a retry. Exec'ing a shell there is unrecoverable (exec is +/// one-way, so the retry can never happen) and hands back a healthy-looking prompt for a session +/// that is still running: the same "failure that looks like success" `confirmBeforeAttach` exists +/// to prevent on the app side. +/// +/// An earlier version of `run_attach` made this a fallback, so this is a REGRESSION pin, and it is +/// the only test anywhere that reaches the `FrameKind::Failure` arm. +/// +/// `WORKROOM_SESSION_COMMAND` is both the discriminator and the hang guard: in the regression the +/// exec'd shell runs the echo and exits, so the test fails on its assertions rather than blocking. +#[test] +fn a_failure_frame_is_reported_rather_than_replaced_by_a_shell() { + use std::io::Write; + use wr_agent::protocol::envelope::{Envelope, Hello, Service}; + use wr_agent::protocol::frame::{Frame, FrameKind}; + + let dir = scratch("failure"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + + // A peer that negotiates successfully — so every earlier fallback branch is passed — and then + // reports a failure, exactly as a session refusing a repaint does. + let accepter = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().expect("accept"); + stream + .write_all(&Hello::current("fake-peer").encode()) + .expect("greet"); + let _ = stream.flush(); + let frame = Frame::new(FrameKind::Failure, b"FAKE-REPAINT-FAILED".to_vec()); + stream + .write_all(&Envelope::new(Service::Terminal, 1, frame.encode()).encode()) + .expect("failure frame"); + let _ = stream.flush(); + // Held open: closing here would race the client's read and arrive as an EOF instead, which + // is a different branch and would make this test pass for the wrong reason. + std::thread::sleep(std::time::Duration::from_secs(3)); + }); + + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("FAKE-REPAINT-FAILED"), + "the agent's failure was not shown to the user, so they cannot know to retry. got: \ + {output:?}" + ); + assert!( + !output.contains("FELL-BACK-TO-SHELL"), + "a Failure frame exec'd a shell. The session is still running and now unreachable from \ + this pane, behind a prompt that looks like a successful reattach. got: {output:?}" + ); + assert!( + !output.contains("will not survive quitting"), + "a Failure frame is not a broken agent and must not print the fallback notice. got: \ + {output:?}" + ); + assert_eq!( + code, + Some(1), + "a reported failure must exit non-zero so the pane does not look like a clean exit" + ); +} + +/// The FIRST `give_up` in `run_attach`, which no other test reaches: the id arrives but the socket +/// does not. Its sibling — socket present, id missing — is +/// `a_missing_session_id_still_leaves_a_working_shell`; both must fall back, because either one +/// alone still means the app invoked us as a pane's command. +#[test] +fn a_missing_session_socket_still_leaves_a_working_shell() { + let dir = scratch("nosocket"); + let (output, code) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "echo FELL-BACK-TO-SHELL"), + ]); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("FELL-BACK-TO-SHELL"), + "attach with no session socket must open a shell, not exit. got: {output:?}" + ); + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert_eq!(code, Some(0)); +} + +/// `first_usable_directory`'s SECOND rung. `a_deleted_working_directory_still_leaves_a_working_shell` +/// proves the chain does not abort the exec, but it runs with `HOME` unset, so it cannot tell the +/// home fallback from the `/` one — and landing a user in `/` when their home exists is the kind of +/// quietly-wrong terminal nobody reports. +#[test] +fn a_deleted_working_directory_falls_back_to_home_before_root() { + let dir = scratch("homecwd"); + let home = std::fs::canonicalize(&dir).expect("canonicalize"); + let socket = dir.join("a.sock"); + let listener = UnixListener::bind(&socket).expect("bind fake agent"); + let accepter = std::thread::spawn(move || { + if let Ok((stream, _)) = listener.accept() { + drop(stream); + } + }); + + let (output, _) = attach(&[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC70", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ( + "WORKROOM_SESSION_CWD", + "/tmp/wr-this-directory-does-not-exist", + ), + ("HOME", home.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "pwd"), + ]); + let _ = accepter.join(); + let _ = std::fs::remove_dir_all(&dir); + + assert!( + output.contains("will not survive quitting"), + "the fallback did not fire, so this test is asserting nothing. got: {output:?}" + ); + assert!( + output.contains(home.to_str().unwrap()), + "a pane whose directory was deleted landed somewhere other than the user's home. got: \ + {output:?}" + ); +}