From a2648b9d649df6caa39b36317cc9508545b78d98 Mon Sep 17 00:00:00 2001 From: Kim Alvefur Date: Thu, 25 Feb 2021 23:58:08 +0100 Subject: mod_http_file_share: Remove correct entries when not all expired files were deleted If any of the expired files could not be deleted then we should not forget about that, we should complain loudly and try again. The code got this backwards and would have removed only the entries referring to still existing files. Test procedure: 1. Upload a file 2. chown root:root http_file_share/ 3. In uploads.list, decrease 'when' enough to ensure expiry 4. Reload mod_http_file_share 5. Should see an error in the logs about failure to delete the file 6. Should see that the metadata in uploads.list is still there 7. chown http_file_share/ back to the previous owner 8. Reload mod_http_file_share 9. Should see logs about successful removal of expired file 10. Should see that the metadata in uploads.list is gone 11. Should see that the file was deleted --- plugins/mod_http_file_share.lua | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/plugins/mod_http_file_share.lua b/plugins/mod_http_file_share.lua index 4c588c82..c66d7233 100644 --- a/plugins/mod_http_file_share.lua +++ b/plugins/mod_http_file_share.lua @@ -383,39 +383,49 @@ if expiry >= 0 and not external_base_url then module:log("info", "Pruning expired files uploaded earlier than %s", dt.datetime(boundary_time)); - local obsolete_files = array(); + local obsolete_uploads = array(); local i = 0; for slot_id in iter do i = i + 1; - obsolete_files:push(get_filename(slot_id)); + obsolete_uploads:push(slot_id); upload_cache:set(slot_id, nil); end sleep(0.1); local n = 0; - obsolete_files:filter(function(filename) + local problem_deleting = false; + obsolete_uploads:filter(function(slot_id) n = n + 1; if i % 100 == 0 then sleep(0.1); end + local filename = get_filename(slot_id); local deleted, err, errno = os.remove(filename); if deleted or errno == ENOENT then - return false; + return true; else module:log("error", "Could not delete file %q: %s", filename, err); - return true; + problem_deleting = true; + return false; end end); + -- obsolete_uploads now contains slot ids for which the files have been + -- deleted and that needs to be cleared from the database local deletion_query = {["end"] = boundary_time}; - if #obsolete_files == 0 then - module:log("info", "All %d expired files deleted", n); + if not problem_deleting then + module:log("info", "All (%d) expired files successfully deleted", n); + -- we can delete based on time else - module:log("warn", "%d out of %d expired files could not be deleted", #obsolete_files, n); - deletion_query = {ids = obsolete_files}; + module:log("warn", "%d out of %d expired files could not be deleted", n-#obsolete_uploads, n); + -- we'll need to delete only those entries where the files were + -- successfully deleted, and then try again with the failed ones. + -- eventually the admin ought to notice and fix the permissions or + -- whatever the problem is. + deletion_query = {ids = obsolete_uploads}; end local removed, err = uploads:delete(nil, deletion_query); - if removed == true or removed == n or removed == #obsolete_files then + if removed == true or removed == n or removed == #obsolete_uploads then module:log("debug", "Removed all metadata for expired uploaded files"); else module:log("error", "Problem removing metadata for deleted files: %s", err); -- cgit v1.2.3