gate the workout health write on a delete that actually cleared - #301
Conversation
exportWorkout and _exportDay both document delete-then-write idempotency but
called the write even when the preceding delete returned false or threw --
they only recorded success = false and carried on. If an already-exported
copy survived the failed delete and the write then succeeded, the store ended
up with two of the same workout, and the false return drove a retry through
the attempts/backoff/give-up machinery that would write a third.
The write cannot simply be gated on `delete() == true`, because the two
stores answer the EMPTY range differently:
* Health Connect (HealthPlugin.deleteData) calls deleteRecords over a time
range and reports success unless it threw. A zero-match range is a normal
success, so false there is always a genuine failure.
* HealthKit (SwiftHealthPlugin.delete) queries our own samples via
HKSource.default() and hands whatever came back -- including an EMPTY
array -- to HKHealthStore.delete, which Apple documents as "Deleting an
empty array fails with an errorInvalidArgument error". So on Apple false
is the normal answer for a window we have never written, and gating on it
would mean no first export ever reaches HealthKit.
healthDeleteClearedRange encodes that asymmetry: gate on Android, never on
Apple. Trusting Apple's false is not optimism, it is the only reading the
platform supports -- and the HealthKit failures that CAN leave one of our
samples behind (share permission never requested, or denied) suppress the
following write for the same authorization reason, so they cannot produce the
duplicate the gate exists to stop. A throw is a genuine failure on both.
That same reading also stops an empty-range delete from flipping a day's
success on Apple, where it was marking essentially every fresh export as
failed and driving the retry cursor with no actual failure behind it.
In the per-day path only the workout block is skipped when the day's WORKOUT
delete fails, so a failed workout export cannot pause that day's unrelated
RHR/HRV/sleep/energy exports. The scalar types still write unconditionally
and can still double on Health Connect -- noted in the rewrite loop, not
fixed here.
healthDeleteClearedRange takes `ios` rather than reading Platform for the
same reason healthActivityForType does, so the new fake-store test can
exercise both branches on a host VM.
Reviewer's GuideThe PR fixes workout delete-then-write idempotency by normalizing HealthKit and Health Connect delete results, suppressing writes when a Health Connect delete genuinely fails or throws, and retrying the delete before writing again; day-level export isolates the workout failure from unrelated health metrics. Sequence diagram for gated workout export retrysequenceDiagram
participant Caller
participant Exporter
participant HealthStore
participant Writer
Caller->>Exporter: exportWorkout(session)
Exporter->>HealthStore: delete(type, startTime, endTime)
alt delete throws or Health Connect returns false
HealthStore-->>Exporter: failure
Exporter-->>Caller: false
Caller->>Exporter: retry exportWorkout(session)
Exporter->>HealthStore: delete(type, startTime, endTime)
else delete succeeds or HealthKit returns false
HealthStore-->>Exporter: cleared
Exporter->>Writer: _writeOneWorkout(session)
Writer-->>Exporter: write result
Exporter-->>Caller: write result
end
Flow diagram for platform-aware workout delete gatingflowchart TD
A[Workout export requested] --> B[_deleteOwnSamples]
B --> C[_health.delete]
C --> D{Delete result}
D -->|throws| E[Return not cleared]
D -->|true| F[healthDeleteClearedRange]
D -->|false| F
F -->|Health Connect and false| E
F -->|HealthKit and false| G[Window treated as cleared]
F -->|true on either platform| G
E --> H[Return false without write]
H --> I[Retry re-attempts delete]
G --> J[_writeOneWorkout]
Flow diagram for isolating day-level workout failureflowchart TD
A[_exportDay] --> B[Rewrite WORKOUT and scalar types]
B --> C{WORKOUT delete cleared?}
C -->|no| D[Set workoutCleared false]
D --> E[Skip day workout writes]
E --> F[Continue unrelated RHR/HRV/sleep/energy exports]
C -->|yes| G[Process day workout rows]
G --> H[_writeOneWorkout]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/health/health_export.dart" line_range="126-127" />
<code_context>
+/// Parameterised on [ios] rather than reading `Platform` for the same reason
+/// [healthActivityForType] is — so a host-VM test can exercise both branches.
+@visibleForTesting
+bool healthDeleteClearedRange({required bool deleted, required bool ios}) =>
+ deleted || ios;
+
/// Cursor for the one-shot Apple Health sleep rewrite. Bump when the writer
</code_context>
<issue_to_address>
**issue (broader_impact):** On Apple, `deleted == false` is treated as a cleared range even when HealthKit returned no readable samples because the app requests only write authorization. An existing app-owned workout can therefore survive the delete while `_writeOneWorkout` writes another copy, recreating the duplicate the gate is intended to prevent.
**Triggers:** When the user has write authorization but lacks HealthKit read authorization, or read access otherwise returns an empty result for an existing sample.
**Suggested fix:** Request and verify the necessary read authorization before relying on the HealthKit query, or make the delete API distinguish an empty range from a failed/unauthorized query instead of treating every Apple `false` as safe.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and this changes the delete-then-write behavior for persisted health records; if the platform-specific interpretation is wrong, a workout could be duplicated or an export could be skipped. Reverting stops the behavior, but any duplicate record already written survives the revert and must be removed or recomputed.
Blocking findings: lib/health/health_export.dart:127
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| bool healthDeleteClearedRange({required bool deleted, required bool ios}) => | ||
| deleted || ios; |
There was a problem hiding this comment.
issue (broader_impact): On Apple, deleted == false is treated as a cleared range even when HealthKit returned no readable samples because the app requests only write authorization. An existing app-owned workout can therefore survive the delete while _writeOneWorkout writes another copy, recreating the duplicate the gate is intended to prevent.
Triggers: When the user has write authorization but lacks HealthKit read authorization, or read access otherwise returns an empty result for an existing sample.
Suggested fix: Request and verify the necessary read authorization before relying on the HealthKit query, or make the delete API distinguish an empty range from a failed/unauthorized query instead of treating every Apple false as safe.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe health export flow now interprets deletion results by platform. Daily and standalone workout exports skip writes when the workout window is not cleared. ChangesHealth export deletion flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change prevents workout writes after failed deletes on Android and after exceptions, but on Apple it treats every non-throwing false delete result as a cleared range. If that result can represent a real deletion failure, a replacement write could leave duplicate workouts, so explicit platform-owner confirmation is advisable before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
The bug
exportWorkoutand_exportDayboth document delete-then-write idempotency, butcalled the write even when the preceding
_health.delete(type: WORKOUT, ...)returned false or threw — they only recorded
success = falseand carried on.If an already-exported copy survived the failed delete and the write then
succeeded, the store ended up with two of the same workout — and the
falsereturn drove a retry through the attempts/backoff/give-up machinery that would
write a third.
Pre-existing; found during review of
claude/workout-general-bowling-143476but not caused by it, so it is split out here.
Why the write can't just be gated on
delete() == trueThe two stores answer the empty range differently, which is the fact the
whole fix hinges on:
falsemeanstruefalseHealthPlugin.deleteData, health 12.2.1) callsdeleteRecordsover a time range and reports success unless it threw. Azero-match range does not throw.
SwiftHealthPlugin.delete) queries our own samples viaHKSource.default()and hands whatever came back — including an emptyarray — to
HKHealthStore.delete. Apple documents that parameter as:"Deleting an empty array fails with an
errorInvalidArgumenterror."So on Apple
falseis the normal answer for a window we have never written.Gating on it would have broken every first export on iOS, permanently.
Two corroborations that this is real:
deleteByUUIDexplicitly guards!samples.isEmpty→
result(false); the rangedelete()just forgot the guard.health_export.dartalready reasoned about exactly this for STEPS: "a falsedelete()flipssuccess… could permanently stall a day's export cursor."The fix
healthDeleteClearedRange({deleted, ios}) => deleted || iosencodes theasymmetry: gate on Android, never on Apple.
Trusting Apple's
falseis not optimism — it is the only reading the platformsupports, and it is safe: the HealthKit failures that can leave one of our
samples behind (share permission never requested, or denied) suppress the
following write for the same authorization reason, so they cannot produce the
duplicate the gate exists to stop. A throw is a genuine failure on both
stores and is the one case that reports "not cleared" everywhere.
Three sites rewired through a
_deleteOwnSampleshelper:exportWorkout— bails without writing when the window is not cleared; stillreturns
falseso the retry happens, but the retry re-attempts the deletefirst.
_exportDay's rewrite loop — tracksworkoutCleared.genuinely failed, so a failed workout export cannot pause that day's unrelated
RHR/HRV/sleep/energy exports.
Side effect worth calling out
On iOS an empty-range delete was flipping a day's
successto false onessentially every fresh export, driving the retry cursor with no actual
failure behind it. That stops too.
Test
test/health_workout_export_delete_gate_test.dart— a fake store over theplugin's own
flutter_healthmethod channel. Verified to fail against thepre-fix code:
Five cases: both platform branches of the predicate, plus delete-false /
delete-throws / delete-succeeds through
exportWorkout. Parameterised oniosrather than reading
Platform, for the same reasonhealthActivityForTypealready is — so a host VM can exercise both branches.
Verification
flutter test—+3169 ~423 -1. The single failure isgen5_pairing_filter_test.dart(Dart↔Swiftpresent(items, allowGen4Retry:)lockstep), confirmed pre-existing by re-running it with
health_export.dartrestored from HEAD — it fails identically.flutter analyzeclean on both changed files.Note for anyone running the suite in a fresh worktree:
lib/l10n/app_localizations*.dartis gitignored generated output and
flutter testdid not auto-generate ithere despite
generate: true. Runflutter gen-l10nfirst or ~55 tests fail tocompile for unrelated reasons.
Not fixed here
The scalar types (RHR/HRV/respiratory/energy) still write unconditionally after
a failed delete and can still double on Health Connect. That needs per-type
"cleared" state threaded through
writeAt/writeGeneric— a different-shapedchange into the retry machinery. Documented in the rewrite loop rather than
silently left.
Summary by Sourcery
Gate workout writes on a cleared deletion window while accounting for the different empty-range responses from Health Connect and HealthKit.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit