Skip to content

fix(core,http): sanitize lastError so one bad value cannot poison a whole batch (#130) - #140

Open
rawo wants to merge 1 commit into
mainfrom
fix/130-last-error-poison-batch
Open

rawo wants to merge 1 commit into
mainfrom
fix/130-last-error-poison-batch

Conversation

@rawo

@rawo rawo commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Closes #130.

Problem

OutboxProcessor.processNext writes every result of a batch with one updateAfterProcessingBatch call inside the claim transaction. If the database rejects one row's last_error, the whole transaction rolls back:

  • entries that were already delivered go back to PENDING
  • retries doesn't advance for anything

The 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:

  • MySQL: an HTTP error body over 64 KB overflows TEXT (ERROR 1406 Data too long).
  • Postgres: a NUL character is refused in text.

Separately, HttpMessageDeliverer read every response body fully into heap with BodyHandlers.ofString().

Fix

  • Core (covers every transport): OutboxEntry.retry and toFailed sanitize the error.
    • NULs are stripped.
    • The text is truncated to the new public OutboxEntry.MAX_LAST_ERROR_LENGTH = 4096 chars with a ... [truncated] marker, never between the two halves of a surrogate pair (MySQL rejects a lone surrogate as an invalid string).
    • 4096 chars × 4 bytes fits comfortably inside MySQL's 65,535-byte TEXT.
  • HTTP: a bounded body handler.
    • 2xx bodies are discarded (BodySubscribers.replacing("")).
    • Error bodies keep only the first HttpMessageDeliverer.MAX_ERROR_BODY_BYTES = 2048 bytes, and the rest is drained without being stored so the connection stays reusable.

Not in this PR

The issue also suggests isolating per-row update failures, e.g. falling back to row-by-row updates so any unexpected DB rejection only affects its own row. That is a larger behavioural change to the store/processor contract. With both known triggers removed I've left it for a follow-up, and I'm happy to open an issue for it.

Tests

  • MysqlLastErrorPoisonBatchTest / PostgresLastErrorPoisonBatchTest (Testcontainers) share lastErrorPoisonBatchTests:
    • control: writing the raw value (100 KB on MySQL, NUL on Postgres) through updateAfterProcessingBatch fails. This proves the scenario is real.
    • a batch of one healthy and one poisoned entry → the healthy one stays DELIVERED, and the poisoned one is PENDING with retries = 1 and a bounded, NUL-free last_error.
  • OutboxEntryLastErrorTest: unchanged short errors, truncation on both retry and toFailed, the exact-limit boundary, NUL stripping, and no split surrogate pairs.
  • HttpMessageDelivererTest: a 1 MB error body is bounded in both deliver() and deliverBatch(), short error bodies are kept whole, and a large success body is discarded while still counting as Success.
  • ./gradlew ktlintCheck build test pass locally.

🤖 Generated with Claude Code

…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 rawo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* rest is cut to [MAX_LAST_ERROR_LENGTH], never between the two halves of a surrogate pair,
* which MySQL would reject as an invalid string.
*/
internal fun sanitizeLastError(error: String): String {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, structure: sanitizing in retry/toFailed covers the processor path, but OutboxEntry is a public data class. copy(lastError = ...) or the constructor (custom processors, tests, listeners that rebuild entries) still reach updateAfterProcessing* unsanitized. The real boundary is the store write (setString(5, entry.lastError) in both stores). Calling sanitizeLastError there too, or only there, would make the guarantee hold for every caller. It is idempotent, so doing it in both places costs nothing.

Nit: the KDoc says "at most 4 bytes per char". A UTF-16 char is at most 3 UTF-8 bytes (a 4-byte code point uses 2 chars), so the real bound is ~12 KB. The conclusion holds; only the wording is off.

*/
private val boundedBodyHandler = HttpResponse.BodyHandler { info ->
if (info.statusCode() in 200..299) {
HttpResponse.BodySubscribers.replacing("")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance / lock duration: replacing("") and boundedUtf8Body both still read the whole body to drain the connection. Heap is now bounded, but time isn't. HttpRequest.timeout only covers waiting for response headers, not body streaming. So an endpoint that drips a large or endless error body keeps deliver() busy indefinitely, and with it the scheduler's claim transaction: the claimed rows stay locked, and on MySQL under REPEATABLE READ so do the gap locks (#129).

This isn't a regression (ofString() behaved the same way), but now that the subscriber is custom it's cheap to fix. Once limit bytes are collected, subscription.cancel() instead of draining. For HTTP/1.1 that closes the connection rather than reusing it, which is the right trade for an error response. HTTP/2 just resets the stream. That needs a small hand-written BodySubscriber instead of ofByteArrayConsumer, which can't cancel. An overall deadline on the batch (future.get(timeout) / orTimeout) would also bound it.

var truncated = false
// ofByteArrayConsumer delivers chunks one at a time, in order (Flow.Subscriber.onNext contract),
// and the mapping runs after the last one, so the unsynchronized state above is safe.
val consumer = HttpResponse.BodySubscribers.ofByteArrayConsumer { chunk ->

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concurrency reasoning checks out. Reactive Streams §1.3 guarantees serial onNext signals with happens-before, and the mapping runs off the completed future, so the unsynchronized ByteArrayOutputStream/truncated state is safe.

Nit: "... [truncated]" now appears in two modules. OutboxEntry.TRUNCATION_MARKER is private, so the HTTP module and both tests hard-code the literal. Consider making it public (or internal plus a test-fixture accessor) so the marker can't drift.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One oversized/invalid last_error rolls back the whole batch: delivered entries revert to PENDING and are redelivered forever

1 participant