fix: cancelled state in logs - #4831
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe logging plugin adds shared status constants and error-classification helpers, replaces hard-coded status literals across hook handling and streaming output processing, and updates tests for cancelled, timeout, success, and error status behavior. ChangesLogging status refactor
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/logging/main.go (1)
1152-1154: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winOverwrites correctly-derived status due to unpopulated
entry.ErrorDetailsParsed(see root cause inoperations.go).This unconditional re-derivation only produces a correct result when the preceding branch explicitly set
entry.ErrorDetailsParsed(true for thebifrostErr != nilsub-branch above, where it's idempotent). For theisFinalChunksub-branch,applyStreamingOutputToEntry(inplugins/logging/operations.go) never setsentry.ErrorDetailsParsed, so this line always evaluateslogStatusForError(nil)→"error", silently overwriting the cancelled status that was just computed insideapplyStreamingOutputToEntry. See the companion comment onoperations.gofor the proposed fix (propagateentry.ErrorDetailsParsed = streamResponse.Data.ErrorDetails).🤖 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 `@plugins/logging/main.go` around lines 1152 - 1154, The status is being recomputed unconditionally from entry.ErrorDetailsParsed, which overwrites the correctly derived cancelled state in the streaming path. Update the logic around logStatusForError in main.go so it only re-derives status when ErrorDetailsParsed is actually populated, or propagate the parsed error details from applyStreamingOutputToEntry in operations.go before this check runs. Use entry.ErrorDetailsParsed, logStatusForError, and applyStreamingOutputToEntry to locate the fix.
🤖 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 `@plugins/logging/operations.go`:
- Around line 387-397: The streaming error handling in
applyStreamingOutputToEntry is only populating entry.Status and the marshalled
entry.ErrorDetails, but not entry.ErrorDetailsParsed, which later causes main.go
to reclassify the status incorrectly. Update applyStreamingOutputToEntry to
assign ErrorDetailsParsed alongside ErrorDetails when
streamResponse.Data.ErrorDetails is present, matching the existing bifrostErr
handling in PostLLMHook and the other streaming call sites. Ensure the
downstream status re-derivation in plugins/logging/main.go sees a non-nil parsed
error so cancelled/timeout statuses are preserved.
---
Duplicate comments:
In `@plugins/logging/main.go`:
- Around line 1152-1154: The status is being recomputed unconditionally from
entry.ErrorDetailsParsed, which overwrites the correctly derived cancelled state
in the streaming path. Update the logic around logStatusForError in main.go so
it only re-derives status when ErrorDetailsParsed is actually populated, or
propagate the parsed error details from applyStreamingOutputToEntry in
operations.go before this check runs. Use entry.ErrorDetailsParsed,
logStatusForError, and applyStreamingOutputToEntry to locate the fix.
🪄 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 Plus
Run ID: 39434ca4-15aa-48f7-b035-75e28e89ca1a
📒 Files selected for processing (3)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.go
d1e6312 to
bb364c1
Compare
bb364c1 to
ee2940f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/logging/operations_test.go (2)
300-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
schemas.Ptr()over&localVarfor these new status-code pointers.New tests introduce
statusCode := 504/499locals purely to take their address (e.g.,StatusCode: &statusCode). Based on learnings, the repo convention prefersbifrost.Ptr()/schemas.Ptr()over the address operator, even in test code — this file already usesschemas.Ptr(...)for theTypefield a few lines below each occurrence.♻️ Example diff for one occurrence
- statusCode := 504 bifrostErr := &schemas.BifrostError{ IsBifrostError: true, - StatusCode: &statusCode, + StatusCode: schemas.Ptr(504),Also applies to: 353-356, 385-388, 403-408
🤖 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 `@plugins/logging/operations_test.go` around lines 300 - 303, The new status-code pointers in the test cases are using local variables with address-of syntax, which is inconsistent with the repo convention. Update the BifrostError constructions in operations_test.go to use schemas.Ptr for StatusCode, matching how Type is already set in the same test helpers, and remove the temporary statusCode locals from each affected test block.Source: Learnings
400-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond assertion re-implements production logic instead of exercising it.
Lines 429-434 manually duplicate the "Path B" re-derivation condition that
PostLLMHookuses downstream (per the AI summary/comment on lines 429-431), rather than invoking the actual production code path. If that downstream reclassification logic inmain.gochanges (e.g., different condition, additional side effect), this test will keep passing even though the real behavior regressed, since it only re-runslogStatusForErrorinline against a hand-copied condition.Consider either:
- Extracting the "Path B" reclassification condition into a small helper function in
operations.gothat bothPostLLMHookand this test call, or- Restructuring the test to exercise
PostLLMHookend-to-end (similar to the other new tests in this file) so the actual downstream code path is verified.🤖 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 `@plugins/logging/operations_test.go` around lines 400 - 439, The test in TestApplyStreamingOutputToEntryPreservesAccumulatorCancelledStatus duplicates the downstream “Path B” reclassification logic instead of verifying the real production path. Refactor the re-derivation condition used by PostLLMHook into a shared helper in LoggerPlugin/operations.go and call that helper from both PostLLMHook and this test, or switch the test to exercise PostLLMHook end-to-end so the actual cancellation reclassification behavior is covered.
🤖 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.
Nitpick comments:
In `@plugins/logging/operations_test.go`:
- Around line 300-303: The new status-code pointers in the test cases are using
local variables with address-of syntax, which is inconsistent with the repo
convention. Update the BifrostError constructions in operations_test.go to use
schemas.Ptr for StatusCode, matching how Type is already set in the same test
helpers, and remove the temporary statusCode locals from each affected test
block.
- Around line 400-439: The test in
TestApplyStreamingOutputToEntryPreservesAccumulatorCancelledStatus duplicates
the downstream “Path B” reclassification logic instead of verifying the real
production path. Refactor the re-derivation condition used by PostLLMHook into a
shared helper in LoggerPlugin/operations.go and call that helper from both
PostLLMHook and this test, or switch the test to exercise PostLLMHook end-to-end
so the actual cancellation reclassification behavior is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62aa0234-7eee-4440-adda-ec49a6591c19
📒 Files selected for processing (3)
plugins/logging/main.goplugins/logging/operations.goplugins/logging/operations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/logging/operations.go
- plugins/logging/main.go
ee2940f to
f9dae1e
Compare
1d6c19c to
ad975da
Compare
Merge activity
|
The base branch was changed.
f9dae1e to
8c9ad5c
Compare
* upstream/dev: feat(mcp): add per-MCP-server tool execution timeout (maximhq#4472) fix: billing on failed responses stream requests anthropic and bedrock (maximhq#4842) fix: gemini openai through signature compatibility (maximhq#4810) fix: cancelled state in logs (maximhq#4831) fix: perplexity responses api compatibility (maximhq#4813) docs: clarify two-layer token refresh behavior and disabled-client refresh token expiry (maximhq#4849) fix: skip background token refresh for disabled/unconfigured MCP clients and guarantee non-nil logger in sync workers (maximhq#4848)
## Summary Requests cancelled by the client (e.g. context deadline exceeded, HTTP 499) were being logged with `status="error"`, making it impossible to distinguish genuine provider/network failures from client-initiated cancellations. This PR introduces a dedicated `"cancelled"` log status for those cases. ## Changes - Introduced log status constants (`logStatusProcessing`, `logStatusSuccess`, `logStatusError`, `logStatusCancelled`) to replace raw string literals throughout `main.go` and `operations.go`. - Added `logStatusForError(*schemas.BifrostError) string` which returns `"cancelled"` when the error represents a client cancellation (HTTP 499, `RequestCancelled` error type, or a `RequestTimedOut` whose message indicates the timeout was driven by a context deadline), and `"error"` otherwise. - Provider-side timeouts (where the message matches `ErrProviderRequestTimedOut`) continue to produce `status="error"` — only context-driven timeouts are treated as cancellations. - Updated the existing cancelled-stream test assertion from `"error"` to `"cancelled"`. - Added two new tests: `TestPostLLMHookContextTimeoutLogsCancelledStatus` (context deadline → `"cancelled"`) and `TestPostLLMHookProviderTimeoutRemainsErrorStatus` (provider timeout → `"error"`). ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/logging/... ``` Expected: all tests pass, including the two new timeout-status tests and the updated cancelled-stream test. ## Breaking changes - [x] Yes - [ ] No Any downstream consumers filtering log rows by `status="error"` to catch cancelled requests will need to also handle `status="cancelled"`. Existing `"error"` rows for true provider/network failures are unaffected. ## Related issues Closes maximhq#3357 ## Security considerations None — this change only affects how log status strings are assigned; no auth, secrets, or PII handling is modified. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Requests cancelled by the client (e.g. context deadline exceeded, HTTP 499) were being logged with `status="error"`, making it impossible to distinguish genuine provider/network failures from client-initiated cancellations. This PR introduces a dedicated `"cancelled"` log status for those cases. ## Changes - Introduced log status constants (`logStatusProcessing`, `logStatusSuccess`, `logStatusError`, `logStatusCancelled`) to replace raw string literals throughout `main.go` and `operations.go`. - Added `logStatusForError(*schemas.BifrostError) string` which returns `"cancelled"` when the error represents a client cancellation (HTTP 499, `RequestCancelled` error type, or a `RequestTimedOut` whose message indicates the timeout was driven by a context deadline), and `"error"` otherwise. - Provider-side timeouts (where the message matches `ErrProviderRequestTimedOut`) continue to produce `status="error"` — only context-driven timeouts are treated as cancellations. - Updated the existing cancelled-stream test assertion from `"error"` to `"cancelled"`. - Added two new tests: `TestPostLLMHookContextTimeoutLogsCancelledStatus` (context deadline → `"cancelled"`) and `TestPostLLMHookProviderTimeoutRemainsErrorStatus` (provider timeout → `"error"`). ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/logging/... ``` Expected: all tests pass, including the two new timeout-status tests and the updated cancelled-stream test. ## Breaking changes - [x] Yes - [ ] No Any downstream consumers filtering log rows by `status="error"` to catch cancelled requests will need to also handle `status="cancelled"`. Existing `"error"` rows for true provider/network failures are unaffected. ## Related issues Closes maximhq#3357 ## Security considerations None — this change only affects how log status strings are assigned; no auth, secrets, or PII handling is modified. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Requests cancelled by the client (e.g. context deadline exceeded, HTTP 499) were being logged with
status="error", making it impossible to distinguish genuine provider/network failures from client-initiated cancellations. This PR introduces a dedicated"cancelled"log status for those cases.Changes
logStatusProcessing,logStatusSuccess,logStatusError,logStatusCancelled) to replace raw string literals throughoutmain.goandoperations.go.logStatusForError(*schemas.BifrostError) stringwhich returns"cancelled"when the error represents a client cancellation (HTTP 499,RequestCancellederror type, or aRequestTimedOutwhose message indicates the timeout was driven by a context deadline), and"error"otherwise.ErrProviderRequestTimedOut) continue to producestatus="error"— only context-driven timeouts are treated as cancellations."error"to"cancelled".TestPostLLMHookContextTimeoutLogsCancelledStatus(context deadline →"cancelled") andTestPostLLMHookProviderTimeoutRemainsErrorStatus(provider timeout →"error").Type of change
Affected areas
How to test
go test ./plugins/logging/...Expected: all tests pass, including the two new timeout-status tests and the updated cancelled-stream test.
Breaking changes
Any downstream consumers filtering log rows by
status="error"to catch cancelled requests will need to also handlestatus="cancelled". Existing"error"rows for true provider/network failures are unaffected.Related issues
Closes #3357
Security considerations
None — this change only affects how log status strings are assigned; no auth, secrets, or PII handling is modified.
Checklist
docs/contributing/README.mdand followed the guidelines