From 3665c3f5c489c14c6f1225b8bf9b50179b650028 Mon Sep 17 00:00:00 2001 From: kfattic <6775519+kfattic@users.noreply.github.com> Date: Wed, 16 Sep 2026 21:24:46 -0500 Subject: [PATCH] Delete Printer: a queue that's already gone is a successful delete Judge the delete by the read-back (is the queue gone?) instead of lpadmin's exit status. Deleting a queue that was removed elsewhere before the sidebar refreshed made lpadmin fail with 'The printer or class does not exist', and the app showed a failure alert although the queue was gone. Now the status bar shows 'Deleted ' and os_log notes it was already deleted. Unit tests for all four outcomes, plus an opt-in live test (CUPSKIT_LIVE_DELETE=1) on a name that never existed. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- Sources/CUPSAdminApp/PrinterStore.swift | 2 +- Sources/CupsKit/QueueActions.swift | 19 +++++++--- Tests/CupsKitTests/QueueActionsTests.swift | 44 +++++++++++++++++++++- 4 files changed, 59 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9f75512..0f2fde8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,7 +68,7 @@ swift build # debug build of everything verifies both BOM ownership entries. The app is notarized and stapled before packaging; the PKG after. - The app icon master is `Icon/AppIcon-1024.png`; `build.sh` generates `AppIcon.icns` from it. Don't redraw it. - Live tests are opt-in and write to queues: `CUPSKIT_LIVE_QUEUE=`, `CUPSKIT_LIVE_JOBS=,`, - `CUPSKIT_LIVE_QUICK=,`. Use throwaway queues (e.g. `lpd://127.0.0.1/…`, paused) and + `CUPSKIT_LIVE_QUICK=,`, `CUPSKIT_LIVE_DELETE=1` (deletes a queue name that never existed; writes nothing). Use throwaway queues (e.g. `lpd://127.0.0.1/…`, paused) and delete them afterwards; the live tests restore what they change. Offline tests use vendor PPDs from `CUPSKIT_PPD_DIRS` (colon-separated, e.g. Canon/HP PPDs extracted with `pkgutil --expand-full`, not installed) and `/Library/Printers/PPDs`, and skip when they aren't there. diff --git a/Sources/CUPSAdminApp/PrinterStore.swift b/Sources/CUPSAdminApp/PrinterStore.swift index d862968..8a75e1a 100644 --- a/Sources/CUPSAdminApp/PrinterStore.swift +++ b/Sources/CUPSAdminApp/PrinterStore.swift @@ -251,7 +251,7 @@ final class PrinterStore { deleted = outcome.succeeded if deleted { report("Deleted \(queue) in \(elapsedText(since: started))") - log.info("deleted \(queue, privacy: .public)") + log.info("deleted \(queue, privacy: .public)\(outcome.message.map { " (\($0))" } ?? "", privacy: .public)") } else { report("ERROR: delete \(queue): \(outcome.message ?? "failed")") fail("Couldn’t delete \(queue)", "\(outcome.message ?? "failed")\n\n\(outcome.command)") diff --git a/Sources/CupsKit/QueueActions.swift b/Sources/CupsKit/QueueActions.swift index d7bc80d..224bf9a 100644 --- a/Sources/CupsKit/QueueActions.swift +++ b/Sources/CupsKit/QueueActions.swift @@ -222,12 +222,21 @@ extension QueueActions { /// `lpadmin -x queue`, verified by the queue being gone. public static func delete(queue: String, client: CupsClient) async throws -> ActionOutcome { let result = try await Task.detached { try CupsTools.delete(queue: queue) }.value - guard result.succeeded else { - return ActionOutcome(succeeded: false, command: result.commandLine, message: result.message.isEmpty ? "exited \(result.status)" : result.message) + return deleteOutcome(queue: queue, result: result, stillExists: try await client.queueExists(queue)) + } + + /// Judged by the read-back, not lpadmin's exit status: the goal is that the queue is gone. A queue that + /// was already deleted (by another app, lpadmin, or a row the sidebar hadn't refreshed yet) makes + /// lpadmin fail with "The printer or class does not exist" — that's still a successful delete. + /// `message` is set on success only in that case ("already deleted"). + static func deleteOutcome(queue: String, result: ToolResult, stillExists: Bool) -> ActionOutcome { + if stillExists { + let message = result.succeeded ? "Not applied: \(queue) still exists" + : result.message.isEmpty ? "exited \(result.status)" : result.message + return ActionOutcome(succeeded: false, command: result.commandLine, message: message) } - let stillThere = try await client.queueExists(queue) - return ActionOutcome(succeeded: !stillThere, command: result.commandLine, - message: stillThere ? "Not applied: \(queue) still exists" : nil) + return ActionOutcome(succeeded: true, command: result.commandLine, + message: result.succeeded ? nil : "\(queue) was already deleted") } /// Sends the CUPS test page (or a generated one-page PDF when it's missing). Returns the job id when verified. diff --git a/Tests/CupsKitTests/QueueActionsTests.swift b/Tests/CupsKitTests/QueueActionsTests.swift index 8fbbab6..da8b669 100644 --- a/Tests/CupsKitTests/QueueActionsTests.swift +++ b/Tests/CupsKitTests/QueueActionsTests.swift @@ -1,4 +1,4 @@ -import CupsKit +@testable import CupsKit import Foundation import Testing @@ -57,3 +57,45 @@ struct LiveQueueActions { } } } + +/// Delete is judged by whether the queue is gone afterwards, not by lpadmin's exit status. +@Suite struct DeleteReadBack { + private func result(_ status: Int32, _ stderr: String = "") -> ToolResult { + ToolResult(executable: CupsTools.lpadminPath, arguments: ["-x", "Old_Queue"], status: status, + standardOutput: Data(), standardError: Data(stderr.utf8)) + } + + @Test func deletedNormally() { + let outcome = QueueActions.deleteOutcome(queue: "Old_Queue", result: result(0), stillExists: false) + #expect(outcome == ActionOutcome(succeeded: true, command: "/usr/sbin/lpadmin -x Old_Queue", message: nil)) + } + + /// The reported bug: the queue was removed elsewhere before Delete Printer ran, lpadmin said + /// "The printer or class does not exist", and the app showed a failure alert. + @Test func alreadyGoneIsASuccess() { + let outcome = QueueActions.deleteOutcome(queue: "Old_Queue", + result: result(1, "lpadmin: The printer or class does not exist."), + stillExists: false) + #expect(outcome.succeeded) + #expect(outcome.message == "Old_Queue was already deleted") + } + + @Test func failureWhileQueueRemainsShowsToolOutput() { + let outcome = QueueActions.deleteOutcome(queue: "Old_Queue", result: result(1, "lpadmin: Forbidden"), stillExists: true) + #expect(!outcome.succeeded && outcome.message == "lpadmin: Forbidden") + } + + @Test func exitZeroButStillThereIsNotApplied() { + let outcome = QueueActions.deleteOutcome(queue: "Old_Queue", result: result(0), stillExists: true) + #expect(!outcome.succeeded && outcome.message == "Not applied: Old_Queue still exists") + } + + /// Opt-in (CUPSKIT_LIVE_DELETE=1): the real lpadmin and cupsd on a name that never existed. Writes nothing. + @Test(.enabled(if: ProcessInfo.processInfo.environment["CUPSKIT_LIVE_DELETE"] != nil)) + func liveDeleteOfMissingQueue() async throws { + let name = "cupsadmin_missing_\(UInt32.random(in: 0 ... .max))" + let outcome = try await QueueActions.delete(queue: name, client: CupsClient()) + #expect(outcome.succeeded, "\(outcome)") + #expect(outcome.message == "\(name) was already deleted") + } +}