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.
107 lines
11 KiB
Markdown
107 lines
11 KiB
Markdown
# Tasks: Alarm System Overhaul — Fail-Safe Stop/Dismiss
|
||
|
||
## Review Workload Forecast
|
||
|
||
| Field | Value |
|
||
|-------|-------|
|
||
| Estimated changed lines (Slice A, code+tests) | ~650–800 |
|
||
| Estimated changed lines (Slice B, P1 follow-up) | ~80–120 |
|
||
| 400-line budget risk | High (well above the 400-line reviewer-cognition guard) |
|
||
| 800-line session budget risk | Medium (near the ceiling; any test-file growth pushes over) |
|
||
| Chained PRs recommended | No (session delivery is direct commits to `main`, no PRs) |
|
||
| Suggested split | Slice A = this delivery (Phases 1–7); Slice B = P1 follow-up (Phase 8) |
|
||
| Delivery strategy | single-pr-default (direct commits to main, no PRs) |
|
||
| Chain strategy | size-exception |
|
||
|
||
Decision needed before apply: Yes
|
||
Chained PRs recommended: No
|
||
Chain strategy: size-exception
|
||
400-line budget risk: High
|
||
|
||
Rationale: no-PR direct-commit strategy forecloses PR chaining, but Slice A crosses the 400-line
|
||
guard on its own. Maintainer must explicitly accept a `size:exception` for Slice A before `sdd-apply`
|
||
proceeds; Slice B (P1) ships as a separate, smaller follow-up commit set once Slice A lands.
|
||
|
||
### Suggested Work Units (sequenced commit batches, not PRs)
|
||
|
||
| Unit | Goal | Focused test command | Runtime harness | Rollback boundary |
|
||
|------|------|-----------------------|------------------|--------------------|
|
||
| A1 — Native stop core (Ph.1–3) | id-agnostic stop, atomic teardown, notification route fix, channel bridge | N/A (no Kotlin build env; grep-verifiable) | Device QA checklist below | Revert `PluriWaveAlarmService.kt`/`MainActivity.kt` hunks; additive-only |
|
||
| A2 — Native auto-silence + durable record (Ph.2) | MISSED transition, firing record, boot cleanup | N/A (grep-verifiable) | Device QA: 10-min + reboot scenarios | Revert `AlarmScheduler.kt`/`PluriWaveAlarmReceiver.kt`; orphaned pref keys unread |
|
||
| A3 — Dart orchestration (Ph.4–6) | mutation guard, force-stop UI, missed bookkeeping | `flutter test test/estado/estado_alarmas_test.dart test/servicios/servicio_alarmas_android_test.dart test/pantallas/pantalla_alarma_sonando_test.dart` | N/A (unit/widget-covered) | Revert Dart hunks; guard is additive to existing stop calls |
|
||
| A4 — l10n (Ph.7) | 4 new keys, en+es only | `flutter test` + CI ARB guard | N/A | Revert 2 ARB files |
|
||
| B1 — P1 fallback banners (Phase 8, follow-up) | FSI/exact-alarm/notification-permission warnings | `flutter test test/estado` | Device QA: FSI-denied scenario | Revert diagnostics-banner hunk only |
|
||
|
||
## Phase 1: Native Stop Core (Kotlin, static-edit)
|
||
|
||
- [x] 1.1 `PluriWaveAlarmService.kt`: add companion `@Volatile var activeRingingId: String?`, set in `startAlarm`, cleared in `stopEverything`. [kotlin-static] → Decision 1. Accept: grep shows `@Volatile` field + both set/clear sites.
|
||
- [x] 1.2 Add `ACTION_STOP_ACTIVE` const; extract `stopEverything()` from the L410-433 teardown (runnables, player stop/release, `activeAlarmId=null`, companion clear, wakelock release, audio-focus abandon, notification cancel, `clearFiringRecord`+`cancelAutoSilence`, `stopForeground(STOP_FOREGROUND_REMOVE)`, `stopSelf()`). [kotlin-static] → NA-2, Decision 2. Accept: grep shows all 3 teardown calls in `stopEverything()` and every stop path (`ACTION_STOP`, `ACTION_STOP_ACTIVE`, `ACTION_SNOOZE`, `ACTION_MISSED`, `onDestroy`) calls it.
|
||
- [x] 1.3 `onStartCommand` `ACTION_STOP_ACTIVE` branch → `stopEverything()` id-agnostically; `stopAlarm(id)` keeps id-scoped wrapper (foreign-id mismatch cancels only that id's notification). [kotlin-static] → NA-1a, NA-1b, NA-1c, Decision 1. Accept: branch present, ignores extras id.
|
||
- [x] 1.4 **(feedback item 1)** Fix `stopPendingIntent(alarmId)`: change dispatched `action` from `ACTION_STOP` to `ACTION_STOP_ACTIVE` so the notification Stop button routes to the id-agnostic stop. [kotlin-static] → SS-4a, NA-1a. Accept: grep confirms `action = ACTION_STOP_ACTIVE` inside `stopPendingIntent`.
|
||
|
||
## Phase 2: Native Auto-Silence + Durable Firing Record (Kotlin, static-edit)
|
||
|
||
- [x] 2.1 `AlarmScheduler.kt`: add `AUTO_SILENCE_MILLIS` (10 min), `recordFiring(id)`/`clearFiringRecord(id)`/`firingRecordAgeMillis(id)` on `KEY_FIRING_IDS`+`firing_<id>` in `pluriwave_alarm_scheduler` prefs. [kotlin-static] → NA-4a, Decision 4.
|
||
- [x] 2.2 `armAutoSilence(id)`/`cancelAutoSilence(id)` via `AlarmManager.setExactAndAllowWhileIdle` → `ACTION_MISSED`; arm+record in `onAlarmFired`; cancel in `snooze`, `skipNext`, `cancelAlarm`, `stopEverything`. [kotlin-static] → NA-3a, Decision 3. Accept: grep shows 1 arm site + 4 cancel sites.
|
||
- [x] 2.3 `onAlarmMissed(id)`: `stopEverything()` if id still rings, post missed notification (reuse pre-notice channel + `AlarmNotificationStrings.missedTitle/missedText`), clear firing record, **no rearm**; one-shot stays disabled. [kotlin-static] → NA-3a, NA-3b, NA-3c. Accept: grep shows no rearm/`programarSiguiente` call inside `onAlarmMissed`.
|
||
- [x] 2.4 `PluriWaveAlarmReceiver.kt`: `ACTION_MISSED` const + branch → `onAlarmMissed`; `pendingMissedIntent` helper. [kotlin-static] → NA-3a.
|
||
- [x] 2.5 `startAlarm`: if `firingRecordAgeMillis(id) > AUTO_SILENCE_MILLIS` → abort + `onAlarmMissed` cleanup; else `recordFiring(id)` before `MediaPlayer.start()`; keep `START_NOT_STICKY`. [kotlin-static] → NA-4a, NA-4b. Accept: grep shows `recordFiring` textually precedes `MediaPlayer.start()` and the stale-check branch exists.
|
||
- [x] 2.6 `cleanupStaleFiringRecords()`; call from `reschedulePersistedAlarms` before rescheduling. [kotlin-static] → NA-5a. Accept: grep shows call precedes the reschedule loop.
|
||
- [x] 2.7 `AlarmNotificationStrings.kt` `missedTitle`/`missedText` getters+setters; `MainActivity.kt setNotificationStrings` passes them through. [kotlin-static] → NA-3a.
|
||
|
||
## Phase 3: Channel Bridge (Kotlin, static-edit)
|
||
|
||
- [x] 3.1 `MainActivity.kt`: add `getActiveRingingAlarmId` (sync companion read) and `stopActiveAlarm` (`{stopped, wasRinging, activeAlarmId}`; internal exception → `result.error("STOP_FAILED", msg, null)`). [kotlin-static] → Decision 1, NA-1a, NA-1b. Accept: grep shows both method names in the channel dispatcher + the `error()` call.
|
||
|
||
## Phase 4: Dart Orchestration — Stop Result + Mutation Guard (TDD)
|
||
|
||
- [x] 4.1 RED — `test/servicios/servicio_alarmas_android_test.dart`: failing test for `ResultadoDetencion{detenido,estabaSonando,alarmaId}` mapping from `stopActiveAlarm`/`alarmaSonandoId()`. [dart-testable] → Decision 5 interface. `flutter test` fails.
|
||
- [x] 4.2 GREEN — `servicio_alarmas_android.dart`: add `ResultadoDetencion`, `alarmaSonandoId()`, `detenerSonidoActivo()`, `PuertoAlarmasAndroid` additions. `flutter test` green.
|
||
- [x] 4.3 RED — `test/helpers/fakes_alarmas.dart` + `estado_alarmas_test.dart`: add `fallaDetener`/`alarmaSonandoIdValor`/`detencionesActivas`; failing tests for SS-1a/SS-1b/SS-1c/SS-1d (`cambiarActiva(false)`/`guardarAlarma`/`eliminarAlarma` stop only when target id == ringing id). [dart-testable]
|
||
- [x] 4.4 GREEN — `estado_alarmas.dart`: add `_detenerSiEstaSonando(id)` guard; wire into `guardarAlarma`, `cambiarActiva(false)`, `eliminarAlarma`. `flutter test` green.
|
||
- [x] 4.5 RED — failing tests for SS-2a/SS-2b: `finalizarEjecucion` calls `detenerSonidoActivo()` directly; confirmed → no `_error`; unconfirmed/failure → `_error` set. [dart-testable]
|
||
- [x] 4.6 GREEN — `estado_alarmas.dart`: wire `finalizarEjecucion` accordingly. `flutter test` green.
|
||
|
||
## Phase 5: Dart UX — Force-Stop Affordance (TDD)
|
||
|
||
- [x] 5.1 RED — failing test for SS-3b: `forzarDetencion()` re-invokes `detenerSonidoActivo()`; success clears failure state, failure keeps it. [dart-testable]
|
||
- [x] 5.2 GREEN — `estado_alarmas.dart`: add `forzarDetencion()`. `flutter test` green.
|
||
- [x] 5.3 RED — `test/pantallas/pantalla_alarma_sonando_test.dart`: failing widget test for SS-3a/SS-3c (failure → screen stays up with a persistent `alarmForceStopAction` banner; confirmed → no banner). [dart-testable]
|
||
- [x] 5.4 GREEN — `pantalla_alarma_sonando.dart`: on `_detener()` failure, stay on screen and render a persistent in-screen banner (not a timed SnackBar) with a force-stop action → `forzarDetencion()`; clears only on a confirmed stop. `flutter test` green.
|
||
- [x] 5.5 **(review round 3 correction)** `_forzarDetencion()` now respects `_salidaEnCurso` (guard reset on failure) and the ringing screen reconciles an external MISSED transition for its own alarm id (`EstadoAlarmas.ultimaAlarmaPerdidaId`) by auto-dismissing. [dart-testable]
|
||
|
||
## Phase 6: Dart — Missed-Event Bookkeeping (TDD)
|
||
|
||
- [x] 6.1 RED — failing test: native `missed` event → `completarEjecucion` records it. [dart-testable]
|
||
- [x] 6.2 GREEN — `estado_alarmas.dart`: wire missed event in `_alRecibirEventoNativo`. `flutter test` green.
|
||
|
||
## Phase 7: l10n (corrected policy — en+es only)
|
||
|
||
- [x] 7.1 `lib/l10n/app_en.arb`: add `alarmStopFailedMessage`, `alarmForceStopAction`, `alarmMissedNotificationTitle`, `alarmMissedNotificationText({name})` with `@`-metadata. [dart-testable] Accept: `flutter test` (Phase 5 widget test) resolves the new keys.
|
||
- [x] 7.2 `lib/l10n/app_es.arb`: mirror the 4 keys, placeholder metadata byte-exact with `en`. [dart-testable]
|
||
- [x] 7.3 **(review round 2 — superseded correction)** The original "do NOT touch the other 11 locales" policy above was itself superseded by the round-2 review fix: the 4 keys were deliberately ADDED, with real translations, to all 13 `lib/l10n/app_*.arb` files instead of falling back to `en`. Accept: CI ARB placeholder-corruption guard passes across all 13 files.
|
||
- [x] 7.4 Wire missed-alarm strings through setNotificationStrings (gap found in apply verification)
|
||
- [x] 7.5 **(review round 3 correction)** Native KDoc (`AlarmScheduler.AUTO_SILENCE_MILLIS`, `AlarmNotificationStrings.missedText`) updated to reference all 13 ARB files instead of only `en`/`es`, matching 7.3.
|
||
|
||
## Phase 8 — Slice B (P1 follow-up, not part of this delivery)
|
||
|
||
- [ ] 8.1 Diagnostics banners bound to `EstadoAlarmas.diagnostico.canUseFullScreenIntent/notificationsEnabled/canScheduleExactAlarms`; new warning strings (en+es only, same policy as Phase 7). [dart-testable] → NA-6b, NA-6c.
|
||
- [ ] 8.2 RED→GREEN Dart tests for exact-alarm-denied and `POST_NOTIFICATIONS`-denied warnings. [dart-testable] → NA-6b, NA-6c.
|
||
- [ ] 8.3 Confirm FSI auto-fallback needs no code change (platform degrades `setFullScreenIntent` when `canUseFullScreenIntent()` is false); log as device-QA-only. [kotlin-static]+[device-qa] → NA-6a.
|
||
|
||
## On-Device QA Checklist (collected — run on real hardware)
|
||
|
||
- [ ] Stop from ringing UI, id match AND id mismatch (NA-1a, NA-1c)
|
||
- [ ] Stop from lock-screen notification (SS-4a, NA-1a — after Phase 1.4 fix)
|
||
- [ ] Disable/edit/delete the ringing alarm from the list (SS-1a/b/c)
|
||
- [ ] Leave a fired alarm untouched 10 minutes → audio stops, missed notification posts, repeating rearms, one-shot stays disabled (NA-3a/b/c)
|
||
- [ ] Kill the app mid-ring → audio stops
|
||
- [ ] Reboot mid-ring → boot cleanup runs, no audio resurrection (NA-5a)
|
||
- [ ] Trigger a concurrent second alarm while one rings (NA-1c)
|
||
- [ ] Force FSI-denied state → heads-up fallback posts instead of full-screen intent (NA-6a)
|
||
|
||
## Open Questions Carried to Apply
|
||
|
||
- [ ] Missed-notification channel: reuse pre-notice channel (default assumed above) vs. dedicated low-importance channel — confirm before Phase 2.3 lands.
|
||
- [ ] `finalizarEjecucion` return contract: `Future<bool>` vs. existing `_error`-field convention (default assumed above: `_error`) — confirm before Phase 4.6 lands.
|