From caede329e3f587abb0438d8b737b7609cd5c14c1 Mon Sep 17 00:00:00 2001 From: g8row Date: Sun, 20 Sep 2026 18:31:17 +0300 Subject: [PATCH 1/4] Say why an item stopped instead of labelling it "Cancelled" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CancellationError that neither `cancel` nor a requeue claimed is an interruption, not a decision — a staging read dropped as iOS suspended the app, say. It landed in a terminal `.cancelled` row labelled only "Cancelled": no reason, no way back, and still sitting there when a later scan backed the same photo up and added an "Already backed up" row beside it. That pair is what #19 reported. Such a row now retries like any other interruption and, if it keeps happening, fails in words the user can act on. `.cancelled` is left meaning one thing only — the user stopped it — so the label can say so. Also count a row dropped because its photo left the library as settled. A continued backup reports its total as settled plus unfinished, so dropping one without counting it walked the Live Activity's total backwards mid-run. The test that pinned the old count is updated with the reason. Fixes #19 Co-Authored-By: Claude Opus 5 --- App/Sources/UploadQueue.swift | 36 ++++++++++++--- .../PhotosBackupTests/UploadQueueTests.swift | 45 ++++++++++++++++++- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/App/Sources/UploadQueue.swift b/App/Sources/UploadQueue.swift index a82e863..ca20985 100644 --- a/App/Sources/UploadQueue.swift +++ b/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() diff --git a/Tests/PhotosBackupTests/UploadQueueTests.swift b/Tests/PhotosBackupTests/UploadQueueTests.swift index 7d1993d..e9fbf6f 100644 --- a/Tests/PhotosBackupTests/UploadQueueTests.swift +++ b/Tests/PhotosBackupTests/UploadQueueTests.swift @@ -233,11 +233,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)) From 676ee51dc4a39137eb106a67ec28db30d02f69ca Mon Sep 17 00:00:00 2001 From: g8row Date: Sun, 20 Sep 2026 18:31:22 +0300 Subject: [PATCH 2/4] Ask the photo library once per finished row, not twice The follow-up hook runs on the main actor as every row finishes, and it fetched the asset twice: once to see whether the item is a Live Photo, once to see whether it carries a Google Photos edit. On a full-library backup that is two library reads per item on the thread drawing the activity list, which is the list #8 reports as slow. One fetch now answers both. The cheap checks go first, so the resource read behind the edit-base test still only runs for an adjusted asset. Co-Authored-By: Claude Opus 5 --- App/Sources/AutomaticBackupCoordinator.swift | 40 +++++++++++--------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/App/Sources/AutomaticBackupCoordinator.swift b/App/Sources/AutomaticBackupCoordinator.swift index 15448c3..dfba538 100644 --- a/App/Sources/AutomaticBackupCoordinator.swift +++ b/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 From 57b16066f226293a88d145a24dd5adef3700a4ad Mon Sep 17 00:00:00 2001 From: g8row Date: Sun, 20 Sep 2026 18:31:29 +0300 Subject: [PATCH 3/4] Run the tests on every pull request, and hold the release toolchain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of the last two pull requests merged with no checks reported: there was no workflow that built the app or ran its 191 tests. One now runs on every pull request and every push to main, reading its simulator from what the runner actually has so an image bump cannot silently break it. The release build's Xcode pin also failed open — a missing Xcode_16.4.app fell through to whatever the image shipped, which would quietly have broken the one thing building in CI is for: a binary tied to the commit and the toolchain it claims. It now fails the run and says what is installed instead. Co-Authored-By: Claude Opus 5 --- .github/workflows/release.yml | 15 +++++-- .github/workflows/tests.yml | 77 +++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/tests.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6ba8359..01e8226 100644 --- a/.github/workflows/release.yml +++ b/.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 diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..f1a1097 --- /dev/null +++ b/.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 From de1a2b36db6f30d1a9248d4e54ae9fbc78500c3c Mon Sep 17 00:00:00 2001 From: g8row Date: Sun, 20 Sep 2026 18:44:31 +0300 Subject: [PATCH 4/4] Stop the rate-limit test racing the pause it waits for The first thing CI caught. `pauseForRateLimit` sets `rateLimitPauseReason` synchronously but only reaches the sleeper once its task body runs, so waiting on the reason alone could observe the pause before any delay had been requested. That ordering held on a fast machine and lost on a loaded GitHub runner, where the assertion saw no delay at all. The wait now covers both. The queue itself is not affected: the reason is what gates `pump`, and it is in place before either. Co-Authored-By: Claude Opus 5 --- Tests/PhotosBackupTests/UploadQueueTests.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Tests/PhotosBackupTests/UploadQueueTests.swift b/Tests/PhotosBackupTests/UploadQueueTests.swift index e9fbf6f..1a8a739 100644 --- a/Tests/PhotosBackupTests/UploadQueueTests.swift +++ b/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])