perf: event-driven CDP readiness detection (−55s startup)#1302
Open
lennney wants to merge 1 commit into
Open
Conversation
Event-driven stderr pipe (Playwright pattern) replaces 120×1s polling. Uses three-phase fallback: Phase 1: oneshot signal from stderr 'DevTools listening on ws://' Phase 2: exponential backoff TCP probe (100ms→10s) Phase 3: 1s-interval polling as ultimate safety net (30 attempts) Also replaces self.inject() → try_inject() in all three phases to eliminate the hidden 20×500ms retry_injection() loop nested inside every backoff step. Before: Phase 2 worst-case ~101s, Phase 3 ~330s, total ~446s After: Phase 2 worst-case ~21s, Phase 3 ~60s, total ~66s
lennney
force-pushed
the
perf/cdp-event-driven
branch
from
3543015 to
1558fff
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
该 PR 旨在将 Codex 启动后 CDP（Chrome DevTools Protocol）就绪检测从“固定 1s×120 次轮询”改为“事件驱动 + 退化回退”的策略，以显著降低健康环境下的启动等待时间，并避免嵌套重试带来的额外延迟。
Changes:
- 在
DefaultLaunchHooks::launch_codex()中改为 pipestderr，并尝试通过解析DevTools listening on ws://...行来触发 CDP 就绪信号。 - 在
DefaultLaunchHooks::ensure_injection()中实现三阶段注入尝试：stderr 信号 → 指数退避 TCP 探测 → 30×1s 兜底轮询。 - 增加
wait_for_cdp_ready()的单元测试，并为 tokio 增补io-utilfeature。
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/codex-plus-core/src/launcher.rs | 引入 stderr 监听 + 三阶段 ensure_injection 流程以加速 CDP 就绪判定与注入重试。 |
| crates/codex-plus-core/tests/launcher.rs | 为 wait_for_cdp_ready 增加异步单测覆盖。 |
| crates/codex-plus-core/Cargo.toml | 为 tokio 启用 io-util 特性以支持 BufReader/read_line/duplex 等 I/O 工具。 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+740
to
+757
| // Read stderr to detect when the CDP port is ready. | ||
| // Chrome/Electron prints "DevTools listening on ws://..." to stderr. | ||
| // This is the same pattern used by Playwright (92k★) for Electron apps. | ||
| if let Some(stderr) = child.stderr.take() { | ||
| let (tx, rx) = tokio::sync::oneshot::channel(); | ||
| *self.cdp_ready.lock().await = Some(rx); | ||
| tokio::spawn(async move { | ||
| if let Err(error) = wait_for_cdp_ready(stderr).await { | ||
| let _ = crate::diagnostic_log::append_diagnostic_log( | ||
| "launcher.cdp_stderr_listener_error", | ||
| serde_json::json!({"message": error.to_string()}), | ||
| ); | ||
| } | ||
| // Drop sender: if ensure_injection is still waiting, it gets | ||
| // Canceled and falls through to the TCP-backoff fallback. | ||
| drop(tx); | ||
| }); | ||
| } |
Comment on lines
+920
to
+926
| let backoff_delays = [100, 200, 400, 800, 1600, 3200, 5000, 10000u64]; | ||
| for delay_ms in &backoff_delays { | ||
| if try_inject(debug_port, helper_port).await.is_ok() { | ||
| return true; | ||
| } | ||
| tokio::time::sleep(std::time::Duration::from_millis(*delay_ms)).await; | ||
| } |
Comment on lines
+1495
to
+1503
| #[tokio::test] | ||
| async fn test_wait_for_cdp_ready_detects_magic_line() { | ||
| let (mut writer, reader) = tokio::io::duplex(1024); | ||
| writer.write_all(b"some garbage\n").await.unwrap(); | ||
| writer.write_all(b"DevTools listening on ws://127.0.0.1:9222\n").await.unwrap(); | ||
| drop(writer); | ||
| let result = wait_for_cdp_ready(reader).await; | ||
| assert!(result.is_ok()); | ||
| } |
Comment on lines
+1513
to
+1523
| #[tokio::test] | ||
| async fn test_wait_for_cdp_ready_ignores_noise_before_magic() { | ||
| let (mut writer, reader) = tokio::io::duplex(1024); | ||
| writer.write_all(b"[INFO] Starting process...\n").await.unwrap(); | ||
| writer.write_all(b"[INFO] Loading configuration...\n").await.unwrap(); | ||
| writer.write_all(b"[WARN] Something deprecated\n").await.unwrap(); | ||
| writer.write_all(b"DevTools listening on ws://127.0.0.1:9222\n").await.unwrap(); | ||
| drop(writer); | ||
| let result = wait_for_cdp_ready(reader).await; | ||
| assert!(result.is_ok()); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the existing 120×1s polling loop in
ensure_injectionwith an event-driven approach:Phase 1: Monitor stderr for Chrome/Electron's
DevTools listening on ws://signal (same pattern used by Playwright, 92k★). Signal arrives in 2-5s on a healthy system.Phase 2: Exponential backoff TCP probe (100ms→10s) as fallback for packaged apps where stderr isn't available.
Phase 3: 1s-interval polling (30 attempts) as ultimate safety net — same as original behaviour.
Also eliminates nested retry:
self.inject()internally calledretry_injection()(20×500ms loop), creating a hidden 10s barrier inside every backoff step. Replaced withtry_inject()which does a single TCP probe.Performance Impact
Design
Pattern validated by Playwright's Electron launcher (
electron.ts): pipe child process stderr, matchDevTools listening on ws://line. All Chromium-based apps (Chrome, Edge, Electron) print this line when launched with--remote-debugging-port.wait_for_cdp_ready()usesimpl AsyncRead + Unpin(not concrete ChildStderr) so unit tests can inject mock pipes.Verification
wait_for_cdp_ready(signal detection, timeout, EOF)apply_relay_config_file_accepts_utf8_bom_configfailure — also present on upstream/main)