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 history loading when state was corrupt/non-existent #5946

Merged
merged 6 commits into from
Dec 31, 2024

Conversation

enyst
Copy link
Collaborator

@enyst enyst commented Dec 31, 2024

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below
    Fix history loading when state was corrupt or not saved properly (such as on a runtime error)

This PR proposes a fix for history lost on runtime errors:

  • fix history to always load the event stream, regardless of State

When the session is restored, the history is initialized from the event stream, but that depended on whether there was a valid State restored. That doesn't make a lot of sense, and it meant that when the execution failed with a runtime error or something bad enough that it didn't get to save the State or the file is corrupt, history was not loaded from the stream. This PR fixes that.


Link of any specific issues this addresses
Fix #5602

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:f5689b4-nikolaik   --name openhands-app-f5689b4   docker.all-hands.dev/all-hands-ai/openhands:f5689b4

@rbren
Copy link
Collaborator

rbren commented Dec 31, 2024

I wish there was a better way to know if state should be restored, so we could at least do a logger.error

@enyst
Copy link
Collaborator Author

enyst commented Dec 31, 2024

I wish there was a better way to know if state should be restored, so we could at least do a logger.error

If the event stream has events, it should have state. I'm not sure how we are going to do with conversations, do they share the same stream? Then we don't know from the existence of the stream alone. Otherwise, I think that suffices.

@enyst
Copy link
Collaborator Author

enyst commented Dec 31, 2024

@openhands-agent Read this PR and understand its diff. Then run the project's unit tests, and fix them if they fail. You know how to set up the unit tests for this particular project.

@openhands-agent
Copy link
Contributor

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

@openhands-agent
Copy link
Contributor

Overview of Changes:
✓ All issues appear to be successfully resolved. The changes addressed:

  • Test failures through proper mocking implementation
  • Docker API usage improvements
  • Removal of subprocess.Popen dependencies
  • Docker version check formatting fixes

Current Status: No remaining issues identified. All tests are passing and the implementation now follows better practices with proper Docker API usage and mocking.

@enyst enyst enabled auto-merge (squash) December 31, 2024 21:28
@enyst enyst merged commit 40d8245 into main Dec 31, 2024
12 of 13 checks passed
@enyst enyst deleted the enyst/history-error branch December 31, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Runtime error loses agent history
3 participants