* Race condition fixed (fixes #1861).

 * Reminders get sent at start of during period (fixes #1821)

10 files changed, 134 insertions, 102 deletions

diff --git a/debian/changelog b/debian/changelog
index 3dee2a0..21f2cbc 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+mauvealert (3.4.3) stable; urgency=low
+
+  * Fixed notification race-conditions.
+  * Reminders now get sent at the start of the next during period.
+
+ -- Patrick J Cherry <patrick@bytemark.co.uk>  Mon, 22 Aug 2011 11:58:28 +0100
+
 mauvealert (3.4.2) stable; urgency=low
 
   * Improved polling interval timing.
diff --git a/lib/mauve/alert.rb b/lib/mauve/alert.rb
index eb5f11b..c294e8c 100644
--- a/lib/mauve/alert.rb
+++ b/lib/mauve/alert.rb
@@ -392,7 +392,7 @@ module Mauve
       #
       #
       def all_raised
-        all(:raised_at.not => nil, :cleared_at => nil) - all_acknowledged
+        all(:raised_at.not => nil, :acknowledged_at => nil, :cleared_at => nil)
       end
 
       def all_acknowledged
@@ -400,7 +400,7 @@ module Mauve
       end
 
       def all_cleared
-        all(:cleared_at.not => nil) - all_acknowledged
+        all(:cleared_at.not => nil, :acknowledged_at => nil)
       end
 
       # Returns a hash of all the :urgent, :normal and :low alerts.
diff --git a/lib/mauve/alert_changed.rb b/lib/mauve/alert_changed.rb
index 83332ff..ac864a0 100644
--- a/lib/mauve/alert_changed.rb
+++ b/lib/mauve/alert_changed.rb
@@ -19,12 +19,10 @@ module Mauve
     property :remind_at, Time
     # property :updated_at, Time, :required => true
 
-    def inspect
+    def to_s
       "#<AlertChanged #{id}: alert_id #{alert_id}, for #{person}, update_type #{update_type}>"
     end
 
-    alias to_s inspect
-    
     belongs_to :alert
     
     def was_relevant=(value)
diff --git a/lib/mauve/alert_group.rb b/lib/mauve/alert_group.rb
index 114c39b..df76f40 100644
--- a/lib/mauve/alert_group.rb
+++ b/lib/mauve/alert_group.rb
@@ -95,16 +95,45 @@ module Mauve
       #
       # If there are no notifications defined. 
       #
-      if  notifications.nil?
+      if notifications.nil?
         logger.warn("No notifications found for #{self.inspect}")
         return
       end
 
       #
+      # This is where we set the reminder -- i.e. on a per-alert-group basis.
+      #
+      remind_at = notifications.inject(nil) do |reminder_time, notification|
+        this_time = notification.remind_at_next(alert)
+        if reminder_time.nil? or (!this_time.nil? and  reminder_time > this_time)
+          this_time
+        else
+          reminder_time 
+        end
+      end
+
+      #
+      # OK got the next reminder time.
+      #
+      unless remind_at.nil?
+        this_reminder = AlertChanged.new(
+          :level => level.to_s,
+          :alert_id => alert.id,
+          :person => self.name,
+          :at => Time.now,
+          :update_type => alert.update_type,
+          :remind_at => remind_at,
+          :was_relevant => true)
+
+        this_reminder.save
+      end
+
+      #
       # The notifications are specified in the config file.
       #
+      sent_to = []
       notifications.each do |notification|
-        notification.notify(alert)
+        sent_to = notification.notify(alert, sent_to)
       end
     end
 
diff --git a/lib/mauve/notification.rb b/lib/mauve/notification.rb
index c57d2a9..43de1a7 100644
--- a/lib/mauve/notification.rb
+++ b/lib/mauve/notification.rb
@@ -145,7 +145,6 @@ module Mauve
     attr_reader :thread_list
 
     def initialize(people, level)
-
       self.level = level
       self.every = 300
       self.people = people
@@ -153,7 +152,7 @@ module Mauve
 
     def logger ;  Log4r::Logger.new self.class.to_s ; end
 
-    def notify(alert)
+    def notify(alert, already_sent_to = [])
 
       if people.nil? or people.empty?
         logger.warn "No people found in for notification #{list}"
@@ -161,9 +160,7 @@ module Mauve
       end
 
       # Should we notify at all?
-      is_relevant = DuringRunner.new(Time.now, alert, &during).now?
-
-      n_sent = 0
+      return already_sent_to unless DuringRunner.new(Time.now, alert, &during).now?
 
       people.collect do |person|
         case person
@@ -176,16 +173,29 @@ module Mauve
             []
         end
       end.flatten.uniq.each do |person|
-        n_sent += 1 if person.send_alert(self.level, alert, is_relevant, remind_at_next(alert))
+        #
+        # A bit of alert de-bouncing.
+        #
+        if already_sent_to.include?(person.username)
+          logger.info("Already sent notification of #{alert} to #{person.username}")
+        else
+          Server.notification_push([person, level, alert])
+          already_sent_to << person.username
+        end
       end
 
-      return n_sent
+      return already_sent_to
     end
     
     def remind_at_next(alert)
-      return DuringRunner.new(Time.now, alert, &during).find_next(every) if alert.raised?
+      return nil unless alert.raised?
+
+      if DuringRunner.new(Time.now, alert, &during).now?
+        return DuringRunner.new(Time.now, alert, &during).find_next(every)
+      else
+        return DuringRunner.new(Time.now, alert, &during).find_next(0)
+      end
 
-      return nil
     end
 
   end
diff --git a/lib/mauve/notifier.rb b/lib/mauve/notifier.rb
index 4737c37..2853485 100644
--- a/lib/mauve/notifier.rb
+++ b/lib/mauve/notifier.rb
@@ -25,7 +25,7 @@ module Mauve
         #
         next if person.nil?
 
-        person.do_send_alert(*args) 
+        person.send_alert(*args) 
       end
     end
 
diff --git a/lib/mauve/person.rb b/lib/mauve/person.rb
index 26425da..60ad60a 100644
--- a/lib/mauve/person.rb
+++ b/lib/mauve/person.rb
@@ -86,72 +86,9 @@ module Mauve
     end 
 
     #
-    # Sends the alert, and updates when the AlertChanged database to set the next reminder.
+    # Sends the alert
     #
-    def send_alert(level, alert, is_relevant=true, remind_at=nil)
-      #
-      # First check that we've not just sent an notification to this person for
-      # this alert
-      #
-      last_reminder = AlertChanged.first(:alert => alert, :person => username, :update_type => alert.update_type, :at.gte => (Time.now - 1.minute) )
-
-      if last_reminder and last_reminder.at >= (Time.now - 1.minute)
-        #
-        #
-        logger.info("Not sending notification to #{username} for #{alert} because one has just been sent.")
-        return false
-      end
-
-      #
-      # For the love of God, do not remind about ack'd or cleared alerts.
-      #
-      if alert.acknowledged? or alert.cleared?
-        remind_at = nil
-      end
-
-      this_reminder = AlertChanged.new(
-        :level => level.to_s,
-        :alert_id => alert.id, 
-        :person => username, 
-        :at => Time.now,
-        :update_type => alert.update_type,
-        :remind_at => remind_at,
-        :was_relevant => is_relevant)
-
-      #
-      # Check to make sure that we've not got a sooner reminder set.
-      #
-      unless remind_at.nil?
-        next_reminder = AlertChanged.first(:alert => alert, :remind_at.gt => Time.now, :person => username, :update_type => alert.update_type)
-
-        if next_reminder
-          #
-          # If the reminder is further in the future than the one we're about
-          # to put on, then just update it.
-          #
-          # Otherwise if it is sooner, we don't need to create a new one.
-          #
-          if next_reminder.remind_at > remind_at
-            next_reminder.remind_at = remind_at
-            logger.info("Not inserting a new reminder, as there is already one in place sooner")
-            this_reminder = next_reminder
-          else
-            this_reminder = nil
-          end
-        end
-      end
-
-      this_reminder.save unless this_reminder.nil?
-
-      if is_relevant
-        Server.notification_push([self, level, alert])
-        return true
-      end
-
-      return false
-    end
-   
-    def do_send_alert(level, alert)
+    def send_alert(level, alert)
       now = Time.now
 
       was_suppressed = self.suppressed?
diff --git a/lib/mauve/version.rb b/lib/mauve/version.rb
index 1c9b8d7..7e94be0 100644
--- a/lib/mauve/version.rb
+++ b/lib/mauve/version.rb
@@ -1,5 +1,5 @@
 module Mauve
 
-  VERSION="3.4.2"
+  VERSION="3.4.3"
 
 end
diff --git a/test/tc_mauve_alert_changed.rb b/test/tc_mauve_alert_changed.rb
index 0e57120..3c43a2a 100644
--- a/test/tc_mauve_alert_changed.rb
+++ b/test/tc_mauve_alert_changed.rb
@@ -7,7 +7,7 @@ require 'mauve/configuration_builder'
 require 'mauve/configuration_builders'
 require 'th_mauve'
 
-class TcMauveAlertChanged < Mauve::UnitTest 
+class TcMauveAlertChanged < Mauve::UnitTest
   include Mauve
 
   def setup
@@ -51,13 +51,13 @@ EOF
     notifications = 1
 
     mins = 0
-    121.times do
+    11.times do
       mins += 1
 
       assert_equal(notifications, Server.instance.notification_buffer.length)
       assert_equal(reminders, AlertChanged.count)
 
-      Timecop.freeze(Time.now+1.minutes)    
+      Timecop.freeze(Time.now+1.minute)
 
       if mins % 5 == 0
         notifications += 1
@@ -67,10 +67,10 @@ EOF
       AlertChanged.all.each{|ac| ac.poll}
     end
 
-    # OK now clear the alert, send one notification and set an alert_changed.
+    # OK now clear the alert, send one notification and but not an alert_changed.
     alert.clear!
     notifications += 1
-    reminders     += 1
+
     assert_equal(notifications, Server.instance.notification_buffer.length)
     assert_equal(reminders,     AlertChanged.count)
 
diff --git a/test/tc_mauve_notification.rb b/test/tc_mauve_notification.rb
index 167d069..018c31a 100644
--- a/test/tc_mauve_notification.rb
+++ b/test/tc_mauve_notification.rb
@@ -195,7 +195,7 @@ alert_group("default") {
     every 10.minutes
   }
   
-  notify("test1") {
+  notify("testers") {
     every 15.minutes
   }
 
@@ -222,30 +222,81 @@ EOF
     alert.raise!
 
     assert_equal(1, Alert.count, "Wrong number of alerts saved")
+    
+    #
+    # Also make sure that only 2 notifications has been sent..
+    #
+    assert_equal(2, Server.instance.notification_buffer.size, "Wrong number of notifications sent")
 
     #
     # Although there are four clauses above for notifications, test1 should be
     # alerted in 10 minutes time, and the 15 minutes clause is ignored, since
     # 10 minutes is sooner.
     #
-    assert_equal(3, AlertChanged.count, "Wrong number of reminders inserted")
+    assert_equal(1, AlertChanged.count, "Wrong number of reminders inserted")
+
+    a = AlertChanged.first 
+    assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
+    assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
+    assert_equal(Time.now + 10.minutes, a.remind_at,"reminder time is wrong for #{a.person}")
 
     #
-    # Also make sure that only 1 notification has been sent..
+    # OK now roll the clock forward 10 minutes
+    # TODO
+
+  end
+
+
+  #
+  # Makes sure a reminder is set at the start of the notify clause.
+  #  
+  def test_reminder_is_set_at_start_of_during
+
+    config=<<EOF
+person ("test1") {
+  all { true }
+}
+
+person ("test2") {
+  all { true }
+}
+
+alert_group("default") {
+  level URGENT
+  notify("test1") {
+    every 10.minutes
+  } 
+
+  notify("test2") {
+    every 10.minutes
+    during { hours_in_day 8..10 }
+  }
+
+}
+EOF
+
+    #
+    # Wind forward until 7.55am
     #
-    assert_equal(1, Server.instance.notification_buffer.size, "Wrong number of notifications sent")
+    Timecop.freeze(Time.now + 7.hours + 55.minutes)
 
-    reminder_times = {
-      "test1" => t + 10.minutes,
-      "test2" => t + 1.hour,
-      "test3" => t + 2.hours
-    }
+    Configuration.current = ConfigurationBuilder.parse(config)
+    Server.instance.setup
+    alert = Alert.new(
+      :alert_id  => "test",
+      :source    => "test",
+      :subject   => "test"
+    )
+    alert.raise!
 
-    AlertChanged.all.each do |a|
-      assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
-      assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
-      assert_equal(reminder_times[a.person], a.remind_at,"reminder time is wrong for #{a.person}")
-    end
+    assert_equal(1, Alert.count, "Wrong number of alerts saved")
+
+    assert_equal(1, AlertChanged.count, "Wrong number of reminders inserted")
+
+    a = AlertChanged.first
+    assert_equal("urgent", a.level, "Level is wrong for #{a.person}")
+    assert_equal("raised", a.update_type, "Update type is wrong for #{a.person}")
+    assert_equal(Time.now + 5.minutes, a.remind_at,"reminder time is wrong for #{a.person}")
 
   end