Skip to content

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

Merged
rawo merged 3 commits into
mainfrom
fix/130-last-error-poison-batch
Nov 4, 2026
Merged

rawo merged 3 commits 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(), and nothing bounded how long that took.

Fix

  • Core (covers every transport): OutboxEntry.sanitizeLastError (public, idempotent):
    • NULs are stripped.
    • The text is truncated to OutboxEntry.MAX_LAST_ERROR_LENGTH = 4096 chars, ending with OutboxEntry.TRUNCATION_MARKER. It never cuts between the two halves of a surrogate pair, because MySQL rejects a lone surrogate as an invalid string.
    • A UTF-16 char is at most 3 UTF-8 bytes, so a stored value is at most ~12 KB, well inside MySQL's 65,535-byte TEXT.
    • It is applied in OutboxEntry.retry/toFailed and again by both stores when writing last_error, so an entry built with copy(lastError = ...) is covered too.
  • HTTP: a cancelling BodySubscriber and an overall exchange deadline.
    • Success bodies are discarded. Error bodies keep only the first HttpMessageDeliverer.MAX_ERROR_BODY_BYTES = 2048 bytes.
    • At most MAX_DRAINED_BODY_BYTES = 64 KB of 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.
    • Each exchange, body included, must finish within EXCHANGE_TIMEOUT = 30 s, or it is cancelled and classified as RetriableFailure. HttpRequest.timeout only 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 shares deliverBatch()'s async path, and an interrupted wait cancels its exchange, as HttpClient.send did.

Behaviour changes to review

  • New: an HTTP exchange is capped at 30 s end to end. Previously only the wait for response headers was bounded; a valid but slowly streamed body could take as long as it needed. The cap can only be changed from inside the module (internal constructor), so this PR adds no public API for it.
  • A success body over 64 KB, or an error body over 64 KB, now closes the HTTP/1.1 connection instead of being drained.

Not in this PR

Isolating per-row update failures, and MySQL tables on a non-utf8mb4 charset (an emoji in an error body would still roll back the batch), are tracked in #141.

Tests

  • MysqlLastErrorPoisonBatchTest / PostgresLastErrorPoisonBatchTest (Testcontainers) share lastErrorPoisonBatchTests:
    • control: writing the raw value (100 KB on MySQL, NUL on Postgres) with plain JDBC 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.
    • an entry whose raw lastError was set with copy(...) is stored sanitized by the store.
  • 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
    • a large success body is discarded and still counts as Success
    • a 1 MB body dripped over 20 s returns in about 1.5 s (read cap)
    • a small body dripped past a 1 s exchange timeout is a RetriableFailure in both paths (deadline)
  • ./gradlew ktlintCheck build test pass locally (464 tests).

🤖 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.

rawo and others added 2 commits November 4, 2026 09:15
…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>
@rawo
rawo merged commit 60fb96f into main Nov 4, 2026
8 checks passed
@rawo
rawo deleted the fix/130-last-error-poison-batch branch November 4, 2026 12:51
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