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 + } }