Repository navigation
Conversation
…tted Move the installed plugin to a backup before committing the staged one, restore it on failure, and recover interrupted updates at startup.
|
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? |
|
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. |
Summary
applyStagedPluginreplaced an installed plugin by deletingdist/<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:The delete is irreversible and happens before the commit step is known to succeed. Callers (
installSubmitinRepoInstall.kt,confirmPluginInstallin the AndroidInstall.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 publicapplyStagedPlugin:dist/<id>missing,backups/<id>present).dist/<id>exists).dist/<id>tobackups/<id>, move the staged directory todist/<id>, then delete the backup.backups/<id>->dist/<id>). If rollback also fails, the backup is left untouched and the thrownPluginSystemError(INSTALL_FAILED) names its path in the message and indetails.backupPath/details.pluginPath, with the rollback failure attached as a suppressed exception.Design notes:
plugins/backups/, not insidedist/. Android discovery deletes every dot-prefixed entry indist/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 existingdist/<id>. Android discovery calls it before scanningdist/.valenced.plugins.uninstall) now also removesbackups/<id>, so a stale backup cannot resurrect an uninstalled plugin through the recovery above.app.valenced.core.Lock, so concurrent installs of the same plugin queue instead of interleaving. No new dependency.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 wheredist/<id>is absent, which is what the recovery paths cover.Other code paths audited
stagePluginZipdeletes only its own temp directory (.repo-install-*,.confirm-*), never an installed plugin.Discovery.ktdeletes dot-prefixed leftovers by design. Backups are outsidedist/.PluginMethods.ktuninstall deletes the installed plugin on purpose. It only gained the backup removal described above.Files.atomicWritealready uses a.bakrestore pattern for files, and is untouched.stagePluginZipC. The swap itself is not done in core there, so nothing to change.Files changed
core/.../plugins/external/PluginReplacement.ktcore/.../plugins/external/Artifacts.ktapplyStagedPlugindelegates to the new implementation (same signature)core/.../plugins/PluginPaths.ktbackupspath (additive)android/.../plugins/external/Discovery.ktdist/android/.../plugins/PluginMethods.ktbackups/<id>core/src/commonTest/.../ApplyStagedPluginTest.ktNo public API was removed or changed.
Tests
ApplyStagedPluginTest(commonTest, uses aForwardingFileSystemto inject move/delete failures):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:
coresources,okio3.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.FakeFileSystem, and a stubExternalManifest.Discovery.kt,PluginMethods.kt) are small and were not compiled.Reviewers/CI should run
./gradlew :core:allTestsand the Android host tests. Concurrency (the lock) is not covered by a test becausecommonTesthas no threads.Known limitations
dist/<id>. It is repaired at the next boot (or the next install of that plugin) rather than prevented.