fix(firmware): delete a version whose update rolled back

[tarakanof]

Closes #330

Why

After a rollback the knob's target stays parked on the failed version. otaTargets counted it as in use, so DELETE /v1/firmware/{v} returned 409. The app showed the error only in the section footer, and it had no way to clear the target.

Server

• otaTargets skips a target parked after its version failed or rolled back (version == target, phase failed/rolled_back). Active offers and pending targets still return 409. Retention (otaKeeps) still keeps parked targets.
• A successful DELETE calls otaForget, which prunes blocked and resets a knob whose last attempt of that version failed: target, retry, version, phase and error are cleared. mode, blocked and the attempt counter are kept. Clearing the target bumps the epoch. Without this, the target would point at a missing image: no offer, auto mode stuck behind it, and Retry left at "waiting for check-in" for good.
• At check-in the server drops a target whose image no longer exists, unless an update is in progress. This covers a Retry that races a DELETE.
• PUT …/ota {"target":null} also dismisses a failed or rolled-back attempt: phase becomes idle, error is cleared and the version stays in blocked. This works with or without a target.
• Before the delete, the parked "Update to X failed" state and Retry behave as before (existing tests).

App

• A refused delete now shows under that image's row, not in the footer. A 409 reads: "A knob is set to update to this version. Cancel that update in the Firmware row, or wait until it has finished, then delete again."
• The failure line gets a Dismiss button next to Retry. A target that is still waiting (idle or offered) gets Cancel Update. Both call KnobOTAModel.cancel() (PUT target:null), which now reports errors under its own .cancel action ("Couldn't clear the update: …").
• Strings synced (scripts/strings.sh sync, then check passes).

Docs

API.md, openapi.yaml (Redocly lint passes) and ARCHITECTURE "Knob firmware updates".

Evidence

• go test -race ./... passes, plus go vet and gofmt. New tests: delete of a parked target (rolled_back and failed) returns 204 and clears the state, after which no offer is made and Retry returns 400; auto mode offers again after the delete; a pending target still returns 409; an active offer returns 409; retention keeps a parked target; check-in clears a dangling target; Dismiss works with and without a target. The first three fail without the fix.
• swift test --package-path macos passes (963 tests). New tests cover the per-row delete error and the 409 mapping, cancel() under .cancel, and canCancel.
• No screenshots: the app wasn't launched (installed bundle id rule).

[tarakanof]

Codex review

Codex traced the code paths and lock ordering but didn't run the tests. Posted on its behalf.

1. P2: DELETE races a concurrent Retry (cmd/ember/devices_ota.go:339).
   • Scenario: checkin caches the parked target's metadata, and DELETE passes otaTargets. A concurrent Retry resets the phase to idle, so checkin offers the cached image before DELETE finishes. otaForget then skips the now-active attempt.
   • Result: DELETE answers 204, but the knob's offered download gets 404.
   • The store lock doesn't cover these registry transitions, and no test pins this interleaving.
2. P2: Dismiss then DELETE leaves a retryable missing image (devices_ota.go:364).
   • Scenario: Dismiss turns rolled_back into idle but keeps Version. DELETE prunes Blocked but skips clearing the attempt, because parked() is false.
   • Result: a later PUT {"retry":true} answers 200 and targets the deleted version. Checkin then silently clears it and offers nothing.
   • The new dismiss/delete test stops after checking DELETE's status.
3. P2: a stale absence can cancel a newly uploaded, explicitly selected target (cmd/ember/devices_ota_http.go:61).
   • Scenario: a target is dangling, so checkin reads in.target == nil. Before checkin's registry update, an upload restores that version and a PUT selects it. Target and mode still match checkin's snapshot, so checkin clears the now-valid target and its retry flag.
   • Fix: the cleanup should validate more than the target string. The new test only covers the uncontended case.

[tarakanof]

Opus review

No blocking findings. The state machine holds up for the cases in #330. Below, findings ranked by severity, then answers to the hunt questions.

Findings

1. Medium-low: retention eviction still leaves the stale state this PR fixes for DELETE. store.put hands evicted versions to otaUnblocker, not otaForget. I probed this in a scratch copy: auto mode, 0.9.14 rolls back, then 6 newer uploads evict 0.9.14. The status stays rolled_back, version=0.9.14, target=null, so the app still shows "Rolled back to …" with Retry. PUT {"retry":true} answers 200 (no image check on the retry path) and sets target=0.9.14. The next check-in's dangling clear drops it, and the row goes quiet with no explanation. A parked manual target is safe because otaKeeps now keeps it. The auto case (no target) is not kept. Suggest: call otaForget for the versions pruning evicted, but not for the replaced version, which still exists. Alternatively, make retry without a target 400 when the image is gone. This was there before the PR, but it is the same bug class.

2. Low: the 409 copy promises a Cancel that isn't always there. "A knob is set to update to this version. Cancel that update in the Firmware row, or wait until it has finished…" is also shown when the 409 comes from an auto offer: version == X && offered, no target. canCancel needs a target, and PUT target:null can't cancel an auto offer server-side anyway. An auto offer can hold the version for up to otaAutoNotStarted (24 h). The same applies during a download. Either reword ("…or wait until it has finished or timed out") or let target:null / Cancel Update also withdraw an auto offer that hasn't started.

3. Low: Cancel Update races a download, without stranding the knob. In offered, the pane refreshes only every 15 s. If the knob has already started (downloading), PUT target:null returns 200: the target is cleared but the phase stays downloading. servable is still true, so the download and install continue, and applyOTAResult still settles the attempt. The knob is not stranded, but the click silently did nothing. In auto mode with a different newest release Y, an idle check-in could re-offer Y over the in-flight X. X's Range requests then 409, and the knob's X failure is ignored (attempt mismatch). This heals itself. Consider answering ota_in_progress for a clear in downloading, so the existing .cancel error line says why.

4. Low (wording/docs): "won't offer this version again" is not always true.

• Dismiss help: "Ember won't offer this version again unless you install it."
• ARCHITECTURE/openapi/PR body: "the version stays in blocked."

A manual not_started timeout does not block (if offer.Auto { o.block() }). After Dismiss, that version shows "Update to X", and Automatic mode would offer it. DELETE followed by a re-upload also unblocks it (by design, already documented for DELETE). Suggest "Clear this failed update." and "stays in blocked if it was blocked".

5. Nit (tests): one mutation survives. Removing !otaActive(o.Phase) from the check-in dangling clear keeps all tests green. The guard is reachable only in odd states (an image missing while an attempt is active, e.g. one that failed to load at boot), but a one-line test would pin it. The dangling clear also bumps the epoch at check-in, which causes one extra check-in. That is harmless.

Hunt questions

• Re-offer of a blocked or rolled-back build: No path found apart from ci: add Go fmt/vet/test workflow #4. otaForget prunes blocked only for versions that no longer exist. Dismiss keeps blocked, and the auto candidate filter and available both skip it. A re-upload of a deleted version is offered again in auto mode, the same as before this PR.
• Knob stranded when the target is cleared mid-offer or mid-download: No. Offered goes to idle. Downloading continues (deploy: Unraid host networking + writable store volume for discovery; release-driven redeploy docs #3). Committed phases are refused with ota_in_progress.
• Epoch: The knob doesn't track the target. The bump only triggers a check-in, and a parked target produced no offer anyway. No confusion.
• Dangling clear vs uploads: get takes the store lock, so a replace or an eviction can't make it miss (otaKeeps keeps targets). The only window is DELETE, then re-upload, then PUT target, all landing between one check-in's get and its updateOTA, which drops the fresh target. That window is milliseconds, so this is acceptable.
• Cancel during "offered" while the knob is already downloading: It doesn't break the knob, see deploy: Unraid host networking + writable store volume for discovery; release-driven redeploy docs #3.
• Lock order: otaForget runs under the store lock and then takes the registry lock, the same order as before. A failed otaForget is logged, and DELETE still answers 204. The check-in dangling clear then covers the target.

Verification

• go test -race ./...: pass. swift test --package-path macos: 963 pass. scripts/strings.sh check: exit 0.

• Mutations (scratch copy). Each of these was caught by a failing test:

  • dropping the parked skip
  • not resetting in otaForget
  • no epoch bump
  • resetting the attempt counter
  • no blocked prune
  • no dangling clear
  • no Dismiss
  • Dismiss unblocking
  • no otaKeeps target
  • otaForget ignoring parked
  • parked without the version check

  Only chore: pre-push hook mirroring CI #5 survived.

• Conventions: no code comments, and the strings are synced. Button names "Cancel Update" and "Dismiss" fit the pane's title case. The per-row delete error replaces the footer cleanly (.delete removed from the footer ForEach).

[tarakanof]

Review fixes (7be310e, cc1b415)

The fix: an in-memory firmware generation (fwGen) in the registry.

• Every removal bumps it in the same registry-locked step as its in-use check: otaTargets for DELETE and replace, otaKeeps for each evicted version. The store lock is still held at that point.
• Check-in reads fwGen and the epoch before it reads the store. It offers or clears only if fwGen, target, mode and attempt are all unchanged.
• PUT with a target or retry first checks the image of the version it would set. It commits only if fwGen and that version are unchanged. It retries up to 3 times, then answers 409 firmware_changed.
• otaReadHook is a test seam that runs between the store read and the registry update.

Finding	Fix	Test (each fails on the previous head)
Codex 1: DELETE races Retry	A Retry before the claim leaves the version held, so DELETE answers 409. A Retry after the claim finds the image gone and gets 400. A check-in that read the store before the claim makes no offer, because fwGen changed.	TestOTARetryDuringADeleteOfTheParkedVersionIsRefused runs Retry inside DELETE's purge via the rename seam, during a check-in paused by otaReadHook. TestOTARetryBeforeADeleteHoldsTheVersion covers the other order.
Codex 2: Dismiss, then DELETE	DELETE and eviction now retire the version on every knob outside an active attempt. A target naming it is cleared (epoch bump). A record of that version is reset unless it is done, whatever its phase. Retry of a version no longer stored answers 400.	TestOTADeleteAfterDismissLeavesNothingToRetry, TestOTARetryRefusesAMissingImage
Codex 3: stale absence clears a re-selected target	The dangling-target clear also needs fwGen, target, attempt and epoch unchanged. PUT target bumps the epoch.	TestOTACheckinKeepsATargetReselectedAfterItsRead (upload and PUT target run inside the check-in hook)
— (same race class)	PUT target racing a DELETE of that version answers 400 instead of setting a dangling target	TestOTAPutTargetRacingItsDeleteIsRefused
Opus 1: eviction	Evicted versions go through the same retire step as DELETE. A replaced version only gets unblocked.	TestOTARetryOfAnEvictedVersionIsRefused
Opus 2: 409 copy	Reworded (the simpler option): "A knob is updating to this version or waiting to. Wait until that update finishes, or use Cancel Update in the Firmware row if it shows, then delete again." Auto offers are not cancellable.	Swift catalog plus mapping test
Opus 3: Cancel during a download	409 ota_in_progress. The app shows it on the .cancel line.	TestOTACancelDuringADownloadIsRefused
Opus 4: Dismiss wording	The tooltip is now "Clear this failed update." Docs say blocked is unchanged and note that a manual not_started never blocks.	—
Opus 5: !otaActive mutation	Pinned. The test fails with the guard removed (checked).	TestOTACheckinKeepsAMissingTargetWhileAnAttemptIsActive

Why an offer for a removed version can't happen now. A check-in that reads the store before the claim either commits before it, which makes the version active so the claim answers 409, or commits after it and sees the fwGen change. A check-in that reads after the claim waits on the store lock until the purge is done. So otaRetire never meets an attempt that is active on a gone version, and it skips active records anyway.

The app maps firmware_changed to "Ember's stored firmware changed while saving. Try again."

Checks: go test -race ./..., vet, gofmt, Redocly lint (the one warning was already there, coredumps example), swift test (964), scripts/strings.sh check. Docs updated: ARCHITECTURE (new "Firmware generation" paragraph), API.md, openapi.

[tarakanof]

Opus re-check

Head: 7be310e (server) and cc1b415 (app). I worked read-only and ran the repros in a scratch copy, which is now deleted.

Verdict: approve. No blockers. Every Codex and Opus finding is fixed. The fwGen scheme adds no livelock, no lock inversion and no wrong suppression. What's left is test gaps and wording nits.

Findings, verified

I reproduced each one with otaReadHook and the rename seam:

• Codex 1, DELETE racing Retry:
  • Retry and then DELETE, both inside a check-in's read window: Retry 200, DELETE 409, the check-in offers 0.9.14, and the download returns 200.
  • DELETE and then Retry: DELETE 204, Retry 400, no offer, and no offer on later check-ins.
  • DELETE inside the read window of a PUT retry: the compare-and-commit fails, the next try finds no image, and the retry returns 400. The status has no target and no version.
  • Auto check-in that commits while the purge is in progress (it read before the claim and commits mid-rename): no offer, DELETE 204.
• Codex 2, Dismiss then DELETE: also checked in auto mode. Dismiss, then DELETE 204, then retry 400 and target 400.
• Codex 3, a re-selected target: DELETE, re-upload with other bytes and PUT target, all inside one check-in's read window. That check-in drops its offer because fwGen changed. The next check-in offers the new sha256 and the target survives.
• Opus 1, eviction: eviction inside a PUT retry's read window gives 400, and the record is reset.
• Opus 3, Cancel during a download: 409 ota_in_progress.
• Opus 5, the !otaActive mutation: now pinned.
• Also checked: a ?replace=1 inside a check-in's read window doesn't offer the stale sha, and the next check-in offers the new sha.

Hunt results

• Livelock or starvation: none.
  • A PUT can lose only if a removal lands in each of its read windows. Forcing a DELETE into every window gives exactly 3 tries, then 409 firmware_changed. The next PUT without contention returns 200.
  • A check-in that loses the compare records the knob's result and offers on the next check-in. An unrelated DELETE costs exactly one check-in.
  • The epoch-gated dangling clear depends only on user-initiated epoch bumps (config, rename, rotate, PUT).
• Lock order: the store, then the registry, everywhere.
  • Check-in, PUT and download call knobFW.get or newestAbove outside updateOTA. otaStatus runs after the registry lock is released.
  • otaTargets, otaKeeps and otaRetire run under the store lock only.
• fwGen on every removal path: yes. DELETE and replace bump through otaTargets, eviction through otaKeeps, and otaRetire bumps again.
  • Edge case: if swapInLocked's rollback rename also fails, the index entry is deleted without otaRetire. fwGen was already bumped and the dangling clear plus the 400 on retry cover it. Nit.
• Wrong suppression: none found. Every compare-and-commit field is stable unless something concurrent changes it. A failed dangling clear falls back to "no offer" and retries on the next check-in.
• Epoch: it bumps only when the retire clears a target, on the dangling clear, and on PUT target or retry. That gives one extra check-in at most and doesn't affect the knob otherwise.
• App mapping:
  • DELETE 409: the body update target maps to the new inUse copy.
  • firmware_changed and the 400 unknown firmware version on retry are mapped.
  • .cancel has its own error line.

Low and nits (none blocking)

1. Test gaps (low). In a scratch copy, these three mutations survive the PR's suite:

   • dropping the fwGen compare in check-in: the parked-target tests pass anyway, because the retire changes Target;
   • dropping the fwGen++ in otaTargets;
   • dropping the fwGen++ in otaKeeps.

   The first two are what stops an auto offer of a version that is being purged: without them I got an offer, DELETE 204, then download 404. A test like this pins both:

   • auto mode, release 0.9.14;
   • inside otaReadHook, run DELETE in a goroutine whose rename seam blocks until the check-in has returned;
   • assert there is no offer.

   A replace inside a check-in's read window, asserting no stale sha, would also pin the check-in compare.

2. Docs nit: ARCHITECTURE says "A PUT with a target or a retry … commits only if fwGen … unchanged". In fact every PUT, including mode-only and target:null, goes through the compare-and-commit and can return firmware_changed. The "store lock before registry lock" sentence still lists only "the in-use checks and the blocked prune". It should also mention the retire.

3. Copy nit: Cancel Update while the knob downloads shows "Couldn't clear the update: The knob is already installing an update…". The knob is downloading, not installing, but the meaning is close enough.

Checks

• go test -race ./...: pass.
• go test -race -count=3 -run OTA ./cmd/ember/: pass.
• swift test --package-path macos: 964 tests pass.
• scripts/strings.sh check: exit 0.
• The worktree is clean.

[tarakanof]

Re-check follow-up (tests, docs and copy only; no server logic changes):

• Test gap: added TestOTAAutoCheckinCommittingMidDeleteDoesNotOffer and TestOTAAutoCheckinCommittingMidEvictionDoesNotOffer. In both, an auto check-in reads the store, then commits while a DELETE or an eviction is held mid-purge on the rename seam. Neither offers. I checked each of the 3 surviving mutations by hand: dropping the check-in's fwGen compare fails both tests, dropping fwGen++ in otaTargets fails the DELETE one, and dropping it in otaKeeps fails the eviction one.
• Docs: ARCHITECTURE now says every PUT goes through the fwGen compare-and-commit, mode-only and target:null included. openapi says the same for firmware_changed. The lock-order sentence now names otaRetire.
• App: a Cancel Update that gets 409 ota_in_progress now reads "The knob has already started this update. Wait until it finishes." (KnobOTAError.cancelFailure).
• Checks: go test -race ./..., vet, gofmt, Redocly lint, swift test (964 pass) and scripts/strings.sh check all pass.