Restore persistent directory when a download fails - #5378
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes update/install rollback behavior so a repository’s persistent_directory is always moved back into place, even when the download step fails, preventing user data from being stranded in /tmp and lost on reboot.
Changes:
- Wraps repository download steps in
try/finallyand restores the persistent directory infinally. - Adds a regression test covering a failed update download with an existing persistent directory.
- Adds an API-usage snapshot for the new test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| custom_components/hacs/repositories/base.py | Ensures persistent directory restoration runs on both success and failure paths during install/update. |
| tests/repositories/test_update_repository.py | Adds regression test verifying persistent user data survives a failed download. |
| tests/snapshots/api-usage/tests/repositories/test_update_repositorytest-update-repository-entity-download-failure-keeps-persistent-directory.json | Records expected API usage for the new regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "hacs-test-org/integration-basic") | ||
|
|
||
| installed_file = Path(repo.localpath) / "__init__.py" | ||
| installed_file.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Is the repo.localpath inside the configuration directory which we've set to be inside a tmp directory for the tests?
There was a problem hiding this comment.
Yes. For an integration localpath is {hacs.core.config_path}/custom_components/{domain}, and hacs.core.config_path is set from hass.config.path(), which the hass fixture points at pytest's tmp dir. Printed during a run it resolves to /tmp/pytest-of-<user>/pytest-<n>/test_update_repository_entity_0/custom_components/example, so the test only writes inside the temporary configuration directory.
The backup and persistent directory that the code moves things through do land in the real system temp dir (tempfile.gettempdir(), so /tmp/hacs_backup/ and /tmp/hacs_persistent_integration/), but that is existing behavior of utils/backup.py that all the other download tests share, not something this test introduces.
| "https://raw.githubusercontent.com/hacs-test-org/integration-basic/1.0.0/README.md": 1, | ||
| "https://raw.githubusercontent.com/hacs-test-org/integration-basic/2.0.0/hacs.json": 1 | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
There was a problem hiding this comment.
These snapshots are generated, not hand written. track_api_usage in tests/conftest.py passes safe_json_dumps(...) to snapshots.assert_match, and that output has no trailing newline, so none of the 96 existing api-usage snapshots have one either. Adding it makes the next run fail:
E AssertionError: value does not match the expected value in snapshot
E assert '{\n "test...: 1\n }\n}' == '{\n "test...1\n }\n}\n'
Leaving these as the fixture writes them.
| "https://raw.githubusercontent.com/hacs-test-org/integration-basic/1.0.0/custom_components/example/manifest.json": 1, | ||
| "https://raw.githubusercontent.com/hacs-test-org/integration-basic/2.0.0/hacs.json": 1 | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
There was a problem hiding this comment.
These snapshots are generated, not hand written. track_api_usage in tests/conftest.py passes safe_json_dumps(...) to snapshots.assert_match, and that output has no trailing newline, so none of the 96 existing api-usage snapshots have one either. Adding it makes the next run fail:
E AssertionError: value does not match the expected value in snapshot
E assert '{\n "test...: 1\n }\n}' == '{\n "test...1\n }\n}\n'
Leaving these as the fixture writes them.
Proposed change
When downloading an update for an installed repository, the persistent directory (
persistent_directoryinhacs.json) is moved out of the install directory into/tmpbefore the backup of the old install is created. That backup therefore never contains the persistent directory.The restore of the persistent directory only happened on the success path. On a failed download, the error path raised before reaching it: the backup restore brought back the old install without the persistent data, which stayed stranded in
/tmpand was gone after a reboot.This change wraps the download steps in
try/finallyand moves the persistent directory restore into thefinallyblock, so it is always moved back in place. This also covers download paths that raise directly (like "No content to download") instead of going through the validation error branch.Added a test that upgrades an installed integration with a persistent directory present, injects a download failure, and asserts the user data is still there afterwards. The test fails on the previous code.
Type of change
Checklist