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

Cache TTL seems to be overwritten by cache control headers #94

Closed
Tracked by #536
ungive opened this issue Jul 13, 2024 · 9 comments · Fixed by darkweak/souin#536
Closed
Tracked by #536

Cache TTL seems to be overwritten by cache control headers #94

ungive opened this issue Jul 13, 2024 · 9 comments · Fixed by darkweak/souin#536

Comments

@ungive
Copy link

ungive commented Jul 13, 2024

I have set my cache's TTL to 30 seconds:

{
    cache {
        ttl 30s
    }
}

The source responds with these headers:

age: 0
cache-control: max-age=120

The result is that the content is cached for 120 seconds, not 30 seconds.
Cache hit response (ttl > 30):

cache-status: Souin; hit; ttl=105; key=

Is this intended behaviour? How can I limit the caching duration? I don't really see a way to do it with the available configuration options.

Thank you!

@darkweak
Copy link
Collaborator

Hello @ungive that seems to be a bug. I will check in the core codebase and if that's true, I'll patch that and anyway I will write a unit test to be sure that doesn't happen.
Thank you for this feedback.

@darkweak
Copy link
Collaborator

@ungive I remembered that darkweak/souin#424 was the reason of this behaviour.
Fastly specifies that the max-age overrides the default TTL. What you can do is to drop the response Cache-Control HTTP header in the caddy configuration.

@ungive
Copy link
Author

ungive commented Jul 19, 2024

Unfortunately dropping the Cache-Control header is undesirable in my use case, as the source of it should have control over how long the response should be cached at most. Each response could theoretically have a different max-age value, some low, some too high. I just wanna cap it. I've solved it now by limiting the max-age on the server that delivers the response, which I fortunately can. Others might not have control over it though.

Fastly specifies that the max-age overrides the default TTL.

I don't quite understand what you mean by that. Do you mean the company Fastly? Do you mean that this is something that won't be fixed or that this just was a requirement for your use case once, so you implemented it like that?

Thanks for the help so far!

@darkweak
Copy link
Collaborator

@ungive I'm reading again the RFC and it specifies

If the max-age response directive (Section 5.2.2.1) is present, use its value

https://www.rfc-editor.org/rfc/rfc9111.html#name-calculating-freshness-lifet
So that's the RFC 😕

@ungive
Copy link
Author

ungive commented Jul 19, 2024

I suppose this would solve my problem:

If the Expires response header field (Section 5.3) is present, use its value minus the value of the Date response header field (using the time the message was received if it is not present, as per Section 6.6.1 of [HTTP]), or

https://www.rfc-editor.org/rfc/rfc9111.html#section-4.2.1-2.3

@darkweak
Copy link
Collaborator

Here is the complete order

If the cache is shared and the s-maxage response directive (Section 5.2.2.10) is present, use its value, or
If the max-age response directive (Section 5.2.2.1) is present, use its value, or
If the Expires response header field (Section 5.3) is present, use its value minus the value of the Date response header field (using the time the message was received if it is not present, as per Section 6.6.1 of [HTTP])

So that's

  1. s-maxage from the Cache-Control http response header
  2. max-age from the Cache-Control http response header
  3. Expires http response header

@ungive
Copy link
Author

ungive commented Jul 19, 2024

Right, 2. would be applied first, 3. wouldn't even be considered if max-age is set.

The order surprises my a bit though imo 3. should come before 2., at least in this context here. Which makes me think that the RFC just suggests the maximum that is allowed to be used to cache a response, to give a guideline for writing a system that caches as much as possible (since that improves performance). In that case the order doesn't really matter, since max-age and Expires are synonymous.

But with cache-handler and/or Souin I think we should have an option to limit how long responses are cached. In my case the origin server serves data I have no control over. It comes from users of my system. Theoretically my cache could be filled up over time if I don't enforce a maximum cache duration.

On a side note, what happens if the cache uses too much memory? Can you configure a memory limit and/or will the cache evict cached responses if available RAM is low?

@darkweak
Copy link
Collaborator

I can make the Expires used before max-age in the codebase if that suits to you.
We could ask to @mnot if there is a reason to respect the RFC order or if we're free to implement Expires treatment before max-age but I'm not confident about his time/availability to answer.

About the sidetone, that's not handled by Souin directly but by the storers themselves.

@darkweak
Copy link
Collaborator

@ungive again from the RFC 😕
If a response includes a Cache-Control header field with the max-age directive (Section 5.2.2.1), a recipient MUST ignore the Expires header field. Likewise, if a response includes the s-maxage directive (Section 5.2.2.10), a shared cache recipient MUST ignore the Expires header field. In both these cases, the value in Expires is only intended for recipients that have not yet implemented the Cache-Control header field.

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 a pull request may close this issue.

2 participants