Truncate response errors vs swallowing if too large#433
Open
selzoc wants to merge 1 commit into
Open
Conversation
WalkthroughAdds UTF-8 boundary-aware truncation for exception response messages when JSON serialization exceeds the configured maxResponseLength. Introduces a truncated suffix constant and truncateExceptionResponse that measures JSON overhead with a probe, computes remaining bytes for the exception message, truncates at UTF-8 boundaries, appends the suffix, and returns a modified exceptionResponse only if it fits. marshalResponse now attempts this truncation after response.Shorten() fails. Ginkgo tests cover in-limit responses, truncation behavior, UTF-8 correctness, fallback to a generic message, and UnlimitedResponseLength behavior. 🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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]
Alphasite
previously approved these changes
May 21, 2026
selzoc
force-pushed
the
truncate-perform-errors
branch
from
a590649 to
feaf4eb
Compare
Alphasite
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
Comment on lines
+93
to
+113
| It("preserves valid UTF-8 when truncating a multi-byte message", func() { | ||
| const maxLen = 100 | ||
| // Build a message entirely of 3-byte UTF-8 runes (e.g. '€' = 0xE2 0x82 0xAC). | ||
| multiByteMsg := strings.Repeat("€", maxLen) | ||
|
|
||
| respBytes, _, err := PerformHandlerWithJSON( | ||
| []byte(`{"method":"big","arguments":[],"reply_to":"fake-reply"}`), | ||
| func(req Request) Response { | ||
| return NewExceptionResponse(errors.New(multiByteMsg)) | ||
| }, | ||
| maxLen, | ||
| logger, | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(len(respBytes)).To(BeNumerically("<=", maxLen)) | ||
|
|
||
| var parsed map[string]map[string]string | ||
| Expect(json.Unmarshal(respBytes, &parsed)).To(Succeed()) | ||
| msg := parsed["exception"]["message"] | ||
| Expect(utf8.ValidString(msg)).To(BeTrue(), "result should be valid UTF-8") | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding a test for messages with JSON-escapable characters.
The truncation logic doesn't account for JSON string escaping. A test with a message containing many " or \ characters would verify the fallback behavior when truncation produces JSON that still exceeds the limit.
📝 Suggested test case
It("falls back to generic error when message contains many escapable characters", func() {
const maxLen = 100
// Each quote becomes \" in JSON (2 bytes), so truncation estimate is off
escapableMsg := strings.Repeat(`"`, maxLen)
respBytes, _, err := PerformHandlerWithJSON(
[]byte(`{"method":"big","arguments":[],"reply_to":"fake-reply"}`),
func(req Request) Response {
return NewExceptionResponse(errors.New(escapableMsg))
},
maxLen,
logger,
)
Expect(err).NotTo(HaveOccurred())
// Should either truncate successfully or fall back to generic error
Expect(len(respBytes)).To(BeNumerically("<=", maxLen))
})🤖 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_test.go` around lines 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.
Comment on lines
+79
to
+93
| msg := excResp.Exception.Message | ||
| contentBytes := maxMsgBytes - len(truncatedSuffix) | ||
|
|
||
| if len(msg) <= maxMsgBytes { | ||
| // Already fits (shouldn't normally reach here, but be safe). | ||
| return excResp, true | ||
| } | ||
|
|
||
| // Truncate at a valid UTF-8 boundary. | ||
| msg = msg[:contentBytes] | ||
| for len(msg) > 0 && !utf8.ValidString(msg) { | ||
| msg = msg[:len(msg)-1] | ||
| } | ||
|
|
||
| excResp.Exception.Message = msg + truncatedSuffix |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
JSON string escaping can cause truncated response to exceed maxLength.
The truncation calculation assumes message bytes map 1:1 to JSON bytes. However, json.Marshal escapes characters like ", \, and control characters (e.g., " → \", newline → \n). If the original message contains such characters, the marshaled JSON will be larger than maxLength.
The caller's safety check at lines 120-121 (len(respJSON) <= maxResponseLength) will catch this and fall back to the generic error, so correctness is preserved. However, this means truncation will silently fail for messages with many escapable characters, defeating the purpose of surfacing error details.
Consider either:
- Document this limitation as acceptable (since fallback exists), or
- Use a conservative estimate (e.g.,
contentBytes / 2) for worst-case escaping, or - Iteratively marshal and trim until it fits.
🤖 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 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.
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
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
2 participants
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.