-
Notifications
You must be signed in to change notification settings - Fork 115
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
Don't fail if rds files are corrupted #471
base: main
Are you sure you want to change the base?
Conversation
I am pretty sure this is due to processx using SIGKILL in a number of places and the finalizer that is writing the coverage is interrupted. @gaborcsardi would know better than I if there is a way to have the improved behavior for this use case, the current behavior is unfortunate. |
Sometimes you have to use sigkill, because R in the subprocess is not interruptible. |
Sure, but if the process is writing the coverage out in an exit finalizer the process is already in the process of exiting, so there must be some case where the SIGKILL is too eager. |
@richfitz You can add some Or, if you don't mind sequential shutdown, then you can probably add a finalizer to your processes, that sends a On Windows there is no |
I don't think that this is the high level parts, unfortunately - I think that this is probably where a task has been spawned and is later cleaned up? I can't easily replicate unfortunately, and after replacing all the I've not seen anything running I do think that the current PR as it stands may be of use, but as always up to you for if you agree :) |
This is a somewhat unsatisfactory PR as I can't replicate this reliably.
As seen in #451 and #315 if the R process is killed when writing the Rds file it will create a corrupted rds file that will break the
covr::package_coverage()
. I've seen this rarely on gha builds (e.g. https://github.com/mrc-ide/rrq/pull/40/checks?check_run_id=2363889569) when usingcallr::r_bg
fairly extensively, but have been unable to trigger in a MWE.The downside is that the coverage report may be somewhat partial (and a message could be added during reading to indicate that) but continuing despite failure might be more graceful for relatively little additional complexity.