Follow-ups from reviewing #17 and #24: CI, a queue perf fix, and issue #19

.github/workflows/release.yml

@@ -20,12 +20,21 @@ jobs:
     steps:
       - uses: actions/checkout@v5
 
+      # The whole point of building here is that a release binary can be tied
+      # to the commit AND the toolchain it claims. Falling back to whatever the
+      # runner image ships would quietly break that, so a missing pin fails the
+      # build instead: bump the version here deliberately when it is retired.
       - name: Select Xcode
         run: |
-          if [ -d /Applications/Xcode_16.4.app ]; then
-            echo "DEVELOPER_DIR=/Applications/Xcode_16.4.app/Contents/Developer" >> "$GITHUB_ENV"
+          if [ ! -d "$XCODE_APP" ]; then
+            echo "::error::$XCODE_APP is not on this runner. Available:"
+            ls -d /Applications/Xcode*.app >&2 || true
+            exit 1
           fi
-          xcodebuild -version
+          echo "DEVELOPER_DIR=$XCODE_APP/Contents/Developer" >> "$GITHUB_ENV"
+          DEVELOPER_DIR="$XCODE_APP/Contents/Developer" xcodebuild -version
+        env:
+          XCODE_APP: /Applications/Xcode_16.4.app
 
       - name: Install xcodegen
         run: brew install xcodegen

.github/workflows/tests.yml

@@ -0,0 +1,77 @@
+name: tests
+
+on:
+  pull_request:
+  push:
+    branches: [main]
+
+concurrency:
+  group: tests-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  test:
+    name: xcodebuild test
+    runs-on: macos-15
+    timeout-minutes: 30
+    steps:
+      - uses: actions/checkout@v5
+
+      # The toolchain the releases are built with. A missing pin fails the run
+      # rather than testing on whatever the image happens to ship; see
+      # release.yml, which pins the same way for the same reason.
+      - name: Select Xcode
+        run: |
+          if [ ! -d "$XCODE_APP" ]; then
+            echo "::error::$XCODE_APP is not on this runner. Available:"
+            ls -d /Applications/Xcode*.app >&2 || true
+            exit 1
+          fi
+          echo "DEVELOPER_DIR=$XCODE_APP/Contents/Developer" >> "$GITHUB_ENV"
+          DEVELOPER_DIR="$XCODE_APP/Contents/Developer" xcodebuild -version
+        env:
+          XCODE_APP: /Applications/Xcode_16.4.app
+
+      - name: Install xcodegen
+        run: brew install xcodegen
+
+      - name: Generate the project
+        run: xcodegen generate
+
+      # Runner images rename their simulators between releases, so the device
+      # is read from what is installed instead of naming one that may be gone
+      # after an image bump. Only iOS runtimes list an iPhone.
+      - name: Pick a simulator
+        run: |
+          name="$(xcrun simctl list devices available \
+            | sed -n 's/^ *\(iPhone [^(]*\) (.*/\1/p' \
+            | sed 's/ *$//' | head -1)"
+          if [ -z "$name" ]; then
+            echo "::error::No iPhone simulator is available on this runner."
+            xcrun simctl list devices available >&2
+            exit 1
+          fi
+          echo "Testing on $name"
+          echo "SIMULATOR_NAME=$name" >> "$GITHUB_ENV"
+
+      - name: Test
+        run: |
+          set -o pipefail
+          xcodebuild test \
+            -project PhotosBackup.xcodeproj \
+            -scheme PhotosBackup \
+            -destination "platform=iOS Simulator,name=$SIMULATOR_NAME" \
+            2>&1 | tee build.log \
+            | grep -E "Test Suite .* (passed|failed)|Test Case .* failed|error:|\*\* TEST" || true
+          # The grep above swallows xcodebuild's status, and a run that never
+          # launches prints no summary at all, so the verdict is read back from
+          # the log itself.
+          grep -q '\*\* TEST SUCCEEDED \*\*' build.log
+
+      - name: Upload the log if it failed
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: xcodebuild-log
+          path: build.log
+          if-no-files-found: ignore

App/Sources/AutomaticBackupCoordinator.swift

@@ -116,8 +116,7 @@ final class AutomaticBackupCoordinator: ObservableObject {
         self.network = network
         self.libraryChanges = libraryChanges ?? PhotoLibraryChangeTracker()
         queue.followUpSources = { [weak self] source in
-            guard let self else { return [] }
-            return self.motionFollowUps(after: source) + self.editBaseFollowUps(after: source)
+            self?.followUps(after: source) ?? []
         }
         #if compiler(>=6.2)
         if #available(iOS 26.0, *) { setUpContinuedBackup() }
@@ -200,22 +199,29 @@ final class AutomaticBackupCoordinator: ObservableObject {
         Task { await queueLivePhotoMotion() }
     }
 
-    /// A Live Photo's motion is committed onto its still, so it is queued once
-    /// the still is in the account: right after the still's row finishes.
-    private func motionFollowUps(after source: MediaSource) -> [MediaSource] {
-        guard preferences.backUpLivePhotoMotion, case .asset(let identifier) = source,
-              let asset = PHAsset.fetchAssets(withLocalIdentifiers: [identifier], options: nil).firstObject,
-              asset.mediaSubtypes.contains(.photoLive) else { return [] }
-        return [.livePhotoMotion(localIdentifier: identifier)]
-    }
-
-    /// A photo edited in the Google Photos app also needs the version that edit
-    /// was applied on, which is what that app checks. Queued after the photo.
-    private func editBaseFollowUps(after source: MediaSource) -> [MediaSource] {
+    /// What else has to back up once this row's source is in the account: a
+    /// Live Photo's motion, which is committed onto the still, and the version
+    /// a Google Photos edit was applied on, which that app checks.
+    ///
+    /// Both answers come from one library fetch. This runs on the main actor
+    /// as every row finishes, so a full-library backup pays for it once per
+    /// item, and asking for the asset twice doubled that for no gain.
+    private func followUps(after source: MediaSource) -> [MediaSource] {
+        // A motion or edit-base row is itself a follow-up and has none of its
+        // own. Checked before the fetch, which is the expensive part.
         guard case .asset(let identifier) = source,
-              let asset = PHAsset.fetchAssets(withLocalIdentifiers: [identifier], options: nil).firstObject,
-              MediaExporter.hasEditBase(asset) else { return [] }
-        return [.editBase(localIdentifier: identifier)]
+              let asset = PHAsset.fetchAssets(withLocalIdentifiers: [identifier], options: nil).firstObject
+        else { return [] }
+        var followUps: [MediaSource] = []
+        if preferences.backUpLivePhotoMotion, asset.mediaSubtypes.contains(.photoLive) {
+            followUps.append(.livePhotoMotion(localIdentifier: identifier))
+        }
+        // Reads the asset's resources, so it is left until last: it only runs
+        // for an asset that carries an adjustment.
+        if MediaExporter.hasEditBase(asset) {
+            followUps.append(.editBase(localIdentifier: identifier))
+        }
+        return followUps
     }
 
     /// Once per account, queue the edit base of every photo already remembered

App/Sources/UploadQueue.swift

@@ -62,7 +62,9 @@ struct UploadItem: Identifiable, Equatable, Sendable {
             case .finalizing: return "Finishing"
             case .alreadyBackedUp: return "Already backed up"
             case .done: return "Backed up"
-            case .cancelled: return "Cancelled"
+            // Only ever the user's own doing now, so it says so: "Cancelled"
+            // on its own read as a failure the app would not explain.
+            case .cancelled: return "Stopped by you"
             case .failed(let reason, _): return reason
             }
         }
@@ -179,9 +181,9 @@ final class UploadQueue: ObservableObject {
     /// in memory only, and read by Diagnostics.
     @Published private(set) var recentFailures: [UploadFailure] = []
     @Published private(set) var failureCount = 0
-    /// Rows that reached a finished state in this session. It only grows, so
-    /// progress measured from it stays monotonic when Clear Finished takes
-    /// rows away.
+    /// Rows that stopped needing work in this session, whether they reached a
+    /// finished state or left the queue resolved. It only grows, so progress
+    /// measured from it stays monotonic when Clear Finished takes rows away.
     private(set) var settledRowCount = 0
     private static let recentFailureLimit = 25
 
@@ -1035,7 +1037,27 @@ final class UploadQueue: ObservableObject {
             // in-flight work for requeue. A later pump restarts it unchanged.
             if requeueCancelled.remove(id) != nil { setState(.queued, at: index); persist(); return }
             if error is CancellationError {
-                setState(.cancelled, at: index); cleanCheckpoint(for: index); persist(); return
+                // Nothing the user did: `cancel` and the requeue paths above
+                // both claim their rows before this, so reaching here means the
+                // work was interrupted on its own — a staging read dropped as
+                // the app was suspended, say. Marking that `.cancelled` left a
+                // bare "Cancelled" row that explained nothing, sat in the list
+                // for good, and reappeared beside the same photo once a later
+                // scan backed it up (issue #19). Treat it as the interruption
+                // it is: retry it, and if it keeps happening say so in words.
+                let attempt = items[index].attempts
+                if attempt < maxAttempts {
+                    setState(.waitingToRetry(attempt: attempt), at: index)
+                    persist()
+                    scheduleRetry(id, after: min(30, pow(2, Double(attempt))))
+                } else {
+                    let interrupted = "Backing this item up kept being interrupted before it finished."
+                    setState(.failed(reason: interrupted, retryable: true), at: index)
+                    recordFailure(name: items[index].name, reason: interrupted, status: nil, stage: stage)
+                    cleanCheckpoint(for: index)
+                    persist()
+                }
+                return
             }
             if error as? MediaExporter.Failure == .iCloudDownloadRequired {
                 items[index].attempts = max(0, items[index].attempts - 1)
@@ -1053,6 +1075,10 @@ final class UploadQueue: ObservableObject {
                 // same photos. Nothing is left to back up and nothing needs the
                 // user: drop the row rather than fail it.
                 cleanCheckpoint(for: index)
+                // The row is settled, not merely gone: a continued backup
+                // measures its total as settled plus unfinished, and dropping
+                // one without counting it walked that total backwards.
+                settledRowCount += 1
                 items.remove(at: index)
                 rebuildDerivedState()
                 persist()

Tests/PhotosBackupTests/UploadQueueTests.swift

@@ -133,7 +133,9 @@ final class UploadQueueTests: XCTestCase {
         let queue = UploadQueue(worker: script.worker(), maxConcurrent: 1, maxAttempts: 1,
                                 sleeper: { await gate.sleep($0) })
         queue.enqueue(sources(2))
-        await settle(queue) { queue.rateLimitPauseReason != nil }
+        // The pause reason is set before the waiting task starts, so waiting on
+        // it alone raced the sleeper and saw no delay yet on a loaded machine.
+        await settle(queue) { queue.rateLimitPauseReason != nil && !gate.requested.isEmpty }
         XCTAssertEqual(queue.items.map(\.state), [.queued, .queued])
         XCTAssertEqual(queue.pauseReason, queue.rateLimitPauseReason)
         XCTAssertEqual(gate.requested, [UploadQueue.rateLimitBaseDelay])
@@ -233,11 +235,54 @@ final class UploadQueueTests: XCTestCase {
         XCTAssertEqual(queue.items.count, 1, "the deleted photo's row is gone")
         XCTAssertEqual(queue.items.first?.state, .done)
         XCTAssertEqual(queue.failedCount, 0)
-        XCTAssertEqual(queue.settledRowCount, 1)
+        // The drop settles the row. A continued backup reports its total as
+        // settled plus unfinished, so leaving the dropped row out of both
+        // walked that total backwards mid-run.
+        XCTAssertEqual(queue.settledRowCount, 2)
         XCTAssertEqual(script.calls, 2)
         assertAggregatesMatchRows(queue)
     }
 
+    /// A `CancellationError` no `cancel` or requeue claimed is an interruption,
+    /// not a decision — the app suspended mid-export, say. It used to land in a
+    /// terminal `.cancelled` row labelled only "Cancelled", which explained
+    /// nothing, stayed in the list for good, and showed up beside the same
+    /// photo once a later scan backed it up (issue #19).
+    func testAnUnclaimedCancellationRetriesThenSaysWhatHappened() async {
+        let script = WorkerScript([], fallback: .fail(CancellationError()))
+        let queue = makeQueue(script, maxConcurrent: 1, maxAttempts: 3)
+        queue.enqueue(oneSource)
+        await settle(queue) { queue.items.first?.state.isFinished == true }
+        XCTAssertEqual(queue.items.first?.state,
+                       .failed(reason: "Backing this item up kept being interrupted before it finished.",
+                               retryable: true))
+        XCTAssertEqual(script.calls, 3, "it is retried rather than given up on at once")
+        assertAggregatesMatchRows(queue)
+    }
+
+    /// The common case: the interruption passes and the row backs up by itself.
+    func testAnUnclaimedCancellationRecoversOnTheNextAttempt() async {
+        let script = WorkerScript([.fail(CancellationError())])
+        let queue = makeQueue(script, maxConcurrent: 1)
+        queue.enqueue(oneSource)
+        await settle(queue) { queue.items.first?.state.isFinished == true }
+        XCTAssertEqual(queue.items.first?.state, .done)
+        XCTAssertEqual(queue.failedCount, 0)
+        assertAggregatesMatchRows(queue)
+    }
+
+    /// `.cancelled` now means one thing only, so the label may say so.
+    func testOnlyTheUsersOwnCancellationReadsAsStopped() async {
+        let script = WorkerScript([], fallback: .block)
+        let queue = makeQueue(script, maxConcurrent: 1)
+        let ids = queue.enqueue(oneSource)
+        await settle(queue) { queue.items.first?.state == .uploading(fraction: 0.5) }
+        queue.cancel(ids[0])
+        await settle(queue) { queue.items.first?.state.isFinished == true }
+        XCTAssertEqual(queue.items.first?.state, .cancelled)
+        XCTAssertEqual(queue.items.first?.state.label, "Stopped by you")
+    }
+
     func testCredentialRejectionHaltsTheQueueAndLeavesWorkRequeued() async {
         let rejection = GPMCError(kind: .credentialRejected, message: "Connect the account again.")
         let script = WorkerScript([.fail(rejection)], fallback: .fail(rejection))