fix(backup): wait for the backup slot on the update path, and stop blaming the backup - #90742
Open
isndotbiz wants to merge 1 commit into
Open
fix(backup): wait for the backup slot on the update path, and stop blaming the backup#90742isndotbiz wants to merge 1 commit into
isndotbiz wants to merge 1 commit into
Conversation
…aming the backup
`hermes update --backup` could print
⚠ Backup skipped (no files found or write failed); continuing update.
and carry on with no rollback point. Neither half of that message was true.
`_backup_operation_lock` waits 0.25s for the shared backup slot. On timeout it
raises BackupInProgressError, which `_write_full_zip_backup` swallowed into a
logger.warning and a None return — indistinguishable, at the call site, from an
empty scan or a failed write. So losing a quarter-second race with any
concurrent snapshot silently skipped the only backup an update takes.
Reproduced on a live install: create_pre_update_backup() called directly wrote
14,003 files / 345 MB in 80s with every .db passing safe-copy, while the same
call under a held lock returned None in exactly 0.25s.
- The update path now waits up to _PRE_UPDATE_LOCK_TIMEOUT (180s) for the slot,
for both the quick snapshot and the full zip. An update that waits beats an
update that proceeds unprotected; the bound keeps a wedged backup from
stalling it forever. The interactive default stays 0.25s — `hermes backup`
should still fail fast.
- One "waiting for the backup slot" warning after 2s, so a multi-minute wait
does not look like a hang.
- raise_if_busy lets the caller tell contention from failure. `hermes update`
now names the lock conflict and says the backup itself is fine; the remaining
None case says "nothing to archive, or the archive could not be written"
instead of asserting both.
Existing callers are untouched: raise_if_busy defaults to False, so
create_pre_migration_backup and `hermes backup` keep the old swallow-and-None
behaviour and the old timeout.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a misleading and unsafe behavior in the hermes update --backup flow where a brief lock-contention race could be reported as a “skipped/failed” backup and allow the update to proceed without a rollback artifact, even though the backup system itself was healthy.
Changes:
- Introduces distinct lock-timeout policies for interactive backups vs. pre-update backups, and adds a one-time “still waiting” notice while contending for the backup slot.
- Makes backup-slot contention distinguishable from “no files / write failed” by optionally re-raising
BackupInProgressErrorinstead of collapsing it into aNoneresult. - Updates
hermes updatemessaging and adds regression tests to ensure contention is reported accurately and the intended timeout is actually plumbed through.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hermes_cli/backup.py |
Adds separate timeouts for update vs. interactive paths; improves lock acquisition behavior; adds raise_if_busy/lock_timeout plumbing to distinguish contention from other None outcomes. |
hermes_cli/update_cmd.py |
Passes the pre-update timeout explicitly to both snapshot and full-zip backups; prints accurate user messaging on lock contention vs. other skip cases. |
tests/hermes_cli/test_backup.py |
Adds a regression test ensuring hermes update --backup reports lock contention (and that patched timeouts actually reach the call sites). |
tests/hermes_cli/test_backup_stability.py |
Adds tests covering the new “busy slot vs. failed write” distinction and verifying the pre-update path uses the longer lock timeout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
hermes update --backupcan printand then run the whole update with no rollback point — on an install where the backup works
perfectly. Neither half of that message is true.
_backup_operation_lock(added in #77913 to serialize snapshots) waits 0.25s for the sharedbackup slot. On timeout it raises
BackupInProgressError, which_write_full_zip_backupswallowsinto a
logger.warningand aNonereturn — the sameNonethe caller gets from an empty scan ora failed write.
_run_pre_update_backuptherefore cannot tell contention from failure, and reportsa lost quarter-second race as a broken backup.
Two things are wrong with that, and this PR fixes both:
hermes backup— the user is sitting there and can retry. The pre-update backup is the onlything standing between a bad update and an unrecoverable
~/.hermes(cf. hermes update --yes wiped entire ~/.hermes/ directory (.env, MEMORY.md, kanban.db, skills, scripts — all gone) #48200), so it shouldwait rather than skip. Any concurrent snapshot — a scheduled backup, the desktop/gateway
process, a second terminal — is enough to lose the race.
broken backup, and found a backup that works.
Related Issue
No existing issue — found while debugging a live install. Adjacent, different causes:
#75724 (a non-SQLite
.dbaborts the full backup) and #48200 (the update that wiped~/.hermes,which is why the pre-update backup exists at all).
Type of Change
Changes Made
hermes_cli/backup.py_BACKUP_LOCK_DEFAULT_TIMEOUT = 0.25— names the existing interactive default; unchanged._PRE_UPDATE_LOCK_TIMEOUT = 180.0— the update path's wait. Bounded so a wedged backup processcannot stall an update forever.
_backup_operation_locklogs one "waiting for the Hermes backup slot" warning after 2s, so amulti-minute wait does not look like a hang.
_write_full_zip_backup(..., lock_timeout=..., raise_if_busy=False)—raise_if_busyre-raisesBackupInProgressErrorinstead of collapsing it intoNone, so a caller can say which happened.create_pre_update_backup(..., lock_timeout=_PRE_UPDATE_LOCK_TIMEOUT, raise_if_busy=False)andcreate_quick_snapshot(..., lock_timeout=_BACKUP_LOCK_DEFAULT_TIMEOUT)plumb it through.hermes_cli/update_cmd.py_PRE_UPDATE_LOCK_TIMEOUTon the update path,passed explicitly at both call sites. Not left to the default argument: a default binds at def
time, so a timeout taken that way cannot be overridden and cannot be tested without sitting
through the real wait (which is how my own test first passed for the wrong reason — see below).
Nonecase says "nothing to archive, or the archive could not be written" insteadof asserting both at once.
Backwards compatibility:
raise_if_busydefaults toFalseand the lock's default timeout isunchanged, so
hermes backupandcreate_pre_migration_backupbehave exactly as before.How to Test
Reproduction (no patch): hold the slot in one process, ask for a backup in another.
Before:
Noneafter 0.25s, andhermes update --backupprints "no files found or write failed".After: the call waits for the slot; if it never frees within the bound,
hermes updatesays theslot was held and that the backup is not the problem.
That the backup is healthy the whole time — same install, slot free:
Tests added in
tests/hermes_cli/test_backup_stability.py:test_busy_slot_is_distinguishable_from_a_failed_writetest_pre_update_backup_reports_a_busy_slot_when_askedtest_pre_update_backup_waits_for_the_slotand in
tests/hermes_cli/test_backup.py:TestRunPreUpdateBackup::test_full_mode_names_lock_contention_instead_of_blaming_the_backup(asserts the old "no files found" wording is gone, and that the patched timeout actually reaches
the call — it caught a real defect: the first version of this patch took the timeout as a default
argument, so the test waited out the full 180s and passed for the wrong reason. The full-suite
run surfaced it as a 120s pytest-timeout; the assertion now fails fast instead.)
Checklist
Code
backup.py, but both are about_safe_copy_dbhanging on locked SQLite sources — a differentlock, a different failure)
pytest tests/ -qand all tests pass —tests/hermes_cliin full: see belowOn the suite: I ran all of
tests/hermes_cli(634 files) under the install's own venv:5,922 passed, 63 skipped, 9 errors in 29m40s. All 9 errors are teardown errors in
tests/hermes_cli/test_web_server_approvals_broadcast.py(
AttributeError: 'types.SimpleNamespace' object has no attribute '_methods'in the_reset_tui_gateway_server_statefixture) — nothing to do with this change; that install carries alocal modification to
hermes_cli/web_server.py, so I would not read those as upstream-cleaneither. The one failure that was mine — the pre-update backup test hitting a 120s timeout — is
the defect described above and is fixed in this PR; the affected suites now run
97 passed in 1m40s (
test_backup.py,test_backup_stability.py,test_cmd_update.py).I did not run the other ~2,400 test files outside
tests/hermes_cli, so I have not checked thatbox.
Documentation & Housekeeping
are internal, so no
cli-config.yaml.examplekey was addedmsvcrt/fcntlsplit; both branches get the same_notice()and the same deadlinearithmetic. Not executed on Windows or macOS.