Skip to content

Don't let an in-flight poll undo a cursor reset - #2220

Draft
mpretty-cyro wants to merge 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/cursor-reset-race
Draft

mpretty-cyro wants to merge 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/cursor-reset-race

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

The bug

Some events reset a swarm's poll cursors so the next poll fetches the swarm's history again. If a poll of that swarm is already in flight, it finishes after the reset and writes its newest hash back as the cursor. That undoes the reset, and the history is never fetched. The window is one poll round trip. It happens in practice because a promotion arrives through the user's own swarm while the group's poller is running.

Reset sites

Every reset goes through LokiAPIDatabase, so the guard covers all of them:

Site Reset
GroupManagerV2Impl.handlePromotion the group's cursors, so a new admin fetches history
GroupManagerV2Impl.handleKicked the group's cursors, so a re-invite downloads history again
ConfigToDatabaseSync.deleteGroupData the group's cursors when the group is removed
Poller multipart-config migration the config namespaces of every swarm
CreateAccountManager / LoadAccountManager every cursor

The guard

LastMessageHashResets counts resets, per swarm and for all swarms.

  • Poll start. Each poll takes a snapshot of the count before it reads any cursor. GroupPoller and Poller both take it at the top of the poll.
  • Cursor write. setLastMessageHashValue now requires that snapshot. It skips the write when the swarm has been reset since then.
  • Ordering. The reset and the guarded write both run under one lock, together with their SQL. A reset can't land between the check and the write, and one that arrives during a write runs after it and clears what it wrote.
  • Namespace-wide clears. The migration's clear isn't scoped to one swarm, so it counts as a reset of every swarm.

The guard counts resets instead of comparing cursor values. A cursor that was already empty reads the same before and after a reset, so a value comparison would let the poll write its hash back.

The in-flight poll's messages are still handled. Only its cursor write is dropped. The next poll starts from the beginning, and dedupe absorbs the messages it fetches twice.

Testing

GroupPollerCursorResetTest drives the real GroupPoller.manualPollOnce() and holds the group messages retrieve open while the reset happens. Cases:

  • a reset while the poll is in flight;
  • a cursor that was already empty;
  • a reset that arrives during the cursor write;
  • control: with no reset, the poll writes its cursor as before.

To make that possible, GroupPoller gets a small seam, namespaces, which defaults to libsession's values. JVM tests can't load the native library those values come from.

The cursor store in the test is a fake that uses the production LastMessageHashResets the same way LokiAPIDatabase does. The SQL wiring in LokiAPIDatabase itself isn't under test, because it needs SQLCipher. Poller's two writes use the same guard, but no test drives them.

Mutations, each killed by the test:

  • guard removed
  • a value comparison in place of the count
  • the check made outside the write's lock
  • the snapshot taken at write time instead of poll start

A swarm's cursors are reset so its history is fetched again: on promotion to
admin, on being kicked, when a group's data is deleted, by the multipart
config migration, and on account create or load. A poll of that swarm already
in flight finished afterwards and wrote its newest hash back, undoing the
reset, so the history was never fetched.

Resets are now counted per swarm in LokiAPIDatabase, and a poll takes the
count before reading any cursor. setLastMessageHashValue takes that snapshot
and skips the write if the swarm was reset since, with the check and the write
under the same lock as the reset. Counting, rather than comparing cursor
values, also catches a cursor that was already empty.

GroupPoller's namespace values become replaceable so a JVM test can drive a
real poll; they default to libsession's.
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.

1 participant