feat(sync): Google Drive sync, hardened and shipped as 2.6.50 - #160
Merged
Conversation
Attachments outside sync: - AttachmentStorage.resolve accepted only file:// URLs while the editor writes editorjs://, so every reference failed to resolve. AttachmentCleaner then read the empty expected set as "everything here is an orphan" and deleted every attachment of a note on the next save. Parsing moves to AttachmentUrl, a pure type covered by JVM tests, which accepts the canonical editorjs:// form and legacy file:// input; cleanup now aborts rather than deleting whenever a reference cannot be resolved. Sync restore writes canonical URLs, so restored attachments render in the editor. Resumable upload: - The chunk replay loop never terminated once a partially acknowledged chunk was completed by a retry, and replayed its buffer at offsets past the end of the file. Rewritten around absolute offsets, rejecting acknowledgements that move backwards, exceed the declared size, cover bytes never sent, or stop making progress; chunk PUTs retry through DriveRequestExecutor. - A zero-byte attachment issued no PUT and reported success without creating anything, which then failed every later sync. It takes an explicit zero-length multipart path. Preferences: - Resolving a preferences conflict applied nothing yet still marked the conflict resolved, so the rejected value won the next sync. It now runs through the pending-preferences journal and marks the conflict resolved only after a durable commit. - setListPreferences issued eleven independent apply() calls and the journal was cleared before they were durable. One editor plus commit(), and the journal is cleared only on success. The journal carries target and baseline digests so replay can tell "already applied" from "still pending" from "the user has since changed these settings"; an unreadable payload is quarantined instead of disabling sync permanently. Conflicts: - Unresolved losing versions existed only in one device's local table, so publishing a merged descendant made them unreachable. Bundles now carry unresolved alternatives and the resolutions that retire them, and a device starting from an empty database recovers both the winner and the alternative. - Provenance is recorded per side and resolution addresses versions (KEEP_WINNER / KEEP_ALTERNATIVE) rather than endpoints, so a conflict between two Drive heads is no longer shown or applied as "this device". - Publishing requires the read context it was derived from instead of taking causal parents from a mutable field. Drive reads: - A missing ancestor bundle is tolerated rather than fatal, since every bundle is a complete checkpoint of the state its descendants inherit. - An attachment is read and verified once per sync instead of two or three times. Also prunes the sync-attachment cache of blobs nothing references, drops the unused ACTION_VIEW filter from the non-exported TrashActivity so the release lint gate passes, and stops .gitignore un-ignoring the whole docs tree. Schema 20 adds journal identity and quarantine plus per-side conflict provenance and version identities.
…ncies npm audit --audit-level=high has been failing the editor CI job independently of any source change, on two advisories in build-time tooling: - browserslist <=4.28.6: unbounded memory growth and a prototype write via untrusted browserslist-stats.json (GHSA-c83g-rgw3-j3cx, GHSA-73wf-gq98-2v4g) - postcss-selector-parser: denial of service through uncontrolled AST recursion (GHSA-w9m9-85wc-3x92) Both are resolved by npm audit fix, which stays within the declared semver ranges: patch and minor bumps of browserslist, postcss-selector-parser and their transitive data packages. Neither ships in the editor bundle. Rebuilding the editor reproduces byte-identical assets, so nothing under app/src/main/assets/editor changes.
…found in review An adversarial review of the sync stack surfaced defects that unit tests could not see because nothing exercised a Room-built record against its own decoded round trip. - attachmentNames was keyed by logical attachment id locally and by SHA-256 after decoding, and a decoded payload also kept the wire-only attachmentIds. A record therefore never hashed equal to itself across a round trip, so every note with an attachment reported a conflict against itself on every sync, forever, and republished a bundle each time. Both sides now produce the same shape, with a round-trip hash test pinning it. - applySnapshot wrote the merged result over records that had moved on locally while the sync was in flight. The snapshot is built before Drive is read and every blob transferred, and the six-hourly worker runs while the user is in the editor, so an edit made in that window was silently dropped. A record newer than the result being applied is now left alone for the next sync. - Conflicts reported by the backend named the winner of the remote fold, which is not necessarily the version the sync applies, so "keep the version the merge selected" could revert a record to a rejected version and republish it. Conflicts are re-pointed at the version actually applied. - A non-canonical logical attachment id passed the local check and then threw inside the manifest, failing every publish for the account while that note existed. - A corrupt remote blob with a valid local copy failed every sync permanently. Blobs are content-addressed and duplicates are tolerated, so the good local copy is published and the account repairs itself. - A missing blob behind an unresolved alternative stopped every device from syncing anything, including devices that could never resolve it. Pinning is best effort; resolution still verifies before it applies. - Recovery did not record the preferences digest on the already-applied path, letting an unchanged copy outrank a genuine edit from another device. - The settled-version set grew for the life of the account and, past the schema record limit, made every publish fail with no way out. It is now bounded. - A crash between the durable preference write and its bookkeeping left the user's choice applied but unversioned and the conflict pending, so the next sync could put the rejected version back. The journal carries the conflict it settles and recovery finishes the job. - Restored notes kept the sending device's URLs inside valueJson, so a received rich note showed its attachments as broken. The editor blocks are re-pointed positionally, and left untouched when they do not line up. Schema 21 carries the journal's conflict bookkeeping. It is a new version rather than an edit to 20, which has already been published on this branch and would fail Room's identity check on any device already running it.
… directory A whole-app review against v2.6.46, the last version users actually run, found these in the local backup path rather than in sync. - Restore extracted archive entries with new File(filesDir, entry.getName()) and only checked that the name started with "attachments". A backup file is untrusted input, and "attachments/../../databases/notes" satisfies that, so an edited archive could write anywhere the app can. Entries are resolved and must land inside the attachment directory. - Restore inserts rather than replaces, so a backup from another device cannot destroy an unrelated note that happens to share a row id. The cost was that restoring a backup onto the library it came from duplicated every note and tag, while the dialog promises duplicates will be ignored. A row whose id is taken by an identical note is now skipped, so re-restoring is a no-op again, and a genuinely different note under that id is still kept alongside rather than overwriting. Tags are matched by name, which is how a note references them and where a duplicate row is indistinguishable to the user. - When a colliding note was inserted under a new id its attachments stayed in the folder named after the old one, so two notes shared a directory and saving the older one deleted the restored note's files as orphans. The files are copied into the new note's folder and both the attachments column and the editor blocks are re-pointed. Copied rather than moved, so a failure part-way leaves the pre-restore state intact. - commitAll threw on a null string value, where the previous per-key write removed the key. Because restore chains the preference write ahead of the notes and tags, a backup carrying an explicit null aborted the entire restore and left nothing written. - Gson maps the editor and backup models by field name, but only data.model was kept from shrinking. A renamed field there does not fail loudly, it deserializes to null: attachments stop resolving and a restore produces empty notes. Verified against the release mapping that the classes stay unrenamed.
…he emulator - The instrumentation job failed with "Timeout waiting for emulator to boot". reactivecircus/android-emulator-runner needs the KVM udev rule on GitHub-hosted Linux runners; without it the x86_64 emulator runs unaccelerated and boot times out before a single test runs. It had passed on earlier commits, so this was flaky rather than broken, and the rule makes it deterministic. - Coverage was uploaded as a downloadable artifact, which nobody opens during review. A coverage job now merges the unit and instrumentation reports and posts a single comment showing overall coverage and the coverage of the files the pull request changes, updating it on each push. - Instrumentation coverage is collected at all, which is what makes those numbers honest: the Room store, the DAOs and the preference adapters are only reachable on a device, so the unit-only report showed them as untested. It reported RoomSyncStore at 0% while twenty on-device tests exercised it. Measured with both reports: data/sync 58.1% to 72.4%, extendedEditor/attach 52.6% to 67.2%, data/database 0% to 38.8%.
Settings arriving with a sync were stored correctly but stayed invisible until the user navigated away and came back. Two separate reasons, both outside the sync code: - Light/dark is owned by AppCompatDelegate. Refreshing the preference caches (ThemePreferencesCache.refresh) only reloads the values; nothing called applyCurrentThemeMode, so the mode kept whatever the process started with. - Theme, dynamic colour and UI scale are read when an activity is created (BaseActivity), so the screen already on display never picked them up. The app does have a path for this, but it is wired to the settings screen's activity result, which a sync never goes through. - commitListPreferences now applies the theme mode after refreshing the caches, posted to the main thread because it runs on a background thread for both a sync apply and a backup restore. - RoomSyncStore distinguishes writing the same values from actually changing them, by comparing the digest either side of the write, and reports it once. - BackupActivity redraws itself after a successful sync that changed settings. Deliberately not redrawn while conflicts are pending: recreating the activity would dismiss the dialog the user is choosing a version in, and the values are stored either way, so they still take effect on the next screen. Identical values do not redraw either, which would otherwise flicker on every sync.
Variant A of the redesign. The dialog rendered both versions into a single message string, which left the one decision it exists for unsupportable: - the two versions were separated only by a newline, so comparing them meant reading the text - winnerUpdatedAt and loserUpdatedAt were already on the row but never shown, and "which one is mine" is usually answered by the time - each side was cut to 120 characters from the start, so a difference past that point never reached the screen at all - Later, Keep version 1 and Keep version 2 sat as three buttons of equal weight, though two of them discard someone's version for good Each version is now its own selectable card carrying its true origin, its timestamp, a "newer" marker, and a preview with the differing span emphasised. The confirm button acts on the selection, so there is one destructive action instead of two, and Later stops competing with them. The preview window is centred on the difference rather than cut from the head, which is what kept a change near the end of a note off screen. The decision logic moved out of the Activity into SyncConflictPresentation: which text to show, where the two versions diverge, which side is newer and what a version even is (text, deletion, settings, untitled). It has no android.* dependency, so all of it is unit-tested — including that a Drive-vs-Drive conflict never claims either side came from this device. The winner starts selected, so tapping through without reading changes nothing.
Found by running a real two-device sync against Google Drive rather than the fake server: a second sync with nothing changed in between reported a conflict for every note, both sides showing identical text and identical timestamps. Two asymmetries between a locally built record and the same record decoded back from a bundle, either of which is enough to make the canonical hashes differ: - addAttachmentMetadata returned early only when the attachments column was null or blank. The editor stores "[]" for a note that simply has none, so the loop ran zero times and still wrote attachmentsManifest, attachmentHashes and attachmentNames as empty. A decoded record carries no attachment fields at all. Empty ones are no longer written. - normalizeNoteAttachmentFields removed attachmentsManifest and attachmentHashes before rebuilding them, but not attachmentNames, which it only re-added when non-empty. A payload that already carried the key kept it on the wire, so the decoded record differed from the local one that produced it. It is now cleared like the other two. The effect was a conflict per note on every sync, forever, with a fresh bundle republished each time — for any note without an attachment, which is most of them. An existing test asserted attachmentHashes was an empty array for a note with no attachments, pinning the first half of this in place; it now asserts the fields are absent. Added round-trip tests that encode a locally built record and require the decoded one to hash identically, at the codec level and end to end from the Room store on a device.
Reproduced on a device: a backup holding notes with ids 1 and 2, restored onto a library where id 1 is taken, ended with one of the two gone and no error. addNotes and addTags are REPLACE inserts. A row whose id is already taken has its id cleared so it is inserted as new, but that happened in the same batch as rows keeping an explicit id, so the reassigned row was handed the next autoincrement value — which was exactly the id a later row in the same batch then claimed. The REPLACE overwrote it, silently. Rows are now inserted in two groups, the ones keeping their id first, which leaves the autoincrement counter past every explicit id in the batch. Restoring onto an empty library still preserves every id exactly, and each row's attachment relocation and metadata are settled per group. Verified on the device that the same restore now keeps all three notes: the local one untouched, the free id preserved, the reassigned one placed after it.
setupEdgeToEdgeInsets pads the root by systemBars.top and returns the insets unconsumed, so CoordinatorLayout went on dispatching them to its children. Both layouts also set fitsSystemWindows on their AppBarLayout, which applied the same top inset a second time — 118px of status bar became 236px, and the dead strip above the toolbar read as an empty second app bar. Every other screen carries fitsSystemWindows on the root alone; these two now match. Verified on device: the toolbar starts at the status bar edge (118) instead of below a blank band (236).
Sign-in runs through Credential Manager and the Drive scope is granted by the authorization client, so both sit on Play services. Nothing checked whether they were there: on a device without them every control on the account tab led to a failure the user could not act on. The check reads the package table rather than asking GoogleApiAvailability. That needs no dependency and, unlike loading a Play services class, cannot fail for the reason it is checking. The manifest declares the package under <queries> because API 30+ package visibility would otherwise report it absent on every device. isConfigured() already gates create(), so an unavailable device takes the path a build without google-services.json takes. That path hid both groups and left the tab blank, which reads as a broken screen; it now shows what is wrong, translated into all eleven locales. GoogleDriveSyncWorker bails on the same check and moves its Firebase calls inside the try. A scheduled worker that throws is rerun and takes the process down in the background, where nobody can see why. Verified on device: with Play services present the account tab and a real sync are unchanged; with the lookup pointed at an absent package the notice renders and the backup and import tabs keep working. 248 unit tests, 60 instrumentation tests, 0 failures; lint and R8 clean.
pasichDev
changed the title
2.6.47, 2.6.48 and 2.6.49 were tagged but never published, so no user is on them and no changelog entry needs to describe the path through them. The three entries collapse into one written for the only update anyone will actually make, 2.6.46 to 2.6.50 — and the 2.6.49 entry claimed a staged rollout had completed for users who never received it. versionCode moves to 50 because 49 is already taken by the v2.6.49 tag, which points at a commit this branch builds on.
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
1 participant
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.
What this is
Everything between the last release users actually received (2.6.46) and now: Google Drive sync, the hardening that made it safe to ship, and the release metadata to match.
2.6.47, 2.6.48 and 2.6.49 were tagged but never published. Nobody is running them, so they are not a migration path anyone has to survive — this ships as 2.6.50 and the changelog collapses those three entries into one, written for someone updating from 2.6.46.
Why it is this large
The PR started as transport hardening. An adversarial review of the sync implementation against the release invariants found four ways a user could lose data, none of which were caught by the existing tests, and three more were found only by running the app on a real device. Fixing them is most of the diff.
Data loss
AttachmentCleanerdeleted anything it could not match to a reference, including references it simply failed to parse. It now returns a typed result and aborts every deletion if a single reference does not resolve — the safe direction is to keep an orphan, not to delete a file in use.commit()rather than a sequence ofapply()calls that a crash could tear.Conflicts
Resolution was endpoint-addressed (
KEEP_LOCAL/KEEP_DRIVE), which is meaningless when both versions came from Drive. It is now version-addressed (KEEP_WINNER/KEEP_ALTERNATIVE), each side carries its own provenance, and unresolved alternatives are durable across devices instead of living only on the device that saw the conflict.Two bugs made the app conflict with itself: empty
attachmentsManifestarrays meant every attachment-free note differed from its own snapshot on the next sync, andattachmentNamesleaked to the wire after normalization. An existing test had pinned the first one in place by asserting the empty array; it was rewritten to assert absence.Interface
GoogleApiAvailability, which would mean loading a Play services class to ask whether Play services exist.AppBarLayoutapplied it again.Schema
Versions 18 through 21, each with a migration and a
MigrationTestHelpertest. Version 20 had been modified after it was pushed, which would have crashed any device already on it; 20.json was restored and a 20→21 migration added instead.Verification
user_version 21; restore idempotent; an attachment survives cleanup, completes a full sync round trip and renders; preferences conflict resolves with the journal cleared; repeat syncs reportconflictCount: 0; tombstones honoured; a minified release build restores a backup correctly. The GMS-absent branch was verified by pointing the lookup at an absent package — the notice renders and backup and import keep working.Known limitations