255772 - whine.pl can get into an endless whining loop

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect)

Description

[Jussi Sirpoma]

whine.pl can set the run_next field to today which causes an endless mailing
(whining) loop. For me it ment about 5000 emails before mysqld was restarted and
whining removed. I'm living in an area where the first day of the week is monday
but I'm not sure how it could be a problem here. Here's some prints from
reset_timer and get_next_date subroutines:

$run_day: Mon
$run_today: 1
$run_time: 1
$now_hour: 17
$now_weekday: 1
$day_num: 1
$add_days: 0
$nextdate: 2004-08-16

The $nextdate should propably have a date which is a week from now.

$ date
Mon Aug 16 16:56:56 EEST 2004

And here's the relevant parts of the whining related tables. The schedules 4 and
6 went into the mailing loop. Schedule 1 has a query which does not return any
bugs and schedule 3 seemed to be ok.

mysql> select * from whine_schedules;
+----+---------+---------+----------+---------------------+---------------+
| id | eventid | run_day | run_time | run_next            | mailto_userid |
+----+---------+---------+----------+---------------------+---------------+
|  1 |       1 | Sun     | 12       | 2004-08-22 12:00:00 |             1 |
|  3 |       3 | MF      | 1        | 2004-08-17 01:00:00 |             1 |
|  4 |       3 | Mon     | 1        | 2004-08-16 01:00:00 |             2 |
|  6 |       3 | Mon     | 1        | 2004-08-16 01:00:00 |             5 |
+----+---------+---------+----------+---------------------+---------------+
4 rows in set (0.00 sec)

mysql> select id,eventid,onemailperbug from whine_queries;
+----+---------+---------------+
| id | eventid | onemailperbug |
+----+---------+---------------+
|  1 |       1 |             0 |
|  4 |       3 |             0 |
+----+---------+---------------+
2 rows in set (0.00 sec)

mysql> select id,owner_userid from whine_events;
+----+--------------+
| id | owner_userid |
+----+--------------+
|  1 |            1 |
|  3 |            1 |
+----+--------------+
2 rows in set (0.00 sec)

The fix could be to change this comparison in get_next_date to use <=

        if ($add_days < 0) { # it's next week
            $add_days += 7;
        }

Comment 1

[Jouni Heikniemi]

Eww!

Comment 2

[Jussi Sirpoma]

Attachment: Comparison fix

This is the fix proposed in comment 0.

Comment 3

[Erik Stambaugh]

(In reply to comment #2)
> Created an attachment (id=156273)
> Comparison fix
> 
> This is the fix proposed in comment 0.

It looks good.  I have a patch that I'm testing right now that should safeguard
against future unforseen infinite loops, and I'd like to work the two together
if possible.

Comment 4

[Jussi Sirpoma]

(In reply to comment #3)
> It looks good.  I have a patch that I'm testing right now that should safeguard
> against future unforseen infinite loops, and I'd like to work the two together
> if possible.

Go ahead. I just had the patch available and a safeguard in a cron executed
script sounds like a good idea.

Comment 5

[Erik Stambaugh]

Attachment: comparison fix plus safeguard

This is Jussi's patch with an added safeguard that stops any schedule from
being run more than once by a single execution of the program, which is
something I'd like to get rolled into place so that any more of my brainos in
whine.pl won't cause people to get 50,000 messages.

Comment 6

[Jouni Heikniemi]

Comment on attachment 156274 [details] [diff] [review]
comparison fix plus safeguard

>Index: whine.pl
>===================================================================
>+# touched by reset_timer.  If reset_timer sees one schedule more than once, it

Nit: s/one/a/

>+# sets it to NULL so it won't come up again.

"sets run_next to NULL so it won't come up again until the next execution of
whine.pl"?

>+    if (grep(/^$schedule_id$/, @seen_schedules)) {
>+        null_schedule($schedule_id);
>+    }

Where exactly do you push schedule ids into @seen_schedules? (yeah...)

Comment 7

[Erik Stambaugh]

Attachment: fixed patch

Great.	One embarrassing oversight replaced by another.  Here's one that should
actually do something.

Comment 8

[Jouni Heikniemi]

Comment on attachment 156280 [details] [diff] [review]
fixed patch

>+# @seen_schedules is a list of all of the schedules that have already been
>+# touched by reset_timer.  If reset_timer sees a schedule more than once, it
>+# sets it to NULL so it won't come up again until the next execution of
>+# whine.pl

Nit: Add a full stop at the end.


My CVS access is still dead since pserver got closed, so I can't actually try
this against the tip. If it passes tests and works for you, r=jouni with the
change above.

Comment 9

[Joel Peshkin]

checked in for erik

Comment 10

[Jussi Sirpoma]

There's still another < to change. It covers the case when run_day has been set
to a day in month:

<http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/whine.pl#652>

Also, in this case the seen check doesn't seem to work and I'm guessing it needs
a return or otherwise the date will be reset before reset_timer returns.