Skip to content

fix(plugins): prevent data loss when a plugin update fails mid-replacement - #7

Open
NexzaDev wants to merge 2 commits into
Valenced:mainfrom
NexzaDev:fix/plugin-update-backup-rollback
Open

NexzaDev wants to merge 2 commits into
Valenced:mainfrom
NexzaDev:fix/plugin-update-backup-rollback

Conversation

@NexzaDev

Copy link
Copy Markdown

Summary

applyStagedPlugin replaced an installed plugin by deleting dist/<id>/ and only then moving the staged directory into place. If the move failed (or the process died between the two calls), the previously working plugin was gone and nothing was installed in its place.

This PR makes the replacement failure-safe: the installed version is moved aside to a backup, the staged version is committed, and on any failure the backup is moved back. The backup is only deleted once the new version is in place.

Root cause

core/.../plugins/external/Artifacts.kt:

paths.fs.deleteRecursively(dest, mustExist = false)
paths.fs.atomicMove(staged.dir, dest)

The delete is irreversible and happens before the commit step is known to succeed. Callers (installSubmit in RepoInstall.kt, confirmPluginInstall in the Android Install.kt) then discard the staged directory in their error paths, so a failed move left no copy of either version.

The fix

New file PluginReplacement.kt, called from the unchanged public applyStagedPlugin:

  1. Recover an interrupted earlier update of the same plugin (dist/<id> missing, backups/<id> present).
  2. Remove a stale backup left by an earlier failed cleanup (only while dist/<id> exists).
  3. First install: move the staged directory into place.
  4. Update: move dist/<id> to backups/<id>, move the staged directory to dist/<id>, then delete the backup.
  5. Any failure in step 4 rolls back (backups/<id> -> dist/<id>). If rollback also fails, the backup is left untouched and the thrown PluginSystemError (INSTALL_FAILED) names its path in the message and in details.backupPath / details.pluginPath, with the rollback failure attached as a suppressed exception.
  6. A failed backup cleanup after a successful commit only logs a warning. The active plugin is never touched by cleanup.

Design notes:

  • Backups live in a new sibling directory plugins/backups/, not inside dist/. Android discovery deletes every dot-prefixed entry in dist/ at boot, so a backup kept there would be wiped exactly when it is needed.
  • restoreInterruptedReplacements(paths) restores backups whose plugin directory is missing (crash between the two moves). It never overwrites an existing dist/<id>. Android discovery calls it before scanning dist/.
  • Uninstall (valenced.plugins.uninstall) now also removes backups/<id>, so a stale backup cannot resurrect an uninstalled plugin through the recovery above.
  • Replacements and recovery are serialised with the existing reentrant app.valenced.core.Lock, so concurrent installs of the same plugin queue instead of interleaving. No new dependency.
  • Every step is an okio atomicMove (a rename) within the same plugin root. The sequence as a whole is deliberately not claimed to be atomic: there is a short window where dist/<id> is absent, which is what the recovery paths cover.

Other code paths audited

  • stagePluginZip deletes only its own temp directory (.repo-install-*, .confirm-*), never an installed plugin.
  • Discovery.kt deletes dot-prefixed leftovers by design. Backups are outside dist/.
  • PluginMethods.kt uninstall deletes the installed plugin on purpose. It only gained the backup removal described above.
  • Files.atomicWrite already uses a .bak restore pattern for files, and is untouched.
  • iOS only stages via stagePluginZipC. The swap itself is not done in core there, so nothing to change.

Files changed

File Change
core/.../plugins/external/PluginReplacement.kt New: backup, commit, rollback, recovery, locking
core/.../plugins/external/Artifacts.kt applyStagedPlugin delegates to the new implementation (same signature)
core/.../plugins/PluginPaths.kt New backups path (additive)
android/.../plugins/external/Discovery.kt Restore interrupted updates before scanning dist/
android/.../plugins/PluginMethods.kt Uninstall also removes backups/<id>
core/src/commonTest/.../ApplyStagedPluginTest.kt New regression tests

No public API was removed or changed.

Tests

ApplyStagedPluginTest (commonTest, uses a ForwardingFileSystem to inject move/delete failures):

  • first install with no previous version
  • update replaces every file of the old version (including removed files)
  • updating one plugin leaves other plugins untouched
  • repeated updates leave no temporary or backup directories
  • failed backup move leaves the old version intact
  • failed commit restores the old version
  • failed rollback preserves the backup and reports its path
  • failed cleanup keeps the new version active
  • leftover backup is cleared by the next update
  • unremovable leftover backup blocks the update without touching the installed version
  • interrupted update recovered before the next apply and at startup
  • startup recovery never overwrites an installed plugin
  • failed first install leaves no plugin behind

Verification (please read)

The full Gradle build could not be run in my environment: the Android SDK is not installed and Gradle plugin/Maven hosts were unreachable. What I actually ran:

  • Compiled the changed core sources, okio 3.18.2 (from source) and the new test class with Kotlin 2.0.21 in a small harness, and ran the 15 new tests with JUnit 4: 15/15 passed.
  • Ran the same tests against the original implementation: 8 of 15 failed, so the tests do detect the bug.
  • Harness differences from the real build: Kotlin 2.0.21 instead of 2.4.10, a temp-directory-backed filesystem instead of FakeFileSystem, and a stub ExternalManifest.
  • The Android edits (Discovery.kt, PluginMethods.kt) are small and were not compiled.

Reviewers/CI should run ./gradlew :core:allTests and the Android host tests. Concurrency (the lock) is not covered by a test because commonTest has no threads.

Known limitations

  • A crash between the two renames still leaves a short window with no dist/<id>. It is repaired at the next boot (or the next install of that plugin) rather than prevented.
  • If a stale backup cannot be deleted, updates of that plugin fail with an explicit error until it can be removed. The installed version is never touched in that case.

…tted

Move the installed plugin to a backup before committing the staged one, restore it on failure, and recover interrupted updates at startup.
@PalmDevs

Copy link
Copy Markdown
Member

Does the issue actually ever happen? If an action as simple as moving a directory results in an error, what guarantees you can restore the backup to the same place?

@NexzaDev

Copy link
Copy Markdown
Author

Yeah, it did happen to me. I updated a plugin and the old version was gone afterwards, with nothing installed in its place.

The thing is, it doesn't need the move to throw an error. The old code deleted the folder first and moved the new one in after, so if the app gets killed or the device reboots between those two steps, nothing fails, the plugin is just gone. Now the old version is kept as a backup until the new one is in place, and if the app dies in the middle, it gets restored on the next start.

As for the rollback, you're right, I can't promise it never fails. But renaming a folder back to a name we just freed is a much simpler operation than the first move, so I'd expect it to work in almost every case. And if it doesn't, the backup stays untouched and the error says where it is, instead of the data being deleted like before.

Happy to make it smaller if you'd rather, but I'd keep the part where nothing gets deleted before the new version is in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants