Browse Source

feat: authoritative per-reservation-ID status check

Adds check_reservation_status to both clients (Airbnb via
StayHostingDetailsQuery's isCanceled, Booking via reservationDetails'
is_cancelled -- no new credential capture needed for the latter, a
HAR confirmed the x-booking-as-token some other page attaches isn't
endpoint-enforced) and a hasattr-dispatched ServiceAPI entry point.

fetch_reservations.py's deletion guard moves from per-service to
per-ID: a service returning 0 reservations with N stored no longer
blanket-guards the whole service -- each missing ID is checked
individually, with the existing delayed re-query as fallback for
whatever the status check can't resolve. Fixes a slow-season false
positive where a genuinely canceled reservation got stuck forever
because a service legitimately having 0-1 bookings in a window was
treated as inherently suspicious.
ZiB 5 days ago
parent
commit
c7ad494408

+ 73 - 8
room_cal/actions/fetch_reservations.py

@@ -27,6 +27,42 @@ def _source_matches(stored: dict, domain: str, key: str, id_: str) -> bool:
   )
 
 
+# Delay before the confirmation re-query, mirroring MonthCache's empty/priceless
+# suspect-retry delay — long enough for a stale read-replica lag to clear.
+_CONFIRM_RETRY_DELAY = 1.0
+
+
+async def _confirm_missing(
+  api: 'ServiceAPI',
+  svc: str,
+  date_from: date,
+  date_to: date,
+  missing_ids: set[str],
+) -> bool:
+  """Re-query a service's reservation list before trusting that stored IDs are gone.
+
+  In slow season a service legitimately having 0-1 bookings for a window is no
+  longer rare, so "returned none, N stored" can't be treated as inherently
+  suspicious on its own. A fresh re-query of the same endpoint — not the
+  calendar, which can be reading from the same cached month payload the first
+  get_reservations call was parsed from (Airbnb) or its own eventual-consistency
+  window (Booking) — catches a read that landed on a stale replica or lost a
+  race with the service's own write path, without introducing a second,
+  unverified data source.
+  """
+  try:
+    client = api.client(svc)
+  except (KeyError, AttributeError):
+    return False
+  await asyncio.sleep(_CONFIRM_RETRY_DELAY)
+  try:
+    reservations = await client.get_reservations(date_from.isoformat(), date_to.isoformat())
+  except Exception:
+    return False
+  still_present = {r.reservation_id for r in reservations} & missing_ids
+  return not still_present
+
+
 async def _remove_reservation_closures(
   closed: Closed,
   checkin_date: date,
@@ -86,6 +122,7 @@ async def fetch_reservations(
     if svc:
       stored_by_service[svc] = stored_by_service.get(svc, 0) + 1
   guarded_services: set[str] = set()
+  guarded_ids: set[str] = set()
   for svc, count in stored_by_service.items():
     if svc in errored_services:
       _log.error(
@@ -94,19 +131,47 @@ async def fetch_reservations(
       )
       guarded_services.add(svc)
     elif svc not in returned_services:
-      _log.error(
-        'fetch_reservations: %r returned 0 reservations but %d are stored; '
-        'skipping deletion(s) — verify service is healthy',
-        svc, count,
-      )
-      guarded_services.add(svc)
+      missing_ids = {
+        rid for rid, (_, data) in existing_by_id.items() if data.get('service') == svc
+      }
+      # Per-ID authoritative check first (StayHostingDetailsQuery's isCanceled for Airbnb,
+      # reservationDetails' is_cancelled for Booking) — a service legitimately having 0-1
+      # bookings in a window is no longer rare in slow season, so "returned none" alone can't
+      # justify blanket-guarding the whole service. hasattr-dispatched: a client without the
+      # capability (api.check_reservation_status returns None for every id) falls straight
+      # through to the bulk re-query fallback below, unchanged from before.
+      check_status = getattr(api, 'check_reservation_status', None)
+      confirmed_canceled: set[str] = set()
+      unresolved: set[str] = set()
+      for rid in missing_ids:
+        status = await check_status(svc, rid) if check_status else None
+        if status:
+          confirmed_canceled.add(rid)
+        else:
+          unresolved.add(rid)
+      if unresolved and await _confirm_missing(api, svc, start_date, end_date, unresolved):
+        confirmed_canceled |= unresolved
+        unresolved = set()
+      if confirmed_canceled:
+        _log.info(
+          'fetch_reservations: %r returned 0 reservations; %d of %d stored reservation(s) '
+          'confirmed genuinely gone — proceeding with deletion(s)',
+          svc, len(confirmed_canceled), count,
+        )
+      if unresolved:
+        _log.error(
+          'fetch_reservations: %r returned 0 reservations; %d of %d stored reservation(s) '
+          'could not be confirmed gone — skipping deletion(s) — verify service is healthy',
+          svc, len(unresolved), count,
+        )
+        guarded_ids.update(unresolved)
 
   # Mass-deletion guard: cancel a service's deletions when a single run would
   # remove more than _MAX_DELETIONS_PER_RUN reservations.
   pending_deletions: dict[str, int] = {}
   for rid, (_, data) in existing_by_id.items():
     svc = data.get('service', '')
-    if svc in guarded_services or rid in service_ids:
+    if svc in guarded_services or rid in guarded_ids or rid in service_ids:
       continue
     pending_deletions[svc] = pending_deletions.get(svc, 0) + 1
   for svc, count in pending_deletions.items():
@@ -121,7 +186,7 @@ async def fetch_reservations(
   changed = False
 
   for rid, (event, data) in existing_by_id.items():
-    if data.get('service') in guarded_services:
+    if data.get('service') in guarded_services or rid in guarded_ids:
       continue
     if rid not in service_ids:
       changed = True

+ 26 - 0
room_cal/service_api/_airbnb.py

@@ -474,6 +474,32 @@ class _AirbnbClient(ServiceClientBase):
     self._details.set(code, det)
     return det
 
+  def _detail_is_canceled(self, det: dict) -> bool | None:
+    """Read cancellation status from a detail's WHOSCOMING_SECTION, if present."""
+    wc_groups = (_detail_section(det, 'WHOSCOMING_SECTION').get('sectionData') or {}).get('reservationGroups', [])
+    if not wc_groups:
+      return None
+    return bool(wc_groups[0].get('isCanceled'))
+
+  async def check_reservation_status(self, reservation_id: str) -> bool | None:
+    """Return True if `reservation_id` is confirmed canceled, None if status can't be determined.
+
+    Queries StayHostingDetailsQuery directly by confirmation code — independent of the
+    calendar/MonthCache, and reuses the same _details TTL cache get_booked_nets/get_reservations
+    populate, so a prior fetch of the same reservation costs no extra network call. Any failure
+    (stale credentials, timeout, unparseable response) falls back to None rather than raising —
+    callers treat this as a best-effort check, not a required one.
+    """
+    try:
+      self._check_fresh()
+      det = await self._fetch_one_detail(reservation_id)
+      if not det:
+        return None
+      return self._detail_is_canceled(det)
+    except Exception:
+      _log.warning('airbnb: check_reservation_status failed for %s', reservation_id, exc_info=True)
+      return None
+
   def _global_thread_id(self, thread_id: str) -> str:
     return base64.b64encode(f'MessageThread:{thread_id}'.encode()).decode()
 

+ 35 - 0
room_cal/service_api/_booking.py

@@ -518,6 +518,41 @@ class _BookingClient(ServiceClientBase):
       for rid, r_data in reservations.items()
     ]
 
+  async def check_reservation_status(self, reservation_id: str) -> bool | None:
+    """Return True if `reservation_id` is confirmed canceled, None if status can't be determined.
+
+    Queries reservationDetails directly by ID — independent of get_reservations/the calendar.
+    Uses the messaging-inbox referer; the x-booking-as-token some other page attaches to this
+    same endpoint isn't required (confirmed via a HAR where the token-less call succeeded
+    identically), and every refresh already primes the messaging session before booking.env is
+    written, so the token-less path is always available at runtime. Any failure (stale
+    credentials, timeout, unparseable response) falls back to None rather than raising —
+    callers treat this as a best-effort check, not a required one.
+    """
+    try:
+      self._check_fresh()
+      async def _attempt() -> httpx.Response:
+        referer = self._messaging_referer()
+        params = {
+          'ses': self._session,
+          'hotel_id': str(self._hotel_id),
+          'lang': self._lang,
+          'res_id': reservation_id,
+        }
+        return await self._http.get(
+          f'{_BASE}/fresa/extranet/messaging/inbox/reservationDetails',
+          params=params,
+          headers=self._rest_headers(referer),
+        )
+      r = await self._request(_attempt, op='reservationDetails')
+      data = r.json().get('data') or {}
+      if 'is_cancelled' not in data:
+        return None
+      return bool(data.get('is_cancelled'))
+    except Exception:
+      _log.warning('booking: check_reservation_status failed for %s', reservation_id, exc_info=True)
+      return None
+
   async def get_thread(self, reservation_id: str) -> BookingThread:
     """Fetch a conversation thread and its messages by reservation ID."""
     self._check_fresh()

+ 12 - 0
room_cal/service_api/client.py

@@ -299,6 +299,18 @@ class ServiceAPI:
     result.sort(key=lambda x: x['start_date'])
     return result
 
+  async def check_reservation_status(self, service: str, reservation_id: str) -> bool | None:
+    """Return True if `reservation_id` on `service` is confirmed canceled, None otherwise.
+
+    None covers both "the client doesn't implement this capability" and "it tried and
+    couldn't determine" — callers should treat both the same way (fall back to another
+    signal), not distinguish them.
+    """
+    client = self._clients.get(service)
+    if client is None or not hasattr(client, 'check_reservation_status'):
+      return None
+    return await client.check_reservation_status(reservation_id)  # type: ignore[attr-defined]
+
   async def diff_remote_sync(self, start_date: date, end_date: date) -> dict[str, list[dict]]:
     """Return per-service integrity issues without making any changes."""
     ds_from, ds_to = start_date.isoformat(), end_date.isoformat()

+ 150 - 0
tests/test_check_reservation_status.py

@@ -0,0 +1,150 @@
+"""Unit tests for the check_reservation_status capability (both clients) and
+ServiceAPI.check_reservation_status's hasattr dispatch.
+
+Deep response-parsing lives here per client; the cross-client contract
+(True = confirmed canceled, None = couldn't determine, never raises) is what
+fetch_reservations.py's guard (tests/test_reservation_guards.py) relies on.
+"""
+import httpx
+import pytest
+
+from room_cal.service_api.client import ServiceAPI
+
+
+def _mock_transport(responses: list) -> httpx.MockTransport:
+  it = iter(responses)
+
+  def handler(request: httpx.Request) -> httpx.Response:
+    item = next(it)
+    if isinstance(item, BaseException):
+      raise item
+    return item
+
+  return httpx.MockTransport(handler)
+
+
+def _airbnb_client():
+  from room_cal.service_api._airbnb import _AirbnbClient
+  return _AirbnbClient(cookies='test', listing_id='12345678')
+
+
+def _booking_client():
+  from room_cal.service_api._booking import _BookingClient
+  return _BookingClient(
+    cookies='test', session='ses1', csrf_token='csrf1',
+    hotel_id=1, account_id=1, room_id=1, rate_id='rate1',
+  )
+
+
+def _stay_hosting_details_response(is_canceled: bool) -> httpx.Response:
+  return httpx.Response(200, json={
+    'data': {
+      'presentation': {
+        'hostingDetails': {
+          'stayHostingDetails': {
+            'rootPlacement': [
+              {
+                'sectionId': 'WHOSCOMING_SECTION',
+                'sectionData': {'reservationGroups': [{'isCanceled': is_canceled}]},
+              },
+            ],
+          },
+        },
+      },
+    },
+  })
+
+
+class TestAirbnbCheckReservationStatus:
+  async def test_canceled(self):
+    client = _airbnb_client()
+    client._http = httpx.AsyncClient(transport=_mock_transport([_stay_hosting_details_response(True)]))
+    assert await client.check_reservation_status('HMXKNXCSSS') is True
+
+  async def test_not_canceled(self):
+    client = _airbnb_client()
+    client._http = httpx.AsyncClient(transport=_mock_transport([_stay_hosting_details_response(False)]))
+    assert await client.check_reservation_status('HMFARE3JC4') is False
+
+  async def test_no_data_returns_none(self):
+    """A confirmation code with no matching detail (e.g. typo) can't be determined."""
+    client = _airbnb_client()
+    client._http = httpx.AsyncClient(
+      transport=_mock_transport([httpx.Response(200, json={'data': {'presentation': {}}})]))
+    assert await client.check_reservation_status('BOGUS') is None
+
+  async def test_timeout_returns_none_not_raise(self):
+    client = _airbnb_client()
+    client._http = httpx.AsyncClient(transport=_mock_transport([httpx.TimeoutException('slow')]))
+    assert await client.check_reservation_status('HMFARE3JC4') is None
+
+  async def test_reuses_details_cache(self):
+    """A reservation already fetched (e.g. by get_booked_nets) makes no extra network call."""
+    client = _airbnb_client()
+    client._details.set('HMXKNXCSSS', {
+      'rootPlacement': [
+        {'sectionId': 'WHOSCOMING_SECTION', 'sectionData': {'reservationGroups': [{'isCanceled': True}]}},
+      ],
+    })
+    client._http = httpx.AsyncClient(transport=_mock_transport([]))  # any call fails the test
+    assert await client.check_reservation_status('HMXKNXCSSS') is True
+
+
+class TestBookingCheckReservationStatus:
+  async def test_canceled(self):
+    client = _booking_client()
+    client._http = httpx.AsyncClient(
+      transport=_mock_transport([httpx.Response(200, json={'success': 1, 'data': {'is_cancelled': 1}})]))
+    assert await client.check_reservation_status('5124584185') is True
+
+  async def test_not_canceled(self):
+    client = _booking_client()
+    client._http = httpx.AsyncClient(
+      transport=_mock_transport([httpx.Response(200, json={'success': 1, 'data': {'is_cancelled': 0}})]))
+    assert await client.check_reservation_status('5162718780') is False
+
+  async def test_missing_field_returns_none(self):
+    client = _booking_client()
+    client._http = httpx.AsyncClient(
+      transport=_mock_transport([httpx.Response(200, json={'success': 1, 'data': {}})]))
+    assert await client.check_reservation_status('0000000000') is None
+
+  async def test_transport_error_returns_none_not_raise(self):
+    client = _booking_client()
+    client._http = httpx.AsyncClient(transport=_mock_transport([httpx.ConnectError('down')]))
+    assert await client.check_reservation_status('5124584185') is None
+
+  async def test_no_as_token_header_sent(self):
+    """The call relies on the post-messaging-prime session, not a page-bootstrapped token."""
+    client = _booking_client()
+    captured: list[httpx.Request] = []
+
+    def handler(request: httpx.Request) -> httpx.Response:
+      captured.append(request)
+      return httpx.Response(200, json={'success': 1, 'data': {'is_cancelled': 0}})
+
+    client._http = httpx.AsyncClient(transport=httpx.MockTransport(handler))
+    await client.check_reservation_status('5162718780')
+    assert len(captured) == 1
+    assert 'x-booking-as-token' not in {h.lower() for h in captured[0].headers.keys()}
+    assert 'hotel_account_id' not in captured[0].url.params
+
+
+class TestServiceAPIDispatch:
+  async def test_unknown_service_returns_none(self):
+    api = ServiceAPI(clients={})
+    assert await api.check_reservation_status('airbnb', 'ANY') is None
+
+  async def test_client_without_capability_returns_none(self):
+    class _NoCapabilityClient:
+      pass
+    api = ServiceAPI(clients={'airbnb': _NoCapabilityClient()})
+    assert await api.check_reservation_status('airbnb', 'ANY') is None
+
+  async def test_dispatches_to_client_method(self):
+    class _StubClient:
+      async def check_reservation_status(self, reservation_id):
+        return reservation_id == 'CANCELED-ID'
+    api = ServiceAPI(clients={'airbnb': _StubClient()})
+    assert await api.check_reservation_status('airbnb', 'CANCELED-ID') is True
+    assert await api.check_reservation_status('airbnb', 'OTHER-ID') is False

+ 29 - 0
tests/test_reservation_guards.py

@@ -55,6 +55,15 @@ class _PartialAPI(_BaseAPI):
     return self._res
 
 
+class _StatusCheckAPI(_BaseAPI):
+  """Simulates a client implementing the check_reservation_status capability."""
+  def __init__(self, statuses: dict[str, bool | None]):
+    self._statuses = statuses
+
+  async def check_reservation_status(self, service, reservation_id):
+    return self._statuses.get(reservation_id)
+
+
 class _NoopClosed:
   """Stand-in for Closed so guard tests don't write closure events to the real store."""
   def __init__(self, *a, **k): pass
@@ -238,6 +247,26 @@ class TestFetchReservationsGuard:
     result = await fetch_reservations(_CHECKIN, _CHECKOUT, api=_BaseAPI())
     assert result.data == []
 
+  async def test_status_check_confirms_cancellation_despite_zero_return(self):
+    """check_reservation_status confirming True lets deletion proceed even with 0 returned."""
+    rid = 'TEST-GUARD-STATUS-CONFIRMED-001'
+    await _seed('airbnb', rid)
+    try:
+      await fetch_reservations(_CHECKIN, _CHECKOUT, api=_StatusCheckAPI({rid: True}))
+      assert not await _exists(rid), "status check confirming canceled must allow deletion"
+    finally:
+      await _delete(rid)
+
+  async def test_status_check_ambiguous_falls_back_to_guard(self):
+    """check_reservation_status returning None (ambiguous) still guards, like no capability at all."""
+    rid = 'TEST-GUARD-STATUS-AMBIGUOUS-001'
+    await _seed('airbnb', rid)
+    try:
+      await fetch_reservations(_CHECKIN, _CHECKOUT, api=_StatusCheckAPI({rid: None}))
+      assert await _exists(rid), "ambiguous status must not authorize deletion"
+    finally:
+      await _delete(rid)
+
 
 # ── fetch_reservations mass-deletion guard integration tests ──────────────────