From cda43f19764d02fffe95fea43922a9051ed789fa Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 4 Sep 2026 16:39:24 -0700 Subject: [PATCH] filer: do not 404 a TUS session on a transient chunk-load failure (#11153) * filer: do not 404 a TUS session on a transient chunk-load failure readTusSessionInfo already proved the session exists before loadTusSessionChunks is called, so a failure there is a read failure, not evidence the session is gone: a volume-server timeout or a canceled request context surfaces through ListDirectoryEntries the same way a missing session would. Every such error was mapped to writeTusSessionNotFound, answering 404 to HEAD/PATCH and 204 to DELETE. A spec-compliant TUS client trusts that and discards the session, orphaning every chunk it had committed until the 24h expiry sweep, or forever if it never issues a DELETE. Only an error matching filer_pb.ErrNotFound is now reported as not found; anything else answers 500 so the client retries against the same session instead of abandoning it. * test: cover a TUS session's transient chunk-load failure Adds a listErr hook to the in-memory test store, alongside the existing commitErr/deleteErr, to simulate a store or RPC failure from ListDirectoryEntries. HEAD, PATCH and DELETE against a live session all answer with a server error instead of a not-found status when the chunk listing fails transiently, and the session is left on disk untouched. A listing failure that genuinely means not found, filer_pb.ErrNotFound, still answers 404 (204 for DELETE). --- weed/server/filer_grpc_server_rename_test.go | 6 ++ weed/server/filer_server_tus_handlers.go | 4 + .../server/filer_server_tus_transient_test.go | 86 +++++++++++++++++++ 3 files changed, 96 insertions(+) create mode 100644 weed/server/filer_server_tus_transient_test.go diff --git a/weed/server/filer_grpc_server_rename_test.go b/weed/server/filer_grpc_server_rename_test.go index 57757ca8d..2c36c9785 100644 --- a/weed/server/filer_grpc_server_rename_test.go +++ b/weed/server/filer_grpc_server_rename_test.go @@ -29,6 +29,7 @@ type renameTestStore struct { findCalls map[string]int commitErr error deleteErr error + listErr error // simulates a transient store/RPC failure from a directory listing findDelay time.Duration // optional: widen check-then-act windows in tests } @@ -107,6 +108,11 @@ func (s *renameTestStore) DeleteFolderChildren(_ context.Context, p util.FullPat func (s *renameTestStore) listDirectoryEntries(dirPath util.FullPath, startFileName string, includeStartFile bool, limit int64, prefix string, eachEntryFunc filer.ListEachEntryFunc) (string, error) { s.mu.Lock() + if s.listErr != nil { + err := s.listErr + s.mu.Unlock() + return "", err + } var entries []*filer.Entry for path, entry := range s.entries { if path == string(dirPath) { diff --git a/weed/server/filer_server_tus_handlers.go b/weed/server/filer_server_tus_handlers.go index e2d9aa218..a48ad142c 100644 --- a/weed/server/filer_server_tus_handlers.go +++ b/weed/server/filer_server_tus_handlers.go @@ -99,6 +99,10 @@ func (fs *FilerServer) tusHandler(w http.ResponseWriter, r *http.Request) { } if err := fs.loadTusSessionChunks(ctx, session); err != nil { glog.Errorf("Failed to load TUS session %s chunks: %v", uploadID, err) + if !errors.Is(err, filer_pb.ErrNotFound) { + http.Error(w, "Failed to load upload state", http.StatusInternalServerError) + return + } writeTusSessionNotFound(w, r.Method) return } diff --git a/weed/server/filer_server_tus_transient_test.go b/weed/server/filer_server_tus_transient_test.go new file mode 100644 index 000000000..b10dc0249 --- /dev/null +++ b/weed/server/filer_server_tus_transient_test.go @@ -0,0 +1,86 @@ +package weed_server + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// TestFilerServer_tusHandler_TransientChunkLoadFailure reproduces #11151: a +// transient failure listing a session's chunk directory (volume timeout, +// canceled context) must not be reported as 404/204, since readTusSessionInfo +// already proved the session exists. A false "not found" makes a compliant +// client discard a live session and orphan every chunk it committed. +func TestFilerServer_tusHandler_TransientChunkLoadFailure(t *testing.T) { + tests := []struct { + name string + method string + }{ + {"HEAD", http.MethodHead}, + {"PATCH", http.MethodPatch}, + {"DELETE", http.MethodDelete}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fs, store := newTusTestServer(t, map[string]string{tusTestUploadID: "/buckets/data/file.bin"}) + store.listErr = context.Canceled + + headers := map[string]string{"Authorization": "Bearer " + signFilerToken(t, tusTestWriteKey, nil, nil)} + if tt.method == http.MethodHead { + headers["Authorization"] = "Bearer " + signFilerToken(t, tusTestReadKey, nil, nil) + } + if tt.method == http.MethodPatch { + headers["Content-Type"] = "application/offset+octet-stream" + headers["Upload-Offset"] = "0" + } + req := tusRequest(tt.method, "/.tus/.uploads/"+tusTestUploadID, headers, "") + rec := httptest.NewRecorder() + fs.tusHandler(rec, req) + + if rec.Code == http.StatusNotFound || rec.Code == http.StatusNoContent { + t.Fatalf("%s on a transient chunk-load failure = %d, want a server error, not a not-found status", tt.method, rec.Code) + } + if rec.Code < 500 { + t.Errorf("%s on a transient chunk-load failure = %d, want a 5xx status", tt.method, rec.Code) + } + + store.listErr = nil + if _, err := store.FindEntry(context.Background(), util.FullPath(fs.tusSessionInfoPath(tusTestUploadID))); err != nil { + t.Errorf("session removed after a transient %s failure: %v", tt.method, err) + } + }) + } +} + +// TestFilerServer_tusHandler_ChunkLoadNotFoundStillNotFound verifies a chunk +// listing failure that genuinely means "not found" is still reported as such, +// so the fix for #11151 does not mask a real absence. +func TestFilerServer_tusHandler_ChunkLoadNotFoundStillNotFound(t *testing.T) { + fs, store := newTusTestServer(t, map[string]string{tusTestUploadID: "/buckets/data/file.bin"}) + store.listErr = filer_pb.ErrNotFound + + req := tusRequest(http.MethodHead, "/.tus/.uploads/"+tusTestUploadID, map[string]string{ + "Authorization": "Bearer " + signFilerToken(t, tusTestReadKey, nil, nil), + }, "") + rec := httptest.NewRecorder() + fs.tusHandler(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("HEAD on a genuinely missing session = %d, want %d", rec.Code, http.StatusNotFound) + } + + deleteReq := tusRequest(http.MethodDelete, "/.tus/.uploads/"+tusTestUploadID, map[string]string{ + "Authorization": "Bearer " + signFilerToken(t, tusTestWriteKey, nil, nil), + }, "") + deleteRec := httptest.NewRecorder() + fs.tusHandler(deleteRec, deleteReq) + + if deleteRec.Code != http.StatusNoContent { + t.Fatalf("DELETE on a genuinely missing session = %d, want %d", deleteRec.Code, http.StatusNoContent) + } +}