Files
agendula/docs/SYNC-PLAN.md
T
makiolaj fd6c302c17 docs(sync): split the sync work into five chunks
SYNC.md decides; it doesn't sequence. SYNC-PLAN.md turns its ~40 scattered
findings into five chunks — mapper, auth/discovery, engine core, incremental
sync + scheduling, hardening — each with a done-when and the traps it must
not get wrong.

Closes SYNC.md's three open questions: vendor dav4jvm/cert4android at our
Java 17 target, server-wins-and-report on 412, External mode stays.
2026-09-04 16:15:45 +02:00

23 KiB
Raw Blame History

Agendula — CalDAV sync: the build plan

Branch: feat/caldav-sync, cut from release/v1.0.0. Design: SYNC.md — the decisions, the traps and the audit trail behind them. This document does not repeat it. It sequences it. Shape: five chunks, one branch, one PR at the end.

SYNC.md is a 1,200-line decision document with roughly forty ⚠️ findings scattered through it, most of which are "the obvious implementation is wrong, here is why". That is the right shape for a design document and the wrong shape for a work queue. This is the work queue.


Decisions taken before the first line of code

SYNC.md left three questions open and named two of them as blocking. All three are now answered, plus one the phases imply. These are not reopened by the implementation chunks — if a chunk finds evidence that overturns one, it stops and says so rather than quietly picking the other branch.

# Question Answer Consequence
1 dav4jvm + cert4android distribution, and the Java-21 wall Vendor a known-good tree, recompiled at our Java 17 target FAIL_ON_PROJECT_REPOS stays; no Ktor/guava/xpp3 tail; MPL-2.0 headers retained and the attribution screen becomes mandatory, not optional. Freezes the API churn that shipped two breaking majors nineteen days apart
2 Conflict policy on 412 Server wins, local edit discarded DAVx5's policy. Terminates, always. The discarded edit is surfaced — a sync report the user can read — because silence is what makes this policy feel like data loss. The draft's "preserve as a duplicate" stays withdrawn: RFC 4791 §4.1 makes it unuploadable
3 External mode's future Kept, unchanged The mapper and the UI stay dual-capable. Retiring it is a separate decision that does not block sync, and it ships and works today
4 iCalendar library ical4j 4.3.0, with the shims SYNC.md § Libraries enumerates BSD-3 (attribution screen again). Costs the 2.2 MB of duplicated zone data, the mandatory ical4j.properties + MapTimeZoneCache + registry shim, and R8 keep rules. biweekly's missing tz database is the disqualifier for CalDAV round-tripping

Scope boundaries

Not in this branch, deliberately:

  • Extraction into floret-kit. SYNC.md § What lands in floret-kit is right that core-dav and core-sync belong there and that Calendula needs the same primitives. Extracting across a submodule boundary while the API is still moving costs more than it buys. Mitigation, which is a real constraint on every chunk: the DAV and iCalendar layers take no task-domain type and no Android UI type, so the later extraction is a file move. Chunk 5 records what moves.
  • Retiring External mode, per decision 3.
  • A release. Nothing here bumps versionCode / versionName. Sync ships when it works against real servers, and that is a decision taken after the device review, not inside this plan.

In scope but not verifiable by CI: everything that needs a real server. The per-server trap matrix (SYNC.md § Server reality) is written as named, skipped integration tests in chunks 3–4 and turned on by hand against a live Nextcloud/Radicale/Baïkal in chunk 5.


The five chunks

# Chunk Rests on The thing it must not get wrong
1 Mapper — VTODO ↔ Room nothing Unknown properties survive a read-modify-write cycle
2 Auth, discovery, account 1 (nothing hard) Never send a credential into an unvalidated redirect chain
3 Engine core — read, write, conflicts 1, 2 A failed resource must not fail the collection
4 Incremental sync + scheduling 3 Never persist a sync token before the bodies it covers are applied
5 Hardening, compliance, real servers 1–4 Ship no licence violation, and no retry loop on a dead app password

Chunks 1 and 2 are genuinely independent of each other; the ordering between them is arbitrary and 1 goes first only because it is the one with a specification that can be written down before any network exists.


Chunk 1 — the mapper

Goal: a bidirectional VTODO ↔ Room-entity mapper whose specification is its fixture corpus. Today ICalendarWriter is the export half only — write-only, hand-rolled, and it silently drops everything it does not model.

The corpus comes first. Not as a testing courtesy: SYNC.md § Unknown properties is explicit that the fixtures that catch bugs are the adversarial ones, and a mapper written before its corpus gets a corpus that ratifies it. Minimum set, each one a named failure the design predicts:

  • a CLASS:CONFIDENTIAL task as Nextcloud rewrites it out of a shared calendar — the one that destroys data on write-back
  • a master plus RECURRENCE-ID overrides in one resource
  • unknown properties nested inside a VALARM — the case a flat property-per-row model cannot represent
  • an entirely unknown sub-component
  • a TZID the device tzdb does not know
  • a UID containing / and @
  • one exceeding CALDAV:max-resource-size
  • RRULE + DUE with no DTSTART, which has no well-defined RECURRENCE-ID
  • X-MOZ-LASTACK / X-MOZ-SNOOZE-TIME (dropping them causes Thunderbird alarm storms) and X-APPLE-SORT-ORDER
  • all four recurring-completion models on read (a/b/c/d — we write (a))

Round-trip assertion, stated the way SYNC.md corrects it. Byte-stability is unachievable and asserting it produces a corpus that gets normalised until it tests nothing. Re-parse both sides into a canonical multiset of (component path, property name, params as a sorted map, unfolded unescaped value) and assert equality modulo an enumerated allowlist: PRODID, DTSTAMP, LAST-MODIFIED, SEQUENCE, VTIMEZONE bodies, fold positions, parameter quoting. Byte-equality is asserted where it means something — the unfolded, unescaped value octets of every untouched property, and full parameter preservation including unknown parameters.

Storage for the unknown half. A new table, and therefore a schema migration to v2 with an exported schema and a MigrationTestHelper test, per the repo's existing pattern in app/schemas/. It must represent nesting (component path), ordering, and opaque unknown sub-components. Cap the total at ~25 kB per resource, as DAVx5 does, for CursorWindow and CALDAV:max-resource-size both.

Deliverables

  • fixture corpus under app/src/test/resources/vtodo/, with a README naming what each fixture is for
  • VTodoParser / VTodoSerialiser behind one interface, no Android types
  • unknown-property table, Room migration v1→v2, migration test
  • the SYNC.md § The rest of the minefield rules as individual tests: SEQUENCE is preserve-verbatim; DTSTAMP regenerates but LAST-MODIFIED does not; COMPLETED is UTC-only; RELATED-TO;RELTYPE=PARENT points up; PRIORITY 0 = undefined; a TRIGGER;RELATED=END alarm without DUE is invalid and must be caught before PUT, because ordinary UI actions reach it
  • ICalendarWriter either becomes the serialiser or is deleted in its favour — two VTODO writers in one app is the defect this chunk exists to prevent
  • ical4j's ical4j.properties, MapTimeZoneCache and registry shim, plus its R8 keep rules (-keep class net.fortuna.ical4j.** { *; } + the -dontwarn set), because that failure is release-only

Also settle here: the VALARM ownership collision. Either the adapter owns alarms and AndroidTasksDataSource.setAlarm stops bulk-deleting property rows, or reminders stay local-only and are never serialised. Picking neither is how one local reminder edit destroys every server-side alarm on that task.

Done when: the corpus round-trips, the migration test passes both directions, and no fixture loses an unknown property.


Chunk 2 — auth, discovery, account

Goal: the app can add a CalDAV account and list its VTODO collections. No syncing yet.

Vendoring, first. A new Gradle module holding dav4jvm + cert4android sources at our Java 17 target, MPL-2.0 headers intact, with a PROVENANCE.md recording the upstream commit — the repo already has this pattern from :provider. Fix the two inherited defects while vendoring, since we now can: 303 is not handled (RFC 6764 §5 names it explicitly) and issue #209 mutates location in place so permanent redirects never reach the caller.

Discovery is the nine-step pipeline in SYNC.md § Generic CalDAV discovery, including every step marked ⚠️ MISSING there. The two filter corrections are the important part and both are inversions of the obvious:

  • supported-calendar-component-set absent means "supports everything." Request properties by name — RFC 4791 §5.2.3 says the property SHOULD NOT come back from an allprop request. Treat empty as all.
  • Classify on resourcetype as an unordered set, with a positive test for CALDAV:calendar — not by excluding schedule-outbox, because SOGo's main personal calendar reports collection + calendar + schedule-outbox simultaneously for every non-Apple client, which is exactly what we are.

The discovery traps table (Posteo SRV-only on 8443, GMX/Web.de path=, null SRV target, Google's dead-end SRV, iCloud/Zoho well-known 401, dav.runbox.com's HTTPS→HTTP downgrade, <D:unauthenticated/> as a 200 that means failure, cross-host home sets) becomes a table-driven unit test over recorded responses.

Nextcloud Login Flow v2 with all five corrections: POST form-encoded (a GET is 405); an explicit User-Agent, because it becomes the app password's name in the user's security settings and okhttp/4.12.0 defeats the entire point of the flow; 404 means pending only and anything that is neither 404 nor 200 stops the poll; validate the endpoint origin and refuse an http downgrade outright; and loginName is not the uid — it is the Basic auth username and nothing else. Custom Tabs needs the <queries> entry, the ACTION_VIEW fallback, {token, endpoint, deadline} persisted before launch, and an explicit "I finished / Cancel" affordance.

Auth is not just Basic. Digest (Baïkal defaults to it, OkHttp has none), Basic sent preemptively via an Interceptor gated to HTTPS and the account's own origin, and domain-detection at account-add time for the three servers whose real error is not "wrong password": Fastmail (app password; Basic plan has no CalDAV), iCloud (app-specific password, 2FA to mint one), Google (OAuth2-only — refuse with an explanation, SYNC.md drops it as a target).

Credentials: Keystore AES/GCM/NoPadding, blob in DataStore. Do not call setUserAuthenticationRequired; do not set setUnlockedDeviceRequired (breaks background sync). AEADBadTagException and KeyPermanentlyInvalidatedException mean re-authenticate, not crash. Exclude the blob — and only the blob — from Auto Backup: a restored ciphertext is permanently undecryptable.

The account plumbing, which is the part that silently does nothing if skipped. AccountManager plus a registered AbstractThreadedSyncAdapter whose onPerformSync enqueues a WorkManager job and waits. Without a registered adapter, hasAuthorityAccess() makes every ContentResolver sync API a silent no-op at targetSdk ≥ 34 — no exception, no log, and it passes on a Robolectric shadow. Manifest: INTERNET, READ_SYNC_SETTINGS, WRITE_SYNC_SETTINGS, ACCESS_NETWORK_STATE; the authenticator <service> exported and guarded by android.permission.ACCOUNT_MANAGER (ACCOUNT_AUTHENTICATOR does not exist). Ship an in-app sync button, since "Sync now" stays greyed out under userVisible="false". Add androidx.work, androidx.hilt:hilt-work and androidx.browser to the catalogue, with the HiltWorkerFactory, the removed default WorkManagerInitializer and an @EarlyEntryPoint for the authenticator.

Also: ship a network-security-config with <certificates src="user"/>, or a user who correctly installs their private CA is still not trusted.

UI in this chunk needs on-device review before it can be called done — the account-add flow is the first screen a self-hoster meets, and per CLAUDE.md nothing UI-facing gets tagged before that review.

Done when: an account can be added against a real Nextcloud and its task lists appear, the account shows in system Settings with a working sync switch, and the discovery trap table is green.


Chunk 3 — engine core

Goal: a full bidirectional sync that is correct but not yet clever — no sync-collection, no scheduling beyond a manual trigger.

Read path. REPORT calendar-query with a VTODO comp-filter and no time-range, matching DAVx5 — which deliberately does not use RFC 6578 for tasks, and omits the time-range because some servers return no tasks without a time at all. Then calendar-multiget in batches, matching returned hrefs against what was asked for, because real servers reply with responses for unrelated URLs. Never request calendar-data inside a REPORT that is not sanctioned to carry it.

Write path. If-None-Match: * on create, If-Match on update and DELETE. Send Prefer: handling=strict to sabre-based servers — the cheapest single fix in the whole audit, because it preserves both our bytes and the ETag. Request Accept-Encoding: identity. Strip W/ and keep a weak flag; if a PUT returns no ETag or a weak one, discard it and re-fetch.

412 means three different things and they get three code paths: on create it means the filename is taken (re-fetch, adopt if the UID matches, else regenerate as a UUID); on update it means the server is newer (decision 2: server wins, report the discard); on update where the resource is gone it is spec-correct and not a conflict at all (HEAD to disambiguate — 404 is delete-vs-edit).

Never write back a body whose ETag does not match the hash of what we downloaded. This is the Nextcloud shared-calendar landmine and it is the single most destructive bug available in this chunk: CalendarObject::get() strips VALARM on read-only shares and reduces CLASS:CONFIDENTIAL to a VEVENT-shaped whitelist that deletes DUE, STATUS, COMPLETED, PERCENT-COMPLETE, PRIORITY and RELATED-TO — while leaving the ETag untouched.

Isolation, in two layers. A failed collection must not fail the account, and — its twin, which the draft omitted — a failed resource must not fail the collection. Per-resource quarantine with a failure counter, not backoff: a single HTTP 400 has halted all calendar sync in DAVx5 for weeks. 507 MUST NOT be auto-retried (RFC 4918 §11.5), and 5xx is not safely retryable either — a contradictory RRULE/EXDATE pair returns 500 from Nextcloud and will do so forever.

Client-side validation before PUT, because sabre returns 415 for things ordinary UI actions produce: DUE before DTSTART, value-type mismatch between them, a present METHOD, multiple UIDs, mixed component types in one resource.

href ≠ UID. Sanitise filenames on vdirsyncer's rule — a–zA–Z0–9_.-+, excluding @ — cap the basename near 200 bytes, fall back to a UUID. DELETE: conditional on If-Match, 404/410 count as success, and a locally deleted resource that was never uploaded is never DELETEd.

Local change detection is is_dirty = 1 OR is_deleted = 1 scoped by list — the columns already exist in the v1 schema. Every downstream insert writes is_dirty = 0 explicitly.

Done when: a task created in Nextcloud web appears locally and vice versa; a concurrent edit resolves server-wins with a visible report; a quarantined resource does not stop its collection.


Chunk 4 — incremental sync and scheduling

Goal: sync-collection as the optimisation on top of chunk 3's baseline, and sync that runs by itself.

Every numbered finding in SYNC.md § RFC 6578 is a test in this chunk. The four that are not optional:

  1. Invalidation has no status code. Ignore the status; match <D:valid-sync-token/> anywhere in the body on any 4xx. Observed in the wild: 403 (sabre), 400 (Google), 409 (Radicale), 412 (Evolution accepts it). Thunderbird accepts only 400 and therefore never recovers from the 403 most of the self-hosted world emits.
  2. Initial sync must not report deletions, so mark-and-sweep is mandatory and initialIncomplete must be persisted atomically with the token — otherwise a resumed partial sync sweeps against an incomplete set and deletes live data.
  3. Persist the token only after the bodies are applied, per page. The RFC's own Appendix B has this backwards. Under WorkManager, process death mid-sync is routine.
  4. A token the server accepts over a change log it already pruned returns 207, zero changes, and no error. RFC 6578 gives no signal for it. The only mitigation is periodic full reconciliation (PROPFIND Depth: 1 + ETag diff) on a slow cadence regardless of the token — so chunk 3's full path is a permanent safety net, not a fallback.

Plus: no DAV:limit (Nextcloud regressed it to a localised HTML error page); 507 on the SELF href is truncation, 507 as the outer status means retry without the limit; an iteration cap and a no-progress guard, because the RFC never requires the token to advance; tokens are opaque and never reused across collections, keyed (accountId, collectionUrl); deletion is <D:status>404</D:status> at <D:response> level, not inside a propstat; a 207 with no <D:sync-token> degrades to PROPFIND rather than throwing; and sync-collection never reports collection property changes, so displayname, colour and read-only status refresh only via PROPFIND on the home set.

Three membership edge cases that each look like a bug: create-then-delete between syncs is reported as removed (the delete handler no-ops on an unseen href); delete-then-recreate at the same URI is reported as changed (re-read the UID from the body); and ACL churn may be reported as removal, so apply a sanity threshold before acting on a large delete batch.

Scheduling has a ceiling. Make sync chunked and resumable — persist the cursor per collection so a killed worker resumes rather than restarts. Periodic sync is a plain PeriodicWorkRequest, no foreground service. "Sync now" from a visible screen uses setExpedited(RUN_AS_NON_EXPEDITED_WORK_REQUEST), and getForegroundInfo is implemented unconditionally — omitting it crashes below API 31 and we support 29. Android 15 forbids starting a dataSync FGS from BOOT_COMPLETED, and we have a boot receiver. Sync hard on app open and on connectivity-regained, and be honest in the UI about cadence: in the rare and restricted buckets network access is off entirely and the worst case is genuinely "once overnight".

Done when: a token invalidation on any of the four status codes recovers; a process kill mid-page loses nothing and duplicates nothing; the periodic worker runs and the manual trigger is instant.


Chunk 5 — hardening, compliance, real servers

Goal: the things that are not features and are not optional.

The server matrix, turned on. Nextcloud, Radicale, Baïkal (on Digest), SOGo, Fastmail, iCloud — each per-server trap from SYNC.md § Server reality as a named test. Two must be explicit because both silently destroy data: round-tripping a task from a shared Nextcloud calendar, and an ETag-unchanged body change on SOGo (its ETag is a row-version counter and the body is regenerated per principal). Also: Nextcloud's per-calendar UID uniqueness (409 no-uid-conflict), its trashbin renaming hrefs to <name>-deleted.ics so delete→recreate→delete returns 403, MKCALENDAR rate-limited 10/hour; SOGo never invalidating a token and using second-granularity ones; Radicale never enforcing supported-calendar-component-set. Feature-detect collection creation via OPTIONS and disable the "new task list" UI where neither MKCALENDAR nor extended MKCOL exists (iCloud is MKCOL-only, Google has neither, Posteo disables it).

Revocation, both directions. On 401: stop that account immediately, mark NEEDS_REAUTH, notify with a deep link into the login flow, and do not retry on a timer — Nextcloud's brute-force protection throttles then 429s per source IP, so a retry loop on a dead app password takes down the user's other Nextcloud clients and looks like we broke their server. Distinguish 401 from 403 from 429/503 (honour Retry-After). On account removal: best-effort DELETE /ocs/v2.php/core/apppassword with OCS-APIRequest: true, or uninstalling never revokes access.

The backup path, which arms itself the day this ships. Post-restore reconciliation must offer to re-attach the account, never prune. Pruning is event-driven off AccountManager's account-removed broadcast, not off "absent from the visible set". Device-verify by restoring onto a fresh device and confirming nothing is deleted — SYNC.md is explicit that excluding the database from backup is the wrong fix, because Auto Backup is currently Local mode's only safety net.

Licence attribution — a plain violation until it exists. MPL-2.0 §3.2(a) (dav4jvm, cert4android) requires telling recipients how to obtain source; §3.4 requires retaining file headers; BSD-3 (ical4j) requires reproducing the copyright notice in binary distributions; Apache-2.0 §4(d) propagates NOTICE. Settings exposes only our own MIT LICENSE today. Note ical4j's POM declares a non-SPDX licence name and a LICENSE URL that 404s, so generators produce empty output — this one is written by hand.

Play compliance. A privacy policy, linked in Console and in the app. File as Collected, not Shared, encrypted in transit — "not collected" is not defensible, because Play defines collection as transmitting off-device irrespective of recipient. Ship a "Remove account and delete local data" action even though the Account Deletion policy does not apply. Do not ship REQUEST_IGNORE_BATTERY_OPTIMIZATIONS in the Play build — generic server sync is not on the acceptable-use list; use ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS.

Minified build. :app runs R8 + resource shrinking in release, and proguard-rules.pro already documents two pruning outages. Verify on the releaseTest build type, on device.

Record the extraction. A short section listing what moves to floret-kit (core-dav, core-sync, Login Flow v2, credential storage, the licence screen) and what stays app-local (the mapper, conflict policy and its UI, storage modes), so the follow-up branch is a file move and not a rediscovery.

Done when: the matrix is green where a server exists to run it against, the attribution screen ships, and a releaseTest build syncs on a real device.


Definition of done, every chunk

  1. It builds: ./gradlew :app:assembleDebug.
  2. Unit tests pass: ./gradlew :app:testDebugUnitTest.
  3. /code-review high on the chunk's diff, with the findings either fixed or answered in the commit message.
  4. One commit per chunk, referencing the chunk number. No release, no tag.

Chunk 2 additionally does not count as done until the account-add UI has had an on-device review with an explicit go-ahead — CLAUDE.md's rule, and this is exactly the case it exists for.

What stops the loop

  • A SYNC.md decision that the evidence overturns. Say so; do not pick the other branch silently.
  • Anything requiring a real server that is not reachable — write the test, mark it skipped, name it in the commit, move on.
  • Anything that would need a device beyond adb install.