Skip to content

fix(sync): stop a pull dropping a saved query whose keyword another query held - #3087

Merged
datlechin merged 1 commit into
mainfrom
fix/sync-favorites-apply-before-token
Sep 23, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/sync-favorites-apply-before-token

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

A saved query pulled from iCloud could be lost for good. Mac A deletes query X (keyword rev) and saves a new query Y with the same keyword. Mac B pulls both in one batch, stores neither, and still reports a successful sync. Y never arrives again, because the pull's change token had already moved past it. Found while investigating #2505.

Root cause

  • SyncCoordinator.applyRemoteChanges applied saved queries and their folders in a detached Task that nothing awaited, and applyPullResult saved the change token and the record cache straight after. A write that failed later had already been acknowledged to CloudKit.
  • Upserts ran before deletes. favorites has a unique index on (keyword, connection_id) that ON CONFLICT(id) DO UPDATE does not cover, so Y's upsert failed while X still held rev (measured with sqlite3: UNIQUE constraint failed: favorites.keyword, favorites.connection_id). Then X was deleted.
  • SQLFavoriteManager.applyRemoteFavorite returned early on a failed write and reported nothing.

Fix

  • The pull builds one RemoteSQLFavoriteBatch, and SQLFavoriteManager.applyRemote applies it in order before anything is acknowledged: favorite deletions, folders, favorites, then folder deletions. It returns .applied, .skipped or .failed.
  • applyPullResult saves the token and the record cache only when every store, saved queries included, stored its part. Otherwise the next pull replays the batch.
  • A keyword clash with a local query that is not being deleted is settled inside one BEGIN IMMEDIATE transaction: the older query (earliest createdAt, then the smaller id) keeps the keyword, the other loses it and is marked dirty so the result syncs. createdAt travels with the record, so every Mac picks the same winner.
  • A remote deletion drops that query's or folder's unsent dirty mark, so it cannot linger.
  • A pull whose batch could not be stored now shows a sync error ("Changes from iCloud could not be saved on this device. They will download again on the next sync.") instead of Synced, and Last Synced is not stamped. Fetch and network errors during a pull are reported as before.
  • docs/features/favorites.mdx says which query keeps a clashing keyword.

Tests

  • SyncCoordinatorSQLFavoritePullTests: a refused favorite holds back the token and cache and reports the pull as not acknowledged; a stored one commits both; a delete and a create with the same keyword in one pull both land; two Macs claiming one keyword leave it with the older query.
  • SQLFavoriteRemoteApplyTests: ordering, outcomes, and remote deletions of a query or a folder removing their dirty marks.
  • RemoteFavoriteKeywordResolverTests: the winner rules, including global queries.
  • SQLFavoriteDeletionSyncTests, SQLFavoriteStorageTests, SQLFavoriteScopeChangeTests, SQLFavoriteFolderScopeTests, SyncChangeTrackerTests, SyncCoordinatorTokenExpiryTests, FavoriteTablesStorageTests, and SyncErrorTests in TableProSyncTests.

Risks

  • A query that loses a keyword clash is pushed as a whole record. If this Mac stays offline while the owning Mac edits that query, the older copy can overwrite the edit when this Mac comes back.
  • If sql_favorites.db cannot be opened or written, every pull that carries a saved query now fails visibly and replays, for every record type, until the store works again. The store still stays unavailable until relaunch after a failed open (existing behaviour, not changed here).

Deliberately not fixed here

An edit made while a sync is running can be reverted by that sync's own echo. Edit query F, a sync starts and pushes it, edit F again during the round trip: the push clears F's dirty flag, the pull brings back the first version and overwrites the second, and the next sync finds F clean, so the second edit is lost everywhere.

A fix was written and taken through three reviews. Each round found new defects in the code the previous round added, so it was removed from this PR. What a correct fix has to handle:

  • SyncChangeTracker.isSuppressed is process-wide. Any dirty mark made off the main actor while a pull applies is dropped.
  • Tags, groups, SSH profiles and credential profiles mark the whole collection dirty on every save, so withholding "dirty" records from a pull also withholds untouched siblings.
  • The dirty check and the write sit on different actors, so a local edit can land between them.
  • Remote deletes and missing records never discard dirty marks, so a gate on dirty ids can block a record forever.
  • Tombstones are a read-modify-write of one UserDefaults array with no lock.
  • A record that is never pushed (a local-only connection's favorite, a rejected record) would never take a pull again.

It needs its own design: task-scoped suppression, per-record edit stamps, per-record dirty marking, and an engine seam so the push, clear and pull sequence can be tested.

@mintlify

mintlify Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 23, 2026, 3:20 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@datlechin
datlechin force-pushed the fix/sync-favorites-apply-before-token branch 2 times, most recently from aaf9ed3 to 2559c00 Compare September 23, 2026 15:50
@datlechin
datlechin merged commit 9d188f2 into main Sep 23, 2026
5 checks passed
@datlechin
datlechin deleted the fix/sync-favorites-apply-before-token branch September 23, 2026 19:19

This branch was successfully deployed

1 active deployment
staging - docs — 2559c00c Deployed Sep 23, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant