cloudflare / cloudflare/pingora

ConnectionFilter has no end-of-connection hook, so downstream connections can be counted starting but never ending

Open
#994 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
27.4k
Forks
1.7k
Avg merge
6h 22m
Merged PRs (30d)
3

Description

## What is the problem your feature solves, or the need it fulfills?

A process that uses Pingora as its entry point cannot report how many downstream connections
it currently has. A connection *starting* can be observed today; a connection *ending* cannot
be observed at all.

`ConnectionFilter::should_accept` (added by #671, behind the `connection_filter` feature) is
called after `accept()` and before the TLS handshake, which is the right moment to see a
connection start. **Nothing is called when one ends.** `run_endpoint` spawns one task per
connection, and that task has three exits — handshake timeout, handshake error, and
`handle_event` returning — and none of them notifies anything. So a number built on
`should_accept` can only ever go up.

This has been asked for more than once:

- #118, "on connect phase for incoming connections" — closed as completed by #671, which
covers the accept moment only.
- #295 — asked specifically for the number of *active* connections (increment on connect,
decrement on disconnect). The suggested answer was a userland `Drop` guard; the issue
was closed by the stale bot, the reporter's last comment being "still relevant".
- #337 — asked how to track *client* connect/disconnect (for WebSocket); the answer was to
use `Tracer`, and the issue was closed as completed.

Pingora already has exactly this shape, on the other side: `upstreams::peer::Tracing`
provides `on_connected` / `on_disconnected`, and `Tracer` is the answer people are pointed
at on #245, #295 and #337. But a `Tracer` is a field on `PeerOptions` and counts connections
*to upstreams*; there is no equivalent for connections *from* downstream. The alternatives
section below says why that is a different question rather than a smaller one.

Worth noting from the other side: `pingora-prometheus` and `pingora-foundations` already give
a place to publish such a number and an endpoint to serve it from. What is missing is anything
in core that reports it.

One caveat we are *not* asking you to change, mentioned so a count is not oversold: in
`ListenerEndpoint::accept()`, if the stream has no socket digest or the digest has no peer
address, `should_accept` is not called at all and the connection is accepted by default. For a
filter that is a safe default; it does mean a count of starts is not exhaustive.

## Describe the solution you'd like

Extend the existing `ConnectionFilter` seam rather than introduce a second one. It already
carries the plumbing (a per-endpoint field, a builder method, `Listeners::set_connection_filter`)
and is already gated behind `connection_filter`, which is off by default.

**One addition**: a defaulted `fn connection_closed(&self)`, called once for each connection
`should_accept` was consulted about, when that connection is gone. No signature changes, no
new data crossing the boundary, and nothing for existing implementors to do.

The pairing is the part worth pinning down: as noted above, `should_accept` is skipped
entirely when there is no peer address. A close hook that fired for *every* accepted
connection would therefore not pair with it, and the obvious `+1` / `-1` implementation would
underflow. Firing it exactly where `should_accept` ran keeps the two in step.

```rust
#[async_trait]
impl ConnectionFilter for MyConnectionCount {
async fn should_accept(&self, _addr: Option<&SocketAddr>) -> bool {
self.live.fetch_add(1, Relaxed);
true
}

// new
fn connection_closed(&self) { self.live.fetch_sub(1, Relaxed); }
}
```

**On attributing connections to a particular listener**: we are deliberately *not* asking for
that here. Today `Listeners::set_connection_filter` installs one instance across every
endpoint (and `add_endpoint` clones that same one), so the hook above yields a process-wide
live count — which is more than is obtainable now. Per-listener attribution follows from #941
by construction, since a filter instance attached to one address already knows its address.
We would rather wait for that than ask for a second mechanism.

Two things we learned the hard way carrying this in a fork, offered for what they are worth
rather than as a prescription:

- **The end hook wants to be synchronous.** The only way we found to guarantee it fires is a
value owned by the spawned task that calls the hook from `Drop` — and `Drop` cannot await.
An `async` close hook therefore forces either a `spawn` per closing connection or a
blocking bridge, both worse than a plain `fn`. `should_accept` can stay `async`.
- **Drive it from that guard, not from explicit calls at each exit.** The task has three
exits; an explicit call has to be written three times, and the failure mode of missing one
is silent — a live-connection number that never comes back down, sitting next to a total
that looks perfectly healthy.

**Backwards compatibility**: a defaulted method is source-compatible for existing
implementors, and the feature is opt-in, so nothing changes for anyone who does not implement
it.

One thing worth knowing before shaping this: the trait exists twice. The real one is in
`listeners/connection_filter.rs` under the feature; `listeners/mod.rs` defines a stub for
when the feature is off, described as being there "for API compatibility". Their signatures
already differ — the stub's `should_accept` is synchronous and takes `&SocketAddr`, the real
one is `async` and takes `Option<&SocketAddr>` — so any addition here needs a decision about
the stub too. We mention it only because it shapes the change, not as a separate report.

**On what this would add to `pingora-core`**: we are aware that core deliberately no longer
carries metrics. #560 and #822 asked for `prometheus` to be made optional, and `842ddd9`
answered by moving the Prometheus HTTP app out into its own `pingora-prometheus` crate,
which now builds entirely on core's public API. What we are asking for sits on the other
side of that same line: one notification, with no metrics dependency, no counter/gauge
distinction and no label vocabulary. The counting stays in the caller, exactly as
`pingora-prometheus` now sits outside core.

**A scope question we would rather ask than assume**: `connection_filter` is named for
*filtering*, and observing when a connection ends is a different concern. If you would prefer
it to live under a differently named feature, or on a sibling trait, we are happy to shape a
PR that way. What we would like to avoid is a second, parallel wiring path alongside the one
`ConnectionFilter` already has.

## Describe alternatives you've considered

- **`upstreams::peer::Tracer`** — the answer given on #245, #295 and #337, so it is worth
addressing up front rather than after a round trip. A `Tracer` is placed in
`PeerOptions` and its `on_connected` / `on_disconnected` fire for connections Pingora opens
*to an upstream*, counting active plus pooled ones. That is a different population from the
connections a listener is currently holding: a downstream connection that never opens an
upstream connection produces no tracer events at all — one rejected by a filter, one that
fails or times out in the TLS handshake, a request served from cache or by a local handler,
or any L4 application that never dials out. Nothing in `listeners/` or
`services/listening.rs` references it.
- **`ServerApp::cleanup` / `HttpServerApp::http_cleanup`** — suggested on #118, so worth
ruling out explicitly. These are per-service, not per-connection: the doc comment on
`cleanup` says it is "called once after the service stops listening to its endpoints", and
the call site in `run_endpoint` is after the accept loop has exited. They cannot see
individual connections at all.
- **Counting in userland with a `Drop` guard** (also suggested on #295). It cannot cover the
window this is about: connections that time out or fail during `io.handshake()` never reach
the application, because `handle_event` is only called on the successful branch. For an
entry point those are a population you specifically want to see.
- **Polling the operating system** (`netstat`-style, also considered on #295): it is a
sampling answer to a question about state transitions, and attributing sockets back to a
particular listener of a particular process is awkward at best.
- **A separate observer trait alongside `ConnectionFilter`**: this duplicates wiring that
already exists (per-endpoint field, builder method, `Listeners::set_*`), which is why we
are proposing to extend the existing seam. See the scope question above if you disagree.
- **Carrying it in a fork** — what we do today. It works, but it is a permanent rebase cost
for something that looks generally useful rather than specific to us, which is why we are
asking here instead of keeping it.

## Additional context

- Related, on the seam itself: #671 (the `ConnectionFilter` trait), #118, #295, #337.
- Related, on listener identity, which we are deliberately leaving alone: #941 (open) attaches
filters per address, which is how we would expect per-listener attribution to arrive; #988,
with PR #991 reporting the addresses a service actually bound and PR #990 fixing the fd-table
collision it needs (all open).
- Related, on end-of-something hooks: #751 (open since 2025-11) adds
`finish_downstream_session` to `ProxyHttp`. That is a per-session hook in `pingora-proxy`;
what is asked for above is a per-connection notification in `pingora-core`, so it also
covers L4 applications and connections that never complete a handshake.
- We maintain a fork carrying this capability and would be glad to send the PR, in whatever
shape you prefer.

**Pingora version**: `main` @ `09696b5`

Contributor guide

Open the contributing guide

Research direction

Read listeners/connection_filter.rs and the API-compatibility stub in listeners/mod.rs, then trace run_endpoint and its handshake and handle_event exits. Check how the existing filter is wired through the endpoint builder and Listeners::set_connection_filter. Done means the requested end-of-connection behavior is paired with should_accept without breaking feature-off implementors.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design, networking
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.