From c9c6e6fb1d7a9d62e4f343909d63de93b6d3b2ca Mon Sep 17 00:00:00 2001 From: Alex K Date: Fri, 4 Sep 2026 02:42:40 +0200 Subject: [PATCH] filer.remote.sync: upload files that were never replicated (#11140) An entry whose content is rewritten unchanged before it first reached the remote took the metadata-only branch, and UpdateFileMetadata returns early when the extended attributes match without checking that the object is there. shouldSendToRemote had already reported the entry as needing to be sent, so the effect was that it stayed local for as long as its content did not change, with the sync reporting healthy progress over it. Require RemoteEntry to be set before treating an update as metadata-only. Gating at the caller covers the S3, GCS and Azure clients, which share the same early return. Fixes #11139 --- weed/command/filer_remote_sync_dir.go | 26 ++++++-- weed/command/filer_remote_sync_dir_test.go | 73 ++++++++++++++++++++++ 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/weed/command/filer_remote_sync_dir.go b/weed/command/filer_remote_sync_dir.go index 4051f396e..3ab69108d 100644 --- a/weed/command/filer_remote_sync_dir.go +++ b/weed/command/filer_remote_sync_dir.go @@ -224,11 +224,9 @@ func (option *RemoteSyncOptions) makeEventProcessor(remoteStorage *remote_pb.Rem if message.NewEntry.IsDirectory { return client.WriteDirectory(dest, message.NewEntry) } - if resp.Directory == message.NewParentPath && message.OldEntry.Name == message.NewEntry.Name { - if filer.IsSameData(message.OldEntry, message.NewEntry) { - glog.V(2).Infof("update meta: %+v", resp) - return client.UpdateFileMetadata(dest, message.OldEntry, message.NewEntry) - } + if isMetadataOnlyUpdate(resp.Directory, message) { + glog.V(2).Infof("update meta: %+v", resp) + return client.UpdateFileMetadata(dest, message.OldEntry, message.NewEntry) } glog.V(2).Infof("update: %+v", resp) glog.V(0).Infof("delete %s", remote_storage.FormatLocation(oldDest)) @@ -305,6 +303,24 @@ func toRemoteStorageLocation(mountDir, sourcePath util.FullPath, remoteMountLoca } } +// isMetadataOnlyUpdate reports whether an update to an existing entry can be +// applied to the remote by rewriting metadata alone, instead of deleting the +// old object and writing the new content. +// +// It requires the object to already be on the remote. A nil RemoteEntry means +// it never got there, and the metadata path would return without ever writing +// it, leaving the entry unreplicated for as long as its content stays the same +// -- shouldSendToRemote has already reported that this entry needs sending. +func isMetadataOnlyUpdate(dir string, message *filer_pb.EventNotification) bool { + if dir != message.NewParentPath || message.OldEntry.Name != message.NewEntry.Name { + return false + } + if !filer.IsSameData(message.OldEntry, message.NewEntry) { + return false + } + return message.NewEntry.RemoteEntry != nil +} + func shouldSendToRemote(entry *filer_pb.Entry) bool { if entry.RemoteEntry == nil { return true diff --git a/weed/command/filer_remote_sync_dir_test.go b/weed/command/filer_remote_sync_dir_test.go index 7b73038d0..8dfa9112b 100644 --- a/weed/command/filer_remote_sync_dir_test.go +++ b/weed/command/filer_remote_sync_dir_test.go @@ -328,3 +328,76 @@ func TestRewriteVersionedSourcePath(t *testing.T) { }) } } + +// TestMetadataOnlyUpdateRequiresRemoteEntry covers the case where a file is +// rewritten with identical content before it was ever replicated. +// +// shouldSendToRemote reports such an entry as needing to be sent, because its +// RemoteEntry is nil. Routing it to the metadata path discards that: the S3, +// GCS and Azure UpdateFileMetadata implementations all return early when the +// extended attributes are unchanged, without checking whether the object is on +// the remote at all. The entry then stays unreplicated for as long as its +// content does not change, while the sync reports healthy progress over it. +func TestMetadataOnlyUpdateRequiresRemoteEntry(t *testing.T) { + const dir = "/buckets/media" + + entry := func(remote *filer_pb.RemoteEntry) *filer_pb.Entry { + return &filer_pb.Entry{ + Name: "output.pdf", + Content: []byte("same bytes"), + RemoteEntry: remote, + } + } + replicated := &filer_pb.RemoteEntry{StorageName: "b2", RemoteETag: "abc", RemoteSize: 10} + + t.Run("never replicated falls through to the write path", func(t *testing.T) { + message := &filer_pb.EventNotification{ + NewParentPath: dir, + OldEntry: entry(nil), + NewEntry: entry(nil), + } + if !shouldSendToRemote(message.NewEntry) { + t.Fatal("an entry with no RemoteEntry should be eligible for sending") + } + if isMetadataOnlyUpdate(dir, message) { + t.Error("expected a content write, not a metadata-only update, for an entry that was never replicated") + } + }) + + t.Run("already replicated stays on the metadata path", func(t *testing.T) { + message := &filer_pb.EventNotification{ + NewParentPath: dir, + OldEntry: entry(replicated), + NewEntry: entry(replicated), + } + if !isMetadataOnlyUpdate(dir, message) { + t.Error("unchanged content on a replicated object should not be rewritten") + } + }) + + t.Run("changed content is written even when replicated", func(t *testing.T) { + newEntry := entry(replicated) + newEntry.Content = []byte("different bytes") + message := &filer_pb.EventNotification{ + NewParentPath: dir, + OldEntry: entry(replicated), + NewEntry: newEntry, + } + if isMetadataOnlyUpdate(dir, message) { + t.Error("changed content must take the write path") + } + }) + + t.Run("rename is written rather than updated in place", func(t *testing.T) { + renamed := entry(replicated) + renamed.Name = "renamed.pdf" + message := &filer_pb.EventNotification{ + NewParentPath: dir, + OldEntry: entry(replicated), + NewEntry: renamed, + } + if isMetadataOnlyUpdate(dir, message) { + t.Error("a rename changes the remote key and must take the write path") + } + }) +}