fix(store): plugin updates keep the old install until the new one succeeds - #405
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
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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 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 | 12 |
| 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
…sults CodeRabbit review on #405 flagged two things in _reinstall_with_rollback, both verified against current code: - Real race: the web UI runs Flask with threaded=True and there's a single update route, so two overlapping requests for the same plugin_id (double-click, two tabs) can interleave. The loser could rename the winner's in-progress install aside mid-download, deleting its own rollback safety net — worse than the bug this function exists to fix. Added a lazy per-plugin_id lock dict (mirrors the plugin_manager per-plugin lock pattern) held for the whole function. - _safe_remove_directory's return value was ignored at both call sites. Stale-aside cleanup failure now aborts cleanly instead of falling through to a rename that would fail anyway with a less useful error; post-success backup-removal failure now logs instead of failing silently (still returns True — the update itself succeeded, and the next update self-heals the leftover aside). Left the third nitpick (test_stale_aside_from_previous_crash_is_cleared) addressed by asserting the stale dir is actually gone and that install_plugin was reached, rather than just the end-to-end result. Added a concurrency regression test asserting install_plugin never runs for the same plugin_id while another call is in flight.
|
Addressed all three review items: Locking (real issue, not a nitpick): confirmed
Test strengthening: All 7 tests in |
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