perf: event-driven CDP readiness detection (−55s startup)#1302
Closed
lennney wants to merge 1 commit into
Closed
Conversation
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()); | ||
| } |
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
1558fff to
9b1eaa8
Compare
Contributor
Author
|
Closing: Phase 1 fast path is dead code (tx never sent in the spawn block). Redesign needed. |
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 120×1s polling loop in
ensure_injectionwith event-driven CDP readiness detection.Why
The current approach polls TCP port availability in 1s intervals × 120 attempts, which adds up to 55s worst-case startup delay even when the process is ready within 2-5s.
Design
Three-phase progressive detection (each phase is a safety net for the previous):
DevTools listening on ws://Also eliminates nested retry:
self.inject()internally calledretry_injection()(20×500ms), creating a hidden 10s delay inside every backoff step. Replaced withtry_inject()which probes once.Pattern Validation
Phase 1 follows Playwright's Electron launcher (source): pipe child stderr, match DevTools signal line. All Chromium-based apps print this when launched with
--remote-debugging-port.API Design
wait_for_cdp_ready()usesimpl AsyncRead + Unpininstead of concreteChildStderr— enables unit test injection with mock pipes.Performance
Known CI Issue
Windows CI fails due to a pre-existing upstream bug in
watcher.rs(ChatGPT-Desktop missing fromAPP_PACKAGE_SPECS). Fix tracked in #1450. macOS and all non-Windows tests pass.