Skip to content

fix(backup): floor pre-update backup_keep to 1 so the new backup survives#19730

Merged
teknium1 merged 1 commit into
mainfrom
hermes/hermes-8c54fd4a
May 4, 2026
Merged

fix(backup): floor pre-update backup_keep to 1 so the new backup survives#19730
teknium1 merged 1 commit into
mainfrom
hermes/hermes-8c54fd4a

Conversation

@teknium1

@teknium1 teknium1 commented May 4, 2026

Copy link
Copy Markdown
Contributor

Salvage of #16555 onto current main.

Summary

updates.backup_keep: 0 (or negative) silently deleted the freshly-created pre-update backup, leaving the user with no recovery point even though Saved: <path> was printed. Floor the keep parameter to 1 inside _prune_pre_update_backups because this helper is only called immediately after a fresh backup is written. Users who genuinely don't want a backup should set updates.pre_update_backup: false.

Validation

Manual review of diff.

Original PR: #16555

…ives

`updates.backup_keep: 0` (or any negative value) wiped the freshly-
created pre-update zip:

  _prune_pre_update_backups(backup_dir, keep=0):
      backups = sorted(..., reverse=True)   # newest first, includes
                                            # the zip we just wrote
      for p in backups[0:]:                 # = all of them
          p.unlink()

The wrapper in `main.py` then printed `Saved: <path>` for a file that
no longer existed (the size lookup is wrapped in `try/except OSError`
which silently degrades to "0 B"), leaving operators believing they had
a recovery point when they had none.

This is a real footgun because some config systems treat 0 as "keep
unlimited"; here it does the opposite — every backup is destroyed
right after creation.

Fix: clamp `keep` to a minimum of 1 inside `_prune_pre_update_backups`
since that helper is only invoked immediately after a fresh backup
is written.  Operators who genuinely want no backups should set
`updates.pre_update_backup: false` (which gates creation entirely)
rather than relying on `backup_keep: 0`.

Also extends the `backup_keep` config docstring to spell out the floor
and point at `pre_update_backup: false` as the off-switch.

## Tests

Three regression tests added in `TestPreUpdateBackup`:

  - `test_keep_zero_does_not_delete_freshly_created_backup` —
    asserts the file persists after `keep=0`
  - `test_keep_negative_does_not_delete_freshly_created_backup` —
    same for negative values
  - `test_keep_zero_still_prunes_older_backups` — proves the floor
    only protects the new backup; older ones are still rotated out

Verified the new tests fail on origin/main (without the floor) and
pass with it; full `tests/hermes_cli/test_backup.py` suite green
(84 tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@teknium1 teknium1 merged commit b46b0c9 into main May 4, 2026
8 of 10 checks passed
@teknium1 teknium1 deleted the hermes/hermes-8c54fd4a branch May 4, 2026 12:07
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants