garethgeorge / backrest

Backrest is a web UI and orchestrator for restic backup.
GNU General Public License v3.0
1.06k stars 34 forks source link

Send keep-alive messages in streams #394

Open edvgui opened 1 month ago

edvgui commented 1 month ago

Is your feature request related to a problem? Please describe. By default, nginx sets the proxy_connect_timeout to 60s:

Syntax: proxy_read_timeout time;
Default: proxy_read_timeout 60s;
Context: http, server, location

Defines a timeout for reading a response from the proxied server.
The timeout is set only between two successive read operations,
not for the transmission of the whole response.
If the proxied server does not transmit anything within this time,
the connection is closed.

http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout

If I understood correctly, the GetOperationEvents call sets up a stream between the client and the server, and the server will post messages on it whenever something is happening which is worth notifying the client about. If there is nothing interesting, the server doesn't send anything. This means that if nothing happens for more that a minute, no message is sent for more than a minute, and nginx will terminate the connection, resulting in a 504 which the web ui doesn't appreciate too much:

operations stream died with exception:  ConnectError: [unavailable] HTTP 504

It does however recover from it as it starts a new connection right away.

Describe the solution you'd like I think it would be nice to send keep-alive messages at regular intervals (i.e. 30 seconds, to make it work with default nginx settings) to make sure the connection doesn't die.

As a bonus, it would be even better if this keep-alive was configurable (via env vars).

Additional context

edvgui commented 1 month ago

A workaround that seems to work is to increase the read timeout to 1d in nginx config.

proxy_read_timeout 1d;

:point_up: Note that I didn't wait that long to confirm it works 'till the end. :v: Even with this easy configuration change on nginx side, I think it could be worth adding this keep-alive here, if it doesn't make things to complicated on the server side.

garethgeorge commented 1 month ago

Definitely open to this, sounds worthwhile for folks behind reverse proxies (which I suspect is common) and is pretty easy to adapt into the existing protocol (keep-alives can just be empty event messages).

It's actually quite nice that you file this now, this is something I'll also want to give some thought to when designing the sync protocol for multi-host management, though in that case I suspect I'll definitely want to make keep alives configurable (60s is probably too frequent).

garethgeorge commented 1 week ago

Implemented an every 60 seconds keepalive in https://github.com/garethgeorge/backrest/commit/79cae5bac39225560966bb4f7bdbf34a525837f7 as a part of some related changes