diff --git a/Cargo.toml b/Cargo.toml index 741a39b38..f5ea11ea1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,9 @@ unsafe_code = "deny" # We use groups as good defaults, but with a lower priority so that we can override them all = { level = "deny", priority = -1 } pedantic = { level = "warn", priority = -1 } - +# Allowed because it's nice to have temporary semantic names, see +# https://github.com/rust-lang/rust-clippy/issues/12512#issuecomment-3316736180 +let_and_return = { level = "allow", priority = 1 } str_to_string = "deny" too_many_lines = "allow" diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 30debdf99..be44e3bf0 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -4,6 +4,7 @@ // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. +use std::collections::HashMap; use std::sync::{Arc, LazyLock}; use axum::{Json, extract::State, response::IntoResponse}; @@ -531,7 +532,7 @@ async fn process_violations_for_compat_login( // automatically remove their oldest devices (when `hard_limit_eviction` // is configured). if session_limit_config.hard_limit_eviction { - // Find the least recently used compat sessions + // Find the least recently used (LRU) compat sessions // // In the future, it may be nice to avoid sessions with // cryptographic state (what does that mean exactly? keys uploaded @@ -540,38 +541,91 @@ async fn process_violations_for_compat_login( // FIXME: We could potentially use // `repo.compat_session().finish_bulk(...)` if it had the ability to // limit and order. - let compat_session_page = repo - .compat_session() - .list( - // TODO: Order by `last_active_at` - CompatSessionFilter::new().for_user(user).active_only(), - Pagination::first(need_to_remove_usize), - ) - .await?; + let lru_compat_sessions = { + // TODO: In the future, instead of all of this faff, we can simply order + // by `last_active_at` + // + // XXX: Since we can't order by `last_active_at` yet, we instead filter + // the list down to "inactive" sessions (`last_active_at` > 90 days + // ago). And by the nature of + // [`mas_data_model::compat::CompatSession::id`] being a `Ulid`/`Uuid` + // (the query is ordered by `compat_session_id`), the first bytes are a + // timestamp so we'll be getting the 'oldest created' sessions which is + // another good proxy. + + let mut edges_to_consider = Vec::new(); + + // First, find the "inactive" sessions + let inactive_threshold_date = clock.now() - Duration::days(90); + let inactive_compat_session_page = repo + .compat_session() + .list( + CompatSessionFilter::new() + .for_user(user) + .active_only() + .with_last_active_before(inactive_threshold_date), + // We fetch a minimum of 100 sessions (more than we need in + // normal cases) so we can sort by `last_active_at` after it + // gets back from the database and can get even closer to + // removing the oldest sessions. + Pagination::first(std::cmp::max(need_to_remove_usize, 100)), + ) + .await?; + edges_to_consider.extend(inactive_compat_session_page.edges); + + // If there aren't enough "inactive" sessions, supplement with active ones + if edges_to_consider.len() < need_to_remove_usize { + let active_compat_session_page = repo + .compat_session() + .list( + CompatSessionFilter::new() + .for_user(user) + .active_only() + .with_last_active_after(inactive_threshold_date), + // We fetch a minimum of 100 sessions (more than we need in + // normal cases) so we can sort by `last_active_at` after it + // gets back from the database and can get even closer to + // removing the oldest sessions. + Pagination::first(std::cmp::max(need_to_remove_usize, 100)), + ) + .await?; + edges_to_consider.extend(active_compat_session_page.edges); + } + + // De-duplicate the sessions across both pages + let compat_session_map = { + let mut compat_session_map = HashMap::new(); + for edge in edges_to_consider { + let (compat_session, _) = edge.node; + compat_session_map.insert(compat_session.id, compat_session); + } + compat_session_map + }; + + // List of compat sessions sorted by `last_active_at` ascending + let sorted_compat_sessions = { + let mut compat_sessions: Vec = + compat_session_map.into_values().collect(); + // Sort by `last_active_at` (ascending) + compat_sessions.sort_by_key(|compat_session| compat_session.last_active_at); + compat_sessions + }; + + sorted_compat_sessions + }; // For now, we only automatically clean up compatibility sessions. // If there aren't enough sessions that we could clean up, we just // throw an error with an explanation. - if compat_session_page.edges.len() < need_to_remove_usize { + if lru_compat_sessions.len() < need_to_remove_usize { return Err(RouteError::PolicyHardSessionLimitReached); } - // Remove the sessions - let num_sessions_removed = { - let mut num_sessions_removed = 0; - for edge in compat_session_page.edges { - let (compat_session, _) = edge.node; - repo.compat_session().finish(clock, compat_session).await?; - num_sessions_removed += 1; - } - num_sessions_removed - }; - - // For now, we only automatically clean up compatibility sessions. - // If there are still too many sessions, we just throw an error with - // an explanation. - if num_sessions_removed < need_to_remove { - return Err(RouteError::PolicyHardSessionLimitReached); + // Remove the sessions (only as much as necessary, `need_to_remove`) + for compat_session in &lru_compat_sessions[0..need_to_remove_usize] { + repo.compat_session() + .finish(clock, compat_session.to_owned()) + .await?; } } else { // Tell the user about the limit