refactor(gax): remove circular ref between resumable upload future and chunk coordinator

[whowes]

This cleans up and helps clarify the layering and responsibility structure ahead of introducing non-happy path features.

In general the Future coordinates the overall upload lifecycle while delegating details of specific operations (e.g. chunk uploads, status listeners, global timeout) to the layer below. Actors on that layer don't maintain explicit references to the Future.

[whowes]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the resumable upload coordination by decoupling ResumableUploadChunkCoordinator from ResumableUploadFutureImpl. The coordinator now manages its own internal state and returns an ApiFuture upon starting, while ResumableUploadFutureImpl has been updated to observe this future and has removed the setInFlightFuture method. However, the review identifies two critical race conditions introduced by this refactoring: one in ResumableUploadChunkCoordinator where a cancellation of the result future after the initial check but before assigning currentChunkFuture can leak a background upload task, and another in ResumableUploadFutureImpl where a cancellation during coordinator.start() is not properly propagated to the newly created upload future. Both issues require adding explicit cancellation checks and propagation logic to prevent background resource leaks.

[gemini-code-assist]

There is a race condition where result can be cancelled after result.isDone() is checked at the beginning of transmitChunk, but before currentChunkFuture is assigned. In this scenario, the cancellation listener registered in start() will have already executed (finding currentChunkFuture to be null or a previous chunk), and the newly created chunkFuture will never be cancelled, leading to a leaked background upload task.

To prevent this, check if result has been cancelled immediately after assigning currentChunkFuture and cancel the chunk future if so.

      this.currentChunkFuture = chunkFuture;
      if (result.isCancelled()) {
        chunkFuture.cancel(true);
      }

[whowes]

Fixed.

[gemini-code-assist]

With the removal of setInFlightFuture, there is a race condition where resultFuture can be cancelled while coordinator.start() is executing. If this happens, resultFuture.isDone() will be true when entering the synchronized block, and the method will return early without cancelling the newly started uploadFuture. This can leak the upload process in the background.

To fix this, check if resultFuture was cancelled when it is done, and propagate the cancellation to uploadFuture accordingly.

            boolean shouldCancel = false;
            synchronized (lock) {
              if (resultFuture.isDone()) {
                shouldCancel = resultFuture.isCancelled();
              } else {
                inFlightFuture = uploadFuture;
              }
            }
            if (shouldCancel) {
              uploadFuture.cancel(true);
              return;
            }

References

1. When concurrent operations (such as cancellation and lazy initialization) are protected by a common lock (e.g., synchronized (this)), atomic state transitions (like compareAndSet) are not strictly necessary as the synchronization already prevents concurrent interleaving.

[whowes]

Fixed.

[sonarqubecloud]

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

Failed conditions
76.1% Coverage on New Code (required ≥ 80%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[sonarqubecloud]

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

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE