-
Notifications
You must be signed in to change notification settings - Fork 32
Unload expired sessions from store #3033
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| // Before querying sessions, unload sessions currently stored in ember data store | ||
| // so that we remove any expired sessions that still might be cached. | ||
| this.store.unloadAll('session'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make sure I understand this, it sounds like if sessions are terminated and we wait until the backend job clears them, the API will still return the removed_ids for these removed terminated sessions.
We previously weren't using include_terminated which wouldn't have returned those IDs but even if we do set it to true, it sounds like it still won't fix the issue because we don't actually have a mechanism to actually remove them from ember data store since we actually only push new records or updated records here
| ? store.push({ data: results }) |
Is this accurate? If so, I wonder if the actual issue/bug is in the handler because we aren't properly removing them from the ember data store 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that is correct, we don't properly clean up the data store. I imagine ur right in this being an issue for all resources where it doesn't get directly removed using destroyRecord. Ill update to perform the cleanup in the handler instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually have a mechanism to actually remove them from ember data store since we actually only push new records or updated records here
Isn't the store empty of all sessions already though by:
this.store.unloadAll('session');
If the db is accurate couldn't we just refresh all the session models associated with the targets being displayed instead of clearing all sessions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was it seems like we shouldn't need to do the unloadAll since if we're pushing to store, I would also expect deleted records to be pushed. I think otherwise this is a surprising "gotcha" since you now have to be aware to also unload records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was thinking the same but I think I was missing the second part: include_terminatedremoved_ids should be enough to refresh clear models from the store
ZedLi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected, thanks for this fix!
270cbcc
priya-patel04
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Description
Expired sessions get cleaned up after a while and the ember data store might still have them cached (unless there was a hard refresh) which will lead to inaccurate display of "Active sessions" on a target list page because we use
peekAllto display active sessions. This is only an issue with the ember data store, the sqlite db correctly removes terminated sessions usingremoved_idsfield from the API when those sessions get cleaned up. Therefore, this PR unloads sessions in the targets route model hook before making API queries so that any removed/terminated sessions get cleaned up from our ember data cache.Screenshots (if appropriate)
Issue seen in Admin UI:
Screen.Recording.2025-10-20.at.1.24.49.PM.mov
Issue seen in Desktop UI:
Screen.Recording.2025-10-17.at.3.12.40.PM.mov
How to Test
Checklist
a11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.