Skip to content

fix(health): hold write-back while the phone is locked and skip rewriting unchanged batches - #2298

Open
Iskrata wants to merge 2 commits into
ryanbr:mainfrom
Iskrata:fix/health-writeback-skip-unchanged
Open

Iskrata wants to merge 2 commits into
ryanbr:mainfrom
Iskrata:fix/health-writeback-skip-unchanged

Conversation

@Iskrata

@Iskrata Iskrata commented Sep 17, 2026

Copy link
Copy Markdown

What this PR does

Apple Health write-back runs after every completed offload, about every 10 minutes while a strap is connected. It rewrote a lot that had not changed, and while the phone was locked it could only fail. This PR:

  • Holds a write-back while the phone is locked. HealthKit refuses reads and writes then (Protected health data is inaccessible), but each locked run still read fourteen days from the store before failing. The run is now owed, and it happens once on protectedDataDidBecomeAvailableNotification.
  • Skips an unchanged batch. Sleep, vitals and workouts each record a fingerprint of the batch that last saved (HealthWriteback.batchFingerprint: FNV-1a over sorted per-sample descriptors, stable across launches). An identical batch skips the delete and the save. Any batch older than unchangedRewriteIntervalSeconds (24 h) is rewritten regardless, so anything removed in the Health app comes back within a day. A workout read that failed or hit its cap never counts as unchanged, so the iOS: workouts NOOP stops tracking are never deleted from Apple Health, so they linger as duplicates #2210 orphan pass still gets its complete window.
  • Stops re-saving 48 hours of heart rate every run. The HR write deleted and re-saved the whole 48 h behind its cursor (~2,880 samples) on each pass, so that a late offload landing in that span would be reconciled. That span is now fingerprinted as written. When it is unchanged, only the last 5 minutes before the cursor (where the newest bucket was still filling when it saved) and everything after it are rewritten. When anything in the 48 h moved, the full window is rewritten as before.

Why

On one install the strap log showed health: write-back step failed — Protected health data is inaccessible on every overnight offload. On unlocked runs, the same fourteen days of sleep and vitals were deleted and re-added every ten minutes with no change in the store.

Type of change

  • Bug fix (battery)

How it was tested

  • New HealthWritebackTests: the fingerprint ignores order but sees every changed value, dropped sample and descriptor boundary; the skip holds only for an identical batch written within the interval.
  • NOOPiOS builds. doc_comment_lint.py is clean; parity_ratchet.py --base upstream/main --offline reports 0 errors.

Android: Health Connect has no locked-device restriction like this, so the first change is iOS-only. Whether the Android exporter has the same unchanged-batch and 48 h HR rewrite cost is something I'll check and follow up on with its own tests.

Checklist

  • No new build warnings introduced
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

@ryanbr

ryanbr commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Reviewed at b99a5fc. Full 18-check roster green. This is ambitious and mostly very good.

The FNV-1a constants are right, the 0x0a separator and the count prefix close the boundary-collision hole, and the tests check exactly those properties, "ab" + "c" against "a" + "bc" included. Choosing FNV over Hasher because Hasher is seeded per launch is the detail that makes a persisted fingerprint work at all, and the doc comment says so. The 24 h valve is the right answer to somebody clearing NOOP's data in the Health app. The readComplete gate genuinely protects the #2210 orphan pass and mirrors the pattern already sitting at line 1207.

I worked through the heart-rate tail math, since that is where the risk concentrates. Recording tailFingerprint(through: lastSaved) after writing only > cursor - settleSeconds is sound: [lastSaved - 48h, lastSaved] falls inside the union of the span the fingerprint had already verified and the span just written. The .strictStartDate on the tail path is also deliberately right, stopping a delete from reaching a sample that starts earlier and overlaps in. Good instincts throughout.

Four things:

  1. The branch is dirty, and it collides with fix(health): hold a still-open night out of Apple Health until it closes #2294. Both create the dispositions array in the same JSON and touch overlapping regions of HealthKitBridge. I would like to take fix(health): hold a still-open night out of Apple Health until it closes #2294 first, since it is smaller and already clean, and have this rebase on top. The two compose naturally in that order: fix(health): hold a still-open night out of Apple Health until it closes #2294 decides what is in a batch, this decides whether to rewrite it.

  2. The fingerprint key is not per-strap, but the heart-rate cursor is. writtenBatchKey is "hkWrittenBatch.v1.\(kind)", while hrWriteCursorKey is documented three lines above as per-strap. On a two-strap install the cursor moves with the strap and the fingerprint does not. In practice you lose the optimisation rather than corrupting anything, because the fingerprints will not match. But this codebase has been bitten before by per-install keys on multi-strap installs, and the asymmetry contradicts the intent documented right beside it. Worth keying it the same way while the key name is still unshipped.

  3. The vitals fingerprint rests on HKQuantity.description. "\($0.sample.quantity)" interpolates an Apple-formatted string such as 60 count/min, which is not a documented stable encoding, and number formatting is not guaranteed invariant. doubleValue(for:) against an explicit unit would be stable by construction. A change there costs one extra rewrite so it self-heals, but this is a persisted fingerprint standing on a system type's description, and that is the kind of thing that moves quietly under an OS update.

  4. The tail logic is correct by a non-obvious argument and has no test. Everything you tested lives in HealthWriteback; the window selection lives in the bridge where no test can reach it. The pure part, given cursor, lastSaved and settle, work out which buckets to write, would move across and be testable exactly like the rest. Given how carefully the other helpers were extracted for testing, this one looks like it was left behind rather than judged untestable.

Minor: the unlock observer is removed after guard let self, so it outlives a deallocated bridge. Negligible for a long-lived object, but a deinit would close it.

Approving after a rebase. I would rather have (2) and (3) in before merge than after, since both the key name and the fingerprint format are persisted and changing either later means a one-off rewrite for every install. (1) and (4) I am happy to take as follow-ups if you would rather keep this moving.

@ryanbr

ryanbr commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Sequencing decided: #959 goes in first, then this.

#959 is the staged HealthKit consent work and it restructures the authorization and type-set region of HealthKitBridge.swift, which is the same file this rewrites. It is the smaller and older of the two and it is close to ready, so it goes ahead and this one absorbs the merge.

What that means when you rebase. The conflict will be in HealthKitBridge.swift and it will be a real one rather than textual, because #959 replaces the single readTypes / writeTypes pair with per-stage accessors behind a ConsentStage enum, and it narrows refreshAuthIfPreviouslyGranted.

One interaction worth checking rather than assuming. After #959, a write path can be a no-op because its stage was never consented to, not because there was nothing to write. Your fingerprint cache records "this batch is written" after a successful save, so the question to satisfy yourself on is whether any path can reach recordWrittenBatch without the save having actually happened. My reading is that it cannot, since HealthKit errors on an unauthorized save and the throw propagates past the record call, but that reasoning depends on #959's final shape and is worth re-checking against it rather than against today's file.

Nothing else changes for this PR. The two things I asked for before merge still stand and are independent of the sequencing: the fingerprint key is not per-strap while the heart-rate cursor three lines above it is documented as per-strap, and the vitals fingerprint rests on HKQuantity.description rather than doubleValue(for:). Both are persisted, which is what makes them cheap now and awkward later.

I rebased this branch onto current main earlier today, so the parity side is already settled: a plain --refresh-derived works now that main's authority is migrated.

…int per strap; fingerprint vitals by value

The rebase onto ryanbr#2294 dropped the closing brace of nightIsStillOpen and of its test, so StrandImport
and both app builds failed. The written-batch key now carries the strap id like hrWriteCursorKey, and
the vitals fingerprint uses the value each sample was built from (bit pattern, fixed unit per metric)
instead of HKQuantity's description. The unlock observer is removed in deinit.
@Iskrata

Iskrata commented Sep 21, 2026

Copy link
Copy Markdown
Author

Pushed 0fbebd0c1. First, the reason CI is red on your rebase da2dcdfdc: resolving against #2294 dropped two closing braces, one after HealthWriteback.nightIsStillOpen and one after its test, so StrandImport and both app builds failed to compile. Both are restored. StrandImport passes (326 tests), NOOPiOS and Strand/macOS build, and parity_ratchet --offline reports 0 errors.

The two items you wanted before merge:

  • (2) Per-strap key. writtenBatchKey is now hkWrittenBatch.v1.<noopDeviceId>.<kind>, matching hrWriteCursorKey. It's still unshipped, so no migration.
  • (3) Vitals fingerprint. No longer built from HKQuantity.description. Each candidate now carries the Double it was built from, and the fingerprint uses its bit pattern. The unit is fixed per metric, and the metric is in the key, so it's stable by construction and never re-reads the quantity.

Minor: the unlock observer is now removed in deinit too.

I'll do (4), moving the tail-window selection into HealthWriteback with a test of the coverage argument, as a follow-up. I'll re-check recordWrittenBatch against #959 once it lands and rebase then. On today's file, save throws on an unauthorized type before the record call, as you read it.

moyasserr added a commit to moyasserr/noop that referenced this pull request Sep 21, 2026
Tricked-dev added a commit to Tricked-dev/noop that referenced this pull request Sep 21, 2026
Adapt the lock-deferral portion of ryanbr/noop PR ryanbr#2298 by @Iskrata (0fbebd0). Test coalescing, relock, foreground consumption, and teardown using an injectable notification center. Preserve existing HR reconciliation; omit the broader fingerprint and tail-caching changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants