diff --git a/Changelog.md b/Changelog.md index 918165f2..5dc72bc3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,6 +3,16 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/) +## Umshiang Bridge 3.5.1 + +### Fixed +* GODT-2963: Use multi error to report file removal errors. +* GODT-2956: Restore old deletion rules. +* GODT-2951: Negative WaitGroup Counter. +* GODT-2590: Fix send on closed channel. +* GODT-2949: Fix close of close channel in event service. + + ## Umshiang Bridge 3.5.0 ### Added diff --git a/Makefile b/Makefile index 6f010be6..24d3ba27 100644 --- a/Makefile +++ b/Makefile @@ -11,7 +11,7 @@ ROOT_DIR:=$(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) .PHONY: build build-gui build-nogui build-launcher versioner hasher # Keep version hardcoded so app build works also without Git repository. -BRIDGE_APP_VERSION?=3.5.0+git +BRIDGE_APP_VERSION?=3.5.1+git APP_VERSION:=${BRIDGE_APP_VERSION} APP_FULL_NAME:=Proton Mail Bridge APP_VENDOR:=Proton AG diff --git a/internal/services/imapservice/connector.go b/internal/services/imapservice/connector.go index 03ca7dbe..e026752f 100644 --- a/internal/services/imapservice/connector.go +++ b/internal/services/imapservice/connector.go @@ -37,6 +37,7 @@ import ( "github.com/ProtonMail/proton-bridge/v3/pkg/message" "github.com/ProtonMail/proton-bridge/v3/pkg/message/parser" "github.com/bradenaw/juniper/stream" + "github.com/bradenaw/juniper/xslices" "github.com/sirupsen/logrus" "golang.org/x/exp/slices" ) @@ -334,8 +335,69 @@ func (s *Connector) RemoveMessagesFromMailbox(ctx context.Context, _ connector.I } if mboxID == proton.TrashLabel || mboxID == proton.DraftsLabel { - if err := s.client.DeleteMessage(ctx, msgIDs...); err != nil { - return err + const ChunkSize = 150 + var msgToPermaDelete []string + + rdLabels := s.labels.Read() + defer rdLabels.Close() + + // There's currently no limit on how many IDs we can filter on, + // but to be nice to API, let's chunk it by 150. + for _, messageIDs := range xslices.Chunk(messageIDs, ChunkSize) { + metadata, err := s.client.GetMessageMetadataPage(ctx, 0, ChunkSize, proton.MessageFilter{ + ID: usertypes.MapTo[imap.MessageID, string](messageIDs), + }) + if err != nil { + return err + } + + // If a message is not preset in any other label other than AllMail, AllDrafts and AllSent, it can be + // permanently deleted. + for _, m := range metadata { + var remainingLabels []string + + for _, id := range m.LabelIDs { + label, ok := rdLabels.GetLabel(id) + if !ok { + // Handle case where this label was newly introduced and we do not yet know about it. + logrus.WithField("labelID", id).Warnf("Unknown label found during expung from Trash, attempting to locate it") + label, err = s.client.GetLabel(ctx, id, proton.LabelTypeFolder, proton.LabelTypeSystem, proton.LabelTypeSystem) + if err != nil { + if errors.Is(err, proton.ErrNoSuchLabel) { + logrus.WithField("labelID", id).Warn("Label does not exist, ignoring") + continue + } + + logrus.WithField("labelID", id).Errorf("Failed to resolve label: %v", err) + return fmt.Errorf("failed to resolve label: %w", err) + } + } + if !WantLabel(label) { + continue + } + + if label.Type == proton.LabelTypeSystem && (id == proton.AllDraftsLabel || + id == proton.AllMailLabel || + id == proton.AllSentLabel || + id == proton.AllScheduledLabel) { + continue + } + + remainingLabels = append(remainingLabels, m.ID) + } + + if len(remainingLabels) == 0 { + msgToPermaDelete = append(msgToPermaDelete, m.ID) + } + } + } + + if len(msgToPermaDelete) != 0 { + logrus.Debugf("Following message(s) will be perma-deleted: %v", msgToPermaDelete) + + if err := s.client.DeleteMessage(ctx, msgToPermaDelete...); err != nil { + return err + } } } diff --git a/internal/services/syncservice/job.go b/internal/services/syncservice/job.go index 8978ae6b..cad55c90 100644 --- a/internal/services/syncservice/job.go +++ b/internal/services/syncservice/job.go @@ -113,13 +113,14 @@ func (j *Job) onStageCompleted(ctx context.Context, count int64) { } func (j *Job) onJobFinished(ctx context.Context, lastMessageID string, count int64) { - defer j.wg.Done() - if err := j.state.SetLastMessageID(ctx, lastMessageID, count); err != nil { j.log.WithError(err).Error("Failed to store last synced message id") j.onError(err) return } + + // j.onError() also calls j.wg.Done(). + j.wg.Done() j.syncReporter.OnProgress(ctx, count) } diff --git a/internal/services/userevents/service.go b/internal/services/userevents/service.go index 52e6d83d..104dfbe8 100644 --- a/internal/services/userevents/service.go +++ b/internal/services/userevents/service.go @@ -192,7 +192,6 @@ func (s *Service) run(ctx context.Context, lastEventID string) { defer s.cpc.Close() defer s.timer.Stop() defer s.log.Info("Exiting service") - defer s.Close() client := network.NewClientRetryWrapper(s.eventSource, &network.ExpCoolDown{}) @@ -303,14 +302,15 @@ func (s *Service) Close() { // Cleanup pending removes. for _, s := range s.pendingSubscriptions { - if s.op == pendingOpRemove { - if !processed.Contains(s.sub) { + if !processed.Contains(s.sub) { + processed.Add(s.sub) + + if s.op == pendingOpRemove { + s.sub.close() + } else { + s.sub.cancel() s.sub.close() } - } else { - s.sub.cancel() - s.sub.close() - processed.Add(s.sub) } } diff --git a/internal/user/user.go b/internal/user/user.go index 4e1470a6..e916bc81 100644 --- a/internal/user/user.go +++ b/internal/user/user.go @@ -589,6 +589,8 @@ func (user *User) Logout(ctx context.Context, withAPI bool) error { return fmt.Errorf("failed to remove user from imap server: %w", err) } + user.tasks.CancelAndWait() + // Stop Services user.serviceGroup.CancelAndWait() @@ -598,8 +600,6 @@ func (user *User) Logout(ctx context.Context, withAPI bool) error { // Close imap service. user.imapService.Close() - user.tasks.CancelAndWait() - if withAPI { user.log.Debug("Logging out from API") @@ -621,6 +621,9 @@ func (user *User) Logout(ctx context.Context, withAPI bool) error { func (user *User) Close() { user.log.Info("Closing user") + // Stop any ongoing background tasks. + user.tasks.CancelAndWait() + // Stop Services user.serviceGroup.CancelAndWait() @@ -630,9 +633,6 @@ func (user *User) Close() { // Close imap service. user.imapService.Close() - // Stop any ongoing background tasks. - user.tasks.CancelAndWait() - // Close the user's API client. user.client.Close() diff --git a/pkg/files/removal.go b/pkg/files/removal.go index 8f0b8db3..594f3010 100644 --- a/pkg/files/removal.go +++ b/pkg/files/removal.go @@ -72,11 +72,12 @@ func remove(dir string, except ...string) error { sort.Sort(sort.Reverse(sort.StringSlice(toRemove))) + var multiErr error for _, target := range toRemove { if err := os.RemoveAll(target); err != nil { - return err + multiErr = multierror.Append(multiErr, err) } } - return nil + return multiErr } diff --git a/tests/features/imap/message/copy.feature b/tests/features/imap/message/copy.feature index 4dd0b6c6..1b44d794 100644 --- a/tests/features/imap/message/copy.feature +++ b/tests/features/imap/message/copy.feature @@ -85,3 +85,18 @@ Feature: IMAP copy messages | from | to | subject | unread | | john.doe@mail.com | [user:user]@[domain] | foo | false | + Scenario: Move message to trash then copy to folder does not delete message + When IMAP client "1" moves the message with subject "foo" from "INBOX" to "Trash" + And it succeeds + Then IMAP client "1" eventually sees the following messages in "Trash": + | from | to | subject | unread | + | john.doe@mail.com | [user:user]@[domain] | foo | false | + When IMAP client "1" copies the message with subject "foo" from "Trash" to "Folders/mbox" + And it succeeds + When IMAP client "1" marks the message with subject "foo" as deleted + Then it succeeds + When IMAP client "1" expunges + Then it succeeds + Then IMAP client "1" eventually sees the following messages in "Folders/mbox": + | from | to | subject | unread | + | john.doe@mail.com | [user:user]@[domain] | foo | false | diff --git a/tests/features/imap/message/delete_from_trash.feature b/tests/features/imap/message/delete_from_trash.feature index a8323e91..15446eed 100644 --- a/tests/features/imap/message/delete_from_trash.feature +++ b/tests/features/imap/message/delete_from_trash.feature @@ -7,7 +7,7 @@ Feature: IMAP remove messages from Trash | label | label | Then it succeeds - Scenario Outline: Message in Trash and some other label is permanently deleted + Scenario Outline: Message in Trash and some other label is not permanently deleted Given the address "[user:user]@[domain]" of account "[user:user]" has the following messages in "Trash": | from | to | subject | body | | john.doe@mail.com | [user:user]@[domain] | foo | hello | @@ -27,8 +27,8 @@ Feature: IMAP remove messages from Trash When IMAP client "1" expunges Then it succeeds And IMAP client "1" eventually sees 1 messages in "Trash" - And IMAP client "1" eventually sees 1 messages in "All Mail" - And IMAP client "1" eventually sees 0 messages in "Labels/label" + And IMAP client "1" eventually sees 2 messages in "All Mail" + And IMAP client "1" eventually sees 1 messages in "Labels/label" Scenario Outline: Message in Trash only is permanently deleted Given the address "[user:user]@[domain]" of account "[user:user]" has the following messages in "Trash":