From 592d6d602134d754eff169dba7a9d9983ce55986 Mon Sep 17 00:00:00 2001
From: Chris Lu <chrislusf@users.noreply.github.com>
Date: Wed, 22 Apr 2026 17:56:15 -0700
Subject: [PATCH] fix(filer/remote): keep re-cache work alive past caller
 cancellation (#9174) (#9193)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(filer/remote): keep re-cache work alive past caller cancellation (#9174)

For multi-GB remote blobs, doCacheRemoteObjectToLocalCluster cannot
finish before the S3 gateway's initial cache wait elapses. When it
does, the gRPC ctx cancellation cascades into the filer's chunk
downloads, the error path calls DeleteUncommittedChunks on every chunk
already written, and the next retry starts over. boto3 splitting the
GET into concurrent ranges (or any client tear-down on first failure)
shortens the window between retries, so the loop never converges.

Detach the caller's ctx with context.WithoutCancel before invoking
the singleflight work so the download runs to completion regardless
of client cancellations. Subsequent waiters — via the in-flight
singleflight, or a fresh retry landing after completion — observe the
cached entry and stream normally.

Same detach pattern is used in filer_server_handlers_write.go:53 and
volume_server_handlers_write.go:51.

* simplify rationale comment

* switch to DoChan so handler can return on caller cancel

Do keeps the handler goroutine blocked for the full detached download
even after the client is gone. DoChan lets the handler select on
ctx.Done() and exit immediately; the singleflight goroutine continues
on bgCtx and the next request either joins it or finds the entry
cached.
---
 weed/server/filer_grpc_server_remote.go | 50 ++++++++++++++++---------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/weed/server/filer_grpc_server_remote.go b/weed/server/filer_grpc_server_remote.go
index eaaf0018a..e7fd0b79d 100644
--- a/weed/server/filer_grpc_server_remote.go
+++ b/weed/server/filer_grpc_server_remote.go
@@ -21,30 +21,44 @@ import (
 )
 
 func (fs *FilerServer) CacheRemoteObjectToLocalCluster(ctx context.Context, req *filer_pb.CacheRemoteObjectToLocalClusterRequest) (*filer_pb.CacheRemoteObjectToLocalClusterResponse, error) {
-	// Use singleflight to deduplicate concurrent caching requests for the same object
+	// Use singleflight to deduplicate concurrent caching requests for the same object.
 	// This benefits all clients: S3 API, filer HTTP, Hadoop, etc.
 	cacheKey := req.Directory + "/" + req.Name
 
-	result, err, shared := fs.remoteCacheGroup.Do(cacheKey, func() (interface{}, error) {
-		return fs.doCacheRemoteObjectToLocalCluster(ctx, req)
+	// Detach from caller ctx: on failure the error path deletes every chunk
+	// already written, so cancelling mid-download loses all progress. For
+	// blobs large enough that the download outlasts the caller's timeout
+	// the retry loop never converges.
+	bgCtx := context.WithoutCancel(ctx)
+
+	// DoChan (vs Do) so the caller can bail out on ctx.Done() while the
+	// singleflight goroutine keeps caching on bgCtx; otherwise this handler
+	// goroutine stays blocked for the full download after the client is gone.
+	ch := fs.remoteCacheGroup.DoChan(cacheKey, func() (interface{}, error) {
+		return fs.doCacheRemoteObjectToLocalCluster(bgCtx, req)
 	})
 
-	if shared {
-		glog.V(2).Infof("CacheRemoteObjectToLocalCluster: shared result for %s", cacheKey)
+	select {
+	case <-ctx.Done():
+		// Caller gave up; the detached cache keeps running and a later
+		// request will find the entry cached (or join the same singleflight).
+		return nil, ctx.Err()
+	case res := <-ch:
+		if res.Shared {
+			glog.V(2).Infof("CacheRemoteObjectToLocalCluster: shared result for %s", cacheKey)
+		}
+		if res.Err != nil {
+			return nil, res.Err
+		}
+		if res.Val == nil {
+			return nil, fmt.Errorf("unexpected nil result from singleflight")
+		}
+		resp, ok := res.Val.(*filer_pb.CacheRemoteObjectToLocalClusterResponse)
+		if !ok {
+			return nil, fmt.Errorf("unexpected result type from singleflight")
+		}
+		return resp, nil
 	}
-
-	if err != nil {
-		return nil, err
-	}
-	if result == nil {
-		return nil, fmt.Errorf("unexpected nil result from singleflight")
-	}
-
-	resp, ok := result.(*filer_pb.CacheRemoteObjectToLocalClusterResponse)
-	if !ok {
-		return nil, fmt.Errorf("unexpected result type from singleflight")
-	}
-	return resp, nil
 }
 
 // doCacheRemoteObjectToLocalCluster performs the actual caching operation.