Fix ProgressManager Bugs (swiftlang#1907)

* addChild fired withMutation of completedCount bug + fix

* fractionCompleted update in interop case with NSProgress parent and ProgressManager grandchild bug + fix

* atomic handling of updates to NSProgress

* add tests

* cleanup

* add #if FOUNDATION_FRAMEWORK check

* cleanup

* cleanup

Author: chloe-yeo

Sources/FoundationEssentials/ProgressManager/ProgressManager+Interop.swift

@@ -133,9 +133,14 @@ internal final class SubprogressBridge: Sendable {
                 return
             }
 
-            // This needs to change totalUnitCount before completedUnitCount otherwise progressBridge will finish and mess up the math
-            self.progressBridge.totalUnitCount = Int64(observerState.totalCount)
-            self.progressBridge.completedUnitCount = Int64(observerState.completedCount)
+            // Use atomic update to avoid corrupting NSProgress's parent accounting.
+            // overallFraction's denominator changes when children finish, and setting
+            // totalUnitCount and completedUnitCount separately causes a momentary spike
+            // that permanently corrupts the parent's bookkeeping.
+            self.progressBridge._setCompletedUnitCount(
+                Int64(observerState.completedCount),
+                totalUnitCount: Int64(observerState.totalCount)
+            )
         }
     }
 }
@@ -298,6 +303,44 @@ extension ProgressManager {
         }
     }
 
+    /// Notifies interop bridge observers so that grandchild progress propagates
+    /// through the SubprogressBridge to the parent NSProgress.
+    internal func notifyInteropObserversOfChildUpdate() {
+        // Phase 1: Collect bridge manager and dirty children info
+        let info: (bridgeManager: ProgressManager, updates: [PendingChildUpdateInfo]?)? = state.withLock { state in
+            guard case .interopObservation(let observation) = state.interopType,
+                  let bridgeManager = observation.subprogressBridge?.manager else {
+                return nil
+            }
+            return (bridgeManager, state.pendingChildrenUpdates())
+        }
+
+        guard let info, let updates = info.updates else {
+            return
+        }
+
+        // Phase 2: Resolve each dirty child's fraction
+        var childrenUpdates: [PendingChildUpdate] = []
+        for update in updates {
+            let updatedFraction = update.manager.updatedProgressFraction()
+            childrenUpdates.append(PendingChildUpdate(
+                index: update.index,
+                updatedFraction: updatedFraction,
+                assignedCount: update.assignedCount
+            ))
+        }
+
+        // Phase 3: Apply updates and compute fraction
+        let observerState = state.withLock { state in
+            state.updateChildrenProgressFraction(updates: childrenUpdates)
+            let fraction = state.overallFraction
+            return ObserverState(totalCount: fraction.total ?? 0, completedCount: fraction.completed)
+        }
+
+        // Notify bridge
+        info.bridgeManager.notifyObservers(with: observerState)
+    }
+    
     internal func addBridge(reporterBridge: ProgressReporterBridge? = nil, nsProgressBridge: Foundation.Progress? = nil) {
         state.withLock { state in
             var interopObservation = InteropObservation(subprogressBridge: nil)

Sources/FoundationEssentials/ProgressManager/ProgressManager.swift

@@ -384,10 +384,12 @@ internal import _FoundationCollections
             state.markChildDirty(at: position)
         }
         if let parents = parents {
-            // rdar://172950622 (Async stream observation of ProgressManager's fractionCompleted does not receive updates from subsubprogress and beyond.)
             markSelfDirty(parents: parents)
-            self.withMutation(keyPath: \.completedCount) {}
         }
+        self.withMutation(keyPath: \.completedCount) {}
+        #if FOUNDATION_FRAMEWORK
+        notifyInteropObserversOfChildUpdate()
+        #endif
     }
 
     internal func updatedProgressFraction() -> ProgressFraction {
@@ -425,32 +427,30 @@ internal import _FoundationCollections
 
     //MARK: Parent - Child Relationship Methods
     internal func addChild(childManager: ProgressManager, assignedCount: Int, childFraction: ProgressFraction) -> Int {
-        self.withMutation(keyPath: \.completedCount) {
-            let (index, parents) = state.withLock { state in
-                let child = Child(manager: childManager,
-                                  assignedCount: assignedCount,
-                                  fraction: childFraction,
-                                  isFractionDirty: true,
-                                  totalFileCountSummary: PropertyStateInt(value: ProgressManager.Properties.TotalFileCount.defaultSummary, isDirty: false),
-                                  completedFileCountSummary: PropertyStateInt(value: ProgressManager.Properties.CompletedFileCount.defaultSummary, isDirty: false),
-                                  totalByteCountSummary: PropertyStateUInt64(value: ProgressManager.Properties.TotalByteCount.defaultSummary, isDirty: false),
-                                  completedByteCountSummary: PropertyStateUInt64(value: ProgressManager.Properties.CompletedByteCount.defaultSummary, isDirty: false),
-                                  throughputSummary: PropertyStateUInt64Array(value: ProgressManager.Properties.Throughput.defaultSummary, isDirty: false),
-                                  estimatedTimeRemainingSummary: PropertyStateDuration(value: ProgressManager.Properties.EstimatedTimeRemaining.defaultSummary, isDirty: false),
-                                  customPropertiesIntSummary: [:],
-                                  customPropertiesUInt64Summary: [:],
-                                  customPropertiesDoubleSummary: [:],
-                                  customPropertiesStringSummary: [:],
-                                  customPropertiesURLSummary: [:],
-                                  customPropertiesUInt64ArraySummary: [:],
-                                  customPropertiesDurationSummary: [:])
-                state.children.append(child)
-                return (state.children.count - 1, state.parents)
-            }
-            // Mark dirty all the way up to the root so that if the branch was marked not dirty right before this it will be marked dirty again (for optimization to work)
-            markSelfDirty(parents: parents)
-            return index
-        }
+        let (index, parents) = state.withLock { state in
+            let child = Child(manager: childManager,
+                              assignedCount: assignedCount,
+                              fraction: childFraction,
+                              isFractionDirty: true,
+                              totalFileCountSummary: PropertyStateInt(value: ProgressManager.Properties.TotalFileCount.defaultSummary, isDirty: false),
+                              completedFileCountSummary: PropertyStateInt(value: ProgressManager.Properties.CompletedFileCount.defaultSummary, isDirty: false),
+                              totalByteCountSummary: PropertyStateUInt64(value: ProgressManager.Properties.TotalByteCount.defaultSummary, isDirty: false),
+                              completedByteCountSummary: PropertyStateUInt64(value: ProgressManager.Properties.CompletedByteCount.defaultSummary, isDirty: false),
+                              throughputSummary: PropertyStateUInt64Array(value: ProgressManager.Properties.Throughput.defaultSummary, isDirty: false),
+                              estimatedTimeRemainingSummary: PropertyStateDuration(value: ProgressManager.Properties.EstimatedTimeRemaining.defaultSummary, isDirty: false),
+                              customPropertiesIntSummary: [:],
+                              customPropertiesUInt64Summary: [:],
+                              customPropertiesDoubleSummary: [:],
+                              customPropertiesStringSummary: [:],
+                              customPropertiesURLSummary: [:],
+                              customPropertiesUInt64ArraySummary: [:],
+                              customPropertiesDurationSummary: [:])
+            state.children.append(child)
+            return (state.children.count - 1, state.parents)
+        }
+        // Mark dirty all the way up to the root so that if the branch was marked not dirty right before this it will be marked dirty again (for optimization to work)
+        markSelfDirty(parents: parents)
+        return index
     }
 
     internal func addParent(parentManager: ProgressManager, positionInParent: Int) {

Tests/FoundationEssentialsTests/ProgressManager/ProgressManagerInteropTests.swift

@@ -303,7 +303,136 @@ import Testing
         receiveProgress(progress: interopChild)
         #expect(overallProgress2.totalUnitCount == 0)
     }
-    
+
+    @Test func interopProgressParentProgressManagerChildAndGrandchildren() async throws {
+        // Caller's NSProgress
+        let overall = Progress.discreteProgress(totalUnitCount: 10)
+        let subprogress = overall.subprogress(assigningCount: 10)
+        
+        // Receiver calls start(), then assigns work to sub-tasks via ProgressReporter
+        let child = subprogress.start(totalCount: 2)
+        let task1 = ProgressManager(totalCount: 100)
+        let task2 = ProgressManager(totalCount: 100)
+        child.assign(count: 1, to: task1.reporter)
+        child.assign(count: 1, to: task2.reporter)
+        
+        // Task 1 reports partial progress
+        task1.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.25, "Task 1 50% of half should be 25% overall: expected 0.25, got \(overall.fractionCompleted)")
+        
+        // Task 1 completes
+        task1.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.5, "Task 1 complete should be 50% overall: expected 0.5, got \(overall.fractionCompleted)")
+        
+        // Task 2 completes
+        task2.complete(count: 100)
+        #expect(overall.fractionCompleted == 1.0, "Both tasks complete should be 100%: expected 1.0, got \(overall.fractionCompleted)")
+    }
+
+    @Test func interopProgressParentProgressManagerGrandchildrenNonSequentialCompletion() async throws {
+        let overall = Progress.discreteProgress(totalUnitCount: 10)
+        let subprogress = overall.subprogress(assigningCount: 10)
+
+        let child = subprogress.start(totalCount: 2)
+        let task1 = ProgressManager(totalCount: 100)
+        let task2 = ProgressManager(totalCount: 100)
+        child.assign(count: 1, to: task1.reporter)
+        child.assign(count: 1, to: task2.reporter)
+
+        // Task 2 reports partial progress first
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.25, "Task 2 50% of half should be 25% overall: expected 0.25, got \(overall.fractionCompleted)")
+
+        // Task 2 completes before task 1
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.5, "Task 2 complete should be 50% overall: expected 0.5, got \(overall.fractionCompleted)")
+
+        // Task 1 completes
+        task1.complete(count: 100)
+        #expect(overall.fractionCompleted == 1.0, "Both tasks complete should be 100%: expected 1.0, got \(overall.fractionCompleted)")
+    }
+
+    @Test func interopProgressParentProgressManagerManyGrandchildren() async throws {
+        let overall = Progress.discreteProgress(totalUnitCount: 10)
+        let subprogress = overall.subprogress(assigningCount: 10)
+
+        let child = subprogress.start(totalCount: 5)
+        var tasks: [ProgressManager] = []
+        for _ in 0..<5 {
+            let task = ProgressManager(totalCount: 100)
+            child.assign(count: 1, to: task.reporter)
+            tasks.append(task)
+        }
+
+        // Complete each task one by one
+        for i in 0..<5 {
+            tasks[i].complete(count: 100)
+            let expected = Double(i + 1) / 5.0
+            #expect(overall.fractionCompleted == expected, "After \(i + 1) of 5 tasks complete: expected \(expected), got \(overall.fractionCompleted)")
+        }
+    }
+
+    @Test func interopProgressParentProgressManagerGrandchildrenPartialProgress() async throws {
+        let overall = Progress.discreteProgress(totalUnitCount: 10)
+        let subprogress = overall.subprogress(assigningCount: 10)
+
+        let child = subprogress.start(totalCount: 2)
+        let task1 = ProgressManager(totalCount: 100)
+        let task2 = ProgressManager(totalCount: 100)
+        child.assign(count: 1, to: task1.reporter)
+        child.assign(count: 1, to: task2.reporter)
+
+        // Task 1 at 25%
+        task1.complete(count: 25)
+        #expect(overall.fractionCompleted == 0.125, "Task 1 25% of half: expected 0.125, got \(overall.fractionCompleted)")
+
+        // Task 1 at 50%
+        task1.complete(count: 25)
+        #expect(overall.fractionCompleted == 0.25, "Task 1 50% of half: expected 0.25, got \(overall.fractionCompleted)")
+
+        // Task 2 at 50%
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.5, "Both at 50%: expected 0.5, got \(overall.fractionCompleted)")
+
+        // Task 1 completes
+        task1.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.75, "Task 1 done, task 2 at 50%: expected 0.75, got \(overall.fractionCompleted)")
+
+        // Task 2 completes
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 1.0, "Both complete: expected 1.0, got \(overall.fractionCompleted)")
+    }
+
+    @Test func interopProgressParentProgressManagerIndeterminateGrandchild() async throws {
+        let overall = Progress.discreteProgress(totalUnitCount: 10)
+        let subprogress = overall.subprogress(assigningCount: 10)
+
+        let child = subprogress.start(totalCount: 2)
+        let task1 = ProgressManager(totalCount: nil) // indeterminate
+        let task2 = ProgressManager(totalCount: 100)
+        child.assign(count: 1, to: task1.reporter)
+        child.assign(count: 1, to: task2.reporter)
+
+        // Indeterminate task should not affect overall progress
+        #expect(overall.fractionCompleted == 0.0, "No progress yet: expected 0.0, got \(overall.fractionCompleted)")
+
+        // Determinate task reports progress
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.25, "Task 2 50% of half: expected 0.25, got \(overall.fractionCompleted)")
+
+        // Indeterminate task becomes determinate
+        task1.setCounts { completed, total in
+            total = 100
+        }
+        task1.complete(count: 50)
+        #expect(overall.fractionCompleted == 0.5, "Both at 50%: expected 0.5, got \(overall.fractionCompleted)")
+
+        // Both complete
+        task1.complete(count: 50)
+        task2.complete(count: 50)
+        #expect(overall.fractionCompleted == 1.0, "Both complete: expected 1.0, got \(overall.fractionCompleted)")
+    }
+
     #if FOUNDATION_EXIT_TESTS
     @Test func indirectParticipationOfProgressInAcyclicGraph() async throws {
         await #expect(processExitsWith: .failure) {

Tests/FoundationEssentialsTests/ProgressManager/ProgressManagerTests.swift

@@ -846,4 +846,32 @@ extension Tag {
         #expect(root.fractionCompleted == 1.0)
         #expect(root.isFinished)
     }
+    
+    @Test func isFinishedObservationNotConsumedByAddChild() async throws {
+        let parent = ProgressManager(totalCount: 1)
+        let subprogress = parent.subprogress(assigningCount: 1)
+
+        // Track how many times onChange fires and what isFinished value it sees
+        let onChangeCount = Mutex(0)
+        let observedIsFinished = Mutex(false)
+        withObservationTracking {
+            _ = parent.isFinished
+        } onChange: {
+            onChangeCount.withLock { $0 += 1 }
+            observedIsFinished.withLock { $0 = parent.isFinished }
+        }
+
+        // start() calls addChild — addChild should NOT consume the observation
+        let child = subprogress.start(totalCount: 10)
+
+        // Verify addChild did not fire the observation
+        #expect(onChangeCount.withLock { $0 } == 0, "addChild should not fire observation — onChange count was \(onChangeCount.withLock { $0 })")
+
+        // Complete the child — this should fire the observation with isFinished == true
+        child.complete(count: 10)
+
+        #expect(parent.isFinished, "Parent should be finished after child completes")
+        #expect(onChangeCount.withLock { $0 } == 1, "onChange should fire exactly once (on child completion) — count was \(onChangeCount.withLock { $0 })")
+        #expect(observedIsFinished.withLock { $0 }, "isFinished observation should fire with true when child completes, not be consumed earlier by addChild")
+    }
 }