TIKA-4848: cross-PR final review (#3108)

Author: tballison

.skills/devs/feature-workflow/SKILL.md

@@ -77,3 +77,30 @@ widen a PR during review.
 | PR review changes an interface | pull piece back to spike |
 | PR review finds out-of-scope issue | todo doc, not this PR |
 | Third review round on a PR | one of the two above applies |
+
+## Cross-cutting refactors (chokepoints, wire formats)
+
+A refactor claiming "every X goes through Y" — a policy chokepoint, a single
+serialization route — is a contract too, with three extra rules. (Learned on
+TIKA-4848, where all three were violated and each violation became a
+post-merge review round.)
+
+- **Enforcement ships with the invariant, default-on.** The PR that creates
+  the chokepoint also adds the build-breaking check (forbidden-apis ban,
+  architecture test) that keeps call sites on it — every module, explicit
+  opt-outs. Opt-in checks document an invariant; only default-on maintains
+  it. Without this, each missed call site is a review finding instead of a
+  compile failure.
+
+- **One thin end-to-end test per transport surface.** A knob that crosses a
+  JVM boundary is exercised through each surface it rides (in-JVM, server,
+  pipes fork, grpc) in the invariant's own PR. Diff review cannot see a
+  parent-resolves/fork-deserializes seam; a wire test can.
+
+- **Producer and consumer change together.** Both halves of a
+  serializer/deserializer or parent/fork protocol pair go in the same PR.
+  Cutting by module puts every seam between two reviews, and seams are where
+  cross-cutting bugs live.
+
+No compat shim without a caller: grep before deprecating-in-place. A shim
+for an API nothing calls is dead weight that still carries bugs.

CHANGES.txt

@@ -1,5 +1,16 @@
 Release 4.1.0 - unreleased
 
+   * Add "exception-reporting" parse-context config to redact and bound
+     exception text in metadata, tika-server error bodies and pipes/grpc
+     messages; FileSystemEmitter writes atomically (TIKA-4848).
+     Compat notes: a truncated TSD envelope now records its read failure
+     under tk:exception:embedded-stream-exception rather than
+     tk:exception:embedded-exception; recordException and
+     recordEmbeddedStreamException no longer strip a bare TikaException
+     wrapper, so the first line of tk:exception:* values may name the
+     wrapper (affects consumers keyed on that line, e.g. eval cause
+     counts across the 4.1 boundary).
+     
    * Audio cover art is emitted as a THUMBNAIL embedded document, like the
      preview image of the document container formats: the front cover (ID3
      APIC and FLAC/Vorbis picture type 3), else the first picture of type
@@ -33,7 +44,7 @@ Release 4.1.0 - unreleased
      records; part of TIKA-4848 step 3 (TIKA-4848).
 
    * Allow image compression settings in PDFBox-based renderer (TIKA-4862).
-     
+
    * The tika-server full and tika-grpc Docker images set OMP_THREAD_LIMIT=1:
      to avoid oversubscribing the CPU under forked parse workers (TIKA-4863).
 
@@ -75,21 +86,6 @@ Release 4.1.0 - unreleased
      THUMBNAIL, so a client had to compare them to find the representative
      one (TIKA-4851).
 
-   * New exception-reporting parse-context config controls how much of an
-     exception is written to tk:exception:* metadata for the container and
-     embedded documents alike: FULL (default), MESSAGE_REDACTED (stack trace
-     without messages) or REDACTED (class names only), plus a maxLength.
-     Config-only; rejected in per-request parse-context. Parsers should
-     format exceptions via ExceptionUtils.format(Throwable, ParseContext);
-     ExceptionUtils.getStackTrace/getFilteredStackTrace and the ParseContext-less
-     EmbeddedDocumentUtil.recordException* / ParserUtils.recordParserFailure
-     overloads are deprecated (TIKA-4848).
-
-   * FileSystemEmitter writes to a sibling ".tmp" file and renames it into
-     place, so readers of the output directory never see a partially written
-     file. Set "atomicWrites": false on the emitter to restore in-place
-     writes (TIKA-4848).
-
    * tika-eval: Profile/Compare accept the batch run's jsonl crash ledger
      (--pipesReport, -pa/-pb) and a run-info json (--runInfo, -ra/-rb), and
      read both from <extracts>/.run-info/ by default (refusing an ambiguous

docs/modules/ROOT/pages/advanced/integration-testing/run-uat-script.adoc

@@ -73,6 +73,11 @@ Coverage includes:
   4.0.0-alpha-1 and beta-1: the resource failed to load and the endpoint 406'd
 * Removed endpoints stay removed (`/translate/*` returns 404)
 * 404 / 405 error handling
+* Exception reporting on a corrupt document (TIKA-4848): `/rmeta` embeds
+  `tk:exception:container-exception` at `200`; raw `/tika` returns `422` with a
+  content-only body; `/unpack` and `/meta/\{field}` carry the container exception
+  in their `422` bodies, asserted free of this server's own frames
+  (`PipesParsingHelper`, `MetadataResource`)
 * OCR (`PUT /tika/text` of an image whose text only exists as pixels): skipped when the
   server has no tesseract (the minimal image / a plain `java -jar` server), and a hard
   failure when `TIKA_UAT_REQUIRE_OCR=1` is set. `docker-tool.sh test-uat` sets that env

docs/modules/ROOT/pages/advanced/setting-limits.adoc

@@ -55,8 +55,9 @@ throughout the parse.
 
 == Configuring limits
 
-Every limit group is a `parse-context` block. This is the configuration
-exercised by `AllLimitsTest` (`tika-serialization/src/test/resources/configs/all-limits-test.json`):
+Every limit group is a `parse-context` block. `AllLimitsTest`
+(`tika-serialization/src/test/resources/configs/all-limits-test.json`) exercises all of
+these but `exception-reporting`, which has its own test:
 
 [source,json]
 ----
@@ -86,6 +87,10 @@ exercised by `AllLimitsTest` (`tika-serialization/src/test/resources/configs/all
       "maxFieldSize": 102400,
       "maxKeySize": 1024,
       "maxValuesPerField": 100
+    },
+    "exception-reporting": {
+      "level": "MESSAGE_REDACTED",
+      "maxLength": 10000
     }
   }
 }
@@ -113,8 +118,9 @@ context.set(OutputLimits.class, new OutputLimits(50000, true, 50, 5, 500000, 50)
 context.set(TimeoutLimits.class, new TimeoutLimits(7200000, 120000));
 ----
 
-Tests: `AllLimitsTest`, `EmbeddedLimitsTest`, `OutputLimitsTest`, and
-`TimeoutLimitsTest` under `tika-serialization/src/test/java/org/apache/tika/config/`.
+Tests: `AllLimitsTest`, `EmbeddedLimitsTest`, `OutputLimitsTest`, `TimeoutLimitsTest` and
+`ExceptionReportingConfigLoadTest` under `tika-serialization/src/test/java/org/apache/tika/config/`;
+`ExceptionReportingParseTest` (tika-core) drives the policy through a real parse.
 
 == Embedded document limits
 
@@ -354,10 +360,17 @@ if ("true".equals(metadata.get(TikaCoreProperties.TRUNCATED_METADATA))) {
 
 `ExceptionReporting` controls how much detail Tika reports when a parse fails:
 the `tk:exception:container-exception`, `tk:exception:embedded-exception`,
-`tk:exception:warn` and `tk:exception:embedded-stream` metadata values. Stack
-traces and exception messages can carry file paths, hostnames and fragments of
-the document; the metadata limiter deliberately never truncates them (see
-<<Always-included fields>>), so this is the only bound on them.
+`tk:exception:warn` and `tk:exception:embedded-stream-exception` metadata
+values. Stack traces and messages can carry file paths, hostnames and fragments
+of the document, and the metadata limiter is no backstop: with no
+`MetadataWriteLimiterFactory` configured these values are unbounded, and with
+one, only `container-exception` and `embedded-exception` are always-included
+(cut at `max(maxFieldSize, 300)` UTF-16 bytes); `warn` and
+`embedded-stream-exception` are ordinary fields, subject to
+`includeFields`/`excludeFields` and dropped past `maxValuesPerField` (see
+<<Always-included fields>>). The limiter's cut appends no marker, so keep
+`maxLength` (characters) under half `maxFieldSize` (UTF-16 bytes) if you want
+the `...[truncated]` marker to survive.
 
 [cols="2,1,3"]
 |===
@@ -371,8 +384,8 @@ chain are kept). `REDACTED`: exception class names only, no frames.
 
 |`maxLength`
 |-1 (unlimited)
-|Maximum characters in the formatted exception; longer values end with
-`...[truncated]`.
+|Length at which the formatted exception is cut; a truncated value is
+`maxLength` characters plus the `...[truncated]` marker.
 |===
 
 [source,json]
@@ -384,14 +397,29 @@ chain are kept). `REDACTED`: exception class names only, no frames.
 ----
 
 The same policy applies to the container and to every embedded document, to
-parser-level warnings (`tk:exception:warn`, `tk:exception:embedded-stream`), to
-the messages a pipes worker returns on fetch/emit/crash, and to tika-server's
-`422` error bodies. It is loaded from the config only and is rejected in a per-request `parse-context`
-(tika-server `/rmeta/config` and friends, `/pipes`, `/async`), so a caller cannot
-turn redaction back off.
+parser-level warnings (`tk:exception:warn`,
+`tk:exception:embedded-stream-exception`), to every pipes result message
+(fetch/emit/crash inside the worker, and the results the parent fabricates when
+the worker never reports one), and to every tika-server error body built from an
+exception -- `422` and `500`. It is loaded from the config only and is
+rejected in a per-request `parse-context` -- tika-server `/rmeta/config` and
+friends, `/pipes` and `/async` answer `400` with the reason, tika-grpc's
+`parse_context_json` answers `INVALID_ARGUMENT` -- so a caller cannot turn
+redaction back off.
 
 Programmatically, parsers and callers should format exceptions through
-`ExceptionUtils.format(Throwable, ParseContext)`.
+`ExceptionUtils.format(Throwable, ParseContext)`. A parser still calling the
+deprecated two-argument `EmbeddedDocumentUtil.recordException` /
+`recordEmbeddedStreamException` -- including any third-party parser compiled
+against 4.0.0 -- reports `FULL` whatever the policy says. To set a policy in a
+parser test:
+
+[source,java]
+----
+ParseContext context = new ParseContext();
+context.set(ExceptionReporting.class,
+        new ExceptionReporting(ExceptionReporting.Level.MESSAGE_REDACTED, 500));
+----
 
 == Recommendations
 

docs/modules/ROOT/pages/migration-to-4x/migrating-tika-server-4x.adoc

@@ -155,7 +155,9 @@ at least a `status` field:
 ----
 
 A `message` field is included when one is available and may contain a server-side stack
-trace, e.g. `{"status": "TIMEOUT", "message": "Task timed out after 60000ms"}`.
+trace, e.g. `{"status": "TIMEOUT", "message": "Task timed out after 60000ms"}`. How much of
+that trace is returned is set by `parse-context.exception-reporting` (default `FULL`) — see
+xref:advanced/setting-limits.adoc#_exception_reporting[Exception reporting].
 
 The HTTP status codes are also more precise:
 
@@ -329,8 +331,14 @@ The following `TikaServerConfig` options have been removed:
   exception detail — including messages carrying file paths and document fragments —
   continued to travel in `tk:exception:*` metadata on successful parses and inside
   `/unpack` zips. Setting it to `false` did not keep stack traces inside the server, which
-  is what the name implied. Exception detail is now always returned; filter Tika's output
-  with a `MetadataFilter` before forwarding it somewhere less trusted.
+  is what the name implied. The replacement is `parse-context.exception-reporting`
+  (`MESSAGE_REDACTED` or `REDACTED`, plus a `maxLength`), one operator-set policy over every
+  channel that turns an exception into caller-visible text — metadata values, error bodies and
+  pipes result messages alike. See
+  xref:advanced/setting-limits.adoc#_exception_reporting[Exception reporting]. A
+  `MetadataFilter` dropping `tk:exception:*` is a convenience, not the replacement: it reaches
+  neither error bodies nor pipes result messages, and with `allowPerRequestConfig` on, a filter
+  supplied in a request's config *replaces* the operator's rather than adding to it.
 
 * `taskTimeoutMillis` - Was the total time allowed per task before the forked process was
   killed; that is now `parse-context.timeout-limits.totalTaskTimeoutMillis` (same

docs/modules/ROOT/pages/pipes/configuration.adoc

@@ -141,6 +141,10 @@ set `EMIT`. Nothing reads the value from this section.
 |When `false`, stop the pipeline on configuration errors (missing fetcher/emitter). When `true`, only stop on fatal initialization failures. Use `true` for server mode, `false` for batch mode.
 |===
 
+How much of a failure is written into `tk:exception:*` metadata and into a `PipesResult`
+message is set by `parse-context.exception-reporting`, not here -- see
+xref:advanced/setting-limits.adoc#_exception_reporting[Exception reporting].
+
 == Async / Emit Batching
 
 These settings control how parsed results are batched before sending to emitters.

docs/modules/ROOT/pages/pipes/parse-modes.adoc

@@ -130,6 +130,8 @@ these values. If unset, the default is `MARKDOWN`.
 
 If the container parse fails (`SAXException`, `EncryptedDocumentException`, or any other `Exception`), the stack trace is caught, logged, and stored on the container metadata as `tk:exception:container-exception`. The parse continues to a return value rather than throwing — callers must check this field if they need to detect failure.
 
+How much of that trace is stored is set by `parse-context.exception-reporting` — see xref:advanced/setting-limits.adoc#_exception_reporting[Exception reporting].
+
 If the configured write limit is reached during concatenation, `tk:exception:write-limit-reached` is set to `true`.
 
 [#content-only-mode]

docs/modules/ROOT/pages/pipes/plugins/filesystem.adoc

@@ -146,7 +146,7 @@ Writes parsed results as files under `basePath`. The relative output path is der
 
 |`atomicWrites`
 |`true`
-|Write each output to a sibling `<name>.<uuid>.tmp` and rename it into place, so a reader of the output directory never sees a partial file. Set to `false` to write in place (one fewer rename per file; needed on filesystems where rename is slow or not atomic). Init-time only.
+|Write each output to a sibling `<name>.<uuid>.tmp` and rename it into place. A reader of the output directory never sees a partial file, but it does see the tmp files: crawlers and watchers over the output directory must ignore `*.tmp`. A worker killed mid-write leaves its `.tmp` behind, and nothing sweeps them. The `onExists: EXCEPTION`/`SKIP` check runs immediately before the rename and is best-effort against two workers emitting the same key concurrently (last writer wins) -- give workers distinct emit keys if that matters. Set to `false` to write in place (one fewer rename per file; required on mounts without rename, e.g. mountpoint-s3). Init-time only.
 |===
 
 [#file-system-iterator]

docs/modules/ROOT/pages/using-tika/cli/index.adoc

@@ -115,6 +115,10 @@ For Pipes-mode options see <<_tika_pipes_processing,Tika Pipes Processing>>.
 Redirect to save: `--convert-config-xml-to-json=tika-config.xml > tika-config.json`.
 |===
 
+The config's `parse-context` applies to the CLI too, so `exception-reporting` bounds the
+`tk:exception:*` values in `-J`/`-j` output — see
+xref:advanced/setting-limits.adoc#_exception_reporting[Exception reporting].
+
 === Output Formatting
 
 [cols="1,3"]

docs/modules/ROOT/pages/using-tika/grpc/index.adoc

@@ -201,6 +201,13 @@ request. Keys are parse-context component names; values are their JSON configs.
 See `META-INF/tika/parse-context.idx` (generated at build time from `@TikaComponent` annotations)
 for the available component names.
 
+A few components are operator-only and never accepted from the wire —
+`exception-reporting` (xref:advanced/setting-limits.adoc#_exception_reporting[Exception
+reporting]) is one. A request naming one is rejected at the API boundary with
+`INVALID_ARGUMENT` and the reason, before the forked worker sees it. A misspelled or
+unregistered component name is refused the same way, so a typo is a client error rather
+than a silent fallback to the server's defaults.
+
 NOTE: This is disabled by default. A request setting `parse_context_json` or
 `additional_fetch_config_json` is rejected with `PERMISSION_DENIED` unless `allowPerRequestConfig`
 is enabled — see <<_capability_flags,Capability flags>>.