Skip to content

fix(health): hold a still-open night out of Apple Health until it closes - #2294

Merged
ryanbr merged 2 commits into
ryanbr:mainfrom
Iskrata:fix/health-hold-open-night
Sep 21, 2026
Merged

ryanbr merged 2 commits into
ryanbr:mainfrom
Iskrata:fix/health-hold-open-night

Conversation

@Iskrata

@Iskrata Iskrata commented Sep 17, 2026

Copy link
Copy Markdown

What this PR does

Stops Apple Health write-back from exporting a night that the strap may still be recording.

A night is detected from whatever has synced so far, so a pass during the night ends it at the newest sample. That truncated night was then written to Apple Health as if it were finished. A reader took its end as the wake, and its day's vitals had been scored from part of the night. On one install, a night slept to 09:36 was in Health as ending at 06:53 until a later write-back replaced it. Replaying the stager over the same data confirms the detector itself is right: with data to 06:54 it ends the night at 06:54, and with the full data at 09:36.

  • HealthWriteback.nightIsStillOpen(endTs:newestHeartRateTs:now:) treats a computed night as open when it ends within openNightMarginSeconds (20 min) of the strap's newest heart rate. After openNightMaxHoldSeconds (2 h) it counts as closed anyway, so a strap taken off at wake does not hold the night back indefinitely.
  • HealthKitBridge.writeBack leaves open nights out of the sleep, heartbeat and vitals writes and skips their day's vitals rows. The next write-back after the night closes writes them normally.
  • The frontier is read from the strap's own heart rate (hrFingerprint(deviceId: noopDeviceId, …)). An Apple Watch still recording therefore cannot make a strap night that stopped at its sync frontier look finished.

Type of change

  • Bug fix

How it was tested

  • New HealthWritebackTests: a night ending at the newest heart rate is open, a night with heart rate 20 min past its end is closed, and the 2 h hold limit.
  • doc_comment_lint.py is clean. parity_ratchet.py --base upstream/main --offline reports 0 errors after recording the new Swift-only helper in parity_dispositions.json.

Swift-only. The Android Health Connect exporter does not hold an open night.

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 marked this pull request as ready for review September 20, 2026 06:43
@ryanbr

ryanbr commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Reviewed at dcab53c. Full 18-check roster green, parity-governance included. This is strong work, and the core predicate is right on every branch I traced:

Situation newestHeartRateTs - endTs Result
Night ends at the sync frontier about 0 open, held
Night over, strap still recording large closed, written
Night over, strap on charge about 0 held, then written at the 2 h cap

The tests are boundary-exact on both sides of both thresholds (2 * 3_600 - 1 true against 2 * 3_600 false, and 20 * 60 reading closed), which is the part people usually leave loose. The parity dispositions are three honest per-symbol entries with a real rationale rather than a blanket waiver. And openDays keys on the night's END day, which is how AnalyticsEngine attributes a session to a day; that is easy to get backwards and you did not.

Four things, none of them blocking:

  1. ?? 0 conflates a failed read with a real answer. (try? await ...hrFingerprint(...).maxTs) ?? 0 gives 0 both when the read throws and when there is no strap heart rate at all, since the SQL is COALESCE(MAX(ts), 0). Then 0 - endTs is a large negative number, which is < openNightMarginSeconds, so every recent night reads as open and is held for up to 2 h. That fails in the safe direction and it self-heals, so I am not asking for a redesign. But it is silent, it is the one path with no test, and for someone on an Apple Watch with no strap heart rate it is the steady state rather than an error case. A strap-log line when the frontier is unknown would make it diagnosable, and a test pinning the 0 case would stop a later change from flipping the direction.

  2. The description claims more than the diff does. It says open nights are kept out of "the sleep, heartbeat and vitals writes", but writeHeartRate is untouched. Holding sleep and vitals while still writing raw heart-rate samples looks correct to me, since the samples are not a night, so I think the prose is loose rather than the code being wrong. Worth confirming rather than assuming, in case something was intended and missed.

  3. An imported session can be caught by its computed twin. openStarts is built from computedSleeps, but sessions lets importedSleeps win at the same startTs. So a complete imported session that shares a start with an open computed night is held back too. Low impact, given it came from Health in the first place.

  4. holdingDays: Set<String> = [] has one call site and you updated it. The default only leaves room for a future caller to skip the hold without noticing.

Approving. If you want to take (1) in this PR I would happily wait for it, otherwise it is a good follow-up.

One scheduling note: this and #2298 both create the dispositions array in the same JSON and touch overlapping regions of HealthKitBridge, so they cannot both merge as they stand. I would rather take this one first, since it is smaller and already clean, and have #2298 rebase on top. That also matches how the two relate: this decides what is in a batch, that one decides whether to rewrite it.

@ryanbr
ryanbr force-pushed the fix/health-hold-open-night branch from dcab53c to e7d69cb Compare September 21, 2026 01:09
@ryanbr
ryanbr merged commit a6c1c01 into ryanbr:main Sep 21, 2026
18 checks passed
Iskrata added a commit to Iskrata/noop that referenced this pull request Sep 21, 2026
…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.
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