TIKA-4793: server-side payload guard, OOM protection, and archive sizing fix

[srujana-kuntumalla]

Summary

Follow-on to the earlier TIKA-4793 configurable-payload-limit work. Four issues were found after that PR:

• OOM in forked JVM for large documents (e.g. 500 MB text with uncapped maxIpcPayloadBytes): ServerProtocolIO.writeFinished() called JsonPipesIpc.toBytes() unconditionally, allocating a full Smile byte[] in one shot. For large content this exhausted heap.
• Heap-bound error on very large single files: Same root cause — Jackson's ByteArrayBuilder cannot produce a byte[] > 2 GB, so content that serializes beyond that limit caused OutOfMemoryError inside toBytes().
• PDF "truncation" past ~80 M chars: For Unicode-heavy PDFs, 80 M chars × 3 bytes/char UTF-8 Smile exceeds the 100 MB default limit. Previously the server wrote the oversized payload anyway, the client threw PayloadLimitExceededException, the IPC stream desynced, and the connection was torn down. Users saw "no content returned" and interpreted it as truncation.
• 7z / rar / iso DYNAMIC emit-strategy sizing: EmitDataImpl.estimateSizeInBytes() used length × 2 (Java UTF-16 heap cost), but the DYNAMIC threshold is in wire bytes (Smile UTF-8). ASCII-heavy archive metadata (file paths, checksums, MIME types) encodes at ~1 byte/char, so estimates were 2× too high and compressed-archive results were incorrectly routed to direct-emit.

Changes

ServerProtocolIO — three-layer guard in writeFinished():

1. Pre-check: if getEstimatedSizeBytes() > maxPayloadBytes, skip serialization entirely (prevents OOM before any allocation).
2. OOM catch: wraps JsonPipesIpc.toBytes() — caught OutOfMemoryError frees the byte-builder segments; the tiny PAYLOAD_LIMIT_EXCEEDED response then serializes cleanly.
3. Post-check: if serialized bytes.length > maxPayloadBytes (CJK-heavy content where the 1 byte/char pre-estimate is optimistic), discard server-side — client sees a clean PAYLOAD_LIMIT_EXCEEDED, no stream desync.

Constructor now takes maxPayloadBytes explicitly.

PipesServer / ConnectionHandler — pass pipesConfig.getMaxIpcPayloadBytes() to ServerProtocolIO (one line each).

EmitDataImpl.estimateSizeInBytes() — change multiplier from length × 2 to length (1 byte/char ≈ Smile UTF-8 ASCII). The server-side post-check is the safety net for content that does serialize larger than the estimate.

ServerProtocolIOTest (new) — unit tests for all three protection layers plus a pin on the corrected estimate formula.

Test plan

• ServerProtocolIOTest — 5 new tests covering pre-check, post-check, OOM path, status-only pass-through, and estimate formula
• PipesMessageTest — 17 existing wire-protocol tests still pass
• No existing integration test behavior changes (all test documents are well below the DYNAMIC strategy thresholds; UNPACK and CONTENT_ONLY modes bypass the threshold entirely)

🤖 Generated with Claude Code

[srujana-kuntumalla]

@tballison @THausherr Could I get review on this PR please?

[nddipiazza]

The fallback frame also needs to fit maxPayloadBytes (or config validation needs to enforce a minimum that guarantees it will). setMaxIpcPayloadBytes() currently accepts any positive value, so with a small configured limit this serialized PAYLOAD_LIMIT_EXCEEDED result is itself larger than the limit. The production client then rejects it in PipesMessage.read(..., maxIpcPayloadBytes), closes the connection, and never receives the clean status this path promises. ServerProtocolIOTest.exchange() masks this by reading with Integer.MAX_VALUE. Please add a test where the client reads the fallback using the same configured limit and either use a bounded/minimal error payload or reject limits too small to carry it.

[nddipiazza]

codex spit this out when i asked it to check for test coverage

[tballison]

From my claude. 🤣

  1. Success clobbering — EMIT_SUCCESS_PASSBACK results (already emitted to
  S3/ES) get replaced with a failure status → retries re-emit already-indexed
  docs. Fix: degrade emitData only, keep the status.
  2. Pre-check false rejection — the "lower-bound" estimate is empirically ~3.6×
  over for short-string-heavy metadata (measured), so big archive results that
  fit under the limit get discarded — the PR's own target case regresses.
  3. Estimate unit change breaks other consumers — AsyncEmitter's heap budget
  and the DYNAMIC threshold now admit ~2× the real heap/content; needs a
  separate heap vs. wire estimate.
  4. OOM catch hazards — bypasses the module's exit-on-OOM/restart policy; and
  since the giant result is still live on the stack, the fallback serialization
  can OOM again, escaping into catch(Throwable) and hanging the client until
  socket timeout.
  5. Coverage gaps — writeIntermediate() and writeCrash() have none of the
  protections; same desync bug remains on those paths. awaitAck() still reads
  with the hardcoded 100 MB default.
  6. Config/docs — setMaxIpcPayloadBytes javadoc wrongly claims requests use the
  built-in default (the limit is bidirectional; a lowered limit makes big
  requests die as undiagnosable UNSPECIFIED_CRASH); knob missing from
  configuration.adoc.
  7. Branch hygiene — needs rebase: merge base predates #2962, so the GitHub
  diff shows ~463 additions when the real change is 5 files / +272 (I
  test-merged: clean, all 26 tests pass), and branch CI never exercises the
  TIKA-4813 timeout model.
  8. Architecture — one fix subsumes Nick's finding plus #2, #4, #5, and the
  post-check's ~2× memory spike: serialize through a size-capped counting stream
  instead of the three-layer estimate/OOM-catch/post-check.

  Note the counting-stream fix also resolves Nick's bug naturally (the tiny
  error frame is the only thing ever buffered). Minor test/comment nits from
  earlier still apply.

[tballison]

Updated review. Let me know if any of these points do not check out.

  Verdict

  The BoundedOutputStream rewrite (19c6b1dc4f) is the right architecture and resolves most of the earlier feedback — 7 of 9 items including Nicholas's fallback-frame issue. But it's
  not mergeable yet: two blockers you've already flagged, one confirmed data-loss regression vs main, and the PR body/title now describe code that doesn't exist.

  Blockers

  1. writeCrash — confirmed by reproduction, not just analysis (ServerProtocolIO.java:166-175). Truncates to limit/2 chars then calls unbounded toBytes() — the only write path that
  bypasses the BoundedOutputStream. Smile is UTF-8, so ≥U+0800 chars encode at 3 bytes/char. Repro: limit=1024, CJK exception message → 1498-byte frame → client throws
  PayloadLimitExceededException, and the server hung in awaitAck() until connection close. Downstream, the crash is misreported as PAYLOAD_LIMIT_EXCEEDED (TASK_EXCEPTION category)
  instead of OOM/TIMEOUT (PROCESS_CRASH) — wrong category for retry logic, crash detail lost. Javadoc claims "always fits." No test covers it. Fix: route through the same
  BoundedOutputStream+fallback as writeFinished, pin with a test.

  2. OOM swallowed post-serialization. writeFinished now allocates up to ~3× limit transiently (BAOS doubling + toByteArray() copy) on the main-loop thread. An OOM there isn't an
  IOException, so it lands in pre-existing catch (Throwable) blocks (PipesServer.java:404, ConnectionHandler.java:183) and the JVM keeps running post-OOM — per-client mode loops on;
  shared mode stays corrupted for all clients. Violates fork-and-die. Pre-existing catch sites, but this PR makes the allocation reachable, so it should carry the fix (rethrow
  Error/exit-on-OOM there, and/or -XX:+ExitOnOutOfMemoryError on the forked JVM). Side note: buf.writeTo(output) instead of toByteArray() would drop one full copy.

  Confirmed regression vs main

  3. Oversized docs vanish from audit output with emitIntermediateResults=true. The static fallback has null emitData/emitKey. On main, the client-side rejection path went through
  buildFatalResult, attaching emitKey + intermediate metadata, so a failure record was emitted. Now AsyncEmitter.add warn-skips the null emitData — no trace of the document in emitter
  output (PipesClient.java:418-424, AsyncEmitter.java:107-113). Fix: in the FINISHED branch, when status is PAYLOAD_LIMIT_EXCEEDED with null emitData, rebuild via buildFatalResult like
  the exception path.

  Your #1 (success clobbering) — still open, structurally foreclosed

  Oversized EMIT_SUCCESS_PASSBACK/EMIT_SUCCESS_PARSE_EXCEPTION (already emitted to S3/ES) still gets replaced wholesale by the failure-category fallback → double-emit on retry. The
  pre-serialized static frame can't carry the original status, so "degrade emitData only, keep the status" was never implemented. Fix: retry serialization with emitData stripped but
  status kept; static frame only if even that overflows. (Not a regression vs main, but it was the ask.)

  Diagnosability (usability)

  The new path gives the operator less than the old teardown did: the WARN logs only the configured limit — no doc/emit key, no actual size (the BoundedOutputStream knows it at abort)
  — and nothing anywhere names the maxIpcPayloadBytes knob. Old path at least logged "length X exceeds maximum Y" with the doc id. Cheap fix: enrich the WARN; serialize a per-doc
  message when the limit has headroom. Also: shared-server mode has independently configured client/server limits — server limit > client limit reproduces the old teardown; docs don't
  say to keep them in sync.

  Docs / hygiene

  - PR body and title are stale: they describe the abandoned three-layer guard, an "archive sizing fix" (×2→×1) that was reverted (final code keeps ×2; only real change is 2→2L
  overflow widening), and a test plan listing tests that don't exist. Author should rewrite both.
  - configuration.adoc:158 -Xmx sentence is backwards: says "lower -Xmx to ~3× this value"; should be "at least ~3×."
  - CHANGES.txt: no TIKA-4793 entry; both the knob and the behavior change (clean status vs teardown) belong there.
  - Validation floor admits guaranteed-broken values (floor = 67-byte fallback frame; anything below a typical FetchEmitTuple makes every request die as undiagnosable
  UNSPECIFIED_CRASH) — a pragmatic floor or load-time WARN would close the footgun the docs currently just describe.
  - awaitAck() still reads with the hardcoded 100 MB default (ServerProtocolIO.java:184) — harmless for empty ACK frames, inconsistent with the bidirectional contract.
  - Minor: BoundedOutputStream int-overflow guard ((long) buf.size() + len), dead serializedSize() helper + stale "estimate formula" comment in the test, stray blank line in
  TikaPipesConfigTest, writeIntermediate javadoc promises a "full result" FINISHED that will almost certainly be the fallback.

  What checks out

  Rebase is clean (merge-base = current main, real diff 9 files +357/−28); no estimate-based pre-check or OOM-catch remains in serialization; overflow aborts before any wire byte, no
  desync possible on the bounded paths; exactly-at-limit is symmetric writer/reader; Nicholas's minimum-limit fix is complete (enforced in both setter and constructor, config load goes
  through the setter, test now reads with the configured limit); PAYLOAD_LIMIT_EXCEEDED is handled sanely by tika-server, tika-grpc, AsyncProcessor; single-threaded use of
  ServerProtocolIO per connection, no sync issues; config can't disable the guard.

[tballison]

@srujana-kuntumalla let me know if I botched anything with my push to your branch.