Conversation
4ndrelim
left a comment
4ndrelim
left a comment
There was a problem hiding this comment.
Was investigating the bug too but saw you raised a PR before i did anything. So left some comments based on my udnerstanding. Seems like the right approach! Ill try to simulate some possible set-ups and test with ur fix.
| } | ||
|
|
||
| setTokens(token: string, refreshToken: string): void { | ||
| useAuthStore.getState().setToken(token); |
There was a problem hiding this comment.
May i check if im understanding the situation right. This fix is for 401 error?
In particular, when we refresh due to 401 error from sending a request with expired tokens, we get the newly generated tokens but this was never updated to auth-store ( and not written to browser localStorage). And so, on the next session, e.g. page reload, auth-store loads the stale tokens leading to 401 token expiry error?
There was a problem hiding this comment.
Yep, that's my understanding at least
| token: resp.token, | ||
| refreshToken: resp.refreshToken, | ||
| }); | ||
| if (this.refreshPromise) { |
There was a problem hiding this comment.
nice dedup logic. Yes this should address the multiple async calls to refresh, leading to multiple redundant token generation, and possible 401 bug across sessions.
This is actually rather tricky to test though since i think the bug is probabilistic? i believe for the current session, neither caller (your e.g. v2/chats/models and v2/chats/conversations) will bug out since auth-store uses last write valid token. It is only when caller_A and caller_B each calls refresh() and backend mongo receives a different set of token from what was written to browser localStorage.
There was a problem hiding this comment.
Yea, it's definitely not consistent. I was able to reproduce it a few (rare) times by lowering the expiration time of the token, and then switching between the settings and chat tabs. Although I am not 100% sure if it is the problem stated in #110
From my testing, my understanding is that it is possible for either caller_A or caller_B to bug out in the current session.
For example, caller_A and caller_B calls refresh() with token_A, there were a few possibilities that I encountered:
- caller_A refreshes to token_B and the API call is retried, caller_B fails to refresh with token_A because the backend is updated to token_B, and this API call is not retried and fails.
- caller_A receives a new token_B and caller_B receives a new token_C at the same time. Assuming the frontend receives token_C latest, it stores it in its
ApiClientinstance. However, in some rare occurrences, the backend stores token_B instead. This causes subsequent API calls to fail. Coincidentally, this happened again literally as I was typing this. - Same as the above but the backend stores the same token as the frontend, so subsequent API calls succeed as per normal.
There was a problem hiding this comment.
ah good catch and test. Yeah but i think the reason the current session bugs out its because the token expired? That is when mongo backend realizes it has a different token from what was written to browser localStorage.
Let me know if im off but i think if the expiration is long (or default), apiclient will simply use the last write token from the multiple refresh() calls and so the bug does not surface. It might surface after token expiry (e.g. 1 day later) when last write token is written to mongo, but no guarantee which token is written to browser localStorage due to network uncertainties (ie both tokens go through auth-store but we don't know which one might be written last to browser). This seems to correspond with past experience where occasionally, logging in a 1-2 days later, the bubble for invalid token somehow repeatedly pops up.
Yup, setting a very small expiration time will increase likelihood of replicating this bug. nice!
As for #110, im not sure if this resolves it either. It seems @wjiayis issue might be elsewhere, specifically latency from API and context. But she will know better so tagging her for inputs.
There was a problem hiding this comment.
feel free to mark it ready once you're done and i will test it before approving.
There was a problem hiding this comment.
Yep, that's my understanding as well. Just that I believe (correct me if I'm wrong) prior to this change, localStorage is actually never written to except right after a new login, hence the bug below:
Example: User refreshes to a new token and exits Overleaf, the next time he re-opens PaperDebugger, it uses the old refresh token.
I think I have nothing else to add for this aside from maybe @wjiayis's inputs, will mark it ready for now, thank you
2 participants
1. Local Storage Not Updated
Access tokens and refresh tokens are only saved to local storage on login, subsequent token refreshes do not update local storage.
Example: User refreshes to a new token and exits Overleaf, the next time he re-opens PaperDebugger, it uses the old refresh token.
Proposed solution: Update authStore whenever tokens are set.
2. Race Conditions When Refreshing
PaperDebugger often calls multiple endpoints at the same time, which results in a race condition if the token needs to be refreshed.
Example:
v2/chats/modelsandv2/chats/conversationsare called at the same time, and the access token needs refreshing, the refresh endpoint is called twice. In some occasions, the frontend uses the 2nd refresh token received which differs from the one stored in the backend. This can be easily reproduced by setting the JWT expiration in the backend to a very short time.Proposed solution: Use a promise for
refresh().Unsure if this fixes the exact problem in #110