Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix session persistence handlers #4927

Closed
wants to merge 2 commits into from
Closed

Fix session persistence handlers #4927

wants to merge 2 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 13, 2024

There is a regression in #4857 — although it appears to be setting up the session persistence handler, that handler (stored in a ref) is not being read. Additionally, it is a little fragile that merely calling createAgent* modifies a ref since create* functions were intended to be stateless. Otherwise it gets very difficult to reason about race conditions.

In this PR, I remove the ref and move the persistence handler to be scoped per-agent like before. The underlying BskyAgent signature changed so that the persistence handler has to be supplied during creation. However, we don't always know the did during its creation (e.g. we wouldn't know the did of an agent whose createAccount call is yet to follow). So we want late binding for the session handler. To do that, I store it in a local variable and swap it out when ready.

I ended up removing prepareAgent handler because it was obscuring what's actually happening. This adds a bit of duplication but should be clearer.

Test Plan

Have not explicitly tested but @haileyok knows a few bugs that this should fix.

Copy link

render bot commented Aug 13, 2024

@@ -266,7 +250,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
// We never reuse agents so let's fully neutralize the previous one.
// This ensures it won't try to consume any refresh tokens.
prevAgent.sessionManager.session = undefined
setPersistSessionHandler(undefined)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I am no longer cleaning up the persistence handler when the current agent gets swapped. I think this should be fine. It didn't used to be cleaned up in the past either, I only added this at some intermediate stage for peace of mind. But we already have a condition here that prevents inactive agents from clearing persisted data. For inactive agents, refreshedAccount should be undefined because we already set prevAgent.sessionManager.session = undefined here. And even if somehow it had a session object, the reducer always matches events with state by the did so it won't update the wrong thing.

@gaearon gaearon requested a review from haileyok August 13, 2024 02:01
Copy link

Old size New size Diff
7.09 MB 7.09 MB 120 B (0.00%)

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 13, 2024

let's go with #4928

@gaearon gaearon closed this Aug 13, 2024
@gaearon gaearon deleted the agent-twks branch August 13, 2024 02:39
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