fix: wire up the trigger engine and close its daily-cap race - #8
Merged
Merged
Conversation
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.
…asks 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related fixes, discovered while investigating TODO.md's "maximumDailyNotifications ledger... verify enforcement end-to-end."
The bigger finding: the trigger engine had no live entry point anywhere
Tracing every caller of
TriggerEngineManager.evaluateAllReminders()before trusting the race was reachable turned up something much bigger than the race itself: none of its callers are reachable in production."weather-refresh"and"trigger-evaluation"BGTasks are both registered but never seeded — nothing calls the permission-request methods that would submit the firstBGAppRefreshTaskRequest.BackgroundWeatherManager.manualRefresh()has zero callers anywhere.WeatherData.evaluateCondition()calls elsewhere in the app are UI-only previews (trigger-prediction cards, the detail screen's live match indicator) — none of them persist a trigger or send a real notification.Net effect: reminders were never evaluated and notifications never sent, in any context, foreground or background. This is different from the earlier background-refresh-quota question (already answered, deliberately left dormant) — this was the app's actual core promise doing nothing at all.
Fix: Dashboard's existing foreground refresh (on launch, and its 5-minute poll when weather data is more than 10 minutes stale) now also evaluates reminders after a successful weather fetch, as an independent
Taskso evaluation latency doesn't hold up the dashboard's own loading indicator. This is the narrowest fix that makes the core loop actually run. Seeding either BGTask remains a separate, deliberately deferred decision with its own quota/battery tradeoffs — not part of this PR.The original finding: a daily-cap race across background tasks
handleBackgroundEvaluation(the"trigger-evaluation"BGTask handler) calledtriggerEngine.evaluateAllActiveReminders()andprocessEvaluationResults()directly, bypassing theisEvaluatingguard every other caller ofevaluateAllReminders()goes through. If this BGTask and another evaluation cycle ever ran close together, both could read a staledailyNotificationCountbefore either recorded its delivery, letting the actual delivered count exceedmaximumDailyNotifications.Fix:
handleBackgroundEvaluationnow routes through the same guardedevaluateAllReminders(isBackground: true)every other caller uses, dropping the duplicate bookkeeping it used to maintain separately.processEvaluationResults's results loop is a plain sequentialforloop (verified — no concurrent dispatch), so with every entry point now sharing one guard, the cap is correctly enforced within and across evaluation cycles. This closes the race cleanly enough that no separate fix to the two independentUserPreferencesfetches insidesendNotificationForResult/recordDeliveryis needed.Verification
SunHatTestssuite: 298 tests / 60 suites, all passed (13.4s) — the one new test: two different reminders both triggered in the same evaluation cycle,maximumDailyNotifications = 1, confirms only the first is delivered and the persisted count reflects it. This is the exact scenario the TODO's notification-ledger item asked to verify and no existing test covered.TriggerEngineManagerisn't currently injectable intoDashboardViewModel(it's referenced as.shared, matching this class's existing pattern forWeatherService/LocationPermissionManager), and making it injectable for one new call felt like more surface area than this fix warranted. Verified by code review and the build; a real fix worth having regardless.