sync(chunk 3): the sync engine
Full bidirectional sync, correct but not yet clever: calendar-query with no time-range, calendar-multiget in batches matched against what was asked for, conditional writes, and per-resource quarantine so one bad task cannot stop a collection. - :caldav gains CalendarCollection (list/fetch/create/update/delete + the three-way 412 triage), ETag with its weak flag, vdirsyncer-style resource names, and the RemoteCalendar seam. - CalDavHttp now sends Accept-Encoding: identity and Prefer: handling=strict, the two headers that keep ETags strong and our bytes unrepaired. - :app gains CollectionSyncer (the reconciliation), SyncEngine, SyncStore, QuarantineStore, SyncReport and a working SyncWorker, plus an in-app sync trigger since ContentResolver.requestSync is gated at our targetSdk. - VTimeZones closes a chunk-1 gap: the mapper emitted TZID with no VTIMEZONE to resolve it, which handling=strict turns from a repair into a rejection. /code-review high raised 11 findings, all fixed. The three that mattered: an empty listing swept the whole list (a VTODO comp-filter some servers mishandle is not proof of deletion, so an empty listing now never sweeps); quarantine never covered creates, so a permanently rejected new task was re-PUT forever; and a RELATED-TO deleted on the server was re-uploaded on the next edit. Reasoning recorded in docs/SYNC-PLAN.md. Chunk 2's on-device review is still outstanding; nothing here has been run on a device or against a real server.
This commit is contained in:
@@ -445,6 +445,102 @@ the columns already exist in the v1 schema. Every downstream insert writes
|
||||
a concurrent edit resolves server-wins with a visible report; a quarantined
|
||||
resource does not stop its collection.
|
||||
|
||||
### Settled while building it
|
||||
|
||||
**⚠️ The mapper emitted `TZID` with nothing to resolve it.** Chunk 1 writes
|
||||
`DTSTART;TZID=Europe/Berlin:…`, and RFC 5545 §3.2.19 requires a `TZID` without a
|
||||
leading solidus to reference a `VTIMEZONE` **in the same object**. Nothing
|
||||
generated one — `VTIMEZONE` sits at `VCALENDAR` level, outside the `VTODO` whose
|
||||
residue the mapper stores, so it was outside the round-trip guarantee entirely.
|
||||
Left alone, every zoned task would have been malformed, and `Prefer:
|
||||
handling=strict` — which chunk 3 sends precisely so servers stop repairing our
|
||||
bytes — turns malformed into rejected. `VTimeZones` now generates the definition
|
||||
from `java.time`'s own rules, which also keeps the zone we *write* in agreement
|
||||
with the zone we compute occurrences in. **Cost accepted: only the currently
|
||||
effective rules are emitted, not the historical transition table.** Tasks are
|
||||
dated now or later.
|
||||
|
||||
**java.time does not encode "the last Sunday" as `-1`.** It arrives as
|
||||
`dayOfMonthIndicator = 25` — "the first Sunday on or after the 25th" — and only
|
||||
the fact that a seven-day window ending on the last day of the month *is* the
|
||||
last occurrence identifies it. The obvious `-1` test never fires for any European
|
||||
zone, and the fallback emits a seven-value `BYMONTHDAY` list where every other
|
||||
client writes `BYDAY=-1SU`.
|
||||
|
||||
**`GetETag.eTag` has already been parsed.** dav4jvm strips the `W/` marker and
|
||||
records it in a separate field, so feeding its value back through an ETag parser
|
||||
reports **every weak tag as strong** — the exact failure the weak flag exists to
|
||||
prevent. `ETag.from(property)` and `ETag.parse(header)` are now different
|
||||
functions for that reason, and a test pins the difference.
|
||||
|
||||
**The discard is reported when the replacement arrives, not when the 412 does.**
|
||||
Announcing it at the conflict — and clearing `is_dirty` there — makes the loss
|
||||
real if the replacement download then fails, with nothing left to retry. The row
|
||||
now stays dirty until the download overwrites it.
|
||||
|
||||
**A `SyncStore` seam, not Robolectric.** The reconciliation is where local edits
|
||||
are discarded, tombstones swept and conflicts resolved; proving that needs a test
|
||||
double for eight methods, not an in-memory Room plus a new test runtime.
|
||||
|
||||
**The mutilation guard is scoped to what is actually reachable.** Nextcloud
|
||||
reduces `CLASS:CONFIDENTIAL` objects served from a *share* while leaving the ETag
|
||||
intact; `VALARM` stripping happens on read-only shares, which are never written
|
||||
to anyway. So the refusal is: shared collection + confidential + already on the
|
||||
server. The collection's `read-only` and `shared` flags are re-read on every sync
|
||||
rather than trusted from account-add, because ACL churn is silent.
|
||||
|
||||
**Manual trigger.** `ContentResolver.requestSync` is gated behind
|
||||
`hasAuthorityAccess()` at our targetSdk and returns silently when it refuses, so
|
||||
the in-app button enqueues the WorkManager job directly via `SyncTrigger`. The
|
||||
sync adapter uses the same path.
|
||||
|
||||
### Found by the review, and worth naming
|
||||
|
||||
**⚠️ An empty listing would have hard-deleted the whole list.** The sweep took
|
||||
"absent from `remote.list()`" as proof of a server-side delete, and its evidence
|
||||
is a `calendar-query` with a `VTODO` comp-filter — the filter whose mishandling
|
||||
is the reason the query carries no time-range in the first place. A server
|
||||
answering it with an empty *successful* multistatus is indistinguishable from an
|
||||
empty collection, and one such answer destroyed every task in that list in one
|
||||
pass. **An empty listing now never sweeps.** Cost accepted: a collection
|
||||
genuinely emptied on the server keeps its local rows until one task reappears
|
||||
there. That is recoverable by hand; the other error is not.
|
||||
|
||||
**Quarantine did not cover creates.** Failures on a resource with no href yet
|
||||
were counted under its UID and read back under its href, so nothing ever read
|
||||
them: a new task the server rejects permanently was re-PUT on every sync forever
|
||||
— the exact DAVx5 failure the counter exists to prevent — while the unread keys
|
||||
grew in DataStore without bound. Resources are now keyed by href *or* `uid:…`,
|
||||
and a success clears both.
|
||||
|
||||
**A `RELATED-TO` deleted on the server never unparented anything.** Only present
|
||||
parents were recorded, and `upsert` carried the old `parent_id` forward — which
|
||||
the writer then resolved straight back into a `RELATED-TO`, re-uploading the link
|
||||
the user had deleted elsewhere.
|
||||
|
||||
**Two validators disagreed about what a DATE is.** `ResourceValidator` tested
|
||||
only `VALUE=DATE` while the mapper also reads a bare eight-digit value as one, so
|
||||
a residue `DTSTART:20260101` beside an authored `DUE;VALUE=DATE:20260102` was
|
||||
rejected locally and never left the device — a case `VTodoMapperTest` already had
|
||||
a test for. The per-component rules now delegate to `VTodoMapper.validate`, which
|
||||
also restores the `TRIGGER;RELATED=END` check that the new validator had silently
|
||||
dropped and left as dead code.
|
||||
|
||||
**An unresolvable `TZID` produced a resource with no definition for it.** A
|
||||
Windows zone name from another client survives in the residue, and `java.time`
|
||||
cannot regenerate a `VTIMEZONE` for it — so with `Prefer: handling=strict` the
|
||||
upload becomes an unexplained rejection. It is now refused by name.
|
||||
**Open for chunk 5:** preserving the server's own `VCALENDAR`-level `VTIMEZONE`
|
||||
would fix it properly, and needs somewhere to put residue that is not the
|
||||
`VTODO`'s.
|
||||
|
||||
**Three narrower ones, all fixed:** quarantine counts were a global
|
||||
read-modify-write, so two accounts syncing at once discarded each other's
|
||||
(now merged inside `edit`); `NeedsSignIn` and `Misconfigured` never called
|
||||
`recordSync`, leaving the accounts screen reporting "synced 5 minutes ago" for an
|
||||
account that cannot sync at all; and `apply()` re-read every row in the list once
|
||||
per downloaded resource.
|
||||
|
||||
---
|
||||
|
||||
## Chunk 4 — incremental sync and scheduling
|
||||
|
||||
Reference in New Issue
Block a user