proxy_cache doesn't respect no-cache from error_page

[piotrprz@…]

I am trying to cache some 404 upstream responses, but not all of them. What's more, I want to use custom 404 error pages. So I have two kinds of requests:

• normal requests: client -> nginx -> proxy_cache -> upstream (normal processing) - standard caching proxy with proxy_intercept_errors
• conditional requests: client -> nginx -> proxy_cache -> upstream (always 404) - get data from cache only when already cached; return 404 when data is not in cache (in that case response body doesn't matter)

It's a bit simplified - I removed all custom logic and upstream balancer code that I add in OpenResty, and put add_header to simulate what I do for 404 responses that should not be cached.

My config:

http {
    proxy_cache_path cache_temp keys_zone=cache:10m;

    server {
        listen      80;

        location = /test {
            proxy_cache cache;
            proxy_cache_key $uri;
            proxy_pass http://localhost:8080;
            proxy_cache_valid 404 5s;

            add_header X-Cache-Status $upstream_cache_status always;
            add_header X-Upstream-Status $upstream_status always;

            proxy_intercept_errors on;
            error_page 404 /404.htm; # comment out this line and caching will be properly skipped
        }

        location = /404.htm {
            add_header X-Cache-Status $upstream_cache_status always;
            add_header X-Upstream-Status $upstream_status always;

            add_header X-Accel-Expires "0" always;
            add_header Cache-Control "no-cache" always;
            add_header Expires "0" always;
            add_header Via "*" always;

            return 404;
        }
    }

    server {
        listen 8080;

        add_header X-Accel-Expires "0" always;
        add_header Cache-Control "no-cache" always;
        add_header Expires "0" always;
        add_header Via "*" always;
        return 404;
    }
}

Now, according to my knowledge, when I issue GET ​http://localhost/test the request should go like this:

1. Request is sent to :8080, which returns 404 error
2. proxy_intercept_errors + error_page replace response with error page defined in /404.htm, which is fetched via internal subrequest
3. Cache-Control etc. should be respected and response should not be cached.

I already moved my special 404 processing to another internal location, so I went around this issue, but I'm reporting it anyway because it seems like a bug to me.

[Maxim Dounin]

Thanks for reporting this. Caching of intercepted error responses uses a special code path, and it uses proxy_cache_valid time configured regardless of any cache control parameters returned in the response. This is how it was introduced in revision ea908f6ae499. I don't think the behaviour is intentional though, this looks like a bug.

The following patch should fix it:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1505835337 -10800
#      Tue Sep 19 18:35:37 2017 +0300
# Node ID 48899ab9d89d654b1664233b9fa907f12667dd77
# Parent  bd2f5843e8cd968715d711b757a7210e26d24465
Cache: fixed caching of intercepted errors (ticket #1382).

When caching intercepted errors, previous behaviour was to use
proxy_cache_valid times specified, regardless of various cache control
headers present in the response.  Fix is to check u->cacheable and
use u->cache->valid_sec as set by various cache control response headers,
similar to how we do this in the normal caching code path.

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -2518,13 +2518,23 @@ ngx_http_upstream_intercept_errors(ngx_h
 #if (NGX_HTTP_CACHE)
 
             if (r->cache) {
-                time_t  valid;
-
-                valid = ngx_http_file_cache_valid(u->conf->cache_valid, status);
-
-                if (valid) {
-                    r->cache->valid_sec = ngx_time() + valid;
-                    r->cache->error = status;
+
+                if (u->cacheable) {
+                    time_t  valid;
+
+                    valid = r->cache->valid_sec;
+
+                    if (valid == 0) {
+                        valid = ngx_http_file_cache_valid(u->conf->cache_valid,
+                                                          status);
+                        if (valid) {
+                            r->cache->valid_sec = ngx_time() + valid;
+                        }
+                    }
+
+                    if (valid) {
+                        r->cache->error = status;
+                    }
                 }
 
                 ngx_http_file_cache_free(r->cache, u->pipe->temp_file);

[piotrprz@…]

Thanks, with this patch it works correctly.

[Maxim Dounin <mdounin@…>]

In 7117:dbd77a638eb7/nginx:

Cache: fixed caching of intercepted errors (ticket #1382).

When caching intercepted errors, previous behaviour was to use
proxy_cache_valid times specified, regardless of various cache control
headers present in the response. Fix is to check u->cacheable and
use u->cache->valid_sec as set by various cache control response headers,
similar to how we do this in the normal caching code path.

[Maxim Dounin]

Patch committed, thanks.

[Sergey Kandaurov <pluknet@…>]

In 1225:66426ca24671/nginx-tests:

Tests: intercepted errors with cache control (ticket #1382).