diff --git a/client/src/App.vue b/client/src/App.vue index 49818a4..30c81ae 100644 --- a/client/src/App.vue +++ b/client/src/App.vue @@ -155,7 +155,15 @@ {{ this.computeTransferButtonText() }} - + +

Config has been updated

+ Set Serial Number + Cancel diff --git a/client/src/components/TheOptions.vue b/client/src/components/TheOptions.vue index c7cd0a9..3d3168f 100644 --- a/client/src/components/TheOptions.vue +++ b/client/src/components/TheOptions.vue @@ -58,12 +58,19 @@

Actions

+
- - Reboot now + + {{ pending === "reboot" ? "Reboot?" : "Reboot now" }} - Shut down{{ + pending === "shutdown" ? "Shut down?" : "Shut down" + }}
@@ -84,6 +91,24 @@ export default { this.setOption(data); this.$emit("set-option", name, value); }, + // First click arms the button, second one within a few seconds does it. + // Arming one disarms the other, so a click meant for Reboot cannot confirm + // a pending Shut down. + confirmAction(which) { + clearTimeout(this.pendingTimer); + if (this.pending !== which) { + this.pending = which; + this.pendingTimer = setTimeout(() => (this.pending = null), 4000); + return; + } + this.pending = null; + this.$emit(which === "reboot" ? "reboot-board" : "shutdown-board"); + }, + }, + // A drawer that is closed and reopened must not still be holding an armed + // button from minutes ago. + beforeUnmount() { + clearTimeout(this.pendingTimer); }, computed: mapGetters(["options"]), created() { @@ -95,6 +120,9 @@ export default { open: Boolean, }, data: () => ({ + // "reboot", "shutdown", or null when neither is armed. + pending: null, + pendingTimer: null, radioItems: [ { label: "Normal", value: 0 }, { label: "90 degrees", value: 90 }, diff --git a/client/tests/unit/InstallGating.spec.js b/client/tests/unit/InstallGating.spec.js index 1ef97c4..a179d31 100644 --- a/client/tests/unit/InstallGating.spec.js +++ b/client/tests/unit/InstallGating.spec.js @@ -169,3 +169,39 @@ describe('IntegrityChecker reports its verdict', () => { expect(w.vm.spinner_visible).toBe(false); }); }); + +// The badge describes one specific file. Clearing the selection - which is what +// deleting the selected image does - has to clear the verdict with it, or a +// green check from the file that is now gone sits next to "Please select one" +// and reads as "the thing I am about to install is fine" (#162). +describe('the integrity verdict belongs to the selected image (#162)', () => { + function stand(selected) { + const fileSelected = vi.fn(); + const self = { + selectedLocalImage: selected, + imageIntegrity: true, + $refs: { integritychecker: { fileSelected } }, + }; + return { self, fileSelected }; + } + + it('tells the checker when the selection is cleared', () => { + const { self, fileSelected } = stand(null); + + App.methods.onSelectedFileChanged.call(self); + + expect(self.imageIntegrity).toBe(null); + // Called with the empty selection rather than skipped: fileSelected() + // hides the icon for an empty name, and skipping the call is exactly what + // left the old verdict on screen. + expect(fileSelected).toHaveBeenCalledWith(null); + }); + + it('still checks a newly selected image', () => { + const { self, fileSelected } = stand('fresh.img.xz'); + + App.methods.onSelectedFileChanged.call(self); + + expect(fileSelected).toHaveBeenCalledWith('fresh.img.xz'); + }); +}) diff --git a/client/tests/unit/ProgressBar.spec.js b/client/tests/unit/ProgressBar.spec.js index 038548e..02d5fbf 100644 --- a/client/tests/unit/ProgressBar.spec.js +++ b/client/tests/unit/ProgressBar.spec.js @@ -96,3 +96,50 @@ describe('ProgressBar board metrics hover', () => { expect(w.vm.metricsVisible).toBe(false); }); }); + +// The countdown is a projection off a clock, so it can run backwards: a start +// time in the future (the board and the browser do not share one), or a +// progress figure at 100 while the estimate is still being recomputed. Neither +// is a reason to show the user "0m:-4s". +describe('the remaining time is never negative', () => { + function mountWith(progressModel) { + return shallowMount(ProgressBar, { + props: { revision: 'A8' }, + global: { + mocks: { $store: { getters: { progress: progressModel } } }, + stubs: { 'w-progress': true, 'w-flex': true, TheMetrics: true } + } + }); + } + + it('clamps a start time in the future', () => { + const w = mountWith({ progress: 50, bandwidth: 1.5, timeStarted: Date.now() + 60000 }); + w.vm.update(); + expect(w.vm.secondsR).toBeGreaterThanOrEqual(0); + expect(w.vm.minutesR).toBeGreaterThanOrEqual(0); + expect(w.vm.seconds).toBeGreaterThanOrEqual(0); + expect(w.vm.minutes).toBeGreaterThanOrEqual(0); + }); + + it('clamps a progress figure past 100', () => { + const w = mountWith({ progress: 140, bandwidth: 8, timeStarted: Date.now() - 30000 }); + w.vm.update(); + expect(w.vm.secondsR).toBeGreaterThanOrEqual(0); + expect(w.vm.minutesR).toBeGreaterThanOrEqual(0); + }); + + it('reports zero rather than NaN before any progress has been made', () => { + const w = mountWith({ progress: 0, bandwidth: 0, timeStarted: Date.now() }); + w.vm.update(); + expect(w.vm.secondsR).toBe(0); + expect(w.vm.minutesR).toBe(0); + }); + + it('still reports a real estimate for an ordinary transfer', () => { + // Half done after 60s, so about 60s left. + const w = mountWith({ progress: 50, bandwidth: 5, timeStarted: Date.now() - 60000 }); + w.vm.update(); + expect(w.vm.minutesR * 60 + w.vm.secondsR).toBeGreaterThan(50); + expect(w.vm.minutesR * 60 + w.vm.secondsR).toBeLessThan(70); + }); +}); diff --git a/client/tests/unit/TheOptions.spec.js b/client/tests/unit/TheOptions.spec.js index 84e4172..b2554cc 100644 --- a/client/tests/unit/TheOptions.spec.js +++ b/client/tests/unit/TheOptions.spec.js @@ -59,3 +59,57 @@ describe('TheOptions', () => { expect(binding[1]).toBe('screenRotation') }) }) + +// Reboot and Shut down sit next to each other and used to fire on one click. +// The two outcomes are not equally cheap: a stray Shut down on a headless board +// needs a person to go and power-cycle it (#163). +describe('TheOptions destructive actions', () => { + it('does not reboot or shut down on the first click', () => { + const { wrapper } = mountOptions() + + wrapper.vm.confirmAction('reboot') + wrapper.vm.confirmAction('shutdown') + + expect(wrapper.emitted('reboot-board')).toBeUndefined() + expect(wrapper.emitted('shutdown-board')).toBeUndefined() + }) + + it('acts on the second click of the same button', () => { + const { wrapper } = mountOptions() + + wrapper.vm.confirmAction('reboot') + wrapper.vm.confirmAction('reboot') + + expect(wrapper.emitted('reboot-board')).toHaveLength(1) + expect(wrapper.emitted('shutdown-board')).toBeUndefined() + }) + + // Arming one has to disarm the other, or a click meant for Reboot would + // confirm a Shut down that was armed moments earlier - the precise mistake + // the confirmation exists to prevent. + it('arming the other button cancels the pending one', () => { + const { wrapper } = mountOptions() + + wrapper.vm.confirmAction('reboot') + wrapper.vm.confirmAction('shutdown') + wrapper.vm.confirmAction('shutdown') + + expect(wrapper.emitted('reboot-board')).toBeUndefined() + expect(wrapper.emitted('shutdown-board')).toHaveLength(1) + }) + + it('disarms itself after a few seconds so a stale click does not act', () => { + vi.useFakeTimers() + try { + const { wrapper } = mountOptions() + + wrapper.vm.confirmAction('reboot') + vi.advanceTimersByTime(5000) + wrapper.vm.confirmAction('reboot') + + expect(wrapper.emitted('reboot-board')).toBeUndefined() + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/reflash/download_test.go b/reflash/download_test.go index 4f99927..dd94bf1 100644 --- a/reflash/download_test.go +++ b/reflash/download_test.go @@ -63,17 +63,24 @@ func TestDownloadCancelStopsTheTransfer(t *testing.T) { startTestDownload(t, srv.URL, "slow.img.xz", 64<<20) path := filepath.Join(images_folder, "slow.img.xz") + // The bytes land in the partial now, and only become the real name once + // they are all there (#159) - so that is what says the transfer is under + // way. + part := path + ".part" waitFor(t, func() bool { - fi, err := os.Stat(path) + fi, err := os.Stat(part) return err == nil && fi.Size() > 0 }) cancelDownload(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/cancel_download", nil)) waitForState(t, CANCELLED) waitFor(t, disconnected.Load) - if _, err := os.Stat(path); !os.IsNotExist(err) { + if _, err := os.Stat(part); !os.IsNotExist(err) { t.Errorf("the cancelled download is still on the drive, stat err %v", err) } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("a cancelled download must not leave an image under the real name, stat err %v", err) + } got, _ := os.ReadFile(mounts) if !strings.HasSuffix(strings.TrimSpace(string(got)), "mounted "+MODE_RO) { t.Errorf("drive not left read-only; mount calls %q", got) diff --git a/reflash/server.go b/reflash/server.go index c819c69..a6929c2 100644 --- a/reflash/server.go +++ b/reflash/server.go @@ -914,25 +914,26 @@ func startDownload(w http.ResponseWriter, r *http.Request) { func goDownload(ctx context.Context, filename string, url string) { disarmReboot() path := images_folder + "/" + filename + part := partialPath(filename) // Every way out removes what was written and puts the drive back to // read-only. This used to panic on a failed create or request, taking the // server down with it. fail := func(what string, err error) { logError(what + ": " + err.Error()) - os.Remove(path) + os.Remove(part) mountUsb(MODE_RO) state.State = ERROR state.Error = "The download failed: " + err.Error() } cancelled := func() { logInfo("Download cancelled.") - os.Remove(path) + os.Remove(part) mountUsb(MODE_RO) state.State = CANCELLED } - out, err := os.Create(path) + out, err := os.Create(part) if err != nil { fail("Could not create "+filename, err) return @@ -982,6 +983,13 @@ func goDownload(ctx context.Context, filename string, url string) { return } + // All the bytes are here, so the new image earns the real name now. Until + // this rename the drive still holds whatever it held before (#159). + if err := os.Rename(part, path); err != nil { + fail("Could not put "+filename+" in place", err) + return + } + duration := time.Since(timeStart) logInfo(fmt.Sprintf("Download finished in %d minutes and %d seconds", int(duration.Minutes()), int(duration.Seconds())%60)) mountUsb(MODE_RO) @@ -1022,7 +1030,7 @@ func uploadStart(w http.ResponseWriter, r *http.Request) { timeStart = time.Now() logInfo("Starting upload at " + timeStart.Format("15:04:05")) logInfo("Filename: " + state.Filename) - f, err := os.Create(images_folder + "/" + state.Filename) + f, err := os.Create(partialPath(state.Filename)) if err != nil { // Report the failure instead of log.Fatal. This runs on the USB // drive, so a full disk or a drive that dropped off the bus is an @@ -1508,6 +1516,16 @@ func uploadFinish(w http.ResponseWriter, r *http.Request) { } state.File = nil } + // Everything is written and flushed, so the upload earns the real name. Any + // image already on the drive under that name survived until this point + // (#159). + if err := os.Rename(partialPath(state.Filename), images_folder+"/"+state.Filename); err != nil { + logError("Could not put " + state.Filename + " in place: " + err.Error()) + state.Error = "The image was uploaded but could not be saved to the USB drive." + mountUsb(MODE_RO) + state.State = ERROR + return + } mountUsb(MODE_RO) duration := time.Since(timeStart) logInfo(fmt.Sprintf("Upload finished in %d minutes and %d seconds", int(duration.Minutes()), int(duration.Seconds())%60)) @@ -1553,9 +1571,11 @@ func uploadCancel(w http.ResponseWriter, r *http.Request) { } if !magic { // A cancelled or failed upload is a truncated image; left in the list - // it could only ever fail its integrity check (#153). + // it could only ever fail its integrity check (#153). Only the partial + // goes - an image already on the drive under the same name was never + // touched, and removing the final name here destroyed it (#159). if state.Filename != "" { - os.Remove(images_folder + "/" + state.Filename) + os.Remove(partialPath(state.Filename)) } mountUsb(MODE_RO) } @@ -1758,7 +1778,9 @@ func refreshProgress() { } state.BytesNow = i } else if state.State == DOWNLOADING { - fi, err := os.Stat(images_folder + "/" + state.Filename) + // The partial, not the final name: that is where the bytes are landing + // until the download completes and renames it into place (#159). + fi, err := os.Stat(partialPath(state.Filename)) if err == nil { state.BytesNow = int(fi.Size()) } @@ -1839,6 +1861,16 @@ func getProgress(w http.ResponseWriter, r *http.Request) { } } +// partialPath is where a download or an upload writes until it has every byte. +// Both used to write straight onto the final name, which os.Create truncates - +// so starting a transfer of an image the drive already held destroyed it before +// a single new byte arrived, and cancelling (or any failure) left nothing at +// all. The suffix keeps it out of getLocalImages' *.img.xz glob, so a partial +// never appears in the install list (#159). +func partialPath(filename string) string { + return images_folder + "/" + filename + ".part" +} + func getLocalImages() []Image { entries, err := filepath.Glob(images_folder + "/*.img.xz") if err != nil { diff --git a/reflash/server_test.go b/reflash/server_test.go index dd5a31c..6d6e65a 100644 --- a/reflash/server_test.go +++ b/reflash/server_test.go @@ -1785,3 +1785,128 @@ func TestRunCommand2TimeoutPassesOutputThrough(t *testing.T) { t.Errorf("stdout = %q, want \"hello\"", strings.TrimSpace(out)) } } + +// An image the drive already holds has to survive a transfer of the same name +// until every new byte has arrived. Both paths wrote straight onto the final +// name, so os.Create truncated the existing image on the first write and the +// cleanup on cancel then removed what was left - pressing Download or Upload on +// an image you already had and changing your mind destroyed it (#159). +func TestCancelledDownloadKeepsTheImageAlreadyOnTheDrive(t *testing.T) { + setupTest(t) + state = &State{State: IDLE} + + filename := "keepme.img.xz" + final := filepath.Join(images_folder, filename) + original := []byte("the image the user already had") + if err := os.WriteFile(final, original, 0o644); err != nil { + t.Fatal(err) + } + + // Holds the response open so the cancel lands mid-transfer rather than + // after the copy has already finished. + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("replacement bytes")) + w.(http.Flusher).Flush() + <-release + })) + // Deferred after srv.Close so LIFO releases the handler first: closing the + // server while a request is still parked on the channel deadlocks, and a + // failed assertion below would otherwise hang the whole test binary rather + // than reporting. + defer srv.Close() + defer close(release) + + body, _ := json.Marshal(map[string]any{ + "filename": filename, "url": srv.URL, "size": 1 << 20, "start_time": 0, + }) + startDownload(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/start_download", bytes.NewReader(body))) + + // Wait until bytes are actually landing, so this tests a cancel during the + // transfer and not a race with its start. + waitFor(t, func() bool { + fi, err := os.Stat(final + ".part") + return err == nil && fi.Size() > 0 + }) + + cancelDownload(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/cancel_download", nil)) + waitFor(t, func() bool { return state.State == CANCELLED }) + + got, err := os.ReadFile(final) + if err != nil { + t.Fatalf("the image the user already had is gone: %v", err) + } + if !bytes.Equal(got, original) { + t.Errorf("existing image was modified: got %q, want %q", got, original) + } + if _, err := os.Stat(final + ".part"); !os.IsNotExist(err) { + t.Error("the partial file was left behind") + } +} + +// The other half: a download that completes does replace the old image, and +// leaves no partial file in the images folder. +func TestCompletedDownloadReplacesTheImage(t *testing.T) { + setupTest(t) + state = &State{State: IDLE} + + filename := "replaceme.img.xz" + final := filepath.Join(images_folder, filename) + if err := os.WriteFile(final, []byte("stale"), 0o644); err != nil { + t.Fatal(err) + } + + fresh := []byte("a complete new image") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(fresh) + })) + defer srv.Close() + + body, _ := json.Marshal(map[string]any{ + "filename": filename, "url": srv.URL, "size": len(fresh), "start_time": 0, + }) + startDownload(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/start_download", bytes.NewReader(body))) + waitFor(t, func() bool { return state.State == FINISHED }) + + got, err := os.ReadFile(final) + if err != nil { + t.Fatalf("reading the downloaded image: %v", err) + } + if !bytes.Equal(got, fresh) { + t.Errorf("image content = %q, want %q", got, fresh) + } + if _, err := os.Stat(final + ".part"); !os.IsNotExist(err) { + t.Error("the partial file was left behind") + } +} + +// Same guarantee for the upload path, which had the same os.Create. +func TestCancelledUploadKeepsTheImageAlreadyOnTheDrive(t *testing.T) { + setupTest(t) + state = &State{State: IDLE} + + filename := "keepme-upload.img.xz" + final := filepath.Join(images_folder, filename) + original := []byte("the image the user already had") + if err := os.WriteFile(final, original, 0o644); err != nil { + t.Fatal(err) + } + + startBody, _ := json.Marshal(map[string]any{ + "filename": filename, "size": 1 << 20, "start_time": 0, + }) + uploadStart(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/upload_start", bytes.NewReader(startBody))) + uploadChunk(httptest.NewRecorder(), httptest.NewRequest("POST", "/api/upload_chunk", bytes.NewReader([]byte("partial bytes")))) + uploadCancel(httptest.NewRecorder(), httptest.NewRequest("PUT", "/api/upload_cancel", nil)) + + got, err := os.ReadFile(final) + if err != nil { + t.Fatalf("the image the user already had is gone: %v", err) + } + if !bytes.Equal(got, original) { + t.Errorf("existing image was modified: got %q, want %q", got, original) + } + if _, err := os.Stat(final + ".part"); !os.IsNotExist(err) { + t.Error("the partial file was left behind") + } +}