filer: clean up manifest resolve error propagation and add webdav tes… (#11297)

filer: clean up manifest resolve error propagation and add webdav test (#78)

Drop GitHub issue references from comments and trim verbose comments.
Replace the viewFromChunksOrErr helper with the existing
NonOverlappingVisibleIntervals + ViewFromVisibleIntervals at the stream
call sites, and add a WebDavFile.Read regression test for the manifest
resolution failure path.

Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>

weed/filer/filechunk_group.go

@@ -23,9 +23,7 @@ type ChunkGroup struct {
 	// manifestCache caches resolved chunk manifest bytes across repeated opens
 	// for the same mount. nil for non-mount callers (no caching).
 	manifestCache *ChunkManifestCache
-	// resolveErr is set when chunk manifest resolution failed, guarded by
+	// resolveErr records a manifest resolution failure, guarded by sectionsLock
-	// sectionsLock. Reads must fail with this error instead of silently
-	// zero-filling the unresolved sections as if they were sparse holes.
 	resolveErr error
 }
 
@@ -99,9 +97,6 @@ func (group *ChunkGroup) ReadDataAt(ctx context.Context, fileSize int64, buff []
 	group.sectionsLock.RLock()
 	defer group.sectionsLock.RUnlock()
 
-	// Fail fast when chunk manifest resolution failed: the sections map is
-	// empty or partial, and zero-filling it would silently return all-zero
-	// data as if the file were one big sparse hole.
 	if group.resolveErr != nil {
 		return 0, 0, group.resolveErr
 	}
@@ -247,8 +242,6 @@ func (group *ChunkGroup) SetChunks(chunks []*filer_pb.FileChunk) error {
 
 		resolvedChunks, err := resolveOneChunkManifest(context.Background(), group.lookupFn, chunk, group.cacheInvalidator, group.manifestCache)
 		if err != nil {
-			// remember the failure so ReadDataAt returns an error instead of
-			// treating the unresolved sections as sparse holes
 			group.resolveErr = err
 			return err
 		}

weed/filer/filechunk_group_test.go

@@ -434,9 +434,7 @@ func TestChunkGroup_SearchChunks(t *testing.T) {
 	}
 }
 
-// Regression test for silent zero-fill reads when chunk manifest resolution
+// ReadDataAt must return an error when chunk manifest resolution fails, instead of zero-filling.
-// fails: ReadDataAt must return an error instead of treating the unresolved
-// sections as sparse holes (https://github.com/seaweedfs/seaweedfs/issues/11286).
 func TestChunkGroup_ReadDataAt_ManifestResolveFailure(t *testing.T) {
 	lookupErr := errors.New("lookup failed")
 	lookupFn := func(ctx context.Context, fileId string) ([]string, error) {
@@ -450,21 +448,17 @@ func TestChunkGroup_ReadDataAt_ManifestResolveFailure(t *testing.T) {
 	group, err := NewChunkGroup(lookupFn, nil, chunks, 1, nil, nil)
 	assert.Error(t, err, "manifest resolution should fail")
 
-	// Reads must fail with the resolve error, not silently return zeros.
 	buff := make([]byte, 16)
 	n, _, readErr := group.ReadDataAt(context.Background(), 1<<20, buff, 0)
 	assert.ErrorIs(t, readErr, lookupErr)
 	assert.Equal(t, 0, n)
 
-	// lseek (SEEK_DATA/SEEK_HOLE) must fail too, not misreport the whole
-	// file as sparse.
 	for _, whence := range []uint32{SEEK_DATA, 4 /* SEEK_HOLE */} {
 		found, _, seekErr := group.SearchChunks(context.Background(), 0, 1<<20, whence)
 		assert.ErrorIs(t, seekErr, lookupErr, "whence %d", whence)
 		assert.False(t, found, "whence %d", whence)
 	}
 
-	// A later successful SetChunks must clear the error.
 	err = group.SetChunks([]*filer_pb.FileChunk{
 		{FileId: "2,data", Offset: 0, Size: 16},
 	})

weed/filer/filechunks.go

@@ -193,19 +193,6 @@ func ViewFromChunks(ctx context.Context, lookupFileIdFn wdclient.LookupFileIdFun
 
 }
 
-// viewFromChunksOrErr is ViewFromChunks with the manifest resolve error
-// propagated. Ignoring it yields empty chunk views and the caller zero-fills
-// the whole requested range (https://github.com/seaweedfs/seaweedfs/issues/11286).
-func viewFromChunksOrErr(ctx context.Context, lookupFileIdFn wdclient.LookupFileIdFunctionType, chunks []*filer_pb.FileChunk, offset int64, size int64) (*IntervalList[*ChunkView], error) {
-
-	visibles, err := NonOverlappingVisibleIntervals(ctx, lookupFileIdFn, chunks, offset, offset+size)
-	if err != nil {
-		return nil, err
-	}
-
-	return ViewFromVisibleIntervals(visibles, offset, size), nil
-}
-
 func ViewFromVisibleIntervals(visibles *IntervalList[*VisibleInterval], offset int64, size int64) (chunkViews *IntervalList[*ChunkView]) {
 
 	stop := offset + size

weed/filer/stream.go

@@ -171,10 +171,11 @@ func retryFetchWithFreshLocations(ctx context.Context, invalidator CacheInvalida
 
 func PrepareStreamContentWithThrottler(ctx context.Context, masterClient wdclient.HasLookupFileIdFunction, jwtFunc VolumeServerJwtFunction, chunks []*filer_pb.FileChunk, offset int64, size int64, downloadMaxBytesPs int64) (DoStreamContent, error) {
 	glog.V(4).InfofCtx(ctx, "prepare to stream content for chunks: %d", len(chunks))
-	chunkViews, err := viewFromChunksOrErr(ctx, masterClient.GetLookupFileIdFunction(), chunks, offset, size)
+	visibles, err := NonOverlappingVisibleIntervals(ctx, masterClient.GetLookupFileIdFunction(), chunks, offset, offset+size)
 	if err != nil {
 		return nil, err
 	}
+	chunkViews := ViewFromVisibleIntervals(visibles, offset, size)
 
 	fileId2Url := make(map[string][]string)
 
@@ -288,10 +289,11 @@ func PrepareStreamContentWithPrefetch(ctx context.Context, masterClient wdclient
 	}
 
 	glog.V(4).InfofCtx(ctx, "prepare to stream content with prefetch=%d for chunks: %d", prefetchAhead, len(chunks))
-	chunkViews, err := viewFromChunksOrErr(ctx, masterClient.GetLookupFileIdFunction(), chunks, offset, size)
+	visibles, err := NonOverlappingVisibleIntervals(ctx, masterClient.GetLookupFileIdFunction(), chunks, offset, offset+size)
 	if err != nil {
 		return nil, err
 	}
+	chunkViews := ViewFromVisibleIntervals(visibles, offset, size)
 
 	fileId2Url := make(map[string][]string)
 

weed/filer/stream_manifest_error_test.go

@@ -8,10 +8,9 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-// Regression test for #11286: stream preparation must fail when chunk
+// Stream preparation must fail when chunk manifest resolution fails, instead of serving a zero-filled stream.
-// manifest resolution fails, instead of serving a zero-filled stream.
 func TestPrepareStreamContent_ManifestResolveFailure(t *testing.T) {
-	master := &testMasterClient{} // no urls registered: every lookup fails
+	master := &testMasterClient{}
 
 	chunks := []*filer_pb.FileChunk{
 		{FileId: "1,1879011dc64abd40", IsChunkManifest: true, Offset: 0, Size: 1 << 20},

weed/server/webdav_server.go

@@ -570,7 +570,6 @@ func (f *WebDavFile) Read(p []byte) (readSize int, err error) {
 	if f.visibleIntervals == nil {
 		f.visibleIntervals, err = filer.NonOverlappingVisibleIntervals(f.ctx, f.fs.filerClient.GetLookupFileIdFunction(), f.entry.GetChunks(), 0, fileSize)
 		if err != nil {
-			// fail instead of streaming zeros for unresolved manifest chunks
 			return 0, err
 		}
 		f.reader = nil

weed/server/webdav_server_test.go

@@ -2,13 +2,20 @@ package weed_server
 
 import (
 	"context"
+	"fmt"
+	"net"
 	"os"
 	"testing"
 
 	"golang.org/x/net/webdav"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
 
+	"github.com/seaweedfs/seaweedfs/weed/pb"
 	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
 	"github.com/seaweedfs/seaweedfs/weed/util"
+	"github.com/seaweedfs/seaweedfs/weed/wdclient"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestToFileInfoName(t *testing.T) {
@@ -54,3 +61,46 @@ func TestFileInfoETag(t *testing.T) {
 		t.Errorf("ETag() = %v, want the stat error", err)
 	}
 }
+
+type noVolumeFiler struct {
+	filer_pb.UnimplementedSeaweedFilerServer
+}
+
+func (f *noVolumeFiler) LookupVolume(_ context.Context, _ *filer_pb.LookupVolumeRequest) (*filer_pb.LookupVolumeResponse, error) {
+	return &filer_pb.LookupVolumeResponse{LocationsMap: map[string]*filer_pb.Locations{}}, nil
+}
+
+func startFakeWebDavFiler(t *testing.T, impl filer_pb.SeaweedFilerServer) pb.ServerAddress {
+	t.Helper()
+	lis, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	srv := grpc.NewServer()
+	filer_pb.RegisterSeaweedFilerServer(srv, impl)
+	go srv.Serve(lis)
+	t.Cleanup(srv.Stop)
+	port := lis.Addr().(*net.TCPAddr).Port
+	return pb.ServerAddress(fmt.Sprintf("127.0.0.1:1.%d", port))
+}
+
+// WebDavFile.Read must fail when chunk manifest resolution fails, instead of streaming zeros.
+func TestWebDavFile_Read_ManifestResolveFailure(t *testing.T) {
+	filerAddr := startFakeWebDavFiler(t, &noVolumeFiler{})
+	fc := wdclient.NewFilerClient([]pb.ServerAddress{filerAddr}, grpc.WithTransportCredentials(insecure.NewCredentials()), "")
+	t.Cleanup(fc.Close)
+
+	fs := &WebDavFileSystem{filerClient: fc}
+	entry := &filer_pb.Entry{
+		Name:       "file",
+		Attributes: &filer_pb.FuseAttributes{FileSize: 1 << 20},
+		Chunks: []*filer_pb.FileChunk{
+			{FileId: "1,1679011dc64abd40", IsChunkManifest: true, Offset: 0, Size: 1 << 20},
+		},
+	}
+	f := &WebDavFile{fs: fs, name: "/file", entry: entry, ctx: context.Background()}
+
+	n, err := f.Read(make([]byte, 16))
+	assert.Error(t, err)
+	assert.Equal(t, 0, n)
+}