Javier Bautista Fernández
29f7d54e85
Root-cause fix for the unstoppable-alarm incident (alarm rang 15 minutes, only uninstall silenced it) plus systematic hardening of every stop path. Native (Kotlin): - Verified stop: stopActiveAlarm now derives its result from the real post-teardown state (companion instance + synchronous stopEverything + activeRingingId check) instead of reporting unconditional success. - Atomic teardown: every stop path (stop action, notification button, snooze, missed, onDestroy, startForeground failure) funnels through one stopEverything() covering audio, wakelock, notification, foreground state and firing-record cleanup; player.release() guarded. - Bounded ringing: 10-minute auto-silence armed via AlarmManager fires a FIRED->MISSED transition with a localized missed-alarm notification; repeating alarms keep their native rearm, deleted alarms never produce ghost MISSED notifications. - Durable firing record with onStartCommand re-validation (resurrection guard) and boot-time stale cleanup; firing records cleared on every refuse/mismatch/cancel path. - New notification-only dismissal channel (dismissAlarmNotificationOnly) so UI-level dedup can never kill a live ring's audio. Flutter (Dart): - Stop/disable/edit/delete of a ringing alarm always attempt to silence it; on native-query failure the stop falls back toward silence via the id-scoped legacy stop. - Verified-stop results surface failures: the ringing screen keeps dismiss-by-design on success, but on a verified failure it stays up with a persistent force-stop banner (guarded against double-dismiss) and auto-dismisses if the ring ends externally (missed/notification). - Missed events sync alarm bookkeeping without opening the ringing UI. - 4 new l10n keys translated across all 13 locales (ARB guard green). 550 tests green, analyzer clean. Reviewed in 3 adversarial 4-lens rounds (2 deterministic + 1 refuter-corroborated critical fixed); formal gentle-ai receipt waived by maintainer authorization (correction scope legitimately exceeded the frozen genesis paths). On-device QA checklist in openspec/changes/alarm-system-overhaul/tasks.md pending before archive.
57 lines
14 KiB
Markdown
57 lines
14 KiB
Markdown
# alarm-system-overhaul — Codebase Deep-Dive (explore phase 1/2)
|
|
|
|
## 1. Inventory
|
|
|
|
### Dart (lib/)
|
|
- `estado/estado_alarmas.dart` — ChangeNotifier, canonical alarm state (`ConfiguracionAlarmas`). Key methods: `guardarAlarma` (L99, calls `android.programar`, NEVER calls `detenerSonidoNativo` even if the saved alarm is the one currently ringing), `eliminarAlarma` (L155, DOES call `android.detenerSonidoNativo` before `cancelar`), `cambiarActiva` (L164, delegates to `guardarAlarma` — same gap), `posponerAlarma` (L195), `posponerProximaDesdePreaviso` (L241), `finalizarEjecucion` (L272, calls `android.ocultarNotificacionAlarma` then `servicio.completarEjecucion`), `_alRecibirEventoNativo`/`_registrarCancelacionSnoozeNativa` (native-event sync), `_importarSnoozesNativosActivos` (cold-start snooze import).
|
|
- `servicios/servicio_alarmas_android.dart` — `ServicioAlarmasAndroid implements PuertoAlarmasAndroid`, wraps `MethodChannel('pluriwave/alarm_scheduler')`. `_logAndInvokeVoid` (L386) invokes the channel with NO try/catch — errors propagate to caller. Methods: `programar`, `cancelar`→`cancelAlarm`, `ocultarNotificacionAlarma`→`dismissAlarmNotification`, `detenerSonidoNativo`→`stopNativeAlarmSound`. `_instalarHandler` (L394) receives native `alarmFired` events.
|
|
- `servicios/servicio_alarmas.dart`, `servicio_programacion_alarmas.dart` — pure scheduling/next-occurrence math + persistence (`persistencia_tolerante.dart`).
|
|
- `pantallas/pantalla_alarma_sonando.dart` — ringing screen, audio-free (native owns audio). `_detener()` (L42) and `_posponer()` (L61) both: single-exit guard `_salidaEnCurso`, call into `EstadoAlarmas`, wrap in try/catch, **`finally { _dismissScreen() }` — the screen ALWAYS closes even if the native stop/snooze call throws** ("dismiss-by-design", intentional per comments, to avoid a stuck screen — but it also means a failed/no-op native stop is invisible to the user). `PopScope(canPop:false)` routes system back through the same `_detener()`.
|
|
- `app.dart` — `_alarmaSonandoActiva`/`_alarmaSonandoId` guard (L107-109) is now `finally`-protected (L388-392) — the historical stuck-modal/skipped-next-ring bug (single failure, two symptoms) is fixed. `_mostrarAlarmaSonando` (L354) correctly no-ops a duplicate delivery of the SAME ring and hides the notification only for a genuinely different concurrent alarm id (L371-373).
|
|
|
|
### Kotlin (android/app/src/main/kotlin/es/freetimelab/pluriwave/)
|
|
- `PluriWaveAlarmService.kt` — foreground service, SOLE audio owner (MediaPlayer on STREAM_ALARM/USAGE_ALARM). `onStartCommand` (L51) dispatches ACTION_STOP→`stopAlarm` (L58), ACTION_SNOOZE→native `AlarmScheduler.snooze` + `stopAlarm` (L61-84), FIRE/null→`startAlarm` (L85). `startAlarm` (L91) early-returns if `activeAlarmId != null` (single-ring-at-a-time). `startAudio`/`startStationAudio`/`startFallbackAudio` implement a 3-stage fallback chain (station → fallback station → bundled WAV) each with a 15s timeout (`scheduleStationFallback`) and a shared exponential dB fade loop (`startFadeLoop`/`computeFadeVolume`). **`stopAlarm` (L390) has an id-scoped guard: `if (alarmId != null && activeAlarmId != null && alarmId != activeAlarmId)` → does NOT stop audio/service, only cancels the notification for the mismatched id** — deliberate (protects the real ring from a second alarm's stop request) but is also the single point where an id mismatch would silently no-op a real stop. `buildNotification` (L436) posts the ONE fire notification (NOTIFICATION_ID=92841) with Snooze+Stop actions as `PendingIntent.getService` DIRECTLY to this service (bypasses Flutter entirely — robust even with a dead engine). WakeLock capped at 10 min (L509) — irrelevant to audio stoppability (CPU only).
|
|
- `PluriWaveAlarmReceiver.kt` — BroadcastReceiver for FIRE/PRE_NOTICE/SKIP_NEXT/POSTPONE_NEXT/SNOOZE_COUNTDOWN/SNOOZE_AGAIN/CANCEL_SNOOZE. `notificationIdForAlarm`/`fireNotificationIdForAlarm` (L274-275) are deterministic hash-based ids (53*hash+7 / 59*hash+9) — two DIFFERENT notification ids per alarm id (pre-notice/countdown vs. fire), so no id collision between the two channels.
|
|
- `AlarmScheduler.kt` — `scheduleAlarm`/`scheduleSpec` (trusts Dart's trigger when fresh, native recompute only as fallback — documented on-device divergence bug already fixed), `onAlarmFired`, `snooze`/`postponeNext`/`snoozeAgain` (anchor semantics documented), `cancelSnooze`, `cancelAlarm` (L659, does NOT touch the running service/audio — only cancels PendingIntents/notifications), `dismissFireNotification` (L676, notification-cancel only, no audio stop), `reschedulePersistedAlarms` (boot/unlock/TZ-change/package-replace/exact-alarm-permission-change).
|
|
- `MainActivity.kt` — MethodChannel `pluriwave/alarm_scheduler` handler (L89): `scheduleAlarm`, `cancelAlarm`, `dismissAlarmNotification` (L144, calls **both** `PluriWaveAlarmService.stop(this,id)` AND `alarmScheduler.dismissFireNotification(id)`), `stopNativeAlarmSound` (L155, calls only `PluriWaveAlarmService.stop`), `diagnostics`, permission requests, `getInitialAlarmIntent`/`getHandledAlarmOccurrences`/`getNativeSnoozeState`, `setNotificationStrings`. `notifyAlarmEvent` (companion, L1204) forwards native-originated events to Flutter ONLY if `activeInstance` (the live Activity) is non-null — dead-engine snoozes rely on cold-start `getNativeSnoozeState` sync instead.
|
|
- `PluriWaveBootReceiver.kt` — BOOT_COMPLETED/LOCKED_BOOT_COMPLETED/USER_UNLOCKED/MY_PACKAGE_REPLACED/TIME_SET/TIMEZONE_CHANGED/SCHEDULE_EXACT_ALARM_PERMISSION_STATE_CHANGED → `AlarmScheduler.reschedulePersistedAlarms()`.
|
|
- `AlarmNotificationStrings.kt`, `NotificationBrand.kt` — device-protected-storage-backed i18n strings pushed from Dart via `setNotificationStrings` (works even before first unlock, direct-boot-aware).
|
|
- No separate `android:process` — service/activity/receivers all share the app's default process; `MainActivity.activeInstance` reachability is NOT a cross-process concern.
|
|
|
|
### Channels / persistence
|
|
- MethodChannel `pluriwave/alarm_scheduler` (Dart↔Kotlin): scheduleAlarm, cancelAlarm, dismissAlarmNotification, stopNativeAlarmSound, diagnostics, requestExactAlarmPermission, requestPostNotificationsPermission, requestFullScreenIntentPermission, requestIgnoreBatteryOptimizations, getInitialAlarmIntent, getHandledAlarmOccurrences, getNativeSnoozeState, setNotificationStrings; reverse direction `alarmFired` (native→Dart).
|
|
- SharedPreferences (regular, per-alarm native spec store) + device-protected-storage prefs (`pluriwave_alarm_channels` migration flag, `AlarmNotificationStrings`).
|
|
- Dart side: `persistencia_tolerante.dart` for alarm config resilience against corruption.
|
|
|
|
## 2. Full lifecycle traces — key points
|
|
- Scheduling always prefers `setAlarmClock` (L235), falls back through `setExactAndAllowWhileIdle`→`setAndAllowWhileIdle`→`set` depending on SDK/exact-alarm permission (`scheduleMainAlarm`, AlarmScheduler.kt L228-278).
|
|
- Fire path: Receiver (ACTION_FIRE) → `AlarmScheduler.onAlarmFired` (reschedule bookkeeping) → `PluriWaveAlarmService.start` (posts FSI notification BEFORE audio prepares) → `startActivity(MainActivity)` (brings UI forward regardless of process state) → native audio 3-stage fallback with fade-in.
|
|
- Stop from notification: `PendingIntent.getService` → service `ACTION_STOP` directly (Flutter-independent, most robust path).
|
|
- Stop from in-app modal: MethodChannel → `stopNativeAlarmSound`/`dismissAlarmNotification` → `PluriWaveAlarmService.stop` (same code path as notification button) — but gated by Flutter engine being alive AND the call succeeding.
|
|
- Dead-app fire: Receiver creates process, starts service + activity; Flutter engine boots concurrently; `getInitialAlarmIntent`/cold-start sync reconciles state once engine is up.
|
|
|
|
## 3. FAILURE-MODE ANALYSIS (ranked by likelihood/evidence)
|
|
|
|
1. **[HIGHEST] Silent no-op on native id mismatch, masked by Dart's "dismiss-by-design".** `PluriWaveAlarmService.stopAlarm` (L400-409) silently no-ops the actual stop when `alarmId != activeAlarmId` (only cancels a notification). This call NEVER throws in that branch, so Dart's `_detener()`/`_posponer()` try/catch never fires and the ringing screen closes as if it worked (comment at pantalla_alarma_sonando.dart:38-41, and the "dismiss-by-design preserved" test explicitly locks in this behavior for the SNOOZE path only). If ANY id-derivation drift exists between what Dart passes and the service's `activeAlarmId` (e.g. after a snooze/reschedule mutates the spec, or during the documented "second alarm during ring" scenario), the user sees the screen close/app return to normal while the native `MediaPlayer` keeps playing — matching "not by opening the app" in the incident exactly (opening the app and tapping Stop APPEARED to work, screen closed, but audio never stopped).
|
|
2. **[HIGH] Toggling an alarm off (or editing/saving it) while it is the one currently ringing does not stop the native audio.** `EstadoAlarmas.guardarAlarma` (L99-116) → `android.programar` → (if now inactive) `cancelar` (native `cancelAlarm`, AlarmScheduler.kt L659) — cancels FUTURE schedules/notifications only, never calls `PluriWaveAlarmService.stop`. Only `eliminarAlarma` (full delete) calls `detenerSonidoNativo` first. A user who — after a failed/ambiguous Stop tap — panics and disables the alarm from the Alarms list will NOT stop the ringing audio, and will have destroyed the association between the alarm config and the still-ringing id, making a subsequent recovery attempt harder to reason about.
|
|
3. **[MEDIUM] Untested failure path for the exact defensive code that exists.** `FakePuertoAlarmasAndroid` (test/helpers/fakes_alarmas.dart) has a `fallaProgramar` failure switch for `programar` (used to test the snooze-failure SnackBar), but NO equivalent switch for `ocultarNotificacionAlarma`/`detenerSonidoNativo`. No test exercises `_detener()`'s catch/finally when the STOP call itself fails — the exact guard meant to catch this class of incident is unverified by CI.
|
|
4. **[MEDIUM] OEM background-execution restrictions / Doze / battery optimization.** `diagnostics` channel already surfaces `isIgnoringBatteryOptimizations` and requests exemption, but this is opt-in/dismissible by the user; on aggressive OEM skins (MIUI/EMUI/etc.) a `startService()` call from a notification action can be delayed or dropped even when the app already runs a foreground service — flagged as a plausible but unverifiable-from-code contributor.
|
|
5. **[LOWER] Reschedule/notification double-post races across concurrent alarms.** Code has explicit guards (`activeAlarmId != null` early-return in `startAlarm`, id-scoped `stopAlarm`) that appear to correctly prevent a second alarm's fire/stop from disturbing an active ring — analysis suggests this is already handled, kept as a residual risk only if the guard's assumptions (single Service instance, sequential onStartCommand dispatch) are violated by an OS-specific behavior.
|
|
|
|
### Other fragilities found (not part of the core incident but real gaps)
|
|
- `cancelAlarm`/`dismissFireNotification` (AlarmScheduler.kt) never stop an active ring — see #2.
|
|
- WakeLock hardcoded 10-minute cap (PluriWaveAlarmService.kt:509) — does not affect stoppability but could affect CPU scheduling on rings intentionally left running longer (fade-in test/edge cases).
|
|
- `alarm-clock-module` OpenSpec change is stuck at `status: planned / phase: tasks-ready` since 2026-05-21 despite the alarm feature clearly being implemented and iterated on since — stale/orphaned SDD tracking artifact, needs reconciliation.
|
|
- `app-quality-and-native-alarms` is `status: proposed / phase: apply-complete` (2026-06-12) and was NEVER moved to verify/archive — explicitly flagged in its own risk table ("`alarm-clock-module` state drift... out of scope to fix mid-flight"). Contains Slice 1 (native reliability: foreground-service type, dedup notifications, channel sound, fallback station, battery exemption, native fade-in) and Slice 2 (full snooze-path audit) — need to verify against CURRENT code which of these already landed (current code already shows FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK|SYSTEM_EXEMPTED, single FSI-owning service notification, fallback station support, native fade-in — so Slice 1 appears substantially implemented even though the artifact was never archived).
|
|
|
|
## 4. Test coverage map
|
|
- Covered (Dart): snooze failure SnackBar + dismiss-by-design (`pantalla_alarma_sonando_dismiss_guard_test.dart`), `_alarmaSonandoActiva` guard regressions, native snooze sync (`estado_alarmas_snooze_test.dart`), pre-notice/countdown templates, alarm cache/corruption/persistence tolerance.
|
|
- NOT covered (Dart): failure/no-op of `ocultarNotificacionAlarma`/`detenerSonidoNativo` from the Stop path (no fake failure switch exists); toggling/saving an alarm while it is the one actively ringing; concurrent-alarm id-mismatch stop scenario end-to-end.
|
|
- NOT covered (Kotlin): **zero** — no Kotlin test files exist in the repo (`android/**/*Test*.kt` glob returns nothing) and there is no Android build environment available in this session to add/run any. All native-service claims above (id-scoped stop guard, 3-stage audio fallback, fade loop, wakelock) are verified only by static code reading, never executed.
|
|
|
|
## 5. Known-debt from prior OpenSpec alarm changes
|
|
- `alarm-clock-module` (2026-05-21): stuck at tasks-ready, never applied/archived in SDD tracking — reconcile or supersede.
|
|
- `app-quality-and-native-alarms` (2026-06-11/12): apply-complete but never verified/archived; its own risk table flags the `alarm-clock-module` drift as deferred. Needs a fresh verify pass against current code to confirm which of its 7 slices actually landed.
|
|
- `alarm-live-countdown` (archived 2026-06-28, PASS WITH WARNINGS): pre-notice l10n + snooze dismiss guard — done, warnings were about Spanish-only button labels (deferred) and absent Kotlin test infra (still absent today).
|
|
- `snooze-reschedule-fix` (archived 2026-07-01, PASS WITH WARNINGS): posponerAlarma/posponerProximaDesdePreaviso error-handling parity — done; noted follow-ups: DI seams for PluriWaveApp testability (still not done — contributed to inability to widget-test app.dart's routing), dedicated l10n key for snooze failure (still reuses androidExactAlarmScheduleError).
|