Skip to content

Conversation

@MellowYarker
Copy link
Contributor

@MellowYarker MellowYarker commented Nov 26, 2025

Prior to yielding to the event loop when we try to persist to SQLite, we save the current alarm time in alarmStateForCommit. When we come back after the operation completes, this value remains unchanged, but alarmScheduledNoLaterThan may have changed while we were async.

Prior to this commit, that meant we might schedule a move-later alarm synchronization with the alarm manager if alarmScheduledNoLaterThan was now earlier than alarmStateForCommit, even if alarmStateForCommit was set to the currently durable (and synced!) alarm time.

Consider the following:

  1. SQLite and the alarm manager have an alarm time of 2 years from now.
  2. The application does a large SQL insert, starting commit 1. We yield to the event loop in commitImpl when we try to persist the write to SQLite, but save alarmStateForCommit as the current alarm time (2 years from now)
  3. The application does a setAlarm() to run the alarm in 5 minutes. This is a "move-earlier" operation, so we sync to the alarm manager eagerly.
  4. The alarm manager now has an alarm time 5 minutes in the future, and we start trying to persist the setAlarm in 5 minutes to SQLite.
  5. The first commit from (2) continues after persisting its large SQL insert. It sees alarmStateForCommit is 2 years from now, and alarmScheduledNoLaterThan is 5 minutes from now. We decide to do a background "move-later" sync with the alarm manager, then finish the commit.
  6. The second commit from (3) finishes persisting the 5 minute alarm to SQLite and finishes its commit.
  7. The background "move-later" alarm from (5) modifies the alarm manager's state to have an alarm time of 2 years in the future.

The end state is we essentially miss the "move-earlier" update in the alarm manager, breaking our model of "move-early eagerly and move-later lazily".

The fix

We track alarmVersion, which is incremented every time the application calls setAlarm(). We capture this value before starting to persist to SQLite and compare it against the current alarmVersion after continuing commitImpl. If our captured alarmVersion differs from the current version, then the application called setAlarm() while our SQLite commit was in-flight, and so we can safely skip attempting a move-later sync with the alarm manager, since a subsequent commit will modify the alarm anyways.

@MellowYarker MellowYarker requested review from a team as code owners November 26, 2025 00:14
@MellowYarker MellowYarker force-pushed the milan/STOR-4521-repro branch 2 times, most recently from 825c1a8 to 362c2fc Compare November 26, 2025 22:15
Prior to yielding to the event loop when we try to persist to SQLite, we
save the current alarm time in `alarmStateForCommit`. When we come back
after the operation completes, this value remains unchanged, but
`alarmScheduledNoLaterThan` may have changed while we were async.

Prior to this commit, that meant we might schedule a move-later alarm
synchronization with the alarm manager if `alarmScheduledNoLaterThan`
was now earlier than `alarmStateForCommit`, even if
`alarmStateForCommit` was set to the currently durable (and synced!)
alarm time.

Consider the following:
1. SQLite and the alarm manager have an alarm time of 2 years from now.
2. The application does a large SQL insert, starting commit 1. We yield
   to the event loop in `commitImpl` when we try to persist the write to
   SQLite, but save `alarmStateForCommit` as the current alarm time
   (2 years from now)
3. The application does a setAlarm() to run the alarm in 5 minutes. This
   is a "move-earlier" operation, so we sync to the alarm manager
   eagerly.
4. The alarm manager now has an alarm time 5 minutes in the future, and
   we start trying to persist the setAlarm in 5 minutes to SQLite.
5. The first commit from (2) continues after persisting its large SQL
   insert. It sees `alarmStateForCommit` is 2 years from now, and
   `alarmScheduledNoLaterThan` is 5 minutes from now. We decide to do a
   background "move-later" sync with the alarm manager, then finish the
   commit.
6. The second commit from (3) finishes persisting the 5 minute alarm to
   SQLite and finishes its commit.
7. The background "move-later" alarm from (5) modifies the alarm
   manager's state to have an alarm time of 2 years in the future.

The end state is we essentially miss the "move-earlier" update in the
alarm manager, breaking our model of "move-early eagerly and
move-later lazily".

**The fix**

We track `alarmVersion`, which is incremented every time the application
calls setAlarm(). We capture this value before starting to persist to
SQLite and compare it against the current `alarmVersion` after continuing
commitImpl. If our captured `alarmVersion` differs from the current
version, then the application called setAlarm() while our SQLite commit
was in-flight, and so we can safely skip attempting a move-later sync
with the alarm manager, since a subsequent commit will modify the alarm
anyways.
@MellowYarker
Copy link
Contributor Author

Latest is a rebase. The MacOS build was hanging at the linking step, not sure why. Figured it would be worth kicking the build from the top.

@MellowYarker MellowYarker merged commit d45ce3a into main Nov 27, 2025
21 of 23 checks passed
@MellowYarker MellowYarker deleted the milan/STOR-4521-repro branch November 27, 2025 02:40
@MellowYarker MellowYarker changed the title Don't sync move-later alarm if alarm didn't change in the commit Don't schedule a move-later alarm if an interleaving commit setAlarm() Nov 27, 2025
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