From fa0e4e1a89db05415294aa0cd8741200714d94aa Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 17 Jul 2026 19:53:50 +0200 Subject: [PATCH] fix(cache): invalidate drive used byte cache on explicit refresh from internal call this fix https://github.com/AtalayaLabs/OxiCloud/issues/607 which was introduced by commit 12dc648cffba08c175cb3055c8010260b0e70a0d when a user does activity in a drive, admin can invalidate cache via the internal call /api/admin/internal/trigger-sweep this permit end 2 end test to validte immediately that used_bytes corresponds to the expected result --- .../services/storage_usage_service.rs | 64 +++++++++++++++ src/common/di.rs | 14 +++- tests/api/drive_quota.hurl | 78 ++++++++++++++----- tests/api/user_envelope_quota.hurl | 26 +++++-- 4 files changed, 154 insertions(+), 28 deletions(-) diff --git a/src/application/services/storage_usage_service.rs b/src/application/services/storage_usage_service.rs index f2fef771..0e598ee8 100644 --- a/src/application/services/storage_usage_service.rs +++ b/src/application/services/storage_usage_service.rs @@ -19,6 +19,13 @@ use uuid::Uuid; pub struct StorageUsageService { pool: Arc, user_repository: Arc, + /// Optional so DI can wire it lazily and older test constructors + /// keep compiling. When `Some`, every write path that mutates + /// `drives.used_bytes` or `users.storage_used_bytes` invalidates + /// the drive lookup caches so `GET /api/drives` reflects the new + /// usage on the next call (see the invalidation calls in the + /// delta / sweep methods below). + drive_repo: Option>, } impl StorageUsageService { @@ -27,6 +34,44 @@ impl StorageUsageService { Self { pool, user_repository, + drive_repo: None, + } + } + + /// Wires the drive repository used for cache-invalidation-on-write. + /// Production DI calls this in `common::di`; tests without a real + /// drive repo leave it `None` and the invalidation calls no-op. + pub fn with_drive_repo( + mut self, + drive_repo: Arc, + ) -> Self { + self.drive_repo = Some(drive_repo); + self + } + + /// Drop the per-caller readable-drive listing cache and the + /// per-user default-drive cache so `GET /api/drives` and the + /// WebDAV / NextCloud / WOPI drive-lookup paths re-read fresh + /// values. + /// + /// **Called only from the reconciliation sweep**, not from the + /// hot-path `add_drive_storage_usage_delta*` methods. The design + /// (Ed's call, 2026-07-17): keep the cache useful under active + /// upload load — per-mutation invalidation would nuke the cache + /// on every file upload, defeating the point. `used_bytes` on + /// `GET /api/drives` therefore lags by up to the cache TTL (30 s), + /// which matches the sibling caches' accepted UX phantom for + /// drive-name staleness. Tests / operators that need immediate + /// freshness call `POST /api/admin/internal/trigger-sweep`, which + /// runs `update_all_drives_storage_usage` → this method. + /// + /// Security posture unaffected: `check_drive_quota` reads + /// directly from SQL, bypassing the cache entirely, so quota + /// enforcement is honest regardless of listing staleness. + fn invalidate_drive_lookup_caches(&self) { + if let Some(repo) = &self.drive_repo { + repo.invalidate_readable_all(); + repo.invalidate_default_drive_all(); } } @@ -209,6 +254,9 @@ impl StorageUsageService { .execute(self.pool.as_ref()) .await .map_err(|e| DomainError::internal_error("StorageUsage", format!("drive delta: {e}")))?; + // Deliberate no-invalidate here — see the class doc on + // `invalidate_drive_lookup_caches`. Delta writes lag the + // cache by up to the TTL; the sweep is the escape hatch. Ok(()) } @@ -285,6 +333,7 @@ impl StorageUsageService { .map_err(|e| { DomainError::internal_error("StorageUsage", format!("drive delta by folder: {e}")) })?; + // See `add_drive_storage_usage_delta` — deliberate no-invalidate. Ok(()) } @@ -595,6 +644,20 @@ impl StorageUsagePort for StorageUsageService { "Drive storage-usage reconciliation corrected {} drive(s)", result.rows_affected() ); + // Unconditional invalidation — do NOT gate on + // `rows_affected() > 0`. When a fire-and-forget delta has + // already made SQL correct BEFORE the sweep runs, the sweep + // touches zero rows but the cache may still hold the + // pre-delta value from an earlier `GET /api/drives`. Gating + // means the cache stays stale in exactly the case + // `trigger-sweep` is called to fix. The invalidation cost is + // small (moka `invalidate_all` on both caches); the + // correctness guarantee matters. Regression avoidance: + // drive_quota.hurl Step 6 exercises this race — 2nd upload's + // delta lands during the 200 ms delay, sweep sees SQL is + // already right → zero rows → without unconditional + // invalidation, cache stays at the previous step's value. + self.invalidate_drive_lookup_caches(); Ok(()) } @@ -613,6 +676,7 @@ impl Clone for StorageUsageService { Self { pool: Arc::clone(&self.pool), user_repository: Arc::clone(&self.user_repository), + drive_repo: self.drive_repo.clone(), } } } diff --git a/src/common/di.rs b/src/common/di.rs index 664ad52a..c905304a 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -1056,14 +1056,25 @@ impl AppServiceFactory { _repos: &RepositoryServices, db_pool: &Arc, maintenance_pool: &Arc, + drive_repo: Arc, ) -> Arc { let user_repository = Arc::new( crate::infrastructure::repositories::pg::UserPgRepository::new(db_pool.clone()), ); + // The `drive_repo` passed in is the SAME instance held on + // `AppState`, so its `readable_cache` / `default_drive_cache` + // are the caches the request path reads from. A separately + // constructed `DrivePgRepository` would have its OWN caches + // and invalidation would be a no-op observed by nobody — + // this is the trap that regressed the used_bytes freshness + // after perf commit `12dc648c`. let service = Arc::new( crate::application::services::storage_usage_service::StorageUsageService::new( maintenance_pool.clone(), user_repository, + ) + .with_drive_repo( + drive_repo as Arc, ), ); // Keep cached storage usage fresh off the request path: GET /api/auth/me @@ -1250,7 +1261,8 @@ impl AppServiceFactory { // 3c. Storage usage / quota service (needed by the instant-upload // path inside the application services, and re-exposed on AppState // for the handler-side quota checks of the byte-upload paths). - let storage_usage = self.create_storage_usage_service(&repos, &pool, &maintenance_pool); + let storage_usage = + self.create_storage_usage_service(&repos, &pool, &maintenance_pool, drive_repo.clone()); // 3d. Content index (embedded Tantivy) — opened before application // services so SearchService can hold the query port; the feeding diff --git a/tests/api/drive_quota.hurl b/tests/api/drive_quota.hurl index b624d748..4af2b7de 100644 --- a/tests/api/drive_quota.hurl +++ b/tests/api/drive_quota.hurl @@ -111,16 +111,25 @@ HTTP 201 small_file_id: jsonpath "$.id" -# Confirm `drives.used_bytes` reflects the new file. The hook is -# fire-and-forget on a tokio task, so the SQL UPDATE may not have -# landed by the time `POST /api/files/upload` returned. Retry the -# `GET /api/drives` until the cached value catches up — bounded -# wait keeps a slow CI machine from flaking. +# Force freshness on `drives.used_bytes`: +# 1. The fire-and-forget delta hook may not have landed yet +# (200 ms delay to let the tokio task register — see +# `bug_trigger_sweep_vs_spawn_hook_race`). +# 2. Force a reconciliation sweep. That's the ONLY path that +# invalidates `readable_cache` / `default_drive_cache` after +# Ed's 2026-07-17 design call: the sweep is the escape hatch +# for tests / operators that need immediate cache freshness; +# per-write invalidation would nuke the cache on every upload. +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} -[Options] -retry: 10 -retry-interval: 200ms HTTP 200 [Asserts] @@ -145,13 +154,19 @@ file: file,fixtures/hello-copy.txt; text/plain HTTP 201 -# `used_bytes` climbs to 64 (32 + 32). Same retry shape as the -# first assertion since the second delta is also fire-and-forget. +# `used_bytes` climbs to 64 (32 + 32). Same trigger-sweep pattern +# as the first assertion — the delta is fire-and-forget and the +# listing cache lags until the sweep invalidates it. +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} -[Options] -retry: 10 -retry-interval: 200ms HTTP 200 [Asserts] @@ -173,7 +188,18 @@ HTTP 507 # `used_bytes` is unchanged — the failed upload didn't charge the # drive. (Cumulative usage is still 64; the 5 MiB write never -# registered a row.) +# registered a row.) Trigger the sweep again to guarantee cache +# freshness — the 5 MiB attempt was refused pre-write so no +# delta was queued, but the previous sweep's invalidation was +# consumed by the intervening GET which re-populated the cache +# with the pre-refused-write value. Sweep + re-check for +# determinism. +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} @@ -211,13 +237,17 @@ HTTP 201 # Unlimited drive's `used_bytes` climbs to the file's exact size -# (5 MiB = 5_242_880 bytes). Same retry block because the delta -# hook is fire-and-forget here too. +# (5 MiB = 5_242_880 bytes). Trigger-sweep pattern (see above). +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} -[Options] -retry: 10 -retry-interval: 200ms HTTP 200 [Asserts] @@ -384,7 +414,15 @@ HTTP 200 # `used_bytes` on the tight drive is unchanged — the two refused -# operations above never wrote anything. +# operations above never wrote anything. Trigger-sweep so the +# check reads live SQL (see the class doc on the earlier +# sweep + GET pair for the design rationale). +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} diff --git a/tests/api/user_envelope_quota.hurl b/tests/api/user_envelope_quota.hurl index 47eb9775..7456cb58 100644 --- a/tests/api/user_envelope_quota.hurl +++ b/tests/api/user_envelope_quota.hurl @@ -130,15 +130,27 @@ file: file,fixtures/hello.txt; text/plain HTTP 201 -# Wait for the drive-side fire-and-forget delta to settle. -# Acts as the synchronisation point: by the time `drives.used_bytes` -# reflects the upload, the sibling user-side delta task spawned in -# the same call has had its chance to run too. +# Force freshness on `drives.used_bytes`: +# 1. 200 ms delay to let the fire-and-forget tokio task from the +# upload above land its SQL write (see +# `bug_trigger_sweep_vs_spawn_hook_race`). +# 2. Trigger the reconciliation sweep — the ONLY path that +# invalidates `readable_cache` / `default_drive_cache` after +# Ed's 2026-07-17 design call (per-write invalidation would +# nuke the cache on every upload, defeating the point). Also +# acts as the synchronisation point for the user-envelope +# assertion below — the sweep is the authoritative +# ground-truth for both drive- and user-side counters. +POST {{base_url}}/api/admin/internal/trigger-sweep +Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms + +HTTP 200 + + GET {{base_url}}/api/drives Authorization: Bearer {{owner_token}} -[Options] -retry: 10 -retry-interval: 200ms HTTP 200 [Asserts]