From e6b312b437494ed8866490ed7be4474625596b5f Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Tue, 26 Sep 2023 09:08:25 +0200 Subject: [PATCH 1/7] fix(GODT-2949): Fix close of close channel in event service This issue is triggered due to the `Service.Close()` call after the go-routine for the event service exists. It is possible that during this period a recently added subscriber with `pendingOpAdd` gets cancelled and closed. However, the subscriber later also enqueues a `pendingOpRemove` which gets processed again with a call in `user.eventService.Close()` leading to the double close panic. This patch simply removes the `s.Close()` from the service, and leaves the cleanup to called externally from user.Close() or user.Logout(). --- internal/services/userevents/service.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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) } } From 236c958703ed2b14702019bd0a2e1ab864b01ce8 Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Tue, 26 Sep 2023 09:20:01 +0200 Subject: [PATCH 2/7] fix(GODT-2590): Fix send on closed channel Ensure periodic user tasks are terminated before the other user services. The panic triggered due to the fact that the telemetry service was shutdown before this periodic task. --- internal/user/user.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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() From bfe25e3a46c88725d9cd63a73de1b1b462c13239 Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Tue, 26 Sep 2023 09:45:27 +0200 Subject: [PATCH 3/7] fix(GODT-2951): Negative WaitGroup Counter Do not defer call to `wg.Done()` in `job.onJobFinished`. If there is an error it will also call `wg.Done()`. --- internal/services/syncservice/job.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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) } From bbe19bf960fba367cfd22e559c12e3d5f19534f5 Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Tue, 26 Sep 2023 12:47:53 +0200 Subject: [PATCH 4/7] fix(GODT-2956): Restore old deletion rules When unlabeling a message from trash we have to check if this message is present in another folder before perma-deleting. --- internal/services/imapservice/connector.go | 66 ++++++++++++++++++- tests/features/imap/message/copy.feature | 15 +++++ .../imap/message/delete_from_trash.feature | 6 +- 3 files changed, 82 insertions(+), 5 deletions(-) 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/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": From 949666724d81e7619f4f79f523c5a0e31314c327 Mon Sep 17 00:00:00 2001 From: Jakub Date: Wed, 27 Sep 2023 10:54:50 +0200 Subject: [PATCH 5/7] chore: Umshiang Bridge 3.5.1 changelog. --- Changelog.md | 9 +++++++++ Makefile | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 918165f2..9538c1f9 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,6 +3,15 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/) +## Umshiang Bridge 3.5.1 + +### Fixed +* 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 From 0d03f84711a5399c640b71dfa1dde64ba372b94e Mon Sep 17 00:00:00 2001 From: Leander Beernaert Date: Wed, 27 Sep 2023 11:30:46 +0200 Subject: [PATCH 6/7] fix(GODT-2963): Use multi error to report file removal errors Do not abort removing files on first error. Collect errors and try to remove as many as possible. This would cause some state files to not be removed on windows. --- pkg/files/removal.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 } From b7ef6e1486a69b3ed7aeb1462d7e9ad4bdeb58db Mon Sep 17 00:00:00 2001 From: Jakub Date: Wed, 27 Sep 2023 13:18:23 +0200 Subject: [PATCH 7/7] chore: Umshiang Bridge 3.5.1 changelog. --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index 9538c1f9..5dc72bc3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,7 @@ 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.