Upon timeout (either through internet loss or server connection loss), cinch replays already-executed messages and commands.

Zarthus commented 7 years ago

The title is a bit longwinded, so allow me to elaborate:

In the cinch timebomb plugin, the bot will re-kick users previously kicked upon disconnect.

This issue is not specific to the cinch timebomb plugin, but it is a relatively minimal example that demonstrates reproducability. In some of my custom plugins I've seen it happening as well, just on a less annoying scale.

Now lets say this plugin has kicked three or four people, and the bot disconnects because server.network.net is being restarted. Upon reconnection, the bot will proceed to re-kick all these people.

From the timebomb plugin, at one of these events:

(18:00:23) * @CinchBot (bot@bothost) Quit (*.net *.split)
(18:02:40) * CinchBot (bot@bothost) has joined #channel
(18:02:46) * ChanServ sets mode: +o CinchBot
(18:05:55) * User3 was kicked by CinchBot (*BOOM!*)
(18:06:11) * User1 was kicked by CinchBot (*BOOM!*)
(18:06:16) * User1 (User1@hostmask) has joined #channel
(18:06:19) * User1 was kicked by CinchBot (*BOOM!*)
...
(18:10:45) * User1 was kicked by CinchBot (*BOOM!*)
(18:10:50) * User1 (User1@hostmask) has joined #channel
(18:10:57) * User1 was kicked by CinchBot (*BOOM!*)
(18:11:02) * User1 (User1@hostmask) has joined #channel

This is not just limited to kicks, and I believe I've seen it happen with mode changes as well. It might be related to timers.

This issue has been around for some time, I don't know when exactly, but it seems to at least happen in 2.3.x, it just seems like nobody has been too bothered by it to report it. I've seen several cinch bots have similar behaviour, including my own.

A restart of cinch (killing the process and booting it back up again) fixes the problem.

petertseng commented 7 years ago

For me, I set start_automatically: false on Timer (passed to constructor) to deal with this

bougyman commented 7 years ago

@petertseng I've seen this happen with many timers as well. Should start_automatically: false be the default, do you think?

petertseng commented 7 years ago

start_automatically

Interesting question. I use timers for two things:

Do something on a regular basis. I usually use DSL timer method to set these. start_automatically should probably be true here. I want the bot to keep doing the thing if it gets disconnected and reconnected.
Do something a certain time in the future once (perhaps this generalises to N times). I usually use Timer Helper to set these. start_automatically should probably be false here. That will stop the bot from re-doing the thing if it gets reconnected.

I personally think unconditional start_automatically: false would trip up too many people who use timers for the first use (automatically do something periodically).

But based on those above uses of timers, one might imagine some possible solutions:

Keep the DSL timer method as-is (start_automatically: true default). Make the Helper Timer method default to start_automatically: false.
If num_shots is a finite number, start_automatically: false by default. If it's infinite, start_automatically: true by default.
Other solution I did not think of.

stop_automatically

There's an interesting snag here with the second kind of timer (do something once at some time in the future).

Let's say at t=0 I ask for a timer to fire in 300 seconds. One could reasonably expect that this timer fires at time t=300. But then the bot gets disconnected and reconnected at t=200. In that case, what should the behaviour be? If start_automatically is false, the timer never fires. If start_automatically is true, the timer fires at t=500 instead. Neither seems to be what was asked for. This seems important for one-shot timers since if it's scheduled once, someone probably cares about the time between schedule and the time between fire.

I just have my bot reschedule the timer at the correct remaining time on reconnect, but I guess I could have set stop_automatically to false instead so that disconnecting doesn't stop the timer. So Cinch doesn't need to try to do the reschedule.

So based on that, maybe stop_automatically should be given the same treatment as start_automatically (whatever that may be).

cinchrb / cinch

Upon timeout (either through internet loss or server connection loss), cinch replays already-executed messages and commands. #239