-
Notifications
You must be signed in to change notification settings - Fork 27
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
Gracefully handle more errors during file copy #2511
base: master
Are you sure you want to change the base?
Conversation
If something goes wrong while copying files, we just want to retry with a new runner. If the error is not expected (which would be only an `Runner::Error::RunnerNotFound`), we log the error to Sentry.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2511 +/- ##
==========================================
+ Coverage 69.37% 69.40% +0.03%
==========================================
Files 199 199
Lines 6347 6348 +1
==========================================
+ Hits 4403 4406 +3
+ Misses 1944 1942 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
However, we should be aware that if someone would be able to find a file copy payload returning an Internal Server Error, they could trigger an infinite loop by CodeOcean always retrying.
On a quick check, I haven't found such payload for users. Teachers, however, are able to do so.
I am not aware of this possibility. In the code, we retry exactly once. If the second attempt fails as well, the resulting exception is not caught and further bubbled up, where it will be handled by the corresponding controller and simply return an error.
🤔 Are they? Feel free to share more details with me (also via email or Slack)... |
I see, thanks for clarifying this mechanism.
They are able to create an Internal Server Error, however, I have not checked the Ruby part that this would actually lead to an Infinite loop (which as you described, does not). |
If something goes wrong while copying files, we just want to retry with a new runner. If the error is not expected (which would be only an
Runner::Error::RunnerNotFound
), we log the error to Sentry.This PR is related to openHPI/poseidon#649.