Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Make Cache::forget compatible with Cache::flexible #52859

Closed
wants to merge 3 commits into from

Conversation

rbadillap
Copy link
Contributor

@rbadillap rbadillap commented Sep 19, 2024

Fixes #52847

Current Behavior

The Cache::flexible method, which implements the "stale-while-revalidate" pattern, currently creates two cache items:

  • The main item with the given key
  • A metadata item with the key + ":created" suffix

When Cache::forget is called, it only removes the main item, leaving the metadata item in place. This leads to unexpected behavior in the cache lifecycle.

The issue

When Cache::forget is called, it only removes the main item, leaving the ":created" timestamp intact.
On the next request, Cache::flexible finds a null value for the main item but an existing ":created" timestamp.

This causes the method to incorrectly assume the item is in the stale or expired state, rather than treating it as a fresh cache miss.

As a result, the cache remains null until the stale period is over, effectively breaking the expected cache regeneration behavior.

Current behavior

current_behavior.mov

Expected behavior

expected_behavior.mov

Proposed Fix

Update the Cache::forget method to remove both the main item and the associated metadata item when called on a key created by Cache::flexible.

PoC

My PoC if you want to interactively test the issue.

<?php

use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Route;

Route::get('/debug', function () {

    $cachedAt = Cache::flexible('current-time', [5, 10], function () {
        $start = microtime(true);
        $when = now()->toTimeString();
        sleep(2);

        $end = microtime(true);
        $executionTime = ($end - $start) * 1000;
        return [
            "when" => $when,
            'execution_time_ms' => round($executionTime, 2),
        ];
    });

    $mainItem = Cache::get('current-time');
    $createdItem = Cache::get('current-time:created');


    return response()->json([
        'status' => 'ok',
        'cached_at' => $cachedAt,
        'current_time' => now()->toTimeString(),
        'cache_state' => [
            'main_item' => $mainItem,
            'created_item' => $createdItem,
        ],
    ]);
});

Route::get('/refresh', function () {
    $beforeForget = [
        'main_item' => Cache::get('current-time'),
        'created_item' => Cache::get('current-time:created'),
    ];

    Cache::forget('current-time');

    $afterForget = [
        'main_item' => Cache::get('current-time'),
        'created_item' => Cache::get('current-time:created'),
    ];

    return response()->json([
        'status' => 'ok',
        'message' => 'Cache forcefully refreshed',
        'time' => now()->toTimeString(),
        'cache_state_before_forget' => $beforeForget,
        'cache_state_after_forget' => $afterForget,
    ]);
});

Route::get('/force-refresh', function () {
    $beforeForget = [
        'main_item' => Cache::get('current-time'),
        'created_item' => Cache::get('current-time:created'),
    ];

    Cache::forget('current-time');
    Cache::forget('current-time:created');

    $afterForget = [
        'main_item' => Cache::get('current-time'),
        'created_item' => Cache::get('current-time:created'),
    ];

    return response()->json([
        'status' => 'ok',
        'message' => 'Cache forcefully refreshed',
        'time' => now()->toTimeString(),
        'cache_state_before' => $beforeForget,
        'cache_state_after' => $afterForget,
    ]);
});

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@rbadillap rbadillap marked this pull request as ready for review September 19, 2024 18:35
@rbadillap
Copy link
Contributor Author

Just noticed this PR fixes #52847

@rbadillap rbadillap marked this pull request as draft September 19, 2024 18:45
@rbadillap rbadillap marked this pull request as ready for review September 19, 2024 20:09
@timacdonald timacdonald self-assigned this Sep 19, 2024
@timacdonald
Copy link
Member

Marking this one as draft while we discuss it internally.

@timacdonald timacdonald marked this pull request as draft September 23, 2024 07:05
@timacdonald
Copy link
Member

I've opened another PR to address this issue: #52891

Thanks for letting us know about the problem.

@rbadillap rbadillap deleted the cache-forget-and-flexible branch October 4, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache::forget() doesn't delete cache created via Cache::flexible()
2 participants