Skip to content

HTTP cache purgers error handling and performance issues #8591

Description

@silverbackdan

API Platform version(s) affected: 5.0.0

Description
SurrogateKeysPurger::purge() is the base of SouinPurger and VarnishXKeyPurger and contains this for purging

  foreach ($this->getChunkedIris($iris) as $chunk) {                                                                                                                                                                                   
      if (\strlen($chunk) > $this->maxHeaderLength) { throw new RuntimeException(...); }                                                                                                                                               
      foreach ($this->clients as $client) {                                                                                                                                                                                            
          $client->request($this->method, '', ['headers' => [$this->header => $chunk]]);                                                                                                                                               
      }                                                                                                                                                                                                                                
  }    

It presents a few issue that could be improved upon, some more serious than others.

  1. This stops purging after the first chunk's failure
  2. It's a partial purge even with a single cache. If there's more than one chunk (header over 1,500 bytes on Souin) and chunk 2 hits a transient 5xx, chunks 3 onwards are skipped.
  3. They run sequentially where we could have better performance if they were run in parallel
  4. "tag too long" RuntimeException is checked during the loop so earlier chunks may have already purged by the time this error is found.

How to reproduce
Theoretically we would reproduce by having a cache server (souin or varnish) throw errors on different chunks. This has become apparent in my application as the cache is made up of hundreds to thousands of individual small responses. This gives static website performance but the CMS benefits. However, on cache purges it starts to show a few holes in these functions and also a couple of issues raised with the cache packages too.

Possible Solution
I've begin to use AI, so here's the disclosure on the fix which I do agree with in theory and am happy to hear your thoughts too.

1. Validate first: check every chunk's length before sending anything, so the "too long" error purges nothing rather than part of the list.
2. Send everything, then collect:
   - Keep each response in $responses[] = $client->request(...) for every chunk and client. They go out concurrently.
   - Then loop over the responses, calling getStatusCode() inside a try, and gather any ExceptionInterface.
3. Report failures, choosing one of:
   - (a) BC-safe: rethrow the first exception after every request has been attempted. The exception type is unchanged, so no caller needs to change. The downside is that failures after the first are only visible if the caller logs.
   - (b) Richer: throw one new exception, e.g. PurgeFailedException, holding every failure. Callers catching HttpClient's ExceptionInterface would need to catch the new type, a small BC change, so it's better suited to a minor release.
4. Tests:
   - Two MockHttpClient clients where the first returns 500: the second must still receive the purge.
   - Three chunks where chunk 2 fails: chunk 3 is still sent.
   - An oversized chunk: nothing is sent at all.

I'd personally probably go straight in for B - I'm not sure why we would need to consider the new Exception a BC break, perhaps we could extend the existing Exceptions so anyone already relying on catching these would also get the new one?

Additional Context
Given a clear error response, we could also have a rich error which details which cache entries have failed to purge, and gives us more options on retries, logging and error tracing. It could also potentially tie into a profiler so we understand the cache status better after a request.

I've got a pile of work on but I will get said AI to write up a PR and read through it to see if I'd tend to agree with it as well and for additional thoughts on the issue and approach.

Ps. hello and sorry not to make it to APIP Con this year! Life is incredibly full-on but I do hope to make a return in 2027!

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions