fix(sync): keep Local only connections' favorites, saved queries and layouts off iCloud

[datlechin]

Stacked on #3201 (fix/favorite-tables-storage). Review and merge that first; this branch's base is its head commit. Found while implementing #3167 (PR #3189, table folders).

What was wrong

Marking a connection Local only (#3178) kept the connection record off iCloud and nothing else. Everything that belongs to the connection still went up:

• Table favorites: collectTableFavorites pushed every dirty entry, whatever its connection.
• Saved queries and their folders: the push read every row of the SQLite store, connection_id included, and filtered nothing.
• Column layouts: the settings category embeds the connection id, and collectSettings pushed every dirty category.
• Database favorites: saves were held by an ad hoc filter, but their deletions were pushed. Removing a database favorite of a Local only connection deleted it on every Mac where that connection still syncs.

Sample connections leaked the same way, and the connection push only checked localOnly, so enableSync pushed the sample connection record too.

Two remote deletions were dropped on the floor: SyncPendingDeletions.parse answered case .favoriteDatabase, .settings: return, so a database favorite or a column layout removed on another Mac stayed here. That dates from #2184; FavoriteDatabasesStorage.removeFavoriteWithoutSync(id:) was written for it and only a test called it. Column layout deletions were tombstoned locally but never pushed either: collectSettings never looked at .settings tombstones, so they sat there until the 30-day prune.

What changed

SyncBoundary (Core/Sync/SyncBoundary.swift) is the one answer to "does this record go to iCloud". It is built each cycle from SyncSettings.syncs(_:) (#3179's exhaustive switch; there is no second one), SyncRecordType.verifiedInProduction and the connection store:

• includes(type): the category syncs and the type is deployed.
• includes(type, owner:): also refuses an owner in excludedConnectionIds (localOnly || isSample, now DatabaseConnection.participatesInSync).
• includes(tombstone, of:): the same question for a stored deletion.

It is direction-agnostic on purpose: lane D reuses it on the pull. Owners: a connection is its own id; table favorites, database favorites, saved queries and folders use their connection id (nil means global, which always passes); a column layout uses its table scope's connection id; every other type has none.

Decision recorded: the brief asked for a boundary that is nil when the connection store cannot be read. This one is never nil; its excludedConnectionIds is nil instead, and then every owned record and owned tombstone is held while unowned ones still go up. One value answers both the type question and the owner question, so the two cannot disagree, and the push does not need settings and a boundary side by side.

Push: push, hold or drop. Each collector returns a SyncPushDisposition per dirty record. An excluded owner means .hold: the mark survives the cycle and goes up when the connection is back in sync. Only a record that cannot be built is dropped. This covers table favorites (both record ids of a favorite whose name holds | or \), database favorites (the ad hoc filter is gone), saved queries, folders and column layouts. The connection record itself keeps its existing rule, decided by its own participatesInSync flag: an excluded connection's own mark is dropped (localOnlyConnectionMarkIsDropped), because every path that puts a connection back in sync marks it dirty.

Owner-held tombstones. Tombstone gained an optional owner (synthesized Codable, so JSON written before this decodes with no owner, and an older build ignores the key). The dependent stores pass it through SyncChangeTracker.markDeleted(_:ids:owner:) or, for a change spanning connections, markDeleted(_:idsByOwner:) (one write, one notification): FavoriteTablesStorage, FavoriteDatabasesStorage, SQLFavoriteManager (the storage now reports each deleted favorite's and folder's scope from the same actor call as the delete), and FileColumnLayoutPersister. A tombstone whose owner is excluded is held. A layout tombstone written before owners existed takes its owner from its own name, so a backlog of old layout deletions cannot leak either.

This reverses the policy stated on collectDatabaseFavorites ("Tombstones are not filtered"). Pushing a Local only connection's deletions removes the favorite on Macs where that connection still syncs, which is a local action leaking out. They are now held and go up if the connection is put back in sync.

Column layout deletions push. .settings tombstones go through the same path as every other type, under the owner rule. A layout tombstone whose category this Mac still holds (cleared and set again under a build that never pushed layout deletions) is retired instead of sent, so the upgrade does not delete live layouts from iCloud. A clear, geometry reset or container drop whose file write fails now changes nothing and records no deletion; before, the tombstone went up while the file kept the layout.

Deleting a Local only or sample connection purges its dependents through a new ConnectionLocalState.Origin.localOnly: no tombstones, dirty marks discarded, and every tombstone still held for that owner is discarded. Without that last step the owner leaves the store, stops being excluded, and every held deletion goes up. deleteConnections splits a mixed selection into the two origins.

Saved queries and folders live in an actor-backed SQLite store, so their purge finishes a moment after the connection has left the connection list. Until it does, the deleted connection's id stays excluded through a small persisted set (SyncMetadataStorage.ownersKeptOffSync), released when the saved-query purge reports success. A push that runs in between, a failed SQLite delete, or a quit in that window therefore still holds the rows instead of uploading them. The set only applies to ids no longer in the connection list, so a connection that exists follows its own flag, and clearAll leaves it alone because it protects local data rather than describing an account.

Pruning now runs at the end of a clean push, with the boundary that push used, and keeps a tombstone that push could not send (pruneTombstones(olderThan:where:)). A deletion held for a Local only connection longer than 30 days survives until it can go up, and a connection put back in sync during the cycle cannot get its held deletion pruned before it is sent.

Remote deletions applied. Database favorites through removeFavoritesWithoutSync(ids:) (the old single-id helper, replaced by the batch form). Column layouts through FileColumnLayoutPersister.removeWithoutSync(storageKeys:). A record name that carries its category resolves directly; a digest-shortened one (#2575) is matched by recomputing the local record names, which is the only case that scans the layout files. A layout file that cannot be written fails the pull (pullNotSaved) and leaves its dirty mark, so the deletion arrives again instead of being acknowledged over a file that still holds the layout. Both respect SyncSettings.syncs(_:) through #3179's gate in parse. They apply regardless of owner here; lane D adds owner gating on the pull side.

One pull-side change, and why it is not a scope skip. A pulled column layout whose category this Mac has tombstoned is no longer applied, which is what every other record type already does. Without it, a pull that arrives before the deletion goes up restores the layout, and the stale-tombstone retirement above then drops the deletion for good. The skipped record is superseded by the local deletion, which is sent (or held and sent later), so advancing the token over it loses nothing.

Deliberately not in this PR

• Nothing on the pull side changes. Remote saves and deletions of a Local only connection's dependents are still applied, and the change token still advances over records a pull skips. Both belong to lane D, stacked on this branch, with the parking and replay that make new pull skips safe.
• Records pushed before this change stay in iCloud. Other Macs keep leaked favorites and layouts until they are removed there.
• Moving a saved query or folder into a Local only connection's scope holds its later edits, but the copy already in iCloud keeps its old scope on other Macs.
• A global saved query inside a folder scoped to a Local only connection goes up and names a folder iCloud does not have; other Macs show it at the root.
• Until lane D lands, a remote edit or a full fetch can replace the local value of a held record. Its mark stays, so the first push after the connection is back in sync sends whatever is local then.
• If the saved-query purge of a deleted Local only connection fails, its rows stay on disk, excluded, with nothing retrying the delete. The other two origins have the same no-retry shape today.
• A global saved query inside a Local only connection's folder moves to the root on this Mac when that connection is deleted, and stays in the folder on Macs that have it.
• Tombstones of a category that stays switched off are kept, not pruned, so they grow with the deletions made while it is off.

For PR #3189 (table folders)

Map .tableFolder and .tableFolderItem in SyncSettings.syncs(_:), use the folder scope's connection id as the owner, replace folderSyncConnections() with syncBoundary(settings:) (its nil-when-unreadable rule is excludedConnectionIds == nil), return .hold from collectTableFolders for an excluded owner, and pass the owner to markDeleted in TableFolderStorage.

Tests

• SyncBoundaryTests: type scope follows category and deployment; Local only and sample owners refused, synced and unknown ones admitted; a deleted connection still being purged stays refused while one that exists again follows its flag; an owner never overrides a category; an unreadable store holds owned records; tombstones by stored owner, and a legacy layout tombstone by its name.
• SyncLocalOnlyDependentsTests, through SyncCoordinator.runSyncCycle on ScriptedSyncTransport:
  • A Local only or sample connection's table and database favorites, saved query, folder and layout are not pushed, and their marks survive.
  • The same for a synced connection go up and clear (control).
  • Their deletions are held; putting the connection back in sync sends the held edits and deletions.
  • An owner-less legacy tombstone still pushes; an unreadable connection store holds owned records and sends a global saved query.
  • A cleared layout sends its deletion; a stale layout tombstone for a live layout is dropped, not sent.
  • Purging a Local only connection leaves no tombstone or mark of any type and pushes nothing; its id stays excluded until the saved-query purge finishes; deleteConnection discards a Local only owner's tombstones and keeps a synced one's.
  • Pruning keeps a 40-day-old held tombstone and drops a pushable one; a held tombstone survives its connection being put back in sync during the push.
  • Remote database favorite and layout deletions apply, a digest-named layout included, and wait while their category is off; an unwritable layout file fails the pull; a pulled layout waiting on its local deletion stays deleted and the deletion goes up.
  • Every dependent store records the connection as the tombstone owner.
• SyncPendingDeletionsTests: database favorite and layout deletions parse, and are withheld by their switches.
• SyncMetadataStorageTests (package): owner round-trip, legacy JSON decodes with no owner, owner removal across types, pruning keeps an unpushable tombstone, owners kept off sync persist and survive clearAll.

Before and after: with the owner check, the two new remote-deletion calls and the .localOnly purge reverted in place (before the review fixes), 14 of the 22 new cases failed (every gating, purge, prune and remote-deletion case); the 8 that passed were the controls and the cases those three reverts do not reach.

Verification

• build: PASS
• test, 25 suites (the new ones plus every suite touching sync, favorites, saved queries, layouts and connection purge): 293 passed, 0 failed
• test, 20 more sync, connection-storage, welcome and grid-layout suites: 232 passed, 0 failed
• package swift test --filter SyncMetadataStorageTests: 22 passed
• lint (all 23 changed Swift files, by file path): 0 violations
• docs: PASS

UI automation: none. The change is in the push and purge paths, with no new UI; the unit suites drive the whole sync cycle through a scripted transport.

Security

A security review of the diff, focused on whether anything a user marked Local only can still reach iCloud, found nothing at confidence 8 or above.

Review

Codex review review-mumxprmw-e9gsgi (4 findings):

• Fixed, P1: deleting a Local only connection left its saved queries behind a fire-and-forget purge with the owner already gone from the boundary. ownersKeptOffSync keeps the id excluded until the purge reports success.
• Fixed, P2: a pull could restore a layout whose deletion was pending, and the stale-tombstone retirement then dropped the deletion. Pulled layouts with a local tombstone are skipped.
• Fixed, P2: a remote layout deletion whose file write failed was acknowledged. It now fails the pull and keeps the mark.
• Dismissed, P1: "held records are not protected on the pull". Pull gating, and never overwriting a locally dirty record on pull, are lane D's scope by the brief; this PR adds no new scope skips on the pull. The interim window is listed under "Deliberately not in this PR".

Codex adversarial review review-mumyipiw-kg2cnh stopped on the Codex usage limit before reporting. Its reasoning headlines were followed up as leads; one held: pruning ran after the cycle with a fresh boundary, so a connection put back in sync mid-cycle could have its held, month-old deletion pruned before it was sent. Pruning now runs inside the push with that push's boundary (heldTombstoneSurvivesAScopeChangeMidCycle).

The code-review skill (high) replaced the adversarial pass (10 findings):

• Fixed: discardTombstones(ownedBy:) missed layout tombstones written before owners existed; it now uses the same owner derivation as the boundary.
• Fixed: failed layout writes still recorded a deletion (see above).
• Fixed: a connection store that failed its first read and succeeded its second made the push drop every connection edit. The connection record now decides by its own flag.
• Fixed: markDeleted(_:idsByOwner:) wrote and notified once per owner; now once.
• Fixed: every settings deletion scanned every layout file; only digest names do now.
• Fixed: a helper was inserted between markDetachedDirty and its doc comment; moved. The new SQL delete reuses run(_:bindings:), which is now internal.
• Dismissed: "the .localOnly purge does not push detached saved queries". A global saved query inside a Local only connection's folder moves to the root here; pushing that would change every other Mac, which is what deleting a Local only connection must not do. Listed under "not in this PR".
• Dismissed: "remote database favorite and layout deletions apply to Local only owners". The brief applies them regardless of owner here; lane D gates every deletion by owner.
• Dismissed: "off-category tombstones are never pruned". That is the brief's item 5 (the critique's F7 fix): pruning them is how a deletion made while a category was off got lost.
• Dismissed: "syncIdsByConnection exists in two stores". They group two unrelated entry types by two different id functions; a shared generic helper would need a protocol for three lines.