From e249f803391d7798f40f6638a7fee527e0e8cb2d Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 19 Apr 2026 08:51:44 +0000 Subject: [PATCH 1/3] fix(lock): replace fragile ISO string comparison with datetime comparison in LockService Added a new helper function _to_aware_datetime() in lock_service.py that normalizes ORM timestamp values to timezone-aware datetime objects, handling naive datetimes, timezone-aware datetimes, and ISO strings. Fixed LockService.acquire() to use datetime comparison instead of fragile string comparison for lock expiry detection (bug #10483). Fixed LockService.renew() similarly to use datetime comparison. Added a new TDD Behave feature file features/tdd_lock_service_naive_datetime_expiry.feature with a scenario that proves the fix works. Added step definitions features/steps/tdd_lock_service_naive_datetime_expiry_steps.py. ISSUES CLOSED: #10483 --- ...ock_service_naive_datetime_expiry_steps.py | 150 ++++++++++++++++++ ...lock_service_naive_datetime_expiry.feature | 25 +++ .../application/services/lock_service.py | 43 ++++- 3 files changed, 210 insertions(+), 8 deletions(-) create mode 100644 features/steps/tdd_lock_service_naive_datetime_expiry_steps.py create mode 100644 features/tdd_lock_service_naive_datetime_expiry.feature diff --git a/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py b/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py new file mode 100644 index 000000000..7df6b2840 --- /dev/null +++ b/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py @@ -0,0 +1,150 @@ +"""Step definitions for tdd_lock_service_naive_datetime_expiry.feature. + +TDD test for bug #10483: LockService.acquire() must use datetime comparison +for lock expiry, not fragile string comparison. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta +from typing import Any + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine, event +from sqlalchemy.orm import Session, sessionmaker + +from cleveragents.application.services.lock_service import LockService +from cleveragents.core.exceptions import LockConflictError +from cleveragents.infrastructure.database.models import Base, LockModel + + +def _make_lock_service_10483(context: Context) -> LockService: + """Create a LockService backed by an in-memory SQLite database.""" + engine = create_engine( + "sqlite:///:memory:", + echo=False, + future=True, + connect_args={"check_same_thread": False}, + ) + + @event.listens_for(engine, "connect") + def _fk(dbapi_conn: Any, _rec: Any) -> None: + cursor = dbapi_conn.cursor() + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + + Base.metadata.create_all(engine) + factory: sessionmaker[Session] = sessionmaker( + bind=engine, + expire_on_commit=False, + autoflush=False, + autocommit=False, + class_=Session, + ) + context.tdd10483_engine = engine + context.tdd10483_session_factory = factory + return LockService(session_factory=factory) + + +@given("a lock service with a real database for tdd 10483") +def step_tdd10483_setup(context: Context) -> None: + """Create a fresh in-memory LockService for TDD #10483 scenario.""" + context.tdd10483_svc = _make_lock_service_10483(context) + context.tdd10483_resource_type = "plan" + context.tdd10483_resource_id = "plan-tdd-10483" + context.tdd10483_holder_owner = "original-owner-10483" + context.tdd10483_new_owner = "new-owner-10483" + context.tdd10483_result = None + context.tdd10483_error = None + + +@given("a non-expired lock exists with a naive datetime expires_at for tdd 10483") +def step_tdd10483_insert_naive_active_lock(context: Context) -> None: + """Insert a non-expired lock whose expires_at is a naive datetime object. + + This simulates what some database drivers return: a naive datetime + (without timezone info) instead of a timezone-aware datetime or ISO string. + + The bug: str(naive_datetime) produces e.g. '2026-04-19 06:00:00' (space + separator), while now_iso uses isoformat() producing e.g. + '2026-04-19T05:30:18+00:00' (T separator). Since space (ASCII 32) < T + (ASCII 84), the comparison str(naive_datetime) >= now_iso is ALWAYS False, + regardless of whether the lock has actually expired. This means a + non-expired lock is silently treated as expired, allowing lock theft. + """ + now = datetime.now(tz=UTC) + # Create a future expiry time: 300 seconds from now (clearly non-expired) + expires_at_aware = now + timedelta(seconds=300) + acquired_at_aware = now - timedelta(seconds=10) + + # Simulate what some ORM drivers return: a naive datetime (no tzinfo). + # str(naive_datetime) = '2026-04-19 06:00:00' (space separator) + # now_iso = '2026-04-19T05:30:18+00:00' (T separator) + # '2026-04-19 06:00:00' >= '2026-04-19T05:30:18+00:00' -> False (space < T) + # Bug: the non-expired lock is incorrectly treated as expired! + expires_at_naive = expires_at_aware.replace(tzinfo=None) + acquired_at_naive = acquired_at_aware.replace(tzinfo=None) + + session: Session = context.tdd10483_session_factory() + lock = LockModel( + owner_id=context.tdd10483_holder_owner, + resource_type=context.tdd10483_resource_type, + resource_id=context.tdd10483_resource_id, + acquired_at=acquired_at_naive, + expires_at=expires_at_naive, + ) + session.add(lock) + session.commit() + session.close() + + +@when("a different owner tries to acquire the same resource for tdd 10483") +def step_tdd10483_acquire(context: Context) -> None: + """Attempt to acquire the lock as a different owner. + + With the bug present: str(naive_future_datetime) produces + '2026-04-19 06:00:00' (space separator), which compares as LESS THAN + now_iso '2026-04-19T05:30:18+00:00' (T separator) because space (32) < T + (84). So the condition existing_expires >= now_iso is False, and the lock + is incorrectly treated as expired -- no LockConflictError is raised. + + With the fix: datetime objects are compared directly after normalizing + timezone info. The future datetime is correctly identified as non-expired, + and LockConflictError is raised. + """ + try: + context.tdd10483_result = context.tdd10483_svc.acquire( + owner_id=context.tdd10483_new_owner, + resource_type=context.tdd10483_resource_type, + resource_id=context.tdd10483_resource_id, + ) + except LockConflictError as exc: + context.tdd10483_error = exc + except Exception as exc: + context.tdd10483_error = exc + + +@then( + "a LockConflictError should be raised because the lock is still active for tdd 10483" +) +def step_tdd10483_assert_conflict(context: Context) -> None: + """Assert that LockConflictError was raised (the lock is still active). + + The fix must compare datetime objects directly, normalizing timezone info: + 1. If expires_at is a string, parse it with fromisoformat(). + 2. If the resulting datetime is naive (no tzinfo), add UTC timezone. + 3. Compare the datetime object against now (also UTC-aware). + + Without the fix, this assertion fails because the naive datetime string + comparison (space < T) incorrectly treats the non-expired lock as expired, + allowing lock theft without raising LockConflictError. + """ + assert isinstance(context.tdd10483_error, LockConflictError), ( + f"Expected LockConflictError to be raised (lock is still active) but got: " + f"{type(context.tdd10483_error).__name__ if context.tdd10483_error else 'no error'} " + f"(result={context.tdd10483_result!r}). " + "This is bug #10483: str(naive_datetime) uses space separator which sorts " + "before 'T' in ISO strings, causing non-expired locks to be incorrectly " + "treated as expired." + ) diff --git a/features/tdd_lock_service_naive_datetime_expiry.feature b/features/tdd_lock_service_naive_datetime_expiry.feature new file mode 100644 index 000000000..d28828c6f --- /dev/null +++ b/features/tdd_lock_service_naive_datetime_expiry.feature @@ -0,0 +1,25 @@ +Feature: TDD Bug #10483 — LockService.acquire() must use datetime comparison for lock expiry + As a developer using LockService + I want lock expiry to be determined by comparing datetime objects + So that non-expired locks held by other owners are not incorrectly treated as expired + + # This test captures bug #10483. The previous implementation converted + # expires_at to a string via str() and compared it lexicographically + # against an ISO 8601 string produced by isoformat(). + # + # The bug: str(naive_datetime) uses a space separator (e.g. '2026-04-19 06:00:00') + # while isoformat() uses 'T' (e.g. '2026-04-19T05:30:18+00:00'). + # Since space (ASCII 32) < 'T' (ASCII 84), the comparison + # str(naive_datetime) >= now_iso was ALWAYS False regardless of whether + # the lock had actually expired. This meant a non-expired lock held by + # another owner was silently treated as expired, allowing lock theft. + # + # The fix replaces the fragile string comparison with a proper datetime + # comparison that normalises timezone info before comparing. + + @tdd_issue @tdd_issue_10483 + Scenario: Bug #10483 — non-expired lock with naive datetime expires_at is not incorrectly treated as expired + Given a lock service with a real database for tdd 10483 + And a non-expired lock exists with a naive datetime expires_at for tdd 10483 + When a different owner tries to acquire the same resource for tdd 10483 + Then a LockConflictError should be raised because the lock is still active for tdd 10483 diff --git a/src/cleveragents/application/services/lock_service.py b/src/cleveragents/application/services/lock_service.py index f4c76214d..8b07f9200 100644 --- a/src/cleveragents/application/services/lock_service.py +++ b/src/cleveragents/application/services/lock_service.py @@ -48,6 +48,30 @@ MIN_LOCK_TTL_SECS: int = 5 VALID_RESOURCE_TYPES: frozenset[str] = frozenset({"plan", "project"}) +def _to_aware_datetime(value: datetime | str) -> datetime: + """Normalise an ORM timestamp value to a timezone-aware ``datetime``. + + Different database drivers and ORM configurations may return + ``expires_at`` / ``acquired_at`` as: + + * A timezone-aware ``datetime`` (ideal case — returned as-is). + * A naive ``datetime`` (no ``tzinfo``) — UTC is assumed and added. + * An ISO 8601 string (e.g. ``'2026-04-19T06:00:00+00:00'`` or + ``'2026-04-19 06:00:00'``) — parsed with ``fromisoformat`` and + then normalised as above. + + Args: + value: The raw ORM column value for a timestamp field. + + Returns: + A timezone-aware ``datetime`` in UTC. + """ + dt: datetime = datetime.fromisoformat(value) if isinstance(value, str) else value + if dt.tzinfo is None: + dt = dt.replace(tzinfo=UTC) + return dt + + class LockService: """Advisory lock manager for plan and project resources. @@ -157,7 +181,6 @@ class LockService: now = datetime.now(tz=UTC) expires = now + timedelta(seconds=ttl_seconds) - now_iso = now.isoformat() expires_iso = expires.isoformat() session = self._session_factory() @@ -170,7 +193,6 @@ class LockService: if existing is not None: existing_owner = str(existing.owner_id) - existing_expires = str(existing.expires_at) if existing_owner == owner_id: # Re-entrant: extend TTL @@ -184,8 +206,10 @@ class LockService: ) return True - # Different owner — check expiry - if existing_expires >= now_iso: + # Different owner — check expiry using datetime comparison to + # avoid fragile ISO string comparison (bug #10483). + existing_expires_dt = _to_aware_datetime(existing.expires_at) + if existing_expires_dt >= now: raise LockConflictError( resource_type=resource_type, resource_id=resource_id, @@ -194,7 +218,7 @@ class LockService: # Expired: replace existing.owner_id = owner_id - existing.acquired_at = now_iso + existing.acquired_at = now.isoformat() existing.expires_at = expires_iso session.commit() self._logger.info( @@ -210,7 +234,7 @@ class LockService: owner_id=owner_id, resource_type=resource_type, resource_id=resource_id, - acquired_at=now_iso, + acquired_at=now.isoformat(), expires_at=expires_iso, ) session.add(lock) @@ -307,7 +331,6 @@ class LockService: self._validate_ttl(ttl_seconds) now = datetime.now(tz=UTC) - now_iso = now.isoformat() expires_iso = (now + timedelta(seconds=ttl_seconds)).isoformat() session = self._session_factory() @@ -321,7 +344,11 @@ class LockService: if lock is None: return False - if str(lock.expires_at) < now_iso: + # Use datetime comparison to avoid fragile ISO string comparison + # (bug #10483): str(datetime) uses space separator while isoformat() + # uses 'T', causing incorrect lexicographic ordering. + lock_expires_dt = _to_aware_datetime(lock.expires_at) + if lock_expires_dt < now: raise LockExpiredError( resource_type=resource_type, resource_id=resource_id, -- 2.52.0 From 92feab2a2d7ce9e133b128fc4fb4d011e51fd3da Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 23 Apr 2026 22:35:52 +0000 Subject: [PATCH 2/3] =?UTF-8?q?fix(lock):=20address=20reviewer=20suggestio?= =?UTF-8?q?ns=20=E2=80=94=20add=20ValueError=20handling=20and=20ISO=20stri?= =?UTF-8?q?ng=20test=20scenario?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...ock_service_naive_datetime_expiry_steps.py | 31 +++++++++++++++++++ ...lock_service_naive_datetime_expiry.feature | 7 +++++ .../application/services/lock_service.py | 14 ++++++++- 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py b/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py index 7df6b2840..cda7dee62 100644 --- a/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py +++ b/features/steps/tdd_lock_service_naive_datetime_expiry_steps.py @@ -99,6 +99,37 @@ def step_tdd10483_insert_naive_active_lock(context: Context) -> None: session.close() +@given("a non-expired lock exists with an ISO string expires_at for tdd 10483") +def step_tdd10483_insert_iso_string_active_lock(context: Context) -> None: + """Insert a non-expired lock whose expires_at is an ISO 8601 string. + + This verifies that the fix correctly handles ISO string timestamps + returned by some database drivers. The string uses the 'T' separator + (as produced by datetime.isoformat()) and includes timezone info. + """ + now = datetime.now(tz=UTC) + # Create a future expiry time: 300 seconds from now (clearly non-expired) + expires_at_aware = now + timedelta(seconds=300) + acquired_at_aware = now - timedelta(seconds=10) + + # Simulate what some ORM drivers return: an ISO 8601 string with T separator + # and timezone info (e.g. '2026-04-19T06:00:00+00:00'). + expires_at_iso = expires_at_aware.isoformat() + acquired_at_iso = acquired_at_aware.isoformat() + + session: Session = context.tdd10483_session_factory() + lock = LockModel( + owner_id=context.tdd10483_holder_owner, + resource_type=context.tdd10483_resource_type, + resource_id=context.tdd10483_resource_id, + acquired_at=acquired_at_iso, + expires_at=expires_at_iso, + ) + session.add(lock) + session.commit() + session.close() + + @when("a different owner tries to acquire the same resource for tdd 10483") def step_tdd10483_acquire(context: Context) -> None: """Attempt to acquire the lock as a different owner. diff --git a/features/tdd_lock_service_naive_datetime_expiry.feature b/features/tdd_lock_service_naive_datetime_expiry.feature index d28828c6f..1f4420bd9 100644 --- a/features/tdd_lock_service_naive_datetime_expiry.feature +++ b/features/tdd_lock_service_naive_datetime_expiry.feature @@ -23,3 +23,10 @@ Feature: TDD Bug #10483 — LockService.acquire() must use datetime comparison f And a non-expired lock exists with a naive datetime expires_at for tdd 10483 When a different owner tries to acquire the same resource for tdd 10483 Then a LockConflictError should be raised because the lock is still active for tdd 10483 + + @tdd_issue @tdd_issue_10483 + Scenario: Bug #10483 — non-expired lock with ISO string expires_at is not incorrectly treated as expired + Given a lock service with a real database for tdd 10483 + And a non-expired lock exists with an ISO string expires_at for tdd 10483 + When a different owner tries to acquire the same resource for tdd 10483 + Then a LockConflictError should be raised because the lock is still active for tdd 10483 diff --git a/src/cleveragents/application/services/lock_service.py b/src/cleveragents/application/services/lock_service.py index 8b07f9200..4c309f007 100644 --- a/src/cleveragents/application/services/lock_service.py +++ b/src/cleveragents/application/services/lock_service.py @@ -65,8 +65,20 @@ def _to_aware_datetime(value: datetime | str) -> datetime: Returns: A timezone-aware ``datetime`` in UTC. + + Raises: + ValueError: If *value* is a string that cannot be parsed as an + ISO 8601 datetime. """ - dt: datetime = datetime.fromisoformat(value) if isinstance(value, str) else value + if isinstance(value, str): + try: + dt: datetime = datetime.fromisoformat(value) + except ValueError as exc: + raise ValueError( + f"Cannot parse timestamp string as ISO 8601 datetime: {value!r}" + ) from exc + else: + dt = value if dt.tzinfo is None: dt = dt.replace(tzinfo=UTC) return dt -- 2.52.0 From 946c496a50bee21daa895c173379cd6bcc798866 Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Fri, 24 Apr 2026 13:52:55 +0000 Subject: [PATCH 3/3] =?UTF-8?q?ci:=20trigger=20CI=20re-run=20=E2=80=94=20i?= =?UTF-8?q?nfrastructure=20failures=20resolved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit -- 2.52.0