-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Document ways to cleanup expired sessions #3221
base: minor
Are you sure you want to change the base?
Document ways to cleanup expired sessions #3221
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Quality Gate failedFailed conditions |
.createQueryBuilder('session') | ||
.select('session.id') | ||
.where('session.expires <= :now', { now }) | ||
.getMany(); |
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.
For existing apps that have been in prod for a while, there might be millions of rows being selected here. I see we only select the id which is good, but I'm wondering whether there could still be issues here?
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 could change this to just run a DELETE query for every row where session.expires <= :now
-> that should not pose any issues I would say, but we also wouldn't be able to retrieve the sessionIds thereafter and thus not be able to delete them from the sessionCacheStrategy -> which I think wouldn't even be necessary since they are already expired so should not be in the cache anymore anyway (?) - haven't had a look at the internals of the sessionCacheStrategy
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.
The Redis session cache uses the built-in Redis TTL mechanism to evict old entries, and the default session cache is in-memory anyway, so yeah that should not be an issue.
@@ -0,0 +1,49 @@ | |||
--- |
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 think we should have a more general topic of "how to run scheduled tasks" with a general explanation of the problem and solution and then the session cleanup as the main example. In future we can then add more examples like cleaning up abandoned orders.
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.
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.
Yes, good idea 👍
I'll pick this back up ASAP but I'm currently not able to dedicate time to it. |
Description
Issue: #3219
This pull request adds a method to the SessionService that will cleanup expired sessions. Furthermore it adds a note to the SessionService docs to explain that sessions are not automatically cleaned up which references the new How-to guide demonstrating how to use Stand-alone CLI scripts to automate the process of cleaning up sessions.
Also updated the
docusaurus-theme-search-typesense
package version to make docs generation work again.Breaking changes
N/A
Screenshots
N/A
Checklist
📌 Always:
👍 Most of the time: