diff --git a/.changeset/superseded-selection-recovery.md b/.changeset/superseded-selection-recovery.md new file mode 100644 index 000000000..7abb47e6a --- /dev/null +++ b/.changeset/superseded-selection-recovery.md @@ -0,0 +1,9 @@ +--- +"ftw": patch +--- + +When a release's newer driver runs instead of a kept older selection, a failed +trial of another version now returns the device to the release's driver. Before, +the recovery restarted a file that no longer ran and left the device stopped, or +made the kept older version run. The kept version now offers "Use this" under +Settings › Devices, so the owner can go back to it. diff --git a/go/internal/api/api_device_repository.go b/go/internal/api/api_device_repository.go index 6abfae2fb..f829e33ed 100644 --- a/go/internal/api/api_device_repository.go +++ b/go/internal/api/api_device_repository.go @@ -303,19 +303,27 @@ func (s *Server) handleDeviceRepositoryVersions(w http.ResponseWriter, r *http.R logicalPath = candidate.Driver.Path } } - release, chosen := "", "" + // A selection the release's newer copy has overtaken is kept but does + // not run, so the picker offers it like any version on disk. + release, chosen, superseded := "", "", "" if logicalPath != "" { release = s.deps.DriverRepository.ReleaseVersion(logicalPath) for _, installed := range versions { - if installed.Active && s.deps.DriverRepository.Chosen(logicalPath, installed.Version) { + if !installed.Active { + continue + } + if s.deps.DriverRepository.Chosen(logicalPath, installed.Version) { chosen = installed.Version } + if !s.deps.DriverRepository.Runs(installed) { + superseded = installed.Version + } } } writeJSON(w, 200, map[string]any{ "driver_id": r.PathValue("id"), "installed": versions, "available": available, "logical_path": logicalPath, "release_version": release, "chosen_version": chosen, - "release_source": s.bundledSource(), + "superseded_version": superseded, "release_source": s.bundledSource(), }) } @@ -362,8 +370,11 @@ func (s *Server) handleDeviceRepositoryActivate(w http.ResponseWriter, r *http.R if restartErr != nil { recoveryErr := error(nil) if original != nil { + // A rollback restores the selection and the choice that went + // with it. Activating the original again would pin a kept + // version the release had overtaken, and run it instead. var recovered state.DriverRepoInstall - recovered, recoveryErr = s.deps.DriverRepository.ActivateInstalled(driverID, original.Version, original.SHA256) + recovered, recoveryErr = s.deps.DriverRepository.Rollback(activated.LogicalPath) if recoveryErr == nil { _, recoveryErr = s.restartManagedDriversExpected(context.Background(), recovered, restartState.ExpectedIdentities) } @@ -524,7 +535,10 @@ func (s *Server) restartManagedDriversExpected(ctx context.Context, artifact sta rel := filepath.FromSlash(strings.TrimPrefix(artifact.LogicalPath, "drivers/")) activePath := filepath.Join(s.managedDriverDir(), rel) targetPath := activePath - if artifact.InstalledPath == "" { + // No managed file runs at this path either when nothing is selected + // (UseBundled) or when the release's newer copy overtakes the selection; + // the release's own file runs then. + if _, err := os.Stat(activePath); artifact.InstalledPath == "" || err != nil { var err error targetPath, err = s.bundledDriverFor(artifact.DriverID, artifact.LogicalPath) if err != nil { diff --git a/go/internal/api/api_device_repository_test.go b/go/internal/api/api_device_repository_test.go index 57d026564..87530bce0 100644 --- a/go/internal/api/api_device_repository_test.go +++ b/go/internal/api/api_device_repository_test.go @@ -497,7 +497,12 @@ func TestDriverCatalogNamesTheDriversThatRunEachFile(t *testing.T) { // drivers/. func (f *driverUpdateFixture) publishAs(id, filename, version string) { f.t.Helper() - source := []byte(strings.Replace(string(updateDriverLua(version, "P1-123", `host.emit("meter", {w=103})`)), + f.publishSourceAs(id, filename, version, "P1-123", `host.emit("meter", {w=103})`) +} + +func (f *driverUpdateFixture) publishSourceAs(id, filename, version, serial, poll string) { + f.t.Helper() + source := []byte(strings.Replace(string(updateDriverLua(version, serial, poll)), `id = "esphome-dsmr"`, `id = "`+id+`"`, 1)) f.mu.Lock() f.lua = source @@ -633,3 +638,76 @@ func TestVersionsShowTheReleaseAndTheOwnersChoice(t *testing.T) { } } } + +// A selection a newer release has overtaken is kept but does not run. A +// failed trial of another version must come back to what ran -- the +// release's copy -- and must not make the kept selection run instead. +func TestFailedTrialOverASupersededSelectionReturnsToTheRelease(t *testing.T) { + f := newDriverUpdateFixture(t, "running") + release := func(version string, watts int) { + t.Helper() + source := strings.Replace(string(updateDriverLua(version, "P1-123", fmt.Sprintf(`host.emit("meter", {w=%d})`, watts))), + `id = "esphome-dsmr"`, `id = "esphome_dsmr"`, 1) + if err := os.WriteFile(f.bundled, []byte(source), 0o644); err != nil { + t.Fatal(err) + } + } + release("1.0.2", 102) + f.publishAs("esphome_dsmr", "esphome_dsmr.lua", "1.0.3") + f.requestFor("esphome_dsmr", "install", `{"repository_id":"test"}`, 200) + f.reading(103) + + // A Core update brings 1.0.4: the 1.0.3 selection stays but no longer runs. + release("1.0.4", 104) + f.s.deps.DriverRepository.ApplyBundled() + f.s.deps.Cfg.Drivers[0].Lua = f.bundled + if err := f.s.deps.Registry.Restart(context.Background(), f.s.deps.Cfg.Drivers[0]); err != nil { + t.Fatal(err) + } + f.reading(104) + w := httptest.NewRecorder() + f.s.Handler().ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/api/device_repository/drivers/esphome_dsmr/versions", nil)) + var versions struct { + Superseded string `json:"superseded_version"` + } + if w.Code != 200 || json.Unmarshal(w.Body.Bytes(), &versions) != nil || versions.Superseded != "1.0.3" { + t.Fatalf("versions: HTTP %d %s; want the kept 1.0.3 named as overtaken", w.Code, w.Body.String()) + } + + stillTheRelease := func(action string) { + t.Helper() + f.reading(104) + if got := f.s.activeManagedDriverVersion("esphome_dsmr"); got != "1.0.3" { + t.Fatalf("after the failed %s the kept selection is %q, want 1.0.3", action, got) + } + if f.s.deps.DriverRepository.Chosen("drivers/esphome_dsmr.lua", "1.0.3") { + t.Fatalf("the failed %s turned the kept 1.0.3 into a choice that runs over the release", action) + } + if _, err := os.Stat(filepath.Join(f.s.managedDriverDir(), "esphome_dsmr.lua")); !os.IsNotExist(err) { + t.Fatalf("after the failed %s a managed file runs instead of the release's (stat err %v)", action, err) + } + } + + // A version for another meter fails its identity check. + f.publishSourceAs("esphome_dsmr", "esphome_dsmr.lua", "1.0.5", "OTHER-METER", `host.emit("meter", {w=105})`) + response := f.requestFor("esphome_dsmr", "install", `{"repository_id":"test"}`, 502) + if msg, _ := response["error"].(string); strings.Contains(msg, "rollback failed") { + t.Fatalf("install recovery failed: %s", msg) + } + stillTheRelease("install") + + // The failed version is on disk now; switching to it fails the same way. + response = f.requestFor("esphome_dsmr", "activate", `{"version":"1.0.5"}`, 502) + if msg, _ := response["error"].(string); strings.Contains(msg, "recovery failed") { + t.Fatalf("activate recovery failed: %s", msg) + } + stillTheRelease("activate") + + // The owner can still go back to the kept version on purpose, and it + // then stays as a choice over the release's newer copy. + f.requestFor("esphome_dsmr", "activate", `{"version":"1.0.3"}`, 200) + f.reading(103) + if !f.s.deps.DriverRepository.Chosen("drivers/esphome_dsmr.lua", "1.0.3") { + t.Fatal("going back to the kept 1.0.3 was not recorded as the owner's choice") + } +} diff --git a/go/internal/driverrepo/manager.go b/go/internal/driverrepo/manager.go index 0edbbdf27..127ea11c8 100644 --- a/go/internal/driverrepo/manager.go +++ b/go/internal/driverrepo/manager.go @@ -960,6 +960,16 @@ func (m *Manager) Chosen(logicalPath, version string) bool { return pinned == version } +// Runs reports whether a managed selection runs, rather than the release's +// newer copy at its path. A selection that does not run is kept all the same. +func (m *Manager) Runs(installed state.DriverRepoInstall) bool { + if m.store == nil { + return true + } + ok, _ := m.runs(installed) + return ok +} + // pinKey holds the version of a managed selection the owner chose over a // newer one that was running. previousPinKey keeps the value it replaced, so // undoing an activation also undoes its effect on the choice. diff --git a/web/driver-versions.test.mjs b/web/driver-versions.test.mjs index cc7d06350..511ed7e88 100644 --- a/web/driver-versions.test.mjs +++ b/web/driver-versions.test.mjs @@ -678,3 +678,20 @@ test("a beta outage is shown but the list still redraws", async () => { assert.match(textOf(panel), /Checked, but beta channel: connection refused/); assert.ok(rowOf(panel, "v1.1.1"), "the stable rows are drawn again"); }); + +test("a kept selection the release has overtaken can be chosen again", async () => { + const { api, calls } = load(); + const panel = element("div"); + api.render(panel, "ferroamp", { ...PAYLOAD, release_version: "1.0.2", superseded_version: "1.0.0" }, { + runningVersion: "1.0.2", runningSource: "bundled", logicalPath: "drivers/ferroamp.lua", + }); + + assert.match(textOf(rowOf(panel, "release")), /running now · this release/); + const kept = rowOf(panel, "v1.0.0"); + assert.doesNotMatch(textOf(kept), /selected/, "the release's copy runs, not the kept 1.0.0"); + assert.match(textOf(kept), /on disk/); + buttonsOf(kept)[0].click(); + await settle(); + assert.equal(calls[0].path, "/api/device_repository/drivers/ferroamp/activate"); + assert.deepEqual(calls[0].body, { version: "1.0.0", sha256: "aa11…" }); +}); diff --git a/web/settings/tabs/devices.js b/web/settings/tabs/devices.js index 481990e15..fdd5a97be 100644 --- a/web/settings/tabs/devices.js +++ b/web/settings/tabs/devices.js @@ -213,6 +213,12 @@ var available = (body && body.available) || []; var release = (body && body.release_version) || ""; var chosen = (body && body.chosen_version) || ""; + // A selection the release's newer copy has overtaken is kept but does not + // run. It is offered like any version on disk, so the owner can go back. + var superseded = (body && body.superseded_version) || ""; + function selected(item, version) { + return !!(item && item.active && version !== superseded); + } var rows = []; var seen = {}; @@ -242,8 +248,8 @@ channel: candidate.channel === "beta" ? "beta" : "stable", changes: changesURL(candidate.repository, driver.source_commit, driver.filename), downloaded: !!onDisk, - active: !!(onDisk && onDisk.active), - chosen: !!(onDisk && onDisk.active && chosen && driver.version === chosen), + active: selected(onDisk, driver.version), + chosen: !!(selected(onDisk, driver.version) && chosen && driver.version === chosen), verification: verificationLabel((driver.metadata || {}).verification_status) }); }); @@ -260,8 +266,8 @@ channel: "", changes: "", downloaded: true, - active: !!item.active, - chosen: !!(item.active && chosen && item.version === chosen), + active: selected(item, item.version), + chosen: !!(selected(item, item.version) && chosen && item.version === chosen), verification: "" }); });