Skip to content

fix(bookmark): Run async on_restore callbacks before the session's effects - #2524

Draft
jat255 wants to merge 1 commit into
schloerke/async-otel-session-spanfrom
jat255/2508-bookmark-on-restore-order
Draft

jat255 wants to merge 1 commit into
schloerke/async-otel-session-spanfrom
jat255/2508-bookmark-on-restore-order

Conversation

@jat255

@jat255 jat255 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Async bookmark on_restore callbacks must finish before the session's other effects and outputs run. With the flush from #2508, those effects started early and saw state that was not restored yet. This PR makes them wait again.

Refs #2508, for its TODO "Ordering for async bookmark on_restore (priority 1000000)". Stacked on #2522.

Summary

On main, the flush awaited each effect in turn. The on_restore effect has priority 1000000, so its callbacks finished (awaits included) before any other effect started. With #2508, the flush starts each effect in priority order but does not wait for its async part. When an on_restore callback awaits, the session's other effects start first. They run once with default values, and then again after the restore. shinychat's enable_bookmarking() registers callbacks that await, so chat apps get this problem.

This PR adds a gate for each session:

  • The restore effect resolves a future when its callbacks finish, also when its run is cancelled (shiny/bookmark/_bookmark.py).
  • Each effect of the same session with a lower priority waits for this future before its body runs (shiny/reactive/_reactives.py). The effects still start in priority order. They keep the session busy, so no outputs go out before the restore is complete.
  • Session._restore_gate() connects the two, and SessionProxy sends module effects to the root session (shiny/session/_session.py).

Review notes

  • The gate is per session, so a slow restore does not delay other sessions. I did not make the flush wait for the restore effect, because the flush is global and that would block all sessions.
  • Effects with a priority of 1000000 or more do not wait. This matches main, where they ran before the restore effect.
  • The gate uses the priority at which the run was queued. set_priority() takes effect on the next invalidation, so a changed priority cannot skip the gate.
  • An alternative is to await the callbacks in the init handler before the first flush. I did not use it, because it blocks the session's receive loop during the restore.
  • R has no equivalent, because its onRestore() callbacks are synchronous.
  • There is no CHANGELOG entry. Compared with the released version, the behavior does not change.
  • A duck-typed session mock now needs _restore_gate(), in the same way that it needs _otel_reactive_update_span(). The mock in tests/pytest/test_poll.py has one now.

Testing

  • 10 new tests in tests/pytest/test_concurrency.py. They cover the first output and effect values, priority order, effects above priority 1000000, other sessions, a callback that raises, a cancelled callback, the session end during a restore, module effects, a waiting effect that is destroyed, and set_priority(). Without the fix, 9 fail. The other one guards the priority 1000000 exception. I also removed the SessionProxy delegation, the finally, and the asyncio.shield() one at a time, and each change made its test fail.
  • The full pytest suite and the Playwright bookmark tests (chromium) pass. check-format, check-lint, and pyright are clean. pyrefly reports 0 errors.

…fects

A flush starts effects in priority order but doesn't wait for their async
parts. An async `on_restore` callback that awaits (as shinychat's do) let
the session's other effects and outputs start with the state not yet
restored, so they ran once with default values.

While the callbacks run, the session's effects with a lower priority than
the restore effect now wait for them before their bodies run. The wait is
per session, so other sessions keep running.
@jat255 jat255 mentioned this pull request Oct 5, 2026
13 of 15 tasks
@jat255
jat255 added this pull request to stack #2516 October 5, 2026 13:21

This branch has not been deployed

No deployments
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