Skip to content

Commit 951df8e

Browse files
fryanpanclaude
andcommitted
ADFA-4128 (6/11): address CodeRabbit review
- F1718-2 stop advertising an E2eTimeline.parse that does not exist - F1718-6 stop the metrics helper swallowing fatals and cancellation - F1718-7 drop the dead telemetry.report import from LiveReloadOrchestratorTest - F1718-8 cover queueMillis in the HostSpans per-field test Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
1 parent 041bebc commit 951df8e

5 files changed

Lines changed: 61 additions & 2 deletions

File tree

‎quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/domain/telemetry/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@ Pure-JVM types for measuring the live-reload loop: one timeline per edit and one
44

55
| File | Purpose |
66
| --- | --- |
7-
| [`E2eTimeline.kt`](E2eTimeline.kt) | One generation's four-stamp timeline plus `StepTimings`, `HostSpans`, `BuildCounts`; derives stage deltas and the grep-stable log `format`/`parse`. |
7+
| [`E2eTimeline.kt`](E2eTimeline.kt) | One generation's four-stamp timeline plus `StepTimings`, `HostSpans`, `BuildCounts`; derives stage deltas and the grep-stable log `format`. Nothing here parses that line back - the benchmark harness's own Python parser does. |
88
| [`QuickBuildMetricsSink.kt`](QuickBuildMetricsSink.kt) | Interface for recording session/build/invalidation/reload/rebuild stats; must be cheap and never throw. Includes a `Noop` implementation. |

‎quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/service/telemetry/MetricsReporting.kt‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.appdevforall.cotg.quickbuild.service.telemetry
22

3+
import kotlinx.coroutines.CancellationException
34
import org.slf4j.Logger
45
import org.slf4j.LoggerFactory
56

@@ -13,12 +14,24 @@ internal val metricsLog: Logger = LoggerFactory.getLogger("QB-Metrics")
1314
* Every metrics call in this package goes through here rather than relying on each class
1415
* to remember its own try/catch.
1516
*
17+
* Two kinds of throwable are NOT swallowed - see the catch clauses for why.
18+
*
1619
* @param block the metrics call; must be side-effect-free beyond reporting, since a
1720
* partial run is swallowed and never retried
1821
*/
1922
internal inline fun report(block: () -> Unit) {
2023
try {
2124
block()
25+
} catch (e: CancellationException) {
26+
// This helper is inline, so a suspending call written inside the lambda compiles.
27+
// Swallowing its cancellation would run the caller's coroutine on past its own
28+
// cancellation, and nothing warns the author - there is no compile error to hit.
29+
throw e
30+
} catch (e: VirtualMachineError) {
31+
// The VM is out of a resource the rest of the build also needs. Logging it formats a
32+
// message and walks a stack trace, and the build then carries on reporting Success -
33+
// hiding the failure on exactly the low-spec devices this product exists for.
34+
throw e
2235
} catch (e: Throwable) {
2336
metricsLog.warn("Quick Build metrics sink failed", e)
2437
}

‎quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/domain/reload/LiveReloadOrchestratorTest.kt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import org.appdevforall.cotg.quickbuild.domain.ChangedFiles
1111
import org.appdevforall.cotg.quickbuild.domain.classify.BuildRoute
1212
import org.appdevforall.cotg.quickbuild.domain.classify.ChangeClassifier
1313
import org.appdevforall.cotg.quickbuild.domain.classify.InvalidationReason
14-
import org.appdevforall.cotg.quickbuild.service.telemetry.report
1514
import org.junit.jupiter.api.Test
1615
import java.io.File
1716

‎quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/domain/telemetry/E2eTimelineGroupsTest.kt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ class E2eTimelineGroupsTest {
5757
fun `each HostSpans field alone makes the group non-empty and counts toward the total`() {
5858
val singles =
5959
listOf(
60+
E2eTimeline.HostSpans(queueMillis = 7),
6061
E2eTimeline.HostSpans(scanMillis = 7),
6162
E2eTimeline.HostSpans(compileRpcMillis = 7),
6263
E2eTimeline.HostSpans(policyMillis = 7),
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package org.appdevforall.cotg.quickbuild.service.telemetry
2+
3+
import com.google.common.truth.Truth.assertThat
4+
import kotlinx.coroutines.CancellationException
5+
import org.junit.jupiter.api.Test
6+
import org.junit.jupiter.api.assertThrows
7+
8+
/**
9+
* What [report] may and may not swallow. A metrics sink must never break a build, but two
10+
* throwables are not the sink's failure to absorb: a [VirtualMachineError] means the whole
11+
* process is out of a resource, and a [CancellationException] belongs to the caller's
12+
* coroutine - [report] is `inline`, so a suspending call written inside the lambda compiles
13+
* with no warning that its cancellation would be eaten here.
14+
*/
15+
class MetricsReportingTest {
16+
@Test
17+
fun `an ordinary sink failure is swallowed`() {
18+
var ran = false
19+
20+
report {
21+
ran = true
22+
throw IllegalStateException("sink is down")
23+
}
24+
25+
// No throw: the build carries on, which is the whole point of the helper.
26+
assertThat(ran).isTrue()
27+
}
28+
29+
@Test
30+
fun `a VirtualMachineError is not swallowed`() {
31+
val fatal = OutOfMemoryError("no heap left for the metrics buffer")
32+
33+
val thrown = assertThrows<OutOfMemoryError> { report { throw fatal } }
34+
35+
assertThat(thrown).isSameInstanceAs(fatal)
36+
}
37+
38+
@Test
39+
fun `a CancellationException reaches the caller's coroutine`() {
40+
val cancelled = CancellationException("session torn down mid-report")
41+
42+
val thrown = assertThrows<CancellationException> { report { throw cancelled } }
43+
44+
assertThat(thrown).isSameInstanceAs(cancelled)
45+
}
46+
}

0 commit comments

Comments
 (0)