diff --git a/lib/main.dart b/lib/main.dart index bda6676..a073073 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -55,6 +55,21 @@ Future main() async { // permanently hidden (`hayCarpetaConfigurada()` has no fuente to ask). registrarFuenteMusicaLocal(FuenteMusicaLocalAutoImpl(prefs: prefs)); + // Silent-error channel (fix/notificacion-media): `AudioService.asyncError` + // had ZERO subscribers app-wide, and a `PublishSubject` with no listeners + // drops what it is given — so every exception `audio_service` catches + // internally was discarded without a trace, which is exactly why the + // "media notification disappeared" report came with no evidence attached. + // Subscribed BEFORE `AudioService.init` below (the getter only touches a + // static subject, so it needs no initialisation) so nothing reported + // during the MediaBrowser handshake is missed, and placed here rather than + // in `conectarHandler` so ONE subscription covers both the on-time and the + // degraded/timeout startup paths. + final subErroresAudio = observarErroresAudio( + AudioService.asyncError, + registrar: registrarErrorAudioService, + ); + // Design "Timeout without re-init": AudioService.init is started exactly // ONCE here and `handlerFuturo` is the only future ever awaited for it — // the plugin caches state internally, so a double-configure call is @@ -69,6 +84,11 @@ Future main() async { // degraded/late-completion paths below. void conectarHandler(PluriWaveAudioHandler handler) { registrarHandler(handler); + // The handler is the only thing this app ever tears down + // (`onTaskRemoved`), so the asyncError subscription's `cancel` travels + // with it and can never leak — same "register from main.dart" convention + // as `registrarHandler` itself. + registrarLimpiezaArranque(subErroresAudio.cancel); final sesionAudio = ServicioAudioSession(objetivo: handler); unawaited(sesionAudio.configurar()); } diff --git a/lib/servicios/arranque_audio.dart b/lib/servicios/arranque_audio.dart index 6ed7fc3..23ac4be 100644 --- a/lib/servicios/arranque_audio.dart +++ b/lib/servicios/arranque_audio.dart @@ -1,4 +1,5 @@ import 'dart:async'; +import 'dart:developer' as developer; import 'package:flutter/material.dart'; @@ -58,6 +59,53 @@ Future> esperarArranqueAudio( } } +/// Subscribes to [errores] — in production `AudioService.asyncError` — and +/// hands every event to [registrar]. Returns the [StreamSubscription] so the +/// caller can cancel it when the handler is torn down. +/// +/// Why this exists: `audio_service` funnels EVERY asynchronous failure of its +/// own observers into that stream and nothing else +/// (`_observePlaybackState`/`_observeMediaItem`/`_observeQueue` each wrap +/// their whole body in `catch (e) { _asyncError.add(e); }`, and the artwork +/// path uses `.catchError(_asyncError.add)`), yet this app had ZERO +/// subscribers on it. A `PublishSubject` with no listeners simply drops +/// events, so the platform-side exception behind "the media notification +/// disappeared" — a rejected `setState`, a failed `setMediaItem`, an +/// Android 12+ `ForegroundServiceStartNotAllowedException` surfacing through +/// the plugin — was being discarded without a single log line. This makes +/// that channel audible. +/// +/// [errores] and [registrar] are both injected — this function never touches +/// the real `audio_service` plugin, so it is testable with a plain +/// [StreamController] (same seam convention as [esperarArranqueAudio] above, +/// and as `decidirAvanceCola`/`debeReaplicarEcualizador` elsewhere). +StreamSubscription observarErroresAudio( + Stream errores, { + required void Function(Object error) registrar, +}) { + return errores.listen( + registrar, + // The plugin only ever feeds this subject through `add`, never + // `addError`, so this branch is purely defensive: a stream-level error + // would otherwise escape as an unhandled zone error, which is strictly + // worse than one more log line. + onError: (Object error, StackTrace _) => registrar(error), + cancelOnError: false, + ); +} + +/// Default [observarErroresAudio] logger: one `[PluriWave]`-prefixed +/// `developer.log` line per swallowed plugin exception, at the same +/// `level: 900` (SEVERE) that `servicio_audio.dart`'s existing error lines +/// use, so a single logcat/DevTools filter catches both. +void registrarErrorAudioService(Object error) { + developer.log( + '[PluriWave] AudioService.asyncError: $error', + name: 'ArranqueAudio', + level: 900, + ); +} + /// Minimal branded bootstrap widget for the degraded path (Design "still /// call runApp, but with a minimal bootstrap widget that keeps waiting on /// the SAME original future"). Shows [_CargandoArranqueAudio] while diff --git a/lib/servicios/servicio_audio.dart b/lib/servicios/servicio_audio.dart index e9ef72b..1c11424 100644 --- a/lib/servicios/servicio_audio.dart +++ b/lib/servicios/servicio_audio.dart @@ -60,6 +60,23 @@ void registrarFuenteMusicaLocal(FuenteMusicaLocalAuto fuente) { _fuenteMusicaLocalGlobal = fuente; } +/// Teardown hook for whatever `main.dart` wired around the handler and must +/// be undone when the handler itself dies — today only the +/// `AudioService.asyncError` subscription (`observarErroresAudio`). Registered +/// from `main.dart`, mirroring [registrarHandler] and the two browse-source +/// registrations above; run exactly once from +/// [PluriWaveAudioHandler.onTaskRemoved]. +/// +/// The direction of the dependency matters: the bootstrap layer injects its +/// cleanup INTO the service layer, so `servicio_audio.dart` never has to +/// import `arranque_audio.dart` (nor the plugin's static error stream) just to +/// be able to close it. +Future Function()? _limpiezaArranqueGlobal; + +void registrarLimpiezaArranque(Future Function() limpieza) { + _limpiezaArranqueGlobal = limpieza; +} + /// Builds the phone-initiated "play a station" `MediaItem` (item 3, Android /// Auto fallback artwork): reuses [artUriPara] (`navegacion_auto.dart`) so a /// station with no usable favicon gets the SAME on-brand rotating fallback @@ -103,6 +120,57 @@ Emisora emisoraDesdeMediaItem(MediaItem mediaItem) { ); } +/// Maps a `just_audio` [ProcessingState] to the `audio_service` +/// [AudioProcessingState] pushed into `playbackState`. Identical to the +/// previous private `_mapProcState` in every case EXCEPT one: +/// [ProcessingState.idle] maps to [AudioProcessingState.loading] while +/// [cambiandoFuente] is `true`. +/// +/// Why that single exception exists — this is the media-notification +/// regression, not a cosmetic tweak: +/// +/// `audio_service`'s `_observePlaybackState` (`audio_service.dart:1131-1136`) +/// calls `AudioService._stop()` — which reaches `stopService()` and cancels +/// the notification through `deactivateMediaSession()` — on ANY transition +/// into `idle` from a non-idle state. The notification is posted at exactly +/// one place, `internalStartForeground()`, reachable only from the +/// `!wasPlaying && playing` edge, and its FIRST statement is +/// `ContextCompat.startForegroundService(...)`, which throws +/// `ForegroundServiceStartNotAllowedException` on API 31+ whenever the +/// process is not in a foreground state. +/// +/// Every station change walked straight into that: `_cambiarFuente` pushes +/// `loading`, then `_recrearPlayer` disposes the old [AudioPlayer] and builds +/// a FRESH one, and a fresh player's first `playerStateStream` event is +/// always `idle`. Forwarded verbatim, that is a `loading -> idle` transition, +/// so the foreground service was torn down mid-source-change and the app then +/// depended on the following `playing: true` edge to restart it. With the +/// screen off, on the lock screen, or on an Android Auto / Bluetooth-initiated +/// start, that restart is exactly the case the platform refuses — audio keeps +/// playing, the notification never comes back. Self-inflicted, on every API +/// level, no plugin patch needed: just stop emitting the transient `idle`. +/// +/// A genuine user stop is unaffected: `stop()` clears the flag BEFORE +/// `_player.stop()`, so its `idle` still reaches `playbackState` as a real +/// `idle` and still tears the service down. Pure — no handler dependency — so +/// the full [ProcessingState] x [cambiandoFuente] matrix is unit-testable +/// directly. +AudioProcessingState mapearEstadoProceso( + ProcessingState proc, { + required bool cambiandoFuente, +}) { + if (cambiandoFuente && proc == ProcessingState.idle) { + return AudioProcessingState.loading; + } + return switch (proc) { + ProcessingState.idle => AudioProcessingState.idle, + ProcessingState.loading => AudioProcessingState.loading, + ProcessingState.buffering => AudioProcessingState.buffering, + ProcessingState.ready => AudioProcessingState.ready, + ProcessingState.completed => AudioProcessingState.completed, + }; +} + /// Custom-action names for the equalizer's `PlaybackStateCompat` custom /// actions on the now-playing screen (Design "EQ custom actions", item 4). /// Public consts so tests and this file's own `customAction` dispatch share @@ -399,6 +467,24 @@ class PluriWaveAudioHandler extends BaseAudioHandler Future _colaCambioFuente = Future.value(); int _revisionFuente = 0; + /// `true` only for the window inside [_cambiarFuente] where the OLD player + /// has been disposed and the FRESH one has not loaded its URL yet — the + /// window in which `playerStateStream` unavoidably emits a transient + /// `idle` that is NOT a stop. [mapearEstadoProceso] masks that one `idle` + /// as `loading` so `audio_service` does not tear the foreground service + /// (and with it the media notification) down mid-source-change; see that + /// function's doc for the full mechanism. + /// + /// A value stuck at `true` is the ONLY risk this flag introduces: a real + /// user stop would then be masked away from `idle` and the service would + /// never stop, leaving an unkillable notification. It is therefore cleared + /// by a `finally` in [_cambiarFuente] (which covers normal completion, + /// both revision-mismatch `return`s, every `rethrow`, and any non-`Exception` + /// `Error` that no catch clause matches), AND eagerly at the top of every + /// catch clause, AND at the start of [stop] and [_gestionarErrorReproduccion] + /// — i.e. before every single `_player.stop()` call in this class. + bool _cambiandoFuente = false; + /// Active local-music queue (Design "single load-bearing invariant"): /// `null` means "not local-queue playback" — the ONLY gate the /// auto-advance/skip/isolation logic reads. Radio never sets this field. @@ -505,7 +591,10 @@ class PluriWaveAudioHandler extends BaseAudioHandler if (colaActiva) MediaAction.skipToNext, }, androidCompactActionIndices: [colaActiva ? 1 : 0], - processingState: _mapProcState(proc), + processingState: mapearEstadoProceso( + proc, + cambiandoFuente: _cambiandoFuente, + ), playing: playing, bufferedPosition: _player.bufferedPosition, speed: _player.speed, @@ -591,6 +680,11 @@ class PluriWaveAudioHandler extends BaseAudioHandler /// only retry exhaustion (or non-network errors) falls through to the /// existing error path, so the user sees a single error — no spam per retry. void _gestionarErrorReproduccion(Object error) { + // Terminal-error path also ends in `_player.stop()` below, and it is + // reachable from `_eventosSub`'s `onError` WHILE a source change is still + // in flight. Dropping the mask here keeps the invariant total: the flag + // is `false` before every `_player.stop()` call in this class. + _cambiandoFuente = false; if (_intentarReconexion(error)) return; String mensaje; @@ -723,16 +817,6 @@ class PluriWaveAudioHandler extends BaseAudioHandler return _textos.audioErrorCannotPlay; } - AudioProcessingState _mapProcState(ProcessingState state) { - return switch (state) { - ProcessingState.idle => AudioProcessingState.idle, - ProcessingState.loading => AudioProcessingState.loading, - ProcessingState.buffering => AudioProcessingState.buffering, - ProcessingState.ready => AudioProcessingState.ready, - ProcessingState.completed => AudioProcessingState.completed, - }; - } - /// Public entry point for EVERY external play (phone `reproducir`, car /// `emisora:`/`grupo:`/`pista:` non-path). ALWAYS clears the /// local queue FIRST (Design ADR-2, the single load-bearing invariant: @@ -872,16 +956,31 @@ class PluriWaveAudioHandler extends BaseAudioHandler errorMessage: null, ), ); + // Opens the masking window BEFORE `_recrearPlayer`, which is what + // disposes the old player and constructs the fresh one whose first + // `playerStateStream` event is the transient `idle` we must not forward + // (see [mapearEstadoProceso]). + _cambiandoFuente = true; try { await _recrearPlayer(); if (revision != _revisionFuente) return; await _player.setUrl(mediaItem.id).timeout(_timeoutCambioFuente); + // Source swap complete: the fresh player's transient `idle` is behind + // us, so stop masking immediately — before anything below can await — + // and let a real `idle` through again from here on. + _cambiandoFuente = false; if (revision != _revisionFuente) return; _iniciarPlaySinBloquear(mediaItem, revision); unawaited(_activarEcualizador()); } on PlayerException catch (e) { + // Cleared BEFORE `_gestionarErrorReproduccion`, not just by the + // `finally`: that method calls `_player.stop()` without awaiting it, so + // the resulting `idle` could otherwise land while the mask was still + // up and be rewritten to `loading`. Same reason in the two clauses + // below. + _cambiandoFuente = false; if (revision == _revisionFuente) { _gestionarErrorReproduccion(e); // Reconnect engaged: complete normally so callers do not surface a @@ -890,6 +989,7 @@ class PluriWaveAudioHandler extends BaseAudioHandler } throw Exception(_mensajeAmigable(e)); } on TimeoutException catch (e) { + _cambiandoFuente = false; // A real network drop usually surfaces as our 12s source timeout: // route it through the reconnect machine instead of a terminal error. if (revision == _revisionFuente) { @@ -898,6 +998,7 @@ class PluriWaveAudioHandler extends BaseAudioHandler } rethrow; } on Exception catch (e, stackTrace) { + _cambiandoFuente = false; developer.log( '[PluriWave] Error inesperado en playMediaItem: $e', name: 'ServicioAudio', @@ -916,6 +1017,15 @@ class PluriWaveAudioHandler extends BaseAudioHandler this.mediaItem.add(null); } rethrow; + } finally { + // Leak-proof backstop. Dart runs `finally` on EVERY exit from the + // block above: normal completion, both `revision != _revisionFuente` + // early returns, every `throw`/`rethrow` out of a catch clause, and — + // crucially — any `Error` (as opposed to `Exception`) that none of the + // three clauses catches. The flag must never depend on a single + // hand-audited exit path, because a `_cambiandoFuente` stuck at `true` + // would mask a REAL stop's `idle` and leave the notification unkillable. + _cambiandoFuente = false; } } @@ -1130,8 +1240,39 @@ class PluriWaveAudioHandler extends BaseAudioHandler // logic above — `onTaskRemoved` (which calls stop()) inherits this for // free (Phase 3 task 3.5). _desactivarCola(); + // Genuine user stop: drop the source-change mask BEFORE `_player.stop()`, + // so its `idle` reaches `playbackState` as a REAL `idle` and + // `audio_service` tears the foreground service down as it always has. + // `stop()` never pushes `idle` itself — `BaseAudioHandler.stop()` is + // empty and the teardown is driven entirely by the player's emission — + // so a stop landing while a station change is still in flight would + // otherwise be masked to `loading` and the notification would become + // unkillable. Paired with `_revisionFuente++` below, which invalidates + // that in-flight change; its `finally` only re-clears the flag, and + // nothing re-arms it (the single `= true` assignment already ran). + _cambiandoFuente = false; _revisionFuente++; await _player.stop(); + // Publish `idle` OURSELVES rather than trusting the player to emit it. + // `just_audio`'s `playerStateStream` is `.distinct()` over a value-equal + // `PlayerState`, so a stop landing on an already-idle player (a station + // change stopped before its native init finished pushing `loading`) + // emits NOTHING. Combined with the source-change mask above — which + // WRITES `loading` into `playbackState` rather than filtering at read + // time — that would leave the state stuck at `loading` forever: + // `audio_service` only tears the service down on a non-idle -> idle + // transition (`audio_service.dart:1131-1136`), so the notification would + // survive as an unkillable "cargando" with a dead Stop button. Strictly + // worse than the bug this branch fixes. Additive and idempotent: when + // the player DOES emit its own `idle`, this simply lands first and the + // duplicate is a no-op transition. + playbackState.add( + playbackState.value.copyWith( + processingState: AudioProcessingState.idle, + playing: false, + errorMessage: null, + ), + ); emisoraActual = null; mediaItem.add(null); await super.stop(); @@ -1201,6 +1342,13 @@ class PluriWaveAudioHandler extends BaseAudioHandler await _androidAudioSessionIdSub?.cancel(); await _player.dispose(); await _androidAudioSessionIdController.close(); + // Handler teardown: release the bootstrap-owned `AudioService.asyncError` + // subscription too, so it cannot outlive the handler it was instrumenting. + // Never throws out of teardown — a failing cleanup hook must not prevent + // the rest of `onTaskRemoved` from having completed above. + try { + await _limpiezaArranqueGlobal?.call(); + } catch (_) {} } Emisora _emisoraDesdeMediaItem(MediaItem mediaItem) { diff --git a/test/servicios/arranque_audio_test.dart b/test/servicios/arranque_audio_test.dart index 288e8a8..c67aa83 100644 --- a/test/servicios/arranque_audio_test.dart +++ b/test/servicios/arranque_audio_test.dart @@ -63,4 +63,93 @@ void main() { expect(handler, 'handler-tardio'); }); }); + + /// fix/notificacion-media — commit 1: `AudioService.asyncError` had zero + /// subscribers, so every exception `audio_service` swallows internally was + /// dropped on the floor. These cover the injectable seam only (Design + /// "Testability" — the stream and the logger are both injected), never the + /// real plugin. + group('observarErroresAudio', () { + test('reenvia al logger cada error emitido, en orden', () async { + final controlador = StreamController.broadcast(); + final registrados = []; + + final sub = observarErroresAudio( + controlador.stream, + registrar: registrados.add, + ); + + controlador.add('fallo-1'); + controlador.add(StateError('fallo-2')); + await controlador.close(); + + expect(registrados, hasLength(2)); + expect(registrados.first, 'fallo-1'); + expect(registrados.last, isA()); + + await sub.cancel(); + }); + + test('cancelar la suscripcion corta el logging — no puede filtrarse ' + 'tras el teardown del handler', () async { + final controlador = StreamController.broadcast(); + final registrados = []; + + final sub = observarErroresAudio( + controlador.stream, + registrar: registrados.add, + ); + + controlador.add('antes-del-cancel'); + // Deja que el evento se entregue antes de cancelar (los broadcast + // controllers entregan en un microtask, no de forma sincrona). + await Future.delayed(Duration.zero); + await sub.cancel(); + + controlador.add('despues-del-cancel'); + await controlador.close(); + + expect( + registrados, + ['antes-del-cancel'], + reason: + 'tras cancelar, la suscripcion no debe seguir viva ni registrar ' + 'nada mas', + ); + }); + + test( + 'un evento de error del propio stream tambien llega al logger', + () async { + final controlador = StreamController.broadcast(); + final registrados = []; + + final sub = observarErroresAudio( + controlador.stream, + registrar: registrados.add, + ); + + // Rama defensiva: el plugin solo usa `add`, nunca `addError`, pero un + // error de stream sin manejar seria una excepcion no capturada. + controlador.addError(const FormatException('stream roto')); + await controlador.close(); + + expect(registrados, hasLength(1)); + expect(registrados.single, isA()); + + await sub.cancel(); + }, + ); + + test('el logger por defecto acepta cualquier objeto sin lanzar', () { + expect( + () => registrarErrorAudioService(StateError('cualquier cosa')), + returnsNormally, + ); + expect( + () => registrarErrorAudioService('un string suelto'), + returnsNormally, + ); + }); + }); } diff --git a/test/servicios/servicio_audio_mapeo_estado_test.dart b/test/servicios/servicio_audio_mapeo_estado_test.dart new file mode 100644 index 0000000..7084a82 --- /dev/null +++ b/test/servicios/servicio_audio_mapeo_estado_test.dart @@ -0,0 +1,99 @@ +import 'package:audio_service/audio_service.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:just_audio/just_audio.dart'; +import 'package:pluriwave/servicios/servicio_audio.dart'; + +/// fix/notificacion-media — commit 2: exhaustive matrix for +/// [mapearEstadoProceso], the pure seam extracted out of the handler's +/// private `_mapProcState`. +/// +/// The handler itself cannot be instantiated in a unit test (MethodChannels), +/// which is exactly why the mapping decision was lifted out: the one line +/// that decides whether `audio_service` tears the foreground service — and +/// with it the media notification — down is now testable on its own. +void main() { + /// The pre-change `_mapProcState` mapping, transcribed literally. Every + /// `cambiandoFuente: false` expectation below is checked against THIS, so a + /// future edit to the production switch that alters any non-masked case + /// fails here instead of silently changing playback semantics. + const mapeoHeredado = { + ProcessingState.idle: AudioProcessingState.idle, + ProcessingState.loading: AudioProcessingState.loading, + ProcessingState.buffering: AudioProcessingState.buffering, + ProcessingState.ready: AudioProcessingState.ready, + ProcessingState.completed: AudioProcessingState.completed, + }; + + group('mapearEstadoProceso', () { + test('la tabla heredada cubre TODOS los ProcessingState — si just_audio ' + 'anade uno nuevo, este test cae antes que la matriz', () { + expect(mapeoHeredado.keys, containsAll(ProcessingState.values)); + expect(ProcessingState.values, hasLength(mapeoHeredado.length)); + }); + + group('cambiandoFuente: false (sin cambio de fuente en vuelo)', () { + for (final proc in ProcessingState.values) { + test('$proc mapea a ${mapeoHeredado[proc]}, igual que antes', () { + expect( + mapearEstadoProceso(proc, cambiandoFuente: false), + mapeoHeredado[proc], + ); + }); + } + }); + + group('cambiandoFuente: true (cambio de emisora en vuelo)', () { + for (final proc in ProcessingState.values) { + final esperado = + proc == ProcessingState.idle + ? AudioProcessingState.loading + : mapeoHeredado[proc]; + test('$proc mapea a $esperado', () { + expect(mapearEstadoProceso(proc, cambiandoFuente: true), esperado); + }); + } + }); + + // ── Las dos direcciones, explicitas ────────────────────────────────── + test('un stop REAL sigue produciendo idle: `stop()` limpia la bandera ' + 'antes de `_player.stop()`, asi que audio_service puede seguir ' + 'cerrando el foreground service', () { + expect( + mapearEstadoProceso(ProcessingState.idle, cambiandoFuente: false), + AudioProcessingState.idle, + ); + }); + + test('el idle transitorio del player recien creado produce loading: la ' + 'notificacion no se cancela a mitad de un cambio de emisora', () { + expect( + mapearEstadoProceso(ProcessingState.idle, cambiandoFuente: true), + AudioProcessingState.loading, + ); + }); + + test('la mascara NO toca ningun estado distinto de idle', () { + for (final proc in ProcessingState.values) { + if (proc == ProcessingState.idle) continue; + expect( + mapearEstadoProceso(proc, cambiandoFuente: true), + mapearEstadoProceso(proc, cambiandoFuente: false), + reason: '$proc debe mapear igual con y sin cambio de fuente en vuelo', + ); + } + }); + + test('idle es el UNICO caso en el que las dos ramas difieren', () { + final distintos = + ProcessingState.values + .where( + (proc) => + mapearEstadoProceso(proc, cambiandoFuente: true) != + mapearEstadoProceso(proc, cambiandoFuente: false), + ) + .toList(); + + expect(distintos, [ProcessingState.idle]); + }); + }); +}