fix(app): trigger iCloud sync before concluding a backup is missing (SELF-3933) - #2277
fix(app): trigger iCloud sync before concluding a backup is missing (SELF-3933)#2277seshanthS wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51af213c50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A successful listing is authoritative: the folder is materialised, so | ||
| // the file is either visible (possibly under a placeholder name this | ||
| // check tolerates) or genuinely absent. | ||
| return entries.some(entry => entry.includes(FILE_NAME)); |
There was a problem hiding this comment.
Retry empty iCloud listings before declaring the backup absent
On a fresh device, the local iCloud container can already exist while its remote file metadata is still arriving, so readdir(FOLDER) may successfully return an empty array. This immediate return bypasses all remaining remoteProbeAttempts, reports no_backup_found, and never calls triggerSync, reproducing the restore failure this change is intended to fix. Treat an empty successful listing as inconclusive and continue probing for the configured budget before concluding that no backup exists.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
76d0e55 to
7312090
Compare
Closes SELF-3933.
Stack: #2272 → #2273 → this. Base is
self-3932-restore-failure-visibility; retarget as the stack merges.Problem
ios.ts download()gated onCloudStorage.exists()— natively a plainFileManager.fileExists(atPath:). iCloud syncs metadata eagerly but file contents lazily: on a fresh device the backup exists only as a.encrypted-private-key.icloudplaceholder until something callsstartDownloadingUbiquitousItem. Nothing ever did (triggerSync()was unused), soexistsstayed false and the user was told "no backup found" — on a fresh install, exactly when restore matters (SELF-2486's repro).Changes
hasRemoteBackup()discriminator — after the local-file check fails: probeexists(PLACEHOLDER_FILE_PATH)(throw-free), then fall back toreaddir(FOLDER)matching any entry containing the filename (tolerates placeholder-naming variants). A successful listing is authoritative; a rejected listing is ambiguous — nativecontentsOfDirectorythrows the sameERR_READ_ERRORfor "folder never created" and a genuine read failure — so rejection getsremoteProbeAttemptsre-probes before concluding "no backup", giving a fresh device's folder metadata a window to arrive instead of a false terminal "no backup found".waitForBackupFile()sync wait — firestriggerSyncat both the plain and placeholder paths (Promise.allSettled) on every poll tick, then pollsexistsagainst a deadline. Both paths because Apple doesn't document which URLisUbiquitousItemaccepts for a non-materialized item and the wrong one throws instead of enqueueing; every tick because the enqueue is idempotent and a single dropped request must not strand the user in a dead retry loop. The poll is the arbiter regardless of whattriggerSyncreports.backup_not_syncedreason + copy — "Your backup is still syncing from iCloud. Keep the app open and try again in a moment." Distinct fromno_backup_foundin both UI and the analyticsreason(AC3). Flows through the choice screen automatically via theCloudBackupErrorReasonunion from fix(app): surface cloud restore failures to the user and telemetry (SELF-3932) #2273.PLACEHOLDER_FILE_PATHin helpers.ts, adjacent toENCRYPTED_FILE_PATH(now derived fromFILE_NAMEso they can't diverge).IosDownloadOptions(syncTimeoutMs30s /pollIntervalMs1s /remoteProbeAttempts3) forwarded byindex.tson the iOS branch only — production callers pass nothing; tests inject tiny budgets instead of fake timers.AC status
withRetries".withRetriesretries on throw and destroys the original error at exhaustion; it can't express a boolean poll and would erase the error codes classification needs. Instead the bounded 30s deadline poll is the retry forexists(vswithRetries' ~9s — satisfying the AC's intent, "a budget suited to first sync"),triggerSyncis repeated fire-and-forget with the poll as arbiter, andreadFilekeepswithRetriesas before.backup_not_syncedvsno_backup_foundin copy +reason.FileUtils.sanitizePathstrips^/+, so the//vs/JS constants resolve to the same container path; a JS-layer path-consistency test pins the three constants to the same folder/filename.Assumptions / notes for reviewers
.name.icloudis Apple convention, not contract. Thereaddirincludes(FILE_NAME)fallback tolerates variants; if both probes miss, behavior degrades to today's (no regression).isUbiquitousItemaccepts the plain path for a placeholder is undocumented — the double-fire makes it moot in code, but only fresh-device QA proves materialization end-to-end.hasRemoteBackup.backup_not_synced.syncWaitedproperty onCLOUD_RESTORE_SUCCESSto size this fix's impact.Validation
All 31 pre-existing cloud-backup tests pass with zero body edits (mock factory + beforeEach defaults only). New coverage: placeholder → triggerSync at both paths → materializes → restores; never materializes →
backup_not_synced;triggerSyncrejected but file lands → still restores; placeholder found via folder listing → sync wait; unrelated listing → instantno_backup_foundwithtriggerSyncnever called; persistent listing failure → probes exhaust →no_backup_found; already-local file skips the sync machinery entirely;existsrejecting mid-poll aborts tobackup_read_failed; path-constant consistency.Device QA required — unit tests cannot substitute (simulator iCloud is flaky; real devices only):
triggerSyncmaterialization and the 30s budget.🤖 Generated with Claude Code