Truncate response errors vs swallowing if too large#433
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds UTF-8 boundary-aware truncation for exception response messages when marshaled JSON exceeds maxResponseLength. Introduces a truncationMarker constant and truncateExceptionResponse which probes JSON overhead with a marker, computes the remaining byte budget for the exception message, truncates the message at UTF-8 boundaries preserving head and tail around the marker while re-marshaling to account for escaping, and returns a modified exceptionResponse only if it fits. marshalResponse now attempts truncation for exception responses returned by response.Shorten() before falling back to the existing oversize error. Ginkgo tests cover behavior, UTF-8 correctness, escaping, fallback, and UnlimitedResponseLength. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Alphasite
previously approved these changes
May 21, 2026
selzoc
force-pushed
the
truncate-perform-errors
branch
from
a590649 to
feaf4eb
Compare
Alphasite
previously approved these changes
May 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@handler/perform_handler_with_json_test.go`:
- Around line 93-113: Add a test to verify behavior when error messages contain
many JSON-escapable characters (quotes/backslashes) by calling
PerformHandlerWithJSON with a NewExceptionResponse whose error string is
strings.Repeat(`"`, maxLen) and assert the returned respBytes length is <=
maxLen; if truncation can't account for escaping, ensure the handler falls back
to a generic error payload instead of producing invalid/oversized JSON. Locate
the test near the existing UTF-8 truncation spec (the It block using
PerformHandlerWithJSON and NewExceptionResponse) and mirror its structure,
asserting either successful truncated JSON or the generic fallback response
while keeping utf8/json validity checks where appropriate.
In `@handler/perform_handler_with_json.go`:
- Around line 79-93: The truncation logic for excResp.Exception.Message assumes
byte-for-byte mapping to JSON, but json.Marshal can expand escapable characters;
fix by iteratively trimming and re-marshaling until the marshaled respJSON
length <= maxResponseLength: start with contentBytes (currently derived from
maxMsgBytes), slice msg to that length ensuring utf8.ValidString, set
excResp.Exception.Message = msg+truncatedSuffix, json.Marshal(excResp) and if
len(respJSON) > maxResponseLength reduce contentBytes (e.g., halve or decrement)
and repeat; ensure the loop breaks when contentBytes <= 0 and falls back to the
generic error path as before.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fe17c39-891c-4781-bce0-6fcae68aed94
📒 Files selected for processing (2)
handler/perform_handler_with_json.gohandler/perform_handler_with_json_test.go
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
selzoc
force-pushed
the
truncate-perform-errors
branch
from
feaf4eb to
9f80ff9
Compare
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
Member
Author
|
Marking as draft until I can test this on an actual vm |
selzoc
force-pushed
the
truncate-perform-errors
branch
from
9f80ff9 to
7976a1e
Compare
Member
Author
|
I tested this with an actual vm, and the before looks like: When I replace the agent with the changes from this pr, we instead get: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
handler/perform_handler_with_json.go (1)
152-168: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid recomputing
response.Shorten()and re-marshaling the truncated candidate.
response.Shorten()is invoked at Line 152 and again at Line 160, and on the truncation path the candidate is marshaled insidetruncateExceptionResponseand then marshaled a second time at Line 162. On this large-response pathShorten()operates on the already-oversized payload, so the duplicate call is the more meaningful waste. Capture the shortened response once and reuse it.♻️ Capture Shorten() result once
if len(respJSON) > maxResponseLength { - respJSON, err = json.Marshal(response.Shorten()) + shortened := response.Shorten() + respJSON, err = json.Marshal(shortened) if err != nil { logger.Error(mbusHandlerLogTag, "Failed to marshal response: %s", err.Error()) return respJSON, bosherr.WrapError(err, "Marshalling JSON response") } - } - if len(respJSON) > maxResponseLength { - if excResp, ok := response.Shorten().(exceptionResponse); ok { - if truncated, ok := truncateExceptionResponse(excResp, maxResponseLength); ok { - respJSON, err = json.Marshal(truncated) - if err == nil && len(respJSON) <= maxResponseLength { - logger.Warn(mbusHandlerLogTag, "Response too large, exception message truncated to fit within %d bytes", maxResponseLength) - return respJSON, nil - } - } - } + if len(respJSON) > maxResponseLength { + if excResp, ok := shortened.(exceptionResponse); ok { + if truncated, ok := truncateExceptionResponse(excResp, maxResponseLength); ok { + respJSON, err = json.Marshal(truncated) + if err == nil && len(respJSON) <= maxResponseLength { + logger.Warn(mbusHandlerLogTag, "Response too large, exception message truncated to fit within %d bytes", maxResponseLength) + return respJSON, nil + } + } + } - respJSON, err = BuildErrorWithJSON(responseMaxLengthErrMsg, logger) - if err != nil { - logger.Error(mbusHandlerLogTag, "Failed to build 'max length exceeded' response: %s", err.Error()) - return respJSON, bosherr.WrapError(err, "Building error") + respJSON, err = BuildErrorWithJSON(responseMaxLengthErrMsg, logger) + if err != nil { + logger.Error(mbusHandlerLogTag, "Failed to build 'max length exceeded' response: %s", err.Error()) + return respJSON, bosherr.WrapError(err, "Building error") + } } }🤖 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 `@handler/perform_handler_with_json.go` around lines 152 - 168, The code calls response.Shorten() multiple times and re-marshals the same data, wasting CPU and risking inconsistent results; change the flow to call response.Shorten() once (store it in a local variable, e.g. shortened := response.Shorten()), use shortened for the initial json.Marshal and for the large-response branch, pass shortened into truncateExceptionResponse and only marshal the final truncated value once before returning; update references in the large-response logic (exceptionResponse type assertion, truncateExceptionResponse, and the subsequent json.Marshal and logger.Warn) to use the stored shortened variable and avoid recomputing Shorten().
🤖 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.
Outside diff comments:
In `@handler/perform_handler_with_json.go`:
- Around line 152-168: The code calls response.Shorten() multiple times and
re-marshals the same data, wasting CPU and risking inconsistent results; change
the flow to call response.Shorten() once (store it in a local variable, e.g.
shortened := response.Shorten()), use shortened for the initial json.Marshal and
for the large-response branch, pass shortened into truncateExceptionResponse and
only marshal the final truncated value once before returning; update references
in the large-response logic (exceptionResponse type assertion,
truncateExceptionResponse, and the subsequent json.Marshal and logger.Warn) to
use the stored shortened variable and avoid recomputing Shorten().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8404c4e-8ddd-45ec-b5ae-c1dcb0ae3504
📒 Files selected for processing (2)
handler/perform_handler_with_json.gohandler/perform_handler_with_json_test.go
selzoc
force-pushed
the
truncate-perform-errors
branch
from
7976a1e to
b59777e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@handler/perform_handler_with_json.go`:
- Around line 82-86: The early-return in truncateExceptionResponse that returns
excResp when len(msg) <= maxMsgBytes can wrongly bypass truncation because JSON
escaping may still push the marshaled size over the limit; modify
truncateExceptionResponse to verify the actual marshaled size (e.g.,
json.Marshal of excResp or its message field) fits before returning and only
short-circuit when the marshaled bytes <= maxMsgBytes, otherwise fall through to
the truncation loop; preserve the panic-safety guard by clamping contentBytes to
min(len(msg), contentBytes) before entering the loop so we never attempt
negative slicing of excResp.Exception.Message.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0db4a1cb-638c-477b-9e7e-94279b4eb332
📒 Files selected for processing (2)
handler/perform_handler_with_json.gohandler/perform_handler_with_json_test.go
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
aramprice
previously approved these changes
May 28, 2026
aramprice
requested review from
a team,
benjaminguttmann-avtq and
lnguyen
and removed request for
a team
Contributor
There was a problem hiding this comment.
Pull request overview
Surfaces oversized exception responses to BOSH task logs by truncating the exception message (preserving head and tail around a ...[truncated]... marker) to fit within the NATS 1 MiB response cap, instead of replacing the entire response with the opaque "Response exceeded maximum allowed length" message.
Changes:
- Adds
truncateExceptionResponsehelper that splits the message into a head and tail around a truncation marker, with an iterative re-marshal loop to account for JSON escape expansion and UTF-8 rune boundaries. - Wires the new truncation step into
marshalResponseafterShorten()forexceptionResponseonly; falls back to the existing generic error if truncation cannot fit. - Adds Ginkgo tests covering normal responses, head/tail preservation, size cap adherence, JSON-escapable input, multi-byte UTF-8 input, unlimited mode, and the small-limit fallback path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| handler/perform_handler_with_json.go | Adds truncateExceptionResponse and integrates it into marshalResponse between Shorten() and the generic max-length fallback. |
| handler/perform_handler_with_json_test.go | New test file exercising truncation behavior, UTF-8 safety, escape expansion, fallback, and unlimited-length pass-through. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // next iteration converges toward the target size. | ||
| newContentBytes := contentBytes * maxLength / len(candidateJSON) | ||
| if newContentBytes >= contentBytes { | ||
| contentBytes-- // guard against floating-point rounding stalls |
We found a case where the prepare step was running a vm out of disk, and there were many tar errors. The large amount of errors resulted in the agent returning "Response exceeded maximum allowed length" with no details, leading us to have to get onto the failing vm and examine the agent logs for more details. This commit will attempt to truncate the error message to fit inside the max message length such that we can surface the error to the bosh task logs. ai-assisted=yes [TNZ-88995]
selzoc
force-pushed
the
truncate-perform-errors
branch
from
b59777e to
5cc4264
Compare
aramprice
approved these changes
May 29, 2026
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We found a case where the prepare step was running a vm out of disk, and there were many tar errors. The large amount of errors resulted in the agent returning "Response exceeded maximum allowed length" with no details, leading us to have to get onto the failing vm and examine the agent logs for more details.
This commit will attempt to truncate the error message to fit inside the max message length such that we can surface the error to the bosh task logs.