Skip to content

feat(gax): surface actionable error messages with upload session URL and stream requirements - #14428

Draft
whowes wants to merge 1 commit into
whowes/resumable-upload-progressfrom
whowes/resumable-upload-errors
Draft

whowes wants to merge 1 commit into
whowes/resumable-upload-progressfrom
whowes/resumable-upload-errors

Conversation

@whowes

@whowes whowes commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Augments terminal failure exceptions with the active upload session URL to aid debugging and session recovery. Clarifies error messages when a server committed offset falls below the buffer base offset.

@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from 1d1b1ca to b6a2334 Compare September 17, 2026 22:08

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request refactors error handling and status tracking during resumable uploads by augmenting exceptions with the upload URL or endpoint to make them more actionable. It also simplifies synchronization in ResumableUploadChunkCoordinator, improves error messages for stream rewinding, and adds comprehensive unit tests. The review feedback highlights two critical issues: first, removing synchronization from closePayload() introduces a potential race condition on payloadClosed, which should be resolved using an explicit lock; second, recreating exceptions in augmentWithUrl discards the original stack trace, which should be preserved by copying it to the new exception instance.

Comment on lines +332 to +336
private @Nullable IOException closePayload() {
if (payloadClosed) {
return null;
}
payloadClosed = true;

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.

high

Removing the lock from closePayload() introduces a potential race condition and visibility issues on the payloadClosed field. Since this is performance-sensitive code, prefer using an explicit lock over the synchronized keyword to protect the shared state while ensuring thread safety and visibility.

  private @Nullable IOException closePayload() {
    lock.lock();
    try {
      if (payloadClosed) {
        return null;
      }
      payloadClosed = true;
    } finally {
      lock.unlock();
    }
References
  1. In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.

Comment on lines +324 to 328
if (augmented != t) {
for (Throwable suppressed : t.getSuppressed()) {
augmented.addSuppressed(suppressed);
}
payloadClosed = true;
}

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.

high

When recreating the exception with the augmented message, the original stack trace of t is lost because a new exception instance is constructed and its stack trace is initialized to the current thread's execution point. To preserve the original stack trace for easier debugging, we should copy the stack trace from the original exception t to the augmented exception.

Suggested change
if (augmented != t) {
for (Throwable suppressed : t.getSuppressed()) {
augmented.addSuppressed(suppressed);
}
payloadClosed = true;
}
if (augmented != t) {
augmented.setStackTrace(t.getStackTrace());
for (Throwable suppressed : t.getSuppressed()) {
augmented.addSuppressed(suppressed);
}
}

@whowes
whowes added this pull request to stack #14429 September 17, 2026 22:16
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from b6a2334 to cff442f Compare September 17, 2026 22:22
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from cff442f to 7719368 Compare September 17, 2026 22:23
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch 2 times, most recently from b437cfb to fa4991c Compare September 18, 2026 03:01
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from fa4991c to 51c92a7 Compare September 18, 2026 03:21
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from 51c92a7 to befccca Compare September 18, 2026 15:05
…and stream requirements

Augments terminal failure exceptions with the active upload session URL to aid debugging and session recovery. Clarifies error messages when a server committed offset falls below the buffer base offset.
@whowes
whowes force-pushed the whowes/resumable-upload-errors branch from befccca to 087add0 Compare September 19, 2026 01:37
@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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.

1 participant