From 533943ad5ebdeca333fdbcdb155497ace73aeafe Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 18:26:44 +0100 Subject: [PATCH 1/9] feat(macapp): the container driver creates, derives and destroys hosts Slice A of #252. Given a container runtime and the ssh fixture's image, `ContainerHostDriver` now provisions its own hosts. `create` runs a base, `deriveFromBase` commits a base's disk and runs a container from it, and `destroy` removes the container along with the image it alone was run from. Each step undoes whatever it had made when a later one fails. A derived container mints its own identity at first boot, before sshd or the agent starts (open question 9): fresh ssh host keys and /etc/machine-id, with the base's broker files and screens dropped. A marker keyed on the container's hostname makes the mint run once, so a restart keeps its pinned host key. boot_id belongs to the kernel and can't be re-minted in a container. The new host key is read out of band through the image's identity.sh, which run.sh now reads it through too. run.sh exports what provisioning needs, and on exit sweeps every container and image carrying its run's label. Tests: ContainerProvisioningIntegrationTests (3) and RemoteHostIntegrationTests (14) pass through run.sh, and so do the 5 over_ssh Rust tests. Two negative controls go red: skipping the re-mint fails the identity assertions, and dropping the image rollback fails the failure-injection test at run, port and exec. Co-Authored-By: Claude Opus 5.5 --- .../Core/ContainerHostDriver.swift | 198 +++++++++++++++-- macapp/WorkroomApp/Core/HostDriver.swift | 3 + ...ontainerProvisioningIntegrationTests.swift | 201 ++++++++++++++++++ vcs/scripts/ssh-fixture/Dockerfile | 3 +- vcs/scripts/ssh-fixture/entrypoint.sh | 20 ++ vcs/scripts/ssh-fixture/identity.sh | 8 + vcs/scripts/ssh-fixture/run.sh | 35 ++- 7 files changed, 442 insertions(+), 26 deletions(-) create mode 100644 macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift create mode 100755 vcs/scripts/ssh-fixture/identity.sh diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index f31a4a48b..b5de70485 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -5,9 +5,10 @@ import Foundation /// 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 { +/// Its hosts are the ones the caller hands in, plus, given `Provisioning`, the containers it makes +/// itself (#252): `create` runs a base from the fixture's image, `deriveFromBase` snapshots a base +/// (`commit`) and runs a container from that, and `destroy` removes either. Nothing here persists. +final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { struct Host: Sendable { let address: String let port: Int @@ -30,36 +31,197 @@ struct ContainerHostDriver: HostTerminalDriver { var resources: String { AgentBootstrap.resources(besideSocket: agentSocket) } } - let traits = HostDriverTraits( - transport: .sshStdio, deriveSpeed: nil, deriveCarriesLiveProcesses: false, - durableDisk: false, maxLifetime: nil) + /// How the driver makes containers of its own: a container runtime's CLI (`docker`), the + /// fixture's image, and the client key every container authorises. Containers are reached on a + /// loopback port the runtime publishes, as `vcs/scripts/ssh-fixture/run.sh` reaches its own. + struct Provisioning: Sendable { + let runtime: URL + let image: String + let user: String + let identityFile: String + /// The public half of `identityFile`, which the entrypoint writes to `authorized_keys`. + let publicKey: String + let agentSocket: String + /// `key=value` labels on every container and image made, so whoever started the fixture can + /// sweep up what a crashed test left (run.sh does). + let labels: [String] + } + + /// A host this driver made: its container, and for a derived one the image it was run from, + /// which only it uses. + private struct Provisioned { + let container: String + let image: String? + } + + /// A derive is a snapshot of the disk only: a commit keeps no process, and the container run + /// from it boots afresh. + var traits: HostDriverTraits { + HostDriverTraits( + transport: .sshStdio, deriveSpeed: provisioning == nil ? nil : .seconds(5), + deriveCarriesLiveProcesses: false, durableDisk: false, maxLifetime: nil) + } - let hosts: [UUID: Host] /// Where each host's `ssh_config` and `known_hosts` are written. let directory: URL + let provisioning: Provisioning? + private let lock = NSLock() + private var hosts: [UUID: Host] + private var provisioned: [UUID: Provisioned] = [:] + + init(hosts: [UUID: Host], directory: URL, provisioning: Provisioning? = nil) { + self.hosts = hosts + self.directory = directory + self.provisioning = provisioning + } + + private func target(_ host: HostID) throws -> (UUID, Host) { + guard case .remote(let id) = host, let target = lock.withLock({ hosts[id] }) else { + throw HostDriverError.unknownHost(host) + } + return (id, target) + } - func create() async throws -> HostID { throw HostDriverError.notImplemented("Creating a base") } + func create() async throws -> HostID { + guard let provisioning else { throw HostDriverError.notImplemented("Creating a base") } + return try await run(provisioning.image, image: nil) + } + /// A snapshot of `base`'s disk, run as a container of its own. The base keeps running: `commit` + /// pauses it only while it copies. The new container mints its own identity at its first boot + /// (`entrypoint.sh`), before sshd or the agent starts, so it serves nothing as its base; its + /// host key is read once that is done and pinned here. func deriveFromBase(_ base: HostID) async throws -> HostID { - throw HostDriverError.notImplemented("Deriving a workroom instance") + guard let provisioning else { + throw HostDriverError.notImplemented("Deriving a workroom instance") + } + guard case .remote(let id) = base, let source = lock.withLock({ provisioned[id] }) else { + throw HostDriverError.unknownHost(base) + } + let image = try await runtime( + ["commit"] + provisioning.labels.flatMap { ["--change", "LABEL \($0)"] } + + [source.container]) + do { + return try await run(image, image: image) + } catch { + await Self.cleanUp { _ = try await self.runtime(["rmi", "--force", image]) } + throw error + } } + /// Removes the container and the image it alone was run from. A host handed in rather than + /// made here is not this driver's to remove. func destroy(_ host: HostID) async throws { - throw HostDriverError.notImplemented("Destroying a host") + guard case .remote(let id) = host, let made = lock.withLock({ provisioned[id] }) else { + if case .remote(let id) = host, lock.withLock({ hosts[id] }) != nil { + throw HostDriverError.notImplemented("Destroying a host this driver did not make") + } + throw HostDriverError.unknownHost(host) + } + _ = try await runtime(["rm", "--force", "--volumes", made.container]) + if let image = made.image { _ = try await runtime(["rmi", "--force", image]) } + lock.withLock { + hosts[id] = nil + provisioned[id] = nil + } + try? FileManager.default.removeItem(at: directory.appendingPathComponent(id.uuidString)) } - func openStream(to host: HostID) async throws -> HostStream { - guard case .remote(let id) = host, let target = hosts[id] else { - throw HostDriverError.unknownHost(host) + /// Runs a container from `source` and waits until it can be reached: its identity minted, its + /// host key pinned, and an ssh login through this driver's own configuration answering. A + /// container that gets no further is removed. + private func run(_ source: String, image: String?) async throws -> HostID { + guard let provisioning else { throw HostDriverError.notImplemented("Provisioning") } + let container = try await runtime( + ["run", "--detach", "--init", "--publish", "127.0.0.1::22"] + + provisioning.labels.flatMap { ["--label", $0] } + + ["--env", "AUTHORIZED_KEY=\(provisioning.publicKey)", source]) + let id = UUID() + do { + let published = try await runtime(["port", container, "22/tcp"]) + guard let port = published.split(separator: ":").last.flatMap({ Int($0) }) else { + throw HostDriverError.provisioning("no published ssh port in \(published)") + } + let hostKey = try await identity(of: container) + lock.withLock { + hosts[id] = Host( + address: "127.0.0.1", port: port, user: provisioning.user, + identityFile: provisioning.identityFile, hostKey: hostKey, + agentSocket: provisioning.agentSocket) + provisioned[id] = Provisioned(container: container, image: image) + } + try await awaitLogin(.remote(id)) + return .remote(id) + } catch { + lock.withLock { + hosts[id] = nil + provisioned[id] = nil + } + await Self.cleanUp { _ = try await self.runtime(["rm", "--force", "--volumes", container]) } + throw error + } + } + + /// The container's ssh host key, read through the runtime once its entrypoint has minted it + /// (the image's `identity.sh`, which run.sh reads it through too): delivered out of band, as + /// every driver's must be (design doc, host-key policy). + private func identity(of container: String) async throws -> String { + for _ in 0..<100 { + if let key = try? await runtime(["exec", container, "identity.sh"]), + key.split(separator: " ").count == 2 + { + return key + } + try await Task.sleep(for: .milliseconds(200)) + } + throw HostDriverError.provisioning("\(container) never minted its identity") + } + + /// sshd starts a moment after the identity is minted, so the first login can be refused. + private func awaitLogin(_ host: HostID) async throws { + var last = "" + for _ in 0..<50 { + let (status, output) = try await exec("true", on: host).communicate(nil, timeout: 20) + if status == 0 { return } + last = output + try await Task.sleep(for: .milliseconds(200)) } + throw HostDriverError.provisioning("ssh never let us in: \(last)") + } + + /// One runtime command; its first line of output, which is all any of these print on success. + /// Its environment is the few variables that say which daemon to talk to, never the app's whole + /// one. + private func runtime(_ arguments: [String]) async throws -> String { + guard let provisioning else { throw HostDriverError.notImplemented("Provisioning") } + let environment = ProcessInfo.processInfo.environment.filter { + ["HOME", "PATH", "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG"].contains($0.key) + } + let (status, output) = try await HostStream.spawn( + provisioning.runtime, arguments, environment: environment, handshakeTimeout: 20 + ).communicate(nil, timeout: 120) + let said = output.trimmingCharacters(in: .whitespacesAndNewlines) + guard status == 0 else { + throw HostDriverError.provisioning( + "\(provisioning.runtime.lastPathComponent) \(arguments[0]) exited \(status): \(said)") + } + return said.split(separator: "\n").first.map(String.init) ?? "" + } + + /// Undoes a step whatever the caller's state: in a task of its own, because a cancelled caller + /// would otherwise cancel the cleanup too and leave the container running. + private static func cleanUp(_ body: @escaping @Sendable () async throws -> Void) async { + await Task { try? await body() }.value + } + + func openStream(to host: HostID) async throws -> HostStream { + let (_, target) = try target(host) return try await exec( Self.relayCommand(binary: target.agentBinary, socket: target.agentSocket), on: host) } func exec(_ command: String, on host: HostID) async throws -> HostStream { - guard case .remote(let id) = host, let target = hosts[id] else { - throw HostDriverError.unknownHost(host) - } + let (id, target) = try target(host) let config = try Self.writeConfiguration( for: target, in: directory.appendingPathComponent(id.uuidString)) return try HostStream.spawn( @@ -77,9 +239,7 @@ struct ContainerHostDriver: HostTerminalDriver { 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 (id, target) = try target(host) let hostDirectory = directory.appendingPathComponent(id.uuidString) let config = try Self.writeConfiguration(for: target, in: hostDirectory) // `-t`: the attach client on the far side wants a terminal, for raw mode and the pane's size, diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift index 534b94cde..17aad3fa9 100644 --- a/macapp/WorkroomApp/Core/HostDriver.swift +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -77,12 +77,15 @@ enum HostDriverError: Error, Equatable, Sendable, LocalizedError { /// Provisioning lands in Phase 4, which is what says how a base and an instance come to exist. case notImplemented(String) case invalidConfiguration(String) + /// Making, deriving or removing a host failed. Whatever the step had made is gone. + case provisioning(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)" + case .provisioning(let detail): return "Couldn't provision the host: \(detail)" } } } diff --git a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift new file mode 100644 index 000000000..df6fba014 --- /dev/null +++ b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift @@ -0,0 +1,201 @@ +import XCTest + +@testable import Workroom + +/// `ContainerHostDriver`'s provisioning (#252): bases and instances derived from them, made from +/// the ssh fixture's image with the container runtime. Skipped unless run through the fixture +/// script, which builds the image and says where it is: +/// +/// vcs/scripts/ssh-fixture/run.sh \ +/// make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/ContainerProvisioningIntegrationTests +final class ContainerProvisioningIntegrationTests: XCTestCase { + private var directory: URL! + /// This test's own label, beside the run's, so what it left is told apart from other tests'. + private var label: String! + private var runtime: URL! + private var connections: [AgentVCSConnection] = [] + + override func setUp() { + super.setUp() + directory = FileManager.default.temporaryDirectory.appendingPathComponent( + "wr-provision-\(UUID().uuidString.prefix(8))") + label = "workroom.test=\(UUID().uuidString.lowercased())" + } + + override func tearDown() async throws { + for connection in connections { await connection.close() } + connections.removeAll() + // Whatever a failed test left, so the next one starts clean. run.sh sweeps the run's label too. + if let runtime, let label { + for container in (try? leftovers("ps", runtime: runtime, label: label)) ?? [] { + _ = try? docker(runtime, ["rm", "--force", "--volumes", container]) + } + for image in (try? leftovers("images", runtime: runtime, label: label)) ?? [] { + _ = try? docker(runtime, ["rmi", "--force", image]) + } + } + try? FileManager.default.removeItem(at: directory) + try await super.tearDown() + } + + private func provisioning(runtime override: URL? = nil) throws -> ContainerHostDriver.Provisioning + { + 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 + } + runtime = URL(fileURLWithPath: try need("WR_SSH_FIXTURE_RUNTIME_PATH")) + return ContainerHostDriver.Provisioning( + runtime: override ?? runtime, image: try need("WR_SSH_FIXTURE_IMAGE"), + user: try need("WR_SSH_FIXTURE_USER"), identityFile: try need("WR_SSH_FIXTURE_IDENTITY"), + publicKey: try need("WR_SSH_FIXTURE_PUBLIC_KEY"), + agentSocket: try need("WR_SSH_FIXTURE_SOCKET"), + labels: [try need("WR_SSH_FIXTURE_LABEL"), label]) + } + + /// The real runtime, run from here rather than through the driver, so a check is never the + /// thing under test. + @discardableResult + private func docker(_ runtime: URL, _ arguments: [String]) throws -> String { + let process = Process() + process.executableURL = runtime + process.arguments = arguments + let output = Pipe() + process.standardOutput = output + process.standardError = output + try process.run() + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + return String(decoding: data, as: UTF8.self).trimmingCharacters(in: .whitespacesAndNewlines) + } + + /// The containers (`ps`) or images (`images`) carrying this test's label. `-a` for both: a + /// commit is untagged, and `images` lists an untagged image only with it. + private func leftovers(_ kind: String, runtime: URL, label: String) throws -> [String] { + try docker(runtime, [kind, "-a", "-q", "--filter", "label=\(label)"]) + .split(separator: "\n").map(String.init) + } + + @discardableResult + private func onHost(_ driver: ContainerHostDriver, _ host: HostID, _ command: String) + async throws -> String + { + let (status, output) = try await driver.exec(command, on: host).communicate(nil, timeout: 20) + XCTAssertEqual(status, 0, "on the host, \(command) failed: \(output)") + return output.trimmingCharacters(in: .whitespacesAndNewlines) + } + + /// The machine identity open question 9 lists: the ssh host key and `/etc/machine-id`. + private func identity(_ driver: ContainerHostDriver, _ host: HostID) async throws -> [String] { + [ + try await onHost(driver, host, "cut -d' ' -f1,2 /etc/ssh/ssh_host_ed25519_key.pub"), + try await onHost(driver, host, "cat /etc/machine-id"), + ] + } + + func testABaseIsReachableItsAgentServesAndDestroyingItLeavesNothing() async throws { + let provisioning = try provisioning() + let driver = ContainerHostDriver(hosts: [:], directory: directory, provisioning: provisioning) + + let base = try await driver.create() + let connection = try await AgentVCSConnection.connect( + host: base, stream: try await driver.openStream(to: base)) + connections.append(connection) + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label).count, 1) + + try await driver.destroy(base) + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) + do { + _ = try await driver.exec("true", on: base) + XCTFail("a destroyed host is still known") + } catch HostDriverError.unknownHost {} + } + + func testInstancesDerivedFromOneBaseCarryItsDiskButMintTheirOwnIdentity() async throws { + let provisioning = try provisioning() + let driver = ContainerHostDriver(hosts: [:], directory: directory, provisioning: provisioning) + let base = try await driver.create() + try await onHost(driver, base, "echo from-the-base > ~/template") + let baseIdentity = try await identity(driver, base) + + let first = try await driver.deriveFromBase(base) + let second = try await driver.deriveFromBase(base) + + for instance in [first, second] { + let template = try await onHost(driver, instance, "cat ~/template") + XCTAssertEqual(template, "from-the-base") + let connection = try await AgentVCSConnection.connect( + host: instance, stream: try await driver.openStream(to: instance)) + connections.append(connection) + } + let firstIdentity = try await identity(driver, first) + let secondIdentity = try await identity(driver, second) + for (name, index) in [("ssh host key", 0), ("machine-id", 1)] { + XCTAssertNotEqual( + firstIdentity[index], secondIdentity[index], "two instances share a \(name)") + XCTAssertNotEqual( + firstIdentity[index], baseIdentity[index], "an instance kept its base's \(name)") + XCTAssertNotEqual( + secondIdentity[index], baseIdentity[index], "an instance kept its base's \(name)") + } + + for host in [first, second, base] { try await driver.destroy(host) } + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) + XCTAssertEqual( + try leftovers("images", runtime: runtime, label: label), [], "a commit outlived its instance") + } + + /// A runtime that runs the real one, except for the subcommand named in `failing` when that + /// file exists: a provider that fails partway through a derive, once the base is made. + private func failingRuntime() throws -> (runtime: URL, failing: URL) { + _ = try provisioning() + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let failing = directory.appendingPathComponent("failing") + let script = directory.appendingPathComponent("runtime") + try """ + #!/bin/sh + if [ -f \(PosixShell.quoted(failing.path)) ] && \\ + [ "$1" = "$(cat \(PosixShell.quoted(failing.path)))" ]; then + echo "injected failure" >&2; exit 1 + fi + exec \(PosixShell.quoted(runtime.path)) "$@" + + """.write(to: script, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: script.path) + return (script, failing) + } + + func testADeriveThatFailsAtEachStepLeavesNoContainerOrImageBehind() async throws { + let (script, failing) = try failingRuntime() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: try provisioning(runtime: script)) + let base = try await driver.create() + let baseContainers = try leftovers("ps", runtime: runtime, label: label) + + // `commit` makes the image and `run` the container; `port` and `exec` come once it is running. + // A failed `exec` reads as an identity never minted, so it carries no runtime message. + for step in ["commit", "run", "port", "exec"] { + try step.write(to: failing, atomically: true, encoding: .utf8) + do { + _ = try await driver.deriveFromBase(base) + XCTFail("a derive that failed at \(step) succeeded") + } catch HostDriverError.provisioning(let detail) { + XCTAssertTrue( + detail.contains(step == "exec" ? "never minted" : "injected failure"), + "\(step): \(detail)") + } + XCTAssertEqual( + try leftovers("ps", runtime: runtime, label: label), baseContainers, + "a derive that failed at \(step) left a container") + XCTAssertEqual( + try leftovers("images", runtime: runtime, label: label), [], + "a derive that failed at \(step) left an image") + } + try FileManager.default.removeItem(at: failing) + try await driver.destroy(base) + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) + } +} diff --git a/vcs/scripts/ssh-fixture/Dockerfile b/vcs/scripts/ssh-fixture/Dockerfile index c575d9596..995b79b62 100644 --- a/vcs/scripts/ssh-fixture/Dockerfile +++ b/vcs/scripts/ssh-fixture/Dockerfile @@ -10,7 +10,6 @@ RUN apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ openssh-server git procps \ && rm -rf /var/lib/apt/lists/* \ - && ssh-keygen -A \ && useradd --create-home --shell /bin/bash --password '*' workroom \ && printf '%s\n' \ 'PasswordAuthentication no' \ @@ -19,5 +18,5 @@ RUN apt-get update \ 'AllowUsers workroom' \ > /etc/ssh/sshd_config.d/fixture.conf -COPY --chmod=755 wr-agent entrypoint.sh /usr/local/bin/ +COPY --chmod=755 wr-agent entrypoint.sh identity.sh /usr/local/bin/ ENTRYPOINT ["/usr/local/bin/entrypoint.sh"] diff --git a/vcs/scripts/ssh-fixture/entrypoint.sh b/vcs/scripts/ssh-fixture/entrypoint.sh index 54370454c..673a8d736 100755 --- a/vcs/scripts/ssh-fixture/entrypoint.sh +++ b/vcs/scripts/ssh-fixture/entrypoint.sh @@ -9,6 +9,26 @@ # PID 1 has to reap them. set -eu +# The machine's identity is minted on its first boot, never inherited (#252, open question 9). A +# container committed from another and run again (`ContainerHostDriver.deriveFromBase`) starts with +# its source's disk: the ssh host keys (which openssh-server's install also bakes into the image), +# `/etc/machine-id`, and the agent's broker files beside its socket. The marker holds the hostname +# the identity was minted for. A restart keeps the container's hostname, so a reboot keeps its +# identity and its pinned host key; a new container has a new hostname, so it mints its own. The +# marker is written last: a boot that dies halfway mints again on the next one. +# +# Not re-mintable here: `/proc/sys/kernel/random/boot_id` is the kernel's, which every container +# on one machine shares. A VM provider's derived instance boots a kernel of its own. +IDENTITY=/etc/workroom-identity +if [ "$(cat "$IDENTITY" 2>/dev/null)" != "$(hostname)" ]; then + rm -f /etc/ssh/ssh_host_* + ssh-keygen -A + tr -d '-' < /proc/sys/kernel/random/uuid > /etc/machine-id + rm -f /run/workroom/broker.json /run/workroom/broker-token.json + rm -rf /home/workroom/.local/state/workroom/screens + hostname > "$IDENTITY" +fi + mkdir -p /run/sshd install -d -o workroom -g workroom -m 700 /run/workroom /home/workroom/.ssh printf '%s\n' "$AUTHORIZED_KEY" > /home/workroom/.ssh/authorized_keys diff --git a/vcs/scripts/ssh-fixture/identity.sh b/vcs/scripts/ssh-fixture/identity.sh new file mode 100755 index 000000000..bf4cff8ba --- /dev/null +++ b/vcs/scripts/ssh-fixture/identity.sh @@ -0,0 +1,8 @@ +#!/bin/sh +# Prints the container's ssh host key, ` `, once its entrypoint has minted its +# identity (#252), and fails until then: until the marker names this container, the keys on disk are +# its image's or are about to be replaced. run.sh and `ContainerHostDriver` read the key to pin +# through this, out of band. +set -eu +[ "$(cat /etc/workroom-identity 2>/dev/null)" = "$(hostname)" ] +cut -d' ' -f1,2 /etc/ssh/ssh_host_ed25519_key.pub diff --git a/vcs/scripts/ssh-fixture/run.sh b/vcs/scripts/ssh-fixture/run.sh index fc14775f5..835ed9e86 100755 --- a/vcs/scripts/ssh-fixture/run.sh +++ b/vcs/scripts/ssh-fixture/run.sh @@ -38,8 +38,18 @@ SCREENS="/home/workroom/.local/state/workroom/screens" STAGE="$(mktemp -d "${TMPDIR:-/tmp}/wr-ssh-fixture.XXXXXX")" # The image goes too: built by ID, one would pile up per run. Its layers stay in the build cache. +# So does whatever the app's tests provisioned from it (#252), which carries this run's label: +# containers first, then the images committed from them, which the fixture's image is a parent of. +LABEL="workroom.fixture=$NAME" cleanup() { "$RUNTIME" rm -f "$NAME" >/dev/null 2>&1 || true + for container in $("$RUNTIME" ps -aq --filter "label=$LABEL" 2>/dev/null); do + "$RUNTIME" rm -f "$container" >/dev/null 2>&1 || true + done + # `-a`: a commit is untagged, and `images` lists an untagged image only with it. + for image in $("$RUNTIME" images -aq --filter "label=$LABEL" 2>/dev/null); do + "$RUNTIME" rmi -f "$image" >/dev/null 2>&1 || true + done if [ -n "${IMAGE:-}" ]; then "$RUNTIME" rmi -f "$IMAGE" >/dev/null 2>&1 || true fi @@ -48,7 +58,7 @@ cleanup() { trap cleanup EXIT cp "$AGENT" "$STAGE/wr-agent" -cp "$HERE/Dockerfile" "$HERE/entrypoint.sh" "$STAGE/" +cp "$HERE/Dockerfile" "$HERE/entrypoint.sh" "$HERE/identity.sh" "$STAGE/" # By image ID, not a tag: a tag is shared, and another run building it between here and `run` below # would swap in a different agent. WR_FIXTURE_BUILD_FLAGS is word-split into extra build flags; CI # passes a GitHub Actions layer cache there so the apt layer is not rebuilt on every run. @@ -67,8 +77,20 @@ PORT="$("$RUNTIME" port "$NAME" 22/tcp | head -1 | sed 's/.*://')" # BatchMode cannot prompt to accept a host key, so the expected key is delivered out of band (here, # read straight out of the container) and pinned. That is the same policy a real driver needs. # Pinned under an alias rather than the port: a restart publishes a new port (the reboot test, -# #232), and the key must still match. -HOST_KEY="$("$RUNTIME" exec "$NAME" cat /etc/ssh/ssh_host_ed25519_key.pub | cut -d' ' -f1,2)" +# #232), and the key must still match. Read once the entrypoint has minted it (#252): until the +# marker names this container, the keys on disk are the image's or about to be replaced +# (identity.sh, which `ContainerHostDriver` reads the key through too). +HOST_KEY="" +for attempt in $(seq 1 100); do + HOST_KEY="$("$RUNTIME" exec "$NAME" identity.sh 2>/dev/null || true)" + [ -n "$HOST_KEY" ] && break + sleep 0.2 +done +if [ -z "$HOST_KEY" ]; then + echo "error: the fixture never minted its identity; its log:" >&2 + "$RUNTIME" logs "$NAME" >&2 + exit 1 +fi printf 'wr-ssh-fixture %s\n' "$HOST_KEY" > "$STAGE/known_hosts" cat > "$STAGE/ssh_config" < Date: Thu, 1 Oct 2026 18:36:41 +0100 Subject: [PATCH 2/9] feat(cli): workroom host set|clear records a host descriptor The app owns the host descriptor's schema (#249), but nothing could write one. `workroom host set --project P [--workroom W] DESCRIPTOR` stores a JSON object verbatim as the project's or workroom's "host", and `host clear` removes it. #252 records a project's base machine this way. It never creates an entry: an unknown project or workroom is refused (ProjectNotFound, WorkroomNotFound), and so is a descriptor that isn't an object (InvalidHostDescriptor), JSON null included, which would otherwise turn a remote workroom local. The command is hidden; it is plumbing for the app. The new error kinds are additive, so schema_version stays 1. Co-Authored-By: Claude Opus 5.5 --- cmd/host.go | 78 ++++++++++++++++++++++++++++++++++ internal/config/config.go | 29 +++++++++++++ internal/config/config_test.go | 58 +++++++++++++++++++++++++ internal/errs/errs.go | 6 +++ internal/errs/errs_test.go | 1 + 5 files changed, 172 insertions(+) create mode 100644 cmd/host.go diff --git a/cmd/host.go b/cmd/host.go new file mode 100644 index 000000000..a6e56ac30 --- /dev/null +++ b/cmd/host.go @@ -0,0 +1,78 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "os" + + "github.com/joelmoss/workroom/internal/config" + "github.com/joelmoss/workroom/internal/errs" + "github.com/spf13/cobra" +) + +var ( + hostProject string + hostWorkroom string +) + +// hostCmd records the host descriptors the desktop app owns (#249, #252): which machine a remote +// workroom lives on, and a project's base machine. The descriptor is stored verbatim; the CLI +// interprets only whether a workroom has one and its "state". +var hostCmd = &cobra.Command{ + Use: "host", + Short: "Set or clear a project's or workroom's host descriptor (used by the desktop app)", + Hidden: true, +} + +var hostSetCmd = &cobra.Command{ + Use: "set DESCRIPTOR", + Short: "Store DESCRIPTOR, a JSON object, as the host descriptor", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + currentCommand = "host" + var host map[string]any + // A JSON null decodes to a nil map, which would clear the descriptor, so it is refused + // with every other non-object. + if err := json.Unmarshal([]byte(args[0]), &host); err != nil || host == nil { + return fmt.Errorf("%w: %s", errs.ErrInvalidHost, args[0]) + } + return setHost(host) + }, +} + +var hostClearCmd = &cobra.Command{ + Use: "clear", + Short: "Remove the host descriptor", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + currentCommand = "host" + return setHost(nil) + }, +} + +func setHost(host map[string]any) error { + cfg, err := config.New("") + if err != nil { + return err + } + dir, err := resolveProject(hostProject) + if err != nil { + return err + } + if err := cfg.SetHost(dir, hostWorkroom, host); err != nil { + return err + } + if jsonOutput { + return writeJSONSuccess(os.Stdout, "host", map[string]any{"project": dir, "workroom": hostWorkroom}) + } + return nil +} + +func init() { + for _, c := range []*cobra.Command{hostSetCmd, hostClearCmd} { + c.Flags().StringVar(&hostProject, "project", "", "Project directory (defaults to the current directory)") + c.Flags().StringVar(&hostWorkroom, "workroom", "", "The project's workroom; omitted, the project's own descriptor") + hostCmd.AddCommand(c) + } + rootCmd.AddCommand(hostCmd) +} diff --git a/internal/config/config.go b/internal/config/config.go index a32f109f9..038434438 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -340,6 +340,35 @@ func (c *Config) SetProjectVCS(parentPath, vcs string) error { }) } +// SetHost stores host verbatim as the "host" descriptor of a project (workroom "") or of one of +// its workrooms (#252). The app owns the schema (#249); this only stores it. A nil host removes the +// descriptor, which turns a workroom local again. The project, and the workroom, must already be +// registered: a descriptor alone is never a new entry. +func (c *Config) SetHost(parentPath, workroom string, host map[string]any) error { + return c.withLock(func() error { + data, err := c.Read() + if err != nil { + return err + } + entry, ok := data[parentPath].(map[string]any) + if !ok || isReserved(parentPath) { + return fmt.Errorf("%w: %s", errs.ErrProjectNotFound, parentPath) + } + if workroom != "" { + workrooms, _ := entry["workrooms"].(map[string]any) + if entry, ok = workrooms[workroom].(map[string]any); !ok { + return fmt.Errorf("%w: '%s' in %s", errs.ErrWorkroomNotFound, workroom, parentPath) + } + } + if host == nil { + delete(entry, "host") + } else { + entry["host"] = host + } + return c.Write(data) + }) +} + // RemoveWorkroom removes a workroom entry. If the parent has no remaining workrooms, it is // removed, unless it carries a host descriptor: that describes a remote machine, and dropping it // with the entry would lose the only record of it. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 558cd56fa..7f8935404 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -456,3 +456,61 @@ func TestDecodeHostDescriptors(t *testing.T) { t.Fatalf("RemoteWorkroomNames = %v", got) } } + +func TestSetHostStoresAndClearsDescriptorsVerbatim(t *testing.T) { + c := newTestConfig(t) + if err := c.AddWorkroom("/project", "foo", "/home/workroom/foo", "git"); err != nil { + t.Fatal(err) + } + base := map[string]any{"base": map[string]any{"container": "abc", "port": json.Number("2222")}} + if err := c.SetHost("/project", "", base); err != nil { + t.Fatal(err) + } + if err := c.SetHost("/project", "foo", map[string]any{"state": "running"}); err != nil { + t.Fatal(err) + } + + projects, err := c.AllProjects() + if err != nil { + t.Fatal(err) + } + project := projects["/project"] + if !reflect.DeepEqual(project.Host, base) { + t.Fatalf("project host = %#v, want %#v", project.Host, base) + } + if !project.Workrooms["foo"].IsRemote() || project.Workrooms["foo"].Path != "/home/workroom/foo" { + t.Fatalf("workroom = %#v, want remote and its path kept", project.Workrooms["foo"]) + } + + if err := c.SetHost("/project", "foo", nil); err != nil { + t.Fatal(err) + } + if err := c.SetHost("/project", "", nil); err != nil { + t.Fatal(err) + } + projects, _ = c.AllProjects() + if projects["/project"].Host != nil || projects["/project"].Workrooms["foo"].IsRemote() { + t.Fatalf("a cleared descriptor survived: %#v", projects["/project"]) + } +} + +func TestSetHostNeverCreatesAnEntry(t *testing.T) { + c := newTestConfig(t) + if err := c.AddProject("/project", "git"); err != nil { + t.Fatal(err) + } + host := map[string]any{"state": "running"} + if err := c.SetHost("/elsewhere", "", host); !errors.Is(err, errs.ErrProjectNotFound) { + t.Fatalf("unknown project: err = %v, want ErrProjectNotFound", err) + } + if err := c.SetHost("/project", "missing", host); !errors.Is(err, errs.ErrWorkroomNotFound) { + t.Fatalf("unknown workroom: err = %v, want ErrWorkroomNotFound", err) + } + if err := c.SetHost("workrooms_dir", "", host); !errors.Is(err, errs.ErrProjectNotFound) { + t.Fatalf("reserved key: err = %v, want ErrProjectNotFound", err) + } + data, _ := c.Read() + if len(data) != 1 { + t.Fatalf("config gained entries: %#v", data) + } +} diff --git a/internal/errs/errs.go b/internal/errs/errs.go index 23979cd69..6b0384fe8 100644 --- a/internal/errs/errs.go +++ b/internal/errs/errs.go @@ -22,6 +22,9 @@ var ( ErrVCSCommand = errors.New("version control command failed") ErrRemoteProject = errors.New("remote projects are not supported: add the project from a local Git or JJ repository. A remote workroom belongs to a local project") ErrRemoteWorkroom = errors.New("not supported for a remote workroom yet") + ErrProjectNotFound = errors.New("project is not registered") + ErrWorkroomNotFound = errors.New("workroom is not registered") + ErrInvalidHost = errors.New("a host descriptor must be a JSON object") ) // classification is one sentinel error's entry in the registry: its stable --json code and @@ -56,6 +59,9 @@ var registry = []classification{ {ErrVCSCommand, "VCSCommandFailed", 1}, {ErrRemoteProject, "RemoteProjectUnsupported", 3}, {ErrRemoteWorkroom, "RemoteWorkroomUnsupported", 3}, + {ErrProjectNotFound, "ProjectNotFound", 3}, + {ErrWorkroomNotFound, "WorkroomNotFound", 3}, + {ErrInvalidHost, "InvalidHostDescriptor", 2}, } func classify(err error) (classification, bool) { diff --git a/internal/errs/errs_test.go b/internal/errs/errs_test.go index e31aadcfa..41d429d3e 100644 --- a/internal/errs/errs_test.go +++ b/internal/errs/errs_test.go @@ -48,6 +48,7 @@ func TestRegistryCoversEveryPublicSentinel(t *testing.T) { ErrJJWorkspaceExists, ErrGitWorktreeExists, ErrJJWorkspaceNotFound, ErrGitWorktreeNotFound, ErrSetup, ErrTeardown, ErrConfirmMismatch, ErrUnsafeDeletePath, ErrCancelled, ErrConfigRead, ErrConfigWrite, ErrVCSCommand, ErrRemoteProject, ErrRemoteWorkroom, + ErrProjectNotFound, ErrWorkroomNotFound, ErrInvalidHost, } if len(registry) != len(sentinels) { t.Fatalf("registry has %d entries, expected %d (one per exported sentinel)", len(registry), len(sentinels)) From fe81096b9674fbf94f4d7e34a673c8348172c561 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 18:55:41 +0100 Subject: [PATCH 3/9] feat(macapp): build a base machine and derive enrolled workrooms from it Slice B of #252: the portable creation sequence, in RemoteProvisioning. A base is provisioned, its repository cloned through the agent's exec service, and recorded through a caller's closure (`workroom host set`, via the new WorkroomCLI.setHost). The clone uses a one-shot read-only token from the broker (BrokerClient.baseCloneToken). It reaches git only as an Authorization header for github.com in the exec request's environment (GIT_CONFIG_COUNT), never in an argument, the remote URL or a file, and it is revoked at GitHub once the clone is over. The base never enrols. refreshBase fetches with a fresh token. A workroom runs derive, then enrol (AgentEnrolment, so each key is its own), then fetch and check out its own branch from origin/HEAD, and is returned serving. A failure at any step cancels the grant and destroys the instance; a failed base build destroys the base and revokes its token. The fixture image now carries a seeded /srv/origin.git for the base to clone by path, since the fixture has no GitHub. Tests: 5 RemoteProvisioningIntegrationTests through run.sh, plus unit tests for the clone environment and the token request. The full app suite passes through the fixture (3108 passed, one pre-existing eval skip), and so do the 5 over_ssh Rust tests. Three negative controls go red: dropping the grant cancel, dropping the instance destroy, and passing the token as `clone -c`, which the disk search finds in .git/config. Co-Authored-By: Claude Opus 5.5 --- .../Core/Broker/BrokerClient.swift | 11 + .../WorkroomApp/Core/RemoteProvisioning.swift | 251 ++++++++++++ macapp/WorkroomApp/Core/WorkroomCLI.swift | 15 + .../Broker/BrokerClientTests.swift | 12 + .../RemoteProvisioningIntegrationTests.swift | 367 ++++++++++++++++++ .../RemoteProvisioningTests.swift | 20 + vcs/scripts/ssh-fixture/Dockerfile | 11 + 7 files changed, 687 insertions(+) create mode 100644 macapp/WorkroomApp/Core/RemoteProvisioning.swift create mode 100644 macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift create mode 100644 macapp/WorkroomAppTests/RemoteProvisioningTests.swift diff --git a/macapp/WorkroomApp/Core/Broker/BrokerClient.swift b/macapp/WorkroomApp/Core/Broker/BrokerClient.swift index ea316332e..9aeba5919 100644 --- a/macapp/WorkroomApp/Core/Broker/BrokerClient.swift +++ b/macapp/WorkroomApp/Core/Broker/BrokerClient.swift @@ -146,6 +146,13 @@ struct BrokerClient: Sendable { let expiresAt: String } + /// A read-only installation token for cloning a base machine (#252). The broker does not keep + /// it, so only GitHub can revoke it (`CloneToken.revoke`); otherwise it lives out its hour. + struct CloneToken: Decodable, Sendable { + let token: String + let expiresAt: String + } + private struct State: Decodable { let state: String } /// Redeems the loopback code, registering this client's key as a new device. @@ -169,6 +176,10 @@ struct BrokerClient: Sendable { let _: State = try await send("DELETE", "broker/grants/\(grantID)", body: nil) } + func baseCloneToken(repository: String) async throws -> CloneToken { + try await send("POST", "broker/base-clone-tokens", body: ["repository": repository]) + } + /// One signed request. A `stale_proof` refusal carries the broker's `Date`; the request is /// signed again on that clock and sent once more. private func send( diff --git a/macapp/WorkroomApp/Core/RemoteProvisioning.swift b/macapp/WorkroomApp/Core/RemoteProvisioning.swift new file mode 100644 index 000000000..007e5fe56 --- /dev/null +++ b/macapp/WorkroomApp/Core/RemoteProvisioning.swift @@ -0,0 +1,251 @@ +import Foundation + +/// Making remote workrooms the portable way (#252; design doc Phase 4, "Remote creation is one +/// sequence"). A project has one **base machine**, its repository cloned and an agent present, +/// which never enrols. Every workroom is derived from it: +/// **derive → re-init machine identity → enrol → fetch and check out → serve.** +/// A driver's speed differs, never the sequence. Re-initialising the machine's identity is the +/// driver's part of the derive (`HostDriver.deriveFromBase`), so a derived instance serves nothing +/// as its base. +/// +/// Every step before the last undoes what the sequence made when a later one fails: the instance, +/// its enrolment grant, and a base's clone token. +/// +/// Repository work runs through the agent's exec service, and the Go CLI stays on the Mac. +enum RemoteProvisioning { + /// What the sequence reaches a provider and the broker through. + struct Environment: Sendable { + let driver: any HostDriver + /// The agent's socket on each host of `driver`, which puts the agent's binary beside it. + let agentSocket: String + let client: BrokerClient + var agentBroker: AgentEnrolment.AgentBroker = .standard + /// A connection to the agent on a host, bootstrapping one there first. + var connect: @Sendable (HostID) async throws -> AgentVCSConnection + /// Lets go of a base's clone token once the clone is done, whether or not it succeeded. + var revoke: @Sendable (BrokerClient.CloneToken) async -> Void = CloneToken.revoke + + init( + driver: any HostDriver, agentSocket: String, client: BrokerClient, + agentBroker: AgentEnrolment.AgentBroker = .standard, + connect: (@Sendable (HostID) async throws -> AgentVCSConnection)? = nil, + revoke: @escaping @Sendable (BrokerClient.CloneToken) async -> Void = CloneToken.revoke + ) { + self.driver = driver + self.agentSocket = agentSocket + self.client = client + self.agentBroker = agentBroker + self.connect = + connect ?? { host in + try await AgentBootstrap.connect(host: host, driver: driver, socket: agentSocket) + } + self.revoke = revoke + } + + var agentBinary: String { AgentBootstrap.binary(besideSocket: agentSocket) } + } + + /// A project's base machine, as the project's host descriptor records it. + struct Base: Codable, Equatable, Sendable { + let host: UUID + /// `owner/name` on GitHub, which the broker mints tokens for. + let repository: String + let cloneURL: String + /// The clone on the host. + let path: String + } + + /// A workroom derived from a base, serving requests over `connection`. + struct Instance: Sendable { + let host: HostID + let grantID: String + let path: String + let branch: String + let connection: AgentVCSConnection + } + + enum Failure: Error, Equatable, LocalizedError { + case git(command: String, detail: String) + + var errorDescription: String? { + switch self { + case .git(let command, let detail): return "git \(command) failed on the host: \(detail)" + } + } + } + + /// Silence allowed on one git command. A clone of a large repository over a slow link is long, + /// but it is not quiet for this long. + static let gitTimeout: TimeInterval = 600 + + // MARK: The base + + /// Provisions a base, clones `cloneURL` into `path` there with a one-shot read-only token from + /// the broker, and records it with `record`. The token reaches git only through the exec + /// request's environment (`cloneEnvironment`), never its arguments, the remote URL or the disk, + /// and is revoked once the clone is over. A failure removes the base. + static func buildBase( + repository: String, cloneURL: String, path: String, in environment: Environment, + record: @Sendable (Base) async throws -> Void + ) async throws -> Base { + let host = try await environment.driver.create() + do { + guard case .remote(let id) = host else { throw HostDriverError.unknownHost(host) } + let connection = try await environment.connect(host) + defer { Task { await connection.close() } } + try await withCloneToken(repository, in: environment) { header in + _ = try await git( + ["clone", "--quiet", "--origin", "origin", "--", cloneURL, path], in: "/", + environment: header, on: connection) + } + let base = Base(host: id, repository: repository, cloneURL: cloneURL, path: path) + try await record(base) + return base + } catch { + await cleanUp { try await environment.driver.destroy(host) } + throw error + } + } + + /// Brings the base's clone up to date with the remote, for workrooms derived from it later: a + /// derived workroom fetches what it lacks itself, so this only shortens that fetch. The base + /// never enrols, so it fetches with a clone token too. + static func refreshBase(_ base: Base, in environment: Environment) async throws { + let connection = try await environment.connect(.remote(base.host)) + defer { Task { await connection.close() } } + try await withCloneToken(base.repository, in: environment) { header in + _ = try await git( + ["fetch", "--quiet", "--prune", "origin"], in: base.path, environment: header, + on: connection) + } + } + + /// Removes the base and its record. Workrooms derived from it are not affected: each is a copy. + static func destroyBase( + _ base: Base, in environment: Environment, forget: @Sendable () async throws -> Void + ) async throws { + try await environment.driver.destroy(.remote(base.host)) + try await forget() + } + + /// Mints a clone token, hands `body` the exec environment carrying it, and revokes it after, + /// whatever `body` did. + private static func withCloneToken( + _ repository: String, in environment: Environment, + _ body: ([String: String]) async throws -> Void + ) async throws { + let token = try await environment.client.baseCloneToken(repository: repository) + do { + try await body(cloneEnvironment(token: token.token)) + } catch { + await cleanUp { await environment.revoke(token) } + throw error + } + await cleanUp { await environment.revoke(token) } + } + + /// git's configuration through its environment (`GIT_CONFIG_COUNT`), so the token is in no + /// argument a process list shows, in no URL git stores, and in no file: an `Authorization` + /// header for github.com only, which is how GitHub takes an installation token over https. + static func cloneEnvironment(token: String) -> [String: String] { + let credentials = Data("x-access-token:\(token)".utf8).base64EncodedString() + return [ + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "http.https://github.com/.extraHeader", + "GIT_CONFIG_VALUE_0": "Authorization: Basic \(credentials)", + ] + } + + // MARK: A workroom + + /// Derives a workroom from `base`, enrols it, and checks its own `branch` out from the remote's + /// default branch. Returns it serving. On a failure at any step, the grant is cancelled (by the + /// enrolment itself when it was the step that failed) and the instance destroyed. + static func derive( + from base: Base, workroom: UUID, branch: String, in environment: Environment + ) async throws -> Instance { + let host = try await environment.driver.deriveFromBase(.remote(base.host)) + var connection: AgentVCSConnection? + var grant: String? + do { + let connected = try await environment.connect(host) + connection = connected + let grantID = try await AgentEnrolment.enrol( + client: environment.client, driver: environment.driver, host: host, + agentBinary: environment.agentBinary, workroomID: workroom, + repository: base.repository, agentBroker: environment.agentBroker) + grant = grantID + // With the instance's own credentials: `wr-agent enrol` made its helper git's. + _ = try await git( + ["fetch", "--quiet", "--prune", "origin"], in: base.path, on: connected) + _ = try await git( + ["switch", "--quiet", "--no-track", "--create", branch, "origin/HEAD"], in: base.path, + on: connected) + return Instance( + host: host, grantID: grantID, path: base.path, branch: branch, connection: connected) + } catch { + let (made, client) = (grant, environment.client) + await cleanUp { + await connection?.close() + if let made { try? await client.cancelGrant(made) } + await environment.agentBroker.release(workroom) + try await environment.driver.destroy(host) + } + throw error + } + } + + /// Takes a workroom down: its grant first, so nothing mints for it once its box is gone, then + /// the box. + static func destroy(_ instance: Instance, workroom: UUID, in environment: Environment) + async throws + { + await instance.connection.close() + try await environment.client.cancelGrant(instance.grantID) + await environment.agentBroker.release(workroom) + try await environment.driver.destroy(instance.host) + } + + // MARK: Plumbing + + /// One git command on the host, through the agent's exec service: the host's own environment, + /// the remote policy pins, and `extra` on top. + static func git( + _ args: [String], in directory: String, environment extra: [String: String] = [:], + on connection: AgentVCSConnection + ) async throws -> String { + var request = AgentCommandRunner.request( + "git", args, in: directory, timeout: gitTimeout, stdin: nil, network: true, + host: connection.host) + request.env.merge(extra) { _, new in new } + let reply = try await connection.request(request, timeout: gitTimeout + 15) + let result = try AgentVCSReply.decode(reply) + guard result.exitCode == 0, !result.timedOut, !result.signaled else { + throw Failure.git( + command: args.first ?? "", + detail: result.stderr.trimmingCharacters(in: .whitespacesAndNewlines)) + } + return result.stdout + } + + /// Undoes a step whatever the caller's state: in a task of its own, so a cancelled sequence + /// still cleans up after itself. + private static func cleanUp(_ body: @escaping @Sendable () async throws -> Void) async { + await Task { try? await body() }.value + } +} + +extension RemoteProvisioning { + enum CloneToken { + /// Revokes an installation token at GitHub, which accepts the call authenticated as the token + /// itself. Best effort: one that fails still expires within its hour. + @Sendable static func revoke(_ token: BrokerClient.CloneToken) async { + guard let url = URL(string: "https://api.github.com/installation/token") else { return } + var request = URLRequest(url: url, timeoutInterval: 15) + request.httpMethod = "DELETE" + request.setValue("Bearer \(token.token)", forHTTPHeaderField: "Authorization") + request.setValue("application/vnd.github+json", forHTTPHeaderField: "Accept") + _ = try? await URLSession.shared.data(for: request) + } + } +} diff --git a/macapp/WorkroomApp/Core/WorkroomCLI.swift b/macapp/WorkroomApp/Core/WorkroomCLI.swift index 2577787d6..7f03140fd 100644 --- a/macapp/WorkroomApp/Core/WorkroomCLI.swift +++ b/macapp/WorkroomApp/Core/WorkroomCLI.swift @@ -156,6 +156,21 @@ final class WorkroomCLI: WorkroomCLIProtocol { return try decode(ListResponse.self, from: result) } + /// Stores `descriptor` as the host descriptor of `project`, or of its `workroom` (#252), or + /// clears it when `descriptor` is nil. The app owns the descriptor's schema; the CLI stores it + /// verbatim and never creates an entry for it. + func setHost(project: String, workroom: String? = nil, descriptor: Data?) async throws { + var args = ["host"] + if let descriptor { + args += ["set", String(decoding: descriptor, as: UTF8.self)] + } else { + args.append("clear") + } + args += ["--json", "--project", project] + if let workroom { args += ["--workroom", workroom] } + try throwIfError(try await run(args, timeout: 5)) + } + /// Registers a project. With `create`, the CLI creates and git-initializes the /// directory if it does not already exist (issue #103); otherwise the path must /// already be a Git/JJ repo. Returns the canonical path the CLI registered (used diff --git a/macapp/WorkroomAppTests/Broker/BrokerClientTests.swift b/macapp/WorkroomAppTests/Broker/BrokerClientTests.swift index 4a23949f3..5355668f1 100644 --- a/macapp/WorkroomAppTests/Broker/BrokerClientTests.swift +++ b/macapp/WorkroomAppTests/Broker/BrokerClientTests.swift @@ -271,6 +271,18 @@ final class BrokerClientTests: XCTestCase { XCTAssertEqual(body["workroom_id"] as? String, id.uuidString.lowercased()) XCTAssertEqual(body["repository"] as? String, "o/r") } + + func testABaseCloneTokenIsAskedForByRepository() async throws { + BrokerStub.reset([.init(body: #"{"token":"ghs_x","expires_at":"2026-10-01T18:00:00Z"}"#)]) + + let token = try await client().baseCloneToken(repository: "o/r") + + XCTAssertEqual(token.token, "ghs_x") + let seen = try XCTUnwrap(BrokerStub.requests.first) + XCTAssertEqual(seen.request.httpMethod, "POST") + XCTAssertEqual(seen.request.url?.path, "/broker/base-clone-tokens") + XCTAssertEqual(seen.json["repository"] as? String, "o/r") + } } final class BrokerCredentialsTests: XCTestCase { diff --git a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift new file mode 100644 index 000000000..2b52f4aa7 --- /dev/null +++ b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift @@ -0,0 +1,367 @@ +import CryptoKit +import XCTest + +@testable import Workroom + +/// The derivation sequence (#252) on the ssh fixture's containers: a base built and cloned, and +/// workrooms derived, enrolled and checked out from it, with every failure undoing what it made. +/// The base clones the fixture image's `/srv/origin.git` by path, since the fixture has no +/// GitHub; the token's handling is pinned by `RemoteProvisioningTests`. The Mac's broker calls go +/// to `BrokerStub`, and each agent's enrolment to a `StubBroker` on this Mac through the agent's +/// own loopback, as a Debug build's does. Skipped unless run through the fixture script: +/// +/// vcs/scripts/ssh-fixture/run.sh \ +/// make app-test APP_TEST_FLAGS=-only-testing:WorkroomAppTests/RemoteProvisioningIntegrationTests +final class RemoteProvisioningIntegrationTests: XCTestCase { + private var directory: URL! + private var label: String! + private var runtime: URL! + private var broker: StubBroker! + private var cleanups: [() async -> Void] = [] + + private static let token = "ghs_fixture_clone_token_0123456789" + private static let path = "/home/workroom/project" + + override func setUp() async throws { + try await super.setUp() + directory = FileManager.default.temporaryDirectory.appendingPathComponent( + "wr-derive-\(UUID().uuidString.prefix(8))") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + label = "workroom.test=\(UUID().uuidString.lowercased())" + broker = try StubBroker() + } + + override func tearDown() async throws { + for cleanup in cleanups.reversed() { await cleanup() } + cleanups.removeAll() + broker?.stop() + if let runtime, let label { + for container in (try? leftovers("ps", label: label)) ?? [] { + _ = try? docker(runtime, ["rm", "--force", "--volumes", container]) + } + for image in (try? leftovers("images", label: label)) ?? [] { + _ = try? docker(runtime, ["rmi", "--force", image]) + } + } + try? FileManager.default.removeItem(at: directory) + try await super.tearDown() + } + + private struct Fixture { + let provisioning: ContainerHostDriver.Provisioning + let agent: URL + } + + private func fixture(runtime override: URL? = nil) 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 + } + runtime = URL(fileURLWithPath: try need("WR_SSH_FIXTURE_RUNTIME_PATH")) + return Fixture( + provisioning: ContainerHostDriver.Provisioning( + runtime: override ?? runtime, image: try need("WR_SSH_FIXTURE_IMAGE"), + user: try need("WR_SSH_FIXTURE_USER"), identityFile: try need("WR_SSH_FIXTURE_IDENTITY"), + publicKey: try need("WR_SSH_FIXTURE_PUBLIC_KEY"), + agentSocket: try need("WR_SSH_FIXTURE_SOCKET"), + labels: [try need("WR_SSH_FIXTURE_LABEL"), label]), + agent: URL(fileURLWithPath: try need("WR_SSH_FIXTURE_AGENT"))) + } + + /// Revoked clone tokens, in order. + private final class Revoked: @unchecked Sendable { + private let lock = NSLock() + private var tokens: [String] = [] + func add(_ token: String) { lock.withLock { tokens.append(token) } } + var all: [String] { lock.withLock { tokens } } + } + + /// The sequence's environment over `driver`. The agent's broker is a reverse forward to + /// `broker` (or `brokerURL`, to make the agent's enrolment fail), over a connection of its own + /// to the host, as the app's registry holds one. + @MainActor + private func environment( + _ fixture: Fixture, driver: ContainerHostDriver, revoked: Revoked, + connect: (@Sendable (HostID) async throws -> AgentVCSConnection)? = nil, + brokerURL: URL? = nil + ) -> RemoteProvisioning.Environment { + let broker = self.broker! + let generation = UUID() + let registry = BrokerReverseForwards( + transport: .init( + forwarding: { host in + let connection = try await AgentVCSConnection.connect( + host: host, stream: try await driver.openStream(to: host)) + return (.init(host: host, generation: generation), try connection.forwarding()) + }, + updates: { host in + AsyncStream { + $0.yield(.init(lease: .init(host: host, generation: generation), status: .connected)) + } + }), + target: { broker.port }) + let agent = fixture.agent + return RemoteProvisioning.Environment( + driver: driver, agentSocket: fixture.provisioning.agentSocket, + client: BrokerClient( + baseURL: BrokerEndpoint.development, key: .software(P256.Signing.PrivateKey()), + session: BrokerStub.session), + agentBroker: .init( + url: { _, workroom, host in + if let brokerURL { return brokerURL } + return try await registry.open(workroom: workroom, host: host) + }, + release: { workroom in await registry.close(workroom: workroom) }), + connect: connect ?? { host in + try await AgentBootstrap.connect( + host: host, driver: driver, socket: fixture.provisioning.agentSocket, + agent: { $0 == "aarch64" || $0 == "x86_64" ? agent : nil }, handOff: false, + resources: nil) + }, + revoke: { revoked.add($0.token) }) + } + + private static let cloneToken = BrokerStub.Answer( + body: #"{"token":"\#(token)","expires_at":"2026-10-01T18:00:00Z"}"#) + private static let grant = BrokerStub.Answer( + status: 201, + body: #"{"grant_id":"g1","enrolment_code":"one-time","repository_id":1,"expires_at":"x"}"#) + private static let cancelled = BrokerStub.Answer(body: #"{"grant_id":"g1","state":"cancelled"}"#) + + @discardableResult + private func docker(_ runtime: URL, _ arguments: [String]) throws -> String { + let process = Process() + process.executableURL = runtime + process.arguments = arguments + let output = Pipe() + process.standardOutput = output + process.standardError = output + try process.run() + let data = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + return String(decoding: data, as: UTF8.self).trimmingCharacters(in: .whitespacesAndNewlines) + } + + /// `-a` for both: a commit is untagged, and `images` lists an untagged image only with it. + private func leftovers(_ kind: String, label: String) throws -> [String] { + try docker(runtime, [kind, "-a", "-q", "--filter", "label=\(label)"]) + .split(separator: "\n").map(String.init) + } + + /// A shell command on the host and its exit status: through the driver's ssh, never the agent. + private func onHost(_ driver: ContainerHostDriver, _ host: HostID, _ command: String) + async throws -> (status: Int32, output: String) + { + let (status, output) = try await driver.exec(command, on: host).communicate(nil, timeout: 30) + return (status, output.trimmingCharacters(in: .whitespacesAndNewlines)) + } + + private func build(_ environment: RemoteProvisioning.Environment) async throws + -> RemoteProvisioning.Base + { + BrokerStub.reset([Self.cloneToken]) + return try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "/srv/origin.git", path: Self.path, in: environment, + record: { _ in }) + } + + private var grantsCancelled: Int { + BrokerStub.requests.filter { + $0.request.httpMethod == "DELETE" && $0.request.url?.path == "/broker/grants/g1" + }.count + } + + // MARK: The base + + @MainActor + func testABaseIsClonedAndRecordedAndHoldsNoEnrolmentAndNoToken() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let revoked = Revoked() + let recorded = Revoked() + BrokerStub.reset([Self.cloneToken]) + + let base = try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "/srv/origin.git", path: Self.path, + in: environment(fixture, driver: driver, revoked: revoked), + record: { recorded.add(String(decoding: try JSONEncoder().encode($0), as: UTF8.self)) }) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + + let host = HostID.remote(base.host) + let readme = try await onHost(driver, host, "cat \(Self.path)/README") + XCTAssertEqual(readme.output, "origin") + let remote = try await onHost(driver, host, "git -C \(Self.path) remote get-url origin") + XCTAssertEqual( + remote.output, "/srv/origin.git", "the remote URL carries something besides the repository") + XCTAssertEqual(revoked.all, [Self.token], "the clone token outlived the clone") + XCTAssertEqual(recorded.all.count, 1) + let seen = try XCTUnwrap(BrokerStub.requests.first) + XCTAssertEqual(seen.request.url?.path, "/broker/base-clone-tokens") + XCTAssertEqual(seen.json["repository"] as? String, "o/r") + // The base never enrols, and the token is nowhere on its disk. + let enrolment = try await onHost(driver, host, "test -e /run/workroom/broker.json") + XCTAssertNotEqual(enrolment.status, 0, "the base holds an enrolment") + let helper = try await onHost(driver, host, "git config --global --get-regexp '^credential'") + XCTAssertNotEqual(helper.status, 0, "the base has a credential helper") + XCTAssertTrue(broker.requests.isEmpty, "the base reached the agents' broker") + // Every file the ssh user can have written (git and the agent run as that user), for the + // token as it is and as git's header carries it. A file holding the token is then written on + // purpose, so the search is shown able to find one. + let encoded = Data("x-access-token:\(Self.token)".utf8).base64EncodedString() + let search = + "find /home /run /tmp /srv /var /etc -xdev -type f -user workroom " + + "-exec grep -lF -e \(Self.token) -e \(encoded) {} + 2>/dev/null; echo end" + let found = try await onHost(driver, host, search).output + XCTAssertEqual(found, "end", "the clone token is on the base's disk") + _ = try await onHost(driver, host, "echo \(Self.token) > /tmp/probe") + let control = try await onHost(driver, host, search).output + XCTAssertEqual(control, "/tmp/probe\nend", "the search cannot find the token") + } + + @MainActor + func testABaseWhoseCloneFailsIsRemovedAndItsTokenRevoked() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let revoked = Revoked() + BrokerStub.reset([Self.cloneToken]) + + do { + _ = try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "/srv/no-such.git", path: Self.path, + in: environment(fixture, driver: driver, revoked: revoked), record: { _ in }) + XCTFail("a clone of nothing succeeded") + } catch RemoteProvisioning.Failure.git(let command, _) { + XCTAssertEqual(command, "clone") + } + + XCTAssertEqual(revoked.all, [Self.token]) + XCTAssertEqual(try leftovers("ps", label: label), [], "a failed base was left running") + } + + @MainActor + func testABaseThatCannotBeRecordedIsRemoved() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + BrokerStub.reset([Self.cloneToken]) + + do { + _ = try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "/srv/origin.git", path: Self.path, + in: environment(fixture, driver: driver, revoked: Revoked()), + record: { _ in throw WorkroomCLIError.timedOut }) + XCTFail("a base that was never recorded was kept") + } catch WorkroomCLIError.timedOut {} + + XCTAssertEqual(try leftovers("ps", label: label), []) + } + + // MARK: Workrooms + + @MainActor + func testWorkroomsDerivedFromABaseServeOnTheirOwnBranchWithTheirOwnEnrolment() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + + var keys: [String] = [] + for branch in ["wr-one", "wr-two"] { + BrokerStub.reset([Self.grant]) + let workroom = UUID() + let instance = try await RemoteProvisioning.derive( + from: base, workroom: workroom, branch: branch, in: environment) + cleanups.append { try? await driver.destroy(instance.host) } + + XCTAssertEqual(instance.grantID, "g1") + // It serves: a request over its own connection, answered by its own agent. + let head = try await RemoteProvisioning.git( + ["rev-parse", "--abbrev-ref", "HEAD"], in: instance.path, on: instance.connection) + XCTAssertEqual(head.trimmingCharacters(in: .whitespacesAndNewlines), branch) + let state = try await onHost(driver, instance.host, "cat /run/workroom/broker.json").output + let enrolment = try XCTUnwrap( + JSONSerialization.jsonObject(with: Data(state.utf8)) as? [String: Any]) + XCTAssertEqual(enrolment["enrolled"] as? Bool, true) + XCTAssertEqual(enrolment["workroom_id"] as? String, workroom.uuidString.lowercased()) + keys.append(try XCTUnwrap(enrolment["key"] as? String)) + + BrokerStub.reset([Self.cancelled]) + try await RemoteProvisioning.destroy(instance, workroom: workroom, in: environment) + XCTAssertEqual(grantsCancelled, 1, "destroying a workroom left its grant live") + } + XCTAssertNotEqual(keys[0], keys[1], "two workrooms share an enrolment key") + XCTAssertEqual(try leftovers("ps", label: label).count, 1, "only the base should be left") + } + + /// A failure at each step after the derive starts leaves no instance and no live grant behind. + @MainActor + func testAWorkroomThatFailsAtEachStepLeavesNoInstanceAndNoGrant() async throws { + // The runtime fails `commit` only when told to, so the base is built with it as it is. + let script = directory.appendingPathComponent("runtime") + let failing = directory.appendingPathComponent("failing") + let fixture = try fixture(runtime: script) + try """ + #!/bin/sh + if [ -f \(PosixShell.quoted(failing.path)) ] && [ "$1" = commit ]; then + echo "injected failure" >&2; exit 1 + fi + exec \(PosixShell.quoted(runtime.path)) "$@" + + """.write(to: script, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: script.path) + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let base = try await build(environment(fixture, driver: driver, revoked: Revoked())) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + let containers = try leftovers("ps", label: label) + + struct Injected: Error {} + // The step, the environment that fails it, the branch, and whether a grant was made by then. + let steps: [(String, RemoteProvisioning.Environment, String, Bool)] = [ + ("derive", environment(fixture, driver: driver, revoked: Revoked()), "wr-a", false), + ( + "connect", + environment( + fixture, driver: driver, revoked: Revoked(), connect: { _ in throw Injected() }), + "wr-b", false + ), + ( + "enrol", + // Nothing listens there, so the agent's enrolment fails after the grant is made. + environment( + fixture, driver: driver, revoked: Revoked(), brokerURL: URL(string: "http://127.0.0.1:9")), + "wr-c", true + ), + // git refuses the name only once the instance is enrolled. + ("checkout", environment(fixture, driver: driver, revoked: Revoked()), "bad..name", true), + ] + for (step, environment, branch, granted) in steps { + if step == "derive" { + try Data().write(to: failing) + } else { + try? FileManager.default.removeItem(at: failing) + } + BrokerStub.reset([Self.grant, Self.cancelled]) + + do { + _ = try await RemoteProvisioning.derive( + from: base, workroom: UUID(), branch: branch, in: environment) + XCTFail("a workroom that failed at \(step) was made") + } catch {} + + XCTAssertEqual( + try leftovers("ps", label: label), containers, "a failure at \(step) left an instance") + XCTAssertEqual( + try leftovers("images", label: label), [], "a failure at \(step) left an image") + XCTAssertEqual( + grantsCancelled, granted ? 1 : 0, "a failure at \(step) left its grant live") + } + } +} diff --git a/macapp/WorkroomAppTests/RemoteProvisioningTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningTests.swift new file mode 100644 index 000000000..d56998a6f --- /dev/null +++ b/macapp/WorkroomAppTests/RemoteProvisioningTests.swift @@ -0,0 +1,20 @@ +import XCTest + +@testable import Workroom + +final class RemoteProvisioningTests: XCTestCase { + /// The clone token reaches git as configuration in its environment, an `Authorization` header + /// for github.com only, and nowhere a process list, a remote URL or a file would show it. + func testTheCloneTokenReachesGitOnlyAsAGitHubHeaderInItsEnvironment() throws { + let environment = RemoteProvisioning.cloneEnvironment(token: "ghs_secret") + + XCTAssertEqual(environment["GIT_CONFIG_COUNT"], "1") + XCTAssertEqual(environment["GIT_CONFIG_KEY_0"], "http.https://github.com/.extraHeader") + let value = try XCTUnwrap(environment["GIT_CONFIG_VALUE_0"]) + XCTAssertTrue(value.hasPrefix("Authorization: Basic ")) + let credentials = try XCTUnwrap( + Data(base64Encoded: String(value.dropFirst("Authorization: Basic ".count)))) + XCTAssertEqual(String(decoding: credentials, as: UTF8.self), "x-access-token:ghs_secret") + XCTAssertFalse(environment.values.contains { $0.contains("ghs_secret") }) + } +} diff --git a/vcs/scripts/ssh-fixture/Dockerfile b/vcs/scripts/ssh-fixture/Dockerfile index 995b79b62..f8d4eee8b 100644 --- a/vcs/scripts/ssh-fixture/Dockerfile +++ b/vcs/scripts/ssh-fixture/Dockerfile @@ -18,5 +18,16 @@ RUN apt-get update \ 'AllowUsers workroom' \ > /etc/ssh/sshd_config.d/fixture.conf +# A repository a base machine clones (#252): the fixture has no GitHub, so the app's provisioning +# tests clone this one, by path, and a derived workroom fetches from its own copy of it. +RUN seed="$(mktemp -d)" \ + && git init -q -b main "$seed" \ + && echo origin > "$seed/README" \ + && git -C "$seed" add README \ + && git -C "$seed" -c user.name=Fixture -c user.email=fixture@example.com commit -qm initial \ + && git clone -q --bare "$seed" /srv/origin.git \ + && rm -rf "$seed" \ + && chown -R workroom:workroom /srv/origin.git + COPY --chmod=755 wr-agent entrypoint.sh identity.sh /usr/local/bin/ ENTRYPOINT ["/usr/local/bin/entrypoint.sh"] From ac51925adfe0099acbcf1f70885c38686d6a6299 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 19:02:10 +0100 Subject: [PATCH 4/9] test(macapp): a derived workroom's pane shows its last screen after a reboot Part of Slice C of #252. The container driver now publishes each container's ssh on a loopback port chosen for it, rather than an ephemeral one, so a restart keeps the host's address the way a provider's box keeps its own. With its identity kept too, a pane's ssh gets back in with the host key pinned at the derive. The new test writes a screen on a derived workroom, stops and starts its container, and reattaches. The pane shows the last screen with the "ended when its host restarted" notice: the derived image keeps the fixture's supervisor, which passes --screens on a directory in the home directory, not on tmpfs /run. Removing --screens turns the test red. Co-Authored-By: Claude Opus 5.5 --- .../Core/ContainerHostDriver.swift | 16 +++--- ...ontainerProvisioningIntegrationTests.swift | 52 +++++++++++++++++-- .../RemoteHostIntegrationTests.swift | 2 +- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index b5de70485..71137d7f3 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -132,20 +132,24 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { /// container that gets no further is removed. private func run(_ source: String, image: String?) async throws -> HostID { guard let provisioning else { throw HostDriverError.notImplemented("Provisioning") } + // A port of its own rather than an ephemeral one (`127.0.0.1::22`): a restart keeps it, so the + // host's address outlives a reboot, as a provider's box keeps its address. + // ponytail: the port is free when chosen, not when the runtime binds it; a run that loses + // that race fails and is rolled back like any other. + guard let (probe, port) = LoopbackSocket.listen(backlog: 1) else { + throw HostDriverError.provisioning("no free loopback port: errno \(errno)") + } + Darwin.close(probe) let container = try await runtime( - ["run", "--detach", "--init", "--publish", "127.0.0.1::22"] + ["run", "--detach", "--init", "--publish", "127.0.0.1:\(port):22"] + provisioning.labels.flatMap { ["--label", $0] } + ["--env", "AUTHORIZED_KEY=\(provisioning.publicKey)", source]) let id = UUID() do { - let published = try await runtime(["port", container, "22/tcp"]) - guard let port = published.split(separator: ":").last.flatMap({ Int($0) }) else { - throw HostDriverError.provisioning("no published ssh port in \(published)") - } let hostKey = try await identity(of: container) lock.withLock { hosts[id] = Host( - address: "127.0.0.1", port: port, user: provisioning.user, + address: "127.0.0.1", port: Int(port), user: provisioning.user, identityFile: provisioning.identityFile, hostKey: hostKey, agentSocket: provisioning.agentSocket) provisioned[id] = Provisioned(container: container, image: image) diff --git a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift index df6fba014..dabca8d67 100644 --- a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift @@ -175,9 +175,9 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { let base = try await driver.create() let baseContainers = try leftovers("ps", runtime: runtime, label: label) - // `commit` makes the image and `run` the container; `port` and `exec` come once it is running. - // A failed `exec` reads as an identity never minted, so it carries no runtime message. - for step in ["commit", "run", "port", "exec"] { + // `commit` makes the image and `run` the container; `exec` comes once it is running. A failed + // `exec` reads as an identity never minted, so it carries no runtime message. + for step in ["commit", "run", "exec"] { try step.write(to: failing, atomically: true, encoding: .utf8) do { _ = try await driver.deriveFromBase(base) @@ -198,4 +198,50 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { try await driver.destroy(base) XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) } + + /// A derived workroom's supervisor keeps screens on a disk that outlives a reboot (#232, #252): + /// a pane that reattaches after the box is stopped and started is shown its last screen, ended, + /// rather than a fresh shell. The restart keeps the host's address and its identity, so the + /// pane's ssh gets in with the key pinned at the derive. + func testAPaneOnADerivedWorkroomIsShownItsLastScreenAfterAReboot() async throws { + let provisioning = try provisioning() + let driver = ContainerHostDriver(hosts: [:], directory: directory, provisioning: provisioning) + let base = try await driver.create() + let instance = try await driver.deriveFromBase(base) + let session = UUID() + + let first = try RemoteHostIntegrationTests.Pane( + command: driver.attachCommand( + to: instance, session: session, workingDirectory: "/home/workroom", restored: false)) + defer { first.dropLink() } + try await Task.sleep(for: .seconds(1)) + first.type("echo LAST-SC\"\"REEN\n") + XCTAssertTrue(first.read(until: "LAST-SCREEN").contains("LAST-SCREEN")) + // The reboot kills the agent outright, so only what it has already written survives. + let record = "~/.local/state/workroom/screens/\(session.uuidString.lowercased()).vt" + var written = false + for _ in 0..<100 where !written { + // Not `onHost`, which fails the test on the misses this loop waits through. + let (status, _) = try await driver.exec("grep -q LAST-SCREEN \(record)", on: instance) + .communicate(nil, timeout: 20) + written = status == 0 + if !written { try await Task.sleep(for: .milliseconds(200)) } + } + XCTAssertTrue(written, "the screen was never written to \(record)") + first.dropLink() + + // The newest container with this test's label is the instance. + let container = try docker(runtime, ["ps", "-l", "-q", "--filter", "label=\(label!)"]) + try docker(runtime, ["restart", "-t", "0", container]) + + let second = try RemoteHostIntegrationTests.Pane( + command: driver.attachCommand( + to: instance, session: session, workingDirectory: "/home/workroom", restored: true)) + defer { second.dropLink() } + let seen = second.read(until: "host restarted", within: 40) + XCTAssertTrue(seen.contains("LAST-SCREEN"), seen) + XCTAssertTrue(seen.contains("ended when its host restarted"), seen) + + for host in [instance, base] { try await driver.destroy(host) } + } } diff --git a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift index 0131de5c4..58692b306 100644 --- a/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteHostIntegrationTests.swift @@ -186,7 +186,7 @@ final class RemoteHostIntegrationTests: XCTestCase { /// 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 { + final class Pane: @unchecked Sendable { let process = Process() private let input = Pipe() private let lock = NSLock() From cb33b02678a6931f0dbc0821e034bae80b379ca1 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 19:10:42 +0100 Subject: [PATCH 5/9] test(macapp): a derived workroom fetches and pushes with the Mac disconnected The rest of Slice C of #252. The fixture gains a GitHub and a broker of its own (fake-github.py, started by the entrypoint as the ssh user with only CAP_NET_BIND_SERVICE): - The broker, on http://127.0.0.1:8081, accepts an enrolment and mints a token per request. - GitHub, on https://github.com, mapped to the container's loopback, with a certificate git trusts for that host only. It serves git's smart HTTP through git-http-backend over /srv. Without a token it minted, or the base clone token baked into the image, it answers 401 with a Basic challenge. The test clones the base over https with the broker's clone token as git's header, derives and enrols a workroom against the box's own broker, and closes the Mac's connection. Then, through the runtime's exec rather than the Mac's link, it shows the workroom fetching and pushing on its own, its credential helper minting from the box's broker. A push with the helper switched off is refused, inside the test. Deleting the workroom's enrolment makes the fetch and push fail. Co-Authored-By: Claude Opus 5.5 --- .../RemoteProvisioningIntegrationTests.swift | 59 +++++++ vcs/scripts/ssh-fixture/Dockerfile | 17 +- vcs/scripts/ssh-fixture/entrypoint.sh | 8 + vcs/scripts/ssh-fixture/fake-github.py | 145 ++++++++++++++++++ vcs/scripts/ssh-fixture/run.sh | 2 +- 5 files changed, 227 insertions(+), 4 deletions(-) create mode 100755 vcs/scripts/ssh-fixture/fake-github.py diff --git a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift index 2b52f4aa7..1fc5a9f5a 100644 --- a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift @@ -364,4 +364,63 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { grantsCancelled, granted ? 1 : 0, "a failure at \(step) left its grant live") } } + + /// The box authenticates on its own (#252): with the Mac's connection gone, a derived workroom + /// fetches and pushes over https to the fixture's GitHub, its credential helper minting from the + /// fixture's broker on the box (`fake-github.py`). The base clones over https too, with the + /// broker's clone token as git's header. Commands run through the runtime's exec, the provider's + /// control plane, not over the Mac's link. + @MainActor + func testADerivedWorkroomFetchesAndPushesWithTheMacDisconnected() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment( + fixture, driver: driver, revoked: Revoked(), + brokerURL: URL(string: "http://127.0.0.1:8081")) + BrokerStub.reset([Self.cloneToken]) + let base = try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "https://github.com/origin.git", path: Self.path, + in: environment, record: { _ in }) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + BrokerStub.reset([Self.grant]) + let instance = try await RemoteProvisioning.derive( + from: base, workroom: UUID(), branch: "wr-pushed", in: environment) + cleanups.append { try? await driver.destroy(instance.host) } + await instance.connection.close() + + // The newest container with this test's label is the instance. + let container = try docker(runtime, ["ps", "-l", "-q", "--filter", "label=\(label!)"]) + func onBox(_ script: String) throws -> String { + try docker( + runtime, + [ + "exec", "--user", "workroom", "--env", "HOME=/home/workroom", "--workdir", Self.path, + container, "sh", "-c", script + "; echo exit=$?", + ]) + } + // Disconnected: no relay to the Mac's agent connection is left on the box, once the closed + // link's ssh has gone. `[w]`, so the pattern does not match the shell running it. + var relays = "" + for _ in 0..<50 { + relays = try onBox("pgrep -f '[w]r-agent relay'") + if relays == "exit=1" { break } + try await Task.sleep(for: .milliseconds(200)) + } + XCTAssertEqual(relays, "exit=1", "a relay to the Mac is still running") + + let commit = try onBox( + "git -c user.name=W -c user.email=w@example.com commit -q --allow-empty -m pushed" + + " && git rev-parse HEAD") + let pushed = try XCTUnwrap(commit.split(separator: "\n").first.map(String.init), commit) + // The fixture's GitHub refuses a push with no token, so the one below is the helper's doing. + let refused = try onBox("git -c credential.https://github.com.helper= push -q origin HEAD 2>&1") + XCTAssertFalse(refused.hasSuffix("exit=0"), "the fixture's GitHub took a push with no token") + + XCTAssertTrue(try onBox("git fetch -q origin").hasSuffix("exit=0"), "the fetch failed") + let push = try onBox("git push -q origin HEAD 2>&1") + XCTAssertTrue(push.hasSuffix("exit=0"), push) + XCTAssertEqual( + try onBox("git -C /srv/origin.git rev-parse refs/heads/wr-pushed"), "\(pushed)\nexit=0") + } } diff --git a/vcs/scripts/ssh-fixture/Dockerfile b/vcs/scripts/ssh-fixture/Dockerfile index f8d4eee8b..336b6ad43 100644 --- a/vcs/scripts/ssh-fixture/Dockerfile +++ b/vcs/scripts/ssh-fixture/Dockerfile @@ -5,10 +5,10 @@ FROM debian:bookworm-slim # git: the VCS test reads a real repository through the relay. procps: pgrep, for the tests that -# find the supervised agent. +# find the supervised agent. python3 and openssl: the fixture's GitHub and broker (#252). RUN apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ - openssh-server git procps \ + openssh-server git procps python3 openssl \ && rm -rf /var/lib/apt/lists/* \ && useradd --create-home --shell /bin/bash --password '*' workroom \ && printf '%s\n' \ @@ -29,5 +29,16 @@ RUN seed="$(mktemp -d)" \ && rm -rf "$seed" \ && chown -R workroom:workroom /srv/origin.git -COPY --chmod=755 wr-agent entrypoint.sh identity.sh /usr/local/bin/ +# The fixture's GitHub (fake-github.py): a certificate for github.com that git trusts for that +# host only, and the base clone token it accepts besides the ones its broker mints. The app's +# provisioning tests hand the same token out as the broker's (RemoteProvisioningIntegrationTests). +RUN mkdir -p /etc/workroom-fixture \ + && openssl req -x509 -newkey rsa:2048 -nodes -days 3650 -subj /CN=github.com \ + -addext subjectAltName=DNS:github.com \ + -keyout /etc/workroom-fixture/github.key -out /etc/workroom-fixture/github.pem 2>/dev/null \ + && chown workroom:workroom /etc/workroom-fixture/github.key \ + && git config --system http.https://github.com/.sslCAInfo /etc/workroom-fixture/github.pem \ + && echo ghs_fixture_clone_token_0123456789 > /etc/workroom-fixture/clone-token + +COPY --chmod=755 wr-agent entrypoint.sh identity.sh fake-github.py /usr/local/bin/ ENTRYPOINT ["/usr/local/bin/entrypoint.sh"] diff --git a/vcs/scripts/ssh-fixture/entrypoint.sh b/vcs/scripts/ssh-fixture/entrypoint.sh index 673a8d736..76437ac9e 100755 --- a/vcs/scripts/ssh-fixture/entrypoint.sh +++ b/vcs/scripts/ssh-fixture/entrypoint.sh @@ -64,4 +64,12 @@ install -o workroom -g workroom -m 700 /usr/local/bin/wr-agent "$AGENT" done ) & +# The fixture's GitHub and broker (fake-github.py, #252), as the ssh user, with only the capability +# it needs for 443. `/etc/hosts` is the runtime's, written afresh at every start and never +# committed, so the name is mapped here each boot. +grep -q ' github.com$' /etc/hosts || echo '127.0.0.1 github.com' >> /etc/hosts +setpriv --reuid=workroom --regid=workroom --init-groups \ + --inh-caps=+net_bind_service --ambient-caps=+net_bind_service \ + env -i PATH=/usr/bin:/bin python3 /usr/local/bin/fake-github.py & + exec /usr/sbin/sshd -D -e diff --git a/vcs/scripts/ssh-fixture/fake-github.py b/vcs/scripts/ssh-fixture/fake-github.py new file mode 100755 index 000000000..5c9aecc47 --- /dev/null +++ b/vcs/scripts/ssh-fixture/fake-github.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +"""The fixture's GitHub and credential broker (#252), so a derived workroom can be shown to fetch +and push on its own, with no Mac attached. + +Two listeners in one process, both on the container's own loopback: + +- The broker, on http://127.0.0.1:8081: an enrolment is accepted, and each token request mints a + token. A fixed port, because every container derived from a base keeps the broker URL its agent + enrolled with. +- GitHub, on https://github.com (entrypoint.sh maps the name to 127.0.0.1, and the image trusts + this certificate for github.com only): git's smart HTTP through git-http-backend, over the + repositories under /srv. A request without a token it accepts is answered 401 with a Basic + challenge, which is what makes git ask its credential helper. It accepts a token this broker + minted, or the base clone token baked into the image, and nothing else. + +Fixture only. It binds 443 as the ssh user with CAP_NET_BIND_SERVICE (entrypoint.sh). +""" + +import base64 +import datetime +import http.server +import json +import secrets +import ssl +import subprocess +import threading + +CONFIG = "/etc/workroom-fixture" +with open(f"{CONFIG}/clone-token", encoding="utf-8") as handle: + CLONE_TOKEN = handle.read().strip() +MINTED = set() +LOCK = threading.Lock() + + +class Broker(http.server.BaseHTTPRequestHandler): + def do_POST(self): + self.rfile.read(int(self.headers.get("Content-Length") or 0)) + if self.path == "/broker/enrolments": + self.reply(201, {"grant_id": "g1"}) + elif self.path == "/broker/tokens": + token = "ghs_minted_" + secrets.token_hex(16) + with LOCK: + MINTED.add(token) + expires = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(hours=1) + self.reply(200, {"token": token, "expires_at": expires.strftime("%Y-%m-%dT%H:%M:%SZ")}) + else: + self.reply(404, {"error": "not_found"}) + + def reply(self, status, body): + data = json.dumps(body).encode() + self.send_response(status) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(data))) + self.end_headers() + self.wfile.write(data) + + def log_message(self, *args): + pass + + +class GitHub(http.server.BaseHTTPRequestHandler): + def authorized(self): + scheme, _, credentials = self.headers.get("Authorization", "").partition(" ") + if scheme.lower() != "basic": + return False + try: + user, _, token = base64.b64decode(credentials).decode().partition(":") + except ValueError: + return False + with LOCK: + return user == "x-access-token" and (token in MINTED or token == CLONE_TOKEN) + + def body(self): + if self.headers.get("Transfer-Encoding", "").lower() == "chunked": + data = b"" + while True: + size = int(self.rfile.readline().split(b";")[0], 16) + if size == 0: + self.rfile.readline() + return data + data += self.rfile.read(size) + self.rfile.readline() + return self.rfile.read(int(self.headers.get("Content-Length") or 0)) + + def serve(self): + if not self.authorized(): + self.send_response(401) + self.send_header("WWW-Authenticate", 'Basic realm="GitHub"') + self.send_header("Content-Length", "0") + self.end_headers() + return + path, _, query = self.path.partition("?") + body = self.body() + env = { + "GIT_PROJECT_ROOT": "/srv", + "GIT_HTTP_EXPORT_ALL": "1", + # git-http-backend serves receive-pack only to an authenticated user. + "REMOTE_USER": "x-access-token", + "REQUEST_METHOD": self.command, + "PATH_INFO": path, + "QUERY_STRING": query, + "CONTENT_TYPE": self.headers.get("Content-Type", ""), + "CONTENT_LENGTH": str(len(body)), + "PATH": "/usr/bin:/bin", + } + for name, value in self.headers.items(): + env["HTTP_" + name.upper().replace("-", "_")] = value + output = subprocess.run( + ["/usr/lib/git-core/git-http-backend"], input=body, env=env, capture_output=True + ).stdout + separator = b"\r\n\r\n" if b"\r\n\r\n" in output else b"\n\n" + head, _, content = output.partition(separator) + status, headers = 200, [] + for line in head.decode().splitlines(): + name, _, value = line.partition(":") + if name.lower() == "status": + status = int(value.split()[0]) + elif name: + headers.append((name, value.strip())) + self.send_response(status) + for name, value in headers: + self.send_header(name, value) + self.send_header("Content-Length", str(len(content))) + self.end_headers() + self.wfile.write(content) + + do_GET = serve + do_POST = serve + + def log_message(self, *args): + pass + + +def main(): + broker = http.server.ThreadingHTTPServer(("127.0.0.1", 8081), Broker) + github = http.server.ThreadingHTTPServer(("127.0.0.1", 443), GitHub) + context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + context.load_cert_chain(f"{CONFIG}/github.pem", f"{CONFIG}/github.key") + github.socket = context.wrap_socket(github.socket, server_side=True) + threading.Thread(target=broker.serve_forever, daemon=True).start() + github.serve_forever() + + +if __name__ == "__main__": + main() diff --git a/vcs/scripts/ssh-fixture/run.sh b/vcs/scripts/ssh-fixture/run.sh index 835ed9e86..0356e66b3 100755 --- a/vcs/scripts/ssh-fixture/run.sh +++ b/vcs/scripts/ssh-fixture/run.sh @@ -58,7 +58,7 @@ cleanup() { trap cleanup EXIT cp "$AGENT" "$STAGE/wr-agent" -cp "$HERE/Dockerfile" "$HERE/entrypoint.sh" "$HERE/identity.sh" "$STAGE/" +cp "$HERE/Dockerfile" "$HERE/entrypoint.sh" "$HERE/identity.sh" "$HERE/fake-github.py" "$STAGE/" # By image ID, not a tag: a tag is shared, and another run building it between here and `run` below # would swap in a different agent. WR_FIXTURE_BUILD_FLAGS is word-split into extra build flags; CI # passes a GitHub Actions layer cache there so the apt layer is not rebuilt on every run. From cbad445e9182fdf012ed308a9528ac859895ec96 Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 19:49:09 +0100 Subject: [PATCH 6/9] fix: address the adversarial review of #252, and record it as built From an independent review of this branch (macapp/AGENTS.md rule 3): - The container driver names what it makes. A run is named workroom-, and a commit carries a workroom.commit label chosen first. A run or commit that outlives its CLI (killed by the silence bound, or a cancelled derive) is then still found and removed; before, its ID was known only from output nobody read. Commits get a 900 s silence bound, since commit prints nothing until it is done. The failure tests now also fail each step after the daemon has done it. - An instance can't be derived from: its disk holds its key and credential helper. The entrypoint's re-mint also deletes the agent's interrupted saves (.broker.json.), which can hold a key. - Destroying a workroom destroys its box even when the broker can't cancel the grant. The key that would mint against the grant goes with the box. The first error is still thrown. - `host set` decodes with UseNumber, as Config.Read does, so an integer above 2^53 is stored as written. It also refuses trailing data. - The base test clones over https, so the token-on-disk and remote-URL checks run where git really sends the token. - gitTimeout's comment called it a silence bound; the agent's exec deadline is a wall clock capped at 610 s. Recorded as a ceiling: a clone over 10 minutes fails. The design doc gains an "As built (#252)" section with the gaps left for #253, #256 and #257. macapp/AGENTS.md now says what writes a host descriptor. Negative controls went red for each fix. Full app suite through the fixture: 3111 passed, 0 failed. Also passing: Rust over_ssh (5), go test, cli-lint, app-lint, app-test-scripts, shellcheck. Co-Authored-By: Claude Opus 5.5 --- cmd/host.go | 23 ++++++-- cmd/host_test.go | 27 +++++++++ docs/designs/remote-workrooms.md | 56 ++++++++++++++++++- macapp/AGENTS.md | 8 +-- .../Core/ContainerHostDriver.swift | 40 +++++++++---- .../WorkroomApp/Core/RemoteProvisioning.swift | 15 +++-- ...ontainerProvisioningIntegrationTests.swift | 30 ++++++++-- .../RemoteProvisioningIntegrationTests.swift | 6 +- vcs/scripts/ssh-fixture/entrypoint.sh | 3 +- 9 files changed, 175 insertions(+), 33 deletions(-) create mode 100644 cmd/host_test.go diff --git a/cmd/host.go b/cmd/host.go index a6e56ac30..0a04ff04c 100644 --- a/cmd/host.go +++ b/cmd/host.go @@ -3,7 +3,9 @@ package cmd import ( "encoding/json" "fmt" + "io" "os" + "strings" "github.com/joelmoss/workroom/internal/config" "github.com/joelmoss/workroom/internal/errs" @@ -30,11 +32,9 @@ var hostSetCmd = &cobra.Command{ Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { currentCommand = "host" - var host map[string]any - // A JSON null decodes to a nil map, which would clear the descriptor, so it is refused - // with every other non-object. - if err := json.Unmarshal([]byte(args[0]), &host); err != nil || host == nil { - return fmt.Errorf("%w: %s", errs.ErrInvalidHost, args[0]) + host, err := decodeHost(args[0]) + if err != nil { + return err } return setHost(host) }, @@ -50,6 +50,19 @@ var hostClearCmd = &cobra.Command{ }, } +// decodeHost reads a descriptor as written: UseNumber, as Config.Read does, so an integer above +// 2^53 is not rewritten through float64. A JSON null decodes to a nil map, which would clear the +// descriptor, so it is refused with every other non-object, and so is trailing data. +func decodeHost(text string) (map[string]any, error) { + var host map[string]any + dec := json.NewDecoder(strings.NewReader(text)) + dec.UseNumber() + if err := dec.Decode(&host); err != nil || host == nil || dec.Decode(new(any)) != io.EOF { + return nil, fmt.Errorf("%w: %s", errs.ErrInvalidHost, text) + } + return host, nil +} + func setHost(host map[string]any) error { cfg, err := config.New("") if err != nil { diff --git a/cmd/host_test.go b/cmd/host_test.go new file mode 100644 index 000000000..d3d6684de --- /dev/null +++ b/cmd/host_test.go @@ -0,0 +1,27 @@ +package cmd + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/joelmoss/workroom/internal/errs" +) + +func TestDecodeHostKeepsNumbersAsWritten(t *testing.T) { + host, err := decodeHost(`{"id":9007199254740993}`) + if err != nil { + t.Fatal(err) + } + if got := host["id"]; got != json.Number("9007199254740993") { + t.Fatalf("id = %#v, want the number as written", got) + } +} + +func TestDecodeHostRefusesWhatIsNotOneObject(t *testing.T) { + for _, text := range []string{`null`, `[]`, `"x"`, `{}}`, `{"a":1} {"b":2}`, ``} { + if _, err := decodeHost(text); !errors.Is(err, errs.ErrInvalidHost) { + t.Errorf("decodeHost(%q) err = %v, want ErrInvalidHost", text, err) + } + } +} diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index 8354e67b7..c4199ea81 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1282,7 +1282,61 @@ these are the subsystems that actually gate "a remote workroom is a real workroo one if that session is gone by then). The same gap applies to a `.directoryMissing` target and predates #249; #253's reconnect work exercises this path, and the fix touches `Core/Session/` (AGENTS.md rule 3). - - Nothing writes a descriptor yet; provisioning (#252) and the app (#253) will. + - `workroom host set|clear` writes a descriptor (#252, below); the app (#253) is its caller. +- **As built (#252, portable derivation on the container driver).** The sequence is + `RemoteProvisioning` in the Mac app, over any `HostDriver`; the container fixture is the first + driver to implement it. + - **The container driver provisions.** Given a container runtime and the fixture's image, + `ContainerHostDriver.create` runs a base, `deriveFromBase` commits the base's disk and runs a + container from that image (a snapshot of the disk, no live processes), and `destroy` removes + a container and the image it alone was run from. Each step undoes what it made when a later + one fails. Every container publishes ssh on a loopback port chosen for it, so a restart keeps + the host's address, as a provider's box keeps its own. + - **Identity (OQ9) is minted at first boot, before sshd or the agent starts.** The entrypoint + keys a marker on the container's hostname. A new container (a derive) gets fresh ssh host + keys and `/etc/machine-id`, and loses the base's broker files and screens; a restart keeps + them all, so the pinned host key still matches after a reboot. The driver reads the new host + key out of band (`identity.sh`, through the runtime's exec) and pins it. Not re-mintable in a + container: `/proc/sys/kernel/random/boot_id` is the kernel's, shared by every container on a + machine. A VM provider's derived instance boots a kernel of its own. + - **The base** is provisioned, cloned through the agent's exec service, and recorded by the + caller (`workroom host set`, a hidden CLI command that stores a descriptor verbatim on an + existing project or workroom and refuses JSON null). The clone token comes from + `POST /broker/base-clone-tokens`. It reaches git only as an `Authorization` header for + github.com, set through `GIT_CONFIG_COUNT` in the exec request's environment, so it appears + in no argument, remote URL or file, and the Mac revokes it at GitHub once the clone is over. + The base never enrols. A base holds the repository cloned and the agent present, and nothing + else: dependencies are not installed (owner's decision, 2026-10-01). `refreshBase` fetches + with a fresh token. + - **A workroom** runs derive, then enrol (`AgentEnrolment`, OQ10, so each key is the + instance's own), then fetch and `git switch --create origin/HEAD`, and is returned + serving over its agent connection. A failure at any step cancels its grant and destroys the + instance. A failed grant cancel does not keep the box: the key that would mint against the + grant goes with it. An instance is never derived from, because its disk holds its key and + credential helper. + - **The driver names what it makes.** The runtime's own IDs are known only from its output, so a + `run` or `commit` that outlives its CLI (killed by the silence bound, or a cancelled derive) + would leave a resource nothing could find. A container is named `workroom-` and a + commit carries a `workroom.commit=` label chosen first, and a failure removes by those. + - **Tests.** `ContainerProvisioningIntegrationTests` and `RemoteProvisioningIntegrationTests` + run through `run.sh`. Each of the six acceptance criteria has a test, and each test has a + negative control that turns it red. The fixture gained a GitHub and a broker of its own + (`fake-github.py`): https for github.com on the container's loopback, trusted for that host + only, behind a token check, so a derived workroom is shown fetching and pushing with the Mac's + connection closed. These Swift suites skip in CI, which has no Docker on its macOS runner, + and were run locally. Only the Rust `over_ssh` set runs against the fixture in CI. + - **Gaps, for the issues that own them.** + - Provisioned hosts live only in the driver's memory, so nothing re-adopts a recorded base + after the app relaunches (#253). + - `broker.json` and the token cache sit beside the agent's socket under `/run/workroom`. The + fixture keeps that across a restart, but a real host's tmpfs `/run` does not, so a rebooted + boxd workroom would lose its enrolment (#256, #257). + - A base's clone must finish within 10 minutes. The agent's exec deadline is a wall clock + (`exec_timeout`, capped at 610 s), not a silence bound, so a large repository over a slow + link fails its clone and its base is removed. Upgrade path: start the clone detached on + the host and poll for it. + - `refreshBase` has no test. + - `deriveSpeed` is declared as 5 s; measured derives on the fixture take 2–3 s. - **Cross-machine session enumeration** (from Phase 1's S4 note): agent-side session naming, a pane→tab mapping on the wire, and workroom UI state stored with the workroom. - **OSC 7 and cmd-click (C7).** `SessionDaemon.swift:395-401` deliberately emits diff --git a/macapp/AGENTS.md b/macapp/AGENTS.md index 198414735..60ed8ed6e 100644 --- a/macapp/AGENTS.md +++ b/macapp/AGENTS.md @@ -254,10 +254,10 @@ side is `Core/Broker/`: A workroom or project entry in `config.json` can carry a `"host"` object, which `list --json` passes through as `host`. The app owns its schema (`HostDescriptor` in `Core/Models.swift`); the -CLI reads only whether a workroom has one (remote) and `state == "destroyed"`. Nothing creates one -yet: provisioning (#252) and the app (#253) will. Every CLI config write preserves an existing one, -so a change to a config writer must keep `host`, or a remote workroom silently turns local. What -the app relies on until then: +CLI reads only whether a workroom has one (remote) and `state == "destroyed"`. `workroom host +set|clear` (#252) writes one verbatim on an existing entry; the app (#253) will be its caller. +Every CLI config write preserves an existing one, so a change to a config writer must keep `host`, +or a remote workroom silently turns local. What the app relies on until then: - `TerminalTarget.unavailability` is `.directoryMissing`, `.remote` or `.hostDestroyed`, and `isMissing` is derived from it. A new local action should guard on `isMissing`, as the existing diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index 71137d7f3..0b45492a5 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -98,13 +98,30 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { guard case .remote(let id) = base, let source = lock.withLock({ provisioned[id] }) else { throw HostDriverError.unknownHost(base) } - let image = try await runtime( - ["commit"] + provisioning.labels.flatMap { ["--change", "LABEL \($0)"] } - + [source.container]) + // An instance has enrolled, and its key and credential helper are on its disk: a copy of it + // would start with them. + guard source.image == nil else { + throw HostDriverError.invalidConfiguration("a workroom instance cannot be derived from") + } + // Labelled with an ID chosen here, because the runtime's own is known only from its output: + // a commit that outlives its CLI (killed by the silence bound, or a cancelled derive) leaves + // an image nothing would otherwise find. + let commit = "workroom.commit=\(UUID().uuidString.lowercased())" do { + let image = try await runtime( + ["commit"] + (provisioning.labels + [commit]).flatMap { ["--change", "LABEL \($0)"] } + + [source.container], + // `commit` prints nothing until it is done, and copying a base's disk takes a while. + timeout: 900) return try await run(image, image: image) } catch { - await Self.cleanUp { _ = try await self.runtime(["rmi", "--force", image]) } + await Self.cleanUp { + for image in try await self.runtime(["images", "-aq", "--filter", "label=\(commit)"]) + .split(separator: "\n") + { + _ = try? await self.runtime(["rmi", "--force", String(image)]) + } + } throw error } } @@ -140,12 +157,15 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { throw HostDriverError.provisioning("no free loopback port: errno \(errno)") } Darwin.close(probe) - let container = try await runtime( - ["run", "--detach", "--init", "--publish", "127.0.0.1:\(port):22"] - + provisioning.labels.flatMap { ["--label", $0] } - + ["--env", "AUTHORIZED_KEY=\(provisioning.publicKey)", source]) let id = UUID() + // Named here, not by the runtime's own ID, which is known only from its output: a run that + // outlives its CLI still leaves a container this name removes. + let container = "workroom-\(id.uuidString.lowercased())" do { + _ = try await runtime( + ["run", "--detach", "--init", "--name", container, "--publish", "127.0.0.1:\(port):22"] + + provisioning.labels.flatMap { ["--label", $0] } + + ["--env", "AUTHORIZED_KEY=\(provisioning.publicKey)", source]) let hostKey = try await identity(of: container) lock.withLock { hosts[id] = Host( @@ -196,14 +216,14 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { /// One runtime command; its first line of output, which is all any of these print on success. /// Its environment is the few variables that say which daemon to talk to, never the app's whole /// one. - private func runtime(_ arguments: [String]) async throws -> String { + private func runtime(_ arguments: [String], timeout: TimeInterval = 120) async throws -> String { guard let provisioning else { throw HostDriverError.notImplemented("Provisioning") } let environment = ProcessInfo.processInfo.environment.filter { ["HOME", "PATH", "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG"].contains($0.key) } let (status, output) = try await HostStream.spawn( provisioning.runtime, arguments, environment: environment, handshakeTimeout: 20 - ).communicate(nil, timeout: 120) + ).communicate(nil, timeout: timeout) let said = output.trimmingCharacters(in: .whitespacesAndNewlines) guard status == 0 else { throw HostDriverError.provisioning( diff --git a/macapp/WorkroomApp/Core/RemoteProvisioning.swift b/macapp/WorkroomApp/Core/RemoteProvisioning.swift index 007e5fe56..ae4bb3520 100644 --- a/macapp/WorkroomApp/Core/RemoteProvisioning.swift +++ b/macapp/WorkroomApp/Core/RemoteProvisioning.swift @@ -74,8 +74,10 @@ enum RemoteProvisioning { } } - /// Silence allowed on one git command. A clone of a large repository over a slow link is long, - /// but it is not quiet for this long. + /// How long one git command may run, start to end: the agent's exec deadline is a wall clock + /// (`exec_timeout` in `vcs.rs`, capped at 610 s), and so is the connection's request deadline. + /// ponytail: a clone that takes longer fails, and its base is removed. Upgrade path: start the + /// clone detached on the host and poll for it, so only silence ends it. static let gitTimeout: TimeInterval = 600 // MARK: The base @@ -196,14 +198,17 @@ enum RemoteProvisioning { } /// Takes a workroom down: its grant first, so nothing mints for it once its box is gone, then - /// the box. + /// the box. A grant the broker could not cancel does not keep the box: the key that would mint + /// against it goes with the box. The first failure is thrown once both have been tried. static func destroy(_ instance: Instance, workroom: UUID, in environment: Environment) async throws { await instance.connection.close() - try await environment.client.cancelGrant(instance.grantID) + var failure: Error? + do { try await environment.client.cancelGrant(instance.grantID) } catch { failure = error } await environment.agentBroker.release(workroom) - try await environment.driver.destroy(instance.host) + do { try await environment.driver.destroy(instance.host) } catch { failure = failure ?? error } + if let failure { throw failure } } // MARK: Plumbing diff --git a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift index dabca8d67..88c7cc99e 100644 --- a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift @@ -149,7 +149,9 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { } /// A runtime that runs the real one, except for the subcommand named in `failing` when that - /// file exists: a provider that fails partway through a derive, once the base is made. + /// file exists: a provider that fails partway through a derive, once the base is made. Named + /// with a `+`, the subcommand runs and THEN fails, as a CLI killed after the daemon took the + /// request does: the resource exists and its ID was never printed. private func failingRuntime() throws -> (runtime: URL, failing: URL) { _ = try provisioning() try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) @@ -157,9 +159,10 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { let script = directory.appendingPathComponent("runtime") try """ #!/bin/sh - if [ -f \(PosixShell.quoted(failing.path)) ] && \\ - [ "$1" = "$(cat \(PosixShell.quoted(failing.path)))" ]; then - echo "injected failure" >&2; exit 1 + step="$(cat \(PosixShell.quoted(failing.path)) 2>/dev/null)" + if [ "$1" = "$step" ]; then echo "injected failure" >&2; exit 1; fi + if [ "$1+" = "$step" ]; then + \(PosixShell.quoted(runtime.path)) "$@" >/dev/null; echo "injected failure" >&2; exit 1 fi exec \(PosixShell.quoted(runtime.path)) "$@" @@ -177,7 +180,7 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { // `commit` makes the image and `run` the container; `exec` comes once it is running. A failed // `exec` reads as an identity never minted, so it carries no runtime message. - for step in ["commit", "run", "exec"] { + for step in ["commit", "commit+", "run", "run+", "exec"] { try step.write(to: failing, atomically: true, encoding: .utf8) do { _ = try await driver.deriveFromBase(base) @@ -244,4 +247,21 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { for host in [instance, base] { try await driver.destroy(host) } } + + /// An instance has enrolled, so its disk holds its key and credential helper: a derive from it + /// would hand both on. + func testAWorkroomInstanceCannotBeDerivedFrom() async throws { + let provisioning = try provisioning() + let driver = ContainerHostDriver(hosts: [:], directory: directory, provisioning: provisioning) + let base = try await driver.create() + let instance = try await driver.deriveFromBase(base) + + do { + _ = try await driver.deriveFromBase(instance) + XCTFail("an instance was derived from") + } catch HostDriverError.invalidConfiguration {} + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label).count, 2) + + for host in [instance, base] { try await driver.destroy(host) } + } } diff --git a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift index 1fc5a9f5a..3be8f0787 100644 --- a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift @@ -185,8 +185,9 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { let recorded = Revoked() BrokerStub.reset([Self.cloneToken]) + // Over https, so git really sends the token: a clone by path never would. let base = try await RemoteProvisioning.buildBase( - repository: "o/r", cloneURL: "/srv/origin.git", path: Self.path, + repository: "o/r", cloneURL: "https://github.com/origin.git", path: Self.path, in: environment(fixture, driver: driver, revoked: revoked), record: { recorded.add(String(decoding: try JSONEncoder().encode($0), as: UTF8.self)) }) cleanups.append { try? await driver.destroy(.remote(base.host)) } @@ -196,7 +197,8 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { XCTAssertEqual(readme.output, "origin") let remote = try await onHost(driver, host, "git -C \(Self.path) remote get-url origin") XCTAssertEqual( - remote.output, "/srv/origin.git", "the remote URL carries something besides the repository") + remote.output, "https://github.com/origin.git", + "the remote URL carries something besides the repository") XCTAssertEqual(revoked.all, [Self.token], "the clone token outlived the clone") XCTAssertEqual(recorded.all.count, 1) let seen = try XCTUnwrap(BrokerStub.requests.first) diff --git a/vcs/scripts/ssh-fixture/entrypoint.sh b/vcs/scripts/ssh-fixture/entrypoint.sh index 76437ac9e..00d185f9d 100755 --- a/vcs/scripts/ssh-fixture/entrypoint.sh +++ b/vcs/scripts/ssh-fixture/entrypoint.sh @@ -24,7 +24,8 @@ if [ "$(cat "$IDENTITY" 2>/dev/null)" != "$(hostname)" ]; then rm -f /etc/ssh/ssh_host_* ssh-keygen -A tr -d '-' < /proc/sys/kernel/random/uuid > /etc/machine-id - rm -f /run/workroom/broker.json /run/workroom/broker-token.json + # With the agent's interrupted saves (`.broker.json.`), which can hold a key. + rm -f /run/workroom/broker.json /run/workroom/broker-token.json /run/workroom/.broker* rm -rf /home/workroom/.local/state/workroom/screens hostname > "$IDENTITY" fi From 7d1f74735cfef69f1b236a503d19e22ca156fe7a Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 21:19:23 +0100 Subject: [PATCH 7/9] fix: address the pre-landing review of #252 From /review (specialists, Red Team, Claude and Codex adversarial passes): - delete-project refuses a project whose own host descriptor is set, in every mode. That descriptor is a base machine's only record; the old guard checked only workroom descriptors, so it orphaned the box. - derive and buildBase throw Failure.rollbackIncomplete, naming the host and grant still live, when undoing a failed step fails too. Before, the cleanup errors were swallowed and nothing recorded what was left running. - BaseLocks serialises a derive's snapshot, a refresh and a destroy per base. docker commit freezes git mid-fetch, so a snapshot taken during a refresh would hand its .lock files to every workroom derived from it. A caller cancelled while it waits gives the base up. - Fetching runs `git remote set-head origin --auto`, and checkout uses refs/remotes/origin/HEAD. fetch never moves origin/HEAD, so after the remote renamed its default branch every derive failed. - `host set|clear` honours --pretend, still checking that the entry exists. The commit sweep reads every line of output, and a failed run removes the host's ssh directory. - New tests: delete-project with a base, the host CLI's JSON envelope and --pretend, destroy with a failed grant cancel, destroyBase, a refused clone token, a rollback that fails, concurrent derives, a renamed default branch, and the per-base lock. Each was shown red without its fix. run.sh exports the fixture's clone token and broker URL. Full app suite through the fixture: 3119 passed, 0 failed. Also passing: Rust over_ssh (5), go test, cli-lint, app-lint, app-test-scripts (17) and shellcheck. Co-Authored-By: Claude Opus 5.5 --- TODOS.md | 13 +- cmd/delete_project.go | 5 + cmd/delete_project_test.go | 33 +++ cmd/host.go | 23 ++- cmd/host_test.go | 100 ++++++++++ docs/designs/remote-workrooms.md | 19 +- .../Core/ContainerHostDriver.swift | 20 +- macapp/WorkroomApp/Core/HostDriver.swift | 4 +- .../WorkroomApp/Core/RemoteProvisioning.swift | 143 +++++++++++-- ...ontainerProvisioningIntegrationTests.swift | 29 ++- .../RemoteProvisioningIntegrationTests.swift | 188 ++++++++++++++++-- .../RemoteProvisioningTests.swift | 47 +++++ vcs/scripts/ssh-fixture/run.sh | 6 +- 13 files changed, 564 insertions(+), 66 deletions(-) diff --git a/TODOS.md b/TODOS.md index a55ecf9b5..7837bfeb9 100644 --- a/TODOS.md +++ b/TODOS.md @@ -33,14 +33,17 @@ the rollback, and destroying the instance destroys the enrolled key. 5 holds bec provisioned VM hosts one workroom; the helper is global on purpose, since it must outrank boxd's system helper and answer the first clone, before any repository exists. -**How to start:** revisit 1, 2 and 4 when #252 (derivation) lands; stage a pending key beside the -current one and swap on success for 2, and have provisioning keep the grant ID when a cancel fails -for 4. For 3, sign in on a Dev build and on a Developer ID Nightly once codaset.dev serves the +**How to start:** #252 (derivation) settles part of 1 and 4. For 1, `ContainerHostDriver` refuses +to derive from an instance; every later driver needs the same refusal, and nothing stops a copy +made outside Workroom. For 4, provisioning now throws `RemoteProvisioning.Failure.rollbackIncomplete` +naming the grant when its own cancel fails, but a cancel that fails inside `AgentEnrolment.enrol` +is still swallowed there, so that grant ID is still lost. For 2, stage a pending key beside the +current one and swap on success. For 3, sign in on a Dev build and on a Developer ID Nightly once codaset.dev serves the broker, and check the item's access list in Keychain Access. Revisit 5 if enrolment ever runs on a bring-your-own host. -**Depends on / blocked by:** #252 for 1, 2 and 4; codaset#43 deployed for 3; a BYO-host enrolment -path for 5. +**Depends on / blocked by:** nothing for 2 and 4's remainder (#252 has landed); codaset#43 +deployed for 3; a BYO-host enrolment path for 5. **Priority:** P2, effort S. diff --git a/cmd/delete_project.go b/cmd/delete_project.go index 371f5d0eb..fc98b4f42 100644 --- a/cmd/delete_project.go +++ b/cmd/delete_project.go @@ -88,6 +88,11 @@ func runDeleteProject(svc *workroom.Service, jsonMode bool, confirm string, with if remote := projects[canon].RemoteWorkroomNames(); len(remote) > 0 { return fmt.Errorf("%w: %s has remote workrooms: %s", errs.ErrRemoteWorkroom, canon, strings.Join(remote, ", ")) } + // The project's own descriptor records its base machine (#252): the same record, at the + // project's level. + if projects[canon].Host != nil { + return fmt.Errorf("%w: %s has a remote host (its base machine)", errs.ErrRemoteWorkroom, canon) + } if fromDisk { // Guard: refuse obviously dangerous paths. diff --git a/cmd/delete_project_test.go b/cmd/delete_project_test.go index c6cc0da6a..fd814c6a1 100644 --- a/cmd/delete_project_test.go +++ b/cmd/delete_project_test.go @@ -432,6 +432,39 @@ func TestUnsafeProjectDeletePath(t *testing.T) { } } +// TestDeleteProjectRefusesAProjectWithABase: a project's own host descriptor records its base +// machine (#252), so every mode refuses a project carrying one even with no remote workroom, +// rather than dropping the only record of a running box. +func TestDeleteProjectRefusesAProjectWithABase(t *testing.T) { + for _, mode := range []struct { + name string + withWR, fromDisk bool + }{{"config-only", false, false}, {"with-workrooms", true, false}, {"from-disk", false, true}} { + t.Run(mode.name, func(t *testing.T) { + fake := &fakeVCS{list: []string{"alpha"}} + svc, cfg := newTestSvc(t, fake) + proj := t.TempDir() + canon, _ := config.CanonicalPath(proj) + cfg.AddWorkroom(canon, "alpha", "/wr/alpha", "git") + if err := cfg.SetHost(canon, "", map[string]any{"base": map[string]any{"host": "h1"}}); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(cfg.Path()) + + err := runDeleteProject(svc, true, canon, mode.withWR, mode.fromDisk, []string{proj}, &bytes.Buffer{}, &bytes.Buffer{}) + if !errors.Is(err, errs.ErrRemoteWorkroom) { + t.Fatalf("expected ErrRemoteWorkroom, got %v", err) + } + if len(fake.deleteCalls) != 0 { + t.Fatalf("workrooms were torn down: %v", fake.deleteCalls) + } + if after, _ := os.ReadFile(cfg.Path()); !bytes.Equal(before, after) { + t.Fatalf("config changed:\n%s", after) + } + }) + } +} + // TestDeleteProjectRefusesRemoteWorkrooms: every mode refuses a project with remote workrooms, // naming them, before tearing anything down or touching config. func TestDeleteProjectRefusesRemoteWorkrooms(t *testing.T) { diff --git a/cmd/host.go b/cmd/host.go index 0a04ff04c..f41aec529 100644 --- a/cmd/host.go +++ b/cmd/host.go @@ -72,7 +72,12 @@ func setHost(host map[string]any) error { if err != nil { return err } - if err := cfg.SetHost(dir, hostWorkroom, host); err != nil { + // --pretend checks the descriptor and that its entry exists, and writes nothing. + if pretend { + if err := hostEntryExists(cfg, dir, hostWorkroom); err != nil { + return err + } + } else if err := cfg.SetHost(dir, hostWorkroom, host); err != nil { return err } if jsonOutput { @@ -81,6 +86,22 @@ func setHost(host map[string]any) error { return nil } +// hostEntryExists refuses an entry SetHost would refuse, for --pretend. +func hostEntryExists(cfg *config.Config, dir, workroom string) error { + projects, err := cfg.AllProjects() + if err != nil { + return err + } + project, ok := projects[dir] + if !ok { + return fmt.Errorf("%w: %s", errs.ErrProjectNotFound, dir) + } + if _, ok := project.Workrooms[workroom]; workroom != "" && !ok { + return fmt.Errorf("%w: '%s' in %s", errs.ErrWorkroomNotFound, workroom, dir) + } + return nil +} + func init() { for _, c := range []*cobra.Command{hostSetCmd, hostClearCmd} { c.Flags().StringVar(&hostProject, "project", "", "Project directory (defaults to the current directory)") diff --git a/cmd/host_test.go b/cmd/host_test.go index d3d6684de..fb0bfb7f7 100644 --- a/cmd/host_test.go +++ b/cmd/host_test.go @@ -3,8 +3,11 @@ package cmd import ( "encoding/json" "errors" + "io" + "os" "testing" + "github.com/joelmoss/workroom/internal/config" "github.com/joelmoss/workroom/internal/errs" ) @@ -25,3 +28,100 @@ func TestDecodeHostRefusesWhatIsNotOneObject(t *testing.T) { } } } + +func TestHostSetWithPretendWritesNothing(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + project := t.TempDir() + cfg, err := config.New("") + if err != nil { + t.Fatal(err) + } + canon, _ := config.CanonicalPath(project) + if err := cfg.AddProject(canon, "git"); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(cfg.Path()) + hostProject, pretend = project, true + t.Cleanup(func() { hostProject, pretend = "", false }) + + if err := setHost(map[string]any{"state": "running"}); err != nil { + t.Fatal(err) + } + if after, _ := os.ReadFile(cfg.Path()); string(after) != string(before) { + t.Fatalf("--pretend wrote the config:\n%s", after) + } +} + +// runHostCLI runs the CLI as the app does and returns its exit code and the JSON envelope it +// printed on stdout. +func runHostCLI(t *testing.T, args ...string) (int, map[string]any) { + t.Helper() + read, write, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + stdout := os.Stdout + os.Stdout = write + // cobra keeps flag values between runs. + t.Cleanup(func() { + jsonOutput, hostProject, hostWorkroom, pretend, currentCommand = false, "", "", false, "" + rootCmd.SetArgs(nil) + }) + rootCmd.SetArgs(args) + code := Execute() + os.Stdout = stdout + write.Close() + out, _ := io.ReadAll(read) + jsonOutput, hostProject, hostWorkroom, pretend = false, "", "", false + var envelope map[string]any + if err := json.Unmarshal(out, &envelope); err != nil { + t.Fatalf("not one JSON envelope: %q", out) + } + return code, envelope +} + +func TestHostCommandsSpeakTheJSONContract(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + project := t.TempDir() + cfg, err := config.New("") + if err != nil { + t.Fatal(err) + } + canon, _ := config.CanonicalPath(project) + if err := cfg.AddWorkroom(canon, "foo", "/home/workroom/foo", "git"); err != nil { + t.Fatal(err) + } + + code, envelope := runHostCLI(t, "host", "set", `{"state":"running"}`, "--project", project, "--workroom", "foo", "--json") + if code != 0 || envelope["ok"] != true || envelope["command"] != "host" || envelope["workroom"] != "foo" { + t.Fatalf("set: exit %d, %v", code, envelope) + } + projects, _ := cfg.AllProjects() + if !projects[canon].Workrooms["foo"].IsRemote() { + t.Fatalf("the descriptor was not stored: %#v", projects[canon]) + } + + for _, refusal := range []struct { + args []string + code int + kind string + }{ + {[]string{"host", "set", "null", "--project", project, "--json"}, 2, "InvalidHostDescriptor"}, + {[]string{"host", "clear", "--project", t.TempDir(), "--json"}, 3, "ProjectNotFound"}, + {[]string{"host", "clear", "--project", project, "--workroom", "nope", "--json"}, 3, "WorkroomNotFound"}, + {[]string{"host", "set", "{}", "--project", project, "--workroom", "nope", "--json", "--pretend"}, 3, "WorkroomNotFound"}, + } { + code, envelope := runHostCLI(t, refusal.args...) + body, _ := envelope["error"].(map[string]any) + if code != refusal.code || envelope["ok"] != false || body["kind"] != refusal.kind { + t.Errorf("%v: exit %d, %v; want exit %d, %s", refusal.args, code, envelope, refusal.code, refusal.kind) + } + } + + code, envelope = runHostCLI(t, "host", "clear", "--project", project, "--workroom", "foo", "--json") + projects, _ = cfg.AllProjects() + if code != 0 || envelope["ok"] != true || projects[canon].Workrooms["foo"].IsRemote() { + t.Fatalf("clear: exit %d, %v, %#v", code, envelope, projects[canon].Workrooms["foo"]) + } +} diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index c4199ea81..aa023fcde 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1273,8 +1273,8 @@ these are the subsystems that actually gate "a remote workroom is a real workroo refused by `delete` like any remote one, so for now the only way to drop its entry is editing the config by hand. The app's delete confirmation for a remote row still describes a local delete; the refusal only comes after it. `RemoveWorkroom`'s implicit cleanup keeps a project - with a descriptor, but an explicit `delete-project` of a project with no remote workrooms - still drops it. Session restore is not gated on availability: + with a descriptor, and an explicit `delete-project` now refuses one too (closed by #252). + Session restore is not gated on availability: `TerminalSessions.materializeLivePersistentSessions` (via `PersistentSessionRecovery`) calls `ensureSurfaceCreated` for every restored pane whose local session is still live, without checking `unavailability`, so a saved pane of a workroom that has since turned remote @@ -1311,9 +1311,18 @@ these are the subsystems that actually gate "a remote workroom is a real workroo - **A workroom** runs derive, then enrol (`AgentEnrolment`, OQ10, so each key is the instance's own), then fetch and `git switch --create origin/HEAD`, and is returned serving over its agent connection. A failure at any step cancels its grant and destroys the - instance. A failed grant cancel does not keep the box: the key that would mint against the - grant goes with it. An instance is never derived from, because its disk holds its key and - credential helper. + instance. If undoing a step fails too, `Failure.rollbackIncomplete` names the host and grant + still live, for the caller to record and finish. A failed grant cancel does not keep the box: + the key that would mint against the grant goes with it. An instance is never derived from, + because its disk holds its key and credential helper. The workroom branches from + `refs/remotes/origin/HEAD` after `git remote set-head origin --auto`, since `fetch` never + moves `origin/HEAD` when the remote's default branch is renamed. + - **One operation at a time on a base.** `BaseLocks` serialises a derive's snapshot, a refresh + and a destroy per base: `docker commit` freezes git wherever it is, so a snapshot taken + during a refresh's fetch would hand its `.lock` files to every workroom derived from it. + In-process only. + - **`delete-project` refuses a project with a base**, as it refuses one with remote + workrooms: the project's own descriptor is the base's only record. - **The driver names what it makes.** The runtime's own IDs are known only from its output, so a `run` or `commit` that outlives its CLI (killed by the silence bound, or a cancelled derive) would leave a resource nothing could find. A container is named `workroom-` and a diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index 0b45492a5..72b2d586d 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -116,9 +116,9 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { return try await run(image, image: image) } catch { await Self.cleanUp { - for image in try await self.runtime(["images", "-aq", "--filter", "label=\(commit)"]) - .split(separator: "\n") - { + for image in try await self.runtime( + ["images", "-aq", "--filter", "label=\(commit)"], allLines: true + ).split(separator: "\n") { _ = try? await self.runtime(["rmi", "--force", String(image)]) } } @@ -182,6 +182,8 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { provisioned[id] = nil } await Self.cleanUp { _ = try await self.runtime(["rm", "--force", "--volumes", container]) } + // The login wait wrote the host's ssh_config and pinned key here. + try? FileManager.default.removeItem(at: directory.appendingPathComponent(id.uuidString)) throw error } } @@ -213,10 +215,12 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { throw HostDriverError.provisioning("ssh never let us in: \(last)") } - /// One runtime command; its first line of output, which is all any of these print on success. - /// Its environment is the few variables that say which daemon to talk to, never the app's whole - /// one. - private func runtime(_ arguments: [String], timeout: TimeInterval = 120) async throws -> String { + /// One runtime command; its first line of output, which is all most of these print on success, + /// or with `allLines` all of it. Its environment is the few variables that say which daemon to + /// talk to, never the app's whole one. + private func runtime( + _ arguments: [String], timeout: TimeInterval = 120, allLines: Bool = false + ) async throws -> String { guard let provisioning else { throw HostDriverError.notImplemented("Provisioning") } let environment = ProcessInfo.processInfo.environment.filter { ["HOME", "PATH", "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG"].contains($0.key) @@ -229,7 +233,7 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { throw HostDriverError.provisioning( "\(provisioning.runtime.lastPathComponent) \(arguments[0]) exited \(status): \(said)") } - return said.split(separator: "\n").first.map(String.init) ?? "" + return allLines ? said : said.split(separator: "\n").first.map(String.init) ?? "" } /// Undoes a step whatever the caller's state: in a task of its own, because a cancelled caller diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift index 17aad3fa9..0c4ff6922 100644 --- a/macapp/WorkroomApp/Core/HostDriver.swift +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -74,7 +74,9 @@ protocol HostTerminalDriver: HostDriver { 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. + /// From a driver that cannot provision, such as a `ContainerHostDriver` given no + /// `Provisioning`, for `create`, `deriveFromBase` and `destroy`; and from `destroy` for a host + /// the driver was handed rather than made. case notImplemented(String) case invalidConfiguration(String) /// Making, deriving or removing a host failed. Whatever the step had made is gone. diff --git a/macapp/WorkroomApp/Core/RemoteProvisioning.swift b/macapp/WorkroomApp/Core/RemoteProvisioning.swift index ae4bb3520..5404dc493 100644 --- a/macapp/WorkroomApp/Core/RemoteProvisioning.swift +++ b/macapp/WorkroomApp/Core/RemoteProvisioning.swift @@ -66,10 +66,16 @@ enum RemoteProvisioning { enum Failure: Error, Equatable, LocalizedError { case git(command: String, detail: String) + /// A step failed with `cause`, and undoing what the sequence had made failed too: `host` is + /// still up (nil once it is gone) and `grantID` still live (nil once cancelled). The caller + /// has to record them and finish the job, since nothing else knows about them. + case rollbackIncomplete(cause: String, host: HostID?, grantID: String?, cleanup: [String]) var errorDescription: String? { switch self { case .git(let command, let detail): return "git \(command) failed on the host: \(detail)" + case .rollbackIncomplete(let cause, _, _, let cleanup): + return "\(cause) Undoing it failed too: \(cleanup.joined(separator: "; "))" } } } @@ -104,8 +110,15 @@ enum RemoteProvisioning { try await record(base) return base } catch { - await cleanUp { try await environment.driver.destroy(host) } - throw error + let failed = await Task { () -> String? in + do { + try await environment.driver.destroy(host) + return nil + } catch { return error.localizedDescription } + }.value + guard let failed else { throw error } + throw Failure.rollbackIncomplete( + cause: error.localizedDescription, host: host, grantID: nil, cleanup: [failed]) } } @@ -113,20 +126,34 @@ enum RemoteProvisioning { /// derived workroom fetches what it lacks itself, so this only shortens that fetch. The base /// never enrols, so it fetches with a clone token too. static func refreshBase(_ base: Base, in environment: Environment) async throws { - let connection = try await environment.connect(.remote(base.host)) - defer { Task { await connection.close() } } - try await withCloneToken(base.repository, in: environment) { header in - _ = try await git( - ["fetch", "--quiet", "--prune", "origin"], in: base.path, environment: header, - on: connection) + try await BaseLocks.shared.exclusively(on: base.host) { + let connection = try await environment.connect(.remote(base.host)) + defer { Task { await connection.close() } } + try await withCloneToken(base.repository, in: environment) { header in + try await fetch(base.path, environment: header, on: connection) + } } } + /// Fetches, then asks the remote which branch is its default. `fetch` never moves + /// `origin/HEAD`, which the clone fixed: after a rename of the default branch it would name the + /// old one, and once `--prune` drops that, nothing at all (git 2.39 has no `followRemoteHEAD`). + private static func fetch( + _ path: String, environment extra: [String: String] = [:], on connection: AgentVCSConnection + ) async throws { + _ = try await git( + ["fetch", "--quiet", "--prune", "origin"], in: path, environment: extra, on: connection) + _ = try await git( + ["remote", "set-head", "origin", "--auto"], in: path, environment: extra, on: connection) + } + /// Removes the base and its record. Workrooms derived from it are not affected: each is a copy. static func destroyBase( _ base: Base, in environment: Environment, forget: @Sendable () async throws -> Void ) async throws { - try await environment.driver.destroy(.remote(base.host)) + try await BaseLocks.shared.exclusively(on: base.host) { + try await environment.driver.destroy(.remote(base.host)) + } try await forget() } @@ -162,11 +189,16 @@ enum RemoteProvisioning { /// Derives a workroom from `base`, enrols it, and checks its own `branch` out from the remote's /// default branch. Returns it serving. On a failure at any step, the grant is cancelled (by the - /// enrolment itself when it was the step that failed) and the instance destroyed. + /// enrolment itself when it was the step that failed) and the instance destroyed; if either of + /// those fails too, `Failure.rollbackIncomplete` says what is still live. static func derive( from base: Base, workroom: UUID, branch: String, in environment: Environment ) async throws -> Instance { - let host = try await environment.driver.deriveFromBase(.remote(base.host)) + // The snapshot only: a commit taken while a refresh's fetch holds its ref locks would hand + // every workroom derived from it those stale `.lock` files. + let host = try await BaseLocks.shared.exclusively(on: base.host) { + try await environment.driver.deriveFromBase(.remote(base.host)) + } var connection: AgentVCSConnection? var grant: String? do { @@ -178,22 +210,38 @@ enum RemoteProvisioning { repository: base.repository, agentBroker: environment.agentBroker) grant = grantID // With the instance's own credentials: `wr-agent enrol` made its helper git's. + try await fetch(base.path, on: connected) + // Fully qualified: a tag or branch named `origin/HEAD` would make the short form ambiguous. _ = try await git( - ["fetch", "--quiet", "--prune", "origin"], in: base.path, on: connected) - _ = try await git( - ["switch", "--quiet", "--no-track", "--create", branch, "origin/HEAD"], in: base.path, - on: connected) + ["switch", "--quiet", "--no-track", "--create", branch, "refs/remotes/origin/HEAD"], + in: base.path, on: connected) return Instance( host: host, grantID: grantID, path: base.path, branch: branch, connection: connected) } catch { - let (made, client) = (grant, environment.client) - await cleanUp { - await connection?.close() - if let made { try? await client.cancelGrant(made) } + let (made, client, opened) = (grant, environment.client, connection) + // In a task of its own, so a cancelled derive still undoes itself. + let (grantLive, hostLive, failures) = await Task { + () -> (String?, HostID?, [String]) in + await opened?.close() + var grantLive: String? + var hostLive: HostID? + var failures: [String] = [] + if let made { + do { try await client.cancelGrant(made) } catch { + grantLive = made + failures.append("cancelling grant \(made): \(error.localizedDescription)") + } + } await environment.agentBroker.release(workroom) - try await environment.driver.destroy(host) - } - throw error + do { try await environment.driver.destroy(host) } catch { + hostLive = host + failures.append("destroying the instance: \(error.localizedDescription)") + } + return (grantLive, hostLive, failures) + }.value + guard !failures.isEmpty else { throw error } + throw Failure.rollbackIncomplete( + cause: error.localizedDescription, host: hostLive, grantID: grantLive, cleanup: failures) } } @@ -240,6 +288,57 @@ enum RemoteProvisioning { } } +/// One operation at a time on each base (#252): a derive's snapshot, a refresh, a destroy. A +/// `docker commit` freezes the base wherever its git happens to be, so a snapshot taken during a +/// refresh's fetch would keep that fetch's `.lock` files. Different bases go on at once. +/// ponytail: in this process only; a second app instance could still overlap. Upgrade path: a +/// lock on the host, taken through the agent. +actor BaseLocks { + static let shared = BaseLocks() + private var busy: Set = [] + private var waiting: [UUID: [CheckedContinuation]] = [:] + + func exclusively(on base: UUID, _ body: @Sendable () async throws -> T) + async throws -> T + { + await acquire(base) + // A caller cancelled while it waited gives the base up rather than start a 15-minute commit + // nobody wants. ponytail: it still waits its turn first; a waiter is not dequeued on + // cancellation. + if Task.isCancelled { + release(base) + throw CancellationError() + } + do { + let value = try await body() + release(base) + return value + } catch { + release(base) + throw error + } + } + + private func acquire(_ base: UUID) async { + guard busy.contains(base) else { + busy.insert(base) + return + } + await withCheckedContinuation { waiting[base, default: []].append($0) } + } + + /// Hands the base straight to the next waiter, so nobody can slip in between. + private func release(_ base: UUID) { + if var queue = waiting[base], !queue.isEmpty { + let next = queue.removeFirst() + waiting[base] = queue.isEmpty ? nil : queue + next.resume() + } else { + busy.remove(base) + } + } +} + extension RemoteProvisioning { enum CloneToken { /// Revokes an installation token at GitHub, which accepts the call authenticated as the token diff --git a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift index 88c7cc99e..78c164dc8 100644 --- a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift @@ -233,8 +233,9 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { XCTAssertTrue(written, "the screen was never written to \(record)") first.dropLink() - // The newest container with this test's label is the instance. - let container = try docker(runtime, ["ps", "-l", "-q", "--filter", "label=\(label!)"]) + // The driver names each container after its host. + guard case .remote(let id) = instance else { return XCTFail("not a remote host") } + let container = "workroom-\(id.uuidString.lowercased())" try docker(runtime, ["restart", "-t", "0", container]) let second = try RemoteHostIntegrationTests.Pane( @@ -264,4 +265,28 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { for host in [instance, base] { try await driver.destroy(host) } } + + /// Derives from one base at once: each gets a host, port and identity of its own, and the + /// driver's registry keeps them apart. + func testConcurrentDerivesFromOneBaseEachGetTheirOwnHost() async throws { + let provisioning = try provisioning() + let driver = ContainerHostDriver(hosts: [:], directory: directory, provisioning: provisioning) + let base = try await driver.create() + + let instances = try await withThrowingTaskGroup(of: HostID.self) { group in + for _ in 0..<3 { group.addTask { try await driver.deriveFromBase(base) } } + return try await group.reduce(into: [HostID]()) { $0.append($1) } + } + + XCTAssertEqual(Set(instances).count, 3) + var keys: Set = [] + for instance in instances { + keys.insert( + try await onHost(driver, instance, "cut -d' ' -f2 /etc/ssh/ssh_host_ed25519_key.pub")) + } + XCTAssertEqual(keys.count, 3, "concurrent derives share a host key") + for host in instances + [base] { try await driver.destroy(host) } + XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) + XCTAssertEqual(try leftovers("images", runtime: runtime, label: label), []) + } } diff --git a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift index 3be8f0787..a5e01ba12 100644 --- a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift @@ -19,7 +19,10 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { private var broker: StubBroker! private var cleanups: [() async -> Void] = [] - private static let token = "ghs_fixture_clone_token_0123456789" + /// The base clone token the fixture's GitHub accepts (run.sh reads it out of the image). + private static var token: String { + ProcessInfo.processInfo.environment["WR_SSH_FIXTURE_CLONE_TOKEN"] ?? "" + } private static let path = "/home/workroom/project" override func setUp() async throws { @@ -61,6 +64,10 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { return value } runtime = URL(fileURLWithPath: try need("WR_SSH_FIXTURE_RUNTIME_PATH")) + // Read where they are used (`token`, the disconnected test's broker); needed here so an + // older run.sh skips these tests rather than failing them. + _ = try need("WR_SSH_FIXTURE_CLONE_TOKEN") + _ = try need("WR_SSH_FIXTURE_BROKER_URL") return Fixture( provisioning: ContainerHostDriver.Provisioning( runtime: override ?? runtime, image: try need("WR_SSH_FIXTURE_IMAGE"), @@ -124,8 +131,10 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { revoke: { revoked.add($0.token) }) } - private static let cloneToken = BrokerStub.Answer( - body: #"{"token":"\#(token)","expires_at":"2026-10-01T18:00:00Z"}"#) + private static var cloneToken: BrokerStub.Answer { + BrokerStub.Answer( + body: #"{"token":"\#(token)","expires_at":"2026-10-01T18:00:00Z"}"#) + } private static let grant = BrokerStub.Answer( status: 201, body: #"{"grant_id":"g1","enrolment_code":"one-time","repository_id":1,"expires_at":"x"}"#) @@ -174,6 +183,24 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { }.count } + /// The fixture with a runtime that runs the real one, except for the subcommand named in the + /// returned file while it exists: a provider failing one step on purpose. + private func failingFixture() throws -> (Fixture, failing: URL) { + let script = directory.appendingPathComponent("runtime") + let failing = directory.appendingPathComponent("failing") + let fixture = try fixture(runtime: script) + try """ + #!/bin/sh + if [ "$1" = "$(cat \(PosixShell.quoted(failing.path)) 2>/dev/null)" ]; then + echo "injected failure" >&2; exit 1 + fi + exec \(PosixShell.quoted(runtime.path)) "$@" + + """.write(to: script, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: script.path) + return (fixture, failing) + } + // MARK: The base @MainActor @@ -306,18 +333,7 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { @MainActor func testAWorkroomThatFailsAtEachStepLeavesNoInstanceAndNoGrant() async throws { // The runtime fails `commit` only when told to, so the base is built with it as it is. - let script = directory.appendingPathComponent("runtime") - let failing = directory.appendingPathComponent("failing") - let fixture = try fixture(runtime: script) - try """ - #!/bin/sh - if [ -f \(PosixShell.quoted(failing.path)) ] && [ "$1" = commit ]; then - echo "injected failure" >&2; exit 1 - fi - exec \(PosixShell.quoted(runtime.path)) "$@" - - """.write(to: script, atomically: true, encoding: .utf8) - try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: script.path) + let (fixture, failing) = try failingFixture() let driver = ContainerHostDriver( hosts: [:], directory: directory, provisioning: fixture.provisioning) let base = try await build(environment(fixture, driver: driver, revoked: Revoked())) @@ -346,7 +362,7 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { ] for (step, environment, branch, granted) in steps { if step == "derive" { - try Data().write(to: failing) + try "commit".write(to: failing, atomically: true, encoding: .utf8) } else { try? FileManager.default.removeItem(at: failing) } @@ -356,7 +372,15 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { _ = try await RemoteProvisioning.derive( from: base, workroom: UUID(), branch: branch, in: environment) XCTFail("a workroom that failed at \(step) was made") - } catch {} + } catch { + // The failure is the step's own, so the assertions below are about that step. + switch (step, error) { + case ("derive", HostDriverError.provisioning), ("connect", is Injected), + ("enrol", BrokerError.agent), ("checkout", RemoteProvisioning.Failure.git("switch", _)): + break + default: XCTFail("\(step) failed for another reason: \(error)") + } + } XCTAssertEqual( try leftovers("ps", label: label), containers, "a failure at \(step) left an instance") @@ -379,7 +403,8 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { hosts: [:], directory: directory, provisioning: fixture.provisioning) let environment = environment( fixture, driver: driver, revoked: Revoked(), - brokerURL: URL(string: "http://127.0.0.1:8081")) + brokerURL: URL(string: ProcessInfo.processInfo.environment["WR_SSH_FIXTURE_BROKER_URL"] ?? "") + ) BrokerStub.reset([Self.cloneToken]) let base = try await RemoteProvisioning.buildBase( repository: "o/r", cloneURL: "https://github.com/origin.git", path: Self.path, @@ -391,8 +416,9 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { cleanups.append { try? await driver.destroy(instance.host) } await instance.connection.close() - // The newest container with this test's label is the instance. - let container = try docker(runtime, ["ps", "-l", "-q", "--filter", "label=\(label!)"]) + // The driver names each container after its host. + guard case .remote(let id) = instance.host else { return XCTFail("not a remote host") } + let container = "workroom-\(id.uuidString.lowercased())" func onBox(_ script: String) throws -> String { try docker( runtime, @@ -425,4 +451,126 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { XCTAssertEqual( try onBox("git -C /srv/origin.git rev-parse refs/heads/wr-pushed"), "\(pushed)\nexit=0") } + + /// A workroom branches from the remote's default branch as it is now, not as it was when the + /// base was cloned: `fetch` never moves `origin/HEAD`, and once `--prune` drops the old default + /// it names nothing. + @MainActor + func testAWorkroomBranchesFromTheDefaultBranchAfterTheRemoteRenamedIt() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + let renamed = try await onHost( + driver, .remote(base.host), + "git -C /srv/origin.git branch -m main trunk" + + " && git -C /srv/origin.git symbolic-ref HEAD refs/heads/trunk") + XCTAssertEqual(renamed.status, 0, renamed.output) + + BrokerStub.reset([Self.grant]) + let instance = try await RemoteProvisioning.derive( + from: base, workroom: UUID(), branch: "wr-renamed", in: environment) + cleanups.append { try? await driver.destroy(instance.host) } + + let head = try await RemoteProvisioning.git( + ["symbolic-ref", "refs/remotes/origin/HEAD"], in: instance.path, on: instance.connection) + XCTAssertEqual( + head.trimmingCharacters(in: .whitespacesAndNewlines), "refs/remotes/origin/trunk") + } + + @MainActor + func testABaseWhoseCloneTokenIsRefusedIsRemovedWithNothingToRevoke() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let revoked = Revoked() + BrokerStub.reset([.init(status: 403, body: #"{"error":"no_read_access"}"#)]) + + do { + _ = try await RemoteProvisioning.buildBase( + repository: "o/r", cloneURL: "/srv/origin.git", path: Self.path, + in: environment(fixture, driver: driver, revoked: revoked), record: { _ in }) + XCTFail("a base was built without a clone token") + } catch BrokerError.refused(let refusal) { + XCTAssertEqual(refusal.code, "no_read_access") + } + XCTAssertEqual(revoked.all, [], "a token that was never minted was revoked") + XCTAssertEqual(try leftovers("ps", label: label), [], "the base outlived its refused token") + } + + @MainActor + func testDestroyingABaseRemovesItAndForgetsItsRecordOnlyOnceItIsGone() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + let forgotten = Revoked() + + try await RemoteProvisioning.destroyBase( + base, in: environment, forget: { forgotten.add("forgot") }) + XCTAssertEqual(forgotten.all, ["forgot"]) + XCTAssertEqual(try leftovers("ps", label: label), []) + + // A base the driver no longer knows keeps its record: nothing has shown it is gone. + do { + try await RemoteProvisioning.destroyBase( + base, in: environment, forget: { forgotten.add("forgot again") }) + XCTFail("an unknown base was destroyed") + } catch HostDriverError.unknownHost {} + XCTAssertEqual(forgotten.all, ["forgot"]) + } + + @MainActor + func testDestroyingAWorkroomWhoseGrantCannotBeCancelledStillRemovesItsBox() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + BrokerStub.reset([Self.grant]) + let workroom = UUID() + let instance = try await RemoteProvisioning.derive( + from: base, workroom: workroom, branch: "wr-cancel", in: environment) + + BrokerStub.reset([.init(status: 503, body: #"{"error":"github_unavailable"}"#)]) + do { + try await RemoteProvisioning.destroy(instance, workroom: workroom, in: environment) + XCTFail("a failed grant cancel was swallowed") + } catch BrokerError.refused {} + XCTAssertEqual(try leftovers("ps", label: label).count, 1, "the instance's box was kept") + } + + /// A failure whose undoing fails too names what is still live (#252 review), rather than + /// leaving a box holding an enrolled key and a live grant that nothing records. + @MainActor + func testAWorkroomWhoseRollbackFailsSaysWhatIsStillLive() async throws { + let (fixture, failing) = try failingFixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + // The checkout fails on its branch name, then the grant cancel and the box's removal fail. + BrokerStub.reset([Self.grant, .init(status: 503, body: #"{"error":"github_unavailable"}"#)]) + try "rm".write(to: failing, atomically: true, encoding: .utf8) + defer { try? FileManager.default.removeItem(at: failing) } + + do { + _ = try await RemoteProvisioning.derive( + from: base, workroom: UUID(), branch: "bad..name", in: environment) + XCTFail("a workroom with an unusable branch was made") + } catch RemoteProvisioning.Failure.rollbackIncomplete( + let cause, let host, let grant, let cleanup) + { + XCTAssertTrue(cause.contains("switch"), cause) + XCTAssertNotNil(host, "the box still up was not named") + XCTAssertEqual(grant, "g1", "the grant still live was not named") + XCTAssertEqual(cleanup.count, 2, "\(cleanup)") + } + XCTAssertEqual(try leftovers("ps", label: label).count, 2, "the instance should still be up") + } } diff --git a/macapp/WorkroomAppTests/RemoteProvisioningTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningTests.swift index d56998a6f..8b7f1ce2b 100644 --- a/macapp/WorkroomAppTests/RemoteProvisioningTests.swift +++ b/macapp/WorkroomAppTests/RemoteProvisioningTests.swift @@ -17,4 +17,51 @@ final class RemoteProvisioningTests: XCTestCase { XCTAssertEqual(String(decoding: credentials, as: UTF8.self), "x-access-token:ghs_secret") XCTAssertFalse(environment.values.contains { $0.contains("ghs_secret") }) } + + /// One operation at a time on a base, and different bases at once. + func testOperationsOnOneBaseTakeTurnsAndOnDifferentBasesOverlap() async throws { + final class Probe: @unchecked Sendable { + private let lock = NSLock() + private var running: [UUID: Int] = [:] + private(set) var most: [UUID: Int] = [:] + var together = 0 + private var now = 0 + func enter(_ base: UUID) { + lock.withLock { + running[base, default: 0] += 1 + most[base] = max(most[base] ?? 0, running[base]!) + now += 1 + together = max(together, now) + } + } + func leave(_ base: UUID) { + lock.withLock { + running[base]! -= 1 + now -= 1 + } + } + var peaks: [UUID: Int] { lock.withLock { most } } + var overlap: Int { lock.withLock { together } } + } + let probe = Probe() + let (first, second) = (UUID(), UUID()) + let locks = BaseLocks() + + try await withThrowingTaskGroup(of: Void.self) { group in + for base in [first, first, first, second, second, second] { + group.addTask { + try await locks.exclusively(on: base) { + probe.enter(base) + try await Task.sleep(for: .milliseconds(50)) + probe.leave(base) + } + } + } + try await group.waitForAll() + } + + XCTAssertEqual(probe.peaks[first], 1, "two operations on one base overlapped") + XCTAssertEqual(probe.peaks[second], 1, "two operations on one base overlapped") + XCTAssertEqual(probe.overlap, 2, "different bases did not run at once") + } } diff --git a/vcs/scripts/ssh-fixture/run.sh b/vcs/scripts/ssh-fixture/run.sh index 0356e66b3..2e5c99f91 100755 --- a/vcs/scripts/ssh-fixture/run.sh +++ b/vcs/scripts/ssh-fixture/run.sh @@ -124,12 +124,14 @@ done # AGENT: the ELF itself, for the tests of the bootstrap that pushes it (#231). The app's tests # take it as their bundled agent, since a Debug build carries no Linux agent of its own. CONTAINER, # RUNTIME and SCREENS: for the test that reboots the box (#232). IMAGE, RUNTIME_PATH, PUBLIC_KEY -# and LABEL: for the app's tests that provision containers of their own from the image (#252). +# and LABEL: for the app's tests that provision containers of their own from the image (#252), +# and CLONE_TOKEN and BROKER_URL for the image's GitHub and broker (fake-github.py). +CLONE_TOKEN="$("$RUNTIME" exec "$NAME" cat /etc/workroom-fixture/clone-token)" 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" "AGENT=$STAGE/wr-agent" \ "CONTAINER=$NAME" "RUNTIME=$RUNTIME" "SCREENS=$SCREENS" "IMAGE=$IMAGE" \ "RUNTIME_PATH=$(command -v "$RUNTIME")" "PUBLIC_KEY=$(cat "$STAGE/id_ed25519.pub")" \ - "LABEL=$LABEL"; do + "LABEL=$LABEL" "CLONE_TOKEN=$CLONE_TOKEN" "BROKER_URL=http://127.0.0.1:8081"; do export "WR_SSH_FIXTURE_$pair" "TEST_RUNNER_WR_SSH_FIXTURE_$pair" done From 53ebd5f0c9e670ada3f67c4fb0f314fdf0479c1f Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 22:11:45 +0100 Subject: [PATCH 8/9] fix: address PR review findings Greptile and CodeRabbit on #280: - delete-project: RemoveProject re-checks for a host descriptor (the project's or a workroom's) under its own lock, so a base recorded after delete-project's first check is no longer removed with the entry. (CodeRabbit) - AgentEnrolment.enrol throws GrantStillLive with the grant ID when its own cancel fails, and derive passes it on in rollbackIncomplete. Before, that grant was lost. (Greptile) - The container driver reports a container or image it could not remove as HostDriverError.leftBehind, instead of dropping the host with its container still running. (Greptile) - Destroying a host twice succeeds, so destroyBase can retry a failed forget. A base the driver never knew still keeps its record. (Greptile) - RemoteProvisioning.destroy reports both a failed cancel and a failed removal in rollbackIncomplete. (Greptile) - refreshBase is tested: a refresh fetches with a fresh token, revokes it, and a refused token leaves the base as it was. (Greptile) - The .provisioning doc no longer promises that cleanup always succeeded. (CodeRabbit) Each new test was shown red without its fix. Full app suite through the fixture: 3123 passed, 0 failed. Also passing: Rust over_ssh (5), go test, cli-lint, app-lint, app-test-scripts and shellcheck. Co-Authored-By: Claude Opus 5.5 --- TODOS.md | 11 +- docs/designs/remote-workrooms.md | 10 +- internal/config/config.go | 11 +- internal/config/config_test.go | 20 +++ .../Core/Broker/AgentEnrolment.swift | 22 +++- .../Core/ContainerHostDriver.swift | 47 +++++--- macapp/WorkroomApp/Core/HostDriver.swift | 10 +- .../WorkroomApp/Core/RemoteProvisioning.swift | 51 ++++++-- .../Broker/BrokerSignInTests.swift | 20 +++ ...ontainerProvisioningIntegrationTests.swift | 31 ++++- .../RemoteProvisioningIntegrationTests.swift | 114 +++++++++++++++++- 11 files changed, 301 insertions(+), 46 deletions(-) diff --git a/TODOS.md b/TODOS.md index 7837bfeb9..8892f6f9b 100644 --- a/TODOS.md +++ b/TODOS.md @@ -35,15 +35,14 @@ system helper and answer the first clone, before any repository exists. **How to start:** #252 (derivation) settles part of 1 and 4. For 1, `ContainerHostDriver` refuses to derive from an instance; every later driver needs the same refusal, and nothing stops a copy -made outside Workroom. For 4, provisioning now throws `RemoteProvisioning.Failure.rollbackIncomplete` -naming the grant when its own cancel fails, but a cancel that fails inside `AgentEnrolment.enrol` -is still swallowed there, so that grant ID is still lost. For 2, stage a pending key beside the -current one and swap on success. For 3, sign in on a Dev build and on a Developer ID Nightly once codaset.dev serves the +made outside Workroom. 4 is fixed: `AgentEnrolment.enrol` throws `GrantStillLive` with the grant ID when its own cancel +fails, and provisioning passes it on in `RemoteProvisioning.Failure.rollbackIncomplete`. For 2, +stage a pending key beside the current one and swap on success. For 3, sign in on a Dev build and on a Developer ID Nightly once codaset.dev serves the broker, and check the item's access list in Keychain Access. Revisit 5 if enrolment ever runs on a bring-your-own host. -**Depends on / blocked by:** nothing for 2 and 4's remainder (#252 has landed); codaset#43 -deployed for 3; a BYO-host enrolment path for 5. +**Depends on / blocked by:** nothing for 2 (#252 has landed); codaset#43 deployed for 3; a BYO-host +enrolment path for 5. **Priority:** P2, effort S. diff --git a/docs/designs/remote-workrooms.md b/docs/designs/remote-workrooms.md index aa023fcde..c4f77c138 100644 --- a/docs/designs/remote-workrooms.md +++ b/docs/designs/remote-workrooms.md @@ -1312,7 +1312,10 @@ these are the subsystems that actually gate "a remote workroom is a real workroo instance's own), then fetch and `git switch --create origin/HEAD`, and is returned serving over its agent connection. A failure at any step cancels its grant and destroys the instance. If undoing a step fails too, `Failure.rollbackIncomplete` names the host and grant - still live, for the caller to record and finish. A failed grant cancel does not keep the box: + still live, for the caller to record and finish, including a grant the enrolment itself + could not cancel (`AgentEnrolment.GrantStillLive`); `destroy` reports the same way. The + driver's own failed cleanup is `HostDriverError.leftBehind`, naming what is still there, and + destroying a host twice succeeds, so a caller can retry what came after it. A failed grant cancel does not keep the box: the key that would mint against the grant goes with it. An instance is never derived from, because its disk holds its key and credential helper. The workroom branches from `refs/remotes/origin/HEAD` after `git remote set-head origin --auto`, since `fetch` never @@ -1344,7 +1347,10 @@ these are the subsystems that actually gate "a remote workroom is a real workroo (`exec_timeout`, capped at 610 s), not a silence bound, so a large repository over a slow link fails its clone and its base is removed. Upgrade path: start the clone detached on the host and poll for it. - - `refreshBase` has no test. + - A `docker commit` or `run` whose CLI was killed can finish in the daemon after the + driver's one cleanup sweep, leaving an image or container nothing records. Deferred to + #253, whose re-adoption needs a reconciler that sweeps labelled resources the driver + does not know, which covers this too (owner's decision, 2026-10-01). - `deriveSpeed` is declared as 5 s; measured derives on the fixture take 2–3 s. - **Cross-machine session enumeration** (from Phase 1's S4 note): agent-side session naming, a pane→tab mapping on the wire, and workroom UI state stored with the workroom. diff --git a/internal/config/config.go b/internal/config/config.go index 038434438..4d4a1fa53 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -429,7 +429,8 @@ func (c *Config) RemoveWorkroomKeepProject(parentPath, name string) error { // config. It does NOT touch the filesystem or VCS — any worktree/workspace teardown // is the caller's job (via Service.Delete). Idempotent: an absent project is a no-op // returning nil. Reserved scalar keys (e.g. "workrooms_dir", "channel") are never -// deletable through here. +// deletable through here, and neither is a project carrying a host descriptor, its own or +// a workroom's (ErrRemoteWorkroom). func (c *Config) RemoveProject(parentPath string) error { return c.withLock(func() error { data, err := c.Read() @@ -439,6 +440,14 @@ func (c *Config) RemoveProject(parentPath string) error { if isReserved(parentPath) { return nil // reserved key, not a project } + // Re-checked here, under the lock, rather than trusted from the caller's earlier read: a + // descriptor is the only record of a remote machine, and one written since (a base + // recorded, a workroom turned remote) must not go with the entry. + if raw, ok := data[parentPath].(map[string]any); ok { + if project := decodeProject(raw); project.Host != nil || len(project.RemoteWorkroomNames()) > 0 { + return fmt.Errorf("%w: %s has a remote host", errs.ErrRemoteWorkroom, parentPath) + } + } delete(data, parentPath) return c.Write(data) }) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7f8935404..9f93795b0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -514,3 +514,23 @@ func TestSetHostNeverCreatesAnEntry(t *testing.T) { t.Fatalf("config gained entries: %#v", data) } } + +// RemoveProject re-checks under its own lock what delete-project checked before tearing down: a +// descriptor written in between (a base recorded, a workroom turned remote) keeps the entry. +func TestRemoveProjectKeepsAProjectThatGainedAHost(t *testing.T) { + for _, workroom := range []string{"", "foo"} { + c := newTestConfig(t) + if err := c.AddWorkroom("/project", "foo", "/foo", "git"); err != nil { + t.Fatal(err) + } + if err := c.SetHost("/project", workroom, map[string]any{"state": "running"}); err != nil { + t.Fatal(err) + } + if err := c.RemoveProject("/project"); !errors.Is(err, errs.ErrRemoteWorkroom) { + t.Fatalf("workroom %q: err = %v, want ErrRemoteWorkroom", workroom, err) + } + if projects, _ := c.AllProjects(); projects["/project"].Workrooms["foo"].Path != "/foo" { + t.Fatalf("workroom %q: the project was removed", workroom) + } + } +} diff --git a/macapp/WorkroomApp/Core/Broker/AgentEnrolment.swift b/macapp/WorkroomApp/Core/Broker/AgentEnrolment.swift index 3f1a2c9a4..e5f25bf89 100644 --- a/macapp/WorkroomApp/Core/Broker/AgentEnrolment.swift +++ b/macapp/WorkroomApp/Core/Broker/AgentEnrolment.swift @@ -76,12 +76,32 @@ enum AgentEnrolment { } catch { // In its own task: a cancelled enrolment's task would cancel this request too, and leave a // grant live that the agent may already have enrolled against. - await Task { try? await client.cancelGrant(grant.grantId) }.value + let cancelFailure = await Task { () -> String? in + do { + try await client.cancelGrant(grant.grantId) + return nil + } catch { return error.localizedDescription } + }.value await agentBroker.release(workroomID) + if let cancelFailure { + throw GrantStillLive(grantID: grant.grantId, cause: error, cancelFailure: cancelFailure) + } throw error } } + /// An enrolment failed with `cause`, and its grant could not be cancelled either: `grantID` is + /// still live, and the caller is the only one left who knows it. + struct GrantStillLive: Error, LocalizedError { + let grantID: String + let cause: any Error + let cancelFailure: String + + var errorDescription: String? { + "\(cause.localizedDescription) Its grant could not be cancelled: \(cancelFailure)" + } + } + /// `wr-agent enrol` prints a broker refusal's code on a `refusal:` line; anything else is the /// agent's own error. Either way it is the agent's failure, never this Mac's: a remote /// `unknown_key` must not sign the Mac out. diff --git a/macapp/WorkroomApp/Core/ContainerHostDriver.swift b/macapp/WorkroomApp/Core/ContainerHostDriver.swift index 72b2d586d..78af3119e 100644 --- a/macapp/WorkroomApp/Core/ContainerHostDriver.swift +++ b/macapp/WorkroomApp/Core/ContainerHostDriver.swift @@ -68,6 +68,9 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { private let lock = NSLock() private var hosts: [UUID: Host] private var provisioned: [UUID: Provisioned] = [:] + /// Hosts this driver has destroyed, so destroying one again succeeds: a caller retrying after a + /// later step of its own failed (forgetting its record) gets past the destroy it already did. + private var destroyed: Set = [] init(hosts: [UUID: Host], directory: URL, provisioning: Provisioning? = nil) { self.hosts = hosts @@ -115,20 +118,33 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { timeout: 900) return try await run(image, image: image) } catch { - await Self.cleanUp { - for image in try await self.runtime( - ["images", "-aq", "--filter", "label=\(commit)"], allLines: true - ).split(separator: "\n") { - _ = try? await self.runtime(["rmi", "--force", String(image)]) - } + let removals = await Task { () -> [String] in + do { + var failed: [String] = [] + for image in try await self.runtime( + ["images", "-aq", "--filter", "label=\(commit)"], allLines: true + ).split(separator: "\n") { + do { _ = try await self.runtime(["rmi", "--force", String(image)]) } catch { + failed.append("image \(image): \(error.localizedDescription)") + } + } + return failed + } catch { return ["images labelled \(commit): \(error.localizedDescription)"] } + }.value + // A run that could not remove its container says so; its image goes on that list too. + var (cause, leftover) = (error.localizedDescription, removals) + if case HostDriverError.leftBehind(let inner, let left) = error { + (cause, leftover) = (inner, left + removals) } - throw error + guard !leftover.isEmpty else { throw error } + throw HostDriverError.leftBehind(cause: cause, leftover: leftover) } } /// Removes the container and the image it alone was run from. A host handed in rather than /// made here is not this driver's to remove. func destroy(_ host: HostID) async throws { + if case .remote(let id) = host, lock.withLock({ destroyed.contains(id) }) { return } guard case .remote(let id) = host, let made = lock.withLock({ provisioned[id] }) else { if case .remote(let id) = host, lock.withLock({ hosts[id] }) != nil { throw HostDriverError.notImplemented("Destroying a host this driver did not make") @@ -140,6 +156,7 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { lock.withLock { hosts[id] = nil provisioned[id] = nil + destroyed.insert(id) } try? FileManager.default.removeItem(at: directory.appendingPathComponent(id.uuidString)) } @@ -181,10 +198,16 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { hosts[id] = nil provisioned[id] = nil } - await Self.cleanUp { _ = try await self.runtime(["rm", "--force", "--volumes", container]) } + let removal = await Task { () -> String? in + do { + _ = try await self.runtime(["rm", "--force", "--volumes", container]) + return nil + } catch { return "container \(container): \(error.localizedDescription)" } + }.value // The login wait wrote the host's ssh_config and pinned key here. try? FileManager.default.removeItem(at: directory.appendingPathComponent(id.uuidString)) - throw error + guard let removal else { throw error } + throw HostDriverError.leftBehind(cause: error.localizedDescription, leftover: [removal]) } } @@ -236,12 +259,6 @@ final class ContainerHostDriver: HostTerminalDriver, @unchecked Sendable { return allLines ? said : said.split(separator: "\n").first.map(String.init) ?? "" } - /// Undoes a step whatever the caller's state: in a task of its own, because a cancelled caller - /// would otherwise cancel the cleanup too and leave the container running. - private static func cleanUp(_ body: @escaping @Sendable () async throws -> Void) async { - await Task { try? await body() }.value - } - func openStream(to host: HostID) async throws -> HostStream { let (_, target) = try target(host) return try await exec( diff --git a/macapp/WorkroomApp/Core/HostDriver.swift b/macapp/WorkroomApp/Core/HostDriver.swift index 0c4ff6922..4d4ba6ab3 100644 --- a/macapp/WorkroomApp/Core/HostDriver.swift +++ b/macapp/WorkroomApp/Core/HostDriver.swift @@ -79,8 +79,13 @@ enum HostDriverError: Error, Equatable, Sendable, LocalizedError { /// the driver was handed rather than made. case notImplemented(String) case invalidConfiguration(String) - /// Making, deriving or removing a host failed. Whatever the step had made is gone. + /// Making, deriving or removing a host failed. A failed `create` or `deriveFromBase` has + /// removed what it made (or says otherwise with `leftBehind`); a failed `destroy` leaves the + /// host as it was, still known to the driver, so it can be tried again. case provisioning(String) + /// A failed `create` or `deriveFromBase` could not remove what it had made: each of `leftover` + /// is still there, named by what the driver called it. + case leftBehind(cause: String, leftover: [String]) var errorDescription: String? { switch self { @@ -88,6 +93,9 @@ enum HostDriverError: Error, Equatable, Sendable, LocalizedError { case .notImplemented(let what): return "\(what) is not implemented yet." case .invalidConfiguration(let detail): return "Invalid host configuration: \(detail)" case .provisioning(let detail): return "Couldn't provision the host: \(detail)" + case .leftBehind(let cause, let leftover): + return + "Couldn't provision the host: \(cause) Still there: \(leftover.joined(separator: "; "))" } } } diff --git a/macapp/WorkroomApp/Core/RemoteProvisioning.swift b/macapp/WorkroomApp/Core/RemoteProvisioning.swift index 5404dc493..8559aa5e3 100644 --- a/macapp/WorkroomApp/Core/RemoteProvisioning.swift +++ b/macapp/WorkroomApp/Core/RemoteProvisioning.swift @@ -204,10 +204,16 @@ enum RemoteProvisioning { do { let connected = try await environment.connect(host) connection = connected - let grantID = try await AgentEnrolment.enrol( - client: environment.client, driver: environment.driver, host: host, - agentBinary: environment.agentBinary, workroomID: workroom, - repository: base.repository, agentBroker: environment.agentBroker) + let grantID: String + do { + grantID = try await AgentEnrolment.enrol( + client: environment.client, driver: environment.driver, host: host, + agentBinary: environment.agentBinary, workroomID: workroom, + repository: base.repository, agentBroker: environment.agentBroker) + } catch let live as AgentEnrolment.GrantStillLive { + // The enrolment's own cancel failed; the grant is still live, and only this knows it. + throw RollbackStart.grantLive(live.grantID, cause: live.cause, failure: live.cancelFailure) + } grant = grantID // With the instance's own credentials: `wr-agent enrol` made its helper git's. try await fetch(base.path, on: connected) @@ -218,6 +224,11 @@ enum RemoteProvisioning { return Instance( host: host, grantID: grantID, path: base.path, branch: branch, connection: connected) } catch { + var error = error + var alreadyFailed: (grant: String, failure: String)? + if case RollbackStart.grantLive(let live, let cause, let failure) = error { + (error, alreadyFailed) = (cause, (live, "cancelling grant \(live): \(failure)")) + } let (made, client, opened) = (grant, environment.client, connection) // In a task of its own, so a cancelled derive still undoes itself. let (grantLive, hostLive, failures) = await Task { @@ -239,24 +250,42 @@ enum RemoteProvisioning { } return (grantLive, hostLive, failures) }.value - guard !failures.isEmpty else { throw error } + let all = (alreadyFailed.map { [$0.failure] } ?? []) + failures + guard !all.isEmpty else { throw error } throw Failure.rollbackIncomplete( - cause: error.localizedDescription, host: hostLive, grantID: grantLive, cleanup: failures) + cause: error.localizedDescription, host: hostLive, + grantID: grantLive ?? alreadyFailed?.grant, cleanup: all) } } + /// Carries a live grant from the enrolment step into `derive`'s rollback. + private enum RollbackStart: Error { + case grantLive(String, cause: any Error, failure: String) + } + /// Takes a workroom down: its grant first, so nothing mints for it once its box is gone, then /// the box. A grant the broker could not cancel does not keep the box: the key that would mint - /// against it goes with the box. The first failure is thrown once both have been tried. + /// against it goes with the box. If either fails, `Failure.rollbackIncomplete` names what is + /// still live once both have been tried. static func destroy(_ instance: Instance, workroom: UUID, in environment: Environment) async throws { await instance.connection.close() - var failure: Error? - do { try await environment.client.cancelGrant(instance.grantID) } catch { failure = error } + var failures: [String] = [] + var (grantLive, hostLive): (String?, HostID?) = (nil, nil) + do { try await environment.client.cancelGrant(instance.grantID) } catch { + grantLive = instance.grantID + failures.append("cancelling grant \(instance.grantID): \(error.localizedDescription)") + } await environment.agentBroker.release(workroom) - do { try await environment.driver.destroy(instance.host) } catch { failure = failure ?? error } - if let failure { throw failure } + do { try await environment.driver.destroy(instance.host) } catch { + hostLive = instance.host + failures.append("destroying the instance: \(error.localizedDescription)") + } + guard !failures.isEmpty else { return } + throw Failure.rollbackIncomplete( + cause: "Destroying the workroom failed.", host: hostLive, grantID: grantLive, + cleanup: failures) } // MARK: Plumbing diff --git a/macapp/WorkroomAppTests/Broker/BrokerSignInTests.swift b/macapp/WorkroomAppTests/Broker/BrokerSignInTests.swift index 911a2bc93..f7522d40f 100644 --- a/macapp/WorkroomAppTests/Broker/BrokerSignInTests.swift +++ b/macapp/WorkroomAppTests/Broker/BrokerSignInTests.swift @@ -361,6 +361,26 @@ final class AgentEnrolmentTests: XCTestCase { XCTAssertEqual(BrokerStub.requests.last?.request.httpMethod, "DELETE") } + /// A failed enrolment whose grant cannot be cancelled either names that grant, so the caller + /// can record it rather than lose it. + func testAFailedEnrolmentWhoseCancelFailsNamesTheLiveGrant() async throws { + BrokerStub.reset([grant, .init(status: 503, body: #"{"error":"github_unavailable"}"#)]) + let file = stdinFile() + defer { try? FileManager.default.removeItem(at: file) } + let driver = StubDriver(stdinFile: file, output: "error: git config failed\n", status: 1) + + do { + _ = try await AgentEnrolment.enrol( + client: client(), driver: driver, host: .remote(UUID()), agentBinary: "wr-agent", + workroomID: UUID(), repository: "o/r", agentBroker: broker.seam) + XCTFail("expected a failure") + } catch let live as AgentEnrolment.GrantStillLive { + XCTAssertEqual(live.grantID, "g1") + guard case BrokerError.agent(let detail) = live.cause else { return XCTFail("\(live.cause)") } + XCTAssertTrue(detail.contains("git config failed"), detail) + } + } + /// A cancelled enrolment still cancels its grant: the agent may have enrolled already. func testACancelledEnrolmentStillCancelsItsGrant() async throws { BrokerStub.reset([grant, .init(body: #"{"grant_id":"g1","state":"cancelled"}"#)]) diff --git a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift index 78c164dc8..029f39020 100644 --- a/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift @@ -159,11 +159,11 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { let script = directory.appendingPathComponent("runtime") try """ #!/bin/sh - step="$(cat \(PosixShell.quoted(failing.path)) 2>/dev/null)" - if [ "$1" = "$step" ]; then echo "injected failure" >&2; exit 1; fi - if [ "$1+" = "$step" ]; then - \(PosixShell.quoted(runtime.path)) "$@" >/dev/null; echo "injected failure" >&2; exit 1 - fi + steps=" $(cat \(PosixShell.quoted(failing.path)) 2>/dev/null) " + case "$steps" in *" $1 "*) echo "injected failure" >&2; exit 1 ;; esac + case "$steps" in *" $1+ "*) + \(PosixShell.quoted(runtime.path)) "$@" >/dev/null; echo "injected failure" >&2; exit 1 ;; + esac exec \(PosixShell.quoted(runtime.path)) "$@" """.write(to: script, atomically: true, encoding: .utf8) @@ -289,4 +289,25 @@ final class ContainerProvisioningIntegrationTests: XCTestCase { XCTAssertEqual(try leftovers("ps", runtime: runtime, label: label), []) XCTAssertEqual(try leftovers("images", runtime: runtime, label: label), []) } + + /// A derive that fails and cannot remove what it made says what is still there (#280 review), + /// rather than dropping the host from its registry with its container still running. + func testADeriveThatCannotRemoveItsContainerSaysWhatIsLeft() async throws { + let (script, failing) = try failingRuntime() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: try provisioning(runtime: script)) + let base = try await driver.create() + // The identity read fails, then removing the container fails too. + try "exec rm".write(to: failing, atomically: true, encoding: .utf8) + + do { + _ = try await driver.deriveFromBase(base) + XCTFail("a derive that failed succeeded") + } catch HostDriverError.leftBehind(let cause, let leftover) { + XCTAssertTrue(cause.contains("never minted"), cause) + XCTAssertTrue(leftover.contains { $0.hasPrefix("container workroom-") }, "\(leftover)") + } + try FileManager.default.removeItem(at: failing) + // tearDown removes what was left, by label. + } } diff --git a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift index a5e01ba12..06a349fb8 100644 --- a/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift +++ b/macapp/WorkroomAppTests/RemoteProvisioningIntegrationTests.swift @@ -509,15 +509,26 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { let base = try await build(environment) let forgotten = Revoked() + // Forgetting fails the first time: the box is gone, and a retry gets past the destroy it + // already did to forget the record. + do { + try await RemoteProvisioning.destroyBase( + base, in: environment, forget: { throw WorkroomCLIError.timedOut }) + XCTFail("a failed forget was swallowed") + } catch WorkroomCLIError.timedOut {} + XCTAssertEqual(try leftovers("ps", label: label), []) try await RemoteProvisioning.destroyBase( base, in: environment, forget: { forgotten.add("forgot") }) XCTAssertEqual(forgotten.all, ["forgot"]) - XCTAssertEqual(try leftovers("ps", label: label), []) - // A base the driver no longer knows keeps its record: nothing has shown it is gone. + // A base this driver never knew (an app relaunched since) keeps its record: nothing has shown + // it is gone. + let relaunched = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) do { try await RemoteProvisioning.destroyBase( - base, in: environment, forget: { forgotten.add("forgot again") }) + base, in: self.environment(fixture, driver: relaunched, revoked: Revoked()), + forget: { forgotten.add("forgot again") }) XCTFail("an unknown base was destroyed") } catch HostDriverError.unknownHost {} XCTAssertEqual(forgotten.all, ["forgot"]) @@ -540,10 +551,41 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { do { try await RemoteProvisioning.destroy(instance, workroom: workroom, in: environment) XCTFail("a failed grant cancel was swallowed") - } catch BrokerError.refused {} + } catch RemoteProvisioning.Failure.rollbackIncomplete(_, let host, let grant, let cleanup) { + XCTAssertNil(host, "the box is gone, but was reported live") + XCTAssertEqual(grant, "g1", "the grant still live was not named") + XCTAssertEqual(cleanup.count, 1, "\(cleanup)") + } XCTAssertEqual(try leftovers("ps", label: label).count, 1, "the instance's box was kept") } + /// Both halves of a workroom's teardown failing are both reported. + @MainActor + func testDestroyingAWorkroomReportsBothAFailedCancelAndAFailedRemoval() async throws { + let (fixture, failing) = try failingFixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let environment = environment(fixture, driver: driver, revoked: Revoked()) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + BrokerStub.reset([Self.grant]) + let workroom = UUID() + let instance = try await RemoteProvisioning.derive( + from: base, workroom: workroom, branch: "wr-both", in: environment) + BrokerStub.reset([.init(status: 503, body: #"{"error":"github_unavailable"}"#)]) + try "rm".write(to: failing, atomically: true, encoding: .utf8) + defer { try? FileManager.default.removeItem(at: failing) } + + do { + try await RemoteProvisioning.destroy(instance, workroom: workroom, in: environment) + XCTFail("a failed teardown was swallowed") + } catch RemoteProvisioning.Failure.rollbackIncomplete(_, let host, let grant, let cleanup) { + XCTAssertEqual(host, instance.host, "the box still up was not named") + XCTAssertEqual(grant, "g1", "the grant still live was not named") + XCTAssertEqual(cleanup.count, 2, "\(cleanup)") + } + } + /// A failure whose undoing fails too names what is still live (#252 review), rather than /// leaving a box holding an enrolled key and a live grant that nothing records. @MainActor @@ -573,4 +615,68 @@ final class RemoteProvisioningIntegrationTests: XCTestCase { } XCTAssertEqual(try leftovers("ps", label: label).count, 2, "the instance should still be up") } + + /// An enrolment whose own grant cancel fails hands that grant to the rollback's report rather + /// than losing it (#280 review). + @MainActor + func testAWorkroomWhoseEnrolmentAndGrantCancelFailNamesTheLiveGrant() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let base = try await build(environment(fixture, driver: driver, revoked: Revoked())) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + let containers = try leftovers("ps", label: label) + // Nothing listens there, so the agent's enrolment fails after the grant is made. + let failing = environment( + fixture, driver: driver, revoked: Revoked(), brokerURL: URL(string: "http://127.0.0.1:9")) + BrokerStub.reset([Self.grant, .init(status: 503, body: #"{"error":"github_unavailable"}"#)]) + + do { + _ = try await RemoteProvisioning.derive( + from: base, workroom: UUID(), branch: "wr-enrol", in: failing) + XCTFail("a workroom that could not enrol was made") + } catch RemoteProvisioning.Failure.rollbackIncomplete(_, let host, let grant, let cleanup) { + XCTAssertNil(host, "the box is gone, but was reported live") + XCTAssertEqual(grant, "g1", "the enrolment's live grant was lost") + XCTAssertEqual(cleanup.count, 1, "\(cleanup)") + } + XCTAssertEqual(try leftovers("ps", label: label), containers, "the instance was kept") + } + + /// A refresh brings the base's clone up to date with a fresh clone token, revoked after, and a + /// workroom derived after it starts from what the refresh fetched. + @MainActor + func testARefreshedBaseHandsItsWorkroomsWhatItFetched() async throws { + let fixture = try fixture() + let driver = ContainerHostDriver( + hosts: [:], directory: directory, provisioning: fixture.provisioning) + let revoked = Revoked() + let environment = environment(fixture, driver: driver, revoked: revoked) + let base = try await build(environment) + cleanups.append { try? await driver.destroy(.remote(base.host)) } + let host = HostID.remote(base.host) + let pushed = try await onHost( + driver, host, + "seed=$(mktemp -d) && git clone -q /srv/origin.git $seed" + + " && git -C $seed -c user.name=a -c user.email=a@b commit -q --allow-empty -m later" + + " && git -C $seed push -q origin HEAD && git -C $seed rev-parse HEAD") + XCTAssertEqual(pushed.status, 0, pushed.output) + + BrokerStub.reset([Self.cloneToken]) + try await RemoteProvisioning.refreshBase(base, in: environment) + + XCTAssertEqual(revoked.all, [Self.token, Self.token], "the refresh's token outlived it") + let fetched = try await onHost( + driver, host, "git -C \(Self.path) rev-parse refs/remotes/origin/main") + XCTAssertEqual(fetched.output, pushed.output, "the refresh did not fetch") + + // A refused token leaves the base as it was, with nothing to revoke. + BrokerStub.reset([.init(status: 403, body: #"{"error":"no_read_access"}"#)]) + do { + try await RemoteProvisioning.refreshBase(base, in: environment) + XCTFail("a refresh ran without a token") + } catch BrokerError.refused {} + XCTAssertEqual(revoked.all.count, 2) + XCTAssertEqual(try leftovers("ps", label: label).count, 1, "a refused refresh took the base") + } } From e5a474fe092427596c71e6528e9886e54630015e Mon Sep 17 00:00:00 2001 From: Joel Moss Date: Thu, 1 Oct 2026 22:23:50 +0100 Subject: [PATCH 9/9] fix: address PR review findings Greptile on #280: delete-project --with-workrooms re-checks before each workroom that the project has no host descriptor. A base recorded during the cascade now stops it before the next workroom. Before, it was caught only at the final RemoveProject, after every workroom had been torn down. --from-disk checks the same way before each teardown script. A descriptor written during a single workroom's teardown still gets past the checks. Closing that would mean holding the config lock across teardown scripts. TestDeleteProjectStopsWhenABaseIsRecordedMidCascade fails without the fix. Co-Authored-By: Claude Opus 5.5 --- cmd/delete_project.go | 39 ++++++++++++++++++++++++++++---------- cmd/delete_project_test.go | 31 ++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/cmd/delete_project.go b/cmd/delete_project.go index fc98b4f42..e3d0177d9 100644 --- a/cmd/delete_project.go +++ b/cmd/delete_project.go @@ -81,18 +81,9 @@ func runDeleteProject(svc *workroom.Service, jsonMode bool, confirm string, with // Refused in every mode, before anything is torn down. --from-disk's safety gate reasons // about local paths and --with-workrooms tears down locally; the config-only mode would drop // the only record of a remote machine. Remote deletion is its own path (#253). - projects, err := svc.Config.AllProjects() - if err != nil { + if err := refuseRemoteProject(svc.Config, canon); err != nil { return err } - if remote := projects[canon].RemoteWorkroomNames(); len(remote) > 0 { - return fmt.Errorf("%w: %s has remote workrooms: %s", errs.ErrRemoteWorkroom, canon, strings.Join(remote, ", ")) - } - // The project's own descriptor records its base machine (#252): the same record, at the - // project's level. - if projects[canon].Host != nil { - return fmt.Errorf("%w: %s has a remote host (its base machine)", errs.ErrRemoteWorkroom, canon) - } if fromDisk { // Guard: refuse obviously dangerous paths. @@ -134,6 +125,10 @@ func runDeleteProject(svc *workroom.Service, jsonMode bool, confirm string, with logWriter := newJSONLogWriter(logSink, "teardown") svc.ScriptLogWriter = logWriter for _, name := range names { + if err := refuseRemoteProject(svc.Config, canon); err != nil { + logWriter.Flush() + return err + } if err := svc.RunTeardown(canon, name); err != nil { logWriter.Flush() return err @@ -166,6 +161,14 @@ func runDeleteProject(svc *workroom.Service, jsonMode bool, confirm string, with logWriter := newJSONLogWriter(logSink, "teardown") svc.ScriptLogWriter = logWriter for _, name := range names { + // Again before each workroom: a descriptor written during the cascade (a base + // recorded) stops it here rather than after every workroom has gone. + // RemoveProject checks once more under its own lock; only a descriptor written + // during one workroom's teardown gets past these. + if err := refuseRemoteProject(svc.Config, canon); err != nil { + logWriter.Flush() + return err + } if err := svc.Delete(canon, name, name); err != nil { logWriter.Flush() return err @@ -260,3 +263,19 @@ func init() { deleteProjectCmd.Flags().BoolVar(&deleteProjectFromDisk, "from-disk", false, "App-only: runs teardowns, drops config, returns trash_paths; the caller (macOS app) moves those dirs to the Trash") rootCmd.AddCommand(deleteProjectCmd) } + +// refuseRemoteProject refuses a project with remote workrooms, or with its own descriptor, which +// records its base machine (#252): either is the only record of a remote machine. +func refuseRemoteProject(cfg *config.Config, canon string) error { + projects, err := cfg.AllProjects() + if err != nil { + return err + } + if remote := projects[canon].RemoteWorkroomNames(); len(remote) > 0 { + return fmt.Errorf("%w: %s has remote workrooms: %s", errs.ErrRemoteWorkroom, canon, strings.Join(remote, ", ")) + } + if projects[canon].Host != nil { + return fmt.Errorf("%w: %s has a remote host (its base machine)", errs.ErrRemoteWorkroom, canon) + } + return nil +} diff --git a/cmd/delete_project_test.go b/cmd/delete_project_test.go index fd814c6a1..75eea6f99 100644 --- a/cmd/delete_project_test.go +++ b/cmd/delete_project_test.go @@ -24,6 +24,7 @@ type fakeVCS struct { deleteCalls []string failOn string // vcsName to fail on (e.g. "workroom/bravo"); "" never fails list []string // names ListWorkrooms returns (Service.Delete's existence check) + onDelete func() // runs after each Delete, to change config mid-cascade } func (f *fakeVCS) Type() vcs.Type { return vcs.TypeGit } @@ -32,6 +33,9 @@ func (f *fakeVCS) Create(_, _, _ string) (string, error) { return "", nil } func (f *fakeVCS) ListWorkrooms(_ string) ([]string, error) { return f.list, nil } func (f *fakeVCS) Delete(_, vcsName, _ string) (string, error) { f.deleteCalls = append(f.deleteCalls, vcsName) + if f.onDelete != nil { + f.onDelete() + } if f.failOn != "" && vcsName == f.failOn { return "", errors.New("boom") } @@ -465,6 +469,33 @@ func TestDeleteProjectRefusesAProjectWithABase(t *testing.T) { } } +// TestDeleteProjectStopsWhenABaseIsRecordedMidCascade: a host descriptor written while +// --with-workrooms tears down (a base recorded) stops the cascade before the next workroom, not +// only at the final config write, so no more local workrooms go than had already gone. +func TestDeleteProjectStopsWhenABaseIsRecordedMidCascade(t *testing.T) { + fake := &fakeVCS{list: []string{"alpha", "bravo", "charlie"}} + svc, cfg := newTestSvc(t, fake) + proj := t.TempDir() + canon, _ := config.CanonicalPath(proj) + for _, name := range []string{"alpha", "bravo", "charlie"} { + cfg.AddWorkroom(canon, name, filepath.Join(t.TempDir(), name), "git") + } + fake.onDelete = func() { + fake.onDelete = nil + if err := cfg.SetHost(canon, "", map[string]any{"base": map[string]any{"host": "h1"}}); err != nil { + t.Fatal(err) + } + } + + err := runDeleteProject(svc, true, canon, true, false, []string{proj}, &bytes.Buffer{}, &bytes.Buffer{}) + if !errors.Is(err, errs.ErrRemoteWorkroom) { + t.Fatalf("expected ErrRemoteWorkroom, got %v", err) + } + if len(fake.deleteCalls) != 1 { + t.Fatalf("the cascade went on after the base was recorded: %v", fake.deleteCalls) + } +} + // TestDeleteProjectRefusesRemoteWorkrooms: every mode refuses a project with remote workrooms, // naming them, before tearing anything down or touching config. func TestDeleteProjectRefusesRemoteWorkrooms(t *testing.T) {