fix(downtime_schedules): rewrite past end on create path#626
Merged
Conversation
`pre_resource_action_hook` already rewrites past `schedule.start` forward on create. This extends the same forward-rewrite to `schedule.end` so one-off downtimes whose full window is in the past no longer 400 with "Downtime cannot be scheduled in the past" at POST time. Rewritten `end` is bumped to `max(now+60s, start+60s)` so the `end > start` invariant is preserved when `start` is future and `end` is past. Also hardens the create-path timestamp comparisons to UTC-aware `datetime` so `.timestamp()` is correct on non-UTC hosts. Update path (backward-clamp of source to destination stored values) is intentionally out of scope for this PR.
Pre-merge review found that when both `start` and `end` are past, the
prior implementation collapsed the customer's window to 60s. Preserve
the source duration when both fields need rewriting: shifted forward but
same length.
Tests now assert:
- upper-bounded rewrite window (catches units-of-hours-vs-seconds bugs)
- duration preservation for both-past case
- `schedule=None` early-return branch (in addition to `{}`)
heyronhay
reviewed
Jul 14, 2026
heyronhay
left a comment
heyronhay
left a comment
Collaborator
There was a problem hiding this comment.
We should never move the end time for a downtime, so we shouldn't do this PR ... I think.
Prior revision shifted `end` forward to preserve source duration. That invented a new customer-visible maintenance window on the destination that never existed on the source. Better: past `end` means the downtime has already closed; skip the resource entirely — an ended maintenance has nothing left to silence. For downtimes with past `start` but future `end`, `start` is bumped forward and `end` is left as-is. The window may shrink but the customer's intended end time is honored. Tests updated to cover: - Past start + future end → start rewritten, end preserved. - Past end (any start) → SkipResource. - Missing/null schedule fields → no-op, no crash.
Contributor
Author
Thanks for the catch. Updated PR. |
michael-richey
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pre_resource_action_hookalready rewrites pastschedule.startforward on the create path. This extends the create-path handling toschedule.end:end→ SkipResource. A downtime whose end time has already passed is a closed maintenance window with nothing left to silence. Replicating it either fails with "Downtime cannot be scheduled in the past" or invents a phantom window on the destination that never existed on the source.startwith futureend→ bumpstart, leaveendas-is. The customer's intended end time is preserved; the window may shrink.Also
Create-path timestamp comparisons hardened to UTC-aware
datetimeso.timestamp()is correct on non-UTC hosts. Prior code compared naiveutcnow()against tz-awareparse()results; correct on UTC hosts, drifted by TZ offset elsewhere.Update path (backward-clamp of source to destination stored values) is intentionally out of scope; a follow-up will address the update-path case.
Test plan
test_custom_client_trust_env.py(unrelated).