From 7347e4a7ee4055b0c7ff87e5ee806ad1e78f0109 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 19:48:51 +0000 Subject: [PATCH 1/2] fix(sync): re-register clips an applied remote change leaves dirty - After a server-wins conflict, send the clip again when its membership rebuild dropped a board with a pending local deletion; the conflict reply now reports any row that still needs sending. - After a push-fed fetch, register the fetched clips the apply left dirty with the engine instead of leaving them for the next explicit start. - Move the still-dirty lookup into SyncOutboundWork so acknowledgement, conflict and fetch paths share it, matching zones by name. - Cover both paths with regression tests and document the behavior. --- CHANGELOG.md | 3 ++ .../Sources/GanchoKit/SyncLocalStore.swift | 4 +- .../GanchoSync/CKSyncEngineAdapter.swift | 42 ++++++++------- .../Sources/GanchoSync/SyncOutboundWork.swift | 28 +++++++++- .../SyncOutboundWorkTests.swift | 54 +++++++++++++++++++ docs/BOARD_DELETION_SYNC.md | 7 ++- docs/releases/v0.10.0.md | 3 +- 7 files changed, 118 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6de15fbc..c417ecf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,9 @@ and release versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0 deletion, instead of letting an incoming copy of the board, or a clip that was still in it, bring it back. The clip's other boards are kept. +- Correct a clip on iCloud right away when an incoming copy of it still lists + a board you deleted, instead of waiting for the next full sync. + - Never let a generated title or a background semantic vector computed for older text land on a clip whose text has since changed. diff --git a/Packages/GanchoKit/Sources/GanchoKit/SyncLocalStore.swift b/Packages/GanchoKit/Sources/GanchoKit/SyncLocalStore.swift index 22048bd3..af1dafae 100644 --- a/Packages/GanchoKit/Sources/GanchoKit/SyncLocalStore.swift +++ b/Packages/GanchoKit/Sources/GanchoKit/SyncLocalStore.swift @@ -119,7 +119,9 @@ public protocol SyncLocalStore: Sendable { /// record so membership rides the clip (the boards extension implements it). func boardIDs(forClip clipID: UUID) async throws -> Set /// Rebuilds a clip's board membership from a synced record, seeding a - /// placeholder board for any id whose metadata hasn't synced yet. + /// placeholder board for any id whose metadata hasn't synced yet. Ids of + /// boards with a pending local deletion are dropped, and dropping one + /// flags the clip for re-upload. func setBoardMembership(clipID: UUID, boardIDs: Set) async throws // Board metadata sync — the board table's mirror of the clip methods above, diff --git a/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift b/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift index ea86306a..df7efe99 100644 --- a/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift +++ b/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift @@ -551,20 +551,13 @@ extension CKSyncEngineAdapter { pendingRecordZoneChanges: work.changes(clipZoneID: zoneID, boardZoneID: boardZoneID)) } - /// Re-adds acknowledged saves whose rows a newer local edit left dirty, - /// reading only the save queues those records can be in. + /// Re-adds saves whose rows the local store still considers dirty after an + /// acknowledgement or an applied fetch. private func requeueStillDirty( - _ acknowledged: Set, into engine: CKSyncEngine + _ recordIDs: Set, into engine: CKSyncEngine ) async throws { - let clips = Set(try await store.pendingUploadIDs()) - let boards = - acknowledged.contains { $0.zoneID == boardZoneID } - ? Set(try await store.pendingBoardUploads().map(\.id)) : [] + let dirty = try await outbound.stillDirty(recordIDs) guard self.engine === engine else { throw CancellationError() } - let dirty = acknowledged.filter { recordID in - guard let id = UUID(uuidString: recordID.recordName) else { return false } - return recordID.zoneID == boardZoneID ? boards.contains(id) : clips.contains(id) - } engine.state.add(pendingRecordZoneChanges: dirty.map { .saveRecord($0) }) } @@ -655,7 +648,7 @@ extension CKSyncEngineAdapter: CKSyncEngineDelegate { case .fetchedDatabaseChanges(let event): await handleFetchedDatabaseChanges(event, syncEngine: syncEngine) case .fetchedRecordZoneChanges(let event): - await handleFetchedRecordZoneChanges(event) + await handleFetchedRecordZoneChanges(event, syncEngine: syncEngine) case .sentRecordZoneChanges(let event): await handleSentRecordZoneChanges(event, syncEngine: syncEngine) case .willFetchChanges, .willSendChanges: @@ -742,13 +735,12 @@ extension CKSyncEngineAdapter: CKSyncEngineDelegate { } private func handleFetchedRecordZoneChanges( - _ event: CKSyncEngine.Event.FetchedRecordZoneChanges + _ event: CKSyncEngine.Event.FetchedRecordZoneChanges, syncEngine: CKSyncEngine ) async { let generation = receiveGeneration + let records = event.modifications.map(\.record) do { - try await applyFetched( - records: event.modifications.map(\.record), - deletions: event.deletions.map(\.recordID)) + try await applyFetched(records: records, deletions: event.deletions.map(\.recordID)) } catch { guard generation == receiveGeneration, !(error is CancellationError) else { return } // The engine may persist its opaque fetch token regardless. The @@ -756,6 +748,20 @@ extension CKSyncEngineAdapter: CKSyncEngineDelegate { receiveHealth.fail(error) emit(.failed(receiveHealth.failure ?? .unknown)) scheduleReceiveRecovery(after: error) + return + } + // An applied clip can come back dirty (its membership rebuild dropped + // a board whose local deletion is still pending). No full re-enqueue + // follows a push-fed fetch, so register those saves here instead of + // leaving them for the next explicit start. + let fetchedClips = Set( + records.compactMap { record -> CKRecord.ID? in + guard record.recordID.zoneID.zoneName == zoneID.zoneName else { return nil } + return UUID(uuidString: record.recordID.recordName).map { recordID(for: $0) } + }) + guard !fetchedClips.isEmpty, !isPaused, !outboundFailures.contains(.reset) else { return } + do { try await requeueStillDirty(fetchedClips, into: syncEngine) } catch { + failOutbound(.pending) } } @@ -876,10 +882,10 @@ extension CKSyncEngineAdapter: CKSyncEngineDelegate { guard let record = failure.error.serverRecord else { throw SyncOutboundFailure.invalidRecord } - let localWon = try await outbound.resolveConflict(record) + let needsSend = try await outbound.resolveConflict(record) outboundFailures.remove(operation) outboundFailures.remove(.acknowledgement(recordID)) - if localWon { + if needsSend { syncEngine.state.add(pendingRecordZoneChanges: [.saveRecord(recordID)]) } } catch { diff --git a/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift b/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift index 3f7c6c0e..b330e5c5 100644 --- a/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift +++ b/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift @@ -105,7 +105,29 @@ struct SyncOutboundWork: Sendable { } } - /// True means a verified local win needs sending again, never a failed apply. + /// The given saves whose local rows are still dirty: a newer local edit + /// behind an acknowledgement, or an applied server copy whose membership + /// rebuild dropped a board with a pending local deletion. Reads only the + /// save queues those records can be in. + func stillDirty(_ recordIDs: Set) async throws -> Set { + let clips = + recordIDs.contains { $0.zoneID.zoneName == clipZone.zoneName } + ? Set(try await store.pendingUploadIDs()) : [] + let boards = + recordIDs.contains { $0.zoneID.zoneName == boardZone.zoneName } + ? Set(try await store.pendingBoardUploads().map(\.id)) : [] + return recordIDs.filter { recordID in + guard let id = UUID(uuidString: recordID.recordName) else { return false } + switch recordID.zoneID.zoneName { + case clipZone.zoneName: return clips.contains(id) + case boardZone.zoneName: return boards.contains(id) + default: return false + } + } + } + + /// True means the record needs sending again — a verified local win, or a + /// server copy that applied but left the row dirty — never a failed apply. func resolveConflict(_ record: CKRecord) async throws -> Bool { if record.recordType == BoardRecordMapper.recordType { guard let board = BoardRecordMapper.decode(record) else { @@ -127,7 +149,9 @@ struct SyncOutboundWork: Sendable { ], boards: [], clipDeletions: [], boardDeletions: []) guard summary.failed == 0 else { throw SyncOutboundFailure.localWrite } - return summary.skippedAsStale > 0 + if summary.skippedAsStale > 0 { return true } + // A plain server win is clean, so this cannot loop. + return try await !stillDirty([record.recordID]).isEmpty } } diff --git a/Packages/GanchoKit/Tests/GanchoSyncTests/SyncOutboundWorkTests.swift b/Packages/GanchoKit/Tests/GanchoSyncTests/SyncOutboundWorkTests.swift index 9d7bda2f..0da6da98 100644 --- a/Packages/GanchoKit/Tests/GanchoSyncTests/SyncOutboundWorkTests.swift +++ b/Packages/GanchoKit/Tests/GanchoSyncTests/SyncOutboundWorkTests.swift @@ -467,4 +467,58 @@ extension SyncOutboundWorkTests { try await fixture.work.acknowledge(record) #expect(try await !fixture.store.pendingBoardUploads().map(\.id).contains(board.id)) } + + @Test("A server-wins conflict that drops a pending-deleted board is sent again") + func serverWinThatStaysDirty() async throws { + let fixture = try Fixture() + defer { fixture.clean() } + let item = try await fixture.insert() + let board = try await fixture.store.createPinboard(name: "synthetic board") + try await fixture.store.assign(clipID: item.id, toBoard: board.id) + try await fixture.store.deletePinboardForSync(id: board.id) + // A newer server copy from another device still names the deleted board. + var newer = item + newer.updatedAt = Date().addingTimeInterval(60) + let server = try #require( + ClipRecordMapper.record( + for: newer, content: .text("synthetic"), systemFields: nil, + zoneID: fixture.work.clipZone, boardIDs: [board.id])) + + #expect(try await fixture.work.resolveConflict(server)) + #expect(try await fixture.store.boardIDs(forClip: item.id).isEmpty) + #expect(try await fixture.store.pendingUploadIDs() == [item.id]) + let replacement = try #require( + try await fixture.work.prepare([server.recordID])[server.recordID]) + #expect(ClipRecordMapper.boardIDs(from: replacement).isEmpty) + } + + @Test("Only fetched clips the apply left dirty are registered for sending") + func fetchedClipsLeftDirty() async throws { + let fixture = try Fixture() + defer { fixture.clean() } + let deleted = try await fixture.store.createPinboard(name: "synthetic deleted") + try await fixture.store.deletePinboardForSync(id: deleted.id) + let kept = try await fixture.store.createPinboard(name: "synthetic kept") + try await fixture.store.markBoardUploaded(id: kept.id, systemFields: Data([1])) + let stale = ClipItem(preview: "synthetic", contentHash: UUID().uuidString) + let clean = ClipItem(preview: "synthetic", contentHash: UUID().uuidString) + let staleRecord = try #require( + ClipRecordMapper.record( + for: stale, content: .text("synthetic"), systemFields: nil, + zoneID: fixture.work.clipZone, boardIDs: [deleted.id])) + let cleanRecord = try #require( + ClipRecordMapper.record( + for: clean, content: .text("synthetic"), systemFields: nil, + zoneID: fixture.work.clipZone, boardIDs: [kept.id])) + let adapter = CKSyncEngineAdapter( + store: fixture.store, containerIdentifier: "iCloud.test.gancho", + stateStore: .init(load: { nil }, save: { _ in })) + + try await adapter.applyFetched(records: [staleRecord, cleanRecord], deletions: []) + + let keptID = CKRecord.ID(recordName: kept.id.uuidString, zoneID: fixture.work.boardZone) + let dirty = try await fixture.work.stillDirty( + [staleRecord.recordID, cleanRecord.recordID, keptID]) + #expect(dirty == [staleRecord.recordID]) + } } diff --git a/docs/BOARD_DELETION_SYNC.md b/docs/BOARD_DELETION_SYNC.md index e903825b..4c459449 100644 --- a/docs/BOARD_DELETION_SYNC.md +++ b/docs/BOARD_DELETION_SYNC.md @@ -11,7 +11,10 @@ Board metadata is skipped, and membership rebuilds exclude only the deleted board IDs. Other memberships, including placeholders for genuinely unknown boards, continue to apply. When a rebuild drops a deleted board ID, the clip is re-queued for upload with its received revision, so the server copy stops -naming the deleted board. A fetched page counts rejected board metadata as +naming the deleted board. The CloudKit adapter registers that upload with the +engine as soon as the clip is applied — after a push-fed fetch and after a +server-wins conflict reply — rather than leaving it for the next explicit +start; a clip that applied cleanly is not re-sent. A fetched page counts rejected board metadata as `skippedAsStale`; the clip itself can still apply normally. The public sync protocol and server-wins policy for ordinary board metadata are @@ -22,3 +25,5 @@ or retroactively remove boards resurrected before the correction. `BoardTombstoneTests` covers metadata, membership-only arrivals, combined pages, preserved unrelated memberships, and the acknowledgement boundary. +`SyncOutboundWorkTests` covers the re-send after a server-wins conflict and +which fetched clips are registered again. diff --git a/docs/releases/v0.10.0.md b/docs/releases/v0.10.0.md index b1feb9a6..1e22f761 100644 --- a/docs/releases/v0.10.0.md +++ b/docs/releases/v0.10.0.md @@ -32,7 +32,8 @@ away. An app-wide review also tightens privacy, sync and keyboard handling. while sync is off no longer return, images and semantic vectors are removed with their clip, and the `gancho` CLI refuses to save or copy secrets unless you ask it to. A board you delete stays deleted while sync is still - sending the deletion, and credit cards are detected inside mixed text. + sending the deletion, clips that still list it are corrected on iCloud right + away, and credit cards are detected inside mixed text. - **Safer storage, backups and licensing.** A failed backup export never damages the previous backup, a malformed Keychain key fails safely instead of being replaced, store recovery sets aside the whole database generation From 21835f61b15a7eab82b40a354d5edc81799aaa9f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 19:59:45 +0000 Subject: [PATCH 2/2] fix(sync): harden re-registration of clips left dirty by a remote apply - Register dirty fetched clips even when only part of a push-fed page applied, then report the receive failure as before. - Treat an engine replaced mid-requeue as superseded work rather than a sync failure. - Return still-dirty saves with this device's own zone IDs, the form batch preparation accepts. - Read the upload queue after a server-wins conflict only when the membership rebuild dropped a board, and re-send if that follow-up read fails after the apply committed. --- .../GanchoSync/CKSyncEngineAdapter.swift | 38 ++++++++++-------- .../Sources/GanchoSync/SyncOutboundWork.swift | 40 +++++++++++++------ docs/BOARD_DELETION_SYNC.md | 9 +++-- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift b/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift index df7efe99..e3cc3d01 100644 --- a/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift +++ b/Packages/GanchoKit/Sources/GanchoSync/CKSyncEngineAdapter.swift @@ -552,12 +552,14 @@ extension CKSyncEngineAdapter { } /// Re-adds saves whose rows the local store still considers dirty after an - /// acknowledgement or an applied fetch. + /// acknowledgement or an applied fetch. A replaced engine is not a failure: + /// the next start re-enqueues every dirty row into its successor. private func requeueStillDirty( _ recordIDs: Set, into engine: CKSyncEngine ) async throws { + guard !recordIDs.isEmpty, self.engine === engine else { return } let dirty = try await outbound.stillDirty(recordIDs) - guard self.engine === engine else { throw CancellationError() } + guard self.engine === engine else { return } engine.state.add(pendingRecordZoneChanges: dirty.map { .saveRecord($0) }) } @@ -739,30 +741,32 @@ extension CKSyncEngineAdapter: CKSyncEngineDelegate { ) async { let generation = receiveGeneration let records = event.modifications.map(\.record) + var applyError: (any Error)? do { try await applyFetched(records: records, deletions: event.deletions.map(\.recordID)) } catch { - guard generation == receiveGeneration, !(error is CancellationError) else { return } - // The engine may persist its opaque fetch token regardless. The - // independent poll checkpoint has NOT moved and can replay it. - receiveHealth.fail(error) - emit(.failed(receiveHealth.failure ?? .unknown)) - scheduleReceiveRecovery(after: error) - return + applyError = error } + guard generation == receiveGeneration, !(applyError is CancellationError) else { return } // An applied clip can come back dirty (its membership rebuild dropped // a board whose local deletion is still pending). No full re-enqueue // follows a push-fed fetch, so register those saves here instead of - // leaving them for the next explicit start. + // leaving them for the next explicit start — also when only part of + // the page applied, since its other changes have already committed. + let clipZoneName = zoneID.zoneName let fetchedClips = Set( - records.compactMap { record -> CKRecord.ID? in - guard record.recordID.zoneID.zoneName == zoneID.zoneName else { return nil } - return UUID(uuidString: record.recordID.recordName).map { recordID(for: $0) } - }) - guard !fetchedClips.isEmpty, !isPaused, !outboundFailures.contains(.reset) else { return } - do { try await requeueStillDirty(fetchedClips, into: syncEngine) } catch { - failOutbound(.pending) + records.map(\.recordID).filter { $0.zoneID.zoneName == clipZoneName }) + if !isPaused, !outboundFailures.contains(.reset) { + do { try await requeueStillDirty(fetchedClips, into: syncEngine) } catch { + failOutbound(.pending) + } } + guard let applyError, generation == receiveGeneration else { return } + // The engine may persist its opaque fetch token regardless. The + // independent poll checkpoint has NOT moved and can replay it. + receiveHealth.fail(applyError) + emit(.failed(receiveHealth.failure ?? .unknown)) + scheduleReceiveRecovery(after: applyError) } /// Applies a batch of fetched changes to the local store — shared by the diff --git a/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift b/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift index b330e5c5..996717c9 100644 --- a/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift +++ b/Packages/GanchoKit/Sources/GanchoSync/SyncOutboundWork.swift @@ -108,7 +108,8 @@ struct SyncOutboundWork: Sendable { /// The given saves whose local rows are still dirty: a newer local edit /// behind an acknowledgement, or an applied server copy whose membership /// rebuild dropped a board with a pending local deletion. Reads only the - /// save queues those records can be in. + /// save queues those records can be in. Zones match by name and the result + /// carries this device's own zone IDs, the form `prepare` accepts. func stillDirty(_ recordIDs: Set) async throws -> Set { let clips = recordIDs.contains { $0.zoneID.zoneName == clipZone.zoneName } @@ -116,14 +117,17 @@ struct SyncOutboundWork: Sendable { let boards = recordIDs.contains { $0.zoneID.zoneName == boardZone.zoneName } ? Set(try await store.pendingBoardUploads().map(\.id)) : [] - return recordIDs.filter { recordID in - guard let id = UUID(uuidString: recordID.recordName) else { return false } - switch recordID.zoneID.zoneName { - case clipZone.zoneName: return clips.contains(id) - case boardZone.zoneName: return boards.contains(id) - default: return false - } - } + return Set( + recordIDs.compactMap { recordID -> CKRecord.ID? in + guard let id = UUID(uuidString: recordID.recordName) else { return nil } + switch recordID.zoneID.zoneName { + case clipZone.zoneName where clips.contains(id): + return CKRecord.ID(recordName: id.uuidString, zoneID: clipZone) + case boardZone.zoneName where boards.contains(id): + return CKRecord.ID(recordName: id.uuidString, zoneID: boardZone) + default: return nil + } + }) } /// True means the record needs sending again — a verified local win, or a @@ -140,18 +144,30 @@ struct SyncOutboundWork: Sendable { guard let decoded = ClipRecordMapper.decode(record) else { throw SyncOutboundFailure.invalidRecord } + let boardIDs = Set(ClipRecordMapper.boardIDs(from: record)) let summary = try await store.applyRemoteChanges( clips: [ .init( item: decoded.item, content: decoded.content, systemFields: ClipRecordMapper.encodeSystemFields(record), - boardIDs: Set(ClipRecordMapper.boardIDs(from: record))) + boardIDs: boardIDs) ], boards: [], clipDeletions: [], boardDeletions: []) guard summary.failed == 0 else { throw SyncOutboundFailure.localWrite } if summary.skippedAsStale > 0 { return true } - // A plain server win is clean, so this cannot loop. - return try await !stillDirty([record.recordID]).isEmpty + // A plain server win is clean, so this cannot loop. Only a membership + // rebuild that dropped a board re-flags the applied row, so the queue + // is read just then: a resync full of server wins must not scan it per + // record. The apply has committed, so an unreadable state re-sends + // instead of reporting a failed apply; preparation skips a clean row. + do { + guard try await store.boardIDs(forClip: decoded.item.id) != boardIDs else { + return false + } + return try await !stillDirty([record.recordID]).isEmpty + } catch { + return true + } } } diff --git a/docs/BOARD_DELETION_SYNC.md b/docs/BOARD_DELETION_SYNC.md index 4c459449..afd9f624 100644 --- a/docs/BOARD_DELETION_SYNC.md +++ b/docs/BOARD_DELETION_SYNC.md @@ -12,10 +12,11 @@ board IDs. Other memberships, including placeholders for genuinely unknown boards, continue to apply. When a rebuild drops a deleted board ID, the clip is re-queued for upload with its received revision, so the server copy stops naming the deleted board. The CloudKit adapter registers that upload with the -engine as soon as the clip is applied — after a push-fed fetch and after a -server-wins conflict reply — rather than leaving it for the next explicit -start; a clip that applied cleanly is not re-sent. A fetched page counts rejected board metadata as -`skippedAsStale`; the clip itself can still apply normally. +engine as soon as the clip is applied — after a push-fed fetch (including a +page that only partly applied) and after a server-wins conflict reply — rather +than leaving it for the next explicit start; a clip that applied cleanly is not +re-sent. A fetched page counts rejected board metadata as `skippedAsStale`; the +clip itself can still apply normally. The public sync protocol and server-wins policy for ordinary board metadata are unchanged. No schema or permanent deletion history is added. After the adapter