Clip a failed item's error message on a character boundary - #63
Merged
Conversation
strlcpy() counts bytes, so a message longer than the 1024 byte buffer whose cut fell inside a multi-byte character left a partial one behind. That partial character was then stored: encoding is validated where input arrives from a client, in pg_client_to_server(), and SPI never crosses that boundary, so the literal the worker assembles in C goes into queue.error_message as whatever bytes it happens to hold. The row is then invalid text in the server encoding. length(), substring() and any client that decodes strictly raise on it, whilst SELECT, left() and LIKE happen to survive, so whether an operator's query breaks depends on which functions it uses -- and the row that breaks it is by definition one that was already reporting a problem. pg_mbcliplen() clips to the last character that fits entirely. It works in the server encoding rather than assuming UTF-8, and it allocates nothing, which matters inside an error handler. The failure is still recorded either way, so this is not the reclaim loop that 004 rules out; TAP 007 therefore asserts that the stored message reads back character-wise, which is the property that breaks, and treats the attempt being charged as a guard rather than the point. Closes #62
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe worker uses 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/t/007_error_message_truncation.pl`:
- Line 43: Update the node initialization in the test around $node->init to pass
the locale C and UTF8 encoding options, ensuring the fixture consistently uses
UTF-8 for its byte-length assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a0f4d95-543a-4c79-bd79-48378654758a
📒 Files selected for processing (2)
src/worker.ctest/t/007_error_message_truncation.pl
Two review points, neither changing what the fix does. strnlen() bounded by the buffer replaces strlen(), and strlcpy() given the clipped length replaces memcpy(). Nothing longer than the buffer can be kept in any case, so bounding the read costs nothing and means a message that is somehow not NUL-terminated cannot make us over-read (CWE-126); trim_whitespace() already measures its input this way for exactly that reason. Reusing strlcpy() also leaves the copy in the hands of a function that cannot overrun the destination. The test now initialises its cluster with an explicit --encoding=UTF8 rather than inheriting the developer's locale. What is under test is encoding-agnostic, since pg_mbcliplen() reads the server encoding, but the fixture's arithmetic is not: it relies on U+4E16 occupying three bytes, and under a single-byte encoding the message would neither reach 1025 bytes nor straddle the boundary.
mason-sharp
approved these changes
Aug 12, 2026
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.
Summary
queue_item_note_error()clipped the message into its fixed 1024 byte buffer withstrlcpy(), which counts bytes, so a longer message whose cut fell inside a multi-byte character left a partial one behind. That partial character was then stored, because encoding is validated where input arrives from a client, inpg_client_to_server(), and SPI never crosses that boundary; a literal the worker assembles in C is stored as whatever bytes it holds.pg_mbcliplen()clips to the last character that fits entirely, works in the server encoding rather than assuming UTF-8, and allocates nothing, which matters inside an error handler.The consequence is a corrupt value rather than a stuck queue:
queue.error_messageis left invalid in the server encoding, solength(),substring()and any client decoding strictly raise on that row, whilstSELECT,left()andLIKEhappen to survive. Whether an operator's query breaks therefore depends on which functions it uses, and the row that breaks it is by definition one that was already reporting a problem.Worth reading #62's correction before this diff: I originally claimed the truncation made the recording
UPDATEunparseable and so put the item in a reclaim loop. Driving the real worker into it disproved that, and the issue now records both the correct mechanism and how my first probe misled me. The fix is the same either way; the test is not, which is the point of having checked.Test plan
test/t/007_error_message_truncation.pldrives the real worker into a 1025 byte message, being 1022 single-byte characters followed by one three-byte character, so the 1023rd byte of the buffer is a lead byte whose other two do not fit. The fault is a trigger on the chunk table, the item issparse_onlyso no embedding is ever requested, and the provider isollama, whose init needs neither a key nor a network round trip.length()raises rather than returning a number, andoctet_length()comes back as 1023 with the message no longer intact. Assertions 1 and 2, that the attempt is charged and the failure is recorded, pass either way and are marked in the test as guards rather than the point, precisely because they do not discriminate.providersfails in my environment only, because the running cluster still had a library preloaded from before the recent commit that made four provider GUCs superuser-only, so the unprivileged-session assertions do not fire. It is untouched by this change and passes in CI.Closes #62