diff --git a/app/Models/User.php b/app/Models/User.php index bfc64461b..bb6db4489 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -24,6 +24,7 @@ class User extends Authenticatable implements OAuthenticatable 'email_verified_at' => 'datetime', '2fa_setup_at' => 'datetime', 'last_active_at' => 'datetime', + 'storage_used_updated_at' => 'datetime', ]; } diff --git a/app/Services/UserStorageService.php b/app/Services/UserStorageService.php index 5d6db398f..9c29b3963 100644 --- a/app/Services/UserStorageService.php +++ b/app/Services/UserStorageService.php @@ -4,11 +4,42 @@ namespace App\Services; use App\Models\Media; use App\Models\User; +use Carbon\Carbon; class UserStorageService { const CACHE_KEY = 'pf:services:user-storage:byId:'; + /** + * How long (in hours) a cached storage_used value is trusted for + * incremental add/subtract before the hot path recalculates it from source. + * + * Mirrors the scheduled `user:storage:recalculate --stale=168` reconciler: + * an active user who uploads or deletes will self-heal a stale counter + * without waiting for the nightly job. + */ + const STALE_AFTER_HOURS = 168; + + /** + * Whether a user's cached storage_used is too old (or missing) to be + * trusted for an incremental delta and should be recalculated from source. + */ + protected static function isStale(User $user): bool + { + $updatedAt = $user->storage_used_updated_at; + if (! $updatedAt) { + return true; + } + + // Be defensive: the value is normally cast to Carbon on the User model, + // but tolerate a raw string if a model is hydrated without casts. + if (! $updatedAt instanceof Carbon) { + $updatedAt = Carbon::parse($updatedAt); + } + + return $updatedAt->lt(now()->subHours(self::STALE_AFTER_HOURS)); + } + public static function get($id) { $user = User::find($id); @@ -51,10 +82,13 @@ class UserStorageService * stored media, without re-summing the whole media table. * * This is the counterpart to decrementStorageUsed and is used on the upload - * path. If storage_used has never been calculated (storage_used_updated_at - * is null) it seeds the counter from source first so the increment is added - * to an accurate base rather than zero. Any drift is corrected by the - * periodic recalculateUpdateStorageUsed reconciler. + * path, AFTER the new media row has been saved. + * + * Self-healing: if the cached counter is missing or stale (see + * STALE_AFTER_HOURS) it recalculates from source instead of trusting the + * incremental value. Because callers save the media row before calling + * this, a from-source recalc already accounts for the new media, so the + * delta is NOT re-applied on the recalc path (that would double count). * * @param int $id User id * @param int $sizeInBytes Size of the added media in bytes @@ -67,13 +101,14 @@ class UserStorageService return null; } - // Seed from source if never calculated, so we add to an accurate base. - $base = $user->storage_used_updated_at - ? (int) $user->storage_used - : self::calculateStorageUsed($id); + // Stale or uncalculated: recompute from source. The just-saved media is + // already included, so return the source value without adding the delta. + if (self::isStale($user)) { + return self::recalculateUpdateStorageUsed($id); + } $sizeInKbs = (int) floor(((int) $sizeInBytes) / 1000); - $updatedVal = max(0, $base + $sizeInKbs); + $updatedVal = max(0, (int) $user->storage_used + $sizeInKbs); $user->storage_used = $updatedVal; $user->storage_used_updated_at = now(); @@ -86,14 +121,14 @@ class UserStorageService * Decrement a user's cached storage_used by the size (in bytes) of removed * media, without re-summing the whole media table. * - * This is the fast path used when media is deleted. Any drift it introduces - * (e.g. double-processed jobs, deletions that bypass this path) is corrected - * by the periodic recalculateUpdateStorageUsed reconciler. The value is - * clamped at zero so it can never go negative. + * This is the fast path used when media is deleted, AFTER the media row has + * been removed. The value is clamped at zero so it can never go negative. * - * Note: only acts on an already-populated counter. If storage_used has - * never been calculated (storage_used_updated_at is null), it leaves the - * value untouched so the next get()/recalculate computes it from source. + * Self-healing: if the cached counter is missing or stale (see + * STALE_AFTER_HOURS) it recalculates from source instead of trusting the + * incremental value. Because callers delete the media row before calling + * this, a from-source recalc already excludes the removed media, so the + * delta is NOT re-applied on the recalc path (that would over-subtract). * * @param int $id User id * @param int $sizeInBytes Size of the removed media in bytes @@ -106,9 +141,10 @@ class UserStorageService return null; } - // Nothing cached yet: let the next full calculation establish the value. - if (! $user->storage_used_updated_at) { - return null; + // Stale or uncalculated: recompute from source. The removed media is + // already excluded, so return the source value without subtracting. + if (self::isStale($user)) { + return self::recalculateUpdateStorageUsed($id); } $sizeInKbs = (int) floor(((int) $sizeInBytes) / 1000); diff --git a/tests/Feature/Services/UserStorageServiceTest.php b/tests/Feature/Services/UserStorageServiceTest.php index 0957c94ea..8dff2607e 100644 --- a/tests/Feature/Services/UserStorageServiceTest.php +++ b/tests/Feature/Services/UserStorageServiceTest.php @@ -131,23 +131,66 @@ it('increases storage_used by the added media size in KB', function () { expect((int) $user->storage_used)->toBe(800); }); -it('seeds from actual media when increasing an uncalculated counter', function () { +it('recalculates from source instead of adding when the counter was never calculated', function () { $user = User::factory()->create(); $user->refresh(); - // Fresh user with pre-existing media but no cached counter yet. + // Fresh user with pre-existing media but no cached counter yet. Callers + // save the media row before calling increaseStorageUsed, so an + // uncalculated (stale) counter recomputes from source rather than adding. expect($user->storage_used_updated_at)->toBeNull(); - makeMedia($user, 200000, 1); // 200 KB already on disk + makeMedia($user, 200000, 1); // 200 KB already on disk (the "just-saved" media) - // Add another 500 KB; base should be seeded from the 200 KB of media. - $result = UserStorageService::increaseStorageUsed($user->id, 500000); + $result = UserStorageService::increaseStorageUsed($user->id, 200000); - expect($result)->toBe(700); + // Source already includes the 200 KB row; delta is NOT re-added. + expect($result)->toBe(200); $user->refresh(); - expect((int) $user->storage_used)->toBe(700); + expect((int) $user->storage_used)->toBe(200); expect($user->storage_used_updated_at)->not->toBeNull(); }); +/* +| Self-healing: when the cached counter is older than STALE_AFTER_HOURS, the +| upload path recalculates from source (which already includes the just-saved +| media) instead of trusting a possibly-drifted incremental value (#7169). +*/ +it('recalculates from source on increase when the counter is stale', function () { + $user = User::factory()->create(); + $user->refresh(); + + // Wildly inflated counter, last touched well beyond the stale window. + $user->storage_used = 999999; + $user->storage_used_updated_at = now()->subHours(UserStorageService::STALE_AFTER_HOURS + 1); + $user->save(); + + // Actual media on disk (the just-saved upload) is 300 KB. + makeMedia($user, 300000, 1); + + $result = UserStorageService::increaseStorageUsed($user->id, 300000); + + expect($result)->toBe(300); + $user->refresh(); + expect((int) $user->storage_used)->toBe(300); +}); + +it('trusts the incremental value on increase when the counter is fresh', function () { + $user = User::factory()->create(); + $user->refresh(); + + // Fresh counter that intentionally disagrees with actual media; the fast + // path must trust it and only add the delta, not recompute. + $user->storage_used = 300; + $user->storage_used_updated_at = now(); + $user->save(); + makeMedia($user, 999000, 1); // disagrees with the cached 300 + + $result = UserStorageService::increaseStorageUsed($user->id, 500000); + + // 300 + 500 = 800 (incremental), NOT recalculated from the 999 KB media. + expect($result)->toBe(800); +}); + it('returns null when increasing a missing user', function () { expect(UserStorageService::increaseStorageUsed(999999, 500000))->toBeNull(); }); @@ -202,19 +245,61 @@ it('clamps decrement at zero and never goes negative', function () { expect((int) $user->storage_used)->toBe(0); }); -it('skips decrement when the counter was never calculated', function () { +it('recalculates from source on decrement when the counter was never calculated', function () { $user = User::factory()->create(); $user->refresh(); - // Fresh user: storage_used_updated_at is null. + // Fresh user: storage_used_updated_at is null (treated as stale). Callers + // delete the media row before calling decrementStorageUsed, so the remaining + // media is the source of truth. expect($user->storage_used_updated_at)->toBeNull(); + makeMedia($user, 150000, 1); // 150 KB of remaining media $result = UserStorageService::decrementStorageUsed($user->id, 500000); - // Skipped so a later get()/recalculate establishes the true value. - expect($result)->toBeNull(); + // Recomputed from remaining media; delta is NOT subtracted on top. + expect($result)->toBe(150); $user->refresh(); - expect($user->storage_used_updated_at)->toBeNull(); + expect((int) $user->storage_used)->toBe(150); + expect($user->storage_used_updated_at)->not->toBeNull(); +}); + +/* +| Self-healing: when the cached counter is older than STALE_AFTER_HOURS, the +| delete path recalculates from source (which already excludes the removed +| media) instead of trusting a possibly-drifted incremental value (#7169). +*/ +it('recalculates from source on decrement when the counter is stale', function () { + $user = User::factory()->create(); + $user->refresh(); + + $user->storage_used = 999999; + $user->storage_used_updated_at = now()->subHours(UserStorageService::STALE_AFTER_HOURS + 1); + $user->save(); + + makeMedia($user, 250000, 1); // 250 KB remaining after the delete + + $result = UserStorageService::decrementStorageUsed($user->id, 500000); + + expect($result)->toBe(250); + $user->refresh(); + expect((int) $user->storage_used)->toBe(250); +}); + +it('trusts the incremental value on decrement when the counter is fresh', function () { + $user = User::factory()->create(); + $user->refresh(); + + // Fresh counter that disagrees with actual media; fast path must trust it. + $user->storage_used = 800; + $user->storage_used_updated_at = now(); + $user->save(); + makeMedia($user, 999000, 1); // disagrees with the cached 800 + + $result = UserStorageService::decrementStorageUsed($user->id, 500000); + + // 800 - 500 = 300 (incremental), NOT recalculated from the 999 KB media. + expect($result)->toBe(300); }); it('returns null when decrementing a missing user', function () {