diff --git a/Catchlight/Database/EncryptedTakeStore.swift b/Catchlight/Database/EncryptedTakeStore.swift index c83d768..9db2b6d 100644 --- a/Catchlight/Database/EncryptedTakeStore.swift +++ b/Catchlight/Database/EncryptedTakeStore.swift @@ -347,53 +347,78 @@ public final class EncryptedTakeStore: TakeStore { // MARK: - TakeStore: Takes public func upsert(_ take: Take) throws { + try queue.sync { try upsertHoldingQueue(take) } + } + + public func applyRemote(_ take: Take) throws -> Bool { try queue.sync { - try exec("BEGIN IMMEDIATE;") - var committed = false - defer { if !committed { try? exec("ROLLBACK;") } } - // Single-Obie invariant under last-write-wins: an incoming Obie - // (e.g. applied from another device by sync) demotes any existing - // one. Without this, the partial unique index rejected the row with - // an opaque writeFailed and the whole upsert rolled back — and the - // in-memory store (no index) silently accepted it, so the two - // implementations diverged on the same input. - if take.isObie { - // Demote any existing Obie. The is_obie COLUMN is only an index — a - // Take's isObie ALSO lives inside its encrypted PAYLOAD, which - // allTakes() decrypts as the source of truth. A column-only UPDATE (the - // previous approach) left the demoted Take's payload still isObie=true, - // so the timeline read TWO Obies and the newly-set one was invisible - // (owner-reported 2026-06-23; pinned by the upsert-demote contract - // test, which the column-only path failed). Re-seal each existing Obie - // with isObie=false so column AND payload agree. Decoding by PAYLOAD - // (not the column) also REPAIRS a store already holding a stray Obie on - // the next Obie set. Mirrors InMemoryTakeStore.upsert. - let sel = try prepare("SELECT id, payload FROM takes WHERE id != ?1;") - defer { sqlite3_finalize(sel) } - bindText(sel, 1, take.id.uuidString) - for var existing in try collectTakes(sel) where existing.isObie { - existing.isObie = false - // Bump modifiedAt so the demotion SYNCS (2026-07-01). pushOutbound - // selects by `modifiedAt > watermark`; without the bump the demoted - // Take was never re-uploaded, the cloud kept a second isObie=true - // blob, and every later pull hit ConflictResolver's (false,false) - // branch — a phantom "changed on another device" conflict. Matches - // setObie. When SYNC applies a remote Obie this can bump a Take the - // fleet already demoted — one redundant, converging re-upload; - // accepted trade-off for never losing the demotion. - existing.modifiedAt = Date() - try insertOrReplace(existing) + // Tombstone check and write INSIDE one queue.sync so a concurrent + // delete (main thread) cannot land between them — the pull loop's + // own guard reads a pull-start snapshot and cannot see a mid-pull + // delete (the delete-resurrection bug, closed 2026-07-23). Deletion + // wins ties (`>=`), matching the pull guard: a remote edit STRICTLY + // after the deletion still resurrects. + let sel = try prepare("SELECT deleted_at FROM tombstones WHERE id = ?1;") + defer { sqlite3_finalize(sel) } + bindText(sel, 1, take.id.uuidString) + if sqlite3_step(sel) == SQLITE_ROW { + guard let deletedAt = ISO8601.date(from: columnText(sel, 0)) else { + throw StorageError.corruptRow("tombstone row has unparseable date") } + if deletedAt >= take.modifiedAt { return false } + } + try upsertHoldingQueue(take) + return true + } + } + + /// The upsert body. NOT serialised — callers hold the queue. + private func upsertHoldingQueue(_ take: Take) throws { + try exec("BEGIN IMMEDIATE;") + var committed = false + defer { if !committed { try? exec("ROLLBACK;") } } + // Single-Obie invariant under last-write-wins: an incoming Obie + // (e.g. applied from another device by sync) demotes any existing + // one. Without this, the partial unique index rejected the row with + // an opaque writeFailed and the whole upsert rolled back — and the + // in-memory store (no index) silently accepted it, so the two + // implementations diverged on the same input. + if take.isObie { + // Demote any existing Obie. The is_obie COLUMN is only an index — a + // Take's isObie ALSO lives inside its encrypted PAYLOAD, which + // allTakes() decrypts as the source of truth. A column-only UPDATE (the + // previous approach) left the demoted Take's payload still isObie=true, + // so the timeline read TWO Obies and the newly-set one was invisible + // (owner-reported 2026-06-23; pinned by the upsert-demote contract + // test, which the column-only path failed). Re-seal each existing Obie + // with isObie=false so column AND payload agree. Decoding by PAYLOAD + // (not the column) also REPAIRS a store already holding a stray Obie on + // the next Obie set. Mirrors InMemoryTakeStore.upsert. + let sel = try prepare("SELECT id, payload FROM takes WHERE id != ?1;") + defer { sqlite3_finalize(sel) } + bindText(sel, 1, take.id.uuidString) + for var existing in try collectTakes(sel) where existing.isObie { + existing.isObie = false + // Bump modifiedAt so the demotion SYNCS (2026-07-01). pushOutbound + // selects by `modifiedAt > watermark`; without the bump the demoted + // Take was never re-uploaded, the cloud kept a second isObie=true + // blob, and every later pull hit ConflictResolver's (false,false) + // branch — a phantom "changed on another device" conflict. Matches + // setObie. When SYNC applies a remote Obie this can bump a Take the + // fleet already demoted — one redundant, converging re-upload; + // accepted trade-off for never losing the demotion. + existing.modifiedAt = Date() + try insertOrReplace(existing) } - try insertOrReplace(take) - // Re-creating an item supersedes any pending tombstone for it. - let ts = try prepare("DELETE FROM tombstones WHERE id = ?1;") - defer { sqlite3_finalize(ts) } - bindText(ts, 1, take.id.uuidString) - guard sqlite3_step(ts) == SQLITE_DONE else { throw StorageError.writeFailed(lastError()) } - try exec("COMMIT;") - committed = true } + try insertOrReplace(take) + // Re-creating an item supersedes any pending tombstone for it. + let ts = try prepare("DELETE FROM tombstones WHERE id = ?1;") + defer { sqlite3_finalize(ts) } + bindText(ts, 1, take.id.uuidString) + guard sqlite3_step(ts) == SQLITE_DONE else { throw StorageError.writeFailed(lastError()) } + try exec("COMMIT;") + committed = true } /// Shared by upsert and migration. NOT serialised — callers hold the queue diff --git a/Catchlight/UI/Views/DailiesView.swift b/Catchlight/UI/Views/DailiesView.swift index 0d2e4e9..9f345e7 100644 --- a/Catchlight/UI/Views/DailiesView.swift +++ b/Catchlight/UI/Views/DailiesView.swift @@ -1270,8 +1270,16 @@ struct DailiesView: View { defer { editDraft = nil; ui.endEditingInPlace() } guard var t = editDraft else { return } t.removeEmptyTextBlocks() + // A tapped-away Take with nothing in it is discarded, NOT saved — and that now + // includes a brand-new Obie (owner 2026-07-20): the Obie widget/intent opens a + // blank Obie draft, and tapping the background with nothing typed used to persist + // an empty pinned Obie. `!t.isObie` was in this test, forcing `isBlank` false for + // any Obie; dropping it treats an Obie like any other Take, matching the locked- + // capture path (`AppModel.saveLockedCapture`), which already discards a blank Obie. + // An EXISTING Obie whose text is cleared is still kept, via the `storedHadContent` + // guard below. let isBlank = t.plainText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty - && !t.isTask && t.timeReminder == nil && !t.isObie + && !t.isTask && t.timeReminder == nil && t.attachments.isEmpty && t.locationReminder == nil let storedCopy = try? vm.store.take(id: t.id) let storedHadContent = (storedCopy?.plainText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty == false) diff --git a/Sources/CatchlightCore/Storage/TakeStore.swift b/Sources/CatchlightCore/Storage/TakeStore.swift index 19c4b23..f71d737 100644 --- a/Sources/CatchlightCore/Storage/TakeStore.swift +++ b/Sources/CatchlightCore/Storage/TakeStore.swift @@ -73,6 +73,32 @@ public protocol TakeStore: AnyObject { /// Remove tombstones once the deletion is durably recorded in the uploaded /// cloud manifest (or applied from a remote tombstone). func purgeTombstones(ids: [UUID]) throws + + /// Apply a Take arriving FROM SYNC unless a live local tombstone supersedes + /// it (`deletedAt >= take.modifiedAt` — deletion wins ties, mirroring the + /// pull loop's resurrection guard; a remote edit STRICTLY after the deletion + /// still resurrects). Returns false when the tombstone wins and NOTHING was + /// written. This exists because the pull loop's own guard reads a tombstone + /// snapshot taken at pull-start — a delete landing mid-pull is invisible to + /// it, and a plain `upsert` would resurrect the Take and clear the fresh + /// tombstone (the delete-resurrection bug, closed 2026-07-23). Stores that + /// serialise their operations MUST check-and-write inside one critical + /// section so a concurrent `delete` cannot interleave. + func applyRemote(_ take: Take) throws -> Bool +} + +public extension TakeStore { + /// Default (read-then-write; race-free only where the caller already is — + /// fine for synchronous test doubles, NOT for concurrent production stores, + /// which must override with an atomic implementation). + func applyRemote(_ take: Take) throws -> Bool { + if let tomb = try tombstones().first(where: { $0.id == take.id }), + tomb.deletedAt >= take.modifiedAt { + return false + } + try upsert(take) + return true + } } /// In-memory `TakeStore` for tests and previews. Not used in production. @@ -110,6 +136,16 @@ public final class InMemoryTakeStore: TakeStore { tombstoneMap[id] = Tombstone(id: id, deletedAt: Date()) } + public func applyRemote(_ take: Take) throws -> Bool { + // Mirrors EncryptedTakeStore.applyRemote so the two implementations stay + // contract-identical for sync-applied rows. + if let tomb = tombstoneMap[take.id], tomb.deletedAt >= take.modifiedAt { + return false + } + try upsert(take) + return true + } + public func tombstones() throws -> [Tombstone] { tombstoneMap.values.sorted { $0.deletedAt < $1.deletedAt } } diff --git a/Sources/CatchlightCore/Sync/SyncEngine.swift b/Sources/CatchlightCore/Sync/SyncEngine.swift index 9a3e485..9183a5c 100644 --- a/Sources/CatchlightCore/Sync/SyncEngine.swift +++ b/Sources/CatchlightCore/Sync/SyncEngine.swift @@ -386,14 +386,17 @@ public final class SyncEngine { } switch ConflictResolver.decide(local: local, remote: remoteTake, lastSync: lastSync) { case .takeRemote(let t): - // (Removed 2026-07-16: a TEMP "REMOVE after repro" trap from the deleted-Take- - // reappears hunt of 2026-07-10 — that bug was CLOSED as non-reproducible and - // PR #128 pulled the traps, but this one was missed. It fired on every sync - // apply, so it would have swamped the breadcrumb budget with dead output, and - // it logged a Take id fragment — the one thing here that leans toward - // identifying content.) - try store.upsert(t) - report.applied.append(t.id) + // RESURRECTION GUARD, part 2 (2026-07-23): the guard above consults + // `pendingTombstoneByID`, a snapshot taken at pull-start — a delete + // committed MID-PULL is invisible to it, and a plain `upsert` here + // would re-create the Take AND clear its fresh tombstone (the + // delete-resurrection bug). `applyRemote` re-checks tombstones + // atomically with the write (the same store critical section a + // concurrent `delete` uses), so the freshest deletion always wins + // ties; a remote edit strictly after it still lands. + if try store.applyRemote(t) { + report.applied.append(t.id) + } case .conflict(let l, let r): report.conflicts.append((local: l, remote: r)) // surfaced; UI resolves case .keepLocal, .noChange: diff --git a/Sources/coreverify/main.swift b/Sources/coreverify/main.swift index 1ae5e77..63d534d 100644 --- a/Sources/coreverify/main.swift +++ b/Sources/coreverify/main.swift @@ -287,6 +287,28 @@ do { let afterDelete = try storeB.take(id: take.id) check("Inbound deletion from another device applied locally", report.deletedLocally == [take.id] && afterDelete == nil) } + // applyRemote — the sync-apply path must not resurrect a Take over a live + // tombstone (delete-resurrection guard, closed 2026-07-23), while a remote + // edit strictly after the deletion still lands (edit-wins). + do { + let store = InMemoryTakeStore() + var t = richTake() + try store.upsert(t) + try store.delete(id: t.id) + let deletedAt = try store.tombstones()[0].deletedAt + + t.modifiedAt = deletedAt // ties go to the deletion + let blocked = try store.applyRemote(t) == false + let stillGone = try store.take(id: t.id) == nil + let tombKept = try store.tombstones().map(\.id) == [t.id] + check("applyRemote blocked by live tombstone (no resurrection, tombstone kept)", blocked && stillGone && tombKept) + + t.modifiedAt = deletedAt.addingTimeInterval(1) + let applied = try store.applyRemote(t) + let restored = try store.take(id: t.id) != nil + let tombCleared = try store.tombstones().isEmpty + check("applyRemote applies a remote edited after the deletion (edit-wins)", applied && restored && tombCleared) + } // Conflict detection + surfaced end-to-end without overwrite. do { let lastSync = Date(timeIntervalSince1970: 1_700_000_000) diff --git a/Tests/CatchlightCoreTests/SyncEngineEdgeCasesTests.swift b/Tests/CatchlightCoreTests/SyncEngineEdgeCasesTests.swift index d321521..f625c7e 100644 --- a/Tests/CatchlightCoreTests/SyncEngineEdgeCasesTests.swift +++ b/Tests/CatchlightCoreTests/SyncEngineEdgeCasesTests.swift @@ -372,6 +372,48 @@ final class SyncEngineEdgeCasesTests: XCTestCase { XCTAssertEqual(try local.take(id: remoteTake.id)?.primaryText, "remote-new") } + // MARK: - Pull: delete lands mid-pull (delete-resurrection race, closed 2026-07-23) + + /// The pull loop snapshots pending tombstones ONCE at pull-start; a user + /// delete committed while the loop is mid-flight is invisible to that + /// snapshot, and a plain `upsert` on the `.takeRemote` branch resurrected + /// the Take and cleared its fresh tombstone. `applyRemote` re-checks + /// tombstones atomically with the write, so the deletion must now win and + /// survive to propagate on the next push. + func testSyncEngine_deleteLandsMidPull_takeStaysDeleted_tombstoneSurvives() throws { + let k = makeKeys() + let cloud = InMemoryCloudFolder() + let t0 = Date(timeIntervalSince1970: 1_700_000_000) + + // Another device pushes the Take at modifiedAt = 200. + let remoteStore = InMemoryTakeStore() + var remoteTake = TestFixtures.richTake() + remoteTake.primaryText = "remote-new" + remoteTake.modifiedAt = t0.addingTimeInterval(200) + try remoteStore.upsert(remoteTake) + try makeEngine(store: remoteStore, cloud: cloud, keys: k, now: { t0.addingTimeInterval(201) }).pushOutbound() + + // Local holds an older, unchanged copy (modifiedAt 100 ≤ watermark 150), + // so the pull decides `.takeRemote`. The wrapper lands the user's delete + // the moment the loop reads this id — after the pull-start snapshot. + let inner = InMemoryTakeStore() + var localTake = remoteTake + localTake.primaryText = "local-old" + localTake.modifiedAt = t0.addingTimeInterval(100) + try inner.upsert(localTake) + inner.setLastSyncDate(t0.addingTimeInterval(150)) + let local = MidPullDeleteStore(wrapping: inner, deleting: localTake.id) + + let report = try makeEngine(store: local, cloud: cloud, keys: k, + now: { t0.addingTimeInterval(300) }).pullInbound() + + XCTAssertTrue(local.deleteFired, "test seam must have exercised the mid-pull delete") + XCTAssertTrue(report.applied.isEmpty, "the mid-pull deletion must win over the remote copy") + XCTAssertNil(try inner.take(id: localTake.id), "the deleted Take must NOT be resurrected") + XCTAssertEqual(try inner.tombstones().map(\.id), [localTake.id], + "the fresh tombstone must survive so the deletion propagates on push") + } + // MARK: - Pull: partial-sync quarantine /// A 5-blob pull where the middle blob is corrupted should quarantine that @@ -593,3 +635,43 @@ final class SyncEngineEdgeCasesTests: XCTestCase { XCTAssertTrue(lock.isStale(now: Date())) } } + +/// Simulates the user deleting a Take MID-PULL: the first time the pull loop +/// reads the target id — which happens AFTER the pull-start tombstone snapshot — +/// the delete lands before the read returns. That is exactly the window the +/// stale snapshot cannot see. Everything else forwards to the wrapped store. +private final class MidPullDeleteStore: TakeStore { + private let wrapped: InMemoryTakeStore + private let targetID: UUID + private(set) var deleteFired = false + + init(wrapping wrapped: InMemoryTakeStore, deleting targetID: UUID) { + self.wrapped = wrapped + self.targetID = targetID + } + + func take(id: UUID) throws -> Take? { + if id == targetID, !deleteFired { + deleteFired = true + try wrapped.delete(id: id) // the user's delete lands here + } + return try wrapped.take(id: id) + } + + func upsert(_ take: Take) throws { try wrapped.upsert(take) } + func delete(id: UUID) throws { try wrapped.delete(id: id) } + func allTakes() throws -> [Take] { try wrapped.allTakes() } + func takesModified(since date: Date?) throws -> [Take] { try wrapped.takesModified(since: date) } + func search(_ query: String) throws -> [Take] { try wrapped.search(query) } + func upsert(_ sequence: CatchlightSequence) throws { try wrapped.upsert(sequence) } + func sequence(id: UUID) throws -> CatchlightSequence? { try wrapped.sequence(id: id) } + func allSequences() throws -> [CatchlightSequence] { try wrapped.allSequences() } + func deleteSequence(id: UUID) throws { try wrapped.deleteSequence(id: id) } + func currentObie() throws -> Take? { try wrapped.currentObie() } + func setObie(id: UUID, replaceExisting: Bool) throws { try wrapped.setObie(id: id, replaceExisting: replaceExisting) } + func lastSyncDate() -> Date? { wrapped.lastSyncDate() } + func setLastSyncDate(_ date: Date) { wrapped.setLastSyncDate(date) } + func tombstones() throws -> [Tombstone] { try wrapped.tombstones() } + func purgeTombstones(ids: [UUID]) throws { try wrapped.purgeTombstones(ids: ids) } + func applyRemote(_ take: Take) throws -> Bool { try wrapped.applyRemote(take) } +} diff --git a/Tests/CatchlightCoreTests/TakeStoreContractTests.swift b/Tests/CatchlightCoreTests/TakeStoreContractTests.swift index 783a111..7924ffb 100644 --- a/Tests/CatchlightCoreTests/TakeStoreContractTests.swift +++ b/Tests/CatchlightCoreTests/TakeStoreContractTests.swift @@ -116,6 +116,42 @@ class TakeStoreContractTests: XCTestCase { XCTAssertNotNil(try store.take(id: take.id)) } + // MARK: - applyRemote — the sync-apply path (delete-resurrection guard) + + func testContract_applyRemote_liveTombstoneWins_nothingWritten() throws { + var take = TestFixtures.richTake() + try store.upsert(take) + try store.delete(id: take.id) + let deletedAt = try XCTUnwrap(store.tombstones().first).deletedAt + + // Remote copy NOT edited after the deletion (ties included) → blocked. + take.modifiedAt = deletedAt + XCTAssertFalse(try store.applyRemote(take)) + XCTAssertNil(try store.take(id: take.id), "blocked apply must not resurrect") + XCTAssertEqual(try store.tombstones().map(\.id), [take.id], + "blocked apply must not clear the pending tombstone") + } + + func testContract_applyRemote_remoteEditedAfterDeletion_resurrects() throws { + var take = TestFixtures.richTake() + try store.upsert(take) + try store.delete(id: take.id) + let deletedAt = try XCTUnwrap(store.tombstones().first).deletedAt + + // Edit-wins: a remote edit STRICTLY after the deletion re-creates the + // Take and supersedes the deletion record, exactly like upsert. + take.modifiedAt = deletedAt.addingTimeInterval(1) + XCTAssertTrue(try store.applyRemote(take)) + XCTAssertNotNil(try store.take(id: take.id)) + XCTAssertTrue(try store.tombstones().isEmpty) + } + + func testContract_applyRemote_noTombstone_behavesLikeUpsert() throws { + let take = TestFixtures.richTake() + XCTAssertTrue(try store.applyRemote(take)) + XCTAssertEqual(try store.take(id: take.id)?.id, take.id) + } + // MARK: - Collection ordering func testContract_allTakes_emptyStoreReturnsEmpty() throws {