Skip to content

Commit e6835d0

Browse files
fix: proper datetime comparison for timestamp filtering
Address review feedback on timezone handling: - WebSocket endpoint now normalizes tz-aware datetimes (matches REST) - event_service.py uses datetime comparison instead of string comparison - Updated tests to reflect the new timezone contract Co-authored-by: openhands <openhands@all-hands.dev>
1 parent 909bbac commit e6835d0

4 files changed

Lines changed: 113 additions & 39 deletions

File tree

openhands-agent-server/openhands/agent_server/event_service.py

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,6 @@ def _search_events_sync(
107107
if not self._conversation:
108108
raise ValueError("inactive_service")
109109

110-
# Convert datetime to ISO string for comparison (ISO strings are comparable)
111-
timestamp_gte_str = timestamp__gte.isoformat() if timestamp__gte else None
112-
timestamp_lt_str = timestamp__lt.isoformat() if timestamp__lt else None
113-
114110
# Collect all events
115111
all_events = []
116112
with self._conversation._state as state:
@@ -132,14 +128,15 @@ def _search_events_sync(
132128
if not self._event_matches_body(event, body):
133129
continue
134130

135-
# Apply timestamp filters if provided (ISO string comparison)
136-
if (
137-
timestamp_gte_str is not None
138-
and event.timestamp < timestamp_gte_str
139-
):
140-
continue
141-
if timestamp_lt_str is not None and event.timestamp >= timestamp_lt_str:
142-
continue
131+
# Apply timestamp filters using proper datetime comparison.
132+
# Event timestamps are naive (server local time), so filter
133+
# datetimes should also be naive or normalized to server timezone.
134+
if timestamp__gte is not None or timestamp__lt is not None:
135+
event_dt = datetime.fromisoformat(event.timestamp)
136+
if timestamp__gte is not None and event_dt < timestamp__gte:
137+
continue
138+
if timestamp__lt is not None and event_dt >= timestamp__lt:
139+
continue
143140

144141
all_events.append(event)
145142

@@ -211,10 +208,6 @@ def _count_events_sync(
211208
if not self._conversation:
212209
raise ValueError("inactive_service")
213210

214-
# Convert datetime to ISO string for comparison (ISO strings are comparable)
215-
timestamp_gte_str = timestamp__gte.isoformat() if timestamp__gte else None
216-
timestamp_lt_str = timestamp__lt.isoformat() if timestamp__lt else None
217-
218211
count = 0
219212
with self._conversation._state as state:
220213
for event in state.events:
@@ -235,14 +228,15 @@ def _count_events_sync(
235228
if not self._event_matches_body(event, body):
236229
continue
237230

238-
# Apply timestamp filters if provided (ISO string comparison)
239-
if (
240-
timestamp_gte_str is not None
241-
and event.timestamp < timestamp_gte_str
242-
):
243-
continue
244-
if timestamp_lt_str is not None and event.timestamp >= timestamp_lt_str:
245-
continue
231+
# Apply timestamp filters using proper datetime comparison.
232+
# Event timestamps are naive (server local time), so filter
233+
# datetimes should also be naive or normalized to server timezone.
234+
if timestamp__gte is not None or timestamp__lt is not None:
235+
event_dt = datetime.fromisoformat(event.timestamp)
236+
if timestamp__gte is not None and event_dt < timestamp__gte:
237+
continue
238+
if timestamp__lt is not None and event_dt >= timestamp__lt:
239+
continue
246240

247241
count += 1
248242

openhands-agent-server/openhands/agent_server/sockets.py

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
from openhands.agent_server.conversation_service import (
2525
get_default_conversation_service,
2626
)
27+
from openhands.agent_server.event_router import normalize_datetime_to_server_timezone
2728
from openhands.agent_server.models import BashEventBase, ExecuteBashRequest
2829
from openhands.agent_server.pub_sub import Subscriber
2930
from openhands.sdk import Event, Message
@@ -52,9 +53,10 @@ async def events_socket(
5253
session_api_key: Optional API key for authentication.
5354
resend_all: If True, resend all existing events when connecting.
5455
after_timestamp: If provided with resend_all=True, only resend events
55-
with timestamps >= this value. Enables efficient bi-directional
56-
loading where REST fetches historical events and WebSocket handles
57-
events after a specific point in time.
56+
with timestamps >= this value. Timestamps are interpreted in server
57+
local time. Timezone-aware datetimes are converted to server timezone.
58+
Enables efficient bi-directional loading where REST fetches historical
59+
events and WebSocket handles events after a specific point in time.
5860
"""
5961
# Perform authentication check before accepting the WebSocket connection
6062
config = get_default_config()
@@ -75,17 +77,25 @@ async def events_socket(
7577
_WebSocketSubscriber(websocket)
7678
)
7779

80+
# Normalize timezone-aware datetimes to server timezone
81+
normalized_after_timestamp = (
82+
normalize_datetime_to_server_timezone(after_timestamp)
83+
if after_timestamp
84+
else None
85+
)
86+
7887
try:
7988
# Resend existing events if requested
8089
if resend_all:
81-
if after_timestamp:
90+
if normalized_after_timestamp:
8291
logger.info(
83-
f"Resending events after {after_timestamp}: {conversation_id}"
92+
f"Resending events after {normalized_after_timestamp}: "
93+
f"{conversation_id}"
8494
)
8595
else:
8696
logger.info(f"Resending all events: {conversation_id}")
8797
async for event in page_iterator(
88-
event_service.search_events, timestamp__gte=after_timestamp
98+
event_service.search_events, timestamp__gte=normalized_after_timestamp
8999
):
90100
await _send_event(event, websocket)
91101

tests/agent_server/test_event_router_websocket.py

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ class TestAfterTimestampFiltering:
456456
async def test_after_timestamp_passed_to_search_events(
457457
self, mock_websocket, mock_event_service, sample_conversation_id
458458
):
459-
"""Test that after_timestamp is passed to search_events when provided."""
459+
"""Test that after_timestamp is normalized and passed to search_events."""
460460
from datetime import datetime
461461
from typing import cast
462462

@@ -476,7 +476,8 @@ async def test_after_timestamp_passed_to_search_events(
476476
mock_event_service.search_events = AsyncMock(return_value=mock_event_page)
477477
mock_websocket.receive_json.side_effect = WebSocketDisconnect()
478478

479-
test_timestamp = datetime(2024, 1, 15, 10, 30, 0, tzinfo=UTC)
479+
# Use a naive timestamp (as would typically come from REST API response)
480+
test_timestamp = datetime(2024, 1, 15, 10, 30, 0)
480481

481482
with (
482483
patch(
@@ -499,11 +500,69 @@ async def test_after_timestamp_passed_to_search_events(
499500
after_timestamp=test_timestamp,
500501
)
501502

502-
# search_events should be called with timestamp__gte parameter
503+
# search_events should be called with the (unchanged) naive timestamp
503504
mock_event_service.search_events.assert_called_once_with(
504505
page_id=None, timestamp__gte=test_timestamp
505506
)
506507

508+
@pytest.mark.asyncio
509+
async def test_after_timestamp_timezone_aware_is_normalized(
510+
self, mock_websocket, mock_event_service, sample_conversation_id
511+
):
512+
"""Test that timezone-aware timestamps are normalized to server timezone."""
513+
from datetime import datetime
514+
from typing import cast
515+
516+
from openhands.agent_server.models import EventPage
517+
from openhands.sdk.event import Event
518+
519+
mock_events = [
520+
MessageEvent(
521+
id="event1",
522+
source="user",
523+
llm_message=Message(role="user", content=[TextContent(text="Hello")]),
524+
),
525+
]
526+
mock_event_page = EventPage(
527+
items=cast(list[Event], mock_events), next_page_id=None
528+
)
529+
mock_event_service.search_events = AsyncMock(return_value=mock_event_page)
530+
mock_websocket.receive_json.side_effect = WebSocketDisconnect()
531+
532+
# Use a timezone-aware timestamp (UTC)
533+
test_timestamp = datetime(2024, 1, 15, 10, 30, 0, tzinfo=UTC)
534+
535+
with (
536+
patch(
537+
"openhands.agent_server.sockets.conversation_service"
538+
) as mock_conv_service,
539+
patch("openhands.agent_server.sockets.get_default_config") as mock_config,
540+
):
541+
mock_config.return_value.session_api_keys = None
542+
mock_conv_service.get_event_service = AsyncMock(
543+
return_value=mock_event_service
544+
)
545+
546+
from openhands.agent_server.sockets import events_socket
547+
548+
await events_socket(
549+
sample_conversation_id,
550+
mock_websocket,
551+
session_api_key=None,
552+
resend_all=True,
553+
after_timestamp=test_timestamp,
554+
)
555+
556+
# search_events should be called with the normalized timestamp
557+
# (converted to server local timezone, which is still tz-aware)
558+
mock_event_service.search_events.assert_called_once()
559+
call_args = mock_event_service.search_events.call_args
560+
passed_timestamp = call_args.kwargs["timestamp__gte"]
561+
# The timestamp should have been converted to local timezone
562+
assert passed_timestamp is not None
563+
# It should represent the same instant in time
564+
assert passed_timestamp == test_timestamp.astimezone(None)
565+
507566
@pytest.mark.asyncio
508567
async def test_after_timestamp_without_resend_all_no_effect(
509568
self, mock_websocket, mock_event_service, sample_conversation_id

tests/agent_server/test_event_service.py

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -397,11 +397,17 @@ async def test_search_events_timestamp_range_filter(
397397
async def test_search_events_timestamp_filter_with_timezone_aware(
398398
self, event_service, mock_conversation_with_timestamped_events
399399
):
400-
"""Test filtering events with timezone-aware datetime."""
400+
"""Test filtering events with timezone-aware datetime requires normalization.
401+
402+
Event timestamps are naive (server local time), so callers must normalize
403+
timezone-aware datetimes to naive before filtering. This is done by the
404+
REST/WebSocket API layer via normalize_datetime_to_server_timezone().
405+
"""
401406
event_service._conversation = mock_conversation_with_timestamped_events
402407

403-
# Filter events >= 12:00:00 UTC (should return events 3, 4, 5)
404-
filter_time = datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC)
408+
# Filter events >= 12:00:00 (naive, as if normalized by API layer)
409+
# The API layer would convert a tz-aware datetime to naive server time
410+
filter_time = datetime(2025, 1, 1, 12, 0, 0) # naive datetime
405411
result = await event_service.search_events(timestamp__gte=filter_time)
406412

407413
assert len(result.items) == 3
@@ -539,11 +545,16 @@ async def test_count_events_timestamp_range_filter(
539545
async def test_count_events_timestamp_filter_with_timezone_aware(
540546
self, event_service, mock_conversation_with_timestamped_events
541547
):
542-
"""Test counting events with timezone-aware datetime."""
548+
"""Test counting events with timezone-aware datetime requires normalization.
549+
550+
Event timestamps are naive (server local time), so callers must normalize
551+
timezone-aware datetimes to naive before filtering. This is done by the
552+
REST/WebSocket API layer via normalize_datetime_to_server_timezone().
553+
"""
543554
event_service._conversation = mock_conversation_with_timestamped_events
544555

545-
# Count events >= 12:00:00 UTC (should return 3)
546-
filter_time = datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC)
556+
# Count events >= 12:00:00 (naive, as if normalized by API layer)
557+
filter_time = datetime(2025, 1, 1, 12, 0, 0) # naive datetime
547558
result = await event_service.count_events(timestamp__gte=filter_time)
548559
assert result == 3
549560

0 commit comments

Comments
 (0)