diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index d979de7ae..10b230a1b 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -11,8 +11,9 @@ Mode: Builder **Phase 2 is merged to master** (2026-09-23, #201 through #218; review follow-ups #220–#226), and **Phase 3 has started.** Its milestones are sub-issues of #154: the Linux agent artifact (#227, merged; its first Nightly DMG is still to be checked), the supervised far-side agent with its stdio -relay and container fixture (#228, in progress), the -app-side transport with `HostDriver` and the container driver (#229), `execve` hand-off (#230), +relay and container fixture (#228, merged), the +app-side transport with `HostDriver` and the container driver (#229, in progress: services first, +terminal panes second), `execve` hand-off (#230), push-on-first-connect bootstrap (#231) and stop-and-reboot screen restoration (#232). The rest of this section is the 2026-09-17 status, kept for the Phase 2 detail it records and corrected where it had gone stale. @@ -993,6 +994,69 @@ these are the subsystems that actually gate "a remote workroom is a real workroo not die with it: their jobs run on, orphaned, while the replacement agent reports no sessions. systemd's default `KillMode=control-group` does this. The fixture's loop does not, which is fine for a test box and wrong for a host with real work on it. + **App side, services built (#229):** `HostDriver` (`Core/HostDriver.swift`) has the four methods + and the declared traits, and `ContainerHostDriver` implements `openStream` as + `ssh -F wr-agent relay --socket `. The stream is one end of a + socketpair whose other end is ssh's stdin and stdout, and `AgentVCSConnection.connect(host:stream:)` + runs the same negotiation over it as over a local socket, so VCS, File, Status and Forward need + no change. ssh exiting is the connection ending, through the same generation and + fail-pending-calls rules as a dead local agent. Terminal panes through the driver are the second + half of #229. + **Remote writes (#229)** keep every decision in the Swift writer (`CLIVCSWriter`) and move only + two things to the host: + - *The disk the failure classifier reads.* It decides a parked rebase, a leftover lock or the last + fetch from the repository's git directory, and on a remote host that directory is there. The + agent's `stat` request reports the facts (existence, directory, modification time, a worktree's + `.git` text) for the paths the classifier names, read after the command they explain. + - *The jj working-copy barrier.* Locally the app's gate holds it for a whole operation, and exec + never takes it. A Mac cannot flock a file on another host, so an exec request for a remote jj + repository names its shared root (`barrier_root`) and the agent takes the barrier around that + command. **Known ceiling:** that is per command, where the local gate is per operation, so a + remote snapshot can land between two commands of one write. The upgrade is an + operation-scoped barrier the client acquires and releases, held by the connection and dropped + when it goes. Two kinds of command skip it: + - A read that cannot snapshot (`--ignore-working-copy`), which has nothing to protect. + - A network command (`jj git fetch`/`push`), because the barrier is handed to the child, and + git's detached helpers would inherit it and hold it after jj exits. jj's op log reconciles a + snapshot that runs alongside one. + Both came with exec service version 3, and the app refuses a remote write to an older agent. + **Known residual, wider remotely:** a jj commit tells "committed, then failed" from "failed" by + whether the op head moved, and a snapshot also moves it. A snapshot in the gap between the + before-read and the commit can make a failed commit read as one that landed. Locally the after-read + already sits outside the gate; the per-command barrier widens the gap. The fix is to compare + `@-`, which a snapshot never rewrites, for jj's `.commit` mode. + **Terminal panes (#229).** A pane is a process libghostty spawns on its own pty, which a socketpair + made in the app cannot become, so a pane does not use `openStream`. A driver that can host panes + implements `HostTerminalDriver.attachCommand`. For ssh that runs + `ssh -t 'env TERM=xterm-256color WORKROOM_SESSION_{ID,SOCKET,CWD}=… wr-agent attach + --no-spawn [--no-create]'`, the far side's attach client in the pty ssh allocates there. + - *`--no-spawn`:* with no agent listening, a shell rather than a spawned agent. Only the + supervisor starts one there. + - *`--no-create` (a restored pane):* the agent refuses a session it does not hold (`CREATE=0` in + the attach request, which older agents ignore). The pane becomes a shell that says the session + ended. Locally the app asks first (`confirmBeforeAttach`); remotely one request answers and + attaches, so the answer cannot go stale in between. + - *A dropped link reconnects.* ssh's own failure (exit 255, as after sleep or a network change) + reattaches the pane as a restored one, backing off to 30s while the host stays unreachable, + and stopping after five quick failures in a row so a permanent one (a mismatched host key) + leaves its message on screen. A session that itself exited 255 is reported as 254 by the + remote attach, so it never reads as a dropped link. The pane's ssh has `EscapeChar none`: what + the user types is theirs, and `~.` would otherwise disconnect it. + - *Closing a remote pane does not end its session yet.* Nothing on the service connection can + ask the host's agent to, so the session is left running and reported as not killed. That + belongs with the host's lifecycle (Phase 4). + - *`CREATE=0` is only as good as the agent that reads it.* An agent older than the flag ignores + it and creates the session. The version hand-off (#230) and the bootstrap (#231) keep the + host's agent current; until they land, a host running an older agent can still hand a + restored pane a fresh shell. + - *Degraded until the agent bootstrap (#231):* `TERM` is `xterm-256color`, since a host rarely + has `xterm-ghostty` terminfo, and there is no shell integration there (its resources are in the + Mac's app bundle), so the title and footer do not follow the shell. + - *Not persisted:* a session is marked remote in memory (`registerRemoteSession`). Phase 4's + remote workrooms re-register their panes on relaunch. + - *An SDK-exec driver has no command to hand libghostty.* It needs a local bridge process, which + is a Phase 4 question. + - **Local needs no supervisor at all. DECIDED — and an earlier claim in this document was wrong.** A previous revision asserted that `SessionDaemon.swift:97-107`'s self-exit (`sessions.isEmpty && connections.isEmpty`) had to go, because the agent would also own VCS reads @@ -1023,8 +1087,30 @@ these are the subsystems that actually gate "a remote workroom is a real workroo product uses everywhere, the container fixture exercises the real path rather than a fallback nobody runs. What remains uncovered by CI is boxd's *live-process* derivation specifically — a narrower gap than the whole fork path, and one a nightly boxd job can cover if it earns it. -- **State a host-key policy** (see Provider Decision). Fresh-key-per-fork plus `BatchMode=yes` cannot prompt, so - the driver has to deliver the expected host key out of band or pin the base's. +- **Host-key policy: pinned and delivered out of band. DECIDED (#229).** The driver writes its own + `ssh_config` and passes it with `-F`, so nothing in `~/.ssh/config` or `/etc/ssh/ssh_config` + applies. The host's key is the only known key (`GlobalKnownHostsFile /dev/null`), with + `StrictHostKeyChecking yes` and `BatchMode yes`: a mismatched key fails at once with "Host key + verification failed." and there is no prompt to hang on. A fresh-key-per-fork driver delivers each + instance's key the same way, from its provider's API. +- **Auth resolution for the ssh transport: host-resolved credentials, no forwarding. DECIDED + (#229).** Nothing from the Mac's environment crosses. A remote exec request carries only policy + pins (`GIT_OPTIONAL_LOCKS`, `GIT_TERMINAL_PROMPT`, `LC_ALL`), and the agent builds the rest from + its own environment (`host_environment` in `vcs.rs`), less variables that redirect git to + another repository. That makes the supervisor's environment what every remote git gets, so a + supervisor should start the agent from a clean one. ssh is kept from prompting without setting + `GIT_SSH_COMMAND`, which would outrank a repository's own `core.sshCommand`: no askpass helper, + and a supervised agent has no terminal. ssh itself runs with an empty environment, + `ForwardAgent no` and `ClearAllForwardings yes`. git on the host authenticates with what the host + has, which in production is a per-workroom deploy key minted on the box (Decisions Made; Phase 4). + Agent forwarding stays rejected (premise 6). The container driver also logs in with a key file + and `IdentityAgent none`; whether a real driver may use the Mac's ssh agent to LOG IN, which is + not forwarding it, is that driver's decision. +- **Peer authentication to shell grade, on every driver. DECIDED (#229).** The exec service runs + `git` with arbitrary arguments, which is arbitrary code execution, so a stream that reaches an + agent is a shell whatever it is called. ssh authenticates to that grade. Every later driver (SDK + exec, WebSocket) must show that it does too before it counts as a driver; `HostDriver`'s doc + carries the requirement. - **Agent bootstrap over the transport.** Workroom.app ships the Linux agent binary and pushes it on first connect rather than baking provider images. One source of truth, no image maintenance, and it works for SDK-transport providers with no image concept. (This is the one piece of Coder's and diff --git a/macapp/WorkroomApp/Core/AgentCommandRunner.swift b/macapp/WorkroomApp/Core/AgentCommandRunner.swift index 645154730..ced392544 100644 --- a/macapp/WorkroomApp/Core/AgentCommandRunner.swift +++ b/macapp/WorkroomApp/Core/AgentCommandRunner.swift @@ -10,6 +10,10 @@ import Foundation /// distinction is load-bearing — see `neverRan` and `outcomeUnknown`. struct AgentCommandRunner: StatusCommandRunning, Sendable { let connection: AgentVCSConnection + /// The shared root of a jj repository on a REMOTE host, whose working-copy barrier the agent + /// takes around each command (`barrier_root` in `vcs.rs`). Nil locally, where the caller's gate + /// holds it for the whole operation and the agent must never take it again. + var barrierRoot: String? func run(_ executable: String, _ args: [String], in directory: String, timeout: TimeInterval) async -> CommandResult @@ -55,11 +59,9 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { } payload = encoded } - let request = AgentExecRequest( - executable: executable, args: args, dir: directory, - timeoutMs: Int((timeout * 1000).rounded(.up)), - stdin: payload, - env: StatusCommandRunner.childEnvironment(network: network)) + let request = Self.request( + executable, args, in: directory, timeout: timeout, stdin: payload, network: network, + host: connection.host, barrierRoot: barrierRoot) let reply: Data do { // Slack above the command's own timeout: the round trip and the agent's own bookkeeping @@ -70,7 +72,12 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { // once it holds the flock — see the comment there. Shielding HERE instead would also cover // `remoteState`'s ungated reads, and a superseded `RemoteStateModel` refresh would then squat // one of this connection's 32 shared slots until the agent answered. - reply = try await connection.request(request, timeout: timeout + 15) + // + // Plus the barrier's own wait where the agent takes one (a remote jj command): it can wait + // up to 30s for the barrier before the command starts (`SnapshotLock::acquire`), and a + // deadline that did not cover that would give up on a command still running. + let barrierWait: TimeInterval = request.barrierRoot == nil ? 0 : 30 + reply = try await connection.request(request, timeout: timeout + 15 + barrierWait) } catch let error as VCSError { // Raised before anything left this process — today only the 1 MiB single-envelope request // ceiling. Nothing ran, and the workroom is fine. @@ -163,6 +170,38 @@ struct AgentCommandRunner: StatusCommandRunning, Sendable { } } +extension AgentCommandRunner { + /// The exec request for `host`. **Nothing from the Mac's environment goes to a remote host.** + /// + /// Locally the request carries the app's whole child environment, which the agent adopts in place + /// of its own stale one (see `StatusCommandRunner.childEnvironment`). A remote host is a different + /// machine: the Mac's `HOME` and `PATH` name nothing there, and its `SSH_AUTH_SOCK`, git identity + /// and credential variables are the user's Mac credentials, which must not cross (design doc, + /// premise 6: agent forwarding was rejected, and git on a remote host authenticates with what the + /// host has). So a remote request sends only `remoteEnvironment`'s policy pins and asks the agent + /// to build the rest from the host's own environment (`host_environment` in `vcs.rs`), which also + /// pins ssh to fail rather than prompt, as `networkEnvironment` does here. + static func request( + _ executable: String, _ args: [String], in directory: String, timeout: TimeInterval, + stdin: String?, network: Bool, host: HostID, barrierRoot: String? = nil + ) -> AgentExecRequest { + let remote = host != .local + return AgentExecRequest( + executable: executable, args: args, dir: directory, + timeoutMs: Int((timeout * 1000).rounded(.up)), + stdin: stdin, + env: remote + ? StatusCommandRunner.remoteEnvironment + : StatusCommandRunner.childEnvironment(network: network), + hostEnvironment: remote ? true : nil, + // Not for a network command (`jj git fetch`/`push`): the barrier is handed to the child, and + // git's detached helpers (`git maintenance --auto`, a credential cache daemon) would inherit + // it and hold it long after jj exits. jj's own op log reconciles a snapshot that runs + // alongside one of those. + barrierRoot: remote && !network ? barrierRoot : nil) + } +} + struct AgentExecRequest: Encodable, Sendable { var version = 1 var kind = "exec" @@ -172,6 +211,11 @@ struct AgentExecRequest: Encodable, Sendable { let timeoutMs: Int var stdin: String? var env: [String: String] + /// Set only for a remote host, and omitted otherwise: an agent that predates the field rejects + /// any request carrying it (`deny_unknown_fields`), and a local agent may be one. + var hostEnvironment: Bool? + /// Remote only, for the same reason: see `AgentCommandRunner.barrierRoot`. + var barrierRoot: String? } struct AgentExecResult: Decodable, Sendable { diff --git a/macapp/WorkroomApp/Core/AgentPortForwarding.swift b/macapp/WorkroomApp/Core/AgentPortForwarding.swift index 5fd59cc93..349a7799f 100644 --- a/macapp/WorkroomApp/Core/AgentPortForwarding.swift +++ b/macapp/WorkroomApp/Core/AgentPortForwarding.swift @@ -278,7 +278,17 @@ final class PortForward: @unchecked Sendable { // blocking `read` is the whole design. _ = fcntl(client, F_SETFL, fcntl(client, F_GETFL) & ~O_NONBLOCK) var enabled: Int32 = 1 - setsockopt(client, SOL_SOCKET, SO_NOSIGPIPE, &enabled, socklen_t(MemoryLayout.size)) + // Checked, because it can only be set here, after `accept`: a client that has already reset + // leaves the socket shut down, `setsockopt` fails with EINVAL, and the pump's first write would + // then raise SIGPIPE and kill the app. Such a client is gone, so there is nothing to forward. + // The slot is released as it goes out of scope. + guard + setsockopt(client, SOL_SOCKET, SO_NOSIGPIPE, &enabled, socklen_t(MemoryLayout.size)) + == 0 + else { + Darwin.close(client) + return + } // What a forward carries is small writes in both directions (chunked HTTP, HMR frames, a wire // protocol); Nagle plus delayed ACK on a loopback hop is latency for nothing. setsockopt(client, IPPROTO_TCP, TCP_NODELAY, &enabled, socklen_t(MemoryLayout.size)) diff --git a/macapp/WorkroomApp/Core/AgentVCSConnection.swift b/macapp/WorkroomApp/Core/AgentVCSConnection.swift index 1ea161c76..a1d766a08 100644 --- a/macapp/WorkroomApp/Core/AgentVCSConnection.swift +++ b/macapp/WorkroomApp/Core/AgentVCSConnection.swift @@ -65,6 +65,9 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// because it gates a FRAMING capability, not the request body: a version-1 agent speaks the same /// `AgentExecRequest` and is fully usable, it just cannot be sent one in pieces. private static let chunkedRequestVersion = 2 + /// The first exec service version a remote host's writes can use: it takes `host_environment` + /// and `barrier_root`, and answers `stat`. + private static let remoteWriteVersion = 3 /// `MAX_ENVELOPE_PAYLOAD` in `protocol/envelope.rs`. private static let maxEnvelopePayload = 1 << 20 /// `MAX_REQUEST` in `vcs.rs` — the reassembled ceiling, mirroring the reply side's. @@ -90,11 +93,15 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// per box, so there is nothing to key subscriptions by. let ceilingPrompts: AsyncStream private let ceilingPrompt: AsyncStream.Continuation + /// The local process carrying a driver's stream (`ssh host wr-agent relay`), or nil for a local + /// agent's socket. Ended when the connection fails, so a closed connection leaves no ssh behind. + private let carrier: HostStream? - private init(host: HostID, descriptor: Int32, helloVersion: UInt16) { + private init(host: HostID, descriptor: Int32, helloVersion: UInt16, carrier: HostStream?) { self.host = host self.descriptor = descriptor self.helloVersion = helloVersion + self.carrier = carrier (disconnection, disconnected) = AsyncStream.makeStream() // Newest-only: a prompt the app never got round to reading is superseded by the next one, and // the deadline in a stale one has passed by definition. @@ -105,32 +112,77 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { deinit { Darwin.close(descriptor) } static func connect(host: HostID, socketPath: String) async throws -> AgentVCSConnection { - let connection = try await runBlocking { + let fd = try await runBlocking { let fd = socket(AF_UNIX, SOCK_STREAM, 0) guard fd >= 0 else { throw HostConnectionError.connectionLost } - do { - var address = sockaddr_un() - address.sun_family = sa_family_t(AF_UNIX) - let path = Array(socketPath.utf8) - guard !path.contains(0), path.count < MemoryLayout.size(ofValue: address.sun_path) else { - throw VCSError.io("Agent socket path is invalid.") - } - withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: path) } - let connected = withUnsafePointer(to: &address) { - $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { - Darwin.connect(fd, $0, socklen_t(MemoryLayout.size)) - } + noSignalOnWrite(fd) + var address = sockaddr_un() + address.sun_family = sa_family_t(AF_UNIX) + let path = Array(socketPath.utf8) + guard !path.contains(0), path.count < MemoryLayout.size(ofValue: address.sun_path) else { + Darwin.close(fd) + throw VCSError.io("Agent socket path is invalid.") + } + withUnsafeMutableBytes(of: &address.sun_path) { $0.copyBytes(from: path) } + let connected = withUnsafePointer(to: &address) { + $0.withMemoryRebound(to: sockaddr.self, capacity: 1) { + Darwin.connect(fd, $0, socklen_t(MemoryLayout.size)) } - guard connected == 0 else { throw HostConnectionError.connectionLost } - var noSignal: Int32 = 1 - setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &noSignal, socklen_t(MemoryLayout.size)) - var timeout = timeval(tv_sec: 2, tv_usec: 0) + } + guard connected == 0 else { + Darwin.close(fd) + throw HostConnectionError.connectionLost + } + return fd + } + return try await negotiate(host: host, descriptor: fd, carrier: nil, handshakeTimeout: 2) + } + + /// Over a driver's stream (`HostDriver.openStream`), which this connection now owns: its + /// descriptor is closed and its process ended with the connection, including when this throws. + /// + /// A handshake the stream never answered is `serviceUnavailable` with what the carrier said, not + /// `connectionLost`. There is nothing to respawn on a remote host, and "Host key verification + /// failed." or the relay's "no agent listening" is the whole diagnosis. + static func connect(host: HostID, stream: HostStream) async throws -> AgentVCSConnection { + do { + return try await negotiate( + host: host, descriptor: stream.handOff(), carrier: stream, + handshakeTimeout: stream.handshakeTimeout) + } catch HostConnectionError.connectionLost { + throw HostConnectionError.serviceUnavailable(await stream.failure()) + } catch AgentControlClient.AgentProtocolError.notAnAgent { + // Commonly a shell startup file on the host that prints to stdout ahead of the relay. Not + // `failure()`: the carrier answered, and saying it did not would contradict this. + throw HostConnectionError.serviceUnavailable( + "The host sent something other than an agent's greeting (does a shell startup file print?)." + ) + } + } + + /// `SO_NOSIGPIPE`, set as soon as the socket exists: a write to a peer that has hung up then + /// fails with `EPIPE` rather than killing the app. Not later: once the peer has gone, `setsockopt` + /// itself fails, and a peer that greets and hangs up before the first write left the flag unset + /// and the hello's `send` raising SIGPIPE. + static func noSignalOnWrite(_ fd: Int32) { + var on: Int32 = 1 + setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &on, socklen_t(MemoryLayout.size)) + } + + private static func negotiate( + host: HostID, descriptor fd: Int32, carrier: HostStream?, handshakeTimeout: TimeInterval + ) async throws -> AgentVCSConnection { + // `SO_NOSIGPIPE` is already set: by `connect(host:socketPath:)` or `HostStream.spawn`, when + // the socket was made (see `noSignalOnWrite`). + let connection = try await runBlocking { + do { + var timeout = timeval(tv_sec: max(1, Int(handshakeTimeout.rounded(.up))), tv_usec: 0) setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, socklen_t(MemoryLayout.size)) setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, socklen_t(MemoryLayout.size)) try send(fd, Data(AgentControlClient.encodeHello(build: "Workroom VCS"))) var hello: [UInt8] = [] var helloVersion: UInt16 = 0 - let deadline = Date().addingTimeInterval(2) + let deadline = Date().addingTimeInterval(handshakeTimeout) while true { if let greeting = try AgentControlClient.decodeHello(hello) { helloVersion = greeting.version @@ -152,20 +204,22 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { } timeout = timeval(tv_sec: 0, tv_usec: 0) setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeout, socklen_t(MemoryLayout.size)) - // The 2s handshake bound must not linger onto ordinary request sends — a legitimately busy - // agent (mid VCS request on another thread) can leave the socket buffer full past 2s with + // The handshake bound must not linger onto ordinary request sends — a legitimately busy + // agent (mid VCS request on another thread) can leave the socket buffer full past it with // no fault of its own; a lingering SO_SNDTIMEO would then report that as connection loss. setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, socklen_t(MemoryLayout.size)) - return AgentVCSConnection(host: host, descriptor: fd, helloVersion: helloVersion) + return AgentVCSConnection( + host: host, descriptor: fd, helloVersion: helloVersion, carrier: carrier) } catch { Darwin.close(fd) + carrier?.end() throw error } } DispatchQueue.global(qos: .userInitiated).async { connection.receive() } do { let reply = try await connection.request( - AgentVCSRequest(method: "capabilities"), timeout: 2) + AgentVCSRequest(method: "capabilities"), timeout: handshakeTimeout) let capabilities = try AgentVCSReply.decode(reply) // `backendVersion`, not `serviceUnavailable`: an agent that answers but speaks different reads // (a newer one left running across an app rollback — it outlives the app and may own @@ -176,7 +230,7 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { // Not yet shared with any other caller, so a plain lock-guarded write is enough — no // concurrent reader can observe a half-set value. connection.lock.withLock { connection._capabilities = capabilities } - await connection.negotiateFiles() + await connection.negotiateFiles(timeout: handshakeTimeout) return connection } catch HostConnectionError.connectionLost, HostConnectionError.notDispatched { // `notDispatched` too: `request` refuses with it once the connection is closed, and a fresh @@ -219,6 +273,16 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { func writer(context: RepositoryContext, reader: VCSProviding) throws -> VCSWriting { guard context.location.host == host else { throw HostConnectionError.mismatchedContext } guard lock.withLock({ !closed }) else { throw HostConnectionError.connectionLost } + // A remote host's writes need what exec version 3 added (#229): the host's own environment, + // the jj barrier taken for each command, and `stat` for the classifier's disk facts. An older + // agent there fails closed rather than writing on Mac-side assumptions. + let remote = host != .local + if remote { + guard let exec = capabilities?.exec, exec >= Self.remoteWriteVersion else { + throw HostConnectionError.serviceUnavailable( + "The host's agent is too old to change a repository there.") + } + } // Presence and version of the SERVICE, not a count of the caller's own methods. `>=` rather // than `==` so an agent that gains a version 3 does not refuse a client speaking 1 — the agent // is what decides whether it still accepts this request, and it answers that on the request @@ -234,12 +298,40 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { guard let exec = capabilities?.exec, exec >= Self.execVersion else { throw VCSError.backendVersion("Agent does not support VCS writes.") } - let engine = CLIVCSWriter( - vcs: context.backend.rawValue, runner: AgentCommandRunner(connection: self), + // A Mac cannot flock the jj barrier on another host, so there the agent takes it for each + // command, under the shared root the local gate would have locked. + let barrierRoot = remote && context.backend == .jj ? try context.requireOwnership().path : nil + var engine = CLIVCSWriter( + vcs: context.backend.rawValue, + runner: AgentCommandRunner(connection: self, barrierRoot: barrierRoot), makeProvider: { _ in AgentCurrentRefProvider(reader: reader) }, gate: .shared) + if remote { + engine.host = host + engine.stat = { [self] paths, read in try await stat(paths: paths, read: read) } + } return try BoundLocalWriter(context: context, reader: reader, writer: engine) } + /// Facts from the host's disk for `CLIVCSWriter`'s failure classifier (`stat` in `vcs.rs`): + /// whether each path exists, is a directory and when it changed, and the text of those in `read`. + func stat(paths: [String], read: [String]) async throws -> DiskSnapshot { + let reply = try await request(AgentStatRequest(paths: paths, read: read), timeout: 15) + var entries: [String: DiskEntry?] = [:] + var texts: [String: String] = [:] + for entry in try AgentVCSReply.decode(reply).entries { + let found = + entry.exists + ? DiskEntry( + isDirectory: entry.isDir, + modifiedAt: entry.modifiedAt.map { Date(timeIntervalSince1970: $0) }) + : nil + // `.some(nil)`: asked about and absent, which a snapshot tells apart from never asked. + entries[entry.path] = .some(found) + if let text = entry.text { texts[entry.path] = text } + } + return DiskSnapshot(entries: entries, texts: texts) + } + /// Probe the File service, if the peer's greeting says it exists. Runs once in `connect()`, before /// this connection is shared, and NEVER fails it: an agent that predates the service, or one whose /// probe is unanswered or unreadable, leaves VCS fully working and just has no file service — @@ -249,14 +341,14 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { /// /// Probed on `Service::File` itself, never through the VCS `capabilities` reply, whose `reads` /// count is compared for equality by every existing client. - private func negotiateFiles() async { + private func negotiateFiles(timeout: TimeInterval) async { // Against the peer's RAW greeting, before any File envelope: a protocol-2 agent silently drops // them, so the probe would otherwise wait out its whole timeout. guard helloVersion >= AgentControlClient.minFileVersion else { return } let outcome: FileNegotiation do { let reply = try await request( - AgentFileRequest(method: "capabilities"), timeout: 2, service: Self.fileService) + AgentFileRequest(method: "capabilities"), timeout: timeout, service: Self.fileService) let capabilities = try AgentFileReply.decode(reply) outcome = capabilities.version == 1 ? .ready : .unsupported } catch { @@ -777,6 +869,7 @@ final class AgentVCSConnection: HostServiceConnection, @unchecked Sendable { return (values, watchers, forwards) } guard let failed else { return } + carrier?.end() ceilingPrompt.finish() for operation in failed.pending { operation.continuation.resume(throwing: error) } // Outside the lock: a handler is caller code. Every subscription learns its watch is gone, so it @@ -804,6 +897,25 @@ struct AgentVCSCapabilities: Decodable { let exec: Int? } +/// `StatRequest` in `vcs.rs`, which rejects unknown fields. +struct AgentStatRequest: Encodable, Sendable { + var version = 1 + var kind = "stat" + let paths: [String] + let read: [String] +} + +struct AgentStatReply: Decodable { + struct Entry: Decodable { + let path: String + let exists: Bool + let isDir: Bool + let modifiedAt: Double? + let text: String? + } + let entries: [Entry] +} + struct AgentVCSRequest: Encodable, Sendable { var version = 1 var root: String? diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift new file mode 100644 index 000000000..152a490b4 --- /dev/null +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -0,0 +1,182 @@ +import Foundation + +/// The first `HostDriver`: a Linux container running sshd and a supervised `wr-agent` +/// (`vcs/scripts/ssh-fixture`, #228), reached over plain ssh-stdio. It is the test fixture for +/// the remote path, and its `openStream` is the ssh transport a real ssh-reachable driver (boxd) +/// will share. +/// +/// Provisioning is Phase 4, so `create`, `deriveFromBase` and `destroy` say so rather than pretend. +/// Which hosts exist is whatever the caller hands in; nothing here persists. +struct ContainerHostDriver: HostTerminalDriver { + struct Host: Sendable { + let address: String + let port: Int + let user: String + /// The private key ssh authenticates with. The only credential used: `IdentitiesOnly` and + /// `IdentityAgent none` keep the Mac's ssh agent and default keys out of it. + let identityFile: String + /// The host's public key, ` `, delivered out of band. It is the only key ssh + /// accepts, and a mismatch fails closed: `BatchMode` cannot prompt to accept a new one. + let hostKey: String + /// The supervised agent's socket on the host. + let agentSocket: String + } + + let traits = HostDriverTraits( + transport: .sshStdio, deriveSpeed: nil, deriveCarriesLiveProcesses: false, + durableDisk: false, maxLifetime: nil) + + let hosts: [UUID: Host] + /// Where each host's `ssh_config` and `known_hosts` are written. + let directory: URL + + func create() async throws -> HostID { throw HostDriverError.notImplemented("Creating a base") } + + func deriveFromBase(_ base: HostID) async throws -> HostID { + throw HostDriverError.notImplemented("Deriving a workroom instance") + } + + func destroy(_ host: HostID) async throws { + throw HostDriverError.notImplemented("Destroying a host") + } + + func openStream(to host: HostID) async throws -> HostStream { + guard case .remote(let id) = host, let target = hosts[id] else { + throw HostDriverError.unknownHost(host) + } + let config = try Self.writeConfiguration( + for: target, in: directory.appendingPathComponent(id.uuidString)) + return try HostStream.spawn( + URL(fileURLWithPath: "/usr/bin/ssh"), + ["-F", config.path, Self.alias, Self.relayCommand(socket: target.agentSocket)], + // Nothing from the app's own environment: ssh needs none of it with this configuration, and + // `SSH_AUTH_SOCK` in particular is a Mac credential. `-F` also keeps `/etc/ssh/ssh_config` + // out, whose `SendEnv LANG LC_*` would send the Mac's locale across. + environment: [:], + // ssh connects and authenticates before the agent can greet. `ConnectTimeout` bounds the + // connect; this leaves room for the rest. + handshakeTimeout: 20) + } + + func attachCommand( + to host: HostID, session: UUID, workingDirectory: String, restored: Bool + ) throws -> String { + guard case .remote(let id) = host, let target = hosts[id] else { + throw HostDriverError.unknownHost(host) + } + let config = try Self.writeConfiguration( + for: target, in: directory.appendingPathComponent(id.uuidString)) + // `-t`: the attach client on the far side wants a terminal, for raw mode and the pane's size, + // and ssh forwards the pane's resizes to it. It outranks the config's `RequestTTY no`, which + // is right for the service stream. + return ["/usr/bin/ssh", "-F", config.path, "-t", Self.alias] + .map(Self.shellQuoted).joined(separator: " ") + + " " + + Self.shellQuoted( + Self.remoteAttachCommand( + session: session, socket: target.agentSocket, workingDirectory: workingDirectory, + restored: restored)) + } + + /// What runs on the host, in the pty ssh allocates there. + /// + /// The session contract is the environment `wr-agent attach` reads, set with `env` because ssh + /// carries none of the pane's own. Deliberately absent: the shell (the host's own login shell + /// applies), Ghostty's resources directory (a path in this Mac's app bundle), and the wakefulness + /// settings (they configure an agent the attach starts, and `--no-spawn` never starts one). + /// `TERM` is `xterm-256color`, not the pane's `xterm-ghostty`, which a host rarely has terminfo + /// for; shipping that terminfo belongs to the agent bootstrap (#231). + static func remoteAttachCommand( + session: UUID, socket: String, workingDirectory: String, restored: Bool + ) -> String { + let variables = [ + "TERM=xterm-256color", + "WORKROOM_SESSION_ID=\(session.uuidString)", + "WORKROOM_SESSION_SOCKET=\(socket)", + "WORKROOM_SESSION_CWD=\(workingDirectory)", + ] + return + (["env"] + variables + ["wr-agent", "attach", "--no-spawn"] + + (restored ? ["--no-create"] : [])) + .map(shellQuoted).joined(separator: " ") + } + + static let alias = "workroom-host" + + /// The command ssh runs on the host, quoted for the remote shell. + static func relayCommand(socket: String) -> String { + "wr-agent relay --socket " + shellQuoted(socket) + } + + /// Writes the host's `ssh_config` and `known_hosts`, and returns the config's path. + /// + /// The whole policy is here, so nothing in `~/.ssh/config` can change it: + /// - **It cannot prompt.** `BatchMode` turns a password, passphrase or unknown-host prompt into a + /// failure, and `StrictHostKeyChecking` with only the pinned key known makes any other key one + /// of those failures. `HostKeyAlgorithms` is deliberately not pinned to the key's type: it + /// names signature algorithms, so for an RSA key it would pin SHA-1 `ssh-rsa`, which OpenSSH + /// 8.8+ servers no longer offer. + /// - **No Mac credential crosses.** No agent forwarding, no X11, no port forwards, and the Mac's + /// ssh agent is not consulted for keys. + /// - **What a pane types is only ever the user's.** A pane's ssh has a terminal, and ssh reads + /// `~.` and friends from one as its own commands (`~.` disconnects, `~^Z` suspends ssh and + /// freezes the pane), so `EscapeChar none`. + /// - **A dead link is noticed.** `ServerAliveInterval` ends ssh after about 45s of silence from a + /// host that stopped answering, which ends the stream and so the connection. It is what bounds + /// a link that dies with nothing to send (#228). + static func writeConfiguration(for host: Host, in directory: URL) throws -> URL { + for (name, value) in [ + ("address", host.address), ("user", host.user), ("identity file", host.identityFile), + ("host key", host.hostKey), ("agent socket", host.agentSocket), + ] { + guard !value.isEmpty, !value.contains(where: { $0.isNewline || $0 == "\"" || $0 == "\0" }) + else { + throw HostDriverError.invalidConfiguration("\(name) is empty or has a quote or newline") + } + } + guard (1...65535).contains(host.port) else { + throw HostDriverError.invalidConfiguration("port \(host.port) is out of range") + } + let key = host.hostKey.split(separator: " ") + guard key.count >= 2 else { + throw HostDriverError.invalidConfiguration("host key must be ` `") + } + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let knownHosts = directory.appendingPathComponent("known_hosts") + let name = host.port == 22 ? host.address : "[\(host.address)]:\(host.port)" + try "\(name) \(key[0]) \(key[1])\n".write(to: knownHosts, atomically: true, encoding: .utf8) + let config = directory.appendingPathComponent("ssh_config") + try """ + Host \(alias) + HostName "\(host.address)" + Port \(host.port) + User "\(host.user)" + IdentityFile "\(host.identityFile)" + IdentitiesOnly yes + IdentityAgent none + BatchMode yes + StrictHostKeyChecking yes + UserKnownHostsFile "\(knownHosts.path)" + GlobalKnownHostsFile /dev/null + UpdateHostKeys no + ForwardAgent no + ForwardX11 no + ClearAllForwardings yes + ControlMaster no + ControlPath none + EscapeChar none + RequestTTY no + ServerAliveInterval 15 + ServerAliveCountMax 3 + ConnectTimeout 10 + LogLevel ERROR + + """.write(to: config, atomically: true, encoding: .utf8) + return config + } + + /// One POSIX shell word, whatever `text` holds. + static func shellQuoted(_ text: String) -> String { + "'" + text.replacingOccurrences(of: "'", with: "'\\''") + "'" + } +} diff --git a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift index 0b063641a..c4bd33c07 100644 --- a/macapp/WorkroomApp/Core/GhosttySurfaceView.swift +++ b/macapp/WorkroomApp/Core/GhosttySurfaceView.swift @@ -458,7 +458,7 @@ final class GhosttySurfaceView: NSView { // the owning backend has no binary, so the global check bought nothing on this path. guard let attach = PersistentSessionService.shared.attachCommand( - forSession: persistentSessionID), + forSession: persistentSessionID, restored: persistentSessionIsRestored), let attachPointer = strdup(attach) else { // A pane that expected a persisted session fell back to a plain login shell. Whatever the @@ -519,8 +519,37 @@ final class GhosttySurfaceView: NSView { return "/bin/sh -c \(shellQuoted(script))" } + /// A pane on a remote host whose ssh lost the link reattaches, rather than sitting on a dead + /// process while its session runs on: 255 is ssh's own failure, as after laptop sleep, a network + /// change or the host rebooting. It reattaches as a restored pane, so a session that ended + /// meanwhile becomes a shell that says so. Backs off to 30s while the host stays unreachable. + private func reconnectIfTheLinkDropped(exitCode: UInt32) { + guard exitCode == 255, let session = persistentSessionID, !isTornDown, + PersistentSessionService.shared.isRemote(session) + else { return } + if Date().timeIntervalSince(lastAttachAt) > 30 { + remoteReconnectDelay = 1 + failedReconnects = 0 + } else { + failedReconnects += 1 + guard failedReconnects < Self.maxFailedReconnects else { return } + } + let delay = remoteReconnectDelay + remoteReconnectDelay = min(delay * 2, 30) + let reconnect = DispatchWorkItem { [weak self] in + guard let self, !self.isTornDown, self.persistentSessionID == session else { return } + self.persistentSessionIsRestored = true + self.reattachPersistentSession() + } + pendingReconnect = reconnect + DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: reconnect) + } + func reattachPersistentSession() { guard persistentSessionID != nil, !isTornDown else { return } + pendingReconnect?.cancel() + pendingReconnect = nil + lastAttachAt = Date() destroySurface() if bounds.width <= 0 || bounds.height <= 0 { setFrameSize(CGSize(width: 800, height: 480)) @@ -883,7 +912,22 @@ final class GhosttySurfaceView: NSView { /// Called by `GhosttyRuntimeAdapter` on `GHOSTTY_ACTION_SHOW_CHILD_EXITED` — the surface's child /// process exited. Forwards to the host; only run tabs act on it (issue #7). + /// How long the next reconnect of a remote pane waits, doubling while the host stays unreachable. + private var remoteReconnectDelay: TimeInterval = 1 + /// When this pane last attached, to tell a link that dropped after a while (start the backoff + /// again) from one that never came up (keep backing off). + private var lastAttachAt = Date.distantPast + /// Reconnects in a row that failed within 30s. Past `maxFailedReconnects` the pane stops and + /// leaves ssh's last error on screen: a mismatched host key or a destroyed host does not heal, + /// and each retry would wipe the message that says so. + private var failedReconnects = 0 + private static let maxFailedReconnects = 5 + /// The reconnect waiting out its delay, cancelled by any other reattach so the pane does not + /// attach twice. + private var pendingReconnect: DispatchWorkItem? + func handleChildExited(exitCode: UInt32) { + reconnectIfTheLinkDropped(exitCode: exitCode) onChildExited?(exitCode) } diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift new file mode 100644 index 000000000..18a30e3a3 --- /dev/null +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -0,0 +1,215 @@ +import Darwin +import Foundation + +/// How a provider's hosts are reached and what they can do, declared so a provider can be gated +/// when it is chosen (design doc, Phase 3 "the stream drivers", and Premise 8). +struct HostDriverTraits: Equatable, Sendable { + enum Transport: Equatable, Sendable { case sshStdio, sdkExec, webSocket } + + let transport: Transport + /// Roughly how long `deriveFromBase` takes, or nil for a driver that cannot derive yet. + let deriveSpeed: Duration? + /// Whether a derived instance carries the base's running processes (a live fork), or only its + /// disk. + let deriveCarriesLiveProcesses: Bool + let durableDisk: Bool + /// The wall clock an instance dies on, or nil for one that lives until destroyed. + let maxLifetime: Duration? +} + +/// One provider. Four methods, and the only one that is not provisioning is `openStream` +/// (design doc, Phase 3). `deriveFromBase` is the ONLY way a workroom instance comes to exist; +/// `create` makes the base it derives from. +/// +/// **Every driver must authenticate its peer as strongly as a shell login.** The agent's exec +/// service runs `git` with arbitrary arguments, which is arbitrary code execution, so a stream +/// that reaches an agent is a shell whatever it is called. ssh meets that bar; an SDK exec or a +/// WebSocket driver must show it does before it counts as a driver. +protocol HostDriver: Sendable { + var traits: HostDriverTraits { get } + /// Provisions a base. + func create() async throws -> HostID + /// Provisions a workroom instance from `base`. + func deriveFromBase(_ base: HostID) async throws -> HostID + /// Takes down a base or an instance. + func destroy(_ host: HostID) async throws + /// A byte stream to the agent on `host`, base or instance, for + /// `AgentVCSConnection.connect(host:stream:)`. + func openStream(to host: HostID) async throws -> HostStream +} + +/// A driver whose hosts a terminal pane can attach to. +/// +/// Not a fifth `HostDriver` method, and not `openStream`: a pane is a process libghostty spawns and +/// wires to its own pty, and a socketpair made in this process cannot become one. So the driver +/// hands over a command instead. For ssh that is `ssh -t wr-agent attach …`, the far side's +/// attach client running in the pty ssh allocates there. An SDK-exec driver has no such command, +/// and would need a local bridge process: a Phase 4 question (design doc, Phase 3). +protocol HostTerminalDriver: HostDriver { + /// The command a pane runs to attach to `session` on `host`, starting in `workingDirectory` if + /// the session is new. `restored` for a pane reattaching after a relaunch or a lost link: the + /// session must already exist there, and if it has ended the pane gets a shell that says so + /// rather than a fresh one that looks like it (`wr-agent attach --no-create`). + func attachCommand(to host: HostID, session: UUID, workingDirectory: String, restored: Bool) + throws -> String +} + +enum HostDriverError: Error, Equatable, Sendable, LocalizedError { + case unknownHost(HostID) + /// Provisioning lands in Phase 4, which is what says how a base and an instance come to exist. + case notImplemented(String) + case invalidConfiguration(String) + + var errorDescription: String? { + switch self { + case .unknownHost: return "This driver has no such host." + case .notImplemented(let what): return "\(what) is not implemented yet." + case .invalidConfiguration(let detail): return "Invalid host configuration: \(detail)" + } + } +} + +/// A bidirectional byte stream to the agent on a far-side host: one end of a socketpair, whose +/// other end is the stdin and stdout of the local process carrying it (`ssh host wr-agent relay`). +/// +/// Whoever connects over it owns both: `AgentVCSConnection` closes the descriptor and ends the +/// process with the connection. The carrier's exit is also how a lost link is noticed. The child +/// holds the only copy of its end of the pair, so when it exits, that end closes and the next read +/// on this one sees EOF. +final class HostStream: @unchecked Sendable { + let descriptor: Int32 + /// How long the agent's greeting may take: a local relay answers at once, ssh must connect and + /// authenticate first. + let handshakeTimeout: TimeInterval + private let process: Process + private let lock = NSLock() + private var errors = Data() + /// stderr reached EOF: everything the carrier said is in `errors`. + private var errorsClosed = false + /// `end()` stopped a carrier that was still running, so its exit status is our own signal. + private var endedByUs = false + /// A connection took the descriptor and the process (`handOff`), so `deinit` leaves them alone. + private var handedOff = false + + var processIdentifier: Int32 { process.processIdentifier } + + private init(descriptor: Int32, process: Process, handshakeTimeout: TimeInterval) { + self.descriptor = descriptor + self.process = process + self.handshakeTimeout = handshakeTimeout + } + + /// A stream nothing connected over is still this object's: an ssh whose stdin never reads EOF + /// holds a healthy link open for good. + deinit { + guard !lock.withLock({ handedOff }) else { return } + Darwin.close(descriptor) + end() + } + + /// Gives the descriptor and the process to the connection made over them, which closes and ends + /// them from then on. + func handOff() -> Int32 { + lock.withLock { handedOff = true } + return descriptor + } + + /// Runs `executable` with a socketpair end as both its stdin and its stdout. + /// + /// `environment` is the carrier's WHOLE environment, never merged with the app's: whatever it + /// holds can reach the far side, so a driver passes exactly what its transport needs. + static func spawn( + _ executable: URL, _ arguments: [String], environment: [String: String], + handshakeTimeout: TimeInterval + ) throws -> HostStream { + var pair: [Int32] = [0, 0] + guard socketpair(AF_UNIX, SOCK_STREAM, 0, &pair) == 0 else { + throw HostConnectionError.serviceUnavailable("socketpair failed: errno \(errno)") + } + let (ours, theirs) = (pair[0], pair[1]) + AgentVCSConnection.noSignalOnWrite(ours) + // Close-on-exec on both, so a process spawned later cannot inherit an end: a stray copy of the + // child's end would keep it open after the carrier exits, and the lost link would never read + // as EOF here. There is still a window between `socketpair` and these two calls, which Darwin + // cannot close (it has no `SOCK_CLOEXEC`); Foundation's own spawns close every descriptor they + // were not given, so only a raw `fork` in that window could catch one. The spawn below dup2s + // the child's end onto 0 and 1, which clears the flag on those copies only. + _ = fcntl(ours, F_SETFD, FD_CLOEXEC) + _ = fcntl(theirs, F_SETFD, FD_CLOEXEC) + + let process = Process() + process.executableURL = executable + process.arguments = arguments + process.environment = environment + let child = FileHandle(fileDescriptor: theirs, closeOnDealloc: false) + process.standardInput = child + process.standardOutput = child + let errors = Pipe() + process.standardError = errors + let stream = HostStream( + descriptor: ours, process: process, handshakeTimeout: handshakeTimeout) + errors.fileHandleForReading.readabilityHandler = { [weak stream] handle in + let data = handle.availableData + guard !data.isEmpty else { + handle.readabilityHandler = nil + stream?.lock.withLock { stream?.errorsClosed = true } + return + } + stream?.collect(data) + } + do { + try process.run() + } catch { + // Only the child's end: `ours` is the stream's, and its `deinit` closes it on the way out. + Darwin.close(theirs) + errors.fileHandleForReading.readabilityHandler = nil + throw HostConnectionError.serviceUnavailable( + "Could not start \(executable.lastPathComponent): \(error.localizedDescription)") + } + // The child now holds the only copy. Keeping this one would hide the child's exit. + Darwin.close(theirs) + return stream + } + + /// Ends the carrier. Idempotent, and safe after it has exited by itself. + func end() { + guard process.isRunning else { return } + lock.withLock { endedByUs = true } + process.terminate() + } + + /// What the carrier said about why the stream ended, for a handshake it never answered: its + /// stderr, or failing that how it ended. Waits for stderr to reach EOF rather than for the + /// process to be reaped: the two are separate events, and the reason ("Host key verification + /// failed.") is in the first. + func failure() async -> String { + for _ in 0..<40 where !lock.withLock({ errorsClosed }) || process.isRunning { + try? await Task.sleep(for: .milliseconds(50)) + } + let (said, endedByUs) = lock.withLock { + (String(decoding: errors, as: UTF8.self), self.endedByUs) + } + let reason = said.trimmingCharacters(in: .whitespacesAndNewlines) + if !reason.isEmpty { return reason } + let name = process.executableURL?.lastPathComponent ?? "carrier" + // `isRunning` first: `terminationReason` and `terminationStatus` raise an Objective-C + // exception for a process that has not been reaped yet, which Swift cannot catch. A carrier + // stopped a moment ago can still be running once the wait above gives up. + if process.isRunning { return "\(name) did not answer in time" } + // Our own SIGTERM, from `end()`: the carrier was still running when the handshake gave up. + if endedByUs, process.terminationReason == .uncaughtSignal, + process.terminationStatus == SIGTERM + { + return "\(name) did not answer in time" + } + return "\(name) exited with status \(process.terminationStatus)" + } + + /// Keeps the first 16 KiB of stderr: enough for any diagnosis, and bounded against a chatty one. + private func collect(_ data: Data) { + lock.withLock { + guard errors.count < 16 * 1024 else { return } + errors.append(data.prefix(16 * 1024 - errors.count)) + } + } +} diff --git a/macapp/WorkroomApp/Core/RepositoryDisk.swift b/macapp/WorkroomApp/Core/RepositoryDisk.swift new file mode 100644 index 000000000..9215270ed --- /dev/null +++ b/macapp/WorkroomApp/Core/RepositoryDisk.swift @@ -0,0 +1,63 @@ +import Foundation + +/// What `CLIVCSWriter`'s failure classifier reads from a repository's disk: whether a path exists, +/// whether it is a directory, when it was modified, and the text of a small file (a worktree's +/// `.git`). Every decision stays in the classifier; only where these facts come from changes. For a +/// local repository they are this Mac's (`LocalDisk`); for one on a remote host, what that host's +/// agent reported (`DiskSnapshot`, from `AgentVCSConnection.stat`), since reading this disk for a +/// path on another machine answers nothing, or worse, the wrong thing. +protocol RepositoryDisk: Sendable { + func entry(_ path: String) -> DiskEntry? + func text(_ path: String) -> String? +} + +struct DiskEntry: Equatable, Sendable { + let isDirectory: Bool + let modifiedAt: Date? +} + +/// This Mac's disk, read live, exactly as the classifier always read it. +struct LocalDisk: RepositoryDisk { + func entry(_ path: String) -> DiskEntry? { + var isDirectory: ObjCBool = false + guard FileManager.default.fileExists(atPath: path, isDirectory: &isDirectory) else { + return nil + } + let modified = + (try? FileManager.default.attributesOfItem(atPath: path))?[.modificationDate] as? Date + return DiskEntry(isDirectory: isDirectory.boolValue, modifiedAt: modified) + } + + func text(_ path: String) -> String? { try? String(contentsOfFile: path, encoding: .utf8) } +} + +/// Facts a remote host reported for a fixed set of paths, read after the command they explain. +/// +/// A path the snapshot was not asked about is a bug in `CLIVCSWriter.classificationPaths`, not a +/// missing file, so it traps in debug builds rather than quietly reading as absent. +struct DiskSnapshot: RepositoryDisk { + let entries: [String: DiskEntry?] + let texts: [String: String] + + /// For a host that could not be asked: every path reads as absent, which is the classifier's + /// "no evidence" answer (no parked rebase, no lock file to name). + static let unknown = DiskSnapshot(entries: [:], texts: [:], complete: false) + + private let complete: Bool + + init(entries: [String: DiskEntry?], texts: [String: String], complete: Bool = true) { + self.entries = entries + self.texts = texts + self.complete = complete + } + + func entry(_ path: String) -> DiskEntry? { + guard let known = entries[path] else { + assert(!complete, "\(path) was not in the snapshot") + return nil + } + return known + } + + func text(_ path: String) -> String? { texts[path] } +} diff --git a/macapp/WorkroomApp/Core/RepositoryServices.swift b/macapp/WorkroomApp/Core/RepositoryServices.swift index 8fc4bfae6..28e690a89 100644 --- a/macapp/WorkroomApp/Core/RepositoryServices.swift +++ b/macapp/WorkroomApp/Core/RepositoryServices.swift @@ -90,8 +90,17 @@ struct BoundLocalWriter: VCSWriting { private var path: String { context.location.path } private let projectRoot: String + /// For a local repository, or a remote one whose writer reads that host's disk and holds its + /// barrier through the agent (`AgentVCSConnection.writer`). The paths here are only ever handed to + /// the writer as strings, never resolved against this disk. init(context: RepositoryContext, reader: VCSProviding, writer: LocalVCSWriting) throws { - _ = try context.location.requireLocalURL() + // A remote repository only with a writer that knows it is one; anything else would run git on + // this Mac against a path on another machine. + if context.location.host != .local { + guard let engine = writer as? CLIVCSWriter, engine.host == context.location.host else { + throw RepositoryRoutingError.unavailable(context.location.host) + } + } projectRoot = try context.requireOwnership().path self.context = context self.reader = reader diff --git a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift index 519ad4f58..69c029ceb 100644 --- a/macapp/WorkroomApp/Core/Session/AgentControlClient.swift +++ b/macapp/WorkroomApp/Core/Session/AgentControlClient.swift @@ -159,6 +159,8 @@ struct AgentControlClient: SessionControlPlane { let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) guard descriptor >= 0 else { return nil } defer { close(descriptor) } + // Before `connect`, not after: see `AgentVCSConnection.noSignalOnWrite`. + AgentVCSConnection.noSignalOnWrite(descriptor) var address = sockaddr_un() address.sun_family = sa_family_t(AF_UNIX) diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift index bdf68f55c..099e219d7 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionControlClient.swift @@ -125,6 +125,9 @@ struct PersistentSessionControlClient { let descriptor = socket(AF_UNIX, SOCK_STREAM, 0) guard descriptor >= 0 else { return .silent } defer { close(descriptor) } + // A daemon that hangs up mid-write must fail the write, not kill the app. Set before `connect`: + // see `AgentVCSConnection.noSignalOnWrite`. + AgentVCSConnection.noSignalOnWrite(descriptor) guard var address = Self.address(for: socketPath) else { return .noListener } let connected = withUnsafePointer(to: &address) { pointer in diff --git a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift index b3d6781c8..8f616562a 100644 --- a/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift +++ b/macapp/WorkroomApp/Core/Session/PersistentSessionService.swift @@ -57,6 +57,17 @@ final class PersistentSessionService { /// 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] = [:] + /// Sessions on a remote host, and the driver that reaches it (#229). Checked before any local + /// owner: a remote session's owner is known by construction, and no local socket can answer for + /// it. Registered by whoever makes a pane for a remote workroom; nothing persists it yet, so a + /// relaunch re-registers it (Phase 4). + private var remoteSessions: [UUID: RemoteSession] = [:] + + private struct RemoteSession { + let host: HostID + let driver: any HostTerminalDriver + let workingDirectory: String + } /// How the agent's health is measured. Injected so a test can drive the unhealthy path without /// a real `wr-agent` to break. @@ -326,6 +337,9 @@ final class PersistentSessionService { // 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 } + // Asked of the host's agent in the attach itself (`--no-create`): one request, so the answer + // cannot go stale between the question and the attach. + if isRemote(sessionID) { return .attachable } guard let owner = backend(forSession: sessionID) else { return .attachable } switch ownership(of: sessionID, in: owner) { case .owned, .unreachable: @@ -351,7 +365,37 @@ final class PersistentSessionService { /// 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? { + /// Points `sessionID` at `host`: its pane attaches there, through `driver`, starting in + /// `workingDirectory` (a path on that host) if the session is new. + func registerRemoteSession( + _ sessionID: UUID, on host: HostID, via driver: any HostTerminalDriver, + workingDirectory: String + ) { + remoteSessions[sessionID] = RemoteSession( + host: host, driver: driver, workingDirectory: workingDirectory) + } + + func isRemote(_ sessionID: UUID) -> Bool { remoteSessions[sessionID] != nil } + + func attachCommand(forSession sessionID: UUID, restored: Bool = false) -> String? { + if let remote = remoteSessions[sessionID] { + do { + return try remote.driver.attachCommand( + to: remote.host, session: sessionID, workingDirectory: remote.workingDirectory, + restored: restored) + } catch { + logger.error( + "no attach command for remote session \(sessionID.uuidString, privacy: .public): \(error)" + ) + // Not nil: nil opens a plain shell, which for this pane would run on the Mac while it + // reads as the remote workroom's. A pane that says why it could not reach the host is + // honest; one on the wrong machine is not. + let notice = "Could not reach this terminal's host: \(error.localizedDescription)" + return "/bin/sh -c " + + ContainerHostDriver.shellQuoted( + "printf '%s\\n' " + ContainerHostDriver.shellQuoted(notice)) + } + } // 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), @@ -369,6 +413,10 @@ final class PersistentSessionService { .defaultShell, resourcesDirectory: String? = GhosttyResources.bundledURL?.path ) -> [(key: String, value: String)] { + // None for a pane on a remote host: its command carries the session's variables to the host + // itself (`HostTerminalDriver.attachCommand`), and a `WORKROOM_SESSION_*` left in this pane's + // environment would point a local `wr-agent` typed there at a session it cannot reach. + if isRemote(sessionID) { return [] } // 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 @@ -449,6 +497,9 @@ final class PersistentSessionService { } func lookup(sessionID: UUID) async -> PersistentSessionLookup { + // A remote session is not on any local helper, and asking one would probe (and wait on) a + // socket that cannot know it. Its host answers for it; this build has no channel for that yet. + if isRemote(sessionID) { return .unreachable } // Routed by OWNER, with no gate on the preferred backend's socket. That gate was left over // from before per-session routing and inverted the drain: with the agent preferred but no // `agent.sock` yet, every session the daemon still owned reported `.unreachable` — and the @@ -470,6 +521,17 @@ final class PersistentSessionService { @discardableResult func endSession(sessionID: UUID) async -> Bool { + // Never routed to a local helper, which would be asked to kill an id it does not hold. Ending + // a session on its host belongs to that host's lifecycle (Phase 4); until then it is reported + // as not killed, which is true, and nothing that gates on this deletes a local directory for + // it. The registration STAYS: the session is still running there, and without it a retry + // would ask the local agent (which "kills" an id it never held) and a reattach would open a + // new session on this Mac. + if isRemote(sessionID) { + logger.error( + "remote session \(sessionID.uuidString, privacy: .public) left running on its host") + return false + } // 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 diff --git a/macapp/WorkroomApp/Core/StatusCommandRunner.swift b/macapp/WorkroomApp/Core/StatusCommandRunner.swift index 230cdcfc0..c84e9b798 100644 --- a/macapp/WorkroomApp/Core/StatusCommandRunner.swift +++ b/macapp/WorkroomApp/Core/StatusCommandRunner.swift @@ -207,6 +207,13 @@ struct StatusCommandRunner: StatusCommandRunning, Sendable { return env } + /// The whole environment a REMOTE exec request carries: the pins that are policy, not identity. + /// The remote agent builds everything else from the host's own environment. See + /// `AgentCommandRunner.request` for why nothing else crosses. + static let remoteEnvironment = [ + "GIT_OPTIONAL_LOCKS": "0", "GIT_TERMINAL_PROMPT": "0", "LC_ALL": "C", + ] + /// The complete environment a `git`/`jj` child gets, native or agent-routed. /// /// Extracted from `run` and made static so `AgentCommandRunner` can send the SAME map to diff --git a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift index f76b85256..2d1b8fa38 100644 --- a/macapp/WorkroomApp/Core/VCSSyncPresentation.swift +++ b/macapp/WorkroomApp/Core/VCSSyncPresentation.swift @@ -644,9 +644,12 @@ enum VCSSyncPresenter { } /// The blocking lock file's path, when the failure has one. + /// Nil for a lock file on a remote host too: both things offered for the path, Reveal in Finder + /// and Copy Lock File Path, are for a file on this Mac. git's own message, in the failure's + /// details, still names it. static func lockPath(of failure: VCSRemoteFailure) -> String? { - guard case .locked(let file) = failure else { return nil } - return file?.path + guard case .locked(let file) = failure, let file, file.isOnThisMac else { return nil } + return file.path } /// The tooltip form of a failure: `describe`'s one-liner plus, where there is one, the remedy. @@ -696,8 +699,8 @@ enum VCSSyncPresenter { details: commitRawOutput(of: failure), recovery: nil, lockPath: { - guard case .locked(let file) = failure else { return nil } - return file?.path + guard case .locked(let file) = failure, let file, file.isOnThisMac else { return nil } + return file.path }()) } diff --git a/macapp/WorkroomApp/Core/VCSWriting.swift b/macapp/WorkroomApp/Core/VCSWriting.swift index 7755377c8..e994b60f2 100644 --- a/macapp/WorkroomApp/Core/VCSWriting.swift +++ b/macapp/WorkroomApp/Core/VCSWriting.swift @@ -139,6 +139,9 @@ struct VCSFailureReport: Identifiable, Equatable, Sendable { struct VCSLockFile: Equatable, Sendable { let path: String let modifiedAt: Date + /// False for a lock file on a remote host: its path names nothing on this Mac, so there is nothing + /// here to reveal in Finder (`VCSSyncPresenter.lockPath`). + var isOnThisMac = true var filename: String { (path as NSString).lastPathComponent } } @@ -384,6 +387,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// `git worktree add` worktrees sharing one `.git`, so a lock lost mid-`pull --rebase` can leave a /// workroom wedged in a rebase. let gate: JJSnapshotGate + /// The host the repository is on. It keys the write gate (`gated`), so a remote path is never + /// resolved against this disk, nor shares a queue with a local repository at the same path. + var host: HostID = .local + /// Where the classifier's disk facts come from for a repository on a remote host: that host's + /// agent (`AgentVCSConnection.stat`). Nil for a local repository, which reads this Mac's disk. + var stat: (@Sendable (_ paths: [String], _ read: [String]) async throws -> DiskSnapshot)? var refTimeout: TimeInterval = 5 var fetchTimeout: TimeInterval = 120 @@ -441,15 +450,12 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// /// Pure string work, no subprocess: `git rev-parse --git-common-dir` is authoritative but costs a /// process for something two file reads answer. - static func commonGitDir(at path: String) -> URL? { + static func commonGitDir(at path: String, disk: any RepositoryDisk = LocalDisk()) -> URL? { let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git") - var isDir: ObjCBool = false - guard FileManager.default.fileExists(atPath: dotGit.path, isDirectory: &isDir) else { - return nil - } - if isDir.boolValue { return dotGit } + guard let entry = disk.entry(dotGit.path) else { return nil } + if entry.isDirectory { return dotGit } // A worktree's `.git` file: `gitdir: /abs/or/relative/path`. - guard let contents = try? String(contentsOf: dotGit, encoding: .utf8) else { return nil } + guard let contents = disk.text(dotGit.path) else { return nil } let line = contents.split(whereSeparator: \.isNewline).first.map(String.init) ?? "" guard line.hasPrefix("gitdir:") else { return nil } let raw = String(line.dropFirst("gitdir:".count)).trimmingCharacters(in: .whitespaces) @@ -466,14 +472,13 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// `FETCH_HEAD`'s modification time. git rewrites it on **every** fetch, no-ops included (verified), /// so its mtime is a complete record of git fetches — including one the user ran in a terminal. - static func gitLastFetch(commonGitDir: URL?) -> VCSLastFetch { + static func gitLastFetch(commonGitDir: URL?, disk: any RepositoryDisk = LocalDisk()) + -> VCSLastFetch + { guard let dir = commonGitDir else { return .unknown } let head = dir.appendingPathComponent("FETCH_HEAD") - guard FileManager.default.fileExists(atPath: head.path) else { return .never } - guard - let attrs = try? FileManager.default.attributesOfItem(atPath: head.path), - let date = attrs[.modificationDate] as? Date - else { return .unknown } + guard let entry = disk.entry(head.path) else { return .never } + guard let date = entry.modifiedAt else { return .unknown } return .at(date) } @@ -1155,7 +1160,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// that state must offer Abort, not Retry. static func classify( _ result: CommandResult, action: VCSRemoteAction, tool: String, - gitDir: URL? = nil + gitDir: URL? = nil, disk: any RepositoryDisk = LocalDisk() ) -> VCSRemoteFailure? { // Checked BEFORE commandNotFound: launchFailed means the process never ran at all (dominated by // a vanished cwd), which is a different fact from commandNotFound's "env ran and searched PATH". @@ -1176,14 +1181,14 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // It races a host-side git that is still rebasing, and that is accepted here for the reason the // `timedOut` branch already accepts it: a parked rebase the user cannot see is the worse state. if result.exitCode == CommandResult.outcomeUnknown { - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } return .outcomeUnknown(result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) } if result.exitCode == CommandResult.commandNotFound { return .toolMissing(tool) } let err = result.stderr + "\n" + result.stdout // A timed-out pull may have left a rebase behind; that reads better than "timed out". if result.timedOut { - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } return .timedOut(action, err) } guard !result.ok else { return nil } @@ -1224,9 +1229,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { && (err.contains("could not be obtained") || err.contains("File exists") || err.contains("Unable to create"))) { - return .locked(lockFile(in: err)) + return .locked(lockFile(in: err, disk: disk)) } - if action == .pull, rebaseInProgress(gitDir: gitDir) { return .rebaseInProgress } + if action == .pull, rebaseInProgress(gitDir: gitDir, disk: disk) { return .rebaseInProgress } // A lock failure that never names a lock. git's message depends on WHICH internal step hit the lock: // a fast-forward pull reports `Unable to create '': File exists.`, but a pull that must really // rebase fails in autostash first and says only `error: could not write index` / `fatal: Cannot @@ -1236,7 +1241,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // So when the symptoms are lock-shaped, ask the DISK instead of the message. Deliberately last: any // failure git explains properly keeps its own classification, and this only speaks for the ones it // doesn't. - if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir) { return .locked(lock) } + if lockSymptom(err), let lock = existingLockFile(gitDir: gitDir, disk: disk) { + return .locked(lock) + } let trimmed = err.trimmingCharacters(in: .whitespacesAndNewlines) // Killed rather than finished, with nothing to say for itself: `exitCode` is the SIGNAL number, // so the fallback below would render "git exited 15" — literally the dialog `a64e4269` ("stop @@ -1306,7 +1313,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// commit. The reachable case is a `post-commit` hook: git has already written the commit and /// moved `HEAD` by the time it runs, so a hook that fails or runs past the timeout leaves a /// perfectly good commit behind a non-zero exit. - static func classifyCommit(_ result: CommandResult, tool: String) -> VCSCommitFailure? { + static func classifyCommit( + _ result: CommandResult, tool: String, disk: any RepositoryDisk = LocalDisk() + ) -> VCSCommitFailure? { if result.exitCode == CommandResult.launchFailed { return .launchFailed } // See `classify`. if result.exitCode == CommandResult.refused { return .other(result.stderr) } @@ -1347,7 +1356,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { && (err.contains("could not be obtained") || err.contains("File exists") || err.contains("Unable to create"))) { - return .locked(lockFile(in: err)) + return .locked(lockFile(in: err, disk: disk)) } let trimmed = err.trimmingCharacters(in: .whitespacesAndNewlines) // Same reasoning as `classify`: a signal number is not an exit status, so don't print it as one. @@ -1357,11 +1366,10 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// Whether a rebase is parked in this worktree. Both directory names are checked: `rebase-merge` for /// an interactive/merge rebase, `rebase-apply` for the am-based one. - static func rebaseInProgress(gitDir: URL?) -> Bool { + static func rebaseInProgress(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) -> Bool { guard let gitDir else { return false } - let fm = FileManager.default - return fm.fileExists(atPath: gitDir.appendingPathComponent("rebase-merge").path) - || fm.fileExists(atPath: gitDir.appendingPathComponent("rebase-apply").path) + return disk.entry(gitDir.appendingPathComponent("rebase-merge").path) != nil + || disk.entry(gitDir.appendingPathComponent("rebase-apply").path) != nil } /// Which multi-step git operation is parked in this worktree, named for the user, or nil if none. @@ -1373,24 +1381,24 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// /// Ordered by specificity: a cherry-pick and a revert both also leave `MERGE_HEAD` behind, so they /// are tested first or every one of them would report "merge". - static func sequencerState(gitDir: URL?) -> String? { + static func sequencerState(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) -> String? { guard let gitDir else { return nil } - let fm = FileManager.default - let markers: [(String, String)] = [ - ("CHERRY_PICK_HEAD", "cherry-pick"), - ("REVERT_HEAD", "revert"), - ("rebase-merge", "rebase"), - ("rebase-apply", "rebase"), - ("BISECT_LOG", "bisect"), - ("MERGE_HEAD", "merge"), - ] - for (file, label) in markers - where fm.fileExists(atPath: gitDir.appendingPathComponent(file).path) { + for (file, label) in sequencerMarkers + where disk.entry(gitDir.appendingPathComponent(file).path) != nil { return label } return nil } + static let sequencerMarkers: [(String, String)] = [ + ("CHERRY_PICK_HEAD", "cherry-pick"), + ("REVERT_HEAD", "revert"), + ("rebase-merge", "rebase"), + ("rebase-apply", "rebase"), + ("BISECT_LOG", "bisect"), + ("MERGE_HEAD", "merge"), + ] + /// The lock file a failure is complaining about, or nil if we can't point at one. /// /// The path is taken from git's OWN message rather than guessed: git names the exact file it couldn't @@ -1401,12 +1409,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// Returns nil for jj's import/export lock, whose message carries no path, and — deliberately — when /// the named file no longer exists: a lock that cleared between the failure and this check WAS /// transient contention, which is exactly the case where Retry is the right offer. - static func lockFile(in stderr: String) -> VCSLockFile? { - guard let path = parseLockPath(stderr) else { return nil } - guard let attrs = try? FileManager.default.attributesOfItem(atPath: path), - let modified = attrs[.modificationDate] as? Date - else { return nil } - return VCSLockFile(path: path, modifiedAt: modified) + static func lockFile(in stderr: String, disk: any RepositoryDisk = LocalDisk()) -> VCSLockFile? { + guard let path = parseLockPath(stderr), let modified = disk.entry(path)?.modifiedAt else { + return nil + } + return VCSLockFile(path: path, modifiedAt: modified, isOnThisMac: disk is LocalDisk) } /// Whether a failure LOOKS like it was caused by a lock, without saying so. @@ -1426,25 +1433,43 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// The first lock file actually present in this repo, checking both the worktree's git dir and the /// common one — a workroom is a `git worktree`, so its `index.lock` and the repo's `packed-refs.lock` /// are in different directories. - static func existingLockFile(gitDir: URL?) -> VCSLockFile? { - guard let gitDir else { return nil } - let fm = FileManager.default + static func existingLockFile(gitDir: URL?, disk: any RepositoryDisk = LocalDisk()) + -> VCSLockFile? + { + for candidate in lockCandidates(gitDir: gitDir) { + guard let modified = disk.entry(candidate.path)?.modifiedAt else { continue } + return VCSLockFile( + path: candidate.path, modifiedAt: modified, isOnThisMac: disk is LocalDisk) + } + return nil + } + + /// Where a lock file can be, in the order `existingLockFile` checks. + static func lockCandidates(gitDir: URL?) -> [URL] { + guard let gitDir else { return [] } var dirs = [gitDir] // `/worktrees/` → ``. Cheap and string-only; `commonGitDir` does the same trip // from a path rather than from an already-resolved git dir. if gitDir.deletingLastPathComponent().lastPathComponent == "worktrees" { dirs.append(gitDir.deletingLastPathComponent().deletingLastPathComponent()) } - for dir in dirs { - for name in knownLockNames { - let candidate = dir.appendingPathComponent(name) - guard let attrs = try? fm.attributesOfItem(atPath: candidate.path), - let modified = attrs[.modificationDate] as? Date - else { continue } - return VCSLockFile(path: candidate.path, modifiedAt: modified) - } + return dirs.flatMap { dir in knownLockNames.map { dir.appendingPathComponent($0) } } + } + + /// Every path the classifier can read for a command run in the worktree whose git directories + /// these are, and whose stderr this is: the set a remote host's `stat` must report, so a + /// `DiskSnapshot` answers each probe above. A probe that reads a path not listed here traps in + /// a debug build. + static func classificationPaths(gitDir: URL?, commonGitDir: URL?, stderr: String) -> [String] { + var paths: [URL] = lockCandidates(gitDir: gitDir) + if let gitDir { + paths += sequencerMarkers.map { gitDir.appendingPathComponent($0.0) } } - return nil + if let commonGitDir { paths.append(commonGitDir.appendingPathComponent("FETCH_HEAD")) } + var unique = paths.map(\.path) + if let lock = parseLockPath(stderr) { unique.append(lock) } + var seen = Set() + return unique.filter { seen.insert($0).inserted } } /// The quoted path out of git's lock errors. Pure, so the parsing is testable without a repo. @@ -1466,14 +1491,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { /// This worktree's OWN git directory — `/worktrees/` for a workroom, the same as the /// common dir for a project root. Rebase state (`rebase-merge`), `HEAD` and `index` live here, unlike /// `FETCH_HEAD` which `commonGitDir` deliberately resolves to the shared copy. - static func worktreeGitDir(at path: String) -> URL? { + static func worktreeGitDir(at path: String, disk: any RepositoryDisk = LocalDisk()) -> URL? { let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git") - var isDir: ObjCBool = false - guard FileManager.default.fileExists(atPath: dotGit.path, isDirectory: &isDir) else { - return nil - } - if isDir.boolValue { return dotGit } - guard let contents = try? String(contentsOf: dotGit, encoding: .utf8) else { return nil } + guard let entry = disk.entry(dotGit.path) else { return nil } + if entry.isDirectory { return dotGit } + guard let contents = disk.text(dotGit.path) else { return nil } let line = contents.split(whereSeparator: \.isNewline).first.map(String.init) ?? "" guard line.hasPrefix("gitdir:") else { return nil } let raw = String(line.dropFirst("gitdir:".count)).trimmingCharacters(in: .whitespaces) @@ -1504,7 +1526,72 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { private func gated( _ projectRoot: String, _ body: @Sendable @escaping () async -> T ) async -> T? { - try? await gate.run(projectRoot: projectRoot, body) + guard case .remote(let id) = host else { + return try? await gate.run(projectRoot: projectRoot, body) + } + // Keyed as the remote location it is: validated, not resolved against this disk. The gate takes + // no process barrier for it, having none to take on another host (`JJProcessBarrier`); the + // agent takes jj's for each command instead (`AgentCommandRunner.barrierRoot`). + guard let repository = try? RepositoryLocation.remote(host: id, path: projectRoot) else { + return nil + } + return try? await gate.run(repository: repository, body) + } + + // MARK: - Disk facts + + /// The disk a classification reads, and the worktree's git directory on it, read AFTER the command + /// it explains: a parked rebase or a leftover lock exists only once that command has failed. + /// + /// Locally, this Mac's disk, live. On a remote host, two round trips to its agent: the worktree's + /// `.git` says where its git directories are, then everything in them at once. A host that cannot + /// be asked reads as no evidence (`DiskSnapshot.unknown`), as a missing file would. + /// + /// The git directories come from the FIRST read, and are what the second was built for: resolved + /// again from the second, a `.git` that changed in between would name paths it never asked about. + private func disk(at path: String, stderr: String = "") async -> ( + disk: any RepositoryDisk, gitDir: URL?, commonGitDir: URL? + ) { + guard let stat else { + return (LocalDisk(), Self.worktreeGitDir(at: path), Self.commonGitDir(at: path)) + } + let dotGit = URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(".git").path + guard let pointer = try? await stat([dotGit], [dotGit]) else { + return (DiskSnapshot.unknown, nil, nil) + } + let gitDir = Self.worktreeGitDir(at: path, disk: pointer) + let commonGitDir = Self.commonGitDir(at: path, disk: pointer) + let paths = + [dotGit] + + Self.classificationPaths(gitDir: gitDir, commonGitDir: commonGitDir, stderr: stderr) + guard let facts = try? await stat(paths, [dotGit]) else { + return (DiskSnapshot.unknown, gitDir, commonGitDir) + } + return (facts, gitDir, commonGitDir) + } + + private func rebaseParked(at path: String) async -> Bool { + let facts = await disk(at: path) + return Self.rebaseInProgress(gitDir: facts.gitDir, disk: facts.disk) + } + + private func parkedOperation(at path: String) async -> String? { + let facts = await disk(at: path) + return Self.sequencerState(gitDir: facts.gitDir, disk: facts.disk) + } + + /// `Self.classify`, reading the disk of wherever this repository lives. `withGitDir: false` for a + /// command whose classification never consulted the worktree's git directory. + private func classify( + _ result: CommandResult, action: VCSRemoteAction, tool: String, at path: String, + withGitDir: Bool = true + ) async -> VCSRemoteFailure? { + // Nothing on disk explains a success, so a remote host is not asked about one. + if result.ok { return Self.classify(result, action: action, tool: tool) } + let facts = await disk(at: path, stderr: result.stderr + "\n" + result.stdout) + return Self.classify( + result, action: action, tool: tool, gitDir: withGitDir ? facts.gitDir : nil, + disk: facts.disk) } // MARK: - Remote state @@ -1524,7 +1611,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { private func gitRemoteState(path: String, current: VCSRef) async -> VCSRemoteResolution { let refs = await run(Self.gitRemoteRefsArgs(), in: path, timeout: refTimeout) - if let failure = Self.classify(refs, action: .fetch, tool: "git") { + if let failure = await classify( + refs, action: .fetch, tool: "git", at: path, withGitDir: false) + { // A blip must not blank a good toolbar; a missing tool or a real error should show. if case .timedOut = failure { return .keepPrior } return .failed(failure) @@ -1553,10 +1642,11 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { tracking = VCSTracking(comparedTo: counterpart, ahead: nil, behind: nil, gone: true) } } + let facts = await disk(at: path) return .state( VCSRemoteState( current: current, tracking: tracking, remotes: remotes, primaryRemote: primary, - lastFetch: Self.gitLastFetch(commonGitDir: Self.commonGitDir(at: path)), + lastFetch: Self.gitLastFetch(commonGitDir: facts.commonGitDir, disk: facts.disk), resolvedAt: Date())) } @@ -1565,7 +1655,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { { // Reads take no lock, so they stay ungated. let list = await run(Self.jjBookmarkListArgs(), in: path, timeout: refTimeout) - if let failure = Self.classify(list, action: .fetch, tool: "jj") { + if let failure = await classify(list, action: .fetch, tool: "jj", at: path, withGitDir: false) { if case .timedOut = failure { return .keepPrior } return .failed(failure) } @@ -1663,9 +1753,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: fetchTimeout, network: true) }) else { return .failed(.other("fetch was cancelled")) } - if let failure = Self.classify( - result, action: .fetch, tool: vcs, gitDir: Self.worktreeGitDir(at: path)) - { + if let failure = await classify(result, action: .fetch, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Fetched \(remote)") @@ -1697,9 +1785,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: pushTimeout, network: true) }) else { return .failed(.other("push was cancelled")) } - if let failure = Self.classify( - result, action: .push, tool: vcs, gitDir: Self.worktreeGitDir(at: path)) - { + if let failure = await classify(result, action: .push, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Pushed to \(remote)") @@ -1712,8 +1798,6 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { // Both backends fetch first, at the project root, for the reasons `opDirectory` documents. For git // `pull` would fetch too, but doing it explicitly at the root keeps `FETCH_HEAD` — and so the // "last fetched" label — correct for every workroom of the project. - // Hoisted above the fetch: BOTH steps classify against it, since a lock can block either one. - let gitDir = Self.worktreeGitDir(at: path) let fetchDir = Self.opDirectory(.fetch, path: path, projectRoot: projectRoot) let fetchArgs = vcs == "jj" ? Self.jjFetchArgs(remote: remote) : Self.gitFetchArgs(remote: remote) @@ -1724,7 +1808,8 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(fetchArgs, in: fetchDir, timeout: fetchTimeout, network: true) }) else { return .failed(.other("pull was cancelled")) } - if let failure = Self.classify(fetched, action: .pull, tool: vcs, gitDir: gitDir) { + // Both steps classify against the worktree's git directory, since a lock can block either one. + if let failure = await classify(fetched, action: .pull, tool: vcs, at: path) { return .failed(failure) } @@ -1750,7 +1835,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { await run(args, in: dir, timeout: pullTimeout, network: true) }) else { return .failed(.other("pull was cancelled")) } - if let failure = Self.classify(result, action: .pull, tool: vcs, gitDir: gitDir) { + if let failure = await classify(result, action: .pull, tool: vcs, at: path) { return .failed(failure) } return .ok(summary: "Pulled from \(remote)") @@ -1766,7 +1851,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { } func abortRebase(path: String, projectRoot: String) async -> VCSRemoteActionResult { - if vcs == "jj", !Self.rebaseInProgress(gitDir: Self.worktreeGitDir(at: path)) { + if vcs == "jj", !(await rebaseParked(at: path)) { // jj's own rebase is atomic and undoable — there is no half-finished state to abort. But a // COLOCATED jj root still has a real `.git`, and a `git rebase` run in that root's terminal // can leave a real `rebase-merge`/`rebase-apply` behind — exactly what `classify` checks for @@ -1790,7 +1875,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { else { return .failed(.other("abort was cancelled")) } // Always "git" here too, never `vcs`, for the same reason: attributing a failure to the wrong // tool would misdirect ".toolMissing"/"exited N" messages. - if let failure = Self.classify(result, action: .abortRebase, tool: "git") { + if let failure = await classify( + result, action: .abortRebase, tool: "git", at: path, withGitDir: false) + { return .failed(failure) } return .ok(summary: "Rebase aborted") @@ -1801,10 +1888,9 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { func commit(path: String, projectRoot: String, request: VCSCommitRequest) async -> VCSCommitResult { guard Self.supports(mode: request.mode, vcs: vcs) else { return .failed(.unsupportedMode) } - let gitDir = Self.worktreeGitDir(at: path) // A parked merge/cherry-pick/rebase/bisect makes a path-limited commit outright invalid, and // finishing it is the user's call. Checked before the ref snapshot so nothing is spawned at all. - if vcs != "jj", let sequencer = Self.sequencerState(gitDir: gitDir) { + if vcs != "jj", let sequencer = await parkedOperation(at: path) { return .failed(.sequencerInProgress(sequencer)) } @@ -1828,7 +1914,15 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { guard let attempt = outcome else { return .failed(.other("commit was cancelled")) } let (before, result) = (attempt.before, attempt.result) - if let failure = Self.classifyCommit(result, tool: vcs) { + // Read only for a failure, as `classify` does: jj's "Nothing changed." exits 0 and is classified + // here too, but no probe reads the disk for it. + let commitDisk: any RepositoryDisk = + stat == nil + ? LocalDisk() + : result.ok + ? DiskSnapshot.unknown + : await disk(at: path, stderr: result.stderr + "\n" + result.stdout).disk + if let failure = Self.classifyCommit(result, tool: vcs, disk: commitDisk) { // The command failed, but did the ref move anyway? A `post-commit` hook runs AFTER git has // written the commit and moved HEAD, so a hook that fails — or that we killed at the timeout — // leaves a real commit behind a non-zero exit. Reporting that as a plain failure invites a @@ -2012,7 +2106,7 @@ struct CLIVCSWriter: LocalVCSWriting, Sendable { let subject = head.ok ? head.stdout.trimmingCharacters(in: .whitespacesAndNewlines) : "" return VCSCommitPreflight( - sequencer: Self.sequencerState(gitDir: Self.worktreeGitDir(at: path)), + sequencer: await parkedOperation(at: path), // Empty on an unborn branch — a repo with no commits has nothing to amend. amendTarget: subject.isEmpty ? nil : subject, currentMessage: nil) diff --git a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift index ffd1126df..19e035252 100644 --- a/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSIntegrationTests.swift @@ -555,6 +555,45 @@ final class AgentVCSIntegrationTests: XCTestCase { /// `exec_never_contends_with_a_held_snapshot_barrier` for the Rust-side proof of the same /// property. If this ever regressed, the commit below would hang for up to 30s and fail the test /// on timeout instead of completing. + /// A remote host's jj writes run under the working-copy barrier, which the agent takes around each + /// command because a Mac cannot flock a file on another host (#229). Driven through a local agent + /// under a remote host id, which takes that path end to end: while this test holds the barrier the + /// commit waits, and it lands once the barrier is released. + func testARemoteJJCommitWaitsOnTheBarrierTheAgentTakes() async throws { + let root = try root() + try run("jj", ["git", "init", "--colocate"], at: root) + try "base\n".write(to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + try run("jj", ["commit", "-m", "initial"], at: root) + try "changed\n".write( + to: root.appendingPathComponent("file"), atomically: true, encoding: .utf8) + let hostID = UUID() + let host = HostID.remote(hostID) + let manager = HostConnectionManager() + let connection = try await connect(host: host) + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: hostID, path: root.path) + try router.register(.init(location: location, backend: .jj, sharedLocation: location)) + let writer = try await router.writer(for: location) + + let local = try await RepositoryLocation.local(root.path) + let held = try XCTUnwrap(JJProcessBarrier.acquire(local)) + let release = Task { + try? await Task.sleep(for: .milliseconds(1500)) + held.release() + } + let started = ContinuousClock.now + let result = await writer.commit( + request: VCSCommitRequest(message: "remote jj commit", files: [], mode: .commit)) + let waited = ContinuousClock.now - started + await release.value + guard case .ok = result else { return XCTFail("jj commit failed: \(result)") } + XCTAssertGreaterThanOrEqual(waited, .milliseconds(1200), "the commit ran without the barrier") + let log = try run("jj", ["log", "--no-graph", "-r", "@-", "-T", "description"], at: root) + XCTAssertTrue(log.contains("remote jj commit"), log) + await connection.close() + } + func testJJCommitRoutesThroughTheAgentWithoutContendingItsOwnSnapshotBarrier() async throws { let root = try root() try run("jj", ["git", "init", "--colocate"], at: root) diff --git a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift index 9d3bf2618..fb1a18142 100644 --- a/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift +++ b/macapp/WorkroomAppTests/AgentVCSProtocolTests.swift @@ -260,6 +260,51 @@ final class AgentVCSProtocolTests: XCTestCase { XCTAssertEqual(request.kind, "exec") } + /// Nothing from the Mac's environment reaches a remote host's exec request (#229): its `HOME` and + /// `PATH` name nothing there, and its `SSH_AUTH_SOCK` is a Mac credential. Checked on the encoded + /// wire bytes, which are what crosses. + func testARemoteExecRequestCarriesNothingFromTheMacsEnvironment() throws { + let encoder = JSONEncoder() + encoder.keyEncodingStrategy = .convertToSnakeCase + for network in [false, true] { + let local = AgentCommandRunner.request( + "git", ["push"], in: "/r", timeout: 1, stdin: nil, network: network, host: .local) + // The control: locally the Mac's environment is the point. + XCTAssertNotNil(local.env["PATH"]) + let localWire = String(decoding: try encoder.encode(local), as: UTF8.self) + // An agent that predates the field rejects a request carrying it, and a local one may. + XCTAssertFalse(localWire.contains("host_environment"), localWire) + + let remote = AgentCommandRunner.request( + "git", ["push"], in: "/r", timeout: 1, stdin: nil, network: network, + host: .remote(UUID())) + XCTAssertEqual(remote.env, StatusCommandRunner.remoteEnvironment) + let wire = String(decoding: try encoder.encode(remote), as: UTF8.self) + for key in ["HOME", "PATH", "SSH_AUTH_SOCK", "USER", "GIT_SSH_COMMAND"] { + XCTAssertFalse(wire.contains("\"\(key)\""), "\(key) crossed (network: \(network)): \(wire)") + } + XCTAssertTrue(wire.contains("\"host_environment\":true"), wire) + } + // The jj barrier the agent takes for a remote command must never be asked of a local agent, + // whose caller already holds it for the whole operation: taken again, it would deadlock. + let local = AgentCommandRunner.request( + "jj", ["commit"], in: "/r", timeout: 1, stdin: nil, network: false, host: .local, + barrierRoot: "/r") + XCTAssertFalse( + String(decoding: try encoder.encode(local), as: UTF8.self).contains("barrier_root")) + let remote = AgentCommandRunner.request( + "jj", ["commit"], in: "/r", timeout: 1, stdin: nil, network: false, host: .remote(UUID()), + barrierRoot: "/r") + XCTAssertTrue( + String(decoding: try encoder.encode(remote), as: UTF8.self).contains( + "\"barrier_root\":\"\\/r\"")) + // Nor for a network command, whose detached git helpers would inherit it. + let fetch = AgentCommandRunner.request( + "jj", ["git", "fetch"], in: "/r", timeout: 1, stdin: nil, network: true, + host: .remote(UUID()), barrierRoot: "/r") + XCTAssertNil(fetch.barrierRoot) + } + // MARK: File service (#211) /// The client's five version constants are the agent's `PROTOCOL_VERSION`, `MIN_VCS_VERSION`, diff --git a/macapp/WorkroomAppTests/HostStreamTests.swift b/macapp/WorkroomAppTests/HostStreamTests.swift new file mode 100644 index 000000000..f955c9a94 --- /dev/null +++ b/macapp/WorkroomAppTests/HostStreamTests.swift @@ -0,0 +1,220 @@ +import Darwin +import XCTest + +@testable import Workroom + +/// A driver's stream with no ssh in it: `wr-agent relay` to a local agent, carried over a +/// socketpair exactly as ssh carries it. Runs wherever `make app-test` does, so CI covers the +/// transport's plumbing; the ssh leg is `RemoteHostIntegrationTests`, against the container. +final class HostStreamTests: XCTestCase { + private var agents: [AgentHarness] = [] + + override func tearDown() { + for agent in agents { agent.stop() } + agents.removeAll() + super.tearDown() + } + + private func agent() throws -> AgentHarness { + let agent = try AgentHarness.start() + agents.append(agent) + return agent + } + + private func relay(to socket: String) throws -> HostStream { + try HostStream.spawn( + try AgentHarness.binaryURL(), ["relay", "--socket", socket], environment: [:], + handshakeTimeout: 5) + } + + private func eventually(_ condition: () async -> Bool) async -> Bool { + for _ in 0..<100 { + if await condition() { return true } + try? await Task.sleep(for: .milliseconds(50)) + } + return await condition() + } + + private static func exited(_ pid: Int32) -> Bool { kill(pid, 0) == -1 && errno == ESRCH } + + func testRequestsAnswerThroughARelayedStream() async throws { + let socket = try agent().socketPath + let host = HostID.remote(UUID()) + let connection = try await AgentVCSConnection.connect(host: host, stream: try relay(to: socket)) + // `connect` already negotiated; this proves the stream keeps carrying traffic after it. + let reply = try await connection.request(AgentVCSRequest(method: "capabilities"), timeout: 5) + XCTAssertEqual(try AgentVCSReply.decode(reply).version, 1) + await connection.close() + } + + /// The carrier dying is the link dying: the manager must see the connection go, with no request + /// in flight to notice it. Without the parent closing its copy of the child's end, the pair never + /// reads EOF and this waits forever. + func testACarrierThatDiesIsALostConnection() async throws { + let socket = try agent().socketPath + let host = HostID.remote(UUID()) + let stream = try relay(to: socket) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { + try await AgentVCSConnection.connect(host: host, stream: stream) + } + let connected = await manager.snapshot(for: host).status + XCTAssertEqual(connected, .connected) + kill(stream.processIdentifier, SIGKILL) + let lost = await eventually { await manager.snapshot(for: host).status == .disconnected } + XCTAssertTrue(lost, "the manager never noticed the carrier die") + } + + /// A closed connection must not leave its ssh behind. + func testClosingTheConnectionEndsTheCarrier() async throws { + let socket = try agent().socketPath + let stream = try relay(to: socket) + let connection = try await AgentVCSConnection.connect(host: .remote(UUID()), stream: stream) + let pid = stream.processIdentifier + await connection.close() + let ended = await eventually { Self.exited(pid) } + XCTAssertTrue(ended, "the carrier outlived its connection") + } + + /// No agent is the relay's own diagnosis, surfaced as it said it: nothing on a remote host can + /// be respawned, so `connectionLost` (which `LocalAgentVCS` answers by spawning an agent) would + /// be the wrong error and would lose the reason. + func testARelayWithNoAgentFailsWithItsOwnReason() async throws { + let missing = "/tmp/wr-none-\(UUID().uuidString.prefix(8)).sock" + let stream = try relay(to: missing) + do { + _ = try await AgentVCSConnection.connect(host: .remote(UUID()), stream: stream) + XCTFail("connected to nothing") + } catch HostConnectionError.serviceUnavailable(let detail) { + XCTAssertTrue(detail.contains("no agent listening"), detail) + } + let ended = await eventually { Self.exited(stream.processIdentifier) } + XCTAssertTrue(ended) + } + + func testTheSSHConfigurationCannotPromptOrForwardAnything() throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let host = ContainerHostDriver.Host( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAAC3Nza", agentSocket: "/run/workroom/agent.sock") + let config = try String( + contentsOf: ContainerHostDriver.writeConfiguration(for: host, in: directory), + encoding: .utf8) + for line in [ + "BatchMode yes", "StrictHostKeyChecking yes", "GlobalKnownHostsFile /dev/null", + "IdentitiesOnly yes", "IdentityAgent none", + "ForwardAgent no", "ClearAllForwardings yes", "ServerAliveInterval 15", + "EscapeChar none", + ] { + XCTAssertTrue(config.contains(" \(line)\n"), "missing \(line)") + } + XCTAssertFalse(config.contains("SendEnv")) + let known = try String( + contentsOf: directory.appendingPathComponent("known_hosts"), encoding: .utf8) + XCTAssertEqual(known, "[127.0.0.1]:2222 ssh-ed25519 AAAAC3Nza\n") + } + + /// A value is written into `ssh_config`, where a newline would start a directive of its own. + func testAConfigurationValueCannotInjectADirective() { + let host = ContainerHostDriver.Host( + address: "127.0.0.1\n ProxyCommand touch /tmp/owned", port: 22, user: "workroom", + identityFile: "/keys/id", hostKey: "ssh-ed25519 AAAA", agentSocket: "/run/a.sock") + XCTAssertThrowsError( + try ContainerHostDriver.writeConfiguration( + for: host, in: FileManager.default.temporaryDirectory)) + } + + /// A remote pane's command (#229): the session contract rides in `env` on the host, and the + /// Mac-only parts of it (the shell, the app bundle's resources) stay behind. + func testARemotePanesCommandCarriesTheSessionAndNothingOfTheMacs() throws { + let session = UUID() + let fresh = ContainerHostDriver.remoteAttachCommand( + session: session, socket: "/run/workroom/agent.sock", + workingDirectory: "/home/w/it's here", restored: false) + XCTAssertEqual( + fresh, + "'env' 'TERM=xterm-256color' 'WORKROOM_SESSION_ID=\(session.uuidString)' " + + "'WORKROOM_SESSION_SOCKET=/run/workroom/agent.sock' " + + "'WORKROOM_SESSION_CWD=/home/w/it'\\''s here' 'wr-agent' 'attach' '--no-spawn'") + let restored = ContainerHostDriver.remoteAttachCommand( + session: session, socket: "/s", workingDirectory: "/w", restored: true) + XCTAssertTrue(restored.hasSuffix("'--no-spawn' '--no-create'"), restored) + for absent in ["WORKROOM_SESSION_SHELL", "WORKROOM_SESSION_RESOURCES", "AWAKE"] { + XCTAssertFalse(fresh.contains(absent), absent) + } + + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let id = UUID() + let driver = ContainerHostDriver( + hosts: [ + id: .init( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAA", agentSocket: "/s") + ], directory: directory) + let command = try driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/w", restored: true) + XCTAssertTrue(command.hasPrefix("'/usr/bin/ssh' '-F' "), command) + XCTAssertTrue(command.contains(" '-t' 'workroom-host' "), command) + } + + /// A remote session is routed to its driver, not to a local helper: its command is the driver's, + /// the pane's own environment carries no session variables, and a restored pane is not asked + /// about locally (the host's agent answers that, in the attach). + @MainActor + func testARemoteSessionIsRoutedToItsDriverNotALocalHelper() async throws { + let service = PersistentSessionService( + probe: { _ in .unhealthy(reason: "none here") }, + ownership: { _ in + XCTFail("a local helper was asked about a remote session") + return .notOwned + }) + let directory = FileManager.default.temporaryDirectory.appendingPathComponent( + UUID().uuidString) + defer { try? FileManager.default.removeItem(at: directory) } + let host = UUID() + let driver = ContainerHostDriver( + hosts: [ + host: .init( + address: "127.0.0.1", port: 2222, user: "workroom", identityFile: "/keys/id", + hostKey: "ssh-ed25519 AAAA", agentSocket: "/s") + ], directory: directory) + let session = UUID() + service.registerRemoteSession( + session, on: .remote(host), via: driver, workingDirectory: "/home/w") + + XCTAssertEqual(service.confirmBeforeAttach(sessionID: session, wasRestored: true), .attachable) + let command = try XCTUnwrap(service.attachCommand(forSession: session, restored: true)) + XCTAssertTrue(command.contains("/usr/bin/ssh"), command) + XCTAssertTrue(command.contains("--no-create"), command) + XCTAssertTrue(service.launchEnvironment(sessionID: session, workingDirectory: "/w").isEmpty) + + // A host the driver cannot reach gets a pane that says so, never a plain shell on this Mac. + let stranded = UUID() + service.registerRemoteSession( + stranded, on: .remote(UUID()), via: driver, workingDirectory: "/home/w") + let notice = try XCTUnwrap(service.attachCommand(forSession: stranded)) + XCTAssertTrue(notice.hasPrefix("/bin/sh -c "), notice) + XCTAssertTrue(notice.contains("Could not reach this terminal"), notice) + + // Closing it reaches no local helper (the ownership closure above fails the test if asked), + // and reports it not killed, which is true. + let killed = await service.endSession(sessionID: session) + XCTAssertFalse(killed) + // Still remote: the session runs on, and a retry or a reattach must not go local. + XCTAssertTrue(service.isRemote(session)) + let again = await service.endSession(sessionID: session) + XCTAssertFalse(again) + XCTAssertTrue( + service.attachCommand(forSession: session, restored: true)?.contains("/usr/bin/ssh") == true) + } + + func testTheRelayCommandQuotesItsSocketForTheRemoteShell() { + XCTAssertEqual( + ContainerHostDriver.relayCommand(socket: "/run/it's here/a.sock"), + "wr-agent relay --socket '/run/it'\\''s here/a.sock'") + } +} diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift new file mode 100644 index 000000000..de033aa46 --- /dev/null +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -0,0 +1,339 @@ +import Darwin +import XCTest + +@testable import Workroom + +/// The app's half of the remote path against a real host (#229): `ContainerHostDriver` over ssh +/// into the container fixture, driven by the real Swift clients. Skipped unless run through the +/// fixture script, which starts the container and says where it is: +/// +/// vcs/scripts/ssh-fixture/run.sh \ +/// make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/RemoteHostIntegrationTests +/// +/// The macOS CI runners have no Docker, so CI covers the stream without ssh (`HostStreamTests`) +/// and the ssh leg from Rust (`remote_transport.rs`, on the Linux runner). +final class RemoteHostIntegrationTests: XCTestCase { + private var directory: URL! + private var connections: [AgentVCSConnection] = [] + + override func setUp() { + super.setUp() + directory = FileManager.default.temporaryDirectory.appendingPathComponent( + "wr-remote-\(UUID().uuidString.prefix(8))") + } + + override func tearDown() async throws { + for connection in connections { await connection.close() } + connections.removeAll() + try? FileManager.default.removeItem(at: directory) + try await super.tearDown() + } + + private struct Fixture { + let config: String + let host: ContainerHostDriver.Host + } + + private func fixture() throws -> Fixture { + let environment = ProcessInfo.processInfo.environment + func need(_ name: String) throws -> String { + guard let value = environment[name], !value.isEmpty else { + throw XCTSkip("\(name) is unset; run these through vcs/scripts/ssh-fixture/run.sh") + } + return value + } + return Fixture( + config: try need("WR_SSH_FIXTURE_CONFIG"), + host: ContainerHostDriver.Host( + address: try need("WR_SSH_FIXTURE_ADDRESS"), + port: Int(try need("WR_SSH_FIXTURE_PORT")) ?? 0, + user: try need("WR_SSH_FIXTURE_USER"), + identityFile: try need("WR_SSH_FIXTURE_IDENTITY"), + hostKey: try need("WR_SSH_FIXTURE_HOST_KEY"), + agentSocket: try need("WR_SSH_FIXTURE_SOCKET"))) + } + + private func connect(_ host: ContainerHostDriver.Host) async throws -> ( + AgentVCSConnection, UUID + ) { + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: host], directory: directory) + let connection = try await AgentVCSConnection.connect( + host: .remote(id), stream: try await driver.openStream(to: .remote(id))) + connections.append(connection) + return (connection, id) + } + + /// Runs `command` on the fixture over the script's own ssh, to set the host up. Not through the + /// driver, so a setup step can never be the thing under test. + @discardableResult + private func onHost(_ fixture: Fixture, _ command: String) throws -> String { + let process = Process() + process.executableURL = URL(fileURLWithPath: "/usr/bin/ssh") + process.arguments = ["-F", fixture.config, "fixture", command] + let output = Pipe() + process.standardOutput = output + process.standardError = output + try process.run() + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + let text = String(decoding: data, as: UTF8.self) + guard process.terminationStatus == 0 else { + XCTFail("on the host, \(command) failed: \(text)") + throw VCSError.io(text) + } + return text.trimmingCharacters(in: .whitespacesAndNewlines) + } + + /// A git repository on the host, with its identity in the repository's own config: the host has + /// no global one, and nothing from the Mac supplies it. + private func repository(_ fixture: Fixture) throws -> String { + let path = "/home/workroom/repo-\(UUID().uuidString.prefix(8))" + try onHost( + fixture, + """ + set -e; git init -q -b main \(path); cd \(path) + git config user.name Remote; git config user.email remote@example.com + echo base > file; git add .; git commit -qm initial + """) + return path + } + + /// VCS reads and writes, File, Status and Forward, all through one ssh-stdio connection and the + /// same manager and router the app uses. + func testEveryServiceAnswersThroughTheDriversStream() async throws { + let fixture = try fixture() + let path = try repository(fixture) + let (connection, id) = try await connect(fixture.host) + let host = HostID.remote(id) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: id, path: path) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + + // VCS reads. + let reader = try await router.reader(for: location) + let ref = try await reader.currentRef() + XCTAssertEqual(ref, VCSRef(name: "main", kind: .branch)) + let page = try await reader.log(limit: 5) + XCTAssertEqual(page.commits.map(\.summary), ["initial"]) + + // A VCS write, through the exec service in the host's environment. + try onHost(fixture, "echo next > \(path)/file") + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "remote commit", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], + mode: .commit)) + guard case .ok = result else { return XCTFail("commit failed: \(result)") } + XCTAssertEqual( + try onHost(fixture, "git -C \(path) log -1 --format='%s %ae'"), + "remote commit remote@example.com") + + // File. + let files = try connection.files( + context: FileContext(location: location, sharedLocation: nil)) + let listing = try await files.list(.git) + XCTAssertEqual(FileListing.parse(listing.stdout, vcs: .git), ["file"]) + let data = try await files.read(path: "file", symlinks: .refuse, maxBytes: 100) + XCTAssertEqual(data, Data("next\n".utf8)) + + // Status. + _ = try await connection.wakefulness().status() + + // Forward, to a process listening in the container: its own sshd, which speaks first. + let forward = try connection.forwarding().listen(remotePort: 22) { _ in } + defer { forward.stop() } + let client = try TCPClient(port: forward.localPort) + defer { client.close() } + let banner = String(decoding: try client.read(8, timeout: 10), as: UTF8.self) + XCTAssertEqual(banner, "SSH-2.0-") + } + + /// A failed remote write is classified from the HOST's disk (#229): a leftover `index.lock` there + /// is named by its path on the host. Read from this Mac's disk, that path is not there, and the + /// failure would name no lock at all. + func testARemoteCommitBlockedByALockNamesTheLockOnTheHost() async throws { + let fixture = try fixture() + let path = try repository(fixture) + try onHost(fixture, "echo next > \(path)/file && touch \(path)/.git/index.lock") + let (connection, id) = try await connect(fixture.host) + let host = HostID.remote(id) + let manager = HostConnectionManager() + _ = try await manager.connect(host: host) { connection } + let router = RepositoryRouter(connections: manager) + let location = try RepositoryLocation.remote(host: id, path: path) + try router.register(.init(location: location, backend: .git, sharedLocation: location)) + let writer = try await router.writer(for: location) + let result = await writer.commit( + request: VCSCommitRequest( + message: "blocked", + files: [ChangedFile(path: "file", change: .modified, oldPath: nil)], + mode: .commit)) + guard case .failed(.locked(let lock)) = result else { + return XCTFail("expected a lock failure, got \(result)") + } + XCTAssertEqual(lock?.path, "\(path)/.git/index.lock") + } + + /// A pane's process, running the command the driver hands libghostty, with its output collected. + /// Run through `/bin/sh -c`, as libghostty runs a pane's command. No local terminal, so ssh + /// allocates none on the host either, and the attach there relays over pipes, as the Rust + /// harnesses' do. + private final class Pane: @unchecked Sendable { + let process = Process() + private let input = Pipe() + private let lock = NSLock() + private var seen = "" + + init(command: String) throws { + process.executableURL = URL(fileURLWithPath: "/bin/sh") + process.arguments = ["-c", command] + process.environment = [:] + process.standardInput = input + let output = Pipe() + process.standardOutput = output + process.standardError = output + output.fileHandleForReading.readabilityHandler = { [weak self] handle in + let data = handle.availableData + guard !data.isEmpty else { + handle.readabilityHandler = nil + return + } + self?.lock.withLock { self?.seen += String(decoding: data, as: UTF8.self) } + } + try process.run() + } + + func type(_ text: String) { input.fileHandleForWriting.write(Data(text.utf8)) } + + func read(until needle: String, within seconds: TimeInterval = 20) -> String { + let deadline = Date().addingTimeInterval(seconds) + while Date() < deadline, !lock.withLock({ seen.contains(needle) }) { + Thread.sleep(forTimeInterval: 0.05) + } + return lock.withLock { seen } + } + + /// The link dropping hard: ssh killed, with no goodbye to the host. + func dropLink() { + kill(process.processIdentifier, SIGKILL) + process.waitUntilExit() + } + } + + /// The app half of the Phase 3 acceptance (#229): a pane attached through the driver keeps its + /// session across a dropped link, and the pane that comes back (a reconnect after sleep, or a + /// relaunch) reaches the SAME shell: its pid, and a variable set before the drop. + func testARemotePaneSurvivesADroppedLinkAndReachesTheSameShell() throws { + let fixture = try fixture() + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: fixture.host], directory: directory) + let session = UUID() + + let first = try Pane( + command: driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/home/workroom", restored: false)) + Thread.sleep(forTimeInterval: 1) + // Quotes split each marker, so the terminal echoing the typed line cannot satisfy the wait. + first.type("MARK=kept; echo PID=$$; echo READ\"\"Y\n") + let before = first.read(until: "READY") + let pid = try XCTUnwrap( + before.range(of: #"PID=\d+"#, options: .regularExpression).map { String(before[$0]) }, + before) + first.dropLink() + + let second = try Pane( + command: driver.attachCommand( + to: .remote(id), session: session, workingDirectory: "/home/workroom", restored: true)) + defer { second.dropLink() } + Thread.sleep(forTimeInterval: 1) + second.type("echo VALUE=$MARK PID=$$ DO\"\"NE\n") + let after = second.read(until: "DONE") + XCTAssertTrue(after.contains("VALUE=kept \(pid) DONE"), after) + XCTAssertFalse(after.contains("has ended"), after) + } + + /// A restored pane whose session ended on the host gets a shell that says so, never a fresh + /// session passed off as the old one: the host's agent refuses to create it (`--no-create`). + func testARestoredPaneWhoseSessionEndedGetsAShellThatSaysSo() throws { + let fixture = try fixture() + let id = UUID() + let driver = ContainerHostDriver(hosts: [id: fixture.host], directory: directory) + let pane = try Pane( + command: driver.attachCommand( + to: .remote(id), session: UUID(), workingDirectory: "/home/workroom", restored: true)) + defer { pane.dropLink() } + let seen = pane.read(until: "has ended") + XCTAssertTrue(seen.contains("has ended"), seen) + } + + /// Nothing from the Mac's environment reaches a remote git (#229): the child runs with the + /// host's `HOME` and `PATH`, no `SSH_AUTH_SOCK`, and no way for ssh to prompt. Read back from the + /// child itself, through a git alias that prints its environment. + func testARemoteCommandRunsInTheHostsEnvironmentNotTheMacs() async throws { + let fixture = try fixture() + let path = try repository(fixture) + let (connection, _) = try await connect(fixture.host) + let result = await AgentCommandRunner(connection: connection).runNetwork( + "git", ["-c", "alias.environment=!env", "environment"], in: path, timeout: 20) + XCTAssertEqual(result.exitCode, 0, result.stderr) + let environment = Dictionary( + result.stdout.split(separator: "\n").compactMap { line -> (String, String)? in + guard let equals = line.firstIndex(of: "=") else { return nil } + return (String(line[.. Vec<(String, String)> { /// `listing_environment` over an explicit variable set, so the scrub and the pins can be tested /// without mutating this process's own environment — which other tests' child processes inherit. -fn scrubbed_environment( +pub(crate) fn scrubbed_environment( vars: impl Iterator, ) -> Vec<(String, String)> { const SCRUBBED: [&str; 7] = [ diff --git a/vcs/crates/wr-agent/src/main.rs b/vcs/crates/wr-agent/src/main.rs index ab944dc7b..432a46062 100644 --- a/vcs/crates/wr-agent/src/main.rs +++ b/vcs/crates/wr-agent/src/main.rs @@ -472,6 +472,12 @@ fn run_attach(args: &[String]) -> ExitCode { // 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(); + // `--no-create`: attach only if the session exists (see `AttachRequest::existing_only`). + request.existing_only = args.iter().any(|arg| arg == "--no-create"); + // `--no-spawn`: never start an agent. On a remote host that is the supervisor's job, and an + // agent started here would be the idle-exit kind a remote host must not have, racing the + // supervised one for the socket (#228). + let no_spawn = args.iter().any(|arg| arg == "--no-spawn"); let give_up = |reason: &str| -> ExitCode { if invoked_by_the_app { @@ -509,6 +515,12 @@ fn run_attach(args: &[String]) -> ExitCode { // `wr-agent protocol` probe can still reach. let mut stream = match serve::connect(&socket) { Some(stream) => stream, + None if no_spawn => { + return fall_back_to_shell( + &request, + "no session agent is listening, and on a remote host only its supervisor starts one", + ); + } None => { let binary = std::env::current_exe().unwrap_or_else(|_| PathBuf::from("wr-agent")); if serve::spawn_agent(&binary, &socket).is_err() { @@ -549,6 +561,31 @@ fn run_attach(args: &[String]) -> ExitCode { ); } + let mut decoder = EnvelopeDecoder::new(); + let mut buffer = [0u8; 8192]; + + // A restored pane's attach (`--no-create`) is answered before raw mode and the relay threads: + // a session that ended becomes the notice-and-shell the app shows locally, and that has to + // start from a cooked terminal with nothing else running. Whatever follows `Attached` stays in + // `decoder` for the loop below. + if request.existing_only { + match await_attached(&mut stream, &mut decoder, &mut buffer) { + Some(true) => {} + Some(false) => { + return fall_back_to_shell( + &request, + "the terminal that was running here has ended, so this is a new shell", + ); + } + None => { + 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 // the end of this function so every `return` below restores the terminal. let _raw = RawMode::enter(); @@ -568,14 +605,9 @@ fn run_attach(args: &[String]) -> ExitCode { std::thread::spawn(move || relay_resizes(resize_stream, columns, rows)); } - let mut decoder = EnvelopeDecoder::new(); - let mut buffer = [0u8; 8192]; let mut stdout = std::io::stdout(); loop { - match stream.read(&mut buffer) { - Ok(0) | Err(_) => break, - Ok(n) => decoder.push(&buffer[..n]), - } + // What is already decoded first: `await_attached` may have left the repaint in `decoder`. loop { match decoder.next_envelope() { Ok(Some(envelope)) => { @@ -600,7 +632,16 @@ fn run_attach(args: &[String]) -> ExitCode { .map(|b| i32::from_be_bytes(b.try_into().unwrap())) .unwrap_or(0); // `ExitCode` is a byte; a shell status is already 0-255. - return ExitCode::from(code.clamp(0, 255) as u8); + let code = code.clamp(0, 255) as u8; + // On a remote host (`--no-spawn`) this status is ssh's, and 255 is + // what ssh reports for its OWN failure, which the app answers by + // reconnecting. A session that ended with 255 must not read as a + // dropped link, so it becomes 254. + return ExitCode::from(if no_spawn && code == 255 { + 254 + } else { + code + }); } // **Deliberately NOT a fallback**, and an earlier version of this made // it one. That was wrong in the worst available direction. @@ -620,6 +661,20 @@ fn run_attach(args: &[String]) -> ExitCode { // 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. + // The one exception: a `--no-create` attach whose session exited + // between the agent's check and its attach. That is "ended", not a + // failure on a live session, so it gets the same shell as a check that + // came back negative. + FrameKind::Failure + if request.existing_only + && frame.payload.starts_with(b"no session ") => + { + drop(_raw); + return fall_back_to_shell( + &request, + "the terminal that was running here has ended, so this is a new shell", + ); + } FrameKind::Failure => { eprintln!("wr-agent: {}", String::from_utf8_lossy(&frame.payload)); return ExitCode::FAILURE; @@ -632,10 +687,41 @@ fn run_attach(args: &[String]) -> ExitCode { Err(_) => return ExitCode::FAILURE, } } + match stream.read(&mut buffer) { + Ok(0) | Err(_) => break, + Ok(n) => decoder.push(&buffer[..n]), + } } ExitCode::SUCCESS } +/// Reads until the agent answers an attach: `Some(true)` for `Attached`, `Some(false)` for a +/// `Failure` (for a `--no-create` attach, the only one before `Attached` is "session ended"), and +/// `None` when the stream ends first. Envelopes after `Attached` stay in `decoder`. +fn await_attached( + stream: &mut std::os::unix::net::UnixStream, + decoder: &mut EnvelopeDecoder, + buffer: &mut [u8], +) -> Option { + loop { + while let Ok(Some(envelope)) = decoder.next_envelope() { + let mut frames = FrameDecoder::new(); + frames.push(&envelope.payload); + while let Ok(Some(frame)) = frames.next_frame() { + match frame.kind { + FrameKind::Attached => return Some(true), + FrameKind::Failure => return Some(false), + _ => {} + } + } + } + match stream.read(buffer) { + Ok(0) | Err(_) => return None, + Ok(n) => decoder.push(&buffer[..n]), + } + } +} + /// The size of the terminal this process was forked into, or zero when there is none (a pipe, a /// test harness) so the agent applies its own default rather than creating a 0x0 pty. fn terminal_size() -> (u16, u16) { diff --git a/vcs/crates/wr-agent/src/serve.rs b/vcs/crates/wr-agent/src/serve.rs index e8dea019e..3c8aaecbd 100644 --- a/vcs/crates/wr-agent/src/serve.rs +++ b/vcs/crates/wr-agent/src/serve.rs @@ -385,9 +385,23 @@ fn dispatch( request.rows, ) }; + // Refused, not created, for a client that asked to attach only to what exists: see + // `AttachRequest::existing_only`. Before any `Attached`, so the client can tell it + // apart from a failure on a session it is already showing. + // + // Read ONCE, for both this and the choice below: read twice, a session that exited in + // between would pass the refusal and then be created. With one read, that race ends in + // `attach()` finding nothing, which the client already takes as "ended". + let exists = sessions.contains(id); + if request.existing_only && !exists { + return reply(Frame::new( + FrameKind::Failure, + format!("session ended: {}", id.to_hyphenated()).into_bytes(), + )); + } // Create on first attach, reattach afterwards. One code path, so a client that // crashed and came back does not have to know which case it is in. - let result = if sessions.contains(id) { + let result = if exists { attach() } else { // Not `` with no arguments: see `shell::invocation`. Spawning the shell @@ -670,6 +684,12 @@ pub struct AttachRequest { pub rows: u16, /// Passed to the child verbatim. The client's environment, not the agent's. pub env: Vec<(OsString, OsString)>, + /// Attach only if the session exists; never create it (`CREATE=0`). A restored pane sends it, + /// because creating would hand back a fresh shell that looks like the one that was running + /// there. The app asks first on a local host (`confirmBeforeAttach`); a pane on a remote host + /// has no such channel, and one request cannot race the answer. Inverted so the default, + /// and an older client that never sends it, keeps create-on-attach. + pub existing_only: bool, } impl AttachRequest { @@ -699,6 +719,9 @@ impl AttachRequest { } put("COLS", self.columns.to_string().as_bytes()); put("ROWS", self.rows.to_string().as_bytes()); + if self.existing_only { + put("CREATE", b"0"); + } for (key, value) in &self.env { let mut entry = b"ENV:".to_vec(); entry.extend_from_slice(key.as_bytes()); @@ -736,6 +759,7 @@ impl AttachRequest { b"ROWS" => { request.rows = value.to_string_lossy().parse().unwrap_or(0); } + b"CREATE" => request.existing_only = value == "0", _ => { if let Some(name) = key.strip_prefix(b"ENV:") { request.env.push((OsString::from_vec(name.to_vec()), value)); @@ -774,6 +798,7 @@ impl AttachRequest { columns: 0, rows: 0, env, + existing_only: false, } } } @@ -959,6 +984,7 @@ mod tests { (OsString::from("TERM"), OsString::from("xterm-ghostty")), (OsString::from("PATH"), OsString::from("/usr/bin:/bin")), ], + existing_only: true, }; let decoded = AttachRequest::decode(&request.encode()).expect("decode"); assert_eq!(decoded, request); diff --git a/vcs/crates/wr-agent/src/vcs.rs b/vcs/crates/wr-agent/src/vcs.rs index af3b9b991..b3e213b91 100644 --- a/vcs/crates/wr-agent/src/vcs.rs +++ b/vcs/crates/wr-agent/src/vcs.rs @@ -57,8 +57,9 @@ const MAX_HISTORY_LIMIT: usize = 10000; /// The exec service's wire version, reported in `capabilities` so a client can tell a capable agent /// from one that predates the service. A version, not a count — see the `capabilities` reply. /// -/// 2 added chunked request reassembly; 1 accepted only single-envelope requests. -const EXEC_SERVICE_VERSION: u32 = 2; +/// 3 added what a REMOTE host's writes need (#229): `host_environment`, `barrier_root`, and the +/// `stat` request. 2 added chunked request reassembly; 1 accepted only single-envelope requests. +const EXEC_SERVICE_VERSION: u32 = 3; /// `CLIVCSWriter.commitTimeout` (Swift) is 600s, the longest legitimate write timeout. Bounds a /// hostile/buggy request from wedging an exec thread indefinitely; the 32-slot `ACTIVE` permit /// already bounds concurrency, this bounds duration. @@ -198,6 +199,55 @@ struct ExecRequest { stdin: Option, #[serde(default)] env: std::collections::BTreeMap, + /// Set by the app for a REMOTE host (#229): `env` is then only policy pins, and the child's + /// environment is built from this agent's own, which is the host's (`host_environment`). The + /// app sends nothing from the Mac there: its `HOME` and `PATH` name nothing on this machine, and + /// its `SSH_AUTH_SOCK` and git identity are the user's Mac credentials. + #[serde(default)] + host_environment: bool, + /// The shared root of a jj repository whose working-copy barrier this command must run under, + /// set by the app for a REMOTE host only. Locally the app holds that barrier itself + /// (`JJSnapshotGate`) for the whole operation, which is why exec never takes it; a Mac cannot + /// flock a file on another host, so there the agent takes it for each command instead. + /// + /// ponytail: per command, where the local gate holds it for a whole operation (a commit's + /// several commands under one acquisition). A snapshot can land between two commands of one + /// remote write. Upgrade path: an operation-scoped barrier the client acquires and releases, + /// held by this connection and dropped when it goes. + #[serde(default)] + barrier_root: Option, +} + +/// The child environment for a remote host: this agent's own, which is the host's, with the file +/// service's scrub and pins (`scrubbed_environment`, so the two host-side paths agree), no graphical +/// prompt helper, and the request's pins on top. +/// +/// ssh is kept from prompting WITHOUT setting `GIT_SSH_COMMAND`, which would outrank a repository's +/// own `core.sshCommand`, the usual home of a deploy key, and so replace the host's ssh for every +/// repository on it. It cannot prompt anyway: `SSH_ASKPASS_REQUIRE=never` rules out a helper, and a +/// supervised agent has no terminal for one. A `GIT_SSH_COMMAND` the host sets itself is kept and +/// gets `BatchMode`, as the app does locally (`StatusCommandRunner.networkEnvironment`). +/// +/// Built from `vars_os`, not `vars`, which panics on one variable that is not UTF-8 and would take +/// the exec thread down before it replied. +fn host_environment( + own: impl Iterator, + pins: std::collections::BTreeMap, +) -> std::collections::BTreeMap { + let mut env: std::collections::BTreeMap = + crate::file::scrubbed_environment(own).into_iter().collect(); + env.remove("SSH_ASKPASS"); + env.remove("DISPLAY"); + env.extend(pins); + if let Some(ssh) = env + .get("GIT_SSH_COMMAND") + .map(|command| command.trim().to_string()) + .filter(|command| !command.is_empty()) + { + env.insert("GIT_SSH_COMMAND".into(), format!("{ssh} -o BatchMode=yes")); + } + env.insert("SSH_ASKPASS_REQUIRE".into(), "never".into()); + env } #[derive(Deserialize)] @@ -206,6 +256,83 @@ enum ExecKind { Exec, } +/// Filesystem facts for the app's write-failure classifier (`CLIVCSWriter`), which reads a +/// repository's git directory to tell a parked rebase, a leftover lock file or the last fetch. On a +/// remote host that directory is here, so the app asks for the facts and keeps every decision on +/// its own side (#229): one classifier, fed the same facts either way. Added with exec version 3. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct StatRequest { + version: u32, + kind: StatKind, + /// Absolute paths. What exists there, whether it is a directory, and when it was modified. + paths: Vec, + /// Paths among `paths` whose text to return as well, when a small regular file: a worktree's + /// `.git` is a file that names its git directory. + #[serde(default)] + read: Vec, +} + +#[derive(Deserialize)] +#[serde(rename_all = "snake_case")] +enum StatKind { + Stat, +} + +/// Far more than the dozen paths one classification asks about, and small enough that a request +/// cannot turn this into a filesystem walk. +const MAX_STAT_PATHS: usize = 64; +/// A worktree's `.git` file is one line. +const MAX_STAT_TEXT: u64 = 4096; + +fn stat(request: StatRequest) -> model::Result { + let StatRequest { + version, + kind: StatKind::Stat, + paths, + read, + } = request; + if version != 1 { + return Err(VcsError::BackendVersion( + "unsupported VCS service version".into(), + )); + } + if paths.len() > MAX_STAT_PATHS { + return Err(io("too many paths in one stat request")); + } + let entries = paths + .iter() + .map(|path| { + // One path this cannot take (relative, or with a `..` from an unstandardized `gitdir:` + // or a hook's message) reads as absent, as a missing file would, rather than failing the + // whole request and taking every other fact with it. + let Ok(target) = absolute(path) else { + return Ok(json!({ "path": path, "exists": false, "is_dir": false, + "modified_at": null, "text": null })); + }; + // Follows links, as `FileManager.fileExists` does for the same question in the app. + let metadata = std::fs::metadata(&target).ok(); + let text = metadata + .as_ref() + .filter(|m| read.contains(path) && m.is_file() && m.len() <= MAX_STAT_TEXT) + .and_then(|_| std::fs::read_to_string(&target).ok()); + let modified = metadata + .as_ref() + .and_then(|m| m.modified().ok()) + .and_then(|time| time.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|since| since.as_secs_f64()); + Ok(json!({ + "path": path, + "exists": metadata.is_some(), + "is_dir": metadata.as_ref().is_some_and(|m| m.is_dir()), + "modified_at": modified, + "text": text, + })) + }) + .collect::>>()?; + Ok(json!({ "entries": entries })) +} + #[derive(Clone, Copy, Deserialize)] #[serde(rename_all = "snake_case")] enum Executable { @@ -352,16 +479,16 @@ pub fn execute(bytes: &[u8]) -> Value { // wire format already spoken by `AgentVCSReader` is untouched. let result = serde_json::from_slice::(bytes) .map_err(io) - .and_then(|value| { - if value.get("kind").and_then(Value::as_str) == Some("exec") { - serde_json::from_value::(value) - .map_err(io) - .and_then(exec) - } else { - serde_json::from_value::(value) - .map_err(io) - .and_then(read) - } + .and_then(|value| match value.get("kind").and_then(Value::as_str) { + Some("exec") => serde_json::from_value::(value) + .map_err(io) + .and_then(exec), + Some("stat") => serde_json::from_value::(value) + .map_err(io) + .and_then(stat), + _ => serde_json::from_value::(value) + .map_err(io) + .and_then(read), }); match result { Ok(result) => json!({"version": 1, "result": result}), @@ -470,6 +597,7 @@ const REAP_GRACE: Duration = Duration::from_secs(2); /// SIGTERM-then-grace-then-SIGKILL shape (`CLIVCSWriter.commitTimeout`'s doc: "Killing a commit is /// categorically more dangerous than killing a fetch" — an immediate SIGKILL denies a `post-commit` /// hook the chance to exit cleanly and can leave `index.lock` behind). +#[cfg(test)] fn run_exec( dir: &Path, executable: &str, @@ -795,6 +923,8 @@ fn exec(request: ExecRequest) -> model::Result { timeout_ms, stdin, env, + host_environment: from_host, + barrier_root, } = request; if version != 1 { return Err(VcsError::BackendVersion( @@ -803,16 +933,36 @@ fn exec(request: ExecRequest) -> model::Result { } let dir = absolute(&dir)?; let timeout = exec_timeout(timeout_ms); + // Held until the child exits, and handed to it, so an agent that dies mid-command leaves the + // barrier held by the child rather than released under it. No `.jj` there, no barrier: a git + // repository has none to take, as `JJProcessBarrier.acquire` decides in the app. + // + // Nor for a command that cannot snapshot (`--ignore-working-copy`): there is nothing to protect, + // and a read waiting out a long push's barrier would time out as a failure the toolbar shows. + let snapshots = !args.iter().any(|arg| arg == "--ignore-working-copy"); + let barrier = match barrier_root.as_deref() { + Some(shared) if snapshots && absolute(shared)?.join(".jj").exists() => { + Some(SnapshotLock::acquire(&dir, Some(shared))?) + } + _ => None, + }; + let env = if from_host { + host_environment(std::env::vars_os(), env) + } else { + env + }; let env: Vec<(&str, &str)> = env.iter().map(|(k, v)| (k.as_str(), v.as_str())).collect(); let stdin = stdin.as_deref().map(latin1_bytes).transpose()?; - let captured = run_exec( + let captured = run_exec_with( &dir, executable.as_str(), &args, timeout, stdin.as_deref(), &env, + barrier.as_ref().map(SnapshotLock::fd), )?; + drop(barrier); let stdout = String::from_utf8_lossy(&captured.stdout); let stderr = String::from_utf8_lossy(&captured.stderr); // Truncate HERE rather than letting `send` refuse the reply. `send`'s blanket @@ -1487,6 +1637,109 @@ mod tests { std::fs::remove_dir_all(&root).unwrap(); } + /// The remote half of that property (#229): a Mac cannot flock the barrier on another host, so + /// an exec request that names a `barrier_root` must run under the barrier, taken here. Held by + /// another holder, the command waits for it rather than running alongside. + #[test] + fn exec_with_a_barrier_root_waits_on_a_held_barrier() { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-barrier-root-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join(".jj/repo")).unwrap(); + let held = SnapshotLock::acquire(&root, root.to_str()).unwrap(); + let releaser = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(700)); + drop(held); + }); + let mut request: Value = + serde_json::from_slice(&exec_request(&root, "git", &["--version"])).unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let start = Instant::now(); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert!(reply.get("result").is_some(), "exec failed: {reply:?}"); + assert!( + start.elapsed() >= Duration::from_millis(600), + "the command ran without waiting for the barrier" + ); + releaser.join().unwrap(); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// A command that cannot snapshot has nothing for the barrier to protect, so it does not wait. + #[test] + fn a_command_that_cannot_snapshot_skips_the_barrier() { + let root = + std::env::temp_dir().join(format!("wr-vcs-exec-barrier-read-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join(".jj/repo")).unwrap(); + let held = SnapshotLock::acquire(&root, root.to_str()).unwrap(); + let mut request: Value = serde_json::from_slice(&exec_request( + &root, + "git", + &["--version", "--ignore-working-copy"], + )) + .unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let start = Instant::now(); + let _ = execute(&serde_json::to_vec(&request).unwrap()); + assert!( + start.elapsed() < Duration::from_secs(5), + "a read waited on the barrier" + ); + drop(held); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// A git repository has no barrier to take, and naming one is not an error: the app sends the + /// shared root for every remote jj command, and decides nothing about git from it. + #[test] + fn a_barrier_root_without_jj_runs_the_command_unlocked() { + let root = git_repo("barrier-root-git"); + let mut request: Value = + serde_json::from_slice(&exec_request(&root, "git", &["status"])).unwrap(); + request["barrier_root"] = json!(root.to_str().unwrap()); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + assert_eq!(reply["result"]["exit_code"], 0, "{reply}"); + std::fs::remove_dir_all(&root).unwrap(); + } + + /// The facts the app's classifier reads from a git directory (#229), for a path that is a + /// directory, a file whose text was asked for, and one that is not there. + #[test] + fn stat_reports_what_is_at_each_path() { + let root = git_repo("stat"); + let git = root.join(".git"); + std::fs::write(root.join("pointer"), "gitdir: /elsewhere\n").unwrap(); + let request = json!({ + "version": 1, + "kind": "stat", + "paths": [git, root.join("pointer"), git.join("rebase-merge")], + "read": [root.join("pointer")], + }); + let reply = execute(&serde_json::to_vec(&request).unwrap()); + let entries = reply["result"]["entries"].as_array().expect("entries"); + assert_eq!(entries[0]["exists"], true); + assert_eq!(entries[0]["is_dir"], true); + assert!(entries[0]["modified_at"].as_f64().unwrap() > 0.0); + assert_eq!(entries[0]["text"], Value::Null, "text was not asked for"); + assert_eq!(entries[1]["is_dir"], false); + assert_eq!(entries[1]["text"], "gitdir: /elsewhere\n"); + assert_eq!(entries[2]["exists"], false); + assert_eq!(entries[2]["modified_at"], Value::Null); + + let relative = json!({"version": 1, "kind": "stat", "paths": ["relative/path", git]}); + let reply = execute(&serde_json::to_vec(&relative).unwrap()); + assert_eq!(reply["result"]["entries"][0]["exists"], false, "{reply}"); + assert_eq!( + reply["result"]["entries"][1]["exists"], true, + "one bad path spoiled the rest" + ); + let many: Vec = (0..=MAX_STAT_PATHS).map(|i| format!("/tmp/{i}")).collect(); + let too_many = json!({"version": 1, "kind": "stat", "paths": many}); + assert!(execute(&serde_json::to_vec(&too_many).unwrap())["error"].is_object()); + std::fs::remove_dir_all(&root).unwrap(); + } + #[test] fn capabilities_reports_the_read_count_and_the_exec_service_version() { let reply = execute(br#"{"version":1,"method":"capabilities"}"#); @@ -1532,6 +1785,62 @@ mod tests { std::fs::remove_dir_all(&root).unwrap(); } + /// A remote host's child environment (#229): the host's own, less what could point git at + /// another repository or let ssh prompt, with the app's pins on top. + #[test] + fn a_host_environment_is_the_hosts_own_with_the_pins_on_top_and_ssh_unable_to_prompt() { + let pairs = |pairs: &[(&str, &str)]| { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect::>() + }; + let os = |pairs: Vec<(String, String)>| { + pairs + .into_iter() + .map(|(k, v)| (k.into(), v.into())) + .collect::>() + }; + let own = pairs(&[ + ("HOME", "/home/workroom"), + ("PATH", "/usr/bin:/bin"), + ("GIT_DIR", "/elsewhere/.git"), + ("SSH_ASKPASS", "/usr/bin/ask"), + ("LC_ALL", "fr_FR.UTF-8"), + ("GIT_SSH_COMMAND", "ssh -i /home/workroom/.ssh/deploy"), + ]); + let pins = pairs(&[("LC_ALL", "C"), ("GIT_TERMINAL_PROMPT", "0")]) + .into_iter() + .collect(); + let env = host_environment(os(own).into_iter(), pins); + assert_eq!(env["HOME"], "/home/workroom"); + assert_eq!(env["PATH"], "/usr/bin:/bin"); + assert!(!env.contains_key("GIT_DIR")); + assert!(!env.contains_key("SSH_ASKPASS")); + assert_eq!(env["LC_ALL"], "C", "a pin outranks the host's value"); + assert_eq!(env["GIT_TERMINAL_PROMPT"], "0"); + assert_eq!( + env["GIT_SSH_COMMAND"], + "ssh -i /home/workroom/.ssh/deploy -o BatchMode=yes" + ); + assert_eq!(env["SSH_ASKPASS_REQUIRE"], "never"); + + // With none of its own, it gets none: set, it would outrank a repository's + // `core.sshCommand`, where a deploy key usually lives. + let bare = host_environment(std::iter::empty(), Default::default()); + assert!(!bare.contains_key("GIT_SSH_COMMAND")); + assert_eq!(bare["SSH_ASKPASS_REQUIRE"], "never"); + + // A variable that is not UTF-8 is dropped, not a panic. + use std::os::unix::ffi::OsStringExt; + let latin1 = vec![( + std::ffi::OsString::from("LEGACY"), + std::ffi::OsString::from_vec(vec![0xe9]), + )]; + let env = host_environment(latin1.into_iter(), Default::default()); + assert!(!env.contains_key("LEGACY")); + } + /// The regression that motivated bounding the drain: a child that exits promptly while leaving /// a BACKGROUND descendant holding its stdout must not hold the exec thread (and with it one of /// the 32 `ACTIVE` permits) for the descendant's lifetime. Before the bound, this returned in 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 index 45227d468..78414a470 100644 --- 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 @@ -44,8 +44,16 @@ fn attach_with_stdin(env: &[(&str, &str)], stdin: &str) -> (String, Option) } fn attach_inner(env: &[(&str, &str)], stdin: Option<&str>) -> (String, Option) { + attach_with_args(&[], env, stdin) +} + +fn attach_with_args( + args: &[&str], + env: &[(&str, &str)], + stdin: Option<&str>, +) -> (String, Option) { let mut command = Command::new(agent_binary()); - command.arg("attach"); + command.arg("attach").args(args); command.env_clear(); command.env("PATH", "/usr/bin:/bin"); for (key, value) in env { @@ -701,3 +709,183 @@ fn a_deleted_working_directory_falls_back_to_home_before_root() { {output:?}" ); } + +/// A healthy agent on `socket`, killed when the guard drops. +struct Agent(std::process::Child); + +impl Drop for Agent { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +fn start_agent(socket: &std::path::Path) -> Agent { + let agent = Agent( + Command::new(agent_binary()) + .args(["serve", "--socket"]) + .arg(socket) + .args(["--idle-timeout", "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"); + agent +} + +/// `--no-spawn` (a pane on a remote host, #229): with no agent listening, a shell rather than a +/// spawned agent. On a remote host the supervisor starts the agent, and one started here would be +/// the idle-exit kind, racing the supervised one for the socket. +#[test] +fn no_spawn_falls_back_to_a_shell_instead_of_starting_an_agent() { + let dir = scratch("no-spawn"); + let socket = dir.join("a.sock"); + let (output, _) = attach_with_args( + &["--no-spawn"], + &[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC72", + ), + ("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 fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ], + None, + ); + std::thread::sleep(std::time::Duration::from_millis(500)); + let spawned = socket.exists(); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("FELL-BACK fb=[1]"), + "expected the fallback shell. got: {output:?}" + ); + assert!(!spawned, "an agent was started despite --no-spawn"); +} + +/// `--no-create` (a restored pane): a session the agent does not hold is refused, not created, +/// and the pane becomes a shell that says so. Created instead, it would be a fresh shell passed off +/// as the one that was running there. +#[test] +fn no_create_turns_an_ended_session_into_a_shell_with_a_notice() { + let dir = scratch("no-create-ended"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let (output, _) = attach_with_args( + &["--no-create"], + &[ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC73", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + "echo NEW-SHELL fb=[${WORKROOM_SESSION_FALLBACK:-unset}]", + ), + ], + None, + ); + let listed = Command::new(agent_binary()) + .args(["list", "--socket"]) + .arg(&socket) + .output() + .expect("list"); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("has ended") && output.contains("NEW-SHELL fb=[1]"), + "expected the ended-session notice and a fallback shell. got: {output:?}" + ); + assert!( + !String::from_utf8_lossy(&listed.stdout).contains("6b9b968d"), + "the agent created the session anyway: {:?}", + String::from_utf8_lossy(&listed.stdout) + ); +} + +/// And a session that exists is reattached as usual: `--no-create` only refuses to make one. +#[test] +fn no_create_reattaches_a_session_that_exists() { + let dir = scratch("no-create-live"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let env = [ + ( + "WORKROOM_SESSION_ID", + "6B9B968D-0BD7-4172-850A-A373DA73BC74", + ), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ( + "WORKROOM_SESSION_COMMAND", + // Its own `sh -c`: a session's command is run as `exec `, which would end at + // `sleep`. + "sh -c 'sleep 2; echo STILL-THE-SAME fb=[${WORKROOM_SESSION_FALLBACK:-unset}]'", + ), + ]; + // The first attach creates it; killed at once, it leaves the session running with no client. + let mut first = Command::new(agent_binary()); + first.arg("attach").env_clear().env("PATH", "/usr/bin:/bin"); + for (key, value) in env { + first.env(key, value); + } + let mut first = first + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn first attach"); + std::thread::sleep(std::time::Duration::from_millis(500)); + let _ = first.kill(); + let _ = first.wait(); + + let (output, _) = attach_with_args(&["--no-create"], &env, None); + let _ = std::fs::remove_dir_all(&dir); + assert!( + output.contains("STILL-THE-SAME fb=[unset]"), + "expected the running session's own output. got: {output:?}" + ); + assert!(!output.contains("has ended"), "got: {output:?}"); +} + +/// On a remote host (`--no-spawn`) the attach's exit status is ssh's, and 255 is ssh's own +/// failure, which the app answers by reconnecting. So a session that ended with 255 exits 254 +/// there; locally it stays 255. +#[test] +fn a_remote_attach_never_exits_255_for_a_session_that_did() { + let dir = scratch("exit-255"); + let socket = dir.join("a.sock"); + let _agent = start_agent(&socket); + let run = |id: &str, args: &[&str]| { + attach_with_args( + args, + &[ + ("WORKROOM_SESSION_ID", id), + ("WORKROOM_SESSION_SOCKET", socket.to_str().unwrap()), + ("WORKROOM_SESSION_SHELL", "/bin/sh"), + ("WORKROOM_SESSION_CWD", dir.to_str().unwrap()), + ("WORKROOM_SESSION_COMMAND", "sh -c 'exit 255'"), + ], + None, + ) + .1 + }; + let local = run("6B9B968D-0BD7-4172-850A-A373DA73BC75", &[]); + let remote = run("6B9B968D-0BD7-4172-850A-A373DA73BC76", &["--no-spawn"]); + let _ = std::fs::remove_dir_all(&dir); + assert_eq!(local, Some(255)); + assert_eq!(remote, Some(254)); +} diff --git a/vcs/scripts/ssh-fixture/entrypoint.sh b/vcs/scripts/ssh-fixture/entrypoint.sh index 9e6de2e34..fa9d2891b 100755 --- a/vcs/scripts/ssh-fixture/entrypoint.sh +++ b/vcs/scripts/ssh-fixture/entrypoint.sh @@ -17,11 +17,12 @@ chmod 600 /home/workroom/.ssh/authorized_keys # `--idle-timeout never`: a remote agent must keep running with no client attached, because its # BUSY/IDLE reports have to keep flowing while the Mac sleeps. Run as the ssh user, so the relay -# that user runs can reach the socket. +# that user runs can reach the socket. `env -i`, because the agent's environment is what every git +# it runs gets (the app sends none from the Mac), and this script's own holds AUTHORIZED_KEY. ( while :; do setpriv --reuid=workroom --regid=workroom --init-groups \ - env HOME=/home/workroom USER=workroom SHELL=/bin/bash \ + env -i HOME=/home/workroom USER=workroom SHELL=/bin/bash PATH=/usr/local/bin:/usr/bin:/bin \ wr-agent serve --socket /run/workroom/agent.sock --idle-timeout never || true sleep 1 done diff --git a/vcs/scripts/ssh-fixture/run.sh b/vcs/scripts/ssh-fixture/run.sh index a79926406..284dfa7b7 100755 --- a/vcs/scripts/ssh-fixture/run.sh +++ b/vcs/scripts/ssh-fixture/run.sh @@ -2,7 +2,7 @@ # # Runs the remote-path tests over real ssh, into a container (issue #228). # -# vcs/scripts/ssh-fixture/run.sh +# vcs/scripts/ssh-fixture/run.sh [command...] # # The argument is a Linux wr-agent for the CONTAINER's architecture, built the way the app bundles # it: static musl, with terminal-state. CI passes the one its `agent-linux` job just built. On an @@ -17,6 +17,14 @@ # pins the container's host key out of band, and runs the ignored `over_ssh_*` tests in # remote_transport.rs against it with BatchMode ssh. The tests run on THIS machine; only the agent # and sshd are in the container, as they will be on a real remote host. +# +# Given a command, it runs that instead, with the fixture's whereabouts in WR_SSH_FIXTURE_*. The +# app's tests take them from there, run from the repository root: +# +# vcs/scripts/ssh-fixture/run.sh \ +# make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/RemoteHostIntegrationTests +# +# xcodebuild hands a test only the variables prefixed TEST_RUNNER_, so each is exported twice. set -euo pipefail AGENT="${1:?usage: run.sh }" @@ -87,10 +95,18 @@ for attempt in $(seq 1 100); do sleep 0.2 done -# One thread: the tests share one agent, and the supervisor test kills it. -cd "$VCS_DIR" -if ! WR_SSH_FIXTURE_CONFIG="$STAGE/ssh_config" WR_SSH_FIXTURE_SOCKET="$SOCKET" \ - cargo test -p wr-agent --test remote_transport -- --ignored --test-threads=1; then +for pair in "CONFIG=$STAGE/ssh_config" "SOCKET=$SOCKET" "ADDRESS=127.0.0.1" "PORT=$PORT" \ + "USER=workroom" "IDENTITY=$STAGE/id_ed25519" "HOST_KEY=$HOST_KEY"; do + export "WR_SSH_FIXTURE_$pair" "TEST_RUNNER_WR_SSH_FIXTURE_$pair" +done + +shift +if [ "$#" -eq 0 ]; then + # One thread: the tests share one agent, and the supervisor test kills it. + cd "$VCS_DIR" + set -- cargo test -p wr-agent --test remote_transport -- --ignored --test-threads=1 +fi +if ! "$@"; then echo "error: the ssh fixture tests failed; the container's log:" >&2 "$RUNTIME" logs "$NAME" >&2 exit 1