Skip to content

Commit db88524

Browse files
committed
Fix scheduled explosion cleanup getting stuck (#475)
<!-- Macroscope's pull request summary starts here --> <!-- Macroscope will only edit the content between these invisible markers, and the markers themselves will not be visible in the GitHub rendered markdown. --> <!-- If you delete either of the start / end markers from your PR's description, Macroscope will append its summary at the bottom of the description. --> ### Prevent scheduled explosion cleanup from getting stuck by adding per-conversation expiration tasks and immediate cancellation in `ScheduledExplosionManager` Add `_expirationTasks` with locking, schedule per-conversation expiration via `ScheduledExplosionManager.scheduleExpirationTask`, post `.conversationExpired` on expiry or immediately if past, and cancel all tasks in `deinit` and `cancelNotifications(for:)`. Include tests for expiry posting and rescan behavior. #### 📍Where to Start Start with `handleScheduledExplosion` in [ScheduledExplosionManager.swift](https://github.com/xmtplabs/convos-ios/pull/475/files#diff-02b035d174d75b0dde80ebc879a4bc253710f806fa9c6aae3f49b2f1a798abcb), then review `scheduleExpirationTask` and `cancelAllTasks`. <!-- Macroscope's review summary starts here --> <sup><a href="https://app.macroscope.com">Macroscope</a> summarized 106ede7.</sup> <!-- Macroscope's review summary ends here --> <!-- Macroscope's pull request summary ends here -->
1 parent 2afc1b5 commit db88524

2 files changed

Lines changed: 122 additions & 2 deletions

File tree

ConvosCore/Sources/ConvosCore/Storage/Workers/ScheduledExplosionManager.swift

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,16 @@ public protocol ScheduledExplosionManagerProtocol {}
66

77
/// @unchecked Sendable: Protocol dependencies (DatabaseReader, AppLifecycle)
88
/// are all Sendable. The `observers` array is only modified during init and deinit.
9-
/// The `schedulingTasks` dictionary is protected by `taskLock` to prevent data races
10-
/// from concurrent notification callbacks.
9+
/// The `schedulingTasks` and `expirationTasks` dictionaries are protected by `taskLock`
10+
/// to prevent data races from concurrent notification callbacks.
1111
final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unchecked Sendable {
1212
private let databaseReader: any DatabaseReader
1313
private let appLifecycle: any AppLifecycleProviding
1414
private let notificationCenter: any UserNotificationCenterProtocol
1515
nonisolated(unsafe) private var observers: [NSObjectProtocol] = []
1616
private let taskLock: NSLock = NSLock()
1717
private var _schedulingTasks: [String: Task<Void, Never>] = [:]
18+
private var _expirationTasks: [String: Task<Void, Never>] = [:]
1819

1920
private enum Constant {
2021
static let reminderIdentifierPrefix: String = "explosion-reminder-"
@@ -37,6 +38,7 @@ final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unche
3738
deinit {
3839
Log.warning("ScheduledExplosionManager deinit - removing observers")
3940
observers.forEach { NotificationCenter.default.removeObserver($0) }
41+
cancelAllTasks()
4042
}
4143

4244
private func setupObservers() {
@@ -83,6 +85,8 @@ final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unche
8385
self.removeSchedulingTask(for: conversationId)
8486
}
8587
taskLock.unlock()
88+
89+
scheduleExpirationTask(conversationId: conversationId, expiresAt: expiresAt)
8690
}
8791

8892
private nonisolated func removeSchedulingTask(for conversationId: String) {
@@ -91,6 +95,43 @@ final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unche
9195
taskLock.unlock()
9296
}
9397

98+
private nonisolated func removeExpirationTask(for conversationId: String) {
99+
taskLock.lock()
100+
_expirationTasks[conversationId] = nil
101+
taskLock.unlock()
102+
}
103+
104+
private func scheduleExpirationTask(conversationId: String, expiresAt: Date) {
105+
taskLock.lock()
106+
_expirationTasks[conversationId]?.cancel()
107+
_expirationTasks[conversationId] = Task { [weak self] in
108+
guard let self else { return }
109+
110+
let interval = expiresAt.timeIntervalSinceNow
111+
guard interval > 0 else {
112+
await self.postConversationExpired(conversationId: conversationId)
113+
self.removeExpirationTask(for: conversationId)
114+
return
115+
}
116+
117+
try? await Task.sleep(for: .seconds(interval))
118+
guard !Task.isCancelled else { return }
119+
120+
await self.postConversationExpired(conversationId: conversationId)
121+
self.removeExpirationTask(for: conversationId)
122+
}
123+
taskLock.unlock()
124+
}
125+
126+
@MainActor
127+
private func postConversationExpired(conversationId: String) {
128+
NotificationCenter.default.post(
129+
name: .conversationExpired,
130+
object: nil,
131+
userInfo: ["conversationId": conversationId]
132+
)
133+
}
134+
94135
func hasSchedulingTask(for conversationId: String) -> Bool {
95136
taskLock.lock()
96137
defer { taskLock.unlock() }
@@ -141,6 +182,10 @@ final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unche
141182
expiresAt: conversation.expiresAt,
142183
conversationName: conversation.name
143184
)
185+
scheduleExpirationTask(
186+
conversationId: conversation.conversationId,
187+
expiresAt: conversation.expiresAt
188+
)
144189
}
145190
} catch {
146191
Log.error("Failed to query scheduled explosions: \(error)")
@@ -241,16 +286,29 @@ final class ScheduledExplosionManager: ScheduledExplosionManagerProtocol, @unche
241286
if let name, !name.isEmpty {
242287
return name
243288
}
289+
} catch is CancellationError {
290+
return "Untitled"
244291
} catch {
245292
Log.error("Failed to fetch conversation name: \(error)")
246293
}
247294
return "Untitled"
248295
}
249296

297+
private func cancelAllTasks() {
298+
taskLock.lock()
299+
_schedulingTasks.values.forEach { $0.cancel() }
300+
_expirationTasks.values.forEach { $0.cancel() }
301+
_schedulingTasks.removeAll()
302+
_expirationTasks.removeAll()
303+
taskLock.unlock()
304+
}
305+
250306
private func cancelNotifications(for conversationId: String) {
251307
taskLock.lock()
252308
_schedulingTasks[conversationId]?.cancel()
253309
_schedulingTasks[conversationId] = nil
310+
_expirationTasks[conversationId]?.cancel()
311+
_expirationTasks[conversationId] = nil
254312
taskLock.unlock()
255313
let reminderIdentifier = "\(Constant.reminderIdentifierPrefix)\(conversationId)"
256314
let explosionIdentifier = "\(Constant.explosionIdentifierPrefix)\(conversationId)"

ConvosCore/Tests/ConvosCoreTests/ScheduledExplosionManagerTests.swift

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,68 @@ struct ScheduledExplosionManagerTests {
167167
#expect(removedIds.contains("explosion-\(conversationId)"))
168168
}
169169

170+
@Test("Posts conversationExpired at expiresAt after scheduling")
171+
func testPostsConversationExpiredAtExpiry() async throws {
172+
let fixtures = ScheduledExplosionTestFixtures()
173+
let expiresAt = Date().addingTimeInterval(1.0)
174+
175+
let capture = NotificationCapture()
176+
capture.startCapturing(.conversationExpired)
177+
defer { capture.stopCapturing() }
178+
179+
let conversationId = fixtures.conversationId
180+
181+
await MainActor.run {
182+
NotificationCenter.default.post(
183+
name: .conversationScheduledExplosion,
184+
object: nil,
185+
userInfo: [
186+
"conversationId": conversationId,
187+
"expiresAt": expiresAt
188+
]
189+
)
190+
}
191+
192+
try await waitUntil(timeout: .seconds(3)) {
193+
capture.hasNotification(.conversationExpired)
194+
}
195+
196+
let notifications = capture.notifications(named: .conversationExpired)
197+
let matched = notifications.contains {
198+
($0.userInfo?["conversationId"] as? String) == conversationId
199+
}
200+
#expect(matched, "Should post conversationExpired for scheduled conversation")
201+
}
202+
203+
@Test("App active rescan schedules deterministic expiry cleanup")
204+
func testRescanSchedulesExpiryCleanup() async throws {
205+
let fixtures = ScheduledExplosionTestFixtures()
206+
let expiresAt = Date().addingTimeInterval(1.0)
207+
try await fixtures.setupConversation(expiresAt: expiresAt)
208+
209+
let capture = NotificationCapture()
210+
capture.startCapturing(.conversationExpired)
211+
defer { capture.stopCapturing() }
212+
213+
let activeNotification = fixtures.appLifecycle.didBecomeActiveNotification
214+
let conversationId = fixtures.conversationId
215+
216+
await MainActor.run {
217+
NotificationCenter.default.post(name: activeNotification, object: nil)
218+
}
219+
220+
try await waitUntil(timeout: .seconds(3)) {
221+
capture.notifications(named: .conversationExpired).contains {
222+
($0.userInfo?["conversationId"] as? String) == conversationId
223+
}
224+
}
225+
226+
let matched = capture.notifications(named: .conversationExpired).contains {
227+
($0.userInfo?["conversationId"] as? String) == conversationId
228+
}
229+
#expect(matched, "Should post conversationExpired after app-active rescan when expiresAt is reached")
230+
}
231+
170232
@Test("Notification content has correct format")
171233
func testNotificationContentFormat() async throws {
172234
let fixtures = ScheduledExplosionTestFixtures()

0 commit comments

Comments
 (0)