Prevent time pattern reschedule if cancelled during job execution

[swoga]

Proposed change

An automation that is triggered via a time entity creates a scheduled job for triggering on the next due date.
However, after triggering, a new job is always scheduled for the next execution.
This becomes a problem if the value of the entity is changed during the execution of the automation.

1. the automation is triggered, by the time set in input_datetime.test

   core/homeassistant/helpers/event.py

   Line 1779 in f21226d

   hass.async_run_hass_job(self.job, localized_now, background=True)

2. automations calls service input_datetime.set_datetime for input_datetime.test, which leads to cancellation

   core/homeassistant/helpers/event.py

   Line 1789 in f21226d

   def async_cancel(self) -> None:

3. automation ends and the time trigger is rescheduled with the old entity status, even though it was canceled

   core/homeassistant/helpers/event.py

   Line 1782 in f21226d

   self._cancel_callback = async_track_point_in_utc_time(

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 Setting a time helper during an automation triggered by this entity leads to an additional execution at a wrong time #117746
• 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.

[home-assistant]

Hi @swoga

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[home-assistant]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Stale

[bdraco]

Good find. 👍 Please add a test case for this change.

[bdraco]

An alternate fix that wouldn't require a new flag is to move hass.async_run_hass_job(self.job, localized_now, background=True) to the end of the function so the timer is already scheduled by the time it runs which allows cancellation to work as expected from job.

That's probably more efficient as well for the common case as it's going to be very rare that the timer handler is cancelled from the job.

[bdraco]

Thank you for detailing out the execution flow, it made reviewing this and understanding the problem nearly immediate.

[swoga]

An alternate fix that wouldn't require a new flag is to move hass.async_run_hass_job(self.job, localized_now, background=True) to the end of the function so the timer is already scheduled by the time it runs which allows cancellation to work as expected from job.

I've given it some thought, but I think it could cause problems.
If jobs have an execution time longer than one second, this would be a potential trouble spot.
e.g. if a time pattern is choosen which is executed every second, the next job is scheduled before the execution is completed

[swoga]

Does async_run_hass_job always wait for the end of a job, or is it allowed for callbacks to be async?
I don't know the internals well enough, but this could probably also be a problem area.

[bdraco]

An alternate fix that wouldn't require a new flag is to move hass.async_run_hass_job(self.job, localized_now, background=True) to the end of the function so the timer is already scheduled by the time it runs which allows cancellation to work as expected from job.

I've given it some thought, but I think it could cause problems.

If jobs have an execution time longer than one second, this would be a potential trouble spot.

e.g. if a time pattern is choosen which is executed every second, the next job is scheduled before the execution is completed

I don't think it could be an issue because we already fetch utcnow to calculate the next time before the job is run. If this was a problem it would already be firing the next one right away.

The job can't block and will finish synchronously as otherwise it would block the event loop.

This is also how _TrackTimeInterval works (which doesn't have this problem)

core/homeassistant/helpers/event.py

Line 1608 in be6598e

hass.async_run_hass_job(self._run_job, now, background=True)

[bdraco]

Since we are likely to ship 2024.5.5 soon, we can push this to 2024.6.x if its not ready by than. Its not a blocker as its been a problem for a while

[swoga]

I will remove the flag and move async_run_hass_job under the rescheduling.
Commit coming in a moment.

[swoga]

I don't think it could be an issue because we already fetch utcnow to calculate the next time before the job is run. If this was a problem it would already be firing the next one right away.

The job can't block and will finish synchronously as otherwise it would block the event loop.

This is also how _TrackTimeInterval works (which doesn't have this problem)

Thanks for the insight and the quick review!

[bdraco]

Thanks @swoga