[codex] Observe remote exec-server lifecycle - #27470
Conversation
Codex Cloud Agents (CCA) couldn't complete this review. The original Codex Review is unaffected. |
cb15fe9 to
08f9dd1
Compare
7a28c0a to
5a94cf1
Compare
69e10d5 to
97d99d1
Compare
5a94cf1 to
1128e27
Compare
97d99d1 to
f8dd96f
Compare
1128e27 to
2d7885f
Compare
f8dd96f to
c475918
Compare
## Why Exec-server JSON-RPC calls can cross local and remote transports, but trace context stopped at the RPC boundary. That made client and server work difficult to correlate when diagnosing latency or failures. ## What changed - Propagate the current W3C trace context on outbound JSON-RPC requests. - Parent inbound request spans from received trace context. - Record the received JSON-RPC method on server spans and keep each span open through response enqueue. - Add only the OTEL dependencies required by the exec-server crate. ## Stack Review and land this stack in order: 1. #27466 — trace exec-server JSON-RPC requests **(this PR)** 2. #27467 — record bounded connection, request, and process lifecycle metrics 3. #27470 — observe remote registration and Noise rendezvous lifecycle ## Validation - `just test -p codex-exec-server --lib` (153 passed) - `just bazel-lock-check` - `just fix -p codex-exec-server`
## Summary - Record bounded connection, request, and process lifecycle metrics. - Report active gauges from callbacks on every collection, including delta exports. - Serialize active-count updates so concurrent starts and finishes cannot publish stale values. - Serialize process exit, explicit termination, and shutdown through the process registry so exactly one completion result wins. - Keep the implementation small with single-owner RAII guards and one real OTLP/HTTP integration test using the existing `wiremock` dependency. ## Root cause Process exit and session shutdown previously used cloned completion state. That avoided duplicate emission, but it duplicated lifecycle ownership and made the ordering harder to reason about. The process registry mutex already defines the lifecycle ordering, so the final implementation stores the metric guard and termination flag directly on the process entry. Whichever path claims the entry first owns the completion result. Production metric export uses delta temporality. Event-only synchronous gauge recordings disappear after the next collection when no count changes, so active counts now use observable callbacks that report current state on every collection. The cleanup also removes the constant `result="accepted"` connection tag, redundant route and response assertions, a custom HTTP collector, and fallback initialization machinery that did not add behavior. ## Stack Review and land this stack in order: 1. #27466 — trace exec-server JSON-RPC requests 2. #27467 — record bounded connection, request, and process lifecycle metrics **(this PR)** 3. #27470 — observe remote registration and Noise rendezvous lifecycle ## Validation - `just test -p codex-exec-server --lib` (158 passed) - `just test -p codex-cli --test exec_server` (3 passed) - `just test -p codex-otel observable_gauge_is_collected_on_every_delta_snapshot` (1 passed) - `CARGO_BUILD_JOBS=1 just fix -p codex-otel -p codex-exec-server` - `just fmt` - `git diff --check`
44642f8 to
5916ef7
Compare
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(unix)] |
There was a problem hiding this comment.
Could we also have a test that covers the windows path? I'm not sure what tokio's ctrl_c handler waits for on Windows but should be possible to exercise it here?
There was a problem hiding this comment.
I investigated this. Tokio’s Windows path waits for CTRL_C_EVENT, but Windows cannot target that event to only the spawned process: group 0 broadcasts to every process sharing the test console, while a nonzero group does not deliver Ctrl-C. CREATE_NEW_PROCESS_GROUP also disables Ctrl-C. Tokio’s own tests therefore stub its internal handler instead of exercising the OS path. A safe integration test here would require a separate console and helper process, or expanding production behavior to accept targetable Ctrl-Break. Both seem disproportionate for this PR, so I’d prefer to rely on Tokio’s signal implementation and Windows CI compilation unless we want to add Ctrl-Break semantics.
Summary
Stack
Review and land this stack in order:
Validation
just test -p codex-exec-server --lib(149 passed)just test -p codex-cli --test exec_server(4 passed)just argument-comment-lintjust bazel-lock-checkjust fix -p codex-exec-server -p codex-clijust fmt