From 6a27c742d4cd6b16acbab2e897dac6715a0637aa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 09:21:35 +0000 Subject: [PATCH 1/5] Fix CalDAV/CardDAV multiget loading entire collections into memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A REPORT multiget for a handful of resources previously listed the whole calendar/address book (every row incl. ical_data/vcard) and filtered by href in Rust with an O(N×M) substring scan. Large collections paid full-table latency, RAM and DB CPU on every sync. - Add find_events_by_ical_uids / get_contacts_by_uids through every layer (domain repo trait → PG repo → storage port/adapter → use case → handler) using one indexed `= ANY($2)` query, mirroring the existing single-UID lookups. - Extract UIDs from multiget hrefs with a shared, tested uid_from_multiget_href helper (percent-decoding, case-insensitive extension strip, collection hrefs rejected). Exact UID matching also removes the false positives the old substring filter allowed. - CalDAV PROPFIND single-event path now uses the existing indexed get_event_by_ical_uid instead of listing all events. - Honour the already-documented limit/offset query params on GET /api/address-books/{id}/contacts: optional ListQuery fields thread through ContactUseCase::list_contacts to a paginated repo query. Omitted params keep returning the full book (frontend and DAV listing/sync paths unchanged). https://claude.ai/code/session_0193Hff42gaA962wThxMGSd1 --- src/application/adapters/mod.rs | 88 +++++++++++++++++++ src/application/ports/calendar_ports.rs | 17 ++++ src/application/ports/carddav_ports.rs | 16 ++++ src/application/services/calendar_service.rs | 26 ++++++ src/application/services/contact_service.rs | 44 ++++++++-- .../repositories/calendar_event_repository.rs | 10 +++ src/domain/repositories/contact_repository.rs | 17 ++++ .../adapters/calendar_storage_adapter.rs | 20 +++++ .../adapters/contact_storage_adapter.rs | 35 +++++++- .../pg/calendar_event_pg_repository.rs | 50 +++++++++++ .../repositories/pg/contact_pg_repository.rs | 70 ++++++++++++++- src/interfaces/api/handlers/caldav_handler.rs | 34 +++---- .../api/handlers/carddav_handler.rs | 28 +++--- .../api/handlers/contacts_handler.rs | 25 +++--- 14 files changed, 431 insertions(+), 49 deletions(-) diff --git a/src/application/adapters/mod.rs b/src/application/adapters/mod.rs index 6fd285f1..08fbab7f 100644 --- a/src/application/adapters/mod.rs +++ b/src/application/adapters/mod.rs @@ -8,3 +8,91 @@ pub mod webdav_adapter; mod caldav_adapter_test; #[cfg(test)] mod carddav_adapter_test; + +/// Extract the resource UID from a DAV multiget `href`. +/// +/// CalDAV/CardDAV multiget REPORTs address object resources by full href +/// (e.g. `/caldav/{calendar_id}/{uid}.ics`, possibly with a username +/// segment). The UID is the last path segment with the protocol +/// `extension` (`.ics` / `.vcf`, matched case-insensitively) stripped, +/// then percent-decoded — hrefs arrive in the XML body, so they have not +/// gone through URL-path decoding, and clients may re-encode hrefs they +/// previously read from the server. +/// +/// Returns `None` for collection hrefs (empty last segment) or segments +/// that are not valid UTF-8 after decoding. +pub fn uid_from_multiget_href(href: &str, extension: &str) -> Option { + // A trailing slash denotes a collection, not an object resource. + if href.ends_with('/') { + return None; + } + let segment = href.rsplit('/').next()?; + + // Case-insensitive ASCII extension strip; the matched tail is ASCII, + // so the byte cut is guaranteed to land on a char boundary. + let bytes = segment.as_bytes(); + let ext = extension.as_bytes(); + let segment = + if bytes.len() >= ext.len() && bytes[bytes.len() - ext.len()..].eq_ignore_ascii_case(ext) { + &segment[..segment.len() - ext.len()] + } else { + segment + }; + + let decoded = percent_encoding::percent_decode_str(segment) + .decode_utf8() + .ok()?; + let uid = decoded.trim(); + (!uid.is_empty()).then(|| uid.to_string()) +} + +#[cfg(test)] +mod multiget_href_tests { + use super::uid_from_multiget_href; + + #[test] + fn plain_caldav_href() { + assert_eq!( + uid_from_multiget_href("/caldav/abc-123/event-uid.ics", ".ics"), + Some("event-uid".to_string()) + ); + } + + #[test] + fn href_with_username_prefix() { + assert_eq!( + uid_from_multiget_href("/carddav/alice/book-1/uid-42.vcf", ".vcf"), + Some("uid-42".to_string()) + ); + } + + #[test] + fn uppercase_extension() { + assert_eq!( + uid_from_multiget_href("/caldav/abc/EVENT.ICS", ".ics"), + Some("EVENT".to_string()) + ); + } + + #[test] + fn percent_encoded_uid() { + assert_eq!( + uid_from_multiget_href("/caldav/abc/uid%40example.com.ics", ".ics"), + Some("uid@example.com".to_string()) + ); + } + + #[test] + fn missing_extension_uses_whole_segment() { + assert_eq!( + uid_from_multiget_href("/caldav/abc/bare-uid", ".ics"), + Some("bare-uid".to_string()) + ); + } + + #[test] + fn collection_href_yields_none() { + assert_eq!(uid_from_multiget_href("/caldav/abc/", ".ics"), None); + assert_eq!(uid_from_multiget_href("", ".ics"), None); + } +} diff --git a/src/application/ports/calendar_ports.rs b/src/application/ports/calendar_ports.rs index 56799533..28728a88 100644 --- a/src/application/ports/calendar_ports.rs +++ b/src/application/ports/calendar_ports.rs @@ -95,6 +95,14 @@ pub trait CalendarStoragePort: Send + Sync + 'static { calendar_id: &str, ical_uid: &str, ) -> Result, DomainError>; + /// Indexed batch lookup by iCalendar UID (`ical_uid = ANY(...)`) — the + /// CalDAV multiget REPORT must use this instead of listing the whole + /// calendar (every row + its `ical_data`) and filtering client-side. + async fn find_events_by_ical_uids( + &self, + calendar_id: &str, + ical_uids: &[String], + ) -> Result, DomainError>; async fn list_events_by_calendar( &self, calendar_id: &str, @@ -197,6 +205,15 @@ pub trait CalendarUseCase: Send + Sync + 'static { ical_uid: &str, user_id: Uuid, ) -> Result, DomainError>; + /// Resolve a batch of events by their iCalendar UIDs with a single + /// indexed query. UIDs without a matching event are silently absent + /// from the result (CalDAV multiget semantics). + async fn get_events_by_ical_uids( + &self, + calendar_id: &str, + ical_uids: &[String], + user_id: Uuid, + ) -> Result, DomainError>; async fn list_events( &self, calendar_id: &str, diff --git a/src/application/ports/carddav_ports.rs b/src/application/ports/carddav_ports.rs index ee49dcec..cb842c14 100644 --- a/src/application/ports/carddav_ports.rs +++ b/src/application/ports/carddav_ports.rs @@ -82,9 +82,25 @@ pub trait ContactUseCase: Send + Sync + 'static { uid: &str, user_id: Uuid, ) -> Result, DomainError>; + /// Resolve a batch of contacts by their vCard UIDs with a single + /// indexed query (`uid = ANY(...)`) — the CardDAV multiget REPORT + /// must use this instead of listing the whole address book and + /// filtering client-side. UIDs without a matching contact are + /// silently absent from the result. + async fn get_contacts_by_uids( + &self, + address_book_id: &str, + uids: &[String], + user_id: Uuid, + ) -> Result, DomainError>; + /// List contacts in an address book. `limit`/`offset` bound the + /// result for paginated callers (REST API); `None` returns the full + /// book, which the CardDAV listing/sync paths rely on. async fn list_contacts( &self, address_book_id: &str, + limit: Option, + offset: Option, user_id: Uuid, ) -> Result, DomainError>; async fn search_contacts( diff --git a/src/application/services/calendar_service.rs b/src/application/services/calendar_service.rs index 6b51cb8a..762a38d9 100644 --- a/src/application/services/calendar_service.rs +++ b/src/application/services/calendar_service.rs @@ -300,6 +300,32 @@ impl CalendarUseCase for CalendarService { .await } + async fn get_events_by_ical_uids( + &self, + calendar_id: &str, + ical_uids: &[String], + user_id: Uuid, + ) -> Result, DomainError> { + let has_access = self + .calendar_storage + .check_calendar_access(calendar_id, user_id) + .await?; + let calendar = self.calendar_storage.get_calendar(calendar_id).await?; + if !has_access && !calendar.is_public { + return Err(DomainError::new( + ErrorKind::AccessDenied, + "Calendar", + "You don't have permission to view events in this calendar", + )); + } + if ical_uids.is_empty() { + return Ok(Vec::new()); + } + self.calendar_storage + .find_events_by_ical_uids(calendar_id, ical_uids) + .await + } + async fn list_events( &self, calendar_id: &str, diff --git a/src/application/services/contact_service.rs b/src/application/services/contact_service.rs index 9d44b306..b87642eb 100644 --- a/src/application/services/contact_service.rs +++ b/src/application/services/contact_service.rs @@ -796,9 +796,34 @@ impl ContactUseCase for ContactService { Ok(contact.map(ContactDto::from)) } + async fn get_contacts_by_uids( + &self, + address_book_id: &str, + uids: &[String], + user_id: Uuid, + ) -> Result, DomainError> { + let id = Uuid::parse_str(address_book_id) + .map_err(|_| DomainError::validation_error("Invalid address book ID format"))?; + + // Check if user has access to the address book + self.check_address_book_access(&id, &user_id).await?; + + if uids.is_empty() { + return Ok(Vec::new()); + } + + let contacts = self + .contact_repository + .get_contacts_by_uids(&id, uids) + .await?; + Ok(contacts.into_iter().map(ContactDto::from).collect()) + } + async fn list_contacts( &self, address_book_id: &str, + limit: Option, + offset: Option, user_id: Uuid, ) -> Result, DomainError> { let id = Uuid::parse_str(address_book_id) @@ -808,10 +833,17 @@ impl ContactUseCase for ContactService { self.check_address_book_access(&id, &user_id).await?; // Get contacts - let contacts = self - .contact_repository - .get_contacts_by_address_book(&id) - .await?; + let contacts = if limit.is_some() || offset.is_some() { + let limit = limit.unwrap_or(100); + let offset = offset.unwrap_or(0); + self.contact_repository + .get_contacts_by_address_book_paginated(&id, limit, offset) + .await? + } else { + self.contact_repository + .get_contacts_by_address_book(&id) + .await? + }; let dtos = contacts.into_iter().map(ContactDto::from).collect(); Ok(dtos) @@ -1324,7 +1356,9 @@ impl StorageUseCase for ContactService { let user_id = Uuid::parse_str(user_id) .map_err(|_| DomainError::validation_error("Invalid user_id format"))?; - let result = self.list_contacts(address_book_id, user_id).await?; + let result = self + .list_contacts(address_book_id, None, None, user_id) + .await?; Ok(serde_json::to_value(result).unwrap()) } "search_contacts" => { diff --git a/src/domain/repositories/calendar_event_repository.rs b/src/domain/repositories/calendar_event_repository.rs index dde25fac..d08cc561 100644 --- a/src/domain/repositories/calendar_event_repository.rs +++ b/src/domain/repositories/calendar_event_repository.rs @@ -53,6 +53,16 @@ pub trait CalendarEventRepository: Send + Sync + 'static { ical_uid: &str, ) -> CalendarEventRepositoryResult>; + /// Finds the events matching any of the given iCalendar UIDs in one + /// indexed query (`ical_uid = ANY(...)`). Used by CalDAV multiget so a + /// request for a handful of events never pays for the whole calendar. + /// UIDs with no matching event are silently absent from the result. + async fn find_events_by_ical_uids( + &self, + calendar_id: &Uuid, + ical_uids: &[String], + ) -> CalendarEventRepositoryResult>; + /// Counts events in a calendar async fn count_events_in_calendar( &self, diff --git a/src/domain/repositories/contact_repository.rs b/src/domain/repositories/contact_repository.rs index 76309ac2..01aae004 100644 --- a/src/domain/repositories/contact_repository.rs +++ b/src/domain/repositories/contact_repository.rs @@ -16,10 +16,27 @@ pub trait ContactRepository: Send + Sync + 'static { address_book_id: &Uuid, uid: &str, ) -> ContactRepositoryResult>; + /// Fetches the contacts matching any of the given vCard UIDs in one + /// indexed query (`uid = ANY(...)`). Used by CardDAV multiget so a + /// request for a handful of contacts never pays for the whole book. + /// UIDs with no matching contact are silently absent from the result. + async fn get_contacts_by_uids( + &self, + address_book_id: &Uuid, + uids: &[String], + ) -> ContactRepositoryResult>; async fn get_contacts_by_address_book( &self, address_book_id: &Uuid, ) -> ContactRepositoryResult>; + /// Same as [`Self::get_contacts_by_address_book`] but bounded by + /// `LIMIT`/`OFFSET` for paginated listings. + async fn get_contacts_by_address_book_paginated( + &self, + address_book_id: &Uuid, + limit: i64, + offset: i64, + ) -> ContactRepositoryResult>; async fn get_contacts_by_email(&self, email: &str) -> ContactRepositoryResult>; async fn get_contacts_by_group(&self, group_id: &Uuid) -> ContactRepositoryResult>; diff --git a/src/infrastructure/adapters/calendar_storage_adapter.rs b/src/infrastructure/adapters/calendar_storage_adapter.rs index e37c3ac5..346ba487 100644 --- a/src/infrastructure/adapters/calendar_storage_adapter.rs +++ b/src/infrastructure/adapters/calendar_storage_adapter.rs @@ -422,6 +422,26 @@ impl CalendarStoragePort for CalendarStorageAdapter { Ok(event.map(CalendarEventDto::from)) } + async fn find_events_by_ical_uids( + &self, + calendar_id: &str, + ical_uids: &[String], + ) -> Result, DomainError> { + let uuid = Uuid::parse_str(calendar_id).map_err(|_| { + DomainError::new( + ErrorKind::InvalidInput, + "Calendar", + "Invalid calendar ID format", + ) + })?; + + let events = self + .event_repository + .find_events_by_ical_uids(&uuid, ical_uids) + .await?; + Ok(events.into_iter().map(CalendarEventDto::from).collect()) + } + async fn list_events_by_calendar( &self, calendar_id: &str, diff --git a/src/infrastructure/adapters/contact_storage_adapter.rs b/src/infrastructure/adapters/contact_storage_adapter.rs index 1f8a06d6..1beece32 100644 --- a/src/infrastructure/adapters/contact_storage_adapter.rs +++ b/src/infrastructure/adapters/contact_storage_adapter.rs @@ -730,9 +730,10 @@ impl ContactUseCase for ContactStorageAdapter { Ok(contact.map(ContactDto::from)) } - async fn list_contacts( + async fn get_contacts_by_uids( &self, address_book_id: &str, + uids: &[String], user_id: Uuid, ) -> Result, DomainError> { let uuid = Self::parse_uuid(address_book_id, "AddressBook")?; @@ -740,13 +741,43 @@ impl ContactUseCase for ContactStorageAdapter { // Check read access self.check_address_book_access(&uuid, user_id).await?; + if uids.is_empty() { + return Ok(Vec::new()); + } + let contacts = self .contact_repository - .get_contacts_by_address_book(&uuid) + .get_contacts_by_uids(&uuid, uids) .await?; Ok(contacts.into_iter().map(ContactDto::from).collect()) } + async fn list_contacts( + &self, + address_book_id: &str, + limit: Option, + offset: Option, + user_id: Uuid, + ) -> Result, DomainError> { + let uuid = Self::parse_uuid(address_book_id, "AddressBook")?; + + // Check read access + self.check_address_book_access(&uuid, user_id).await?; + + let contacts = if limit.is_some() || offset.is_some() { + let limit = limit.unwrap_or(100); + let offset = offset.unwrap_or(0); + self.contact_repository + .get_contacts_by_address_book_paginated(&uuid, limit, offset) + .await? + } else { + self.contact_repository + .get_contacts_by_address_book(&uuid) + .await? + }; + Ok(contacts.into_iter().map(ContactDto::from).collect()) + } + async fn search_contacts( &self, address_book_id: &str, diff --git a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs index 8af1a991..8d5cc81e 100644 --- a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs +++ b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs @@ -370,6 +370,56 @@ impl CalendarEventRepository for CalendarEventPgRepository { } } + async fn find_events_by_ical_uids( + &self, + calendar_id: &Uuid, + ical_uids: &[String], + ) -> CalendarEventRepositoryResult> { + let rows = sqlx::query( + r#" + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data + FROM caldav.calendar_events + WHERE calendar_id = $1 AND ical_uid = ANY($2) + ORDER BY start_time + "#, + ) + .bind(calendar_id) + .bind(ical_uids) + .fetch_all(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!("Failed to get calendar events by UIDs: {}", e)) + })?; + + let mut events = Vec::new(); + for row in rows { + let event = CalendarEvent::with_id( + row.get("id"), + row.get("calendar_id"), + row.get("summary"), + row.get::, _>("description"), + row.get::, _>("location"), + row.get("start_time"), + row.get("end_time"), + row.get("all_day"), + row.get::, _>("rrule"), + row.get("ical_uid"), + row.get("ical_data"), + row.get("created_at"), + row.get("updated_at"), + ) + .map_err(|e| { + DomainError::database_error(format!("Error creating calendar event: {}", e)) + })?; + events.push(event); + } + + Ok(events) + } + async fn count_events_in_calendar( &self, calendar_id: &Uuid, diff --git a/src/infrastructure/repositories/pg/contact_pg_repository.rs b/src/infrastructure/repositories/pg/contact_pg_repository.rs index da145b87..02ea9008 100644 --- a/src/infrastructure/repositories/pg/contact_pg_repository.rs +++ b/src/infrastructure/repositories/pg/contact_pg_repository.rs @@ -247,13 +247,44 @@ impl ContactRepository for ContactPgRepository { } } + async fn get_contacts_by_uids( + &self, + address_book_id: &Uuid, + uids: &[String], + ) -> ContactRepositoryResult> { + let rows = sqlx::query( + r#" + SELECT + id, address_book_id, uid, full_name, first_name, last_name, nickname, + email, phone, address, organization, title, notes, photo_url, + birthday, anniversary, vcard, etag, created_at, updated_at + FROM carddav.contacts + WHERE address_book_id = $1 AND uid = ANY($2) + ORDER BY full_name, first_name, last_name + "#, + ) + .bind(address_book_id) + .bind(uids) + .fetch_all(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!("Failed to get contacts by uids: {}", e)) + })?; + + let mut contacts = Vec::new(); + for row in &rows { + contacts.push(Self::row_to_contact(row)?); + } + Ok(contacts) + } + async fn get_contacts_by_address_book( &self, address_book_id: &Uuid, ) -> ContactRepositoryResult> { let rows = sqlx::query( r#" - SELECT + SELECT id, address_book_id, uid, full_name, first_name, last_name, nickname, email, phone, address, organization, title, notes, photo_url, birthday, anniversary, vcard, etag, created_at, updated_at @@ -276,6 +307,43 @@ impl ContactRepository for ContactPgRepository { Ok(contacts) } + async fn get_contacts_by_address_book_paginated( + &self, + address_book_id: &Uuid, + limit: i64, + offset: i64, + ) -> ContactRepositoryResult> { + let rows = sqlx::query( + r#" + SELECT + id, address_book_id, uid, full_name, first_name, last_name, nickname, + email, phone, address, organization, title, notes, photo_url, + birthday, anniversary, vcard, etag, created_at, updated_at + FROM carddav.contacts + WHERE address_book_id = $1 + ORDER BY full_name, first_name, last_name + LIMIT $2 OFFSET $3 + "#, + ) + .bind(address_book_id) + .bind(limit) + .bind(offset) + .fetch_all(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!( + "Failed to get contacts by address book (paginated): {}", + e + )) + })?; + + let mut contacts = Vec::new(); + for row in &rows { + contacts.push(Self::row_to_contact(row)?); + } + Ok(contacts) + } + async fn get_contacts_by_email(&self, email: &str) -> ContactRepositoryResult> { let search_pattern = super::like_escape(email); diff --git a/src/interfaces/api/handlers/caldav_handler.rs b/src/interfaces/api/handlers/caldav_handler.rs index 98fded62..b6d6eb88 100644 --- a/src/interfaces/api/handlers/caldav_handler.rs +++ b/src/interfaces/api/handlers/caldav_handler.rs @@ -27,6 +27,7 @@ use std::fmt::Write; use std::sync::Arc; use crate::application::adapters::caldav_adapter::{CalDavAdapter, CalDavReportType}; +use crate::application::adapters::uid_from_multiget_href; use crate::application::adapters::webdav_adapter::{PropFindRequest, PropFindType}; use crate::application::dtos::calendar_dto::{ CreateCalendarDto, CreateEventICalDto, UpdateCalendarDto, @@ -422,17 +423,13 @@ async fn handle_propfind( } }; - // Individual event .ics + // Individual event .ics — indexed lookup by iCalendar UID. let ical_uid = event_path.trim_end_matches(".ics"); - let events = calendar_service - .list_events(calendar_id, None, None, user.id) + let event = calendar_service + .get_event_by_ical_uid(calendar_id, ical_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?; - - let event = events - .iter() - .find(|e| e.ical_uid == ical_uid) + .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))? .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?; let base_href = &format!("/caldav/{}/", calendar_id); @@ -444,7 +441,7 @@ async fn handle_propfind( let mut response_body = Vec::new(); CalDavAdapter::generate_calendar_events_response( &mut response_body, - std::slice::from_ref(event), + std::slice::from_ref(&event), &report_type, base_href, ) @@ -502,15 +499,18 @@ async fn handle_report( } } CalDavReportType::CalendarMultiget { hrefs, .. } => { - let all_events = calendar_service - .list_events(calendar_id, None, None, user.id) - .await - .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?; + // Indexed batch lookup (`ical_uid = ANY(...)`) — a multiget for + // a handful of events must not pay for listing the whole + // calendar and filtering client-side. + let uids: Vec = hrefs + .iter() + .filter_map(|href| uid_from_multiget_href(href, ".ics")) + .collect(); - all_events - .into_iter() - .filter(|evt| hrefs.iter().any(|href| href.contains(&evt.ical_uid))) - .collect() + calendar_service + .get_events_by_ical_uids(calendar_id, &uids, user.id) + .await + .map_err(|e| AppError::internal_error(format!("Failed to fetch events: {}", e)))? } CalDavReportType::SyncCollection { .. } => calendar_service .list_events(calendar_id, None, None, user.id) diff --git a/src/interfaces/api/handlers/carddav_handler.rs b/src/interfaces/api/handlers/carddav_handler.rs index 5b58c1fd..1f45b017 100644 --- a/src/interfaces/api/handlers/carddav_handler.rs +++ b/src/interfaces/api/handlers/carddav_handler.rs @@ -28,6 +28,7 @@ use std::sync::Arc; use crate::application::adapters::carddav_adapter::{ CardDavAdapter, CardDavReportType, contact_to_vcard, }; +use crate::application::adapters::uid_from_multiget_href; use crate::application::adapters::webdav_adapter::{PropFindRequest, PropFindType}; use crate::application::dtos::address_book_dto::{CreateAddressBookDto, UpdateAddressBookDto}; use crate::application::dtos::contact_dto::CreateContactVCardDto; @@ -269,7 +270,7 @@ async fn handle_propfind( let contacts = if depth != "0" { contact_svc - .list_contacts(address_book_id, user.id) + .list_contacts(address_book_id, None, None, user.id) .await .unwrap_or_default() } else { @@ -359,22 +360,25 @@ async fn handle_report( let contacts = match &report { CardDavReportType::AddressbookQuery { .. } => contact_svc - .list_contacts(address_book_id, user.id) + .list_contacts(address_book_id, None, None, user.id) .await .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?, CardDavReportType::AddressbookMultiget { hrefs, .. } => { - let all_contacts = contact_svc - .list_contacts(address_book_id, user.id) - .await - .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?; + // Indexed batch lookup (`uid = ANY(...)`) — a multiget for a + // handful of contacts must not pay for listing the whole + // address book and filtering client-side. + let uids: Vec = hrefs + .iter() + .filter_map(|href| uid_from_multiget_href(href, ".vcf")) + .collect(); - all_contacts - .into_iter() - .filter(|c| hrefs.iter().any(|href| href.contains(&c.uid))) - .collect() + contact_svc + .get_contacts_by_uids(address_book_id, &uids, user.id) + .await + .map_err(|e| AppError::internal_error(format!("Failed to fetch contacts: {}", e)))? } CardDavReportType::SyncCollection { .. } => contact_svc - .list_contacts(address_book_id, user.id) + .list_contacts(address_book_id, None, None, user.id) .await .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?, }; @@ -559,7 +563,7 @@ async fn handle_get( if parts.len() < 2 { // GET on address book collection — return all contacts as vcf let contacts = contact_svc - .list_contacts(address_book_id, user.id) + .list_contacts(address_book_id, None, None, user.id) .await .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?; diff --git a/src/interfaces/api/handlers/contacts_handler.rs b/src/interfaces/api/handlers/contacts_handler.rs index e7f68641..daa63b55 100644 --- a/src/interfaces/api/handlers/contacts_handler.rs +++ b/src/interfaces/api/handlers/contacts_handler.rs @@ -119,16 +119,14 @@ pub struct AddMemberRequest { } /// Query parameters for paginated listing. +/// +/// Both fields are optional: when omitted, regular address books return +/// the full contact list (the frontend relies on this), while the system +/// book falls back to its own defaults (limit 100, offset 0). #[derive(Deserialize)] pub struct ListQuery { - #[serde(default = "default_limit")] - limit: i64, - #[serde(default)] - offset: i64, -} - -fn default_limit() -> i64 { - 100 + limit: Option, + offset: Option, } // ── Helpers ────────────────────────────────────────────────────────────────── @@ -455,8 +453,8 @@ pub async fn delete_address_book( path = "/api/address-books/{book_id}/contacts", params( ("book_id" = String, Path, description = "Address book UUID or \"system\""), - ("limit" = Option, Query, description = "Max results (default 100)"), - ("offset" = Option, Query, description = "Pagination offset (default 0)"), + ("limit" = Option, Query, description = "Max results (omit for the full book; system book defaults to 100)"), + ("offset" = Option, Query, description = "Pagination offset (omit for none; system book defaults to 0)"), ), responses( (status = 200, description = "List of contacts"), @@ -488,7 +486,10 @@ pub async fn list_contacts( return e.into_response(); } let caller_id = auth_user.id.to_string(); - match auth_service.list_users(params.limit, params.offset).await { + match auth_service + .list_users(params.limit.unwrap_or(100), params.offset.unwrap_or(0)) + .await + { Ok(users) => { let contacts: Vec = users .into_iter() @@ -505,7 +506,7 @@ pub async fn list_contacts( } else { match state .contact_service - .list_contacts(&book_id, auth_user.id) + .list_contacts(&book_id, params.limit, params.offset, auth_user.id) .await { Ok(contacts) => (StatusCode::OK, Json(contacts)).into_response(), From a3a2d2f1cf6df094d02211b9aacdb3039129c8a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 09:38:35 +0000 Subject: [PATCH 2/5] Cut recurring Argon2 cost and remove per-lock WebDAV timers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hot-path fixes for DAV sync clients, which poll continuously: 1. App-password Basic Auth cache TTL 30s → 300s. Every cache miss costs a full Argon2id verification (~50-100ms CPU) plus two DB queries; at 30s a continuously-syncing client re-paid that every 30s. 5 min cuts it ~10x. Safe because revoke() already invalidates the user's cached entries immediately; expiry/deactivation are only re-checked on a miss, so they now have a <=5 min grace window (comparable to a JWT access-token lifetime) — documented on the constant. 2. WebDAV lock store: replace the tokio::spawn + sleep scheduled per acquire/refresh with Moka's per-entry Expiry policy. Office clients refresh locks constantly, leaving thousands of orphaned sleeping timers pinned in the runtime that were never cancelled. by_path now carries the exact per-lock TTL via LockExpiry (no background tasks); by_token keeps a 24h backstop and resolves through by_path, so a lingering reverse-index entry can never resurrect an expired lock. Adds unit tests for the expiry policy and acquire/refresh/release (the module had none). https://claude.ai/code/session_0193Hff42gaA962wThxMGSd1 --- .../services/app_password_service.rs | 28 +- .../services/webdav_lock_service.rs | 243 ++++++++++++++---- 2 files changed, 214 insertions(+), 57 deletions(-) diff --git a/src/application/services/app_password_service.rs b/src/application/services/app_password_service.rs index ee02434d..e603f65a 100644 --- a/src/application/services/app_password_service.rs +++ b/src/application/services/app_password_service.rs @@ -30,9 +30,25 @@ const NC_APP_PASSWORD_GROUP_LEN: usize = 5; const NC_PREFIX_LEN: usize = 8; /// TTL for cached Basic Auth verification results. -/// Balances performance (avoids repeated Argon2id + DB queries) with security -/// (limits the window during which a revoked app password remains usable). -const BASIC_AUTH_CACHE_TTL_SECS: u64 = 30; +/// +/// DAV sync clients (Nautilus, Windows Explorer, Apple Calendar, …) poll +/// continuously, and every cache miss costs a full Argon2id verification +/// (~50–100 ms of CPU) plus two DB round-trips. A 30 s TTL re-paid that +/// cost every 30 s per client; 5 min cuts it ~10× under steady sync load. +/// +/// Security envelope of this window: +/// - **Revocation is immediate**: `revoke()` calls `invalidate_entries_if` +/// on this cache for the user, so a revoked password never survives in +/// cache regardless of TTL. +/// - **Expiry / deactivation are bounded by the TTL**: `expires_at` and +/// `user.is_active()` are only re-checked on a cache *miss* (the DB +/// query filters them), so an app password that expires — or a user +/// deactivated via `set_user_active` — may keep authenticating from +/// cache for at most this long. 5 min is comparable to a typical JWT +/// access-token lifetime, so the grace window is consistent across +/// auth surfaces. Lengthen with care; shorten if a tighter bound on +/// post-deactivation access is required. +const BASIC_AUTH_CACHE_TTL_SECS: u64 = 300; /// Maximum number of cached Basic Auth verifications. /// Each entry is ~160 bytes (32-byte key + 4 small strings), so 10 000 @@ -61,9 +77,9 @@ pub struct AppPasswordService { /// /// **Value**: the authenticated identity (user_id, username, email, role). /// - /// **Eviction**: TTL-based (30 s) + capacity-based (10 000 entries). - /// Failed verifications are *never* cached, so brute-force attackers - /// always pay the full Argon2id cost. + /// **Eviction**: TTL-based (see `BASIC_AUTH_CACHE_TTL_SECS`) + + /// capacity-based (10 000 entries). Failed verifications are *never* + /// cached, so brute-force attackers always pay the full Argon2id cost. auth_cache: Cache<[u8; 32], CachedBasicAuthResult>, } diff --git a/src/infrastructure/services/webdav_lock_service.rs b/src/infrastructure/services/webdav_lock_service.rs index efcfaf48..2b4ee389 100644 --- a/src/infrastructure/services/webdav_lock_service.rs +++ b/src/infrastructure/services/webdav_lock_service.rs @@ -1,15 +1,23 @@ //! WebDAV lock store backed by Moka (in-memory cache with per-entry TTL). //! -//! Locks are automatically evicted when their timeout expires, preventing -//! orphaned locks from accumulating. Two caches are maintained: +//! Each lock expires automatically at its own RFC 4918 `Timeout`, enforced +//! by Moka's [`Expiry`](moka::Expiry) policy. There are **no background +//! tasks and no per-lock timers** — Office clients refresh locks +//! constantly, and spawning a `sleep` future per acquire/refresh used to +//! leave thousands of orphaned timers pinned in the runtime. Two caches are +//! maintained: //! -//! - `by_path` : path → `LockEntry` (for LOCK conflict detection) -//! - `by_token` : token → path (for fast UNLOCK / refresh lookups) +//! - `by_path` : path → `LockEntry` (source of truth; precise per-lock TTL) +//! - `by_token` : token → path (reverse index for UNLOCK / refresh) //! -//! Both caches share the same TTL so entries disappear together. +//! `by_path` carries the exact per-lock TTL via `Expiry`; `by_token` keeps a +//! 24 h backstop TTL. A reverse-index entry that outlives its lock is +//! harmless: every lookup resolves through `by_path`, which is +//! authoritative, so an expired lock reads as absent even before its token +//! mapping is evicted. use std::sync::Arc; -use std::time::Duration; +use std::time::{Duration, Instant}; use crate::application::adapters::webdav_adapter::{LockInfo, LockScope}; @@ -26,6 +34,40 @@ pub struct LockEntry { pub path: String, } +/// Per-entry expiration policy for the `by_path` cache. +/// +/// Moka calls this on insert (create) and re-insert (update, i.e. refresh) +/// to derive each lock's TTL from its own `Timeout` header — replacing the +/// old "global TTL + one spawned timer per lock" scheme. Reads do not +/// extend the lock (the default `expire_after_read` leaves the remaining +/// duration untouched). +struct LockExpiry; + +impl moka::Expiry for LockExpiry { + fn expire_after_create( + &self, + _path: &String, + entry: &LockEntry, + _created_at: Instant, + ) -> Option { + Some(WebDavLockStore::parse_timeout( + entry.info.timeout.as_deref(), + )) + } + + fn expire_after_update( + &self, + _path: &String, + entry: &LockEntry, + _updated_at: Instant, + _remaining: Option, + ) -> Option { + Some(WebDavLockStore::parse_timeout( + entry.info.timeout.as_deref(), + )) + } +} + /// In-memory WebDAV lock store with automatic TTL-based expiration. /// /// Uses Moka's `sync::Cache` — lock-free (sharded) reads, bounded size, @@ -42,13 +84,16 @@ impl WebDavLockStore { /// /// * `max_capacity` — upper bound on simultaneous locks (evicts LRU on overflow). pub fn new(max_capacity: u64) -> Self { - // We use `expire_after` (per-entry TTL) via insert with explicit ttl, - // so we configure a generous global time_to_live as a safety net. + // `by_path` is the source of truth: each lock expires at its own + // `Timeout` via the `LockExpiry` policy (no spawned timers). let by_path = moka::sync::Cache::builder() .max_capacity(max_capacity) - .time_to_live(Duration::from_secs(MAX_LOCK_TIMEOUT_SECS)) + .expire_after(LockExpiry) .build(); + // `by_token` is a reverse index; a 24 h backstop TTL bounds any + // mapping that outlives its lock. Lookups resolve through `by_path`, + // so a lingering entry here never resurrects an expired lock. let by_token = moka::sync::Cache::builder() .max_capacity(max_capacity) .time_to_live(Duration::from_secs(MAX_LOCK_TIMEOUT_SECS)) @@ -72,38 +117,17 @@ impl WebDavLockStore { return Err(existing); } - let ttl = Self::parse_timeout(info.timeout.as_deref()); let entry = LockEntry { info, path: path.to_owned(), }; + // `LockExpiry` derives the TTL from `entry.info.timeout` on insert — + // no spawned timer needed. self.by_path.insert(path.to_owned(), entry.clone()); self.by_token .insert(entry.info.token.clone(), path.to_owned()); - // Moka 0.12 does not expose per-entry set_expiration_after_insert at - // insert time. We rely on the global `time_to_live` as an upper bound - // and use the `invalidate_after` helper below for custom TTL. - // - // To implement shorter-than-max TTL we schedule an async invalidation. - if ttl.as_secs() < MAX_LOCK_TIMEOUT_SECS { - let by_path = self.by_path.clone(); - let by_token = self.by_token.clone(); - let token = entry.info.token.clone(); - let path_owned = path.to_owned(); - tokio::spawn(async move { - tokio::time::sleep(ttl).await; - // Only remove if the entry still matches (wasn't refreshed/replaced) - if let Some(e) = by_path.get(&path_owned) - && e.info.token == token - { - by_path.invalidate(&path_owned); - by_token.invalidate(&token); - } - }); - } - Ok(entry) } @@ -120,29 +144,13 @@ impl WebDavLockStore { } let ttl = Self::parse_timeout(new_timeout.or(entry.info.timeout.as_deref())); - let timeout_str = format!("Second-{}", ttl.as_secs()); - entry.info.timeout = Some(timeout_str.clone()); + // Normalize the stored timeout so `LockExpiry` recomputes the new TTL + // from it on re-insert (Moka fires `expire_after_update`). + entry.info.timeout = Some(format!("Second-{}", ttl.as_secs())); - // Re-insert to reset the TTL self.by_path.insert(path.clone(), entry.clone()); self.by_token.insert(token.to_owned(), path.clone()); - if ttl.as_secs() < MAX_LOCK_TIMEOUT_SECS { - let by_path = self.by_path.clone(); - let by_token = self.by_token.clone(); - let token_owned = token.to_owned(); - let path_owned = path.clone(); - tokio::spawn(async move { - tokio::time::sleep(ttl).await; - if let Some(e) = by_path.get(&path_owned) - && e.info.token == token_owned - { - by_path.invalidate(&path_owned); - by_token.invalidate(&token_owned); - } - }); - } - Some(entry) } @@ -212,3 +220,136 @@ pub fn create_webdav_lock_store() -> Arc { // if the cap is reached, so stale entries are cleaned automatically. Arc::new(WebDavLockStore::new(10_000)) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::application::adapters::webdav_adapter::LockType; + use moka::Expiry; + + fn lock_info(token: &str, timeout: Option<&str>, scope: LockScope) -> LockInfo { + LockInfo { + token: token.to_owned(), + owner: Some("tester".to_owned()), + depth: "0".to_owned(), + timeout: timeout.map(str::to_owned), + scope, + type_: LockType::Write, + } + } + + fn entry(token: &str, timeout: Option<&str>) -> LockEntry { + LockEntry { + info: lock_info(token, timeout, LockScope::Exclusive), + path: "/file.txt".to_owned(), + } + } + + #[test] + fn expiry_uses_per_entry_timeout() { + let now = Instant::now(); + let key = "/file.txt".to_owned(); + + // Explicit Second-NNN → that exact duration. + let e = entry("t", Some("Second-300")); + assert_eq!( + LockExpiry.expire_after_create(&key, &e, now), + Some(Duration::from_secs(300)) + ); + // Refresh path (update) recomputes from the (normalized) timeout. + assert_eq!( + LockExpiry.expire_after_update(&key, &e, now, None), + Some(Duration::from_secs(300)) + ); + } + + #[test] + fn expiry_clamps_infinite_and_defaults_none() { + let now = Instant::now(); + let key = "/file.txt".to_owned(); + + let infinite = entry("t", Some("Infinite")); + assert_eq!( + LockExpiry.expire_after_create(&key, &infinite, now), + Some(Duration::from_secs(MAX_LOCK_TIMEOUT_SECS)) + ); + + let none = entry("t", None); + assert_eq!( + LockExpiry.expire_after_create(&key, &none, now), + Some(Duration::from_secs(DEFAULT_LOCK_TIMEOUT_SECS)) + ); + + // Over-large requests are clamped to the maximum. + let huge = entry("t", Some("Second-999999999")); + assert_eq!( + LockExpiry.expire_after_create(&key, &huge, now), + Some(Duration::from_secs(MAX_LOCK_TIMEOUT_SECS)) + ); + } + + #[test] + fn acquire_get_release_roundtrip() { + let store = WebDavLockStore::new(16); + let info = lock_info("urn:token-1", Some("Second-600"), LockScope::Exclusive); + + let acquired = store.acquire("/a.txt", info).expect("acquire"); + assert_eq!(acquired.info.token, "urn:token-1"); + + // Resolvable by both indexes. + assert_eq!( + store.get_by_path("/a.txt").map(|e| e.info.token.clone()), + Some("urn:token-1".to_owned()) + ); + assert_eq!( + store.get_by_token("urn:token-1").map(|e| e.path.clone()), + Some("/a.txt".to_owned()) + ); + + assert!(store.release("urn:token-1")); + assert!(store.get_by_path("/a.txt").is_none()); + assert!(store.get_by_token("urn:token-1").is_none()); + // Releasing an unknown token reports nothing removed. + assert!(!store.release("urn:token-1")); + } + + #[test] + fn exclusive_lock_conflicts() { + let store = WebDavLockStore::new(16); + store + .acquire( + "/a.txt", + lock_info("urn:token-1", Some("Second-600"), LockScope::Exclusive), + ) + .expect("first acquire"); + + let conflict = store.acquire( + "/a.txt", + lock_info("urn:token-2", Some("Second-600"), LockScope::Exclusive), + ); + assert!(conflict.is_err()); + // The original holder is returned so the caller can report it. + assert_eq!(conflict.unwrap_err().info.token, "urn:token-1"); + } + + #[test] + fn refresh_normalizes_timeout_and_keeps_lock() { + let store = WebDavLockStore::new(16); + store + .acquire( + "/a.txt", + lock_info("urn:token-1", Some("Infinite"), LockScope::Exclusive), + ) + .expect("acquire"); + + let refreshed = store + .refresh("urn:token-1", Some("Second-120")) + .expect("refresh"); + assert_eq!(refreshed.info.timeout.as_deref(), Some("Second-120")); + // Still present and still addressable by token. + assert!(store.get_by_token("urn:token-1").is_some()); + + // Refreshing an unknown token yields None. + assert!(store.refresh("urn:unknown", Some("Second-120")).is_none()); + } +} From ecbdaee19e443e16900a17fd5fb8b4ab6829f6eb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 09:58:46 +0000 Subject: [PATCH 3/5] Frontend perf: pdf.js smart preload, admin poll cleanup, list render hoisting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three targeted frontend fixes: 1. pdf.js smart preload (thumbnail.js, resourceIcon.js). The first PDF thumbnail of a session stalled 1-2s on the lazy import of the ~1.3 MB pdf.js stack. buildResourceIcon() now fires thumbnail.preloadPdf() the moment a PDF row enters the DOM, warming both the module (~300 KB, via the now promise-memoized getPdfjsLib, shared with real users) and the worker script (~1 MB, via a cache-priming fetch — pdf.js only requests it on first getDocument). Only folders that actually contain PDFs pay the download; idempotent after first call, resets on failure so transient offline retries. 2. Admin migration poll cleanup (admin.js). The 2s setInterval kept hitting the API and updating hidden DOM after leaving the Storage tab, and polled a failing endpoint forever after session expiry (!resp.ok returned without clearing). New stopMigrationPolling() helper, invoked on tab switch away from Storage, on non-running status, and on failed polls; tab re-entry re-arms via loadStorage(). 3. resourceList.js render hoisting. Per-row i18n.t() type-cell lookups and the fully item-invariant _renderCustomActions() HTML were recomputed for every row; they now resolve once per batch via _buildItemLabels() (per-category labels memoized, rebuilt each batch so locale switches keep working). _findLaneByKey() swaps the container-wide attribute querySelector for an O(1) _lanes Map kept in sync at the only lane create/wipe sites. https://claude.ai/code/session_0193Hff42gaA962wThxMGSd1 --- static/js/components/resourceIcon.js | 7 +++ static/js/components/resourceList.js | 93 +++++++++++++++++++++++----- static/js/features/thumbnail.js | 71 +++++++++++++++++---- static/js/views/admin/admin.js | 32 ++++++++-- 4 files changed, 170 insertions(+), 33 deletions(-) diff --git a/static/js/components/resourceIcon.js b/static/js/components/resourceIcon.js index ec0a797d..6f50d790 100644 --- a/static/js/components/resourceIcon.js +++ b/static/js/components/resourceIcon.js @@ -36,6 +36,13 @@ function buildResourceIcon(item, resourceType) { const canThumbnail = thumbnail?.canHandle(file) ?? false; if (canThumbnail) { + // A PDF just entered the list: warm up the pdf.js stack (~1.3 MB) + // in the background now, so a thumbnail cache-miss below doesn't + // stall its first render on the library download. Idempotent. + if (file.mime_type === 'application/pdf') { + thumbnail.preloadPdf(); + } + const img = document.createElement('img'); img.className = 'file-thumb'; img.src = `/api/files/${file.id}/thumbnail/icon`; diff --git a/static/js/components/resourceList.js b/static/js/components/resourceList.js index 73b633fc..63c82380 100644 --- a/static/js/components/resourceList.js +++ b/static/js/components/resourceList.js @@ -140,6 +140,18 @@ export class ResourceListComponent { */ this._lastGroupEl = null; + /** + * Live swimlane wrappers currently in the DOM, keyed by group key — + * lets `_findLaneByKey()` resolve in O(1) instead of a container-wide + * `querySelector` per lookup. Kept in sync with the DOM: entries are + * added where lanes are created (`_appendItems`, + * `_ensureJustAddedLane`) and the map is cleared on the full-container + * wipes in `render()` / `clear()`; lanes are never removed + * individually anywhere else. + * @type {Map} + */ + this._lanes = new Map(); + /** * Optional grouping-key resolver stored between `render()` / `append()` * calls so `addItem()` can place a new row in the correct swimlane @@ -203,6 +215,7 @@ export class ResourceListComponent { // Reset group tracking for the fresh render this._lastGroupKey = undefined; this._lastGroupEl = null; + this._lanes.clear(); this._groupFn = groupFn; this._groupLabelFn = groupLabelFn; this._headerNodeFn = headerNodeFn; @@ -244,6 +257,7 @@ export class ResourceListComponent { this._lastClickedIndex = -1; this._lastGroupKey = undefined; this._lastGroupEl = null; + this._lanes.clear(); this._groupFn = undefined; this._groupLabelFn = undefined; this._headerNodeFn = undefined; @@ -348,7 +362,8 @@ export class ResourceListComponent { // "New" swimlane, creating it on first call. const lane = this._ensureJustAddedLane(); this._items.set(item.id, item); - row = isFile ? this._createFileItem(/** @type {FileItem} */ (item)) : this._createFolderItem(/** @type {FolderItem} */ (item)); + const labels = this._buildItemLabels(); + row = isFile ? this._createFileItem(/** @type {FileItem} */ (item), labels) : this._createFolderItem(/** @type {FolderItem} */ (item), labels); lane.appendChild(row); } else { // Flat list (no grouping) — append at the end like before. @@ -394,6 +409,7 @@ export class ResourceListComponent { const lane = document.createElement('div'); lane.className = 'resource-list__swimlane-group resource-list__swimlane-group--just-added'; lane.dataset.groupKey = JUST_ADDED_KEY; + this._lanes.set(JUST_ADDED_KEY, lane); const header = document.createElement('div'); header.className = 'resource-list__swimlane-header'; @@ -420,13 +436,14 @@ export class ResourceListComponent { * Locate an on-screen swimlane wrapper by its group key. Returns * `null` when no swimlane currently matches. * + * O(1) via the `_lanes` registry — see its declaration for how it is + * kept in sync with the DOM. + * * @param {string} key * @returns {HTMLElement | null} */ _findLaneByKey(key) { - // CSS.escape covers arbitrary key shapes (dates with colons, - // UUIDs with dashes, etc.) so the attribute selector is safe. - return /** @type {HTMLElement | null} */ (this._container.querySelector(`.resource-list__swimlane-group[data-group-key="${CSS.escape(String(key))}"]`)); + return this._lanes.get(String(key)) ?? null; } /** @@ -523,6 +540,9 @@ export class ResourceListComponent { _appendItems(items, groupFn, groupLabelFn, headerNodeFn) { const fragment = document.createDocumentFragment(); + // Resolve batch-invariant labels once, not once per row. + const labels = this._buildItemLabels(); + // Start from the persisted key so load-more pages continue seamlessly. let lastGroupKey = this._lastGroupKey; @@ -545,11 +565,12 @@ export class ResourceListComponent { if (key !== null) { fragmentGroup = document.createElement('div'); fragmentGroup.className = 'resource-list__swimlane-group'; - // Stamp the group key on the wrapper so `addItem()` - // can locate this swimlane later via - // `_findLaneByKey()` and append into it without a - // full re-render. + // Stamp the group key on the wrapper (handy in + // devtools) and register it in `_lanes` so + // `_findLaneByKey()` can locate this swimlane later + // without a container-wide query. fragmentGroup.dataset.groupKey = key; + this._lanes.set(key, fragmentGroup); fragmentGroup.appendChild(this._createGroupHeader(key, groupLabelFn, headerNodeFn)); fragment.appendChild(fragmentGroup); } @@ -558,7 +579,9 @@ export class ResourceListComponent { // Dispatch to the correct renderer: files have mime_type, folders do not. const isFile = 'mime_type' in item; - const itemEl = isFile ? this._createFileItem(/** @type {FileItem} */ (item)) : this._createFolderItem(/** @type {FolderItem} */ (item)); + const itemEl = isFile + ? this._createFileItem(/** @type {FileItem} */ (item), labels) + : this._createFolderItem(/** @type {FolderItem} */ (item), labels); // Priority: live DOM group (load-more continuation) > current fragment group > bare container const target = liveGroup ?? fragmentGroup; @@ -600,12 +623,50 @@ export class ResourceListComponent { return el; } + /** + * @typedef {Object} ItemLabels + * @property {string} folderTypeLabel - Type-cell label for folders. + * @property {string} customActionsHtml - Pre-rendered inline-action buttons. + * @property {(category: string) => string} fileTypeLabel - Type-cell label + * for a file category (memoized per batch). + */ + + /** + * Resolve every per-row value that does not depend on the item once per + * batch: the i18n lookups for the type cell and the custom-actions HTML + * are identical for all 50 rows of a page, so repeating them in + * `_createFileItem` / `_createFolderItem` was pure overhead. Built fresh + * on every call (never cached on the instance), so a locale switch is + * picked up naturally by the next render/append. + * + * @returns {ItemLabels} + */ + _buildItemLabels() { + const fallbackTypeLabel = i18n.t('files.file_types.document'); + /** @type {Map} */ + const byCategory = new Map(); + return { + folderTypeLabel: i18n.t('files.file_types.folder'), + customActionsHtml: this._renderCustomActions(), + fileTypeLabel(category) { + if (!category) return fallbackTypeLabel; + let label = byCategory.get(category); + if (label === undefined) { + label = i18n.t(`files.file_types.${category.toLowerCase()}`) || category; + byCategory.set(category, label); + } + return label; + } + }; + } + /** * Build a .file-item DOM element for a folder. * @param {FolderItem} folder + * @param {ItemLabels} labels - Batch-invariant labels from `_buildItemLabels()`. * @returns {HTMLElement} */ - _createFolderItem(folder) { + _createFolderItem(folder, labels) { const cfg = this._cfg; const el = document.createElement('div'); const modClass = cfg.itemModifierClass ? ` ${cfg.itemModifierClass}` : ''; @@ -632,11 +693,11 @@ export class ResourceListComponent {
${cfg.showPath ? `
${escapeHtml(folder.path || '')}
` : ''} - ${cfg.showType ? `
${i18n.t('files.file_types.folder')}
` : ''} + ${cfg.showType ? `
${labels.folderTypeLabel}
` : ''}
--
${formattedDate}
- ${this._renderCustomActions()} + ${labels.customActionsHtml} ${cfg.showFavorite ? `` : ''} ${cfg.showContextMenu ? '' : ''}
@@ -649,12 +710,12 @@ export class ResourceListComponent { /** * Build a .file-item DOM element for a file. * @param {FileItem} file + * @param {ItemLabels} labels - Batch-invariant labels from `_buildItemLabels()`. * @returns {HTMLElement} */ - _createFileItem(file) { + _createFileItem(file, labels) { const cfg = this._cfg; - const cat = file.category || ''; - const typeLabel = cat ? i18n.t(`files.file_types.${cat.toLowerCase()}`) || cat : i18n.t('files.file_types.document'); + const typeLabel = labels.fileTypeLabel(file.category || ''); const fileSize = file.size_formatted || formatFileSize(file.size); const dateVal = /** @type {Record} */ (/** @type {unknown} */ (file))[cfg.dateField] ?? file.modified_at; const formattedDate = cfg.dateFormatter ? cfg.dateFormatter(dateVal) : formatDateTime(new Date(dateVal)); @@ -685,7 +746,7 @@ export class ResourceListComponent {
${fileSize}
${formattedDate}
- ${this._renderCustomActions()} + ${labels.customActionsHtml} ${cfg.showFavorite ? `` : ''} ${cfg.showContextMenu ? '' : ''}
diff --git a/static/js/features/thumbnail.js b/static/js/features/thumbnail.js index 17f23922..c0aa0ea3 100644 --- a/static/js/features/thumbnail.js +++ b/static/js/features/thumbnail.js @@ -2,29 +2,47 @@ import { getCsrfHeaders } from '../core/csrf.js'; /** @import {FileItem} from '../core/types.js' */ +// IMPORTANT: absolute paths so the dynamic import resolves correctly both in +// dev mode (native ESM, module at /js/features/thumbnail.js) and in release +// mode (IIFE bundle at /js/app.{hash}.js — relative '../vendors/…' would +// incorrectly resolve to /vendors/… instead of /js/vendors/…). +const PDFJS_LIB_URL = '/js/vendors/pdf.min.mjs'; +const PDFJS_WORKER_URL = '/js/vendors/pdf.worker.min.mjs'; + /** - * use any type so tsc will not scan library - * @type {any} + * Memoized import of pdf.min.mjs (in-flight or settled). + * use any type so tsc will not scan library. + * Reset to null on failure so a later call retries (e.g. transient offline). + * @type {Promise | null} */ -let _pdfjsLib = null; +let _pdfjsLibPromise = null; + +/** True once the worker script warm-up fetch has completed successfully. */ +let _pdfWorkerWarmed = false; // TODO: do we need to add a max concurrncy ? /** * Lazy-loads pdf.min.mjs on first use via dynamic import so it is never * bundled into the IIFE (it uses top-level await which breaks IIFE wrapping). + * Memoizing the promise (rather than the resolved module) lets concurrent + * callers — e.g. `preloadPdf()` racing the first real thumbnail — share a + * single network fetch. * @returns {Promise} */ -async function getPdfjsLib() { - if (_pdfjsLib) return _pdfjsLib; - // IMPORTANT: use an absolute path so the import resolves correctly both in - // dev mode (native ESM, module at /js/features/thumbnail.js) and in release - // mode (IIFE bundle at /js/app.{hash}.js — relative '../vendors/…' would - // incorrectly resolve to /vendors/… instead of /js/vendors/…). - const lib = '/js/vendors/pdf.min.mjs'; - _pdfjsLib = /** @type {any} */ (await import(lib)); - _pdfjsLib.GlobalWorkerOptions.workerSrc = '/js/vendors/pdf.worker.min.mjs'; - return _pdfjsLib; +function getPdfjsLib() { + if (!_pdfjsLibPromise) { + _pdfjsLibPromise = import(PDFJS_LIB_URL) + .then((lib) => { + lib.GlobalWorkerOptions.workerSrc = PDFJS_WORKER_URL; + return lib; + }) + .catch((err) => { + _pdfjsLibPromise = null; // allow retry after a failed load + throw err; + }); + } + return _pdfjsLibPromise; } export const thumbnail = { @@ -43,6 +61,33 @@ export const thumbnail = { return false; }, + /** + * Fire-and-forget warm-up of the pdf.js stack (module + worker script). + * + * Called the moment a PDF row enters the DOM (see resourceIcon.js), so + * the ~1.3 MB library downloads in the background while the user is + * still looking at the list — instead of stalling the first thumbnail + * render on it. Idempotent and cheap after the first call, and only + * folders that actually contain PDFs ever pay the download. + */ + preloadPdf() { + // Module (≈300 KB): shares the memoized promise with real users. + getPdfjsLib().catch(() => { + /* transient failure — the first real use retries */ + }); + + // Worker (≈1 MB): pdf.js only fetches it via `new Worker(...)` on the + // first getDocument(), so prime the HTTP cache with a plain fetch. + // Reading the body ensures the download completes and is cacheable. + if (_pdfWorkerWarmed) return; + _pdfWorkerWarmed = true; + fetch(PDFJS_WORKER_URL) + .then((r) => (r.ok ? r.blob() : Promise.reject(new Error(`HTTP ${r.status}`)))) + .catch(() => { + _pdfWorkerWarmed = false; // allow retry on a later sighting + }); + }, + // TODO: use these informations from server ? SIZES: { icon: { width: 150, height: 150 }, diff --git a/static/js/views/admin/admin.js b/static/js/views/admin/admin.js index 6bcd5843..6a3330d6 100644 --- a/static/js/views/admin/admin.js +++ b/static/js/views/admin/admin.js @@ -151,6 +151,10 @@ function switchTab(name, el) { } activeTabName = name; + // The migration auto-poll only makes sense while the Storage tab is + // visible — without this it would keep hitting the API every 2 s + // (and updating hidden DOM) for as long as a migration runs. + if (name !== 'storage') stopMigrationPolling(); if (name === 'users') loadUsers(); if (name === 'dashboard') loadDashboard(); if (name === 'storage') loadStorage(); @@ -908,6 +912,22 @@ async function testStorageConnection() { /** @type {ReturnType | null} */ let migrationPollTimer = null; +/** + * Stop the 2 s migration auto-poll if it is armed. + * + * Called when the poll observes a non-running status, when a poll request + * fails (an expired admin session would otherwise be retried every 2 s + * forever), and when the user leaves the Storage tab. Re-entering the tab + * re-arms it via `loadStorage()` → `loadMigrationStatus()` while a + * migration is running. + */ +function stopMigrationPolling() { + if (migrationPollTimer) { + clearInterval(migrationPollTimer); + migrationPollTimer = null; + } +} + /** * @param {string} msg * @param {string} type @@ -979,7 +999,12 @@ async function loadMigrationStatus() { headers: headers(), credentials: 'same-origin' }); - if (!resp.ok) return; + if (!resp.ok) { + // Don't keep hammering a failing endpoint (e.g. expired session); + // any migration button or tab re-entry re-arms the poll. + stopMigrationPolling(); + return; + } const m = await resp.json(); updateMigrationUI(m); @@ -988,9 +1013,8 @@ async function loadMigrationStatus() { if (!migrationPollTimer) { migrationPollTimer = setInterval(loadMigrationStatus, 2000); } - } else if (migrationPollTimer) { - clearInterval(migrationPollTimer); - migrationPollTimer = null; + } else { + stopMigrationPolling(); } } catch (_e) { /* ignore */ From 54c494419c1441642d052954fa37c7f24075c59c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 10:43:45 +0000 Subject: [PATCH 4/5] Re-chunk pre-CDC legacy blobs into CDC manifests at startup Files uploaded before chunk_manifests landed (20260414000000) are stored as ONE whole-file blob with no manifest. Every legacy fallback in DedupService exists to serve them, and the cost concentrates on Range reads: with encryption enabled, seeking inside a legacy video decrypts the ENTIRE blob (AES-GCM is all-or-nothing), where a CDC file decrypts only the overlapping <=1 MiB chunks. This adds a one-time, idempotent background migration (spawned from the composition root after dedup init, maintenance pool) that converts each legacy blob into a regular CDC file, indistinguishable from a native upload: 1. Spool the blob through the normal read path (decrypts when encryption is on) to a per-attempt-unique temp file, verifying BLAKE3 == hash; sizes come from the verified spool, never from the legacy storage.blobs.size column (the manifest's total_size drives Range arithmetic). 2. CDC-chunk + store chunks via the existing store_chunks (one manifest reference per distinct chunk). 3. One short accounting TX with the blob row locked: manifest INSERT with ref_count = N current file references, blob ref_count -= N, row deleted only at exactly 0 - so single-chunk files (chunk hash == file hash) keep the physical blob, which IS the chunk; only bookkeeping moves, no bytes are rewritten. 4. Physical whole-file blob deleted only when its row dropped. Races lean on the row lock: a concurrent identical upload landing a legacy reference after commit keeps the blob row alive and that file readable via the fallback (bounded space leak, never data loss); a crash between chunk store and the TX over-counts one file's chunk refs (also a bounded leak). Corrupt blobs (content != hash) are logged, counted, excluded from the sweep and left untouched, with a hard cap before aborting. Per-hash failures never block the sweep; manifests are the resumability marker, so a restart continues where it left off. The legacy read/write fallbacks stay in place as the safety net while a deployment converges; they can be deleted once fleets report "legacy re-chunk: nothing to do". Opt-out via OXICLOUD_LEGACY_RECHUNK=false (documented in example.env) for metered remote backends where the one-time re-read should be scheduled deliberately. Covered by five integration tests against real PostgreSQL (multi-chunk accounting + Range across a chunk boundary, single-chunk physical-blob preservation, corrupt-blob isolation, empty blob, and the full encrypted-backend roundtrip); they run concurrently, which also exercises the cross-sweep race handling. https://claude.ai/code/session_0193Hff42gaA962wThxMGSd1 --- example.env | 9 + src/common/config.rs | 15 + src/common/di.rs | 12 + src/infrastructure/services/dedup_service.rs | 747 +++++++++++++++++++ 4 files changed, 783 insertions(+) diff --git a/example.env b/example.env index 30c4093e..c1dffd63 100644 --- a/example.env +++ b/example.env @@ -87,6 +87,15 @@ OXICLOUD_SERVER_HOST=127.0.0.1 # fresher sync detection, higher = fewer background UPDATEs. Minimum: 100. #OXICLOUD_TREE_ETAG_FLUSH_MS=500 +# One-time startup migration that converts pre-CDC whole-file blobs (files +# uploaded before chunked dedup landed) into CDC chunk manifests, in the +# background on the maintenance pool. Fixes the legacy penalty where a Range +# read (video seek) reads — and with encryption, DECRYPTS — the entire blob. +# Idempotent; a no-op once no legacy blobs remain. Disable only on metered +# remote backends (S3/Azure egress) where the one-time re-read of every +# legacy blob should be scheduled deliberately, e.g. off-peak. +#OXICLOUD_LEGACY_RECHUNK=true + # Allow multiple processes to bind to the same port (SO_REUSEPORT). # DISABLED by default — leaving this off means a second accidental instance # will fail immediately with "address already in use", which is the safe behaviour. diff --git a/src/common/config.rs b/src/common/config.rs index f620dad3..ffc89a93 100644 --- a/src/common/config.rs +++ b/src/common/config.rs @@ -254,6 +254,14 @@ pub struct StorageConfig { /// bound on how stale an ancestor folder's ETag can be after a change. /// Default: 500. Env: `OXICLOUD_TREE_ETAG_FLUSH_MS`. pub tree_etag_flush_ms: u64, + /// Startup background migration that re-chunks legacy whole-file blobs + /// (written before CDC chunking landed) into chunk manifests, so Range + /// reads stop paying a full-blob read — and, with encryption enabled, a + /// full-blob decrypt. Idempotent and incremental; a no-op (one COUNT + /// query) once no legacy blobs remain. Disable on metered remote + /// backends where the one-time re-read of every legacy blob should be + /// scheduled deliberately. Default: true. Env: `OXICLOUD_LEGACY_RECHUNK`. + pub legacy_rechunk_enabled: bool, /// Which blob storage backend to use (`local`, `s3`, or `azure`). pub backend: StorageBackendType, /// S3-compatible backend configuration (used when `backend == S3`). @@ -397,6 +405,7 @@ impl Default for StorageConfig { chunk_dir: None, usage_reconcile_secs: 600, // 10 minutes tree_etag_flush_ms: 500, + legacy_rechunk_enabled: true, backend: StorageBackendType::Local, s3: None, azure: None, @@ -1305,6 +1314,12 @@ impl AppConfig { config.storage.tree_etag_flush_ms = val; } + // Legacy whole-file blob re-chunk migration (startup background task) + if let Ok(enabled) = env::var("OXICLOUD_LEGACY_RECHUNK") { + config.storage.legacy_rechunk_enabled = + enabled.eq_ignore_ascii_case("true") || enabled == "1"; + } + // Storage backend selection if let Ok(backend) = env::var("OXICLOUD_STORAGE_BACKEND") { match backend.to_lowercase().as_str() { diff --git a/src/common/di.rs b/src/common/di.rs index 9e694792..08b483f0 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -319,6 +319,18 @@ impl AppServiceFactory { ); dedup_service.initialize().await?; + // One-time background migration: re-chunk pre-CDC whole-file blobs + // into chunk manifests so Range reads (and, with encryption, partial + // decrypts) stop paying for the entire blob. No-op once converged. + if self.config.storage.legacy_rechunk_enabled { + dedup_service.spawn_legacy_rechunk(); + } else { + tracing::info!( + "Legacy re-chunk migration disabled (OXICLOUD_LEGACY_RECHUNK=false) — \ + pre-CDC whole-file blobs, if any, will keep using the legacy read path" + ); + } + tracing::info!( "Core services initialized: path service, file content cache, thumbnails, chunked upload, image transcode, dedup (PRIMARY blob storage)" ); diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index 8b27dec7..2d06f6ec 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -1515,6 +1515,401 @@ impl DedupService { Ok((total_deleted, total_bytes)) } + + // ── Legacy whole-file blob re-chunk migration ──────────────── + // + // Files uploaded before CDC chunking landed (migration + // 20260414000000_chunk_manifests) are stored as ONE whole-file blob with + // no manifest. Every legacy fallback in this service exists to serve + // them — and with encryption enabled, a Range read of one decrypts the + // ENTIRE blob (AES-GCM is all-or-nothing per blob). + // + // This migration converts each legacy blob into a regular CDC file: + // after it, the converted file is indistinguishable from a native CDC + // upload, every read takes the chunked path, and the legacy fallbacks + // go permanently cold (they remain as the safety net while a deployment + // is mid-migration; they can be deleted from the codebase once fleets + // report `legacy re-chunk: nothing to do`). + // + // Per-hash algorithm: + // 1. Spool the blob to a temp file via the normal read path (this + // decrypts it when encryption is on), verifying BLAKE3 == hash. + // 2. CDC-chunk the spool + store chunks (`store_chunks` bumps each + // distinct chunk once — the manifest's reference). + // 3. One short accounting TX with the blob row locked: + // manifest INSERT with ref_count = N (current file rows referencing + // the hash), blob ref_count -= N (those references now live on the + // manifest), DELETE the blob row only if it hits exactly 0. + // 4. Physically delete the whole-file blob only when its row was + // removed. Single-chunk files (chunk hash == file hash) keep the + // physical blob — it IS the chunk; only the bookkeeping moves. + // + // Concurrency: the row lock serializes against the file-delete trigger + // and the legacy dedup-hit path. A racing identical upload can land one + // legacy reference after our commit; the blob row then survives (> 0) + // and that file stays readable through the legacy fallback — a bounded + // space leak, never data loss. A crash between step 2 and 3 leaks one + // +1 on that file's chunk refs (re-run re-bumps); also a bounded leak, + // never data loss. + + /// Count legacy whole-file blobs still referenced by at least one file + /// row (the migration's work queue). Runs on the maintenance pool. + pub async fn count_legacy_blobs(&self) -> Result { + sqlx::query_scalar( + "SELECT COUNT(*) FROM storage.blobs b + WHERE NOT EXISTS (SELECT 1 FROM storage.chunk_manifests m + WHERE m.file_hash = b.hash) + AND EXISTS (SELECT 1 FROM storage.files f + WHERE f.blob_hash = b.hash)", + ) + .fetch_one(self.maintenance_pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Count legacy blobs: {e}"))) + } + + /// Spawn the legacy re-chunk migration as a background task. + /// + /// Zero-cost when no legacy blobs exist (one COUNT query, debug log). + /// Called from the composition root after `initialize()`. + pub fn spawn_legacy_rechunk(self: &Arc) { + let svc = Arc::clone(self); + tokio::spawn(async move { + match svc.count_legacy_blobs().await { + Ok(0) => { + tracing::debug!("Legacy re-chunk: no legacy whole-file blobs — nothing to do"); + } + Ok(n) => { + tracing::info!( + "Legacy re-chunk: {n} pre-CDC whole-file blob(s) referenced by files — \ + starting background migration (maintenance pool)" + ); + match svc.rechunk_legacy_blobs().await { + Ok(report) => tracing::info!( + migrated = report.migrated, + failed = report.failed, + freed_bytes = report.freed_bytes, + "Legacy re-chunk complete: {} blob(s) converted to CDC manifests, \ + {} failed (left untouched), {} bytes of whole-file blobs freed", + report.migrated, + report.failed, + report.freed_bytes, + ), + Err(e) => tracing::error!("Legacy re-chunk aborted: {e}"), + } + } + Err(e) => tracing::error!("Legacy re-chunk: startup count failed: {e}"), + } + }); + } + + /// Convert every legacy whole-file blob into CDC chunks + manifest. + /// + /// Incremental and resumable: a manifest row is the per-hash "done" + /// marker, so re-running after a crash continues where it left off. + /// Per-hash failures (e.g. a corrupt blob that no longer matches its + /// hash) are logged, counted, and skipped — they never block the sweep. + pub async fn rechunk_legacy_blobs(&self) -> Result { + const BATCH_SIZE: i64 = 64; + /// Hard cap on per-hash failures before aborting the sweep — if + /// this many blobs are corrupt something is systemically wrong and + /// an operator should look before we touch anything else. + const MAX_FAILURES: usize = 1_000; + + let mut report = LegacyRechunkReport::default(); + // Failed hashes are excluded from the candidate query so a corrupt + // blob cannot make the sweep loop forever. + let mut failed_hashes: Vec = Vec::new(); + + loop { + let batch: Vec<(String, Option)> = sqlx::query_as( + "SELECT b.hash, b.content_type FROM storage.blobs b + WHERE NOT EXISTS (SELECT 1 FROM storage.chunk_manifests m + WHERE m.file_hash = b.hash) + AND EXISTS (SELECT 1 FROM storage.files f + WHERE f.blob_hash = b.hash) + AND NOT (b.hash = ANY($2)) + ORDER BY b.hash + LIMIT $1", + ) + .bind(BATCH_SIZE) + .bind(&failed_hashes) + .fetch_all(self.maintenance_pool.as_ref()) + .await + .map_err(|e| { + DomainError::internal_error("Dedup", format!("Legacy candidate query: {e}")) + })?; + + if batch.is_empty() { + break; + } + + for (hash, content_type) in batch { + match self.rechunk_one_legacy_blob(&hash, content_type).await { + Ok(freed) => { + report.migrated += 1; + report.freed_bytes += freed; + if report.migrated % 50 == 0 { + tracing::info!( + "Legacy re-chunk progress: {} migrated, {} failed", + report.migrated, + report.failed + ); + } + } + Err(e) => { + report.failed += 1; + tracing::error!( + "Legacy re-chunk: blob {} failed (left untouched): {e}", + &hash[..hash.len().min(12)], + ); + failed_hashes.push(hash); + if failed_hashes.len() >= MAX_FAILURES { + return Err(DomainError::internal_error( + "Dedup", + format!( + "Legacy re-chunk: aborting after {MAX_FAILURES} per-blob \ + failures — inspect blob storage integrity" + ), + )); + } + } + } + tokio::task::yield_now().await; + } + } + + Ok(report) + } + + /// Migrate a single legacy whole-file blob. Returns the number of + /// physical bytes freed (0 when the blob doubles as its own chunk). + async fn rechunk_one_legacy_blob( + &self, + hash: &str, + content_type: Option, + ) -> Result { + // ── 1. Spool + verify (decrypts via the normal read path) ── + // The spooled, hash-verified plaintext is the source of truth for + // sizes — `storage.blobs.size` is legacy metadata we don't trust + // for the manifest's Range arithmetic. + // + // The path carries a per-attempt UUID: two processes sharing a temp + // dir and racing on the same hash must never truncate or delete each + // other's in-flight spool. + let spool = std::env::temp_dir().join(format!( + "oxicloud-rechunk-{}-{}.tmp", + &hash[..hash.len().min(16)], + uuid::Uuid::new_v4() + )); + let result = self.spool_and_chunk(hash, &spool).await; + let _ = fs::remove_file(&spool).await; + let (chunk_hashes, chunk_sizes) = result?; + let total_size: u64 = chunk_sizes.iter().sum(); + + // ── 2. Accounting TX: move the file references onto the manifest ── + let mut tx = + self.maintenance_pool.begin().await.map_err(|e| { + DomainError::internal_error("Dedup", format!("Rechunk TX begin: {e}")) + })?; + + // Lock the legacy blob row — serializes against the file-delete + // trigger and the legacy dedup-hit path for this hash. + let blob_row_exists = sqlx::query_scalar::<_, i32>( + "SELECT ref_count FROM storage.blobs WHERE hash = $1 FOR UPDATE", + ) + .bind(hash) + .fetch_optional(&mut *tx) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk lock blob: {e}")))? + .is_some(); + + let file_refs: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM storage.files WHERE blob_hash = $1") + .bind(hash) + .fetch_one(&mut *tx) + .await + .map_err(|e| { + DomainError::internal_error("Dedup", format!("Rechunk count refs: {e}")) + })?; + + // ref_count = N file references; if every reference vanished while + // we were spooling, the zero-ref manifest is swept by the existing + // GC (which also unwinds the chunk refs taken in store_chunks). + let inserted = sqlx::query( + "INSERT INTO storage.chunk_manifests + (file_hash, chunk_hashes, chunk_sizes, total_size, chunk_count, + content_type, ref_count) + VALUES ($1, $2, $3, $4, $5, $6, $7) + ON CONFLICT (file_hash) DO NOTHING", + ) + .bind(hash) + .bind(&chunk_hashes) + .bind(chunk_sizes.iter().map(|s| *s as i64).collect::>()) + .bind(total_size as i64) + .bind(chunk_hashes.len() as i32) + .bind(&content_type) + .bind(file_refs as i32) + .execute(&mut *tx) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk manifest: {e}")))? + .rows_affected(); + + if inserted == 0 { + // A manifest appeared concurrently — only possible if the same + // content was re-uploaded and fully stored during our spool. + // Their bookkeeping is already correct; drop ours. + tx.rollback().await.ok(); + self.release_chunk_refs(&chunk_hashes).await; + return Ok(0); + } + + // The N file references now live on the manifest; remove them from + // the legacy blob and drop its row only when nothing else (other + // manifests using this blob as a chunk, racing legacy references) + // still points at it. + let mut blob_row_deleted = false; + if blob_row_exists { + sqlx::query( + "UPDATE storage.blobs + SET ref_count = GREATEST(ref_count - $2, 0) + WHERE hash = $1", + ) + .bind(hash) + .bind(file_refs as i32) + .execute(&mut *tx) + .await + .map_err(|e| { + DomainError::internal_error("Dedup", format!("Rechunk deref blob: {e}")) + })?; + + blob_row_deleted = + sqlx::query("DELETE FROM storage.blobs WHERE hash = $1 AND ref_count = 0") + .bind(hash) + .execute(&mut *tx) + .await + .map_err(|e| { + DomainError::internal_error("Dedup", format!("Rechunk drop blob: {e}")) + })? + .rows_affected() + > 0; + } + + tx.commit() + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk commit: {e}")))?; + + // ── 3. Physical cleanup (after commit) ── + // Deleted row ⇒ the hash is not one of its own chunks (a single-chunk + // file keeps ref_count ≥ 1 from the manifest), but guard anyway. + let mut freed = 0; + if blob_row_deleted && !chunk_hashes.iter().any(|c| c == hash) { + match self.backend.delete_blob(hash).await { + Ok(()) => freed = total_size, + Err(e) => tracing::warn!( + "Legacy re-chunk: converted {} but failed to delete the \ + old whole-file blob (GC will not retry — row is gone): {e}", + &hash[..hash.len().min(12)], + ), + } + } + + tracing::debug!( + "Legacy re-chunk: {} → {} chunk(s), {} file ref(s) moved to manifest{}", + &hash[..hash.len().min(12)], + chunk_hashes.len(), + file_refs, + if blob_row_deleted { + ", whole-file blob freed" + } else { + "" + }, + ); + + Ok(freed) + } + + /// Spool a legacy blob to `spool`, verify its BLAKE3 matches `hash`, + /// CDC-chunk it and store the chunks. Returns (chunk_hashes, chunk_sizes). + async fn spool_and_chunk( + &self, + hash: &str, + spool: &Path, + ) -> Result<(Vec, Vec), DomainError> { + use tokio::io::AsyncWriteExt; + + let mut stream = self.read_blob_stream(hash).await?; + let file = fs::File::create(spool) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk spool: {e}")))?; + let mut writer = tokio::io::BufWriter::with_capacity(512 * 1024, file); + let mut hasher = blake3::Hasher::new(); + + while let Some(chunk) = stream.next().await { + let chunk = chunk + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk read: {e}")))?; + hasher.update(&chunk); + writer + .write_all(&chunk) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk write: {e}")))?; + } + writer + .flush() + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk flush: {e}")))?; + + let actual = hasher.finalize().to_hex().to_string(); + if actual != hash { + return Err(DomainError::internal_error( + "Dedup", + format!("Blob content does not match its hash (expected {hash}, got {actual})"), + )); + } + + // Empty blobs can't be mmap'd by the CDC analyser; they become an + // empty manifest (the chunked read path streams zero chunks). + let spooled_len = fs::metadata(spool) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("Rechunk stat: {e}")))? + .len(); + if spooled_len == 0 { + return Ok((Vec::new(), Vec::new())); + } + + let chunks = Self::cdc_chunk_file(spool) + .await + .map_err(DomainError::from)?; + self.store_chunks(spool, &chunks).await + } + + /// Best-effort compensation: drop the per-manifest chunk references + /// taken by `store_chunks` when the manifest insert was abandoned. + async fn release_chunk_refs(&self, chunk_hashes: &[String]) { + if chunk_hashes.is_empty() { + return; + } + if let Err(e) = sqlx::query( + "UPDATE storage.blobs SET ref_count = GREATEST(ref_count - 1, 0) + WHERE hash = ANY($1)", + ) + .bind(chunk_hashes) + .execute(self.maintenance_pool.as_ref()) + .await + { + tracing::warn!("Legacy re-chunk: failed to release chunk refs: {e}"); + } + } +} + +/// Outcome of a [`DedupService::rechunk_legacy_blobs`] sweep. +#[derive(Debug, Default, Clone, Copy)] +pub struct LegacyRechunkReport { + /// Legacy blobs successfully converted to CDC manifests. + pub migrated: u64, + /// Blobs that failed (corrupt / unreadable) and were left untouched. + pub failed: u64, + /// Physical bytes of whole-file blobs deleted after conversion. + pub freed_bytes: u64, } // ─── Port implementation ───────────────────────────────────────────────────── @@ -1937,3 +2332,355 @@ mod tests { ); } } + +// ───────────────────────────────────────────────────────────────────────────── +// Integration tests for the legacy re-chunk migration — require the test +// database (run via `just test-integration`, which spawns it and applies +// migrations). Gated on `--cfg integration_tests` like the other PG suites. +// +// Each test seeds its own synthetic "legacy" state (a whole-file blob row in +// `storage.blobs` + file rows pointing at it, no manifest) with unique +// `rust-test-rechunk-*` names, then runs the sweep and asserts on the DB +// state for ITS hash only — concurrent test sweeps may migrate each other's +// blobs first, which is fine (and exercises the idempotency paths). +// ───────────────────────────────────────────────────────────────────────────── +#[cfg(integration_tests)] +#[allow(dead_code)] +mod rechunk_integration_tests { + use super::*; + use crate::infrastructure::services::encrypted_blob_backend::EncryptedBlobBackend; + use crate::infrastructure::services::local_blob_backend::LocalBlobBackend; + use crate::integration_test_support::{ensure_clean_test_db, test_db_url}; + use sqlx::Row; + use sqlx::postgres::PgPoolOptions; + use tempfile::TempDir; + use uuid::Uuid; + + async fn test_pool() -> Arc { + let pool = PgPoolOptions::new() + .max_connections(4) + .connect(&test_db_url()) + .await + .expect("connect to test DB — run tests/common/spawn-db.sh first"); + ensure_clean_test_db(&pool).await; + Arc::new(pool) + } + + async fn seed_user(pool: &PgPool) -> Uuid { + sqlx::query("SELECT id FROM auth.users LIMIT 1") + .fetch_one(pool) + .await + .map(|r| r.get::("id")) + .expect("auth.users must be seeded (init-test-schema.sh)") + } + + /// Plain local backend in a fresh temp dir. + async fn local_svc(pool: &Arc, dir: &TempDir) -> DedupService { + let backend = Arc::new(LocalBlobBackend::new(&dir.path().join("blobs"))); + backend.initialize().await.expect("init backend"); + DedupService::new(backend, pool.clone(), pool.clone()) + } + + /// AES-256-GCM-encrypted local backend in a fresh temp dir. + async fn encrypted_svc(pool: &Arc, dir: &TempDir) -> DedupService { + let inner = Arc::new(LocalBlobBackend::new(&dir.path().join("blobs"))); + inner.initialize().await.expect("init backend"); + let key = EncryptedBlobBackend::generate_key(); + let backend = Arc::new(EncryptedBlobBackend::new(inner, &key)); + DedupService::new(backend, pool.clone(), pool.clone()) + } + + /// Non-trivial content of `len` bytes + a random 16-byte tail, so every + /// invocation produces a unique hash — stale rows left behind by a + /// previously failed run (panics skip cleanup) can never collide with + /// the current one. + fn content(len: usize, salt: u8) -> Vec { + let mut data: Vec = (0..len) + .map(|i| { + ((i % 251) as u8) + .wrapping_add(salt) + .wrapping_add((i / 7919) as u8) + }) + .collect(); + data.extend_from_slice(Uuid::new_v4().as_bytes()); + data + } + + /// Seed a pre-CDC legacy blob: physical blob via the backend + a + /// `storage.blobs` row (ref_count = n_files) + `n_files` file rows. + /// Returns (hash, file row ids). When `corrupt_stored_bytes` is Some, + /// the PHYSICAL content differs from the indexed hash. + async fn seed_legacy( + svc: &DedupService, + pool: &PgPool, + dir: &TempDir, + data: &[u8], + n_files: i32, + label: &str, + corrupt_stored_bytes: Option<&[u8]>, + ) -> (String, Vec) { + let hash = blake3::hash(data).to_hex().to_string(); + let stored = corrupt_stored_bytes.unwrap_or(data); + + let src = dir.path().join(format!("seed-{label}.tmp")); + tokio::fs::write(&src, stored).await.expect("write seed"); + svc.backend().put_blob(&hash, &src).await.expect("put blob"); + + sqlx::query( + "INSERT INTO storage.blobs (hash, size, ref_count, content_type) + VALUES ($1, $2, $3, 'application/octet-stream') + ON CONFLICT (hash) DO UPDATE SET ref_count = storage.blobs.ref_count + $3", + ) + .bind(&hash) + .bind(data.len() as i64) + .bind(n_files) + .execute(pool) + .await + .expect("insert legacy blob row"); + + let user_id = seed_user(pool).await; + let mut file_ids = Vec::new(); + for i in 0..n_files { + let name = format!( + "rust-test-rechunk-{label}-{}-{i}", + &Uuid::new_v4().to_string()[..8] + ); + let id: Uuid = sqlx::query_scalar( + "INSERT INTO storage.files (name, user_id, blob_hash, size) + VALUES ($1, $2, $3, $4) RETURNING id", + ) + .bind(&name) + .bind(user_id) + .bind(&hash) + .bind(data.len() as i64) + .fetch_one(pool) + .await + .expect("insert file row"); + file_ids.push(id); + } + (hash, file_ids) + } + + /// Best-effort cleanup of everything a test seeded/created for `hash`. + async fn cleanup(pool: &PgPool, hash: &str, file_ids: &[Uuid]) { + let chunks: Option> = sqlx::query_scalar( + "SELECT chunk_hashes FROM storage.chunk_manifests WHERE file_hash = $1", + ) + .bind(hash) + .fetch_optional(pool) + .await + .unwrap_or(None); + + let _ = sqlx::query("DELETE FROM storage.files WHERE id = ANY($1)") + .bind(file_ids) + .execute(pool) + .await; + // Also scrub test-named rows from previously failed runs (panics + // skip the end-of-test cleanup) that reference the same hash. + let _ = sqlx::query( + "DELETE FROM storage.files + WHERE blob_hash = $1 AND name LIKE 'rust-test-rechunk-%'", + ) + .bind(hash) + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM storage.chunk_manifests WHERE file_hash = $1") + .bind(hash) + .execute(pool) + .await; + let mut to_drop = chunks.unwrap_or_default(); + to_drop.push(hash.to_string()); + let _ = sqlx::query("DELETE FROM storage.blobs WHERE hash = ANY($1)") + .bind(&to_drop) + .execute(pool) + .await; + } + + async fn collect(svc: &DedupService, hash: &str) -> Vec { + let mut out = Vec::new(); + let mut stream = svc.read_blob_stream(hash).await.expect("stream"); + while let Some(chunk) = stream.next().await { + out.extend_from_slice(&chunk.expect("chunk")); + } + out + } + + /// Manifest row (ref_count, total_size, chunk_hashes), if present. + async fn manifest(pool: &PgPool, hash: &str) -> Option<(i32, i64, Vec)> { + sqlx::query_as( + "SELECT ref_count, total_size, chunk_hashes + FROM storage.chunk_manifests WHERE file_hash = $1", + ) + .bind(hash) + .fetch_optional(pool) + .await + .expect("manifest query") + } + + async fn blob_row(pool: &PgPool, hash: &str) -> Option { + sqlx::query_scalar("SELECT ref_count FROM storage.blobs WHERE hash = $1") + .bind(hash) + .fetch_optional(pool) + .await + .expect("blob query") + } + + // ── 1. Multi-chunk blob: refs move to manifest, whole-file blob freed ── + #[tokio::test] + async fn rechunk_multi_chunk_moves_refs_and_frees_blob() { + let pool = test_pool().await; + let dir = TempDir::new().unwrap(); + let svc = local_svc(&pool, &dir).await; + + // 3 MiB ⇒ ≥ 3 CDC chunks (max chunk = 1 MiB), 2 referencing files. + let data = content(3 * 1024 * 1024, 1); + let (hash, files) = seed_legacy(&svc, &pool, &dir, &data, 2, "multi", None).await; + + assert!(svc.count_legacy_blobs().await.unwrap() >= 1); + svc.rechunk_legacy_blobs().await.expect("sweep"); + + let (rc, total, chunks) = manifest(&pool, &hash).await.expect("manifest created"); + assert_eq!(rc, 2, "both file references must move to the manifest"); + assert_eq!(total, data.len() as i64); + assert!(chunks.len() >= 3, "3 MiB must split into ≥3 chunks"); + + // Whole-file blob fully dereferenced: row gone, physical file gone. + assert_eq!(blob_row(&pool, &hash).await, None); + assert!(!svc.backend().blob_exists(&hash).await.unwrap()); + + // Every chunk row carries exactly the manifest's reference. + for c in &chunks { + assert_eq!(blob_row(&pool, c).await, Some(1), "chunk {c}"); + } + + // Content integrity through the chunked read path + a Range that + // crosses a chunk boundary. + assert_eq!(collect(&svc, &hash).await, data); + let mut ranged = Vec::new(); + let mut s = svc + .read_blob_range_stream(&hash, 1_500_000, Some(1_500_100)) + .await + .expect("range"); + while let Some(chunk) = s.next().await { + ranged.extend_from_slice(&chunk.expect("chunk")); + } + assert_eq!(ranged, &data[1_500_000..1_500_100]); + + cleanup(&pool, &hash, &files).await; + } + + // ── 2. Single-chunk blob: physical blob IS the chunk and must survive ── + #[tokio::test] + async fn rechunk_single_chunk_keeps_physical_blob() { + let pool = test_pool().await; + let dir = TempDir::new().unwrap(); + let svc = local_svc(&pool, &dir).await; + + // 50 KB < CDC_MIN_CHUNK ⇒ exactly one chunk whose hash == file hash. + let data = content(50 * 1024, 2); + let (hash, files) = seed_legacy(&svc, &pool, &dir, &data, 1, "single", None).await; + + svc.rechunk_legacy_blobs().await.expect("sweep"); + + let (rc, total, chunks) = manifest(&pool, &hash).await.expect("manifest created"); + assert_eq!(rc, 1); + assert_eq!(total, data.len() as i64); + assert_eq!(chunks, vec![hash.clone()], "the file IS its single chunk"); + + // Blob row survives with exactly the manifest's chunk reference; + // the physical bytes were never rewritten. + assert_eq!(blob_row(&pool, &hash).await, Some(1)); + assert!(svc.backend().blob_exists(&hash).await.unwrap()); + assert_eq!(collect(&svc, &hash).await, data); + + cleanup(&pool, &hash, &files).await; + } + + // ── 3. Corrupt blob (content ≠ hash): fail, count, leave untouched ── + #[tokio::test] + async fn rechunk_corrupt_blob_left_untouched() { + let pool = test_pool().await; + let dir = TempDir::new().unwrap(); + let svc = local_svc(&pool, &dir).await; + + let data = content(100 * 1024, 3); + let mut wrong = data.clone(); + wrong[0] ^= 0xFF; + let (hash, files) = seed_legacy(&svc, &pool, &dir, &data, 1, "corrupt", Some(&wrong)).await; + + let report = svc.rechunk_legacy_blobs().await.expect("sweep"); + assert!(report.failed >= 1, "the corrupt blob must be counted"); + + // Nothing was touched: no manifest, blob row + refs + file intact. + assert_eq!(manifest(&pool, &hash).await, None); + assert_eq!(blob_row(&pool, &hash).await, Some(1)); + assert!(svc.backend().blob_exists(&hash).await.unwrap()); + let files_left: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM storage.files WHERE blob_hash = $1") + .bind(&hash) + .fetch_one(pool.as_ref()) + .await + .unwrap(); + assert_eq!(files_left, 1); + + cleanup(&pool, &hash, &files).await; + } + + // ── 4. Empty blob: empty manifest, empty stream ── + #[tokio::test] + async fn rechunk_empty_blob() { + let pool = test_pool().await; + let dir = TempDir::new().unwrap(); + let svc = local_svc(&pool, &dir).await; + + // The empty-content hash is a constant (no per-run uniqueness is + // possible), so scrub any leftovers from a previously failed run. + let empty_hash = blake3::hash(&[]).to_hex().to_string(); + cleanup(&pool, &empty_hash, &[]).await; + + let (hash, files) = seed_legacy(&svc, &pool, &dir, &[], 1, "empty", None).await; + + svc.rechunk_legacy_blobs().await.expect("sweep"); + + let (rc, total, chunks) = manifest(&pool, &hash).await.expect("manifest created"); + assert_eq!((rc, total), (1, 0)); + assert!(chunks.is_empty()); + assert!(collect(&svc, &hash).await.is_empty()); + + cleanup(&pool, &hash, &files).await; + } + + // ── 5. Encrypted backend: spool decrypts, chunks re-encrypt, Range works ── + #[tokio::test] + async fn rechunk_encrypted_multi_chunk_roundtrip() { + let pool = test_pool().await; + let dir = TempDir::new().unwrap(); + let svc = encrypted_svc(&pool, &dir).await; + + let data = content(2 * 1024 * 1024 + 333, 4); + let (hash, files) = seed_legacy(&svc, &pool, &dir, &data, 1, "enc", None).await; + + svc.rechunk_legacy_blobs().await.expect("sweep"); + + let (rc, total, chunks) = manifest(&pool, &hash).await.expect("manifest created"); + assert_eq!(rc, 1); + assert_eq!(total, data.len() as i64); + assert!(chunks.len() >= 2); + assert_eq!(blob_row(&pool, &hash).await, None, "whole-file blob freed"); + + // The point of the whole migration: a Range read now decrypts only + // the overlapping ≤1 MiB chunks, and returns correct plaintext. + assert_eq!(collect(&svc, &hash).await, data); + let mut ranged = Vec::new(); + let mut s = svc + .read_blob_range_stream(&hash, 1_100_000, Some(1_100_064)) + .await + .expect("range"); + while let Some(chunk) = s.next().await { + ranged.extend_from_slice(&chunk.expect("chunk")); + } + assert_eq!(ranged, &data[1_100_000..1_100_064]); + + cleanup(&pool, &hash, &files).await; + } +} From 23de7e503bb5a9fbc99c9397061cd47d2a9cc8e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 10:56:33 +0000 Subject: [PATCH 5/5] Cache Arc in JWT validation; bump Docker base images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JWT validation cache now stores Arc and validate_token returns Arc. On a cache hit — the 99% path for every authenticated request — the moka lookup was deep-cloning the whole claims struct (5 Strings: sub, jti, username, email, role) on every call. It is now a refcount bump. Read-only callers (admin middleware) go through Deref and allocate nothing; the auth middleware clones only the three fields it moves into CurrentUser (was 5 clones, now 3), and the admin paths clone only role (was 5, now 1). A new test asserts the hit path returns a pointer-equal Arc. TokenServicePort::validate_token is the single trait method touched; its only implementor is JwtTokenService and the only production callers are the auth and admin middleware (the WOPI handler uses a separate WopiTokenService). Dockerfile: rust:1.94.1-alpine3.23 -> rust:1.96-alpine3.24 and alpine:3.23.3 -> alpine:3.24.0 for the runtime stage. https://claude.ai/code/session_0193Hff42gaA962wThxMGSd1 --- Dockerfile | 4 +- src/application/ports/auth_ports.rs | 11 +++++- src/infrastructure/services/jwt_service.rs | 43 ++++++++++++++++++---- src/interfaces/middleware/admin.rs | 4 +- src/interfaces/middleware/auth.rs | 12 +++--- 5 files changed, 55 insertions(+), 19 deletions(-) diff --git a/Dockerfile b/Dockerfile index e50b8107..117edafa 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ # ─── Stage 1: Shared build base (avoids duplicate apk install) ──────────────── -FROM rust:1.94.1-alpine3.23 AS base +FROM rust:1.96-alpine3.24 AS base # sqlx's postgres driver speaks the wire protocol in pure Rust (no pq-sys in # Cargo.lock) and TLS goes through rustls, so libpq headers are never needed at # build time. perl/make/gcc/musl-dev remain for the C builds of aws-lc-sys. @@ -41,7 +41,7 @@ ARG DATABASE_URL="postgres://postgres:postgres@localhost/oxicloud" RUN DATABASE_URL="${DATABASE_URL}" cargo build --release # ─── Stage 4: Minimal runtime image ────────────────────────────────────────── -FROM alpine:3.23.3 +FROM alpine:3.24.0 # OCI image metadata LABEL org.opencontainers.image.title="OxiCloud" \ diff --git a/src/application/ports/auth_ports.rs b/src/application/ports/auth_ports.rs index 57f9782c..5e9c79fd 100644 --- a/src/application/ports/auth_ports.rs +++ b/src/application/ports/auth_ports.rs @@ -3,6 +3,7 @@ use crate::domain::entities::app_password::AppPassword; use crate::domain::entities::device_code::DeviceCode; use crate::domain::entities::session::Session; use crate::domain::entities::user::User; +use std::sync::Arc; use uuid::Uuid; // ============================================================================ @@ -51,8 +52,14 @@ pub trait TokenServicePort: Send + Sync + 'static { /// Generate an access token for a user fn generate_access_token(&self, user: &User) -> Result; - /// Validate a token and extract its claims - fn validate_token(&self, token: &str) -> Result; + /// Validate a token and extract its claims. + /// + /// Returns `Arc` so the implementation's validation cache can + /// hand back a hot entry with a refcount bump instead of deep-cloning the + /// (multi-`String`) claims on every authenticated request. Callers that + /// only read fields go through `Deref`; the few that retain a field clone + /// just that one. + fn validate_token(&self, token: &str) -> Result, DomainError>; /// Generate a refresh token fn generate_refresh_token(&self) -> String; diff --git a/src/infrastructure/services/jwt_service.rs b/src/infrastructure/services/jwt_service.rs index 2e70f111..37d825f5 100644 --- a/src/infrastructure/services/jwt_service.rs +++ b/src/infrastructure/services/jwt_service.rs @@ -13,6 +13,7 @@ use chrono::Utc; use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey, Header, Validation, decode, encode}; use moka::sync::Cache; use serde::{Deserialize, Serialize}; +use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Duration; use uuid::Uuid; @@ -69,8 +70,9 @@ impl From for TokenClaims { /// /// The cache uses the **BLAKE3** hash of the raw token string as key (32-byte, /// ~0.1 µs to compute — 20× cheaper than HMAC verification) and stores the -/// validated `TokenClaims`. On a cache hit the HMAC step is completely -/// skipped. +/// validated claims behind an `Arc`. On a cache hit the HMAC step is +/// completely skipped and the lookup returns a refcount bump rather than a +/// deep clone of the (multi-`String`) `TokenClaims`. /// /// **Security properties**: /// - TTL of 30 s bounds the window in which a revoked token remains valid. @@ -84,8 +86,9 @@ pub struct JwtTokenService { access_token_expiry: i64, /// Expiration time for refresh tokens in seconds refresh_token_expiry: i64, - /// Validation result cache: blake3(token) → TokenClaims - validation_cache: Cache<[u8; 32], TokenClaims>, + /// Validation result cache: blake3(token) → Arc. + /// `Arc` so a cache hit is a refcount bump, not a multi-`String` clone. + validation_cache: Cache<[u8; 32], Arc>, /// Cache hit counter (for observability / metrics) cache_hits: AtomicU64, /// Cache miss counter @@ -194,7 +197,7 @@ impl TokenServicePort for JwtTokenService { }) } - fn validate_token(&self, token: &str) -> Result { + fn validate_token(&self, token: &str) -> Result, DomainError> { // ── 1. Fast-path: check the validation cache ───────────── let key = Self::token_hash(token); @@ -232,14 +235,15 @@ impl TokenServicePort for JwtTokenService { ), })?; - let claims: TokenClaims = token_data.claims.into(); + let claims = Arc::new(TokenClaims::from(token_data.claims)); // ── 3. Store in cache for subsequent requests ──────────── // Only cache tokens that won't expire within the cache TTL window, // avoiding stale positives right at the boundary. let remaining_secs = claims.exp - Utc::now().timestamp(); if remaining_secs > VALIDATION_CACHE_TTL_SECS as i64 { - self.validation_cache.insert(key, claims.clone()); + // Refcount bump — the claims live once behind the `Arc`. + self.validation_cache.insert(key, Arc::clone(&claims)); } Ok(claims) @@ -348,6 +352,31 @@ mod tests { assert_eq!(misses, 1, "Expected 1 cache miss"); } + #[test] + fn test_cache_hit_returns_same_arc_not_a_clone() { + let service = JwtTokenService::new( + "test_secret_key_at_least_32_bytes_long".to_string(), + 3600, + 86400, + ); + let token = service + .generate_access_token(&create_test_user()) + .expect("Should generate token"); + + // Miss populates the cache; hit must hand back the very same + // allocation (pointer-equal Arc), proving the hot path is a refcount + // bump rather than a deep clone of the claims' Strings. + let first = service.validate_token(&token).expect("miss"); + let second = service.validate_token(&token).expect("hit"); + + assert!( + Arc::ptr_eq(&first, &second), + "cache hit must return the same Arc, not a fresh allocation" + ); + let (hits, misses) = service.cache_stats(); + assert_eq!((hits, misses), (1, 1)); + } + #[test] fn test_invalid_token_not_cached() { let service = JwtTokenService::new("secret".to_string(), 3600, 86400); diff --git a/src/interfaces/middleware/admin.rs b/src/interfaces/middleware/admin.rs index b8662601..8ddd13c2 100644 --- a/src/interfaces/middleware/admin.rs +++ b/src/interfaces/middleware/admin.rs @@ -54,7 +54,7 @@ pub async fn require_admin( Ok(( Uuid::parse_str(&claims.sub) .map_err(|_| AppError::internal_error("Invalid user ID in token"))?, - claims.role, + claims.role.clone(), )) } @@ -87,6 +87,6 @@ pub async fn require_authenticated( Ok(( Uuid::parse_str(&claims.sub) .map_err(|_| AppError::internal_error("Invalid user ID in token"))?, - claims.role, + claims.role.clone(), )) } diff --git a/src/interfaces/middleware/auth.rs b/src/interfaces/middleware/auth.rs index e2e4ca5b..34ca7c1a 100644 --- a/src/interfaces/middleware/auth.rs +++ b/src/interfaces/middleware/auth.rs @@ -183,9 +183,9 @@ pub async fn auth_middleware( })?; let current_user = Arc::new(CurrentUser { id: user_id, - username: claims.username, - email: claims.email, - role: claims.role, + username: claims.username.clone(), + email: claims.email.clone(), + role: claims.role.clone(), }); request.extensions_mut().insert(current_user); tracing::Span::current().record("user_id", user_id.to_string()); @@ -287,9 +287,9 @@ pub async fn auth_middleware( })?; let current_user = Arc::new(CurrentUser { id: user_id, - username: claims.username, - email: claims.email, - role: claims.role, + username: claims.username.clone(), + email: claims.email.clone(), + role: claims.role.clone(), }); request.extensions_mut().insert(current_user); request.extensions_mut().insert(CookieAuthenticated);