From a1e84a1cd46fa3c640c58f535d54ade84072d7d8 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Thu, 27 Aug 2026 23:34:51 +1200 Subject: [PATCH] hrv: state the real pct overflow bound, and pin that the wrap could land positive Follow-up to #1685, which widened HrvAnalyzer.pct correctly. Two things about it are worth getting exactly right, because the doc comments it added are what the next person will reason from. THE QUOTED BOUND IS THE WRONG ONE, AND IT UNDERSTATES THE EXPOSURE. Both comments say the overflow starts above 2147483647 / 200 = 10,737,418 ms. That is the bound for `part * 200` in isolation, but the numerator is `part * 200 + total`. Since part <= total, the numerator first exceeds Int.MAX_VALUE at total >= 10,683,999 (2147483647 / 201), about 53,000 ms lower and 2.97 h rather than 2.98 h. 10,737,418 is also the last value that does NOT overflow even for the multiply alone; that starts at 10,737,419. THE WRAP DID NOT ALWAYS LAND NEGATIVE. This is the half that matters. A 100% multi-share 8 h night, pct(28_800_000, 28_800_000), returned 25 in 32 bits: positive, plausible, and wrong by 75 points. So a negative multiMs was a symptom of the bug and never a test for it, and anyone re-reading pre-fix Android captures for #1008 / #1118 / #1331 / #1451 cannot filter on the sign. Every Android multiMs past the bound is suspect, including the ones that read fine. That is a stronger claim than the one the original made, and it changes how the existing logs should be treated. Comments on both platforms corrected, plus a twin test pinning the exact boundary and the readable-wrong case: 10,683,998 was still computed correctly in 32 bits, 10,683,999 wrapped to -100, 28,800,000 wrapped to 25. Verified rather than reasoned. Two independently written models of 32-bit truncation - one in Swift via the standalone oracle, one in Python - agree on all five values, and both reproduce the -16 the submitter's real 08-22 and 08-26 nights printed, which is what makes them worth trusting for the 25. The new test was falsified before being trusted: with pct reverted to Int arithmetic it fails expected:<100> but was:<-100> on the 10,683,999 case - a value the original comment described as safe, which is the correction, executable. Restored, 10 tests 0 failures, on freshly cleared result XMLs. python3 Tools/doc_comment_lint.py: OK, no new detached doc comments. --- .../Sources/StrandAnalytics/HRVAnalyzer.swift | 16 +++++++++++----- .../HRVAnalyzerSampleOrdTests.swift | 14 ++++++++++++++ .../java/com/noop/analytics/HrvAnalyzer.kt | 18 ++++++++++++------ .../noop/analytics/HrvAnalyzerSampleOrdTest.kt | 15 +++++++++++++++ 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/HRVAnalyzer.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/HRVAnalyzer.swift index 603aff60c4..f435b709a7 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/HRVAnalyzer.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/HRVAnalyzer.swift @@ -810,11 +810,17 @@ public enum HRVAnalyzer { /// Whole-percent, integer half-up, so both platforms round a tie the same way. 0 when `total` is 0. /// /// The arithmetic is widened to `Int64` because `deliveryHistogram` passes BEAT-TIME IN - /// MILLISECONDS summed over a whole night, not a row count. `part * 200` needs more than 32 bits - /// above 10,737,418 ms (2.98 h of beat-time on multi-delivery seconds) and a real over-count night - /// carries 3-4x that. `Int` is 64-bit on iOS/macOS so this platform read correctly while the Kotlin - /// twin wrapped to a negative percentage; it is 32-bit on `arm64_32`, and StrandAnalytics already - /// builds for watchOS, so the width is stated rather than inherited. Twin of Kotlin `pct`. + /// MILLISECONDS summed over a whole night, not a row count. The numerator is `part * 200 + total`, + /// so with `part <= total` it needs more than 32 bits from `total >= 10,683,999` ms (2.97 h of + /// beat-time on multi-delivery seconds) and a real over-count night carries 3-4x that. The multiply + /// ALONE would survive to `part >= 10,737,419`; the `+ total` term is what brings the bound down, so + /// quoting the multiply's limit understates the exposure. `Int` is 64-bit on iOS/macOS so this + /// platform read correctly while the Kotlin twin wrapped; it is 32-bit on `arm64_32`, and + /// StrandAnalytics already builds for watchOS, so the width is stated rather than inherited. + /// + /// The wrap did not always land negative, which is the dangerous half: a 100% multi-share 8 h night + /// (`pct(28_800_000, 28_800_000)`) returned 25 in 32 bits - positive, plausible, and wrong by 75 + /// points. A negative percentage was a symptom of this, never the test for it. Twin of Kotlin `pct`. static func pct(_ part: Int, _ total: Int) -> Int { total > 0 ? Int((Int64(part) * 200 + Int64(total)) / (Int64(total) * 2)) : 0 } diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/HRVAnalyzerSampleOrdTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/HRVAnalyzerSampleOrdTests.swift index 83d6e8c6a7..45e1ff6361 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/HRVAnalyzerSampleOrdTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/HRVAnalyzerSampleOrdTests.swift @@ -91,4 +91,18 @@ final class HRVAnalyzerSampleOrdTests: XCTestCase { XCTAssertEqual(HRVAnalyzer.pct(15_264_177, 35_498_088), 43) XCTAssertEqual(HRVAnalyzer.pct(36_296_594, 39_886_368), 91) } + + /// Twin of Kotlin `pctWrapWasNotAlwaysNegative`. The 32-bit wrap did not always produce a NEGATIVE + /// percentage, so "is it negative" was never a sound test for an affected capture. + /// + /// These pin the exact boundary and the readable-wrong case. The bound is `part * 200 + total` + /// exceeding `Int32.max`, not `part * 200` alone: 10,683,998 was still computed correctly in 32 + /// bits, 10,683,999 wrapped to -100, and a 100% multi-share 8 h night wrapped to 25 - positive, + /// plausible, and wrong by 75 points. `Int` is 64-bit here, so these pass either way; they are a + /// parity pin and a guard for `arm64_32`, where the Kotlin failure would reproduce exactly. + func testPctWrapWasNotAlwaysNegative() { + XCTAssertEqual(HRVAnalyzer.pct(10_683_998, 10_683_998), 100) + XCTAssertEqual(HRVAnalyzer.pct(10_683_999, 10_683_999), 100) + XCTAssertEqual(HRVAnalyzer.pct(28_800_000, 28_800_000), 100) + } } diff --git a/android/app/src/main/java/com/noop/analytics/HrvAnalyzer.kt b/android/app/src/main/java/com/noop/analytics/HrvAnalyzer.kt index c82530ff4a..1e9cdf6a8b 100644 --- a/android/app/src/main/java/com/noop/analytics/HrvAnalyzer.kt +++ b/android/app/src/main/java/com/noop/analytics/HrvAnalyzer.kt @@ -775,12 +775,18 @@ object HrvAnalyzer { /** Whole-percent, integer half-up, so both platforms round a tie the same way. 0 when [total] is 0. * * The arithmetic is widened to `Long` because [deliveryHistogram] passes BEAT-TIME IN MILLISECONDS - * summed over a whole night, not a row count. `part * 200` overflows a 32-bit `Int` above - * 2147483647 / 200 = 10,737,418 ms, i.e. 2.98 h of beat-time on multi-delivery seconds, and a real - * over-count night carries 3-4x that. Kotlin's `Int` wraps silently, so `multiMs` was reported - * NEGATIVE on every such night while the Swift twin (64-bit `Int`) reported the right value from the - * same source - the parity these two are supposed to hold. Widening the operation rather than the - * signature keeps every caller and the half-up tie behaviour untouched. */ + * summed over a whole night, not a row count. The numerator is `part * 200 + total`, so with + * `part <= total` it needs more than 32 bits from `total >= 10,683,999` ms - 2.97 h of beat-time on + * multi-delivery seconds, and a real over-count night carries 3-4x that. The multiply ALONE would + * survive to `part >= 10,737,419`; the `+ total` term is what brings the bound down, so quoting the + * multiply's limit understates the exposure. Kotlin's `Int` wraps silently while the Swift twin + * (64-bit `Int`) read correctly from the same source - the parity these two are supposed to hold. + * + * The wrap did NOT always land negative, which is the dangerous half: a 100% multi-share 8 h night + * (`pct(28_800_000, 28_800_000)`) returned 25 - positive, plausible, and wrong by 75 points. A + * negative `multiMs` was a symptom of this, never the test for it, so every pre-fix Android capture + * past the bound is suspect including the ones that read fine. Widening the operation rather than + * the signature keeps every caller and the half-up tie behaviour untouched. */ internal fun pct(part: Int, total: Int): Int = if (total > 0) ((part.toLong() * 200 + total) / (total.toLong() * 2)).toInt() else 0 diff --git a/android/app/src/test/java/com/noop/analytics/HrvAnalyzerSampleOrdTest.kt b/android/app/src/test/java/com/noop/analytics/HrvAnalyzerSampleOrdTest.kt index 6549aacbf1..1d2c9fb7ad 100644 --- a/android/app/src/test/java/com/noop/analytics/HrvAnalyzerSampleOrdTest.kt +++ b/android/app/src/test/java/com/noop/analytics/HrvAnalyzerSampleOrdTest.kt @@ -118,4 +118,19 @@ class HrvAnalyzerSampleOrdTest { // meanNN 928 ms). Also wrapped to -16, which is what the log reported for that night. assertEquals(91, HrvAnalyzer.pct(36_296_594, 39_886_368)) } + + /** + * The 32-bit wrap did not always produce a NEGATIVE percentage, so "is it negative" was never a + * sound test for an affected capture - which matters for anyone re-reading pre-fix Android logs. + * + * These pin the exact boundary and the readable-wrong case. The bound is `part * 200 + total` + * exceeding `Int.MAX_VALUE`, not `part * 200` alone: 10,683,998 was still computed correctly in 32 + * bits, 10,683,999 wrapped to -100, and a 100% multi-share 8 h night wrapped to 25 - positive, + * plausible, and wrong by 75 points. Twin of Swift `testPctWrapWasNotAlwaysNegative`. + */ + @Test fun pctWrapWasNotAlwaysNegative() { + assertEquals(100, HrvAnalyzer.pct(10_683_998, 10_683_998)) // last total 32 bits still got right + assertEquals(100, HrvAnalyzer.pct(10_683_999, 10_683_999)) // first that wrapped, to -100 + assertEquals(100, HrvAnalyzer.pct(28_800_000, 28_800_000)) // wrapped to 25, not to anything negative + } }