Add a flash_firmware action to push an image to one node #84
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/flash-firmware-action"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
CCS-UHA-25: a Home Assistant action,
campervan.flash_firmware, with a devicepicker (filtered to this integration's own devices) and a file selector,
driven from Developer Tools → Actions. No frontend code.
The uploaded image is used for one session and discarded:
updater/firmware.pygets a small process-global overlay (
set_uploaded_image/clear_uploaded_image,guarded by
UPLOADED_LOCK) thatFirmwareCache.newest_per_nodemerges on top ofwhat's on disk, so the control API,
/api/nodes, and both manifest renderers allsee it for free without their own logic changing. It never touches
image.json.CachedImage.versionbecomesstr | None. ANoneversion is spec section11.1's "fetch unconditionally" entry: the manifest omits the
versionkeyrather than sending
null, the image is served from a literal/uploaded/path segment rather than
Nonein a URL, and the realNoneis threadedthrough to
declined_in_advance, which already returnsNoneforoffered=None(a test now exercises that branch directly).The updater gets one new route,
POST /api/nodes/<node>/flash, handledoutside the existing
_drain/MAX_BODY(64 KiB) path used by every otherroute: it requires
Content-Length(chunked refused), caps the body at 2 MiB(twice a real image, nowhere near enough to fill the disk), and streams to a
temporary file that's deleted once the session ends, win or lose.
On the Home Assistant side, the picked device is resolved back to a node
address through the device registry (
device_identifierinentity.pyisthe only place that scheme is written, so the handler is its inverse) — a
device with no such identifier, or one belonging to no loaded entry of this
integration, is refused before anything is sent, since the selector's filter
only shapes the dashboard and a scripted call isn't bound by it. The uploaded
file is read and its first byte checked against the ESP32 app-image magic
(
0xE9) before anything is sent to the updater either.manifest.jsongainsfile_uploadas a dependency.Worth flagging on its own: the updater reports
started: true/state: finishedfor any session that ran to completion, success orfailure —
startedalone does not mean the node actually took the image.async_flash_firmwareguards on the outcome reason too (updated/returned,mirroring
update.py's ownTOOK), and a test(
test_a_session_that_did_not_take_is_raised) pins this down: a session thatcomes back
"lost"still raises, even thoughstartedisTrue.Tested: the action end to end through a real control API instance (loopback
HTTP, not mocked), a non-image file and an unrecognised/foreign device both
refused before any request is made, the manifest omitting
versionfor anuploaded image,
declined_in_advanceexercised with a genuineNone, thebody cap and the chunked/missing/short-body
Content-Lengthcases, a nodetype outside the registry, a busy link, and the temporary file's cleanup on
both a successful and a failed session.
./tools/check(ruff, mypy --strict,pytest) is green.
test probe - checking if a bare review posts
@ -32,0 +63,4 @@except ValueError as err:raise vol.Invalid(f"{value} is not a node address") from errif not RESERVED_NODE < parsed < BROADCAST:raise vol.Invalid(f"{value} is not a node address")Coverage shows lines 65-66 (the
RESERVED_NODE < parsed < BROADCASTbounds check) are never hit by a test —test_a_bad_node_address_is_refused_by_the_schemaonly exercises the non-numeric branch ("bananas"). This is exactly the duplicated logic the module comment calls out as a drift risk againstupdater.api._node_type; a case like"0x00"or"0xFF"would close the gap and would actually catch that drift if it ever happens.@ -49,0 +115,4 @@entries = call.hass.config_entries.async_loaded_entries(DOMAIN)if not entries:raise HomeAssistantError("There is no bus set up to flash the node over.")Line 118 (
raise HomeAssistantError("There is no bus set up to flash the node over.")) is also uncovered per the coverage report — no test callsflash_firmwarewith zero loaded config entries. Minor, but worth a quick test alongside the others intest_flash_firmware.py.Summary: checked this against spec section 11.1 (fetched the live doc — the "fetched unconditionally" wording is quoted correctly in the code comments), ran ruff/ruff format/mypy --strict/pytest from the worktree (all green, 1201 passed), and traced the one-shot-upload constraint, the magic-byte-before-network ordering, and the flash route's body handling end to end. No correctness bug or design-constraint violation found. Two inline comments already posted above (services.py coverage gaps); one more below. All are non-blocking. (The earlier "test probe" comment on this PR was a tooling hiccup, not a real review — please disregard it.)
@ -32,0 +63,4 @@except ValueError as err:raise vol.Invalid(f"{value} is not a node address") from errif not RESERVED_NODE < parsed < BROADCAST:raise vol.Invalid(f"{value} is not a node address")Coverage shows lines 65-66 (the
RESERVED_NODE < parsed < BROADCASTbounds check) are never hit by a test —test_a_bad_node_address_is_refused_by_the_schemaonly exercises the non-numeric branch ("bananas"). This is exactly the duplicated logic the module comment calls out as a drift risk againstupdater.api._node_type; a case like"0x00"or"0xFF"would close the gap and would actually catch that drift if it ever happens.@ -49,0 +115,4 @@entries = call.hass.config_entries.async_loaded_entries(DOMAIN)if not entries:raise HomeAssistantError("There is no bus set up to flash the node over.")Line 118 (
raise HomeAssistantError("There is no bus set up to flash the node over.")) is also uncovered per the coverage report — no test callsflash_firmwarewith zero loaded config entries. Minor, but worth a quick test alongside the others intest_flash_firmware.py._read_metadatadoesversion=str(recorded["version"])unconditionally. Nothing in this branch writes anullversion toimage.jsontoday (confirmed:set_uploaded_imagenever touches it,storealways has a real version), so this isn't reachable — butCachedImage.versionis nowstr | None, so a straynullon disk would silently become the string"None", a fake-looking version rather than the honest sentinel. Worth a defensivestr(v) if (v := recorded["version"]) is not None else None, though not urgent given the write path guarantees this today.Addressed the review, plus one requested design change on top:
Node picking changed from free text to a device selector. Rob's ask (relayed after the first review pass): typing
0x10from memory isn't "picking the node".services.yaml'snodefield is nowselector: device: filter: integration: campervan, andservices.py::_resolve_noderesolves the chosendevice_idback to a node address through the device registry, usingdevice_identifierfromentity.py(the same scheme the integration already writes) as its inverse. A device with no such identifier, or one belonging to no loaded entry of this integration, is refused withServiceValidationErrorbefore anything reaches the updater — the selector'sfilteronly shapes what the dashboard offers, not what a scripted call can pass, so that boundary is checked server-side. This let me delete the duplicated_node_address()hex-or-decimal validator entirely (finding 1 below is now moot as a result — the code it pointed at doesn't exist any more). Two new tests cover an unknown device id and a device that belongs to a different integration/entry.Finding 3 (
_read_metadata'sstr(recorded["version"])): fixed. Anullon disk now staysNonerather than becoming the string"None", with a test (test_a_null_version_on_disk_reads_as_none_not_the_word_none) forcing the case by hand-editingimage.json.Finding 2 / general coverage: the "no config entries loaded" path is now folded into
_resolve_node's single check (an entry with nothing loaded can't own any device, so it's the same branch as "wrong device"), and is covered by the same two new tests. While closing coverage gaps I also found theflashroute's own untested edges — a node type in bounds but outside theNoderegistry, a body shorter than its declaredContent-Length, and the link reporting busy — and added tests for those too.updater/firmware.pyand the flash-route paths inupdater/api.pyare now fully covered.All green:
ruff check,ruff format --check,mypy custom_components tests tools,pytest(1206 passed). Ready for another pass.Round 2. The device-selector rework (
d52e234) checks out:_resolve_nodecorrectly requires the device's owning entry to be a loadedcampervanentry (device.config_entries & loaded.keys()), which covers both the "wrong integration" and "unloaded entry" cases, and theTOOK_THE_IMAGE/import-cycle comment traces out correctly (update.pyimportsCampervanConfigEntryfrom__init__.py, which importsservices.py, soservices.pyimportingupdate.pywould cycle). All three round-1 nice-to-haves are addressed by65b8f26; two of them (the node-address-bounds-check gap and the "no bus set up" branch) are actually moot now since the free-text validator they were about was deleted outright by the device-selector change.ruff check,ruff format --check,mypy custom_components tests tools, andpytestare all green (1206 passed) from this worktree.One new finding, found while tracing the upload overlay end to end — not introduced by this round's commits but not caught in round 1 either, so flagging now.
@ -31,0 +83,4 @@try:return int(hex_node, 16), loaded[owner]except ValueError:continueNon-blocking: the task brief's third suggested case — a real
campervandevice whose owning config entry has since been unloaded — isn't directly tested. The logic looks correct by inspection (the entry drops out ofasync_loaded_entrieson unload, so the intersection in_resolve_nodegoes empty and this raises), but neither existing test (unknown device,device from another integration) actually exercises that specific path. A test that unloads the entry and then callsflashwith the staledevice.idwould pin it down.Separately, coverage shows lines 85-86 (
except ValueError: continue) are never hit —device_identifieris the only writer of this scheme and always emits valid hex, so this is defensive-only. Not worth a test on its own, just noting it's dead code as things stand.@ -206,0 +246,4 @@with UPLOADED_LOCK:_UPLOADED.pop(node_type, None)clear_uploaded_image(node_type)unconditionally pops whatever is currently registered for that node type, keyed only by node type, not by which flash session put it there.Concrete sequence: request A flashes node 0x10, calls
set_uploaded_image(imageA), then blocks for minutes inself.link.run_update(...). A second flash request for the same node 0x10 arrives while A is still running (double-click, retried service call, whatever) — call it B. B'sflash()callsset_uploaded_image(imageB)(overwriting A's entry), thenself.link.run_update(0x10, imageB)returnsBUSYimmediately (the link only runs one session globally), and B'sfinallycallsclear_uploaded_image(0x10)— which deletes the node's overlay entry entirely, even though A's session is still genuinely in progress.From that point on, if node 0x10 re-polls the manifest mid-OTA (a stalled transfer retrying, for instance),
FirmwareCache.newest_per_nodeno longer has an uploaded entry to overlay and falls back to whatever's on disk — not the image A's session is actually running — which would fail or misdirect a flash that was otherwise proceeding correctly.Fix: have
clear_uploaded_image(or its caller) only remove the entry if it's still the one this call set, e.g. take theCachedImageand compare identity before popping:and pass
imagefromControlApi.flash'sfinally. Narrow to trigger (needs a genuine duplicate/racing flash of the same node), but the fix is small and the failure mode is a firmware session silently losing track of its own image mid-flight.Fixed the overlay-clobber bug, and the dead code.
The bug. Traced it through carefully, and the identity-based
clear_uploaded_imagealone (as first suggested) does not actually close it. Walking the exact interleaving: A sets its overlay and blocks inrun_updatefor minutes; B (same node, retried/double-clicked) sets its own overlay, clobbering A's dict entry; B'srun_updatereturnsBUSYstraight away; B'sfinallythen callsclear_uploaded_image— and since B is still the current occupant of that slot at that point (nothing else has touched it), an identity check against B's own image still says "yes, delete it". The slot is empty for the rest of A's session either way. The identity check is real and worth having (it protects a third session's still-current entry from a late, stale clear — that's what the "set, set again, clear with the first, assert the second survives" test inupdater/firmware.py::clear_uploaded_imagenow covers), but it doesn't stop B's set from clobbering A's slot in the first place.So I added the other half:
ControlApi.flash()now checksself.link.update_state(node_type).state is UpdateState.RUNNINGbefore constructing the image or callingset_uploaded_imageat all. If a session for that exact node type is already running, it's refused immediately with a 409, and the overlay is never touched — no clobber to begin with. This usesupdate_state, which is already public onNodeLinkand already reflects exactly this fact (BusLink.run_updatesetsself._updates[node_type]toRUNNINGsynchronously, before it starts blocking), so nothing inlink.py's locking changed. There's a narrow residual race (two requests for the same node arriving in the same instant, both passing the pre-check before either sets its overlay) that the identity-checked clear still guards against as a fallback — I didn't try to close that race with anything more elaborate, since doing so would mean touching the lock inlink.py, which I was asked not to do.New tests:
test_a_flash_for_a_node_already_running_never_touches_the_overlay(the pre-check, via aFakeLinkseeded as already running) andtest_clearing_a_superseded_upload_does_not_take_the_current_one_with_it(the identity check, via the exact sequence asked for).Dead code: deleted the unreachable
except ValueError: continueinservices.py::_resolve_node—device_identifieris the only writer of that identifier scheme and always emits valid hex, so the branch could never fire. Replaced with a one-line comment saying why, rather than a case that reads as real.Nice-to-have: added
test_a_device_whose_entry_has_been_unloaded_is_refused— a couple of lines, using the entry's ownasync_unloadbefore attempting the flash.All green:
ruff check,ruff format --check,mypy custom_components tests tools,pytest(1209 passed).Closed the residual race — without touching
link.py.You're right that locking around the
update_state()-based pre-check wouldn't actually have worked:update_state()reflects state thatBusLink.run_update()sets under its own lock, and since that call has to happen outside our lock (it blocks for minutes), there's an unavoidable gap between "we release our lock" and "the link's own lock records RUNNING" — a second request could land in exactly that gap and still clobber the first, no matter how tightly we wrapped the check.So instead of locking around a check that reads another module's separately-locked state, I made the reservation self-contained:
updater/firmware.pygetstry_set_uploaded_image(image) -> bool, which checksimage.node_type in _UPLOADEDand sets it in the sameUPLOADED_LOCKacquisition — check and set are one atomic step, entirely within the thing we already own and already lock. No dependency onlink.py's state or its lock at all, so nothing there changes.ControlApi.flash()now calls this instead of the oldupdate_state()peek; on aFalse(slot already taken), it refuses with 409 beforerun_updateis ever called, and before anything is written that a node's manifest fetch could see.set_uploaded_image(the old unconditional setter) stays, since several tests — including the deliberately-overwriting one for the identity-clear fix — still want unconditional-overwrite semantics for setting up test state;try_set_uploaded_imageis the one real code reaches for.Proved it rather than just asserting it:
test_only_one_concurrent_reservation_for_a_node_type_ever_winsfires 50 threads attry_set_uploaded_imagefor the same node type through aBarrier(maximum contention) and asserts exactly one wins. Ran it five times in a row locally with no flake. Also rewrotetest_a_flash_for_a_node_already_running_never_touches_the_overlay(now..._with_an_upload_already_in_flight_is_refused) to seed the collision through_UPLOADEDdirectly rather than throughlink.update_state(), since that's the actual mechanism now.This didn't require reaching into
link.pyat all, so no need to check back on that front — the residual window is gone, not just narrowed.All green:
ruff check,ruff format --check,mypy custom_components tests tools,pytest(1210 passed).Added the short comment on the
/update-versus-flash window, per the "out of scope, note it" call — inControlApi.flash(), right where the reservation is held andrun_updateis about to be called. No behaviour change. All green (ruff,mypy,pytest, 1210 passed).Waiting on the outstanding review round against
f3e71f3now for0d1a2ffas well.Round 3. Reviewed against the current tip,
0d1a2ff(two commits landed on the branch while this review was in progress, on top of thece1ad12/239d28fpair the brief named — noted below since they matter to the verdict).The round-2 overlay-clobber race.
ce1ad12alone (checkinglink.update_state(node_type)before touching the overlay, then relying on the identity-checkedclear_uploaded_imageas a backstop) does not close it. I traced the exact interleaving and then reproduced it mechanically: two threads callingControlApi.flash()for the same node, synchronised with athreading.Barrierso both cross theupdate_statepre-check before either is recorded asRUNNING. Both pass the check, both build aCachedImage(thesha256_ofhash is real, non-trivial work — that's the window), and whichever'sset_uploaded_imageruns last wins the overlay slot. In a losing ordering the loser's ownfinally: clear_uploaded_image()then removes that slot by identity while the winner's session is still live and minutes from finishing — the identity check does not protect against this, because at that moment the loser's image genuinely is the current occupant. Confirmed empirically (not just reasoned): the overlay went toNonemid-session in a live repro run, and the reverse ordering (wrong image left in the slot for the winning session) is reachable too by the same mechanism.f3e71f3("Make the flash overlay reservation atomic, not just its clear") does close it —try_set_uploaded_imagemakes the check-and-reserve one atomic step underUPLOADED_LOCK, so a losing request is refused before it ever writes to_UPLOADED. Re-ran the same barrier-synchronised repro against this commit five times: the overlay consistently reflects only the winning session's own image throughout, and the loser gets a 409 with_UPLOADEDuntouched.test_only_one_concurrent_reservation_for_a_node_type_ever_wins(50 threads on a barrier, asserts exactly onetry_set_uploaded_imagesucceeds) is a good direct test of this and would fail if the check-then-set were ever split back into two lock scopes.0d1a2ff's comment about the remainingupdate()-vs-flash()window (a regular auto-update session for a node, racing a manual flash for the same node, sinceupdate()never touches_UPLOADED) checks out on tracing — it's real but genuinely bounded to one function call (try_set_uploaded_imagesucceeding whilerun_update's own single global run-slot then immediately refusesflash()as busy), not the minutes-long window the original bug had. Leaving it documented rather than unifying the reservation across both routes is a reasonable call.test_clearing_a_superseded_upload_does_not_take_the_current_one_with_it: still meaningful. It exercisesset_uploaded_image/clear_uploaded_imagedirectly rather than throughControlApi.flash(), and that path (a stalefinallyclear landing after two later sessions have already come and gone) is now only reachable through significant scheduling jitter rather than the everyday race — but it's stillclear_uploaded_image's own contract, worth keeping as a unit test regardless of how hard the full-stack path is to hit.Dead code removal (
239d28f): confirmed genuinely dead.device_identifier()inentity.py(f"node-{node:02x}"from anint) is the only writer of that identifier scheme — grepped foridentifiers=and(DOMAIN,acrosscustom_components/campervan/, nothing else constructs one.Nice-to-have, not blocking:
set_uploaded_image(the unconditional setter) is no longer called from any production path afterf3e71f3—api.pynow only callstry_set_uploaded_image. It's still used directly by several tests to seed state, which is a fine reason to keep it, but worth a beat of thought on whether it should be documented as test-only or folded away.ruff check,ruff format --check,mypy custom_components tests tools updater, andpytest(1210 passed) are all green at0d1a2ff. No blocking findings on the current tip.Re-reviewed at
0d1a2ff(f3e71f3plus the follow-up comment). The atomicity fix itself is correct:try_set_uploaded_imagechecksimage.node_type in _UPLOADEDand sets it inside oneUPLOADED_LOCKacquisition,ControlApi.flash()builds the image (environment resolved, sha256 computed) before calling it and treatsFalseas a clean 409 with nothing touched, andclear_uploaded_imagein thefinallyis still the identity-checked version. Traced both threads through end to end — no window where two flash requests for the same node type can both believe they hold the slot, or where one clobbers the other.ruff check,ruff format --check,mypy custom_components tests toolsandpytest -qare all green at this commit (1210 passed).One real finding on the new concurrency test itself:
test_only_one_concurrent_reservation_for_a_node_type_ever_wins(tests/test_firmware_cache.py:416) doesn't actually catch the bug it's meant to guard against. I revertedtry_set_uploaded_imagelocally to the pre-fix shape — check under oneUPLOADED_LOCKacquisition, release, then set under a second acquisition, no work in between — and ran this test 20 times. It passed all 20 (won.count(True) == 1every time). Only when I added atime.sleep(0.001)between the two acquisitions did it reliably fail (50/50 wins). The problem is CPython's default GIL switch interval (sys.getswitchinterval()= 5ms here): the gap between two back-to-back, work-free lock acquisitions is far shorter than that, so on a 24-core box the first thread through routinely completes the whole check-then-set sequence before the GIL ever hands off to a second thread — real OS threads and aBarrierfor contention don't help if the critical section is too short for a switch to land in it. So as written, the test would not catch a regression back to the exact non-atomic shape this branch was written to fix — it's currently more a smoke test than a proof of atomicity. Atime.sleep(even a trivial one) inserted between check and set inside the reservation under test, or asserting via a monkeypatched/instrumented version that forces a yield between the two steps, would make it a real trap again.Judgement call, not a finding I'd block on: the
/update-vs-flash window0d1a2ffdocuments inupdater/api.py(reservation held beforerun_updatelearns of an already-running/updatesession for the same node, closed only oncerun_updatereturnsBUSY) is real but is a different, much narrower bug class than the one this round targeted — one function call wide rather than minutes — and it's now explained in a comment at the point it occurs. I agree with leaving it open rather than unifying the two routes' locking for it.Both fixed.
1. The concurrency test now forces the interleaving it needs, and I verified it. Replaced the 50-threads-and-a-
Barrierversion (which, you're right, never actually races on a critical section that short) with adictsubclass standing in for_UPLOADEDwhose__contains__sleeps 50ms before answering.try_set_uploaded_image's check calls straight into that, so with the correct single-acquisition implementation the whole check-and-set stays inside oneUPLOADED_LOCKhold and a second thread just blocks on the lock for the duration — only one winner, sleep or not. I verified it the way you asked: temporarily revertedtry_set_uploaded_imageto the two-acquisition shape (check under onewith UPLOADED_LOCK:, set under a second), ran the test three times, watched it fail2 == 1every time, then restored the fix and confirmed it passes five times in a row. That revert-and-watch-it-fail step was manual, not committed — only the working version and the pause-forcing dict subclass are in the diff.2.
set_uploaded_imagedeleted.updater/firmware.pyno longer has it. The three test files that used it for setup now either calltry_set_uploaded_image(the ordinary case — slot's empty, reservation should succeed, so it doubles as a cheap assertion) or, in the one test that deliberately wants an overwritetry_set_uploaded_imageexists to prevent (test_clearing_a_superseded_upload_does_not_take_the_current_one_with_it), writefirmware._UPLOADED[LIGHTING] = seconddirectly with a comment saying why it's reaching past the guard on purpose.All green:
ruff check,ruff format --check,mypy custom_components tests tools,pytest(1210 passed).