From 8541f0f7069ad7c91d508ee7766cafbae908d400 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 13:44:21 +0200 Subject: [PATCH 1/7] Fixes reserved_slots_by_reservation dropping the slot on non-partly allocations --- src/libres/db/scheduler.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/libres/db/scheduler.py b/src/libres/db/scheduler.py index c569e21..16a9365 100644 --- a/src/libres/db/scheduler.py +++ b/src/libres/db/scheduler.py @@ -2297,22 +2297,29 @@ def reserved_slots_by_reservation( if id is None: return query - # allocation_id is ambiguous when multiple reservations share a token - # on a partly_available allocation; filter by time range instead. - # start is None for group reservations — the or_ includes all their - # slots. + # Map each slot to the requested reservation via its allocation. On + # partly_available allocations siblings of the same token are told + # apart by time range; non-partly allocations are reserved whole, so + # their slot is wider than the reservation and is matched by group. + # start is None for group reservations (include all their slots). return ( query .join(Reservation, and_( Reservation.token == ReservedSlot.reservation_token, Reservation.id == id )) + .join(Allocation, Allocation.id == ReservedSlot.allocation_id) .filter(or_( Reservation.start.is_(None), and_( + Allocation.partly_available.is_(False), + Allocation.group == Reservation.target, + ), + and_( + Allocation.partly_available.is_(True), ReservedSlot.start >= Reservation.start, ReservedSlot.end <= Reservation.end, - ) + ), )) ) @@ -2338,12 +2345,18 @@ def reserved_slots_by_blocker( ReservationBlocker.token == ReservedSlot.reservation_token, ReservationBlocker.id == id )) + .join(Allocation, Allocation.id == ReservedSlot.allocation_id) .filter(or_( ReservationBlocker.start.is_(None), and_( + Allocation.partly_available.is_(False), + Allocation.group == ReservationBlocker.target, + ), + and_( + Allocation.partly_available.is_(True), ReservedSlot.start >= ReservationBlocker.start, ReservedSlot.end <= ReservationBlocker.end, - ) + ), )) ) From 42f40fc55348c90b970d0bc2be1c1775db7792a9 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 13:47:00 +0200 Subject: [PATCH 2/7] Adds test --- tests/test_scheduler.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 966a413..d9442d0 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -691,6 +691,41 @@ def test_remove_reservation_does_not_affect_sibling_reservations( assert remaining_count == slots_a_count +def test_remove_reservation_on_non_partly_allocation_removes_slot( + scheduler: Scheduler, +) -> None: + """On a non-partly allocation the reserved slot spans the whole + allocation, even if the reservation was made for a narrower (but + contained) range. Matching slots by time range would then drop the slot, + leaving an orphaned ReservedSlot behind that keeps showing up on the + calendar after the reservation was removed (OGC-3388).""" + dates = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0)) + scheduler.allocate(dates, partly_available=False) + + # a contained sub-range is accepted on a whole-only allocation, but the + # slot still covers the entire allocation + sub = (datetime(2014, 3, 7, 9, 0), datetime(2014, 3, 7, 17, 0)) + token = scheduler.reserve('user@example.org', sub) + scheduler.commit() + scheduler.approve_reservations(token) + scheduler.commit() + + reservation = scheduler.reservations_by_token(token).one() + slot = scheduler.reserved_slots_by_reservation(token).one() + # the slot is wider than the reservation range + assert slot.start < reservation.start or slot.end > reservation.end + + # the slot must still be attributed to this reservation ... + assert scheduler.reserved_slots_by_reservation( + token, reservation.id + ).count() == 1 + + # ... and removing the reservation must not leave an orphaned slot + scheduler.remove_reservation(token, reservation.id) + scheduler.commit() + assert scheduler.reserved_slots_by_type(token, 'reservation').count() == 0 + + def test_remove_blocker_does_not_affect_sibling_blockers( scheduler: Scheduler, ) -> None: From 7f79f453f47e7e63ce4b852c770a252e8f413f36 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 13:47:36 +0200 Subject: [PATCH 3/7] Update history --- HISTORY.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/HISTORY.rst b/HISTORY.rst index 1548234..d759686 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -1,6 +1,14 @@ Changelog --------- +1.1.3 (unreleased) +~~~~~~~~~~~~~~~~~~~ + +- Fixes reserved_slots_by_reservation dropping the slot on non-partly_available allocations + + On a non-partly allocation the ReservedSlot spans the whole allocation even if the reservation was made for a narrower (but contained) range. Matching slots to a reservation by time range would then drop that slot, so removing the reservation left an orphaned ReservedSlot behind that kept showing up on the calendar. Slots are now attributed to non-partly reservations by allocation group; the time range is only used to disambiguate sibling reservations on partly available allocations. + [Tschuppi81] + 1.1.2 (16.06.2026) ~~~~~~~~~~~~~~~~~~~ From acc493b6ad1ffa18c0927a356e0c2f7ff36aa5bf Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 14:54:09 +0200 Subject: [PATCH 4/7] Simplify things with a dedicated source_id for ReservedSlot --- HISTORY.rst | 13 +++-- src/libres/db/models/reserved_slot.py | 4 ++ src/libres/db/scheduler.py | 78 +++++++++------------------ tests/test_scheduler.py | 34 ++++++++++++ 4 files changed, 73 insertions(+), 56 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index d759686..1047b6f 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -4,9 +4,16 @@ Changelog 1.1.3 (unreleased) ~~~~~~~~~~~~~~~~~~~ -- Fixes reserved_slots_by_reservation dropping the slot on non-partly_available allocations - - On a non-partly allocation the ReservedSlot spans the whole allocation even if the reservation was made for a narrower (but contained) range. Matching slots to a reservation by time range would then drop that slot, so removing the reservation left an orphaned ReservedSlot behind that kept showing up on the calendar. Slots are now attributed to non-partly reservations by allocation group; the time range is only used to disambiguate sibling reservations on partly available allocations. +- Records the owning reservation/blocker id on each reserved slot + + ReservedSlot gains a ``source_id`` column holding the id of its owning + reservation or blocker (see ``source_type``). A slot can now be attributed to + its exact object directly, instead of inferring it from the allocation and + time range. This fixes ``reserved_slots_by_reservation`` dropping the slot on + non-partly_available allocations (where the slot spans the whole allocation + and is wider than a narrower reservation), which left orphaned slots behind on + removal. Consumers must add the column and backfill it (see the onegov + ``add_source_id_to_reserved_slots`` upgrade). [Tschuppi81] 1.1.2 (16.06.2026) diff --git a/src/libres/db/models/reserved_slot.py b/src/libres/db/models/reserved_slot.py index 63351d1..afb6ba6 100644 --- a/src/libres/db/models/reserved_slot.py +++ b/src/libres/db/models/reserved_slot.py @@ -68,6 +68,10 @@ class ReservedSlot(TimestampMixin, ORMBase): reservation_token: Mapped[UUID] + # id of the owning Reservation or ReservationBlocker (see source_type); + # nullable only for rows predating the column (backfilled by a migration) + source_id: Mapped[int | None] = mapped_column(index=True) + __table_args__ = ( Index('reservation_resource_ix', 'reservation_token', 'resource'), # NOTE: GiST index for temporal queries on reserved slots diff --git a/src/libres/db/scheduler.py b/src/libres/db/scheduler.py index 16a9365..9fc751b 100644 --- a/src/libres/db/scheduler.py +++ b/src/libres/db/scheduler.py @@ -9,7 +9,7 @@ from sqlalchemy import exc from sqlalchemy import func from sqlalchemy.orm import selectinload -from sqlalchemy.sql import and_, not_, or_ +from sqlalchemy.sql import and_, not_ from uuid import uuid4 as new_uuid, UUID from libres.context.core import ContextServicesMixin @@ -1491,6 +1491,7 @@ def _approve_reservation_record( slot.resource = allocation.resource slot.reservation_token = reservation.token slot.source_type = 'reservation' + slot.source_id = reservation.id # the slots are written with the allocation allocation.reserved_slots.append(slot) @@ -1836,6 +1837,7 @@ def create_reserved_slots( allocation: Allocation, start: datetime, end: datetime, + source_id: int, including_mirrors: bool = True ) -> None: for slot_start, slot_end in allocation.all_slots(start, end): @@ -1845,6 +1847,7 @@ def create_reserved_slots( slot.resource = allocation.resource slot.reservation_token = token slot.source_type = 'blocker' + slot.source_id = source_id # the slots are written with the allocation allocation.reserved_slots.append(slot) @@ -1859,7 +1862,7 @@ def create_reserved_slots( for mirror in self.allocation_mirrors_by_master(allocation): create_reserved_slots( - mirror, start, end, + mirror, start, end, source_id, including_mirrors=False ) @@ -1878,11 +1881,15 @@ def new_blockers_by_group( blocker.resource = self.resource blocker.reason = reason + # flush to assign the blocker id before its slots reference it + self.session.add(blocker) + self.session.flush() for allocation in self.allocations_by_group(group): create_reserved_slots( allocation, allocation._start, - allocation._end + allocation._end, + blocker.id ) yield blocker @@ -1920,7 +1927,12 @@ def new_blockers_by_dates( blocker.resource = self.resource blocker.reason = reason - create_reserved_slots(allocation, start, end) + # flush to assign the id before its slots reference it + self.session.add(blocker) + self.session.flush() + create_reserved_slots( + allocation, start, end, blocker.id + ) yield blocker @@ -1936,9 +1948,6 @@ def new_blockers_by_dates( if not reserved_slots: raise errors.NotReservableError - for blocker in blockers: - self.session.add(blocker) - return blockers def remove_blocker( @@ -2020,7 +2029,14 @@ def change_blocker( reason=old_reason, token=token ) + # add_blocker linked the new slots to the freshly assigned id; + # realign them when we force the original id back onto the blocker + new_id = new_blocker.id new_blocker.id = id + if new_id != id: + for slot in self.reserved_slots_by_blocker(token).filter( + ReservedSlot.source_id == new_id): + slot.source_id = id return new_blocker @@ -2297,31 +2313,7 @@ def reserved_slots_by_reservation( if id is None: return query - # Map each slot to the requested reservation via its allocation. On - # partly_available allocations siblings of the same token are told - # apart by time range; non-partly allocations are reserved whole, so - # their slot is wider than the reservation and is matched by group. - # start is None for group reservations (include all their slots). - return ( - query - .join(Reservation, and_( - Reservation.token == ReservedSlot.reservation_token, - Reservation.id == id - )) - .join(Allocation, Allocation.id == ReservedSlot.allocation_id) - .filter(or_( - Reservation.start.is_(None), - and_( - Allocation.partly_available.is_(False), - Allocation.group == Reservation.target, - ), - and_( - Allocation.partly_available.is_(True), - ReservedSlot.start >= Reservation.start, - ReservedSlot.end <= Reservation.end, - ), - )) - ) + return query.filter(ReservedSlot.source_id == id) def reserved_slots_by_blocker( self, @@ -2338,27 +2330,7 @@ def reserved_slots_by_blocker( if id is None: return query - # Same rationale as reserved_slots_by_reservation. - return ( - query - .join(ReservationBlocker, and_( - ReservationBlocker.token == ReservedSlot.reservation_token, - ReservationBlocker.id == id - )) - .join(Allocation, Allocation.id == ReservedSlot.allocation_id) - .filter(or_( - ReservationBlocker.start.is_(None), - and_( - Allocation.partly_available.is_(False), - Allocation.group == ReservationBlocker.target, - ), - and_( - Allocation.partly_available.is_(True), - ReservedSlot.start >= ReservationBlocker.start, - ReservedSlot.end <= ReservationBlocker.end, - ), - )) - ) + return query.filter(ReservedSlot.source_id == id) def reservations_by_group(self, group: UUID) -> Query[Reservation]: tokens = self.managed_reservations().with_entities(Reservation.token) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index d9442d0..4ca3166 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -711,6 +711,7 @@ def test_remove_reservation_on_non_partly_allocation_removes_slot( scheduler.commit() reservation = scheduler.reservations_by_token(token).one() + assert reservation.start is not None and reservation.end is not None slot = scheduler.reserved_slots_by_reservation(token).one() # the slot is wider than the reservation range assert slot.start < reservation.start or slot.end > reservation.end @@ -726,6 +727,39 @@ def test_remove_reservation_on_non_partly_allocation_removes_slot( assert scheduler.reserved_slots_by_type(token, 'reservation').count() == 0 +def test_reserved_slots_store_source_id(scheduler: Scheduler) -> None: + """Every reserved slot records the id of its owning reservation/blocker + (source_id), so it can be attributed to its exact object directly.""" + dates = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0)) + scheduler.allocate(dates, partly_available=True) + + token = scheduler.reserve( + 'user@example.org', + (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 10, 0)) + ) + scheduler.commit() + scheduler.approve_reservations(token) + scheduler.commit() + + reservation = scheduler.reservations_by_token(token).one() + slots = scheduler.reserved_slots_by_reservation(token).all() + assert slots + for slot in slots: + assert slot.source_type == 'reservation' + assert slot.source_id == reservation.id + + blocker = scheduler.add_blocker( + (datetime(2014, 3, 7, 10, 0), datetime(2014, 3, 7, 12, 0)) + )[0] + scheduler.commit() + + blocker_slots = scheduler.reserved_slots_by_blocker(blocker.token).all() + assert blocker_slots + for slot in blocker_slots: + assert slot.source_type == 'blocker' + assert slot.source_id == blocker.id + + def test_remove_blocker_does_not_affect_sibling_blockers( scheduler: Scheduler, ) -> None: From a838bfc9f4500623da6f0ce09f8be2ea2b052435 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert <124258444+Tschuppi81@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:09:04 -0400 Subject: [PATCH 5/7] source_id shall not be nullable Co-authored-by: David Salvisberg --- src/libres/db/models/reserved_slot.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libres/db/models/reserved_slot.py b/src/libres/db/models/reserved_slot.py index afb6ba6..0456c35 100644 --- a/src/libres/db/models/reserved_slot.py +++ b/src/libres/db/models/reserved_slot.py @@ -70,7 +70,7 @@ class ReservedSlot(TimestampMixin, ORMBase): # id of the owning Reservation or ReservationBlocker (see source_type); # nullable only for rows predating the column (backfilled by a migration) - source_id: Mapped[int | None] = mapped_column(index=True) + source_id: Mapped[int] = mapped_column(index=True) __table_args__ = ( Index('reservation_resource_ix', 'reservation_token', 'resource'), From 313f24722bff598a147187ba32f636a84e397266 Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 15:33:39 +0200 Subject: [PATCH 6/7] Integrate pr comments --- src/libres/db/models/reserved_slot.py | 3 +-- src/libres/db/scheduler.py | 28 +++++++++++++-------------- tests/test_allocation.py | 2 ++ tests/test_scheduler.py | 1 + 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/libres/db/models/reserved_slot.py b/src/libres/db/models/reserved_slot.py index 0456c35..26a80e7 100644 --- a/src/libres/db/models/reserved_slot.py +++ b/src/libres/db/models/reserved_slot.py @@ -68,8 +68,7 @@ class ReservedSlot(TimestampMixin, ORMBase): reservation_token: Mapped[UUID] - # id of the owning Reservation or ReservationBlocker (see source_type); - # nullable only for rows predating the column (backfilled by a migration) + # id of the owning Reservation or ReservationBlocker (see source_type) source_id: Mapped[int] = mapped_column(index=True) __table_args__ = ( diff --git a/src/libres/db/scheduler.py b/src/libres/db/scheduler.py index 9fc751b..3f2c043 100644 --- a/src/libres/db/scheduler.py +++ b/src/libres/db/scheduler.py @@ -1730,7 +1730,8 @@ def add_blocker( dates: _dtrange | Collection[_dtrange], group: None = ..., reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... @overload @@ -1739,7 +1740,8 @@ def add_blocker( dates: None, group: UUID, reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... @overload @@ -1749,7 +1751,8 @@ def add_blocker( *, group: UUID, reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... def add_blocker( @@ -1757,7 +1760,8 @@ def add_blocker( dates: _dtrange | Collection[_dtrange] | None = None, group: UUID | None = None, reason: str | None = None, - token: UUID | None = None + token: UUID | None = None, + id: int | None = None ) -> list[ReservationBlocker]: """ Adds a blocker to one or many allocations. @@ -1828,7 +1832,6 @@ def add_blocker( elif not allocation.contains(start, end): raise errors.TimerangeTooLong - # ok, we're good to go if token is None: token = new_uuid() reserved_slots = [] @@ -1880,6 +1883,8 @@ def new_blockers_by_group( blocker.target_type = 'group' blocker.resource = self.resource blocker.reason = reason + if id is not None: + blocker.id = id # flush to assign the blocker id before its slots reference it self.session.add(blocker) @@ -1926,6 +1931,8 @@ def new_blockers_by_dates( blocker.target_type = 'allocation' blocker.resource = self.resource blocker.reason = reason + if id is not None: + blocker.id = id # flush to assign the id before its slots reference it self.session.add(blocker) @@ -2027,16 +2034,9 @@ def change_blocker( new_blocker, = self.add_blocker( dates=(new_start, new_end), reason=old_reason, - token=token + token=token, + id=id ) - # add_blocker linked the new slots to the freshly assigned id; - # realign them when we force the original id back onto the blocker - new_id = new_blocker.id - new_blocker.id = id - if new_id != id: - for slot in self.reserved_slots_by_blocker(token).filter( - ReservedSlot.source_id == new_id): - slot.source_id = id return new_blocker diff --git a/tests/test_allocation.py b/tests/test_allocation.py index 68fabff..a1e8c39 100644 --- a/tests/test_allocation.py +++ b/tests/test_allocation.py @@ -255,6 +255,7 @@ def add_reservation( slot.allocation = allocation slot.reservation_token = reservation slot.source_type = 'reservation' + slot.source_id = 1 # synthetic slot, no real owner scheduler.session.add(slot) scheduler.session.flush() scheduler.session.refresh(allocation) @@ -281,6 +282,7 @@ def add_blocker( slot.allocation = allocation slot.reservation_token = blocker slot.source_type = 'blocker' + slot.source_id = 1 # synthetic slot, no real owner scheduler.session.add(slot) scheduler.session.flush() scheduler.session.refresh(allocation) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 4ca3166..ead75db 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -751,6 +751,7 @@ def test_reserved_slots_store_source_id(scheduler: Scheduler) -> None: blocker = scheduler.add_blocker( (datetime(2014, 3, 7, 10, 0), datetime(2014, 3, 7, 12, 0)) )[0] + assert blocker.id is not None scheduler.commit() blocker_slots = scheduler.reserved_slots_by_blocker(blocker.token).all() From 586b0b073b810516bb8830a2598163dcdbd1895c Mon Sep 17 00:00:00 2001 From: Reto Tschuppert Date: Thu, 13 Aug 2026 15:59:09 +0200 Subject: [PATCH 7/7] Fix linter --- tests/test_scheduler.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index ead75db..bef3e32 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -711,7 +711,8 @@ def test_remove_reservation_on_non_partly_allocation_removes_slot( scheduler.commit() reservation = scheduler.reservations_by_token(token).one() - assert reservation.start is not None and reservation.end is not None + assert reservation.start is not None + assert reservation.end is not None slot = scheduler.reserved_slots_by_reservation(token).one() # the slot is wider than the reservation range assert slot.start < reservation.start or slot.end > reservation.end