fix(connection): make duration rounding deterministic - #1473
Merged
Conversation
ryanbr
added a commit
that referenced
this pull request
Aug 20, 2026
Fourth re-review, parity pass. The two implementations rounded beat-time with DIFFERENT functions: Swift `.rounded()` is half-away-from-zero, Kotlin `kotlin.math.round` is half-toward-positive-infinity. They agree here only because these sums are positive. That is the same shape as #1473, where `%.1f` looked equivalent across platforms and was not - Foundation rendered 50 ms as 0.0 while Java rendered 0.1. An agreement that holds only for the inputs we happen to feed it is not parity, it is a coincidence waiting for a negative or a different call site. Both now use `x + 0.5` truncated - half-up, no stdlib rounding on either side, so the agreement is structural. Pinned by a mirrored test including 2.5 -> 3, which half-to-even would render as 2. Also verified this pass, mechanically rather than by eye: the two functions match on all seven structural decisions (ord-0 test, the >= 2 multi threshold, the 4+ histogram clamp, the unknown skip, the beat-time sum, msToInt, secsNoStart), and the three rendered strings extracted from both test suites are identical. Verification: compileFullDebugKotlin clean, full Android unit suite green, HrvAnalyzerSampleOrdTest 8, doc_comment_lint clean.
ryanbr
added a commit
that referenced
this pull request
Aug 20, 2026
Instrumentation for #1331 / #1008 / #1118 / #1451, which are one defect. `ord` restarts at 0 on every delivery, so two rows on one second both carrying ord 0 came from two different offloads writing the same wall second. The 5/MG log on #1451 shows exactly that - `-1s[872#0, 893#0]` - but densestSecondWindowSample only dumps the 5-8 seconds around the densest one. That is a sample, not a measurement, and the fix turns on a question it cannot answer: is the over-count mostly seconds touched by SEVERAL deliveries, or genuinely too many beats inside one? If the former, the fix belongs at ingest and this sizes it. rr deliveries secs[1/2/3/4+]=2/1/0/0 multiSec=33% multiRows=33% multiMs=36% maxDeliv=2 secsNoStart=0 ordUnknown=0 Read-side only: no new capture, no write-path change, so it lights up on data already stored the moment the app opens. Four review passes, four things it would otherwise have got wrong - none of which would have failed a build, because each produced a plausible number rather than an error: - multiRows divided by every row including unattributable ones, so a night that half-predates the ord column would have read artificially benign - the exact conclusion this exists to prevent. It now divides by attributable rows. - Seconds whose ord-0 row was absorbed by the primary key vanished from `secs` entirely, shrinking the multiSec denominator invisibly. Now counted as secsNoStart. - rrMs was taken and never read - only its count. Coverage is the sum of rrMs over wall span, so beat-time is what inflates it; multiMs now reports the share on multi-delivery seconds, which is the number the fix is sized against. - Beat-time was rounded with `.rounded()` on Swift and kotlin.math.round on Kotlin, which agree only for positive values - the #1473 shape. Both now use `x + 0.5` truncated, so the agreement is structural. Optimised on the last pass: four collections keyed by the same second became one tally map, one hash lookup per row instead of three or four. This runs once per over-counted night and analyzeRecent re-scores ~21 days every 15 minutes, so the redundancy was millions of lookups per cycle for a diagnostic that feeds nothing. The verbatim-line tests passed untouched through the rewrite, which is what made it safe to attempt. Verification: mirrored tests on both platforms asserting the rendered line verbatim, the three expected strings extracted from both suites and compared identical, full Android unit suite green, HrvAnalyzerSampleOrdTest 8, doc_comment_lint clean, 14 CI checks including test (StrandAnalytics), and both app-build legs green. No behaviour change: nothing here feeds a stored value, a gate or a score.
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
2 participants
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.
Summary
ConnectionTrace.sessionHeldSuffixuse explicit integer HALF-UP quantization to one decimal on Swift and Androidprintf/Locale/Double tie rounding from the shared diagnostic contractVerification
0.0vs0.1) and 250 ms (0.2vs0.3) failed; 150 ms and existing values passedgit diff --check: cleanAndroid execution was serial and bounded: JDK 17, no daemon, one worker, no parallel execution, Kotlin in-process, 1536 MiB heap. Temporary Linux-only Swift gates were fully reverted before commit.
Fixes the parity contract tracked at bhelm#87.
Provenance: #1023 / #1020.