cleveragents/cleveragents-core

Fork 3

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService #10483

New issue

Closed

opened 2026-04-18 10:05:38 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-18 10:05:38 +00:00

Owner

Metadata

Commit Message: fix(lock): replace fragile ISO string comparison with datetime comparison in LockService
Branch: bugfix/auto3-lock-service-timestamp-comparison

Background and Context

[AUTO-BUG-3] — LockService.acquire() in src/cleveragents/application/services/lock_service.py checks whether an existing lock has expired by converting the ORM expires_at value to a string and comparing it lexicographically against an ISO 8601 string:

existing_expires = str(existing.expires_at)  # str() of ORM value
if existing_expires >= now_iso:              # string comparison
    raise LockConflictError(...)

This is fragile: the comparison is correct only when both strings are in the identical ISO 8601 format. Different database drivers or ORM configurations may return expires_at as a naive datetime, a datetime with a different timezone representation (Z vs +00:00), or a string without microseconds. Any format mismatch causes the comparison to silently produce wrong results — an expired lock may be treated as still held, blocking legitimate lock acquisition indefinitely.

Code Evidence (verified in current codebase):

File: src/cleveragents/application/services/lock_service.py, lines ~169-170:

existing_expires = str(existing.expires_at)  # converts ORM value to string
if existing_expires >= now_iso:              # string comparison — fragile
    raise LockConflictError(
        resource_type=resource_type,
        resource_id=resource_id,
        owner_id=existing_owner,
    )

Context (lines ~150-155):

now = datetime.now(tz=UTC)
expires = now + timedelta(seconds=ttl_seconds)
now_iso = now.isoformat()
expires_iso = expires.isoformat()

The expires_at column value is converted to a string via str() and then compared lexicographically against now_iso. This works only when both strings are in the exact same ISO 8601 format with the same timezone representation. If the database driver returns a datetime object, a naive datetime, or a timestamp in a different format (e.g., without microseconds, or with Z instead of +00:00), the string comparison silently produces incorrect results — a lock that has expired may be treated as still active, or vice versa.

Expected Behavior

The expiry check must compare datetime objects directly, normalizing timezone information:

from datetime import datetime, timezone
existing_expires_dt = existing.expires_at
if isinstance(existing_expires_dt, str):
    existing_expires_dt = datetime.fromisoformat(existing_expires_dt)
if existing_expires_dt.tzinfo is None:
    existing_expires_dt = existing_expires_dt.replace(tzinfo=timezone.utc)
if existing_expires_dt >= now:
    raise LockConflictError(...)

Acceptance Criteria

Lock expiry is determined by comparing datetime objects, not strings.
The comparison handles both timezone-aware and naive datetime values returned by the ORM.
An expired lock is correctly detected as expired regardless of the database driver's timestamp format.
All existing LockService tests continue to pass.
New TDD test (tagged @tdd_issue, @tdd_issue_<N>, @tdd_expected_fail) verifies correct expiry detection with a naive datetime value.

Subtasks

Write a Behave TDD scenario that injects a naive datetime as expires_at and asserts the lock is correctly detected as expired; tag it @tdd_issue, @tdd_issue_<N>, @tdd_expected_fail.
Replace the string comparison in LockService.acquire() with a datetime comparison that normalizes timezone info.
Audit any other timestamp comparisons in lock_service.py for the same pattern.
Remove @tdd_expected_fail tag once the fix is in place.
Verify coverage ≥97% via nox -s coverage_report.
Run nox (all default sessions) and fix any errors.

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

Automated by CleverAgents Bot
Agent: new-issue-creator

## Metadata - **Commit Message**: `fix(lock): replace fragile ISO string comparison with datetime comparison in LockService` - **Branch**: `bugfix/auto3-lock-service-timestamp-comparison` ## Background and Context `[AUTO-BUG-3]` — `LockService.acquire()` in `src/cleveragents/application/services/lock_service.py` checks whether an existing lock has expired by converting the ORM `expires_at` value to a string and comparing it lexicographically against an ISO 8601 string: ```python existing_expires = str(existing.expires_at) # str() of ORM value if existing_expires >= now_iso: # string comparison raise LockConflictError(...) ``` This is fragile: the comparison is correct only when both strings are in the identical ISO 8601 format. Different database drivers or ORM configurations may return `expires_at` as a naive `datetime`, a `datetime` with a different timezone representation (`Z` vs `+00:00`), or a string without microseconds. Any format mismatch causes the comparison to silently produce wrong results — an expired lock may be treated as still held, blocking legitimate lock acquisition indefinitely. **Code Evidence** (verified in current codebase): File: `src/cleveragents/application/services/lock_service.py`, lines ~169-170: ```python existing_expires = str(existing.expires_at) # converts ORM value to string if existing_expires >= now_iso: # string comparison — fragile raise LockConflictError( resource_type=resource_type, resource_id=resource_id, owner_id=existing_owner, ) ``` Context (lines ~150-155): ```python now = datetime.now(tz=UTC) expires = now + timedelta(seconds=ttl_seconds) now_iso = now.isoformat() expires_iso = expires.isoformat() ``` The `expires_at` column value is converted to a string via `str()` and then compared lexicographically against `now_iso`. This works only when both strings are in the exact same ISO 8601 format with the same timezone representation. If the database driver returns a `datetime` object, a naive datetime, or a timestamp in a different format (e.g., without microseconds, or with `Z` instead of `+00:00`), the string comparison silently produces incorrect results — a lock that has expired may be treated as still active, or vice versa. ## Expected Behavior The expiry check must compare `datetime` objects directly, normalizing timezone information: ```python from datetime import datetime, timezone existing_expires_dt = existing.expires_at if isinstance(existing_expires_dt, str): existing_expires_dt = datetime.fromisoformat(existing_expires_dt) if existing_expires_dt.tzinfo is None: existing_expires_dt = existing_expires_dt.replace(tzinfo=timezone.utc) if existing_expires_dt >= now: raise LockConflictError(...) ``` ## Acceptance Criteria - [x] Lock expiry is determined by comparing `datetime` objects, not strings. - [x] The comparison handles both timezone-aware and naive `datetime` values returned by the ORM. - [x] An expired lock is correctly detected as expired regardless of the database driver's timestamp format. - [x] All existing `LockService` tests continue to pass. - [x] New TDD test (tagged `@tdd_issue`, `@tdd_issue_<N>`, `@tdd_expected_fail`) verifies correct expiry detection with a naive datetime value. ## Subtasks - [x] Write a Behave TDD scenario that injects a naive `datetime` as `expires_at` and asserts the lock is correctly detected as expired; tag it `@tdd_issue`, `@tdd_issue_<N>`, `@tdd_expected_fail`. - [x] Replace the string comparison in `LockService.acquire()` with a `datetime` comparison that normalizes timezone info. - [x] Audit any other timestamp comparisons in `lock_service.py` for the same pattern. - [x] Remove `@tdd_expected_fail` tag once the fix is in place. - [x] Verify coverage ≥97% via `nox -s coverage_report`. - [x] Run `nox` (all default sessions) and fix any errors. ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. --- **Automated by CleverAgents Bot** Agent: new-issue-creator

HAL9000 added the

labels

2026-04-18 10:06:29 +00:00

HAL9000 referenced this issue

2026-04-18 10:32:21 +00:00

[AUTO-BUG-POOL] Status: Bug Hunt Pool Supervisor (Cycle 40) #10519

HAL9000 referenced this issue

2026-04-19 04:31:57 +00:00

[AUTO-IMP-POOL] Status: Cycle 1 - Full Pool Active #10685

HAL9000 referenced this issue from a commit

2026-04-19 08:52:02 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService

HAL9000 referenced this issue from a pull request that will close it,

2026-04-19 09:37:37 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService #10738

HAL9000 added

and removed

labels

2026-04-19 09:44:14 +00:00

HAL9000 commented

2026-04-19 09:45:01 +00:00

Author

Owner

Implementation Attempt — Tier 1: Haiku — Success

Implemented the fix for bug #10483: fragile ISO string comparison in LockService.

What was done:

Added _to_aware_datetime() helper function in lock_service.py that normalizes ORM timestamp values to timezone-aware datetime objects (handles naive datetimes, timezone-aware datetimes, and ISO strings)
Fixed LockService.acquire() to use datetime comparison instead of fragile string comparison for lock expiry detection
Fixed LockService.renew() similarly to use datetime comparison
Added TDD Behave test features/tdd_lock_service_naive_datetime_expiry.feature proving the fix works
Added step definitions features/steps/tdd_lock_service_naive_datetime_expiry_steps.py

Root cause of 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, causing non-expired locks to be silently treated as expired, allowing lock theft.

Quality gate status: lint ✓, typecheck ✓, unit_tests ✓ (28 scenarios), integration_tests ✓ (1962 tests)

PR: #10738 — #10738

Automated by CleverAgents Bot
Supervisor: Implementation Pool | Agent: implementation-worker

**Implementation Attempt** — Tier 1: Haiku — Success Implemented the fix for bug #10483: fragile ISO string comparison in `LockService`. **What was done:** - Added `_to_aware_datetime()` helper function in `lock_service.py` that normalizes ORM timestamp values to timezone-aware datetime objects (handles naive datetimes, timezone-aware datetimes, and ISO strings) - Fixed `LockService.acquire()` to use datetime comparison instead of fragile string comparison for lock expiry detection - Fixed `LockService.renew()` similarly to use datetime comparison - Added TDD Behave test `features/tdd_lock_service_naive_datetime_expiry.feature` proving the fix works - Added step definitions `features/steps/tdd_lock_service_naive_datetime_expiry_steps.py` **Root cause of 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, causing non-expired locks to be silently treated as expired, allowing lock theft. **Quality gate status:** lint ✓, typecheck ✓, unit_tests ✓ (28 scenarios), integration_tests ✓ (1962 tests) **PR:** #10738 — https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/10738 --- **Automated by CleverAgents Bot** Supervisor: Implementation Pool | Agent: implementation-worker

HAL9000 referenced this issue from a commit

2026-04-23 15:26:42 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService

HAL9000 referenced this issue from a commit

2026-04-23 18:29:51 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService

HAL9000 referenced this issue from a commit

2026-04-24 01:07:28 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService

HAL9000 referenced this issue from a commit

2026-04-24 03:13:39 +00:00

fix(lock): replace fragile ISO string comparison with datetime comparison in LockService