fix(firmware): keep blocked state consistent with firmware on disk

[tarakanof]

Closes #325

What and why

1. DELETE 404 no longer unblocks a version whose files are still on disk. remove unblocks on 404 only when neither <v>/ nor an .old-<v>-* aside dir exists (a dir that failed to load at boot, or an earlier failed RemoveAll, keeps the block, so a restart can't bring back a rolled-back build unblocked).
2. Replace is atomic, with every failure path consistent. The replace already wrote a .tmp- dir and swapped it in via an .old- aside. Fixed the failure handling:
   • new dir can't move in → old dir moved back, index/SHA/block unchanged, error returned;
   • that rollback also fails → version dropped from the index (no /bin for a missing dir), block kept; boot restores the aside;
   • aside cleanup fails after the swap → swap counts as done: index takes the new SHA, unblock fires, upload answers 201 and the handler logs the leftover (boot deletes it). Same 201 + log now applies to a failed eviction after a successful store (was 500 though the bytes were stored).
3. Related: DELETE and retention eviction now purge a version's .old- copies too. Before, a leftover aside plus a later DELETE meant boot would restore the deleted version, unblocked.

Store gains rename/removeAll seams (defaults os.Rename/os.RemoveAll); new tests and the two existing chmod-based tests inject failures through them (no permission tricks, no root skip).

Docs: API.md, openapi.yaml (DELETE/upload descriptions), ARCHITECTURE "Knob firmware updates".

Tests

• TestFirmwareStoreDeleteOfAnUnindexedVersionUnblocksOnlyWithoutFilesOnDisk
• TestFirmwareStoreReplaceCommitsWhenTheOldCopyCannotBeRemoved
• TestFirmwareStoreDeleteRemovesALeftoverOldCopy
• TestFirmwareStoreReplaceRollsBackWhenTheNewCopyCannotMoveIn
• TestFirmwareStoreReplaceWhoseRollbackFailsHidesTheVersionUntilBootRestoresIt
• HTTP: TestFirmwareDeleteOfAnUnloadedVersionKeepsItBlocked, TestFirmwareReplaceWithAStuckOldCopyStillAnswersCreatedAndUnblocks

Mutation check: reverting the 404 check or the "swap done" return makes 4 of the store tests fail.

Evidence

gofmt -l cmd internal            # clean
go vet ./...                     # clean
go test ./... -race -count=1     # all ok (cmd/ember 55s)
redocly lint docs/openapi.yaml   # valid; 1 warning, pre-existing on main (coredumps example)

Not fixed here (follow-ups)

• putELF creates its temp file inside <v>/ without the store lock; a concurrent DELETE's RemoveAll can race it (ENOTEMPTY), leaving the version out of the index, dir on disk, block kept. Consistent after this PR (404 won't unblock; boot sweeps the dir) but the DELETE answers 500.
• Boot: if <v>/ exists but is invalid (no meta) and a valid .old-<v>-* exists, the aside is deleted before <v>/ is swept, losing both. Only reachable by external corruption.
• A fresh upload of a version that is not indexed but still blocked (e.g. after a failed DELETE) keeps the block; only ?replace=1 of an indexed version unblocks. Probably intended.

[tarakanof]

Codex review

(Run by Codex in a read-only sandbox; posted on its behalf. go test could not run there; findings are from code paths, not reproduced.)

1. P2: failed cleanup can bring back blocked bytes without their block (cmd/ember/firmware.go:364).
   • Scenario: replace blocked image A with B. The aside cleanup of .old-A fails, but the replace clears the block.
   • A later DELETE or eviction fails to remove .old-A, carries on and removes B.
   • On boot, A is restored and is eligible for automatic OTA again.
   • The new delete test uses no unblock callback during the replace, so it misses this.
2. P2: retrying a replace after a failed rollback leaves the new image blocked (cmd/ember/firmware.go:429).
   • The failed rollback drops the index entry.
   • A later successful ?replace=1 upload sees exists == false and skips the unblock, even though it installs different bytes.
   • It returns 201, but automatic OTA and available still exclude that version.

[tarakanof]

Opus review

go vet ./cmd/ember/ clean, go test -race -count=1 ./cmd/ember/ ok (56s), gofmt -l clean, no added code comments. Two findings reproduced with throwaway tests in a scratch copy (not pushed).

1. Medium: a partly failed DELETE (or eviction) can bring back the pre-replace build, unblocked, at boot

purgeLocked removes every copy and keeps going after an error. copiesLocked returns names in ReadDir order, so .old-<v>-* comes before <v>. If the aside removal fails and <v>/ is removed, the index entry is gone and remove returns 500. At boot recoverAsideLocked sees no <v>/ and restores the aside, which holds the old bytes.

Reproduced:

1. Upload 0.9.14 (A). A knob blocks 0.9.14.
2. ?replace=1 with B while .old- removal fails. You get 201, unblock([0.9.14]), and the A aside stays on disk.
3. DELETE /v1/firmware/0.9.14 while .old- removal still fails (likely if step 2's cause persists). 0.9.14/ (B) is removed, the call returns 500, no unblock (nothing is blocked).
4. Restart. 0.9.14 loads with A's SHA and is not blocked. This is the "rolled-back build comes back unblocked" case fix(firmware): blocked state can disagree with on-disk firmware (DELETE 404, failed replace) #325 is about.

TestFirmwareStoreDeleteRemovesALeftoverOldCopy asserts that the old copy is restored at boot. It doesn't notice those bytes are the pre-replace ones that the replace unblocked. Eviction has the same shape: pruneLocked → purgeLocked.

Suggested fix: in purgeLocked, remove the asides first and stop before <v>/ if any of them fails. <v>/ then survives, and boot deletes the aside because <v> is present. Alternatively, don't restore an aside at boot when the version was deleted, but that needs persisted state. The ordering fix is simpler.

2. Low/medium: retrying a ?replace=1 whose rollback failed returns 201 and doesn't unblock

After the double rename failure, swapInLocked drops the version from the index. The natural client step is to rerun publish.sh --replace. That retry goes down the !exists path, gets 201, and evicted gets no d.Version, so the version stays blocked. Reproduced: retry created=true err=<nil> unblocked=[]. API.md and openapi say a ?replace=1 upload drops the version from blocked, so docs and behaviour disagree on this path. publish.sh reports success, and the knob keeps skipping the new bytes until someone deletes and re-uploads. Suggested fix: when replace is set and the swap succeeded, unblock d.Version even if it wasn't indexed. The bytes on disk are new either way. Or document the exception.

3. Low: a 404 DELETE now refuses to unblock but can't clear what blocks it

For an unindexed <v>/ (it failed to load with an I/O error, which boot doesn't sweep) or a stray .old-<v>-*, DELETE answers 404, keeps the block and leaves the files. The only API way out is to upload v then DELETE it. Consider purging the on-disk copies in the 404 branch and unblocking when that succeeds (still 404). The admin's intent is clear and the state would converge. Optional.

4. Low: mutations that survive (tests don't pin these)

• pruneLocked back to removeAllErr(s.versionDir(v)) (no aside purge on eviction): all tests pass. No test covers "eviction removes .old- copies".
• Dropping pruneErr from put's return (evicted, _ := s.pruneLocked(...), return err): all pass. TestFirmwareStoreKeepsTheBlockWhenAnEvictionFails only checks that early puts have no error and never asserts the error on the evicting put. There's also no HTTP test for "eviction cleanup fails → 201 + body".
• remove's 404 branch unblocking on a copiesLocked error (err != nil || len==0): passes. The ReadDir-error branch is untested. Minor.

The other mutations are caught: rollback delete(s.index), the 404 copies check, copiesLocked aside matching, the errors.Join(err, …) → pruneErr swap, and the handler && !created.

5. Nits: docs

• openapi upload: "if the swap fails the old bytes stay and the answer is an error" doesn't hold when the rollback also fails (the version is hidden until a restart). ARCHITECTURE has this right.
• API.md: "a ?replace=1 upload, and each version retention evicts, are dropped from…" reads as the upload being dropped. Suggest "the version a ?replace=1 upload replaces, and each version retention evicts, is dropped…". API.md also leaves out the 201 for an eviction cleanup failure, which openapi covers.
• API.md / openapi DELETE: "no directory for the version on disk" should also cover the .old-<v>-* aside, as ARCHITECTURE does.
• ARCHITECTURE: the added sentence ends in an overlong line ("…copies with it. A dev-seed build…"). Rewrap.

Checked, OK

• 201 on cleanup failure for clients: publish.sh only branches on curl's --fail-with-body (2xx), then PUTs the ELF and checks sha256 via GET /v1/firmware. Both work because the new bytes are indexed. That's better than before: a 500 used to abort a publish whose bytes were stored. The macOS app (KnobService.uploadFirmware) treats any 2xx as success and never sends replace. Safe.
• Lock coverage: every new path (copiesLocked, purgeLocked, the rollback, the unblock callback) runs under s.mu. /bin open() takes the fd under the lock, so a replace or delete in flight doesn't break a running download. newestAbove and available read only the index, and the hidden-after-rollback version is out of it. putELF after a replace is rejected by the SHA check under the lock. Its temp file can end up in an aside, which RemoveAll handles because no new entries are created. The DELETE/ENOTEMPTY race is the known follow-up.
• asideVersion with prerelease versions (.old-1.2.3-rc1-<n> → 1.2.3-rc1) is fine, because MkdirTemp's suffix never contains -.
• Repo conventions: no comments, conventional commit, seams through struct fields like writeFile, and the chmod/root-skip tests are gone.

Verdict: fix #1 before merging (it's the bug class this PR targets). #2 is worth fixing or documenting. The rest are optional.

[tarakanof]

Review fixes (00271e5)

Finding	Fix
Opus 1 / Codex P2 #1: failed DELETE or eviction can restore pre-replace bytes unblocked	purgeLocked removes .old-<v>-* first and stops before <v>/ if any fails (DELETE and eviction). Tests: TestFirmwareStoreFailedDeleteNeverLeavesOnlyThePreReplaceCopy, TestFirmwareStoreFailedEvictionNeverLeavesOnlyThePreReplaceCopy. Both block across the replace and assert that after a restart the replacement's SHA loads, not A's.
Opus 2 / Codex P2 #2: a retried replace after a failed rollback stays blocked	When the swap succeeds with replace set, put unblocks d.Version on the !exists path too. Test: TestFirmwareStoreReplaceRetriedAfterAFailedRollbackUnblocks.
Opus 3: a 404 DELETE can't clear what blocks it	The 404 branch purges any unindexed <v>/ and .old- copies, then unblocks. A failed purge or an unreadable store dir answers 500 and keeps the block. Tests: TestFirmwareStoreDeleteOfAnUnindexedVersionPurgesItsFilesThenUnblocks, HTTP TestFirmwareDeleteOfAnUnloadedVersionUnblocksOnlyOnceItsFilesAreGone.
Opus 4a: eviction without aside purge survives	TestFirmwareStoreEvictionRemovesALeftoverOldCopy
Opus 4b: dropping pruneErr survives	TestFirmwareStoreKeepsTheBlockWhenAnEvictionFails now asserts the evicting put returns created plus an error. HTTP TestFirmwareUploadWhoseEvictionCleanupFailsAnswersCreated asserts 201 and the body.
Opus 4c: a 404 unblocking on a dir read error survives	New readDir seam. Test: TestFirmwareStoreDeleteKeepsTheBlockWhenTheDirCannotBeRead.
Opus 5: doc nits	openapi: rollback-failure wording and the replace param. API.md: the replace sentence reworded, plus the 201-on-cleanup note. DELETE docs (API.md, openapi, ARCHITECTURE) now cover the .old- copies, the 404 purge and the 500. The ARCHITECTURE paragraph is rewrapped.

Mutation check, each one reverted on its own; every one fails at least one test:

• eviction calling removeAllErr(versionDir);
• evicted, _ := pruneLocked;
• a 404 that ignores purge or read errors;
• a purge that keeps going after an aside fails;
• if exists { alone.

gofmt -l cmd internal          # clean
go vet ./...                   # clean
go test ./... -race -count=1   # all ok
redocly lint docs/openapi.yaml # valid; 1 warning that is already on main (coredumps example)

[tarakanof]

Opus re-check

This re-checks 00271e5 against the Opus and Codex reviews. I ran throwaway HTTP-level tests in a scratch copy, then deleted it. Nothing was pushed.

gofmt -l cmd internal is clean, go vet ./... is clean and go test -race -count=1 ./... passes.

Original findings, reproduced

Finding	Result
Opus 1 / Codex #1, DELETE	Fixed. Blocked A, then ?replace=1 B with a stuck aside: 201, unblocked. DELETE returns 500 and leaves .old-0.9.14-* and 0.9.14/. After a restart, 0.9.14 loads with B's SHA.
Opus 1 / Codex #1, eviction	Fixed. The same setup followed by 5 uploads leaves 0.9.1/ (B) beside the aside, and B loads after a restart.
Opus 2 / Codex #2	Fixed. The failed rollback gives 500 and keeps the block. A ?replace=1 retry gives 201 and clears it, and the retried bytes load after a restart (the aside is swept at boot).
Opus 3	Fixed. An unindexed 0.9.14/ plus .old-0.9.14-* with the aside stuck gives 500 and keeps the block. Once removal works, the next DELETE gives 404, clears the block and leaves only .old-0.9.14-rc1-* (prerelease asides don't cross-match).
Opus 4, 5	Tests and docs added as listed in the fix map.

New or remaining

1. Low/medium: the !exists upload path can still leave only the pre-replace aside. swapInLocked runs _ = s.removeAll(dir) before the rename, without purging the asides first. Reproduced:

   1. Blocked A, then ?replace=1 B with a stuck aside: 201, unblocked.
   2. DELETE returns 500. The aside and 0.9.14/ (B) stay, as designed, but the index entry is dropped.
   3. A plain upload of 0.9.14 C with a failing tmp → 0.9.14 rename: removeAll deletes B, the rename fails and the upload returns 500. Only .old-0.9.14-* (A) is left.
   4. After a restart, 0.9.14 loads with A's SHA, unblocked.

   The same applies after a failed eviction. It takes a stuck aside plus a rename failure, but ARCHITECTURE now says "boot can never restore the pre-replace bytes without their block". Fix: in the !exists branch, call purgeLocked(version) and fail the upload if it errors, instead of the bare removeAll(dir).

2. Low: after a failed indexed DELETE or eviction, the index and disk disagree until a restart. delete(s.index, v) runs before purgeLocked, so an intact <v>/ drops out of GET /v1/firmware and comes back at boot. Nothing is unblocked, so it's safe. In the macOS app the image disappears after the error and can't be retried from the UI. Option: drop the index entry only once purgeLocked succeeds.

3. Low: the 404 purge skips the target check. An indexed targeted version gives 409. An unindexed one (it failed to load at boot) is now purged with 404 while otaTargets is still true, which leaves a dangling target. Before this commit the 404 path left the files alone. Consider running inUse before the purge, or document it.

404 → 500 and clients

• cinder/firmware/tools/publish.sh never sends DELETE, so it isn't affected.
• The macOS app (KnobOTAModel.delete) swallows APIError.http(404, _) and surfaces other errors, then reloads the list and status. A 500 now shows a delete error where it used to report silent success with the block kept. That's the honest answer, and the user can retry. No client mishandles it.

Nit

• ARCHITECTURE has an 85-column line: ".old-<version>-* directories are removed first), only once its files are gone, and". Rewrap it.

Verdict: the original findings are fixed and verified. #1 is the same bug class through a new path, so a small fix is worth making before merge. #2, #3 and the nit are optional.

[tarakanof]

Re-check fixes (1b20edb)

Finding	Fix
1. The !exists upload path could leave only the pre-replace aside	swapInLocked's !exists branch now calls purgeLocked (asides before <v>/) instead of _ = removeAll(dir), and fails the upload with no swap if that purge fails. The 4-step repro: TestFirmwareStoreUploadOverAnUnindexedVersionNeverLeavesOnlyThePreReplaceCopy. With the aside stuck, the upload fails and B loads after a restart. Once the aside clears and the rename fails, A does not load after a restart. The failed-eviction variant: TestFirmwareStoreFailedEvictionKeepsTheReplacementIndexed. After fix 2, B stays indexed, so a plain upload gets 409 and the restart loads B, not A.
2. A failed indexed DELETE or eviction dropped the index entry	The entry is dropped only after purgeLocked succeeds. The version stays listed and the call can be retried. Covered by TestFirmwareStoreKeepsTheBlockWhenRemoveAllFails (entry kept, retry gives 204 and unblocks) and TestFirmwareStoreKeepsTheBlockWhenAnEvictionFails (0.9.1 kept, and the next upload evicts and unblocks it).
3. The 404 purge skipped the target check	The inUse guard now runs before the purge, whether or not the version is indexed, and answers 409. Test: TestFirmwareStoreDeleteOfAnUnindexedTargetIsRefused.
Nit: ARCHITECTURE	Rewrapped the 85-column line and the 106-column line that followed it. The boot-restore sentence now says what the code guarantees: DELETE, eviction and an unindexed upload remove the .old- copies before <v>/ and stop on failure. API.md and openapi document the 409 for unindexed targets and that a version stays listed after a 500.

Mutation check, each one reverted on its own; every one fails at least one test:

• the old !exists removeAll;
• the in-use guard applied only when indexed;
• eviction dropping the index entry first;
• DELETE dropping the index entry first.

gofmt -l cmd internal          # clean
go vet ./...                   # clean
go test ./... -race -count=1   # all ok
redocly lint docs/openapi.yaml # valid; 1 warning that is already on main

[tarakanof]

Opus re-check 2

This re-checks 1b20edb against the Opus re-check, the Opus review and the Codex review. I ran throwaway HTTP tests in a scratch copy, restarting with a fresh App on the same DB and firmware dir, then deleted the copy. Nothing was pushed.

gofmt -l cmd internal is clean, go vet ./... is clean and go test -race -count=1 ./... passes (cmd/ember 61s).

Earlier findings, reproduced over HTTP plus a restart

Finding	Result
Opus 1 / Codex 1, DELETE	Fixed. Blocked A, then ?replace=1 B with a stuck aside: 201, unblocked. DELETE: 500, aside and 0.9.14/ kept, still listed with B's SHA. After a restart B loads and the aside is swept.
Opus 1 / Codex 1, eviction	Fixed. Five more uploads all answer 201, 0.9.1 stays listed (B), and B loads after a restart.
Opus 2 / Codex 2	Fixed. The double rename failure gives 500, the version is hidden and the block kept. The ?replace=1 retry gives 201 and unblocks. B loads after a restart and no aside is left.
Opus 3	Fixed. Unindexed 0.9.14/ plus a stuck .old-: 500, block kept. Once the aside clears: 404, unblocked, dir empty.
Re-check 1, !exists path	Fixed. Indexed variant: the plain upload of C now gets 409. Retried with ?replace=1 and a failing tmp → 0.9.14 rename, it rolls back (500), and B loads after a restart. Unindexed variant, aside stuck: the upload fails (500) with both dirs intact, and B loads after a restart. Unindexed variant, aside clears, rename fails: the purge removes everything, the upload fails (500), nothing loads after a restart, and A never comes back.
Re-check 2	Fixed. A failed DELETE answers 500 and the version stays listed and blocked. A retry gives 204 and unblocks.
Re-check 3	Fixed. For an unindexed target, DELETE answers 409 and the files stay. After target: null, it answers 404 and the version is purged.

New

1. Medium-low, introduced by keeping the entry: a <v>/ removal that fails part-way leaves an indexed version with no cinder.bin.

   purgeLocked runs RemoveAll(<v>/) last. RemoveAll usually fails after it has removed some children (EIO or EACCES on one file, or ENOTEMPTY in the known putELF temp-file race). The index entry is now kept, so the store advertises bytes it can't serve. To reproduce, inject a removeAll for 0.9.14 that deletes cinder.bin and then errors. Both versions are release, and the knob is in auto mode running 0.9.13.

   Step	Result
   DELETE	500. 0.9.14/ holds only meta.json.
   GET /v1/firmware	Lists 0.9.14.
   OTA status	available = 0.9.14.
   Checkin	Auto offer of 0.9.14.
   Knob GET /v1/devices/self/firmware/0.9.14	500 firmware storage failed. The knob reports a failure and 0.9.14 gets blocked.
   publish.sh rerun with the same bytes	POST 200 and POST ?replace=1 200 (the same-SHA early return), then PUT /elf 204. The list shows sha256 matching and elf: true, so publish.sh prints "verified". Admin /bin still answers 500.
   Eviction variant	0.9.1 is listed with only meta.json, and /bin answers 500 until a later upload retries the eviction.

   How a version stuck this way recovers: a DELETE retry gives 204 and unblocks it. Re-uploading the same bytes doesn't repair it, with or without replace. Only different bytes with ?replace=1 swap a fresh dir in. After a restart the boot sweep removes the dir, because cinder.bin is missing, and keeps the block. A DELETE then answers 404 and unblocks. So the state converges, but until then auto mode offers a version it can't serve, and publish.sh reports success when it shouldn't. Before 1b20edb the entry was dropped first, which hid the half-removed dir. In the putELF race, the ELF PUT then answered 404 instead of committing.

   Suggested fix: make the live removal atomic. In purgeLocked, after the asides are gone, rename <v>/ to a .tmp-<v>-* name. If the rename fails, nothing was touched: keep the entry and return the error, as now. If it succeeds, the version is gone. Drop the entry and unblock, then RemoveAll the renamed dir as best effort and log a failure. Boot already sweeps .tmp- dirs (a non-semver name fails to load and gets removed), and asideVersion never matches them, so it can't restore them. This keeps the "retry on failure" property for the common case and closes the half-removed one. Test: a removeAll that deletes cinder.bin and then fails. Assert the version is either fully listed and servable, or not listed.

2. Low, docs.

   • openapi DELETE: "an earlier failed removal; the answer is then 404". After 1b20edb a failed removal keeps the entry, so a retry answers 204. It's 404 only after a restart dropped it, or for the hidden-after-rollback case. Reword or drop the example.
   • ARCHITECTURE: "an upload of a version the index does not hold … stop if one fails (the version stays indexed and the call can be retried)". In that upload case the version was never indexed. Something like "(an indexed version stays indexed …)".
   • openapi upload: "a version whose files could not be removed stays blocked" could add "and listed", to match DELETE.
   • ARCHITECTURE, Record paragraph: the reflow left a short orphan line ("from, and the offered"). Rewrap.

Clients

• publish.sh: after a failed DELETE or eviction, a plain publish of other bytes for that version now gets 409 (it was 201 when the entry was dropped). With --fail-with-body, publish.sh prints {"error":"this version is stored with other bytes; upload with ?replace=1"} and exits 1. That's the honest answer, and --replace works (exists path, rename swap, unblock), unless the version is a target. Same bytes give 200 and a "verified" result, which is correct except in the half-removed case in finding 1.
• macOS app: KnobOTAModel.delete swallows only 404. A 500 shows the error, then loadImages reloads the list, which now still contains the version, so the user can retry from the UI. This fixes the previous re-check's "can't be retried" note. The unindexed-target 409 can't be reached from the app, because it deletes only listed images. The upload never sends replace, so the new 409 after a failed eviction surfaces as an error, and the way out is to delete and re-upload. That's the same as any other version stored with different bytes.

Checked, OK

• The in-use guard runs before the index lookup, so an unindexed version in an active offer also gets 409. That's consistent with the docs, and it clears once the knob reports.
• A failed eviction keeps the entry, so it still counts toward the 5 kept. Every later upload retries it and answers 201 with a log line. Unblocking happens only on the retry that succeeds.
• pruneLocked deletes the entry only after the purge succeeds, and the !exists purge runs before the rename. The 4 mutations listed in the fix map were not re-run.

Verdict: 1 blocker. Every earlier finding is fixed and reproduced. Fix finding 1 before merge: the change that keeps the entry introduced it, and the fix is small. The doc items in finding 2 are optional.

[tarakanof]

Re-check 2 fixes (656eca5)

Finding	Fix
1. A partial RemoveAll left an indexed version with no cinder.bin	purgeLocked now renames each copy to .tmp-<v>-*, .old- asides first and <v>/ last, and stops at the first rename that fails. A failed rename touches nothing, so the entry stays indexed, whole and blocked (500, retryable). Once <v>/ is renamed away, the entry is dropped and the version unblocked. The retired dirs are then deleted best effort, and the store logs any leftover (it now takes the app logger). Boot sweeps .tmp- dirs: the name isn't semver, so it never loads, and asideVersion never matches it. Asides are always gone before <v>/, so the pre-replace guarantee still holds. The index only ever holds whole dirs, so /bin, available/auto offers and same-bytes re-uploads can't see a half-removed version.
Tests	TestFirmwareStoreDeleteThatStopsPartWayLeavesNoHalfVersion: removeAll deletes cinder.bin and then fails. The DELETE gives 204, unblocks, and the version is no longer listed or offered. A same-bytes re-upload gives 201 and the bin opens. After a restart the version loads and no leftover remains. ...EvictionThatStopsPartWayLeavesNoHalfVersion covers the same for eviction. ...BootNeverRestoresARetiredCopy leaves a whole copy in .tmp-, and boot neither loads nor keeps it. HTTP: TestFirmwareDeleteThatStopsPartWayNeverOffersAHalfVersion (204, unblocked, /bin 404, re-upload 201, /bin 200). Tests for a stuck DELETE or eviction now inject rename failures.
2a. openapi DELETE example	Reworded to "a directory that failed to load at boot". A failed removal now keeps the version listed and whole.
2b. ARCHITECTURE "stays indexed"	Now "an indexed version stays indexed, whole and blocked", inside the new retire description.
2c. openapi upload	"stays blocked and listed".
2d. ARCHITECTURE orphan line	Record paragraph rewrapped.

Mutation check: going back to RemoveAll-based purging makes 8 tests fail, the new partial and boot tests among them.

gofmt -l cmd internal          # clean
go vet ./...                   # clean
go test ./... -race -count=1   # all ok
redocly lint docs/openapi.yaml # valid; 1 warning that is already on main

[tarakanof]

Opus re-check 3

This re-checks 656eca5 against re-check 2 and every earlier round. I ran throwaway HTTP tests in a scratch copy, restarting with a fresh App on the same DB and firmware dir, then deleted the copy. Nothing was pushed.

gofmt -l cmd internal is clean, go vet ./... is clean and go test -race -count=1 ./... passes (cmd/ember 55s).

Earlier findings, reproduced over HTTP plus a restart

Finding	Result
Opus 1 / Codex 1, DELETE	Fixed. Blocked A, then ?replace=1 B with a stuck aside: 201, unblocked. DELETE with the .old- rename failing: 500, still listed with B's SHA and blocked. After a restart B loads and the aside is swept. Variant where the renames work but RemoveAll of .tmp- fails: 204, two .tmp-0.9.14-* leftovers (one holds A), and nothing loads after a restart.
Opus 1 / Codex 1, eviction	Fixed. Five more uploads: 201 each, 0.9.1 stays listed with B, B loads after a restart. With the renames working and .tmp- removal failing, the next eviction leaves .tmp- leftovers, and 0.9.1 does not come back after a restart.
Opus 2 / Codex 2	Fixed. Double rename failure: 500, hidden, blocked. ?replace=1 retry: 201, unblocked. B loads after a restart and no aside is left.
Opus 3	Fixed. Unindexed 0.9.14/ plus a stuck .old-: 500, block kept. Once renames work: 404, unblocked, only .old-0.9.14-rc1-* left (no cross-match).
Re-check 1, !exists path	Fixed. Indexed: plain C gets 409, and ?replace=1 C with a failing tmp → 0.9.14 rename rolls back (500), so B is indexed and loads after a restart. Unindexed with the aside stuck: 500, both dirs intact, B loads after a restart. Unindexed with the aside clearing and the rename failing: 500, the dir is empty, and nothing loads after a restart.
Re-check 2 #2, #3	Fixed. A failed DELETE answers 500 and the version stays listed and blocked, and the retry answers 204 and unblocks. An unindexed target answers 409 and its files stay. After target: null the DELETE answers 404 and the version is purged.
Re-check 2 blocker, half-removed version	Fixed. Auto mode, 0.9.14 blocked, removeAll deletes cinder.bin and then fails. DELETE: 204, unblocked, not listed, available null, no auto offer, /bin 404, one .tmp-0.9.14-* leftover. A same-bytes re-upload with removeAll still broken gives 201, /bin 200 with full length, an auto offer and a knob download of 200 with full length. After a restart it loads and the leftover is swept. The eviction variant gives 201, 0.9.1 drops out of the list and /bin answers 404.

Mutation: retiring <v>/ before the asides fails 4 store tests, so the ordering is pinned.

New design, probed

• Crash after the <v>/ rename, before the unblock and before deletion. Simulated with a whole copy moved to .tmp-0.9.14-*. At boot it is not loaded and is swept, and the block is kept. DELETE then answers 404 and unblocks. That is safe: it fails toward blocked. A crash after the asides are retired but before <v>/ is retired leaves B indexed. With both retired, nothing loads.
• Rename collisions and .tmp- naming. Each retire name comes from MkdirTemp plus Remove under s.mu. The only other .tmp-<v>-* creator in s.dir is put, which runs under the same lock, and putELF writes inside <v>/, not in s.dir. copiesLocked and asideVersion never match .tmp-, and recoverAsideLocked only restores .old-. At boot a .tmp- dir fails semverPattern and is removed. If reading it fails, it is still never indexed.
• Index vs crash. The index is in memory and rebuilt from disk, so there is no index save to lose. The block lives in the DB and is cleared only after the rename. Every crash point therefore leaves the version blocked or gone, never unblocked and loadable.
• Logger. ensureStore passes a.logger, which it already uses on the line before. Every NewApp call site passes a non-nil logger, and tests use discardLogger().
• Concurrency. 150 rounds of upload and DELETE ran under -race, with 4 goroutines fetching /bin and 1 running PUT /elf in a loop. /bin only answered 404 or 200 with full length, never 500 or a short body. The ELF PUT answered only 204 or 404, and DELETE always answered 204. The old putELF ENOTEMPTY 500 on DELETE is gone, because the rename doesn't care about a temp file inside <v>/. An open /bin fd survives the rename and the unlink.

Low / nits (none block)

1. Low: retired leftovers are not retried until a restart. A later DELETE or eviction ignores .tmp- dirs, so with a persistent RemoveAll failure they pile up, and usage() counts them. They are logged and swept at boot and can never load. Optional: sweep .tmp- leftovers at the start of purgeLocked, or leave it as is.
2. Nit, docs: ARCHITECTURE says DELETE unblocks "only once its files are gone". With best-effort deletion the files may linger in .tmp-, so "once <version>/ is renamed away" is more exact. The retire paragraph itself already says this.
3. Nit, wrap: openapi upload has a 93-column line ("rename. If the swap fails … moved back; if"), and ARCHITECTURE has a short orphan line ("dev-seed build (cinder's local default with").

Verdict: approve. Every finding from all earlier rounds is fixed and reproduced over HTTP plus a restart. In the new design, the holes I found need a filesystem failure, a crash or both, and none can resurrect a blocked build or serve a broken /bin.