Repository navigation
analytics: collapse the per-day sleep-skip spam to one line per pass - #1626
Merged
Merged
Conversation
A scoring pass walks a fixed recent window, so a day whose raw HR never arrived is re-read, re-skipped and re-logged on every pass, forever. The strap banks days rather than weeks of raw HR while an import can supply a much longer spine of daily rows, so the steady state for an importing user is a permanent block of un-scoreable days. In a field capture that came to 1262 lines — 21 days re-skipped across 63 passes in two hours, about a fifth of everything the log had to say. The strap log is a fixed-size rolling buffer, so this is not merely noise: it evicts the older lines an investigation needs. Collect the skipped days and emit ONE line per pass, grouped by sample count. Lossless — every day and its own HR count is still printed, listed rather than reduced to a range, because a range would hide a gap and which days lack raw HR is exactly what an investigation is looking for. Nothing skipped stays silent. The Android side has a mechanical constraint worth recording. The first cut put the accumulator in a local, and that blew the JVM method-size budget #1524 guards by 444 bytes — seven times the 64 bytes of headroom the method had. The cost was not the three added lines: analyzeRecentOnCpu is `suspend`, so one more local live across a loop full of suspension points is saved and restored at every one of them. Moving it to a field — safe for the same reason dayScanCache is a field, every pass runs under analyzeGate — brought it back to +24 bytes, and moving the null-check into the collector left the method 40 bytes under budget rather than 1. Both platforms, byte-identical output, verified by diffing the two implementations across five cases rather than by eye. Verified: compileFullDebugKotlin clean; the full com.noop.analytics suite (1685 tests) green; 5 new JVM tests and a twinned Swift test in StrandAnalytics, which default CI runs; the #1524 budget test passes at 61495/61535; doc_comment_lint and i18n_audit --ci clean.
Re-review of the previous commit: on both platforms the summary emit was inserted between the #1538 comment and the dayCache line that comment describes, so it read as documentation for the new call instead. Same shape as the detached-doc-comment trap, but with `//` comments, which the doc lint does not check — it only binds `/** */` and `///` blocks. Moved above the comment block on both sides. No behaviour change; the summary still emits immediately before the dayCache line, in the same order on both platforms. Also confirmed while re-reviewing: both analyzeRecentOnCpu call sites are inside analyzeGate.withLock (opened 420, closed 437), so the collector field genuinely has no concurrent access; the #899 heal re-pass calls the method a second time under that same lock, where reset() correctly gives that pass its own summary, matching the old two-sets-of-lines behaviour; and there is no early return between reset() and emit() on either platform, so a pass can never collect days and then drop them. compileFullDebugKotlin clean; budget test 61495/61535; 5 JVM tests green; doc_comment_lint clean.
Parity re-review of the collapse change. Kotlin gates on a named constant, MIN_HR_SAMPLES = 200, in one place. Swift carried the bare literal at the guard, and the previous commit added a SECOND executable copy at the new summary call — so a threshold change had to find every copy on this platform and exactly one on the other, and I had just made that worse. Added `IntelligenceEngine.minHrSamples` and routed both uses through it. The two constants are the same number; nothing about the emitted line changes. Also recorded the one structural asymmetry deliberately, at the Swift declaration: the collector is a LOCAL here and a FIELD in Kotlin, because Kotlin's analyzeRecentOnCpu is `suspend` and sits against the JVM method-size ceiling, where an extra local live across the loop costs a save/restore at every suspension point. Swift has no such constraint and the emitted line is identical either way — the note says not to "align" the two shapes, since doing so would either cost Kotlin its margin or add a field to Swift for no reason. Two prose references to `hr.count >= 200` are left as they are: the value is unchanged, so they still describe the behaviour accurately. compileFullDebugKotlin clean; budget test 61495/61535; JVM tests green; doc_comment_lint and i18n_audit clean. The Swift edits are app-target and need app-build.
Both new uses sit inside the Task.detached @sendable closure, and `Self.` in that position raises questions the explicit type name does not — dynamic Self on a class, and whether the reference implies a capture. The class is final so `Self` should resolve statically, but no app-target Swift compiles outside app-build, and "should" is doing real work in that sentence. The boring form costs a few characters and cannot be wrong. Recorded the reason at the declaration so it does not get tidied back. Also confirmed while re-reviewing: on both platforms the skip path `continue`s before the day reaches the reuse cache, so no cached day can replay a diagnostic in the old per-day format — the cache's own docstring already says a night under the sample floor never reaches it. No behaviour change. compileFullDebugKotlin clean; budget 61495/61535; JVM tests green; doc_comment_lint and i18n_audit clean.
ryanbr
added a commit
that referenced
this pull request
Aug 26, 2026
Re-review, and this one would have produced a confidently wrong diagnostic. The outcome line rendered onCharacteristicWrite's status through writeStatusLabel, which maps BluetoothStatusCodes — the return value of writeCharacteristic, a DIFFERENT enumeration that collides with GATT_* on small integers: status 1 GATT_INVALID_HANDLE -> "ERROR_BLUETOOTH_NOT_ENABLED(1)" status 2 GATT_READ_NOT_PERMITTED -> "ERROR_BLUETOOTH_NOT_ALLOWED(2)" status 3 GATT_WRITE_NOT_PERMITTED -> "ERROR_DEVICE_NOT_BONDED(3)" In a line whose only job is explaining a bond failure, naming the wrong error is worse than naming none. Added a GATT-domain labeller that names the codes that matter for a bond — INSUFFICIENT_AUTHENTICATION(5) and INSUFFICIENT_ENCRYPTION(15), the pair BondRefusalGiveUp already keys on, plus GATT_ERROR(133) — and leaves everything else a bare number rather than guessing. A test asserts the two mappers DISAGREE on 1/2/3. If they ever agree, one of them has been pointed at the wrong domain. Also moved the disconnect report above the #1151 comment it had been inserted into. It had landed between that comment and flushFrameTimingSummary(), so the comment read as describing my block — the same splitting mistake as the #1538 one in #1626, and the third comment/doc rebinding this session. Both other report sites verified in place: the rejection clear is inside the `if (!ok)` block, and the callback report is the first statement of the 5/MG branch, before didBond is set. compileFullDebugKotlin clean; com.noop.ble suite green; doc_comment_lint clean — and gated on this time rather than run alongside the commit.
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
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.
The problem
A scoring pass walks a fixed recent window, so a day whose raw HR never arrived is re-read, re-skipped and re-logged every pass, forever:
The strap banks days rather than weeks of raw HR, while an import can supply a much longer spine of daily rows — so for an importing user the steady state is a permanent block of un-scoreable days that can never become scoreable.
In the field capture on #1617 that was 1262 lines, about a fifth of everything the log had to say. The strap log is a fixed-size rolling buffer, so this is not merely noise — it evicts the older lines an investigation needs. I hit this reading that log.
The change
Collect the skipped days; emit one line per pass, grouped by sample count:
Lossless on purpose. Every day and its own HR count is still printed, listed rather than reduced to a range — a range would hide a gap, and which days lack raw HR is exactly what an investigation is looking for. A day sitting just under the floor (
hrSamples=199) still shows its number, which is the tuning signal. Nothing skipped stays silent.A mechanical constraint worth recording
The first cut put the accumulator in a local, and that blew the JVM method-size budget #1524 guards by 444 bytes — seven times the 64 bytes of headroom
analyzeRecentOnCpuhad (main sits at 61,471 of 61,535).The cost was not the three added lines.
analyzeRecentOnCpuissuspend, so one more local live across a loop full of suspension points is saved and restored at every one of them. Two moves fixed it:The field is safe for the same reason
dayScanCacheis a field: every pass runs underanalyzeGate, so there is no concurrent access — the existing docstring already states that guarantee. Landing at a margin of 1 would have left the next person to touch this method with no room, hence the second move.Parity
Both platforms, byte-identical output — verified by extracting the Swift implementation, running it standalone, and diffing its stdout against the JVM's across five cases, not by eye. The Swift helper lives in
StrandAnalyticsrather than the app target specifically soswift testcovers it in default CI.Verification
compileFullDebugKotlinclean; the fullcom.noop.analyticssuite — 1685 tests — green under--rerun-tasks --no-build-cache.StrandAnalytics(a wired, pre-existing test target, sotest (StrandAnalytics)runs it).doc_comment_lintclean — it caught me inserting the new field betweendayScanCache's doc block and its declaration, which is fixed.i18n_audit --ci mainclean.