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

Conversation

openhands-agent
Copy link
Contributor

@openhands-agent openhands-agent commented Jan 15, 2025

This pull request fixes #6037.

The issue has been successfully resolved based on the changes made. The original error occurred because the LogStreamer class was trying to access attributes that could be None when initialization failed, specifically causing an AttributeError on 'logs' and 'stdout_thread'.

The fix addresses this by:

  1. Initializing critical attributes (stdout_thread, log_generator) to None at the start
  2. Wrapping the initialization logic in a try-except block to handle failures gracefully
  3. Adding defensive checks in _stream_logs() to verify the log_generator exists before use
  4. Adding proper error logging when initialization fails

These changes directly prevent the original AttributeError by ensuring the attributes always exist (even if None) and adding proper error handling around their initialization and usage. The comprehensive test suite verifies the fix works by explicitly testing initialization failures, missing generators, and cleanup scenarios.

The code changes are a complete solution because they handle all paths that could have led to the original error, while maintaining proper functionality when everything works correctly. The error handling and initialization patterns implemented would prevent the reported NoneType attribute errors from occurring.

Automatic fix generated by OpenHands 🙌


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:3916373-nikolaik   --name openhands-app-3916373   docker.all-hands.dev/all-hands-ai/openhands:3916373

@openhands-agent
Copy link
Contributor Author

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor Author

Overview of Changes:

✅ Successfully Resolved:

  • Complete conversion from unittest to pytest framework
  • Maintained full test coverage while improving code structure
  • Added new test case for del without thread
  • Implemented pytest fixtures for better code organization
  • Preserved original bug fix (hasattr check in LogStreamer.del)

🔍 No Remaining Issues:
The changes appear to be comprehensive and successfully address all the original feedback. The conversion to pytest is complete and properly implemented, while maintaining functionality and improving code quality.

Conclusion: All issues have been successfully resolved with no outstanding concerns.

@enyst enyst added lint-fix and removed lint-fix labels Jan 15, 2025
@enyst enyst requested a review from xingyaoww January 15, 2025 09:05
@enyst
Copy link
Collaborator

enyst commented Jan 15, 2025

LGTM, just order of initialization and error handling

@enyst enyst marked this pull request as ready for review January 15, 2025 09:07
@enyst enyst added lint-fix and removed lint-fix labels Jan 15, 2025
self.log_generator = container.logs(stream=True, follow=True)
# Initialize all attributes before starting the thread on this instance
self.stdout_thread = None
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.

@openhands-agent
Copy link
Contributor Author

OpenHands started fixing the pr! You can monitor the progress here.

@openhands-agent
Copy link
Contributor Author

Overview of Changes:

  • Added null safety check for self.log_generator in the close() method
  • Change ensures type-safe handling of the optional log_generator attribute
  • Modification is minimal and preserves existing functionality

Status: ✅ All Issues Resolved

  • The code now properly handles the optional type throughout all relevant methods
  • Mypy type checking requirements are satisfied
  • Runtime safety is maintained through appropriate null checks

No remaining issues identified - the changes successfully address both type safety and runtime concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: [Resolver] crashes on main
2 participants