The C1 fix from review #007 added keep_alive_interval(15s) +
keep_alive_timeout(60s) on the HTTP/2 builders. However,
keep_alive_timeout is the timeout for receiving a PONG response to a
PING, not an idle timeout. Well-behaved HTTP/2 clients (including
crawlers) respond to PINGs, resetting the timeout indefinitely. After
6 days of uptime, the proxy had 930+ idle connections (some 145 hours
old) that were never reaped.
Fix: add a custom idle timeout that tracks real request activity and
closes connections with no in-flight requests for longer than the
configured timeout, regardless of PING/PONG activity.
- IdleState: per-connection last_activity + in_flight counter
- IdleTrackingService: tower Service wrapper that updates last_activity
and in_flight on call() and on response completion (prevents killing
long-running requests like large git clones)
- idle_watchdog: races against serve_connection in tokio::select!,
fires only when in_flight == 0 and idle_for >= timeout
- HTTP/1.1 path: header_read_timeout already covers between-request
idle (verified in hyper source); watchdog is defense-in-depth
- HTTP/2 path: watchdog is the primary fix (hyper has no native
request-activity-based idle timeout)
Also refactored build_manual_server_config to extract
build_manual_server_config_from_certs for testability (in-memory
certs/keys for integration tests with rcgen).
Tests: 8 unit tests for IdleState/watchdog logic, 2 integration tests
verifying idle connections are closed and active ones are not.
Closes review #008.
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.
- Add reverse-proxy-4xx and reverse-proxy-badbots fail2ban filters
- Set backend=auto and ignoreip on all jails (fixes silent no-match
when defaults-debian.conf inherits systemd backend)
- Document three-jail setup and REQUEST log format in README
- Add review #007 covering connection lifecycle, logging, and deployment
drift triggered by the 2026-07-24 FD exhaustion incident
Security review #005 identified critical vulnerabilities in the Unix domain
socket admin API (C1 symlink race, C2 no auth, C3 info leak, W1-W7, S1-S6).
ADR-028 (already accepted) replaces the socket with an authenticated HTTP
admin API on the health check port. This commit adds the remaining spec work:
- ADR-029: Config file TOCTOU mitigation (mtime check on reload)
- ADR-030: Store cli_allow_wildcard_bind in ConfigReloadHandle for consistent
reload validation
- Implementation tasks for the admin HTTP migration (fix/admin-http-api),
TOCTOU fix (fix/config-reload-toctou), and wildcard flag fix
(fix/wildcard-flag-reload)
- Updated review #005 status to resolved with per-finding disposition
- Resolved OQ-16: POST for state-changing admin endpoints, GET for read-only
- Updated all architecture docs to reference new ADRs, use admin_key_path
instead of admin_socket_path, and reflect POST method for /admin/reload
Architecture updates based on gaps discovered during live deployment testing:
- ADR-023: HTTP/2 client-facing support via ALPN-based protocol detection.
The spec previously said HTTP/2 was out of scope, but the deployment
revealed that modern browsers negotiate HTTP/2 via ALPN. The proxy now
correctly detects the negotiated ALPN protocol and uses the appropriate
HTTP server builder (http2::Builder for h2, auto::Builder for http/1.1).
Upstream connections remain HTTP/1.1. Host resolution now falls back to
URI host for HTTP/2 :authority pseudo-headers.
- ADR-024: ANSI-disabled logging. All tracing-subscriber layers now use
with_ansi(false) to prevent ANSI escape codes in log output, which broke
fail2ban regex matching in Docker deployments. Also documents the fail2ban
regex anchor fix (^RATE_LIMIT → RATE_LIMIT).
Bug fixes found by architecture review:
- Fix missing ALPN protocols in manual TLS mode. build_manual_server_config
and build_multi_domain_server_config did not set alpn_protocols, meaning
manual TLS mode could not support HTTP/2. Added h2 and http/1.1 ALPN
entries to both functions (acme-tls/1 only in ACME mode).
- Fix missing with_ansi(false) in JSON log format. The init_json function
with file output did not disable ANSI on stdout or file layers, which would
break fail2ban in production JSON logging mode.
Other spec updates:
- All document statuses updated from draft to reviewed
- proxy.md: documented Server header removal, upstream HTTPS client,
two-phase timeout enforcement, HTTP/2 host resolution, connect timeout
- tls.md: documented ALPN configuration differing by mode (ACME vs manual)
- overview.md: added HTTP/2 client-facing support to scope, updated crate
deps (hyper-rustls, rustls-native-certs, hyper-util), clarified out-of-scope
- config.md: fixed http_port type (u16→u32) to match implementation, added
ANSI-disabled note for LoggingConfig
- operations.md: documented ANSI-disabled logging, fail2ban regex anchor
- open-questions.md: updated OQ-09 resolution (connect timeout fully
implemented), OQ-10 (C2 bug is fixed)
Resolve OQ-08 through OQ-12 after reviewing implementation findings:
- OQ-08: Remove /health route from the main HTTPS listener entirely.
Health checking belongs on port 9900 and admin socket only, not on
the public-facing proxy. This eliminates upstream collision problems
and special-case routing logic. (ADR-022)
- OQ-09: Not an architectural unknown — ADR-015 already decided on a
separate connect timeout. The implementation gap is a known issue.
- OQ-10: Not an open question — acme_contact is already specified as
required in config.md. The empty contact list is bug C2.
- OQ-11: Hardcoded is_https=true is correct for a TLS-terminating
proxy. HTTP listener redirects, doesn't proxy. Just needs a comment.
- OQ-12: Access logging is already specified as mandatory/always-on in
operations.md. Missing log_request! calls are bug W13.
Updated docs: proxy.md, operations.md, overview.md, config.md,
open-questions.md, README.md, ADR-013. Created ADR-022.
Analyzed 29 findings from the implementation review (002-implementation-review.md)
and identified 8 architecture-level concerns requiring spec changes:
Architecture gaps addressed:
- C2: Added acme_contact field to config.md, tls.md, and operations.md.
Let's Encrypt requires a contact email for production; the spec was missing
this required field.
- C4: Added StaticConfig drift tracking requirement to config.md reload
section. ConfigReloadHandle must update its stored StaticConfig after each
successful reload to prevent stale warnings.
- W1: Updated shutdown sequence in operations.md to specify that server tasks
should be joined (not aborted) during the drain window.
- W5: Added health check path collision note to proxy.md.
- W13: Clarified that access logging is always-on in operations.md.
- W14: Updated X-Forwarded-Proto description in proxy.md to clarify that it
is always 'https' since the HTTP listener redirects rather than proxies.
New open questions added:
- OQ-08: Should /health use a less common path to avoid upstream collision?
- OQ-09: How should upstream_connect_timeout_secs be enforced?
- OQ-10: Should ACME contact email be a required config field?
- OQ-11: How should X-Forwarded-Proto be derived per-listener?
- OQ-12: Should request access logging be mandatory or optional?
The remaining 21 findings are implementation-level bugs, code quality issues,
or Phase 2 improvements that don't require architecture spec changes.
W13: No request access logging - log_request! macro defined but never called
W14: is_https hardcoded to true on ProxyState - X-Forwarded-Proto always https
S9: Rate limiting silently bypassed when no client IP found
S10: Integration test TOML has [[listeners.listeners.sites]] typo
S11: No Server response header added by proxy (upstream's is stripped)
Comprehensive pre-implementation review of all architecture specs (overview,
proxy, tls, config, operations, 20 ADRs, open questions). Findings cover:
- Site routing model contradiction (per-listener vs global)
- X-Forwarded-For security model (edge proxy should replace, not append)
- Missing hop-by-hop header handling rules
- Undefined ACME failure behavior at startup/renewal
- Unspecified startup sequence and partial failure semantics
- Ambiguous per-listener vs shared router architecture
- Rate limiter state behavior on config reload
Plus warnings about admin socket protocol, Host header port handling,
port validation gaps, upstream format validation, TLS error handling,
shutdown draining, error response bodies, reload race conditions, and more.
- ADR-020: Document defense-in-depth rationale for running in a minimal
Docker container (memory-safe language + container isolation), flexible
upstream addressing (Docker DNS, loopback, LAN, tunnel endpoints),
file-primary logging for fail2ban, and volume mount strategy
- ADR-016: Add allow_wildcard_bind override for container deployments
where 0.0.0.0 is correct inside the container network namespace
- operations.md: Add container deployment section with Docker Compose
example, networking table, volume mounts, and health check integration;
flip logging to file-primary for fail2ban reliability; note systemd as
alternative to container deployment
- config.md: Restructure logging fields into nested LoggingConfig (matching
TOML [logging] section), add allow_wildcard_bind, shutdown_timeout_secs,
and log_file_path fields; clarify upstream addressing supports Docker
DNS and tunnel endpoints; update validation rule for 0.0.0.0 override
- overview.md: Update architecture diagram for container model with Docker
networking and volume mounts; add ADR-020 reference
- proxy.md: Clarify X-Forwarded-Proto is determined by listener port, not
hardcoded 80/443
- ADR-013: Fix health_check_port default contradiction (default is 9900,
not 0/disabled as previously stated)
Introduce [[listeners]] configuration to support both dedicated-IP
(1 IP = 1 cert = 1 domain) and shared-IP (SAN certificate) deployment
models. Each listener is an independent TLS endpoint with its own bind
address, TLS config, and site routing. OQ-07 is now resolved.
Changes:
- Add ADR-019 for multi-config listener support
- Update config format from [server] to [[listeners]] entries
- Update tls.md for per-listener TLS and certificate provisioning
- Update overview.md architecture diagram and scope
- Update proxy.md for per-listener HTTP redirect
- Fix stale references in ADR-010, ADR-011, ADR-016
- Update OQ-05 resolution (per-listener bind_addr supersedes)
- Add unique-host rationale to config validation rules
- Architecture review: fix all 3 critical and 6 warning issues
Promote multi-site support from Phase 2 to Phase 1 (ADR-010): the proxy
must support git.alk.dev and alk.dev from initial release. Add multi-domain
TLS configuration (ADR-011): acme_domains array replaces acme_domain string,
single SAN certificate via rustls-acme.
Key changes:
- ADR-010: Multi-site in Phase 1 — avoids config format migration later
- ADR-011: Multi-domain TLS — single SAN cert, acme_domains Vec<String>
- ADR-002: Updated rationale for multi-site (one upstream per domain)
- overview.md: Phase 1 now includes multi-site, alk.dev pass-through,
dual licensing (MIT OR Apache-2.0), real IP removed
- config.md: acme_domain → acme_domains, TOML example shows both sites,
validation adds unique host check, real IP replaced with 203.0.113.10
- tls.md: Multi-domain SNI section moved from Future to current, manual
mode uses ResolvesServerCert for SNI mapping, TOML header fixed
- proxy.md: Updated for multi-site, removed single-domain language
- operations.md: RFC 5737 documentation IPs, clarified rate limit eviction
semantics (distinct scan interval vs eviction age)
- open-questions.md: OQ-05 resolved (single bind_addr sufficient), new
OQ-07 (per-site TLS overrides)
Review fixes:
- acme_domains (plural) consistently used across all docs and diagram
- ADR-011 clearly scopes acme_domain as previous design
- Inline decision rationale extracted: tls.md hot-reload → ADR-004 ref,
config.md static/dynamic → ADR-008 ref
- TOML section headers consistent (server.tls)
