features/steps/tdd_lock_service_naive_datetime_expiry_steps.py

@@ -0,0 +1,181 @@
+"""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()
+
+
+@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.
+
+    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."
+    )

features/tdd_lock_service_naive_datetime_expiry.feature

@@ -0,0 +1,32 @@
+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
+
+  @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

src/cleveragents/application/services/lock_service.py

@@ -48,6 +48,42 @@ 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.
+
+    Raises:
+        ValueError: If *value* is a string that cannot be parsed as an
+            ISO 8601 datetime.
+    """
+    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
+
+
 class LockService:
     """Advisory lock manager for plan and project resources.
 
@@ -157,7 +193,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 +205,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 +218,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 +230,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 +246,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 +343,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 +356,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,