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 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 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..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]) @@ -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))