sync(chunk 4): incremental sync and scheduling
RFC 6578 as an optimisation on top of chunk 3's full path, and sync that runs by itself. - :caldav gains sync-collection, with invalidation matched on DAV:valid-sync-token in the body on any 4xx rather than on a status code — 400, 403, 409 and 412 are all used in the wild, and matching the status is why Thunderbird never recovers from sabre's 403. No DAV:limit (Nextcloud regressed it to an HTML error page); 507 on our own href is truncation. - The engine persists the token per page, after the bodies. An iteration cap and a no-progress guard, since the RFC never requires the token to advance. The full path runs anyway every 24h: a token the server accepts over a pruned change log returns 207, zero changes and no error, and the protocol gives no other way to notice. - The three membership traps: an unknown removed href is a no-op, a delete-then-recreate is re-identified from the UID in the body, and a mass removal is refused in favour of a real listing, because ACL churn looks exactly like one. - Scheduling is a PeriodicWorkRequest with a network constraint, expedited only for the in-app button. getForegroundInfo is implemented unconditionally (setExpedited falls back to a foreground service below API 31 and the default throws) but declares no service type, which would have pulled back the Android 15 dataSync budget and a Play video-demo requirement. initialIncomplete turned out not to be needed: adopting a token only after a full reconciliation completes removes the hazard it guarded, so there is nothing to persist atomically with anything. /code-review high raised 8 findings, all fixed. The two that mattered: the cadence clock sat in the backed-up DataStore, so a restore would have made the engine trust a stale token for a day — both sync-state stores now have their own excluded file; and the incremental download path lacked the write-phase guard, overwriting local edits that had never reached the server. Reasoning in docs/SYNC-PLAN.md. Chunk 2's on-device review is still outstanding; none of this has run on a device or against a real server.
This commit is contained in:
@@ -600,6 +600,96 @@ genuinely "once overnight".
|
||||
process kill mid-page loses nothing and duplicates nothing; the periodic worker
|
||||
runs and the manual trigger is instant.
|
||||
|
||||
### Settled while building it
|
||||
|
||||
**`initialIncomplete` turned out not to need a column — or to exist.** The plan
|
||||
called for it to be persisted atomically with the token, to stop a resumed
|
||||
partial sync sweeping against an incomplete set. But the mark-and-sweep it
|
||||
protects *is* chunk 3's full path, so the rule that removes the whole hazard is
|
||||
simpler: **a token is adopted only when a full reconciliation completes.** A run
|
||||
killed part-way stores no token and the next one starts over. No second column,
|
||||
nothing to write atomically with anything.
|
||||
|
||||
**The token is taken from the PROPFIND that *precedes* the read, not after it.**
|
||||
Both are imprecise; only one is imprecise safely. A token minted after the
|
||||
listing silently swallows everything that changed while we were reading it; one
|
||||
minted before merely re-reports those changes next time, and a re-report costs an
|
||||
ETag comparison.
|
||||
|
||||
**Where the cadence clock lives.** `task_lists.sync_token` is domain state and
|
||||
stays in Room. "When did this collection last reconcile in full" is scheduling
|
||||
bookkeeping, so it is in DataStore (`SyncCadenceStore`) — and deliberately
|
||||
outside any backup, because a restored "checked recently" on a fresh install is
|
||||
exactly the stale trust the full path exists to break.
|
||||
|
||||
**⚠️ `ForegroundInfo` must carry no `foregroundServiceType`.** Implementing
|
||||
`getForegroundInfo` is mandatory — `setExpedited` falls back to a foreground
|
||||
service below API 31 and the default implementation throws, so a worker without
|
||||
it crashes on every API 29/30 device. But passing
|
||||
`FOREGROUND_SERVICE_TYPE_DATA_SYNC` re-introduces the entire tail the design
|
||||
rejected: the `FOREGROUND_SERVICE_DATA_SYNC` permission, Android 15's
|
||||
six-hours-per-24 budget with its fatal `RemoteServiceException`, and a Play video
|
||||
demo per declared type. Lint caught it, correctly. The type is unnecessary
|
||||
anyway: above API 30 `setExpedited` uses an expedited job and never calls
|
||||
`getForegroundInfo`, and types only became mandatory at API 34.
|
||||
|
||||
**No boot receiver for sync.** WorkManager restores its own schedule across a
|
||||
reboot, and Android 15 forbids starting a `dataSync` FGS from `BOOT_COMPLETED` —
|
||||
so a design that needed one would have had no way to run. App open calls
|
||||
`rescheduleAll()` instead, which is idempotent under `KEEP` and repairs the one
|
||||
case WorkManager cannot: a schedule lost to "clear app data" or to a restore.
|
||||
|
||||
**Expedited is spent on the button and nowhere else.** The quota is per-app and
|
||||
exhaustible; a background trigger that spends it leaves none for the trigger the
|
||||
user is actually watching.
|
||||
|
||||
### Found by the review, and worth naming
|
||||
|
||||
**⚠️ The cadence clock was in the backed-up DataStore — while its own KDoc, and
|
||||
this document, claimed it was not.** Auto Backup includes `datastore/`, and only
|
||||
`agendula_credentials` was excluded. Restored onto a new device it would say
|
||||
"reconciled minutes ago", so the restored sync token would be trusted for another
|
||||
day: exactly the silently pruned change log the full path exists to catch. Both
|
||||
sync-state stores now live in their own `agendula_sync_state` file, excluded
|
||||
alongside the credentials — a restored quarantine count is the same class of lie,
|
||||
silently skipping resources that were never tried on this device.
|
||||
|
||||
**The incremental download path had no write-phase guard.** `downloadPhase` skips
|
||||
a row that is still dirty; `downloadChanged` did not. A share demoted to
|
||||
read-only leaves the upload refused and the row dirty, and the next change-log
|
||||
entry for it overwrote the user's unsent edit with no `DiscardedEdit` at all. The
|
||||
guard cannot simply be copied, either — the 412 path *deliberately* leaves the
|
||||
row dirty and waits for the download — so it is "dirty **and not awaiting a
|
||||
refetch**".
|
||||
|
||||
**A change log describes the past, and `applyRemovals` treated it as the
|
||||
present.** An entry predating this run's own upload deleted the row we had just
|
||||
written, leaving an orphan on the server and nothing on the device. It now
|
||||
respects `touched`, and reports a lost edit the way the sweep does.
|
||||
|
||||
**An abandoned incremental attempt poisoned a successful run.** `ChangeSet.Failed`
|
||||
and the mass-removal refusal both set `report.failure`, which the full fallback
|
||||
never cleared — so a server answering `sync-collection` with a bare 400 refused
|
||||
the new token, skipped the cadence record, and showed "last sync didn't finish"
|
||||
on every other sync while nothing was wrong with the data. A fallback that
|
||||
succeeds now demotes the attempt to `incrementalNote`.
|
||||
|
||||
**`refetch` was silently dropped on a successful incremental run**, so a PUT
|
||||
accepted without a strong ETag never got its canonical body back on a server that
|
||||
does not echo our own writes into its change log.
|
||||
|
||||
**Two of my own patches were wrong in ways the compiler could not see.** `fatal()`
|
||||
was written with its KDoc and never called — a `python` patch aborted on its
|
||||
second assertion after the first had already matched, so nothing was written, and
|
||||
the function sat there describing a bug it was not preventing. And the
|
||||
`delete-then-recreate` lookup read the href index *after* the same index had been
|
||||
re-pointed at the rows just written, so it never found the displaced row. The
|
||||
first was caught by the review, the second by a test.
|
||||
|
||||
**Two smaller ones:** the displaced lookup reintroduced the per-resource
|
||||
full-table scan the comment twelve lines above it forbids, and `MainActivity`
|
||||
re-ran the app-open sync on every rotation, theme switch and locale change.
|
||||
|
||||
---
|
||||
|
||||
## Chunk 5 — hardening, compliance, real servers
|
||||
|
||||
Reference in New Issue
Block a user