magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Built-in FPC cache is broken in 2.4.7 for some configurations

Open thlassche opened this issue 1 year ago • 11 comments

Preconditions and environment

  • Magento version: 2.4.7
  • Use built in FPC
  • Have aMAGE_RUN_CODE parameter set

Steps to reproduce

  1. Enable built-in FPC
  2. Have a MAGE_RUN_CODE parameter in your $_SERVER array
  3. Load a frontend page
  4. Load the same page again

Expected result

Page is cached, and in developer mode a header X-Magento-Cache-Debug: HIT is set

Actual result

Page is uncached, and in developer mode a header X-Magento-Cache-Debug: MISS is set

Additional information

In 2.4.7 the identifier for saving a page to cache was decoupled from the fetching identifier:

https://github.com/magento/magento2/blob/725b6483932b4bbbbd9f463de80c82914346643d/app/code/Magento/PageCache/Model/App/Request/Http/IdentifierForSave.php#L18

However the fetching identifier has plugins on it that prepend the cache key with some parameters:

https://github.com/magento/magento2/blob/725b6483932b4bbbbd9f463de80c82914346643d/app/code/Magento/PageCache/Model/App/CacheIdentifierPlugin.php#L67

That plugin is not added to the saving part, hence the key will always mismatch if there are additional parameters

Release note

No response

Triage and priority

  • [ ] Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • [ ] Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • [x] Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • [ ] Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • [ ] Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

thlassche avatar Apr 17 '24 06:04 thlassche

Hi @thlassche. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:


Join Magento Community Engineering Slack and ask your questions in #github channel. :warning: According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting. :clock10: You can find the schedule on the Magento Community Calendar page. :telephone_receiver: The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

m2-assistant[bot] avatar Apr 17 '24 06:04 m2-assistant[bot]

Workaround, add this in a module:

<namespace>\<module>\Plugin\CacheIdentifierPlugin.php
<?php
namespace <namespace>\<module>\Plugin;
use Magento\PageCache\Model\App\Request\Http\IdentifierForSave;
use Magento\Store\Model\StoreManager;

class CacheIdentifierPlugin
{
    /**
     * @var \Magento\Framework\View\DesignExceptions
     */
    private $designExceptions;

    /**
     * @var \Magento\Framework\App\RequestInterface
     */
    private $request;

    /**
     * @var \Magento\PageCache\Model\Config
     */
    private $config;

    /**
     * @param \Magento\Framework\View\DesignExceptions $designExceptions
     * @param \Magento\Framework\App\RequestInterface $request
     * @param \Magento\PageCache\Model\Config $config
     */
    public function __construct(
        \Magento\Framework\View\DesignExceptions $designExceptions,
        \Magento\Framework\App\RequestInterface $request,
        \Magento\PageCache\Model\Config $config
    ) {
        $this->designExceptions = $designExceptions;
        $this->request = $request;
        $this->config = $config;
    }
    
    public function afterGetValue(IdentifierForSave $identifier, $result)
    {
        if ($this->config->getType() === \Magento\PageCache\Model\Config::BUILT_IN && $this->config->isEnabled()) {
            $identifierPrefix = '';
            /* @phpstan-ignore-next-line */
            $ruleDesignException = $this->designExceptions->getThemeByRequest($this->request);
            if ($ruleDesignException !== false) {
                $identifierPrefix .= 'DESIGN' . '=' . $ruleDesignException . '|';
            }

            /* @phpstan-ignore-next-line */
            if ($runType = $this->request->getServerValue(StoreManager::PARAM_RUN_TYPE)) {
                $identifierPrefix .= StoreManager::PARAM_RUN_TYPE . '=' .  $runType . '|';
            }

            /* @phpstan-ignore-next-line */
            if ($runCode = $this->request->getServerValue(StoreManager::PARAM_RUN_CODE)) {
                $identifierPrefix .= StoreManager::PARAM_RUN_CODE . '=' . $runCode . '|';
            }

            return $identifierPrefix . $result;

        }
        return $result;
    }
}
<namespace>\<module>\etc\frontend\di.xml
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">

    <type name="Magento\PageCache\Model\App\Request\Http\IdentifierForSave">
        <plugin name="core-app-area-design-exception-plugin"
                type="<namespace>\<module>\Plugin\CacheIdentifierPlugin" sortOrder="10"/>
    </type>
</config>


thlassche avatar Apr 17 '24 06:04 thlassche

Hi @engcom-Hotel. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

  • [ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
  • [ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
  • [ ] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
  • [ ] 4. Verify that the issue is reproducible on 2.4-develop branch
    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
  • [ ] 5. Add label Issue: Confirmed once verification is complete.
  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Apr 18 '24 05:04 m2-assistant[bot]

Hello @thlassche,

Thanks for the report and collaboration!

We have tried to reproduce the issue in both 2.4.7 and 2.4-develop and the issue is reproducible in both instances.

But if we remove MAGE_RUN_CODE, the X-Magento-Cache-Debug is showing as HIT. Please refer to the below screenshot for reference:

With MAGE_RUN_CODE 247

Without MAGE_RUN_CODE 247_wo_mageruncode

Hence confirming the issue.

Thanks

engcom-Hotel avatar Apr 18 '24 07:04 engcom-Hotel

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-11819 is successfully created for this GitHub issue.

github-jira-sync-bot avatar Apr 18 '24 07:04 github-jira-sync-bot

:white_check_mark: Confirmed by @engcom-Hotel. Thank you for verifying the issue.
Issue Available: @engcom-Hotel, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

m2-assistant[bot] avatar Apr 18 '24 07:04 m2-assistant[bot]

@magento I am working on this

digitalrisedorset avatar Apr 22 '24 11:04 digitalrisedorset

I added a fix with the PR 38646. There is no automated tests at this time but the fix was added in the 2.4-develop branch and I verified both 2.4.7 and 2.4-develop had this issue

digitalrisedorset avatar Apr 22 '24 14:04 digitalrisedorset

Cheers @digitalrisedorset

magento-module-page-cache-fpc-identifier.patch

ThisIsRuddy avatar Apr 29 '24 08:04 ThisIsRuddy

Hi,

Internal team has started to work on it

Thanks.

engcom-Bravo avatar May 10 '24 06:05 engcom-Bravo

Cheers @digitalrisedorset

magento-module-page-cache-fpc-identifier.patch

@digitalrisedorset I think the file Identifier.php is missing in the patch file. Also the line <preference for="Magento\Framework\App\PageCache\IdentifierInterface" type="Magento\PageCache\Model\App\Request\Http\Identifier"/> is missing in the di.xml file.

sanmic avatar Jun 28 '24 22:06 sanmic

Hello,

As I can see this issue got fixed in the scope of the internal Jira ticket AC-11819 by the internal team Related commits:https://github.com/search?q=repo%3Amagento%2Fmagento2+AC-11819&type=commits

Based on the Jira ticket, the target version is 2.4.8-beta1.

Thanks

engcom-Bravo avatar Jul 04 '24 06:07 engcom-Bravo

@engcom-Bravo Question for some clarity; Can we expect this to land in 2.4.7-p3 as well? If not; can there be an official patch made? The 'related commits' link includes some 20k+ line changes...

vandijkstef avatar Aug 28 '24 13:08 vandijkstef

Looking at the commits, 2 of them seem to be where most of the PR size comes from. I'd be happy to compile a patch for 2.4.7-p3 that would only include the files that make the fix working. However, we have 2 caveats with this idea:

  1. I am no official and I can only offer a fix that I can suggest to the team but cannot assure it will be approved. This patch would only include the first and last commits since I see them as needed and don't see the rest as related
  2. I believe the PR I made had some side-effects with functional tests. If this is the case, that would explain the 2 other commits. For this second point, the patch would fix the problem but we could have to live with some functional tests failing

If you wanted to try this path, I'd be happy to test a possible patch. It would be ideal to be able to liaise with your team when the patch is on the test environment to consolidate the patch integration and possibly help for the official approval to take place.

digitalrisedorset avatar Aug 28 '24 15:08 digitalrisedorset

@engcom-Hotel Some clarity on this issue please? Did this somehow hit 2.4.7-p3 undocumented? Or can we expect a patch for the 2.4.7 release line anywhere before april 2025?

Alternatively, do I need to accept that 2.4.7 just is 'screwed' unless we manually override half the core, potentially drastically increasing complexity for future updates?

vandijkstef avatar Oct 29 '24 16:10 vandijkstef

Hello @vandijkstef,

Can you please confirm if this issue can be reproducible for you with Redis in the cloud or Varnish configured in your instance?

Thanks

engcom-Hotel avatar Oct 30 '24 10:10 engcom-Hotel

@engcom-Hotel I can confirm this issue is existing in mage-2.4.7 and did not magically disappear.

Can you confirm if the resolution of this issue will be (or has been) included in any of the releases in the 2.4.7 line?

vandijkstef avatar Oct 31 '24 12:10 vandijkstef

Hello @vandijkstef,

Actually, we are in discussion with PO for this. But is it reproducible for you Redis in the cloud or Varnish configured in your instance? Can you please confirm the same?

Thanks

engcom-Hotel avatar Nov 04 '24 06:11 engcom-Hotel

I can confirm that on my multiwebsite/multistore Magento 2.4.7-p3 with Redis and Varnish, even with applied patch I still have the issue. On the Redis service restart the system works normally for about 5 minutes, and they you notice significant perormance drop, especially on Add To Cart CTA. I do hope that official patch will come out sooner than later because we cannot wait for official patch to come out in April with Magento 2.4.8

And I am affraid to revert Magento to previous version due to CosmicSting issue 🗡️ :)

Crashead avatar Nov 14 '24 21:11 Crashead

@engcom-Hotel

I can confirm that the specific issue appears on Magento 2.4.7 with FPC implemented on both Files and Redis. I have not tested Magento 2.4.7 with Varnish. We always talk about multi-store implementations where the routing to the appropriate store view is regulated by the .htaccess file with directives such as the following ones:

SetEnvIf Host fr.dlastore.com MAGE_RUN_CODE=french_dla SetEnvIf Host fr.dlastore.com MAGE_RUN_TYPE=store

I believe that an official patch should be issued for this problem. There must be many store owners with multi-store implementations who may have upgraded to Magento 2.4.7 and they have not noticed that their store actually works without FPC.

There is only one 3rd party FPC module which works with Magento 2.4.7, but it has other problems, and for this reason I would not like to mention which is that very module.

dandrikop avatar Nov 16 '24 13:11 dandrikop

any update for this? We have the same problem on 2.4.7-p4

agendartobias avatar Apr 10 '25 07:04 agendartobias

any update for this? We have the same problem on 2.4.7-p4

2.4.8 release notes: AC-11819: Built-in FPC cache is broken in 2.4.7 for some configurations

Fix note: The system now correctly caches pages when the MAGE_RUN_CODE parameter is set, ensuring optimal performance. Previously, pages were not being cached under these conditions, leading to potential performance issues. GitHub issue: https://github.com/magento/magento2/issues/38626 GitHub code contribution: https://github.com/magento/magento2/pull/38646, https://github.com/magento/magento2/commit/0c53bbf7

sanmic avatar Apr 10 '25 07:04 sanmic

Hey, thank you, but is there Quality patch for 2.4.7?

agendartobias avatar Apr 10 '25 07:04 agendartobias

Hi, feel freel to reach out to me on slack and I can support the fix that I made initially. I have lost track whether it was merged but I do understand the changes since it was my commit and it is possible to integrate them in your install @agendartobias

digitalrisedorset avatar Apr 10 '25 14:04 digitalrisedorset

Hello @digitalrisedorset,

I have not understood the roadmap of this Github issue. Is there any official patch for Magneto 2.4.7 regarding the FPC problem reported herein?

dandrikop avatar May 20 '25 12:05 dandrikop

I have helped someone on this issue and it seems there are out there some Magento modules that make this issue not pressing to be resolved. The fix can be integrated in your website reasonably easily. But like all things with Magento, if the installed modules in your site interacts with the cache in some ways, then the fix may not be applicable. I am happy to take a look at your site and help you if applicable @dandrikop

  • also, the fix only works if your FPC uses Varnish?

digitalrisedorset avatar May 20 '25 14:05 digitalrisedorset

@digitalrisedorset, @agendartobias, @sanmic, @thlassche, @engcom-Hotel, I tried the patch 315dd68 with Magento 2.4.7-p5 that is supposed to fix the problem on Magneto 2.4.8. Although this patch was successfully installed, the problem persisted on Magento 2.4.7-p5 multi-store implementation. Indeed, if the quality patch ACSD-62591 for Magento 2.4.7 has been installed, it should be reverted; otherwise the 315dd68 cannot be installed.

In any case, things became worse with 315dd68 on Magento 2.4.7 comparing with ACSD-62591. Namely, the pages were not cached in FPC at all with 315dd68.

dandrikop avatar May 20 '25 21:05 dandrikop

@dandrikop so if i revert ACSD-62591 for Magento 2.4.7 and patch it with #315dd68 FPC works for multistore?

agendartobias avatar May 21 '25 06:05 agendartobias

@agendartobias, the situation with Magento 2.4.7-p5 and multi-store configuration is as follows:

  1. If no patch is installed, the FPC does not work at all; i.e. the pages are actually cached by FPC but when revisited, Magento cannot locate them in FPC so as to bring them from there. Indeed, if you're on development mode, the HTTP header X-Magento-Cache-Debug is always MISS.
  2. If you install the quality patch ACSD-62591, the problem with the HTTP header X-Magento-Cache-Debug is resolved. However, given that the cache keys do not include the environmental variables MAGE_RUN_CODE and MAGE_RUN_TYPE, as it happened till Magento 2.4.6, some Full Page Cache Warmers don't work.
  3. If you revert the quality patch ACSD-62591, so as to install the patch 315dd68, then you actually return to point 1 above.

Given that the commit 315dd68 is supposed to resolve the problem on Magento 2.4.8, but it didn't work on Magento 2.4.7, this situation makes me think that the problem may also persist on Magento 2.4.8; i.e. FPC somehow works, but not 3rd party modules related with Full Page Cache Warming.

dandrikop avatar May 21 '25 06:05 dandrikop