fix: clear repeatable read transaction before running access tasks#24400
Merged
Conversation
Clear the session-scoped transaction before running pending access tasks during session unlock. This ensures that effects triggered by access tasks see fresh signal values instead of stale cached values from the repeatable read transaction. Previously, the transaction was cleared after running access tasks and pushing. This caused effects to read from the cached transaction state, missing concurrent updates that happened during the UIDL request. Fixes #24399 https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
|
|
StringNode doesn't have a "text" property - use textValue() directly. https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
Legioth
previously requested changes
May 21, 2026
Member
Legioth
left a comment
Legioth
left a comment
There was a problem hiding this comment.
Shouldn't we instead isolate access tasks from each other so that each task can have its own repeatable read transaction? Otherwise, one tasks might poison the repeatable read transaction for a subsequent task.
Instead of clearing the transaction once before all access tasks, clear it before EACH task so that each task gets its own fresh repeatable read transaction. This prevents one task from poisoning the cached values for subsequent tasks. Each access task now: - Starts with a cleared session-scoped transaction - Gets a fresh transaction created on-demand when it first reads a signal - Has repeatable reads within itself - Is isolated from other tasks' cached values https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
Each access task now runs in its own isolated write-through transaction that uses ROOT as the outer, ensuring tasks see fresh signal values and don't share cached values with each other or with the session-scoped transaction. This addresses review feedback to properly isolate access tasks rather than just clearing the session-scoped transaction before each task. https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
…ch task The isolated transaction wrapper broke effect tracking and CurrentInstance handling. Reverting to the simpler approach of clearing the session-scoped transaction before each access task, which allows fresh signal reads without interfering with other mechanisms. https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
Legioth
dismissed
their stale review
New approach seems to make sense but I don't have time to review the test right now. Removing my review so that someone else feels like they could have a look.
Legioth
requested changes
May 22, 2026
…tasks Addresses review feedback: - Remove test from TransactionTest that didn't use the pending access queue - Add test in VaadinServiceTest that verifies clearSessionScopedTransaction is called before each pending access task runs https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
Member
|
I tested a locally built snapshot in my actual application and seems like it fixes the original issue that I observed. |
Legioth
requested changes
May 22, 2026
Addresses Legioth's review feedback: the prior test only verified clearSessionScopedTransaction was invoked, not that the fix actually solves the original issue. Replace it with a test that reproduces the issue #24399 scenario end-to-end: - Effect on a UI reads a shared signal during initial run, priming the session-scoped repeatable-read transaction. - Signal is mutated outside the session transaction (via runWithoutTransaction), simulating a concurrent write from another session or a background message handler. - Effect dispatcher routes the re-run through ui.access(), which enqueues it in the pending access queue while the session lock is held. - When pending access tasks run (simulating session unlock at the end of the UIDL request), the effect must observe the fresh value. Without the fix, the effect re-run sees the cached stale value ("initial"). With the fix, it sees the fresh value ("external").
|
vaadin-bot
added a commit
that referenced
this pull request
May 22, 2026
…24400) (CP: 25.1) (#24426) This PR cherry-picks changes from the original PR #24400 to branch 25.1. --- #### Original PR description > ## Summary > > - Clear the session-scoped transaction **before** running pending access tasks during session unlock > - This ensures effects triggered by access tasks see fresh signal values instead of stale cached values from the repeatable read transaction > - Added a unit test to verify that clearing the transaction fallback allows fresh reads > > ## Details > > When a shared signal is updated concurrently with an ongoing UIDL request, effects associated with that UI would run with the repeatable read transaction used for the request. If the transaction cached the old value, effects would miss the update. > > The fix moves `sessionScopedTransaction = null` to execute before `runPendingAccessTasks()` instead of after, ensuring access tasks read fresh values. > > ## Test plan > > - [ ] Existing unit tests pass > - [ ] New `clearingFallback_allowsFreshReads` test verifies the fix behavior > - [ ] Manual test with the reproduction case from the issue > > Fixes #24399 > > Slack thread: https://vaadin.slack.com/archives/C6RAXJATF/p1779368249043829?thread_ts=1779368185.339629&cid=C6RAXJATF > > https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR > > --- > _Generated by [Claude Code](https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR)_ Co-authored-by: Artur Signell <artur@vaadin.com> Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
When a shared signal is updated concurrently with an ongoing UIDL request, effects associated with that UI would run with the repeatable read transaction used for the request. If the transaction cached the old value, effects would miss the update.
The fix moves
sessionScopedTransaction = nullto execute beforerunPendingAccessTasks()instead of after, ensuring access tasks read fresh values.Test plan
clearingFallback_allowsFreshReadstest verifies the fix behaviorFixes #24399
Slack thread: https://vaadin.slack.com/archives/C6RAXJATF/p1779368249043829?thread_ts=1779368185.339629&cid=C6RAXJATF
https://claude.ai/code/session_01QW3F8K4uxGyaUNa9xwoKTR
Generated by Claude Code