From 62dfd98838c52b7375b7b4d6d6cabc50b7c8a0e7 Mon Sep 17 00:00:00 2001 From: Wesley Keetch Date: Sun, 30 Aug 2026 18:46:59 -0400 Subject: [PATCH 1/2] fix: wire up the trigger engine's only live evaluation entry point Traced every caller of TriggerEngineManager.evaluateAllReminders() while investigating a background-task race and found none are reachable in production: the weather-refresh and trigger-evaluation BGTasks are both registered but never seeded (nothing calls their permission-request methods), BackgroundWeatherManager.manualRefresh() has zero callers, and no App Intent or manual action reaches it either. WeatherData.evaluateCondition() calls elsewhere are UI-only previews (trigger-prediction cards, the detail screen's live match indicator) that never persist a trigger or notify. Net effect: reminders were never evaluated and notifications never sent, in any context, foreground or background. Dashboard's existing foreground refresh (on launch, and its 5-minute poll when data is >10 min stale) now also evaluates reminders after a successful weather fetch, as an independent Task so evaluation latency (it fetches weather per reminder location, not just the dashboard's own) doesn't hold up the dashboard's own loading indicator. This is the narrowest fix that makes the core notification loop actually run; seeding either BGTask remains a separate, deliberately deferred decision with its own quota/battery tradeoffs. --- SunHat/ViewModels/DashboardViewModel.swift | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/SunHat/ViewModels/DashboardViewModel.swift b/SunHat/ViewModels/DashboardViewModel.swift index fddc2e4..4597f13 100644 --- a/SunHat/ViewModels/DashboardViewModel.swift +++ b/SunHat/ViewModels/DashboardViewModel.swift @@ -162,6 +162,14 @@ final class DashboardViewModel: ObservableObject { logger.info("Weather data refresh completed successfully") + // Fresh weather may satisfy a reminder's condition. Evaluation + // fetches weather per reminder location (not just this + // dashboard's own location), so it runs as an independent task + // rather than blocking this refresh's own isLoading state. + Task { + await TriggerEngineManager.shared.evaluateAllReminders() + } + } catch is CancellationError { // A superseded refresh is not an error; leave current state as-is. isLoading = false From fa9aeb1dee353112f21c04e48b318d2c8f9d6cc1 Mon Sep 17 00:00:00 2001 From: Wesley Keetch Date: Sun, 30 Aug 2026 18:47:10 -0400 Subject: [PATCH 2/2] fix: close the daily-notification-cap TOCTOU race across background tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleBackgroundEvaluation (the 'trigger-evaluation' BGTask handler) called triggerEngine.evaluateAllActiveReminders() and processEvaluationResults() directly, bypassing the isEvaluating guard that every other caller of evaluateAllReminders() goes through. If this BGTask and another evaluation cycle (e.g. the weather-refresh BGTask, or the dashboard-driven evaluation added in the previous commit) ever ran close together, both could read a stale dailyNotificationCount before either recorded its delivery, letting the actual delivered count exceed maximumDailyNotifications. handleBackgroundEvaluation now routes its work through the same guarded evaluateAllReminders(isBackground: true) every other caller uses, dropping the duplicate isEvaluating/lastEvaluationTime/evaluationResults/performance bookkeeping it used to maintain separately. processEvaluationResults' results loop is a plain sequential for loop (verified, no concurrent dispatch), so with every entry point now sharing one guard, the cap is correctly enforced within and across evaluation cycles without needing a separate fix to the UserPreferences fetch inside sendNotificationForResult. New test: two different reminders triggered in the same evaluation cycle, maximumDailyNotifications=1, confirms only the first is delivered and the persisted count reflects it — the exact scenario the TODO's notification- ledger item asked to verify and no existing test covered. --- .../Trigger/TriggerEngineManager.swift | 30 +++----- SunHatTests/TriggerEngineManagerTests.swift | 75 +++++++++++++++++++ 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/SunHat/Services/Trigger/TriggerEngineManager.swift b/SunHat/Services/Trigger/TriggerEngineManager.swift index bc92cc1..7ccc74b 100644 --- a/SunHat/Services/Trigger/TriggerEngineManager.swift +++ b/SunHat/Services/Trigger/TriggerEngineManager.swift @@ -167,19 +167,14 @@ final class TriggerEngineManager: ObservableObject { private func handleBackgroundEvaluation(_ task: BGAppRefreshTask) async { logger.info("Starting background trigger evaluation") - guard let triggerEngine = triggerEngine else { - logger.error("TriggerEngine not configured for background evaluation") - task.setTaskCompleted(success: false) - return - } - - let startTime = Date() - - let workTask = Task { () -> [TriggerEvaluationResult] in - let results = await triggerEngine.evaluateAllActiveReminders() - guard !Task.isCancelled else { return [] } - await processEvaluationResults(results, isBackground: true) - return results + // Routed through the same guarded entry point as every other + // caller, so this can never run concurrently with a foreground or + // weather-refresh-triggered evaluation and double-count against + // the daily notification cap. evaluateAllReminders already does + // all the bookkeeping (isEvaluating, lastEvaluationTime, + // evaluationResults, performance metrics) this used to duplicate. + let workTask = Task { + await evaluateAllReminders(isBackground: true) } task.expirationHandler = { @@ -187,14 +182,7 @@ final class TriggerEngineManager: ObservableObject { workTask.cancel() } - let results = await workTask.value - let duration = Date().timeIntervalSince(startTime) - - lastEvaluationTime = Date() - evaluationResults = results - updatePerformanceMetrics(duration: duration) - - logger.info("Background evaluation completed: \(results.count) reminders in \(duration)s") + await workTask.value scheduleBackgroundEvaluation() task.setTaskCompleted(success: !workTask.isCancelled) } diff --git a/SunHatTests/TriggerEngineManagerTests.swift b/SunHatTests/TriggerEngineManagerTests.swift index a3596c3..9a1806e 100644 --- a/SunHatTests/TriggerEngineManagerTests.swift +++ b/SunHatTests/TriggerEngineManagerTests.swift @@ -104,6 +104,61 @@ struct TriggerEngineManagerTests { #expect(persisted?.totalNotificationsSent == 1) } + @Test("The daily notification cap is enforced across different reminders in one evaluation cycle") + func dailyCapSuppressesLaterRemindersInSameCycle() async throws { + let container = try makeContainer() + let context = ModelContext(container) + + let conditionA = TriggerCondition() + let reminderA = WeatherReminder(title: "Water plants", triggerCondition: conditionA) + let conditionB = TriggerCondition() + let reminderB = WeatherReminder(title: "Walk the dog", triggerCondition: conditionB) + + let preferences = UserPreferences() + preferences.maximumDailyNotifications = 1 + preferences.dailyNotificationCount = 0 + // Avoid the test's outcome depending on the wall-clock time or day + // of week it happens to run on; only the daily cap is under test. + preferences.quietHoursEnabled = false + preferences.allowWeekendNotifications = true + + context.insert(reminderA) + context.insert(reminderB) + context.insert(preferences) + try context.save() + + let sender = RecordingTriggerNotificationSender() + let manager = TriggerEngineManager( + modelContainer: container, + registerBackgroundTask: false, + notificationManager: sender + ) + let resultA = TriggerEvaluationResult( + reminderId: reminderA.id, + conditionData: ModelDataConverter.convertTriggerCondition(conditionA), + triggered: true, + triggerReason: "Condition met" + ) + let resultB = TriggerEvaluationResult( + reminderId: reminderB.id, + conditionData: ModelDataConverter.convertTriggerCondition(conditionB), + triggered: true, + triggerReason: "Condition met" + ) + + // processEvaluationResults processes its results sequentially (a plain + // for loop, no concurrent dispatch), so within a single evaluation + // cycle the cap check for reminderB always sees reminderA's already- + // recorded delivery, regardless of the results' relative ordering. + await manager.processEvaluationResults([resultA, resultB]) + + #expect(await sender.deliveryCount == 1) + #expect(manager.triggeredReminders == [reminderA.id]) + + let refreshedPreferences = try Self.fetchPreferences(in: container) + #expect(refreshedPreferences?.dailyNotificationCount == 1) + } + @Test("A failed delivery remains eligible for a later retry") func failedDeliveryCanRetry() async throws { let container = try makeContainer() @@ -145,6 +200,13 @@ struct TriggerEngineManagerTests { return try context.fetch(descriptor).first } + /// Reads UserPreferences back through a fresh context, so assertions see + /// persisted state rather than the stale instance the test inserted. + private static func fetchPreferences(in container: ModelContainer) throws -> UserPreferences? { + let context = ModelContext(container) + return try context.fetch(FetchDescriptor()).first + } + private func makeContainer() throws -> ModelContainer { let configuration = ModelConfiguration( schema: SunHatModelSchema.schema, @@ -157,6 +219,19 @@ struct TriggerEngineManagerTests { } } +private actor RecordingTriggerNotificationSender: TriggerNotificationSending { + private(set) var deliveryCount = 0 + + func configure(modelContainer: ModelContainer?) async {} + + func sendTriggerNotification( + for result: TriggerEvaluationResult, + isBackground: Bool + ) async throws { + deliveryCount += 1 + } +} + private actor FailOnceTriggerNotificationSender: TriggerNotificationSending { private(set) var deliveryCount = 0