With the "max_duration" option or the "--max-duration" flag, run the archives and repository checks separately so they don't interfere with one another (#988).

2025-02-06 11:52:16 -08:00 · 2025-02-06 11:52:16 -08:00 · ffaa99ba15
commit ffaa99ba15
parent 5dc0b08f22
4 changed files with 247 additions and 120 deletions
--- a/3
+++ b/3
@ -2,6 +2,9 @@
 * #987: Fix a "list" action error when the "encryption_passcommand" option is set.
 * #987: When both "encryption_passcommand" and "encryption_passphrase" are configured, prefer
   "encryption_passphrase" even if it's an empty value.
+ * #988: With the "max_duration" option or the "--max-duration" flag, run the archives and
+   repository checks separately so they don't interfere with one another. Previously, borgmatic
+   refused to run checks in this situation.
 * #989: Fix the log message code to avoid using Python 3.10+ logging features. Now borgmatic will
   work with Python 3.9 again.

--- a/borgmatic/borg/check.py
+++ b/borgmatic/borg/check.py
@ -64,15 +64,11 @@ def make_check_name_flags(checks, archive_filter_flags):

        ('--repository-only',)

-    However, if both "repository" and "archives" are in checks, then omit them from the returned
-    flags because Borg does both checks by default. If "data" is in checks, that implies "archives".
+    However, if both "repository" and "archives" are in checks, then omit the "only" flags from the
+    returned flags because Borg does both checks by default. Note that a "data" check only works
+    along with an "archives" check.
    '''
-    if 'data' in checks:
-        data_flags = ('--verify-data',)
-        checks.update({'archives'})
-    else:
-        data_flags = ()
-
+    data_flags = ('--verify-data',) if 'data' in checks else ()
    common_flags = (archive_filter_flags if 'archives' in checks else ()) + data_flags

    if {'repository', 'archives'}.issubset(checks):
@ -142,49 +138,50 @@ def check_archives(
    except StopIteration:
        repository_check_config = {}

-    if check_arguments.max_duration and 'archives' in checks:
-        raise ValueError('The archives check cannot run when the --max-duration flag is used')
-    if repository_check_config.get('max_duration') and 'archives' in checks:
-        raise ValueError(
-            'The archives check cannot run when the repository check has the max_duration option set'
-        )
-
    max_duration = check_arguments.max_duration or repository_check_config.get('max_duration')
+
    umask = config.get('umask')
-
    borg_exit_codes = config.get('borg_exit_codes')
-
-    full_command = (
-        (local_path, 'check')
-        + (('--repair',) if check_arguments.repair else ())
-        + (('--max-duration', str(max_duration)) if max_duration else ())
-        + make_check_name_flags(checks, archive_filter_flags)
-        + (('--remote-path', remote_path) if remote_path else ())
-        + (('--umask', str(umask)) if umask else ())
-        + (('--log-json',) if global_arguments.log_json else ())
-        + (('--lock-wait', str(lock_wait)) if lock_wait else ())
-        + verbosity_flags
-        + (('--progress',) if check_arguments.progress else ())
-        + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
-        + flags.make_repository_flags(repository_path, local_borg_version)
-    )
-
    working_directory = borgmatic.config.paths.get_working_directory(config)

-    # The Borg repair option triggers an interactive prompt, which won't work when output is
-    # captured. And progress messes with the terminal directly.
-    if check_arguments.repair or check_arguments.progress:
-        execute_command(
-            full_command,
-            output_file=DO_NOT_CAPTURE,
-            extra_environment=environment.make_environment(config),
-            working_directory=working_directory,
-            borg_local_path=local_path,
-            borg_exit_codes=borg_exit_codes,
-        )
-    else:
+    if 'data' in checks:
+        checks.add('archives')
+
+    grouped_checks = (checks,)
+
+    # If max_duration is set, then archives and repository checks need to be run separately, as Borg
+    # doesn't support --max-duration along with an archives checks.
+    if max_duration and 'archives' in checks and 'repository' in checks:
+        checks.remove('repository')
+        grouped_checks = (checks, {'repository'})
+
+    for checks_subset in grouped_checks:
+        full_command = (
+            (local_path, 'check')
+            + (('--repair',) if check_arguments.repair else ())
+            + (
+                ('--max-duration', str(max_duration))
+                if max_duration and 'repository' in checks_subset
+                else ()
+            )
+            + make_check_name_flags(checks_subset, archive_filter_flags)
+            + (('--remote-path', remote_path) if remote_path else ())
+            + (('--umask', str(umask)) if umask else ())
+            + (('--log-json',) if global_arguments.log_json else ())
+            + (('--lock-wait', str(lock_wait)) if lock_wait else ())
+            + verbosity_flags
+            + (('--progress',) if check_arguments.progress else ())
+            + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+            + flags.make_repository_flags(repository_path, local_borg_version)
+        )
+
        execute_command(
            full_command,
+            # The Borg repair option triggers an interactive prompt, which won't work when output is
+            # captured. And progress messes with the terminal directly.
+            output_file=(
+                DO_NOT_CAPTURE if check_arguments.repair or check_arguments.progress else None
+            ),
            extra_environment=environment.make_environment(config),
            working_directory=working_directory,
            borg_local_path=local_path,
--- a/borgmatic/config/schema.yaml
+++ b/borgmatic/config/schema.yaml
@ -632,8 +632,8 @@ properties:
                              long-running repository check into multiple
                              partial checks. Defaults to no interruption. Only
                              applies to the "repository" check, does not check
-                              the repository index, and is not compatible with a
-                              simultaneous "archives" check or "--repair" flag.
+                              the repository index and is not compatible with
+                              the "--repair" flag.
                          example: 3600
                - required:
                    - name
--- a/tests/unit/borg/test_check.py
+++ b/tests/unit/borg/test_check.py
@ -8,13 +8,16 @@ from borgmatic.borg import check as module
 from ..test_verbosity import insert_logging_mock


-def insert_execute_command_mock(command, working_directory=None, borg_exit_codes=None):
+def insert_execute_command_mock(
+    command, output_file=None, working_directory=None, borg_exit_codes=None
+):
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(
        working_directory,
    )
    flexmock(module).should_receive('execute_command').with_args(
        command,
+        output_file=output_file,
        extra_environment=None,
        working_directory=working_directory,
        borg_local_path=command[0],
@ -250,11 +253,11 @@ def test_make_check_name_flags_without_archives_check_and_with_archive_filter_fl
    assert flags == ('--repository-only',)


-def test_make_check_name_flags_with_data_check_returns_flag_and_implies_archives():
+def test_make_check_name_flags_with_archives_and_data_check_returns_verify_data_flag():
    flexmock(module.feature).should_receive('available').and_return(True)
    flexmock(module.flags).should_receive('make_match_archives_flags').and_return(())

-    flags = module.make_check_name_flags({'data'}, ())
+    flags = module.make_check_name_flags({'archives', 'data'}, ())

    assert flags == (
        '--archives-only',
@ -262,6 +265,15 @@ def test_make_check_name_flags_with_data_check_returns_flag_and_implies_archives
    )


+def test_make_check_name_flags_with_repository_and_data_check_returns_verify_data_flag():
+    flexmock(module.feature).should_receive('available').and_return(True)
+    flexmock(module.flags).should_receive('make_match_archives_flags').and_return(())
+
+    flags = module.make_check_name_flags({'archives', 'data', 'repository'}, ())
+
+    assert flags == ('--verify-data',)
+
+
 def test_make_check_name_flags_with_extract_omits_extract_flag():
    flexmock(module.feature).should_receive('available').and_return(True)
    flexmock(module.flags).should_receive('make_match_archives_flags').and_return(())
@ -271,21 +283,6 @@ def test_make_check_name_flags_with_extract_omits_extract_flag():
    assert flags == ()


-def test_make_check_name_flags_with_repository_and_data_checks_does_not_return_repository_only():
-    flexmock(module.feature).should_receive('available').and_return(True)
-    flexmock(module.flags).should_receive('make_match_archives_flags').and_return(())
-
-    flags = module.make_check_name_flags(
-        {
-            'repository',
-            'data',
-        },
-        (),
-    )
-
-    assert flags == ('--verify-data',)
-
-
 def test_get_repository_id_with_valid_json_does_not_raise():
    config = {}
    flexmock(module.repo_info).should_receive('display_repository_info').and_return(
@ -336,7 +333,9 @@ def test_get_repository_id_with_missing_json_keys_raises():

 def test_check_archives_with_progress_passes_through_to_borg():
    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
@ -369,7 +368,9 @@ def test_check_archives_with_progress_passes_through_to_borg():

 def test_check_archives_with_repair_passes_through_to_borg():
    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
@ -402,12 +403,15 @@ def test_check_archives_with_repair_passes_through_to_borg():

 def test_check_archives_with_max_duration_flag_passes_through_to_borg():
    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
    flexmock(module).should_receive('execute_command').with_args(
        ('borg', 'check', '--max-duration', '33', 'repo'),
+        output_file=None,
        extra_environment=None,
        working_directory=None,
        borg_local_path='borg',
@ -432,37 +436,17 @@ def test_check_archives_with_max_duration_flag_passes_through_to_borg():
    )


-def test_check_archives_with_max_duration_flag_and_archives_check_errors():
-    config = {}
-    flexmock(module).should_receive('execute_command').never()
-
-    with pytest.raises(ValueError):
-        module.check_archives(
-            repository_path='repo',
-            config=config,
-            local_borg_version='1.2.3',
-            check_arguments=flexmock(
-                progress=None,
-                repair=None,
-                only_checks=None,
-                force=None,
-                match_archives=None,
-                max_duration=33,
-            ),
-            global_arguments=flexmock(log_json=False),
-            checks={'repository', 'archives'},
-            archive_filter_flags=(),
-        )
-
-
 def test_check_archives_with_max_duration_option_passes_through_to_borg():
    config = {'checks': [{'name': 'repository', 'max_duration': 33}]}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
    flexmock(module).should_receive('execute_command').with_args(
        ('borg', 'check', '--max-duration', '33', 'repo'),
+        output_file=None,
        extra_environment=None,
        working_directory=None,
        borg_local_path='borg',
@ -487,37 +471,145 @@ def test_check_archives_with_max_duration_option_passes_through_to_borg():
    )


-def test_check_archives_with_max_duration_option_and_archives_check_errors():
-    config = {'checks': [{'name': 'repository', 'max_duration': 33}]}
-    flexmock(module).should_receive('execute_command').never()
+def test_check_archives_with_max_duration_option_and_archives_check_runs_repository_check_separately():
+    config = {'checks': [{'name': 'repository', 'max_duration': 33}, {'name': 'archives'}]}
+    flexmock(module).should_receive('make_check_name_flags').with_args({'archives'}, ()).and_return(
+        ('--archives-only',)
+    )
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(('--repository-only',))
+    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
+    insert_execute_command_mock(('borg', 'check', '--archives-only', 'repo'))
+    insert_execute_command_mock(
+        ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo')
+    )

-    with pytest.raises(ValueError):
-        module.check_archives(
-            repository_path='repo',
-            config=config,
-            local_borg_version='1.2.3',
-            check_arguments=flexmock(
-                progress=None,
-                repair=None,
-                only_checks=None,
-                force=None,
-                match_archives=None,
-                max_duration=None,
-            ),
-            global_arguments=flexmock(log_json=False),
-            checks={'repository', 'archives'},
-            archive_filter_flags=(),
-        )
+    module.check_archives(
+        repository_path='repo',
+        config=config,
+        local_borg_version='1.2.3',
+        check_arguments=flexmock(
+            progress=None,
+            repair=None,
+            only_checks=None,
+            force=None,
+            match_archives=None,
+            max_duration=None,
+        ),
+        global_arguments=flexmock(log_json=False),
+        checks={'repository', 'archives'},
+        archive_filter_flags=(),
+    )
+
+
+def test_check_archives_with_max_duration_flag_and_archives_check_runs_repository_check_separately():
+    config = {'checks': [{'name': 'repository'}, {'name': 'archives'}]}
+    flexmock(module).should_receive('make_check_name_flags').with_args({'archives'}, ()).and_return(
+        ('--archives-only',)
+    )
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(('--repository-only',))
+    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
+    insert_execute_command_mock(('borg', 'check', '--archives-only', 'repo'))
+    insert_execute_command_mock(
+        ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo')
+    )
+
+    module.check_archives(
+        repository_path='repo',
+        config=config,
+        local_borg_version='1.2.3',
+        check_arguments=flexmock(
+            progress=None,
+            repair=None,
+            only_checks=None,
+            force=None,
+            match_archives=None,
+            max_duration=33,
+        ),
+        global_arguments=flexmock(log_json=False),
+        checks={'repository', 'archives'},
+        archive_filter_flags=(),
+    )
+
+
+def test_check_archives_with_max_duration_option_and_data_check_runs_repository_check_separately():
+    config = {'checks': [{'name': 'repository', 'max_duration': 33}, {'name': 'data'}]}
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'data', 'archives'}, ()
+    ).and_return(('--archives-only', '--verify-data'))
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(('--repository-only',))
+    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
+    insert_execute_command_mock(('borg', 'check', '--archives-only', '--verify-data', 'repo'))
+    insert_execute_command_mock(
+        ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo')
+    )
+
+    module.check_archives(
+        repository_path='repo',
+        config=config,
+        local_borg_version='1.2.3',
+        check_arguments=flexmock(
+            progress=None,
+            repair=None,
+            only_checks=None,
+            force=None,
+            match_archives=None,
+            max_duration=None,
+        ),
+        global_arguments=flexmock(log_json=False),
+        checks={'repository', 'data'},
+        archive_filter_flags=(),
+    )
+
+
+def test_check_archives_with_max_duration_flag_and_data_check_runs_repository_check_separately():
+    config = {'checks': [{'name': 'repository'}, {'name': 'data'}]}
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'data', 'archives'}, ()
+    ).and_return(('--archives-only', '--verify-data'))
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(('--repository-only',))
+    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
+    insert_execute_command_mock(('borg', 'check', '--archives-only', '--verify-data', 'repo'))
+    insert_execute_command_mock(
+        ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo')
+    )
+
+    module.check_archives(
+        repository_path='repo',
+        config=config,
+        local_borg_version='1.2.3',
+        check_arguments=flexmock(
+            progress=None,
+            repair=None,
+            only_checks=None,
+            force=None,
+            match_archives=None,
+            max_duration=33,
+        ),
+        global_arguments=flexmock(log_json=False),
+        checks={'repository', 'data'},
+        archive_filter_flags=(),
+    )


 def test_check_archives_with_max_duration_flag_overrides_max_duration_option():
    config = {'checks': [{'name': 'repository', 'max_duration': 33}]}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
    flexmock(module).should_receive('execute_command').with_args(
        ('borg', 'check', '--max-duration', '44', 'repo'),
+        output_file=None,
        extra_environment=None,
        working_directory=None,
        borg_local_path='borg',
@ -575,9 +667,37 @@ def test_check_archives_calls_borg_with_parameters(checks):
    )


+def test_check_archives_with_data_check_implies_archives_check_calls_borg_with_parameters():
+    config = {}
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'data', 'archives'}, ()
+    ).and_return(())
+    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
+    insert_execute_command_mock(('borg', 'check', 'repo'))
+
+    module.check_archives(
+        repository_path='repo',
+        config=config,
+        local_borg_version='1.2.3',
+        check_arguments=flexmock(
+            progress=None,
+            repair=None,
+            only_checks=None,
+            force=None,
+            match_archives=None,
+            max_duration=None,
+        ),
+        global_arguments=flexmock(log_json=False),
+        checks={'data'},
+        archive_filter_flags=(),
+    )
+
+
 def test_check_archives_with_log_info_passes_through_to_borg():
    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    insert_logging_mock(logging.INFO)
    insert_execute_command_mock(('borg', 'check', '--info', 'repo'))
@ -602,7 +722,9 @@ def test_check_archives_with_log_info_passes_through_to_borg():

 def test_check_archives_with_log_debug_passes_through_to_borg():
    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    insert_logging_mock(logging.DEBUG)
    insert_execute_command_mock(('borg', 'check', '--debug', '--show-rc', 'repo'))
@ -806,7 +928,9 @@ def test_check_archives_with_retention_prefix():

 def test_check_archives_with_extra_borg_options_passes_through_to_borg():
    config = {'extra_borg_options': {'check': '--extra --options'}}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    insert_execute_command_mock(('borg', 'check', '--extra', '--options', 'repo'))

@ -829,15 +953,16 @@ def test_check_archives_with_extra_borg_options_passes_through_to_borg():


 def test_check_archives_with_match_archives_passes_through_to_borg():
-    config = {}
-    flexmock(module).should_receive('make_check_name_flags').and_return(
-        ('--match-archives', 'foo-*')
-    )
+    config = {'checks': [{'name': 'archives'}]}
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'archives'}, object
+    ).and_return(('--match-archives', 'foo-*'))
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
    flexmock(module).should_receive('execute_command').with_args(
        ('borg', 'check', '--match-archives', 'foo-*', 'repo'),
+        output_file=None,
        extra_environment=None,
        working_directory=None,
        borg_local_path='borg',
@ -864,7 +989,9 @@ def test_check_archives_with_match_archives_passes_through_to_borg():

 def test_check_archives_calls_borg_with_working_directory():
    config = {'working_directory': '/working/dir'}
-    flexmock(module).should_receive('make_check_name_flags').and_return(())
+    flexmock(module).should_receive('make_check_name_flags').with_args(
+        {'repository'}, ()
+    ).and_return(())
    flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
    flexmock(module.environment).should_receive('make_environment')
    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)