Re: max_stale broken?

 On Tue, 26 Apr 2011 10:18:50 -0600, Alex Rousskov wrote:
> Hello,
>
> Can somebody double check the following code, please?
>
>> /*
>> * At this point the response is stale, unless one of
>> * the override options kicks in.
>> * NOTE: max-stale config blocks the overrides.
>> */
>> int max_stale = (R->max_stale >= 0 ? R->max_stale :
>> Config.maxStale);
>> if ( max_stale >= 0 && staleness < max_stale) {
>> debugs(22, 3, "refreshCheck: YES: max-stale limit");
>> if (request)
>> request->flags.fail_on_validation_err = 1;
>> return STALE_MAX_STALE;
>> }
>
>
> It makes no sense to me, and I see 60-second stale objects being
> declared STALE_MAX_STALE with the default configuration where
> max-stale
> is 1 week. The code declares the object stale if the object
> "staleness"
> is _less_ than the configured maximum. I would expect the opposite!
>
> It is not clear from the squid.conf documentation what the exact
> intent
> of the max-stale option is with the relation to other checks such as
> refresh_pattern.max, but at the minimum, should not the comparison be
> reversed?

 Yes. This appears to be wrong. Frankly I'm a little confused as to how
 it got that way. I did test this rather thoroughly since it was a tricky
 directive when I started the portage.

 "max_stale" directive is a global default. "refresh_pattern
 max-stale=N" is a specific override of the global. Cache-Control weighs
 in occasionally *if* it is smaller than either of the config values.

 All of them are "up to" limits in accordance with RFC 2616. Where the
 client MAY receive a stale object UP TO an unspecified configured limit.
 (Thus we can avoid violations and Cache-Control weighing in if the local
 admin wants shorter staleness).

 They all have no effect on 1xx-4xx server responses or fresh HITs. Only
 on 5xx when revalidating a stale HIT.

>
> if (max_stale >= 0 && staleness > max_stale)
> ...
> return STALE_...
>
> Please note that since Config.maxStale defaults to one week, the
> max_stale variable is not negative and the above check kicks in for
> all
> stale objects by default, no matter what refresh_pattern max is!
> Again,
> it is not clear to me whether that is intentional. If it is, we
> should
> document this surprising effect better.

 The limit is min(config, Cache-Control: max-stale). The above if() is
 supposed to be a shortcut for min().

 "1 week" is an arbitrary value copied from squid-2. It could be up to a
 year, but IMO having a server 5xx'ing for a whole week before anyone
 notices is already a bad situation.
  FWIW there was no limit on staleness prior to the portage. 3.0 and 3.1
 will simply emit stale HITs *forever* or until the validation succeeds.

>
> Another option would be to make the check specific to sf.max cases:
>
> if (sf.max && max_stale >= 0 && staleness <= max_stale)
> ...
> return FRESH_...
>
> but that has problems of its own (why would we need max_stale
> suboption
> in refresh_pattern then?).

 ie with the config:
  max_stale 3600 second
  refresh_pattern *.html 0 100% 4320 max-stale=6000

 When cache-control *does not* get involved object /index.html should be
 stale HIT for 6000 seconds. Whereas /image.jpg could be stale HIT for up
 to 3600 seconds.

 When cache-control *does* get involved things get a bit tricky and
 dodgy. The idea is that min(config, Cache-Control) limit is used, but
 not strictly since the refresh_pattern estimation may not be applied.

 Amos
Received on Tue Apr 26 2011 - 23:23:51 MDT