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") + } + }) +}