Skip to content

feat: measure provider startup before adding prepared sessions - #17

Merged
dviejokfs merged 3 commits into
mainfrom
feat/session-startup-timings
Sep 23, 2026
Merged

dviejokfs merged 3 commits into
mainfrom
feat/session-startup-timings

Conversation

@dviejokfs

@dviejokfs dviejokfs commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Fleet currently relaunches the SDK provider process for every turn: the retained client preserves session identity but reports retained_process: false. Add optional startup observations so embedders can measure the delay before implementing true process retention and early preparation.

AgentRuntimeBuilder::startup_observer reports payload-free monotonic boundaries for validation, concurrency admission, command/sandbox preparation, process spawn, initial stdin, stream attachment, first output, first nonempty assistant text, and completion/failure/cancellation/timeout/abandonment. Observers are disabled by default and do not modify provider events, remote protocols, prompts or process lifetime. Under unwinding builds, observer panics cannot fail the turn. Callbacks must remain nonblocking; the guide recommends bounded export.

ADR 0004 proposes opt-in prepared provider processes and documents the Fleet adoption boundary: launch context and MCP credentials currently arrive at turn time, so context resolution and revocation must be reconciled before preparation is enabled. This PR is the measurement foundation; it does not implement prepare() or claim faster sessions yet.

Validation:

  • Full macOS SDK suite: 311 passed, 1 existing ignored test.
  • Strict all-target/all-feature Clippy, cargo check, formatting and documentation checks pass.
  • Tests prove stage order, monotonic elapsed times, separation of first output from first text, payload exclusion, queue wait, future abandonment, validation failure, cancellation, timeout, and observer panic isolation.
  • Built on merged PR fix(windows): preserve provider launch environment and test native shims #16; cross-platform CI runs on this PR.

Live baseline (one two-turn pair per provider, macOS; pairs ran concurrently):

  • Claude new/resumed first text: 5,274 / 3,548 ms; process spawn: 4 / 3 ms.
  • Codex new/resumed first text: 5,236 / 4,719 ms; process spawn: 6 / 1 ms.
  • The example uses an isolated temporary workspace and prints timing metadata only. Both second turns resume session identity but still spawn fresh processes.

These are diagnostic samples, not controlled percentiles or Fleet end-to-end measurements. Most observed first-text time is after process creation; provider-ready and turn-accepted instrumentation is needed to separate initialization/MCP work from model latency before predicting preparation gains.

Review fixes:

  • Disable a failed observer across runtime clones and later turns; contain panic-payload destructor failures and suppress callbacks during caller unwinding.
  • Expose cumulative event_delivery_elapsed alongside elapsed time. Include normal events, terminal warnings, and interrupted waits while preserving bounded event processing. Document first-text as consumer-observed, not pure provider latency.
  • Five added regression tests cover nested panic cleanup, shared disablement, caller unwinding, interrupted delivery, slow consumers, and terminal warning delivery.

Evidence for the review fixes:

Before: observer_payload_cleanup_cannot_escape_or_repeat_after_failure FAILED
        observer cleanup escaped the isolation boundary
Before: startup_timings_separate_event_delivery_from_first_text_latency FAILED
        first-text latency must separately report time spent delivering earlier events
Before: startup_timings_include_terminal_warning_delivery FAILED
        terminal warning delivery must be included in cumulative sink wait

cargo test --locked --all-features
311 passed, 1 ignored (8 suites)
cargo test --locked startup -- --nocapture
11 passed, 0 failed
cargo clippy --locked --all-targets --all-features -- -D warnings
Passed
cargo check --lib --locked --all-features
Passed
RUSTDOCFLAGS='-D warnings' cargo doc --all-features --no-deps
Passed
cargo fmt --all -- --check
Passed
git diff --check
Passed

@dviejokfs

Copy link
Copy Markdown
Contributor Author

@greptile-apps

@greptile-apps

greptile-apps Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the remaining previous findings are non-blocking documentation issues concerning the StreamsAttached wording and README link placement.

Findings

  1. P2 Readiness boundary is overstated ▶
  2. P2 Guide link is misplaced ▶

Summary

Adds opt-in, payload-free startup timing observations across validation, concurrency admission, process setup, output processing, event delivery, and terminal outcomes.

  • Exposes startup observer types through the public API and integrates tracing into normal and sandbox-recovery execution.
  • Separates cumulative event-sink delivery time from observed first-text latency.
  • Contains observer panics, shares disablement across runtime clones, and suppresses callbacks during caller unwinding.
  • Adds usage documentation, a live timing example, regression coverage, and an ADR for future prepared provider processes.
Diagram
sequenceDiagram
    participant Caller
    participant Runtime
    participant Observer
    participant Provider
    participant Sink

    Caller->>Runtime: run(request)
    Runtime->>Observer: Started / Validated
    Runtime->>Observer: WaitingForPermit / PermitAcquired
    Runtime->>Provider: prepare command and spawn
    Runtime->>Observer: CommandPrepared / ProcessSpawned
    Runtime->>Provider: write initial input and attach streams
    Runtime->>Observer: InitialInputWritten / StreamsAttached
    Provider-->>Runtime: first output line
    Runtime->>Observer: FirstOutput
    Runtime->>Sink: emit normalized events
    Runtime->>Observer: FirstText with cumulative delivery wait
    Runtime-->>Caller: result or error
    Runtime->>Observer: terminal outcome
Loading

Reviews (2) · Last reviewed commit: "fix: isolate startup observers and accou..."

Comment thread src/startup.rs
Comment on lines +29 to +30
/// Provider streams were attached, including HTTP readiness where required.
StreamsAttached,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Readiness boundary is overstated

StreamsAttached is documented as including required HTTP readiness, but OpenCode serve mode records it immediately after attach() returns, before the background bridge finishes polling /global/health. Consumers may therefore treat the provider as ready up to roughly ten seconds early and attribute the readiness delay to the wrong interval. The description should clarify that this boundary only means the stream carrier was installed.

Suggested change
/// Provider streams were attached, including HTTP readiness where required.
StreamsAttached,
/// Provider streams were attached; HTTP readiness may still be pending.
StreamsAttached,

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread README.md

Licensed under either the Apache License, Version 2.0 or the MIT License, at
your option.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Guide link is misplaced

The new “Measure session startup” link appears beneath the License section instead of in the Documentation list. This makes it look like a license-related item and leaves the new guide out of the README’s documentation index. Move it into the existing Documentation list.

@dviejokfs
dviejokfs merged commit cee2aa9 into main Sep 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant