bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package upgrade can fail when the package metadata is out of sync. The
upgrade process first deletes the package, tries to download the new
version. If the download fails for some reason (metadata out of sync or
the file not available), the upgrade aborts but the earlier package
deletion is not rolled back as one would expect for an upgrade
transaction. Ideally the download would be performed first such that the
upgrade process wouldn't lead to a missing package.

I just executed M-x package-upgrade-all. The following messages appeared
in the log:

Package ‘<package>-<old-version>’ deleted.
Contacting host: elpa.gnu.org:443
package--with-response-buffer-1:
https://elpa.gnu.org/devel/<pkgname>-<new-version>.tar: No Data

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Package upgrade can fail when the package metadata is out of sync. The
> upgrade process first deletes the package, tries to download the new
> version. If the download fails for some reason (metadata out of sync or
> the file not available), the upgrade aborts but the earlier package
> deletion is not rolled back as one would expect for an upgrade
> transaction. Ideally the download would be performed first such that the
> upgrade process wouldn't lead to a missing package.
>
> I just executed M-x package-upgrade-all. The following messages appeared
> in the log:
>
> Package ‘<package>-<old-version>’ deleted.
> Contacting host: elpa.gnu.org:443
> package--with-response-buffer-1:
> https://elpa.gnu.org/devel/<pkgname>-<new-version>.tar: No Data

I cannot test this out right now, but it seems that the fix should be
straight-forward


[-- Attachment #2: Type: text/plain, Size: 1182 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 438af781393..46b2d7580c1 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2268,12 +2268,16 @@ package-upgrade
     ;; `pkg-desc' will be nil when the package is an "active built-in".
     (if (and pkg-desc (package-vc-p pkg-desc))
         (package-vc-upgrade pkg-desc)
-      (when pkg-desc
-        (package-delete pkg-desc 'force 'dont-unselect))
       (package-install package
                        ;; An active built-in has never been "selected"
                        ;; before.  Mark it as installed explicitly.
-                       (and pkg-desc 'dont-select)))))
+                       (and pkg-desc 'dont-select))
+      ;; We delete the old package via the descriptor after installing
+      ;; the new package to avoid losing the package if there issues
+      ;; during installation (Bug#74556).
+      (cl-assert (package-desc-dir pkg-desc))
+      (when pkg-desc
+        (package-delete pkg-desc 'force 'dont-unselect)))))
 
 (defun package--upgradeable-packages (&optional include-builtins)
   ;; Initialize the package system to get the list of package

[-- Attachment #3: Type: text/plain, Size: 234 bytes --]


It might make sense to try and "deactivate" a package before installing
the new package.  Looking into some second-try fallback for
package-install to refresh the package index if a package was not found
would also be a good idea ^^

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

Philip Kaludercic <philipk@posteo.net> writes:

[...]

> It might make sense to try and "deactivate" a package before installing
> the new package.  Looking into some second-try fallback for
> package-install to refresh the package index if a package was not found
> would also be a good idea ^^

This might do it?


[-- Attachment #2: Type: text/plain, Size: 1299 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 438af781393..3800d8fa56d 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2095,7 +2095,7 @@ package-archive-base
   "Return the package described by DESC."
   (cdr (assoc (package-desc-archive desc) package-archives)))
 
-(defun package-install-from-archive (pkg-desc)
+(defun package-install-from-archive-1 (pkg-desc)
   "Download and install a package defined by PKG-DESC."
   ;; This won't happen, unless the archive is doing something wrong.
   (when (eq (package-desc-kind pkg-desc) 'dir)
@@ -2141,6 +2141,15 @@ package-install-from-archive
                                                  package-alist))))
                  (setf (package-desc-signed (car pkg-descs)) t))))))))))
 
+(defun package-install-from-archive (pkg-desc)
+  "Download and install PKG-DESC, refreshing the archive if necessary."
+  (condition-case msg
+      (package-install-from-archive-1 pkg-desc)
+    (error
+     (when (string-match-p "\\`Error retrieving: " (cadr msg))
+       (package-refresh-contents)
+       (package-install-from-archive-1 pkg-desc)))))
+
 ;;;###autoload
 (defun package-installed-p (package &optional min-version)
   "Return non-nil if PACKAGE, of MIN-VERSION or newer, is installed.

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Eli Zaretskii]

> Cc: 74556@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Thu, 28 Nov 2024 11:34:51 +0000
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> [...]
> 
> > It might make sense to try and "deactivate" a package before installing
> > the new package.  Looking into some second-try fallback for
> > package-install to refresh the package index if a package was not found
> > would also be a good idea ^^
> 
> This might do it?

Philip, please install this on the emacs-30 branch, unless you see any
problems with the change.  We'd like to make another pretest soon.

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 74556@debbugs.gnu.org
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>> 
>> Philip Kaludercic <philipk@posteo.net> writes:
>> 
>> [...]
>> 
>> > It might make sense to try and "deactivate" a package before installing
>> > the new package.  Looking into some second-try fallback for
>> > package-install to refresh the package index if a package was not found
>> > would also be a good idea ^^
>> 
>> This might do it?
>
> Philip, please install this on the emacs-30 branch, unless you see any
> problems with the change.  We'd like to make another pretest soon.

Done.  But we should keep the report open as there might be better
approaches to discuss in the future.

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 74556@debbugs.gnu.org
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>>> 
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>> 
>>> [...]
>>> 
>>> > It might make sense to try and "deactivate" a package before installing
>>> > the new package.  Looking into some second-try fallback for
>>> > package-install to refresh the package index if a package was not found
>>> > would also be a good idea ^^
>>> 
>>> This might do it?
>>
>> Philip, please install this on the emacs-30 branch, unless you see any
>> problems with the change.  We'd like to make another pretest soon.
>
> Done.  But we should keep the report open as there might be better
> approaches to discuss in the future.

Hello Philip,

I just tried the modified `package-upgrade' function and it doesn't seem
to work. It seems to break the upgrade procedure in an even worse way,
at least in my setup. Now `package-install' is tried first with the
package symbol, which will be a no-op, since the package is already
installed. Afterwards the package is deleted and we always end up with
no package. Probably `package-install' should also be called with a
package descriptor of the new package version?

Thanks!

Daniel

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: 74556@debbugs.gnu.org
>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>>>> 
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>> 
>>>> [...]
>>>> 
>>>> > It might make sense to try and "deactivate" a package before installing
>>>> > the new package.  Looking into some second-try fallback for
>>>> > package-install to refresh the package index if a package was not found
>>>> > would also be a good idea ^^
>>>> 
>>>> This might do it?
>>>
>>> Philip, please install this on the emacs-30 branch, unless you see any
>>> problems with the change.  We'd like to make another pretest soon.
>>
>> Done.  But we should keep the report open as there might be better
>> approaches to discuss in the future.
>
> Hello Philip,
>
> I just tried the modified `package-upgrade' function and it doesn't seem
> to work. It seems to break the upgrade procedure in an even worse way,
> at least in my setup. Now `package-install' is tried first with the
> package symbol, which will be a no-op, since the package is already
> installed. Afterwards the package is deleted and we always end up with
> no package. Probably `package-install' should also be called with a
> package descriptor of the new package version?

Right, my sincere apologies for that oversight.  That being said, I
don't feel comfortable fixing this right now as I am short on time to
fix and test something like this on the "emacs-30" branch.  My vote
would be to revert the commit and try to tackle the issue on the
"master" branch.  An alternative I can propose that would be closer to
the original code might be


[-- Attachment #2: Type: text/plain, Size: 1450 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index fad0762e126..93b647edcea 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2268,16 +2285,14 @@ package-upgrade
     ;; `pkg-desc' will be nil when the package is an "active built-in".
     (if (and pkg-desc (package-vc-p pkg-desc))
         (package-vc-upgrade pkg-desc)
-      (package-install package
-                       ;; An active built-in has never been "selected"
-                       ;; before.  Mark it as installed explicitly.
-                       (and pkg-desc 'dont-select))
-      ;; We delete the old package via the descriptor after installing
-      ;; the new package to avoid losing the package if there issues
-      ;; during installation (Bug#74556).
-      (when pkg-desc
-        (cl-assert (package-desc-dir pkg-desc))
-        (package-delete pkg-desc 'force 'dont-unselect)))))
+      (unwind-protect
+          (when pkg-desc
+            (cl-assert (package-desc-dir pkg-desc))
+            (package-delete pkg-desc 'force 'dont-unselect))
+        (package-install package
+                         ;; An active built-in has never been "selected"
+                         ;; before.  Mark it as installed explicitly.
+                         (and pkg-desc 'dont-select))))))
 
 (defun package--upgradeable-packages (&optional include-builtins)
   ;; Initialize the package system to get the list of package

[-- Attachment #3: Type: text/plain, Size: 22 bytes --]


> Thanks!
>
> Daniel

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Daniel Mendler <mail@daniel-mendler.de> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> Cc: 74556@debbugs.gnu.org
>>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>>>>> 
>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>> 
>>>>> [...]
>>>>> 
>>>>> > It might make sense to try and "deactivate" a package before installing
>>>>> > the new package.  Looking into some second-try fallback for
>>>>> > package-install to refresh the package index if a package was not found
>>>>> > would also be a good idea ^^
>>>>> 
>>>>> This might do it?
>>>>
>>>> Philip, please install this on the emacs-30 branch, unless you see any
>>>> problems with the change.  We'd like to make another pretest soon.
>>>
>>> Done.  But we should keep the report open as there might be better
>>> approaches to discuss in the future.
>>
>> Hello Philip,
>>
>> I just tried the modified `package-upgrade' function and it doesn't seem
>> to work. It seems to break the upgrade procedure in an even worse way,
>> at least in my setup. Now `package-install' is tried first with the
>> package symbol, which will be a no-op, since the package is already
>> installed. Afterwards the package is deleted and we always end up with
>> no package. Probably `package-install' should also be called with a
>> package descriptor of the new package version?
>
> Right, my sincere apologies for that oversight.  That being said, I
> don't feel comfortable fixing this right now as I am short on time to
> fix and test something like this on the "emacs-30" branch.  My vote
> would be to revert the commit and try to tackle the issue on the
> "master" branch.  An alternative I can propose that would be closer to
> the original code might be

Yes, I also vote to revert your commit on the emacs-30 release branch.
The issue isn't severe (and not a regression), so I'd say it is okay to
fix the issue on the master branch.

I don't understand how the code you proposed works. It seems that after
the deletion, if the installation fails, the package will stay deleted?
Anyway, no hurry from my side to get this fixed.

Thanks.

Daniel

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

Daniel Mendler <mail@daniel-mendler.de> > Philip Kaludercic <philipk@posteo.net> writes:
>
>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>>>> Cc: 74556@debbugs.gnu.org
>>>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>>>>>> 
>>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>>> 
>>>>>> [...]
>>>>>> 
>>>>>> > It might make sense to try and "deactivate" a package before installing
>>>>>> > the new package.  Looking into some second-try fallback for
>>>>>> > package-install to refresh the package index if a package was not found
>>>>>> > would also be a good idea ^^
>>>>>> 
>>>>>> This might do it?
>>>>>
>>>>> Philip, please install this on the emacs-30 branch, unless you see any
>>>>> problems with the change.  We'd like to make another pretest soon.
>>>>
>>>> Done.  But we should keep the report open as there might be better
>>>> approaches to discuss in the future.
>>>
>>> Hello Philip,
>>>
>>> I just tried the modified `package-upgrade' function and it doesn't seem
>>> to work. It seems to break the upgrade procedure in an even worse way,
>>> at least in my setup. Now `package-install' is tried first with the
>>> package symbol, which will be a no-op, since the package is already
>>> installed. Afterwards the package is deleted and we always end up with
>>> no package. Probably `package-install' should also be called with a
>>> package descriptor of the new package version?
>>
>> Right, my sincere apologies for that oversight.  That being said, I
>> don't feel comfortable fixing this right now as I am short on time to
>> fix and test something like this on the "emacs-30" branch.  My vote
>> would be to revert the commit and try to tackle the issue on the
>> "master" branch.  An alternative I can propose that would be closer to
>> the original code might be
>
> Yes, I also vote to revert your commit on the emacs-30 release branch.
> The issue isn't severe (and not a regression), so I'd say it is okay to
> fix the issue on the master branch.

Eli, what do you say?

> I don't understand how the code you proposed works. It seems that after
> the deletion, if the installation fails, the package will stay deleted?
> Anyway, no hurry from my side to get this fixed.

Forget it, I didn't think it through.

> Thanks.
>
> Daniel

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  74556@debbugs.gnu.org
> Date: Tue, 10 Dec 2024 20:31:59 +0000
> 
> Daniel Mendler <mail@daniel-mendler.de> > Philip Kaludercic <philipk@posteo.net> writes:
> >
> >> Daniel Mendler <mail@daniel-mendler.de> writes:
> >>
> >>> Philip Kaludercic <philipk@posteo.net> writes:
> >>>
> >>>> Eli Zaretskii <eliz@gnu.org> writes:
> >>>>
> >>>>>> Cc: 74556@debbugs.gnu.org
> >>>>>> From: Philip Kaludercic <philipk@posteo.net>
> >>>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
> >>>>>> 
> >>>>>> Philip Kaludercic <philipk@posteo.net> writes:
> >>>>>> 
> >>>>>> [...]
> >>>>>> 
> >>>>>> > It might make sense to try and "deactivate" a package before installing
> >>>>>> > the new package.  Looking into some second-try fallback for
> >>>>>> > package-install to refresh the package index if a package was not found
> >>>>>> > would also be a good idea ^^
> >>>>>> 
> >>>>>> This might do it?
> >>>>>
> >>>>> Philip, please install this on the emacs-30 branch, unless you see any
> >>>>> problems with the change.  We'd like to make another pretest soon.
> >>>>
> >>>> Done.  But we should keep the report open as there might be better
> >>>> approaches to discuss in the future.
> >>>
> >>> Hello Philip,
> >>>
> >>> I just tried the modified `package-upgrade' function and it doesn't seem
> >>> to work. It seems to break the upgrade procedure in an even worse way,
> >>> at least in my setup. Now `package-install' is tried first with the
> >>> package symbol, which will be a no-op, since the package is already
> >>> installed. Afterwards the package is deleted and we always end up with
> >>> no package. Probably `package-install' should also be called with a
> >>> package descriptor of the new package version?
> >>
> >> Right, my sincere apologies for that oversight.  That being said, I
> >> don't feel comfortable fixing this right now as I am short on time to
> >> fix and test something like this on the "emacs-30" branch.  My vote
> >> would be to revert the commit and try to tackle the issue on the
> >> "master" branch.  An alternative I can propose that would be closer to
> >> the original code might be
> >
> > Yes, I also vote to revert your commit on the emacs-30 release branch.
> > The issue isn't severe (and not a regression), so I'd say it is okay to
> > fix the issue on the master branch.
> 
> Eli, what do you say?

It looks like you are in agreement, so please revert on emacs-30.
(Unless you also want to revert on master, don't forget to say "do not
merge" in the log message.)

Thanks.

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  74556@debbugs.gnu.org
>> Date: Tue, 10 Dec 2024 20:31:59 +0000
>> 
>> Daniel Mendler <mail@daniel-mendler.de> > Philip Kaludercic <philipk@posteo.net> writes:
>> >
>> >> Daniel Mendler <mail@daniel-mendler.de> writes:
>> >>
>> >>> Philip Kaludercic <philipk@posteo.net> writes:
>> >>>
>> >>>> Eli Zaretskii <eliz@gnu.org> writes:
>> >>>>
>> >>>>>> Cc: 74556@debbugs.gnu.org
>> >>>>>> From: Philip Kaludercic <philipk@posteo.net>
>> >>>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>> >>>>>> 
>> >>>>>> Philip Kaludercic <philipk@posteo.net> writes:
>> >>>>>> 
>> >>>>>> [...]
>> >>>>>> 
>> >>>>>> > It might make sense to try and "deactivate" a package before installing
>> >>>>>> > the new package.  Looking into some second-try fallback for
>> >>>>>> > package-install to refresh the package index if a package was not found
>> >>>>>> > would also be a good idea ^^
>> >>>>>> 
>> >>>>>> This might do it?
>> >>>>>
>> >>>>> Philip, please install this on the emacs-30 branch, unless you see any
>> >>>>> problems with the change.  We'd like to make another pretest soon.
>> >>>>
>> >>>> Done.  But we should keep the report open as there might be better
>> >>>> approaches to discuss in the future.
>> >>>
>> >>> Hello Philip,
>> >>>
>> >>> I just tried the modified `package-upgrade' function and it doesn't seem
>> >>> to work. It seems to break the upgrade procedure in an even worse way,
>> >>> at least in my setup. Now `package-install' is tried first with the
>> >>> package symbol, which will be a no-op, since the package is already
>> >>> installed. Afterwards the package is deleted and we always end up with
>> >>> no package. Probably `package-install' should also be called with a
>> >>> package descriptor of the new package version?
>> >>
>> >> Right, my sincere apologies for that oversight.  That being said, I
>> >> don't feel comfortable fixing this right now as I am short on time to
>> >> fix and test something like this on the "emacs-30" branch.  My vote
>> >> would be to revert the commit and try to tackle the issue on the
>> >> "master" branch.  An alternative I can propose that would be closer to
>> >> the original code might be
>> >
>> > Yes, I also vote to revert your commit on the emacs-30 release branch.
>> > The issue isn't severe (and not a regression), so I'd say it is okay to
>> > fix the issue on the master branch.
>> 
>> Eli, what do you say?
>
> It looks like you are in agreement, so please revert on emacs-30.

Done

> (Unless you also want to revert on master, don't forget to say "do not
> merge" in the log message.)

No, it doesn't make sense to keep the current change on master either.

> Thanks.