cask uninstall: opt-in quit/signal on upgrade/reinstall

Library/Homebrew/cask/artifact/abstract_uninstall.rb

@@ -29,14 +29,20 @@ class AbstractUninstall < AbstractArtifact
         :rmdir,
       ].freeze
 
+      # Opt-ins to run certain directives during upgrade/reinstall.
+      UPGRADE_METADATA_KEYS = [
+        :quit_on_upgrade,
+        :signal_on_upgrade,
+      ].freeze
+
       def self.from_args(cask, **directives)
         new(cask, **directives)
       end
 
       attr_reader :directives
 
       def initialize(cask, **directives)
-        directives.assert_valid_keys(*ORDERED_DIRECTIVES)
+        directives.assert_valid_keys(*ORDERED_DIRECTIVES, *UPGRADE_METADATA_KEYS)
 
         super
         directives[:signal] = Array(directives[:signal]).flatten.each_slice(2).to_a

Library/Homebrew/cask/artifact/uninstall.rb

@@ -13,7 +13,16 @@ def uninstall_phase(upgrade: false, reinstall: false, **options)
         filtered_directives = ORDERED_DIRECTIVES.filter do |directive_sym|
           next false if directive_sym == :rmdir
 
-          next false if (upgrade || reinstall) && UPGRADE_REINSTALL_SKIP_DIRECTIVES.include?(directive_sym)
+          if (upgrade || reinstall) && UPGRADE_REINSTALL_SKIP_DIRECTIVES.include?(directive_sym)
+            case directive_sym
+            when :quit
+              next false unless directives.fetch(:quit_on_upgrade, false)
+            when :signal
+              next false unless directives.fetch(:signal_on_upgrade, false)
+            else
+              next false
+            end
+          end
 
           true
         end

Library/Homebrew/rubocops/cask/uninstall_methods_order.rb

@@ -13,6 +13,13 @@ class UninstallMethodsOrder < Base
 
         MSG = T.let("`%<method>s` method out of order", String)
 
+        # These keys are ignored when checking method order.
+        # Mirrors AbstractUninstall::UPGRADE_METADATA_KEYS.
+        UPGRADE_METADATA_KEYS = T.let(
+          [:quit_on_upgrade, :signal_on_upgrade].freeze,
+          T::Array[Symbol],
+        )
+
         sig { params(node: AST::SendNode).void }
         def on_send(node)
           return unless [:zap, :uninstall].include?(node.method_name)
@@ -21,6 +28,12 @@ def on_send(node)
           return if hash_node.nil? || (!hash_node.is_a?(AST::Node) && !hash_node.hash_type?)
 
           method_nodes = hash_node.pairs.map(&:key)
+
+          method_nodes = method_nodes.reject do |method|
+            name = method.children.first
+            UPGRADE_METADATA_KEYS.include?(name)
+          end
+
           expected_order = method_nodes.sort_by { |method| method_order_index(method) }
           comments = processed_source.comments
 

Library/Homebrew/test/cask/artifact/uninstall_spec.rb

@@ -4,7 +4,119 @@
 
 RSpec.describe Cask::Artifact::Uninstall, :cask do
   describe "#uninstall_phase" do
+    let(:fake_system_command) { NeverSudoSystemCommand }
+
     include_examples "#uninstall_phase or #zap_phase"
+
+    describe "upgrade/reinstall opt-in uninstall directives" do
+      context "with-uninstall-quit" do
+        let(:cask) { Cask::CaskLoader.load(cask_path("with-uninstall-quit")) }
+        let(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+        it "skips :quit by default during upgrade" do
+          quit_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            quit_called ||= directive == :quit && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(upgrade: true, command: fake_system_command)
+
+          expect(quit_called).to be false
+        end
+
+        it "skips :quit by default during reinstall" do
+          quit_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            quit_called ||= directive == :quit && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(reinstall: true, command: fake_system_command)
+
+          expect(quit_called).to be false
+        end
+      end
+
+      context "with-uninstall-quit-on-upgrade" do
+        let(:cask) { Cask::CaskLoader.load(cask_path("with-uninstall-quit-on-upgrade")) }
+        let(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+        it "invokes :quit during upgrade" do
+          quit_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            quit_called ||= directive == :quit && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(upgrade: true, command: fake_system_command)
+
+          expect(quit_called).to be true
+        end
+
+        it "invokes :quit during reinstall" do
+          quit_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            quit_called ||= directive == :quit && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(reinstall: true, command: fake_system_command)
+
+          expect(quit_called).to be true
+        end
+      end
+
+      context "with-uninstall-signal" do
+        let(:cask) { Cask::CaskLoader.load(cask_path("with-uninstall-signal")) }
+        let(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+        it "skips :signal by default during upgrade" do
+          signal_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            signal_called ||= directive == :signal && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(upgrade: true, command: fake_system_command)
+
+          expect(signal_called).to be false
+        end
+
+        it "skips :signal by default during reinstall" do
+          signal_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            signal_called ||= directive == :signal && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(reinstall: true, command: fake_system_command)
+
+          expect(signal_called).to be false
+        end
+      end
+
+      context "with-uninstall-signal-on-upgrade" do
+        let(:cask) { Cask::CaskLoader.load(cask_path("with-uninstall-signal-on-upgrade")) }
+        let(:artifact) { cask.artifacts.find { |a| a.is_a?(described_class) } }
+
+        it "invokes :signal during upgrade" do
+          signal_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            signal_called ||= directive == :signal && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(upgrade: true, command: fake_system_command)
+
+          expect(signal_called).to be true
+        end
+
+        it "invokes :signal during reinstall" do
+          signal_called = false
+          allow(artifact).to receive(:dispatch_uninstall_directive) do |directive, **options|
+            signal_called ||= directive == :signal && options[:command] == fake_system_command
+          end
+
+          artifact.uninstall_phase(reinstall: true, command: fake_system_command)
+
+          expect(signal_called).to be true
+        end
+      end
+    end
   end
 
   describe "#post_uninstall_phase" do

Library/Homebrew/test/rubocops/cask/uninstall_methods_order_spec.rb

@@ -78,6 +78,28 @@
           end
         CASK
       end
+
+      it "ignores opt-in boolean keys such as quit_on_upgrade and signal_on_upgrade" do
+        expect_no_offenses(<<~CASK)
+          cask "foo" do
+            url "https://example.com/foo.zip"
+
+            uninstall quit:      "com.example.foo",
+                      quit_on_upgrade: true,
+                      signal:    ["TERM", "com.example.foo"],
+                      signal_on_upgrade: true,
+                      script:  {
+                        executable: "/usr/local/bin/foo",
+                        sudo:       false,
+                      },
+                      pkgutil: "org.foo.bar",
+                      delete:  [
+                        "/usr/local/bin/foo",
+                        "/usr/local/bin/foobar",
+                      ]
+          end
+        CASK
+      end
     end
 
     context "with a single method" do

Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-quit-on-upgrade.rb

@@ -0,0 +1,11 @@
+cask "with-uninstall-quit-on-upgrade" do
+  version "1.2.3"
+  sha256 "8c62a2b791cf5f0da6066a0a4b6e85f62949cd60975da062df44adf887f4370b"
+
+  url "file://#{TEST_FIXTURE_DIR}/cask/MyFancyPkg.zip"
+  homepage "https://brew.sh/fancy-pkg"
+
+  pkg "MyFancyPkg/Fancy.pkg"
+
+  uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
+end

Library/Homebrew/test/support/fixtures/cask/Casks/with-uninstall-signal-on-upgrade.rb

@@ -0,0 +1,14 @@
+cask "with-uninstall-signal-on-upgrade" do
+  version "1.2.3"
+  sha256 "8c62a2b791cf5f0da6066a0a4b6e85f62949cd60975da062df44adf887f4370b"
+
+  url "file://#{TEST_FIXTURE_DIR}/cask/MyFancyPkg.zip"
+  homepage "https://brew.sh/fancy-pkg"
+
+  pkg "MyFancyPkg/Fancy.pkg"
+
+  uninstall signal: [
+    ["TERM", "my.fancy.package.app"],
+    ["KILL", "my.fancy.package.app"],
+  ], signal_on_upgrade: true
+end

docs/Cask-Cookbook.md

@@ -794,8 +794,8 @@ The easiest and most useful `uninstall` directive is [`pkgutil:`](#uninstall-pkg
 
 * **`early_script:`** (string or hash) - like [`script:`](#uninstall-script), but runs early (for special cases, best avoided)
 * [`launchctl:`](#uninstall-launchctl) (string or array) - IDs of `launchd` jobs to remove
-* [`quit:`](#uninstall-quit) (string or array) - bundle IDs of running applications to quit (does not run when uninstall is initiated by `brew upgrade` or `brew reinstall`)
-* [`signal:`](#uninstall-signal) (array of arrays) - signal numbers and bundle IDs of running applications to send a Unix signal to, for when `quit:` does not work (does not run when uninstall is initiated by `brew upgrade` or `brew reinstall`)
+* [`quit:`](#uninstall-quit) (string or array) - bundle IDs of running applications to quit (does not run when uninstall is initiated by `brew upgrade` or `brew reinstall` unless `quit_on_upgrade: true` is present)
+* [`signal:`](#uninstall-signal) (array of arrays) - signal numbers and bundle IDs of running applications to send a Unix signal to, for when `quit:` does not work (does not run when uninstall is initiated by `brew upgrade` or `brew reinstall` unless `signal_on_upgrade: true` is present)
 * [`login_item:`](#uninstall-login_item) (string or array) - names of login items to remove
 * [`kext:`](#uninstall-kext) (string or array) - bundle IDs of kexts to unload from the system
 * [`script:`](#uninstall-script) (string or hash) - relative path to an uninstall script to be run via *sudo*; use hash if args are needed
@@ -852,6 +852,14 @@ IDs for all installed `launchd` jobs can be listed using [`list_installed_launch
 
 #### `uninstall` *quit*
 
+Some applications need to be quit (or explicitly signaled) in order to be safely upgraded in-place. By default, Homebrew does not run `:quit` or `:signal` directives when an uninstall is being performed as part of a `brew upgrade` or `brew reinstall` operation. This avoids unexpectedly terminating user processes during automated upgrades and is designed to avoid data loss; `uninstall quit:` is equivalent to a normal macOS quit and can still discard unsaved in-memory state such as web forms or editor buffers.
+
+Use `quit_on_upgrade: true` only when the application genuinely needs to be closed to upgrade or reinstall correctly and you have verified that it handles a normal macOS quit without losing important user data. In that case, you may opt a cask into running this directive during an upgrade or reinstall by adding `quit_on_upgrade: true` to the `uninstall` stanza:
+
+```ruby
+uninstall quit: "com.example.app", quit_on_upgrade: true
+```
+
 Bundle IDs for currently running applications can be listed using [`list_running_app_ids`](https://github.com/Homebrew/homebrew-cask/blob/HEAD/developer/bin/list_running_app_ids):
 
 ```bash
@@ -892,6 +900,12 @@ Note that when multiple running processes match the given bundle ID, all matchin
 
 Unlike `quit:` directives, Unix signals originate from the current user, not from the superuser. This is construed as a safety feature, since the superuser is capable of bringing down the system via signals. However, this inconsistency could also be considered a bug, and may be addressed in some fashion in a future version.
 
+To opt a cask into running this directive during an upgrade or reinstall, add `signal_on_upgrade: true` to the `uninstall` stanza:
+
+```ruby
+uninstall signal: [["TERM", "com.example.daemon"]], signal_on_upgrade: true
+```
+
 #### `uninstall` *login_item*
 
 Login items associated with an application bundle on disk can be listed using [`list_login_items_for_app`](https://github.com/Homebrew/homebrew-cask/blob/HEAD/developer/bin/list_login_items_for_app):