Nginx ignores proxy_no_cache

[Maxim Dounin]

Hello!

On Thu, Jan 30, 2025 at 11:01:32AM +0200, Clima Gabriel wrote:

> Hello Maxim,
> Hope this helps.
> We encounter disk failures fairly often and what the kernel will do most of
> the time is re-mount the disk as read-only.
> What I did was add this check which checks if the disk is healthy before
> hand and executes the proxy_no_cache path if it is.
> It doesn't cover all the possible disk failures, like sometimes you'll just
> get IO errors and still return 5XX to clients. But to cover all cases a
> much cleverer reshuffling of the code is needed.
> 
> 
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> index d7f427d50..839ed6c0d 100644
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -8,6 +8,7 @@
>  #include <ngx_config.h>
>  #include <ngx_core.h>
>  #include <ngx_http.h>
> +#include <sys/statvfs.h>
> 
>  #include <ngx_http_proxy_module.h>
> 
> @@ -3424,6 +3425,16 @@ ngx_http_upstream_send_response(ngx_http_request_t
> *r, ngx_http_upstream_t *u)
>          break;
> 
>      default: /* NGX_OK */
> +        if (r->cache) {
> +            struct statvfs  fs;
> +            if (statvfs((char *)r->cache->file_cache->path->name.data,
> &fs) == -1) {
> +                return NGX_ERROR;
> +            }
> +            if ((fs.f_flag & ST_RDONLY) != 0) {
> +                u->cacheable = 0;
> +                break;
> +            }
> +        }
> 
>          if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {

I cannot say I like this approach.

Rather, I would recommend adding an explicit test via the 
proxy_no_cache configuration directive if that's an expected state 
in your setup.  Right now this certainly can be done with the 
embedded Perl module, but we can consider extending "if" with "-r" 
and "-w" tests to make things easier.

Alternatively, we can consider handling (at least some) cache 
access failures gracefully and ensure that we'll fall back to 
normal proxying if this happens.  This wasn't done previously to 
keep the code simple, but can be reconsidered if there is an 
understanding that this is quite important in some setups and 
there is a simple enough way to handle such failures.

Please also note that right now even proxying won't work if 
a temporary file is needed and cannot be created, as file access 
failures are considered fatal.

OTOH, my personal preference is to keep disks mirrored, this 
ensures that a single disk failure won't affect server operations 
and provides better performance as a bonus.

(Note well that the code in question was modified in freenginx to 
address the issue reported by Kirill in the thread you are 
replying to, and your patch won't apply, see 
https://freenginx.org/hg/nginx/rev/c5623963c29e for details.)

[...]

-- 
Maxim Dounin
http://mdounin.ru/