Skip to content

TIKA-4793: make Pipes IPC payload limit configurable#2962

Open
srujana-kuntumalla wants to merge 4 commits into
apache:mainfrom
srujana-kuntumalla:TIKA-4793
Open

TIKA-4793: make Pipes IPC payload limit configurable#2962
srujana-kuntumalla wants to merge 4 commits into
apache:mainfrom
srujana-kuntumalla:TIKA-4793

Conversation

@srujana-kuntumalla

Copy link
Copy Markdown
Contributor

The hard-coded 100 MB ceiling in PipesMessage was not operator-tunable. Add PipesConfig.maxIpcPayloadBytes (default 100 MB) and thread it through PipesClient, PipesServer, ConnectionHandler, and ServerProtocolIO so the limit is applied on every read() call. The write path is unchanged. Includes unit tests for default value, JSON loading, and validation.

Thanks for your contribution to Apache Tika! Your help is appreciated!

Before opening the pull request, please verify that

  • there is an open issue on the Tika issue tracker which describes the problem or the improvement. We cannot accept pull requests without an issue because the change wouldn't be listed in the release notes.
  • the issue ID (TIKA-XXXX)
    • is referenced in the title of the pull request
    • and placed in front of your commit messages surrounded by square brackets ([TIKA-XXXX] Issue or pull request title)
  • commits are squashed into a single one (or few commits for larger changes)
  • Tika is successfully built and unit tests pass by running ./mvnw clean test
  • there should be no conflicts when merging the pull request branch into the recent main branch. If there are conflicts, please try to rebase the pull request branch on top of a freshly pulled main branch
  • if you add new module that downstream users will depend upon add it to relevant group in tika-bom/pom.xml.

We will be able to faster integrate your pull request if these conditions are met. If you have any questions how to fix your problem or about using Tika in general, please sign up for the Tika mailing list. Thanks!

The hard-coded 100 MB ceiling in PipesMessage was not operator-tunable.
Add PipesConfig.maxIpcPayloadBytes (default 100 MB) and thread it through
PipesClient, PipesServer, ConnectionHandler, and ServerProtocolIO so the
limit is applied on every read() call. The write path is unchanged.
Includes unit tests for default value, JSON loading, and validation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@srujana-kuntumalla

Copy link
Copy Markdown
Contributor Author

@tballison @THausherr Can you please review this PR? TIA

@tballison

Copy link
Copy Markdown
Contributor

My agent recommends these four small changes.

1. Kill the duplicated constant. Change PipesConfig.DEFAULT_MAX_IPC_PAYLOAD_BYTES = 100L * 1024 * 1024 to reference PipesMessage.MAX_PAYLOAD_BYTES
  directly, so the "must stay equal" invariant is compiler-enforced instead of comment-enforced. (Do before merge.)
  2. Add a JSON-invalid-value load test. Load {"pipes":{"maxIpcPayloadBytes":0}} through PipesConfig.load and assert it throws. This locks in that
  Jackson binds via the validating setter — behavior that's otherwise invisible and would silently break if the mapper's visibility config ever
  changed. (Do before merge.)
  3. Unify how the limit is accessed. PipesClient caches it into a private final field; ConnectionHandler and PipesServer call
  pipesConfig.getMaxIpcPayloadBytes() inline. Pick one for consistency (dropping the field matches the other two). (Polish.)
  4. Add a one-line javadoc note that the limit is a hard, connection-terminating check that must be configured identically on both ends, and is
  startup-only (restart to change). (Polish.)

wdyt?

@tballison

Copy link
Copy Markdown
Contributor

Other thing is that tika-pipes was designed to emit rather than passing back to the pipesclient? What's your use case where you're passing back that much data?

@tballison

tballison commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

K, this one is more important. We should surface the source of the problem when a payload is too big, and we don't want to have to restart a pipesserver when this is hit.

Recommendation this surfaces (I'd promote it above my #3/#4): give the limit breach a distinct, honest status — a TASK_EXCEPTION-category value like
  OUTPUT_TOO_LARGE / PAYLOAD_LIMIT_EXCEEDED — caught specifically in PipesClient so that: the client gets a clear, actionable error with the size in
  it, the server is not restarted, and tika-server surfaces the reason in the response body. Right now the PR makes the ceiling tunable but leaves the
  diagnosis when it trips as a mislabeled crash. Worth raising with the author — arguably the change isn't complete without it.

srujana-kuntumalla and others added 2 commits July 21, 2026 21:43
Remove final from PipesMessage.MAX_PAYLOAD_BYTES (long) so it can be set
at runtime. PipesConfig.setMaxIpcPayloadBytes() updates the static whenever
the limit is changed via JSON config or programmatically. No changes to
call sites — all existing PipesMessage.read() callers pick up the new value
automatically through the shared static.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove final from PipesMessage.MAX_PAYLOAD_BYTES so it can be updated
at runtime. Add maxIpcPayloadBytes to PipesConfig (int, default 100 MB)
with a setter that updates PipesMessage.MAX_PAYLOAD_BYTES as a side
effect. Both client and server JVMs load from the same tika-config.json
so setting it once covers both ends automatically. No changes to call
sites — all existing PipesMessage.read() callers pick up the value
through the shared static.

Configurable via tika-config.json:
  {"pipes": {"maxIpcPayloadBytes": 209715200}}

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@srujana-kuntumalla

Copy link
Copy Markdown
Contributor Author

I have updated the PR to keep the changes minimal to allow max payload size to be configurable.

@tballison

Copy link
Copy Markdown
Contributor

I agree with my claude on this one. Seriously, thank you for opening this and iterating on it.

Thanks for iterating. The last two commits trade the threaded read() param for a mutable global static (PipesMessage.MAX_PAYLOAD_BYTES un-final'd,
  set as a side effect of the config setter). I'd push back on that — the global loses per-config semantics (multiple PipesConfig in one JVM → last
  load() wins, so getMaxIpcPayloadBytes() can disagree with what's enforced), pollutes tests (testMaxIpcPayloadBytesFromJson sets 200 MB and never
  resets), and races (non-volatile, read from the IPC loops). The earlier threaded approach (e2c987f) was verbose but correct. (The long → int switch
  is a good call, though.)

  Two things still open regardless:
  1. Over-limit payloads throw a bare IOException that PipesClient reports as UNSPECIFIED_CRASH/process_crash — reason dropped from the HTTP response
  and the healthy server gets restarted. Deserves a distinct TASK_EXCEPTION status (e.g. OUTPUT_TOO_LARGE).
  2. Add a test that loads {"pipes":{"maxIpcPayloadBytes":0}} via PipesConfig.load and asserts rejection.

…_EXCEEDED status

- Restore MAX_PAYLOAD_BYTES to final; add read(DataInputStream, int) overload
  so callers can pass a per-connection limit without mutating shared state
- PipesConfig setter no longer has the global side-effect; PipesClient captures
  maxIpcPayloadBytes at construction and passes it to read() in waitForServer()
- Add PAYLOAD_LIMIT_EXCEEDED(TASK_EXCEPTION) to RESULT_STATUS so oversized
  responses are treated as per-document errors, not process crashes
- Introduce PayloadLimitExceededException (IOException subtype) and catch it
  specifically in PipesClient: close the desynchronized connection but do not
  restart the healthy server
- Add JSON zero-value rejection test through the deserialization path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@srujana-kuntumalla

Copy link
Copy Markdown
Contributor Author

Appreciate the feedback! I have updated the PR with threaded limit and PAYLOAD_LIMIT_EXCEEDED status

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.

2 participants