Repository navigation
Stop an active trace from disabling the re-score reuse cache - #1575
Merged
Merged
Conversation
Turning on a Test Centre trace disabled the #1005 per-day reuse cache outright, so the diagnostic cost roughly what it was measuring. From a WHOOP 4.0 log with sleep + HRV traces on: analyzeRecent dayCache reused=0/0 size=0 days=21 (every pass) analyzeRecent cost prep=211663ms score=38583ms 13 minutes of heavy CPU in a ~70-minute session, on battery, 84% of it in the windowed store reads — every one of 21 days re-read and re-scored on every pass, because `dayCacheEligible` required all three per-day trace sinks to be null. The reason was real: a reused night would not re-emit its trace lines. The fix is the shape the always-on lines already use — record and replay. Swift needed almost nothing: `DayScan` already carries sleepTrace/hrvTrace/stepsTrace, a hit appends the whole scan, and the main-actor loop replays all three. It was safe by construction and the gate was the only cost. Kotlin emits its trace inline during pass 1, so it needed per-day recorders that append-and-forward, mirroring the existing `dayDiag` wrapper, plus replay on a hit. One correctness catch found while wiring it: the per-window HRV DETAIL is emitted only for `dayStart == nowLocalMidnight`. Once trace lines are replayed, the night cached as "today" would keep replaying its detailed trace after midnight, when a fresh scan would emit only the summary. That flag now forms part of the per-day key on both platforms, so the one affected day invalidates at rollover. The new parameter is deliberately NOT defaulted: #1567 was caused by exactly that, a defaulted parameter a caller silently omitted. `analyzeRecentOnCpu` is within ~100 bytes of the JVM's 64 KB per-method ceiling, so the recorders are built by a small class and the replay + the (pre-existing, unchanged) steps raw-counter emit are lifted into helpers. IntelligenceEngineJacocoBudgetTest caught this and is what the sizes were tuned against; no baseline was raised.
…miss
Re-review. My recorders appended AND forwarded live, so a freshly-scored night interleaved
its sleep and HRV lines as analyzeDay ran, while a REUSED night emitted them grouped by
channel. The same night read differently depending on a cache hit — which breaks the exact
promise the cache is built on, and the one I quoted defending it.
It also diverged from Swift, whose sink has always been collect-only
let traceSink = sleepTraceActive ? { sleepTrace.append($0) } : nil
with a single grouped emit in the main-actor loop. So Swift was already consistent and
Kotlin was about to stop being.
The recorders now collect only, and both the miss and hit paths emit through the same
`replayDayTraces`. One function, so the two can no longer drift. Emit order is
sleep -> hrv -> steps on both platforms, matching Swift's existing loop order.
This does change Kotlin's live trace ordering: those lines now land grouped at the end of
each day's block rather than interleaved through it. The content is identical, the
platforms now agree, and a cached night is genuinely indistinguishable from a fresh one.
Checked that no path drops collected lines: the cache-hit `continue` emits first, and the
<200-HR-sample `continue` runs before analyzeDay, so nothing has been collected yet.
Re-review, both small and both in the same spirit as the PR.
The per-day trace recorders were allocated at the top of the loop, before the cache-hit
`continue` — so every reused day built three ArrayLists and up to three closures and threw
them away. That is garbage on precisely the path this change exists to make cheap. They
now sit after the hit check, still well ahead of the analyzeDay call that uses them.
And `sleepSink?.let { _ -> { l -> sleep.add(l) } }` reads as though it captures the sink
when it is only testing it for null. Written as the null test it is.
Also checked this pass and found nothing to change: on both platforms and on both the hit
and miss paths, the day's always-on diag lines are emitted BEFORE its trace lines, so the
per-day block has the same shape everywhere.
Found while answering "so this PR helps with battery?" — which forced the question of who benefits and who pays. The rollover guard added the detail flag to the per-day cache key unconditionally. With the HRV trace OFF no detail line is ever produced, so the flag describes nothing — but it still flipped "d" to "s" at midnight and invalidated yesterday. That charged EVERY user one extra day's re-score per rollover to protect lines they never see, on the default path this change is not otherwise meant to touch. Gated on the trace being active on both platforms. The default path is now byte-for-byte what it was, and the cost is what the PR claims: paid only while a trace is on. Note this is the cache KEY only. The `hrvWindowDetail` argument that analyzeDay uses to decide whether to EMIT the detail is unchanged — that one is about what a fresh scan produces, not about what invalidates a reused one.
ryanbr
added a commit
that referenced
this pull request
Aug 24, 2026
…ebase of #1568) @pipiche38's #1568 rebased onto current main, with one integration fix on top. Their three commits are preserved with their authorship. #1568 was green and CLEAN, but the green was ~10 hours stale and the conflict was never textual: merging their branch with main pushed analyzeRecentOnCpu 30 bytes past the JaCoCo budget, because their SpO2 candidate branch and #1575's trace recorders both add to a method already within ~100 bytes of the JVM's 64 KB ceiling. Neither exceeded it alone. Verified on the integrated tree: 4283 Android tests 0 failures, release lint and i18n clean, and app-build green on macOS and iOS — the first compile of the integrated Swift, since their local Xcode builds predated the merge.
pipiche38
pushed a commit
to pipiche38/noop
that referenced
this pull request
Aug 24, 2026
Integration fix for @pipiche38's ryanbr#1568, rebased onto current main. Their branch passed CI and so did main, but the MERGE of the two failed: analyzeRecentOnCpu instrumented Code length 61565 exceeds the 61535-byte budget Thirty bytes. `analyzeRecentOnCpu` sits within ~100 bytes of the JVM's 64 KB per-method ceiling, and their device-conditional SpO2 candidate branch plus the trace recorders from ryanbr#1575 both add to it. Neither exceeded the budget alone — only together, which is invisible to either PR's own CI. ryanbr#1568's checks were also ~10 hours stale by then, predating all six commits main had gained, and GitHub reported the branch CLEAN because the conflict is not textual. Their 14-line branch becomes a call to a new private `spo2CandidateMean`. Behaviour is unchanged: same two paths, same `?.first`, same null cases, still gated by the caller's toggle, still never written to `spo2Pct` and never scored. Also worth recording for them: the "2 pre-existing failures in RecoveryDriversTest" in the PR description do not reproduce. That class is 10/10 on main and 10/10 on their own head, and their full suite is 4270/0 rather than the 2404/2 reported — a stale local Gradle/KSP cache, which this repo has a documented history of. Nothing was broken. Integrated tree: 4283 tests, 0 failures. Release lint, doc-comments and i18n clean.
xxblyxx
added a commit
to xxblyxx/noop
that referenced
this pull request
Sep 1, 2026
…ache Port of upstream ryanbr#1575 (Swift half). Turning on a Sleep/HRV/Steps trace disabled the ryanbr#1005 per-day reuse cache outright, so every night was re-read and re-scored on every pass — the diagnostic cost roughly what it was measuring. Upstream measured `prep=211663ms score=38583ms` on a WHOOP 4.0 with two traces on: 13 minutes of CPU in a ~70-minute session, on battery. The old reasoning was that a reused night would not re-emit its gate trace. In Swift that was already false, and I verified it in THIS tree rather than carrying upstream's claim about its own code: `DayScan` carries sleepTrace / hrvTrace / stepsTrace, a cache hit does `out.append(cached.scan)`, and the main-actor fold replays all three from the scan for every entry in `out`, hits included. That replay is the load-bearing half — had it covered only some of the three, flipping the gate would silently drop trace lines. It covers all three, so the gate was the only cost. One correctness catch that comes with it: the per-window HRV DETAIL is emitted only for `dayStart == nowLocalMidnight`. Once traces are replayed, the night cached as "today" would keep replaying detail after local midnight, when a fresh scan would emit only the one-line summary — silently breaking the cache's whole promise that a reused night is indistinguishable from a freshly-scored one. The entitlement is now part of the per-day key, so exactly the one affected day invalidates at rollover. Kept upstream's `hrvTraceActive &&` gate on that flag. With the HRV trace off the flag describes nothing, but it would still flip at midnight and invalidate yesterday — charging every user an extra day's re-score to protect lines they never see. The parameter is deliberately NOT defaulted: upstream's ryanbr#1567 was caused by exactly that, a defaulted parameter a caller silently omitted, which quietly changed the skin-temp scale. Not load-bearing for the symptom under investigation — this device's only Test Centre key is `testcentre.active.workouts = False`, so no trace is active and the 2026-08-26 numbers are unaffected. Taken now because the analyze-pass-cost work turns traces ON to investigate; left alone, the instrument would dominate the measurement it exists to take. Verification: `xcodegen generate`; `xcodebuild -scheme NOOPiOS -destination generic/platform=iOS build` BUILD SUCCEEDED; `xcodebuild -scheme Strand -destination platform=macOS test` 1221 tests, 1 skipped, 0 failures; `swift test` in Packages/StrandAnalytics 1501 tests, 0 failures, including two new AnalyzeRecentDayCacheTests cases pinning that the detail flag invalidates and that it is inert with the trace off; doc_comment_lint and i18n_audit clean. No CI covers the app targets, so these local runs are the gate. Not yet observed on device. Kotlin twin: NOT written, and Android keeps the old gate. Upstream's Kotlin port is 110+ lines of per-day recorder plumbing, needed because that engine emits its trace inline during pass 1 rather than collecting it onto a scan — a genuinely different change, not a mirror of this one. It cannot be compiled or tested on this machine (no JDK, no Android SDK) and no CI covers it, so writing it blind alongside a correctness-sensitive cache-key change would be worse than leaving the platforms explicitly out of step here. The key strings are already documented as not required to match across platforms (the cache is in-memory and per-platform); what must match is the invalidation contract, and Android's is unchanged because its gate is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017g6WqitM2Y76kkYsHzpXab
xxblyxx
added a commit
to xxblyxx/noop
that referenced
this pull request
Sep 1, 2026
…ntry Two claims from `4104608d` and `a2eac10d`, neither of which is a speedup — the entry says so explicitly, because the obvious misreading is "the perf fix is pending validation". Commits 1 and 1b make the pass measurable and remove a latent trap; the morning is expected to feel exactly as slow as it did on 2026-08-26. (a) The three new diagnostic lines are readable and DECISIVE on a real morning — that they appear on every completed pass, pair with `re-score: done`, and actually separate a pass-signature drop from per-day eligibility, which is the ambiguity that left the 2026-08-26 diagnosis resting on inference. Installed after that morning's sync, so the canonical launch-pass measurement is the next one. (b) The ryanbr#1575 port did not break trace replay. This is the larger debt and it is entirely unobserved: no Test Centre trace has ever been active on this device, so the path Commit 1b now depends on — a cache HIT replaying three trace arrays — has never once run. The package tests pin the key's invalidation contract, not the replay. The owner was advised not to turn traces on (log noise, 2,000-line tail), which is right for (a) and is exactly what leaves (b) unchecked, so (b) needs its own deliberate session rather than riding along on a normal morning. `passes-if` decides NOW, before the data exists, what each outcome of the second pass's `DROPPED` line means — including the two that REFUTE the follow-up plan's premise (a `sig changed:` naming something other than the three sleep-derived fields, or no `DROPPED` line at all alongside `reused=0`). Written this way so a future session cannot read a result as confirmation after the fact. The Commit 4 gate (`prep` vs `score`) is in the same table, with upstream's 60-84%-reads prior flagged as their hardware and their store, not necessarily transferable here. Also records the working `plistlib` pull command — `plutil -extract strapLog.tail` treats the literal dot as a key path and fails, which both sibling plans got wrong — and the actual log retention (2,000 live lines plus 3 generations x 1,000), which bounds how long after a sync the check is still possible. Verified with `python3 Tools/pending-validation.py --list`: the entry parses and reports `not until 2026-08-27`. Not ripe today, so the SessionStart hook stays quiet, which is correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017g6WqitM2Y76kkYsHzpXab
xxblyxx
added a commit
to xxblyxx/noop
that referenced
this pull request
Sep 1, 2026
The commit that targets the owner's actual complaint. "When I open the app in the morning" is a cold-launch pass by construction: dayScanCache is in-memory only and dayScanCacheConfigSig starts "", which never equals a real signature, so the FIRST pass of every process drops the whole cache and re-reads plus re-scores every night. Measured 2026-08-27, backgrounded, 9 nights: 2403s. Commits 1, 1b, 2 and 5 do nothing for that pass. SHAPE: the plan recommended rebuilding each night from banked state and persisting only the cache key. Audited first, and it is not possible losslessly: nightlySkinTempC is read by the fold (:1503, :1532) and nothing banks the raw degC mean - only the baseline-relative 2dp skinTempDevC, nil outright when the baseline is unusable. workouts lose zoneTimePct/avgHRRPct/hrmax/caloriesKJ into WorkoutRow, and bouts overlapping a real workout are never banked at all. Shipped instead: persist the PROJECTION pass 2 consumes. A cached scan has one consumer - on a hit, pass 1 does out.append(cached.scan); continue and performs no read, no score, no side effect. Pass 2 reads all 13 DayScan fields but only 7 of DayResult's 15; the other 8 are read only inside pass 1 before caching, and pass 2 recomputes its own from baselines2 anyway. Restoring them at DayResult's own defaults reproduces what a cache hit hands pass 2 today, byte for byte. Also smaller than serializing DayScan wholesale: two new Codable types, not nine. INVALIDATION - the plan was wrong in both directions, corrected here: - The ".dataChange drops the cache, one line" seam DOES NOT EXIST. removeAll() appears once (:854), gated on the signature alone; AnalyzePolicy is a pure rate-limiter that never touches the cache. - Manual sleep edits and dismissed sleep spans need NO invalidation. Both apply in pass 2 (:1701-1703, :2186), fresh every pass, after the cache. A registry owner change is already in the per-day key. - The one real hole, which the plan missed: the ryanbr#899 banked-sleep heal deletes banked rows whose only route into a later pass is bandSleepStateSamples - a pass-1 read NOT in the per-day key - and its own re-arm would then re-run a pass that reuses the very days it invalidated. Until now that healed by accident on relaunch. Dropped at the delete site, since the heal raises no analyzeRecent call of its own. - Second hole, also not in the plan: the three trace arrays are replayed on a cache HIT (ryanbr#1575), so a persisted scan captured with the Sleep trace on would replay trace lines after it was switched off. The three trace-active booleans now sign the pass: one drop per toggle, which is what ryanbr#1575 intended. STORAGE: Application Support/noop-dayscan-cache.json, version-tagged, isExcludedFromBackup. Not UserDefaults - the two session bands are on a 30s grid (~960 entries/night), so this is hundreds of KB and UserDefaults loads wholesale at launch. Deliberately NOT added to BackupSettings.whitelist, which is opt-in, so a restored .noopbak cannot import another device's cache (precedent: noop.analyzeWatermark, :2372). Any decode failure or version mismatch reads as "no cache" - i.e. the cold pass we already have. No Kotlin twin, as a judgement call rather than a rule: a device-local derived cache with no schema, migration, formula or stored analytics result. It changes WHICH days recompute, never WHAT they compute to, so both platforms still produce identical scores; Android keeps the in-memory-only cache and repeats more work after a relaunch. KOTLIN NOT COMPILED HERE either way - no JDK, no Android SDK, no CI. Verification: - xcodebuild -scheme Strand -destination platform=macOS test -> 1228 tests, 0 failures (1 pre-existing skip: XiaomiImporterIntegrationTests). Includes 7 new DayScanCacheStoreTests: field-by-field round trip of everything the fold reads, an edited session's adjusted onset, the 8 omitted fields pinned at their defaults, version mismatch rejected, corrupt file rejected, save/load including the config signature, and clear. - Packages/StrandAnalytics: swift build clean after the two Codable additions. - App targets compiled (CI does not build them): -scheme Strand macOS and -scheme NOOPiOS -destination id=819D37A3... both BUILD SUCCEEDED. - python3 Tools/doc_comment_lint.py -> OK. - NOT observed on hardware. Expected: pass 1 reuses 8 of 9 nights, so ~300s instead of 2403s. Recorded as claim (c) in PENDING_VALIDATION, along with the two sub-claims no test can reach: the ryanbr#899 heal drop and the trace-toggle drop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013p6RwiHQZaea2aNPh5jySa
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.
Turning on a Test Centre trace disabled the #1005 per-day reuse cache outright, so the diagnostic
cost roughly what it was measuring.
From a WHOOP 4.0 log with sleep + HRV traces on:
13 minutes of heavy CPU in a ~70-minute session, on battery, 84% of it in the windowed store
reads — all 21 days re-read and re-scored on every pass, because
dayCacheEligiblerequired allthree per-day trace sinks to be null.
The reason was real: a reused night would not re-emit its trace lines. The fix is the shape the
always-on lines already use — record and replay.
The two platforms needed very different work
DayScancarriessleepTrace/hrvTrace/stepsTrace, a hit appends the whole scan, and the main-actor loop replays all three. The gate was the only thing costing the re-score.dayDiagwrapper — plus replay on a hit.Same contract, different amount of code, because the engines emit diagnostics at different points.
A correctness catch found while wiring it
The per-window HRV detail is emitted only for
dayStart == nowLocalMidnight. Once trace linesare replayed, the night cached as "today" would keep replaying its detailed trace after midnight —
when a fresh scan would emit only the one-line summary. That silently breaks the cache's whole
promise, that a reused night is indistinguishable from a freshly-scored one.
The flag is now part of the per-day key on both platforms, so exactly the one affected day
invalidates at rollover. Cost: one day's re-score per midnight, and only while a trace is on.
The new parameter is deliberately not defaulted. #1567 — merged earlier today — was caused by
precisely that: a defaulted parameter a caller silently omitted. Every call site now has to state
its answer.
Parity
Diffed, not assumed:
"\(owner)|\(hrCount):\(hrMaxTs):\(anchor):\(hrvWindowDetail ? "d" : "s")""$owner|$hrCount:$hrMaxTs:$anchor:${if (hrvWindowDetail) "d" else "s"}"let dayCacheEligible = trueval dayCacheEligible = trueA divergence I introduced and then removed. The first cut had the Kotlin recorders append and
forward live, so a freshly-scored night interleaved its sleep and HRV lines while a reused night
emitted them grouped — the same night reading differently depending on a cache hit, which breaks the
exact promise the cache is built on. Swift has always been collect-only
(
traceSink = { sleepTrace.append($0) }) with one grouped emit. Kotlin now matches: recorderscollect, and both the hit and miss paths emit through the same
replayDayTraces, so the twocannot drift.
Consequence worth stating: Kotlin's live trace lines now land grouped at the end of each day's
block rather than interleaved through it. Same content, and the platforms now agree.
Bytecode budget
analyzeRecentOnCpusits within ~100 bytes of the JVM's 64 KB per-method ceiling. The first cutpushed the JaCoCo-instrumented size to 63954 against a 61535 budget, so:
IntelligenceEngineJacocoBudgetTest(#1524) caught it and is what the sizes were tuned against. Nobaseline was raised.
Verification
:app:testFullDebugUnitTest --no-build-cache --rerun-tasks— 4274 tests, 0 failures.doc_comment_lint.pyOK.swift-packages.yml.Strand/Data/IntelligenceEngine.swiftis app-target.reused=~20/21instead of
reused=0/0, andprepcollapsing with it. That is the number to check, not the tests.Honest coverage note
The engine-level behaviour — that a reused night under an active trace replays the identical lines —
is not unit-tested. There is no engine-with-repository harness in the tree, and building one is
its own piece of work. What is tested is the key contract; the rest rides on the device measurement
above.