[astro] Refactor event scheduling

[Nadahar]

The problem itself is described in #19961.

The solution implemented here is that scheduled events are all assigned an identifier, which makes it possible to know if a particular event is already scheduled. With that possible, the scheduling can make sure that the same event isn't scheduled twice. If a particular event is scheduled, but a schedule already exists, the existing schedule is cancelled and the new one scheduled, in case the new schedule is more accurate. The exception is if the schedule is about to execute very shortly, and the schedules themselves are close in time. In case the schedule can't be cancelled in time, the new schedule is discarded instead, and the existing schedule is allowed to run.

The result of this is that scheduling events should work at any time, also when the job that generates the schedules run, and that schedules can be scheduled with overlap. This should prevent "holes" in the schedule.

The daily scheduling job is hard coded to run at 00:00:30. I see no reason to change that. Originally, it ran at 00:00:00, but it was moved as a "temporary fix" to avoid some collisions with everything that happens at midnight. But, with the overlapping, continuous scheduling, it doesn't really matter when it runs, it might as well run just after midnight. It also runs during Thing initialization. Previously, the scheduling was limited to events that were at the same date (in the local time zone) as the current time at the time the scheduling job was run. That was essentially the protection against double schedules, but it also prevented anything from happening between 00:00:00 and 00:00:30, and it couldn't schedule things to happen exactly at (civil) midnight.

To allow schedules to run while the scheduling job is running, the schedule is now done with overlap. It no longer cares about the current date or time zone, it schedules all events within a hard-coded time window of 25 hours. That should be enough for the next scheduled scheduling job to generate new schedules. The time window can easily be adjusted, but it's hard to see the point in scheduling for a longer period.

In addition, this fixes the "night end" event. The binding always considers "night" to be the next night, the one that starts on the current date. That means that the "night end" event will always be the next day/outside the scheduling window, even if it's currently a "night in progress" when the schedules are created. The reason, I assume, is that these things are stored as Ranges which must have a start and an end, which means that you can't "mix two different nights". I've made special handling for "night" which doesn't blindly follow the Range concept, so it will schedule "last night's" end event as night end, if that event hasn't yet occurred. This should make the "night end" event start to work/be scheduled when the schedules are created shortly after midnight.

Fixes #19961.

[Nadahar]

It's a bit hard to properly test this except to let it run and pay attention to the log and see that things are scheduled as planned. Since I run 4.2.3 on my production system, that isn't viable for me to do. So, I feel that somebody with a more recent production system should ideally test run it for a couple of days and verify that everything schedules as planned. All scheduling is logged if DEBUG logging is enabled for the binding.

[Nadahar]

This should also enable #19981.

[Copilot]

Pull request overview

This PR refactors event scheduling in the Astro binding to fix issues around midnight (00:00:00 - 00:00:30) where events would not be scheduled. The refactoring introduces an identifier-based scheduling system that prevents duplicate schedules and allows for 25-hour overlapping scheduling windows.

Changes:

• Implemented identifier-based scheduling to prevent duplicate events and handle reschedules intelligently
• Extended the scheduling time window from same-day-only to 25 hours to eliminate the problematic midnight gap
• Fixed the "night end" event to properly schedule when the binding initializes shortly after midnight

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
DateTimeUtils.java	Added isWithinTimeWindow helper methods for Calendar and Instant; removed deprecated isTimeGreaterEquals methods
Job.java	Updated all schedule methods to accept an identifier parameter; replaced same-day scheduling logic with time window checks; removed unused imports
DailyJobSun.java	Added special handling for night events to schedule previous night's end event if not yet occurred
CompositeJob.java	Removed entire file as composite jobs are no longer needed with individual event identifiers
AstroThingHandler.java	Changed from Set to Map-based schedule tracking with identifiers; added logic to cancel and replace existing schedules
AstroBindingConstants.java	Added job identifier constants

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Nadahar]

I've addressed typos, added constants and added a test for the new methods. When doing that, I discovered that using TimeUnit with Instant isn't as streamlined as it could be, obviously TimeUnit is no longer hip. So, I switched that method to using ChronoUnit instead, which is soo much hotter (but fundamentally exactly the same).

[lsiepel]

This seems like a good aproach to fix the events without ends. An issue i added to my todo list a year or two ago, but never got to it. So thanks!
Anyway, this needs to be tested (per label) have you created a JAR and published it somewhere so it is easy for some users to do real-life tests ? I would also be able to drop the jar in my system and see what happens, but personally i hardly use any of these events. So maybe better to ask the community.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Nadahar]

By the way, nobody has commented on the fact that CompositeJob has been deleted. It was created to ensure that multiple events taking place at the same time (like noon start and end) would happen in a predictable order (start before end), which isn't the case if they are both just scheduled. This is however incompatible with the logic here, which requires that each event schedule is referenced individually, to be able to do the "deduplication logic". But, it turns out that when checking, CompositeJob wasn't used at all anymore, and must have been abandoned in the past for other reasons. What is done today is that instead of scheduling these events at the same time, a one-minute separation is applied - so that only "noon start" is actually correct for noon - "noon end" always happen 1 minute after the fact.

This means that I decided to just remove CompositeJob, which would otherwise have caused a headache when trying to keep track of the individual events.

[Nadahar]

Anyway, this needs to be tested (per label) have you created a JAR and published it somewhere so it is easy for some users to do real-life tests ? I would also be able to drop the jar in my system and see what happens, but personally i hardly use any of these events. So maybe better to ask the community.

I have created a JAR and sent it to the user that reported the issue, @bjscheue. It hasn't been tested/I've gotten no feedback yet. I haven't created a public post asking for testing. The thing is that it's a bit hard to test, since you really must follow scheduling over some time and verify that "it does what it should". By enabling DEBUG logging for the binding, all scheduling decisions are logged when the scheduling job runs (right after midnight or when the thing is disabled/enabled). I have done testing in that I've done that and scrutinized the result, using different time zones and locations.

But it's hard to "assert" that everything will behave as intended from that.

Here is the bundle for those who wants to test - just remove the .txt extension after downloading:

org.openhab.binding.astro-5.2.0-SNAPSHOT.jar.txt

Do you think I should create a forum post asking for testing, given how must "patience" actually testing it would require? Ideally, one should make rules that fire from all the events and logged something, and then verify over time that all the rules triggered as they should, once per event.

I can't do any of this on my production system since it's running 4.2.3 (and I'm not about to try to backport this to 4.2.3 for testing purposes). My dev environment doesn't run long enough continuously to be useful beyond the testing I have already done.

[lsiepel]

Totally understand the test cases. I'd like to merge this before the next milestone, together with as many astro PR's as possible to get some milage on them. It is a heavy used binding, so i would expect to get reports after that milestone.
Still a sanity test would be good and from what you mentioned, those have been performed. I'll merge it and add the snapshot to my production. I don;t use this for critical situations.

[Nadahar]

I just got a report that @bjscheue has installed the JAR and testing has started. Everything looks good so far. He has added some extra rules to test some of the triggers, and the first "new one" that is up for testing is the one happening at 0:00, when the "eveningNight" ends and "morningNight" starts. There is no guarantee that the end of "eventingNight" will fire before "morningNight", they will be scheduled to run at the same time, and the exact order is random. It's hard to see why this should be a problem though, a user can use either event - both would be redundant.

[Nadahar]

@lsiepel I've attempted to improve the documentation of the logic. We should have Copilot another go at it, as I'm sure I've made some mistakes/typos. I stupidly elected not to ask for Copilot permissions when I was granted membership - something I regret now. I didn't want to "grab" what might be a scarce resource, but it doesn't really help when I must ask others to do it instead 😕

[lsiepel]

I stupidly elected not to ask for Copilot permissions when I was granted membership

I would expect this to be fixable :
GitHub => Setttings => Copilot => Get Copilot from an organization

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/handler/AstroThingHandler.java:411

• The new scheduling deduplication logic introduced in the schedule method (lines 390-411) lacks test coverage. This is a critical part of the refactoring that prevents duplicate event scheduling and handles edge cases when events are scheduled close together in time. Tests should verify that: 1) events can be rescheduled when the old event has sufficient time remaining, 2) new schedules are rejected when the old schedule is about to execute and times are similar, 3) schedules are replaced when time differences exceed MAX_SCHEDULE_DIFFERENCE_MS, and 4) the deduplication works correctly with the new identifier-based tracking.

    private void schedule(String identifier, Job job, long sleepTimeMs) {
        monitor.lock();
        try {
            tidyScheduledFutures();
            ScheduledFuture<?> future = scheduledFutures.get(identifier);
            if (future != null && !future.isDone()) {
                // The event is already scheduled
                long delay;
                if ((delay = future.getDelay(TimeUnit.MILLISECONDS)) < MIN_TIME_TO_SCHEDULE_MS
                        && Math.abs(delay - sleepTimeMs) <= MAX_SCHEDULE_DIFFERENCE_MS) {
                    // if the previously scheduled event is about to run very soon and their schedules are similar,
                    // we don't know if we can cancel it in time, so we let it run and don't schedule the new one.
                    return;
                }
                future.cancel(true);
            }
            future = scheduler.schedule(job, sleepTimeMs, TimeUnit.MILLISECONDS);
            scheduledFutures.put(identifier, future);
        } finally {
            monitor.unlock();
        }
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The special handling for the night end event (lines 89-96) lacks test coverage. This logic fixes a critical issue where the night end event would not be scheduled correctly when schedules are created shortly after midnight. Tests should verify that: 1) when astroDawn start is in the future (before dawn), it's used as the night end event, 2) when astroDawn start is in the past or doesn't exist, the next night's end is used, and 3) the correct event is scheduled in various scenarios around midnight and dawn times.

[Nadahar]

I have to give up on testing this unless somebody has some bright ideas. The reason is that I can't get control over the scheduler, which is inherited from BaseThingHandler in Core, and is defined like this:

    protected final ScheduledExecutorService scheduler = ThreadPoolManager
            .getScheduledPool(THING_HANDLER_THREADPOOL_NAME);

There's no room for mocking the scheduler, and thus I can't check what schedules have been made. I have verified the functionality through manual testing though.

[Nadahar]

I would expect this to be fixable : GitHub => Setttings => Copilot => Get Copilot from an organization

I only have one choice there: "Buy Copilot Business". My guess is that OH has a certain number of "seats" that they can distribute, and that if I had left the option enabled, I would have somehow "applied" for one of these, but since I removed the checkbox (as I wasn't quite prepared/didn't know what I requested), nobody was ever asked if I were to be assigned this. My guess is that @kaikreuzer is the one that handles this 😉

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Nadahar]

I rebased the branch to include the many PRs that have been merged for the binding recently.

[lsiepel]

Thanks LGTM

[Nadahar]

I didn't expect for it to be merged until the user feedback was in, but I can just make follow-up PRs.

A preliminary user report is in, and so far it all looks good except for the morningNight start event, which doesn't fire. The reason is obvious, it's "hardcoded" to be at 0:00 in the local time zone, which means that it's always in the past when the scheduling job runs, so it won't be scheduled. It's easily solved by making the scheduler always use "tomorrow's" morningNight for the start event.

In a way, some refactoring of the whole logic for scheduling might have been the best solution, the scheduling schedules based on the calculated events that are always seen "from the day we're in", while the scheduler really needs to know when these events take place within the time window under consideration, which might mean "tomorrow's" event. But, the only place where this will "bite" that I can think of, is the morningNight start - and potentially solar midnight when that's merged. So, I think that rather than doing another largish refactoring, I'll just deal with those two as special cases.

[lsiepel]

s that it's always in the past when the scheduling job runs, so it won't be scheduled. It's easily solved by making the scheduler always use "tomorrow's" morningNight for the start event.

I do: #20078 (comment) these PR's are very hard to test as you said, there might be some regressions, but i rather merge it soon (as i did) to get some mileage. I know you will be available and react quickly as usual when some regression occurs. We still have 6 months to stable.

[Nadahar]

It's not a regression though, since the event didn't work in the past either 😉

But, the intention is that it should start working...