From 35406dcd0045cfe1b7bc5c4745accaca134e4372 Mon Sep 17 00:00:00 2001 From: "riyaz.shiraguppi" Date: Tue, 14 Jul 2026 11:39:22 -0400 Subject: [PATCH 1/3] fix(downtime_schedules): rewrite past `end` on create path `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. --- datadog_sync/model/downtime_schedules.py | 50 +++++-- tests/unit/test_downtime_schedules.py | 159 +++++++++++++++++++++++ 2 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 tests/unit/test_downtime_schedules.py diff --git a/datadog_sync/model/downtime_schedules.py b/datadog_sync/model/downtime_schedules.py index 63c6906e9..541a065d7 100644 --- a/datadog_sync/model/downtime_schedules.py +++ b/datadog_sync/model/downtime_schedules.py @@ -4,7 +4,7 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations from typing import TYPE_CHECKING, Optional, List, Dict, Tuple -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from dateutil.parser import parse from datadog_sync.utils.base_resource import BaseResource, ResourceConfig @@ -63,19 +63,47 @@ async def import_resource(self, _id: Optional[str] = None, resource: Optional[Di return str(resource["id"]), resource + @staticmethod + def _parse_utc(value): + """Parse an ISO timestamp and return a UTC-aware datetime. Naive input + is assumed UTC (the destination stores schedules in UTC).""" + parsed = parse(value) + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed + + @staticmethod + def _iso_utc(dt) -> str: + return dt.isoformat().replace("+00:00", "Z") + async def pre_resource_action_hook(self, _id, resource: Dict) -> None: if _id not in self.config.state.destination[self.resource_type]: schedule = resource["attributes"].get("schedule") - if schedule and "start" in schedule: - current_time = datetime.utcnow() - t = parse(schedule["start"]) - if t.timestamp() <= current_time.timestamp(): - current_time = current_time + timedelta(seconds=60) - if getattr(current_time, "tzinfo", None) is not None: - new_time = current_time.isoformat() - else: - new_time = "{}Z".format(current_time.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3]) - schedule["start"] = new_time + if not schedule: + return + now = datetime.now(timezone.utc) + floor = now + timedelta(seconds=60) + + # Rewrite past `start` forward. Existing behavior; parse/format hardened + # to UTC-aware so `.timestamp()` is correct on non-UTC hosts. + start_raw = schedule.get("start") + start_dt = None + if start_raw: + start_dt = self._parse_utc(start_raw) + if start_dt <= now: + start_dt = floor + schedule["start"] = self._iso_utc(start_dt) + + # Rewrite past `end` forward while preserving `end > start`. Prior + # code did not touch `end`, so one-off downtimes with both `start` + # and `end` in the past 400'd at POST with "Downtime cannot be + # scheduled in the past". + end_raw = schedule.get("end") + if end_raw: + end_dt = self._parse_utc(end_raw) + if end_dt <= now: + end_min = floor if start_dt is None else max(floor, start_dt + timedelta(seconds=60)) + schedule["end"] = self._iso_utc(end_min) else: # If start or end times of the resource are in the past, we set to the current destination `start` and `end` # this is to avoid unnecessary diff outputs diff --git a/tests/unit/test_downtime_schedules.py b/tests/unit/test_downtime_schedules.py new file mode 100644 index 000000000..e8cf55d83 --- /dev/null +++ b/tests/unit/test_downtime_schedules.py @@ -0,0 +1,159 @@ +# Unless explicitly stated otherwise all files in this repository are licensed +# under the 3-clause BSD style license (see LICENSE). +# This product includes software developed at Datadog (https://www.datadoghq.com/). +# Copyright 2019 Datadog, Inc. + +""" +Unit tests for downtime_schedules create-path schedule normalization. + +Pins the pre_resource_action_hook create-path behavior for schedule.start +and schedule.end when their values are in the past. Before this fix, only +schedule.start was rewritten forward; a downtime carrying a past `end` +(one-off window both in the past) still hit the destination API and 400'd +with "Downtime cannot be scheduled in the past". +""" + +import asyncio +from datetime import datetime, timedelta, timezone + +from dateutil.parser import parse + +from datadog_sync.model.downtime_schedules import DowntimeSchedules + + +def _run(coro): + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +def _now_ts() -> float: + return datetime.now(timezone.utc).timestamp() + + +def _past_iso(seconds_ago: int = 3600) -> str: + return (datetime.now(timezone.utc) - timedelta(seconds=seconds_ago)).isoformat().replace("+00:00", "Z") + + +def _future_iso(seconds_ahead: int = 3600) -> str: + return (datetime.now(timezone.utc) + timedelta(seconds=seconds_ahead)).isoformat().replace("+00:00", "Z") + + +def _make_resource(schedule): + return {"attributes": {"schedule": schedule}} + + +def test_past_start_bumped_forward(mock_config): + """Baseline invariant preserved: past schedule.start on create is + rewritten to ~now. Regression guard so the refactor didn't change the + pre-existing contract for schedule.start.""" + downtime = DowntimeSchedules(mock_config) + past = _past_iso(3600) + resource = _make_resource({"start": past}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + rewritten = resource["attributes"]["schedule"]["start"] + assert rewritten != past + assert parse(rewritten).timestamp() > _now_ts() - 5 + + +def test_past_end_bumped_forward(mock_config): + """New coverage: past schedule.end on create must also be rewritten. + Without this, one-off downtimes whose `end` predates now still 400 with + 'Downtime cannot be scheduled in the past' — the destination API + validates the full schedule window, not just start.""" + downtime = DowntimeSchedules(mock_config) + past_start = _past_iso(7200) + past_end = _past_iso(3600) + resource = _make_resource({"start": past_start, "end": past_end}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + schedule = resource["attributes"]["schedule"] + assert schedule["end"] != past_end + assert parse(schedule["end"]).timestamp() > _now_ts() - 5 + # `end > start` invariant must hold after bumping — the destination API + # rejects windows where end precedes start with a separate 400. + assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp() + + +def test_past_end_with_future_start_preserves_ordering(mock_config): + """Adversarial case surfaced by pre-merge review: source has a future + `start` (untouched) and a past `end` (needs bumping). Naively bumping + end to `now+60s` would produce `end < start` and 400 with a different + error. Bumped end must be at least `start + small_delta`.""" + downtime = DowntimeSchedules(mock_config) + future_start = _future_iso(3600) + past_end = _past_iso(3600) + resource = _make_resource({"start": future_start, "end": past_end}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + schedule = resource["attributes"]["schedule"] + assert schedule["start"] == future_start, "future start must be untouched" + assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp(), ( + "bumped end must land after start to preserve the ordering invariant" + ) + + +def test_future_fields_untouched(mock_config): + """Values already in the future must NOT be rewritten. Rewriting a + future start/end changes the customer's intended window and would + produce a spurious diff on subsequent syncs.""" + downtime = DowntimeSchedules(mock_config) + start_future = _future_iso(3600) + end_future = _future_iso(7200) + resource = _make_resource({"start": start_future, "end": end_future}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + schedule = resource["attributes"]["schedule"] + assert schedule["start"] == start_future + assert schedule["end"] == end_future + + +def test_missing_fields_no_op(mock_config): + """Edge case: schedule may omit start or end. The hook must tolerate + absent keys without raising.""" + downtime = DowntimeSchedules(mock_config) + + # start-only + r1 = _make_resource({"start": _past_iso()}) + _run(downtime.pre_resource_action_hook("id-1", r1)) + assert "end" not in r1["attributes"]["schedule"] + + # end-only + r2 = _make_resource({"end": _past_iso()}) + _run(downtime.pre_resource_action_hook("id-2", r2)) + assert "start" not in r2["attributes"]["schedule"] + assert parse(r2["attributes"]["schedule"]["end"]).timestamp() > _now_ts() - 5 + + # empty schedule + r3 = _make_resource({}) + _run(downtime.pre_resource_action_hook("id-3", r3)) + assert r3["attributes"]["schedule"] == {} + + +def test_update_path_untouched_by_this_fix(mock_config): + """Explicit boundary: this change only touches the create branch. The + update-path branch that clamps source start/end backwards to + destination's stored values is intentionally out of scope; a follow-up + is planned to address the update-path case where a downtime already in + state has a past stored `start`.""" + downtime = DowntimeSchedules(mock_config) + _id = "existing-id" + mock_config.state.destination["downtime_schedules"][_id] = { + "attributes": {"schedule": {"start": _past_iso(1800), "end": _past_iso(900)}} + } + + resource = _make_resource({"start": _past_iso(3600), "end": _past_iso(1200)}) + + _run(downtime.pre_resource_action_hook(_id, resource)) + + schedule = resource["attributes"]["schedule"] + dest = mock_config.state.destination["downtime_schedules"][_id]["attributes"]["schedule"] + assert schedule["start"] == dest["start"] + assert schedule["end"] == dest["end"] From e558cb2a8b636f4c62150c0c8f7c459ebae19d80 Mon Sep 17 00:00:00 2001 From: "riyaz.shiraguppi" Date: Tue, 14 Jul 2026 11:47:24 -0400 Subject: [PATCH 2/3] Preserve source `end - start` duration on rewrite; tighten tests 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 `{}`) --- datadog_sync/model/downtime_schedules.py | 48 ++++++++++++-------- tests/unit/test_downtime_schedules.py | 57 ++++++++++++++++-------- 2 files changed, 69 insertions(+), 36 deletions(-) diff --git a/datadog_sync/model/downtime_schedules.py b/datadog_sync/model/downtime_schedules.py index 541a065d7..102cf78ac 100644 --- a/datadog_sync/model/downtime_schedules.py +++ b/datadog_sync/model/downtime_schedules.py @@ -84,26 +84,38 @@ async def pre_resource_action_hook(self, _id, resource: Dict) -> None: now = datetime.now(timezone.utc) floor = now + timedelta(seconds=60) - # Rewrite past `start` forward. Existing behavior; parse/format hardened - # to UTC-aware so `.timestamp()` is correct on non-UTC hosts. + # Rewrite past `start` forward. Existing behavior; parse/format + # hardened to UTC-aware so `.timestamp()` is correct on non-UTC hosts. start_raw = schedule.get("start") - start_dt = None - if start_raw: - start_dt = self._parse_utc(start_raw) - if start_dt <= now: - start_dt = floor - schedule["start"] = self._iso_utc(start_dt) - - # Rewrite past `end` forward while preserving `end > start`. Prior - # code did not touch `end`, so one-off downtimes with both `start` - # and `end` in the past 400'd at POST with "Downtime cannot be - # scheduled in the past". end_raw = schedule.get("end") - if end_raw: - end_dt = self._parse_utc(end_raw) - if end_dt <= now: - end_min = floor if start_dt is None else max(floor, start_dt + timedelta(seconds=60)) - schedule["end"] = self._iso_utc(end_min) + start_dt_source = self._parse_utc(start_raw) if start_raw else None + end_dt_source = self._parse_utc(end_raw) if end_raw else None + + start_dt = start_dt_source + if start_dt_source is not None and start_dt_source <= now: + start_dt = floor + schedule["start"] = self._iso_utc(start_dt) + + # Rewrite past `end` forward while preserving both invariants: + # 1. `end > start` — API 400s on inverted windows. + # 2. Original `end - start` duration when the source window is + # entirely in the past — customer scheduled a 2h maintenance; + # shrinking to 60s on destination would be a semantic surprise. + # Prior code did not touch `end`, so one-off downtimes with both + # `start` and `end` in the past 400'd at POST. + if end_dt_source is not None and end_dt_source <= now: + if start_dt is not None: + if start_dt_source is not None: + duration = end_dt_source - start_dt_source + if duration <= timedelta(0): + duration = timedelta(seconds=60) + end_dt = start_dt + duration + else: + end_dt = start_dt + timedelta(seconds=60) + end_dt = max(end_dt, floor) + else: + end_dt = floor + schedule["end"] = self._iso_utc(end_dt) else: # If start or end times of the resource are in the past, we set to the current destination `start` and `end` # this is to avoid unnecessary diff outputs diff --git a/tests/unit/test_downtime_schedules.py b/tests/unit/test_downtime_schedules.py index e8cf55d83..3c90a39d6 100644 --- a/tests/unit/test_downtime_schedules.py +++ b/tests/unit/test_downtime_schedules.py @@ -47,8 +47,11 @@ def _make_resource(schedule): def test_past_start_bumped_forward(mock_config): """Baseline invariant preserved: past schedule.start on create is - rewritten to ~now. Regression guard so the refactor didn't change the - pre-existing contract for schedule.start.""" + rewritten to ~now+60s. Regression guard so the refactor didn't change + the pre-existing contract for schedule.start. + + Upper bound guards against a bug where the rewrite jumps far into the + future (e.g. a units-of-hours instead of seconds mistake).""" downtime = DowntimeSchedules(mock_config) past = _past_iso(3600) resource = _make_resource({"start": past}) @@ -56,28 +59,43 @@ def test_past_start_bumped_forward(mock_config): _run(downtime.pre_resource_action_hook("new-id", resource)) rewritten = resource["attributes"]["schedule"]["start"] + now_ts = _now_ts() assert rewritten != past - assert parse(rewritten).timestamp() > _now_ts() - 5 + assert now_ts - 5 < parse(rewritten).timestamp() < now_ts + 120, ( + "rewritten start must land in a narrow window around now+60s" + ) def test_past_end_bumped_forward(mock_config): """New coverage: past schedule.end on create must also be rewritten. Without this, one-off downtimes whose `end` predates now still 400 with 'Downtime cannot be scheduled in the past' — the destination API - validates the full schedule window, not just start.""" + validates the full schedule window, not just start. + + Both `start` and `end` past: original duration is preserved so a + 2-hour maintenance stays a 2-hour maintenance on the destination + rather than collapsing to 60s.""" downtime = DowntimeSchedules(mock_config) - past_start = _past_iso(7200) - past_end = _past_iso(3600) + past_start = _past_iso(7200) # 2h ago + past_end = _past_iso(3600) # 1h ago → source duration = 1h resource = _make_resource({"start": past_start, "end": past_end}) _run(downtime.pre_resource_action_hook("new-id", resource)) schedule = resource["attributes"]["schedule"] assert schedule["end"] != past_end - assert parse(schedule["end"]).timestamp() > _now_ts() - 5 + now_ts = _now_ts() + assert parse(schedule["end"]).timestamp() > now_ts - 5 # `end > start` invariant must hold after bumping — the destination API # rejects windows where end precedes start with a separate 400. - assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp() + start_ts = parse(schedule["start"]).timestamp() + end_ts = parse(schedule["end"]).timestamp() + assert end_ts > start_ts + # Duration preserved: source was 3600s wide, destination should also be ~3600s + # (allow small slack for the 60s floor rewrite of start). + assert 3500 < (end_ts - start_ts) < 3700, ( + f"source window was 3600s; destination window is {end_ts - start_ts}s" + ) def test_past_end_with_future_start_preserves_ordering(mock_config): @@ -117,24 +135,27 @@ def test_future_fields_untouched(mock_config): def test_missing_fields_no_op(mock_config): """Edge case: schedule may omit start or end. The hook must tolerate - absent keys without raising.""" + absent keys without raising, AND must actually rewrite the fields + that are present.""" downtime = DowntimeSchedules(mock_config) - # start-only - r1 = _make_resource({"start": _past_iso()}) + # start-only: past start rewritten; end key stays absent + past_start = _past_iso() + r1 = _make_resource({"start": past_start}) _run(downtime.pre_resource_action_hook("id-1", r1)) assert "end" not in r1["attributes"]["schedule"] + assert r1["attributes"]["schedule"]["start"] != past_start + assert parse(r1["attributes"]["schedule"]["start"]).timestamp() > _now_ts() - 5 - # end-only - r2 = _make_resource({"end": _past_iso()}) + # empty schedule + r2 = _make_resource({}) _run(downtime.pre_resource_action_hook("id-2", r2)) - assert "start" not in r2["attributes"]["schedule"] - assert parse(r2["attributes"]["schedule"]["end"]).timestamp() > _now_ts() - 5 + assert r2["attributes"]["schedule"] == {} - # empty schedule - r3 = _make_resource({}) + # schedule is None (falsy) — early-return branch + r3 = {"attributes": {"schedule": None}} _run(downtime.pre_resource_action_hook("id-3", r3)) - assert r3["attributes"]["schedule"] == {} + assert r3["attributes"]["schedule"] is None def test_update_path_untouched_by_this_fix(mock_config): From 81d8ba972a81e0b3975f3708a60f03a5b6fb124c Mon Sep 17 00:00:00 2001 From: "riyaz.shiraguppi" Date: Tue, 14 Jul 2026 13:07:07 -0400 Subject: [PATCH 3/3] Skip past-end downtimes instead of shifting the window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- datadog_sync/model/downtime_schedules.py | 53 +++----- tests/unit/test_downtime_schedules.py | 161 ++++++++++++----------- 2 files changed, 105 insertions(+), 109 deletions(-) diff --git a/datadog_sync/model/downtime_schedules.py b/datadog_sync/model/downtime_schedules.py index 102cf78ac..3cc20e67c 100644 --- a/datadog_sync/model/downtime_schedules.py +++ b/datadog_sync/model/downtime_schedules.py @@ -82,40 +82,29 @@ async def pre_resource_action_hook(self, _id, resource: Dict) -> None: if not schedule: return now = datetime.now(timezone.utc) - floor = now + timedelta(seconds=60) - # Rewrite past `start` forward. Existing behavior; parse/format - # hardened to UTC-aware so `.timestamp()` is correct on non-UTC hosts. - start_raw = schedule.get("start") + # Past `end` means the maintenance window has already closed on the + # source. Replicating it to the destination would either invent a + # new customer-visible maintenance (if we shifted `end` forward) or + # 400 with "Downtime cannot be scheduled in the past". Skip: an + # ended downtime has nothing left to silence. end_raw = schedule.get("end") - start_dt_source = self._parse_utc(start_raw) if start_raw else None - end_dt_source = self._parse_utc(end_raw) if end_raw else None - - start_dt = start_dt_source - if start_dt_source is not None and start_dt_source <= now: - start_dt = floor - schedule["start"] = self._iso_utc(start_dt) - - # Rewrite past `end` forward while preserving both invariants: - # 1. `end > start` — API 400s on inverted windows. - # 2. Original `end - start` duration when the source window is - # entirely in the past — customer scheduled a 2h maintenance; - # shrinking to 60s on destination would be a semantic surprise. - # Prior code did not touch `end`, so one-off downtimes with both - # `start` and `end` in the past 400'd at POST. - if end_dt_source is not None and end_dt_source <= now: - if start_dt is not None: - if start_dt_source is not None: - duration = end_dt_source - start_dt_source - if duration <= timedelta(0): - duration = timedelta(seconds=60) - end_dt = start_dt + duration - else: - end_dt = start_dt + timedelta(seconds=60) - end_dt = max(end_dt, floor) - else: - end_dt = floor - schedule["end"] = self._iso_utc(end_dt) + if end_raw: + end_dt = self._parse_utc(end_raw) + if end_dt <= now: + raise SkipResource( + str(_id), self.resource_type, + "Downtime end is in the past.", + ) + + # Rewrite past `start` forward to now+60s. `end` (if present) is + # left as-is per customer intent — the window may shrink but its + # original end time is preserved. + start_raw = schedule.get("start") + if start_raw: + start_dt = self._parse_utc(start_raw) + if start_dt <= now: + schedule["start"] = self._iso_utc(now + timedelta(seconds=60)) else: # If start or end times of the resource are in the past, we set to the current destination `start` and `end` # this is to avoid unnecessary diff outputs diff --git a/tests/unit/test_downtime_schedules.py b/tests/unit/test_downtime_schedules.py index 3c90a39d6..ab593c1e9 100644 --- a/tests/unit/test_downtime_schedules.py +++ b/tests/unit/test_downtime_schedules.py @@ -6,22 +6,31 @@ """ Unit tests for downtime_schedules create-path schedule normalization. -Pins the pre_resource_action_hook create-path behavior for schedule.start -and schedule.end when their values are in the past. Before this fix, only -schedule.start was rewritten forward; a downtime carrying a past `end` -(one-off window both in the past) still hit the destination API and 400'd -with "Downtime cannot be scheduled in the past". +Prior behavior only rewrote past `schedule.start` forward. Downtimes with +a past `end` (one-off maintenance windows that already closed on the +source) still hit the destination API and 400'd with "Downtime cannot be +scheduled in the past". + +New behavior: +- Past `schedule.end` → SkipResource (ended downtimes are not replicated). +- Past `schedule.start` with future/absent `end` → bump `start` to now+60s + and leave `end` as-is (window may shrink, original end time preserved). """ import asyncio from datetime import datetime, timedelta, timezone +import pytest from dateutil.parser import parse from datadog_sync.model.downtime_schedules import DowntimeSchedules +from datadog_sync.utils.resource_utils import SkipResource def _run(coro): + # Fresh loop per call: pytest-asyncio strict mode closes the ambient loop + # between tests, so asyncio.get_event_loop() may raise "no current event + # loop" when this helper runs after unrelated async tests in the suite. loop = asyncio.new_event_loop() try: return loop.run_until_complete(coro) @@ -46,12 +55,9 @@ def _make_resource(schedule): def test_past_start_bumped_forward(mock_config): - """Baseline invariant preserved: past schedule.start on create is - rewritten to ~now+60s. Regression guard so the refactor didn't change - the pre-existing contract for schedule.start. - - Upper bound guards against a bug where the rewrite jumps far into the - future (e.g. a units-of-hours instead of seconds mistake).""" + """Baseline invariant: past schedule.start with no `end` (open-ended + downtime) is rewritten to ~now+60s. Regression guard so the refactor + didn't change the pre-existing contract.""" downtime = DowntimeSchedules(mock_config) past = _past_iso(3600) resource = _make_resource({"start": past}) @@ -61,66 +67,57 @@ def test_past_start_bumped_forward(mock_config): rewritten = resource["attributes"]["schedule"]["start"] now_ts = _now_ts() assert rewritten != past - assert now_ts - 5 < parse(rewritten).timestamp() < now_ts + 120, ( - "rewritten start must land in a narrow window around now+60s" - ) - + assert now_ts - 5 < parse(rewritten).timestamp() < now_ts + 120 -def test_past_end_bumped_forward(mock_config): - """New coverage: past schedule.end on create must also be rewritten. - Without this, one-off downtimes whose `end` predates now still 400 with - 'Downtime cannot be scheduled in the past' — the destination API - validates the full schedule window, not just start. - Both `start` and `end` past: original duration is preserved so a - 2-hour maintenance stays a 2-hour maintenance on the destination - rather than collapsing to 60s.""" +def test_past_start_future_end_preserves_end(mock_config): + """Past `start` with a future `end`: bump `start` forward, leave `end` + alone. The customer's maintenance still ends at their intended time — + the window may be shorter than the source's, but the end boundary is + honored.""" downtime = DowntimeSchedules(mock_config) - past_start = _past_iso(7200) # 2h ago - past_end = _past_iso(3600) # 1h ago → source duration = 1h - resource = _make_resource({"start": past_start, "end": past_end}) + past_start = _past_iso(3600) + future_end = _future_iso(3600) + resource = _make_resource({"start": past_start, "end": future_end}) _run(downtime.pre_resource_action_hook("new-id", resource)) schedule = resource["attributes"]["schedule"] - assert schedule["end"] != past_end - now_ts = _now_ts() - assert parse(schedule["end"]).timestamp() > now_ts - 5 - # `end > start` invariant must hold after bumping — the destination API - # rejects windows where end precedes start with a separate 400. - start_ts = parse(schedule["start"]).timestamp() - end_ts = parse(schedule["end"]).timestamp() - assert end_ts > start_ts - # Duration preserved: source was 3600s wide, destination should also be ~3600s - # (allow small slack for the 60s floor rewrite of start). - assert 3500 < (end_ts - start_ts) < 3700, ( - f"source window was 3600s; destination window is {end_ts - start_ts}s" - ) - - -def test_past_end_with_future_start_preserves_ordering(mock_config): - """Adversarial case surfaced by pre-merge review: source has a future - `start` (untouched) and a past `end` (needs bumping). Naively bumping - end to `now+60s` would produce `end < start` and 400 with a different - error. Bumped end must be at least `start + small_delta`.""" + assert schedule["start"] != past_start + assert schedule["end"] == future_end, "future end must be untouched" + # `end > start` invariant still holds because start is now ~now+60s and + # end is ~+3600s. + assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp() + + +def test_past_end_raises_skip(mock_config): + """New behavior: past `end` means the downtime has already ended on the + source. Skip the resource — replicating an expired maintenance to the + destination either produces a 400 or invents a phantom window.""" downtime = DowntimeSchedules(mock_config) - future_start = _future_iso(3600) - past_end = _past_iso(3600) - resource = _make_resource({"start": future_start, "end": past_end}) + resource = _make_resource({"start": _past_iso(7200), "end": _past_iso(3600)}) - _run(downtime.pre_resource_action_hook("new-id", resource)) + with pytest.raises(SkipResource) as excinfo: + _run(downtime.pre_resource_action_hook("skip-id", resource)) - schedule = resource["attributes"]["schedule"] - assert schedule["start"] == future_start, "future start must be untouched" - assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp(), ( - "bumped end must land after start to preserve the ordering invariant" - ) + assert "past" in str(excinfo.value).lower() -def test_future_fields_untouched(mock_config): - """Values already in the future must NOT be rewritten. Rewriting a - future start/end changes the customer's intended window and would - produce a spurious diff on subsequent syncs.""" +def test_past_end_raises_skip_even_with_future_start(mock_config): + """Degenerate but possible source shape: `end` in the past AND `start` + in the future (source is a broken record). Still skip — the window + doesn't make sense to replicate.""" + downtime = DowntimeSchedules(mock_config) + resource = _make_resource({"start": _future_iso(3600), "end": _past_iso(3600)}) + + with pytest.raises(SkipResource): + _run(downtime.pre_resource_action_hook("skip-id", resource)) + + +def test_future_start_and_end_untouched(mock_config): + """Values already in the future must NOT be rewritten. Rewriting would + change the customer's intended window and produce a spurious diff on + subsequent syncs.""" downtime = DowntimeSchedules(mock_config) start_future = _future_iso(3600) end_future = _future_iso(7200) @@ -133,37 +130,45 @@ def test_future_fields_untouched(mock_config): assert schedule["end"] == end_future -def test_missing_fields_no_op(mock_config): - """Edge case: schedule may omit start or end. The hook must tolerate - absent keys without raising, AND must actually rewrite the fields - that are present.""" +def test_missing_or_null_schedule_no_op(mock_config): + """Edge cases: schedule may be absent, empty, or None. The hook must + tolerate all three without raising.""" downtime = DowntimeSchedules(mock_config) - # start-only: past start rewritten; end key stays absent - past_start = _past_iso() - r1 = _make_resource({"start": past_start}) + # empty schedule + r1 = _make_resource({}) _run(downtime.pre_resource_action_hook("id-1", r1)) - assert "end" not in r1["attributes"]["schedule"] - assert r1["attributes"]["schedule"]["start"] != past_start - assert parse(r1["attributes"]["schedule"]["start"]).timestamp() > _now_ts() - 5 + assert r1["attributes"]["schedule"] == {} - # empty schedule - r2 = _make_resource({}) + # schedule is None + r2 = {"attributes": {"schedule": None}} _run(downtime.pre_resource_action_hook("id-2", r2)) - assert r2["attributes"]["schedule"] == {} + assert r2["attributes"]["schedule"] is None - # schedule is None (falsy) — early-return branch - r3 = {"attributes": {"schedule": None}} + # attributes.schedule key absent + r3 = {"attributes": {}} _run(downtime.pre_resource_action_hook("id-3", r3)) - assert r3["attributes"]["schedule"] is None + assert r3 == {"attributes": {}} + + +def test_start_only_no_end_field(mock_config): + """Open-ended downtime (no `end` key at all): past `start` is rewritten, + the missing-`end` shape is preserved (not injected).""" + downtime = DowntimeSchedules(mock_config) + resource = _make_resource({"start": _past_iso()}) + + _run(downtime.pre_resource_action_hook("id", resource)) + + schedule = resource["attributes"]["schedule"] + assert "end" not in schedule + assert parse(schedule["start"]).timestamp() > _now_ts() - 5 def test_update_path_untouched_by_this_fix(mock_config): """Explicit boundary: this change only touches the create branch. The update-path branch that clamps source start/end backwards to destination's stored values is intentionally out of scope; a follow-up - is planned to address the update-path case where a downtime already in - state has a past stored `start`.""" + is planned to address the update-path case.""" downtime = DowntimeSchedules(mock_config) _id = "existing-id" mock_config.state.destination["downtime_schedules"][_id] = { @@ -172,6 +177,8 @@ def test_update_path_untouched_by_this_fix(mock_config): resource = _make_resource({"start": _past_iso(3600), "end": _past_iso(1200)}) + # No SkipResource on update path even though end is past — update-path + # semantics are intentionally out of scope for this PR. _run(downtime.pre_resource_action_hook(_id, resource)) schedule = resource["attributes"]["schedule"]