Refresh flags inside the transaction

Author: joeyates

lib/imap/backup/account/folder_backup.rb

@@ -29,11 +29,13 @@ def run
 
       serializer.apply_uid_validity(folder.uid_validity)
 
-      download_serializer.transaction do
+      serializer.transaction do
         downloader.run
+        FlagRefresher.new(folder, serializer).run if account.mirror_mode || refresh
       end
-
-      clean_up
+      # After the transaction the serializer will have any appended messages
+      # so we can check differences between the server and the local backup
+      LocalOnlyMessageDeleter.new(folder, raw_serializer).run if account.mirror_mode
     end
 
     private
@@ -55,34 +57,29 @@ def folder_ok?
       true
     end
 
-    def clean_up
-      LocalOnlyMessageDeleter.new(folder, serializer).run if account.mirror_mode
-      FlagRefresher.new(folder, serializer).run if account.mirror_mode || refresh
-    end
-
     def downloader
       @downloader ||= Downloader.new(
         folder,
-        download_serializer,
+        serializer,
         multi_fetch_size: account.multi_fetch_size,
         reset_seen_flags_after_fetch: account.reset_seen_flags_after_fetch
       )
     end
 
-    def download_serializer
-      @download_serializer ||=
+    def serializer
+      @serializer ||=
         case account.download_strategy
         when "direct"
-          serializer
+          raw_serializer
         when "delay_metadata"
-          Serializer::DelayedMetadataSerializer.new(serializer: serializer)
+          Serializer::DelayedMetadataSerializer.new(serializer: raw_serializer)
         else
           raise "Unknown download strategy '#{account.download_strategy}'"
         end
     end
 
-    def serializer
-      @serializer ||= Serializer.new(account.local_path, folder.name)
+    def raw_serializer
+      @raw_serializer ||= Serializer.new(account.local_path, folder.name)
     end
   end
 end

lib/imap/backup/serializer.rb

@@ -28,6 +28,7 @@ def self.folder_path_for(path:, folder:)
     extend Forwardable
 
     def_delegator :mbox, :pathname, :mbox_pathname
+    def_delegator :imap, :update
 
     # Get message metadata
     # @param uid [Integer] a message UID
@@ -77,7 +78,7 @@ def initialize(path, folder)
       @validated = nil
     end
 
-    # Calls the supplied block.
+    # Calls the supplied block without implementing transactional behaviour.
     # This method is present so that this class implements the same
     # interface as {DelayedMetadataSerializer}
     # @param block [block] the block that is wrapped by the transaction
@@ -177,18 +178,6 @@ def append(uid, message, flags)
       appender.append(uid: uid, message: message, flags: flags)
     end
 
-    # Updates a messages flags
-    # @param uid [Integer] the message's UID
-    # @param flags [Array<Symbol>] the flags to set on the message
-    # @return [void]
-    def update(uid, flags: nil)
-      message = imap.get(uid)
-      return if !message
-
-      message.flags = flags if flags
-      imap.save
-    end
-
     # Enumerates over a series of messages.
     # When called without a block, returns an Enumerator
     # @param required_uids [Array<Integer>] the UIDs of the message to enumerate over

lib/imap/backup/serializer/delayed_metadata_serializer.rb

@@ -10,7 +10,7 @@ module Imap; end
 module Imap::Backup
   class Serializer; end
 
-  # Wraps the Serializer, delaying metadata appends
+  # Wraps the Serializer, delaying metadata changes
   class Serializer::DelayedMetadataSerializer
     extend Forwardable
 
@@ -31,7 +31,7 @@ def initialize(serializer:)
     def transaction(&block)
       tsx.fail_in_transaction!(:transaction, message: "nested transactions are not supported")
 
-      tsx.begin({metadata: []}) do
+      tsx.begin({appends: [], updates: []}) do
         mbox.transaction do
           block.call
 
@@ -42,7 +42,21 @@ def transaction(&block)
       end
     end
 
-    # Appends a message to the mbox file and adds the metadata
+    # Sets the folder's UID validity via the serializer
+    #
+    # @param uid_validity [Integer] the UID validity to apply
+    # @raise [RuntimeError] if called inside a transaction
+    # @return [void]
+    def apply_uid_validity(uid_validity)
+      tsx.fail_in_transaction!(
+        :transaction,
+        message: "UID validity cannot be changed in a transaction"
+      )
+
+      serializer.apply_uid_validity(uid_validity)
+    end
+
+    # Appends a message to the mbox file and adds the appended message's metadata
     # to the transaction
     #
     # @param uid [Integer] the UID of the message
@@ -53,20 +67,34 @@ def append(uid, message, flags)
       tsx.fail_outside_transaction!(:append)
       mboxrd_message = Email::Mboxrd::Message.new(message)
       serialized = mboxrd_message.to_serialized
-      tsx.data[:metadata] << {uid: uid, length: serialized.bytesize, flags: flags}
+      tsx.data[:appends] << {uid: uid, length: serialized.bytesize, flags: flags}
       mbox.append(serialized)
     end
 
+    # Stores changes to a message's metadata for later update
+    #
+    # @param uid [Integer] the UID of the message
+    # @param length [Integer] the length of the message
+    # @param flags [Array<Symbol>] the flags for the message
+    # @return [void]
+    def update(uid, length: nil, flags: nil)
+      tsx.fail_outside_transaction!(:update)
+      tsx.data[:updates] << {uid: uid, length: length, flags: flags}
+    end
+
     private
 
     attr_reader :serializer
 
     def commit
       # rubocop:disable Lint/RescueException
       imap.transaction do
-        tsx.data[:metadata].each do |m|
+        tsx.data[:appends].each do |m|
           imap.append m[:uid], m[:length], flags: m[:flags]
         end
+        tsx.data[:updates].each do |m|
+          imap.update m[:uid], length: m[:length], flags: m[:flags]
+        end
       rescue Exception => e
         Logger.logger.error "#{self.class} handling #{e.class}"
         imap.rollback

lib/imap/backup/serializer/imap.rb

@@ -97,11 +97,27 @@ def append(uid, length, flags: [])
       save
     end
 
-    # Get message metadata
+    # Updates a message's length and/or flags
+    # @param uid [Integer] the existing message's UID
+    # @param length [Integer] the length of the message (as stored on disk)
+    # @param flags [Array[Symbol]] the message's flags
+    # @raise [RuntimeError] if the UID does not exist
+    # @return [void]
+    def update(uid, length: nil, flags: nil)
+      index = messages.find_index { |m| m.uid == uid }
+      raise "UID #{uid} not found" if !index
+
+      messages[index].length = length if length
+      messages[index].flags = flags if flags
+      save
+    end
+
+    # Get a copy of message metadata
     # @param uid [Integer] a message UID
     # @return [Serializer::Message]
     def get(uid)
-      messages.find { |m| m.uid == uid }
+      message = messages.find { |m| m.uid == uid }
+      message&.dup
     end
 
     # Deletes the metadata file
@@ -158,11 +174,15 @@ def uids
       messages.map(&:uid)
     end
 
-    # Update a message's metadata, replacing its UID
+    # Update a message's UID
     # @param old [Integer] the existing message UID
     # @param new [Integer] the new UID to apply to the message
+    # @raise [RuntimeError] if the new UID already exists
     # @return [void]
     def update_uid(old, new)
+      existing = messages.find_index { |m| m.uid == new }
+      raise "UID #{new} already exists" if existing
+
       index = messages.find_index { |m| m.uid == old }
       return if index.nil?
 

spec/features/backup_spec.rb

@@ -100,7 +100,6 @@
           write_config
           backup.run
           test_server.set_flags folder, [1], [:Seen]
-          super()
         end
 
         it "updates flags" do
@@ -182,27 +181,46 @@
 
   context "in mirror mode" do
     let(:account_config) { super().merge(mirror_mode: true) }
-    let(:imap_path) { File.join(account_config[:local_path], "Foo.imap") }
-    let(:mbox_path) { File.join(account_config[:local_path], "Foo.mbox") }
+    let(:foo_imap_path) { File.join(account_config[:local_path], "Foo.imap") }
+    let(:foo_mbox_path) { File.join(account_config[:local_path], "Foo.mbox") }
 
     let!(:setup) do
       create_directory account_config[:local_path]
-      File.write(imap_path, "existing imap")
-      File.write(mbox_path, "existing mbox")
+      File.write(foo_imap_path, "existing imap")
+      File.write(foo_mbox_path, "existing mbox")
       super()
     end
 
     context "with folders that are not being backed up" do
       it "deletes .imap files" do
         run_command_and_stop command
 
-        expect(File.exist?(imap_path)).to be false
+        expect(File.exist?(foo_imap_path)).to be false
       end
 
       it "deletes .mbox files" do
         run_command_and_stop command
 
-        expect(File.exist?(mbox_path)).to be false
+        expect(File.exist?(foo_mbox_path)).to be false
+      end
+    end
+
+    context "with messages that have been deleted from the server" do
+      let(:setup) do
+        super()
+        backup.run
+      end
+      let(:mbox_two) { to_mbox_entry(**message_two) }
+      let(:mbox_three) { to_mbox_entry(**message_three) }
+
+      it "deletes messages from the local backup" do
+        expect(mbox_content(email, folder)).to eq(messages_as_mbox)
+        test_server.delete_email folder, 1
+        test_server.send_email folder, **message_three
+
+        run_command_and_stop command
+
+        expect(mbox_content(email, folder)).to eq(mbox_two + mbox_three)
       end
     end
   end

spec/features/migrate_spec.rb

@@ -27,6 +27,7 @@
   after do
     destination_server.delete_folder source_folder
     destination_server.disconnect
+    test_server.disconnect
   end
 
   context "when a config path is supplied" do

spec/features/support/20_test_email_server.rb

@@ -34,14 +34,12 @@ def reconnect
   def disconnect
     return if !@imap
 
-    if !imap.disconnected?
-      begin
-        imap.logout
-      rescue EOFError
-        # ignore occasional error when closing connection
-      end
-      imap.disconnect
+    begin
+      imap.logout
+    rescue IOError
+      # ignore occasional error when closing connection
     end
+    imap.disconnect
 
     @imap = nil
   end

spec/unit/serializer_spec.rb

@@ -107,7 +107,8 @@ module Imap::Backup
         rename: nil,
         save: nil,
         uid_validity: existing_uid_validity,
-        "uid_validity=": nil
+        "uid_validity=": nil,
+        update: nil
       )
     end
     let(:mbox) do
@@ -247,36 +248,13 @@ module Imap::Backup
 
     describe "#update" do
       let(:flags) { [:Foo] }
-      let(:message) { instance_double(Serializer::Message, "flags=": nil) }
 
       before do
-        allow(imap).to receive(:get) { message }
-
         subject.update(33, flags: flags)
       end
 
-      it "updates the message flags" do
-        expect(message).to have_received(:flags=).with(flags)
-      end
-
-      it "saves the .imap file" do
-        expect(imap).to have_received(:save)
-      end
-
-      context "when no flags are supplied" do
-        let(:flags) { nil }
-
-        it "does not update the message flags" do
-          expect(message).to_not have_received(:flags=)
-        end
-      end
-
-      context "when the UID is not known" do
-        let(:message) { nil }
-
-        it "does not save" do
-          expect(imap).to_not have_received(:save)
-        end
+      it "updates the .imap file" do
+        expect(imap).to have_received(:update).with(33, flags: flags)
       end
     end