Harden native snapshot polish#5
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5b951a106
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "path": entry.get("path"), | ||
| "dry_run_required": True, | ||
| "forget_args": runner.forget_args(tags, dry_run=True), | ||
| "destructive_args_after_confirmation": runner.forget_args(tags, dry_run=False), |
There was a problem hiding this comment.
When snapshot purge-plan is used for a retired repo, the generated destructive command is just restic forget filtered by tags, with no snapshot IDs and no retention policy. The restic docs describe forget as applying a --keep-* policy, while --tag only restricts which snapshots are considered (https://restic.readthedocs.io/en/stable/060_forget.html), so executing this plan for any retired repo's snapshots will fail instead of purging them. The plan needs to identify the matching snapshot IDs or include a valid retention policy before presenting it as the confirmation command.
Useful? React with 👍 / 👎.
| Returns: | ||
| Dict with health check results | ||
| """ | ||
| config = Config() |
There was a problem hiding this comment.
If the user's discovered config file is malformed, this new eager Config() raises before check_config_file() can report the invalid config, so bbman health/health reporting exits with an exception in exactly the scenario the health command is meant to diagnose. Keep this construction inside a try/except or defer snapshot profile checks until after a valid config is available so malformed configs still produce a health report.
Useful? React with 👍 / 👎.
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/usr/bin/env bbackup snapshot run --profile {shlex.quote(profile.name)} |
There was a problem hiding this comment.
When snapshot schedule is rendered from a non-default config via the global --config, the generated systemd services later run plain bbackup snapshot run --profile ... and cannot see that config. In that setup every timer fails with “Snapshot profile not found” even though the schedule command succeeded; include the resolved config path in each ExecStart or otherwise ensure the installed service loads the same profile.
Useful? React with 👍 / 👎.
| for tag in tags: | ||
| args.extend(["--tag", tag]) | ||
| for exclude in self.profile.exclude_paths: | ||
| args.extend(["--exclude", exclude]) |
There was a problem hiding this comment.
For profiles that use the documented/example excludes such as ~/.cache or $HOME/..., this passes the literal YAML value to restic while the backup path itself is expanded. Because subprocess.run does not perform shell expansion and restic matches --exclude patterns against full paths (https://restic.readthedocs.io/en/latest/040_backup.html#excluding-files), those excludes won't match /home/... paths and the directories will be backed up. Expand user/env paths before appending absolute excludes.
Useful? React with 👍 / 👎.
| snapshot_id = _extract_snapshot_id(result["stdout"]) | ||
| if snapshot_id: | ||
| mark_snapshot_success(profile, item["repo_id"], snapshot_id) | ||
| return {"profile": profile.name, "results": results, "success": all(r["ok"] for r in results)} |
There was a problem hiding this comment.
If discovery finds no repos and include_paths is empty, this returns success: true because all([]) is true. A scheduled profile with a typo in repo_homes will therefore exit 0 without creating any snapshot, so operators get a green run while nothing was protected; return an error when the plan contains no backup commands.
Useful? React with 👍 / 👎.
| @click.pass_context | ||
| def snapshot_plan_cmd(ctx, profile, output, input_json): | ||
| """Resolve a snapshot profile without executing restic.""" | ||
| merge_json_input(ctx, input_json) |
There was a problem hiding this comment.
After merge_json_input() updates ctx.params, this command still uses the original profile local, and the other new snapshot commands repeat the same pattern for profile, dry_run, and read_data_subset. Agent callers that provide --input-json to override these options will run the CLI-supplied/default values instead; reload the affected locals from ctx.params as the backup command does.
Useful? React with 👍 / 👎.
| def _repository_initialized_check(profile: SnapshotProfile) -> Dict[str, Any]: | ||
| if not shutil.which("restic") or not profile.repository: | ||
| return {"ok": False, "message": "restic or repository missing"} | ||
| runner = ResticRunner(profile) |
There was a problem hiding this comment.
For a profile that sets a non-restic engine together with a repository, check_snapshot_profile() records the failed engine check but still reaches this ResticRunner construction, which raises SnapshotError and aborts the whole health check instead of reporting that profile as not ready. Skip repository probing when the engine check has already failed or convert the exception into a failed check result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e233472995
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| and existing_id not in {repo.repo_id, repo.legacy_repo_id} | ||
| and data.get("fingerprint") not in {repo.fingerprint, repo.legacy_fingerprint} |
There was a problem hiding this comment.
When a legacy state file was written before the current HEAD, the stored head-derived repo_id/fingerprint won't equal repo.legacy_repo_id/repo.legacy_fingerprint for the newly discovered HEAD. This condition then classifies the same active path as active_path_identity_conflict, so the plan omits the repo and snapshot_run refuses to back it up. Migration needs to tolerate the prior head-derived identity before treating it as an identity conflict.
Useful? React with 👍 / 👎.
| return env | ||
|
|
||
| def base_args(self) -> List[str]: | ||
| args = ["restic", "-r", self.profile.repository] |
There was a problem hiding this comment.
For a local repository configured as ~/restic or $RESTIC_REPO, this passes the literal value to subprocess.run; only password/cache/backup paths are expanded later. Since no shell expands argv, restic initializes/checks a repository named literally ~ or $RESTIC_REPO (or fails) instead of the intended location. Expand local path repositories before adding -r, while leaving remote backend URLs unchanged.
Useful? React with 👍 / 👎.
| config: Config = ctx.obj["config"] | ||
| console: Console = ctx.obj["console"] | ||
| snapshot_profile = _snapshot_profile_or_exit(config, profile, output) | ||
| target_path = Path(target).expanduser() |
There was a problem hiding this comment.
When the restore target contains an environment variable, such as JSON input with "target":"$RESTORE_DIR", this preflight checks only the literal path after ~ expansion, while snapshot_restore() later uses expand_path() before passing the target to restic. If the expanded directory is non-empty, the guard is bypassed and restic restores into it anyway; use the same expansion here that the restore command uses.
Useful? React with 👍 / 👎.
| origin = _git(root, ["config", "--get", "remote.origin.url"]) or "" | ||
| head = _git(root, ["rev-parse", "HEAD"]) or "" | ||
| git_dir = _git(root, ["rev-parse", "--git-dir"]) or "" | ||
| fingerprint_source = "\n".join([str(root), origin, git_dir]) |
There was a problem hiding this comment.
For a retired repository with no remote origin, deleting it and running git init at the same path produces the same fingerprint inputs here (root, empty origin, and .git). reconcile_repos then reactivates the old repo_id instead of raising a path-reuse conflict, mixing the new repo's snapshots with the retired repo's ledger and purge history; include a repo-internal marker such as the first seen commit for retired-path checks.
Useful? React with 👍 / 👎.
| if dry_run: | ||
| plan["dry_run"] = True | ||
| return plan |
There was a problem hiding this comment.
When snapshot run --dry-run encounters critical reconcile alerts, such as a repo that disappeared before any successful snapshot, this returns a plan without any success/ok field, so _snapshot_result defaults it to success and exits 0 even though the non-dry run immediately refuses the same plan. Mark the dry-run result unsuccessful when plan["alerts"] is non-empty so preflight checks do not go green for an un-runnable profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62c7ae0f98
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| args = runner.restore_args(snapshot_id, target, include) | ||
| if dry_run: | ||
| return {"dry_run": True, "args": args} | ||
| _require_snapshot_operation_preflight(profile) |
There was a problem hiding this comment.
When restoring on a replacement machine or after the host name changes, this full operation preflight rejects the restore before invoking restic because _require_snapshot_operation_preflight includes _hostname_check (and local state/cache checks) for the profile's original host_id. Restic restore only needs repository credentials and a target, so disaster-recovery restores from a profile created for the backed-up host fail with a hostname/state preflight error unless the user edits the profile or spoofs the hostname; use a restore-specific preflight that skips host/state requirements.
Useful? React with 👍 / 👎.
| if not entry.get("successful_snapshot_ids"): | ||
| raise SnapshotError(f"Cannot retire repo without a successful snapshot: {repo_id}") | ||
| entry["status"] = "retired" |
There was a problem hiding this comment.
When an active repository already has a successful snapshot, snapshot retire --repo-id unconditionally flips it to retired without checking whether the path is still discovered. Since purge_plan only refuses entries whose current status is active, a mistyped or stale ID can immediately be treated as safe to purge even though the repo is still present and scheduled for backups; verify the repo is no longer active before changing this status.
Useful? React with 👍 / 👎.
| expand_path(profile.state_dir).mkdir(parents=True, exist_ok=True) | ||
| _require_snapshot_operation_preflight(profile, require_initialized=False) |
There was a problem hiding this comment.
For profiles that omit state_dir, state_file() already defines a default under ~/.local/state, but init expands the raw empty string here, which resolves to the current working directory, and then the following preflight still reports state_dir: missing. Such profiles can never be initialized despite the fallback used by state load/save; create the resolved state_file(profile).parent or otherwise apply the same default before preflight.
Useful? React with 👍 / 👎.
| console: Console = ctx.obj["console"] | ||
| snapshot_profile = _snapshot_profile_or_exit(config, profile, output) | ||
| target_path = expand_path(target) | ||
| if not dry_run and target_path.exists() and any(target_path.iterdir()): |
There was a problem hiding this comment.
If --target names an existing file, this preflight calls iterdir() on that file before the SnapshotError handler, so the command raises NotADirectoryError and loses the structured JSON/plain error path instead of reporting an invalid restore target. Check target_path.is_dir() before iterating so bad targets fail cleanly.
Useful? React with 👍 / 👎.
1 participant
Summary
Issue links
Validation
Docker integration was not run locally because the Docker socket returned permission denied.