Skip to content

feat: add allowSameSecondScheduling for same-second alarm coexistence#404

Merged
gdelataillade merged 5 commits into
gdelataillade:mainfrom
Fi2zz:feat/allow-same-second-scheduling
Jun 12, 2026
Merged

feat: add allowSameSecondScheduling for same-second alarm coexistence#404
gdelataillade merged 5 commits into
gdelataillade:mainfrom
Fi2zz:feat/allow-same-second-scheduling

Conversation

@Fi2zz

@Fi2zz Fi2zz commented Jun 10, 2026

Copy link
Copy Markdown
Contributor
  • Dart: add allowSameSecondScheduling to AlarmSettings
  • iOS: implement ringing queue with LIFO order, currentAlarmId protection
  • Android: implement ringing queue with LIFO order, per-id audio completion listener
  • Example: add toggles and demo buttons for both strategies, keep stopped alarms in list

- Dart: add allowSameSecondScheduling to AlarmSettings
- iOS: implement ringing queue with LIFO order, currentAlarmId protection
- Android: implement ringing queue with LIFO order, per-id audio completion listener
- Example: add toggles and demo buttons for both strategies, keep stopped alarms in list
@Fi2zz
Fi2zz force-pushed the feat/allow-same-second-scheduling branch from 873f65d to 93c38e7 Compare June 10, 2026 08:19
- Dart: add allowSameSecondScheduling to AlarmSettings
- iOS: implement ringing queue with LIFO order, currentAlarmId protection
- Android: implement ringing queue with LIFO order, per-id audio completion listener
- Example: add toggles and demo buttons for both strategies, keep stopped alarms in list

@gdelataillade gdelataillade left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Fi2zz, thanks a lot for the PR — I took a deep look. The wire/model plumbing is complete and correct on all three layers, and the iOS side genuinely addresses the concurrency concerns from #403: the currentAlarmId guard in AlarmRingManager.stop(id:) even fixes an existing race on main (stopping an overridden alarm used to kill the newer alarm's ring). The per-id audio completion listeners on Android are also exactly what I asked for.

That said, I found a few blocking issues — mostly on the Android side — that need to be addressed before this can merge.

Blocking issues

1. Android: the ringing queue dies on the most common stop path

The queue lives in AlarmService, but stopping a ringing alarm from Dart (Alarm.stop(id) — i.e. the stop button in the app's own ring screen) goes through AlarmApiImpl.stopAlarm(), which calls context.stopService(...). That destroys the whole service, and your onDestroy() clears ringingQueue / queuedAlarmSettings.

Concretely, in your own "3 seq" demo: alarm 1 rings, alarms 2 and 3 are queued. If the user stops alarm 1 from the app's ring screen (not the notification button), the service is destroyed and alarms 2 and 3 are silently dropped — and they stay in storage as ghost "scheduled" alarms that will never ring (their PendingIntents already fired), with no alarmStopped notification to Dart. Sequential ringing currently only works when alarms are stopped via the notification's Stop button (handleStopAlarmCommandstopAlarm, which has the dequeue logic).

Suggested fix: route AlarmApiImpl.stopAlarm through the service's stopAlarm (e.g. deliver the existing STOP_ALARM action to the running service instead of calling stopService), so the queue survives and promotion runs.

2. Android: a dismissed queued alarm can still ring

If the user stops a queued alarm from Dart (it's not in ringingAlarmIds, so the stopService branch is skipped), it gets unsaved from storage but remains in the service's queue. When the currently ringing alarm is stopped via the notification button, the deleted alarm is promoted and rings anyway.

You handled this correctly on iOS (guard self.alarms[nextId] != nil), but Android has no equivalent check. Suggested fix: validate against AlarmStorage before promoting, and/or remove the id from ringingQueue / queuedAlarmSettings when a queued alarm is stopped.

3. Sequential queueing is not gated behind the new flag

Both queue checks (Android AlarmService.onStartCommand and iOS AlarmManager.ringAlarm) only test allowAlarmOverlap — not allowSameSecondScheduling. This changes behavior for every existing user with default settings: today, an alarm that fires while another is ringing is dropped and Dart is notified it stopped; with this PR it's silently queued and rings whenever the first one stops — possibly much later (an 8:05 alarm firing mid-8:00-alarm now rings at 8:20 when the user finally stops the first one).

The issue explicitly stated "the current behavior should remain the default for backward compatibility", and the README addition documents sequential ringing as part of the new flag, which isn't what the code does. Please gate the queueing behind the flag — or if you think queue-when-busy deserves to be its own opt-in (it's arguably a separate feature from same-second scheduling), give it its own flag. Either way it must not change default behavior.

4. Unrelated regression in checkAlarm (lib/alarm.dart)

The PR removes the branch that re-emits an actively-ringing alarm to the ringing stream on app restart, replacing it with an unconditional stop(). On Android this path is load-bearing: with androidStopAlarmOnTermination: false, the foreground service keeps ringing after the app is killed; when the user reopens the app, main re-surfaces the ring UI — with this change, the alarm is silenced the moment the app launches. This change isn't mentioned in the PR description and isn't needed for the feature. Please revert it.

Minor

  • CI will fail: flutter analyze exits 1 on this branch — 4 info-level lints, all in the example app (line length ×2, redundant argument value, missing const).
  • LIFO ordering: with three queued alarms they ring in reverse arrival order on both platforms. FIFO would match user intuition better — or at least document the LIFO choice.
  • The example-app rework (alarmHistory keeping stopped alarms visible, Navigator.removeRoute) is fine for demoing, though a bit more opinionated than the feature needs.

The iOS half is close to merge-ready — the remaining work is concentrated on the Android stop paths and the gating. Happy to re-review once these are addressed!

- Android: route Alarm.stop through STOP_ALARM intent to service instead of stopService
- Android: remove stopped alarms from ringingQueue, validate storage before promotion
- Gate sequential ringing behind allowSameSecondScheduling on both platforms
- Revert checkAlarm regression: restore ringing stream re-emit on app restart
- Fix CI lints in example app
@Fi2zz
Fi2zz force-pushed the feat/allow-same-second-scheduling branch from 93c38e7 to 9e19fd8 Compare June 10, 2026 09:43

@gdelataillade gdelataillade left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround @Fi2zz — I re-reviewed 9e19fd8 and it's almost there. Verified resolved:

  • Gating: queueing now requires allowSameSecondScheduling == true on both platforms; with default settings the behavior is identical to main (drop + notify). The README is accurate now too.
  • Dismissed queued alarms: stopAlarm removes the id from the queue, and triggerNextQueuedAlarm() validates against AlarmStorage before promoting — mirrors the iOS guard nicely.
  • checkAlarm revert: exact restore of the ring-recovery path.
  • Lints: flutter analyze exits 0 on the branch now.

One remaining issue with the AlarmApiImpl.stopAlarm fix:

context.startService() is now called unconditionally — background crash risk

The queue does survive Dart-initiated stops now (👍), but startService(stopIntent) runs on every Alarm.stop(), including when no alarm is ringing and the service isn't running. If Alarm.stop() is called while the app is backgrounded with no foreground service active — e.g. canceling a scheduled alarm from an FCM/background isolate handler — startService throws IllegalStateException: Not allowed to start service Intent on API 26+. The old code never hit this because it only called stopService, and only when ringing.

Side effects of the same change: stopping a future alarm spins up the whole service just to deliver STOP_ALARM (×N for stopAll), and non-ringing stops send alarmStopped to Flutter twice (once from AlarmApiImpl, once from the service's unsaveAlarm) — harmless but noisy.

Suggested fix — do what AlarmReceiver already does for the notification stop button: call the running service directly and skip the intent entirely:

// Deliver the stop to the running service so it can clean up ringing
// alarms and dequeue if needed. If the service isn't running, there is
// nothing to ring/dequeue and storage cleanup below is sufficient.
AlarmService.instance?.handleStopAlarmCommand(id)

No background-start restriction, no service churn, and handleStopAlarmCommandunsaveAlarmstopAlarm runs the same dequeue/promotion logic you added.

That's the only blocker left — everything else looks merge-ready. 🙏

…rvice

- Replace context.startService(stopIntent) with AlarmService.instance?.handleStopAlarmCommand(id)
- Avoids IllegalStateException on API 26+ when stopping alarms from background
- Eliminates double alarmStopped notification to Flutter and unnecessary service churn
@Fi2zz

Fi2zz commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@gdelataillade Got it, thanks for the feedback. The issue has been fixed now.

…id on queue

- playAudio internally released the previous player via stopAudio, which
  since the per-id listener change also removed the completion listener
  AlarmService had just registered, so non-looping alarms never ran their
  end-of-audio cleanup (vibration/volume/audio focus). Split the player
  teardown into releaseMediaPlayer so playAudio keeps the listener.
- onStartCommand assigned alarmId before the queue decision, so a queued
  alarm overwrote the ringing alarm id that onTaskRemoved relies on for
  cleanup. alarmId is now only set in ringAlarm.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gdelataillade

Copy link
Copy Markdown
Owner

Heads-up @Fi2zz: a second review pass surfaced two more Android issues introduced by the queue/listener changes, both confirmed against the current head. Since they were small and this PR is otherwise ready, I pushed ddf2dcd directly to your branch (thanks for leaving maintainer edits enabled) rather than asking for another round:

  1. Non-looping alarms lost their completion cleanup. playAudio() internally calls stopAudio(id) to release any previous player for that id — and since the per-id listener change, stopAudio() also removes onAudioCompleteListeners[id]. Because AlarmService.ringAlarm registers the listener before calling playAudio, the listener was wiped immediately, so non-looping alarms never stopped vibration / restored volume / abandoned audio focus when the audio ended. Fixed by splitting the player teardown into a private releaseMediaPlayer(id) that playAudio uses internally, while the public stopAudio(id) keeps removing the listener. Also cleared the listener map in cleanUp() to match the other maps.

  2. Queued alarms overwrote the "current alarm" id used by onTaskRemoved. onStartCommand assigned alarmId = id before the queue decision, so if alarm A was ringing and B got queued, swiping the app away cleaned up B instead of A — wrong alarmStopped event, and the ringing alarm left as a ghost in storage. Since ringAlarm() already sets alarmId, the early assignment is simply removed.

Kotlin compiles cleanly and flutter analyze is unaffected. With these two on top of your fixes, I'm happy with the state of the PR.

@gdelataillade
gdelataillade merged commit 3536ba0 into gdelataillade:main Jun 12, 2026
2 checks passed
@gdelataillade

Copy link
Copy Markdown
Owner

Will be added in the next release!

@Fi2zz
Fi2zz deleted the feat/allow-same-second-scheduling branch June 23, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants