Pre-push review: T-1765 cancellation fix

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex a6d0a14..72f122d 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ### Fixed +- 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 before and after lock acquisition, while both create services guard their persistence boundary; existing selective `insertOrDelete` save-failure cleanup is unchanged. 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 both timing windows across tasks and milestones.+ - 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..bfaf2f3 100644--- a/Transit/Transit/Services/DisplayIDAllocator.swift+++ b/Transit/Transit/Services/DisplayIDAllocator.swift@@ -268,18 +268,25 @@ private actor AllocationGate {      /// Runs `body` while holding the lock. Other callers queue until it returns.     ///+    /// Cancellation is checked before acquisition and again after acquisition,+    /// before `body` starts. The second check closes the race where cancellation+    /// arrives while `acquire()` is handing an uncontended or just-released lock+    /// to this caller (T-1765).+    ///     /// 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 {+        try Task.checkCancellation()         let acquired = await acquire()         guard acquired else { throw CancellationError() }         defer { release() }+        try Task.checkCancellation()         return try await body()     } 

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..357095c 100644--- a/Transit/TransitTests/CancelledCreateTests.swift+++ b/Transit/TransitTests/CancelledCreateTests.swift@@ -3,23 +3,40 @@ import SwiftData import Testing @testable import Transit -/// Regression tests for T-1426: "Cancelled creates still persist provisional records".+/// 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 +46,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 +92,7 @@ struct CancelledCreateTests {         }          func saveCounter(nextDisplayID: Int, expectedChangeTag: String?) async throws {+            saveAttempts += 1             guard expectedChangeTag == "\(changeTag)" else {                 throw DisplayIDAllocator.Error.conflict             }@@ -90,6 +111,63 @@ 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 {@@ -137,6 +215,63 @@ 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 {

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..f9d3018--- /dev/null+++ b/specs/bugfixes/pre-cancelled-creates-can-still-persist-records/report.md@@ -0,0 +1,109 @@+# 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 before gate acquisition and again after acquisition before running the body. Check once more 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:285-289` - `AllocationGate.run` checks cancellation before acquisition and again after acquiring the lock but before invoking the body; the existing `defer` releases an acquired lock when the second check throws.+- `Transit/Transit/Services/TaskService.swift:150` - 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/TransitTests/CancelledCreateTests.swift` - Adds deterministic pre-cancelled uncontended and cancellation-during-successful-allocation regressions for both entity types while retaining the existing contended-gate cases.++**Approach rationale:** The gate checks prevent 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.++**Alternatives considered:**+- 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.+- 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.++## 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 checks before gate acquisition and before allocation body execution |+| `Transit/Transit/Services/TaskService.swift` | Post-allocation cancellation check before insertion |+| `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