From 1b60a5029c172571faaf6a66303b06dbffa10406 Mon Sep 17 00:00:00 2001 From: Xiaochuan Hu Date: Thu, 23 Jul 2026 12:54:33 +0800 Subject: [PATCH 1/2] fix timer session cleanup race --- .github/workflows/ci.yml | 3 ++ .gitignore | 1 + .../managers/SystemTimerBridge.swift | 11 +++-- DynamicIsland/managers/TimerLifecycle.swift | 32 +++++++++++++ DynamicIsland/managers/TimerManager.swift | 46 ++++++++++++++++++- tests/TimerLifecycleRegression.swift | 29 ++++++++++++ tests/test_timer_lifecycle.py | 35 ++++++++++++++ 7 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 DynamicIsland/managers/TimerLifecycle.swift create mode 100644 tests/TimerLifecycleRegression.swift create mode 100644 tests/test_timer_lifecycle.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e4d723c5..6887ff3f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,9 @@ jobs: - name: Validate privacy configuration run: python3 -m unittest tests.test_privacy_configuration + - name: Validate timer lifecycle cleanup + run: python3 -m unittest tests.test_timer_lifecycle + - name: Download Metal Toolchain # The downloadable Metal Toolchain is an Xcode 26+ (macOS 26) component; older Xcode ships metal. run: | diff --git a/.gitignore b/.gitignore index 632bf871..17167ffe 100644 --- a/.gitignore +++ b/.gitignore @@ -140,6 +140,7 @@ PrivateWorks *.py !tests/test_privacy_configuration.py +!tests/test_timer_lifecycle.py *.sh comparison_report.json *.txt diff --git a/DynamicIsland/managers/SystemTimerBridge.swift b/DynamicIsland/managers/SystemTimerBridge.swift index a5e34685..e37ee8e8 100644 --- a/DynamicIsland/managers/SystemTimerBridge.swift +++ b/DynamicIsland/managers/SystemTimerBridge.swift @@ -822,12 +822,17 @@ final class SystemTimerBridge { DispatchQueue.main.async { guard TimerManager.shared.isExternalTimerActive else { return } if didComplete { + guard let completedSessionID = TimerManager.shared.activeSessionID else { return } self.logDebug("Timer finished; scheduling delayed cleanup") DispatchQueue.main.asyncAfter(deadline: .now() + 3.2) { - if TimerManager.shared.isExternalTimerActive { - self.logDebug("Delayed cleanup firing; ending external timer") - TimerManager.shared.endExternalTimer(triggerSmoothClose: false) + guard TimerManager.shared.activeSource == .external, + TimerManager.shared.activeSessionID == completedSessionID else { + self.logDebug("Skipping delayed cleanup because a newer timer session is active") + return } + + self.logDebug("Delayed cleanup firing; ending external timer") + TimerManager.shared.endExternalTimer(triggerSmoothClose: false) } } else { self.logDebug("Ending external timer immediately") diff --git a/DynamicIsland/managers/TimerLifecycle.swift b/DynamicIsland/managers/TimerLifecycle.swift new file mode 100644 index 00000000..450c622e --- /dev/null +++ b/DynamicIsland/managers/TimerLifecycle.swift @@ -0,0 +1,32 @@ +/* + * Atoll (DynamicIsland) + * Copyright (C) 2024-2026 Atoll Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + */ + +import Foundation + +/// Gives each timer run its own identity so delayed work from an earlier run +/// cannot mutate a timer that was started in the meantime. +struct TimerLifecycle { + private(set) var sessionID: UUID? + + @discardableResult + mutating func beginSession() -> UUID { + let sessionID = UUID() + self.sessionID = sessionID + return sessionID + } + + mutating func endSession() { + sessionID = nil + } + + func isCurrent(_ candidate: UUID) -> Bool { + sessionID == candidate + } +} diff --git a/DynamicIsland/managers/TimerManager.swift b/DynamicIsland/managers/TimerManager.swift index 88fe3876..576e3ec0 100644 --- a/DynamicIsland/managers/TimerManager.swift +++ b/DynamicIsland/managers/TimerManager.swift @@ -125,6 +125,14 @@ class TimerManager: ObservableObject { private var timerInstance: Timer? private var cancellables = Set() private var soundPlayer: AVAudioPlayer? + private var smoothCloseWorkItem: DispatchWorkItem? + private var lifecycle = TimerLifecycle() + + /// Identifies the current timer run. Delayed cleanup must match this value + /// before changing presentation state. + var activeSessionID: UUID? { + lifecycle.sessionID + } // MARK: - Initialization private init() { // Simple initialization @@ -132,6 +140,7 @@ class TimerManager: ObservableObject { deinit { timerInstance?.invalidate() + smoothCloseWorkItem?.cancel() soundPlayer?.stop() cancellables.removeAll() } @@ -144,6 +153,8 @@ class TimerManager: ObservableObject { // Stop any existing timer timerInstance?.invalidate() + soundPlayer?.stop() + beginTimerSession() // Start new timer withAnimation(.smooth) { @@ -275,6 +286,7 @@ class TimerManager: ObservableObject { timerInstance?.invalidate() timerInstance = nil soundPlayer?.stop() + beginTimerSession() activeSource = .external @@ -298,6 +310,16 @@ class TimerManager: ObservableObject { func updateExternalTimer(remaining: TimeInterval, totalDuration: TimeInterval?, isPaused paused: Bool, name: String? = nil) { guard activeSource == .external else { return } + // Clock may reuse both the timer ID and duration. A positive update + // after completion is nevertheless a new run and must invalidate the + // previous run's delayed close. + if remaining > 0 && (isFinished || isOvertime || remainingTime <= 0) { + beginTimerSession() + withAnimation(.smooth) { + isTimerActive = true + } + } + if let name, !name.isEmpty, name != timerName { timerName = name } @@ -342,6 +364,7 @@ class TimerManager: ObservableObject { } private func resetTimer() { + endTimerSession() withAnimation(.smooth) { isTimerActive = false } @@ -381,12 +404,33 @@ class TimerManager: ObservableObject { } private func scheduleSmoothClose() { + guard let sessionID = activeSessionID else { return } + + smoothCloseWorkItem?.cancel() + // Wait 3 seconds then smoothly close the live activity - DispatchQueue.main.asyncAfter(deadline: .now() + 3.0) { + let workItem = DispatchWorkItem { [weak self] in + guard let self, self.lifecycle.isCurrent(sessionID) else { return } + self.smoothCloseWorkItem = nil withAnimation(.easeInOut(duration: 1.0)) { self.isTimerActive = false } } + + smoothCloseWorkItem = workItem + DispatchQueue.main.asyncAfter(deadline: .now() + 3.0, execute: workItem) + } + + private func beginTimerSession() { + smoothCloseWorkItem?.cancel() + smoothCloseWorkItem = nil + lifecycle.beginSession() + } + + private func endTimerSession() { + smoothCloseWorkItem?.cancel() + smoothCloseWorkItem = nil + lifecycle.endSession() } private func playTimerSound() { diff --git a/tests/TimerLifecycleRegression.swift b/tests/TimerLifecycleRegression.swift new file mode 100644 index 00000000..414bbcc7 --- /dev/null +++ b/tests/TimerLifecycleRegression.swift @@ -0,0 +1,29 @@ +import Foundation + +@main +struct TimerLifecycleRegression { + static func main() { + var lifecycle = TimerLifecycle() + + let completedTimer = lifecycle.beginSession() + precondition(lifecycle.isCurrent(completedTimer)) + var islandIsVisible = true + + // Reusing the same duration or external timer ID must still create a + // different run. Cleanup captured for completedTimer is now stale. + let replacementTimer = lifecycle.beginSession() + precondition(replacementTimer != completedTimer) + precondition(!lifecycle.isCurrent(completedTimer)) + precondition(lifecycle.isCurrent(replacementTimer)) + + // This is the guard used by delayed Island cleanup. The stale closure + // must leave the replacement timer visible. + if lifecycle.isCurrent(completedTimer) { + islandIsVisible = false + } + precondition(islandIsVisible) + + lifecycle.endSession() + precondition(!lifecycle.isCurrent(replacementTimer)) + } +} diff --git a/tests/test_timer_lifecycle.py b/tests/test_timer_lifecycle.py new file mode 100644 index 00000000..64c8ad1b --- /dev/null +++ b/tests/test_timer_lifecycle.py @@ -0,0 +1,35 @@ +import os +import pathlib +import subprocess +import tempfile +import unittest + + +ROOT = pathlib.Path(__file__).resolve().parents[1] + + +class TimerLifecycleTests(unittest.TestCase): + def test_replacement_session_invalidates_delayed_cleanup_token(self): + with tempfile.TemporaryDirectory() as temporary_directory: + temporary_path = pathlib.Path(temporary_directory) + executable = temporary_path / "timer-lifecycle-regression" + environment = os.environ.copy() + environment["CLANG_MODULE_CACHE_PATH"] = str(temporary_path / "module-cache") + environment["SWIFT_MODULECACHE_PATH"] = str(temporary_path / "module-cache") + subprocess.run( + [ + "swiftc", + str(ROOT / "DynamicIsland/managers/TimerLifecycle.swift"), + str(ROOT / "tests/TimerLifecycleRegression.swift"), + "-o", + str(executable), + ], + check=True, + cwd=ROOT, + env=environment, + ) + subprocess.run([str(executable)], check=True, cwd=ROOT) + + +if __name__ == "__main__": + unittest.main() From 5dc7198ff5bf06340d7cf1ca55c17739ba16b5ee Mon Sep 17 00:00:00 2001 From: Xiaochuan Hu Date: Thu, 23 Jul 2026 13:18:36 +0800 Subject: [PATCH 2/2] fix completed timer session capture --- DynamicIsland/managers/SystemTimerBridge.swift | 2 +- DynamicIsland/managers/TimerLifecycle.swift | 9 +++++++++ DynamicIsland/managers/TimerManager.swift | 7 +++++++ tests/TimerLifecycleRegression.swift | 3 +++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/DynamicIsland/managers/SystemTimerBridge.swift b/DynamicIsland/managers/SystemTimerBridge.swift index e37ee8e8..5ca2e36b 100644 --- a/DynamicIsland/managers/SystemTimerBridge.swift +++ b/DynamicIsland/managers/SystemTimerBridge.swift @@ -822,7 +822,7 @@ final class SystemTimerBridge { DispatchQueue.main.async { guard TimerManager.shared.isExternalTimerActive else { return } if didComplete { - guard let completedSessionID = TimerManager.shared.activeSessionID else { return } + guard let completedSessionID = TimerManager.shared.completedSessionID else { return } self.logDebug("Timer finished; scheduling delayed cleanup") DispatchQueue.main.asyncAfter(deadline: .now() + 3.2) { guard TimerManager.shared.activeSource == .external, diff --git a/DynamicIsland/managers/TimerLifecycle.swift b/DynamicIsland/managers/TimerLifecycle.swift index 450c622e..88a4835c 100644 --- a/DynamicIsland/managers/TimerLifecycle.swift +++ b/DynamicIsland/managers/TimerLifecycle.swift @@ -14,16 +14,25 @@ import Foundation /// cannot mutate a timer that was started in the meantime. struct TimerLifecycle { private(set) var sessionID: UUID? + private(set) var completedSessionID: UUID? @discardableResult mutating func beginSession() -> UUID { let sessionID = UUID() self.sessionID = sessionID + completedSessionID = nil return sessionID } + @discardableResult + mutating func completeSession() -> UUID? { + completedSessionID = sessionID + return completedSessionID + } + mutating func endSession() { sessionID = nil + completedSessionID = nil } func isCurrent(_ candidate: UUID) -> Bool { diff --git a/DynamicIsland/managers/TimerManager.swift b/DynamicIsland/managers/TimerManager.swift index 576e3ec0..8a7d9222 100644 --- a/DynamicIsland/managers/TimerManager.swift +++ b/DynamicIsland/managers/TimerManager.swift @@ -133,6 +133,12 @@ class TimerManager: ObservableObject { var activeSessionID: UUID? { lifecycle.sessionID } + + /// Session that most recently reached completion. Starting a replacement + /// session clears this value before any delayed cleanup can capture it. + var completedSessionID: UUID? { + lifecycle.completedSessionID + } // MARK: - Initialization private init() { // Simple initialization @@ -341,6 +347,7 @@ class TimerManager: ObservableObject { func completeExternalTimer() { guard activeSource == .external else { return } + lifecycle.completeSession() remainingTime = 0 elapsedTime = totalDuration isOvertime = false diff --git a/tests/TimerLifecycleRegression.swift b/tests/TimerLifecycleRegression.swift index 414bbcc7..03804a9a 100644 --- a/tests/TimerLifecycleRegression.swift +++ b/tests/TimerLifecycleRegression.swift @@ -7,6 +7,8 @@ struct TimerLifecycleRegression { let completedTimer = lifecycle.beginSession() precondition(lifecycle.isCurrent(completedTimer)) + precondition(lifecycle.completeSession() == completedTimer) + precondition(lifecycle.completedSessionID == completedTimer) var islandIsVisible = true // Reusing the same duration or external timer ID must still create a @@ -15,6 +17,7 @@ struct TimerLifecycleRegression { precondition(replacementTimer != completedTimer) precondition(!lifecycle.isCurrent(completedTimer)) precondition(lifecycle.isCurrent(replacementTimer)) + precondition(lifecycle.completedSessionID == nil) // This is the guard used by delayed Island cleanup. The stale closure // must leave the replacement timer visible.