feat(forcekill): Support killing running applications and services#6603
Open
MetalDevOps wants to merge 14 commits intoScoopInstaller:developfrom
Open
feat(forcekill): Support killing running applications and services#6603MetalDevOps wants to merge 14 commits intoScoopInstaller:developfrom
MetalDevOps wants to merge 14 commits intoScoopInstaller:developfrom
Conversation
…fore updates or uninstalls
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces a boolean running-process check with Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "scoop-forcekill / scoop-unforcekill"
participant InstallInfo as "install-info JSON"
participant Updater as "scoop-update / CLI ops"
participant Checker as "check_running_process"
participant Stopper as "stop_running_process"
participant Services as "Windows Services"
participant Processes as "System Processes"
User->>CLI: scoop forcekill <app> [--service list]
CLI->>InstallInfo: read/update install-info (set forcekill, forcekill_services)
CLI-->>User: confirm
User->>Updater: scoop update/uninstall/reset <app>
Updater->>Checker: check_running_process(app, scope)
Checker->>InstallInfo: read forcekill / NO_JUNCTION / install-info
alt forcekill requested
Checker-->>Updater: result {Blocked=false, Forcekill=true, ServicesToStop, RunningProcesses, ProcessDir}
Updater->>Stopper: stop_running_process(result)
Stopper->>Services: stop ServicesToStop
Stopper->>Processes: forcekill RunningProcesses
Stopper-->>Updater: {ServicesToRestart, ProcessesToRestart}
Updater->>Updater: perform update/uninstall/reset
Updater->>Services: restart ServicesToRestart
Updater->>Processes: restart ProcessesToRestart
Updater-->>User: operation complete
else blocked or not forcing
Checker-->>Updater: Blocked=true or no action
Updater-->>User: skipped or proceed accordingly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
lib/install.ps1 (2)
534-548:test_running_processnow has significant side effects despite its "test_" naming.The function name suggests a read-only check, but it now stops services (line 568) and kills processes (line 580). Consider renaming to something like
check_and_stop_running_processor splitting the concern: one function to test, another to stop. This naming mismatch will surprise future maintainers.Additionally, the function now returns three different types (
$true,$false, or a[hashtable]), making the contract harder to reason about. The caller inscoop-update.ps1already has to branch on-eq $truevs-is [hashtable]. A single return type (e.g., always a hashtable with aBlockedflag) would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` around lines 534 - 548, The function test_running_process currently performs side effects (stopping services and killing processes) and returns mixed types; split responsibilities and normalize the return contract: create a pure tester function (keep name test_running_process or rename to check_running_process) that only inspects install_info and returns a consistent hashtable (e.g., @{Blocked=$true/$false; Details=$json; ForcekillServices=$forcekill_services}) and move the stopping/killing logic into a new function (e.g., stop_running_processes or stop_and_kill_running_process) that accepts the hashtable returned by the tester and performs the side effects; update callers (like the code in scoop-update.ps1 that currently branches on -eq $true vs -is [hashtable]) to call the tester first, inspect the .Blocked flag and then call the new stopper when needed, ensuring all returns from the tester are a hashtable for predictable handling.
555-555:Get-CimInstance Win32_Servicescans all services — consider filtering earlier.This queries every service on the system, then filters in PowerShell. On machines with many services, this is unnecessarily expensive. You could use a WQL filter to narrow results:
Filter at the WMI layer
- $foundServices = Get-CimInstance Win32_Service | Where-Object { $_.PathName -and $_.PathName -like "*$processdir*" } + $escapedDir = $processdir -replace '\\', '\\\\' + $foundServices = Get-CimInstance -Query "SELECT Name, PathName FROM Win32_Service WHERE PathName LIKE '%$escapedDir%'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` at line 555, Replace the client-side filtering pipeline that uses Get-CimInstance Win32_Service | Where-Object { $_.PathName -and $_.PathName -like "*$processdir*" } with a WMI-side filter to avoid enumerating all services: construct a WQL filter string (e.g. $filter = "PathName LIKE '%$processdir%'" making sure to escape any single quotes/backslashes in $processdir if necessary) and call Get-CimInstance -ClassName Win32_Service -Filter $filter (or Get-CimInstance -Query "SELECT * FROM Win32_Service WHERE PathName LIKE '%$processdir%'") so the filtering happens in Get-CimInstance instead of in Where-Object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 576-589: In the $forcekill branch handle killed processes that
lack a Path by collecting their identifiers before Stop-Process so they aren't
lost: when iterating $running_processes, build $processesToRestart from those
with a Path (existing logic) and also build a separate $untrackableProcesses
list (e.g., process Id/Name) for those without Path, log a warning about them,
and include that list in the returned $result (add an UntrackableProcesses key)
or set a boolean like HadKilledProcesses to ensure you don't return $false when
any processes were force-killed; update references to $processesToRestart,
Stop-Process, and the final return to return the enriched $result instead of
$false when kills occurred.
- Around line 554-571: The Stop-Service and Stop-Process calls inside the
$forcekill block currently swallow errors (using -ErrorAction SilentlyContinue),
so failures to stop services/processes can go unnoticed and later cause
file-lock update failures; modify the logic around Get-CimInstance
Win32_Service, $servicesToStop, Stop-Service and Stop-Process to (1) check for
administrative privileges before attempting service stops and surface a warning
if not elevated, (2) attempt to stop each service/process but capture errors
instead of SilentlyContinue (or remove SilentlyContinue) and then verify the
status after the stop attempt, and (3) if a service/process remains running,
emit a warn with the service/process name, the observed status and the captured
error (if any) so the user knows which locks may block the update (use the
existing warn helper). Ensure $servicesToStop is still deduplicated and iterate
per-item so you can verify and report per-service/process results.
In `@libexec/scoop-forcekill.ps1`:
- Around line 64-67: Validate service names before persisting: when handling
$opt.service in the block that sets $install.forcekill_services,
split/trim/dedupe the names, call Get-Service for each candidate (e.g., via
Get-Service -Name <name> -ErrorAction SilentlyContinue) and only include names
that actually resolve; for names that do not exist, emit a warning or error
(processLogger or Write-Warning) and do not persist them to
$install.forcekill_services to avoid later silent failures in Stop-Service.
Ensure you reference the same variables ($opt.service and
$install.forcekill_services) and preserve existing behavior for valid service
names.
- Line 73: The script never assigns $exitcode so exit always returns 0;
initialize $exitcode (e.g., 0) near the top of scoop-forcekill.ps1 and update it
to a non‑zero value (e.g., 1) wherever an app failure is detected (the branches
that fail the `installed` check or the `install_info` check inside the main loop
that processes apps), ensuring any failure sets $exitcode=1 before the final
`exit $exitcode` so the script returns a failure code when appropriate.
- Line 23: The getopt invocation assigning $opt, $apps, $err currently registers
only the short option 'g' and the long options 'global' and 'service='; update
that call so the short-options string includes the service short flag with a
required value (i.e., change the short-options argument from 'g' to 'gs:') so
'-s' behaves like '--service' when parsing $args in the $opt, $apps, $err =
getopt $args 'g' 'global', 'service=' call.
In `@libexec/scoop-update.ps1`:
- Around line 396-411: stopped_processes holds exe paths from the old version
and Test-Path will fail after the update; update the restart logic in the update
flow so each $proc is rebased to the new install location (e.g., compute the
relative path from the old install dir captured by test_running_process, then
join that relative path onto the new/current junction for the app) before
calling Start-Process; modify test_running_process (or its caller) to return or
expose the old process base dir or the relative path portion so the update
routine can reconstruct the correct new path and then use Start-Process
-FilePath <rebasedPath> (with the same ErrorAction) instead of blindly using the
original $proc.
- Around line 297-310: The variable $app is being reassigned (e.g., to
"$bucket/$app" or $install.url) before the restart warning logs use it, which
corrupts the displayed app name; fix by capturing the original app name into a
separate variable (for example $originalAppName = $app) before any mutation and
then use $originalAppName in the restart/logging logic that references the app
(the Write-Host restart warnings and any places that inspect $stopped_services
or $stopped_processes for messages), leaving the mutated $app for install/URL
purposes.
- Around line 397-401: The loop that restarts services (variables
stopped_services and svc using Start-Service) currently swallows failures;
modify it to detect and report errors by invoking Start-Service with
-ErrorAction Stop inside a try/catch (or capture -ErrorVariable) and on failure
call warn with a clear message containing the service name ($svc), the
application ($app), and the error details so users know manual intervention is
required. Ensure the changed block still iterates stopped_services and only
converts silent failures into logged warnings that include the caught exception
message and/or error variable.
---
Nitpick comments:
In `@lib/install.ps1`:
- Around line 534-548: The function test_running_process currently performs side
effects (stopping services and killing processes) and returns mixed types; split
responsibilities and normalize the return contract: create a pure tester
function (keep name test_running_process or rename to check_running_process)
that only inspects install_info and returns a consistent hashtable (e.g.,
@{Blocked=$true/$false; Details=$json; ForcekillServices=$forcekill_services})
and move the stopping/killing logic into a new function (e.g.,
stop_running_processes or stop_and_kill_running_process) that accepts the
hashtable returned by the tester and performs the side effects; update callers
(like the code in scoop-update.ps1 that currently branches on -eq $true vs -is
[hashtable]) to call the tester first, inspect the .Blocked flag and then call
the new stopper when needed, ensuring all returns from the tester are a
hashtable for predictable handling.
- Line 555: Replace the client-side filtering pipeline that uses Get-CimInstance
Win32_Service | Where-Object { $_.PathName -and $_.PathName -like
"*$processdir*" } with a WMI-side filter to avoid enumerating all services:
construct a WQL filter string (e.g. $filter = "PathName LIKE '%$processdir%'"
making sure to escape any single quotes/backslashes in $processdir if necessary)
and call Get-CimInstance -ClassName Win32_Service -Filter $filter (or
Get-CimInstance -Query "SELECT * FROM Win32_Service WHERE PathName LIKE
'%$processdir%'") so the filtering happens in Get-CimInstance instead of in
Where-Object.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 5: The changelog entry uses the incorrect scope label "**core:**"; update
it to reflect the actual feature scope by using a command-specific label (e.g.,
"**scoop-forcekill:**" or "**scoop-update:**") consistent with existing entries
for new commands — reference the added scripts scoop-forcekill.ps1,
scoop-unforcekill.ps1, scoop-update.ps1 and the install changes in install.ps1
when choosing the correct label, and replace "**core:**" with that new label in
the CHANGELOG.md entry.
- Around line 5-6: In the CHANGELOG.md v0.5.3 section, the entry starting with
"**autoupdate:** GitHub predefined hashes support" is missing the leading "- "
bullet; edit the line containing the unique text "**autoupdate:** GitHub
predefined hashes support ([`#6416`](...), [`#6435`](...))" and prepend "- " so it
matches the other list entries (e.g., the "**core:** Support killing running
applications and services" entry) and keeps formatting consistent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libexec/scoop-forcekill.ps1 (1)
63-64: Prefer$json.PSObject.PropertiesoverGet-Member -MemberType Propertiesfor PSCustomObject copy.
-MemberType Propertiesis a compound wildcard that includesAliasProperty,ScriptProperty, andCodePropertyin addition toNoteProperty. For aPSCustomObjectdeserialized from JSON onlyNotePropertymembers exist, but the broader filter is an unnecessary footgun if the object is ever extended.$json.PSObject.Propertiesis the idiomatic, precise way to enumerate a PSCustomObject's own properties.♻️ Proposed refactor
- $install = @{} - $json | Get-Member -MemberType Properties | ForEach-Object { $install.Add($_.Name, $json.($_.Name)) } + $install = @{} + $json.PSObject.Properties | ForEach-Object { $install[$_.Name] = $_.Value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libexec/scoop-forcekill.ps1` around lines 63 - 64, The loop that builds $install uses "$json | Get-Member -MemberType Properties | ForEach-Object { $install.Add($_.Name, $json.($_.Name)) }", which uses a broad MemberType filter; change it to enumerate the PSCustomObject's own properties via $json.PSObject.Properties and add each property's Value to $install (e.g., iterate $json.PSObject.Properties and call $install.Add($_.Name, $_.Value)) so you only copy the object's actual properties (referencing $install and $json/PSObject.Properties and the existing ForEach-Object usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 5-6: Update the CHANGELOG entry to ensure the scope label reads
"**scoop-forcekill:**" (replacing the old "**core:**" label if present), make
sure the second line begins with the leading "- " bullet, and confirm the PR
reference for `#6603` uses the project convention with "/pull/" (i.e.,
https://github.com/ScoopInstaller/Scoop/pull/6603); apply the same formatting
rules to similar entries like "**autoupdate:**" to keep scope labels and
bulleting consistent.
---
Nitpick comments:
In `@libexec/scoop-forcekill.ps1`:
- Around line 63-64: The loop that builds $install uses "$json | Get-Member
-MemberType Properties | ForEach-Object { $install.Add($_.Name, $json.($_.Name))
}", which uses a broad MemberType filter; change it to enumerate the
PSCustomObject's own properties via $json.PSObject.Properties and add each
property's Value to $install (e.g., iterate $json.PSObject.Properties and call
$install.Add($_.Name, $_.Value)) so you only copy the object's actual properties
(referencing $install and $json/PSObject.Properties and the existing
ForEach-Object usage).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 647-649: The code currently pushes services into $stopped_services
when the service exists but isn't running ($status branch), causing previously
stopped services to be restarted; change the logic so only services that this
function actually stops are added to $stopped_services: in the loop handling
$servicesToStop, remove the elseif ($status) { $stopped_services += $svc }
branch and instead append $svc to $stopped_services only after a successful
Stop-Service call (or after you verify the service transitioned from Running to
Stopped), keeping the variables $servicesToStop, $stopped_services, $status and
ServicesToRestart as the reference points.
In `@libexec/scoop-update.ps1`:
- Around line 413-430: check_running_process currently sets ProcessDir using
"appdir $app $global | Convert-Path" which returns the app root and makes path
rebasing a no-op; update check_running_process to compute ProcessDir the same
way currentdir handles NO_JUNCTION (i.e., resolve the version-specific directory
instead of the app root), reusing the logic in currentdir or calling
currentdir/$currentdir-equivalent so that $old_processdir and $new_processdir
contain the full versioned paths and the
$proc.Replace($old_processdir,$new_processdir) actually changes the version
prefix; ensure this handles both NO_JUNCTION and junction modes consistently and
preserves existing behavior when versioned directories are absent.
---
Duplicate comments:
In `@libexec/scoop-update.ps1`:
- Around line 402-411: No change required: the service restart loop correctly
uses Start-Service with -ErrorAction Stop inside the try/catch and logs
failures; keep the current block that iterates $stopped_services, calls
Start-Service -Name $svc -ErrorAction Stop, and catches errors to warn "Failed
to restart service '$svc': $($_.Exception.Message)" for proper error handling
and logging.
added 2 commits
February 22, 2026 01:02
…o feature/forcekill # Conflicts: # CHANGELOG.md # libexec/scoop-reset.ps1
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 557-558: The WQL LIKE filter in the $filter construction uses
$wmi_dir but doesn't escape '_' (WQL single-character wildcard) and uses
non-standard single-quote escaping; fix the $wmi_dir escaping by adding a
replacement that turns every '_' into '[_]' and change the single-quote
replacement to double single-quotes (''), e.g. update the pipeline that builds
$wmi_dir (the -replace calls applied to $processdir used for $wmi_dir) so it
also does -replace '_', '[_]' and -replace "'", "''" (keep the existing
backslash escaping for '\' intact) before interpolating into $filter.
- Around line 600-634: When forcekill is requested and there are services to
stop ($forcekill and $servicesToStop), perform the is_admin check before killing
any $running_processes so you don't terminate app processes when you can't stop
services and must abort; if not elevated, warn and set $blocked = $true without
calling Stop-Process. Concretely, move or duplicate the admin check (is_admin)
up into the forcekill branch that enumerates $running_processes (the block using
Stop-Process and $still_running), or delay calling Stop-Process in that branch
until after is_admin succeeds, referencing $running_processes, $servicesToStop,
$forcekill, is_admin and Stop-Process to locate the code to change. Ensure
behavior for the IGNORE_RUNNING_PROCESSES path is unchanged.
In `@libexec/scoop-reset.ps1`:
- Around line 72-77: stop_running_process currently returns ServicesToRestart
and ProcessesToRestart but scoop-reset.ps1 ignores them, so services/processes
force-killed during reset are never restarted; update the reset flow (after
persist_permission) to capture the return value from stop_running_process (the
ServicesToRestart and ProcessesToRestart lists from
check_running_process/stop_running_process) and add a restart block mirroring
the logic in scoop-update.ps1 to restart any entries in ServicesToRestart and
relaunch items in ProcessesToRestart; reference check_running_process,
stop_running_process, ServicesToRestart, ProcessesToRestart and ensure the
restart occurs only when stop_running_process did not set Blocked.
---
Duplicate comments:
In `@lib/install.ps1`:
- Around line 604-612: The current logic builds $stopped_processes by filtering
$running_processes to those with a Path, so processes killed in the Stop-Process
loop that lack a Path are not recorded in ProcessesToRestart; change the
assembly of the restart list to include all stopped processes (use
$running_processes as the source rather than filtering by Path) and when a
process has no Path emit a diagnostic warning including its Id/Name so admins
know it was killed but untrackable; update references to
$stopped_processes/ProcessesToRestart and the Stop-Process loop (the foreach
over $running_processes and the warn call) to ensure entries with missing Path
are still added to the restart list (with a null or placeholder Path) while
still logging the warning.
- Around line 650-652: The current elseif ($status) branch adds any non-Running
service to $stopped_services, causing services the user already stopped to be
queued for restart; change the logic so only services actually stopped by this
function are added to $stopped_services — i.e., remove or stop using the elseif
($status) path to append to $stopped_services and instead append to
$stopped_services only when Stop-Service (in the stop_running_process flow) was
invoked and succeeded; ensure stop_running_process and the consumer
(ServicesToRestart / scoop-update) rely on $stopped_services that contains only
services this script stopped.
- Line 553: The $processdir assignment currently uses appdir ($processdir =
appdir $app $global | Convert-Path) which yields the app root (…\apps\discord)
and breaks NO_JUNCTION path rebasing; change it so that in NO_JUNCTION mode
$processdir points at the versioned directory (the same as currentdir) — e.g.
detect NO_JUNCTION and set $processdir = Convert-Path $currentdir (or, if
currentdir is not available, resolve the single version subfolder under $(appdir
$app $global) and Convert-Path that); otherwise retain the original appdir
behavior. Ensure you update references to $processdir used later (e.g., in
scoop-update.ps1 replacement logic) so the Replace is applied against the
versioned path.
In `@libexec/scoop-update.ps1`:
- Around line 413-430: The upstream issue is that check_running_process is
populating running_ret.ProcessDir with the app root instead of the versioned
executable folder, which makes the rebasing Replace in scoop-update.ps1 a no-op;
update check_running_process in lib/install.ps1 so that ProcessDir stores the
actual version-specific path (the directory containing the running executable)
rather than appdir($app), e.g. derive it from the discovered process executable
path (use Split-Path -Parent or equivalent when setting ProcessDir) so that code
referencing running_ret.ProcessDir in scoop-update.ps1 will correctly rebase
$proc against $old_processdir/$new_processdir.
added 3 commits
February 22, 2026 01:16
…fix path rebasing version bugs
…stop_running_process
HUMORCE
suggested changes
Mar 2, 2026
Member
HUMORCE
left a comment
HUMORCE
left a comment
There was a problem hiding this comment.
Some additional processes/services of apps reside outside Scoop managed directory, and the manifest does not include a complete list.
It is precisely these apps that require this feature.
Author
|
Hey @HUMORCE I haven't encountered any apps in my Scoop workflow that reside outside the managed directory. Could you provide a few examples of apps that behave this way? |
added 4 commits
March 4, 2026 21:56
…luding process and service management using Restart Manager.
…ctions for merging, writing details, and checking visibility of tracked processes
…kingProcesses and Get-RunningProcessesInDir for improved process path handling
…ss tests with additional mocks and scenarios
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Replacing applications that are currently running (like Discord, Telegram, or services) can lock files and fail the update/uninstall process. This provides a smoother experience by force-closing them and restarting them afterward.
How Has This Been Tested?
Unit tests and local process/service hooking scenarios were tested.
Checklist:
developbranch.Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation