fix(rag): give every TikaReader read its own content handler

[KANLON]

AgentScope-Java Version

2.0.3-SNAPSHOT (main @ 70686e4)

Description

TikaReader keeps a single ContentHandler instance in a field and passes it to
AutoDetectParser.parse(...) on every read:

private final ContentHandler handler;      // created once
...
parser.parse(is, this.handler, metadata, context);   // same instance every read
return this.handler.toString();

BodyContentHandler (the default) is a sink that accumulates everything written to it — it
wraps a StringWriter and exposes no reset API. Because the instance is shared, the extracted
text grows monotonically instead of being per-document:

reader.read(docA) -> "ALPHA"
reader.read(docA) -> "ALPHA\nALPHA"        // same file read twice: duplicated
reader.read(docB) -> "ALPHA\nALPHA\nBETA"  // docB carries docA's text with it
reader.read(docA) -> "ALPHA\nALPHA\nBETA\nALPHA"

A freshly constructed reader is correct, which is why the existing single-read tests never caught
it. Any application that builds one TikaReader and indexes several documents (the documented
usage pattern) gets documents whose chunks contain the concatenation of every document read
before them, so the resulting embeddings and retrieved context are wrong — silently, with no
error.

The same shared instance also makes concurrent reads of one reader interfere with each other.

Why the accumulation is not intended behaviour

Sharing one handler looks like a "configure once" choice, but accumulation is not a usable feature:

• read is documented as a function of its input, and the output contradicts accumulation.
  createDocuments derives doc_id from the current path only (SHA256(path)) and indexes
  chunks as 0..n of that path. With a shared handler, the documents returned for the second file
  carry doc_id = SHA256(second) while their text also contains the first file — metadata and
  content disagree. There is no way to read accumulated state back: the only exit is read(),
  which presents it as the text of the file it was asked to read.
• It breaks idempotency. Reading the same file twice yields "ALPHA" then "ALPHA\nALPHA".
  No document reader is meant to be non-idempotent on identical input.
• Sibling readers are stateless. TextReader re-reads input.asString() on every call, so one
  reader can serve any number of documents. TikaReader is the only one that carries state
  across calls.
• The original commit shows no such intent. feat(rag): Support tika document parser (#538)
  describes the change only as supporting the parser; the field is commented
  Handler to manage content extraction and its tests never read twice. The shape is simply the
  standard Tika usage — new BodyContentHandler(...) immediately before a single
  parser.parse(...) — lifted into a field, which turns a per-parse buffer into a shared one.

This change also keeps the caller-supplied constructor on its old semantics, so a caller who
really did hand in a shared handler is unaffected; only the default constructor, where the caller
never chose a handler at all, gets per-read isolation.

Fix

• The field becomes a Supplier<ContentHandler> handlerFactory, consulted once per read.
• TikaReader() now asks for a fresh BodyContentHandler(-1) per read, so consecutive reads are
  independent.
• A new constructor TikaReader(int, SplitStrategy, int, Supplier<ContentHandler>) lets callers
  opt into per-read isolation for custom handlers too.
• The existing TikaReader(int, SplitStrategy, int, ContentHandler) constructor keeps its
  behaviour and its null check (singletonHandler); its javadoc now states that the supplied
  handler is reused as-is and that a stateful handler will carry text across reads.
• extractTextFromTika uses the per-read handler instead of the field; a handler factory that
  returns null at read time is rejected with the same message as the constructor check
  (Objects.requireNonNull).

Compatibility

Passing a null literal as the 4th argument is now ambiguous between the two overloads and needs
a cast — new TikaReader(512, SplitStrategy.PARAGRAPH, 50, (ContentHandler) null). This only
affects a literal null (which throws anyway); existing code that passes a real handler compiles
unchanged. The existing test testNullContentHandler was updated accordingly.

The only in-repository usage outside the tests is the documentation example
(docs/v2/{zh,en}/integration/rag/simple.md), which uses the default constructor and only reads
one document — unaffected.

Tests

Added to TikaReaderTest:

• testReusedReaderDoesNotAccumulateEarlierContent — reads two documents with one reader and
  asserts the second result equals its own text. Fails on the unpatched revision:
  reading the same document twice must be idempotent expected:<ALPHA[]> but was:<ALPHA[ALPHA]>.
• testCallerSuppliedHandlerIsUsed — a caller-supplied ToXMLContentHandler is still honoured.

Verified locally:

mvn -pl agentscope-extensions/agentscope-extensions-rag/agentscope-extensions-rag-simple -am test
  → BUILD SUCCESS (parent, core, extensions, model, dashscope, ollama, rag, rag-simple)
  → rag-simple: Tests run 452, Failures 0, Errors 0 (TikaReaderTest 10/10)
mvn -pl agentscope-extensions/agentscope-extensions-rag/agentscope-extensions-rag-simple spotless:check
  → BUILD SUCCESS

Checklist

• Code has been formatted with mvn spotless:apply
• All tests are passing (mvn test)
• Javadoc comments are complete and follow project conventions
• Related documentation has been updated (e.g. links, examples, etc.) — javadoc on the
  handler-reusing constructor and on the new factory constructor
• Code is ready for review

[oss-maintainer]

Summary

Real bug, correctly diagnosed and cleanly fixed: BodyContentHandler is an accumulating sink with no reset, so keeping one instance in a field made every later read() return the concatenation of all previously parsed documents (and made concurrent reads of one reader interfere). Switching the field to a per-read Supplier<ContentHandler> is the right shape, the legacy ContentHandler constructor keeps its old semantics on purpose, and the new testReusedReaderDoesNotAccumulateEarlierContent fails on the unpatched revision — that is exactly the regression guard this needed. Javadoc on both constructors is clear about which one isolates per read.

Two non-blocking points plus one test gap are pinned inline: the new overload makes a bare null 4th argument ambiguous for existing callers (a source-compat change worth calling out in release notes, or avoidable with a static factory), the read-time Objects.requireNonNull surfaces a factory contract violation as a wrapped ReaderException, and neither the new Supplier constructor nor the concurrency aspect of the bug is covered by a test.

Not approving yet: CI (build on ubuntu/windows) is still pending on c2689e8, and CLA/mergeability are fine (license/cla signed, MERGEABLE, merge_state=BLOCKED only because checks are in progress).

Checklist notes

• [Warning] TikaReader.java:106 — ContentHandler/Supplier<ContentHandler> overload ambiguity for null literals (source compatibility).
• [Info] TikaReader.java:183 — read-time null handling + constructor validation order.
• [Warning] TikaReaderTest.java:198 — new Supplier constructor and thread-safety claim are untested.

---

Automated review by github-manager-bot and first-time-contributor follow-up: welcome, this is a well-written PR.

[codecov]

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...java/io/agentscope/core/rag/reader/TikaReader.java	91.30%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Re-review after the force-push (c2689e82 -> 01f31709). All three points from my previous review are resolved, and the API shape is better for it:

• compile-time ambiguity — the Supplier<ContentHandler> variant is no longer a public constructor overload, so new TikaReader(size, strategy, overlap, null) resolves unambiguously to the existing ContentHandler constructor; the per-read variant is reached through TikaReader.perReadHandler(...).
• null from the factory — the IllegalStateException is now thrown outside the try, so it is no longer swallowed and re-wrapped as a generic ReaderException("Failed to read document from: ..."); a misconfigured factory is distinguishable from a parse failure, and there is a test pinning it.
• test coverage of the new entry point — null factory, factory returning null, one handler per read, caller-supplied handler still honoured, sequential reuse, and a 4-thread concurrency check. That is a proper regression suite for a bug that was invisible until someone read two documents.

The fix itself is right: BodyContentHandler is an accumulating sink with no reset, so one instance per read is the only correct default, and parser.parse(...) + handler.toString() inside the same callable keeps the handler's lifetime scoped to the read.

Two non-blocking notes inline: the factory is per subscription rather than per read(...) call (matters for retryWhen/re-subscription and is worth one sentence plus one test), and the legacy constructor still carries the original accumulation for existing callers — a reuse warning or a @Deprecated there would close the loop.

CI: ubuntu, License, Module Sync and codecov pass; build (windows-latest) was still queued when I looked. CLA signed. Approving — thanks for the quick turnaround, @KANLON.

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

Re-review after the force-push to 4cda8f17: the per-read handler fix is intact and the new tests are the strongest part of this PR — accumulation, resubscription, factory-invocation counting and a 4-thread concurrency case all assert the actual invariant instead of just the return value. Four non-blocking nits below (one version-string issue, three wording/contract details).

Commenting rather than approving: license/cla has no status on 4cda8f17 yet (the check was green on the previously approved head 01f31709), and both build jobs are still in progress, so I cannot re-approve this head. Once CLA + build go green there is nothing here I would block on.

Findings

• [Warning] TikaReader.java:91 — @Deprecated(since = "2.0.3") is off by one, main is already 2.0.4-SNAPSHOT; also worth an explicit forRemoval decision
• [Info] TikaReader.java:131 — the null-factory branch reuses the "content handler cannot be null" message, so two different failure modes are indistinguishable
• [Info] TikaReader.java:147 — the deprecated path keeps the old accumulation bug silently; a one-time log.warn would surface it for callers who do not build with -Xlint:deprecation
• [Info] TikaReader.java:165 — a factory that throws escapes unwrapped while everything after it becomes ReaderException, and the @throws IllegalStateException on perReadHandler describes an onError signal, not a synchronous throw

Suggestions

No structural change requested. The Supplier<ContentHandler> + private canonical constructor shape is the right way to keep the old public signature compiling, and Mono.fromCallable is already the correct per-subscription boundary, so consulting the factory inside the callable is right.

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

Re-review after the force-push to 4cda8f17: the per-read handler fix is intact and the new tests are the strongest part of this PR — accumulation, resubscription, factory-invocation counting and a 4-thread concurrency case all assert the actual invariant instead of just the return value. Four non-blocking nits below (one version-string issue, three wording/contract details).

Commenting rather than approving: license/cla has no status on 4cda8f17 yet (the check was green on the previously approved head 01f31709), and both build jobs are still in progress, so I cannot re-approve this head. Once CLA + build go green there is nothing here I would block on.

Findings

• [Warning] TikaReader.java:91 — @Deprecated(since = "2.0.3") is off by one, main is already 2.0.4-SNAPSHOT; also worth an explicit forRemoval decision
• [Info] TikaReader.java:131 — the null-factory branch reuses the "content handler cannot be null" message, so two different failure modes are indistinguishable
• [Info] TikaReader.java:147 — the deprecated path keeps the old accumulation bug silently; a one-time log.warn would surface it for callers who do not build with -Xlint:deprecation
• [Info] TikaReader.java:165 — a factory that throws escapes unwrapped while everything after it becomes ReaderException, and the @throws IllegalStateException on perReadHandler describes an onError signal, not a synchronous throw

Suggestions

No structural change requested. The Supplier<ContentHandler> + private canonical constructor shape is the right way to keep the old public signature compiling, and Mono.fromCallable is already the correct per-subscription boundary, so consulting the factory inside the callable is right.

---

Automated review by github-manager-bot

[oss-maintainer]

[INFO] This message is "content handler cannot be null", but at this point the null argument is the factory, not a handler — testNullPerReadHandlerFactory asserts IllegalArgumentException from exactly this line. Two distinct failure modes then produce an identical message (:149 is the real handler case). Suggest "content handler factory cannot be null" here so a stack trace tells you which side misbehaved; the wording at :149 can stay as-is.

[oss-maintainer]

[INFO] singletonHandler preserves the old shared-handler behaviour silently, and there is no in-repo caller left that would trip over it (docs/v2/{en,zh}/integration/rag/simple.md:44 and every test use the default constructor). Callers who do not compile with -Xlint:deprecation therefore keep the bug with no signal. A one-time log.warn("TikaReader created with a shared ContentHandler; text will accumulate across reads — use TikaReader.perReadHandler(...) instead") here costs nothing and turns a silent data-corruption path into a visible one. Fine to skip if the deprecation warning is considered sufficient.

[oss-maintainer]

[INFO] Two small contract nits around this line:

1. The factory call sits outside the try, so a factory that throws escapes raw while every extraction failure is wrapped into ReaderException — downstream onErrorMap(ReaderException.class, ...) style handling will not see it. Wrapping the get() too (or documenting the asymmetry) keeps the error channel uniform. The null-return case asserted by testNullFromPerReadHandlerFactory is unaffected.
2. Because of this placement, perReadHandler's javadoc @throws IllegalStateException if the factory returns null during a read never throws synchronously — it arrives as onError, which is also what the Reader interface documents ("errors ... propagated through the Mono"). Suggest rewording to "signals" to avoid implying a synchronous throw.

[oss-maintainer]

Summary

Third pass, on the reworded head 4a944296 (force-pushed over 4cda8f17, which I reviewed at 04:03 UTC). Good news first: three of my four previous points are resolved in this revision, and the resolution is the one I would have picked —

• @Deprecated(since = ...) now reads 2.0.4, matching <revision>2.0.4-SNAPSHOT</revision> in pom.xml.
• Legacy path is no longer silent — singletonHandler warns once on the second subscription, which is exactly the missing signal I asked for.
• Factory-null is now distinguishable — construction-time IllegalArgumentException("handler factory cannot be null") vs read-time IllegalStateException("content handler factory returned null"), and the javadoc says "per read subscription" consistently now, so the retry/resubscribe semantics are documented correctly.

What is left is one genuinely open item and three small ones, all inline: the throwing-factory case is still not wrapped while the javadoc at :115 claims it belongs to the same error channel as everything else; the null-handler check on the legacy constructor moved behind validateChunkingParameters, which changes which message a caller gets and is the likely reason codecov still reports two uncovered lines in the file; forRemoval is undecided and the new warn-once path has no test.

Core fix remains correct and well guarded: a ContentHandler is an accumulating sink with no reset, so per-read creation is the only sound default, and testReusedReaderDoesNotAccumulateEarlierContent / testPerReadHandlerFactoryIsConsultedOnResubscription / testConcurrentReadsStayIsolated assert the actual invariant rather than the return value. Docs (docs/v2/{en,zh}/integration/rag/simple.md:44) only use the default constructor, so they stay accurate — no companion change needed there.

Not approving on this head yet: build (windows-latest) is still in progress (ubuntu build, license, module-sync, codecov/patch and license/cla are green, CLA signed). Nothing above is a blocker — once the windows build goes green, fix or document the factory-throw asymmetry and this is good to go from my side.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Minor behaviour change on the legacy constructor, easy to miss: the null-handler check now runs after validateChunkingParameters(...) (line 95) instead of before it. Pre-patch, new TikaReader(0, PARAGRAPH, 50, null) failed with "content handler cannot be null"; it now fails with the chunking message, because singletonHandler(...) (line 152) is only reached after super(...) returns — Java will not let you check the argument before the super() call here.

The only test covering this path (testNullContentHandler) uses valid chunk params, so it still passes, and codecov reports the null branch at :153 as one of the two uncovered lines.

If the reorder is intentional, no code change needed — but then the @throws IllegalArgumentException javadoc at :87 should say which check wins. Otherwise add a dedicated case:

@Test
void nullHandlerStillRejectedWithItsOwnMessage() {
    IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
            () -> new TikaReader(512, SplitStrategy.PARAGRAPH, 50, (ContentHandler) null));
    assertEquals("content handler cannot be null", e.getMessage());
}

[oss-maintainer]

[Warning] This was the one point from my previous review that is still open, and the new javadoc at :115-117 now describes behaviour the code does not implement. handlerFactory.get() sits outside the try:

• factory returns null → IllegalStateException, documented and tested (testNullFromPerReadHandlerFactory) ✅
• factory throws → the raw exception propagates and is not wrapped, while every other failure in this method becomes a ReaderException

The javadoc says "If the factory returns null or throws during a read subscription, the returned publisher fails with the corresponding error" — a caller that groups failures with onErrorMap(ReaderException.class, ...) (the pattern this very method uses at :194) will not see a throwing factory. Pick one:

// (a) uniform error channel
ContentHandler handler;
try {
    handler = handlerFactory.get();
} catch (Exception e) {
    throw new ReaderException("Content handler factory failed", e);
}
if (handler == null) {
    throw new IllegalStateException("content handler factory returned null");
}

// (b) keep the code, make the javadoc state the asymmetry
* <p>A factory that returns null fails the publisher with {@link IllegalStateException};
* a factory that throws propagates unchanged, i.e. it is not wrapped in {@link ReaderException}.

[oss-maintainer]

[Info] since = "2.0.4" now matches <revision>2.0.4-SNAPSHOT</revision> in pom.xml — that resolves the off-by-one, thanks. Two leftovers from the same thread, both non-blocking:

1. forRemoval is still undecided. Repo precedent in this module is MilvusStore.java:208 → @Deprecated(since = "1.0.11", forRemoval = true). If this constructor is meant to go once nobody needs a shared handler, commit to it here; if shared handlers stay supported permanently, a @implNote plus the warn you already added is more honest than an open-ended deprecation.
2. The warn-once path at :155-162 is the only runtime signal for legacy callers and has no test — two reads plus a Logback ListAppender asserting exactly one warning would pin both it and the uncovered :153 branch. TikaReaderTest will need @SuppressWarnings("deprecation") for the constructor call, which is also a useful signal that the tests are deliberately exercising the legacy path.

Nit only: "TikaReader is reusing one content handler across read subscriptions; ..." reads better as "...reused a single content handler across read subscriptions; text from earlier reads may have appeared in later ones..." — the warning fires on the second subscription, i.e. after the damage.

[oss-maintainer]

[Info] Javadoc layout nit: this <p> paragraph comes after the @return / @param / @throws block tags, so the generated docs render it as part of the @throws description instead of as prose. Move it above the first @param.

While you are in this block — @throws IllegalArgumentException if parameters are invalid or the factory is null is correct but only reachable through this factory method (the default constructor passes a lambda), which is a nice property of the perReadHandler(...) shape: it keeps the Supplier variant off the overload set, so new TikaReader(512, PARAGRAPH, 50, null) still resolves unambiguously to the ContentHandler constructor. Worth one line in the release notes alongside the deprecation, since that is what a caller grepping for the constructor will hit.

[oss-maintainer]

Summary

Delta re-review of the force-push (4a944296 -> 19d530e8). The only change is in perReadHandler's javadoc, and it closes both open items from my previous pass:

• the <p> block moved above the first @param, so it renders as prose instead of being folded into @throws (TikaReader.java:113-117);
• the error-channel asymmetry is now stated the way the code behaves — "returns null -> IllegalStateException", "throws -> propagates unchanged, not wrapped in ReaderException" — instead of promising a uniform failure. Documenting the asymmetry is a legitimate call; it keeps a caller-supplied factory exception distinguishable from an extraction failure. If you would rather have one error channel, wrapping the handlerFactory.get() call in a try and rethrowing as ReaderException is a two-line change, but leaving it documented is fine.

The behavioural core of the fix is unchanged and was already verified: one ContentHandler per read subscription (so no cross-document accumulation), singletonHandler(...) retaining the legacy contract with a one-time log.warn on the second call, @Deprecated(since = "2.0.4") matching pom.xml, and the factory variant kept off the overload set so new TikaReader(512, PARAGRAPH, 50, null) still resolves unambiguously to the ContentHandler constructor.

Findings

None blocking. Two non-blocking leftovers from the earlier thread, recorded for the record:

• [Info] TikaReader.java:153 (null-handler branch) and :155-162 (warn-once path) still have no test; a Logback ListAppender case asserting exactly one warning after two reads would pin both.
• [Info] forRemoval on the deprecated constructor is still undecided — repo precedent in this module is MilvusStore.java:208.

Verdict

LGTM. CLA signed, license/cla green, full CI green on 19d530e8, mergeStateStatus=CLEAN. Merging is a maintainer action; I will not merge.

---

Automated review by github-manager-bot - @oss-maintainer re-review on head 19d530e8