Pre-push review: PR #197 — cancelled creates

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex a144750..ce9dfa5 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  - MCP `initialize` now requires object params with string `protocolVersion`, object `capabilities`, and typed `clientInfo` identity fields (T-1778). Missing or malformed method parameters return JSON-RPC `-32602 Invalid Params`. JSON-RPC request decoding now rejects scalar/null top-level `params` server-wide with `-32600 Invalid Request`. Supported `2025-03-26` requests are echoed and unsupported versions receive the server's latest advertised supported version. Focused handshake tests cover absent params/fields, wrong types, malformed client information, request-shape errors, and both negotiation paths. - MCP server start, stop, and same-port restart now run through one async desired-state lifecycle coordinator (T-1826). Each active listener owns its Hummingbird `ServiceGroup` and run task; teardown explicitly triggers graceful shutdown and awaits the group before rebinding, rather than assuming cancellation completion releases the socket. Graceful shutdown is time-bounded so a stalled request cannot wedge the coordinator, and the group installs no SIGTERM/SIGINT traps (`runService()`'s default) because those change the process's signal disposition permanently. Requests arriving during teardown replace the pending target so a burst converges on the latest state, Settings uses an explicit restart operation, and live loopback regressions cover stop, 20 serialized same-port repetitions, different-port restart, rapid off/on, an occupied port surfacing the bind failure, an invalid port releasing the running listener, bounded shutdown escalation, empty signal configuration, and reusable bind-probe behavior.+- Already-cancelled task and milestone creates now stop before an uncontended display-ID allocation, and cancellation during a successful non-cooperative allocation is re-checked before SwiftData insertion (T-1765). `AllocationGate` checks cancellation once it holds the lock and before running the allocation body, while both create services guard their persistence boundary; existing selective `insertOrDelete` save-failure cleanup is unchanged. The gate check is shared by every allocator caller, so a cancelled display-ID promotion or duplicate-cleanup pass now stops at the current record and retries on the next pass. If the counter advance already succeeded, cancellation may leave a harmless gap in the display-ID sequence rather than persisting a ghost record. Four deterministic regressions cover the new timing windows across tasks and milestones, and the two retained queued-cancellation cases now prove that their contender entered and was removed from the gate queue. - The MCP Streamable HTTP endpoint now accepts non-empty JSON-RPC request/notification batches for advertised protocol `2025-03-26` (T-1834). It decodes each member independently, dispatches requests and notifications sequentially, omits notification responses, preserves response-array semantics, returns HTTP 202 with no body for all-notification batches, emits per-member `-32600` errors for invalid entries, handles empty batches as one invalid-request object, and rejects lifecycle-invalid batched `initialize` calls. Route-level regression tests cover each behavior and unchanged single-request responses. - MCP `create_task`, `CreateTaskIntent`, and `AddTaskSheet.persist` now create a task plus optional milestone atomically in one `TaskService` save (T-1768). The relationship is validated and attached before insertion; failed saves delete the pending aggregate instead of relying on a second, fallible compensating deletion. Surface-level failure regressions verify no task remains or is resurrected by a later save. `CreateTaskIntent` maps the merged save's failures per error kind, so a storage failure that was previously reported as `INTERNAL_ERROR` by the separate milestone-assignment step still reports `INTERNAL_ERROR` rather than collapsing into `INVALID_INPUT`; `milestoneProjectMismatch` from the service boundary surfaces as `MILESTONE_PROJECT_MISMATCH`. - Duplicate cleanup now re-probes a task or milestone loser's committed display ID after asynchronous allocation and immediately before mutation (T-2019). Peer changes that land during allocation are preserved and reported as `stale-id`; task cleanup no longer emits a false audit comment for an overwritten peer repair. Deterministic gated regressions cover both record types and the task audit path.

diff --git a/Transit/Transit/Services/DisplayIDAllocator.swift b/Transit/Transit/Services/DisplayIDAllocator.swiftindex d820ab1..bb7988f 100644--- a/Transit/Transit/Services/DisplayIDAllocator.swift+++ b/Transit/Transit/Services/DisplayIDAllocator.swift@@ -68,7 +68,7 @@ final class DisplayIDAllocator: @unchecked Sendable {     /// on the CounterStore's compare-and-swap.     ///     /// Must only be accessed from @MainActor callers (see note above).-    private var allocationGate: AllocationGate = .init()+    private let allocationGate: AllocationGate      /// IDs this process has already handed out but whose owners may not yet have     /// committed them to the local store. The caller's `usedIDs` closure only@@ -79,10 +79,20 @@ final class DisplayIDAllocator: @unchecked Sendable {     /// window (T-1395). Only mutated while holding the gate, on @MainActor.     private var issuedIDs: Set<Int> = [] -    init(store: CounterStore, retryLimit: Int = 5, isCloudSyncActive: Bool = true) {+    init(+        store: CounterStore,+        retryLimit: Int = 5,+        isCloudSyncActive: Bool = true,+        onWaiterQueued: (@Sendable () -> Void)? = nil,+        onWaiterCancelled: (@Sendable () -> Void)? = nil+    ) {         self.counterStore = store         self.retryLimit = max(1, retryLimit)         self.isCloudSyncActive = isCloudSyncActive+        self.allocationGate = AllocationGate(+            onWaiterQueued: onWaiterQueued,+            onWaiterCancelled: onWaiterCancelled+        )     }      convenience init(@@ -258,6 +268,10 @@ final class DisplayIDAllocator: @unchecked Sendable { /// sequentially even when many callers race. private actor AllocationGate {     private var isLocked = false+    /// Test-only lifecycle observers make the queued-cancellation regression+    /// deterministic. Production callers leave both nil.+    private let onWaiterQueued: (@Sendable () -> Void)?+    private let onWaiterCancelled: (@Sendable () -> Void)?     /// FIFO queue of suspended callers, keyed by a monotonically increasing id so     /// a cancelled caller can locate and remove its own continuation without     /// disturbing arrival order. `Bool` is the acquisition outcome handed to the@@ -266,20 +280,38 @@ private actor AllocationGate {     private var waiters: [(id: UInt64, continuation: CheckedContinuation<Bool, Never>)] = []     private var nextWaiterID: UInt64 = 0 +    init(+        onWaiterQueued: (@Sendable () -> Void)? = nil,+        onWaiterCancelled: (@Sendable () -> Void)? = nil+    ) {+        self.onWaiterQueued = onWaiterQueued+        self.onWaiterCancelled = onWaiterCancelled+    }+     /// Runs `body` while holding the lock. Other callers queue until it returns.     ///+    /// Cancellation is checked after acquisition, before `body` starts. That+    /// closes the two windows the waiter-queue handling below does not cover: a+    /// caller that is already cancelled when it takes an uncontended lock, and a+    /// caller that is cancelled while `release()` is handing it the lock (T-1765).+    /// The check deliberately sits *after* `acquire()` — an equivalent pre-acquire+    /// check would be redundant (both windows still end in this check) and would+    /// stop cancelled callers from ever reaching the waiter queue, leaving the+    /// queued-waiter path below untestable.+    ///     /// Acquisition is cancellation-aware: if the calling Task is cancelled while     /// suspended in the waiter queue, its continuation is removed and resumed     /// rather than left pending (which would otherwise trip the runtime's     /// "continuation leaked" check on teardown). A waiter that is cancelled out of     /// the queue never held the lock, so `run` throws `CancellationError` without     /// calling `release` — the lock is never lost. If cancellation races and loses-    /// (the lock was already handed to this waiter via `release`), the waiter keeps-    /// the lock, runs `body`, and releases normally.+    /// (the lock was already handed to this waiter via `release`), the post-acquire+    /// check observes it, and `defer` still releases the lock without running `body`.     func run<T: Sendable>(_ body: @Sendable () async throws -> T) async throws -> T {         let acquired = await acquire()         guard acquired else { throw CancellationError() }         defer { release() }+        try Task.checkCancellation()         return try await body()     } @@ -295,6 +327,7 @@ private actor AllocationGate {         return await withTaskCancellationHandler {             await withCheckedContinuation { continuation in                 waiters.append((id: id, continuation: continuation))+                onWaiterQueued?()             }         } onCancel: {             Task { await self.cancelWaiter(id: id) }@@ -308,6 +341,7 @@ private actor AllocationGate {     private func cancelWaiter(id: UInt64) {         guard let index = waiters.firstIndex(where: { $0.id == id }) else { return }         let waiter = waiters.remove(at: index)+        onWaiterCancelled?()         waiter.continuation.resume(returning: false)     } 

diff --git a/Transit/Transit/Services/MilestoneService.swift b/Transit/Transit/Services/MilestoneService.swiftindex ab6e4f5..1eb25e3 100644--- a/Transit/Transit/Services/MilestoneService.swift+++ b/Transit/Transit/Services/MilestoneService.swift@@ -75,6 +75,12 @@ final class MilestoneService {             displayID = .provisional         } +        // Counter stores are not required to cooperate with Swift cancellation.+        // Re-check after allocation handling and before any post-await model work+        // so a successfully allocated ID cannot turn a cancelled operation into a+        // persisted milestone (T-1765).+        try Task.checkCancellation()+         // Re-check uniqueness after the allocation await. The check above ran         // before this method suspended, so a concurrent create could have         // committed the same name in the meantime (T-1764). CloudKit-backed

diff --git a/Transit/Transit/Services/TaskService.swift b/Transit/Transit/Services/TaskService.swiftindex d1091d8..b342d53 100644--- a/Transit/Transit/Services/TaskService.swift+++ b/Transit/Transit/Services/TaskService.swift@@ -4,6 +4,7 @@ import SwiftData /// Coordinates task creation, status changes, and lookups. Uses StatusEngine /// for all status transitions and DisplayIDAllocator for display ID assignment. @MainActor @Observable+// swiftlint:disable:next type_body_length final class TaskService {      enum Error: Swift.Error, LocalizedError, Equatable {@@ -143,6 +144,12 @@ final class TaskService {             displayID = .provisional         } +        // Counter stores are not required to cooperate with Swift cancellation.+        // Re-check after allocation handling and immediately before constructing+        // and inserting the model so a successfully allocated ID cannot turn a+        // cancelled operation into a persisted task (T-1765).+        try Task.checkCancellation()+         let task = TransitTask(             name: trimmedName,             description: description,

diff --git a/Transit/TransitTests/CancelledCreateTests.swift b/Transit/TransitTests/CancelledCreateTests.swiftindex c827071..36847f5 100644--- a/Transit/TransitTests/CancelledCreateTests.swift+++ b/Transit/TransitTests/CancelledCreateTests.swift@@ -3,23 +3,72 @@ import SwiftData import Testing @testable import Transit -/// Regression tests for T-1426: "Cancelled creates still persist provisional records".+/// Observes the allocation gate's queued-waiter lifecycle. The tests wait for+/// both events before releasing the holder, so cancellation cannot pass merely+/// by short-circuiting before it reaches the waiter queue.+private actor WaiterQueueProbe {+    private var hasQueuedWaiter = false+    private var hasRemovedWaiter = false+    private var queuedContinuation: CheckedContinuation<Void, Never>?+    private var removedContinuation: CheckedContinuation<Void, Never>?++    func recordQueuedWaiter() {+        hasQueuedWaiter = true+        queuedContinuation?.resume()+        queuedContinuation = nil+    }++    func recordRemovedWaiter() {+        hasRemovedWaiter = true+        removedContinuation?.resume()+        removedContinuation = nil+    }++    func waitUntilWaiterQueued() async {+        guard !hasQueuedWaiter else { return }+        await withCheckedContinuation { queuedContinuation = $0 }+    }++    func waitUntilWaiterRemoved() async {+        guard !hasRemovedWaiter else { return }+        await withCheckedContinuation { removedContinuation = $0 }+    }+}++/// Regression tests for cancelled task and milestone creates (T-1426, T-1765). ///-/// The T-1395 allocation gate (`DisplayIDAllocator`'s `AllocationGate`) propagates-/// `CancellationError` when a caller is cancelled while waiting for display-ID-/// allocation. Both creation services previously caught *every* allocation error-/// and converted it into a `.provisional` ID, which meant a cancelled create still-/// inserted and saved a new provisional record instead of aborting.+/// T-1426 covered cancellation while queued behind a contended allocation gate.+/// T-1765 closes two remaining paths: an already-cancelled caller could acquire a+/// free gate, and cancellation during a non-cooperative successful allocation was+/// not observed before insertion. ///-/// These tests contend the allocation gate: a first ("holder") create acquires the-/// gate and blocks inside the counter store, so a second create queues behind it.-/// Cancelling the queued create must surface `CancellationError` and must NOT mutate-/// persistent state. Genuine allocation failures (CloudKit/offline) still fall back-/// to provisional IDs — covered by the existing allocator/concurrency suites.+/// The tests cover pre-cancelled uncontended creates, cancellation while the+/// allocation body succeeds, and cancellation while queued behind a gate holder.+/// In every case cancellation must surface as `CancellationError` and must not+/// mutate persistent state. Genuine CloudKit/offline allocation failures still+/// fall back to provisional IDs in the existing allocator/concurrency suites. @MainActor @Suite(.serialized) struct CancelledCreateTests { -    // MARK: - Test double+    // MARK: - Test doubles++    /// A non-cancellation-cooperative start barrier. Cancelling a task while it+    /// waits here ensures the create path begins with cancellation already set.+    private actor StartGate {+        private var isReleased = false+        private var continuation: CheckedContinuation<Void, Never>?++        func wait() async {+            guard !isReleased else { return }+            await withCheckedContinuation { continuation = $0 }+        }++        func release() {+            isReleased = true+            continuation?.resume()+            continuation = nil+        }+    }      /// A counter store whose **first** `loadCounter` call blocks until the test     /// explicitly releases it. Because `allocateNextID` reads the counter while@@ -29,6 +78,9 @@ struct CancelledCreateTests {     private actor GatedCounterStore: DisplayIDAllocator.CounterStore {         private var nextDisplayID: Int         private var changeTag = 0+        private var saveAttempts = 0++        var saveAttemptCount: Int { saveAttempts }          private var firstLoadStarted = false         private var firstLoadReached: CheckedContinuation<Void, Never>?@@ -72,6 +124,7 @@ struct CancelledCreateTests {         }          func saveCounter(nextDisplayID: Int, expectedChangeTag: String?) async throws {+            saveAttempts += 1             guard expectedChangeTag == "\(changeTag)" else {                 throw DisplayIDAllocator.Error.conflict             }@@ -90,13 +143,75 @@ struct CancelledCreateTests {      // MARK: - Tasks +    /// A create that begins already cancelled must fail before an uncontended+    /// allocation gate reaches the counter store.+    @Test func preCancelledUncontendedTaskCreateDoesNotPersistRecord() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let startGate = StartGate()+        let store = InMemoryCounterStore()+        let allocator = DisplayIDAllocator(store: store)+        let service = TaskService(modelContext: context, displayIDAllocator: allocator)+        let project = makeProject(in: context)++        let operation = Task { @MainActor in+            await startGate.wait()+            _ = try await service.createTask(+                name: "Cancelled", description: nil, type: .feature, project: project+            )+        }+        operation.cancel()+        await startGate.release()++        await #expect(throws: CancellationError.self) {+            try await operation.value+        }++        let storeWasNeverAccessed = await store.wasNeverAccessed+        #expect(storeWasNeverAccessed, "A pre-cancelled create must not enter the counter store")+        #expect(try context.fetch(FetchDescriptor<TransitTask>()).isEmpty)+    }++    /// Cancellation while a non-cooperative counter store completes a successful+    /// allocation must still abort before task insertion.+    @Test func taskCancelledDuringSuccessfulAllocationDoesNotPersistRecord() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let store = GatedCounterStore()+        let allocator = DisplayIDAllocator(store: store)+        let service = TaskService(modelContext: context, displayIDAllocator: allocator)+        let project = makeProject(in: context)++        let operation = Task { @MainActor in+            _ = try await service.createTask(+                name: "Cancelled", description: nil, type: .feature, project: project+            )+        }+        await store.waitUntilGateHeld()+        operation.cancel()+        await store.releaseGate()++        await #expect(throws: CancellationError.self) {+            try await operation.value+        }++        let saveAttempts = await store.saveAttemptCount+        #expect(saveAttempts == 1, "The allocation must succeed before cancellation is observed")+        #expect(try context.fetch(FetchDescriptor<TransitTask>()).isEmpty)+    }+     /// A task create that is cancelled while queued behind a gate-holding create     /// must throw `CancellationError` and must NOT persist any (provisional) task.     @Test func cancelledTaskCreateDoesNotPersistProvisionalRecord() async throws {         let testContainer = try TestModelContainer()         let context = testContainer.context         let store = GatedCounterStore()-        let allocator = DisplayIDAllocator(store: store)+        let queueProbe = WaiterQueueProbe()+        let allocator = DisplayIDAllocator(+            store: store,+            onWaiterQueued: { Task { await queueProbe.recordQueuedWaiter() } },+            onWaiterCancelled: { Task { await queueProbe.recordRemovedWaiter() } }+        )         let service = TaskService(modelContext: context, displayIDAllocator: allocator)         let project = makeProject(in: context) @@ -114,7 +229,9 @@ struct CancelledCreateTests {                 name: "Cancelled", description: nil, type: .feature, project: project             )         }+        await queueProbe.waitUntilWaiterQueued()         contender.cancel()+        await queueProbe.waitUntilWaiterRemoved()          await #expect(throws: CancellationError.self) {             try await contender.value@@ -137,13 +254,75 @@ struct CancelledCreateTests {      // MARK: - Milestones +    /// A milestone create that begins already cancelled must fail before an+    /// uncontended allocation gate reaches the counter store.+    @Test func preCancelledUncontendedMilestoneCreateDoesNotPersistRecord() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let startGate = StartGate()+        let store = InMemoryCounterStore()+        let allocator = DisplayIDAllocator(store: store)+        let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)+        let project = makeProject(in: context)++        let operation = Task { @MainActor in+            await startGate.wait()+            _ = try await service.createMilestone(+                name: "Cancelled", description: nil, project: project+            )+        }+        operation.cancel()+        await startGate.release()++        await #expect(throws: CancellationError.self) {+            try await operation.value+        }++        let storeWasNeverAccessed = await store.wasNeverAccessed+        #expect(storeWasNeverAccessed, "A pre-cancelled create must not enter the counter store")+        #expect(try context.fetch(FetchDescriptor<Milestone>()).isEmpty)+    }++    /// Cancellation while a non-cooperative counter store completes a successful+    /// allocation must still abort before milestone insertion.+    @Test func milestoneCancelledDuringSuccessfulAllocationDoesNotPersistRecord() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let store = GatedCounterStore()+        let allocator = DisplayIDAllocator(store: store)+        let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)+        let project = makeProject(in: context)++        let operation = Task { @MainActor in+            _ = try await service.createMilestone(+                name: "Cancelled", description: nil, project: project+            )+        }+        await store.waitUntilGateHeld()+        operation.cancel()+        await store.releaseGate()++        await #expect(throws: CancellationError.self) {+            try await operation.value+        }++        let saveAttempts = await store.saveAttemptCount+        #expect(saveAttempts == 1, "The allocation must succeed before cancellation is observed")+        #expect(try context.fetch(FetchDescriptor<Milestone>()).isEmpty)+    }+     /// A milestone create that is cancelled while queued behind a gate-holding     /// create must throw `CancellationError` and must NOT persist any record.     @Test func cancelledMilestoneCreateDoesNotPersistProvisionalRecord() async throws {         let testContainer = try TestModelContainer()         let context = testContainer.context         let store = GatedCounterStore()-        let allocator = DisplayIDAllocator(store: store)+        let queueProbe = WaiterQueueProbe()+        let allocator = DisplayIDAllocator(+            store: store,+            onWaiterQueued: { Task { await queueProbe.recordQueuedWaiter() } },+            onWaiterCancelled: { Task { await queueProbe.recordRemovedWaiter() } }+        )         let service = MilestoneService(modelContext: context, displayIDAllocator: allocator)         let project = makeProject(in: context) @@ -159,7 +338,9 @@ struct CancelledCreateTests {                 name: "Cancelled", description: nil, project: project             )         }+        await queueProbe.waitUntilWaiterQueued()         contender.cancel()+        await queueProbe.waitUntilWaiterRemoved()          await #expect(throws: CancellationError.self) {             try await contender.value

diff --git a/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/implementation.md b/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/implementation.mdnew file mode 100644index 0000000..4b4b162--- /dev/null+++ b/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/implementation.md@@ -0,0 +1,110 @@+# Implementation Explanation: Pre-cancelled creates can still persist records (T-1765)++Explains the change at three expertise levels, followed by a completeness assessment.++## Beginner Level++### What Changed++When you create a task or milestone in Transit, the app first asks a shared counter for the next human-friendly number (`T-42`, `M-3`), then writes the record to the database. Getting that number can take a moment, because the counter lives in iCloud.++Sometimes the work gets *cancelled* while that's happening — you close the sheet, the app moves to the background, an automation client hangs up. The bug: the cancellation was often ignored, and the record got written anyway.++The fix adds three "are we still wanted?" checkpoints. If the answer is no, the operation stops and reports that it was cancelled, without writing anything.++### Why It Matters++A cancelled create that still writes a record produces a task nobody asked for. Worse, the caller believes nothing happened, so a retry produces a *second* copy. Because Transit syncs through iCloud, both copies land on every device.++### Key Concepts++- **Cancellation in Swift is cooperative.** Cancelling doesn't forcibly stop anything — it sets a flag. Code has to *check* the flag and stop voluntarily. Think of a "please stop" note left on someone's desk: it only works if they read it. The bug was code that never read the note.+- **Display ID.** The `T-42` number shown in the UI, separate from the internal identifier. It comes from a shared counter so no two records get the same one.+- **The allocation gate.** A queue that lets only one create at a time talk to the counter, so two simultaneous creates can't grab the same number.++---++## Intermediate Level++### Changes Overview++| File | Change |+|------|--------|+| `Services/DisplayIDAllocator.swift` | `AllocationGate.run` calls `Task.checkCancellation()` after acquiring the lock, before running the body |+| `Services/TaskService.swift` | `createTask` re-checks cancellation after allocation handling, before constructing and inserting the model |+| `Services/MilestoneService.swift` | `createMilestone` re-checks cancellation after allocation handling, before the uniqueness re-check and insertion |+| `TransitTests/CancelledCreateTests.swift` | Four new pre-cancelled/in-flight regressions, plus deterministic queue-enqueue and queue-removal assertions for the two retained T-1426 cases |++### Implementation Approach++The pre-existing cancellation handling (T-1426) covered exactly one window: a caller *queued* behind another create's gate hold. `AllocationGate.acquire()` wraps its `withCheckedContinuation` in a `withTaskCancellationHandler`, so a cancelled waiter gets pulled out of the queue and resumed with `false`, and `run` throws `CancellationError`.++Two windows were left open:++1. **Uncontended acquisition.** If the gate is free, `acquire()` returns `true` immediately without ever consulting cancellation. An already-cancelled caller sailed straight into the allocation.+2. **Successful non-cooperative allocation.** `CounterStore` is a protocol. Nothing obliges an implementation to throw when its caller is cancelled — a store built on `withCheckedContinuation` or a non-cancellable external API will simply complete. The caller then held a valid ID and proceeded to insert.++The fix places checks at the two boundaries that own those windows: the gate boundary (`run`, post-acquire) and the persistence boundary (each create service, immediately before model construction).++The service-level checks sit *after* the `do`/`catch` that converts allocation failures into a `.provisional` ID. That placement is deliberate — a check inside the `do` block would be skipped whenever the fallback path ran.++### Trade-offs++- **Two layers rather than one.** The gate check alone is insufficient: with iCloud sync disabled, `allocateNextID` throws `.cloudSyncInactive` *before* the gate is ever entered, so no gate check runs at all and the service falls back to a provisional ID. The service checks are the only cancellation guard on that path.+- **Accepting a numbering gap.** If cancellation lands after `saveCounter` commits, the allocated ID is burned. Undoing it would mean a downward compare-and-swap racing every other allocator. A gap is invisible to users; a ghost record is not. This matches how existing save-failure cleanup already behaves.+- **`insertOrDelete` left alone.** Cancellation is now caught *before* insertion, so no new rollback mechanism was needed. A context-wide rollback would have risked the SwiftData resurrection problem documented in T-452 and would discard unrelated edits on the shared context.++---++## Expert Level++### Technical Deep Dive++The load-bearing subtlety is *where* the gate check goes. An earlier revision of this branch checked cancellation both before and after `acquire()`. The pre-acquire check was removed during pre-push review for two reasons:++1. **It is redundant.** Trace both windows. Uncontended: `acquire()` returns `true` synchronously on the actor, and the post-acquire check throws — identical outcome, identical side effects (`body` never runs, so the store is never touched). Contended-and-already-cancelled: the caller enters `withTaskCancellationHandler`, `onCancel` fires, `cancelWaiter` resumes it with `false`, and the `guard acquired` throws. Neither window can reach `body` either way.++2. **It silently disabled the T-1426 regressions.** The contended tests construct `Task { @MainActor in … }` from a `@MainActor` test body and call `.cancel()` before yielding the actor, so the child observes `isCancelled == true` at its first instruction. A pre-acquire check throws there — the contender never enters `waiters`, and `cancelWaiter` plus the `guard acquired else` branch become unreachable. Since `CancelledCreateTests` is the only file in the suite that cancels anything, that path went to zero coverage.++Both claims were verified empirically rather than argued. Replacing `guard acquired else { throw CancellationError() }` with a `fatalError` left all six tests green while the pre-acquire check was present, and crashed the two contended tests once it was removed. Separately, deleting the post-acquire check fails the two pre-cancelled tests. Each of the three checks is now pinned by at least one regression.++The `defer { release() }` placement is correct across every exit. It is registered *after* `guard acquired`, so it arms only when the caller genuinely holds the lock. `cancelWaiter` and `release` are both actor-isolated and both remove the waiter from `waiters` before resuming it, making `acquired == false` ⟺ "never held the lock" an invariant. A waiter handed the lock by `release()` that then throws at the post-acquire check re-enters `release()`, passing the lock to the next queued caller.++There is no ordering hazard in `acquire()` when the caller is already cancelled and the gate is held. `onCancel` spawns `Task { await self.cancelWaiter(id:) }`, which must hop to the `AllocationGate` actor; `acquire()` is already executing on that actor and appends the waiter synchronously inside the `withCheckedContinuation` closure before suspending. Actor isolation therefore guarantees the append happens-before the dequeue — the waiter cannot be stranded.++### Architecture Impact++`AllocationGate.run` has one caller (`allocateNextID`) but five transitive ones. Beyond the two create paths, the new check also affects `DisplayIDAllocator.promoteProvisionalTasks`, `MilestoneService.promoteProvisionalMilestones`, and both `DisplayIDMaintenanceService` reassignment loops. A cancelled pass in those now aborts at the current record rather than continuing to burn CloudKit allocations for every remaining one — a modest throughput improvement, and consistent with loops that already `break` on error and retry next pass.++The change also sharpens an existing contract: `CounterStore` conformances are explicitly *not* required to be cancellation-cooperative. Responsibility for observing cancellation sits with the gate and with whoever owns the irreversible write, not with the dependency.++### Potential Issues++- **`DisplayIDMaintenanceService` error mapping.** It converts any thrown allocation error into `GroupFailure(code: .allocationFailed, message: error.localizedDescription)`. For `CancellationError` that renders as `The operation couldn't be completed. (Swift.CancellationError error 1.)`. Reachable via the MCP `reassign_duplicate_display_ids` tool when a Hummingbird request task is cancelled on client disconnect — at which point nothing reads the envelope. Left for a separate ticket; noted rather than widened into this bugfix.+- **Promotion and maintenance still lack a post-allocation check.** The same non-cooperative-store argument that motivates the service checks applies to those four call sites: a cancelled pass can still commit exactly one record before the *next* iteration trips the gate check. These are idempotent repair writes rather than ghost records, so impact is low, but the invariant is not yet uniform.+- **`issuedIDs` accumulates phantom entries.** IDs allocated to cancelled callers are never pruned from the in-process collision set. Harmless — the guard only excludes numbers — and immaterial at this app's scale.+- **Duplicated allocation policy.** `TaskService.createTask` and `MilestoneService.createMilestone` now hold ~30 character-identical lines of allocate-or-fall-back-to-provisional logic, edited in lockstep three times (T-1395, T-1426, T-1765). Extracting it into `DisplayIDAllocator` would also retire the `type_body_length` suppression this change made necessary. Deliberately deferred rather than folded into a bugfix branch.++---++## Completeness Assessment++**Fully implemented**++- Cancellation before an uncontended gate acquisition aborts before the counter store is touched (`AllocationGate.run`, verified by two regressions asserting `wasNeverAccessed`).+- Cancellation during a successful non-cooperative allocation aborts before insertion (`TaskService.createTask`, `MilestoneService.createMilestone`, verified by two regressions).+- The pre-existing queued-waiter path (T-1426) retains its coverage.+- `insertOrDelete` remains the persistence boundary for both creates; no rollback semantics changed.+- CHANGELOG and bugfix report describe the shipped behaviour, including the cross-cutting effect on promotion and duplicate cleanup.++**Partially implemented**++- The cancellation invariant is enforced at create boundaries only. Promotion (`promoteProvisionalTasks`, `promoteProvisionalMilestones`) and duplicate cleanup (`DisplayIDMaintenanceService`) gain the gate check but have no post-allocation check of their own.+- `saveAttempts == 1` in the two allocation-window tests asserts a call count on the test double rather than the documented gap behaviour. A stronger assertion would run a second, uncancelled create and expect display ID `2`.++**Not implemented (deliberately out of scope)**++- Extraction of the duplicated allocation-policy block shared by the two create services.+- `CancellationError` mapping in `DisplayIDMaintenanceService`'s `GroupFailure` envelope.++**No divergence** between the implementation and the claims in `report.md` or the CHANGELOG entry after this review's corrections.

diff --git a/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/report.md b/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/report.mdnew file mode 100644index 0000000..ce3ab2e--- /dev/null+++ b/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/report.md@@ -0,0 +1,116 @@+# Bugfix Report: Pre-cancelled creates can still persist records++**Date:** 2026-08-02+**Status:** Fixed++## Description of the Issue++A task or milestone create can persist a record even though its calling Swift task was already cancelled, or was cancelled while a non-cooperative display-ID allocation completed successfully.++**Reproduction steps:**+1. Start a task or milestone create with an uncontended display-ID allocation gate, but cancel the operation before it enters the service; or cancel it while the counter store is suspended in a successful allocation.+2. Allow the operation and counter store to continue.+3. Observe that the create returns successfully and the task or milestone is inserted and saved.++**Impact:** Cancelled callers can receive or expect cancellation while Transit still creates a durable task or milestone. Retrying can then create duplicate user-visible work, and the successful counter allocation also makes the persisted record look like an intentional create.++## Investigation Summary++The investigation followed the four-phase systematic debugging workflow.++- **Phase 1 — overview:** Expected cancellation to abort before any SwiftData insertion. Actual behavior persists a record when cancellation is not observed by a suspension point.+- **Phase 2 — inspection:** `AllocationGate.acquire()` grants a free lock without checking cancellation; `AllocationGate.run` starts its body after acquisition without checking cancellation; `TaskService.createTask` and `MilestoneService.createMilestone` do not inspect cancellation after `allocateNextID` returns.+- **Phase 3 — root cause:** Cancellation was treated as an outcome emitted by the queued-waiter cancellation handler rather than cooperative state that every successful path must inspect.+- **Phase 4 — proposed solution:** Check cancellation after gate acquisition, before running the body. Check again in both create services after allocation handling and before model construction/insertion. Preserve `insertOrDelete` so save-failure cleanup remains selective and does not reintroduce SwiftData rollback resurrection bugs.++**Symptoms examined:** Pre-cancelled uncontended operations and cancellation during a non-cooperative but successful counter allocation.++**Code inspected:** `DisplayIDAllocator.AllocationGate`, `TaskService.createTask`, `MilestoneService.createMilestone`, existing T-1426 cancellation regressions, and the prior T-1426 fix.++**Hypotheses tested:** The queued-waiter cancellation path is correct for contended allocation, but does not cover cancellation before an uncontended acquisition or during an already-running allocator body.++## Discovered Root Cause++**Defect type:** Cooperative-cancellation race / missing cancellation validation++**Five Whys:**+1. Why is a cancelled create persisted? Because execution reaches model insertion and save.+2. Why does execution continue? Because successful allocation returns an ID despite the task's cancelled state.+3. Why does allocation run for a pre-cancelled caller? Because the free-gate path immediately returns `true` without checking cancellation.+4. Why is cancellation during allocation missed? Because the counter store is allowed to be non-cooperative and neither the gate boundary nor create service checks cancellation after the await.+5. Why did existing coverage miss this? Because T-1426 only exercised cancellation while queued behind a contended gate, where `cancelWaiter` explicitly resumes with `false`.++**Root cause:** Cancellation awareness was implemented only in the gate's queued-waiter removal path. The successful uncontended and successful in-flight allocation paths had no cooperative `Task.checkCancellation()` boundary before persistence.++**Contributing factors:** Swift cancellation is cooperative; an async counter store is not required to throw when its caller is cancelled. The create services intentionally convert genuine allocation failures to provisional IDs, so cancellation must remain explicitly distinguished from offline failures.++**Assumptions validated:** `ModelContext.insertOrDelete` is the required create rollback mechanism. Replacing it with context-wide rollback would risk retaining or later resurrecting inserted `@Model` instances and discarding unrelated shared-context edits.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/DisplayIDAllocator.swift:292` - `AllocationGate.run` checks cancellation after acquiring the lock but before invoking the body; the existing `defer` releases the acquired lock when the check throws.+- `Transit/Transit/Services/TaskService.swift:151` - Re-checks cancellation after permanent/provisional allocation handling and immediately before task construction and insertion.+- `Transit/Transit/Services/MilestoneService.swift:82` - Re-checks cancellation after allocation handling and before the synchronous uniqueness re-check and insertion.+- `Transit/Transit/Services/TaskService.swift:7` - Adds a targeted `type_body_length` suppression. The added check pushes the type body to 253 counted lines against SwiftLint's default 250 warning threshold, and `make lint` runs `--strict`. Matches existing precedent in `DisplayIDMaintenanceService.swift` and `MCPToolHandler.swift`.+- `Transit/TransitTests/CancelledCreateTests.swift` - Adds deterministic pre-cancelled uncontended and cancellation-during-successful-allocation regressions for both entity types, and strengthens the retained contended-gate cases to prove a waiter entered and was removed from the gate queue before the holder is released.++**Approach rationale:** The gate check prevents cancelled work from entering a newly acquired allocation critical section, while the service checks protect the persistence boundary when a counter store completes successfully without observing cancellation. Together they cover both dependency-level and operation-level responsibility. The existing `modelContext.insertOrDelete` calls remain unchanged, preserving selective cleanup on save failure instead of introducing a context-wide rollback. If cancellation arrives after `saveCounter` succeeds, the allocated display ID is intentionally left unused; the sequence may contain a gap, matching existing save-failure behavior and preferring a skipped number over a ghost record.++The gate check is placed *after* `acquire()` rather than before it. A pre-acquire check was written first and then removed during pre-push review: it is redundant (both the uncontended and hand-off windows still terminate at the post-acquire check), and it prevents an already-cancelled caller from ever entering the waiter queue, which silently disabled the T-1426 regressions for the queued-waiter path. This was confirmed empirically — with a pre-acquire check present, replacing `guard acquired else { throw CancellationError() }` with a `fatalError` left all six tests green; without it, the two contended tests hit that path. Coverage of both gate windows now holds: removing the post-acquire check fails the two pre-cancelled tests, and removing the queued-waiter guard fails the two contended tests.++`AllocationGate.run` is shared by every `allocateNextID` caller, so the new check also affects display-ID promotion (`DisplayIDAllocator.promoteProvisionalTasks`, `MilestoneService.promoteProvisionalMilestones`) and duplicate cleanup (`DisplayIDMaintenanceService`). A cancelled pass there now aborts at the current record instead of continuing to allocate — the intended behavior, since those loops already `break` on error and retry on the next pass. One rough edge is accepted: `DisplayIDMaintenanceService` maps the thrown `CancellationError` into a `GroupFailure(code: .allocationFailed)` whose message renders as `The operation couldn't be completed. (Swift.CancellationError error 1.)`. That envelope is only produced for a caller that has already been cancelled and therefore discards the result, so it is left for a separate ticket rather than widened into this bugfix.++**Alternatives considered:**+- Check cancellation before gate acquisition as well - Rejected as redundant, and it removes test reachability of the queued-waiter cancellation path (see above).+- Check only inside `AllocationGate` after the body returns - Rejected because create services own the irreversible insertion boundary and should independently reject cancellation after any allocator implementation returns. The gate is also bypassed entirely when iCloud sync is off (`allocateNextID` throws `.cloudSyncInactive` before the gate and the services fall back to a provisional ID), so the service checks are the only cancellation guard on that path.+- Rely on counter stores to throw cancellation - Rejected because Swift cancellation is cooperative and protocol implementations may legitimately use non-throwing continuations or external APIs that still complete successfully.+- Roll back the context after insertion - Rejected because cancellation can be checked before insertion, and SwiftData rollback is not reliable cleanup for newly inserted models and could discard unrelated shared-context edits.+- Extract the duplicated allocate-or-fall-back-to-provisional block shared by `TaskService` and `MilestoneService` into `DisplayIDAllocator` - Real duplication (~30 lines, now edited in lockstep for the third time across T-1395, T-1426, T-1765), but rejected for this bugfix as an unrelated refactor of two service hot paths. Worth a follow-up ticket; it would also retire the `type_body_length` suppression.++## Regression Test++**Test file:** `Transit/TransitTests/CancelledCreateTests.swift`++**Test names:**+- `preCancelledUncontendedTaskCreateDoesNotPersistRecord`+- `taskCancelledDuringSuccessfulAllocationDoesNotPersistRecord`+- `preCancelledUncontendedMilestoneCreateDoesNotPersistRecord`+- `milestoneCancelledDuringSuccessfulAllocationDoesNotPersistRecord`++**What they verify:** Already-cancelled operations never enter an uncontended counter store, and operations cancelled during a successful non-cooperative allocation throw `CancellationError` without inserting task or milestone records.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/DisplayIDAllocator.swift` | Cancellation check after gate acquisition, before allocation body execution |+| `Transit/Transit/Services/TaskService.swift` | Post-allocation cancellation check before insertion; `type_body_length` suppression |+| `Transit/Transit/Services/MilestoneService.swift` | Post-allocation cancellation check before uniqueness re-check/insertion |+| `Transit/TransitTests/CancelledCreateTests.swift` | Four cancellation regressions and successful-allocation instrumentation |++## Verification++**Automated:**+- [x] Regression tests fail before the fix (four T-1765 cases fail; two existing T-1426 cases pass)+- [x] Regression tests pass after the fix (`make test-quick PIPE_PRETTY=`)+- [ ] Full test suite passes — two `make test PIPE_PRETTY=` runs passed all T-1765 regressions on iOS but failed unrelated UI tests. The first failed `testClearAll`, `testEditViewPreservesTaskMilestone`, and `testDataMaintenanceGoldenPath`; the post-review rerun also failed three settings-navigation cases (`testSettingsHasBackChevron`, `testSettingsWithNoProjectsShowsCreatePrompt`, `testTappingGearPushesSettingsView`), confirming simulator/UI-suite instability outside the changed cancellation paths.+- [x] Linters/validators pass (`make lint`)++**Manual verification:**+- Review the final diff to confirm `insertOrDelete` remains the persistence boundary for both create paths.++## Prevention++**Recommendations to avoid similar bugs:**+- Treat Swift cancellation as cooperative state and check it at boundaries around non-cooperative async dependencies.+- Keep a final cancellation check immediately before irreversible persistence when the preceding await may succeed after cancellation.+- Test both queued cancellation and successful non-cooperative dependency completion.++## Related++- Transit T-1765+- Transit T-1426+- Transit T-1395