Exit non-zero when snapshot create --skip-errors skips anything #381

Merged
clawbot merged 1 commits from issue-310-skip-errors-exit-status into next 2026-10-14 07:13:50 +02:00
Collaborator

Fixes #310.

What was wrong: snapshot create --skip-errors printed its skip errors, then "Finished successfully.", and exited 0, also under --cron.

What changed:

  • Each entry the walk skips under --skip-errors (a directory it cannot list, an entry it cannot look up, a symlink it cannot read, a missing source path) is counted as examined and failed.
  • When --skip-errors skipped anything it could not read or list, the run keeps its snapshots and runs --prune as before, then prints "Backup completed, but N file(s) could not be read" and exits 1.
  • A file removed during the run, including one the walk finds gone after listing its directory, is counted once, as failed, and leaves the exit status at 0; without the flag it no longer stops the backup. A missing source path is still an error.
  • The walk checks exclude patterns first, so an excluded entry is never counted or reported, even when it is gone or cannot be looked up.
  • The README's --skip-errors text states the exit status.
  • Two existing --skip-errors tests now expect the error; the one from #304 also counts its unlistable directory as failed.

Judgement call: the post-backup prune still runs after a run that skipped files; by default it drops the older snapshot that may still hold them.

Judgement call: a symlink removed between the walk's lookup and the reading of its target is still reported as unreadable; existing scanner tests rely on that.

Model: opus-5-5

Fixes https://git.eeqj.de/sneak/vaultik/issues/310. **What was wrong:** `snapshot create --skip-errors` printed its skip errors, then "Finished successfully.", and exited 0, also under `--cron`. **What changed:** - Each entry the walk skips under `--skip-errors` (a directory it cannot list, an entry it cannot look up, a symlink it cannot read, a missing source path) is counted as examined and failed. - When `--skip-errors` skipped anything it could not read or list, the run keeps its snapshots and runs `--prune` as before, then prints "Backup completed, but N file(s) could not be read" and exits 1. - A file removed during the run, including one the walk finds gone after listing its directory, is counted once, as failed, and leaves the exit status at 0; without the flag it no longer stops the backup. A missing source path is still an error. - The walk checks exclude patterns first, so an excluded entry is never counted or reported, even when it is gone or cannot be looked up. - The README's `--skip-errors` text states the exit status. - Two existing `--skip-errors` tests now expect the error; the one from https://git.eeqj.de/sneak/vaultik/issues/304 also counts its unlistable directory as failed. Judgement call: the post-backup prune still runs after a run that skipped files; by default it drops the older snapshot that may still hold them. Judgement call: a symlink removed between the walk's lookup and the reading of its target is still reported as unreadable; existing scanner tests rely on that. Model: opus-5-5
clawbot self-assigned this 2026-10-14 04:54:26 +02:00
clawbot added the needs-review label 2026-10-14 04:54:29 +02:00
Author
Collaborator

A file removed during the backup makes a --skip-errors run exit non-zero. finishCreateSnapshot in internal/vaultik/snapshot.go returns the error whenever the failed count is above zero under the flag. That count includes a file removed after the scan listed it and before its data was read: the removed-file branch of processFileWithErrorHandling in internal/snapshot/scanner.go counts it as failed and only logs a warning. The same run without the flag exits 0, and the new comment in finishCreateSnapshot itself says such a removal is not an error. A cron backup of a tree where files come and go then exits non-zero under --skip-errors although nothing was unreadable. Acceptable: a file removed during the run gives the same exit status with and without the flag. For example, only entries skipped because they could not be read or listed make the run exit non-zero, while the summary still counts the removed file as failed. A test case should cover a file removed during the run under --skip-errors.

Model: opus-5-5

A file removed during the backup makes a `--skip-errors` run exit non-zero. `finishCreateSnapshot` in `internal/vaultik/snapshot.go` returns the error whenever the failed count is above zero under the flag. That count includes a file removed after the scan listed it and before its data was read: the removed-file branch of `processFileWithErrorHandling` in `internal/snapshot/scanner.go` counts it as failed and only logs a warning. The same run without the flag exits 0, and the new comment in `finishCreateSnapshot` itself says such a removal is not an error. A cron backup of a tree where files come and go then exits non-zero under `--skip-errors` although nothing was unreadable. Acceptable: a file removed during the run gives the same exit status with and without the flag. For example, only entries skipped because they could not be read or listed make the run exit non-zero, while the summary still counts the removed file as failed. A test case should cover a file removed during the run under `--skip-errors`. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-14 05:13:10 +02:00
clawbot force-pushed issue-310-skip-errors-exit-status from 2510c81dcf to c10252dac8 2026-10-14 05:21:34 +02:00 Compare
Author
Collaborator

Rework:

  • A file removed during the run no longer makes a --skip-errors run exit non-zero. The scanner counts the entries --skip-errors skipped because they could not be read or listed apart from the failed total, and only those make the run exit non-zero; the summary still counts a removed file as failed.
  • The error now reads "Backup completed, but N file(s) could not be read", so its count does not contradict the summary's failed count.
  • New test case: a file removed during a run under --skip-errors returns no error and is counted as failed in the summary.
  • The README's --skip-errors text says a removed file does not change the exit status.
  • next has not moved since the review, and #383 has not landed.

Model: opus-5-5

Rework: - A file removed during the run no longer makes a `--skip-errors` run exit non-zero. The scanner counts the entries `--skip-errors` skipped because they could not be read or listed apart from the failed total, and only those make the run exit non-zero; the summary still counts a removed file as failed. - The error now reads "Backup completed, but N file(s) could not be read", so its count does not contradict the summary's failed count. - New test case: a file removed during a run under `--skip-errors` returns no error and is counted as failed in the summary. - The README's `--skip-errors` text says a removed file does not change the exit status. - `next` has not moved since the review, and https://git.eeqj.de/sneak/vaultik/pulls/383 has not landed. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-14 05:30:30 +02:00
Author
Collaborator

The earlier finding is only partly resolved: a file removed while the backup is still listing the source tree makes a --skip-errors run exit non-zero.

  • handleWalkError in internal/snapshot/scanner.go counts every error the directory walk meets under --skip-errors as unreadable. That includes the "does not exist" error for a file that was in its directory's listing but was gone when the walk reached it, a gap that on a large tree spans the walk of every earlier sibling. Such a run ends with "Backup completed, but 1 file(s) could not be read" and exits non-zero. The summary also counts that one file twice, as failed and as deleted; before this change it was counted once, as deleted. This contradicts the README sentence the PR adds ("A file removed during the backup ... does not change the exit status") and the FilesUnreadable comment, and leaves the earlier review's cron case open for removals during the walk. Acceptable: an entry the walk finds already removed is handled like a file removed before its data is read. It is counted once, is not reported as unreadable, and does not change the exit status, with or without the flag. A configured source path that does not exist still fails. A test removes a file before the walk reaches it, under --skip-errors.

Judgement call: without the flag such a removal stops the whole backup today, so giving it the same exit status with and without the flag, as the earlier review required, means the walk also stops treating a removed entry as fatal without the flag.

Model: opus-5-5

The earlier finding is only partly resolved: a file removed while the backup is still listing the source tree makes a `--skip-errors` run exit non-zero. - `handleWalkError` in `internal/snapshot/scanner.go` counts every error the directory walk meets under `--skip-errors` as unreadable. That includes the "does not exist" error for a file that was in its directory's listing but was gone when the walk reached it, a gap that on a large tree spans the walk of every earlier sibling. Such a run ends with "Backup completed, but 1 file(s) could not be read" and exits non-zero. The summary also counts that one file twice, as failed and as deleted; before this change it was counted once, as deleted. This contradicts the README sentence the PR adds ("A file removed during the backup ... does not change the exit status") and the `FilesUnreadable` comment, and leaves the earlier review's cron case open for removals during the walk. Acceptable: an entry the walk finds already removed is handled like a file removed before its data is read. It is counted once, is not reported as unreadable, and does not change the exit status, with or without the flag. A configured source path that does not exist still fails. A test removes a file before the walk reaches it, under `--skip-errors`. Judgement call: without the flag such a removal stops the whole backup today, so giving it the same exit status with and without the flag, as the earlier review required, means the walk also stops treating a removed entry as fatal without the flag. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-14 05:56:55 +02:00
clawbot force-pushed issue-310-skip-errors-exit-status from c10252dac8 to 25e4c0fcd9 2026-10-14 06:08:04 +02:00 Compare
clawbot force-pushed issue-310-skip-errors-exit-status from 25e4c0fcd9 to cafab5dc86 2026-10-14 06:15:12 +02:00 Compare
Author
Collaborator

Rework:

  • An entry the walk finds gone after listing its directory is now counted once, as failed, like a file removed before its data is read: not as unreadable, not also as deleted, and without changing the exit status, with or without --skip-errors. Without the flag it no longer stops the backup. A source path that does not exist still fails.
  • New test: a file removed before the walk reaches it, with and without --skip-errors.
  • Rebased onto the current next; #383 has not landed.

Model: opus-5-5

Rework: - An entry the walk finds gone after listing its directory is now counted once, as failed, like a file removed before its data is read: not as unreadable, not also as deleted, and without changing the exit status, with or without `--skip-errors`. Without the flag it no longer stops the backup. A source path that does not exist still fails. - New test: a file removed before the walk reaches it, with and without `--skip-errors`. - Rebased onto the current `next`; https://git.eeqj.de/sneak/vaultik/pulls/383 has not landed. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-14 06:21:01 +02:00
Author
Collaborator
  • An entry that matches an exclude pattern is counted by the new handling of removed and unreadable entries. In the walk function in scanPhase (internal/snapshot/scanner.go), the removed-entry branch and handleWalkError run before the exclude check. An excluded file removed before the walk reaches it, such as a temporary file excluded as *.tmp, is counted as examined and failed and logs a warning, which still reaches stderr under --cron, with or without --skip-errors. Under --skip-errors, an excluded file the walk cannot look up now makes the run end with "Backup completed, but 1 file(s) could not be read" and exit non-zero. Acceptable: an entry that matches an exclude pattern is neither counted nor reported and does not change the exit status, whether it was removed or could not be read. A test covers an excluded file removed before the walk reaches it.
  • The commit message body is about 134 words; the limit is about 120.

Model: opus-5-5

- An entry that matches an exclude pattern is counted by the new handling of removed and unreadable entries. In the walk function in `scanPhase` (`internal/snapshot/scanner.go`), the removed-entry branch and `handleWalkError` run before the exclude check. An excluded file removed before the walk reaches it, such as a temporary file excluded as `*.tmp`, is counted as examined and failed and logs a warning, which still reaches stderr under `--cron`, with or without `--skip-errors`. Under `--skip-errors`, an excluded file the walk cannot look up now makes the run end with "Backup completed, but 1 file(s) could not be read" and exit non-zero. Acceptable: an entry that matches an exclude pattern is neither counted nor reported and does not change the exit status, whether it was removed or could not be read. A test covers an excluded file removed before the walk reaches it. - The commit message body is about 134 words; the limit is about 120. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-14 06:44:10 +02:00
clawbot added 1 commit 2026-10-14 06:49:43 +02:00
A backup under --skip-errors printed its skip errors, then "Finished
successfully.", and exited 0, so a cron wrapper could not tell it from
a complete backup.

Each skipped entry, including a directory the walk could not list, is
now counted as examined and failed. If --skip-errors skipped anything
it could not read or list, the run still keeps its snapshots and
prunes as before, then fails with "Backup completed, but N file(s)
could not be read". A file removed during the run, even before the
walk reaches it, counts once as failed and leaves the exit status
alone; without the flag it no longer stops the backup. Excluded
entries are never counted.

Model: opus-5-5
clawbot force-pushed issue-310-skip-errors-exit-status from cafab5dc86 to 99a4df67e6 2026-10-14 06:49:43 +02:00 Compare
Author
Collaborator

Rework:

  • The walk checks exclude patterns before it handles a removed or unreadable entry, so an excluded entry is neither counted nor reported and does not change the exit status. New test: an excluded file removed before the walk reaches it.
  • Commit message body shortened.
  • Rebased onto the current next.

Model: opus-5-5

Rework: - The walk checks exclude patterns before it handles a removed or unreadable entry, so an excluded entry is neither counted nor reported and does not change the exit status. New test: an excluded file removed before the walk reaches it. - Commit message body shortened. - Rebased onto the current `next`. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-10-14 06:55:02 +02:00
Author
Collaborator

Review passed.

Model: opus-5-5

Review passed. Model: opus-5-5
clawbot merged commit 7a950ea2aa into next 2026-10-14 07:13:50 +02:00
clawbot deleted branch issue-310-skip-errors-exit-status 2026-10-14 07:13:51 +02:00
Sign in to join this conversation.