From 66c466e5782f7f36ae4e85c9da6e99d8890b025a Mon Sep 17 00:00:00 2001 From: Shailendra005 Date: Thu, 6 Aug 2026 18:51:23 +0530 Subject: [PATCH 1/2] fix(agent-server): size bash event pages by returned events, not files read --- .../openhands/agent_server/bash_service.py | 31 ++++----- tests/agent_server/test_bash_service.py | 67 ++++++++++++++++++- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/bash_service.py b/openhands-agent-server/openhands/agent_server/bash_service.py index 64dbd441ba..4120f4f709 100644 --- a/openhands-agent-server/openhands/agent_server/bash_service.py +++ b/openhands-agent-server/openhands/agent_server/bash_service.py @@ -149,7 +149,6 @@ async def search_bash_events( files = [file for file in files if file.name < timestamp_lt_str] # Handle pagination - page_files = [] start_index = 0 # Find the starting point if page_id is provided @@ -159,27 +158,29 @@ async def search_bash_events( start_index = i break - # Collect items for this page + # Collect items for this page. Filtering happens inside this loop so the + # boundary is measured in returned events rather than files read: sizing + # the page by file count first lets `order__gt` empty a page while + # `next_page_id` is still set, walking the client through under-full or + # entirely empty pages. + page_events = [] next_page_id = None for i in range(start_index, len(files)): - if len(page_files) >= limit: + if len(page_events) >= limit: # We have collected enough items for this page # Set next_page_id to the current file for next page next_page_id = str(files[i].name) break - page_files.append(files[i]) - # Load only the page files (not all files) - page_events = [] - for file_path in page_files: - event = self._load_event_from_file(file_path) - if event is not None: - # Filter by order if specified (only applies to BashOutput events) - if order__gt is not None: - event_order = getattr(event, "order", None) - if event_order is not None and event_order <= order__gt: - continue - page_events.append(event) + event = self._load_event_from_file(files[i]) + if event is None: + continue + # Filter by order if specified (only applies to BashOutput events) + if order__gt is not None: + event_order = getattr(event, "order", None) + if event_order is not None and event_order <= order__gt: + continue + page_events.append(event) return BashEventPage(items=page_events, next_page_id=next_page_id) diff --git a/tests/agent_server/test_bash_service.py b/tests/agent_server/test_bash_service.py index d74e875f8a..9d342125b5 100644 --- a/tests/agent_server/test_bash_service.py +++ b/tests/agent_server/test_bash_service.py @@ -5,10 +5,10 @@ import logging import time from collections.abc import AsyncIterator -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from pathlib import Path from unittest.mock import AsyncMock, patch -from uuid import UUID +from uuid import UUID, uuid4 import httpx import pytest @@ -18,7 +18,7 @@ from openhands.agent_server import bash_router as bash_router_module from openhands.agent_server.bash_service import BashEventService from openhands.agent_server.config import Config -from openhands.agent_server.models import BashCommand +from openhands.agent_server.models import BashCommand, BashOutput from openhands.agent_server.server_details_router import ( mark_initialization_complete, server_details_router, @@ -195,3 +195,64 @@ async def test_run_retention_cleanup_loop_purges_old_events(tmp_path: Path): assert len(service._get_event_files_by_pattern("*")) == 0, ( "Old event file should have been purged by the retention loop" ) + + +# --------------------------------------------------------------------------- +# search_bash_events pagination with order__gt +# --------------------------------------------------------------------------- + + +def test_order_filter_does_not_undersize_pages(tmp_path: Path): + """The page boundary must count returned events, not files read (#4388). + + Filtering after the page was already sized by file count let `order__gt` + hand back a short (or empty) page while `next_page_id` was still set, so a + client following the cursor walked a run of near-empty pages. + """ + service = BashEventService(bash_events_dir=tmp_path / "bash_events") + command_id = uuid4() + + # 10 events the filter rejects, then 5 it accepts. + for order in range(15): + service._save_event_to_file( + BashOutput( + command_id=command_id, + order=order, + stdout=f"chunk {order}", + timestamp=_OLD + timedelta(seconds=order), + ) + ) + + page = asyncio.run( + service.search_bash_events(order__gt=9, limit=5, kind__eq="BashOutput") + ) + + # Files sort by name (timestamp + id), so compare the set of orders rather + # than their sequence. + assert sorted(event.order for event in page.items) == [10, 11, 12, 13, 14] + assert page.next_page_id is None + + +def test_order_filter_never_returns_an_empty_page_with_a_cursor(tmp_path: Path): + """An all-filtered range must terminate instead of handing back a cursor.""" + service = BashEventService(bash_events_dir=tmp_path / "bash_events") + command_id = uuid4() + + for order in range(10): + service._save_event_to_file( + BashOutput( + command_id=command_id, + order=order, + stdout=f"chunk {order}", + timestamp=_OLD + timedelta(seconds=order), + ) + ) + + page = asyncio.run( + service.search_bash_events(order__gt=100, limit=5, kind__eq="BashOutput") + ) + + assert page.items == [] + assert page.next_page_id is None, ( + "an empty page must not advertise a next page, or clients loop forever" + ) From b80119f24ce79b1f37f553a96c2efff5f900bc9a Mon Sep 17 00:00:00 2001 From: Shailendra005 Date: Fri, 7 Aug 2026 04:28:15 +0530 Subject: [PATCH 2/2] test(agent-server): use the file's async test style for the new pagination tests --- tests/agent_server/test_bash_service.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/agent_server/test_bash_service.py b/tests/agent_server/test_bash_service.py index 9d342125b5..6390e84992 100644 --- a/tests/agent_server/test_bash_service.py +++ b/tests/agent_server/test_bash_service.py @@ -202,7 +202,7 @@ async def test_run_retention_cleanup_loop_purges_old_events(tmp_path: Path): # --------------------------------------------------------------------------- -def test_order_filter_does_not_undersize_pages(tmp_path: Path): +async def test_order_filter_does_not_undersize_pages(tmp_path: Path): """The page boundary must count returned events, not files read (#4388). Filtering after the page was already sized by file count let `order__gt` @@ -223,17 +223,15 @@ def test_order_filter_does_not_undersize_pages(tmp_path: Path): ) ) - page = asyncio.run( - service.search_bash_events(order__gt=9, limit=5, kind__eq="BashOutput") - ) + page = await service.search_bash_events(order__gt=9, limit=5, kind__eq="BashOutput") - # Files sort by name (timestamp + id), so compare the set of orders rather - # than their sequence. - assert sorted(event.order for event in page.items) == [10, 11, 12, 13, 14] + # `items` is typed as the event base class, so narrow before reading `order`. + orders = sorted(e.order for e in page.items if isinstance(e, BashOutput)) + assert orders == [10, 11, 12, 13, 14] assert page.next_page_id is None -def test_order_filter_never_returns_an_empty_page_with_a_cursor(tmp_path: Path): +async def test_order_filter_never_returns_an_empty_page_with_a_cursor(tmp_path: Path): """An all-filtered range must terminate instead of handing back a cursor.""" service = BashEventService(bash_events_dir=tmp_path / "bash_events") command_id = uuid4() @@ -248,8 +246,8 @@ def test_order_filter_never_returns_an_empty_page_with_a_cursor(tmp_path: Path): ) ) - page = asyncio.run( - service.search_bash_events(order__gt=100, limit=5, kind__eq="BashOutput") + page = await service.search_bash_events( + order__gt=100, limit=5, kind__eq="BashOutput" ) assert page.items == []