[ARO-28590] Fix the case where MIMO Scheduler cancels valid tasks where there's a ScheduleAcross

[hawkowl]

Which issue this PR addresses:

Fixes ARO-28590

What this PR does / why we need it:

When a recurring schedule has a scheduleAcross, and a scheduled time passes, the Scheduler stops recognising the existing tasks for that previous scheduled time as valid and cancels them. This fixes that.

Test plan for issue:

Unit tests

Is there any documentation that needs to be updated for this PR?

N/A, bug fix

How do you know this will function as expected in production?

Unit testing :)

[Copilot]

Pull request overview

Fixes a MIMO scheduler edge case where, when scheduleAcross is configured, manifests for the most-recent schedule period can be incorrectly treated as invalid after the schedule time passes and then cancelled.

Changes:

• Extends the scheduler’s “valid periods” set to include a prior schedule time when the current time is within the scheduleAcross window.
• Skips manifest creation for schedule-start times that are already in the past, to avoid creating late manifests for the prior period.
• Updates unit tests to cover the scheduleAcross window behavior and makes the test clock configurable per test case.

Reviewed changes

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

File	Description
pkg/mimo/scheduler/manager.go	Adjusts valid-period calculation and manifest create/cancel behavior to preserve manifests within scheduleAcross.
pkg/mimo/scheduler/manager_test.go	Adds test coverage for the scheduleAcross window scenario and improves time control in tests.

Comments suppressed due to low confidence (2)

pkg/mimo/scheduler/manager.go:107

• The scheduleAcross value is parsed but negative durations are currently accepted. A negative scheduleAcross will produce negative offsets in PercentWithinPeriod and invert the “within scheduleAcross” window calculation, which can lead to unexpected scheduling/cancellation behavior. Consider rejecting negative scheduleAcross early with a clear error.

	scheduleAcross, err := time.ParseDuration(doc.MaintenanceSchedule.ScheduleAcross)
	if err != nil {
		a.log.Errorf("unrecognised scheduleacross: %s", err.Error())
		return false, err
	}

pkg/mimo/scheduler/manager_test.go:977

• This comment says the window “starts in the past 30s”, but RunAfter is ~51 minutes after the test's Now() (00:00:01). Updating the comment would better reflect what the test is asserting.

						// starts in the past 30s
						RunAfter:  time.Date(2026, 1, 1, 0, 51, 15, 0, time.UTC).Unix(),
						RunBefore: time.Date(2026, 1, 1, 1, 51, 15, 0, time.UTC).Unix(),

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

pkg/mimo/scheduler/manager_test.go:562

• Same as above: this expected Info-level per-manifest cancellation log entry will no longer be present if the per-manifest cancellation log is demoted to Debug to reduce production log volume. Consider removing this assertion and relying on the existing cancellation metric/state assertions.

				{
					"level":       gomega.Equal(logrus.InfoLevel),
					"msg":         gomega.Equal("cancelled unneeded manifest id=07070707-0707-0707-0707-070707070001 (2026-01-06T00:51:15Z)"),
					"resource_id": gomega.Equal(strings.ToLower(clusterResourceID)),
				},

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

pkg/mimo/scheduler/manager.go:128

• The "within scheduleAcross" logic only adds a single prior scheduled time computed from Next(now.Add(-scheduleAcross), calDef). If scheduleAcross spans multiple schedule intervals (it’s only validated as a duration), there can be more than one schedule start inside the window, and manifests for later starts may still be cancelled as "unneeded". Also, this block shadows hasFutureTime, which makes the control flow harder to reason about.

	// We may be within a scheduleAcross window of the previous schedule run,
	// add that to the expected periods so we don't cancel them
	scheduleAtStartOfScheduleAcross, hasFutureTime := Next(now.Add(-scheduleAcross), calDef)
	if hasFutureTime && !next.Equal(scheduleAtStartOfScheduleAcross) {
		periods_friendly = append(periods_friendly, fmt.Sprintf("%s (within scheduleAcross)", scheduleAtStartOfScheduleAcross.Format(friendlyDateFormat)))

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

pkg/mimo/scheduler/manager.go:130

• Next() always returns a time strictly after the input (it rounds and then adds 1s). When now.Add(-scheduleAcross) lands exactly on a schedule boundary (e.g., now==01:00:00 with schedule=00:00:00 and scheduleAcross=1h), Next(now.Add(-scheduleAcross), ...) will skip the boundary and return the next day, so the previous period is not included in periods and manifests still valid at the end of the scheduleAcross window can still be cancelled. Make the window-start lookup inclusive (and avoid doing it when scheduleAcross==0).

	// We may be within a scheduleAcross window of the previous schedule run,
	// add that to the expected periods so we don't cancel them
	scheduleAtStartOfScheduleAcross, hasFutureTime := Next(now.Add(-scheduleAcross), calDef)
	if hasFutureTime && !next.Equal(scheduleAtStartOfScheduleAcross) {
		periods_friendly = append(periods_friendly, fmt.Sprintf("%s (within scheduleAcross)", scheduleAtStartOfScheduleAcross.Format(friendlyDateFormat)))
		periods = append(periods, scheduleAtStartOfScheduleAcross)
	}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

pkg/mimo/scheduler/manager.go:219

• The found manifest Debugf uses %s with a time.Time (target), which results in malformed %!s(time.Time=...) output. Format the timestamp explicitly for consistent logs.

				newManifest, err := manifestsDB.Create(ctx, &api.MaintenanceManifestDocument{

Dockerfile.ci-rp:17

• This PR is described as a MIMO Scheduler bug fix, but it also changes the CI Dockerfile to drop npm audit. That’s a separate behavior change (security/CI signal) and is not mentioned in the PR description. Consider splitting this into a separate PR or updating the description to explain why the audit step is being removed and where vulnerability scanning is enforced instead (e.g., .github/workflows/npm-audit.yml).

# Clean install the package
RUN npm ci

pkg/mimo/scheduler/manager.go:218

• The Debugf format strings use %s with time.Time values (e.g., target/targetWithOffset). This produces %!s(time.Time=...) output rather than readable timestamps, which makes troubleshooting harder. Format the times explicitly (consistent with the Info log below that uses friendlyDateFormat/RFC3339).

This issue also appears on line 219 of the same file.

				if target.Before(now) {
					// Don't create manifests if the specified schedule start time is within the past
					clusterLog.Debugf("skipping manifest creation for %s window (%s)", target, targetWithOffset)
					continue
				}
				clusterLog.Debugf("creating manifest for %s window (%s)", target, targetWithOffset)

pkg/mimo/scheduler/manager.go:128

• The boolean return value from Next is named hasFutureTime twice here, which shadows the earlier variable and makes the control flow harder to follow. Use a distinct name for the scheduleAcross-window check to avoid confusion.

	// We may be within a scheduleAcross window of the previous schedule run,
	// add that to the expected periods so we don't cancel them
	scheduleAtStartOfScheduleAcross, hasFutureTime := Next(now.Add(-scheduleAcross), calDef)
	if hasFutureTime && !next.Equal(scheduleAtStartOfScheduleAcross) {
		periods_friendly = append(periods_friendly, fmt.Sprintf("%s (within scheduleAcross)", scheduleAtStartOfScheduleAcross.Format(friendlyDateFormat)))

pkg/mimo/scheduler/manager.go:106

• This error log doesn't include the invalid ScheduleAcross value, and the message text is hard to search (scheduleacross is inconsistent with the field name). Including the actual value makes production diagnosis much easier.

	scheduleAcross, err := time.ParseDuration(doc.MaintenanceSchedule.ScheduleAcross)
	if err != nil {
		a.log.Errorf("unrecognised scheduleacross: %s", err.Error())
		return false, err

pkg/mimo/scheduler/manager.go:300

• PercentWithinPeriod now models scheduleAcross, but the parameter name is still scheduleWithin and the conversions are more complex than needed. Renaming the parameter to reflect its meaning will improve readability and reduce future mistakes when modifying this logic.

// Given a period and a float from 0.0-1.0, calculate the target time within
// that duration rounded to the second.
func PercentWithinPeriod(percent float64, scheduleWithin time.Duration) time.Duration {

pkg/mimo/scheduler/manager_test.go:1423

• There are two identical test cases for "100% of 0s"; the later one is redundant and makes it harder to see the intended coverage. Removing the duplicate keeps the table focused on distinct scenarios.

		{
			desc:      "100% of 0s",
			percent:   1.0,
			period:    time.Duration(0),
			endPeriod: 0,