Mailroom fd hygiene#1610
Conversation
Coverage Report for CI Build 28010466432Coverage decreased (-0.03%) to 85.181%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
5688896 to
d7b59f2
Compare
a777a52 to
7c52590
Compare
7c52590 to
44b06a9
Compare
|
rebased with new lockfile updates |
caarloshenriq
left a comment
caarloshenriq
left a comment
There was a problem hiding this comment.
tACK 44b06a9
I tested it locally, and everything works as expected.
There was a problem hiding this comment.
soft NACK on the approach:
This is roughly 1000 lines of symptom management standing in front of a root cause that the PR itself, and #1608 before it, names as the actual fix. I don't think we should land this before we try multiplexing. The last commit mentions ~1009 connections from a single (presumably well-behaved) relay. Multiplexing should collapse that to a handful.
#1608 already fixed the immediate fire, so why not try the proper fix before shipping another stopgap?
There are two components of this PR that are worth keeping: the bootstrap timeout and cap (first commit) and the connection metrics to confirm the fix we end up shipping works. A single trivial global connection cap as a DoS backstop is fine by me too, since multiplexing only helps cooperative peers and won't stop an adversary opening raw sockets.
On another note, there are a bunch of unrelated formatting changes and confusing commit sequencing (e.g. commit 2 rewrites a lot of commit 1) that harm the readability of this PR as is.
edit: implemented relay -> directory multiplexing here for further discussion
CONNECT and WebSocket bootstrap tunnels proxied bytes with no timeout and were spawned without any concurrency limit. Each tunnel pins two file descriptors (the inbound upgraded socket and the outbound gateway stream), so a stalled or malicious client could hold descriptors open indefinitely and exhaust the process limit -- the one relay path whose descriptor use was unbounded. Add a shared TunnelLimits holding a semaphore and a timeout. Acquire an owned permit before any work and reject with 503 when the budget is exhausted; hold the permit for the tunnel's lifetime. Wrap the whole tunnel future -- upgrade, gateway connect, and copy -- in one timeout that tears it down once exceeded, releasing its descriptors. Total tunnel descriptor use is now bounded by the concurrency cap.
The bootstrap tunnel bound added in the previous commit is invisible: an operator cannot see how close tunnel concurrency runs to the cap or whether requests are being shed, so the fix cannot be confirmed in production. Thread the MetricsService into the relay and record two metrics from the bootstrap path: an active-tunnel gauge, held by an RAII guard so it cannot drift past a panic or timeout teardown, and a shed counter incremented when a tunnel is rejected at the concurrency cap. The existing per-request connection metrics are left unchanged.
44b06a9 to
44b06ac
Compare
|
Updated to keep "the bootstrap timeout and cap (first commit) and the connection metrics" as requested, @spacebear21.
This isn't trivial because of the Rather than the PR's original |
| let gateway_addr = gateway_origin | ||
| .to_socket_addr() | ||
| .await | ||
| .map_err(|e| Error::InternalServerError(Box::new(e)))? | ||
| .ok_or_else(|| Error::NotFound)?; |
There was a problem hiding this comment.
For reviewer: This assignment moved into serve_after_upgrade
5 participants
2026-06-23 Update: This PR has been reworked according to this comment to focus on fixing the OHTTP bootstrap mechanism specifically. Rather than have a global FD permit because the runaway FD issue was solved by multiplexing in #1655
#1608 increased the process file descriptor limit but did not address the reason the limit was being reached. this addresses that with transport-level fd hygeine including timeing out idle/header-stalled connections, software admission caps below the EMFILE limit, and optional, per-source limits.
Permits are acquired at the listener level, right after a TCP socket is accepted. If mailroom waits til the request gets to the middleware, it may have already admitted idle connections without ever producing HTTP requests.
There's a global inbound cap clamped by RLIMIT_NOFILE - fd_reserve so that fds for logging, acme, config reading that keep the process moving have headroom, and for outbound bootstrap connections.
The per-source cap is added but disabled by default. So in the future if there's one peer OHTTP relay that's using up all the fds with inbound, it can be individually limited. For now there are few relays and one may be producing the lion's share of inbound descriptor usage. A default cap of
Noneavoids a stale default becoming an overlooked bottleneck when max_inbound_connections is raised.The tunnel cap is separate because CONNECT/WebSocket bootstrap tunnels pin both inbound upgraded connection and an outbound gateway connection.
h2 was already served before this PR (but I'm not sure if it has been used in practice). header_read_timeout is reused as the h2 keep-alive ping interval & timeout. This doesn't introduce h1-style header idle reaping for h2.
connectionmetrics now actually count actual TCP connections instead of request middleware events. These measure transport health infrastructure overload risk rather than actual payjoin usage. Payjoin activity would be more accurately measured from domain events or HTTP request events, tbd in follow-up.Disclosure: authored & reviewed with adversarial runs of Claude/Codex models.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.