Skip to content

fix(client): report the error message of a failed async task #776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RaphaelFakhri wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(client): report the error message of a failed async task #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...ng-serve-api/src/main/java/ai/docling/serve/api/task/response/TaskStatusPollResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* - {@code taskStatus}: The current status of the task, as defined in the {@code TaskStatus} enum.
* - {@code taskPosition}: A numerical representation of the task's position in the processing queue.
* - {@code taskStatusMetadata}: Metadata providing detailed statistics and progress information (optional).
* - {@code errorMessage}: The error message reported by Docling Serve when the task failed (optional).
*/
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@lombok.extern.jackson.Jacksonized
Expand Down Expand Up @@ -79,6 +80,16 @@ public class TaskStatusPollResponse {
@Nullable
private TaskStatusMetadata taskStatusMetadata;

/**
* Error message reported by Docling Serve when the task failed.
*
* @param errorMessage the error message, or null if not available
* @return the error message, or null if not available
*/
@JsonProperty("error_message")
@Nullable
private String errorMessage;
Comment on lines +83 to +91

@edeandrea edeandrea Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field shape is right, and I want to say so explicitly because the alternative is tempting. Nullable String plus @Nullable matches the four fields already on this type; returning Optional would follow our general "Optional over null" guidance but would make this one field behave unlike its neighbors inside a Lombok wire-format DTO. Consistency is the better call here.

And on what's deliberately not here, so it doesn't look like an oversight: upstream also carries failure: PublicFailureInfo (category / message / retryable / phase). Leaving it out is correct for this PR — it's Ray-only and v1.25.0+, while error_message works everywhere we support. I've opened #778 for it, which also captures an unknown-enum-value policy question we need to settle before adding FailureCategory as a Java enum.


/**
* Builder for creating {@link TaskStatusPollResponse} instances.
* Generated by Lombok's {@code @Builder} annotation.
Expand All @@ -90,6 +101,7 @@ public class TaskStatusPollResponse {
* <li>{@code taskStatus(TaskStatus)} - Set the task status</li>
* <li>{@code taskPosition(Long)} - Set the queue position</li>
* <li>{@code taskStatusMetadata(TaskStatusMetadata)} - Set the task metadata</li>
* <li>{@code errorMessage(String)} - Set the error message</li>
* </ul>
*/
public static class Builder {
Expand Down
8 changes: 6 additions & 2 deletions ...ocling-serve-client/src/main/java/ai/docling/serve/client/operations/AsyncOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import ai.docling.serve.api.task.request.TaskResultRequest;
import ai.docling.serve.api.task.request.TaskStatusPollRequest;
import ai.docling.serve.api.task.response.TaskStatusPollResponse;
import ai.docling.serve.api.util.Utils;
import ai.docling.serve.api.util.ValidationUtils;

/**
Expand Down Expand Up @@ -128,8 +129,11 @@ private <O> CompletionStage<O> pollTaskStatus(TaskStatusPollResponse statusRespo
}

case FAILURE -> {
var errorMessage = Optional.ofNullable(statusResponse.getTaskStatusMetadata())
.map(metadata -> "Task failed: %s".formatted(metadata))
// Prefer the reason reported by Docling Serve, as the task metadata only holds document counts
var errorMessage = Optional.ofNullable(statusResponse.getErrorMessage())
.filter(Utils::isNotNullOrBlank)
.or(() -> Optional.ofNullable(statusResponse.getTaskStatusMetadata()).map(Object::toString))
.map(reason -> "Task failed: %s".formatted(reason))
.orElse("Task failed");
Comment on lines +132 to 137

@edeandrea edeandrea Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this so you don't wonder whether I skipped it: it's correct as written, including dropping the TaskStatusMetadata dump when there's a reason to show instead.

I'd actually drafted a comment asking you to keep the counts, on the theory that a failed batch would be reporting something like "3 of 10 documents failed." It isn't. FAILURE only ever gets set from an except Exception handler — per-document failures go into the result's errors list and the task still completes SUCCESS. So those counts are whatever partial progress had been recorded when the task died, not a failure tally. Swapping them for the actual reason is a straight improvement, and the dump was only ever there because nothing better was available.


yield CompletableFuture.failedStage(
Expand Down
32 changes: 32 additions & 0 deletions ...t/src/test/java/ai/docling/serve/client/AbstractDoclingServeClientAsyncExecutorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,38 @@ void failedTaskRunsEveryPollOnCustomExecutor() {
getWireMock().verify(0, getRequestedFor(urlPathEqualTo(RESULT_PATH)));
}

@Test
void failedTaskReportsTheErrorMessageOfTheServer() {
getWireMock().stubFor(
post(urlPathEqualTo("/v1/convert/source/async"))
.willReturn(okJson(taskStatus("pending")))
);

getWireMock().stubFor(
get(urlPathEqualTo(POLL_PATH))
.willReturn(okJson("""
{
"task_id": "%s",
"task_status": "failure",
"task_meta": {
"num_docs": 1,
"num_processed": 1,
"num_succeeded": 0,
"num_failed": 1
},
"error_message": "Input document is not valid"
}
""".formatted(TASK_ID)))
);
Comment on lines +165 to +180

@edeandrea edeandrea Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stub covers the case the issue describes, which is the right one to lead with. Could you add four more variations on it? Together they pin the whole decision tree in the FAILURE branch rather than one path through it.

To be clear on where these go: they belong in the new AsyncTaskFailureTests class from my other comment, next to the relocated test — not here. I'm only commenting on this line because it's the stub they're all variations of.

  1. "error_message": "" → expect Task failed: TaskStatusMetadata(...). This is the only thing that exercises the .filter(Utils::isNotNullOrBlank) guard on AsyncOperations:134. The server never emits a blank value, so if that guard ever breaks, no other test and no real deployment will catch it.
  2. error_message present, no task_meta → expect Task failed: Input document is not valid.
  3. task_meta present, no error_message → expect Task failed: TaskStatusMetadata(...). This is the exact scenario Async task failure hides the error message returned by Docling Serve #775 opens with, and it's untested both before and after the PR — failedTaskRunsEveryPollOnCustomExecutor has neither field, so it only ever proves the bare Task failed fallback.
  4. Any of the above through chunkSourceWithHybridChunkerAsync. The description says the fix covers the chunk methods and it does, since they share AsyncOperations — one assertion turns that from reasoning into coverage, and Async task failure hides the error message returned by Docling Serve #775 names the chunk methods explicitly. Worth keeping it with the other failure cases rather than in ChunkTests, since what it's checking is failure reporting, not chunking.


var future = client(null)
.convertSourceAsync(convertRequest())
.toCompletableFuture();

assertThatThrownBy(() -> future.get(10, TimeUnit.SECONDS))
.hasRootCauseMessage("Async conversion failed for task %s: Task failed: Input document is not valid".formatted(TASK_ID));
}
Comment on lines +158 to +188

@edeandrea edeandrea Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call adding coverage rather than trusting the change — one request though, on where it lives.

Could you move it into a new @Nested class AsyncTaskFailureTests in AbstractDoclingServeClientTests and drop it from here?

AbstractDoclingServeClientTests is where we exercise what the client does with an actual request/response pair over the wire, which is exactly what this test checks — that a failure poll body carrying error_message comes back out as a usable exception. It's also where the other two fixes in this release ended up: NonJsonResponseTests and UnprocessableEntityResponseTests were both added as focused @Nested classes for precisely this "server sent X, client should report Y" shape, so a third fits the groove.

This class is a narrower thing — it's all about the asyncExecutor builder option, and every other test here leans on this.executions or this.threadNames to prove work landed on the right executor. Yours passes client(null) and needs neither, which is a good sign it wants a different home.

NonJsonResponseTests (line 491) is the closest template: a @Nested class, an @AfterEach calling getWiremockServer().resetAll(), and getDoclingClient(false, true) for a WireMock-backed client. You'll want your own TASK_ID and stub helper in the new class.

Two nearby classes look like candidates but aren't, so you don't lose time on the detour: TaskTests runs against a real container via Testcontainers, and EmptyRequiredStringTests keeps its TASK_ID and taskStatus helpers private and is scoped just as tightly as this one.


@Test
void asyncOperationsWorkWithoutCustomExecutor() throws Exception {
stubTask("/v1/convert/source/async", "success");
Expand Down
1 change: 1 addition & 0 deletions docs/src/doc/docs/whats-new.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Docling Java {{ gradle.project_version }} includes important breaking changes, a

### {{ gradle.project_version }}

* **A failed async task reports the reason given by Docling Serve** — When an async conversion or chunking task failed, the exception only showed the task metadata, which holds document counts, so the cause of the failure was lost. `TaskStatusPollResponse` now has an `errorMessage` field, mapped from the `error_message` that Docling Serve returns, and the exception message includes it when it is present.

@edeandrea edeandrea Nov 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entry is accurate, but I think it will generate a bug report in a few weeks. Could you append the last sentence here?

Suggested change
* **A failed async task reports the reason given by Docling Serve** — When an async conversion or chunking task failed, the exception only showed the task metadata, which holds document counts, so the cause of the failure was lost. `TaskStatusPollResponse` now has an `errorMessage` field, mapped from the `error_message` that Docling Serve returns, and the exception message includes it when it is present.
* **A failed async task reports the reason given by Docling Serve** — When an async conversion or chunking task failed, the exception only showed the task metadata, which holds document counts, so the cause of the failure was lost. `TaskStatusPollResponse` now has an `errorMessage` field, mapped from the `error_message` that Docling Serve returns, and the exception message includes it when it is present. How much detail that message carries depends on the server's error-sanitization setting: by default Docling Serve reports a generic `Internal processing error.` for most infrastructure failures, and the underlying exception text only when it is started with `DOCLING_SERVE_DEBUG_ERROR_DETAILS=true`.

The reason: build_public_task_error in docling-jobkit returns the real exception text for only four exception types. Everything else — timeouts, OOM, internal errors — comes back as the constant "Internal processing error." unless debug detail is switched on. Someone will hit a timeout, see a message that says nothing, and reasonably assume this feature is broken.

* **New `HeadingHierarchyOptions` and `headingHierarchyOptions` on `ConvertDocumentOptions`** — When set, `headingHierarchyOptions` enables PDF section-header level inference from bookmarks, outline numbering and visual style. The helper getter `doPdfHeadingHierarchy()` is derived from the presence of these options (returns `true` when non-null, `false` otherwise), so callers only need to pass the options to enable hierarchy inference. The options class covers all docling-serve fields (`useBookmarks`, `useNumbering`, `useStyle`, `useFontStyle`, `styleSizeTolerance`, `numberingSchemes`, `maxLevel`, `bookmarkMatchThreshold`).
* **Breaking: new `DoclingServeApiProvider` SPI** — Implementations are now provided through `ai.docling.serve.api.spi.DoclingServeApiProvider`, which receives an immutable `DoclingServeApiConfig`, so new options can be added without breaking implementations. A provider can declare the options it doesn't honor via `unsupportedOptions()`, to have them ignored, logged or rejected when a caller sets them. `DoclingServeApi` has a new abstract `config()` method. `DoclingServeApiBuilderFactory`, `DoclingServeApi.DoclingApiBuilder` and `DoclingServeApi.toBuilder()` are deprecated for removal; legacy factories are still used when no provider is found. `docling-serve-api` now depends on `slf4j-api`. See [Migrating to `DoclingServeApiProvider`](docling-serve/serve-api-provider-migration.md).
* **Breaking: `DoclingServeApi.builder()` returns a `DoclingServeApiBuilder`** — It used to return the builder of the implementation through a generic `<T, B> B` return type. Fluent chains and `var` declarations are unaffected, but code declaring the builder type must be updated. Client-specific settings such as `httpClientBuilder` are available from `DoclingServeClient.builder()`, which detects the version of Jackson the same way, and the JSON mapper from `DoclingServeJackson3Client.builder()` / `DoclingServeJackson2Client.builder()`, which are now public. An API can be copied with `api.config().toBuilder()`.
Expand Down
Loading