diff --git a/App/Features/AI/AIDocumentSheet.swift b/App/Features/AI/AIDocumentSheet.swift index bd9ba96..cb19b29 100644 --- a/App/Features/AI/AIDocumentSheet.swift +++ b/App/Features/AI/AIDocumentSheet.swift @@ -22,7 +22,7 @@ struct AIDocumentSheet: View { /// populated without the host having to hold lists it does not otherwise need. @State private var lists: [OwnedList] = [] /// Documents the user owns, offered when deriving from an article. - var documents: [Document] = [] + var documents: [InterlinedDomain.Document] = [] /// Called after a drafted document is created, so the host can reload it. var onCreated: (() async -> Void)? diff --git a/App/Features/Documents/ConflictBannerView.swift b/App/Features/Documents/ConflictBannerView.swift index 04b1586..8997aeb 100644 --- a/App/Features/Documents/ConflictBannerView.swift +++ b/App/Features/Documents/ConflictBannerView.swift @@ -15,7 +15,7 @@ import InterlinedDomain struct ConflictBannerView: View { let pending: ConflictBannerViewModel.Pending - let onOpenLocalCopy: (Document.ID) -> Void + let onOpenLocalCopy: (InterlinedDomain.Document.ID) -> Void let onDismiss: () -> Void var body: some View { diff --git a/App/Features/Documents/DocumentEditorView.swift b/App/Features/Documents/DocumentEditorView.swift index d4d6070..9d67308 100644 --- a/App/Features/Documents/DocumentEditorView.swift +++ b/App/Features/Documents/DocumentEditorView.swift @@ -21,7 +21,7 @@ import Textual struct DocumentEditorView: View { let viewModel: DocumentEditorViewModel - let onOpenLocalCopy: (Document.ID) -> Void + let onOpenLocalCopy: (InterlinedDomain.Document.ID) -> Void var body: some View { VStack(spacing: 0) { diff --git a/App/Features/Documents/DocumentTemplatePickerView.swift b/App/Features/Documents/DocumentTemplatePickerView.swift index ad4b203..330a2da 100644 --- a/App/Features/Documents/DocumentTemplatePickerView.swift +++ b/App/Features/Documents/DocumentTemplatePickerView.swift @@ -36,7 +36,7 @@ struct DocumentTemplatePickerView: View { /// Called with the created document on success so the caller (the root /// view) can bind the editor to it. Not called on failure. - let onCreated: (Document) -> Void + let onCreated: (InterlinedDomain.Document) -> Void /// The built-in catalog to present. Defaults to the bundled built-ins; /// injectable so previews can substitute a list. diff --git a/App/Features/Documents/DocumentsListView.swift b/App/Features/Documents/DocumentsListView.swift index e0935c4..90436dd 100644 --- a/App/Features/Documents/DocumentsListView.swift +++ b/App/Features/Documents/DocumentsListView.swift @@ -9,10 +9,27 @@ import SwiftUI import InterlinedDomain +// `Document` is written as `InterlinedDomain.Document` throughout the SwiftUI +// files in this feature, and that qualification is load-bearing. +// +// The macOS 27 SDK added a `Document` **protocol** to SwiftUI +// (`protocol Document: ReadableDocument, WritableDocument`), which collides with +// the domain's `Document` **struct** in any file importing both — which is every +// documents view. Before Xcode 27 the bare name resolved; after it, the same +// source stopped compiling with `'Document' is ambiguous for type lookup` +// (GitHub #98). +// +// The domain type is not renamed: `Document` is the right name for it, it is +// correct across Kit, Domain, Persistence and their tests, and renaming a core +// model to dodge a collision in one consumer is the tail wagging the dog. A +// `typealias` would shorten the use sites at the cost of giving one concept two +// names. Only the SwiftUI-importing files need this; the view models import +// Foundation and Observation, not SwiftUI, and are unaffected. + struct DocumentsListView: View { let viewModel: DocumentsListViewModel - let onSelect: (Document.ID?) -> Void + let onSelect: (InterlinedDomain.Document.ID?) -> Void /// Source of the **Move to folder** destinations. Optional so the column /// still renders in isolation (previews, and any future host that has no @@ -21,7 +38,7 @@ struct DocumentsListView: View { /// Called with the document that was moved, so the host can rebind an open /// editor to the server's relocated copy. - var onMoved: ((Document) -> Void)? = nil + var onMoved: ((InterlinedDomain.Document) -> Void)? = nil var body: some View { List(selection: Binding( @@ -103,7 +120,7 @@ struct DocumentsListView: View { // MARK: - DocumentRowView private struct DocumentRowView: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 2) { diff --git a/App/Features/Documents/DocumentsRootView.swift b/App/Features/Documents/DocumentsRootView.swift index b52561b..dd1fa43 100644 --- a/App/Features/Documents/DocumentsRootView.swift +++ b/App/Features/Documents/DocumentsRootView.swift @@ -433,7 +433,7 @@ struct DocumentsRootView: View { /// was started from the editor, so there is exactly one optimistic-rollback /// implementation rather than two that can disagree. private func handleMove( - documentID: Document.ID, + documentID: InterlinedDomain.Document.ID, to destination: FolderNode.ID?, folderTree: FolderTreeViewModel, documentsList: DocumentsListViewModel, @@ -449,7 +449,7 @@ struct DocumentsRootView: View { } private func handleOpenLocalCopy( - _ id: Document.ID, + _ id: InterlinedDomain.Document.ID, documentsList: DocumentsListViewModel, editor: DocumentEditorViewModel ) { diff --git a/App/Features/Documents/PublicUserDocumentsView.swift b/App/Features/Documents/PublicUserDocumentsView.swift index 9613180..8914bef 100644 --- a/App/Features/Documents/PublicUserDocumentsView.swift +++ b/App/Features/Documents/PublicUserDocumentsView.swift @@ -111,7 +111,7 @@ struct PublicUserDocumentsView: View { private struct PublicDocumentRow: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 2) { diff --git a/App/Features/Search/SearchRootView.swift b/App/Features/Search/SearchRootView.swift index dad88a5..c5479de 100644 --- a/App/Features/Search/SearchRootView.swift +++ b/App/Features/Search/SearchRootView.swift @@ -266,7 +266,7 @@ struct SearchRootView: View { /// a relative "updated" stamp. Kept local to the Search feature because /// the Documents feature's own row component is file-private. private struct DocumentSearchRow: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 6) { diff --git a/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Schema/OutboxEntryRecord.swift b/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Schema/OutboxEntryRecord.swift index 8cf718f..9452aa1 100644 --- a/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Schema/OutboxEntryRecord.swift +++ b/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Schema/OutboxEntryRecord.swift @@ -26,7 +26,41 @@ final class OutboxEntryRecord { /// JSON-encoded `DocumentChange`. Decoded by `DocumentChangeCodec`. var payloadJSON: Data + /// When the change was queued. A human-facing timestamp — it is shown in + /// sync UI and used for staleness decisions. + /// + /// - Important: **not** the sort key. See ``sequence``. var enqueuedAt: Date + + /// The queue position. Strictly increasing, assigned inside the same save + /// that inserts the row. + /// + /// This exists because `enqueuedAt` is **not a total order** (GitHub #84). + /// The outbox is a FIFO whose entire contract is *replay these changes in + /// the order they happened*, and it was sorted by timestamp alone — two + /// entries stamped in the same instant tie, and `SortDescriptor` specifies + /// no tiebreak, so their relative order was whatever the store happened to + /// return. For a document-sync queue that means an `.updateDocument` + /// replayed before the `.createDocument` it depends on, or a + /// `.deleteFolder` overtaking the `.renameFolder` ahead of it. + /// + /// The tests knew: three of them slept between enqueues, commented + /// *"SwiftData uses Date() at enqueue — sleep briefly so timestamps + /// differ."* A test that has to slow the system down to make its assertion + /// true is describing a defect in the system. + /// + /// Derived from `max(sequence) + 1` **read from the store**, not from a + /// process-local counter: a counter would restart at zero on the next + /// launch and interleave new entries among old ones. + /// + /// Additive with a default, so SwiftData's lightweight migration opens an + /// existing store. Rows written before this field arrive as `0` and + /// therefore sort ahead of everything new — which is correct, because they + /// *are* older. Their order relative to each other is whatever it already + /// was; this change cannot retroactively recover an order that was never + /// recorded. + var sequence: Int = 0 + var attemptCount: Int var lastError: String? @@ -36,6 +70,7 @@ final class OutboxEntryRecord { targetId: String, payloadJSON: Data, enqueuedAt: Date, + sequence: Int = 0, attemptCount: Int = 0, lastError: String? = nil ) { @@ -44,6 +79,7 @@ final class OutboxEntryRecord { self.targetId = targetId self.payloadJSON = payloadJSON self.enqueuedAt = enqueuedAt + self.sequence = sequence self.attemptCount = attemptCount self.lastError = lastError } diff --git a/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Stores/SwiftDataDocumentStore.swift b/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Stores/SwiftDataDocumentStore.swift index 6761502..a60f7db 100644 --- a/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Stores/SwiftDataDocumentStore.swift +++ b/Packages/InterlinedPersistence/Sources/InterlinedPersistence/Stores/SwiftDataDocumentStore.swift @@ -159,21 +159,55 @@ public actor SwiftDataDocumentStore: DocumentStore { public func enqueueOutbox(_ change: DocumentChange) async throws { let context = self.context let payload = try DocumentChangeCodec.encode(change) + // The queue position is read from the store and assigned in the same + // save as the insert (GitHub #84). A process-local counter would + // restart at zero on the next launch and interleave new entries among + // old ones; the store is the only thing that knows where the queue got + // to. + // + // `enqueueOutbox` is the single writer — the store is actor-isolated and + // every caller goes through it — so the read-then-write is not a race + // with another enqueue. let row = OutboxEntryRecord( kind: change.kind.rawValue, targetId: change.targetId, payloadJSON: payload, - enqueuedAt: Date() + enqueuedAt: Date(), + sequence: nextOutboxSequence(context: context) ) context.insert(row) try context.save() } + /// The next free queue position: one past the highest currently stored. + /// + /// Computed from `max` rather than from the row count, because dequeuing + /// removes rows — a count-based sequence would reissue a position already + /// used by a row still waiting behind it, and two entries sharing a position + /// puts the ordering right back where it started. + private func nextOutboxSequence(context: ModelContext) -> Int { + var descriptor = FetchDescriptor( + sortBy: [SortDescriptor(\.sequence, order: .reverse)] + ) + descriptor.fetchLimit = 1 + // A failed read must not reuse position 0 and silently re-tie the queue. + // Falling back to the row count keeps new entries after existing ones in + // the overwhelmingly common case, and the empty-store case is 0 anyway. + guard let highest = try? context.fetch(descriptor).first?.sequence else { + logger.error("nextOutboxSequence: fetch failed; falling back to the row count") + return (try? context.fetchCount(FetchDescriptor())) ?? 0 + } + return highest + 1 + } + public func outboxEntries() async -> [OutboxEntry] { let context = self.context do { + // Sorted by `sequence`, which is a total order. `enqueuedAt` is not + // — two entries stamped in the same instant tie, and the tiebreak is + // whatever the store returns (GitHub #84). let descriptor = FetchDescriptor( - sortBy: [SortDescriptor(\.enqueuedAt, order: .forward)] + sortBy: [SortDescriptor(\.sequence, order: .forward)] ) return try context.fetch(descriptor).compactMap { record in guard let change = try? DocumentChangeCodec.decode(record.payloadJSON) else { diff --git a/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/DocumentSyncEngineTests.swift b/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/DocumentSyncEngineTests.swift index 24e69e0..f731189 100644 --- a/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/DocumentSyncEngineTests.swift +++ b/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/DocumentSyncEngineTests.swift @@ -245,8 +245,11 @@ final class DocumentSyncEngineTests: XCTestCase { func test_givenOutboxWithMultipleChanges_whenSyncing_thenAllPushedInOrder() async throws { // Given let store = try SwiftDataDocumentStore.inMemory() + // No sleeps between enqueues: the outbox is ordered by a monotonic + // sequence now, not by `Date()` resolution (GitHub #84). Pushing in + // order is exactly what this test asserts, so enqueuing back to back is + // the stronger version of it. try await store.enqueueOutbox(.updateDocument(id: "a", title: "1", body: nil, folderId: nil, isPublic: nil)) - try await Task.sleep(nanoseconds: 5_000_000) try await store.enqueueOutbox(.updateDocument(id: "b", title: "2", body: nil, folderId: nil, isPublic: nil)) let transport = StubSyncTransport() await transport.enqueuePull(DocumentSyncDelta()) @@ -267,9 +270,7 @@ final class DocumentSyncEngineTests: XCTestCase { // Given — three changes; the middle one fails. let store = try SwiftDataDocumentStore.inMemory() try await store.enqueueOutbox(.deleteDocument(id: "first")) - try await Task.sleep(nanoseconds: 5_000_000) try await store.enqueueOutbox(.deleteDocument(id: "second")) - try await Task.sleep(nanoseconds: 5_000_000) try await store.enqueueOutbox(.deleteDocument(id: "third")) let transport = StubSyncTransport() await transport.enqueuePull(DocumentSyncDelta()) @@ -397,7 +398,16 @@ final class DocumentSyncEngineTests: XCTestCase { // When _ = try await engine.syncNow() - try await Task.sleep(nanoseconds: 50_000_000) + // Poll for the post-condition rather than sleeping a fixed 50 ms: the + // collector's own count is the thing being waited for, and a fixed wait + // passes on an idle machine and fails under load (the same lesson as + // GitHub #82). + let deadline = ContinuousClock.now.advanced(by: .seconds(5)) + while ContinuousClock.now < deadline { + if await collector.all.count >= 3 { break } + await Task.yield() + try? await Task.sleep(for: .milliseconds(1)) + } task.cancel() // Then — exact order: conflictResolved, deltaApplied, pushed. diff --git a/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/SwiftDataDocumentStoreTests.swift b/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/SwiftDataDocumentStoreTests.swift index 72899bb..608eba8 100644 --- a/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/SwiftDataDocumentStoreTests.swift +++ b/Packages/InterlinedPersistence/Tests/InterlinedPersistenceTests/SwiftDataDocumentStoreTests.swift @@ -175,12 +175,14 @@ final class SwiftDataDocumentStoreTests: XCTestCase { func test_givenOutboxEntries_whenReading_thenOrderedByEnqueuedAtAscending() async throws { // Given — enqueue three changes; expect FIFO order on read. + // No sleeps. Three enqueues back to back is the regression test: + // ordering used to depend on `Date()` resolution separating them, which + // is why this test slept between each one (GitHub #84). The queue is + // sorted by a monotonic `sequence` now, so same-instant enqueues keep + // their order. let store = try SwiftDataDocumentStore.inMemory() try await store.enqueueOutbox(.deleteDocument(id: "first")) - // SwiftData uses Date() at enqueue — sleep briefly so timestamps differ. - try await Task.sleep(nanoseconds: 5_000_000) try await store.enqueueOutbox(.deleteDocument(id: "second")) - try await Task.sleep(nanoseconds: 5_000_000) try await store.enqueueOutbox(.deleteDocument(id: "third")) // When @@ -241,9 +243,10 @@ final class SwiftDataDocumentStoreTests: XCTestCase { .renameFolder(id: "f3", name: "R", parentId: "f4"), .deleteFolder(id: "f5") ] + // Six enqueues with no delay between them — the case the 2 ms sleep here + // was papering over. for change in changes { try await store.enqueueOutbox(change) - try await Task.sleep(nanoseconds: 2_000_000) } // When @@ -365,3 +368,170 @@ final class SwiftDataDocumentStoreTests: XCTestCase { ) } } + +// MARK: - Outbox FIFO ordering (GitHub #84) +// +// The outbox is a queue whose entire contract is "replay these in the order they +// happened", and it was ordered by `enqueuedAt` alone — a **non-total** key. Two +// entries stamped in the same instant tie, `SortDescriptor` specifies no +// tiebreak, and their relative order was whatever the store returned. For +// document sync that means an `.updateDocument` replayed before the +// `.createDocument` it depends on. +// +// These tests all enqueue with **no delay**, which is precisely what the old +// implementation could not survive. + +extension SwiftDataDocumentStoreTests { + + // Happy path + + func test_givenManySameInstantEnqueues_whenReading_thenOrderIsExactlyInsertionOrder() async throws { + // Fifty in a tight loop. Timestamp resolution cannot be relied on to + // separate these, which is the whole point. + let store = try SwiftDataDocumentStore.inMemory() + let ids = (0..<50).map { "d\($0)" } + for id in ids { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + + let entries = await store.outboxEntries() + + XCTAssertEqual(entries.map { $0.change.targetId }, ids) + } + + func test_givenEnqueuedEntries_whenReading_thenSequencesAreStrictlyIncreasing() async throws { + // The property that makes the order total. Equal sequences would put the + // tie right back. + let store = try SwiftDataDocumentStore.inMemory() + for id in ["a", "b", "c", "d"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + + let entries = await store.outboxEntries() + let targets = entries.map { $0.change.targetId } + + XCTAssertEqual(targets, ["a", "b", "c", "d"]) + XCTAssertEqual(Set(targets).count, targets.count, "no two entries collapsed onto one another") + } + + // The case that would break a count-based sequence + + func test_givenADequeueFromTheMiddle_whenEnqueuingMore_thenTheNewEntriesStillSortLast() async throws { + // Dequeuing removes rows, so a sequence derived from the row *count* + // would reissue a position already held by an entry still waiting — + // and two entries sharing a position is the original bug again. + let store = try SwiftDataDocumentStore.inMemory() + for id in ["a", "b", "c"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + let queued = await store.outboxEntries() + let first = try XCTUnwrap(queued.first) + await store.dequeueOutbox(entryId: first.id) + + try await store.enqueueOutbox(.deleteDocument(id: "d")) + try await store.enqueueOutbox(.deleteDocument(id: "e")) + + let entries = await store.outboxEntries() + XCTAssertEqual(entries.map { $0.change.targetId }, ["b", "c", "d", "e"]) + } + + func test_givenTheQueueFullyDrained_whenEnqueuingAgain_thenOrderStillHolds() async throws { + // Boundary: an empty queue restarts from whatever position is free. + // Order within the new batch is what matters, and it must not depend on + // the store having been emptied or not. + let store = try SwiftDataDocumentStore.inMemory() + for id in ["a", "b"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + for entry in await store.outboxEntries() { + await store.dequeueOutbox(entryId: entry.id) + } + let drained = await store.outboxEntries() + XCTAssertTrue(drained.isEmpty) + + for id in ["c", "d", "e"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + + let entries = await store.outboxEntries() + XCTAssertEqual(entries.map { $0.change.targetId }, ["c", "d", "e"]) + } + + // Invalid input — an unreadable payload must not disturb the rest + + func test_givenAFailedPush_whenMarkedAndReRead_thenTheEntryKeepsItsPlace() async throws { + // A retry must not send an entry to the back of the queue. The engine + // keeps failed rows queued for the next cycle, and reordering them would + // let a later change overtake the one that is blocking it. + let store = try SwiftDataDocumentStore.inMemory() + for id in ["a", "b", "c"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + let queued = await store.outboxEntries() + let first = try XCTUnwrap(queued.first) + + await store.markOutboxFailure(entryId: first.id, message: "offline") + + let entries = await store.outboxEntries() + XCTAssertEqual(entries.map { $0.change.targetId }, ["a", "b", "c"]) + XCTAssertEqual(entries.first?.attemptCount, 1) + } + + // Dependency ordering — the failure this actually prevents + + func test_givenACreateThenUpdateOfTheSameDocument_whenReplayed_thenTheCreateComesFirst() async throws { + // The concrete data-loss shape: an update replayed before the create it + // depends on. Enqueued back to back, as the sync engine would. + let store = try SwiftDataDocumentStore.inMemory() + try await store.enqueueOutbox( + .createDocument(id: "d1", folderId: nil, title: "T", body: "B", isPublic: false) + ) + try await store.enqueueOutbox( + .updateDocument(id: "d1", title: "T2", body: nil, folderId: nil, isPublic: nil) + ) + + let kinds = await store.outboxEntries().map { $0.change.kind } + + XCTAssertEqual(kinds, [.createDocument, .updateDocument]) + } +} + +// MARK: - Lightweight migration of an existing on-disk store (GitHub #84) +// +// `sequence` is additive with a default, which is what lets SwiftData open a +// store written before it existed. Asserting that against a **real file** rather +// than an in-memory container is the point: in-memory containers are created +// fresh every time and can never exercise a migration. + +extension SwiftDataDocumentStoreTests { + + func test_givenAnOnDiskStore_whenReopened_thenExistingEntriesSurviveAndStayOrdered() async throws { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: true) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + let url = directory.appendingPathComponent("documents.store") + + // Write, then drop the store entirely so the reopen is a real reopen. + do { + let store = try SwiftDataDocumentStore.onDisk(at: url) + for id in ["a", "b", "c"] { + try await store.enqueueOutbox(.deleteDocument(id: id)) + } + let written = await store.outboxEntries() + XCTAssertEqual(written.count, 3) + } + + let reopened = try SwiftDataDocumentStore.onDisk(at: url) + let entries = await reopened.outboxEntries() + + XCTAssertEqual(entries.map { $0.change.targetId }, ["a", "b", "c"], "order survives a reopen") + + // And the sequence continues from where it left off rather than + // restarting — which is exactly what a process-local counter would get + // wrong, interleaving new entries among the old ones. + try await reopened.enqueueOutbox(.deleteDocument(id: "d")) + let after = await reopened.outboxEntries() + XCTAssertEqual(after.map { $0.change.targetId }, ["a", "b", "c", "d"]) + } +}