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

Document ways to cleanup expired sessions #3221

Open
wants to merge 4 commits into
base: minor
Choose a base branch
from

Conversation

pujux
Copy link
Contributor

@pujux pujux commented Nov 20, 2024

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:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

vercel bot commented Nov 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Nov 20, 2024 9:15pm

@pujux pujux changed the base branch from master to minor November 20, 2024 21:12
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
6.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

.createQueryBuilder('session')
.select('session.id')
.where('session.expires <= :now', { now })
.getMany();
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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 @@
---
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea! should I put that into the "Developer Guide" section under "Advanced Topics" just like the "Migrating from v1" topic?
image

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea 👍

@pujux
Copy link
Contributor Author

pujux commented Dec 3, 2024

I'll pick this back up ASAP but I'm currently not able to dedicate time to it.

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.

2 participants