[PATCH] Cache: re-fetch responses cached with invalid HTTP header

[Maxim Dounin]

Hello!

On Fri, Jul 10, 2015 at 03:09:58PM -0700, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1436566103 25200
> #      Fri Jul 10 15:08:23 2015 -0700
> # Node ID 596ce484f8b5519df8803754f093e37b26806699
> # Parent  dcae651b2a0cbd3de2f1fd5cf5b8c72627db94fd
> Cache: re-fetch responses cached with invalid HTTP header.
> 
> Previously, such responses would be read into the buffer, fail,
> then nginx would connect to the upstream to fetch new response,
> but that response would be read into the same buffer, after the
> invalid response, leading to a persistent failure state.
> 
> This patch resets buffer, so that the new response can be used.
> 
> Signed-off-by: Piotr Sikora <piotr at cloudflare.com>
> 
> diff -r dcae651b2a0c -r 596ce484f8b5 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c	Tue Jul 07 16:38:49 2015 +0300
> +++ b/src/http/ngx_http_upstream.c	Fri Jul 10 15:08:23 2015 -0700
> @@ -843,6 +843,20 @@
>              return rc;
>          }
>  
> +        u->cache_status = NGX_HTTP_CACHE_MISS;
> +
> +        /* fall through */
> +
> +    case NGX_DECLINED:
> +
> +        if ((size_t) (u->buffer.end - u->buffer.start) < u->conf->buffer_size) {
> +            u->buffer.start = NULL;
> +
> +        } else {
> +            u->buffer.pos = u->buffer.start + c->header_start;
> +            u->buffer.last = u->buffer.pos;
> +        }
> +
>          break;
>  
>      case NGX_HTTP_CACHE_STALE:

This change doesn't looks sufficient to properly handle the 
problem.  At least I see duplicate headers in a first response 
when the code is triggered:

$ curl -i http://127.0.0.1:8080/
HTTP/1.1 200 OK
Server: nginx/1.9.3
Date: Mon, 13 Jul 2015 20:03:10 GMT
Content-Type: text/html
Content-Length: 1047
Connection: keep-alive
Last-Modified: Mon, 13 Feb 2012 01:20:52 GMT
ETag: "4f386574-417"
Last-Modified: Mon, 13 Feb 2012 01:20:52 GMT
ETag: "4f386574-417"
Accept-Ranges: bytes

I haven't digged into details, but likely it's because headers 
from a cached invalid response were already pushed to 
r->headers_out.

It might be better idea to actually implement what's already 
marked as TODO in ngx_http_upstream_cache_send().

    /* TODO: delete file */

-- 
Maxim Dounin
http://nginx.org/