max_stale broken?

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?

  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.

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?).

Thank you,

Alex.
Received on Tue Apr 26 2011 - 16:19:19 MDT