Auto expire has race condition

[amb@…]

The auto expiration of deleted programs has a race condition.

Recorded programs to be deleted are marked for deletion by setting deletepending=1 in the database and they are also added to a list to be deleted. The deletepending flag in the database stops this recording from appearing in the recording list or in the list of recordings to delete.

The race condition occurs if mythbackend is killed or dies after the recording is flagged for deletion but before it is deleted. The flag in the database stops the same recording from being added to the list again. The file therefore remains on the disk and the entry remains in the database but it is invisible in the frontend recording list and will not be re-added to the list for deletion.

The patch attached is one solution, it may not be optimum. It ignores the deletepending flag if it was set more than 5 minutes ago. This stops the recording becoming invisible so it can be added to the list again and really get deleted.

[amb@…]

Possible patch

[sphery]

FWIW, I'm not a fan of the time-based approach to ignoring deletepending flag--especially with such a short time.

The approach I had considered using several years ago to work around this race condition (since it's likely that many people--primarily those who don't have have slow deletes enabled, since the time allowing this race to be hit when slow deletes is enabled is negligible--have one or more stuck files) was to have the autoexpire thread pick up all deletepending recordings on master backend startup and restart deleting them. Unfortunately, I didn't get around to testing how it would work/how to handle the case where the user has multiple backends and the master backend is restarted while remote backends are in the process of deleting files. This has the benefit of defining a simple condition under which deletepending is invalid (when the system is just starting up), and it won't be affected by system speed/deletion speed/...

A better approach is to avoid the race condition altogether, rather than work around it--though we'll still need clean-up code, as above, for the old files on people's systems. It may well be that now that the Deleted recording group is always used as a means of providing a delete queue for MythTV, that we can change deletepending to be a UI-only field, now, and when a recording is deleted, simply move it to the Deleted recording group (if not already there) to serve the old purpose of deletepending. So, that means that the "WHERE %1 AND deletepending = 0" that you modified in autoexpire.cpp could remove the deletepending reference (leaving "WHERE %1"). However, it may also mean that we'd need to add code to the autoexpirer to remember (in memory) that it's currently deleting some given file so it can remove that file from the deleteList. I haven't explored this particular avoid-the-race-condition idea in detail, so comments/suggestions/reasons why it won't work are welcomed.

[amb@…]

The approach of re-scanning the list for pending deletes when mythbackend is started is certainly one way to do it and much better than my idea of a 5 minute grace time.

Removing the deletepending flag from the database altogether is a much better solution though.

1. It simplifies things in general
2. The rescanning at startup (above) is not needed because the list will be rescanned after a short time anyway and if there is no deletepending flag then it can't stop them being deleted.
3. It is only a snapshot - a file in the Deleted recordings group that isn't pending for deletion right now might be pending for deletion in 1 second's time so the list is obsolete as soon as it is created.
4. The UI should already handle the case of files disappearing from recording groups (e.g. deleted from a different frontend) so listing a file even while it is being deleted and then having it disappear should be OK.
5. The frontend should already handle the case of files being deleted while being watched (e.g. deleted permanently from a different frontend) so having them listed and selectable for watching even when about to be deleted should be OK.

I can't see anything wrong with removing deletepending completely that isn't already a bug of another type (points 3, 4 and 5 above).

[sphery]

My primary concern with 4/5 is the principle of least surprise. If a user sits down and starts watching a show as it's being deleted, they may be upset. Then again, after a quick look in autoexpire.cpp, it seems that we don't take into account whether a show is being watched when deciding what to expire (please correct me if I'm wrong), so it's not much different from someone starting to watch the show and the expirer deciding to remove it shortly thereafter. So maybe that's a task for the future, and might as well be ignored for this issue.

[Stuart Auchterlonie]

Moving remaining open tickets to 0.28.2 milestone

[Stuart Auchterlonie]

Moving remaining open tickets to 29.2 milestone

[jpoet]

sphery, do you have time to work on this?

[Stuart Auchterlonie]

We have moved all bug tracking to github [1]

If you continue to have this issue, please open a new issue at github, referencing this ticket.

[1] - ​https://github.com/MythTV/mythtv/issues