refactor(restic_backup): extract helpers from run() to reduce complexity (S3776) #83
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Claude/backup-script:fix/issue-8-S3776-restic-backup-complexity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #8
ResticBackupStep.run()had cognitive complexity 29 (SonarQube ceiling: 15).Refactor
_failed_result()— single shared FAILEDStepResultbuilder. Collapses three near-identical 6-line blocks (init wrong-password, init other-error, backup-loop wrong-password) into one call site each. Sameduration_s = time.monotonic() - started, same[-1024:]stderr_tail._init_repo_or_fail()— wraps the idempotentrestic.initcall. ReturnsNoneon success (repo created or already exists) or a FAILEDStepResulton eitherResticWrongPasswordorResticError._dry_run_result()— the dry-run short-circuit, including theBACKUP_SUCCESS_SCRATCH_KEY = "dry-run"sentinel that lets a downstreamResticForgetStepexercise its own dry-run path instead of being gated out by the no-snapshot guard._run_backup_with_retry()— drives thenetwork_retrying()loop aroundrestic.backup, raisingTimeoutErroronresult.timed_outandResticRepoLockedonreturncode == 11to trigger retry. Surfaces the wrong-password fast path via a sentinel third return (early_exit) so the caller short-circuits without ever reaching classification — wrong password is never retryable._classify_result()— maps a non-NoneResticBackupResultto SUCCESS / PARTIAL / FAILED, populatesextras(repo,returncode,file_errors, and whensummaryis present:snapshot_id,bytes_added,files_new,files_changed), and gates theBACKUP_SUCCESS_SCRATCH_KEYflag.run()itself is now ~20 lines: init → dry-run? → retry → no-result guard → classify.Behavior
Identical. Key invariants preserved:
ResticRepoLocked, 12 (ResticWrongPassword) → FAILED with no retry, other non-zero → FAILED.BACKUP_SUCCESS_SCRATCH_KEYis set only whenoutcome in (SUCCESS, PARTIAL) and result.summary is not None and result.summary.snapshot_id. If restic exits 0 but the JSON summary never arrived (truncated stream), the flag is still withheld soResticForgetStepcannot silently prune the last good snapshot. Pinned bytest_backup_does_not_set_scratch_flag_when_summary_missingand..._when_snapshot_id_empty.extraspayload unchanged: same keys, same values, same conditional inclusion whenresult.summary is not None.network_retrying()wraps onlyrestic.backup, neverrestic.init. Init failures fail the step immediately.except Exception(notBaseException) soSystemExit/KeyboardInterrupt/ the streamer's kill-signal propagate untouched.stderr_tail/exceptionfields on the final return unchanged: same[-1024:]slice onresult.stderronly whenoutcome is not SUCCESS and result.stderr, samerepr(last_exc)only whenlast_exc and outcome is FAILED.self,rollback()is still the inherited base-class no-op. Pinned bytest_restic_backup_rollback_is_safe_noop.Verification
pytest -q tests/unit/— passes (207/207). Includestests/unit/test_restic_forget_guard.pywhich directly exercises 8 of the behavioral cases (scratch flag set on SUCCESS-with-id, set on PARTIAL-with-id, not set on FAILED, not set when summary missing, not set when snapshot_id empty, dry-run sets sentinel, end-to-end backup-then-forget shares scratch, failed backup blocks forget).mypy --strict src/gardien/core src/gardien/config src/gardien/engine src/gardien/steps— passes (Success: no issues found in 26 source files).analyze_code_snippeton the refactored file —issueCount: 0.Split ResticBackupStep.run() into focused, single-purpose helpers: - _failed_result(): shared FAILED StepResult builder for the three identical error-shaping sites (init wrong-password, init other, backup-loop wrong-password). - _init_repo_or_fail(): wraps the idempotent restic.init call and returns None on success or a FAILED StepResult on either restic error. - _dry_run_result(): the dry-run short-circuit, including the scratch sentinel that lets a downstream ResticForgetStep exercise its own dry-run path. - _run_backup_with_retry(): drives the network_retrying() loop around restic.backup, raising on timeout / exit-11 to trigger retry, and surfacing the wrong-password fast path via a sentinel third return. - _classify_result(): maps the final ResticBackupResult to SUCCESS / PARTIAL / FAILED, populates extras, and gates the BACKUP_SUCCESS_SCRATCH_KEY flag. Behavior is identical: - Same exit-code → Outcome mapping (0 SUCCESS, 3 PARTIAL, 11 retry, 12 FAILED, other FAILED). - Same data-safety gate: BACKUP_SUCCESS_SCRATCH_KEY only set when a real snapshot_id was observed (or "dry-run" sentinel in dry-run). - Same extras payload (repo, returncode, file_errors, snapshot_id, bytes_added, files_new, files_changed). - Same retry semantics (network_retrying around backup, never around init). - rollback() is still the inherited base-class no-op, preserving the CLAUDE.md idempotency invariant. Closes #8View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.