feat(health): Health Connect export — Phases 0–6 (complete write-only integration) - #50
Merged
Merged
Conversation
Write-only Health Connect export for the Android port, mirroring the iOS HealthKit export (docs/health-connect-integration.md). Phase 0 is foundation only: no data is written yet. - androidx.health.connect:connect-client:1.1.0 (the only stable). Its AAR requires compileSdk 36 + AGP 8.9.1, so this also bumps the toolchain: AGP 8.7.0 -> 8.9.1, Gradle wrapper 8.9 -> 8.11.1, compileSdk 35 -> 36 (targetSdk stays 35). - Manifest: exactly the ten Phase 1-4 WRITE_* permissions (no READ_*, no Phase 5 types), <queries> for com.google.android.apps.healthdata, HealthConnectRationaleActivity (ACTION_SHOW_PERMISSIONS_RATIONALE) and the API 34+ ViewPermissionUsageActivity alias guarded by START_VIEW_PERMISSION_USAGE. - health/ package: HealthConnectSdk (three-state getSdkStatus wrapper), HealthConnectPermissions (derived via HealthPermission.getWritePermission; the route uses PERMISSION_WRITE_EXERCISE_ROUTE), HealthConnectPrefsStore (MetricPrefsStore pattern; separate monotonic-watermark key; tolerant decode), HealthConnectRationaleActivity (in-app rationale, no hosted URL). - Settings: HealthConnectSettingsScreen (master toggle -> permission sheet via PermissionController.createRequestPermissionResultContract(); launcher input is a Set<String>; partial grants first-class; per-type toggles; availability install/update rows; first-enable backfill dialog; HRV RMSSD caveat), row in SettingsScreen, route in PulseLoopApp. - Tests: 15 new (prefs decode tolerance + watermark monotonicity; permission derivation pins). Full suite: 890 tests green. Verified against the merged debug manifest (ten WRITE, zero READ). - Runtime verification on emulator-5554 pending (no AVD on this machine). Verified against the official Health Connect docs (get-started, exercise-routes, 1.1.0 API reference) and the Gadgetbridge reference.
Verified end-to-end on pulseloop_test (Pixel 7, android-35/google_apis): APEX-based Health Connect (com.android.healthfitness) is detected as available by the 1.1.0 client; permission sheet, privacy-link -> ViewPermissionUsageActivity alias route, backfill dialog, connected state, per-type rows, prefs persistence across restart, and all ten OS-level WRITE_* grants. API 30 graceful-degradation check remains pending (needs a second image).
…emperature, worker, triggers) Health Connect write-only export for the four Phase 1 vitals, per android/docs/health-connect-integration.md: - HealthConnectTypeMappings: pure id scheme (pl-hr-<hourEpochMs>[-i] / pl-m-<kind>-<epochMs>, ported from iOS HealthKitTypeMappings), local-hour bucketing, Gadgetbridge HR segmentation (local-date change / >15 min gap / 1000 samples), platform plausibility bounds, demo/mock source exclusion. - HealthConnectExporter: pass orchestrator (EXPORT_NEW_ONLY stamp-and-return, live per-kind granted-permission check, per-kind watermark highs, group wm = min of per-kind highs) plus the shared chunking engine: 200 records/insert, 5 retries, 1/2/4/8/16 s backoff, SecurityException aborts immediately, watermark advances only to the last completed chunk. - VitalsExporter: heart rate as per-hour HeartRateRecord series (one clientRecordId per segment, version = max createdAt in the hour); SpO2, HRV-as-RMSSD, and finger temperature as one instantaneous record per row (version 1). - HealthConnectExportWorker: debounced (15 s, REPLACE) one-time worker; hard gate on NOT_ASKED; stamps lastSyncAt + lastSyncSummary. Triggers: ring sync done, background sync done, first-enable permission grant, and backfill-dialog answer. - DAO: createdSince / rangeReal (real-data filters in SQL). - 25 new unit tests (915 suite green); settings screen and §8 session log updated. Runtime-verified on an API 35 AVD by injecting known rows and driving the real worker: backfill exported exactly hr 2 (two segments from a 38-min gap), spo2 1, hrv 1, temp 1 with the demo row excluded; immediate re-run exported nothing (watermark); a watermark reset re-upserted the same five clientRecordIds without duplication.
One SleepSessionRecord per waking-day session (pl-sleep-<dayEpochMs>, multi-session days disambiguated), stage sort/clamp/overlap normalization, SLEEP watermark on updatedAt, verified on-device incl. in-place growth and block-UUID churn.
…, distance)
One StepsRecord + ActiveCaloriesBurnedRecord + DistanceRecord per ActivityDailyEntity,
spanning the local day clamped to min(endOfDay, now) so today never ends in the future.
Identity is pl-act-<metric>-<dayEpochMs> with clientRecordVersion = row.updatedAt, so a day
that grows through the afternoon re-upserts the same three records in place. Unlike sleep,
activity_daily.date really is uniquely indexed, so no suffix scheme is needed — but it is
re-normalized through startOfDayLocal (as iOS does), since a date stored in another timezone
would emit a second overlapping record for one calendar day, and Health Connect sums an app's
own overlapping records rather than de-duplicating them.
iOS's workoutNetting is ported, with two changes forced by this app's data model:
- Distance netting drops iOS's walk/run type filter. iOS needs it because HealthKit splits
distanceWalkingRunning from distanceCycling; Health Connect has a single DistanceRecord,
and ActivityRollup.credit folds in every useGps session regardless of type, so the netting
set is useGps sessions of any type. Keeping the filter would double-count a GPS ride.
- Netting stays switched off until Phase 4 exists. iOS gates it on exportWorkouts alone
because its workout exporter already ships. Subtracting here, before WorkoutExporter,
would take energy and metres out of every day containing a workout with nothing writing
the difference back — and a write-only export cannot repair it. WORKOUTS_EXPORTED is the
single flag Phase 4 flips; netting also requires WRITE_EXERCISE to be granted, which still
matters afterwards because the toggle can be on while the permission is denied.
Two watermark defects found while reviewing this phase are fixed here, both in code Phases 1
and 2 already shipped:
- The pending-row queries now ORDER BY updatedAt, not date. healthConnectInsertChunked
advances to the max high water of the last successful chunk, which is only sound when
records arrive in high-water order, and a history re-sync restamps an old day's updatedAt.
Sleep had the same defect.
- A failed pass now clamps its watermark to the largest completed high water strictly below
everything still pending. Phase 3 is the first group where one source row emits several
records sharing one high water, so a chunk boundary falling inside a day could otherwise
strand an unlanded sibling.
The steps guard stops at the app's own corruption threshold (200 000) rather than the
platform's 1 000 000: EventPersistenceSubscriber self-heals such rows only on the next sync,
and a write-only store cannot be retracted in the meantime.
24 new tests, suite at 963. Runtime-verified on the pulseloop_test AVD (API 35) against the
provider's own store, with a discriminating case per netting rule — including a non-GPS gym
session whose calories net but whose distance must not.
ExerciseSessionRecord per finished ActivitySessionEntity (type map, ActivityMeta.label title, notes), embedded ExerciseRoute from accepted GPS fixes (sanitised: session window, finite/in-range coords, duplicate timestamps, >= 2 points), plus sibling ActiveCaloriesBurnedRecord/DistanceRecord over the session window. Siblings == netting set: energy for every finished session, distance for useGps only (Phase 3 amendment). - WORKOUTS_EXPORTED flipped to true in this commit (plan: same commit as the exporter); one-time netting-flip reset (nettingFlipDone + resetWatermarks) re-upserts daily records exported un-netted under the Phase 3 build, closing the pre-flip staleness window. - Stale-record decision: a netted leftover <= 0 is DROPPED, floor-value overwrite rejected (cannot reliably repair — the stale day is not re-selected; would fabricate values) — bounded, documented on activityLeftover. - Netting imperfections resolved: workoutNetting now skips the same sub-minute sessions ActivityRollup.credit never credits (ported minutesFor); the applyActivityBucketAtomic past-day overwrite adjudicated self-healing (it restamps updatedAt, so the day re-exports). - 1 MB single-record fallback: parse limit from the platform message, decimate the offending route to ~90% of the limit (first/last kept, no duplicate timestamps), retry immediately without backoff (Gadgetbridge pattern); SecurityException always aborts. - Deletion hooks: UI trash + coach delete_activity_session remove the session record plus its -energy/-dist siblings from Health Connect (best-effort, never fails the local delete); the route travels inside the session record and cascades. - Sibling records gated on their own write permissions (partial grants are first-class). - DAOs: activity_sessions finishedUpdatedSince (ORDER BY updatedAt ASC — the chunked-watermark invariant), activity_gps_points forSessions (one query per pending set). Runtime-verified on the API 35 AVD: real UI walk (1 accepted + 3 speed-rejected fixes -> session written without a route), injected 24-point route (20 land: rejected/duplicate/ out-of-window dropped), netting arithmetic to the milli (640 = 342.672 + 180 + 117.328; 7900 = 5900 + 2000; 250 = 100 + 90 + 60), <= 0 leftover drop, re-run no duplicates (same-version idempotent upsert confirmed), flip reset repaired a pre-flip day (250 -> 160), and UI delete removed session + siblings + route rows from the provider store.
…etion gate, flip consent
MeasurementEntity.kindRaw is persisted as MeasurementKind.<X>.name on every write path (EventPersistenceSubscriber, DemoDataSeeder, MetricsService, MeasurementModal), but Phase 1's VitalsExporter queried createdSince/rangeReal by the .key (hr/spo2/hrv/temp). The DAO filters on kindRaw = :kind, so the vitals pass matched no real or demo data and silently exported nothing for live ring history - masked in Phase 1's runtime check, which injected kindRaw='hr' rows to match the (incorrect) query. Route every vitals query through a kindKey -> kindRaw name map so the id token and the stored column value stop being conflated. No change to clientRecordIds (the id token is unchanged); only the query is corrected.
…utrition Beyond-iOS Health Connect types (plan §4), write-only, on feat/health-connect-foundation. Data: - BloodPressureRecord: pair systolic/diastolic MeasurementEntity rows by exact timestamp (id pl-m-bp-<ts>); drop unpaired, out-of-range (sys 60..200, dia 30..150) and demo/mock rows. - BloodGlucoseRecord: cap 900.0 mg/dL (verified from the 1.1.0 AAR: 50 mmol/L ceiling at 1/18 factor; the plan's 900.91 is looser than this client). Drop 20..900.0. - RespiratoryRateRecord (5..60), Vo2MaxRecord (1..100, MEASUREMENT_METHOD_OTHER). - RestingHeartRateRecord: single constant id pl-resting-hr, version = hrRestingBaselineUpdatedAt, Math.round bpm. - NutritionRecord from MealEntryEntity (id pl-meal-<id>, version = createdAt, Mass.milligrams sodium, interval start + 60s, skips empty meal). Engine: - RESTING_HR + NUTRITION groups in HealthConnectExporter.run(); VITALS group now covers glucose/resp/vo2/bp via the .name kindRaw map (also fixes the Phase 1 latent .key/.name mismatch, 054b5d2). - 6 WRITE_* manifest permissions (16 total, 0 READ_*); 6 per-type toggles (default on) + RESTING_HR watermark; MealEntryDao.createdSince; 5 settings rows. Review (observer stage B): - BLOCKER fixed: nutrition energy cap was 1000x too loose (Energy.calories is small cal, so the ceiling is 100,000 kcal, not 1e8) — a 6-digit kcal typo would have wedged the export work. Now 100_000 + boundary tests. - SHOULD-FIX: nutrition export now also gates on the app's nutritionEnabled feature toggle (iOS parity; the un-gated "Open Nutrition Log" entry would otherwise leak). Verified: 1008 unit tests pass; runtime on emulator-5554 (API 35, adb root) — 16 perms granted, 16 WRITE / 0 READ manifest, injected fixtures exported exactly as designed (guards/pairing/exclusion), provider store records correct, and a re-run produced no duplicates (upsert idempotency).
…dAt, archive stamp "remove PulseLoop data" (iOS removeAllExportedData parity): per-type owner-scoped time-range deletion (15 record classes, WRITE-gated, per-class catch/continue), export work cancelled first, watermarks + last-sync state cleared, backfillChoice + enabled deliberately untouched. Uses the time-range overload (platform owner-scoped per the 1.1.0 KDoc + AOSP caller-package force-filter), not clientRecordIds (write-only can't enumerate IDs; the ID overload aborts the whole transaction on an unknown ID). Grant/revocation watermark resets: uniform grow-reset over the 6 watermark groups (16 WRITE perms mapped; the four Phase-5 kinds glucose/resp/vo2/bp -> VITALS; no key special-cased). onAppStart (MainActivity.onResume) + the settings LaunchedEffect and permission-launcher reconcile; the full-revocation reset offer is state-based and one-shot (revocationOfferDismissed), Gadgetbridge pattern. Meal updatedAt: PulseLoopDatabase v20->v21 (ADD COLUMN + createdAt backfill inside the upgrade transaction), MealEntryDao.updatedSince, NutritionExporter watermark + record version on updatedAt, DataArchive DTO round-trip with createdAt backfill for old archives. Archive-restore watermark stamp: after the restore Room transaction, gated on the device's enabled, stamps all six watermarks to now (iOS DataArchiveService.swift parity). Settings: "Remove PulseLoop data" card + confirm dialog (runCatching-guarded creation). HealthConnectExportWorker: cancel(context) + SecurityException path re-reads the live grant set, reconciles, and corrects the stored set. New HealthConnectPermissionReconcileTest (8 tests, hand-rolled FakeSharedPreferences, no mocks). .gitignore += dist/. Docs: ios-sync.md row #80 marked done + the HealthKit-only residual; plan §8 Phase 6 entry + status line. Runtime-verified on emulator-5554 (API 35, adb root): the 20->21 migration, grow-reset (grown groups nulled, sentinel kept, then backfilled), and removal with a foreign canary (all our records deleted, a foreign app's record survived — owner-scoping proven at runtime). Final gates passed: release R8 smoke (the minified build ran the Health Connect client with no class-stripping crash) and the API 30 no-Health-Connect-image graceful-degradation check (provider-less API 30 AVD: no crash, the Health Connect screen shows the actionable update/install state).
…ge observer MINOR) onAppStart detected a grow and reset the watermarks, but only the two UI paths (settings LaunchedEffect + permission launcher) cleared revocationOfferDismissed. A pure out-of-band dismiss -> re-grant -> revoke cycle therefore left the one-shot revocation offer suppressed until the next in-app re-grant. No correctness impact (the grow-from-empty backstop always re-exports on re-grant), but the section 8 "cleared on grow" contract was only half-true. onAppStart now clears the flag in the same store.update when a grow is detected, matching the two UI paths. Pre-merge observer pass: NO BLOCKERS / NO SHOULD-FIX (ready to merge on correctness; the pass-1 MAJOR was independently re-verified against AOSP main). The remaining NIT (launcher offer lacks the hadSync guard the state-based path has) is noted and deferred as cosmetic.
…observer NIT) The launcher-callback full-revocation offer fired on allRevoked regardless of whether any export had ever run, so a user who granted and immediately revoked without ever exporting was offered a "reset" for already-empty state — a harmless no-op with slightly off "re-exports your history" wording. The state-based LaunchedEffect offer already carried the hadSync guard; the launcher path now applies the same condition (lastSyncAt non-null or any watermark set), so both offer paths require prior export state.
foureight84
commented
Aug 18, 2026
foureight84
left a comment
foureight84
left a comment
Owner
Author
There was a problem hiding this comment.
Code review of the full diff (main...feat/health-connect-foundation, 44 files, ~6.3k lines), read hunk by hunk with API contracts checked against the connect-client-1.1.0.aar bytecode (record constructor arg order, Metadata.autoRecorded/activelyRecorded, ExerciseSessionRecord validation). Record constructors, permission strings and the chunked-watermark clamp math all check out.
10 findings inline: 1 critical (missing continue bricks all future export passes), 3 major (silent permanent data loss on watermark stamp-forward, aborted sibling deletes, removal undone by the next sync), 4 minor, 2 nits.
…, 2 NIT) Fix every inline review comment on the Health Connect export PR: - CRITICAL: WorkoutExporter — the missing continue after the INVALID (zero/negative duration) arm let the session fall through to ExerciseSessionRecord, which throws and escapes build->run->doWork (only SecurityException is caught), silently killing workouts/resting-HR/nutrition export forever. - MAJOR: the EXPORT_NEW_ONLY first-enable sentinel now uses a dedicated newOnlyStamped flag instead of inferring "first enable" from wm0.vitals == null, which a Phase-6 grow-reset also nulls (was re-stamping every group to now and dropping pending rows). - MAJOR: HealthConnectWorkoutDeletion — each of the three deleteRecords is now isolated in its own runCatching so one unknown (conditional-sibling) id can't abort the rest and orphan a record the app can no longer manage. - MAJOR: HealthConnectRemoval — removeAll also clears enabled + backfillChoice (NOT_ASKED) + newOnlyStamped so the next background trigger can't re-export the whole history and silently undo the removal; re-enabling re-offers the backfill dialog. - MINOR: re-enabling a vitals kind resets the VITALS watermark (toggle-side equivalent of the permission grow-reset) so off-period readings backfill; single-type groups freeze their watermark while off and already backfill on re-enable. - MINOR: onAppStart reconcile guard is now just !prefs.enabled, so a re-grant made after a full revocation (stored set empty) is detected as a grow and backfills. - MINOR: ActivityExporter.loadNetting bounds on normalized day starts (startOfDayLocal), matching the netting keys, so a timezone-shifted session in [dayStart, rawDate) still nets. - MINOR: backfill dialog onDismissRequest now sets enabled=false (iOS Cancel parity) instead of leaving the export enabled but hard-gated on backfillChoice=NOT_ASKED. - NIT: fixed the stale "export engine lands in the next phase" copy. - NIT: dropped the dead context param + Health Connect hook from PendingActionExecutor.execute and corrected both doc comments that claimed the coach delete path fires the deletion hook. Verified: testDebugUnitTest (1020 tests, 0 failures), assembleDebug, assembleRelease (R8).
foureight84
commented
Aug 19, 2026
foureight84
left a comment
foureight84
left a comment
Owner
Author
There was a problem hiding this comment.
Re-review after the fixes for review #4966137548. All 10 previous findings verified fixed.
| # | Fix | Verdict |
|---|---|---|
| 1 | continue in the INVALID arm (WorkoutExporter.kt:144) |
Holds; blockedFuture/empty-records interplay still correct |
| 2 | newOnlyStamped sentinel replaces wm0.vitals == null |
Holds mechanically — but see the new finding below |
| 3 | Per-delete runCatching |
Holds; all three record classes isolated |
| 4 | Removal clears enabled/backfillChoice/newOnlyStamped |
Holds; re-enable re-offers the dialog |
| 5 | Toggle-on resets the VITALS watermark | Holds; groupFor maps each row to one group, only VITALS is shared |
| 6 | onAppStart guard is !prefs.enabled |
Holds; re-grant after full revocation now registers as a grow |
| 7 | loadNetting bounds on normalized day starts |
Holds and is provably tight — startOfDayLocal is monotonic and ≤ raw date |
| 8 | Dialog dismiss sets enabled = false |
Holds for explicit dismiss; see the recreation case below |
| 9 | Stale "next phase" copy | Holds |
| 10 | Dead context param dropped |
Holds; execute still has no production callers |
No regression test was added for any of the ten. The CRITICAL one lives in WorkoutExporter.build, which still has no test at all (only the pure selectWorkoutSession mapping is covered) — which is exactly why the fall-through survived the first pass.
4 new findings inline: 1 major (introduced by fix #2), 2 minor, 1 doc nit.
Fix the four inline review comments from PR #50's second review pass: - MAJOR: a watermark reset (grow-reset on a re-granted permission, or the re-enable-vitals-toggle reset) nulled the group watermark, and a null watermark means export-from-epoch - so an EXPORT_NEW_ONLY user who re-granted a permission (or flipped a vitals row off->on) had their pre-consent history re-exported, exactly what the consent meant to prevent. Persist the consent instant (newOnlyConsentAt, recorded by the first-enable sentinel) and have resetWatermarks clamp a NEW_ONLY group reset to it instead of nulling; removeAll clears it so a fresh re-enable re-stamps. - MINOR: showBackfillDialog was remember, so a rotation/process death while it was up destroyed it without running onDismissRequest, stranding enabled=true + backfillChoice=NOT_ASKED (Connected but nothing exports, no re-offer). Derive it from persisted state the way the revocation offer does; recreation re-shows it and the explicit dismiss is redundant. - MINOR: the newOnlyStamped=false default reproduced the original bug once at the upgrade boundary (a pre-fix EXPORT_NEW_ONLY blob re-ran the first-enable branch after updating and dropped pending rows). In load(), seed newOnlyStamped=true + newOnlyConsentAt=min(watermarks) when the key is absent and any watermark is non-null (safe: the min is >= the true consent, so it never leaks pre-consent history). - NIT: doc drift after the pass-1 removal fix - the Phase 6 plan line, the Design-locks line, and the removal live-verification note now record that removeAll also turns the export OFF + resets backfillChoice/newOnlyStamped; fixed the stale MainActivity.onResume comment and the backfill dialog's false You-can-change-this-later copy. Verified: testDebugUnitTest (health package: 152 tests, 0 failures - 7 new in HealthConnectPrefsStoreTest) + assembleDebug green.
foureight84
commented
Aug 19, 2026
foureight84
left a comment
foureight84
left a comment
Owner
Author
There was a problem hiding this comment.
Third pass, after the fixes for review #4970615711.
| Finding | Status |
|---|---|
| (A) MAJOR — NEW_ONLY pre-consent leak | Partially fixed. newOnlyConsentAt is recorded and resetWatermarks clamps to it; both paths named in the finding (grow-reset, VITALS toggle-on) are covered. A third watermark-nulling path — clearWatermarks() in the revocation dialog — was left un-clamped (finding 1) |
(B) MINOR — remember backfill dialog |
Fixed. Derived from persisted state, no imperative writers remain. One placement gap (finding 2) |
| (C) MINOR — upgrade-boundary re-stamp | Fixed as written, but guards a boundary that cannot exist (finding 4) |
| (D) NIT — doc drift | Fixed. Phase 6 plan line, §8 design locks, removal verification note, MainActivity comment and the dialog copy all corrected |
| Regression tests | Landed this time — 7 new HealthConnectPrefsStoreTest cases covering the clamp (single key, multi key, non-NEW_ONLY null regression, on-disk persistence) and the seed. Suite green: 24 tests / 0 failures |
Root cause worth fixing once, at the read site
Three rounds have now produced the same defect at three different call sites, because a null watermark is overloaded — it means both "never exported" and "export from epoch" — and the NEW_ONLY consent boundary is enforced at every write site instead of the single read site. There are three paths that null a watermark (resetWatermarks, plus clearWatermarks from removal and from the revocation dialog); each round has patched one or two of them.
Having the exporter compute effectiveWatermark = max(stored ?: 0, newOnlyConsentAt ?: 0) instead of stored ?: 0 makes every nulling path safe by construction — it closes findings 1 and 3 together and stops a future call site from reopening the class. Worth doing instead of patching clearWatermarks site by site.
4 findings inline: 1 major, 3 minor.
Root cause (reviewer): a null group watermark is overloaded — it means both "never exported" and "export from epoch" (createdSince(kind, 0)) — and the EXPORT_NEW_ONLY consent boundary was enforced at every watermark-nulling WRITE site (three rounds, three nulling paths) instead of the single READ site. Read-site clamp (findings 1 MAJOR + 3 MINOR, safe by construction): - effectiveWatermark(stored, newOnlyConsentAt) = max(stored ?: 0, newOnlyConsentAt ?: 0) is applied at all six group build() SELECT sites instead of stored ?: 0. Every nulling path (resetWatermarks, clearWatermarks from removal AND the revocation dialog) is now safe, and a future nulling call site cannot reopen the class. The stored watermark still drives the monotonic advance — only the SELECT is floored. - Closes the revocation-dialog clearWatermarks leak (MAJOR 3814246783) and the sentinel stamp-before-persist TOCTOU (MINOR 3814246800): a nulled watermark still floors at the consent instant recorded by the sentinel's update. Availability gate (MINOR 3814246789): the derived backfill dialog now requires HealthConnectAvailability.AVAILABLE, so a device whose provider is uninstalled / needs an update no longer pops the dialog over the "needs an update" card. Drop the upgrade-boundary seed (MINOR 3814246812): origin/main has no health/ code (the feature ships whole in this PR), so no installed blob can be missing the key; the raw-substring probe also silently depended on encodeDefaults and was ungated on backfillChoice (a NOT_ASKED archive-restore blob would have seeded newOnlyStamped=true and skipped the stamp pass). The read-site clamp supersedes the seed entirely. Tests: 4 new effectiveWatermark cases; removed the 3 obsolete seed cases. Health package 153 tests, 0 failures; assembleDebug green.
foureight84
commented
Aug 19, 2026
foureight84
left a comment
foureight84
left a comment
Owner
Author
There was a problem hiding this comment.
Fourth pass, after the fixes for review #4973700659. Diff since e9b36a4 is 6 files / +74 / −90. All four findings verified fixed; no new major.
(1) MAJOR — clearWatermarks() bypassing the consent clamp: fixed, and fixed at the right level. The read-site clamp was adopted — effectiveWatermark(stored, consent) = max(stored ?: 0, consent ?: 0) at HealthConnectExporter.kt:154. All six group SELECT sites route through it (vitals :328, sleep :368, activity :420, workouts :475, resting HR :528, nutrition :567), and there is no other read path — SettingsSubScreens:2482/2511 read watermarks only for the hadSync heuristic, DataArchiveService:597 only stamps forward. The revocation dialog's clearWatermarks() now yields effectiveWatermark(null, T) == T.
Passing a non-null Long where build() declares Long? is safe: the vitals/sleep/activity/nutrition/workout exporters all do watermark ?: 0L and use it purely as a lower bound; RestingHeartRateExporter:43 is the one null-sensitive site (if (watermark != null && updatedAt <= watermark)) and is equivalent at 0L for any real baseline timestamp. Every advance still compares against raw wm0.X ?: 0L, and since selected rows are all > effectiveWatermark, no advance can be suppressed or rewound.
(2) MINOR — availability gate: fixed. SettingsSubScreens:2437 now requires availability == AVAILABLE, and the premise checks out: the master Switch and permissionLauncher.launch live only inside the AVAILABLE branch (:2553–2573) and the LaunchedEffect(availability) returns early otherwise, so the gate cannot strand a reachable state.
(3) MINOR — sentinel write-ordering race: fixed by construction. The bad interleaving (setWatermark×6 → interleaved reset → update) now lands on wm=null, consent=T, which the read clamp floors at T. The invariant the clamp relies on — newOnlyConsentAt != null ⟹ backfillChoice == EXPORT_NEW_ONLY — holds: the only writers are the sentinel (Exporter:244) and HealthConnectRemoval:108–118, and the only route out of EXPORT_NEW_ONLY is removal, which nulls consent in the same copy. The retained write-site clamp in resetWatermarks gates on backfillChoice, the read clamp on consent != null; given that invariant they are equivalent, so the two are consistent.
(4) MINOR — dead upgrade seed: correctly removed. Verified against origin/main (cdfb39a): no com/pulseloop/health/ source exists there, so no released blob can be missing newOnlyStamped. Dropping loadWatermarks() from load() also removes a field-initialization-order coupling; the three obsolete tests are gone and every remaining import is still used.
Regression tests: 4 new pure-function cases in HealthConnectExporterTest for the clamp, plus the 4 pass-2 resetWatermarks cases retained. One coverage gap worth closing: they exercise the helper only — nothing asserts that the exporter's six SELECT sites actually route through it, so a seventh site added later without the clamp would reopen this class silently.
I could not construct any remaining path in shipped code that nulls or lowers a watermark for an EXPORT_NEW_ONLY user without the read clamp catching it.
2 findings inline: 1 minor, 1 nit.
MINOR (3814608810) — HealthConnectPrefsStore's mutators were non-atomic read-modify-writes over a single JSON blob, with genuinely concurrent writers (export worker dispatcher, removal scope, settings on the main thread). Two writers reading the same snapshot silently dropped each other's fields: a data-type Switch flipped off just as a long EXPORT_ALL backfill pass finished snapped back on and re-exported that type. A single writeLock now serializes read -> transform -> StateFlow assign -> persist in update/setWatermark/resetWatermarks/clearWatermarks; the lock spans the persist so the on-disk blob can't be ordered differently from the flow. NIT (3814608816) — an install that ran bef3048 (newOnlyStamped, before newOnlyConsentAt landed in e9b36a4) decodes to stamped-but-no-consent with every watermark stamped, so the sentinel never re-fires and the read clamp degrades to the unclamped `stored ?: 0` permanently — the first nulling path would export the full pre-consent history. repairMissingConsentInstant() in the store's init back-fills the consent from the oldest non-null watermark (the inverse of the stamp; never earlier than the real boundary), gated on EXPORT_NEW_ONLY + stamped + null consent, and no-ops when there's nothing to reconstruct from. 5 new HealthConnectPrefsStoreTest cases (2 concurrency, 3 recovery). Full testDebugUnitTest: 1033 tests, 0 failures; compileDebugKotlin green.
…, 2 NIT)
MAJOR — "Remove PulseLoop data" ran in the settings screen's
rememberCoroutineScope(). Leaving the screen cancelled it at the next
deleteRecords suspension: some of the 15 record classes deleted, the rest
alive, clearWatermarks() and the enabled=false / backfillChoice=NOT_ASKED
reset never run, and no status message anywhere. The watermarks then claim
records were exported that no longer exist, and write-only means nothing
re-exports them. New HealthConnectRemovalWorker (unique work, KEEP so a
double-tap can't start two, no retry) owns the run; removeAll's delete loop
and state reset are wrapped in NonCancellable; the outcome reports through a
new persisted HealthConnectPrefs.removalStatus (REMOVAL_IN_PROGRESS sentinel
disables the button and reads "Removing…", then a dismissible result card),
so it survives navigation, rotation and process death.
MEDIUM — the removal/export race was not actually closed. cancelUniqueWork
only records the cancellation; a RUNNING worker keeps going to its next
suspension point, so a pass mid-insertRecords could re-write records the
removal had just deleted, and its non-suspending setWatermark could land after
clearWatermarks(). A process-wide HealthConnectExportWorker.passMutex is now
taken by both doWork and removeAll — the genuine iOS isSyncing-latch analogue.
Cancel-then-lock: queued passes dropped, an in-flight one waited out.
MINOR — full revocation was a dead end in-app. isConnected going false hid the
Data Types card AND the whole "Remove exported data" card, exactly when a user
wants to clear what was already exported, and there was no entry point to the
Health Connect app — also the only way out of the platform's "denied twice ->
the sheet stops appearing" state. The last-sync + removal cards now gate on
isConnected || hasExportState, the removal card says in red that deleting
needs WRITE and PulseLoop has none, and an always-present "Open Health
Connect" button (getHealthConnectManageDataIntent, falling back to the store
listing) lands on PulseLoop's own page in the provider.
MINOR — availability was a keyless remember {}, so after "Install / Update
Health Connect" -> Play -> back the screen kept rendering the unavailable card
with LaunchedEffect(availability) never re-firing. Re-read on every ON_RESUME.
MINOR — an archive restore could silently void an EXPORT_ALL backfill.
importFile stamped all six watermarks gated only on `enabled`; with NOT_ASKED
(the first-enable dialog still up) a restore then "Sync all history" exported
nothing, because EXPORT_ALL means "from epoch" purely by way of null
watermarks. The stamp now also requires backfillChoice != NOT_ASKED.
MINOR — the rationale screen (the permission sheet's privacy-policy target)
listed only the Phase 1-4 types; BP, glucose, respiratory rate, VO2max,
resting HR and nutrition are all requested and written. All 13 are now named.
MINOR — "Last sync:" rendered only the summary, so it read "Last sync:
skipped: nutrition (nutrition feature off)" with no time. Now "Last sync 4h
ago — <summary>" via DeviceHeroStatus.relativeShort.
NIT — trailing never-exportable rows pinned a group watermark. vitals / sleep
/ activity / nutrition dropped rows without contributing a high water, so a
demo-sourced, implausible, stage-less or empty row above every exportable one
stopped the watermark below it and every later pass re-selected and
re-upserted the tail behind it. Phase 4 solved exactly this for workouts
(invalidHighWater) and never generalized: workoutsWatermarkAdvance is renamed
watermarkAdvance and now drives all five groups, each exporter reporting a
droppedHighWater (BpPairingResult.outOfRangeHighWater for the paired case),
still applied ONLY on a fully completed pass. Deliberately not counted as
dropped: a future-dated activity day and an unpaired blood-pressure row —
those are not-yet-exportable rather than never, and advancing past them would
lose them.
NIT — the stored granted set had two definitions: the permission-sheet
callback filtered to HealthConnectPermissions.all while onAppStart, the
settings LaunchedEffect and the worker's SecurityException path stored
getGrantedPermissions() verbatim. Identical today, but they would disagree on
every reconcile once a health permission outside `all` were granted. One
HealthConnectPermissionReconcile.storedSetOf() is now used by every path.
UI, found while driving the flow on the emulator — both pre-existing rather
than introduced by this PR, fixed here anyway:
- The two PulseColors.cardSoft cards rendered near-invisible.
CardDefaults.cardColors(containerColor = …) leaves contentColor Unspecified,
so the Text fell back to a dark LocalContentColor on a dark card. All three
call sites in SettingsSubScreens.kt (the Strava one included) now pass
contentColor = PulseColors.textPrimary.
- A doubled status-bar inset left a tall dead band above every pushed screen's
title and above "Step 1 of 5": PulseLoopApp's paddedComposable already
insets the route subtree by the outer Scaffold's system bars, and then
SettingsSubScreen, NutritionScreen, DebugScreen (inner Scaffold + TopAppBar
defaults) and OnboardingScreen (windowInsetsPadding(WindowInsets.statusBars))
applied it again. All four now contribute zero insets of their own.
Tests: 4 new — 2 for BpPairingResult.outOfRangeHighWater (out-of-range
counted, unpaired never; null when nothing is out of range) and 2 for
storedSetOf (filters + sorts; idempotent, so an unrequested grant is not seen
as a grow/shrink). Full testDebugUnitTest: 1037 tests, 0 failures;
assembleDebug and assembleRelease (R8) green.
Runtime-verified on emulator-5554 (API 35, windowed): the full first-run
sign-in flow; removal through the new worker (last-sync cleared, result card,
backfill dialog correctly re-offered on re-enable); the fully-revoked state
(pm revoke x16 -> revocation dialog, removal card present with its WRITE
warning, "Open Health Connect" reachable); and both inset fixes.
foureight84
added a commit
that referenced
this pull request
Aug 21, 2026
Health Connect export (PR #50) is a new user-facing integration — 16 write permissions, its own settings screen, and 13 exported data types — so this is a minor bump rather than another 2.5.0 build. versionCode stays 37 (set on the feature branch, never tagged); the release tag is v2.6.0+37.
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.
Summary
Complete Health Connect export integration for PulseLoop Android — the port of the iOS Apple Health sync (PR #80), write-only (PulseLoop never reads data back). Seven phases (0–6), runtime-verified on an API 35 emulator, with both final release gates passed and two observer review passes clean.
What's in it
removeAllExportedDataparity): per-type, owner-scoped, time-range deletion across the 15 record classes (WRITE-gated, per-class isolated), export work cancelled first, watermarks + last-sync cleared,backfillChoice/enableduntouched.WRITE_*perms mapped; the four Phase-5 kinds → VITALS).onAppStart+ settings/launcher reconcile; one-shot full-revocation reset offer (Gadgetbridge pattern).updatedAt(v20→v21 migration + backfill) driving the nutrition watermark/record version.enabled).Permissions: 16
WRITE_*, 0READ_*(write-only by design).Design notes
deleteRecords(class, TimeRangeFilter)), which the platform owner-scopes to the calling app (1.1.0 KDoc + AOSPHealthConnectServiceImplforce-sets the caller package filter; WRITE-only). TheclientRecordIdsoverload is infeasible: write-only can't enumerate stored IDs, and that overload aborts the whole transaction on any unknown ID.Verification
HealthConnectPermissionReconcileTest(8 tests, no mocks) + full suite green.assembleDebugandassembleRelease(R8-minified) green.getGrantedPermissionsIPC under R8, no class-strip crash) PASS; API 30 no-Health-Connect graceful degradation (provider-less AVD: no crash, actionable update prompt) PASS.main).Docs
android/docs/health-connect-integration.md— design + 7-phase plan + §8 session log (read the newest entry when resuming).android/docs/ios-sync.md— port-queue row #80 marked complete; HealthKit-only residual: profile import can't port (Health Connect has no date-of-birth / biological-sex type).Test plan