Ensure scripts with timeouts of zero timeout immediately

[bdraco]

Proposed change

Ensure scripts with timeouts of zero timeout immediately. A zero wait timeout was seen as no timeout

This fixes a regression in 2024.4.x from https://github.com/home-assistant/core/pull/113183/files#r1571980213 and restores the behavior to what it was with 2024.3.x.

It was not an intentional to have no timeout when its set to zero and this likely breaks some use cases where the user was expecting the condition to be either true immediately or timeout. Its been pointed out that #109586 requested that a zero timeout be treated as no timeout but it seems the reasoning for that is that there is no way to turn off the timeout in the UI once its been chosen. From a backend perspective it makes more sense to me to fix the regression, and in the future adjust the UI so the timeout can be removed without having to manually edit in YAML to remove it.

fixes #115786

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• I have followed the perfect PR recommendations
• The code has been formatted using Ruff (ruff format homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[karwosts]

We may want to consider the effect this has on the frontend, it may be confusing.

Currently no timeout is rendered as 00:00:00:000

e.g.: wait_for_trigger: [] displays as:

So currently timeout: 0 and no timeout have the same behavior, and both render the same.

If we make timeout: 0 act as immediate timeout, this means that both no timeout, and instant timeout will both render identically in the frontend UI, but will have different behavior.

There may be people with legacy automations saved that accidentally touched this control, or changed it back to 0, and may be lingering unaware in their yaml. For those this would be a breaking change?

[bdraco]

I'm pretty sure this is a regression in 2024.4.x and
a zero timeout was immediate in 2024.3.x

Plan is to validate that before I mark it ready, but I can't
do that right now because I'm in flight

[bdraco]

I did manage to get enough bandwidth to downgrade to 2024.3.3 in flight
and confirmed 0 does finish right away so this is fixing a regression in
2024.4.x

[bdraco]

I did not consider the zero use case when I refactored the
script helper timeout handling in 2024.4.x.

I guess that means we did not ever consider it on the frontend
either?

[bdraco]

#109586 looks like they want the behavior the other way

Its not clear what the correct behavior is here I'm going to close
this PR and leave it as-is

[bdraco]

Also not clear is if we did timeout right away with zero, should the template/trigger still fire if the condition is true or should the timeout force it to timeout right away. With the wait template it would still fire, but with the trigger it would not.

[bdraco]

It seems like we should fix the regression (which is what this PR aims to do). However, the correct behavior is not clear here, a second opinion is needed.

[bdraco]

https://www.home-assistant.io/docs/scripts/#wait-timeout it does not appear to be documented how a zero timeout should be handled

[bdraco]

I tend to side on restoring the status quo since even if it was not documented changing the behavior is a regression. Also it does not seem to make much sense to specify a timeout and than not expect it to be enforced, but the same could be said about a zero timeout not making much sense

[bdraco]

We may want to consider the effect this has on the frontend, it may be confusing.

Currently no timeout is rendered as 00:00:00:000

e.g.: wait_for_trigger: [] displays as:

So currently timeout: 0 and no timeout have the same behavior, and both render the same.

If we make timeout: 0 act as immediate timeout, this means that both no timeout, and instant timeout will both render identically in the frontend UI, but will have different behavior.

There may be people with legacy automations saved that accidentally touched this control, or changed it back to 0, and may be lingering unaware in their yaml. For those this would be a breaking change?

After reading the discussion in #109586 it seems like we should fix the regression and than modify the UI to be able to delete the timeout without having to drop back to YAML to remove it.

[karwosts]

modify the UI to be able to delete the timeout without having to drop back to YAML to remove it.

related:
home-assistant/frontend#18590
home-assistant/frontend#19360

[frenck]

Thanks, @bdraco 👍

Odd situation, but I agree we should address the regression for now first.

../Frenck

[ildar170975]

Thanks a lot! Guys, please add a corr. remark to Docs.

[frenck]

Thanks a lot! Guys, please add a corr. remark to Docs.

I think the gist is: We don't know what the expected or correct way to handling it is. This for now, fixes a regression, but I honestly don't think this is the correct behavior we should document and aim for (or maybe it is, but that need to be figured out).

[RobertMe]

To add a use case for this (I've been hit by this bug). I'm using a wait_template to wait for something to happen / be the defined state, and the following steps act based on "did it happen?" (i.e.: did the wait complete or not). But in some specific scenarios I don't want to wait, I want to know if it's the current state right now. I obviously could wrap the wait in an if etc, but currently I made the timeout a template which is either 00:01:00 or 00:00:00. (And I think I even wrote this automation before if existed).

So while I do agree that a hardcoded timeout of 00:00:00 doesn't really make sense, I do believe there is a use case for it when it's conditional, based on a template. As it keeps the script a bit cleaner (doesn't require wrapping the wait_template in an if)

[ildar170975]

Another use-case - using in a blueprint:

        - wait_template: !input input_TEMPLATE_WAIT_CONDITION
          timeout:
            minutes: !input input_VALUE_TIMEOUT
          continue_on_timeout: true

where input_VALUE_TIMEOUT may be 0.

[frenck]

I don't think this is the right place for this discussion. Let's keep it at the review here :)