Fix updater self-termination during deb upgrades - #1430
Conversation
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for isolating the Debian updater lifecycle issue and documenting the reproduced failure. Two blockers remain:
- Skipping service shutdown during
prerm upgradeleaves the old daemon running after/usr/bin/codex-update-manageris replaced. The current updater has no replacement detection,postinstdoes not restart an active unit, and the next rebuild receives acurrent_exe()path ending in(deleted). Please preserve the in-flight package transaction, then safely exit/restart the daemon only after the install result andInstalledstate have been persisted. - The added smoke assertions only check for strings and still pass if the actual
exit 0guard is removed. Please add behavioral coverage for the stagedDEBIAN/prerm: upgrade must avoid service side effects, removal/deconfiguration must retain cleanup, and a successful self-upgrade must restart onto the new binary and allow the next rebuild.
Please run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until that model reports no remaining blockers before requesting maintainer re-review.
The Debian prerm script unconditionally stopped codex-update-manager.service, including when invoked for an upgrade. When an automatic update was launched by codex-update-manager itself, the service spawned pkexec -> apt -> dpkg and dpkg then ran the package prerm. Stopping the originating user service from that prerm interrupted its own privileged installation. That left root-owned apt/dpkg processes in the user-service cgroup, produced repeated "Operation not permitted" cleanup failures, and caused the updater to restart and rebuild the same candidate again. Skip user-service shutdown for the Debian upgrade prerm action. Removal and deconfiguration paths retain the existing stop/disable behavior. The running daemon can then finish the package transaction, detect that its on-disk updater binary was replaced, and let systemd restart the updated daemon normally. Add smoke-test assertions covering the upgrade guard. Observed before the fix: - successful polkit authentication followed by updater service termination - orphaned apt/dpkg processes remaining in the user-service cgroup - repeated rebuild/password cycles - multi-GB memory and CPU use on every retry Validated after the fix: - shell syntax checks pass - tests/scripts_smoke.sh passes all 57 tests - git diff --check passes - a full automatic signed upstream update completed end-to-end - updater reached status installed - no orphaned apt/dpkg processes or self-termination loop remained
Preserve Debian self-upgrade transactions by leaving the updater service running from prerm during package upgrades, while retaining normal cleanup for removal and deconfiguration. Detect executable replacement by device/inode. After a successful managed install, persist and read back Installed state before exiting with the Restart=on-failure code, allowing systemd to relaunch the new binary. Harden subsequent rebuilds against /proc/self/exe paths carrying the kernel's " (deleted)" suffix. Exercise the staged Debian maintainer script behavior and a real old-inode/new-binary replacement lifecycle. This restores the replacement-restart lifecycle originally introduced in 28f03cf and lost during the signed-package refactor 47f6d69; the changelog continued to describe this behavior.
|
Addressed the updater self-termination and stale-binary lifecycle issues from the review. The Debian The updater now detects executable replacement by device/inode rather than content. This handles the normal The builder path is also hardened so a subsequent rebuild does not use a stale Behavioral coverage added for the requested cases:
I also mutation-tested the Debian regression harness by removing the I ran repeated adversarial review passes over the updated diff and fixed the issues they exposed, including production-script staging in the test path, healthy-current-vs-installed binary resolution, exercising the real restart exit path, non-root CI skip behavior, and test-shell quoting. Validation on the rebased branch:
The branch is rebased onto current |
f6e0dbe to
64ed90a
Compare
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for the thorough lifecycle fix. The original stale-daemon path and the staged prerm behavior are substantially improved, but three blockers remain:
passive_replacement_disposition()blocks forever whenever the persisted status isInstalling. If a separateinstall-readyor rollback process replaces the updater and then dies before savingInstalled, the still-running service daemon will see the replacement every 15 seconds but will neither recover the interrupted transaction nor restart onto the installed binary. Please add bounded, ownership-aware recovery (for example, a lock/lease/liveness signal) so an active installer is protected while an abandonedInstallingstate can converge safely.tests/deb-prerm.test.jschooses/run/user/${40000 + pid % 20000}without proving that the directory was created by the test. Both privileged branches accept a pre-existing path; the sudo branch chmods it to 0777, and cleanup recursively removes the whole directory. A UID collision can therefore modify or delete another user's active runtime directory. Please reserve a guaranteed-new numeric directory atomically, track/validate that it belongs to this fixture, and clean up only what the test created.successful_self_replacement_restarts_on_new_binary_and_next_build_uses_itdoes not exercise the production install lifecycle: the child callsexit_for_replacement()directly on a marker and the parent manually starts the replacement. It never runsinstall_ready(), observes the package result, or proves the requiredsave Installed -> read back -> exitordering. Moving or deleting the productionrestart_after_persisted_install()call would leave this test green. Please add behavioral coverage through the production lifecycle so that exact ordering regression is caught.
Please run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until that model reports no remaining blockers before requesting maintainer re-review.
Make updater replacement and abandoned-install recovery fail closed and ownership-aware. - persist PID/start-time transaction ownership before privileged package mutation - gate pkexec launch until package-command ownership is durably saved - reconcile abandoned Debian installs only from fully configured package state - reject ambiguous same-version and unknown pre-install recovery evidence - preserve pre-transaction installed version for recovery and rollback bookkeeping - exercise the production install/restart/readback lifecycle in replacement tests - harden Debian prerm runtime-dir fixtures with verified fixture ownership - make executable replacement fixtures deterministic under parallel test execution
Avoid publishing the long-lived updater daemon as the owner of a package transaction before the privileged command can start. Persist the exact gated child identity before releasing package mutation, distinguish failures that are provably pre-mutation from ambiguous outcomes, and clear failed pre-launch transactions so a running daemon cannot remain stuck in Installing. Also make abandoned-install grace fail toward recovery when the persisted wall-clock start time is in the future, preserving the bounded recovery guarantee across clock rollback, with regression coverage for both cases.
|
Since the last review, I reworked the updater replacement/recovery path around the three issues you called out and then did another adversarial pass over the resulting state machine. The main changes are:
I also added regressions for the new ownership/recovery cases, the production replacement lifecycle, post-release install/rollback failures, and the Debian |
Preserve durable install recovery evidence when a gated package command exits unsuccessfully after mutation may have started, including rollback. Keep the updater service alive through Debian failed-upgrade unwind paths and add regressions for both cases.
Summary
Prevent the Debian package
prermhook from stoppingcodex-update-manager.servicewhen the package is being upgraded.The updater can launch its own package upgrade through:
codex-update-manager -> pkexec -> apt -> dpkgDuring that transaction,
dpkginvokes the packageprerm. The existing script unconditionally stopscodex-update-manager.service, including for the Debianupgradeaction. That causes the service to terminate the privileged installation it launched itself.Observed failure
On Linux Mint, a normal automatic update repeatedly produced this sequence:
apt/dpkgbegan installing the rebuiltcodex-desktoppackage.prermstoppedcodex-update-manager.service.apt/dpkgdescendants remained behind.Operation not permittedwhile trying to kill the cgroup.The loop also repeatedly consumed several GB of RAM and significant CPU while rebuilding the package.
Fix
Return immediately from
codex-update-manager.prermfor the Debianupgradeaction.Removal/deconfiguration behavior is unchanged, so actual package removal still stops/disables the user service.
This lets the running updater finish the package transaction. Its existing replacement detection can then observe that its on-disk binary changed and allow systemd to restart the updated daemon normally.
Validation
Verified locally with:
tests/scripts_smoke.sh— 57/57 tests passinggit diff --checkThe automatic update completed successfully to upstream
26.901.41123, the updater reachedinstalled, and there were no orphanedapt/dpkgprocesses or further self-termination/rebuild loop.A smoke-test assertion was added so the Debian upgrade guard cannot be removed accidentally.