Skip to content

Increment alarmVersion more selectively#5639

Merged
MellowYarker merged 2 commits intomainfrom
milan/STOR-4521-more-fixes
Dec 4, 2025
Merged

Increment alarmVersion more selectively#5639
MellowYarker merged 2 commits intomainfrom
milan/STOR-4521-more-fixes

Conversation

@MellowYarker
Copy link
Copy Markdown
Contributor

@MellowYarker MellowYarker commented Dec 3, 2025

I tested this in our internal code base and verified that without this fix, move-later alarms do get dropped before they sync to the alarm manager if we setAlarm(sameTime) repeatedly. That's not ideal, but it's fine because we self-correct when we armAlarmHandler(). Anyways, should be fixed now.

MellowYarker and others added 2 commits December 3, 2025 16:51
We missed a couple of places in the previous alarms fix commit, like
deleteAll. Furthermore, we probably only want to be incrementing the
alarmVersion if setAlarm() actually provided a new value, since calling
setAlarm(T) when T is already the set alarm is a no-op. This could have
posed a problem, namely, if I move an alarm later from T to T + 10, and
I call setAlarm(T + 10) multiple times, then each time that would
increment the alarmVersion, but only the first call would trigger a
commitImpl. The first commit would determine that its
alarmVersionBeforeAsync was less than the current alarmVersion, and
would assume a subsequent commit would handle any alarms, but there
wouldn't be any more commits so we would just fail to schedule the alarm
with the alarm manager.

Luckily, this would only affect move-later alarms, which don't have to
sync with the alarm manager immediately.
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@MellowYarker MellowYarker requested review from a team as code owners December 3, 2025 22:17
@MellowYarker MellowYarker merged commit 8069747 into main Dec 4, 2025
21 checks passed
@MellowYarker MellowYarker deleted the milan/STOR-4521-more-fixes branch December 4, 2025 13:06
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