service: fix caveat for SSH sessions - #23464
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes service-status detection on macOS when Homebrew is run over SSH, so Utils::Service.running? can correctly detect services loaded in gui/<uid> or user/<uid> domains and produce the right caveats (e.g., suggesting brew services restart after upgrades).
Changes:
- Update
Utils::Service.running?to uselaunchctl print <domain>/<label>for gui/user domains, with a fallback tolaunchctl list <label>for compatibility. - Add unit tests covering the
running?decision paths for “no manager”,systemctl, andlaunchctlbehaviour.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/utils/service.rb | Switches macOS service detection to domain-aware launchctl print, with compatibility fallback. |
| Library/Homebrew/test/utils/service_spec.rb | Adds specs for Utils::Service.running? across manager availability and launchctl domain probing. |
Suppressed comments (1)
Library/Homebrew/test/utils/service_spec.rb:56
- This spec also interpolates the real
Process.uid/Process.euidinto its expectations. If the suite is ever executed as root (Process.euid.zero?),Utils::Service.running?won’t try the gui/user domains and this example will fail. StubbingProcess.uid/Process.euidmakes the behaviour under test deterministic.
it "stops trying after first successful domain" do
f = formula do
T.bind(self, T.class_of(Formula))
url "foo-1.0"
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
59adf5b to
2a8c5e5
Compare
97e5c1b to
09097ab
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks. brew tests failing.
| if (launchctl = self.launchctl) | ||
| label = formula.plist_name | ||
|
|
||
| # When connected via SSH, `launchctl list <label>` only searches | ||
| # the system domain, so it won't find services loaded in the GUI | ||
| # domain. Use `launchctl print <domain>/<label>` which works | ||
| # across sessions, falling back to the bare label for backward | ||
| # compatibility. | ||
| candidate_domains = Homebrew::Services::System.candidate_domain_targets | ||
|
|
||
| candidate_domains.any? { |domain| quiet_system(launchctl, "print", "#{domain}/#{label}") } || | ||
| quiet_system(launchctl, "list", label) |
There was a problem hiding this comment.
How does this differ/overlap with status_output_success_type in formula_wrapper.rb or find_launchctl_with_wildcard/uninstall_launchctl in abstract_uninstall.rb or running in services/cli.rb?
It looks like they are all doing pretty similar things but in different ways that will have different results. Feels like we need a single source of truth for "find this service" or "is this service running" across all locations or it'll just be whack-a-mole with behaviour differing depending on where it's run.
There was a problem hiding this comment.
The use cases seem slightly different across the call sites: checking whether a service running, extracting the status from the output, listing services. I've consolidated a bit more, ptal.
8500bc0 to
a987d7f
Compare
When connected via SSH, `launchctl list <label>` only searches the system domain, so it won't find services loaded in the `gui/<uid>` domain. This caused `Utils::Service.running?` to incorrectly return false for running services, which in turn made `brew upgrade` print "brew services start" instead of the correct "brew services restart". Consolidate the "is this service running?" logic into a single source of truth: `Homebrew::Services::System.launchctl_service_running?`. This iterates `candidate_domain_targets` to try domain-qualified `launchctl print <domain>/<label>` before falling back to a bare `launchctl list <label>`. Both `Utils::Service.running?` and `FormulaWrapper#loaded?` now use this method. Refactor `FormulaWrapper#status_output_success_type` to also iterate `candidate_domain_targets` instead of only trying the single `domain_target`, which over SSH returns `user/<euid>` and misses services loaded in the `gui/<uid>` domain.
a987d7f to
cd98005
Compare
When connected via SSH,
launchctl list <label>only searches the system domain, so it won't find services loaded in thegui/<uid>domain. This causedUtils::Service.running?to incorrectly return false for running services, which in turn madebrew upgradeprint "brew services start" instead of the correct "brew services restart".Fix by using
launchctl print <domain>/<label>(which works across sessions) for the gui and user domains, falling back to the barelaunchctl list <label>for backward compatibility. This mirrors howFormulaWrapper#status_output_success_typealready checks service status forbrew services.Output for a running service over SSH before this PR:
Output with this PR:
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Claude Opus 5 was used to find the discrepancy between
brew infoandbrew servicesand create the patch.