From bd7b0710a8c56e14d704e07efa0c764e6a18c45b Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 17 Jul 2026 19:46:54 +0200 Subject: [PATCH] fix(cache): invalidate root folder cache on rename this fix https://github.com/AtalayaLabs/OxiCloud/issues/607 which was introduced by commit 12dc648cffba08c175cb3055c8010260b0e70a0d when a user rename a root folder, this invalidate the cache still some UX effect displaying phantom drive is grant is revoked, cache is 30s of TTL so this UX glitch is acceptable --- src/application/services/folder_service.rs | 23 +++++++- src/domain/repositories/drive_repository.rs | 23 ++++++++ .../repositories/pg/drive_pg_repository.rs | 52 +++++++++++++++---- 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/application/services/folder_service.rs b/src/application/services/folder_service.rs index c677ec0d..1ae050e1 100644 --- a/src/application/services/folder_service.rs +++ b/src/application/services/folder_service.rs @@ -581,7 +581,7 @@ impl FolderUseCase for FolderService { ) .await?; - let folder = self + let renamed = self .folder_storage .rename_folder(id, dto.name, caller_id) .await @@ -592,7 +592,26 @@ impl FolderUseCase for FolderService { ) })?; - Ok(FolderDto::from(folder)) + // Root folders double as the drive's display name (see the + // `required_perm` branch above and `drive_pg_repository.rs` + // `readable_cache` + `default_drive_cache` docs). + // `drives.name` is sourced from `folders.name` of the root + // folder, so a rename affects BOTH caches — every user's + // readable-drive list AND the per-user default-drive lookup. + // Both are 30 s TTL; without the invalidation, `GET /api/drives` + // returns the stale name for up to that window after a root + // rename. Surfaced by `tests/api/drives_membership.hurl` + // Step 23. Regression from commit `12dc648c` ("perf: round 4 — + // drive-selector cache") which added the caches without + // wiring the root-rename invalidation. + if folder.parent_id().is_none() + && let Some(drive_repo) = &self.drive_repo + { + drive_repo.invalidate_readable_all(); + drive_repo.invalidate_default_drive_all(); + } + + Ok(FolderDto::from(renamed)) } /// Moves a folder to a new parent. Requires `Update` on the source and diff --git a/src/domain/repositories/drive_repository.rs b/src/domain/repositories/drive_repository.rs index 74d82523..b5e5f984 100644 --- a/src/domain/repositories/drive_repository.rs +++ b/src/domain/repositories/drive_repository.rs @@ -184,6 +184,29 @@ pub trait DriveRepository: Send + Sync + 'static { /// content first so a single click can't wipe a populated drive. async fn is_empty(&self, drive_id: Uuid) -> Result; + /// Drop the cached readable-drive list for one user. Called by + /// service-layer code paths that mutate state affecting a specific + /// caller's drive listing (grant writes, membership changes) but + /// don't reach through the drive-repo itself. Default no-op — the + /// no-cache stubs need no plumbing. + async fn invalidate_readable_for_user(&self, _user_id: Uuid) {} + + /// Drop every cached readable-drive list. Called when the affected + /// user set is unknown at this layer — group-subject grants, drive + /// deletion, policy edits, root-folder renames (drive.name is + /// sourced from the root folder, so a rename affects the listing + /// for every user with a grant on the drive). Default no-op. + fn invalidate_readable_all(&self) {} + + /// Drop every entry in the "default drive per user" cache. Called + /// from paths that mutate a drive's display name or its root + /// folder id at the concrete cache level (root-folder rename is + /// the only one today). Same class of bug as + /// `invalidate_readable_all` — the cache holds a `DriveWithRootName` + /// with `root_folder_name` baked in, so a rename would otherwise + /// stay stale for the cache TTL. Default no-op. + fn invalidate_default_drive_all(&self) {} + /// Hard-delete a drive: its `role_grants` rows, its root folder, /// and the drive row itself, in one transaction. Caller is /// responsible for ensuring `is_empty` first; this method does diff --git a/src/infrastructure/repositories/pg/drive_pg_repository.rs b/src/infrastructure/repositories/pg/drive_pg_repository.rs index 4b007d1f..87b45610 100644 --- a/src/infrastructure/repositories/pg/drive_pg_repository.rs +++ b/src/infrastructure/repositories/pg/drive_pg_repository.rs @@ -25,10 +25,11 @@ use crate::domain::repositories::drive_repository::{ /// policy edits — all of which invalidate explicitly below), yet it is /// re-resolved on EVERY NextCloud request (basic-auth chroot), every /// native `/webdav` request (Mode-B scope resolution) and every WOPI -/// call. 30 s mirrors `drive_role_cache` in `pg_acl_engine.rs` and bounds -/// the one non-invalidated staleness source: a root-folder *rename*, -/// which doesn't pass through this repository. Measured in -/// `benches/CHROOT-CACHE.md`. +/// call. 30 s mirrors `drive_role_cache` in `pg_acl_engine.rs`. Root- +/// folder renames — which don't pass through this repository directly +/// — invalidate via the `DriveRepository::invalidate_default_drive_all` +/// trait hook called from `folder_service::rename_folder_with_perms` +/// when `parent_id IS NULL`. Measured in `benches/CHROOT-CACHE.md`. const DEFAULT_DRIVE_CACHE_TTL: Duration = Duration::from_secs(30); /// One entry per active user; entries are small (a `Drive` + a name). @@ -56,11 +57,19 @@ pub struct DrivePgRepository { /// through this repository or `DriveManagementService` invalidates /// explicitly (per-user when the subject is a User, whole cache for /// Group subjects, whose transitive membership is not resolvable - /// here). Residual staleness — a root-folder rename or a grant - /// written by a path that can't reach this cache — is bounded by - /// the same 30 s TTL the sibling caches accept; actual permission - /// enforcement is unaffected (the ACL engine re-checks per - /// operation with its own invalidation). + /// here). Root-folder renames — which update `drive.name` because it + /// reads through `folders.name` of the root row — also invalidate, + /// via the trait's `invalidate_readable_all` hook called from + /// `folder_service::rename_folder_with_perms` when + /// `parent_id IS NULL`. That path was missed by the perf commit + /// that introduced this cache (`12dc648c`) and surfaced by + /// `drives_membership.hurl` Step 23; the trait hook closes it + /// without folder_service knowing about the concrete moka cache. + /// + /// Residual staleness — a grant written by a path that can't reach + /// this cache — is bounded by the same 30 s TTL the sibling caches + /// accept; actual permission enforcement is unaffected (the ACL + /// engine re-checks per operation with its own invalidation). readable_cache: Cache>>, } @@ -93,6 +102,15 @@ impl DrivePgRepository { self.readable_cache.invalidate_all(); } + /// Drop every cached `default_drive_cache` entry. Exposed as a + /// `pub` sibling of the whole-cache invalidators above so trait + /// callers holding a `dyn DriveRepository` can trigger the same + /// cleanup path (e.g. `folder_service` on root-folder rename — + /// see `impl DriveRepository` below). + pub fn invalidate_default_drive_all(&self) { + self.default_drive_cache.invalidate_all(); + } + fn map_sqlx_err(context: &'static str, e: sqlx::Error) -> DriveRepositoryError { if let sqlx::Error::Database(ref dberr) = e && let Some(code) = dberr.code() @@ -212,6 +230,22 @@ impl DrivePgRepository { #[async_trait::async_trait] impl DriveRepository for DrivePgRepository { + async fn invalidate_readable_for_user(&self, user_id: Uuid) { + // Delegate to the inherent method — the trait forwarding lets + // callers holding a `dyn DriveRepository` (e.g. `folder_service` + // on a root-folder rename) trigger invalidation without knowing + // about the concrete cache. + DrivePgRepository::invalidate_readable_for_user(self, user_id).await; + } + + fn invalidate_readable_all(&self) { + DrivePgRepository::invalidate_readable_all(self); + } + + fn invalidate_default_drive_all(&self) { + DrivePgRepository::invalidate_default_drive_all(self); + } + async fn create_personal_drive_atomic( &self, owner_id: Uuid,