Add file lock to prevent concurrent executions #6557
Conversation
WalkthroughImplements a file-based exclusive lock in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as scoop.ps1
participant LockFile as Lock File
participant Command as Subcommand
User->>Script: invoke scoop command
Script->>LockFile: attempt exclusive open (write)
alt Lock unavailable
Note over Script,LockFile `#f6f0d9`: print waiting message once\nretry until writable
LockFile-->>Script: lock acquired
else Lock acquired immediately
LockFile-->>Script: lock acquired
end
Script->>Script: restore aliases / re-aliasing
Script->>Command: dispatch selected subcommand
Command-->>Script: returns result or error
Script->>LockFile: close/dispose lock (finally)
Script-->>User: exit (success or code 1 for unknown command)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
5-11: Fill in the CHANGELOG reference instead of using an empty[#]linkThe new feature entry uses a placeholder link:
- **core:** Add file lock to prevent concurrent executions ([#](https://github.com/ScoopInstaller/Scoop/issues/))This will render as a broken/meaningless reference. Please either point it at the actual issue/PR number or drop the link entirely, e.g.:
- - **core:** Add file lock to prevent concurrent executions ([#](https://github.com/ScoopInstaller/Scoop/issues/)) + - **core:** Add file lock to prevent concurrent executions + # or, once known: + - **core:** Add file lock to prevent concurrent executions ([#6557](https://github.com/ScoopInstaller/Scoop/pull/6557))
🧹 Nitpick comments (1)
.gitignore (1)
1-10: Consider anchoring the ignore rule to the repo rootThe lock file used by
bin/scoop.ps1lives at the repo root ($PSScriptRoot\..\.lock), but.gitignorecurrently ignores any.lockfile anywhere:.lockIf you only intend to ignore the root lock file, anchoring makes this more precise:
- .lock + /.lockNot a blocker, but it reduces the chance of accidentally hiding unrelated
.lockfiles elsewhere in the tree.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)bin/scoop.ps1(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T01:35:53.228Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:105-109
Timestamp: 2025-08-31T01:35:53.228Z
Learning: In the Scoop codebase, the `abort` function calls `exit` to terminate the entire PowerShell script execution rather than throwing catchable exceptions. Functions that call `abort` on errors cannot be wrapped in try-catch blocks for error handling.
Applied to files:
bin/scoop.ps1
🔇 Additional comments (1)
bin/scoop.ps1 (1)
24-73: Command dispatch under the lock looks solid and keeps the lock lifecycle well-scopedThe
try { ... } finally { $lock_stream.Close(); $lock_stream.Dispose() }structure around the subcommand dispatch is a good fit here:
$subCommandrouting for help/version/known commands remains clear and preserves existing behavior.- Unknown commands now warn and exit with code 1, which is appropriate for CLI ergonomics.
- Closing/disposing the lock stream in
finallycorrectly ensures the lock is released on both success and failure paths (exceptions, early returns, etc.), so the mutex doesn’t leak.Apart from the lock acquisition concerns noted above, this part of the change reads cleanly and keeps the new global serialization behavior nicely contained.
| $lock_file_path = "$PSScriptRoot\..\.lock" | ||
| $lock_show_waiting_message = $true | ||
|
|
||
| # for aliases where there's a local function, re-alias so the function takes precedence | ||
| $aliases = Get-Alias | Where-Object { $_.Options -notmatch 'ReadOnly|AllScope' } | ForEach-Object { $_.Name } | ||
| Get-ChildItem Function: | Where-Object -Property Name -In -Value $aliases | ForEach-Object { | ||
| Set-Alias -Name $_.Name -Value Local:$($_.Name) -Scope Script | ||
| while (-not $lock_stream.CanWrite) { | ||
| try { | ||
| $lock_stream = [System.IO.File]::Open($lock_file_path, [System.IO.FileMode]::Create, [System.IO.FileAccess]::Write) | ||
| } catch { | ||
| if ($lock_show_waiting_message) { | ||
| 'Waiting for exclusive access...' | ||
| $lock_show_waiting_message = $false | ||
| } | ||
| Start-Sleep -Seconds 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Tighten lock acquisition error handling to avoid masking non-contention failures
The lock loop works correctly for normal contention, but the generic catch means any failure opening the lock file (e.g. permission error, invalid path) will cause an infinite retry with only a single "Waiting for exclusive access..." line:
$lock_file_path = "$PSScriptRoot\..\.lock"
$lock_show_waiting_message = $true
while (-not $lock_stream.CanWrite) {
try {
$lock_stream = [System.IO.File]::Open($lock_file_path, [System.IO.FileMode]::Create, [System.IO.FileAccess]::Write)
} catch {
if ($lock_show_waiting_message) {
'Waiting for exclusive access...'
$lock_show_waiting_message = $false
}
Start-Sleep -Seconds 1
}
}That makes genuine configuration/IO problems look like normal queueing and leaves the user with a hung scoop process and no actionable error.
It would be safer to:
- Only retry for contention-style errors (e.g.
System.IO.IOExceptionfrom a sharing violation), and - Let other exceptions bubble to the caller so the failure is visible.
- Emit the waiting message via
warn(or similar) so it doesn’t pollute the success pipeline.
For example:
-$lock_file_path = "$PSScriptRoot\..\.lock"
-$lock_show_waiting_message = $true
-
-while (-not $lock_stream.CanWrite) {
- try {
- $lock_stream = [System.IO.File]::Open($lock_file_path, [System.IO.FileMode]::Create, [System.IO.FileAccess]::Write)
- } catch {
- if ($lock_show_waiting_message) {
- 'Waiting for exclusive access...'
- $lock_show_waiting_message = $false
- }
- Start-Sleep -Seconds 1
- }
-}
+$lock_file_path = "$PSScriptRoot\..\.lock"
+$lock_show_waiting_message = $true
+$lock_stream = $null
+
+while ($true) {
+ try {
+ $lock_stream = [System.IO.File]::Open(
+ $lock_file_path,
+ [System.IO.FileMode]::Create,
+ [System.IO.FileAccess]::Write,
+ [System.IO.FileShare]::None
+ )
+ break
+ } catch [System.IO.IOException] {
+ if ($lock_show_waiting_message) {
+ warn 'Waiting for exclusive access...'
+ $lock_show_waiting_message = $false
+ }
+ Start-Sleep -Seconds 1
+ }
+}This preserves the 1‑second retry behavior for true contention, but fails fast (with a clear error) on misconfiguration or other unexpected IO problems, and keeps the waiting message off the success pipeline.
🤖 Prompt for AI Agents
In bin/scoop.ps1 around lines 9 to 22, the catch block currently swallows all
exceptions and prints a single plain string, causing infinite retries on
non-contention errors and polluting success output; change the logic to only
retry for contention-style IO errors (i.e. when the exception is a
System.IO.IOException indicating a sharing violation), rethrow any other
exceptions so they surface to the caller, and emit the waiting message with
Write-Warning (or similar) instead of writing to stdout; keep the 1-second retry
loop for true contention but fail fast for other IO/configuration errors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bin/scoop.ps1 (1)
9-21: Still retries forever on anyIOExceptionand prints status to stdoutCatching all
System.IO.IOExceptionvalues here still turns any IO failure (not just sharing violations) into an infinite 1‑second retry loop with a single"Waiting for exclusive access..."line written to the success pipeline. That leaves users with a hungscoopwhen, e.g., the lock path is inaccessible for non‑contention reasons.It would be safer to:
- Only retry on sharing‑violation style IO errors.
- Re‑throw other IO exceptions so misconfiguration is visible.
- Emit the waiting message via
warn(or similar) instead of writing a bare string.For example:
-$lock_file_path = "$PSScriptRoot\..\.lock" -$lock_show_waiting_message = $true - -while (-not $lock_stream.CanWrite) { - try { - $lock_stream = [System.IO.File]::Open($lock_file_path, [System.IO.FileMode]::Create, [System.IO.FileAccess]::Write) - } catch [System.IO.IOException] { - if ($lock_show_waiting_message) { - 'Waiting for exclusive access...' - $lock_show_waiting_message = $false - } - Start-Sleep -Seconds 1 - } -} +$lock_file_path = "$PSScriptRoot\..\.lock" +$lock_show_waiting_message = $true +$lock_stream = $null + +while ($true) { + try { + $lock_stream = [System.IO.File]::Open( + $lock_file_path, + [System.IO.FileMode]::Create, + [System.IO.FileAccess]::Write, + [System.IO.FileShare]::None + ) + break + } catch [System.IO.IOException] { + # Only treat sharing violations as contention; surface other IO problems. + $errorCode = $_.Exception.HResult -band 0xFFFF + if ($errorCode -notin 32, 33) { + throw + } + + if ($lock_show_waiting_message) { + warn 'Waiting for exclusive access...' + $lock_show_waiting_message = $false + } + Start-Sleep -Seconds 1 + } +}This keeps the queueing behavior for real contention while failing fast and noisily for unexpected IO errors, and keeps lock‑wait noise off the success stream. Based on learnings.
#!/bin/bash # Optional: confirm how Scoop currently reports waiting vs. failure. # Run this inside a Scoop dev checkout after applying (or not applying) the patch: # 1) Start one long-running `scoop` process that holds the lock. # 2) Start a second `scoop` and observe: # - whether waiting messages go to stdout vs. stderr # - behavior if you temporarily make the repo root read-only to force a non-sharing IOException. echo "Manually verify lock behavior under contention and misconfigured lock path."
🧹 Nitpick comments (1)
.gitignore (1)
4-4: Scope.lockignore to repo root (optional)Given the lock path is
$PSScriptRoot\..\.lock, you can narrow the ignore to the root file only to avoid hiding any other.lockfiles:-.lock +/.lockNot required, but slightly clearer about intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)bin/scoop.ps1(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T01:39:35.684Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:135-135
Timestamp: 2025-08-31T01:39:35.684Z
Learning: In the Scoop codebase, `Invoke-ScoopDownload` in lib/download.ps1 calls `abort` when hash verification fails, which terminates script execution completely rather than trying remaining mirrors. The scoop-download.ps1 script should maintain consistency with this behavior by using `continue app_loop` to skip to the next app rather than trying remaining mirrors for the current app.
Applied to files:
bin/scoop.ps1
📚 Learning: 2025-08-31T01:35:53.228Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:105-109
Timestamp: 2025-08-31T01:35:53.228Z
Learning: In the Scoop codebase, the `abort` function calls `exit` to terminate the entire PowerShell script execution rather than throwing catchable exceptions. Functions that call `abort` on errors cannot be wrapped in try-catch blocks for error handling.
Applied to files:
bin/scoop.ps1
🔇 Additional comments (2)
bin/scoop.ps1 (1)
24-73: Guarded dispatch + unknown‑command handling look goodWrapping subcommand dispatch in
try/finallyto ensure the lock stream is closed, restoring the alias re-aliasing inside the guarded region, and adding an explicit default that warns and exits with code 1 for unknown commands all look consistent and correct for the main entrypoint.CHANGELOG.md (1)
5-10: Changelog entry is consistent with existing styleThe new
**core:** Add file lock to prevent concurrent executionsentry matches the existing formatting and correctly describes the change.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bin/scoop.ps1 (1)
9-29: Lock acquisition loop is solid; consider clearer termination and non-polluting status outputThe lock logic looks correct and much safer now: catching only
[System.IO.IOException]and filtering by HResult 32/33 prevents infinite retries on non-contention errors. A couple of small refinements could make this more robust and readable:
The condition
while (-not $lock_stream.CanWrite)currently relies on$lock_streambeing$nullandSet-StrictMode -Off, so$null.CanWriteevaluates to$null, making-not $null$true. This is subtle and easy to break with future refactors. Consider a more explicit pattern such as:$lock_stream = $null while (-not $lock_stream) { try { $lock_stream = [System.IO.File]::Open( $lock_file_path, [System.IO.FileMode]::Create, [System.IO.FileAccess]::Write ) } catch [System.IO.IOException] { # existing HResult handling... } }(Optionally reassign
$lock_stream = $nullin the catch block if you choose a different loop condition.)The bare string
'Waiting for exclusive access...'writes to the success pipeline. For callers that parsescoopoutput (e.g.,scoop list | ...), this introduces unexpected noise whenever contention happens. Emitting this viawarn(fromcore.ps1) orWrite-Warninginstead keeps user feedback on stderr and preserves the success stream for structured output:if ($lock_show_waiting_message) { warn 'Waiting for exclusive access...' $lock_show_waiting_message = $false }Optional: if you want the exclusivity to be more obvious to future readers, you could switch to the 4‑argument overload and pass
[System.IO.FileShare]::Noneexplicitly, even though the current 3‑argument overload already defaults toFileShare.None.These are all incremental improvements; the current behavior for contention vs. non‑contention errors is already correct.
🧹 Nitpick comments (1)
bin/scoop.ps1 (1)
31-80: try/finally wrapping of subcommand dispatch looks correct; minor defensive cleanup possibleWrapping the entire subcommand dispatch in a
try { ... } finally { ... }is a good way to guarantee the lock is released on normal completion and on terminating errors thrown within thetryblock. A couple of small notes:
Given the acquisition loop only falls through once
$lock_streamis a writable stream,$lock_streamshould always be non‑null when enteringfinally. To make this more future‑proof against changes to the loop, you could defensively guard the cleanup:finally { if ($lock_stream) { $lock_stream.Close() $lock_stream.Dispose() } }Calling both
Close()andDispose()on aFileStreamis harmless but redundant; either is sufficient. Not urgent, but you could simplify to justDispose().The new default branch for unknown
$subCommandvalues:warn "scoop: '$subCommand' isn't a scoop command. See 'scoop help'." exit 1is a nice improvement for scriptability: it surfaces a clear error message and a non‑zero exit code while still running under the lock, so the lock is released via
finallyon the way out.Overall, the restructuring under
try/finallylooks sound; the above points are minor polish.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)bin/scoop.ps1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- .gitignore
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T01:39:35.684Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:135-135
Timestamp: 2025-08-31T01:39:35.684Z
Learning: In the Scoop codebase, `Invoke-ScoopDownload` in lib/download.ps1 calls `abort` when hash verification fails, which terminates script execution completely rather than trying remaining mirrors. The scoop-download.ps1 script should maintain consistency with this behavior by using `continue app_loop` to skip to the next app rather than trying remaining mirrors for the current app.
Applied to files:
bin/scoop.ps1
📚 Learning: 2025-08-31T01:35:53.228Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:105-109
Timestamp: 2025-08-31T01:35:53.228Z
Learning: In the Scoop codebase, the `abort` function calls `exit` to terminate the entire PowerShell script execution rather than throwing catchable exceptions. Functions that call `abort` on errors cannot be wrapped in try-catch blocks for error handling.
Applied to files:
bin/scoop.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/scoop.ps1 (1)
23-25: Consider usingwarnfor consistency with Scoop's error handling patterns.The
Write-Hostfunction works, but using thewarnfunction (imported from core.ps1) would be more consistent with how Scoop handles user-facing messages elsewhere in the codebase.Apply this diff:
- if ($lock_show_waiting_message) { - Write-Host 'Waiting for exclusive access...' - $lock_show_waiting_message = $false - } + if ($lock_show_waiting_message) { + warn 'Waiting for exclusive access...' + $lock_show_waiting_message = $false + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)bin/scoop.ps1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T01:39:35.684Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:135-135
Timestamp: 2025-08-31T01:39:35.684Z
Learning: In the Scoop codebase, `Invoke-ScoopDownload` in lib/download.ps1 calls `abort` when hash verification fails, which terminates script execution completely rather than trying remaining mirrors. The scoop-download.ps1 script should maintain consistency with this behavior by using `continue app_loop` to skip to the next app rather than trying remaining mirrors for the current app.
Applied to files:
bin/scoop.ps1
📚 Learning: 2025-08-31T01:35:53.228Z
Learnt from: z-Fng
Repo: ScoopInstaller/Scoop PR: 6473
File: libexec/scoop-download.ps1:105-109
Timestamp: 2025-08-31T01:35:53.228Z
Learning: In the Scoop codebase, the `abort` function calls `exit` to terminate the entire PowerShell script execution rather than throwing catchable exceptions. Functions that call `abort` on errors cannot be wrapped in try-catch blocks for error handling.
Applied to files:
bin/scoop.ps1
🔇 Additional comments (1)
.gitignore (1)
4-4: LGTM!Correctly ignores the lock file created by the locking mechanism.
| } finally { | ||
| $lock_stream.Dispose() | ||
| } |
There was a problem hiding this comment.
Add null check before disposing the lock stream.
If an unexpected exception occurs during lock acquisition (before $lock_stream is successfully assigned), the finally block will attempt to call Dispose() on a null object, which will throw a NullReferenceException and mask the original exception.
Apply this diff to add a defensive null check:
} finally {
- $lock_stream.Dispose()
+ if ($null -ne $lock_stream) {
+ $lock_stream.Dispose()
+ }
}🤖 Prompt for AI Agents
In bin/scoop.ps1 around lines 77 to 79, the finally block disposes $lock_stream
without checking for null which can throw if assignment failed; update the
finally block to check if $lock_stream is not $null before calling Dispose()
(i.e., wrap Dispose() call in an if ($lock_stream -ne $null) {
$lock_stream.Dispose() } or use conditional invocation) to avoid masking the
original exception.
|
Question: is there a shared persistent state that Scoop reads/writes when updating or installing apps? As far as I know Scoop was one of the few apps that supported doing multiple installs/updates (of different apps of course) at the same time. Running a scheduled task for update is fine, but what's the use case of running |
|
@rashil2000 more on the context, to be honest... and also to get feedback from you :) Thought I'm a developer, I'm dealing with IT for a small company, so I'm trying to use my dev habits to manage PCs. Workstations run Windows, and we have a couple of in-house apps. I've chosen to install them and keep them updated through Scoop. Installation itself is automatized through Intune depending on groups the user or machine belong to, but it can't handle updates. So the first thing Intune does is install Scoop, and then Intune only install apps through Scoop. This allows user to install extra apps. Intune also installs a scheduled task as described, to get things up-to-date. We're not big enough to get fixed, verified versions of apps... so I prefer to always have the latest versions, with the tiny risk that an app may break. Last detail: Intune can install its things as device or as user, and it's kind of a nightmare regarding permissions. When as device, install process belongs to SYSTEM account, and is elevated. When as user, install process belongs to the user, and is automatically elevated iff the user is admin. This last point is really painful: user context is not the same for all users. For instance, when Intune installs Scoop for admin users, buckets (Git repos) somehow get wrong permissions, and non-elevated shells cannot use Scoop anymore... so there are some tricks to get back on track. Because Scoop takes care of updates, I don't have to deal with that on my apps, and that's great. Whenever CICD pushes a new version, Many users reported many times that the app couldn't launch anymore. My assumption is that they launched the app right at startup, the app tells them to update it, so they do it, but the scheduled task is also doing it, and boom. Thankfully, the fix is simply uninstall and reinstall.
|
|
This should ideally be done on a per-app basis. The We can also show the status 'Updating' in the |
2 participants
Description
This patch adds a file lock to ensure only one process uses Scoop. Extra processes wait 1 second and retry, so dequeuing is not handled in order.
The diff shows a lot of modified lines, but it's actually just an indent increase due to the try-finally block.
Motivation and Context
I've created a scheduled task to automate
scoop update *every morning or at every startup. Whether it's good or bad is another topic :) But I've noticed some conflicts. For instance,scoop listshowsinstall failedwhile the app is being installed. I suspect that I got corrupted git-buckets due to parallel updates too.This is also in line with my expectations of a service. IMHO Scoop acts like a session-wide service that several processes can access, so it must ensure it's safe to do so. The fact that Scoop exclusively works with PS1 scripts is an implementation detail.
Note that global installs made by different process owners could still conflicts. A better version would be to pinpoint which commands are subjects to lock, and in these cases whether the lock should be global. This would also have the benefit not to lock for things like displaying help. I'd love to have that, but there's more effort to put, so please tell me what you think first.
How Has This Been Tested?
Locally with multiple shells.
Checklist:
developbranch.Summary by CodeRabbit
New Features
Bug Fixes
Chores