feat(openai): integrate exact lifecycle and trusted sessions - #1258
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
9c71109 to
ce58b04
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
crates/mesh-llm-host-runtime/src/logging/openai_lifecycle.rs (1)
355-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
trackedlock scope inbackend_dispatched.
backend_dispatchedholds theself.trackedmutex acrossmerge_request_metadata,enqueue_event, andstart_attempt. The sibling handlers added in this change (backend_terminal,backend_stream_first_item,response_completed,stream_terminal) release the lock before they callself.service. This method is the only one that does not.
LifecycleGuardis cloneable; line 538 already clones it. Clone the guard, release the lock, then perform the service calls and store the attempt under a second short lock.♻️ Suggested restructure
fn backend_dispatched(&self, request_id: RequestId, operation: OpenAiBackendOperation) { - let mut tracked = lock_recover(&self.tracked); - let Some(TrackedRequest::Active(active)) = tracked.requests.get_mut(&request_id) else { - return; - }; + let guard = { + let tracked = lock_recover(&self.tracked); + let Some(TrackedRequest::Active(active)) = tracked.requests.get(&request_id) else { + return; + }; + active.guard.clone() + }; let metadata = RequestSummaryMetadata::from_parts( None, None, Some("openai_frontend"), Some(operation_label(operation)), ); self.service .merge_request_metadata(request_id, metadata.clone()); let event = LifecycleEvent::RouteSelected { model: None, provider: metadata.provider().map(str::to_owned), engine: metadata.engine().map(str::to_owned), }; - if let Ok(payload) = serde_json::to_string(&event) { - let _ = self - .service - .enqueue_event(request_id, ReplayChannel::Operations, payload); - } - let attempt_id = self.service.start_attempt(request_id, &active.guard); - active.backend_attempt = Some((operation, attempt_id)); + self.enqueue_operation_event(request_id, event); + let attempt_id = self.service.start_attempt(request_id, &guard); + let mut tracked = lock_recover(&self.tracked); + if let Some(TrackedRequest::Active(active)) = tracked.requests.get_mut(&request_id) { + active.backend_attempt = Some((operation, attempt_id)); + } }The change also reuses the new
enqueue_operation_eventhelper instead of repeating the serialize-and-enqueue block.🤖 Prompt for 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. In `@crates/mesh-llm-host-runtime/src/logging/openai_lifecycle.rs` around lines 355 - 381, Update backend_dispatched to clone the active LifecycleGuard while holding the tracked lock, then release that lock before calling merge_request_metadata, enqueue_operation_event, and start_attempt. After obtaining the attempt ID, reacquire self.tracked briefly and update the matching active request’s backend_attempt, preserving the existing request lookup behavior. Reuse enqueue_operation_event for the RouteSelected event.crates/mesh-llm-host-runtime/src/api/routes/logs/dto.rs (1)
342-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one shared
event_kindmapping.
crates/mesh-llm-host-runtime/src/api/routes/logs/dto.rsandcrates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rscontain identicalevent_kindfunctions. This PR had to addbackend_stream_first_itemandusage_recordedto both. If a future variant is added to only one map, the REST DTO and the SSE protocol emit differentkindstrings for the same event, and the UIeventKindSchemapicklist rejects the unknown value.Move the mapping into one module in
crates/mesh-llm-host-runtime/src/api/routes/logs/and let both call sites use it.🤖 Prompt for 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. In `@crates/mesh-llm-host-runtime/src/api/routes/logs/dto.rs` around lines 342 - 362, The duplicate event_kind mappings in dto.rs and events/protocol.rs can diverge; extract one shared event_kind function into a common logs module, then update both DTO and SSE protocol call sites to reuse it. Preserve all existing variant-to-string mappings, including backend_stream_first_item and usage_recorded, and remove the duplicate local definitions.crates/mesh-llm-ui/src/features/logs/components/LogRequestEvidenceTimeline.test.tsx (1)
91-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a partial-usage case.
This test covers only the all-counters-present path.
tokenUsageEntriesincrates/mesh-llm-ui/src/features/logs/lib/log-token-usage.tsomits each entry when the counter isundefined, and falls back fromcompletionTokensto the legacytokensfield. Neither branch is exercised.Add one case with a subset of counters, for example
promptTokensset andcachedPromptTokensomitted, and one case wheretokenssupplies the completion count.Based on learnings, tests in
crates/mesh-llm-ui/**/*.test.{ts,tsx}should "Cover edge cases in tests: missing status, empty peers, client nodes, warm/cold models, malformed attachment data, and localStorage failures."🤖 Prompt for 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. In `@crates/mesh-llm-ui/src/features/logs/components/LogRequestEvidenceTimeline.test.tsx` around lines 91 - 114, Extend the usage evidence tests around LogRequestEvidenceTimeline with one partial-usage case that omits cachedPromptTokens and verifies tokenUsageEntries excludes the missing counter, plus one case where the legacy tokens field supplies the completion count when completionTokens is absent. Keep the existing all-counters-present assertion unchanged.Source: Learnings
crates/openai-frontend/src/errors.rs (1)
133-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing 499 constant instead of the inline literal.
The crate already defines
CLIENT_CLOSED_REQUEST_STATUSinlifecycle.rs;backend_lifecycle.rsimports it at line 9 andrequest_lifecycle.rscompares against it. This constructor re-declares 499 as a literal and unwraps it. Two definitions of the same status can drift.
StatusCode::from_u16(499)never fails, so theexpectis safe. The concern is duplication, not a panic.♻️ Proposed change to reuse the shared constant
pub fn cancelled(message: impl Into<String>) -> Self { Self::from_kind( - StatusCode::from_u16(499).expect("499 is a valid HTTP status"), + StatusCode::from_u16(crate::lifecycle::CLIENT_CLOSED_REQUEST_STATUS) + .expect("the client-closed status is a valid HTTP status"), OpenAiErrorKind::Cancelled, message, ) }🤖 Prompt for 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. In `@crates/openai-frontend/src/errors.rs` around lines 133 - 139, Update the cancelled constructor to use the existing CLIENT_CLOSED_REQUEST_STATUS constant from lifecycle.rs instead of constructing status 499 inline. Preserve the current OpenAiErrorKind::Cancelled and message handling while removing the duplicate conversion and expect.crates/openai-frontend/src/router.rs (1)
339-344: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the full response to read usage.
response.clone()duplicates the entire response body on every non-streaming request. The clone exists only because&response.usageis borrowed in the same call expression. Chat completion bodies can be large.Extract the usage first, then move the response. The same pattern appears at lines 675-680 in
completionsand at lines 599-601 inresponses.♻️ Proposed change to remove the clone
state.response_completed( &context, OpenAiBackendOperation::ChatCompletion, &response.usage, ); - Ok(json_response_with_usage(response.clone(), &response.usage)) + let usage = response.usage.clone(); + Ok(json_response_with_usage(response, &usage)) }If
UsageisCopy, drop the.clone()on the extracted value.🤖 Prompt for 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. In `@crates/openai-frontend/src/router.rs` around lines 339 - 344, In the non-streaming chat completion flow, extract `response.usage` into a local before calling `json_response_with_usage`, then move `response` into that function instead of cloning it. Apply the same ownership pattern in the `completions` and `responses` flows, and omit the usage clone if `Usage` implements `Copy`.crates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rs (1)
154-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
event_kindmapping betweenprotocol.rsanddto.rs.Both files contain identical 17-arm mappings. Move the mapping into one shared function to prevent future divergence.
🤖 Prompt for 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. In `@crates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rs` around lines 154 - 158, Extract the duplicated 17-arm LifecycleEvent-to-event_kind mapping from protocol.rs and dto.rs into one shared function, then update both callers to reuse it. Preserve every existing string mapping, including the StreamStarted, StreamChunk, StreamCompleted, and UsageRecorded variants.
🤖 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 `@crates/openai-frontend/src/errors.rs`:
- Line 214: Update the OpenAiErrorKind::Cancelled mapping to use
invalid_request_error as the error type while retaining request_cancelled as the
error code.
In `@crates/openai-frontend/src/router.rs`:
- Around line 369-388: Refactor the responses handler into semantic helper
functions for its streaming and non-streaming branches, keeping each branch’s
existing behavior and error propagation unchanged. Move the logic currently
represented by the ResponsesStream flow, including call_backend_with_context and
observe_backend_stream, into the streaming helper, and place the regular
response path in a corresponding non-streaming helper so responses is below the
configured line threshold.
In `@crates/skippy-server/src/frontend/generation/cache_hints.rs`:
- Around line 62-71: Update the untrusted-session branch in the session label
construction to include process_nonce() alongside the timestamp and sequence,
ensuring session_id differs across processes. Keep the trusted agent-session
path based solely on stable_wire_id and preserve its existing stable identity
behavior.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/api/routes/logs/dto.rs`:
- Around line 342-362: The duplicate event_kind mappings in dto.rs and
events/protocol.rs can diverge; extract one shared event_kind function into a
common logs module, then update both DTO and SSE protocol call sites to reuse
it. Preserve all existing variant-to-string mappings, including
backend_stream_first_item and usage_recorded, and remove the duplicate local
definitions.
In `@crates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rs`:
- Around line 154-158: Extract the duplicated 17-arm
LifecycleEvent-to-event_kind mapping from protocol.rs and dto.rs into one shared
function, then update both callers to reuse it. Preserve every existing string
mapping, including the StreamStarted, StreamChunk, StreamCompleted, and
UsageRecorded variants.
In `@crates/mesh-llm-host-runtime/src/logging/openai_lifecycle.rs`:
- Around line 355-381: Update backend_dispatched to clone the active
LifecycleGuard while holding the tracked lock, then release that lock before
calling merge_request_metadata, enqueue_operation_event, and start_attempt.
After obtaining the attempt ID, reacquire self.tracked briefly and update the
matching active request’s backend_attempt, preserving the existing request
lookup behavior. Reuse enqueue_operation_event for the RouteSelected event.
In
`@crates/mesh-llm-ui/src/features/logs/components/LogRequestEvidenceTimeline.test.tsx`:
- Around line 91-114: Extend the usage evidence tests around
LogRequestEvidenceTimeline with one partial-usage case that omits
cachedPromptTokens and verifies tokenUsageEntries excludes the missing counter,
plus one case where the legacy tokens field supplies the completion count when
completionTokens is absent. Keep the existing all-counters-present assertion
unchanged.
In `@crates/openai-frontend/src/errors.rs`:
- Around line 133-139: Update the cancelled constructor to use the existing
CLIENT_CLOSED_REQUEST_STATUS constant from lifecycle.rs instead of constructing
status 499 inline. Preserve the current OpenAiErrorKind::Cancelled and message
handling while removing the duplicate conversion and expect.
In `@crates/openai-frontend/src/router.rs`:
- Around line 339-344: In the non-streaming chat completion flow, extract
`response.usage` into a local before calling `json_response_with_usage`, then
move `response` into that function instead of cloning it. Apply the same
ownership pattern in the `completions` and `responses` flows, and omit the usage
clone if `Usage` implements `Copy`.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37f32e22-4be1-48a3-9c00-c1563147dfd9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
crates/mesh-llm-events/src/logging/events.rscrates/mesh-llm-events/src/logging/presentation.rscrates/mesh-llm-events/src/logging/tests.rscrates/mesh-llm-host-runtime/src/api/routes/logs/dto.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rscrates/mesh-llm-host-runtime/src/logging/openai_lifecycle.rscrates/mesh-llm-log-store/src/repositories.rscrates/mesh-llm-tui/src/output/logging_projection.rscrates/mesh-llm-ui/src/features/logs/api/schemas.test.tscrates/mesh-llm-ui/src/features/logs/api/schemas.tscrates/mesh-llm-ui/src/features/logs/components/LogRequestEvidenceTimeline.test.tsxcrates/mesh-llm-ui/src/features/logs/lib/log-fixtures.test.tscrates/mesh-llm-ui/src/features/logs/lib/log-fixtures/lifecycle-events.tscrates/mesh-llm-ui/src/features/logs/lib/log-timeline.test.tscrates/mesh-llm-ui/src/features/logs/lib/log-timeline.tscrates/mesh-llm-ui/src/features/logs/lib/log-token-usage.tscrates/openai-frontend/src/backend.rscrates/openai-frontend/src/backend_lifecycle.rscrates/openai-frontend/src/common.rscrates/openai-frontend/src/errors.rscrates/openai-frontend/src/lib.rscrates/openai-frontend/src/lifecycle.rscrates/openai-frontend/src/request_lifecycle.rscrates/openai-frontend/src/router.rscrates/openai-frontend/src/router_tests.rscrates/openai-frontend/src/stream_lifecycle.rscrates/openai-frontend/tests/lifecycle_observer.rscrates/openai-frontend/tests/request_context_wrappers.rscrates/skippy-server/Cargo.tomlcrates/skippy-server/src/frontend/backend.rscrates/skippy-server/src/frontend/backend/tests.rscrates/skippy-server/src/frontend/generation/cache_hints.rscrates/skippy-server/src/frontend/generation/queue.rscrates/skippy-server/src/frontend/generation/server.rscrates/skippy-server/src/frontend/generation/types.rscrates/skippy-server/src/frontend/local_generation/linear_decode.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/tests/generation.rscrates/skippy-server/src/frontend/tests/multimodal.rs
4239da0 to
dfa78b4
Compare
1 participant
Summary
Stacked on #1176 (
logging-ui-v3). Recreates the functional behavior from #1234 using the typed lifecycle, durable logging, API, TUI, and console constructs introduced by #1174–#1176.include_usagewire behaviorDeliberate differences from #1234
RequestIdand typed lifecycle observer; do not add a parallel free-form tracing lifecycle[DONE], so drop-after-DONE remains successfulValidation
cargo fmt --all -- --checkcargo test -p openai-frontend(172 unit + 10 integration/contract tests)cargo test -p skippy-server --lib(414 tests)cargo test -p mesh-llm-events(81 tests)cargo test -p mesh-llm-log-store(114 tests)cargo test -p mesh-llm-host-runtime --lib logging::openai_lifecycle(8 tests)cargo test -p mesh-llm-host-runtime --lib api::routes::logs::dto::usage_tests(1 test)cargo clippy -p openai-frontend --all-targets -- -D warningscargo clippy -p skippy-server --all-targets -- -D warningscargo clippy -p mesh-llm-host-runtime -p mesh-llm-events -p mesh-llm-log-store -p mesh-llm-tui --all-targets -- -D warningsjust ui-test(1,301 passed, 3 skipped)just build(backend-neutral debug host + packaged Metal runtime)./target/debug/mesh-llm --versionHardware-backed two-node/public-mesh/agent-loop validation remains appropriate before merge; this draft intentionally leaves those deployment gates to the stacked review/CI phase.
Supersedes the implementation approach in #1234; references its behavior and review findings without cherry-picking its commits.
Summary by CodeRabbit