Repository navigation
fix(core,http): sanitize lastError so one bad value cannot poison a whole batch (#130) - #140
Conversation
…hole batch All results of a batch are persisted in one update inside the claim transaction. A last_error the database rejected rolled back the entire batch: already-delivered entries reverted to PENDING with retries unchanged and were redelivered on every tick, forever. An HTTP error body over 64 KB (MySQL TEXT overflow) or a NUL character (Postgres) was enough. - OutboxEntry.retry/toFailed strip NULs and truncate to MAX_LAST_ERROR_LENGTH (4096 chars) without splitting a surrogate pair -- covers every transport. - HttpMessageDeliverer discards success bodies and keeps only the first MAX_ERROR_BODY_BYTES (2 KB) of an error body instead of buffering the whole response in heap. - MySQL/Postgres integration tests: a control proving each database rejects the raw value, and a processor test proving the delivered entry of the same batch now stays DELIVERED while the failing one advances to retries=1. Closes #130 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rawo
left a comment
There was a problem hiding this comment.
Review focused on transactions/data loss, memory, performance and structure. The fix is correct for both triggers in #130. sanitizeLastError keeps the result ≤ MAX_LAST_ERROR_LENGTH, the surrogate check is right, and the bounded body handler does cap heap per request. The control tests are a good touch. No blockers. Inline notes below, roughly in order of importance.
Residual poison-batch risk (worth tracking in the follow-up you mention): sanitizing only covers the two known values. One more cheap trigger remains on MySQL: okapi_outbox is created without an explicit CHARACTER SET, so it inherits the schema/server default. On a schema that is still latin1 or utf8mb3, which is common on 5.7-era or migrated databases, an error body containing an emoji or any character outside that charset fails in strict mode with ERROR 1366 Incorrect string value. That rolls back the whole batch again, delivered entries included. Two options: add DEFAULT CHARSET=utf8mb4 in a new changeset (the existing-table case needs an ALTER), or prioritise the per-row fallback so any unexpected rejection only affects its own row. I'd open that issue now rather than later.
…ison-batch # Conflicts: # CHANGELOG.md
…y time Review follow-ups for #130: - Both stores apply OutboxEntry.sanitizeLastError (now public, idempotent) when writing last_error, so entries built with copy(lastError = ...) are covered, not only retry()/toFailed(). The control test now writes the raw value with plain JDBC; a new test covers the store boundary. - OutboxEntry.TRUNCATION_MARKER is public and reused by the HTTP module and tests. KDoc: a UTF-16 char is at most 3 UTF-8 bytes (~12 KB bound). - HttpMessageDeliverer: a cancelling BodySubscriber reads at most MAX_DRAINED_BODY_BYTES (64 KB) of any body, and each exchange, body included, must finish within EXCHANGE_TIMEOUT (30 s) or is cancelled as RetriableFailure. HttpRequest.timeout only covers the headers, so a dripping body could hold the claim transaction open indefinitely. deliver() now shares the async path, and an interrupted wait cancels its exchange. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Closes #130.
Problem
OutboxProcessor.processNextwrites every result of a batch with oneupdateAfterProcessingBatchcall inside the claim transaction. If the database rejects one row'slast_error, the whole transaction rolls back:PENDINGretriesdoesn't advance for anythingThe next tick redelivers the entire batch, and this repeats indefinitely. Under Exposed's default retry it happens three times per tick. Two values trigger it easily:
TEXT(ERROR 1406 Data too long).text.Separately,
HttpMessageDelivererread every response body fully into heap withBodyHandlers.ofString(), and nothing bounded how long that took.Fix
OutboxEntry.sanitizeLastError(public, idempotent):OutboxEntry.MAX_LAST_ERROR_LENGTH = 4096chars, ending withOutboxEntry.TRUNCATION_MARKER. It never cuts between the two halves of a surrogate pair, because MySQL rejects a lone surrogate as an invalid string.TEXT.OutboxEntry.retry/toFailedand again by both stores when writinglast_error, so an entry built withcopy(lastError = ...)is covered too.BodySubscriberand an overall exchange deadline.HttpMessageDeliverer.MAX_ERROR_BODY_BYTES = 2048bytes.MAX_DRAINED_BODY_BYTES = 64 KBof any body is read. Up to that size the rest is drained, so the connection is reused; past it the subscription is cancelled. That closes an HTTP/1.1 connection, while HTTP/2 resets only the stream.EXCHANGE_TIMEOUT = 30 s, or it is cancelled and classified asRetriableFailure.HttpRequest.timeoutonly covers waiting for the response headers: on JDK 25, a 1 s request timeout still waited 10.5 s for a dripped body. Without the deadline, a slow body held delivery, and with it the claim transaction, open indefinitely.deliver()now sharesdeliverBatch()'s async path, and an interrupted wait cancels its exchange, asHttpClient.senddid.Behaviour changes to review
internalconstructor), so this PR adds no public API for it.Not in this PR
Isolating per-row update failures, and MySQL tables on a non-
utf8mb4charset (an emoji in an error body would still roll back the batch), are tracked in #141.Tests
MysqlLastErrorPoisonBatchTest/PostgresLastErrorPoisonBatchTest(Testcontainers) sharelastErrorPoisonBatchTests:DELIVERED, and the poisoned one isPENDINGwithretries = 1and a bounded, NUL-freelast_error.lastErrorwas set withcopy(...)is stored sanitized by the store.OutboxEntryLastErrorTest: unchanged short errors, truncation on bothretryandtoFailed, the exact-limit boundary, NUL stripping, and no split surrogate pairs.HttpMessageDelivererTest:deliver()anddeliverBatch()SuccessRetriableFailurein both paths (deadline)./gradlew ktlintCheck build testpass locally (464 tests).🤖 Generated with Claude Code