feat(logging): expose audited local log APIs and runtime telemetry - #1175
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:
📝 WalkthroughWalkthroughThis PR adds a complete local logging and audit platform. It introduces durable SQLite storage ( ChangesLocal Logging Platform (Core)
Local Logging API
Operational Audit Events
OpenAI Lifecycle Observability
Log Store Engine (mesh-llm-log-store)
Other
Estimated code review effort: 5 (Critical) | ~240 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAiIngress
participant OpenAiLifecycleLoggingAdapter
participant LoggingService
participant LogStore
participant ReplayBus
Client->>OpenAiIngress: HTTP request
OpenAiIngress->>OpenAiLifecycleLoggingAdapter: claim lifecycle ownership
OpenAiLifecycleLoggingAdapter->>LoggingService: register admitted event
OpenAiIngress->>OpenAiIngress: route and dispatch request
OpenAiIngress->>OpenAiLifecycleLoggingAdapter: record terminal outcome (status, usage)
OpenAiLifecycleLoggingAdapter->>LoggingService: enqueue terminal lifecycle event
LoggingService->>ReplayBus: push replay record
LoggingService->>LogStore: persist summary and lifecycle event
ReplayBus-->>Client: SSE replay/live event (via Logging API stream)
sequenceDiagram
participant Client
participant LogsApi
participant LoggingQueryFacade
participant LogStore
participant ArtifactFileStore
Client->>LogsApi: POST /api/logs/requests/{id}/delete
LogsApi->>LoggingQueryFacade: prepare_delete_request
LoggingQueryFacade->>LogStore: persist immutable receipt and targets
LogsApi->>LoggingQueryFacade: execute_prepared_delete_request
LoggingQueryFacade->>ArtifactFileStore: delete artifact files (cancellable)
ArtifactFileStore-->>LoggingQueryFacade: deletion progress
LoggingQueryFacade->>LogStore: complete receipt, cascade delete rows
LogsApi-->>Client: completed or pending (202) receipt
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. |
feb1bde to
9063ec9
Compare
de9d7e4 to
497f25e
Compare
1913498 to
6284058
Compare
6284058 to
e0ab901
Compare
d59d873 to
aff70f2
Compare
aff70f2 to
9771428
Compare
9771428 to
bc88dad
Compare
bc88dad to
8fafa6b
Compare
8fafa6b to
356ca7a
Compare
0f7702b to
327be35
Compare
a69f411 to
fe69495
Compare
1d3aebc to
d25681e
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-events/src/logging/presentation.rs (1)
67-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude terminal status in failed and rejected summaries.
FailedandRejectednow carrystatus_code, but these branches discard it.presentation_local_summary()therefore omits the terminal HTTP status for failed and rejected requests, unlike completed attempts and requests. Useappend_statusfor both branches.Proposed fix
- LifecycleEvent::Failed { .. } => "request failed".to_string(), - LifecycleEvent::Rejected { .. } => "request rejected".to_string(), + LifecycleEvent::Failed { status_code, .. } => { + append_status("request failed".to_string(), status_code) + } + LifecycleEvent::Rejected { status_code, .. } => { + append_status("request rejected".to_string(), status_code) + }🤖 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-events/src/logging/presentation.rs` around lines 67 - 76, Update the LifecycleEvent::Failed and LifecycleEvent::Rejected branches in presentation_local_summary() to extract their status_code and pass the corresponding summary through append_status, preserving the existing “request failed” and “request rejected” messages while including the terminal HTTP status.
🟡 Minor comments (20)
scripts/qa-logging-recovery.sh-391-394 (1)
391-394: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not suppress rejected-request transport failures.
The trailing
|| truemakessubmit_rejected_requestreturn success after a curl failure. The later request-list timeout then reportsPREREQ, which can hide a failed logging path.Return failure when curl cannot contact the node. Also verify that the response represents the expected rejected request before using it as lifecycle evidence.
🤖 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 `@scripts/qa-logging-recovery.sh` around lines 391 - 394, The curl invocation in submit_rejected_request must propagate transport failures instead of masking them with || true. Remove the unconditional success fallback, then validate the returned response matches the expected rejected request before treating it as lifecycle evidence; only proceed to the request-list timeout check after both conditions succeed.scripts/qa-logging-recovery.sh-600-604 (1)
600-604: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the typed unavailable error body.
This check accepts any HTTP 503 response. It does not verify the documented typed logging-unavailable response. A proxy or unrelated server failure can therefore pass the certification.
Parse the response envelope and require the expected unavailable error code before recording
logging_fail_openasPASS.🤖 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 `@scripts/qa-logging-recovery.sh` around lines 600 - 604, Update the logging_fail_open validation around the curl request and record_result call to parse fail-open-logs.json and require the documented typed logging-unavailable error code in the response envelope, not merely HTTP status 503. Record PASS only when both the status and expected error code match; otherwise preserve the existing FAIL path and diagnostic status.scripts/qa-control-plane-mixed-version.sh-1182-1187 (1)
1182-1187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate every canonical model reference before sorting.
An inventory such as
[{"metadata": {}}]passes this check. Its reference list is[None], andsorted([None])succeeds. The harness then reports a valid typed inventory without a canonical model reference.Require each entry to be an object with a non-empty string
canonical_model_refand a metadata object before comparing the ordered references.🤖 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 `@scripts/qa-control-plane-mixed-version.sh` around lines 1182 - 1187, Update the inventory validation after the list check to require every entry to be an object with a non-empty string canonical_model_ref and a metadata dictionary before building refs or sorting. Then compare the validated references for ordering, preserving the existing failure behavior for invalid or unsorted inventories.crates/mesh-llm-host-runtime/src/api/routes/logs/parse.rs-473-481 (1)
473-481: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe path-shape heuristic redacts legitimate operator reasons.
The
path_shapedtest is broader than a path check:
value.as_bytes().get(1) == Some(&b':')matches any reason whose second byte is a colon. A reason such asa: cleanup after incidentbecomes[REDACTED].value.contains('\\')matches any reason containing a backslash anywhere.When the heuristic fires, the entire reason is replaced. The audit entry then records no operator intent at all, and
maintenance_reasonstores[REDACTED]as the maintenance reason. The caller receives no signal that the text was discarded.Narrow the drive-letter check to a single ASCII alphabetic first byte followed by
:and a separator, and applyapply_redactioninstead of whole-value replacement for the backslash case.🔧 Proposed narrowing
- let path_shaped = value.starts_with('/') - || value.starts_with("~/") - || value.as_bytes().get(1) == Some(&b':') - || value.contains('\\'); + let bytes = value.as_bytes(); + let windows_drive = bytes.first().is_some_and(u8::is_ascii_alphabetic) + && bytes.get(1) == Some(&b':') + && matches!(bytes.get(2), Some(b'\\') | Some(b'/')); + let path_shaped = value.starts_with('/') || value.starts_with("~/") || windows_drive;🤖 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/parse.rs` around lines 473 - 481, Refine the path_shaped logic around the value sanitization expression: require the drive-letter form to have an ASCII alphabetic first byte, a colon, and a path separator, and handle backslash-containing values through crate::logging::policy::apply_redaction rather than replacing the entire reason. Preserve whole-value redaction only for clearly identified path-shaped inputs such as absolute or home-relative paths, while retaining operator text and maintenance_reason content whenever possible.crates/mesh-llm-host-runtime/src/api/routes/diagnostics.rs-10-22 (1)
10-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
/api/diagnosticsreturns 404 when the request carries a query string.
crates/mesh-llm-host-runtime/src/api/routes/mod.rsmatches this route onpath_onlybut passes the fullpath.GET /api/diagnostics?foo=1therefore reacheshandle, fails the exact equality at Line 18, and returns 404. The split-readiness branch usesstarts_with, so it does not have this behavior.🐛 Proposed fix
- if path == "/api/diagnostics" { + if path.split('?').next() == Some("/api/diagnostics") { return handle_general_diagnostics(stream, state).await; }🤖 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/diagnostics.rs` around lines 10 - 22, Update handle in the diagnostics route so the general diagnostics comparison uses the query-free path, matching the path_only value used by routes::mod. Preserve the existing split-readiness routing and 404 behavior for unrelated paths, while allowing /api/diagnostics requests with query strings to reach handle_general_diagnostics.crates/mesh-llm-host-runtime/src/api/routes/logs/delete.rs-144-158 (1)
144-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBuild the shared deadline from the injected time caps, not the constant.
Line 150 uses
DELETE_TIME_CAPwhile the surrounding code honorsprepare_time_capandexecution_time_cap. The injected caps therefore control only the Tokio timeouts, and the cooperative store cancellation still runs on the fixed two-second budget. That makes the parameters misleading and weakens the test at Lines 317-394.🐛 Proposed fix
- let control = MaintenanceDeadline::new(DELETE_TIME_CAP); + let control = MaintenanceDeadline::new(prepare_time_cap.max(execution_time_cap));🤖 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/delete.rs` around lines 144 - 158, Update the maintenance deadline initialization in the delete handler to use the injected prepare_time_cap and execution_time_cap values instead of the fixed DELETE_TIME_CAP constant. Ensure MaintenanceDeadline enforces the same configured caps used by timeout_maintenance, while preserving the existing prepare and execution flow.crates/mesh-llm-host-runtime/src/api/routes/logs/events/session.rs-338-362 (1)
338-362: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the
expecton the lifecycle gap frame with theErrhandling used on the audit path.
recovery_cursoris an opaque cursor supplied by the durable query layer, and this function does not bound its length. Ifframe()rejects the result againstMAX_FRAME_BYTES, thisexpectpanics inside the spawned producer task.The audit path already degrades gracefully for the same condition. See Line 230 (
if let Ok(frame) = gap) and Line 277.🛡️ Proposed fix
- .filter_map(|channel| { - let requested = subscription.cursor.sequence(*channel); - let channel_evicted_through = evicted_through.sequence(*channel); - (requested < channel_evicted_through).then(|| { - let gap = GapData::new( - *channel, - requested.saturating_add(1), - channel_evicted_through, - recovery_cursor.clone(), - ); - gap_frame(cursor_from_replay(latest), &gap) - .expect("bounded replay-gap data fits the SSE frame cap") - }) - }) + .filter_map(|channel| { + let requested = subscription.cursor.sequence(*channel); + let channel_evicted_through = evicted_through.sequence(*channel); + if requested >= channel_evicted_through { + return None; + } + let gap = GapData::new( + *channel, + requested.saturating_add(1), + channel_evicted_through, + recovery_cursor.clone(), + ); + gap_frame(cursor_from_replay(latest), &gap).ok() + })🤖 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/session.rs` around lines 338 - 362, Update gap_frames to handle a failed gap_frame conversion without panicking: replace the expect with Err-aware filtering so oversized lifecycle gap frames are skipped, matching the audit path’s graceful handling around the referenced gap processing. Preserve successful frame collection and the existing gap construction behavior.crates/mesh-llm-host-runtime/src/logging/metrics.rs-171-180 (1)
171-180: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the read guard before you call the sink.
Line 175 shadows the guard binding with the cloned
Option<Arc<...>>. Shadowing does not drop the guard. The read lock stays held until the end ofrecord, sosink.record(metric)runs while the lock is held. A sink that blocks then blocksset_sink, and a sink that callsrecordagain can deadlock against a waiting writer.Bind the guard to a separate name and drop it before the call.
🔒 Proposed fix to release the guard
pub(crate) fn record(&self, metric: LoggingMetric) { - let Ok(sink) = self.sink.try_read() else { + let Ok(guard) = self.sink.try_read() else { return; }; - let sink = sink.clone(); + let sink = guard.clone(); + drop(guard); let Some(sink) = sink else { return; }; let _ = catch_unwind(AssertUnwindSafe(|| sink.record(metric))); }🤖 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/metrics.rs` around lines 171 - 180, Update LoggingMetricRecorder::record to bind the try_read guard separately from the cloned sink, then explicitly release the guard before invoking sink.record(metric). Preserve the existing early returns and panic handling while ensuring the sink call occurs without the read lock held.crates/mesh-llm-log-store/src/api_acceptance_tests/cursor_pagination.rs-11-12 (1)
11-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the comment about unique timestamps.
The comment states that unique sequential timestamps cause gaps, and that a cursor at T3 skips T4. That does not match the keyset implementation. The pagination predicate is a strict tuple comparison
(occurred_at, artifact_id) < (?, ?)against the last returned row, so it never skips a row, whether or not timestamps repeat. The duplicate timestamps in this test exercise the tiebreak path; they are not required for correctness.Reword the comment so it does not document a constraint that the implementation does not have.
🤖 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-log-store/src/api_acceptance_tests/cursor_pagination.rs` around lines 11 - 12, Reword the comments in the cursor pagination test to remove the claim that unique sequential timestamps cause gaps or that a cursor at T3 skips T4. State that duplicate timestamps are included to exercise the artifact_id tiebreak path, while pagination remains correct with unique timestamps.crates/mesh-llm-log-store/src/api_acceptance_tests/cursor_pagination.rs-215-228 (1)
215-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe ordering assertion does not test ordering.
Line 222 copies
all_ids, line 223 sorts the copy descending, and line 224 asserts onsorted[0]. The sort discards the order that pagination produced. The assertion then only states thatsame-ts-0004is the lexicographic maximum of the five inserted IDs, which holds for any page order. The comment on line 221 states that DESC ordering is verified, but it is not.Assert on
all_idsdirectly.💚 Proposed fix
- // Verify ordering: DESC on (created_at, request_id), so highest ID first. - let mut sorted = all_ids.clone(); - sorted.sort_unstable_by(|a, b| b.cmp(a)); - assert_eq!( - sorted[0], "same-ts-0004", - "DESC order means highest ID first" - ); + // Verify ordering: DESC on (created_at, request_id), so highest ID first. + let expected: Vec<String> = (0..5u32) + .rev() + .map(|i| format!("same-ts-{i:04}")) + .collect(); + assert_eq!( + all_ids, expected, + "pages must arrive in DESC (created_at, request_id) order" + );🤖 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-log-store/src/api_acceptance_tests/cursor_pagination.rs` around lines 215 - 228, Remove the copied-and-sorted validation around all_ids and assert directly on the pagination result, such as checking all_ids[0] is "same-ts-0004". Keep the existing count assertion and DESC ordering comment, ensuring the test verifies the order returned by pagination rather than reordering it first.crates/mesh-llm-log-store/src/api_acceptance_tests/mod.rs-35-40 (1)
35-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
TestClockstops being monotonic after 60 calls.
nowformatsn % 60into the seconds field. Call 60 returns the same string as call 0, and every later call returns a timestamp earlier than the preceding ones. Durable ordering, keyset cursors, and terminal-at comparisons all key on this string. Any acceptance test in this module tree that issues more than 60 clock reads gets silently inverted ordering rather than a failure.Widen the formatted range so the clock stays monotonic.
💚 Proposed fix
impl ClockTrait for TestClock { fn now(&self) -> String { let n = self.instant.fetch_add(1, Ordering::Relaxed); - format!("2025-01-01T00:00:{:02}Z", n % 60) + format!( + "2025-01-01T{:02}:{:02}:{:02}Z", + (n / 3600) % 24, + (n / 60) % 60, + n % 60 + ) } }🤖 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-log-store/src/api_acceptance_tests/mod.rs` around lines 35 - 40, Update TestClock::now to format the incrementing instant without wrapping at 60, using a sufficiently wide seconds field so successive calls remain lexicographically monotonic beyond 60 reads while preserving the existing timestamp format.crates/mesh-llm-log-store/src/capture.rs-226-237 (1)
226-237: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MaintenanceExecutionCancelledmisrepresents a disabled capture facade.A disabled capture facade is not a cancelled or timed-out operation. The API layer maps
MaintenanceExecutionCancelledto a cancellation/deadline outcome, so an operator sees a timeout instead of "artifact capture unavailable".crates/mesh-llm-log-store/src/maintenance/metadata_delete.rsalready usesLogStoreError::ArtifactDeletionUnavailablefor the same "no trusted artifact owner" condition. Use that variant here and indelete_request_cascade,prepare_delete_request, andexecute_prepared_delete_request.🤖 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-log-store/src/capture.rs` around lines 226 - 237, Replace the disabled-facade error returned by execute_cleanup with LogStoreError::ArtifactDeletionUnavailable instead of MaintenanceExecutionCancelled. Apply the same variant consistently in delete_request_cascade, prepare_delete_request, and execute_prepared_delete_request when no available capture store exists, preserving cancellation handling for actual cancelled or timed-out operations.crates/mesh-llm-host-runtime/src/network/openai/response/stream_translation.rs-147-157 (1)
147-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe two stream adapters end a failed stream differently.
For the normalized path, Lines 147-148 write the chunked terminator
0\r\n\r\nand shut the socket down before the!done_seencheck at Line 149. The client receives a well-formed, complete-looking chunked response that simply lacksdata: [DONE].For the translated path, the
!done_seencheck at Line 252 returns before any terminator is written. The client sees a truncated chunked body.Both cases represent the same failure class, so a client cannot use the framing to detect it consistently. Choose one convention for both adapters.
Also applies to: 252-259
🤖 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/network/openai/response/stream_translation.rs` around lines 147 - 157, Make the normalized and translated stream adapters use the same failure framing when !done_seen. Align the end-of-stream handling around the relevant adapter completion logic so both paths either write the chunked terminator and shut down before returning the incomplete-stream error, or both return before doing so; preserve the existing stream_error and successful completion behavior.crates/mesh-llm-host-runtime/src/network/openai/response/routing.rs-228-245 (1)
228-245: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe test does not cover what its name states.
Three problems:
_routeis unused. Both loop iterations run the identicaltokio::io::duplexforwarding, so the remote path is never exercised.route_remote_attemptforwards throughforward_peer_request, notforward_buffered_request.- No request-ID logic runs.
forward_buffered_requestis awrite_allwrapper, so the assertion only proves thatwrite_allcopies bytes.- The remote path applies
prepare_peer_forwarded_requestbefore writing, which can change the bytes. The test claims byte preservation for that path without exercising it.Either drop the loop and rename the test to describe the local wrapper, or assert on
prepare_peer_forwarded_requestoutput for the remote claim.🤖 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/network/openai/response/routing.rs` around lines 228 - 245, The test local_and_remote_forwarding_preserve_the_canonical_request_id_bytes does not exercise either route-specific request-ID behavior. Remove the unused route loop and rename the test to describe only forward_buffered_request, or replace the remote iteration with an assertion against prepare_peer_forwarded_request output before forwarding so the remote byte transformation is actually covered.crates/mesh-llm-host-runtime/src/network/openai/response/pipeline.rs-158-163 (1)
158-163: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA failed
shutdownis reported as a dropped response.Both paths write the complete body first and then call
shutdown. If onlyshutdownfails, the client already received the full response. ReturningDroppedrecords a delivered response as a dropped one, which corrupts the terminal outcome for that request.A client that closes its read side immediately after reading makes this path reachable.
Treat a
shutdownerror as a completed response.🐛 Proposed fix for the streaming path
- if client_stream.write_all(b"0\r\n\r\n").await.is_err() - || client_stream.shutdown().await.is_err() - { + if client_stream.write_all(b"0\r\n\r\n").await.is_err() { return PipelineProxyResult::Dropped; } + let _ = client_stream.shutdown().await; completed_pipeline_response(status, usage_parser.usage)🐛 Proposed fix for the non-streaming path
if client_stream.write_all(header.as_bytes()).await.is_err() || client_stream.write_all(&resp_bytes).await.is_err() - || client_stream.shutdown().await.is_err() { PipelineProxyResult::Dropped } else { + let _ = client_stream.shutdown().await; completed_pipeline_response(status, usage) }Also applies to: 234-241
🤖 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/network/openai/response/pipeline.rs` around lines 158 - 163, Update the response-finalization logic in both streaming and non-streaming paths around client_stream.write_all and client_stream.shutdown: return Dropped only when writing the complete body fails, and ignore shutdown errors so a fully written response proceeds to completed_pipeline_response(status, usage_parser.usage).crates/mesh-llm-host-runtime/src/network/openai/transport.rs-99-138 (1)
99-138: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThree independent terminal-outcome mappers classify the same HTTP status differently. The file now contains three functions that each convert a result into a
TerminalOutcome, and none of them share a status-classification rule. A single upstream status therefore lands in different buckets depending on which path terminalizes the request, so audit and telemetry consumers cannot reconcile them. Extract one status-to-TerminalOutcomeclassifier and call it from all three.
crates/mesh-llm-host-runtime/src/network/openai/transport.rs#L99-L138:RouteDispatchOutcome::terminal_outcometreats only200..=299as success, so aResponded(302)becomesFailedWithStatus.crates/mesh-llm-host-runtime/src/network/openai/transport.rs#L831-L872:terminal_outcome_for_mesh_route_resulttreats(200..400)as success at Lines 838 and 841, so a delivered 302 becomesCompletedWithStatus.proxy_result_metadataat Line 733 uses the same200..400range.crates/mesh-llm-host-runtime/src/network/openai/transport.rs#L874-L888:terminal_outcome_for_mesh_request_failureignores the status it just sent.ModelUnavailablesends HTTP 429 at Lines 581-587 but maps toFailed, whereas the dispatch mapper maps every 4xx toRejectedWithStatus.🤖 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/network/openai/transport.rs` around lines 99 - 138, Extract a shared HTTP status-to-TerminalOutcome classifier and use it from RouteDispatchOutcome::terminal_outcome, terminal_outcome_for_mesh_route_result, and terminal_outcome_for_mesh_request_failure so identical statuses receive identical classifications. Apply one consistent success range, preserve 4xx as RejectedWithStatus, classify other statuses as FailedWithStatus, and ensure failure paths use their actual HTTP status, including ModelUnavailable’s 429. Update proxy_result_metadata to use the same status range. Affected sites: crates/mesh-llm-host-runtime/src/network/openai/transport.rs:99-138, 831-872, and 874-888 all require routing through the shared classifier.crates/mesh-llm-host-runtime/src/network/openai/ingress.rs-611-618 (1)
611-618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord the route selection before the admission response is written.
check_activity_admissionwrites the observed 503 and returnsErr(outcome). Only after that does this branch callroute_selected_with_metadata. The lifecycle therefore records the response observation beforeRouteSelected.Move the metadata call above the admission check so the event order matches the request order.
♻️ Proposed fix
+ route_observer.route_selected_with_metadata(Some(model_name), Some("plugin"), Some("admission")); match check_activity_admission( tcp_stream, &ctx.node.activity_policy_guard, crate::runtime::IngressType::PluginDispatch, route_observer, ) .await { Ok(stream) => tcp_stream = stream, - Err(outcome) => { - route_observer.route_selected_with_metadata( - Some(model_name), - Some("plugin"), - Some("admission"), - ); - return outcome; - } + Err(outcome) => return outcome, }Note that this also records the selection on the allowed path. If that is not wanted, keep the call inside the
Errarm but move it beforecheck_activity_admissionusing an early admission probe.🤖 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/network/openai/ingress.rs` around lines 611 - 618, Move the route_selected_with_metadata call associated with admission from the Err(outcome) handling to before check_activity_admission, ensuring RouteSelected is recorded before any admission response is written. Preserve the existing model_name, "plugin", and "admission" metadata, and retain the allowed-path behavior described by the review.crates/mesh-llm-host-runtime/src/logging/persistence.rs-339-345 (1)
339-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve severity when the audit record also carries a detail.
insert_audit_entryhas no severity parameter, so severity reaches storage only insidedetail_json. Theelsebranch at Line 341 is the only place that writes it.When a record carries both a detail and a severity, the
ifbranch runs and the severity is discarded. Operator filtering on audit severity then returns inconsistent results, because whether severity is present depends on whether an unrelated detail field was set.Merge the severity into the detail object instead of choosing between them.
🐛 Proposed fix to merge severity into the detail
- let detail_json = if let Some(detail_json) = record.detail_json() { - Some(apply_redaction(&sanitize_paths_in_text(detail_json)).0) - } else { - record - .severity() - .map(|severity| serde_json::json!({ "severity": severity.as_str() }).to_string()) - }; + let severity = record.severity().map(|severity| severity.as_str()); + let detail_json = match (record.detail_json(), severity) { + (Some(detail_json), severity) => { + let sanitized = apply_redaction(&sanitize_paths_in_text(detail_json)).0; + Some(match (severity, serde_json::from_str::<serde_json::Value>(&sanitized)) { + (Some(severity), Ok(serde_json::Value::Object(mut fields))) => { + fields + .entry("severity") + .or_insert_with(|| serde_json::Value::String(severity.to_string())); + serde_json::Value::Object(fields).to_string() + } + _ => sanitized, + }) + } + (None, Some(severity)) => { + Some(serde_json::json!({ "severity": severity }).to_string()) + } + (None, None) => None, + };The
or_insert_withcall keeps an explicit severity already present in the detail. Add a test that builds a record with both fields and asserts the persisted detail contains the severity.🤖 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/persistence.rs` around lines 339 - 345, Update the detail_json construction in insert_audit_entry so records containing both detail_json and severity preserve both values. Parse the sanitized detail as a JSON object, insert severity only when that key is absent, and serialize the merged object; retain the existing severity-only fallback for records without detail. Add a test covering both fields and asserting the persisted detail includes severity.crates/mesh-llm-host-runtime/src/logging/service.rs-1693-1705 (1)
1693-1705: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRecover from a poisoned delivery mutex in the status read.
persistence_worker_statepanics when the delivery mutex is poisoned.LoggingRuntimeState::statuscalls this method on the trusted-local status path, so one earlier panic under the delivery lock converts every later status read into a panic. Logging status must stay fail-open. Recover the guard like the other health reads in this feature.🛡️ Proposed fix
pub(crate) fn persistence_worker_state(&self) -> PersistenceWorkerState { - let delivery = self.delivery.lock().expect("delivery mutex poisoned"); + let delivery = self + .delivery + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); match &*delivery {🤖 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/service.rs` around lines 1693 - 1705, Update LoggingRuntimeState::persistence_worker_state to recover from a poisoned delivery mutex instead of calling expect, following the guard-recovery pattern used by the other health reads. Preserve the existing DeliveryMode-to-PersistenceWorkerState mapping and ensure status reads remain fail-open.crates/mesh-llm-host-runtime/src/logging/service/artifact_persistence.rs-116-151 (1)
116-151: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCheck the size bound before validating JSON content.
Line 123 calls
validated_media_kind(media_kind, Some(content))before the size check at Line 124. When the declared essence is JSON,validated_media_kindparses the whole slice withserde_json::from_slice. An oversized body is therefore fully parsed on the ingress thread and then discarded as metadata-only. The size guard exists to keep large bodies off the request path, and this parse costs more than the copy the guard avoids.Compute the oversize decision first, then skip content validation for the metadata-only record.
⚡ Proposed fix
- let media_kind = validated_media_kind(media_kind, Some(content)); - let (content, memory_permit) = if content.len() > self.config.artifact_command_max_bytes { + let oversized = content.len() > self.config.artifact_command_max_bytes; + // A metadata-only record stores no bytes, so content validation would + // parse a large body only to discard it. + let media_kind = validated_media_kind(media_kind, (!oversized).then_some(content)); + let (content, memory_permit) = if oversized { ( ArtifactCaptureContent::Unavailable( ArtifactUnavailableReason::CaptureContentLimitExceeded, ), None, )🤖 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/service/artifact_persistence.rs` around lines 116 - 151, Update enqueue_openai_artifact_body to determine whether content exceeds artifact_command_max_bytes before calling validated_media_kind. For oversized content, skip JSON/content validation and produce the existing metadata-only record; retain validated_media_kind for content within the size limit and preserve the current capture and memory-budget behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e0ecb96-a5ea-41de-9aac-261c56f56e62
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (191)
crates/mesh-llm-commands/Cargo.tomlcrates/mesh-llm-commands/src/auth.rscrates/mesh-llm-commands/src/auth/tests.rscrates/mesh-llm-commands/src/lib.rscrates/mesh-llm-commands/src/operational_logging.rscrates/mesh-llm-events/src/logging/envelope.rscrates/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/Cargo.tomlcrates/mesh-llm-host-runtime/src/api/access.rscrates/mesh-llm-host-runtime/src/api/http.rscrates/mesh-llm-host-runtime/src/api/management_lifecycle.rscrates/mesh-llm-host-runtime/src/api/mod.rscrates/mesh-llm-host-runtime/src/api/routes/diagnostics.rscrates/mesh-llm-host-runtime/src/api/routes/logs/cleanup.rscrates/mesh-llm-host-runtime/src/api/routes/logs/delete.rscrates/mesh-llm-host-runtime/src/api/routes/logs/dto.rscrates/mesh-llm-host-runtime/src/api/routes/logs/error.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/mod.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/protocol.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/query.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/session.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/session/tests.rscrates/mesh-llm-host-runtime/src/api/routes/logs/events/stream.rscrates/mesh-llm-host-runtime/src/api/routes/logs/export.rscrates/mesh-llm-host-runtime/src/api/routes/logs/maintenance_control.rscrates/mesh-llm-host-runtime/src/api/routes/logs/mod.rscrates/mesh-llm-host-runtime/src/api/routes/logs/parse.rscrates/mesh-llm-host-runtime/src/api/routes/logs/tests.rscrates/mesh-llm-host-runtime/src/api/routes/logs/webhook_retry.rscrates/mesh-llm-host-runtime/src/api/routes/mcp.rscrates/mesh-llm-host-runtime/src/api/routes/mod.rscrates/mesh-llm-host-runtime/src/api/routes/plugins.rscrates/mesh-llm-host-runtime/src/api/routes/runtime.rscrates/mesh-llm-host-runtime/src/api/server.rscrates/mesh-llm-host-runtime/src/api/status.rscrates/mesh-llm-host-runtime/src/api/tests/logs_api_routes.rscrates/mesh-llm-host-runtime/src/api/tests/logs_api_routes/access_and_mutation.rscrates/mesh-llm-host-runtime/src/api/tests/logs_api_routes/event_stream.rscrates/mesh-llm-host-runtime/src/api/tests/logs_api_routes/read_and_export.rscrates/mesh-llm-host-runtime/src/api/tests/logs_api_routes/read_and_export/export.rscrates/mesh-llm-host-runtime/src/api/tests/management_http.rscrates/mesh-llm-host-runtime/src/api/tests/mod.rscrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/logging/bus.rscrates/mesh-llm-host-runtime/src/logging/cleanup.rscrates/mesh-llm-host-runtime/src/logging/cleanup/tests.rscrates/mesh-llm-host-runtime/src/logging/lifecycle.rscrates/mesh-llm-host-runtime/src/logging/management_lifecycle.rscrates/mesh-llm-host-runtime/src/logging/metrics.rscrates/mesh-llm-host-runtime/src/logging/mod.rscrates/mesh-llm-host-runtime/src/logging/openai_lifecycle.rscrates/mesh-llm-host-runtime/src/logging/operator_audit.rscrates/mesh-llm-host-runtime/src/logging/output_projection.rscrates/mesh-llm-host-runtime/src/logging/persistence.rscrates/mesh-llm-host-runtime/src/logging/policy.rscrates/mesh-llm-host-runtime/src/logging/policy/artifact_redaction.rscrates/mesh-llm-host-runtime/src/logging/raw_mesh_lifecycle.rscrates/mesh-llm-host-runtime/src/logging/raw_mesh_lifecycle/tests.rscrates/mesh-llm-host-runtime/src/logging/registry.rscrates/mesh-llm-host-runtime/src/logging/request_metadata.rscrates/mesh-llm-host-runtime/src/logging/runtime_state.rscrates/mesh-llm-host-runtime/src/logging/runtime_state/query_facade.rscrates/mesh-llm-host-runtime/src/logging/runtime_state/tests.rscrates/mesh-llm-host-runtime/src/logging/runtime_state/workers.rscrates/mesh-llm-host-runtime/src/logging/service.rscrates/mesh-llm-host-runtime/src/logging/service/artifact_persistence.rscrates/mesh-llm-host-runtime/src/logging/service/operational_audit.rscrates/mesh-llm-host-runtime/src/logging/service_tests.rscrates/mesh-llm-host-runtime/src/logging/service_tests/configuration.rscrates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rscrates/mesh-llm-host-runtime/src/logging/service_tests/lifecycle_registry.rscrates/mesh-llm-host-runtime/src/logging/webhook_delivery.rscrates/mesh-llm-host-runtime/src/logging/webhook_delivery/tests.rscrates/mesh-llm-host-runtime/src/logging/webhook_scheduler.rscrates/mesh-llm-host-runtime/src/mesh/connections.rscrates/mesh-llm-host-runtime/src/mesh/gossip.rscrates/mesh-llm-host-runtime/src/mesh/mod.rscrates/mesh-llm-host-runtime/src/mesh/operational_logging.rscrates/mesh-llm-host-runtime/src/network/openai/ingress.rscrates/mesh-llm-host-runtime/src/network/openai/ingress_tests/durable_artifacts.rscrates/mesh-llm-host-runtime/src/network/openai/moa_gateway/mod.rscrates/mesh-llm-host-runtime/src/network/openai/moa_gateway/progress.rscrates/mesh-llm-host-runtime/src/network/openai/moa_gateway/streaming.rscrates/mesh-llm-host-runtime/src/network/openai/mod.rscrates/mesh-llm-host-runtime/src/network/openai/parse_failure.rscrates/mesh-llm-host-runtime/src/network/openai/request_parse.rscrates/mesh-llm-host-runtime/src/network/openai/response.rscrates/mesh-llm-host-runtime/src/network/openai/response/common.rscrates/mesh-llm-host-runtime/src/network/openai/response/dispatch.rscrates/mesh-llm-host-runtime/src/network/openai/response/external_endpoint.rscrates/mesh-llm-host-runtime/src/network/openai/response/json_adaptation.rscrates/mesh-llm-host-runtime/src/network/openai/response/pipeline.rscrates/mesh-llm-host-runtime/src/network/openai/response/relay.rscrates/mesh-llm-host-runtime/src/network/openai/response/routing.rscrates/mesh-llm-host-runtime/src/network/openai/response/send.rscrates/mesh-llm-host-runtime/src/network/openai/response/stream_translation.rscrates/mesh-llm-host-runtime/src/network/openai/transport.rscrates/mesh-llm-host-runtime/src/network/openai/transport_tests.rscrates/mesh-llm-host-runtime/src/network/openai/transport_tests/durable_artifacts.rscrates/mesh-llm-host-runtime/src/network/openai/transport_tests/lifecycle.rscrates/mesh-llm-host-runtime/src/network/openai/transport_tests/routing.rscrates/mesh-llm-host-runtime/src/network/tunnel.rscrates/mesh-llm-host-runtime/src/runtime/auto_join.rscrates/mesh-llm-host-runtime/src/runtime/config_state.rscrates/mesh-llm-host-runtime/src/runtime/control_loop.rscrates/mesh-llm-host-runtime/src/runtime/discovery.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local/native_runtime_events.rscrates/mesh-llm-host-runtime/src/runtime/local/native_runtime_events/tests.rscrates/mesh-llm-host-runtime/src/runtime/mod.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle/load.rscrates/mesh-llm-host-runtime/src/runtime/model_lifecycle/unload.rscrates/mesh-llm-host-runtime/src/runtime/operational_logging.rscrates/mesh-llm-host-runtime/src/runtime/operational_logging/tests.rscrates/mesh-llm-host-runtime/src/runtime/run_auto.rscrates/mesh-llm-host-runtime/src/runtime/startup_handles.rscrates/mesh-llm-host-runtime/src/runtime/survey.rscrates/mesh-llm-host-runtime/src/runtime/survey/logging_metrics.rscrates/mesh-llm-host-runtime/src/runtime/tests/logging.rscrates/mesh-llm-host-runtime/src/runtime/tests/mod.rscrates/mesh-llm-host-runtime/src/runtime_data/api_views.rscrates/mesh-llm-host-runtime/src/runtime_data/mod.rscrates/mesh-llm-host-runtime/src/sdk/embedded_logging.rscrates/mesh-llm-log-store/src/api_acceptance_tests/cursor_pagination.rscrates/mesh-llm-log-store/src/api_acceptance_tests/mod.rscrates/mesh-llm-log-store/src/api_acceptance_tests/retention_cleanup.rscrates/mesh-llm-log-store/src/api_acceptance_tests/retention_policy.rscrates/mesh-llm-log-store/src/api_acceptance_tests/schema_lifecycle.rscrates/mesh-llm-log-store/src/api_acceptance_tests/summary_audit.rscrates/mesh-llm-log-store/src/api_acceptance_tests/summary_events.rscrates/mesh-llm-log-store/src/api_acceptance_tests/webhook.rscrates/mesh-llm-log-store/src/artifact_repository.rscrates/mesh-llm-log-store/src/artifact_unavailable_tests.rscrates/mesh-llm-log-store/src/artifacts.rscrates/mesh-llm-log-store/src/artifacts_tests.rscrates/mesh-llm-log-store/src/capture.rscrates/mesh-llm-log-store/src/cursor.rscrates/mesh-llm-log-store/src/error.rscrates/mesh-llm-log-store/src/lib.rscrates/mesh-llm-log-store/src/maintenance.rscrates/mesh-llm-log-store/src/maintenance/execution.rscrates/mesh-llm-log-store/src/maintenance/metadata_delete.rscrates/mesh-llm-log-store/src/maintenance/scope_filters.rscrates/mesh-llm-log-store/src/maintenance/tests.rscrates/mesh-llm-log-store/src/maintenance/tests/cleanup.rscrates/mesh-llm-log-store/src/maintenance/tests/delete_one.rscrates/mesh-llm-log-store/src/migrations.rscrates/mesh-llm-log-store/src/query/mod.rscrates/mesh-llm-log-store/src/query/related.rscrates/mesh-llm-log-store/src/query/requests.rscrates/mesh-llm-log-store/src/query_pagination_tests.rscrates/mesh-llm-log-store/src/query_tests.rscrates/mesh-llm-log-store/src/repositories.rscrates/mesh-llm-log-store/src/repositories/cleanup.rscrates/mesh-llm-log-store/src/store.rscrates/mesh-llm-log-store/src/tests/cursor_pagination.rscrates/mesh-llm-log-store/src/tests/mod.rscrates/mesh-llm-log-store/src/tests/retention_cleanup.rscrates/mesh-llm-log-store/src/tests/store_setup.rscrates/mesh-llm-log-store/src/tests/summary_records.rscrates/mesh-llm-log-store/src/timestamps.rscrates/mesh-llm/Cargo.tomlcrates/mesh-llm/src/commands/mod.rscrates/mesh-llm/src/lib.rscrates/mesh-mixture-of-agents/src/lib.rscrates/openai-frontend/Cargo.tomlcrates/openai-frontend/README.mdcrates/openai-frontend/src/backend.rscrates/openai-frontend/src/lib.rscrates/openai-frontend/src/lifecycle.rscrates/openai-frontend/src/responses.rscrates/openai-frontend/src/router.rscrates/openai-frontend/src/router/stream_lifecycle.rscrates/openai-frontend/src/router_tests.rscrates/openai-frontend/tests/benchy_contract.rscrates/openai-frontend/tests/lifecycle_observer.rscrates/skippy-server/src/embedded.rscrates/skippy-server/src/lib.rsdocs/plugins/telemetry.mdscripts/qa-control-plane-mixed-version.shscripts/qa-logging-recovery.shscripts/tests/test_logging_api_docs.pyscripts/tests/test_logging_module_boundaries.pyscripts/tests/test_qa_logging_recovery.pywebsite/src/_data/docs.jswebsite/src/docs/pages/api-reference.mdwebsite/src/docs/pages/logging-api.md
e99ba12 to
6140c27
Compare
…#1291) The ingress rewrite in #1175 collapsed MoaInterceptResult::Degraded into NotMoa while wiring route observers through the MoA intercept. The gateway still rewrote model=mesh to the real served model, but routing kept using the stale decision.effective_model ("mesh") and 404'd every single-node model=mesh request. Released 0.75.0 is unaffected. Restore the Degraded { stream, model } variant, route with the rewritten model, and pin the contract with a regression test that fails loudly if Degraded is ever folded into NotMoa again. Repro: mesh-llm serve with one model, POST /v1/chat/completions with model="mesh" -> 404 before, 200 after.
1 participant
Summary
Second PR in the logging stack, based on #1174. This layer connects the logging foundation to runtime boundaries and exposes audited, trusted-local operator APIs.
Request and runtime instrumentation
Trusted-local logs API
Maintenance and delivery
Reliability and compatibility
Validation
Stack
Guardrails
The management endpoints and SSE stream are trusted-local operator surfaces, not mesh protocols. This PR does not change protobuf/gossip schemas, ALPN labels, the native ABI, or add an OTLP log-record API. It intentionally contains no console implementation; the typed UI is isolated in #1176.
Summary by CodeRabbit