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

[ShanireZ]

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:

• "on connect" phase for incoming connections #118, "on connect phase for incoming connections" — closed as completed by feat: Add ConnectionFilter trait for TCP-level connection filtering #671, which
  covers the accept moment only.
• [Doc] using tracers to track connections #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".
• [Question or Feature] how to track client connect/disconnect esp. for websocket #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.

#[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 Query - how to report on number of connections in pool to upstream? #245, [Doc] using tracers to track connections #295 and [Question or Feature] how to track client connect/disconnect esp. for websocket #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 "on connect" phase for incoming connections #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 [Doc] using tracers to track connections #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 [Doc] using tracers to track connections #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: feat: Add ConnectionFilter trait for TCP-level connection filtering #671 (the ConnectionFilter trait), "on connect" phase for incoming connections #118, [Doc] using tracers to track connections #295, [Question or Feature] how to track client connect/disconnect esp. for websocket #337.
• Related, on listener identity, which we are deliberately leaving alone: feat(listeners): add add_endpoint_with_filter for granular per-address L4 filtering #941 (open) attaches
  filters per address, which is how we would expect per-listener attribution to arrive; Report actual listener addresses after binding #988,
  with PR services: report bound listener addresses #991 reporting the addresses a service actually bound and PR listeners: preserve distinct sockets across graceful upgrades #990 fixing the fd-table
  collision it needs (all open).
• Related, on end-of-something hooks: Add finish_downstream_session to ProxyHttp #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

[ShanireZ]

Correction to my own issue: I missed that most of this machinery already exists on the
downstream side, and the request is smaller than I wrote it.

Stream carries pub tracer: Option<Tracer>, and Stream::drop calls on_disconnected() on
it (pingora-core/src/protocols/l4/stream.rs). The upstream connector sets it —
t.0.on_connected(); stream.tracer = Some(t); in pingora-core/src/connectors/l4.rs — and
Tracer is Clone. On the accept path it is left None, and listeners/ and services/ do
not reference tracer at all.

So the sentence "there is no equivalent for connections from downstream" in the issue body is
wrong: the mechanism lives on the shared Stream type, it is just only wired on the outbound
side. And since ServerApp::process_new receives mut session: Stream, an application
implementing that trait can already set session.tracer and get a per-connection disconnect
callback today. My opening claim was too strong.

What is actually missing is narrower than the issue says:

1. The accept-to-handshake window. An application only sees a connection once
   io.handshake() has succeeded — handle_event is called on the Ok branch in
   run_endpoint — so connections that time out or fail during the handshake never reach it,
   and cannot be counted by setting session.tracer. For an entry point those are the
   interesting ones.
2. A way to attach a tracer at the listener, so the whole life of a connection is covered
   rather than the post-handshake part, and so it works for any app rather than only ones that
   remember to set the field.

That makes this need no new trait method at all. An optional Tracer on Listeners (or on
ListenerEndpointBuilder) would be enough, with the accept path doing what the connector
already does:

let t = tracer.clone();
t.0.on_connected();
stream.tracer = Some(t);

Stream::drop already handles the other half, so the two pair by construction — no ordering
question and no double counting. Please disregard the connection_closed proposal in the
issue body; reusing the existing Tracing trait is better than adding a method to
ConnectionFilter, and it is also the mechanism already suggested on #245, #295 and #337.

Per-listener attribution I would still leave to #941, as the issue says.

Apologies for the noise — I should have read Stream before writing that paragraph. Happy to
send a PR for the listener-side tracer if that shape looks right to you.