Repository navigation
Store outbox errors that contain null characters or exceed the column in bytes - #54320
Conversation
… in bytes A receiver's error body with U+0000 (PostgreSQL) or more than 2048 UTF-8 bytes (Oracle, SQL Server with a UTF-8 collation) could not be stored in LAST_ERROR. That rolled back the whole drainer tick, so other rows were pushed again on every tick and the failing row was never dead-lettered. Closes keycloak#54319 Signed-off-by: Semih702 <semih702do@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation addresses the persistence constraints with regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
Fixes #54319 by making outbox error messages safe to persist across supported databases, preventing failed error writes from stalling SSF delivery. No schema changes are required.
Changes:
- Removes U+0000 and caps errors at 2048 UTF-8 bytes, preserving code point boundaries and the truncation marker.
- Adds persistence coverage for null characters and multibyte messages.
- Adds SSF regression tests verifying that problematic receiver responses do not cause duplicate pushes to healthy receivers.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/events/outbox/OutboxStoreTests.java |
Tests sanitized, byte-limited error persistence. |
ssf/tests/base/src/test/java/org/keycloak/tests/ssf/transmitter/SsfTransmitterPushFailureTests.java |
Tests healthy delivery alongside receivers returning problematic errors. |
model/jpa/src/main/java/org/keycloak/events/outbox/OutboxStore.java |
Sanitizes null characters and truncates errors by UTF-8 byte length. |
Review effort: Balanced
There was a problem hiding this comment.
Thanks for the fix. Stripping null characters and truncating by UTF-8 bytes without splitting a code point does what #54319 asks. A few things to address before merging:
-
Literal NUL bytes in the test sources. See the inline comment.
-
The new tests can't catch a regression in CI. See the earlier comment.
-
An existing test still checks characters against what is now a byte limit. See the inline comment.
-
SsfPushDeliveryHandler.truncateDetailis now out of step with the store. That file isn't in this PR, so the comment is here instead of inline.truncateDetailstill caps the detail at 1024 UTF-16 chars withsubstring:- Its Javadoc (line 178) says the VARCHAR(2048) column "comfortably absorbs the prefix + url + detail". Now that the limit is in bytes, that's no longer true: 1024 three-byte characters are about 3 KB, so
OutboxStore.truncateErroralways ends up doing the real truncation for non-ASCII bodies. substring(0, 1024)can split a surrogate pair and leave a lone high surrogate, which is stored as?or U+FFFD.
At minimum the Javadoc should be updated. Ideally
truncateDetailwould also avoid splitting a surrogate pair (for example, back off one char whenCharacter.isHighSurrogate(detail.charAt(1023))). - Its Javadoc (line 178) says the VARCHAR(2048) column "comfortably absorbs the prefix + url + detail". Now that the limit is in bytes, that's no longer true: 1024 three-byte characters are about 3 KB, so
…rom splitting surrogates Closes keycloak#54319 Signed-off-by: Semih702 <semih702do@gmail.com>
The cap is in UTF-8 bytes since the previous commits. Closes keycloak#54319 Signed-off-by: Semih702 <semih702do@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes address the reported persistence failures with regression coverage and no identified blocking issues; tests were not executed during this review.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
Thanks, all four points are addressed. I checked the Store IT logs: recordFailure_storesErrorWithNullCharacterAndMultiByteCharacters runs on postgres, mssql, mysql and mariadb, and is skipped on oracle as intended.
Closes #54319
OutboxStore.truncateErrornow makes sureLAST_ERRORcan always be stored:VARCHAR(2048)in bytes. It cuts at a code point boundary and keeps the...marker.Messages without U+0000 that fit in 2048 bytes are stored unchanged. No schema change.
SsfPushDeliveryHandler.truncateDetailno longer splits a surrogate pair, and its Javadoc now says the final cap is the store's, in UTF-8 bytes.Tests:
OutboxStoreTests.recordFailure_storesErrorWithNullCharacterAndMultiByteCharacters, tagged@DatabaseTestso Store IT runs it (disabled on Oracle until Outbox: make the generic outbox usable for multiple consumers #54139, see SSF push outbox stalls when a receiver's error response cannot be stored in LAST_ERROR #54319). OncerecordFailuresucceeds, the drainer tick has nothing to roll back.recordFailure_bumpsAttemptsAndSchedulesRetryAndTruncatesLongErrornow checks the UTF-8 byte length.SsfPushDeliveryHandlerTruncateDetailTest(unit test).OutboxStoreTests#recordFailure*invalid byte sequence for encoding "UTF8": 0x00)End to end, before the change, a receiver answering
500 with such a body made the healthy receiver of the same event get the push 10 extra times in 5 s (PostgreSQL with U+0000, SQL Server with 1,100 ×é). I checked that locally with SSF tests that are not part of this PR, asssf/tests/baseruns only on dev-mem in CI.This also applies on top of #54139, which keeps
truncateErroras it is.AI disclosure: Claude Code was used to assist with analysis, reproduction and implementation. I understand every change and can revise it.