Skip to content

cask/uninstall: skip signalling other users' processes - #24082

Merged
MikeMcQuaid merged 3 commits into
Homebrew:mainfrom
hyuraku:cask-signal-pid-owner
Sep 25, 2026
Merged

MikeMcQuaid merged 3 commits into
Homebrew:mainfrom
hyuraku:cask-signal-pid-owner

Conversation

@hyuraku

@hyuraku hyuraku commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

uninstall signal: sends the signal to all PIDs for the bundle ID in one
Process.kill call. If another user's PID comes first, Errno::EPERM
stops the current user's app from being signalled while its files are
still removed. A TODO asked to check the owner of each PID.

Now only PIDs whose real UID matches Process.uid are signalled, and
other users' PIDs or PIDs with an unknown owner are skipped with a
warning. I used the real UID and not the effective UID because the kernel
checks the target's real (or saved) UID when it decides if a signal is
allowed.

The UID comes from proc_bsdshortinfo via a new LibProc.real_uid, which
LibProc.parent_pid already reads, so no extra process is spawned.
Behaviour is unchanged when all PIDs are the current user's. I added tests
for other users' PIDs, for an unknown owner, and for LibProc.real_uid.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI (Claude Code) was used to review the existing diff, and draft this PR description.
I reviewed its output and verified the changes with the targeted spec and brew lgtm.

Copilot AI lite review requested due to automatic review settings September 24, 2026 12:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

馃煝 Approval recommended

The ownership filtering and tests are covered; the remaining warning-wording issue is a minor nit.

Review effort: Lite
Findings: 1 Low severity

Open (1)
What changed in this PR

Updates macOS cask uninstallation to signal only processes owned by the current user, while skipping unknown or foreign owners.

Changes:

  • Adds real UID lookup via LibProc.real_uid.
  • Filters process signalling by real UID.
  • Adds tests for UID lookup and filtering behavior.
File Summary
Library/鈥婬omebrew/鈥媡est/鈥媜s/鈥媘ac/鈥媐fi/鈥媗ibproc_spec.rb Tests real UID lookup.
Library/鈥婬omebrew/鈥媡est/鈥媍ask/鈥媋rtifact/鈥媢ninstall_spec.rb Tests process filtering.
Library/鈥婬omebrew/鈥媡est/鈥媍ask/鈥媋rtifact/鈥媠hared_examples/鈥媢ninstall_zap.rb Updates signal test setup.
Library/鈥婬omebrew/鈥媜s/鈥媘ac/鈥媐fi/鈥媗ibproc.rb Adds real UID process inspection.
Library/鈥婬omebrew/鈥媏xtend/鈥媜s/鈥媘ac/鈥媍ask/鈥媋rtifact/鈥媋bstract_uninstall.rb Exposes macOS owner lookup.
Library/鈥婬omebrew/鈥媍ask/鈥媋rtifact/鈥媋bstract_uninstall.rb Filters signalling targets by UID.

馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
Comment thread Library/Homebrew/cask/artifact/abstract_uninstall.rb Outdated
@MikeMcQuaid

Copy link
Copy Markdown
Member

Thanks @hyuraku!

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Sep 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 25, 2026
@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Sep 25, 2026
Merged via the queue into Homebrew:main with commit 86650d0 Sep 25, 2026
51 checks passed
@hyuraku
hyuraku deleted the cask-signal-pid-owner branch September 26, 2026 07:32
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.

3 participants