Commit 2279cd0
authored
CDP telemetry fixes (#251)
## Overview
### Bug
When chromium restarted mid-session (`PATCH /chromium/flags`,
`/configure`, etc., or a crash + supervisord autorestart), the CDP proxy
emitted `cdp_disconnect.reason = "client_close"` instead of
`"upstream_changed"`. Live repro confirmed: ~1.6s after the WS opened,
reason was `client_close` even though chromium clearly went away.
### Root cause
A goroutine watched `mgr.Subscribe()` for the new chromium URL and
stamped `reasonOverride = upstream_changed` before triggering pump
teardown. But the upstream's TCP EOF (chromium dying) reaches the pump
*before* the new "DevTools listening on" log line is emitted (~5–8 s
gap). So `cleanup` always ran first, saw `reasonOverride == nil`, and
fell through to `client_close`.
### Fix
- `wsproxy.Pump`'s `onClose` callback now receives a `PumpExitCause`
(`client` / `upstream` / `context`).
- Deleted the URL-watcher goroutine + `reasonOverride` in
`devtoolsproxy`. Reason resolution moved into cleanup as a small helper,
`resolveDisconnectReason`, which:
- returns `client_close` / `context_cancelled` immediately from the
cause,
- on upstream cause, checks `mgr.Current()` and otherwise waits up to
`restartConfirmWait` (10 s) on `urlCh` for a new URL — new URL ⇒
`upstream_changed`, timeout ⇒ `upstream_error`.
### Tests
- `TestResolveDisconnectReason` — 6 table cases covering all four
reasons (sub-100 ms).
- `TestWebSocketProxyHandler_EmitsUpstreamChangedOnMidStreamRestart` —
integration through the handler with `restartConfirmWait` shortened.
- Existing dial-time `upstream_error` + client_close tests continue to
pass.
### Verification
Repro'd live against the headful image. Before fix:
`reason=client_close, duration_ms=1620`. After fix:
`reason=upstream_changed, duration_ms=10388`.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Changes CDP WebSocket teardown and telemetry classification on
Chromium restarts; well-tested but affects automation clients that rely
on disconnect reasons and session timing.
>
> **Overview**
> Fixes **CDP disconnect telemetry** when Chromium restarts mid-session:
`cdp_disconnect` was often emitted as `client_close` because upstream
EOF arrived before the new DevTools URL was published.
>
> **`wsproxy.Pump`** now passes a **`PumpExitCause`** (`client` /
`upstream` / `context`) into cleanup. **`devtoolsproxy`** drops the
URL-watcher `reasonOverride` path; on upstream exit it
**`resolveDisconnectReason`** polls `UpstreamManager.Current()` for up
to **`restartConfirmWait`** (10s) to choose **`upstream_changed`** vs
**`upstream_error`**. When the upstream URL changes while a session is
still open, the watcher **closes the upstream WebSocket** so the pump
exits with the right cause. **`cdp_disconnect`** uses a pinned
**`disconnectedAt`** for **`duration_ms`** and event timestamp so
metrics are not inflated by the restart poll.
>
> New unit/integration tests cover reason resolution, mid-stream
restart, and proactive kick on URL change. The rest of the diff is
mostly **import ordering**, **gofmt**, and comment/whitespace cleanup
across API, events, policy, and e2e helpers.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
f43d049. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
---------
Co-authored-by: Sayan- <1415138+Sayan-@users.noreply.github.com>1 parent 0dc1a65 commit 2279cd0
26 files changed
Lines changed: 433 additions & 140 deletions
File tree
- server
- cmd
- api/api
- chromium-launcher
- shell
- wrapper
- e2e
- lib
- cdpclient
- cdpmonitor
- devtoolsproxy
- events
- policy
- wsproxy
- scripts/concurrent_stop_test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
16 | 15 | | |
17 | 16 | | |
18 | 17 | | |
19 | 18 | | |
20 | 19 | | |
21 | 20 | | |
22 | 21 | | |
| 22 | + | |
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
| |||
99 | 99 | | |
100 | 100 | | |
101 | 101 | | |
102 | | - | |
| 102 | + | |
103 | 103 | | |
104 | 104 | | |
105 | 105 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
16 | 15 | | |
17 | 16 | | |
18 | 17 | | |
19 | 18 | | |
20 | 19 | | |
| 20 | + | |
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
262 | 262 | | |
263 | 263 | | |
264 | 264 | | |
265 | | - | |
| 265 | + | |
266 | 266 | | |
267 | 267 | | |
268 | 268 | | |
| |||
302 | 302 | | |
303 | 303 | | |
304 | 304 | | |
305 | | - | |
306 | | - | |
307 | | - | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
308 | 308 | | |
309 | 309 | | |
310 | 310 | | |
311 | | - | |
312 | | - | |
313 | | - | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
314 | 314 | | |
315 | 315 | | |
316 | 316 | | |
317 | | - | |
318 | | - | |
319 | | - | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
320 | 320 | | |
321 | 321 | | |
322 | 322 | | |
323 | | - | |
324 | | - | |
325 | | - | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
326 | 326 | | |
327 | 327 | | |
328 | 328 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
15 | | - | |
16 | 15 | | |
17 | 16 | | |
18 | 17 | | |
19 | 18 | | |
| 19 | + | |
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| |||
655 | 655 | | |
656 | 656 | | |
657 | 657 | | |
658 | | - | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
19 | | - | |
20 | 19 | | |
21 | 20 | | |
22 | 21 | | |
23 | 22 | | |
| 23 | + | |
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
26 | | - | |
27 | 26 | | |
28 | 27 | | |
29 | 28 | | |
| 29 | + | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
17 | 16 | | |
18 | 17 | | |
| 18 | + | |
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
135 | 135 | | |
136 | 136 | | |
137 | 137 | | |
138 | | - | |
139 | 138 | | |
140 | 139 | | |
141 | 140 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | | - | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
20 | | - | |
21 | 20 | | |
| 21 | + | |
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| |||
0 commit comments