Wire the update session into the control API #67
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/node-link-runs-sessions"
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?
Fills the seam between the updater's two halves.
updater/link.pyis a realNodeLinkoverupdater/bus.py: it openscan0, listens forSYS_ANNOUNCEso it knows what build each node last announced, and runs
UpdateSessionon arequest.
/api/statusreports the link, and an update request now answers withthe session's own outcome —
updated,unchanged,returned,failedwithits cause, or
lost.POST /api/nodes/<node>/updateanswers when the session finishes, not when itstarts, because the outcome only exists then: a node's return is the one word
on whether the image took, and a rollback is only tellable from a build
identifier that did not change.
POST /api/checkalready made the same choice.GETon the same path says where an update got to without starting one, sorunning, finished and never started stay three separate answers.
The API answers on threads and the session is a coroutine, so the link hands
the session to the service's loop with
run_coroutine_threadsafeand blocksonly the request thread. Nothing on the loop is touched from a handler.
Behavioural notes for a reviewer:
can0isconnected: falseandreason: "no_link", not a crashor a restart loop. The service still serves the API and still caches firmware.
409, usingUpdateSession's ownone-at-a-time rule rather than a second guard.
installed_versionis now what a node announced — auint16and a build id.That does not order against a release's CalVer, so
update_availableis nullrather than a comparison of two unrelated things.
Tested through the fake bus the session's own tests use: no bus at all, nodes
announcing, an update that takes, one that comes back
unchanged, one thatfails with a cause, and a second update asked for while one runs.
CCS-UHA-18.
Six things, most serious first.
The service wiring has no test. Delete
link=linkfromControlApi(...)inupdater/service.pyand the whole suite still passes, 1053 green — the API falls back toNoLink()and every install in the van answersno_link.test_the_service_gives_the_api_a_link_to_the_busonly checks a task namednode-linkexists, which is not what its docstring claims. Assertcontrol.linkis theBusLink, or drive a POST through the API the service built.A busy refusal reports the wrong node's state. With lighting updating,
POST /api/nodes/0x12/updateanswers{"node": "bathroom", "state": "running"}, andGETon the same path a moment later saysnever_started.ALREADY_RUNNINGshould beUpdateState.NEVERfor the node asked about, or the busy branch inapi.updateshould render state fromlink.update_state(node_type).Build zero still reads as a rollback.
_hearmapsbuild or None, butsession._watch(session.py:468) takes a mid-session announce's build verbatim, and_returnedthen compares0 == 0and answersunchanged.FakeNode(build=0, before=0)gives outcomeunchanged, "the node came back on build 00000000, the one it went in on", and HA raises a rollback at the user. A node answeringQRY_DESCRIPTORSmid-session is the path, and the session's own docstring says that happens. session.py is outside this diff, but the rule this PR adds is only applied on one of the two waysbeforegets set.Moving the install's answer to the end of the session broke what
async_update_asked_forwas for (update.py:174). It exists to clear a stale state 6 when a retry is asked for; it is now only reached minutes later, and only on success, by which time the node's own announce has already cleared it throughOtaStore._announced. Sofetch_failed: truefrom the previous attempt stays showing for the whole of the retry.test_asking_for_another_update_clears_the_last_failurestill passes only because the mock returnsupdatedwith no announce on the bus, which cannot happen in the van.Nothing tests shutdown during a session —
_stop_the_session,GAVE_UP/STOPPED, and theCancelledErrorbranch of_waited(link.py 204-205, 237-238, 244). It does work; it is also the path that stops a request thread hanging for ever, so it should not be the uncovered one._stop_the_sessionreadsself._runningwithoutself._held. Run it between_schedulereturning andself._running = runningand the new future is never cancelled, so the request thread blocks onrunning.result()for ever. Narrow, and the thread is a daemon, but the fix is onewith self._held:.This clears a stale state 6 so a retry does not show the previous attempt's failure. Now that the POST answers at the end of the session, it is reached minutes late and only on success, when the node's announce has already cleared it. The stale
fetch_failed: trueshows for the whole retry. Move it above theasync_installcall, or drop it.@ -0,0 +40,4 @@STOPPED = "stopped""""Why an update has no outcome: the service was asked to stop during it."""ALREADY_RUNNING = UpdateStatus(UpdateState.RUNNINGhere is the other node's session. A busyPOST /api/nodes/0x12/updateanswers{"node": "bathroom", "state": "running"}whileGETon the same path saysnever_started. UseUpdateState.NEVER, or render the busy branch inapi.updatefromlink.update_state(node_type).@ -0,0 +93,4 @@One at a time is `UpdateSession`'s own rule rather than one invented here,because one hotspot serves one node. This holds the session it started sothat a second request can be told, rather than left to find out by beingThe reason is wrong:
UpdateSession.runchecksself._runningbeforeraise_hotspot(), so a second request is never refused after the network went up. The real reason for this guard is that_runningis a single slot — a second session would overwrite the handle the shutdown path cancels.@ -0,0 +232,4 @@def _stop_the_session(self) -> None:"""Cancel an update still running, so nothing waits on a loop that has gone."""running = self._runningReads
self._runningoutsideself._held. Between_schedulereturning and the assignment inrun_updatethis sees the old value, the new future is never cancelled, and the request thread waits onrunning.result()for ever.@ -41,3 +49,4 @@control = ControlApi(cache=FirmwareCache(settings.firmware_cache),releases=ForgejoReleases(settings.registry_token),link=link,Not covered: removing this line leaves the whole suite green, and every install in the van would answer
no_link.Taken, in
701bbb2:BusLink. Droppinglink=linkfails it.ALREADY_RUNNINGnow saysnever_started, which is the refused node's own state.GETon the same path agrees.session.py: a mid-session announce of build zero no longer replaces what there was to judge by, and_returnedreads a returning zero as nothing to judge against. New session test, plus the link-level one.test_a_service_stopped_during_an_update_lets_the_request_gocovers the cancel: stop set mid-session, the held request thread comes backfinished/stopped._stop_the_sessionnow takes the lock.The class docstring's reason for the busy answer is rewritten — you are right that the session refuses before raising the hotspot, and the real reason is that there is one slot because there is one session.
On 4,
async_update_asked_forstays where it is, deliberately. Clearing before the request would wipe a genuine state 6 when the updater answersno_linkand nothing is sent at all, which is the case the test below it pins. During a real session the node's own state 4 clears the flag on the way to writing, so the call at the end only covers the case where no state 4 was heard — a missed frame, or a session that ended before Home Assistant saw one. I have renamed the test and rewritten its docstring, which claimed the old reasoning.Second round. Two of the fixes have no test holding them down; both mutations leave all 1055 green.
The
_watchhalf of the build-zero fix is the half that hides a rollback, and nothing covers it. Putsession.py:470back toget("build", build)and the suite still passes, butFakeNode(before=0, build=OLD_BUILD)run withbuild=OLD_BUILDthen answersupdatedfor a node that came back on the build it went in on.test_a_build_of_zero_identifies_nothing_and_judges_nothingcannot catch it because_returned's own zero guard maps the returning zero to None first, so both paths reachRETURNEDeither way. A case where the return is a real build is what separates them.ALREADY_RUNNINGback toUpdateState.RUNNINGalso leaves the suite green —test_a_second_update_while_one_is_running_is_refusedchecks the status, the outcome and the wording, but not the refused node's own state, which was the finding. One more assertion onrefused.payload["state"].On 4, your reasoning holds and the existing
test_an_install_that_sent_nothing_leaves_the_last_failure_showingis the case that settles it — clearing in front of the request would wipe a genuine state 6 that nothing had overtaken. The ~30 seconds of stale flag before the node reports state 4 is a fair price. No change wanted.@ -0,0 +43,4 @@ALREADY_RUNNING = UpdateStatus(# Never started, because this is the node that was refused rather than the# one holding the hotspot. Its own state says nothing has run for it.UpdateState.NEVER,Not covered:
UpdateState.RUNNINGhere still passes the whole suite. Addassert refused.payload["state"] == UpdateState.NEVERtotest_a_second_update_while_one_is_running_is_refused.@ -468,1 +468,3 @@build = received.values.get("build", build)# Zero is the build script's own "not from a checkout"# and identifies nothing, so it replaces nothing.build = received.values.get("build") or buildNot covered. Reverting this to
get("build", build)keeps every test green, and a node that announces zero mid-session and then rolls back reads asupdated— the rollback is lost, which is the one thing nothing else on the bus reports. A test with the return on a real build (before=0,build=OLD_BUILD, callerbuild=OLD_BUILD, expectUNCHANGED) separates this from_returned's guard.Clean. Both new assertions bite: reverting
_watchtoget("build", build)fails onlytest_an_announce_of_no_build_leaves_the_one_to_judge_by_standing, andALREADY_RUNNINGback toUpdateState.RUNNINGfails onlytest_a_second_update_while_one_is_running_is_refused. Nothing further from me.