diff --git a/internal/session/errors.go b/internal/session/errors.go index fb4291bd..82a8b540 100644 --- a/internal/session/errors.go +++ b/internal/session/errors.go @@ -37,6 +37,8 @@ func shouldReportIMAPCommandError(err error) bool { return false case errors.Is(err, rfc822.ErrNoSuchPart): return false + case errors.Is(err, state.ErrKnownRecoveredMessage): + return false } return true diff --git a/internal/state/actions.go b/internal/state/actions.go index 797d0d19..9b11c8f7 100644 --- a/internal/state/actions.go +++ b/internal/state/actions.go @@ -192,23 +192,23 @@ func (state *State) actionCreateRecoveredMessage( literal []byte, flags imap.FlagSet, date time.Time, -) error { +) (bool, error) { internalID := imap.NewInternalMessageID() remoteID := ids.NewRecoveredRemoteMessageID(internalID) parsedMessage, err := imap.NewParsedMessage(literal) if err != nil { - return err + return false, err } alreadyKnown, err := state.user.GetRecoveredMessageHashesMap().Insert(internalID, literal) if err == nil && alreadyKnown { // Message is already known to us, so we ignore it. - return nil + return true, nil } if err := state.user.GetStore().SetUnchecked(internalID, bytes.NewReader(literal)); err != nil { - return fmt.Errorf("failed to store message literal: %w", err) + return false, fmt.Errorf("failed to store message literal: %w", err) } req := db.CreateMessageReq{ @@ -228,7 +228,7 @@ func (state *State) actionCreateRecoveredMessage( messageUID, flagSet, err := db.CreateAndAddMessageToMailbox(ctx, tx, recoveryMBoxID.InternalID, &req) if err != nil { - return err + return false, err } if err := state.user.QueueOrApplyStateUpdate(ctx, tx, newExistsStateUpdateWithExists( @@ -236,10 +236,10 @@ func (state *State) actionCreateRecoveredMessage( []*exists{newExists(ids.MessageIDPair{InternalID: internalID, RemoteID: remoteID}, messageUID, flagSet)}, nil, )); err != nil { - return err + return false, err } - return nil + return false, nil } func (state *State) actionAddMessagesToMailbox( diff --git a/internal/state/mailbox.go b/internal/state/mailbox.go index e11d53d2..f3cccf09 100644 --- a/internal/state/mailbox.go +++ b/internal/state/mailbox.go @@ -2,6 +2,8 @@ package state import ( "context" + "errors" + "fmt" "strings" "time" @@ -231,15 +233,22 @@ func (m *Mailbox) AppendRegular(ctx context.Context, literal []byte, flags imap. }) } +var ErrKnownRecoveredMessage = errors.New("known recovered message, possible duplication") + func (m *Mailbox) Append(ctx context.Context, literal []byte, flags imap.FlagSet, date time.Time) (imap.UID, error) { uid, err := m.AppendRegular(ctx, literal, flags, date) if err != nil { // Failed to append to mailbox attempt to insert into recovery mailbox. - if err := m.state.db().Write(ctx, func(ctx context.Context, tx *ent.Tx) error { + knownMessage, recoverErr := db.WriteResult(ctx, m.state.db(), func(ctx context.Context, tx *ent.Tx) (bool, error) { return m.state.actionCreateRecoveredMessage(ctx, tx, literal, flags, date) - }); err != nil { - logrus.WithError(err).Error("Failed to insert message into recovery mailbox") - reporter.ExceptionWithContext(ctx, "Failed to insert message into recovery mailbox", reporter.Context{"error": err}) + }) + if recoverErr != nil && !knownMessage { + logrus.WithError(recoverErr).Error("Failed to insert message into recovery mailbox") + reporter.ExceptionWithContext(ctx, "Failed to insert message into recovery mailbox", reporter.Context{"error": recoverErr}) + } + + if knownMessage { + err = fmt.Errorf("%v: %w", err, ErrKnownRecoveredMessage) } } diff --git a/tests/recovery_mailbox_test.go b/tests/recovery_mailbox_test.go index e18490c3..9a4af26c 100644 --- a/tests/recovery_mailbox_test.go +++ b/tests/recovery_mailbox_test.go @@ -238,6 +238,38 @@ func TestFailedAppendAreDedupedInRecoveryMailbox(t *testing.T) { }) } +func TestRecoveryMailboxOnlyReportsOnFirstDedupedMessage(t *testing.T) { + runOneToOneTestClientWithAuth(t, defaultServerOptions(t, withConnectorBuilder(&failAppendLabelConnectorBuilder{})), func(client *client.Client, s *testSession) { + { + status, err := client.Status(ids.GluonRecoveryMailboxName, []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(0), status.Messages) + } + + status, err := client.Select("INBOX", false) + require.NoError(t, err) + require.Equal(t, uint32(0), status.Messages) + require.Error(t, doAppendWithClient(client, "INBOX", "To: Foo@bar.com", time.Now())) + require.Error(t, doAppendWithClient(client, "INBOX", "To: Foo@bar.com", time.Now())) + + { + status, err := client.Status(ids.GluonRecoveryMailboxName, []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(1), status.Messages) + } + { + status, err := client.Status("INBOX", []goimap.StatusItem{goimap.StatusMessages}) + require.NoError(t, err) + require.Equal(t, uint32(0), status.Messages) + } + + { + reports := s.reporter.getReports() + require.Equal(t, 1, len(reports)) + } + }) +} + func TestRecoveryMBoxCanBeCopiedOutOfDedup(t *testing.T) { runOneToOneTestClientWithAuth(t, defaultServerOptions(t, withConnectorBuilder(&recoveryDedupConnectorConnectorBuilder{})), func(client *client.Client, s *testSession) { // Insert first message, fails.