perf(openai): optimized turn session — background prewarm and prefix telemetry (PR8)#723
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an opt-in OpenAI turn-session provider for headless ChangesOpenAI turn-session optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant runExec
participant OptimizedTurnSessions
participant Agent
participant OpenAIProvider
CLI->>runExec: execute headless command
runExec->>OptimizedTurnSessions: resolve eligible session
OptimizedTurnSessions->>OpenAIProvider: validate provider and capabilities
runExec->>Agent: pass session provider and model switcher
Agent->>OpenAIProvider: prewarm and stream completion
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/exec_turn_session_test.go`:
- Around line 72-79: Update the polling deadline in the prewarm probe test to
exceed the production prewarmTimeout bound of 3 seconds, while preserving the
existing polling loop and exact-one-probe assertion.
In `@internal/providers/openai/session.go`:
- Around line 154-163: Update canonicalToolsDigest to include each tool’s
description alongside its name and parameters when constructing the fingerprint,
so description-only changes affect prefix_stable. Add a regression test covering
two otherwise identical tools with different descriptions and verify their
digests differ.
In `@internal/providers/turn_session.go`:
- Around line 48-52: The optimized-session fallback in
internal/providers/turn_session.go around resolveCapabilities must not construct
a provider with empty capabilities; return (nil, false) when capability
resolution fails so callers retain the default path. In internal/cli/exec.go
around the switched-model fallback session provider, resolve the switched
model’s capabilities before constructing the provider and preserve that
capability projection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc1817cb-d875-483c-9188-d7299148c037
📒 Files selected for processing (8)
README.mdinternal/cli/exec.gointernal/cli/exec_turn_session_test.gointernal/providers/openai/session.gointernal/providers/openai/session_test.gointernal/providers/turn_session.gointernal/providers/turn_session_gate_test.gointernal/trace/trace.go
|
Addressed in
The one part I'm keeping: the exec fallback wrap for a switched model stays zero-caps. That fallback fires only when the switched model is not eligible for the optimized session, and it mirrors the agent loop's own legacy-switcher wrap, which also carries zero capabilities — the two fallback paths should stay consistent, and nothing consumes capabilities on that path today. Resolving caps there would also mean a second registry resolve per swap for parity with a path that deliberately reports "unknown". Happy to revisit when something actually reads capabilities on the fallback path. CI running on the new head. |
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed at 08904df. Requesting changes for the following verified issues:
-
internal/providers/openai/session.go: the prewarm uses providerio.SendWithRetry, which also records provider_connect. Optimized traces therefore combine the HEAD prewarm with the completion POST, so the documented A/B provider_connect comparison cannot isolate the claimed improvement. Keep the probe under its dedicated provider_prewarm span without contaminating provider_connect.
-
internal/providers/turn_session.go and internal/cli/exec.go: capability-resolution failures and switched-model fallback sessions currently use empty capabilities. Preserve the resolved projection, or safely retain the default path when resolution fails.
-
internal/providers/openai/session.go: canonicalToolsDigest omits tool descriptions even though descriptions are serialized on the OpenAI request. Include descriptions and add a description-only drift regression test.
-
internal/providers/openai/session.go: macOS still sends the extra HEAD probe although the shared transport disables keep-alive reuse there. That creates overhead with no reusable connection and conflicts with the stated no-regression expectation. Skip the ineffective probe on that transport/platform.
-
internal/cli/exec_turn_session_test.go: the two-second polling deadline is shorter than the production three-second prewarm bound. Use a deadline longer than the bound; the package-private prewarmTimeout constant cannot be referenced directly from the CLI package.
Focused race tests, vet, build, formatting, and diff checks passed. All GitHub checks are green. The reported lint findings are pre-existing and outside this PRs changed files.
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed through head 35dfe9f. The focused session/gate race tests, vet, and diff hygiene pass, and the description/capability/test-bound fixes are sound. Requesting changes for four remaining PR8 blockers:\n\n1. calls using the traced run context. That helper records , so an optimized run emits one connect span for the HEAD probe plus another for the completion POST. samples every span by name, which contaminates the exact A/B metric this PR says to compare. Keep the probe solely under (for example, issue the single HEAD without the provider-connect instrumentation) and add a trace regression asserting prewarm does not increment/add .\n\n2. The compatibility fingerprint excludes and sorts the tool slice. Both can change the actual OpenAI wire request; tool order is preserved by request serialization, and the split explicitly lists prompt-cache key in . Because is per-session, the cross-session rationale for excluding the cache key does not apply. Include the cache key, preserve wire tool order, and replace the current ignore/order-insensitive tests with drift regressions.\n\n3. On macOS the default transport disables keep-alives, yet the optimized path still launches the extra HEAD that cannot be reused. Skip the prewarm when the effective transport cannot retain idle connections (and test it), rather than adding known overhead under a performance flag.\n\n4. The program release gates require benchmark results for performance-changing PRs: baseline vs optimized P50/P95 plus success/verification rate on identical tasks. The PR currently gives only instructions and an expected effect. Please attach the actual A/B results after fixing the trace contamination; the performance-smoke job does not exercise/prove this gated path.\n\nAlso, PR8's planned includes the instruction/tools hashes established by PR2. If this partial per-provider fingerprint intentionally remains telemetry-only, narrow its naming/claims accordingly; it must not be presented as sufficient compatibility protection for future response-state reuse without incorporating the existing complete-prefix signal and remaining wire-affecting fields.
|
Clarification for my requested-changes review: the identifiers stripped by formatting are Prewarm, providerio.SendWithRetry, provider_connect, provider_prewarm, PromptCacheKey, RequestPrefix, and lastFingerprint. Specifically, Prewarm currently calls providerio.SendWithRetry with the traced context, adding an unwanted provider_connect sample; and RequestPrefix compatibility must treat PromptCacheKey and wire tool order as drift. The four requested changes and validation summary otherwise rendered correctly. |
…ingerprint wire order and cache key
|
Thanks both — the reviews converged on real problems and Trace contamination (both of you, #1): fixed. The probe no longer goes through the retry helper — it issues one macOS overhead (both): fixed by skipping, not documenting. Fingerprint semantics (anand #2): you're right on both counts, fixed. The cross-session rationale for excluding Naming/claims (anand, last point): narrowed. The fingerprint doc and the PR body now state explicitly that this is per-provider request-parameter telemetry, NOT a complete compatibility fingerprint — it deliberately does not incorporate the Capabilities (gnanam #2): the gate half landed in the previous commit — capability-resolution failure now returns the default path instead of an empty-caps optimized session (your "or safely retain the default path" branch). The exec fallback wrap keeps zero caps deliberately: it only fires for switch targets ineligible for optimization and mirrors the agent loop's own legacy wrap; happy to revisit when something consumes capabilities on that path. Benchmark results (anand #4): the one open item. Agreed the gate requires numbers, and with the trace contamination fixed the A/B is now clean to run. A real measurement needs a live official-OpenAI account run of the bench suite (the performance-smoke job doesn't exercise this gated path, as you note) — I'll run baseline vs |
|
@anandh8x — with the trace contamination fixed in Exact procedure, both runs on identical tasks: Compare: first-request Everything else from both reviews is resolved on the current head; once the numbers are attached this should be ready for a re-pass. |
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed at 95fad4f. The macOS probe-test regression is fixed correctly: probe-expecting tests now pin reusable transports and the focused race suite passes locally.
Three blockers remain:
-
internal/cli/exec.go:439 still constructs the switched-model fallback session with an empty ProviderCapabilities projection. The earlier review thread covered both the initial and switched paths, but only the initial OptimizedTurnSessions failure path was corrected. Preserve the switched models resolved capabilities or safely retain the appropriate default path.
-
The performance-program release evidence is still missing. Please attach baseline-versus-optimized P50/P95 and success/verification-rate results for identical tasks after the trace fix; instructions and the performance-smoke check do not demonstrate the gated paths effect.
-
The PR descriptions Verification section is stale: it still says the fingerprint ignores cache keys and is tool-order-insensitive, contradicting the current implementation and tests. Update it to describe cache-key drift, wire-order preservation, description hashing, and the macOS skip behavior.
Focused race tests, vet, build, formatting, diff hygiene, and changed-package lint pass locally. The newest platform CI run was still in progress at review time.
|
All three addressed:
CI running on the new head. |
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewed current head 95fad4f. The implementation fixes from my prior review are sound: prewarm no longer contaminates provider_connect, non-reusable transports skip the probe, cache-key and wire tool-order changes now drift, and the focused OpenAI TurnSession race suite passes. I am not approving yet because the PR8 release evidence is still missing: attach baseline-versus-optimized first-request provider_connect P50/P95 plus success and verification rates from identical official-OpenAI tasks. The PR description Verification section must also be updated because it still says cache keys are ignored and tools are order-insensitive, contradicting the current code and tests. Once those two items land, this is ready for the final approval pass.
|
@anandh8x — crossing wires slightly: your re-review snapshot was
That leaves exactly one open item: the benchmark run, and it needs an official-OpenAI account, which I don't have — see my earlier comment with the exact A/B procedure addressed to you. Two ways to close it:
Either way the PR ships gated-off, so the numbers gate the claim, not user-facing behavior. |
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed at 37ea2a7. No remaining code findings.
The switched-model fallback now preserves the resolved capability projection through providers.DefaultTurnSessions, with focused regression coverage. The PR description has also been refreshed to match the current cache-key, tool-order, description-drift, provider-connect isolation, and macOS skip behavior.
Focused race tests, vet, build, formatting, diff hygiene, and changed-package lint all pass locally.
One release blocker remains: attach baseline-versus-optimized official-OpenAI results for identical tasks, including first-request provider_connect P50/P95 plus success and verification rates. The documented procedure and performance-smoke check do not substitute for measured evidence of the gated path.
Once those results are attached and the final CI run is green, the code is ready for approval.
gnanam1990
left a comment
There was a problem hiding this comment.
Re-reviewed at 37ea2a7. Approved: no remaining code findings. The prewarm isolation, transport gating, fingerprint semantics, capability propagation, cross-platform tests, documentation, and focused validation are sound. Live official-OpenAI A/B benchmark evidence is intentionally deferred to the planned OAuth OpenAI work and is not a merge blocker for this feature-gated, default-off change.
|
@anandh8x — following up on the numbers: please run the A/B yourself on your GPT account. You're the reviewer gating on the evidence and you hold the access; everything else on this PR is done and both of you have confirmed no remaining code findings. One technical note so the run measures the right path: a ChatGPT-subscription OAuth login resolves to the chatgpt catalog, which this PR's allowlist deliberately excludes — with that profile the gate silently falls back to the default path and the A/B shows nothing. The run needs a plain api.openai.com profile with an API key. The nav suite is small, so the cost is trivial on any pay-as-you-go key. Procedure is in my earlier comment (baseline unset vs |
anandh8x
left a comment
There was a problem hiding this comment.
Approved at 37ea2a7. All code findings are resolved: prewarm tracing is isolated, non-reusable transports skip the probe, fingerprint telemetry matches cache-key and wire tool-order behavior, switched-model capabilities are preserved, cross-platform coverage is in place, and CI is green. Following the maintainer decision, live official-OpenAI A/B evidence is deferred to the planned OAuth/OpenAI work and is not a merge blocker for this default-off feature gate.
Summary
Implements PR8 of the Zero Agent Performance Program: one optimized provider turn-session, for the official OpenAI path only, behind the TurnSession seam from #720. Feature-gated and disabled by default (
ZERO_OPENAI_TURN_SESSION, off unless set truthy;0/falsenever enable).What the optimized session does
provider_prewarmspan and can never contaminate theprovider_connectA/B metric (regression-tested). When the transport cannot retain idle connections (the macOS shared transport disables keep-alives), the probe is skipped entirely rather than spent — no request, no trace stamps (tested). Advisory by contract — it can never fail or delay a run.prefix_stable/prefix_driftcounters. Messages are excluded (they grow every turn; this fingerprints request parameters, not conversation content). Scope, stated plainly: this is per-provider telemetry, not a complete compatibility fingerprint — it deliberately does not incorporate the prompt-prefix hashes (complete_prefix) the agent loop already emits, and any future stateful response reuse must combine both signals plus the remaining wire-affecting fields before reusing provider-side state. On chat completions there is no server-side response state to invalidate, so drift is telemetry-only today.Provider.StreamCompletionverbatim. The session never changes a request.Gate and allowlist
providers.OptimizedTurnSessionsreturns the optimized session only when: the env gate is on, the resolved provider kind is official OpenAI (explicitly not openai-compatible gateways — the constructor merges the two kinds, so the gate branches on the resolved kind), the profile is not the ChatGPT catalog, and the provider value is the concrete OpenAI provider (a fake, chatgpt-flavored, or nil provider falls back safely). Everything else — and every run with the gate off — leavesOptions.TurnSessionProvidernil, so the loop executes literally today's code path, not an equivalent one.Wiring is headless-exec only (the benchmarkable surface): exec sets
TurnSessionProviderand, when escalation is enabled and the run start is optimized, aModelSessionSwitcherthat re-evaluates eligibility per switched model and falls back to the default adapter otherwise. The TUI is deliberately untouched — a mechanical follow-up once measured.Performance expectations (no unmeasured claims)
Expected effect is confined to the first request of a run on Linux/Windows: the handshake overlaps prompt assembly, so the first
provider_connectspan should shrink by roughly one TCP+TLS round-trip set. Two honest caveats, documented in code:Of the plan's four configs: "reused connection" is already the baseline (the shared transport pools connections); "previous-response reuse" is inapplicable on chat completions (
previous_response_idis a Responses-API field, which the allowlist excludes) — both documented rather than built, so no retention-semantics change exists in this PR and the privacy opt-in rule is satisfied by omission.A/B on identical tasks: run the same
zero-perf-bench turnsuite withZERO_OPENAI_TURN_SESSIONunset vs=1(the childzero exec --traceinherits the environment) and compare the first-requestprovider_connectspan; optimized traces additionally carryprovider_prewarm+prewarm_attempts=1.Safety properties
Preserves permissions, sandboxing, secret scrubbing, output ceilings, transcript ordering, one result per tool call, reconnect/stall-retry behavior, and cancellation. Gate off ⇒ nil session field ⇒ the loop's default-adapter path, byte-identical. The prewarm probe carries no credentials. No new dependencies.
Verification
go build ./...,go vet ./...,gofmtclean on all touched filesinternal/trace,internal/zeroruntime,internal/providers(incl. the 180s openai suite),internal/agent, targetedinternal/cliprovider_prewarmand a regression asserts the probe never stampsprovider_connect; the probe is skipped on keep-alive-disabled transports (zero requests, zero counters — the macOS case), and probe-expecting tests pin reusable transports so all three platforms take the same test path; Stream delegates verbatim (SSE-verified against the direct provider); fingerprint ignores message growth but drifts on cache-key change, tool-set change, reasoning-effort change, description-only change, and tool reorder (wire order is preserved, not sorted), and an effort the wire omits fingerprints like no effort; stable/drift counters; gate default-off, falsey/truthy parsing, official-OpenAI-only, rejects compatible/chatgpt-catalog/foreign-provider values, refuses on capability-resolution failure; the switched-model fallback preserves the switched profile's resolved capabilities; end-to-end exec test proving the server sees the prewarm HEAD plus the turn's POST under the gate, and fallback-to-default for non-OpenAI providers with the gate onDependencies
Summary by CodeRabbit
zero exec, enabled withZERO_OPENAI_TURN_SESSION=1.