Wait for recorder to fully stop before completing upload

[becky-gilbert]

Summary

1. Waiting for the recorder to stop before completing the upload

This PR adds an await to RecordRTC's stopRecording method. This function returns a promise, but we were not waiting for that promise to resolve before calling S3's complete upload function. This might be the reason for the video files that have been cut off at the end: #392. The critical change is in app/services/video-recorder.js line 461:

await recorder.stopRecording();

I can't confirm that this will fix the issue, because I can't reproduce the video recording clipping problem. But I'm confident that this change should not introduce any new problems.

A little more context:

Under the hood, the RecordRTC recorder's stopRecording method calls the web api media recorder stop method and returns a promise. The web api stop method needs to trigger the 'data available' event one last time before actually stopping, so that the last bit of data can be saved. Our own recorder's stop method was correctly async and waited for the upload to complete before resolving, but did not wait for the recorder's stop method to resolve before starting to complete the upload. So it's possible that there's a small bit of data that's missed when we moved on from stopping the recorder to completing the upload. If so, this would explain why the recordings are only ever clipped a little bit and by a variable amount.

Dev question:

Why wouldn't we be seeing errors when that last data available callback is called after the upload completion process has started?

2. Error catching/logging

This PR also throws some more informative errors, instead of just rejecting promises, so that we can get more helpful errors in Sentry. In general, if a promise needs to be explicitly rejected, then it needs to include a reason (otherwise we get an "Uncaught (in promise) undefined" error), and best practices are to actually throw an error as the reason. In places where we're using a catch block instead of calling reject, we can just throw an error (because the promise has already been rejected at that point).

This improved error catching/logging is especially useful for differentiating video recording problems where something has gone wrong on our end, vs something that has happened on the participant's end that we have no control over (e.g. internet connection is disrupted). Right now, those cases are mixed together, both for researchers and for us (in Sentry).

Question:

What would we like the experiment runner to do when the maximum upload duration has been reached? My guess is that the experiment should not stop with an error, but rather should continue on as though the file has successfully uploaded? And that we want error logging both in Sentry and in the experiment data?

[bleonar5]

This all looks great, and all the additional error throwing is a good idea.

I don't have much insight on why an error wasn't being thrown when the stopRecording promise wasn't awaited. All of this processing happening on the client side, right? maybe the user had just left the page by the time the promise resolved. Or maybe for some reason, the late promise doesn't actually cause any error in the upload process, it just results in the last chunk of video being added after the upload bus had already left the station, so to speak.

I agree that users shouldn't encounter any error when the upload limit is reached. Ideally, I think it should be flagged in the database somehow, so when users look at the response video in the UI, maybe they see a little visual indicator above it that says "Video cut short, exceeded upload duration limit". Maybe the researcher could also be sent an automatic email when the upload length limit is exceeded? These are just some thoughts

[becky-gilbert]

Thanks for your feedback @bleonar5!

the late promise doesn't actually cause any error in the upload process, it just results in the last chunk of video being added after the upload bus had already left the station, so to speak.

Yes this is what I'm thinking too. I tried to create this scenario by throttling my connection speed, with the idea that this would slow down the stop recording process so that I can see what happens when the complete upload function is called too soon. But annoyingly everything still worked. (In retrospect, the processes that need to be awaited in stop recording probably don't depend on http requests). But maybe I can force this scenario via the code somehow, so that I can see whether it actually does reproduce the problem.

Thanks also for your thoughts on what to do when the upload time limit is reached. Glad to hear that my intuition is correct about not throwing an actual error, but rather just record that it happened somewhere.