webdav: answer 204 to a PUT that replaces a file (#12233)

* webdav: answer 204 to a PUT that replaces a file

* webdav: decide the 204 from what OpenFile actually replaced

A Stat before the write left a window where a delete or create in
between made the status wrong, and it cost a second filer lookup. The
OpenFile create path already knows whether it removed an existing
entry, so it now marks the request context and the writer maps 201 to
204 only on a real replace.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* test/webdav: cover OpenFile's own put-outcome flag for both create paths

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* webdav: take the put outcome from the create's OldEntry

A stat-then-create still races: a second writer landing between the
check and the write makes the reported status describe a state the
write never saw. The filer decides create-vs-replace under its per-path
lock and reports the previous entry in the response's metadata event,
so flag the PUT replace from OldEntry instead of a pre-write stat. The
in-place update keeps the replaced entry's crtime and inode and reuses
the filer's own chunk cleanup, so the separate removeAll goes away, and
an exclusive create now defers to the filer's atomic O_EXCL instead of
a racy local check.

* webdav: send no body with the 204 a replacing PUT answers

---------

Co-authored-by: user.email <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

weed/server/webdav_server.go

@@ -2,6 +2,7 @@ package weed_server
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -88,6 +89,33 @@ func NewWebDavServer(option *WebDavOption) (ws *WebDavServer, err error) {
 	return ws, nil
 }
 
+// putOutcomeKey carries the flag WebDavFileSystem.OpenFile sets when a PUT
+// replaced an existing entry, so the PUT can answer 204 instead of 201
+type putOutcomeKey struct{}
+
+// putOutcomeWriter turns the 201 of a PUT that replaced a file into 204
+// and swallows the status text webdav.Handler writes after it
+type putOutcomeWriter struct {
+	http.ResponseWriter
+	replaced  *bool
+	noContent bool
+}
+
+func (w *putOutcomeWriter) WriteHeader(code int) {
+	if code == http.StatusCreated && *w.replaced {
+		code = http.StatusNoContent
+		w.noContent = true
+	}
+	w.ResponseWriter.WriteHeader(code)
+}
+
+func (w *putOutcomeWriter) Write(b []byte) (int, error) {
+	if w.noContent {
+		return len(b), nil
+	}
+	return w.ResponseWriter.Write(b)
+}
+
 // an overwriting MOVE or COPY removes the destination before reading the
 // source, so a destination that contains the source removes it too; a
 // destination under the source copies itself unless the copy is shallow
@@ -218,6 +246,14 @@ func (h copyMoveGuardHandler) moveHandler(r *http.Request) http.Handler {
 }
 
 func (h copyMoveGuardHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	// a PUT that replaces a file answers 204 (RFC 9110 9.3.4); webdav.Handler
+	// always answers 201, so OpenFile reports a replace on the request
+	// context and the writer rewrites the status it sends
+	if r.Method == "PUT" {
+		replaced := new(bool)
+		w = &putOutcomeWriter{ResponseWriter: w, replaced: replaced}
+		r = r.WithContext(context.WithValue(r.Context(), putOutcomeKey{}, replaced))
+	}
 	// RFC 4918 10.6: a missing Overwrite header means T, but webdav.Handler
 	// only overwrites on MOVE when it is set (golang/go#66059)
 	if r.Method == "MOVE" && len(r.Header.Values("Overwrite")) == 0 {
@@ -471,18 +507,11 @@ func (fs *WebDavFileSystem) OpenFile(ctx context.Context, fullFilePath string, f
 		if err := fs.checkParentDir(ctx, dir); err != nil {
 			return nil, err
 		}
-		fi, err := fs.stat(ctx, fullFilePath)
-		if err == nil {
-			if flag&os.O_EXCL != 0 || fi.IsDir() {
-				return nil, os.ErrExist
-			}
-			fs.removeAll(ctx, fullFilePath, false)
-		} else if err != os.ErrNotExist {
-			return nil, err
-		}
 
+		var resp *filer_pb.CreateEntryResponse
 		err = fs.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error {
-			if err := filer_pb.CreateEntry(context.Background(), client, &filer_pb.CreateEntryRequest{
+			var err error
+			resp, err = filer_pb.CreateEntryWithResponse(context.Background(), client, &filer_pb.CreateEntryRequest{
 				Directory: dir,
 				Entry: &filer_pb.Entry{
 					Name:        name,
@@ -496,14 +525,26 @@ func (fs *WebDavFileSystem) OpenFile(ctx context.Context, fullFilePath string, f
 						TtlSec:   0,
 					},
 				},
+				OExcl:      flag&os.O_EXCL != 0,
 				Signatures: []int32{fs.signature},
-			}); err != nil {
-				return fmt.Errorf("create %s: %v", fullFilePath, err)
-			}
-			return nil
+			})
+			return err
 		})
 		if err != nil {
-			return nil, err
+			// a taken name — an exclusive create, or a file landing on a
+			// directory — is the os.ErrExist x/net/webdav answers 405 for
+			if errors.Is(err, filer_pb.ErrEntryAlreadyExists) || errors.Is(err, filer_pb.ErrExistingIsDirectory) {
+				return nil, os.ErrExist
+			}
+			return nil, fmt.Errorf("create %s: %w", fullFilePath, err)
+		}
+		// the filer fills OldEntry under its path lock while writing, so the
+		// outcome matches what the write did even against racing writers: a
+		// PUT that landed on an entry answers 204, a fresh create answers 201
+		if resp.GetMetadataEvent().GetEventNotification().GetOldEntry() != nil {
+			if replaced, ok := ctx.Value(putOutcomeKey{}).(*bool); ok {
+				*replaced = true
+			}
 		}
 		return &WebDavFile{
 			fs:          fs,

weed/server/webdav_server_test.go

@@ -98,7 +98,30 @@ func (f *createRecordingFiler) LookupDirectoryEntry(_ context.Context, req *file
 
 func (f *createRecordingFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) {
 	f.req = req
-	return &filer_pb.CreateEntryResponse{}, nil
+	p := string(util.NewFullPath(req.Directory, req.Entry.Name))
+	resp := &filer_pb.CreateEntryResponse{}
+	if f.dirs[p] && !req.Entry.IsDirectory {
+		resp.Error = "existing entry is a directory"
+		resp.ErrorCode = filer_pb.FilerError_EXISTING_IS_DIRECTORY
+		return resp, nil
+	}
+	if req.OExcl && (f.files[p] || f.dirs[p]) {
+		resp.Error = "entry already exists"
+		resp.ErrorCode = filer_pb.FilerError_ENTRY_ALREADY_EXISTS
+		return resp, nil
+	}
+	if f.files[p] {
+		resp.MetadataEvent = &filer_pb.SubscribeMetadataResponse{
+			EventNotification: &filer_pb.EventNotification{
+				OldEntry: &filer_pb.Entry{Name: req.Entry.Name},
+			},
+		}
+	}
+	if f.files == nil {
+		f.files = map[string]bool{}
+	}
+	f.files[p] = true
+	return resp, nil
 }
 
 func (f *createRecordingFiler) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest) (*filer_pb.DeleteEntryResponse, error) {
@@ -124,6 +147,16 @@ func (f *deleteRecordingFiler) DeleteEntry(_ context.Context, req *filer_pb.Dele
 	return &filer_pb.DeleteEntryResponse{}, nil
 }
 
+func (f *deleteRecordingFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) {
+	if f.existing != nil && f.existing.IsDirectory && !req.Entry.IsDirectory {
+		return &filer_pb.CreateEntryResponse{
+			Error:     "existing entry is a directory",
+			ErrorCode: filer_pb.FilerError_EXISTING_IS_DIRECTORY,
+		}, nil
+	}
+	return &filer_pb.CreateEntryResponse{}, nil
+}
+
 func startFakeWebDavFiler(t *testing.T, impl filer_pb.SeaweedFilerServer) pb.ServerAddress {
 	t.Helper()
 	lis, err := net.Listen("tcp", "127.0.0.1:0")
@@ -842,6 +875,67 @@ func TestWebDavFileSystem_OpenFile_DoesNotReplaceFolder(t *testing.T) {
 	assert.Nil(t, filer.req)
 }
 
+// putOutcomeFS mimics the filer-backed fs: creating over an existing name
+// marks the request's PUT outcome as a replace.
+type putOutcomeFS struct {
+	webdav.FileSystem
+}
+
+func (f putOutcomeFS) OpenFile(ctx context.Context, name string, flag int, perm os.FileMode) (webdav.File, error) {
+	if flag&os.O_CREATE != 0 {
+		if _, err := f.FileSystem.Stat(ctx, name); err == nil {
+			if replaced, ok := ctx.Value(putOutcomeKey{}).(*bool); ok {
+				*replaced = true
+			}
+		}
+	}
+	return f.FileSystem.OpenFile(ctx, name, flag, perm)
+}
+
+func TestCopyMoveGuardHandler_PutStatus(t *testing.T) {
+	fs := putOutcomeFS{FileSystem: webdav.NewMemFS()}
+	h := copyMoveGuardHandler{
+		fs:   fs,
+		next: &webdav.Handler{FileSystem: fs, LockSystem: webdav.NewMemLS()},
+	}
+	put := func(body string) *httptest.ResponseRecorder {
+		req := httptest.NewRequest("PUT", "http://x/a.txt", strings.NewReader(body))
+		w := httptest.NewRecorder()
+		h.ServeHTTP(w, req)
+		return w
+	}
+	assert.Equal(t, http.StatusCreated, put("one").Code)
+	w := put("two")
+	assert.Equal(t, http.StatusNoContent, w.Code)
+	assert.Empty(t, w.Body.String(), "a 204 carries no body")
+}
+
+// The 204 comes from OpenFile's own bookkeeping: creating over an existing
+// entry flags the request context, a fresh create does not.
+func TestWebDavFileSystem_OpenFile_PutOutcomeFlag(t *testing.T) {
+	filer := &createRecordingFiler{files: map[string]bool{"/old.txt": true}}
+	filerAddr := startFakeWebDavFiler(t, filer)
+	fs := &WebDavFileSystem{option: &WebDavOption{
+		Filer:          filerAddr,
+		GrpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()),
+	}}
+
+	open := func(name string) bool {
+		replaced := new(bool)
+		ctx := context.WithValue(context.Background(), putOutcomeKey{}, replaced)
+		f, err := fs.OpenFile(ctx, name, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
+		assert.NoError(t, err)
+		if f != nil {
+			assert.NoError(t, f.Close())
+		}
+		return *replaced
+	}
+
+	assert.True(t, open("/old.txt"))
+	assert.False(t, open("/new.txt"))
+	assert.True(t, open("/new.txt"), "a PUT over the entry the first create made is a replace")
+}
+
 type renameRecordingFiler struct {
 	filer_pb.UnimplementedSeaweedFilerServer
 	req  *filer_pb.AtomicRenameEntryRequest