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

Unauthenticated session expiry #5270

Closed
wants to merge 1 commit into from

Conversation

mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Oct 17, 2024

Follow up for openstreetmap/operations#1107

The goal of this PR is to set the expire_after value for unauthenticated users to a fairly low value (read: a few hours). This way, memcached entries are no longer created with a TTL of 0 (unlimited).

Previously, logged on user sessions were evicted first, because their TTL is set to 30 days in session_persistence.rb / session_methods.rb. As a result, a number of users reported that they had to repeatedly sign in to osm.org, since their session was gone.

The chosen approach is based on what Gitlab is doing in https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/middleware/unauthenticated_session_expiry.rb, with the difference that I replaced redis by memcached, and warden by a simple logged on user check.

osm_session cookies inherit the same low expire_after value now, rather than being session cookies. I don’t think this should be causing issues. This needs to be doubled checked as well. Cookies for unauthenticated users are primarily used for csrf protection, and are updated with every backend roundtrip. They are not relevant for API endpoints.

It still needs some tests. However, I'm not sure what the best way would be.


Prometheus stats: https://prometheus.openstreetmap.org/d/l4zgNUdMz/memcached?orgId=1&refresh=1m&var-instance=spike-06&var-instance=spike-07&var-instance=spike-08&from=now-30d&to=now

@mmd-osm mmd-osm force-pushed the patch/sessionexpiry branch from c3d8be0 to f99b2ad Compare October 17, 2024 17:58
@mmd-osm mmd-osm marked this pull request as draft October 17, 2024 20:46
@mmd-osm mmd-osm force-pushed the patch/sessionexpiry branch 8 times, most recently from 9608220 to 429cb59 Compare October 22, 2024 18:13
@tomhughes
Copy link
Member

Not sure why you closed this? I actually spent some time yesterday digging into it...

I am interested in knowing if you've found any sources backing up the theory that entries with no expiry can somehow swamp out ones with an expiry as I'm not really clear on how that would happen?

@mmd-osm
Copy link
Contributor Author

mmd-osm commented Oct 28, 2024

I can't validate the assumption in production and don't know what test cases to add. Or in other words, I was stuck with the current state and decided to close it, as there's no way to make any progress on it.

Assuming a slab is full and new entries need to be added. There are two possible options: entries that expire sooner (rather than never) get evicted first, or some random entry will be evicted. Both options are bad, since they might remove valid user sessions.

To me, this question is not super important. I would rather let the LRU crawler clean up short lived anonymous sessions as soon as possible, and never get to a state where eviction is unavoidable. Ideally, this change would keep the items in the cache at a very low number all the time, and never get to a point of 80%+ memory usage.

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