Fix hang when a stale database dump named pipe from an aborted borgmatic run remains on disk (#316).

NEWS

@@ -1,4 +1,6 @@
 1.5.6.dev0
+ * #316: Fix hang when a stale database dump named pipe from an aborted borgmatic run remains on
+   disk.
  * Tweak comment indentation in generated configuration file for clarity.
 
 1.5.5

borgmatic/hooks/dump.py

@@ -48,46 +48,24 @@ def create_named_pipe_for_dump(dump_path):
     os.mkfifo(dump_path, mode=0o600)
 
 
-def remove_database_dumps(dump_path, databases, database_type_name, log_prefix, dry_run):
+def remove_database_dumps(dump_path, database_type_name, log_prefix, dry_run):
     '''
-    Remove the database dumps for the given databases in the dump directory path. The databases are
+    Remove all database dumps in the given dump directory path (including the directory itself). If
-    supplied as a sequence of dicts, one dict describing each database as per the configuration
+    this is a dry run, then don't actually remove anything.
-    schema. Use the name of the database type and the log prefix in any log entries. If this is a
-    dry run, then don't actually remove anything.
     '''
-    if not databases:
-        logger.debug('{}: No {} databases configured'.format(log_prefix, database_type_name))
-        return
-
     dry_run_label = ' (dry run; not actually removing anything)' if dry_run else ''
 
     logger.info(
         '{}: Removing {} database dumps{}'.format(log_prefix, database_type_name, dry_run_label)
     )
 
-    for database in databases:
+    expanded_path = os.path.expanduser(dump_path)
-        dump_filename = make_database_dump_filename(
-            dump_path, database['name'], database.get('hostname')
-        )
 
-        logger.debug(
+    if dry_run:
-            '{}: Removing {} database dump {} from {}{}'.format(
+        return
-                log_prefix, database_type_name, database['name'], dump_filename, dry_run_label
-            )
-        )
-        if dry_run:
-            continue
 
-        if os.path.exists(dump_filename):
+    if os.path.exists(expanded_path):
-            if os.path.isdir(dump_filename):
+        shutil.rmtree(expanded_path)
-                shutil.rmtree(dump_filename)
-            else:
-                os.remove(dump_filename)
-
-        dump_file_dir = os.path.dirname(dump_filename)
-
-        if os.path.exists(dump_file_dir) and len(os.listdir(dump_file_dir)) == 0:
-            os.rmdir(dump_file_dir)
 
 
 def convert_glob_patterns_to_borg_patterns(patterns):

borgmatic/hooks/mysql.py

@@ -118,14 +118,11 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
 
 def remove_database_dumps(databases, log_prefix, location_config, dry_run):  # pragma: no cover
     '''
-    Remove the database dumps for the given databases. The databases are supplied as a sequence of
+    Remove all database dump files for this hook regardless of the given databases. Use the log
-    dicts, one dict describing each database as per the configuration schema. Use the log prefix in
+    prefix in any log entries. Use the given location configuration dict to construct the
-    any log entries. Use the given location configuration dict to construct the destination path. If
+    destination path. If this is a dry run, then don't actually remove anything.
-    this is a dry run, then don't actually remove anything.
     '''
-    dump.remove_database_dumps(
+    dump.remove_database_dumps(make_dump_path(location_config), 'MySQL', log_prefix, dry_run)
-        make_dump_path(location_config), databases, 'MySQL', log_prefix, dry_run
-    )
 
 
 def make_database_dump_pattern(

borgmatic/hooks/postgresql.py

@@ -82,14 +82,11 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
 
 def remove_database_dumps(databases, log_prefix, location_config, dry_run):  # pragma: no cover
     '''
-    Remove the database dumps for the given databases. The databases are supplied as a sequence of
+    Remove all database dump files for this hook regardless of the given databases. Use the log
-    dicts, one dict describing each database as per the configuration schema. Use the log prefix in
+    prefix in any log entries. Use the given location configuration dict to construct the
-    any log entries. Use the given location configuration dict to construct the destination path. If
+    destination path. If this is a dry run, then don't actually remove anything.
-    this is a dry run, then don't actually remove anything.
     '''
-    dump.remove_database_dumps(
+    dump.remove_database_dumps(make_dump_path(location_config), 'PostgreSQL', log_prefix, dry_run)
-        make_dump_path(location_config), databases, 'PostgreSQL', log_prefix, dry_run
-    )
 
 
 def make_database_dump_pattern(

tests/unit/hooks/test_dump.py

@@ -47,69 +47,28 @@ def test_create_named_pipe_for_dump_does_not_raise():
     module.create_named_pipe_for_dump('/path/to/pipe')
 
 
-def test_remove_database_dumps_removes_dump_for_each_database():
+def test_remove_database_dumps_removes_dump_path():
-    databases = [{'name': 'foo'}, {'name': 'bar'}]
+    flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost')
-    flexmock(module).should_receive('make_database_dump_filename').with_args(
-        'databases', 'foo', None
-    ).and_return('databases/localhost/foo')
-    flexmock(module).should_receive('make_database_dump_filename').with_args(
-        'databases', 'bar', None
-    ).and_return('databases/localhost/bar')
-
     flexmock(module.os.path).should_receive('exists').and_return(True)
-    flexmock(module.os.path).should_receive('isdir').and_return(False)
+    flexmock(module.shutil).should_receive('rmtree').with_args('databases/localhost').once()
-    flexmock(module.os).should_receive('remove').with_args('databases/localhost/foo').once()
-    flexmock(module.os).should_receive('remove').with_args('databases/localhost/bar').once()
-    flexmock(module.os).should_receive('listdir').with_args('databases/localhost').and_return(
-        ['bar']
-    ).and_return([])
 
-    flexmock(module.os).should_receive('rmdir').with_args('databases/localhost').once()
+    module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=False)
-
-    module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False)
-
-
-def test_remove_database_dumps_removes_dump_in_directory_format():
-    databases = [{'name': 'foo'}]
-    flexmock(module).should_receive('make_database_dump_filename').with_args(
-        'databases', 'foo', None
-    ).and_return('databases/localhost/foo')
-
-    flexmock(module.os.path).should_receive('exists').and_return(True)
-    flexmock(module.os.path).should_receive('isdir').and_return(True)
-    flexmock(module.os).should_receive('remove').never()
-    flexmock(module.shutil).should_receive('rmtree').with_args('databases/localhost/foo').once()
-    flexmock(module.os).should_receive('listdir').with_args('databases/localhost').and_return([])
-    flexmock(module.os).should_receive('rmdir').with_args('databases/localhost').once()
-
-    module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False)
 
 
 def test_remove_database_dumps_with_dry_run_skips_removal():
-    databases = [{'name': 'foo'}, {'name': 'bar'}]
+    flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost')
-    flexmock(module.os).should_receive('rmdir').never()
+    flexmock(module.os.path).should_receive('exists').never()
-    flexmock(module.os).should_receive('remove').never()
-
-    module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=True)
-
-
-def test_remove_database_dumps_without_dump_present_skips_removal():
-    databases = [{'name': 'foo'}]
-    flexmock(module).should_receive('make_database_dump_filename').with_args(
-        'databases', 'foo', None
-    ).and_return('databases/localhost/foo')
-
-    flexmock(module.os.path).should_receive('exists').and_return(False)
-    flexmock(module.os.path).should_receive('isdir').never()
-    flexmock(module.os).should_receive('remove').never()
     flexmock(module.shutil).should_receive('rmtree').never()
-    flexmock(module.os).should_receive('rmdir').never()
 
-    module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False)
+    module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=True)
 
 
-def test_remove_database_dumps_without_databases_does_not_raise():
+def test_remove_database_dumps_without_dump_path_present_skips_removal():
-    module.remove_database_dumps('databases', [], 'SuperDB', 'test.yaml', dry_run=False)
+    flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost')
+    flexmock(module.os.path).should_receive('exists').and_return(False)
+    flexmock(module.shutil).should_receive('rmtree').never()
+
+    module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=False)
 
 
 def test_convert_glob_patterns_to_borg_patterns_removes_leading_slash():