fix(store): plugin updates keep the old install until the new one succeeds#405
fix(store): plugin updates keep the old install until the new one succeeds#405ChuckBuilds wants to merge 1 commit into
Conversation
…ceeds Both reinstall paths in update_plugin — the monorepo-migration remote switch AND the routine archive update every store user hits — deleted the installed plugin directory BEFORE downloading its replacement. A mid-update failure (bad network, registry error) permanently destroyed the plugin. Seen in the field: a Pi with broken DNS lost 12 plugins in one update pass during the monorepo migration. New _reinstall_with_rollback: rename the old install aside (using the '.standalone-backup-' name pattern plugin discovery already excludes), run install_plugin, remove the aside on success — restore it on ANY failure, clearing partial-download debris first. A stale aside from a previous crash is cleared before starting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
📝 WalkthroughWalkthroughPlugin reinstall operations now preserve the existing directory during installation, restore it after failures, and clean up temporary artifacts. Two update paths use this helper, with tests covering failures, successful updates, stale backups, and plugin discovery. ChangesPlugin reinstall rollback
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/plugin_system/store_manager.py (2)
2284-2285: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked
_safe_remove_directoryreturn values hide cleanup failures.Both the stale-aside clear and the post-success aside removal ignore the boolean result of
_safe_remove_directory. If either genuinely fails (all three removal strategies exhausted), the function proceeds silently — a stale-clear failure surfaces later only as an opaque renameOSError, and a post-success failure leaves an orphaned.standalone-backup-migratingdirectory with no log trace. On a Pi's constrained storage, silently accumulating undetected orphaned directories works against the guideline to clean up resources and log clearly for remote debugging.As per coding guidelines, "Optimize code for Raspberry Pi's limited RAM and CPU capabilities" and "Clean up resources regularly to manage memory effectively."🛠️ Proposed fix
- if backup_path.exists(): - self._safe_remove_directory(backup_path) + if backup_path.exists(): + if not self._safe_remove_directory(backup_path): + self.logger.warning( + f"Could not fully clear stale backup {backup_path} before reinstalling {plugin_id}")if installed: - self._safe_remove_directory(backup_path) + if not self._safe_remove_directory(backup_path): + self.logger.warning( + f"Reinstall of {plugin_id} succeeded but stale backup {backup_path} could not be removed") return TrueAlso applies to: 2299-2300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/store_manager.py` around lines 2284 - 2285, Check the boolean result of _safe_remove_directory in both the stale-aside cleanup and post-success backup removal paths. When removal fails, explicitly log the cleanup failure with the affected backup_path and preserve the appropriate failure behavior instead of continuing silently. Ensure successful removals retain the existing flow.Source: Coding guidelines
2266-2318: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize
_reinstall_with_rollback()per plugin(src/plugin_system/store_manager.py:2266-2318)— there’s no per-plugin lock around the rename/install/restore sequence, so concurrent updates for the sameplugin_idcan race and turn into a spurious failed reinstall. A small lock around this path would make the rollback flow deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/store_manager.py` around lines 2266 - 2318, Serialize _reinstall_with_rollback per plugin_id by acquiring a lock before the backup rename and holding it through install_plugin, cleanup, and rollback, releasing it on every return or exception. Reuse the store manager’s existing lock pattern if available; ensure different plugin IDs can still proceed concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_store_update_rollback.py`:
- Around line 93-101: Strengthen test_stale_aside_from_previous_crash_is_cleared
to directly verify the stale backup directory is removed and the reinstall
proceeds far enough to invoke the mocked install_plugin. Assert the stale path
no longer exists after _reinstall_with_rollback, while preserving the expected
failed result and plugin directory assertions.
---
Nitpick comments:
In `@src/plugin_system/store_manager.py`:
- Around line 2284-2285: Check the boolean result of _safe_remove_directory in
both the stale-aside cleanup and post-success backup removal paths. When removal
fails, explicitly log the cleanup failure with the affected backup_path and
preserve the appropriate failure behavior instead of continuing silently. Ensure
successful removals retain the existing flow.
- Around line 2266-2318: Serialize _reinstall_with_rollback per plugin_id by
acquiring a lock before the backup rename and holding it through install_plugin,
cleanup, and rollback, releasing it on every return or exception. Reuse the
store manager’s existing lock pattern if available; ensure different plugin IDs
can still proceed concurrently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7620ae9-256a-400a-906e-76e4098fc4a0
📒 Files selected for processing (2)
src/plugin_system/store_manager.pytest/test_store_update_rollback.py
| def test_stale_aside_from_previous_crash_is_cleared(self, store): | ||
| mgr, plugin_dir = store | ||
| stale = plugin_dir.parent / f"{PLUGIN_ID}.standalone-backup-migrating" | ||
| stale.mkdir() | ||
| (stale / "old.txt").write_text("stale") | ||
| with patch.object(mgr, "install_plugin", return_value=False): | ||
| ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) | ||
| assert ok is False | ||
| assert plugin_dir.exists() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assertions don't actually prove the stale aside was cleared.
If the stale-clear at the top of _reinstall_with_rollback silently failed, plugin_path.rename(backup_path) would raise (destination non-empty) and return False before ever touching plugin_dir — leaving plugin_dir.exists() true and ok is False, which is exactly what this test asserts today. The test would pass identically whether or not the stale-clear logic works, so it doesn't actually cover the regression its name claims to guard against.
✅ Proposed strengthening
- with patch.object(mgr, "install_plugin", return_value=False):
+ with patch.object(mgr, "install_plugin", return_value=False) as mock_install:
ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir)
+ mock_install.assert_called_once()
assert ok is False
assert plugin_dir.exists()
+ assert "old version marker" in (plugin_dir / "manager.py").read_text()
+ assert not (plugin_dir / "old.txt").exists()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_stale_aside_from_previous_crash_is_cleared(self, store): | |
| mgr, plugin_dir = store | |
| stale = plugin_dir.parent / f"{PLUGIN_ID}.standalone-backup-migrating" | |
| stale.mkdir() | |
| (stale / "old.txt").write_text("stale") | |
| with patch.object(mgr, "install_plugin", return_value=False): | |
| ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) | |
| assert ok is False | |
| assert plugin_dir.exists() | |
| def test_stale_aside_from_previous_crash_is_cleared(self, store): | |
| mgr, plugin_dir = store | |
| stale = plugin_dir.parent / f"{PLUGIN_ID}.standalone-backup-migrating" | |
| stale.mkdir() | |
| (stale / "old.txt").write_text("stale") | |
| with patch.object(mgr, "install_plugin", return_value=False) as mock_install: | |
| ok = mgr._reinstall_with_rollback(PLUGIN_ID, plugin_dir) | |
| mock_install.assert_called_once() | |
| assert ok is False | |
| assert plugin_dir.exists() | |
| assert "old version marker" in (plugin_dir / "manager.py").read_text() | |
| assert not (plugin_dir / "old.txt").exists() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_store_update_rollback.py` around lines 93 - 101, Strengthen
test_stale_aside_from_previous_crash_is_cleared to directly verify the stale
backup directory is removed and the reinstall proceeds far enough to invoke the
mocked install_plugin. Assert the stale path no longer exists after
_reinstall_with_rollback, while preserving the expected failed result and plugin
directory assertions.
1 participant
Summary
Data-loss fix. Both reinstall paths in
update_plugin— the monorepo-migration remote switch and the routine archive update that every store user hits — did_safe_remove_directory(plugin_path)beforeinstall_plugin(). Any mid-update failure (flaky WiFi, DNS outage, registry hiccup) permanently destroyed the plugin.This isn't theoretical: during testing, a Pi with broken outbound DNS ran an update pass and lost 12 plugins in one go — every plugin whose git remote pointed at the old pre-monorepo repos was deleted and none could be re-downloaded.
The fix (
_reinstall_with_rollback): rename the old install aside →install_plugin()→ remove the aside on success, restore it on any failure (clearing partial-download debris first). The aside name uses the.standalone-backup-pattern that plugin discovery already excludes, so a crash mid-update never yields a phantom duplicate plugin. A stale aside from a previous crash is cleared before starting.Verification
6 new unit tests: failed install restores the old version; install exception restores; success removes the aside; partial-download debris is replaced by the old version; stale asides are cleared; the aside is invisible to
_scan_directory_for_plugins. Store cache + uninstall/reconcile suites green.🤖 Generated with Claude Code
https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
Summary by CodeRabbit
Bug Fixes
Tests