Settle the store back to loaded when a reconnect window receives only a stale-slot value#1740
Open
mcintyre94 wants to merge 1 commit into
Open
Conversation
…ly a stale-slot value `createReactiveStoreWithInitialValueAndSlotTracking` keeps `lastUpdateSlot` across `connect()` windows so the surfaced value never regresses to an older slot. The slot guard in `handleSlottedValue`, however, dropped a stale-slot response wholesale — value and status transition alike — so a successful-but-older response could not lift the store out of `loading`. In practice this stranded a refresh: `useTrackedData`'s `refresh()` maps to `connect()` (not `reset()`), so `lastUpdateSlot` carries over. If the re-fetch is answered by a lagging RPC node at an older slot and the account is quiet (its subscription emits nothing), the store sits in `loading` indefinitely — a perpetual refresh spinner over retained stale data, with no error and no settle until the account next changes. A stale-slot response now settles the store back to `loaded` while retaining the newer data it already holds, rather than adopting the older value. The monotonic-value guarantee is unchanged; only the liveness of the status transition is fixed. Adds tests covering the stale-slot reconnect via both the initial-value and stream sources, plus the subscriber notification on settle.
This was referenced Jun 12, 2026
Member
Author
BundleMonFiles updated (25)
Unchanged files (122)
Total files change +8.83KB +1.67% Final result: ✅ View report in BundleMon website ➡️ |
Member
Author
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Fixes a stuck-in-loading bug in createReactiveStoreWithInitialValueAndSlotTracking. lastUpdateSlot persists across connect() windows (deliberately, to prevent regression), but the previous code dropped stale-slot responses entirely — including the status transition. If a refresh window was answered only by a lagging RPC (stale slot) and the subscription stayed quiet, the store sat in loading forever.
The fix keeps the previous (newer) data and lastUpdateSlot, but still transitions the status back to loaded. The drop-the-value invariant is preserved; only the status now gets to escape loading.
Looks good to me — minimal, targeted, well-tested. Approving in spirit (no write access to formally approve).
Correctness notes
lastUpdateSlotis intentionally not updated in the stale branch — the surfaced value still cannot regress. ✅currentState.data !== undefinedguard is defensively correct. In practice, hittingslot < lastUpdateSlotwithlastUpdateSlot > -1nimplies a value was previously set, andreset()clears both together. The guard is belt-and-braces against any future path that might desync them — fine to keep.error: undefinedon the stale-loaded transition mirrors the fresh-slot path. If a prior connection window errored after loading data, a stale-but-successful response on the new window now clears the error and surfacesloaded— consistent with treating any successful response (even stale-slot) as recovery.- No spurious notifies on repeated stale arrivals: after the first stale →
loaded, subsequent stale values hitsetState's status/data/error equality short-circuit. ✅ - First-ever value (
lastUpdateSlot === -1n) can't take the stale branch since any non-negative slot is>= -1n. ✅
Tests
Good coverage: initial-value source stale path, stream source stale path, and subscriber-notification semantics on the loading → loaded settle. All three use jest.runAllTimersAsync() per the repo convention.
Changeset
Present, patch, scoped to @solana/kit. Appropriate for a bug fix.
For subsequent reviewers
- Double-check whether you want the stale-slot path to clear a prior
error(current behavior — I think it's right, but worth a sanity check given it's a behavior change beyond just unstickingloading). - The
currentState.data !== undefinedguard means a stale slot arriving while data is stillundefined(e.g. a hypothetical future path wherelastUpdateSlotgets bumped without setting data) would silently do nothing. Today that path doesn't exist, so this is fine.
Contributor
|
Documentation Preview: https://kit-docs-4ve3r9gcg-anza-tech.vercel.app |
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
3 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.
Problem
Spotted while reviewing the react stack, but the issue is in
createReactiveStoreWithInitialValueAndSlotTrackingIt persists the last seen slot across refreshes (good), and drops stale data. But there was a race condition: if a refresh happens, and the RPC returns outdated data on an older slot than previously seen, it gets dropped and the store stays stuck in a loading state until a subscription message happens (which might never happen).
Summary of Changes
Now in this case, we change the status to
loaded, while keeping the previous newer data intact.