Skip to content

Report downstream connection lifetime through a listener Tracer - #995

Open
ShanireZ wants to merge 1 commit into
cloudflare:mainfrom
ShanireZ:listener-tracer
Open

Report downstream connection lifetime through a listener Tracer#995
ShanireZ wants to merge 1 commit into
cloudflare:mainfrom
ShanireZ:listener-tracer

Conversation

@ShanireZ

@ShanireZ ShanireZ commented Sep 4, 2026

Copy link
Copy Markdown

Refs #994

Stream already carries an optional Tracer and reports on_disconnected() from its Drop,
but only the upstream connector ever sets it (connectors::l4). Downstream connections have no
equivalent. An application implementing ServerApp can set session.tracer once process_new
hands it the stream, but by then the handshake has already succeeded, so connections that time
out or fail during io.handshake() are never seen — and for an entry point those are the
interesting ones.

This adds an optional Tracer to Listeners, threads it through to each TransportStack, and
attaches a clone of it to every accepted stream, the same way connectors::l4 does for outbound
connections:

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

Stream's Drop already reports the disconnect, so the two calls pair by construction: no
ordering question, no double counting, and no way to add one without the other.

Why this covers the handshake window

The tracer rides on the L4 stream, and UninitializedStream::handshake() keeps that stream
alive in both branches — Ok(Box::new(self.l4)) when there is no TLS, and
tls.tls_handshake(self.l4) when there is. A connection that never completes the handshake
therefore still reports its disconnect when the stream is dropped.

Scope

  • One file. One new public method (Listeners::set_tracer), shaped after the existing
    set_pre_tls_callback. No signature changes and no new trait.
  • No behaviour change when no tracer is set, which remains the default.
  • Nothing here knows about metrics: the tracer is the caller's, exactly as it already is for
    upstream connections.
  • Attributing connections to a particular listener is deliberately out of scope; feat(listeners): add add_endpoint_with_filter for granular per-address L4 filtering #941 would
    provide that by construction, since a filter instance attached to one address already knows
    its address.

Tests

Two are added, both in listeners::test:

  • a connection dropped without ever handshaking reports on_connected and on_disconnected
    exactly once each — the window an application-side counter cannot see;
  • the no-tracer default path is unchanged, which fails if accept() ever starts assuming a
    tracer is present.

Verification

Against main @ 09696b5, in a container:

Gate Result
cargo fmt --all -- --check passes
cargo clippy --all-targets --all -- --allow=unknown-lints --deny=warnings passes
cargo test -p pingora-core --lib 557 passed / 2 failed, against 555 / 2 on the unpatched baseline
cargo test --workspace --lib --bins --tests identical set of failing test names before and after; only the passed count moves, by the two tests added here
git am replay applies cleanly on 09696b5, and the replayed tree is byte-identical to the development tree

The two pingora-core failures (connectors::l4::tests::test_conn_timeout and
test_bind_to_port_range_on_connect) and the pingora-proxy integration failures are present on
the unpatched baseline as well; the latter need openresty, which the container does not have.
MSRV (cargo +1.85.0 check), cargo audit and cargo machete were not available locally and
are left to CI.

`Stream` already carries an optional `Tracer` and reports `on_disconnected()`
from its `Drop`, but only the upstream connector ever sets it. Downstream
connections have no equivalent today: an application implementing `ServerApp`
can set `session.tracer` itself once `process_new` hands it the stream, but by
then the handshake has already succeeded, so connections that time out or fail
during `io.handshake()` are never seen. Counting live downstream connections is
therefore not possible for the accept-to-handshake window.

Add an optional `Tracer` to `Listeners`, thread it through to each
`TransportStack`, and attach a clone of it to every accepted stream the same way
`connectors::l4` does for outbound connections: call `on_connected()` right after
`accept()` returns, before the TLS handshake, and let `Stream`'s `Drop` report
the disconnect. The tracer rides on the L4 stream, which `handshake()` keeps
alive in both the plain and the TLS case, so the two calls pair by construction
and the whole life of the connection is covered.

No new trait, no signature changes, and no behaviour change when no tracer is
set, which remains the default.

Signed-off-by: Shanire <shanire86@gmail.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-04T08:05:49.534759Z 0dda18b PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

ShanireZ added a commit to ShanireZ/Fulcrum that referenced this pull request Sep 4, 2026
经 owner 单独授权发出 —— **两次以 owner 名义的外部写入**:
先把补丁推成 `ShanireZ/pingora` 的 `listener-tracer` 分支,再 `gh pr create`。

**PR [cloudflare/pingora#995](cloudflare/pingora#995
· base `main` · head `ShanireZ:listener-tracer`
· **一个文件** `pingora-core/src/listeners/mod.rs` **`+135/-1`**
· **一个提交** `0dda18b`(带 `Signed-off-by`)
· 正文开头 `Refs #994`

## 落地核对(⛔ 不手打、发后逐行核)

标题与正文用 `extract_pr_body.py` 从 `pr-6-listener-tracer.md` 切出、`--body-file` 发出。
发后回取:标题一致 · 正文 **66 行逐行相同**。
⚠ ⚠ **第三次了**:那段一次性比较脚本又报「差 3 个字符」,**又是假报** ——
拆穿它的还是**逐行 diff**(0 段差异)。★ 结论不变:
**量具坏掉时它不沉默,它给一个像样的错答案;换一种独立的量法,别再读它的输出。**

## 发之前当天重做的核对(⛔ 不引用上一轮)

| 查什么 | 结果 |
|---|---|
| #994 有没有回话 | **OPEN,无维护者回复、无 label** |
| 上游 `main` 动没动 | **没动**,仍 `09696b5` ⇒ 补丁基线仍准 |
| 有没有人抢先 | `set_tracer` / `listener tracer` / `downstream tracer` / `connection tracer` **两条通道全 0** |
| `cargo fmt --check` | ✅ `RC=0` |
| `cargo clippy … --deny=warnings` | ✅ `RC=0` |
| `cargo test -p pingora-core --lib` | **557 / 2 failed**,两条新测试 `ok` |
| 能不能开 PR | ✅ fork 在 |

## ⏳ 下一步

等上游回话。⚠ CONTRIBUTING 不承诺及时评审,#941 挂了六周 0 评论
⇒ ⛔ **别把它当成会很快有下文的事**。
★ 判断是否落地看**改动有没有出现在 `main`**,⛔ 不看 PR 状态 —— 本目录纪律,
而投稿一正是「PR 被 close 而改动进了 `main`」。

门禁:`plan-refs.py` 绿 · `docs-check.py` 绿。只改 `upstream-pr/` 下三份 Markdown。

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ShanireZ added a commit to ShanireZ/Fulcrum that referenced this pull request Sep 4, 2026
PR [cloudflare/pingora#995](cloudflare/pingora#995) 的 CI 跑完:

| job | 结果 |
|---|---|
| `pingora (1.85.0)` | **pass** 3m39s |
| `pingora (1.97.1)` | **pass** 10m58s |
| `pingora (nightly)` | **pass** 8m7s |
| `semgrep-oss` | **pass** 33s |

⚠ 读的是**报告正文**不是退出码(`gh pr checks` 与后台 `--watch` 两次一致)——
本仓那条纪律:**退出码只判命令跑没跑起来,绿不绿只看报告正文**。

## ★★★ 它正好补上了本地验证的两处缺口

材料 §2.2 的验证表里有两句「留给上游 CI」,现在都兑现了:

1. **MSRV** —— `pingora (1.85.0)` 那道就是 `cargo +1.85.0 check`,
   本地镜像里没有那个工具链(rustc 1.98),跑不了;
2. **带 openresty 的完整套件** —— `pingora (1.97.1)` 跑的正是它。
   ⇒ 本地那 **99 条 `pingora-proxy` 集成失败是环境造成的**,这一下坐实了:
   ⛔ 那句话此前只是「基线也失败」的对照推断,现在有了正面证据。

⇒ ⛔ 「留给上游 CI」不再是欠账。

门禁:`plan-refs.py` 绿 · `docs-check.py` 绿。只改 `upstream-pr/` 下两份 Markdown。

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant