Adjustments based on code review

Trio/Sources/Services/Network/Nightscout/NightscoutManager.swift

@@ -13,10 +13,7 @@ protocol NightscoutManager: GlucoseSource {
     func deleteCarbs(withID id: String) async
     func deleteInsulin(withID id: String) async
     func deleteGlucose(withID id: String, withDate date: Date) async
-    func uploadDeviceStatus() async
-    func uploadGlucose() async
     func uploadCarbs() async
-    func uploadPumpHistory() async
     func uploadOverrides() async
     func uploadTempTargets() async
     func uploadProfiles() async throws
@@ -47,8 +44,9 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
     /// Coalesces and serializes upload runs so no two runs of the same pipeline overlap.
     /// Runs execute `performUpload(for:)`, provided at init. The public `upload*()`
     /// methods and `requestUpload(_:)` go through the serializer; the `performUpload*()`
-    /// bodies must never call those entry points, or a same-pipeline run deadlocks
-    /// behind itself.
+    /// bodies must not call the awaitable `upload*()` entry points. The serializer
+    /// asserts on such a call in debug builds and downgrades it to a fire-and-forget
+    /// request in release.
     private var uploadSerializer: NightscoutUploadSerializer!
 
     /// Request an upload for a pipeline (enqueue work). Safe to call from anywhere.
@@ -110,14 +108,11 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
 
     // MARK: - Upload triggers
 
-    //
     // Each upload pipeline is driven by an NSFetchedResultsController whose predicate is the
     // "not yet uploaded to Nightscout" set for that entity. The controller fires whenever
     // un-uploaded items appear (or drop out after a successful upload), which we map to a
-    // `requestUpload(pipeline)` call (coalesced and serialized per pipeline). Bound to the viewContext, they
-    // also pick up batch-inserted glucose via the persistent history merge in CoreDataStack —
-    // replacing the previous changedObjects publisher plus the glucoseStorage.updatePublisher
-    // fallback.
+    // `requestUpload(pipeline)` call (coalesced and serialized per pipeline). Bound to the viewContext,
+    // they also pick up batch-inserted glucose via the persistent history merge in CoreDataStack.
 
     let determinationUploadControllerDelegate = FetchedResultsControllerDelegate()
     lazy var determinationUploadController: NSFetchedResultsController<OrefDetermination> = {
@@ -282,7 +277,7 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
 
         /// Ensure that Nightscout Manager holds the `lastEnactedDetermination`, if one exists, on initialization.
         /// We have to set this here in `init()`, so there's a `lastEnactedDetermination` available after an app restart
-        /// for `uploadDeviceStatus()`, as within that fuction `lastEnactedDetermination` is reassigned at the very end of the function.
+        /// for `performUploadDeviceStatus()`, as within that function `lastEnactedDetermination` is reassigned at the very end of the function.
         /// This way, we ensure the latest enacted determination is always part of `devicestatus` and avoid having instances
         /// where the first uploaded non-enacted determination (i.e., "suggested"), lacks the "enacted" data.
         Task {
@@ -537,11 +532,6 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
     /// - Schedule a task to upload pod age data separately.
     ///
     /// - Note: Ensure `nightscoutAPI` is initialized and `isUploadEnabled` is set to `true` before invoking this function.
-    /// - Returns: Nothing.
-    func uploadDeviceStatus() async {
-        await uploadSerializer.run(.deviceStatus)
-    }
-
     private func performUploadDeviceStatus() async throws {
         guard let nightscout = nightscoutAPI, isUploadEnabled else {
             debug(.nightscout, "NS API not available or upload disabled. Aborting NS Status upload.")
@@ -901,10 +891,6 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
         }
     }
 
-    func uploadGlucose() async {
-        await uploadSerializer.run(.glucose)
-    }
-
     private func performUploadGlucose() async {
         do {
             try await uploadGlucose(glucoseStorage.getGlucoseNotYetUploadedToNightscout())
@@ -917,10 +903,6 @@ final class BaseNightscoutManager: NightscoutManager, Injectable {
         }
     }
 
-    func uploadPumpHistory() async {
-        await uploadSerializer.run(.pumpHistory)
-    }
-
     private func performUploadPumpHistory() async {
         do {
             try await uploadPumpHistory(pumpHistoryStorage.getPumpHistoryNotYetUploadedToNightscout())

Trio/Sources/Services/Network/Nightscout/NightscoutUploadSerializer.swift

@@ -10,20 +10,38 @@ import Foundation
 /// An idle pipeline starts a run immediately; while a run is in flight, exactly one
 /// follow-up is queued and further requests coalesce into it. No request is dropped.
 /// Every run executes the upload operation provided at init.
+///
+/// A `run(_:)` issued from inside its own pipeline's upload operation would deadlock
+/// awaiting itself; the serializer asserts in debug builds and downgrades the call to
+/// a fire-and-forget request in release.
 actor NightscoutUploadSerializer {
     /// Uploads everything still pending for a pipeline.
     private let uploadOperation: @Sendable(NightscoutUploadPipeline) async -> Void
 
+    /// Called when `run(_:)` is invoked from inside the same pipeline's upload
+    /// operation. Asserts by default; tests inject a recorder.
+    private let onReentrantRun: @Sendable(NightscoutUploadPipeline) -> Void
+
+    /// Pipeline whose upload operation the current task is executing, if any.
+    @TaskLocal private static var activePipeline: NightscoutUploadPipeline?
+
     /// Run currently in flight per pipeline.
     private var current: [NightscoutUploadPipeline: Task<Void, Never>] = [:]
     /// Follow-up run queued behind the current one, at most one per pipeline.
     private var queued: [NightscoutUploadPipeline: Task<Void, Never>] = [:]
 
-    init(uploadOperation: @escaping @Sendable(NightscoutUploadPipeline) async -> Void) {
+    init(
+        uploadOperation: @escaping @Sendable(NightscoutUploadPipeline) async -> Void,
+        onReentrantRun: @escaping @Sendable(NightscoutUploadPipeline) -> Void = { pipeline in
+            assertionFailure("run(.\(pipeline)) called from inside its own upload operation")
+        }
+    ) {
         self.uploadOperation = uploadOperation
+        self.onReentrantRun = onReentrantRun
     }
 
     /// Fire-and-forget request. Coalesces into an already queued follow-up if present.
+    /// Safe to call from inside an upload operation.
     func request(_ pipeline: NightscoutUploadPipeline) {
         _ = scheduleRun(pipeline)
     }
@@ -31,6 +49,11 @@ actor NightscoutUploadSerializer {
     /// Awaitable request: returns once the run serving it has completed. For callers
     /// that must know the upload attempt has finished before proceeding.
     func run(_ pipeline: NightscoutUploadPipeline) async {
+        guard Self.activePipeline != pipeline else {
+            onReentrantRun(pipeline)
+            request(pipeline)
+            return
+        }
         await scheduleRun(pipeline).value
     }
 
@@ -44,14 +67,18 @@ actor NightscoutUploadSerializer {
             let followUp = Task { [running, uploadOperation, weak self] in
                 await running.value
                 await self?.promoteQueuedRun(pipeline)
-                await uploadOperation(pipeline)
+                await Self.$activePipeline.withValue(pipeline) {
+                    await uploadOperation(pipeline)
+                }
                 await self?.finishCurrentRun(pipeline)
             }
             queued[pipeline] = followUp
             return followUp
         }
         let run = Task { [uploadOperation, weak self] in
-            await uploadOperation(pipeline)
+            await Self.$activePipeline.withValue(pipeline) {
+                await uploadOperation(pipeline)
+            }
             await self?.finishCurrentRun(pipeline)
         }
         current[pipeline] = run

TrioTests/NightscoutUploadSerializerTests.swift

@@ -51,13 +51,17 @@ import Testing
         }
     }
 
-    /// Polls until `condition` is true, giving up after ~2 seconds so a broken
-    /// serializer fails the test instead of hanging it.
+    /// Thrown when `waitUntil` gives up on its condition.
+    private struct TimeoutError: Error {}
+
+    /// Polls until `condition` is true, throwing after ~2 seconds so a broken
+    /// serializer fails the test at the wait site instead of hanging it.
     private func waitUntil(_ condition: @escaping @Sendable() async -> Bool) async throws {
         for _ in 0 ..< 400 {
             if await condition() { return }
             try await Task.sleep(nanoseconds: 5_000_000)
         }
+        throw TimeoutError()
     }
 
     @Test("Concurrent runs of the same pipeline never overlap") func noOverlapWithinPipeline() async {
@@ -139,6 +143,45 @@ import Testing
         #expect(await log.events == ["run-1", "run-2"])
     }
 
+    /// Holds the serializer so an upload operation can call back into it.
+    private actor SerializerBox {
+        private var serializer: NightscoutUploadSerializer?
+
+        func set(_ serializer: NightscoutUploadSerializer) { self.serializer = serializer }
+        func get() -> NightscoutUploadSerializer? { serializer }
+    }
+
+    @Test(
+        "A re-entrant run from inside its own operation degrades to a follow-up instead of deadlocking"
+    ) func reentrantRunDoesNotDeadlock() async throws {
+        let log = EventLog()
+        let reentrantLog = EventLog()
+        let box = SerializerBox()
+
+        let serializer = NightscoutUploadSerializer(
+            uploadOperation: { _ in
+                let n = await log.beginRun()
+                await log.append("run-\(n)")
+                if n == 1 {
+                    // Forbidden re-entrant call; the guard must turn it into a queued follow-up.
+                    await box.get()?.run(.overrides)
+                }
+            },
+            onReentrantRun: { pipeline in
+                Task { await reentrantLog.append("reentrant-\(pipeline)") }
+            }
+        )
+        await box.set(serializer)
+
+        await serializer.request(.overrides)
+
+        try await waitUntil { await log.events.contains("run-2") }
+        try await waitUntil { await reentrantLog.events.isEmpty == false }
+
+        #expect(await log.events == ["run-1", "run-2"])
+        #expect(await reentrantLog.events == ["reentrant-overrides"])
+    }
+
     @Test("Different pipelines run independently of each other") func pipelinesAreIndependent() async throws {
         let log = EventLog()
         let gate = Gate()