Skip to content

Keep Scoop package listings working when scoop commands fail - #5321

Merged
Gabriel Dufresne (GabrielDuf) merged 2 commits into
mainfrom
fix/5262-scoop-listing-stream-deadlock
Aug 25, 2026
Merged

Keep Scoop package listings working when scoop commands fail#5321
Gabriel Dufresne (GabrielDuf) merged 2 commits into
mainfrom
fix/5262-scoop-listing-stream-deadlock

Conversation

@GabrielDuf

Copy link
Copy Markdown
Contributor

This pull request refactors how process output is read and logged in the Scoop package manager integration, improving reliability when dealing with processes that produce large amounts of output, especially on standard error. It introduces a new ScoopProcess helper class to centralize and streamline process output handling, replaces duplicated code with calls to this helper, and adds robust timeout and error handling. Comprehensive tests are also added to ensure the new logic handles edge cases like pipe buffer overflows.

Process Output Handling Improvements

  • Introduced the ScoopProcess helper class to encapsulate logic for reading process standard output and error streams, supporting both line-by-line and full-output reads, and ensuring proper logging and process cleanup. This prevents deadlocks and handles large error outputs robustly.
  • Refactored all usages in Scoop.cs, ScoopPkgDetailsHelper.cs, and ScoopSourceHelper.cs to use ScoopProcess.ReadLines and ScoopProcess.ReadToEnd instead of duplicating output reading and logging logic, simplifying these methods and improving reliability.

Timeouts and Error Handling

  • Enhanced version probing and stream draining with explicit timeouts and error logging in _loadManagerVersion, preventing hangs and reporting issues if the process does not respond in time.
  • Improved the RunListingTaskWithTimeout logic in PackageManager.cs to allow disabling timeouts only when appropriate, and added a RefreshPackageIndexesSafely method to handle exceptions gracefully and warn the user if index refresh fails.

Testing Enhancements

  • Added a TestProcessTaskLogger fake for unit testing process output logging.
  • Added comprehensive tests in ScoopProcessTests to verify that reading process output works even when the standard error stream is flooded, ensuring the new logic is robust against pipe buffer limitations.

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.

Pull request overview

Refactors Scoop process handling to prevent output-pipe deadlocks and improve timeout resilience.

Changes:

  • Centralizes concurrent stdout/stderr draining in ScoopProcess.
  • Adds safer listing and index-refresh timeout handling.
  • Adds Windows tests for stderr pipe flooding.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ScoopProcessTests.cs Tests process output under heavy stderr.
TestProcessTaskLogger.cs Adds a process logger test fake.
PackageManager.cs Adds tracked timeouts and safe index refresh.
Scoop.cs Uses centralized process handling.
ScoopSourceHelper.cs Refactors source-list output reading.
ScoopProcess.cs Implements concurrent stream draining.
ScoopPkgDetailsHelper.cs Reads stderr concurrently with manifest output.
Suppressed comments (2)

src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs:472

  • The scoop list process is still missing from the timeout tracker. This path runs both for installed-package listings and inside update listings, so a hung process survives the 60-second timeout and the automatic retry may attach to or leave behind the same blocked task. Register it before calling ReadLines.
            return ParseInstalledPackages(ScoopProcess.ReadLines(p, logger));

src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs:503

  • A failed scoop update normally reports failure through a nonzero exit code, but ReadToEnd only records that code in the task logger and returns normally. Consequently RefreshPackageIndexesSafely never enters its warning path for ordinary command failures and treats the refresh as successful. Convert the nonzero result into an exception here so the new safe wrapper can report it and continue listing.
            ScoopProcess.ReadToEnd(p, logger);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs

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.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs:567

  • When this timeout fires, the code still assigns any partial stdout drained after Kill to version; the Unknown fallback only runs when that output is empty. Since scoop --version can emit output before hanging, the manager may be loaded with a partial/multiline value despite the warning promising an unknown version. Track the timeout and discard stdout for the version value while still draining it.
            if (!process.WaitForExit(VersionProbeTimeout))
            {
                Logger.Warn(
                    $"\"scoop --version\" did not finish after {VersionProbeTimeout / 1000} seconds, "
                        + "it will be killed and Scoop will be loaded with an unknown version"

@randy-but-a-ro randy-but-a-ro Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉

Integration Details
{
	"deliveryId": "34e57cb0-a0ac-11f1-8ed0-5c10713c445a",
	"headSha": "aad9ed6256fff163bc7a700a2d07021a8ea88057",
	"reviewer": "copilot-pull-request-reviewer[bot]"
}

@GabrielDuf
Gabriel Dufresne (GabrielDuf) merged commit 4108308 into main Aug 25, 2026
6 checks passed
@GabrielDuf
Gabriel Dufresne (GabrielDuf) deleted the fix/5262-scoop-listing-stream-deadlock branch August 25, 2026 18:00
martesi added a commit to martesi/UniGetUI that referenced this pull request Aug 30, 2026
martesi added a commit to martesi/UniGetUI that referenced this pull request Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

[IMPROVEMENT] Don't fail to scoop list operations if scoop update fails

2 participants