From 0ba7f963d727812ca60fde8af3fa53b25e14fe84 Mon Sep 17 00:00:00 2001 From: MasterYoav Date: Fri, 18 Sep 2026 10:34:45 +0300 Subject: [PATCH] Uninstall with Docker quit removed nothing and said it removed everything. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every removal ran against no daemon inside a try?, then the keys and preferences were wiped and the screen announced success over every volume still on disk. It now wakes the runtime first, measures what is left, and on failure keeps the keys so the same button works again. The fake driver removed volumes with no daemon, which real Docker never does — that is why no test could see this. Co-Authored-By: Claude Opus 5 (1M context) --- apps/mac/Sources/XBotCore/AppState.swift | 9 +++- apps/mac/Sources/XBotRuntime/FakeDriver.swift | 7 ++- .../XBotRuntime/RuntimeController.swift | 13 +++++- .../XBotUI/Settings/SettingsRootView.swift | 17 ++++++- .../Tests/XBotRuntimeTests/RuntimeTests.swift | 46 +++++++++++++++++++ docs/11-packaging-and-updates.md | 6 +++ 6 files changed, 92 insertions(+), 6 deletions(-) diff --git a/apps/mac/Sources/XBotCore/AppState.swift b/apps/mac/Sources/XBotCore/AppState.swift index 17f70cf..4b8ccc2 100644 --- a/apps/mac/Sources/XBotCore/AppState.swift +++ b/apps/mac/Sources/XBotCore/AppState.swift @@ -912,11 +912,15 @@ public final class AppState { /// /// Returns when the data is gone. Moving the app itself to the Trash is the person's to do, /// and the screen says so. - public func uninstall() async { + /// False when the container runtime could not be reached. Then nothing is removed — not the + /// keys, not the preferences — so the person can start Docker and try again from the same + /// place, rather than be left with an app that has forgotten everything except the gigabytes. + @discardableResult + public func uninstall() async -> Bool { isEngineBusy = true defer { isEngineBusy = false } - await runtime?.uninstall() + if let runtime, await !runtime.uninstall() { return false } // Every key this app has ever written. Listed rather than enumerated, because a wildcard // sweep of the login keychain is not something this app should ever perform. @@ -958,6 +962,7 @@ public final class AppState { questionsForYou = [:] engineHealth = nil engineBaseURL = nil + return true } public static var appVersion: String { diff --git a/apps/mac/Sources/XBotRuntime/FakeDriver.swift b/apps/mac/Sources/XBotRuntime/FakeDriver.swift index f881a14..4135afa 100644 --- a/apps/mac/Sources/XBotRuntime/FakeDriver.swift +++ b/apps/mac/Sources/XBotRuntime/FakeDriver.swift @@ -122,7 +122,12 @@ public actor FakeDriver: ContainerDriver { public func createVolume(_ name: String) async throws { volumes.insert(name) } public func volumeExists(_ name: String) async -> Bool { volumes.contains(name) } - public func removeVolume(_ name: String) async throws { volumes.remove(name) } + /// Refused while the daemon is down, as the real one is: with no daemon there is nobody to + /// delete anything, and a fake that said otherwise hid exactly that from the uninstall tests. + public func removeVolume(_ name: String) async throws { + guard case .ready = await probe() else { throw RuntimeError.daemonUnavailable } + volumes.remove(name) + } /// Records the exec and writes a stand-in file, so a caller that depends on the file existing /// is exercised rather than only the call being counted. diff --git a/apps/mac/Sources/XBotRuntime/RuntimeController.swift b/apps/mac/Sources/XBotRuntime/RuntimeController.swift index 579f71d..ee497ed 100644 --- a/apps/mac/Sources/XBotRuntime/RuntimeController.swift +++ b/apps/mac/Sources/XBotRuntime/RuntimeController.swift @@ -590,7 +590,13 @@ public actor RuntimeController { /// Every step tolerates work already done. An uninstall that fails halfway and cannot be run /// again leaves exactly the orphaned state it exists to prevent, so a missing container or an /// already-deleted volume is not an error. - public func uninstall() async { + /// Whether the engine's data is gone. False when the runtime could not be reached, in which case + /// nothing was removed and the caller must not report otherwise. + @discardableResult + public func uninstall() async -> Bool { + // Wake the runtime first. With no daemon every removal below fails inside a `try?`, and the + // volumes — the conversations, agents and browser logins — stay exactly where they were. + guard await ensureRuntimeReady() else { return false } await stop() if let handle = await driver.containerNamed(Self.engineContainerName) { try? await driver.remove(handle) @@ -604,6 +610,11 @@ public actor RuntimeController { // Back to stopped, not notDetected: the runtime is still installed and still working — it // is only xBot's own data that is gone. state = .stopped + // Measured rather than assumed: a volume that is still there is data that is still there. + for volume in [Self.dataVolume, Self.workspaceVolume, Self.profilesVolume] { + if await driver.volumeExists(volume) { return false } + } + return true } /// What the running engine is using. Nil when it is not running or Docker would not say. diff --git a/apps/mac/Sources/XBotUI/Settings/SettingsRootView.swift b/apps/mac/Sources/XBotUI/Settings/SettingsRootView.swift index f96ecad..a78b056 100644 --- a/apps/mac/Sources/XBotUI/Settings/SettingsRootView.swift +++ b/apps/mac/Sources/XBotUI/Settings/SettingsRootView.swift @@ -9,6 +9,7 @@ public struct AdvancedSettingsView: View { @State private var isConfirmingUninstall = false @State private var hasUninstalled = false + @State private var uninstallFailed = false public var body: some View { Form { @@ -79,6 +80,17 @@ public struct AdvancedSettingsView: View { ) .foregroundStyle(Palette.textSecondary) } else { + if uninstallFailed { + // Said, not skipped: the alternative was announcing success over every + // volume still on disk. Keys and preferences are kept, so a retry works. + Text( + String( + localized: + "Your data couldn't be removed, because Docker didn't respond. Make sure Docker Desktop or Colima is running, then try again." + ) + ) + .foregroundStyle(Palette.textSecondary) + } Button(String(localized: "Remove all xBot data…"), role: .destructive) { isConfirmingUninstall = true } @@ -107,8 +119,9 @@ public struct AdvancedSettingsView: View { ) { Button(String(localized: "Remove everything"), role: .destructive) { Task { - await state.uninstall() - hasUninstalled = true + let removed = await state.uninstall() + hasUninstalled = removed + uninstallFailed = !removed } } Button(String(localized: "Cancel"), role: .cancel) {} diff --git a/apps/mac/Tests/XBotRuntimeTests/RuntimeTests.swift b/apps/mac/Tests/XBotRuntimeTests/RuntimeTests.swift index 8a5a9a9..641f19a 100644 --- a/apps/mac/Tests/XBotRuntimeTests/RuntimeTests.swift +++ b/apps/mac/Tests/XBotRuntimeTests/RuntimeTests.swift @@ -823,6 +823,52 @@ extension RuntimeControllerTests { #expect(await controller.state == .stopped) } + /** + Uninstall with the runtime stopped wakes it first. + + Docker Desktop quit, or the Colima VM paused — ordinary on a Mac, and likely on the day somebody + decides to remove an app. Every removal used to run against no daemon, fail inside a `try?`, + and leave every volume where it was while the screen said everything had been removed. + */ + @Test func uninstallWakesAStoppedRuntimeBeforeRemovingAnything() async { + let driver = FakeDriver() + let controller = RuntimeController( + driver: driver, + image: ImageReference(repository: "xbot/engine", tag: "1"), + health: { _ in EngineHealth(engineVersion: "0.0.5", schemaVersion: "0000") }, + ports: isolatedPortStore(), + dumpURL: isolatedDumpURL() + ) + await controller.start(environment: environment) + await driver.stopDaemon() + + let removed = await controller.uninstall() + + #expect(removed) + #expect(await driver.daemonStartRequested) + #expect(await driver.remainingVolumes.isEmpty) + } + + /// A runtime that will not come up means nothing was removed, and uninstall has to say so + /// rather than let the app wipe its keys and announce success over volumes it never touched. + @Test func uninstallThatCannotReachTheRuntimeSaysSo() async { + let driver = FakeDriver(script: .init(daemonStartSucceeds: false)) + let controller = RuntimeController( + driver: driver, + image: ImageReference(repository: "xbot/engine", tag: "1"), + health: { _ in EngineHealth(engineVersion: "0.0.5", schemaVersion: "0000") }, + ports: isolatedPortStore(), + dumpURL: isolatedDumpURL() + ) + await controller.start(environment: environment) + await driver.stopDaemon() + + let removed = await controller.uninstall() + + #expect(!removed) + #expect(!(await driver.remainingVolumes.isEmpty)) + } + /// The pre-upgrade dump is the database in plain SQL — agents, settings, everything the volume /// held. It lives outside the volume on purpose, so removing the volumes never touched it, and /// the screen said "Everything xBot stored has been removed" while a copy sat in Application diff --git a/docs/11-packaging-and-updates.md b/docs/11-packaging-and-updates.md index a11346e..72ebe11 100644 --- a/docs/11-packaging-and-updates.md +++ b/docs/11-packaging-and-updates.md @@ -312,6 +312,12 @@ is a bad citizen with a reputation problem. **Does not remove the container runtime**, because the user may have installed it for something else. It says so. +**It wakes the runtime first**, because nothing can be deleted without a daemon, and on the day +somebody removes an app Docker Desktop may well be quit. If the runtime will not come up, or a volume +is still there afterwards, uninstall reports failure and removes **nothing else** — the keys and +preferences stay, so trying again from the same button works. It used to run every removal against +no daemon inside a `try?` and then say everything was gone. + **Also ship a standalone uninstaller script** in the DMG for the user who already dragged the app to the Trash and then found the volumes. It is `Uninstall xBot.command`, copied from `scripts/uninstall-xbot.command`, and it mirrors `AppState.uninstall()` step for step.