Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #6037: [Bug]: [Resolver] crashes on main #6284

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
24 changes: 17 additions & 7 deletions openhands/runtime/utils/log_streamer.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,26 @@ def __init__(
logFn: Callable,
):
self.log = logFn
self.log_generator = container.logs(stream=True, follow=True)
# Initialize all attributes before starting the thread on this instance
self.stdout_thread = None
enyst marked this conversation as resolved.
Show resolved Hide resolved
self.log_generator = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@openhands-agent Since self.log_generator has now a type | None, so it's optional, check this file and adapt the code using log_generator so that mypy is happy. Keep it minimal, only make the absolutely necessary tweaks for that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Openhands fix success summary

The changes successfully address the feedback by properly handling the optional type of self.log_generator. Specifically:

  1. The code added an explicit self.log_generator is not None check before attempting to access the close attribute in the close() method
  2. This change directly addresses the mypy concern about log_generator being of type | None by ensuring null safety before attribute access
  3. The change is minimal and focused, only modifying the one location where additional type safety was needed
  4. The modification maintains the existing functionality while adding proper type checking
  5. The rest of the code already had appropriate null checks in place (in _stream_logs() and __del__())

The change will prevent potential runtime errors from trying to access attributes on None while satisfying mypy's type checking requirements. This directly resolves the feedback's request to adapt the code to handle the optional type of log_generator in a type-safe way.

self._stop_event = threading.Event()

# Start the stdout streaming thread
self.stdout_thread = threading.Thread(target=self._stream_logs)
self.stdout_thread.daemon = True
self.stdout_thread.start()
try:
self.log_generator = container.logs(stream=True, follow=True)
# Start the stdout streaming thread
self.stdout_thread = threading.Thread(target=self._stream_logs)
self.stdout_thread.daemon = True
self.stdout_thread.start()
except Exception as e:
self.log('error', f'Failed to initialize log streaming: {e}')

def _stream_logs(self):
"""Stream logs from the Docker container to stdout."""
if not self.log_generator:
self.log('error', 'Log generator not initialized')
return

try:
for log_line in self.log_generator:
if self._stop_event.is_set():
Expand All @@ -38,7 +48,7 @@ def _stream_logs(self):
self.log('error', f'Error streaming docker logs to stdout: {e}')

def __del__(self):
if self.stdout_thread and self.stdout_thread.is_alive():
if hasattr(self, 'stdout_thread') and self.stdout_thread and self.stdout_thread.is_alive():
self.close(timeout=5)

def close(self, timeout: float = 5.0):
Expand All @@ -47,5 +57,5 @@ def close(self, timeout: float = 5.0):
if self.stdout_thread and self.stdout_thread.is_alive():
self.stdout_thread.join(timeout)
# Close the log generator to release the file descriptor
if hasattr(self.log_generator, 'close'):
if self.log_generator is not None and hasattr(self.log_generator, 'close'):
self.log_generator.close()
73 changes: 73 additions & 0 deletions tests/unit/test_log_streamer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
from unittest.mock import Mock

import pytest

from openhands.runtime.utils.log_streamer import LogStreamer


@pytest.fixture
def mock_container():
return Mock()


@pytest.fixture
def mock_log_fn():
return Mock()


def test_init_failure_handling(mock_container, mock_log_fn):
"""Test that LogStreamer handles initialization failures gracefully."""
mock_container.logs.side_effect = Exception('Test error')

streamer = LogStreamer(mock_container, mock_log_fn)
assert streamer.stdout_thread is None
assert streamer.log_generator is None
mock_log_fn.assert_called_with(
'error', 'Failed to initialize log streaming: Test error'
)


def test_stream_logs_without_generator(mock_container, mock_log_fn):
"""Test that _stream_logs handles missing log generator gracefully."""
streamer = LogStreamer(mock_container, mock_log_fn)
streamer.log_generator = None
streamer._stream_logs()
mock_log_fn.assert_called_with('error', 'Log generator not initialized')


def test_cleanup_without_thread(mock_container, mock_log_fn):
"""Test that cleanup works even if stdout_thread is not initialized."""
streamer = LogStreamer(mock_container, mock_log_fn)
streamer.stdout_thread = None
streamer.close() # Should not raise any exceptions


def test_normal_operation(mock_container, mock_log_fn):
"""Test normal operation of LogStreamer."""
mock_logs = [b'test log 1\n', b'test log 2\n']
mock_container.logs.return_value = mock_logs

streamer = LogStreamer(mock_container, mock_log_fn)
assert streamer.stdout_thread is not None
assert streamer.log_generator is not None

# Let the thread process the logs
streamer.close()

# Verify logs were processed
expected_calls = [
('debug', '[inside container] test log 1'),
('debug', '[inside container] test log 2'),
]
actual_calls = [(args[0], args[1]) for args, _ in mock_log_fn.call_args_list]
for expected in expected_calls:
assert expected in actual_calls


def test_del_without_thread(mock_container, mock_log_fn):
"""Test that __del__ works even if stdout_thread was not initialized."""
streamer = LogStreamer(mock_container, mock_log_fn)
delattr(
streamer, 'stdout_thread'
) # Simulate case where the thread was never created
streamer.__del__() # Should not raise any exceptions
Loading