history: print build errors at the end of history logs

[crazy-max]

fixes #3132
supersedes and closes #3756

docker buildx history logs now prints the stored build error after replaying the progress output. This makes errors that are not associated with a vertex, such as cache-key computation failures, visible without requiring a separate history inspect command. Error sources and failed-vertex logs use the same formatting as history inspect.

Raw JSON output remains an event-only NDJSON stream and does not perform the additional error-detail lookup. Integration coverage verifies both the plain and raw JSON behavior.

This can be reproduced with a missing build context file:

FROM scratch
COPY missing /

$ docker buildx build --progress=quiet --output=type=cacheonly .
ERROR: failed to build: failed to solve: failed to compute cache key: "/missing": not found

$ docker buildx history logs <build-ref> --progress=plain

The stored error and source location are now printed after the progress output:

Error: Unknown failed to compute cache key: failed to calculate checksum: "/missing": not found
Dockerfile:2
--------------------
   1 |     FROM scratch
   2 | >>> COPY missing /
   3 |
--------------------

Enable --debug to see stack traces for error

[tonistiigi]

Agentic review:

Review: #4087

Commit: 978e4df800ccad3ba98281a16067c15b0f6aa015
Scope: The four-file patch against its parent.
Recommendation: Request changes for the raw JSON regression and resolve the reported failure in the newly added integration test.

The main completed-build case works: the command now prints the stored error and Dockerfile location after progress output. The shared formatting extraction preserves history inspect behavior. The findings below distinguish changes introduced by this patch from existing limitations and optional design improvements.

Findings requiring changes

[P2] Skip error-detail retrieval in raw JSON mode

Changed code: commands/history/logs.go:87.

Previously, a successful status replay ended with printer.Wait(). The patch adds an unconditional loadErrorOutput call, although printLogError explicitly discards its result in raw JSON mode.

For a failed build with an external error, this performs another content read. If the error identifies a vertex, it also replays the entire status stream to collect that vertex's logs. None of those decoded details are emitted in raw JSON mode. The helper returns the stored build error as data (errOut); its separate error return represents a failure retrieving or decoding that data.

Consequently, raw JSON replay now depends on additional RPCs succeeding after the requested logs have already been printed. A failure reading the error blob or receiving the second stream changes a formerly successful replay into a command failure. Even when everything succeeds, the additional replay and allocations are unnecessary.

Fix: After waiting for the printer, return printerErr immediately for RawJSONMode, before loading error details. Add a command-level test that verifies raw JSON makes no supplemental error-detail requests; the existing formatter-only test cannot catch this.

Evidence: Confirmed by the changed control flow. Supplemental-RPC failure was not fault-injected. This finding concerns a new execution dependency, not output wording or formatting preferences.

[P2] Make the new integration test reliable on multinode workers

Changed code: tests/history.go:134.

The new test queries history using --filter=status=error. The supplied Claude review reports that this test fails on remote+multinode with:

failed to parse history filters status===error: ... unsupported operator "==="

The underlying bug predates this patch: queryRecords converts and overwrites the shared filters variable inside node goroutines. A later conversion can receive an already-converted filter. The CI matrix includes the affected worker.

The patch does not introduce that production race. Its relevance here is narrower: the newly added test invokes the affected path and is reported to fail in a supported CI configuration.

Fix: The smallest change within this patch is to omit the status filter and keep selecting the record by its unique build name, as the test already does. Alternatively, fix the shared filter mutation with appropriate regression coverage. Verify the new test on the multinode worker.

Evidence: The failure was reported by the supplied review, not independently reproduced in this review. The shared mutation and CI configuration were inspected directly. The single-node remote test passed.

Relevant observations that are not merge blockers

The shared inspect loader duplicates work for log replay

The new call to loadErrorOutput retrieves a failed-vertex log tail by opening a second status stream. The logs command has already rendered the full stream and the progress renderer's failure tail. Sharing the entire inspect loader therefore adds both another tail and another full replay.

This is directly caused by the patch's design, but no material performance regression was measured in this review. Consider sharing error decoding and source formatting while making vertex-log retrieval optional. The helper's existing post-read trimming also does not bound memory; this patch extends use of that behavior to history logs rather than introducing the helper's implementation.

Active-build summaries remain incomplete

I reproduced that attaching while RUN sleep 10; exit 1 is running produces no final error summary, while replaying the same record after completion does. The new summary reads the record snapshot obtained before streaming, whose error fields remain empty.

However, the reference documentation describes this command as printing logs for a completed build. The patch does not remove an existing active-build summary. This is an edge case in the new feature, not an established regression in documented behavior, and should not independently block this patch. Supporting it would require refreshing completion metadata after streaming.

Cleanup changes are harmless but have limited effect

The deferred printer wait improves cleanup on early returns, and repeated calls to Wait are safe. The added CloseSend calls are redundant because the generated server-streaming client already closes the send side. They do not introduce a regression and should not be treated as a blocker. Cancelling an RPC context is what releases an unfinished receive stream.

Header capitalization, gRPC code presentation, source/error ordering, and the debug hint are output-design choices. Without an agreed output contract, this review does not classify them as correctness defects.

Validation

• History and progress unit tests passed locally and in a container.
• The added TestHistoryLogsError passed with the remote worker and a freshly built binary.
• TestHistoryInspect passed with the remote worker and the same binary.
• A manual missing-file build confirmed the intended completed-build summary and source location.
• An active-build reproduction confirmed the limitation described above.
• Patch whitespace checks passed.

The full CI matrix was not run. No tracked repository files were modified during review.

[crazy-max]

@tonistiigi Addressed both P2 findings. Raw JSON now returns immediately after printer.Wait(), before loading any stored error details, so it doesn't introduce additional content reads or status replay dependencies.

The integration test no longer uses the racy status filter. It selects the failed record by its unique build name and now also verifies that raw JSON remains an event-only NDJSON stream without the appended error summary.

I left the broader queryRecords filter mutation and active-build behavior out of this change since they are pre-existing issues and not required for #3132.

[jsternberg]

What's with this change? Do we need a comment to explain this?

[crazy-max]

Removed this and the matching call in runLogs. Status is server-streaming, and the generated client already closes the send side before returning the stream, so these calls were no-ops.

[jsternberg]

Why this change?

[tonistiigi]

This is better as still carries the context values to the internal component.

[jsternberg]

This seems to bury the printer error if one occurred and it seems to me like this should just print out that it couldn't load the error output instead of returning this as an error.

[crazy-max]

Fixed. A failure now prints a diagnostic to stderr, while the command still returns printerErr, so the supplemental failure can't mask the printer result or change an otherwise successful replay into a failure.