Fix connection lifecycle, pool bounds, and log rotation (review #007)
Critical: - C1: Set server-side idle/keep-alive timeouts on both hyper builders (http2 keep_alive_interval=15s + keep_alive_timeout, http1 header_read_timeout). Both builders now set TokioTimer (required to avoid runtime panic). Prevents FD exhaustion from abandoned TLS connections — the root cause of the 2026-07-24 outage. - C2: Add Semaphore(max_connections) gating the accept loop. Provides backpressure via OS TCP backlog when all permits are taken. Warnings: - W1: Add SIGUSR1 log-reopen handler. New ReopenableFileWriter (Arc<ArcSwap<File>> via custom MakeWriter) atomically swaps the log file. Enables postrotate logrotate without copytruncate, which caused the 1.15GB sparse file that wedged fail2ban. - W2: Set pool_max_idle_per_host(10) on both upstream clients, bounding idle upstream connections per host. - W3: Add connection_idle_timeout_secs to StaticConfig (default 60). - W4: Add max_connections to StaticConfig (default 1024). Both new fields are validated (> 0) and included in static config drift detection on reload. Docs (config.md, README, ADR-009) updated.
This commit is contained in:
@@ -90,6 +90,8 @@ Immutable after startup. Changes require a process restart.
|
||||
| `health_check_port` | `u16` | Port for local health check endpoint (default: `9900`; set to `0` to disable; bound to `127.0.0.1` only; see ADR-013, ADR-022) |
|
||||
| `admin_key_path` | `String` | Path to file containing the admin Bearer token (default: `/etc/reverse-proxy/admin-key`; empty string to disable admin endpoints; see ADR-028) |
|
||||
| `shutdown_timeout_secs` | `u64` | Maximum seconds to wait for in-flight requests during graceful shutdown (default: `30`) |
|
||||
| `connection_idle_timeout_secs` | `u64` | Server-side idle timeout for client TLS connections. Idle HTTP/2 connections are closed after this duration (with keep-alive pings at 15s intervals to detect dead peers). HTTP/1.1 connections are closed if the client doesn't send a complete request header within this duration. Prevents FD exhaustion from abandoned connections (default: `60`; must be > 0; see review #007 C1) |
|
||||
| `max_connections` | `usize` | Maximum number of concurrent client TLS connections. When the limit is reached, new connections wait in the OS TCP backlog until a slot frees (default: `1024`; must be > 0; see review #007 C2) |
|
||||
| `logging` | `LoggingConfig` | Logging configuration (see below) |
|
||||
|
||||
**LoggingConfig** (nested in `[logging]` TOML section):
|
||||
@@ -105,9 +107,11 @@ This is critical for fail2ban regex matching and Docker log output (see ADR-024)
|
||||
Both text and JSON formats produce plain-text output without color codes.
|
||||
|
||||
**Note**: The entire `LoggingConfig` (including `log_file_path`) is static and
|
||||
requires a process restart to change. Log file path changes require reopening
|
||||
file handles, which is complex and low-value for Phase 1. Log rotation (Phase 2)
|
||||
will be handled via signal-based or built-in rotation.
|
||||
requires a process restart to change. However, the log file can be reopened
|
||||
without restarting by sending `SIGUSR1` to the process — this closes the
|
||||
current file handle and opens a new one at the same path. This enables
|
||||
standard `postrotate` logrotate configs (rename + signal) without the
|
||||
`copytruncate` workaround that creates sparse files. See review #007 W1.
|
||||
|
||||
**ListenerConfig** (per-listener static config):
|
||||
|
||||
@@ -180,6 +184,8 @@ Phase 2.
|
||||
| `health_check_port` | `u16` | `9900` | No |
|
||||
| `admin_key_path` | `String` | `/etc/reverse-proxy/admin-key` | No |
|
||||
| `shutdown_timeout_secs` | `u64` | `30` | No |
|
||||
| `connection_idle_timeout_secs` | `u64` | `60` | No |
|
||||
| `max_connections` | `usize` | `1024` | No |
|
||||
| `logging.level` | `String` | `"info"` | No |
|
||||
| `logging.format` | `String` | `"text"` | No |
|
||||
| `logging.log_file_path` | `String` | (not set) | No |
|
||||
@@ -306,6 +312,8 @@ certificate:
|
||||
# Global settings
|
||||
health_check_port = 9900 # Local health check (0 to disable)
|
||||
admin_key_path = "/etc/reverse-proxy/admin-key" # Empty string to disable
|
||||
# connection_idle_timeout_secs = 60 # Server-side idle timeout (default: 60)
|
||||
# max_connections = 1024 # Max concurrent TLS connections (default: 1024)
|
||||
|
||||
[logging]
|
||||
level = "info"
|
||||
@@ -446,8 +454,13 @@ On startup, the config is validated:
|
||||
email) or `"mailto:user"` (no `@`) are rejected. Let's Encrypt requires
|
||||
a contact email for production certificate requests.
|
||||
20. `admin_key_path` must be either an empty string (disabled) or an absolute
|
||||
path. Relative paths and paths containing `..` are rejected. This prevents
|
||||
path traversal attacks on the admin key file.
|
||||
path. Relative paths and paths containing `..` are rejected. This prevents
|
||||
path traversal attacks on the admin key file.
|
||||
21. `connection_idle_timeout_secs` must be > 0. A zero value would disable
|
||||
the server-side idle timeout, reintroducing the FD exhaustion bug from
|
||||
review #007 C1.
|
||||
22. `max_connections` must be > 0. A zero value would deadlock the connection
|
||||
semaphore, preventing any client connection from being accepted.
|
||||
|
||||
On SIGHUP reload, the same validation applies. If the new config fails
|
||||
validation, the reload is rejected and the old config remains active. An error
|
||||
|
||||
@@ -10,6 +10,8 @@ The proxy needs to handle Unix signals for:
|
||||
- **Graceful shutdown**: SIGTERM and SIGINT should stop accepting new
|
||||
connections, drain in-flight requests, then exit.
|
||||
- **Config reload**: SIGHUP should trigger a DynamicConfig reload from disk.
|
||||
- **Log reopen**: SIGUSR1 should close and reopen the log file, enabling
|
||||
`postrotate` logrotate configs without `copytruncate` (see review #007 W1).
|
||||
|
||||
Two approaches for signal handling:
|
||||
- **`tokio::signal`**: Built into tokio. Handles SIGTERM and SIGINT via
|
||||
@@ -22,6 +24,7 @@ Two approaches for signal handling:
|
||||
Use `signal-hook` for all signal handling. Specifically:
|
||||
- `signal-hook::flag` to set termination flags on SIGTERM/SIGINT
|
||||
- `signal-hook` to register a SIGHUP handler that triggers config reload
|
||||
- `signal-hook` to register a SIGUSR1 handler that reopens the log file
|
||||
|
||||
`tokio::signal::ctrl_c()` is registered as a secondary shutdown trigger; both
|
||||
mechanisms converge on the same shutdown path. This is a belt-and-suspenders
|
||||
@@ -34,6 +37,10 @@ The shutdown sequence:
|
||||
for in-flight requests to complete, then exit with code 0.
|
||||
2. On SIGHUP: re-read config file, validate, and swap DynamicConfig if valid.
|
||||
Log the result.
|
||||
3. On SIGUSR1: close the current log file handle and open a new one at the
|
||||
same path. Enables standard `postrotate` logrotate configs (rename + signal)
|
||||
without `copytruncate`, which creates sparse files when the FD offset is
|
||||
high. See review #007 W1.
|
||||
|
||||
## Rationale
|
||||
|
||||
|
||||
Reference in New Issue
Block a user