From 82b4b281bd446ae3154cd882609c53d04f245694 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 11 Oct 2026 17:12:31 +0000 Subject: [PATCH 1/2] fix(watcher): guard AlarmReceiver finish() with FinishOnce Applies the same OEM double-finish protection added to the widget broadcast in #360. AlarmReceiver used the bare `goAsync()` + `finally { pendingResult.finish() }` pattern; an OEM pre-finishing the broadcast before the coroutine's finally block throws `IllegalStateException("Broadcast already finished")`, which escapes the coroutine and kills the process. `FinishOnce` wraps the finish call with an `AtomicBoolean` guard and catches (logs) `IllegalStateException` on the second call, matching the behaviour already in `widget/AsyncBroadcast.kt`. No timing budget is added: heartbeat parsing has no widget rendering equivalent, so the existing coroutine lifetime is preserved. Adds three regression tests covering: - OEM double-finish (the crash scenario) - Normal single finish - Exception during heartbeat work still finishes the broadcast Closes the follow-up noted in #360. Git-Session-Id: 6d3fd149-5f73-4a7b-97c4-d50cc5ca7c5e --- .../android/watcher/AlarmReceiver.kt | 4 +- .../watcher/AlarmReceiverFinishOnceTest.kt | 102 ++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt diff --git a/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt b/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt index 7c0942da..80ee5893 100644 --- a/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt +++ b/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt @@ -8,6 +8,7 @@ import android.util.Log import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch +import net.activitywatch.android.widget.AsyncBroadcast private const val TAG = "AlarmReceiver" @@ -40,13 +41,14 @@ class AlarmReceiver : BroadcastReceiver() { Log.w(TAG, "Action ${intent.action}, running sendHeartbeats") if (UsageStatsWatcher.isUsageAllowed(context)) { val pendingResult = goAsync() + val finish = AsyncBroadcast.FinishOnce(pendingResult::finish) CoroutineScope(Dispatchers.IO).launch { try { usw.sendHeartbeatsSuspend() } catch (e: Exception) { Log.e(TAG, "Failed to send heartbeats from alarm", e) } finally { - pendingResult.finish() + finish.finish() } } } diff --git a/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt b/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt new file mode 100644 index 00000000..e472e630 --- /dev/null +++ b/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt @@ -0,0 +1,102 @@ +package net.activitywatch.android.watcher + +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.atomic.AtomicInteger +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import net.activitywatch.android.widget.AsyncBroadcast +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Regression tests for the AlarmReceiver goAsync + finish pattern after applying FinishOnce. + * + * Without FinishOnce, an OEM pre-finishing the broadcast before the coroutine's `finally` block + * causes `PendingResult.finish()` → `IllegalStateException("Broadcast already finished")`, which + * escapes the coroutine and kills the process. This is the same crash class fixed for the widget + * in ActivityWatch/aw-android#360. + */ +class AlarmReceiverFinishOnceTest { + + private val uncaught = CopyOnWriteArrayList() + private val logged = CopyOnWriteArrayList() + private val log: (String, Throwable?) -> Unit = { msg, _ -> logged.add(msg) } + + private fun scope() = CoroutineScope( + SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, e -> uncaught.add(e) } + ) + + /** + * OEM pre-finishes the broadcast before the coroutine's finally block; without FinishOnce + * this would throw IllegalStateException and escape the coroutine. + */ + @Test + fun oemDoubleFinish_doesNotEscapeCoroutine() = runBlocking { + val finishCalls = AtomicInteger() + val finish = AsyncBroadcast.FinishOnce( + finish = { + finishCalls.incrementAndGet() + throw IllegalStateException("Broadcast already finished") + }, + log = log, + ) + + val job = scope().launch { + try { + // heartbeat work (no-op in test) + } catch (e: Exception) { + // logged in production + } finally { + finish.finish() + } + } + job.join() + + assertEquals("finish() must be attempted exactly once", 1, finishCalls.get()) + assertTrue("IllegalStateException escaped the coroutine: $uncaught", uncaught.isEmpty()) + assertTrue("missing 'already finished' log entry", logged.any { it.contains("already finished") }) + } + + @Test + fun normalHeartbeat_finishesExactlyOnce() = runBlocking { + val finishCalls = AtomicInteger() + val finish = AsyncBroadcast.FinishOnce({ finishCalls.incrementAndGet() }, log) + + val job = scope().launch { + try { + // heartbeat work succeeds + } finally { + finish.finish() + } + } + job.join() + + assertEquals(1, finishCalls.get()) + assertTrue(uncaught.isEmpty()) + } + + @Test + fun heartbeatException_stillFinishes() = runBlocking { + val finishCalls = AtomicInteger() + val finish = AsyncBroadcast.FinishOnce({ finishCalls.incrementAndGet() }, log) + + val job = scope().launch { + try { + throw RuntimeException("server unreachable") + } catch (e: Exception) { + // logged + } finally { + finish.finish() + } + } + job.join() + + assertEquals(1, finishCalls.get()) + assertTrue(uncaught.isEmpty()) + } +} From 48dfd8bcf81f9a65f446e351f08a749a4efd2450 Mon Sep 17 00:00:00 2001 From: Bob Date: Sun, 11 Oct 2026 17:49:35 +0000 Subject: [PATCH 2/2] test(watcher): exercise production alarm completion path Git-Session-Id: 0a1fa3a7-bab6-5521-8eb2-20f4461d0318 --- .../android/watcher/AlarmReceiver.kt | 31 ++++-- .../watcher/AlarmReceiverFinishOnceTest.kt | 101 +++++++----------- 2 files changed, 61 insertions(+), 71 deletions(-) diff --git a/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt b/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt index 80ee5893..af92f54e 100644 --- a/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt +++ b/mobile/src/main/java/net/activitywatch/android/watcher/AlarmReceiver.kt @@ -7,11 +7,31 @@ import android.content.Intent import android.util.Log import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import net.activitywatch.android.widget.AsyncBroadcast private const val TAG = "AlarmReceiver" +/** The receiver's heartbeat/completion path, with its existing unbounded IO lifetime. */ +internal fun launchAlarmHeartbeats( + finish: () -> Unit, + scope: CoroutineScope = CoroutineScope(Dispatchers.IO), + log: (String, Throwable?) -> Unit = { message, error -> Log.e(TAG, message, error); Unit }, + work: suspend () -> Unit, +): Job { + val once = AsyncBroadcast.FinishOnce(finish, log) + return scope.launch { + try { + work() + } catch (e: Exception) { + log("Failed to send heartbeats from alarm", e) + } finally { + once.finish() + } + } +} + class AlarmReceiver : BroadcastReceiver() { override fun onReceive(context: Context, intent: Intent) { Log.w(TAG, "AlarmReceiver called") @@ -41,15 +61,8 @@ class AlarmReceiver : BroadcastReceiver() { Log.w(TAG, "Action ${intent.action}, running sendHeartbeats") if (UsageStatsWatcher.isUsageAllowed(context)) { val pendingResult = goAsync() - val finish = AsyncBroadcast.FinishOnce(pendingResult::finish) - CoroutineScope(Dispatchers.IO).launch { - try { - usw.sendHeartbeatsSuspend() - } catch (e: Exception) { - Log.e(TAG, "Failed to send heartbeats from alarm", e) - } finally { - finish.finish() - } + launchAlarmHeartbeats(finish = pendingResult::finish) { + usw.sendHeartbeatsSuspend() } } } else { diff --git a/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt b/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt index e472e630..072f13c3 100644 --- a/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt +++ b/mobile/src/test/java/net/activitywatch/android/watcher/AlarmReceiverFinishOnceTest.kt @@ -6,97 +6,74 @@ import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking -import net.activitywatch.android.widget.AsyncBroadcast import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame import org.junit.Assert.assertTrue import org.junit.Test -/** - * Regression tests for the AlarmReceiver goAsync + finish pattern after applying FinishOnce. - * - * Without FinishOnce, an OEM pre-finishing the broadcast before the coroutine's `finally` block - * causes `PendingResult.finish()` → `IllegalStateException("Broadcast already finished")`, which - * escapes the coroutine and kills the process. This is the same crash class fixed for the widget - * in ActivityWatch/aw-android#360. - */ +/** Tests the completion path called by AlarmReceiver, not a copy of its coroutine. */ class AlarmReceiverFinishOnceTest { - private val uncaught = CopyOnWriteArrayList() - private val logged = CopyOnWriteArrayList() - private val log: (String, Throwable?) -> Unit = { msg, _ -> logged.add(msg) } + private val logged = CopyOnWriteArrayList>() + private val log: (String, Throwable?) -> Unit = { msg, error -> logged.add(msg to error) } private fun scope() = CoroutineScope( SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, e -> uncaught.add(e) } ) - /** - * OEM pre-finishes the broadcast before the coroutine's finally block; without FinishOnce - * this would throw IllegalStateException and escape the coroutine. - */ + /** Simulates the framework finishing first, so the receiver's finish attempt throws. */ @Test fun oemDoubleFinish_doesNotEscapeCoroutine() = runBlocking { val finishCalls = AtomicInteger() - val finish = AsyncBroadcast.FinishOnce( - finish = { - finishCalls.incrementAndGet() - throw IllegalStateException("Broadcast already finished") - }, - log = log, - ) - - val job = scope().launch { - try { - // heartbeat work (no-op in test) - } catch (e: Exception) { - // logged in production - } finally { - finish.finish() - } + val alreadyFinished = java.util.concurrent.atomic.AtomicBoolean(false) + val pendingFinish = { + finishCalls.incrementAndGet() + check(alreadyFinished.compareAndSet(false, true)) { "Broadcast already finished" } } - job.join() + pendingFinish() // Framework/OEM finishes the broadcast before our coroutine completes. - assertEquals("finish() must be attempted exactly once", 1, finishCalls.get()) + launchAlarmHeartbeats( + finish = pendingFinish, + scope = scope(), + log = log, + work = {}, + ).join() + + assertEquals("one framework finish and one receiver attempt", 2, finishCalls.get()) assertTrue("IllegalStateException escaped the coroutine: $uncaught", uncaught.isEmpty()) - assertTrue("missing 'already finished' log entry", logged.any { it.contains("already finished") }) + assertTrue(logged.any { it.first.contains("already finished") && it.second is IllegalStateException }) } @Test - fun normalHeartbeat_finishesExactlyOnce() = runBlocking { - val finishCalls = AtomicInteger() - val finish = AsyncBroadcast.FinishOnce({ finishCalls.incrementAndGet() }, log) - - val job = scope().launch { - try { - // heartbeat work succeeds - } finally { - finish.finish() - } - } - job.join() + fun normalHeartbeat_finishesExactlyOnceAfterWork() = runBlocking { + val events = CopyOnWriteArrayList() + launchAlarmHeartbeats( + finish = { events.add("finish"); Unit }, + scope = scope(), + log = log, + work = { events.add("heartbeat") }, + ).join() - assertEquals(1, finishCalls.get()) + assertEquals(listOf("heartbeat", "finish"), events.toList()) assertTrue(uncaught.isEmpty()) + assertTrue(logged.isEmpty()) } @Test - fun heartbeatException_stillFinishes() = runBlocking { + fun heartbeatException_stillFinishesAndLogsFailure() = runBlocking { val finishCalls = AtomicInteger() - val finish = AsyncBroadcast.FinishOnce({ finishCalls.incrementAndGet() }, log) - - val job = scope().launch { - try { - throw RuntimeException("server unreachable") - } catch (e: Exception) { - // logged - } finally { - finish.finish() - } - } - job.join() + val failure = RuntimeException("server unreachable") + launchAlarmHeartbeats( + finish = { finishCalls.incrementAndGet(); Unit }, + scope = scope(), + log = log, + work = { throw failure }, + ).join() assertEquals(1, finishCalls.get()) assertTrue(uncaught.isEmpty()) + assertEquals(1, logged.size) + assertSame(failure, logged.single().second) } }