Revive token socket_record after expiration #5977
Merged
Conversation
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
If the socket_record expires in redis, but the instance managing it is still alive, then re-link the token to the sid. For `del` and `evicted`, do not re-link the token as this is an explicit disconnect.
CodSpeed Performance ReportMerging #5977 will not alter performanceComparing Summary
|
Contributor
Greptile OverviewGreptile SummaryThis PR implements token revival logic when Redis records expire while the managing instance is still alive. The change also fixes a critical bug in Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Redis
participant Subscriber as Redis Keyspace Subscriber
participant TokenManager as RedisTokenManager
participant LocalDict as token_to_socket dict
Note over Redis: Token expires in Redis
Redis->>Subscriber: keyspace event: "expired"
Subscriber->>TokenManager: _handle_socket_record_del(token, expired=True)
TokenManager->>LocalDict: pop(token)
LocalDict-->>TokenManager: socket_record
Note over TokenManager: Check instance_id == self.instance_id
alt expired=True
TokenManager->>TokenManager: link_token_to_sid(token, sid)
TokenManager->>Redis: set(token_key, socket_record, ex=...)
TokenManager->>LocalDict: token_to_socket[token] = socket_record
Note over TokenManager: Token revived!
end
TokenManager->>LocalDict: sid_to_token.pop(sid)
Note over Redis: Explicit delete
Redis->>Subscriber: keyspace event: "del"
Subscriber->>TokenManager: _handle_socket_record_del(token, expired=False)
TokenManager->>LocalDict: pop(token)
LocalDict-->>TokenManager: socket_record
Note over TokenManager: Check instance_id == self.instance_id
alt expired=False
Note over TokenManager: No revival - token stays deleted
end
TokenManager->>LocalDict: sid_to_token.pop(sid)
|
| expired: Whether the deletion was due to expiration. | ||
| """ | ||
| if ( | ||
| socket_record := self.token_to_socket.pop(token, None) |
Contributor
There was a problem hiding this comment.
logic: the token has been popped from token_to_socket on line 242, but then re-inserted during link_token_to_sid on line 247 - this creates a race where the socket_record is temporarily missing
Suggested change
| socket_record := self.token_to_socket.pop(token, None) | |
| ) is not None and socket_record.instance_id == self.instance_id: | |
| if expired: | |
| # Keep the record alive as long as this process is alive and not deleted. | |
| await self.link_token_to_sid(token, socket_record.sid) | |
| self.sid_to_token.pop(socket_record.sid, None) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: reflex/utils/token_manager.py
Line: 242:242
Comment:
**logic:** the token has been popped from `token_to_socket` on line 242, but then re-inserted during `link_token_to_sid` on line 247 - this creates a race where the socket_record is temporarily missing
```suggestion
) is not None and socket_record.instance_id == self.instance_id:
if expired:
# Keep the record alive as long as this process is alive and not deleted.
await self.link_token_to_sid(token, socket_record.sid)
self.sid_to_token.pop(socket_record.sid, None)
```
How can I resolve this? If you propose a fix, please make it concise.
adhami3310
approved these changes
Nov 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
If the socket_record expires in redis, but the instance managing it is still alive, then re-link the token to the sid.
For
delandevicted, do not re-link the token as this is an explicit disconnect.