Skip to content

fix(v3/updater): remove the staging dir when the download fails - #6205

Open
manduinca wants to merge 1 commit into
wailsapp:masterfrom
manduinca:fix/updater-download-tempdir-cleanup
Open

manduinca wants to merge 1 commit into
wailsapp:masterfrom
manduinca:fix/updater-download-tempdir-cleanup

Conversation

@manduinca

@manduinca manduinca commented Oct 1, 2026 •

Copy link
Copy Markdown

Description

Updater.download returns "", "", err on every error path. With named results, that assignment runs before the deferred cleanup, so dir is already "" when the deferred os.RemoveAll(dir) reads it. Nothing gets removed and the wails-update-* directory with the partial .artifact stays in the temp dir after every failed download, as described in the issue.

The fix keeps the directory in a local (tmpDir) for the deferred cleanup and the success return. The error paths and their return values are unchanged.

Fixes #6185

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • WEP (proposal only; no implementation)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

TestDownloadAndInstall_DownloadError_RemovesStagingDir makes the provider fail the transfer with io.ErrUnexpectedEOF and counts the wails-update-* directories before and after, the same way TestDownloadAndInstall_FailedFlow_RemovesStagingDir does for the verify stage. On master it fails with staging dir leaked: 0 → 1; with the fix it passes.

From v3: go test ./pkg/updater/... passes (all six packages), go vet ./pkg/updater/ is clean and both changed files are gofmt-clean.

  • Windows
  • macOS
  • Linux

Test Configuration

macOS 26 (arm64), Go 1.26.2. These are package tests only; wails doctor was not run since no app was built.

Checklist:

  • (v2 only) I have updated website/src/pages/changelog.mdx with details of this PR (v3 changelog entries are added automatically)
  • My code follows the general coding style of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • Bug Fixes
    • Temporary staging files are now cleaned up when an update download fails.

download returns "", "", err on every error path, which clears the named
result dir before the deferred os.RemoveAll(dir) runs. The cleanup then
removes "" and the wails-update-* directory with the partial artifact is
left behind. Keep the directory in a local for the deferred cleanup.

Fixes wailsapp#6185
@github-actions github-actions Bot added Bug Something isn't working v3 labels Oct 1, 2026
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: wailsapp/wails/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 12c769e4-7670-4c8a-b8b4-c836d94653b1

📥 Commits

Reviewing files that changed from the base of the PR and between a699216 and 86ec5ef.

📒 Files selected for processing (2)
  • v3/pkg/updater/download.go
  • v3/pkg/updater/updater_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Updater.download now retains the temporary directory path for deferred cleanup when a download fails. A regression test checks that a provider download error does not leave an additional staging directory.

Changes

Updater download cleanup

Layer / File(s) Summary
Download cleanup and regression test
v3/pkg/updater/download.go, v3/pkg/updater/updater_test.go
Deferred cleanup removes the captured temporary directory on failure. The test checks that a download error does not increase the staging-directory count.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 86ec5

The change fixes failed-download staging cleanup without altering successful downloads or error handling. No actionable merge-blocking risk remains; merge after normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: removing the v3 updater staging directory when a download fails.
Description check ✅ Passed The description explains the defect, fix, issue reference, test coverage, environment, and checklist status. It provides sufficient reproduction and validation details. The omitted wails doctor output…
Linked Issues check ✅ Passed The PR addresses the coding requirement in issue #6185. v3/pkg/updater/download.go keeps the staging directory in tmpDir for deferred cleanup and for the successful return. Error returns leave the…
Out of Scope Changes check ✅ Passed The changes stay within issue #6185. The production change fixes failed-download staging cleanup. The added test verifies that behavior. No unrelated production behavior or additional leftover categor…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

A rabbit watched the download fail,
Then swept the staging path without fail.
The temp dir vanished from its sight,
A test checked each folder right.
“No leftovers,” said the rabbit, bright!

Comment @coderabbitai help to get the list of available commands.

@taliesin-ai

Copy link
Copy Markdown
Collaborator

Automated v3 GA test run (taliesin-ai) for head 86ec5ef77aeb7d636b1b4a0f0a408557d35e8570, run 2026-10-04 against master baseline f21e67fa.

Platform CLI build Unit tests (./pkg/... ./internal/...) Example build (examples/window)
macOS (arm64) PASS PASS (51/51 pkgs) PASS
Windows (amd64) PASS 51/52: internal/webview2/pkg/edge FAIL* PASS
Linux not tested (node offline)

* Same failure on master f21e67fa, so this PR doesn't introduce it. TestCookieManager needs a live WebView2 controller. It times out after 60s in the non-interactive SSH session the runner uses, so this is a test-environment limitation and not a code failure.

The app was not launched (no GUI session over SSH). This run doesn't review or approve the PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v3] updater: failed downloads leave the temp dir behind (named-return shadows the deferred RemoveAll)

2 participants