GoogleContainerTools / GoogleContainerTools/jib

Timed-out blob upload PATCH is replayed into the same upload session

Open
#4,526 0 comments 0 reactions 0 assignees View on GitHub
priority: p3
Dominant language
Java
Stars
14.5k
Forks
1.5k
PR merge metrics
No merged PRs in 30d

Description

## Description

When a registry consumes a blob upload `PATCH` but takes longer than Jib's read timeout to return
`202 Accepted`, Jib automatically sends the same `PATCH` body again to the same upload URL.

The body is replayable, but the upload operation is not necessarily idempotent. A streaming
registry can append the complete body again. The closing `PUT` then fails with `DIGEST_INVALID`
because the upload contains `blob || blob`.

This appears related to #3994, which also reports `DIGEST_INVALID` after a timed-out registry
request was retried.

## Reproduction

The following self-contained unit test reproduces the behavior on current Jib `master`
(`fb949e2676afbbd7dd7a1ef61e20251931325654`). It uses only a local JDK `HttpServer` and
synthetic data.

Add
`jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientPatchRetryTest.java`:

```java
package com.google.cloud.tools.jib.http;

import static com.google.common.truth.Truth.assertThat;

import com.google.cloud.tools.jib.blob.Blobs;
import com.google.common.io.ByteStreams;
import com.sun.net.httpserver.HttpServer;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import org.junit.Test;

public class FailoverHttpClientPatchRetryTest {

@Test
public void timeoutAfterBodyWasConsumed_doesNotReplayPatch() throws IOException {
byte[] body = "crepecake".getBytes(StandardCharsets.UTF_8);
List receivedBodies = Collections.synchronizedList(new ArrayList<>());
AtomicInteger requests = new AtomicInteger();
HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
ExecutorService executor = Executors.newCachedThreadPool();
server.setExecutor(executor);
server
.createContext("/upload")
.setHandler(
exchange -> {
receivedBodies.add(ByteStreams.toByteArray(exchange.getRequestBody()));
if (requests.getAndIncrement() == 0) {
try {
Thread.sleep(500); // Longer than the client read timeout.
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
}
}
exchange.sendResponseHeaders(202, -1);
exchange.close();
});

try {
server.start();
Request patch =
Request.builder()
.setBody(new BlobHttpContent(Blobs.from("crepecake"), "application/octet-stream"))
.setHttpTimeout(100)
.build();
IOException failure = null;
try (Response ignored =
new FailoverHttpClient(true, false, event -> {})
.call(
"PATCH",
new URL("http://localhost:" + server.getAddress().getPort() + "/upload"),
patch)) {
// A timed-out PATCH has an unknown outcome and must not be replayed blindly.
} catch (IOException ex) {
failure = ex;
}

assertThat(requests.get()).isEqualTo(1);
assertThat(receivedBodies).hasSize(1);
assertThat(receivedBodies.get(0)).isEqualTo(body);
assertThat(failure).isNotNull();
} finally {
server.stop(0);
executor.shutdownNow();
}
}
}
```

Run:

```shell
./gradlew :jib-core:test \
--tests com.google.cloud.tools.jib.http.FailoverHttpClientPatchRetryTest
```

The test fails because the server receives two complete requests:

```text
FailoverHttpClientPatchRetryTest > timeoutAfterBodyWasConsumed_doesNotReplayPatch FAILED
expected: 1
but was : 2
```

## Cause

`FailoverHttpClient` installs a generic `HttpBackOffIOExceptionHandler`.
`BlobHttpContent.retrySupported()` only checks whether the body can be read again, without taking
the HTTP method or registry upload semantics into account. `BlobPusher.Writer` sends the body with
`PATCH` and reuses the same upload URL.

## Possible solution directions

- Do not automatically retry `PATCH` in the generic HTTP I/O retry handler.
- Send explicit `Content-Length` and `Content-Range` bounds and handle an out-of-order `416`.
- After an ambiguous timeout, query the current upload offset or start a fresh upload session.

I would be happy to prepare a PR once the preferred recovery behavior is agreed.

Contributor guide

Open the contributing guide

Research direction

Start with the reproducer in jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientPatchRetryTest.java and run the Gradle command provided. Then read FailoverHttpClient, BlobHttpContent.retrySupported(), and BlobPusher.Writer to trace PATCH retry handling. Done means the agreed recovery behavior is implemented and the test no longer observes a duplicate PATCH body.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.