Core: Automation action delay not working

[adonno]

The problem

I have a delay that isn't working sometimes on my automation. It seems to be depending on time of the day.

Environment

• Home Assistant release with the issue: 0.104.3
• Last working Home Assistant release (if known): /
• Operating environment (Hass.io/Docker/Windows/etc.): Hass.io
• Integration causing this issue:
• Link to integration documentation on our website:

Problem-relevant configuration.yaml

---
alias: Switch on lights when coming home
initial_state: true
trigger:
  - platform: state
    entity_id: person.andrea
    to: home
  - platform: state
    entity_id: person.rina
    to: home
condition:
  condition: state
  entity_id: switch.day_night  #off = night 
  state: 'off'
action:
  - service: notify.ios_iphoneadonno
    data:
      title: Welcome home
      message: Welcome
  - service: light.turn_on
    entity_id: light.hall_entree
  - service: light.turn_on
    entity_id: light.lum_ext_avant
  - delay: '00:05:00'
  - service: light.turn_off
    entity_id: light.hall_entree
  - service: light.turn_off
    entity_id: light.lum_ext_avant
#add message just to be sure delay isn't taken in to account if both notifications are within the same minute 
  - service: notify.ios_iphoneadonno
    data:
      title: Welcome home
      message: lights off

Traceback/Error logs

Additional information

[probot-home-assistant[bot]]

Hey there @home-assistant/core, mind taking a look at this issue as its been labeled with a integration (automation) you are listed as a codeowner for? Thanks!

[pnbruckner]

You automation is affected by a classic behavior of automations that many are unaware of. If an automation that was previously triggered is waiting in a delay or wait_template step when it is triggered again, the action part is not restarted. Instead the delay or wait_template is aborted and the action part immediately continues with the next step.

So, if the two person entities change to home within 5 minutes of each other, the second event will abort the delay and make the lights turn off immediately (and send the last notification.)

As far as I know, this is not a bug but rather the intended behavior. Or, at least, it's always worked this was as best as I can tell.

[adonno]

Why though, that doesn't make any sense IMHO. I would expect it to be reset and to restart waiting. Now the only reason that I would see this making sense if someone would like to force the automation to run through. But then again ... why would you put a delay then.

[pnbruckner]

Can't really argue about _why_ it works that way. As I said, as far as I know, it's always worked that way. And I don't necessarily agree with it working that way. That's just how it is and I don't think it is considered a bug. But that's just me.

BTW, feel free to browse the community forum. It's discussed there probably a million times. I know I've replied on topics related to this myself probably dozens of times.

If you want it to work a different way, then I _think_ that would be considered a feature request, which should be made on the community forum. But if you think this _is_ a bug (which I would tend to agree with), then I suppose leave this open. Although, I'm not sure how it will get resolved, especially since it would be a BREAKING CHANGE and would probably affect thousands of existing automations.

Lastly, the usual way to deal with this is to move the action steps into a script, then in the automation's action, first cancel the script (using script.turn_off), in case it was already running, then start the script.

[Misiu]

@adonno take a look at my thread: https://community.home-assistant.io/t/automation-trigger-for-state-crazy-behaviour-maybe-a-bug/140745/2
I had the exact same problem - I wasn't aware that triggering the same automation skips the delay.
I also think this is a bug.
If you want a delay then you should have a delay, automation shouldn't skip it.
If this is a feature, then at least it should be documented correctly (with bold, red text)

[adonno]

By starting/stopping scripts that are invoked by automation in order to have it working we create a workaround. If we need a workaround something is not working as expected.
I still think this is a bug. Especially if HA has to get more userfriendly, these hurdles should be avoided for "less" experienced users. But that's just me.

Now if this is an expected behavior then the documentation has to be changed.
To " yeah there is a delay but it's kinda useless" 😂
Jokes aside, The documentation should be adapted if it is not considered a bug

Edit: referring to @pnbruckner,

it's always worked that way

It's not because it worked that way that it is right or that it should work that way :joy: . We wouldn't evolve if we wouldn't do some disruptive changes sometimes.

[pnbruckner]

I'm not arguing that it is right, or that it should stay the way it is. As I've already said, I tend to agree it's a bug, but I wasn't around when it was first written that way, and since the documentation says nothing about it it's hard to tell if it was intentional or accidental.

I'm also just cautioning that it would be a rather major breaking change. But, yes, sometimes that is required, and it seems it happens on almost a daily basis in this project, so that's also not a reason not to change it.

I actually believe it would be very easy to fix. Just add this:

await self.action_script.async_stop()

before this:

https://github.com/home-assistant/home-assistant/blob/151f60658c3b677432872f715950780639fe9024/homeassistant/components/automation/__init__.py#L350

[balloob]

We should remove this behavior. We already did for scripts turn_on command.

[pnbruckner]

@balloob do you mean the change I suggested above? Will that do the trick? If so I'd be happy to submit a PR. Or is there more to it?

[balloob]

I much rather have us remove the behavior from the script helper and instead raise an error. That way it can never happen again

[pnbruckner]

I agree that changing the Script helper so that calling async_run to start it when it's already running should raise an error instead of continuing the script with the next step. I can certainly do that. (I've already looked at the code and I know how I'd implement that. It's slightly non-trivial due to the way delay and wait_template are implemented.)

However, that, by itself, doesn't solve the problem noted here. When an automation is re-triggered when the previous run hasn't completed it should not simply result in an error. It should either stop and then restart the action (i.e., Script, as I suggested above), or it should ignore the trigger event and leave the Script running where it was.

I'm not sure which is better. Actually, I think it should be possible for the user to decide by adding a new config option for automations where they could specify which behavior they want for each automation, since I don't think either would always be correct for all use cases. In that case the question then becomes which is the better default behavior. In my mind that would be restarting the script. I would think that is what most people would expect, and the behavior that would probably be the most useful or the most commonly wanted. I think there would be a lot of complaints if trigger events were missed (i.e., ignored) by default.

How about I go ahead and submit a PR with how I'd do this and we can continue the discussion there if needed?

[balloob]

Sounds good. I agree that restart makes most sense. A common use case for delay is turn light on, wait 30 minutes, turn light off. That will continue to work.

[pnbruckner]

So, as is not uncommon, after looking into doing this I discovered it's a bit more complicated than it appeared on the surface. I read through the related code extensively to make sure I understood how it currently works, and how I might go about implementing the changes. I discovered two main issues:

1. Automations actually need to have the ability to run their action sequences multiple times in parallel. Typically this would be used when the action isn't "cancellable" (i.e., it does not contain delay or wait_template.) A common use case would be when trigger events can come close together and can't be lost; e.g., when they're counted, etc. This actually "works" to some extent and I'm sure there are many, many existing automations that depend on it.
2. The current implementation of the helper Script class is seriously flawed when it comes to running multiple times in parallel. The main reason is that the running context is split between the async_run coroutine (which is fine, since it runs in a separate task for each call) and the Script object (which is _not_ fine because it is shared by each invocation of async_run.) Mostly the flaws are observed in the use case described in this issue, but there are other manifestations that are likely overlooked or subtle.

So I've spent quite a bit of time reorganizing the Script helper such that it _can_ support multiple parallel runs. The main change was to create another class -- _ScriptRun -- that it instantiates and uses for each "run." Much of the code and data attributes from Script have moved into _ScriptRun. There is a new "mode" option that determines what happens if async_run is called when a previous run hasn't completed. The options are as follows. All can be used with automations, and "error" is used for scripts (which won't call async_run multiple times in parallel anyway.)

• allow: Multiple parallel runs are allowed, whether or not they are "cancellable."
• error: An exception is raised if async_run is called before a previous run has completed.
• restart: If a previous run has not completed it is stopped before the sequence is run (again.)
• skip: The new run returns without doing anything, and the previous run continues unaltered.

To minimize "breakage" and surprises, the default mode is "restart" if the sequence contains delay or wait_template, and "allow" otherwise.

I will submit a PR with these enhancements shortly for review.

[pnbruckner]

As mentioned in closed PR, the changes will come in a set of smaller PRs. I'll reference this issue in the PR that directly affects the automation integration.

[stale[bot]]

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.
Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍
This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.