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 {