Fix upload deletion failures - #4301
Closed
cmigliorini wants to merge 24 commits into
Closed
cmigliorini wants to merge 24 commits into
cmigliorini wants to merge 24 commits into
Conversation
to accomodate for long uploads, failures.
PHAssetChangeRequest.deleteAssets always raises the native "Delete X Photos?" confirmation sheet, and removeUploadedAssetsIfNeeded ran on every foreground timer tick as soon as any asset qualified. During a busy stretch - a whole camera-roll backlog uploading in sequence, say - that meant the sheet could pop again for almost every single asset as it finished, in quick succession: disruptive, with no safety benefit over batching them. Debounce to at most once every 5 minutes while other work is still queued; skip the wait once the queue is empty so the last batch isn't left stranded until the interval happens to elapse. This only changes when the check runs, not what it considers eligible - getAssetLocalIdentifiersUploadedAsync always returns every currently-qualifying asset, not just newly-finished ones, so delaying the call can only batch more assets into one prompt; it can never cause an eligible asset to be skipped or lost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQyopQ6WKaar5MuCj7m7h4
When a background-session upload finished while the app was in the foreground, its metadata row was deleted and the finished upload was only staged in the in-memory NCMetadataUploadTranfersSuccess buffer, to be written back to Realm (status = Normal, assetLocalIdentifier intact) later by NCNetworkingProcess's foreground timer. That timer is not a safe place to depend on for this: it can be wedged indefinitely by a single stuck camera-roll asset extraction (NCCameraRoll's PHImageManager/AVAsset calls have no timeout and don't observe cancellation), and even without that, killing the app before the next opportunistic flush silently drops the buffered record. Either way the completed upload stayed invisible to the Files UI and, critically, to "remove after upload" cleanup, which only ever sees a completed upload once it is durably written with status = Normal. A field log showed this exactly: uploads confirmed present on the server never produced a corresponding "Uploaded file" success log locally, and were never offered for camera-roll deletion. Flush the buffer right after appending to it, so a completed upload is durably recorded the moment it finishes, independent of whether NCNetworkingProcess's timer ever ticks again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
extractImage/extractVideo awaited PHImageManager.requestImageDataAndOrientation, PHImageManager.requestAVAsset(forVideo:), and AVAssetExportSession.exportAsynchronously with no timeout. If a completion handler never fired - the common real-world case being a large video that needs a stalled/slow iCloud download - the await hung forever. That hang is fatal beyond the one asset: extractCameraRoll runs inside NCNetworkingProcess's single-flight foreground timer task, which only starts a new tick once the previous one has finished. A single stuck asset therefore wedged the entire pipeline permanently - all future uploads, downloads, zombie-transfer detection, and "remove after upload" cleanup - with no error surfaced anywhere, until the app was force-quit. A field log matched this exactly: uploads stopped dead after one asset, and no amount of reopening the app produced any further activity. Add withExtractionTimeout, which races the request against a 300s cancellable sleep. On timeout it actively cancels the underlying request (PHImageManager.cancelImageRequest / AVAssetExportSession's cancelExport()) instead of just walking away - both are documented to still invoke their completion handler afterwards, which is what lets the loser of the race actually resolve under structured concurrency instead of leaving the task group waiting forever. 300s balances letting a real slow-but-working iCloud download finish against bounding the worst case to a few minutes instead of indefinitely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two follow-ups to the extraction timeout fix: 1. createMetadataLivePhoto (the Live Photo paired-video fetch) had the same unbounded-wait shape as extractImage/extractVideo: PHAssetResourceManager.writeData(for:...) can stall indefinitely on a resource that needs an iCloud download, and this is reached from the same extractCameraRoll call that must never hang forever. Add writeResourceData, using the same withExtractionTimeout helper (cancelDataRequest on timeout) to bound it the same way. 2. NCNetworkingProcess.runMetadataPipelineAsync deleted a queued upload's metadata outright whenever extraction returned empty - which is also what a timeout/transient failure now returns, since it flows through the same catch-and-return-empty path as any other extraction error. That conflated two different situations: the asset genuinely no longer being in the photo library (nothing left to upload, fine to drop) versus extraction merely failing while the asset still exists (fine to retry). Only delete when the asset is confirmed gone; otherwise mark the row metadataStatusUploadError so it flows through the same 5-minutes-later retry every other upload failure already gets, instead of silently vanishing from the queue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PHAssetResourceManager.writeData(for:toFile:options:completionHandler:) returns Void, not a request id - unlike PHImageManager's requests, it gives no handle to actively cancel. The previous commit assumed it did (reusing withExtractionTimeout, which cancels via a captured request id on timeout), which failed to compile: "cannot assign value of type 'Void' to type 'PHAssetResourceDataRequestID'". Since there's nothing to cancel, withExtractionTimeout's task group is also the wrong tool here: its structured-concurrency scope-exit waits for every child to finish before returning, which would hang forever with no way to force the write's completion handler to fire early. Race it manually instead: the write's completion handler and a separate timeout Task both may try to resolve the same continuation, guarded by a small lock-based ResumeGuard so only the first one to arrive actually resumes it. The loser's eventual callback becomes a no-op - the write may keep running as an orphaned operation afterward, but writeResourceData itself now reliably returns within extractionTimeout regardless. Verified with a full Debug build of the Nextcloud scheme for iOS Simulator (BUILD SUCCEEDED, 0 errors). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHaXTFLZq4hwDS2TdogLqC
…celled NCNetworkingProcess.cancelCurrentUpload() cancels the in-flight chunk upload task/request on every app-background transition, not just on an explicit user action (the progress banner's Cancel button calls the exact same method). uploadChunkFile's CancellationError handling treated every cancellation identically: uploadCancelFile deleted the queued metadata outright, with no error status and no retry. For an auto-uploaded video (routed through the chunked path once it exceeds the chunk-size threshold), this meant simply backgrounding the app mid-upload silently and permanently dropped it from the pipeline - NCAutoUpload's discovery bookmark had already advanced past the asset the moment it was queued, so it was never re-offered by a later scan either. The asset itself was correctly left alone in the camera roll (the upload never having succeeded), so nothing was destructively lost, but the video would never be uploaded. Reproduced via two field tests: a rapid foreground/background cycling session where two videos were queued, chunk-uploaded a few bytes, then vanished with no further log activity; and a control run that stayed foregrounded the whole time and uploaded successfully. Mirror the policy uploadError(withMetadata:) already applies on the non-chunk upload path for a cancelled transfer: an automatic upload (sessionSelector == selectorUploadAutoUpload) is requeued as metadataStatusUploadError for retry - the chunks already written to disk let that retry resume rather than restart from scratch - while a manually-initiated upload is still discarded outright on cancel, unchanged. Also switch the cancellation sentinel from this file's own one-off errorCode -5 to the codebase's standard NSURLErrorCancelled, matching the download path and uploadError itself, so both catch branches go through the same, already-correct classification instead of a separate ad hoc check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vu9FJkwigTJd2y5UFB2jVw
BGAppRefreshTask and BGProcessingTask are registered independently (AppDelegate+AppRefresh.swift, AppDelegate+AppProcessing.swift) and both call NCAutoUpload.shared.autoUploadBackgroundSync() directly, with no coordination between them. iOS is free to fire both around the same time, and a field log showed exactly that: both tasks starting in the same second, each independently discovering and queuing the same "new" assets, then each independently dispatching uploads for them. The duplicate uploads collided at the server (WebDAV 423 Locked) on some of the redundant attempts. Worse, uploadComplete's fallback lookup (NCNetworking+NextcloudKitDelegate.swift) - reached whenever it can't find the exact metadata row for a specific completing task by serverUrl+fileName+sessionTaskIdentifier - deletes *any* row matching just that file's serverUrl+fileName, with no scoping to the specific duplicate. With several near-simultaneous completions (success and 423 alike) racing through that fallback, it could delete a different duplicate's row entirely, including one that had already reached status = Normal - silently erasing the local "this was uploaded successfully" record even though the file had genuinely landed on the server. A field test showed exactly this outcome: three photos confirmed present on the server (re-downloadable, so genuinely uploaded) that were nonetheless still sitting in the camera roll, with no trace of them left in the local transfer queue at all. NCAutoUpload is a plain class, not an actor, so this needs an explicit lock rather than relying on isolation the way NCNetworkingProcess's own single-flight `currentTask` check can. Add an NSLock-guarded isBackgroundSyncing flag around autoUploadBackgroundSync(): a second concurrent caller logs and backs off immediately rather than running a redundant pass, the same "no gain from queueing up behind it" policy NCNetworkingProcess's timer already uses - there will be another opportunity soon regardless (the next BGTask run, or the next foreground activation). Also add a diagnostic log line confirming NCBackgroundLocationUploadManager.start() actually runs, to help narrow down a separate, still-open question: significant-location- change monitoring gets armed repeatedly across multiple field tests, but "Triggered by location change" has never once been observed firing, even after several-kilometer trips with a confirmed cell tower change. Field-validated: a follow-up test reproduced the same concurrent BGT firing (both handlers starting within the same second again), and this time logged "Auto upload background sync already running, skipping" instead of racing - zero 423 errors anywhere in that day's log, versus five in the log that first surfaced this bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHaXTFLZq4hwDS2TdogLqC
…tion Background-triggered auto-upload discovery (BGAppRefreshTask/ BGProcessingTask -> autoUploadBackgroundSync -> initAutoUpload -> getCameraRollAssets) could see a stale, frozen snapshot of the Camera Roll collection, silently missing photos/videos captured while the app was backgrounded - sometimes for hours - until the app was foregrounded again, at which point the very same code path immediately found everything. Added diagnostic logging to getCameraRollAssets (authorization status, the autoUploadSinceDate bookmark, and both a filtered and an unfiltered baseline asset count) to confirm this precisely: a field log showed the unfiltered count of the same collection frozen at 2 across two separate background checks eight seconds apart, while the true count (confirmed once foregrounded) was already 53. This isn't a predicate or bookmark bug - the unfiltered, no-predicate count itself was stale, meaning the app's connection to PhotoKit simply wasn't reflecting the library's current state during headless background execution. The one place this app registers a PHPhotoLibraryChangeObserver (AlbumModel.swift, the album-picker settings screen) is only ever active while that specific screen is open - never during normal use, and never in the background - so PhotoKit had no reason to keep this process's view current outside of foreground activity. Add NCPhotoLibraryObserver, a persistent observer registered once at app launch and kept for the app's entire lifetime (including background execution). It doesn't need to act on change notifications, only to exist, to keep PhotoKit treating this process as a live subscriber. Field-validated with a 300km/multi-hour test: "Photo library change observed" (this observer's callback) began firing as soon as new photos were actually taken, and every background discovery check from that point on tracked the growing library correctly and in real time (39/42/52/94 unfiltered, correctly finding 37/3/10/42 new assets each time) - all via BGTaskScheduler alone, no foreground activation needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHaXTFLZq4hwDS2TdogLqC
NCAutoUpload's background sync loop called uploadFileInBackground unconditionally, even for assets that extraction determined need chunking (chunk > 0) or are E2E-encrypted. Both cases have their metadata.session reassigned to the plain foreground sessionUpload identifier during extraction, which NKBackground.upload doesn't recognize as one of the three background session identifiers it matches against — its upload task ends up nil, yet it still reports .success, logged upstream as the cryptic "Background upload task creation failed: ..., task: nil, error: 0". Chunked uploads require a live foreground Alamofire session regardless (NCNetworkingProcess.uploadChunk) and can't run from a background execution context at all, so there's nothing to attempt here. Skip these items with a clear log line instead, leaving them queued (status stays waitUpload) for NCNetworkingProcess's foreground pipeline, which already branches correctly on chunk > 0, to pick up once the app is foregrounded. Field-validated: a 300km drive test (log-3.txt, pre-fix) showed 7 videos hit the silent background-upload failure before eventually uploading in the foreground. Post-fix (log-5b.txt), the same scenario logs a clean "Deferring ... needs chunked upload, which requires the app in the foreground" for each one, with zero background-upload-failure errors, and both uploaded successfully once the app was foregrounded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fier `realm.add(_:update: .all)` overwrites every property of an existing row with the same primary key (`ocId`), including ones the incoming object has no way of knowing about. `assetLocalIdentifier` is exactly that: it is set locally when auto-upload queues a camera-roll asset, and it is the only link back to the asset that "remove after upload" cleanup has, since `getAssetLocalIdentifiersUploadedAsync` filters on `assetLocalIdentifier != ''`. Metadata rebuilt from a server response — any PROPFIND of the folder a photo was just uploaded to — goes through `convertFileToMetadata`, which never populates that field. Writing it back blindly therefore erased the link and left the asset permanently un-deletable from the camera roll, with no error and no retry. Because it depends on sync timing racing cleanup, it hit an arbitrary subset of each batch, which is why "some pictures are never removed" looked intermittent. An empty incoming value now never overwrites a stored non-empty one. Nothing legitimately clears the field through this path: the one place that should clear it (`clearAssetLocalIdentifiersAsync`, after a confirmed deletion or a deliberate refusal) writes it directly. Field-validated: before the fix, a prompt covered 4 of 5 eligible photos, and a later batch passed 11 identifiers for only 8 surviving assets. After it, every batch matched exactly — 7 of 7, then 8 of 8 across two sessions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four related defects in `removeUploadedAssetsIfNeeded`, all of which surfaced during field testing of the camera-roll cleanup: - The result of `PHPhotoLibrary.performChanges` was discarded, and the tracked identifiers were cleared unconditionally afterwards. A deletion that failed — or that the user declined — therefore still retired those assets from tracking, permanently forgetting to ever retry them. - `PHPhotosError.userCancelled` is reported both when the user taps Cancel and when the sheet is torn down because the app left the foreground, so the error alone cannot tell a refusal from an interruption: locking the screen with the sheet open was indistinguishable from answering "no". The app state settles it — presenting the sheet only makes the app resign active, it never backgrounds it — so a `didEnterBackground` while the sheet was up now marks the attempt as interrupted and retried. This needs `lastDidEnterBackgroundDate` (added to NCAppStateManager) rather than reading `isAppInBackground` at completion time, so a transition that has already been reversed by the time the handler runs is still detected. - A genuine refusal now retires exactly the set that was put to the user, by clearing those identifiers, and leaves the feature itself on: assets uploaded later are proposed normally. - The wait before prompting was measured from the previous attempt, which is ancient whenever the app has been running a while. The interval was then trivially satisfied, and the sheet popped for whichever single asset happened to finish uploading first, with the rest arriving in a second prompt minutes later. It is now measured from when the current batch of candidates first appeared, giving "prompt once the queue drains, or after the interval if it never does". The queue-empty fast path is also suppressed after a failed attempt, so an idle queue can no longer re-trigger the sheet on every timer tick. Field-validated: a screen lock now logs "interrupted … will retry" and re-prompts after the interval; a real Cancel logs "won't propose these again" and later photos are still proposed; and batches now arrive as a single prompt (7 of 7, then 8 of 8 including 4 carried over from the previous evening) instead of splitting into 1-then-17. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eted Dispatching a background-session upload only creates the task; the transfer is scheduled by the system and can land seconds or hours later, possibly only once the app is foregrounded again. The log previously showed only the dispatch, so "the file uploaded while backgrounded" and "the task sat queued until the app was reopened" were indistinguishable after the fact — a 33-minute stall in one field log could not be attributed without guessing. Two additions: - The dispatch line now names the session. The Wi-Fi-only session (`sessionUploadBackgroundWWan`) is configured with `allowsCellularAccess = false`, so its tasks legitimately sit idle until Wi-Fi appears; without the identifier that is indistinguishable from the OS deferring a transfer on the ordinary background session. - `uploadComplete` now logs the completion together with the app state at that moment, which is what makes the two cases separable. The app-state part is compiled only into the main app: `isAppInBackground` lives in that target, while this file is also built into the extensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.fitness` tunes the location subsystem for pedestrian sports activity, which is a poor description of a feature whose only job is to be woken after the device has moved somewhere — driving included. `.other` is CoreLocation's own default and bakes in no assumption about how the user is moving, without `.automotiveNavigation`'s much higher power and accuracy profile, which is meant for actual turn-by-turn navigation rather than an occasional background wake-up. This was tried as a possible explanation for significant-location-change monitoring never firing; a subsequent field test showed it is not the cause, so this is a correctness change rather than a fix, and the underlying problem is still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`SceneDelegate.startNextcloud` set `isAppInBackground = false` unconditionally
("App not in background"), but a scene also connects when the system launches
the app straight into the background — a BGTask run, or a relaunch for a
significant location change. Nothing corrected the flag afterwards, because
`didEnterBackground` cannot fire for an app that never entered the background,
so it stayed wrong for the whole of such a launch.
Everything gated on the flag then behaved as though the user were looking at
the app: the extract/upload loop, download-in-background, live-photo pairing,
tab-bar timers, viewer loading, and `didUpdateLocations`, which returns early
when it believes it is running in the foreground.
Now read from `UIApplication.shared.applicationState` instead.
Field-validated via the upload-completion log line added earlier: before the
fix, completions during a BGTask-initiated launch were recorded as "while app
foregrounded" 29 seconds before the app was actually foregrounded; after it,
all 23 completions across a full background-only outing correctly read "while
app backgrounded".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ations Every early return in `didUpdateLocations` was silent, so an absent "Triggered by location change" could mean either "the OS never delivered an update" or "it was delivered and then discarded" — with no way to tell which. Since significant-location-change monitoring had never once been observed to fire in the field, that distinction was the whole question. Delivery is now logged first, and the foreground guard logs when it suppresses an update, so the three outcomes — never delivered, delivered but ignored, delivered and acted on — are distinguishable from the log alone. The first field test with this in place produced zero delivery lines across a qualifying trip, while showing a bare background launch timed exactly with the movement: iOS is relaunching the app for the location event, but the relaunched process has no CLLocationManager and no delegate to hand it to, because the manager is only ever instantiated from sceneDidEnterBackground, which does not run for a launch that starts in the background. That is addressed separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… events
Significant-location-change monitoring had never once fired in the field,
across a week of qualifying tests: monitoring was armed (its own log line
confirmed it), "Always" authorization was granted, the `location` background
mode was present, and iOS's own "used your location in the background" notice
showed the OS associating location use with the app — yet `didUpdateLocations`
was never called, and the app was never relaunched with the `.location` key.
The cause was where the manager was created. `NCBackgroundLocationUploadManager`
is a plain NSObject whose `static let shared` initialises lazily on whichever
thread first touches it, and in practice that was always
`sceneDidEnterBackground`'s `group.addTask { … }` closure — a Swift
cooperative-pool thread with no run loop. Core Location delivers delegate
events on the thread the manager was created on, and that thread must have an
active run loop. A manager created on a cooperative-pool thread arms monitoring
without error and then simply never receives a callback, and the registration
never functions at the system level either, which is why no relaunch ever
happened.
The start/stop call now hops to the main actor, and the initialiser asserts it
is on the main thread so a regression fails immediately in Debug rather than
as another week of "it never fires". The other two entry points that can
trigger the lazy initialiser (`AppDelegate` and the `@MainActor`
`requestAuthorizationAlwaysAsync`) were already on main.
Field-validated: the very next run produced the first location deliveries ever
recorded —
Location monitoring started
[LOCATION] Location update delivered: 48.8797…, 2.2419…
[LOCATION] Triggered by location change: 48.8797…, 2.2419…
[BGSYNC] getCameraRollAssets …
— twice, each driving a discovery pass end to end.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When an app that has started significant-change monitoring is terminated, the
system relaunches it for the next event, passing the `.location` launch key.
Apple's contract for that relaunch is that the app must create a new
CLLocationManager, assign its delegate and call
startMonitoringSignificantLocationChanges() again from
didFinishLaunchingWithOptions; the pending update is then delivered to that
delegate. See:
https://developer.apple.com/documentation/corelocation/cllocationmanager/startmonitoringsignificantlocationchanges%28%29
This app never did that. The manager is only ever instantiated from
sceneDidEnterBackground, which does not run for a launch that starts in the
background, or from the settings screen. A location relaunch therefore produced
a process with no manager and no delegate, iOS had nowhere to hand the event,
and the app went back to sleep having done nothing.
didFinishLaunchingWithOptions now checks for the `.location` key, logs the
launch reason, and re-arms monitoring through the shared manager.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… ones Significant-location-change monitoring is per process. Once the process that armed it is gone, whichever process replaces it needs its own CLLocationManager with a delegate, or iOS has nothing to hand a location event to. Until now the only places that created one were sceneDidEnterBackground (never runs for a launch that begins in the background) and the `.location` branch added in efa2450478 — which under the scene lifecycle never matched, since launchOptions[.location] came through nil on every observed relaunch. So any *other* background launch — a BGTask run, a PhotoKit relaunch, a background-URLSession wake — produced a process with no manager at all. A location event arriving while that process was alive was delivered to an app with no listener and dropped, with no relaunch either, since the app was already running. log-f.txt showed exactly this: two unrelated relaunches mid-drive and no movement-triggered delivery afterwards. didFinishLaunchingWithOptions now re-arms monitoring on every launch where authorization is `.authorizedAlways` and the location preference is on (the same gate sceneDidEnterBackground uses, minus the per-account auto-upload check — the database is not open yet, the sync the delegate kicks off honours that setting itself, and the next backgrounding stops monitoring if it should be off). A `.location` launch skips the gate: the system would not have relaunched us otherwise. This is Apple's contract for such relaunches: https://developer.apple.com/documentation/corelocation/cllocationmanager/startmonitoringsignificantlocationchanges%28%29 Field-validated twice: log-g.txt (5 km drive) — two genuine movement-triggered relaunches at 15:35:11 and 16:01:06, each with "Re-arming location monitoring at launch" → "Location update delivered" → "Triggered by location change"; log-h.txt (1.6 km out-and-back walk) — relaunch 17:13:54 three minutes into the walk, then a second delivery at 17:19:47 into the same still-alive process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… discovery initAutoUpload bailed out with `guard networking.isOnline`, which is false while `networkReachability` is still nil — i.e. until Alamofire's observer has fired for the first time. A location-triggered sync runs before that: the initial fix is delivered synchronously inside startMonitoringSignificantLocationChanges() during didFinishLaunchingWithOptions, a second or two into a cold background launch. Every such sync therefore returned 0 without ever querying the camera roll. log-g.txt showed it plainly: two genuine movement-triggered relaunches mid-drive, each reporting "Auto upload found 0 new items" with photos waiting. Discovery and queueing need no network anyway, and the background URLSession copes with connectivity on its own, so the guard now only bails when reachability is *known* to be `.notReachable`, and logs when it does. Field-validated in log-h.txt: the location relaunch at 17:13:54 went straight through to "getCameraRollAssets … filteredCount=2" and dispatched two uploads on the background session within four seconds of launch, and the second delivery at 17:19:47 did the same for two more — the first mid-walk background dispatch ever recorded on this branch. Note what that test also showed: transfers started while the app is in the background are always discretionary, whatever `isDiscretionary` and `allowsCellularAccess` say (https://developer.apple.com/documentation/foundation/urlsessionconfiguration/isdiscretionary). The phone was on cellular (mostly 5G) for the whole walk, Wi-Fi off. iOS held the four queued uploads for ~22 minutes; three of them went through at 17:35:56, the minute the phone was plugged in — still on cellular, no Wi-Fi involved — and the fourth only after the app was foregrounded at 17:50. Power alone was enough to release them. This fix gets the queueing done mid-walk; when the bytes move is the system's call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion applicationWillTerminate posted a local notification asking the user to keep the app running for a better experience. With the chunked-upload fix on this branch, being backgrounded or terminated mid-upload is no longer a loss: the upload is requeued and picked up by the next background pass or foreground session, so there is nothing left for the notification to warn about. Small uploads never needed it — they run out of process on the background URLSession and survive termination by design. And a user closing the app while a chunked upload is visibly in progress in the foreground already knows they are interrupting it; a notification after the fact adds noise, not information. Also removes the UNNotificationSettings snapshot that existed only to gate this notification, and the source string from en.lproj (the other locales are maintained by the translation bot). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing to verbose The location log lines wrote the device's latitude/longitude to the log file. That file is what users attach to bug reports, so a trail of timestamped positions is a way to track a person; nothing in the pipeline needs the coordinates anyway. Both the delivery line and the pre-existing "Triggered by location change" line now log the event without them. Five of the log lines added while chasing the background-upload bugs on this branch were diagnostic scaffolding rather than something a user's log should carry at the default level, and now require the verbose level: - "Location monitoring started" (redundant with the launch-time re-arm line and "Location monitoring stopped"; fired on every backgrounding) - "Location update delivered" (pre-guard diagnostic; "Triggered by location change" and "Location update ignored" record the outcome) - "Photo library change observed" (several lines per photo) - both "getCameraRollAssets: …" lines (internals of every discovery pass) The second of those carried an unfiltered PhotoKit fetch over the whole library, done purely to print the count; it is now only run when the line will actually be written. The app's log level comes from NCPreferences().log. Kept at the default level: the launch-time re-arm line, "Location update ignored", "Auto upload skipped: network not reachable", the missing-collection error, the sync-overlap notice, "Deferring … needs chunked upload", "Uploading file … on session …", "Upload completed … while app …", and the four camera-roll removal outcomes — each is one line per event and explains something a user would otherwise report as "nothing happened". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
handleAppRefresh logged "Start refresh task" and, from the expiration handler, "Refresh task expired" — but nothing at all on normal completion. So a refresh task that did its work left no end marker, and an expiry was the only way a run could ever announce it had stopped. Reading a field log, that makes three quite different outcomes look identical: the pass finished, the pass is still in flight, or the process was killed mid-pass. That distinction matters here, because an expiry genuinely truncates work: in one field log a refresh task started at 10:20:10, logged its single discovered asset at 10:20:11 and expired at 10:20:12 without dispatching it — the upload only went out on the next task, 19 minutes later. Budgets observed on device range from 2 to 25 seconds with no relation to how much work is pending, so knowing whether a given pass ran to completion is not a detail. Now mirrors the processing task's existing "Stop processing task", with the same success/error emoji split. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
|
Thanks for the work @cmigliorini Some of the fragile areas mentioned here are already well known to us, and we are currently testing the migration to the new PhotoKit background upload architecture. Using AI to analyse the repository is not, by itself, the difficult part — we all have access to the same tools and can ask them to inspect the codebase and suggest changes. The real engineering work is being able to critically evaluate those suggestions, understand their implications within the existing architecture, and take responsibility for the proposed solution. That is particularly important in areas such as background execution, PhotoKit, lifecycle, concurrency and persistence, where code can look perfectly reasonable and still be architecturally wrong or introduce subtle regressions. Before proposing changes of this scope, I would strongly recommend first gaining a deeper understanding of the relevant iOS APIs and of how this codebase is designed. Otherwise, the difficult part is simply transferred to the maintainers, who then have to determine whether the AI-generated analysis and implementation are actually correct. |
Author
|
Hi @marinofaggiana , thanks for the reply. What my work amounted to is the testing itself, and it showed a few major flaws in the legacy mechanism, including -- that's pretty usual with this kind of things -- a few race conditions, and the fixes demonstrate improvements. Now, if you're not considering maintaining the legacy autoupload, it makes sense to ignore this... I own iOS27 capable devices so if the upcoming version fixes these, then I'm not impacted and will be happy. At any rate, I'm willing to participate in the beta testing, if you could include me to the group? |
Member
|
Definitely! In fact, thank you. |
Author
|
Regarding APIs understanding, I have a few insights I'm happy to share, would you like me to do so here, in your PR, in your issue ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submitting this as draft, see #4299. Still need to review some of it, and of course comply with all requirements, including sign-off, if this seems like something you want to accept.
🤖 AI (if applicable)