From 88cccc184e2dac28d7826c3aa374fb61ba54289b Mon Sep 17 00:00:00 2001 From: yetone Date: Fri, 2 Oct 2026 13:26:13 +0800 Subject: [PATCH] =?UTF-8?q?update:=20on=20Windows,=20an=20exe=20something?= =?UTF-8?q?=20holds=20open=20for=20a=20moment=20still=20goes=20aside=20for?= =?UTF-8?q?=20the=20new=20version,=20and=20one=20held=20for=20good=20says?= =?UTF-8?q?=20why=20in=20full,=20with=20the=20release=20page=20a=20click?= =?UTF-8?q?=20away=20(freecss=20on=20Discord:=20"0.1.628=20=E5=B7=B2?= =?UTF-8?q?=E4=B8=8B=E8=BD=BD=20=C2=B7=20couldn't=20move=20magpie-windows-?= =?UTF-8?q?amd64.exe=20aside=20to=20put=20the=20new=20version=20in:=20rena?= =?UTF-8?q?me=20C:\Us=E2=80=A6")=20=E2=80=94=20InstallBinary=20renamed=20t?= =?UTF-8?q?he=20running=20exe=20to=20.old=20once=20and=20gave=20up=20on=20?= =?UTF-8?q?the=20first=20error,=20which=20Go=20prints=20as=20"rename=20=20.old:=20",=20two=20full=20paths=20ahead=20of?= =?UTF-8?q?=20the=20reason,=20and=20the=20version=20row=20cut=20it=20off?= =?UTF-8?q?=20on=20one=20line,=20so=20what=20failed=20couldn't=20be=20read?= =?UTF-8?q?.=20A=20leftover=20.old=20was=20already=20handled=20(RemoveOld,?= =?UTF-8?q?=20then=20.old-2=E2=80=A6,=20TestInstallBinaryBesideARunningOld?= =?UTF-8?q?),=20and=20Windows=20renames=20a=20running=20image=20(it=20is?= =?UTF-8?q?=20mapped=20with=20FILE=5FSHARE=5FDELETE),=20so=20the=20likely?= =?UTF-8?q?=20cause=20is=20another=20program=20holding=20the=20exe=20open?= =?UTF-8?q?=20without=20delete=20sharing=20=E2=80=94=20an=20antivirus=20sc?= =?UTF-8?q?an,=20OneDrive=20or=20a=20similar=20sync=20client=20=E2=80=94?= =?UTF-8?q?=20or=20a=20folder=20that=20refuses=20the=20change=20(access=20?= =?UTF-8?q?denied);=20which=20one=20isn't=20known=20until=20the=20full=20m?= =?UTF-8?q?essage=20is=20read.=20moveAside=20now=20tries=20again=20for=20a?= =?UTF-8?q?bout=203=20s=20(0.1,=200.2,=200.4,=200.8,=201.5=20s=20apart),?= =?UTF-8?q?=20clearing=20a=20read-only=20attribute=20after=20the=20first?= =?UTF-8?q?=20failure,=20with=20a=20fresh=20.old=20name=20each=20try;=20a?= =?UTF-8?q?=20missing=20exe=20isn't=20retried.=20When=20it=20still=20can't?= =?UTF-8?q?,=20the=20message=20is=20the=20reason=20without=20the=20paths?= =?UTF-8?q?=20and=20what=20to=20do:=20ERROR=5FSHARING=5FVIOLATION/ERROR=5F?= =?UTF-8?q?LOCK=5FVIOLATION=20says=20another=20program=20has=20it=20open?= =?UTF-8?q?=20(often=20an=20antivirus=20or=20OneDrive),=20access=20denied?= =?UTF-8?q?=20names=20the=20folder=20Windows=20won't=20let=20magpie=20chan?= =?UTF-8?q?ge,=20anything=20else=20says=20to=20download=20the=20new=20vers?= =?UTF-8?q?ion;=20the=20download=20is=20kept=20for=20another=20try,=20as?= =?UTF-8?q?=20before.=20The=20version=20row=20wraps=20a=20failed=20install?= =?UTF-8?q?'s=20or=20download's=20reason=20(.sub.wraps)=20with=20the=20who?= =?UTF-8?q?le=20text=20as=20its=20tooltip,=20and=20a=20ready-but-failed=20?= =?UTF-8?q?install=20offers=20Download,=20which=20opens=20the=20release=20?= =?UTF-8?q?page.=20The=20Windows=20test=20box=20wasn't=20reachable=20from?= =?UTF-8?q?=20this=20session=20(ssh=20key=20refused),=20so=20nothing=20ran?= =?UTF-8?q?=20on=20Windows;=20TestInstallBinaryHeldOpen=20(windows-only:?= =?UTF-8?q?=20an=20exe=20held=20open=20without=20FILE=5FSHARE=5FDELETE=20f?= =?UTF-8?q?or=200.5=20s=20still=20installs;=20held=20for=20good,=20the=20e?= =?UTF-8?q?rror=20says=20another=20program=20has=20it=20open=20and=20the?= =?UTF-8?q?=20download=20is=20kept)=20was=20cross-compiled=20with=20GOOS?= =?UTF-8?q?=3Dwindows=20go=20test=20-c=20but=20not=20run.=20TestMoveAsideT?= =?UTF-8?q?riesAgain=20and=20TestMoveAsideSaysWhy=20(portable,=20with=20a?= =?UTF-8?q?=20fake=20rename)=20and=20the=20new=20update-check.test.cjs=20c?= =?UTF-8?q?ase=20(webkit,=20en=20and=20zh:=20the=20reason=20is=20not=20cut?= =?UTF-8?q?=20off,=20its=20tooltip=20is=20the=20full=20text,=20Download=20?= =?UTF-8?q?opens=20the=20release=20page,=20a=20later=20state=20is=20one=20?= =?UTF-8?q?line=20again)=20fail=20without=20the=20change;=20Chromium=20cou?= =?UTF-8?q?ldn't=20launch=20here=20(its=20Playwright=20cache=20has=20no=20?= =?UTF-8?q?ICU=20data).=20go=20vet=20./internal/...,=20go=20build=20-tags?= =?UTF-8?q?=20nogui=20(darwin,=20GOOS=3Dlinux),=20GOOS=3Dwindows=20go=20bu?= =?UTF-8?q?ild=20and=20vet,=20go=20test=20-tags=20nogui=20./...=20pass=20(?= =?UTF-8?q?TestFXInState=20failed=20once=20on=20a=20TempDir=20cleanup=20ra?= =?UTF-8?q?ce,=20passes=20on=20the=20rerun;=20untouched).?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/gui/assets/app.css | 2 + internal/gui/assets/app.js | 7 +++ internal/gui/tests/update-check.test.cjs | 29 +++++++++ internal/update/aside_test.go | 79 ++++++++++++++++++++++++ internal/update/install_windows_test.go | 48 ++++++++++++++ internal/update/update.go | 56 ++++++++++++++++- 6 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 internal/update/aside_test.go diff --git a/internal/gui/assets/app.css b/internal/gui/assets/app.css index 678aee06..cdb1a175 100644 --- a/internal/gui/assets/app.css +++ b/internal/gui/assets/app.css @@ -2006,6 +2006,8 @@ input.words { width: 240px; max-width: 100%; } .row.pref.rule-row { flex-wrap: wrap; row-gap: 8px; } .row.pref.rule-row .who { flex: 1 1 100%; } .row.pref.rule-row .sub { white-space: normal; } +/* an update that failed says why in full (the version row) */ +.row .sub.wraps { white-space: normal; overflow-wrap: anywhere; } .prefs .val.rule-add { flex: 1 1 100%; max-width: none; margin-left: 0; gap: 6px; } input.words.rule-kind { flex: none; width: 110px; } input.words.rule-match { flex: 1 1 auto; width: auto; min-width: 0; } diff --git a/internal/gui/assets/app.js b/internal/gui/assets/app.js index 75282c8d..c4bdcee1 100644 --- a/internal/gui/assets/app.js +++ b/internal/gui/assets/app.js @@ -12185,6 +12185,7 @@ async function renderUpdate(r, u) { const who = r.querySelector(".who"), val = r.querySelector(".val"); const sub = who.querySelector(".sub") || who.appendChild(el("div", "sub")); sub.title = ""; + sub.classList.remove("wraps"); for (const b of val.querySelectorAll("button")) b.remove(); const btn = (label, fn, dim) => { const b = el("button", "text" + (dim ? " busy" : ""), label); @@ -12220,6 +12221,10 @@ async function renderUpdate(r, u) { switch (u.state) { case "ready": sub.textContent = t("{v} is downloaded", { v: u.latest }) + (u.error ? " · " + u.error : ""); + // a failed install says why in full, and offers the release page to + // put the new version in by hand + sub.title = u.error || ""; + if (u.error) sub.classList.add("wraps"); // back with an answer only when it didn't restart btn(t("Restart to update"), async () => { const a = await api("update/install", installFrom()).catch(() => ({ state: "error" })); @@ -12227,6 +12232,7 @@ async function renderUpdate(r, u) { sub.textContent = t("Restarting…"); backAsNew(u.current); }); + if (u.error && u.url) btn(t("Download"), () => (web ? window.open(u.url, "_blank", "noopener") : api("open", { url: u.url }))); break; case "available": sub.textContent = t("{v} is out", { v: u.latest }); @@ -12252,6 +12258,7 @@ async function renderUpdate(r, u) { // the reason in sight: "timed out" says try a proxy, a 404 says wait sub.textContent = t(u.latest ? "Couldn't download {v}" : "Couldn't check for updates", { v: u.latest }) + (u.error ? " · " + u.error.replace(/^Get "[^"]*": /, "") : ""); sub.title = u.error || ""; + if (u.error) sub.classList.add("wraps"); btn(t("Check"), check); break; case "": // not asked yet: with automatic updates off, only this asks diff --git a/internal/gui/tests/update-check.test.cjs b/internal/gui/tests/update-check.test.cjs index c721e993..84985759 100644 --- a/internal/gui/tests/update-check.test.cjs +++ b/internal/gui/tests/update-check.test.cjs @@ -223,6 +223,35 @@ for (const engine of (process.env.BROWSER ? [process.env.BROWSER] : ["chromium", assert.equal(ctl.posts, 0, "watching a check already under way does not start another"); assert.deepEqual(errors, []); }); + + // freecss on Discord: "0.1.628 已下载 · couldn't move + // magpie-windows-amd64.exe aside to put the new version in: rename C:\Us…" + // — the reason was cut off. It is read in full, and the release page is + // a click away. + await t.test(lang + ": an install that failed says why in full", async () => { + const error = "couldn't move magpie-windows-amd64.exe aside to put the new version in: The process cannot access the file because it is being used by another process; another program has it open (often an antivirus or OneDrive): let magpie through it and restart to update again, or download the new version and put it in place of this one"; + const url = "https://github.com/yetone/magpie-releases/releases/tag/v0.1.628"; + const ctl = fresh({ update: { state: "ready", current: "0.1.627", latest: "0.1.628", error, url } }); + const { page, errors } = await open(lang, ctl); + const opened = []; + await page.route("**/api/open", async (route) => { opened.push(route.request().postDataJSON()); await route.fulfill({ json: {} }); }); + const row = rowOf(page, w.version); + const sub = row.locator(".sub"); + await page.waitForFunction((e) => document.querySelector("#about .row.pref .sub")?.textContent.includes(e), error); + assert.equal(await sub.getAttribute("title"), error); + const cut = await sub.evaluate((s) => s.scrollWidth > s.clientWidth + 1); + assert.equal(cut, false, "the reason must not be cut off"); + const download = row.locator("button", { hasText: lang === "zh" ? "下载" : "Download" }); + await download.click(); + assert.deepEqual(opened, [{ url }]); + + // a later state is one line again + ctl.update = { state: "latest", current: "0.1.628" }; + await page.evaluate(() => { void renderUpdate(document.querySelector("#about .row.pref")); }); + await page.waitForFunction((latest) => document.querySelector("#about .row.pref .sub")?.textContent.trim() === latest, w.latest); + assert.equal(await sub.evaluate((s) => s.classList.contains("wraps")), false); + assert.deepEqual(errors, []); + }); } }); } diff --git a/internal/update/aside_test.go b/internal/update/aside_test.go new file mode 100644 index 00000000..308ff26f --- /dev/null +++ b/internal/update/aside_test.go @@ -0,0 +1,79 @@ +package update + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// fakeRename has renameFile fail with err the first n times, then rename. +func fakeRename(t *testing.T, n int, err error) *int { + calls := 0 + waits, rename := asideWaits, renameFile + asideWaits = []time.Duration{time.Millisecond, time.Millisecond, time.Millisecond} + renameFile = func(from, to string) error { + calls++ + if calls <= n { + return &os.LinkError{Op: "rename", Old: from, New: to, Err: err} + } + return os.Rename(from, to) + } + t.Cleanup(func() { asideWaits, renameFile = waits, rename }) + return &calls +} + +// Something holding the running exe open for a moment (an antivirus scan, +// OneDrive) doesn't fail the update: moving it aside is tried again. +func TestMoveAsideTriesAgain(t *testing.T) { + exe := filepath.Join(t.TempDir(), "magpie-windows-amd64.exe") + os.WriteFile(exe, []byte("v1"), 0o755) + calls := fakeRename(t, 2, errSharingViolation) + if err := moveAside(exe); err != nil { + t.Fatal(err) + } + if *calls != 3 { + t.Fatalf("%d tries, want 3", *calls) + } + if b, _ := os.ReadFile(exe + ".old"); string(b) != "v1" { + t.Fatal("not moved to .old") + } +} + +// One that keeps it says why and what to do, without the paths the +// version row has no room for. +func TestMoveAsideSaysWhy(t *testing.T) { + exe := filepath.Join(t.TempDir(), "magpie-windows-amd64.exe") + os.WriteFile(exe, []byte("v1"), 0o755) + calls := fakeRename(t, 100, errSharingViolation) + err := moveAside(exe) + if err == nil { + t.Fatal("moved") + } + if *calls != 4 { + t.Fatalf("%d tries, want 4", *calls) + } + msg := err.Error() + for _, want := range []string{"couldn't move magpie-windows-amd64.exe aside", strings.TrimRight(errSharingViolation.Error(), ". 。"), "another program has it open", "download the new version"} { + if !strings.Contains(msg, want) { + t.Errorf("%q lacks %q", msg, want) + } + } + if strings.Contains(msg, exe) { + t.Errorf("%q has the path", msg) + } + + // a folder magpie may not change says so + fakeRename(t, 100, os.ErrPermission) + if msg := moveAside(exe).Error(); !strings.Contains(msg, "doesn't let magpie change files in "+filepath.Dir(exe)) { + t.Errorf("%q", msg) + } + + // gone is not tried again + calls = fakeRename(t, 100, os.ErrNotExist) + moveAside(exe) + if *calls != 1 { + t.Fatalf("%d tries of a missing exe", *calls) + } +} diff --git a/internal/update/install_windows_test.go b/internal/update/install_windows_test.go index d3dcdb25..7fc42c2e 100644 --- a/internal/update/install_windows_test.go +++ b/internal/update/install_windows_test.go @@ -4,7 +4,10 @@ import ( "os" "os/exec" "path/filepath" + "strings" + "syscall" "testing" + "time" ) // What the last update moved aside can still be running (a `magpie serve` @@ -43,3 +46,48 @@ func TestInstallBinaryBesideARunningOld(t *testing.T) { t.Fatal("the running exe was not moved beside the old:", err) } } + +// holdOpen opens path as an antivirus scan does: reading, without letting +// it be renamed or removed (no FILE_SHARE_DELETE). +func holdOpen(t *testing.T, path string) syscall.Handle { + p, _ := syscall.UTF16PtrFromString(path) + h, err := syscall.CreateFile(p, syscall.GENERIC_READ, syscall.FILE_SHARE_READ|syscall.FILE_SHARE_WRITE, nil, syscall.OPEN_EXISTING, 0, 0) + if err != nil { + t.Fatal(err) + } + return h +} + +// Held open for a moment, the exe still goes aside and the new version in; +// held for good, the update says what holds it up and keeps the download. +func TestInstallBinaryHeldOpen(t *testing.T) { + dir := t.TempDir() + exe := filepath.Join(dir, "magpie-windows-amd64.exe") + staged := exe + ".new" + os.WriteFile(exe, []byte("old version"), 0o755) + os.WriteFile(staged, []byte("new version"), 0o755) + + h := holdOpen(t, exe) + if os.Rename(exe, exe+".x") == nil { + t.Fatal("a held exe could be renamed; the test proves nothing") + } + go func() { time.Sleep(500 * time.Millisecond); syscall.CloseHandle(h) }() + if err := InstallBinary(staged, exe); err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(exe); string(got) != "new version" { + t.Fatalf("exe is %q", got) + } + + os.WriteFile(staged, []byte("newer version"), 0o755) + h = holdOpen(t, exe) + defer syscall.CloseHandle(h) + err := InstallBinary(staged, exe) + if err == nil || !strings.Contains(err.Error(), "another program has it open") { + t.Fatalf("held for good: %v", err) + } + t.Log(err) + if _, err := os.Stat(staged); err != nil { + t.Fatal("the download was not kept for another try:", err) + } +} diff --git a/internal/update/update.go b/internal/update/update.go index 5c7bda2a..eb8f1cfe 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -17,6 +17,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "os" "path/filepath" @@ -24,6 +25,7 @@ import ( "runtime" "strconv" "strings" + "syscall" "time" "github.com/yetone/magpie/internal/appdir" @@ -380,8 +382,8 @@ func InstallBinary(staged, exe string) error { // replaced then: this one goes beside it, under a name of its own. // The download is kept, for another try. RemoveOld(exe) - if err := os.Rename(exe, oldName(exe)); err != nil { - return fmt.Errorf("couldn't move %s aside to put the new version in: %w", filepath.Base(exe), err) + if err := moveAside(exe); err != nil { + return err } } if err := os.Rename(staged, exe); err != nil { @@ -393,6 +395,56 @@ func InstallBinary(staged, exe string) error { return nil } +// renameFile is os.Rename; tests make it fail. +var renameFile = os.Rename + +// asideWaits are the pauses between tries at moving the running exe aside: +// an antivirus scanning it or a sync client (OneDrive) reading it holds it +// open, which Windows won't rename under, for a moment, and lets go. +var asideWaits = []time.Duration{100 * time.Millisecond, 200 * time.Millisecond, 400 * time.Millisecond, 800 * time.Millisecond, 1500 * time.Millisecond} + +// Windows' errors for a file another program holds open without letting +// it be renamed. +const ( + errSharingViolation syscall.Errno = 32 + errLockViolation syscall.Errno = 33 +) + +// moveAside moves the running exe out of the way of the new version, under +// a name of its own, trying again for a few seconds while something has it +// open. What it says when it can't is the reason and what to do: the error +// is shown as is in the version row. +func moveAside(exe string) error { + var err error + for i := 0; ; i++ { + if err = renameFile(exe, oldName(exe)); err == nil { + return nil + } + if errors.Is(err, fs.ErrNotExist) || i == len(asideWaits) { + break + } + if i == 0 { + os.Chmod(exe, 0o755) // a read-only exe: the attribute off + } + time.Sleep(asideWaits[i]) + } + why := err.Error() + var le *os.LinkError + if errors.As(err, &le) { + why = le.Err.Error() // the paths are known; the reason is what's new + } + var hint string + switch { + case errors.Is(err, errSharingViolation), errors.Is(err, errLockViolation): + hint = "another program has it open (often an antivirus or OneDrive): let magpie through it and restart to update again, or download the new version and put it in place of this one" + case errors.Is(err, fs.ErrPermission): + hint = "Windows doesn't let magpie change files in " + filepath.Dir(exe) + " (an antivirus' folder protection, or the folder's permissions): let magpie through, or download the new version and put it in place of this one" + default: + hint = "download the new version and put it in place of this one" + } + return fmt.Errorf("couldn't move %s aside to put the new version in: %s; %s", filepath.Base(exe), strings.TrimRight(why, ". 。"), hint) +} + // oldName is where a running exe is moved aside to: exe.old, or when // that is still there (in use), exe.old-2, exe.old-3… func oldName(exe string) string {