From ea179963c0a4c8ddd9b4bbfc27b6fe9ee3bfbf9b Mon Sep 17 00:00:00 2001 From: Bruce Zou Date: Mon, 14 Sep 2026 09:34:09 +0800 Subject: [PATCH] =?UTF-8?q?filer:=20clean=20up=20manifest=20resolve=20erro?= =?UTF-8?q?r=20propagation=20and=20add=20webdav=20tes=E2=80=A6=20(#11297)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- weed/filer/filechunk_group.go | 9 +---- weed/filer/filechunk_group_test.go | 8 +--- weed/filer/filechunks.go | 13 ------ weed/filer/stream.go | 6 ++- weed/filer/stream_manifest_error_test.go | 5 +-- weed/server/webdav_server.go | 1 - weed/server/webdav_server_test.go | 50 ++++++++++++++++++++++++ 7 files changed, 58 insertions(+), 34 deletions(-) diff --git a/weed/filer/filechunk_group.go b/weed/filer/filechunk_group.go index f920a7a7d..2a8d1894f 100644 --- a/weed/filer/filechunk_group.go +++ b/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 - // sectionsLock. Reads must fail with this error instead of silently - // zero-filling the unresolved sections as if they were sparse holes. + // resolveErr records a manifest resolution failure, guarded by sectionsLock 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 } diff --git a/weed/filer/filechunk_group_test.go b/weed/filer/filechunk_group_test.go index 3bfa4a7f9..396f52a60 100644 --- a/weed/filer/filechunk_group_test.go +++ b/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 -// fails: ReadDataAt must return an error instead of treating the unresolved -// sections as sparse holes (https://github.com/seaweedfs/seaweedfs/issues/11286). +// ReadDataAt must return an error when chunk manifest resolution fails, instead of zero-filling. 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}, }) diff --git a/weed/filer/filechunks.go b/weed/filer/filechunks.go index 1102e2fe0..8cd8bb843 100644 --- a/weed/filer/filechunks.go +++ b/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 diff --git a/weed/filer/stream.go b/weed/filer/stream.go index dcb66d765..6ac2a7375 100644 --- a/weed/filer/stream.go +++ b/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) diff --git a/weed/filer/stream_manifest_error_test.go b/weed/filer/stream_manifest_error_test.go index 349559db5..2a2e5e517 100644 --- a/weed/filer/stream_manifest_error_test.go +++ b/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 -// manifest resolution fails, instead of serving a zero-filled stream. +// Stream preparation must fail when chunk 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}, diff --git a/weed/server/webdav_server.go b/weed/server/webdav_server.go index 88d64a431..f2939243a 100644 --- a/weed/server/webdav_server.go +++ b/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 diff --git a/weed/server/webdav_server_test.go b/weed/server/webdav_server_test.go index 3e82d70c5..e33e9b8c1 100644 --- a/weed/server/webdav_server_test.go +++ b/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) +}