From ef5c81e2afe905a752ed9a76d255ce78efb32f44 Mon Sep 17 00:00:00 2001 From: Milos Pesic Date: Wed, 4 Nov 2026 14:15:59 +0100 Subject: [PATCH] Retry GCS requests Go's HTTP/2 client ends with its connection When Go's HTTP/2 client closes a connection, on its own write error or on the server's GOAWAY followed by a close, every open request on it fails with an error the GCS SDK's retry predicate does not match, so each of those uploads is lost. The client's predicate now adds both. Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- gcp.go | 16 ++++++++++++++++ transport_test.go | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6ef9a56..1f31a2c 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ Object storage on S3 and S3-compatible stores, GCS, Azure Blob, OCI Object Stora - **S3 semantics** where stores differ. `DeleteObjects` of a missing path succeeds, as S3's delete does. It tries every path, and when some are not deleted fails with a `*DeleteError` whose `Failed` names each one with its error (`errors.As`); every other path is deleted, and a retry is safe. - **Keys** are the store's keys as given. The local directory, as a filesystem, drops a leading `/`, cleans `//` and `.`, refuses keys outside it, and cannot hold both `a` and `a/b`. - **Timeouts**: a connection that moves no bytes either way for 5 minutes is cut. An HTTP/2 connection with no inbound frame for 30 seconds is pinged and cut when the ping goes unanswered for 15 seconds, so a dead peer fails every request on it within 45 seconds with an error the SDKs retry. The ping waits behind the frame being written, so a peer accepting less than one 16 KiB frame per 15 seconds is cut as well; any faster transfer, however slow, never is. The context bounds the call. -- **Retries**: every store retries throttling, 5xx and network errors with backoff, including AliOSS, whose SDK does not. A stream given to `UploadObject` is sent once on AliOSS. +- **Retries**: every store retries throttling, 5xx and network errors with backoff, including AliOSS, whose SDK does not. GCS also retries the errors with which Go's HTTP/2 client ends every request on a connection it is closing (force closed, or the server's GOAWAY), which the SDK does not. A stream given to `UploadObject` is sent once on AliOSS. - **Proxy**: `ProxyConfig` (S3, GCS) carries its credentials for http and https targets, honors `NO_PROXY`, and never proxies the cloud metadata services. - **Local writes** go in place, as a file is written, so mounted volumes and file watchers see one write; `IfAbsent` writes a temporary file (`.upload-*`, never listed) and links it into place. diff --git a/gcp.go b/gcp.go index ddf1301..f95ce6a 100644 --- a/gcp.go +++ b/gcp.go @@ -120,9 +120,25 @@ func NewGCP(conf *GCPConfig) (Storage, error) { if u.client, err = storage.NewClient(ctx, opts...); err != nil { return nil, err } + u.client.SetRetry(storage.WithErrorFunc(gcsShouldRetry)) return u, nil } +// gcsShouldRetry is the SDK's predicate plus the errors with which Go's HTTP/2 +// client ends every request on a connection it is closing. Such a request may +// have been received, which an overwrite or a precondition tolerates. +func gcsShouldRetry(err error) bool { + if err == nil { + return false + } + if storage.ShouldRetry(err) { + return true + } + msg := err.Error() + return strings.Contains(msg, "http2: client connection force closed") || + strings.Contains(msg, "http2: server sent GOAWAY and closed the connection") +} + func (s *gcpStorage) UploadFile(ctx context.Context, storagePath, filepath string, opts PutOptions) (string, int64, error) { file, err := os.Open(filepath) if err != nil { diff --git a/transport_test.go b/transport_test.go index 2f5ad99..91dbdc8 100644 --- a/transport_test.go +++ b/transport_test.go @@ -17,10 +17,12 @@ package storage import ( "context" "crypto/tls" + "errors" "io" "net" "net/http" "net/http/httptest" + "net/url" "os" "testing" "time" @@ -29,6 +31,52 @@ import ( "github.com/stretchr/testify/require" ) +// The errors with which Go's HTTP/2 client ends every request on a connection +// it is closing are retried on GCS, where the SDK's own predicate leaves them. +func TestGCSRetriesConnectionClose(t *testing.T) { + t.Run("server GOAWAY", func(t *testing.T) { + // The server completes the handshake, takes the request's HEADERS, + // then says goodbye with the request's stream still open and closes. + srv, client := h2Server(t, nil, func(_ *http.Server, c *tls.Conn, _ http.Handler) { + _, _ = c.Write([]byte{0, 0, 0, 0x4, 0, 0, 0, 0, 0}) // an empty SETTINGS frame + _, _ = io.ReadFull(c, make([]byte, 24)) // the client preface + hdr := make([]byte, 9) + for { + if _, err := io.ReadFull(c, hdr); err != nil { + return + } + payload := make([]byte, int(hdr[0])<<16|int(hdr[1])<<8|int(hdr[2])) + if _, err := io.ReadFull(c, payload); err != nil { + return + } + if hdr[3] == 0x1 { // HEADERS + break + } + } + // GOAWAY: last stream 1, NO_ERROR. + _, _ = c.Write([]byte{0, 0, 8, 0x7, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0}) + }) + + _, err := client.Get(srv.URL) + require.ErrorContains(t, err, "http2: server sent GOAWAY and closed the connection") + require.False(t, gcs.ShouldRetry(err), "the SDK's predicate, which this package adds to") + require.True(t, gcsShouldRetry(err)) + }) + + t.Run("client force closed", func(t *testing.T) { + err := &url.Error{Op: "Post", URL: "https://storage.googleapis.com/upload/storage/v1/b/bucket/o", + Err: errors.New("http2: client connection force closed via ClientConn.Close")} + require.False(t, gcs.ShouldRetry(err)) + require.True(t, gcsShouldRetry(err)) + }) + + t.Run("unchanged otherwise", func(t *testing.T) { + require.False(t, gcsShouldRetry(nil)) + require.False(t, gcsShouldRetry(gcs.ErrObjectNotExist)) + require.True(t, gcsShouldRetry(&url.Error{Op: "Post", URL: "https://storage.googleapis.com", Err: errors.New("read: connection reset by peer")})) + }) +} + // A dead HTTP/2 peer, one that keeps the connection open but answers nothing, // not even pings, fails the request well before the idle timeout, with an // error the GCS SDK retries. A live one that is merely slow is never cut.