Repository navigation
Make session creation to be idempotent - #1677
Conversation
edwinyyyu
left a comment
edwinyyyu
left a comment
There was a problem hiding this comment.
Disagree with tenant lifecycle here, but it's not any worse than what's already in the codebase.
| session = sessions.scalars().first() | ||
| if session is not None: | ||
| if ( | ||
| session.configuration == configuration |
There was a problem hiding this comment.
#1539's create_new_session_if_not_exist accepts an existing row only when session.status != SessionDataManager.SessionStatus.Deleted as well as the three data matches; this port has the three matches without the status condition. So on main a session whose row is marked deleted (delete in progress, or a delete whose purge has not run) is accepted by a matching create, and create_episodic_memory then builds an instance for it, which open_episodic_memory would refuse with SessionDeletedError.
Verified on 843f00cd9 with the manager test's setup: create a session, close_session, update_session_status(key, "delete"), then create_episodic_memory with the same data — no error, where #1539 raises SessionAlreadyExistsError and this branch's open_episodic_memory raises SessionDeletedError. #1624's port of the deleted-session test drops its re-create assertion because of this, and says so in its body.
2 participants
Purpose of the change
Make episodic memory session creation to be idempotent
Description
In current logic, when creating a session, if the session key already exists, the creation will fail. This logic will not work with scale out because multiple workers may create the same session simultaneously. With this PR, if the new session has the same configuration with the existing one, the creation will succeed.
Fixes/Closes
Fixes #(issue number)
Type of change
[Please delete options that are not relevant.]
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
[Please delete options that are not relevant.]
Test Results: [Attach logs, screenshots, or relevant output]
Checklist
[Please delete options that are not relevant.]
Maintainer Checklist
Screenshots/Gifs
[If applicable, add screenshots or GIFs that show the changes in action. This is especially helpful for API responses. Otherwise, delete this section or type "N/A".]
Further comments
[Add any other relevant information here, such as potential side effects, future considerations, or any specific questions for the reviewer. Otherwise, type "None".]