Fix/workmanager scope and lifecycle - #83
Merged
Merged
Conversation
Workmanager().cancelAll() maps straight to the native WorkManager.cancelAllWork() (verified in the workmanager_android plugin source) — unscoped, OS-wide, not limited to jobs the Dart plugin itself registered. EdgeApplication.onCreate() (native Kotlin) schedules the KeepAliveWorker FGS-restart watchdog on that same shared WorkManager instance BEFORE this Dart main() runs, so the unscoped cancelAll() was wiping the watchdog out on every single cold start — silently defeating the exact background-reliability mechanism it exists for. Replaced with cancelByUniqueName() targeting only the two legacy task names (openstrap.derive.heavy / openstrap.sync) this cleanup actually means to remove. Made those names public in background_derivation.dart (kHeavyDeriveTaskName / kSyncTaskName) instead of duplicating the literals in main.dart. flutter analyze: no new issues.
- app.dart: construct _LiveBanner's AnimationController in initState instead of a late-final field initializer. - pressable.dart: guard the pointer-event setState calls with `mounted` — pointer callbacks can fire after the widget disposes. - step_calibration_screen.dart: cache the AppState ref in initState and use it in dispose() instead of calling context.read there, which can fail once the widget tree is tearing down. - health_export.dart: export continuous per-minute heart rate from decoded_onehz; spread active/basal energy into hourly buckets; subtract workout calories from the daily active-energy total to avoid double-counting against the totalEnergyBurned already attached to each exported HKWorkout.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe changes add persisted health-export retries, hourly energy and minute-level heart-rate samples, public WorkManager task names with Android cleanup, and lifecycle-safe widget state handling. ChangesHealth data export
WorkManager task cleanup
Widget lifecycle handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HealthExport
participant LocalDb
participant HealthPlatform
HealthExport->>LocalDb: Load retry state and export cursor
LocalDb-->>HealthExport: Return calories, workouts, and minute readings
HealthExport->>HealthExport: Compute hourly energy and minute heart-rate samples
HealthExport->>HealthPlatform: Delete and write health samples
HealthPlatform-->>HealthExport: Return operation results
HealthExport->>LocalDb: Persist retry state and export cursor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/health/health_export.dart`:
- Around line 274-284: Update _exportDay and the related export blocks around
the workout query and health writes so failures are not swallowed: catch errors
per sample, track an aggregate success flag, mark workout-query failures as
unsuccessful, and return false whenever required prerequisites or expected
writes fail. Preserve processing of remaining samples while ensuring finalized
days cannot advance after partial exports.
- Around line 275-277: Update the sessionsInRange call in the health export flow
to make the upper bound exclusive by passing dayEnd minus one second, or use an
equivalent exclusive-range query. Keep dayStart inclusive so sessions starting
at the next midnight are excluded from the current day.
- Around line 288-295: Update the daily health export logic around the hourly
ACTIVE_ENERGY_BURNED loop to calculate the next local calendar midnight rather
than using dayStart plus 24 elapsed hours. Use that boundary for the hourly
iteration and associated heart-rate query, ensuring buckets remain within the
intended local date across DST transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8e9cc22-62fa-4c19-a6d7-e0b9398a266d
📒 Files selected for processing (6)
lib/app.dartlib/compute/background_derivation.dartlib/health/health_export.dartlib/main.dartlib/ui/design/pressable.dartlib/ui/today/step_calibration_screen.dart
- Aggregate success per day instead of unconditionally returning true: every write/query now catches independently (was one try/catch per loop, so a single failed write aborted the rest of that loop's samples too) and _exportDay returns false if anything failed. - Exclusive upper bound (dayEnd - 1s) on both sessionsInRange calls (calorie subtraction AND workout export) so a workout starting exactly at midnight no longer gets double-subtracted/double-written across both adjacent days. - DST-safe day boundary: dayEnd is now the real next local midnight (calendar-field DateTime construction) instead of dayStart + 24h of absolute Duration, and the hourly active/basal energy buckets are built from that real span instead of fixed 1-hour offsets. Verified against 2026-03-08 (23h spring-forward) and 2026-11-01 (25h fall-back) under TZ=America/New_York — every bucket, including the last, now stays inside the correct calendar day. exportAll()'s cursor now actually uses _exportDay's success signal (previously it only checked `finalized`, never the day's own result). Went with bounded retry-with-backoff (5m/30m/2h/6h/24h, persisted as a small JSON blob in the existing sync_cursor table) rather than indefinite blocking or a bare CodeRabbit-literal fix, matching existing precedent for this exact class of problem: derivation_engine.dart's skip markers "so a pathological day isn't retried forever" and the BLE layer's BondRefusalGiveUp. After 6 failed attempts a day is given up on (cursor advances past it, logged) so one persistently-failing day can't wedge every later day's export forever. Permission-not-granted writes silently no-op rather than throw, so they never enter this retry path. flutter analyze: clean (same 5 pre-existing unrelated issues). flutter test --concurrency=1: 457/457 passing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/health/health_export.dart`:
- Around line 245-287: Update the retry handling around _exportDay to persist
each entry’s finalized state and detect a transition from non-finalized to
finalized, resetting attempts and retry metadata before retrying the finalized
payload. Ensure a newly finalized day is not skipped because of the prior cap,
and set giveUp immediately when a finalized export attempt reaches
_kMaxExportAttempts while preserving cursor advancement behavior.
- Around line 369-375: Update the bucket construction around bucketSpan and
bucketBounds to advance by one elapsed hour until dayEnd, rather than dividing
the day into 24 equal spans. Derive bucketCount from the generated boundaries
and use it for energy division and all related loops in the active/basal energy
writers. Ensure the final bucket remains clipped to dayEnd.
- Around line 196-204: Update _backoffFor so attempts == 1 selects the first
_kRetryBackoff entry, while preserving clamping for zero and values beyond the
available retry schedule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 692d79f2-4dc0-402c-8287-3fe9b951eca9
📒 Files selected for processing (1)
lib/health/health_export.dart
Comment on lines
+245
to
+287
| final entry = (retryState[date] as Map?)?.cast<String, dynamic>(); | ||
| final attempts = (entry?['attempts'] as num?)?.toInt() ?? 0; | ||
| final lastAttemptMs = (entry?['last_ms'] as num?)?.toInt(); | ||
| final nowMs = DateTime.now().millisecondsSinceEpoch; | ||
|
|
||
| var ok = false; | ||
| var giveUp = false; | ||
| if (attempts >= _kMaxExportAttempts) { | ||
| giveUp = true; | ||
| } else if (lastAttemptMs != null && | ||
| nowMs - lastAttemptMs < _backoffFor(attempts).inMilliseconds) { | ||
| // Not due for retry yet — don't hammer the health store on every | ||
| // drain/derive pass; counts as "not done" for the cursor below. | ||
| } else { | ||
| ok = await _exportDay(date, bundle); // delete-then-write (idempotent) | ||
| if (ok) { | ||
| if (entry != null) { | ||
| retryState.remove(date); | ||
| retryStateDirty = true; | ||
| } | ||
| } else { | ||
| final nextAttempts = attempts + 1; | ||
| retryState[date] = {'attempts': nextAttempts, 'last_ms': nowMs}; | ||
| retryStateDirty = true; | ||
| debugPrint( | ||
| '[health] day $date export incomplete (attempt $nextAttempts/$_kMaxExportAttempts)'); | ||
| if (nextAttempts >= _kMaxExportAttempts) { | ||
| debugPrint( | ||
| '[health] day $date exceeded $_kMaxExportAttempts export attempts — giving up, will stop blocking newer days'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (ok) { | ||
| done++; | ||
| onProgress?.call(done); | ||
| } | ||
| // Advance the cursor only while the finalized prefix stays unbroken; the | ||
| // first non-finalized day stops it (that day re-exports next pass). | ||
| if (prefixContiguous && finalized) { | ||
| // Advance the cursor only while the finalized prefix stays unbroken — | ||
| // a non-finalized day, a still-backing-off retry, or a day still | ||
| // under the attempt cap all stop it (re-checked next pass); a | ||
| // given-up day counts alongside a genuine success so it can't wedge | ||
| // every later day's cursor forever. | ||
| if (prefixContiguous && finalized && (ok || giveUp)) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset capped retries when a day becomes finalized.
A non-finalized day that reaches six failures remains permanently capped. When it later becomes finalized, _exportDay() is skipped and the cursor advances without ever attempting the finalized payload. Persist the entry’s finalized state and reset attempts on that transition; also set giveUp immediately when a finalized attempt reaches the cap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/health/health_export.dart` around lines 245 - 287, Update the retry
handling around _exportDay to persist each entry’s finalized state and detect a
transition from non-finalized to finalized, resetting attempts and retry
metadata before retrying the finalized payload. Ensure a newly finalized day is
not skipped because of the prior cap, and set giveUp immediately when a
finalized export attempt reaches _kMaxExportAttempts while preserving cursor
advancement behavior.
- _backoffFor off-by-one: entries record attempts starting at 1 (right after the first failure), but the lookup indexed by attempts directly, so the first retry got the 30min tier instead of the intended 5min one. Index by attempts-1. - Retry cap didn't reset on the non-finalized -> finalized transition: a day that racked up 6 failures while still in the mutable "recent tail" state would hit giveUp=true forever after finalizing, so _exportDay was never even attempted against the final payload. Now persists `finalized` per retry entry and resets attempts/backoff on that transition, so a newly-finalized day always gets a fresh attempt budget instead of inheriting a cap earned against the old version. - Hourly energy buckets were dividing the day into 24 equal spans (57.5min/62.5min "hours" on the two DST-transition days) instead of real elapsed clock-hours. Switched to building boundaries by adding one real hour at a time up to dayEnd, clipping the final bucket; bucketCount now varies (23/24/25) instead of the bucket width. Re-verified under TZ=America/New_York on both transition dates: every bucket is exactly 1:00:00 except the correctly-clipped final one, and the last boundary still lands exactly on dayEnd. flutter analyze: clean (same 5 pre-existing unrelated issues). flutter test --concurrency=1: 457/457 passing.
This was referenced Aug 3, 2026
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
1 participant
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.
This pull request introduces several improvements and fixes across the codebase, focusing on background task management, health data export accuracy, and UI robustness. The most significant changes include safer and more precise handling of background tasks to avoid interfering with unrelated system jobs, enhancements to health data export granularity, and minor UI code safety improvements.
Background Task Management:
kHeavyDeriveTaskName,kSyncTaskName) to ensure that only the intended WorkManager jobs are cancelled during app startup, preventing accidental cancellation of unrelated jobs like native watchdogs. (lib/compute/background_derivation.dart,lib/main.dart) [1] [2] [3] [4] [5] [6]Health Data Export Improvements:
lib/health/health_export.dart)lib/health/health_export.dart)HealthDataType.HEART_RATEin the list of exported health data types. (lib/health/health_export.dart)UI and Code Safety:
Pressablewidget to check if the widget is still mounted before callingsetState, preventing possible errors. (lib/ui/design/pressable.dart)StepCalibrationScreen, cached theAppStateinstance ininitStateto avoid repeated context lookups and ensure proper cleanup. (lib/ui/today/step_calibration_screen.dart) [1] [2]Other:
_pulseanimation controller in_LiveBannerStateto occur ininitState, aligning with best practices for resource management. (lib/app.dart)Summary by CodeRabbit