Don't let an unreachable radio restart-loop the updater #85

Claude · 2026-09-21T09:30:19Z

Claude commented

2026-09-21 09:30:19 +00:00

On a host where AppArmor refuses the container's D-Bus connection to NetworkManager, radio.recover() in run() raised RadioError unguarded, so main() exited non-zero and compose restarted the container forever.

recover() is best-effort tidy-up of a hotspot and uplink an earlier run died holding, not a precondition — nothing else the service does needs the radio, and a session that needs a hotspot already fails on its own. It's now guarded the same way restore() already is, with one clear _LOGGER.error line at start saying firmware updates won't work on this host, rather than a stack trace repeating every restart.

restore() on the way out is downgraded to a debug line with no traceback when the radio was already known unreachable from recover(), so stopping doesn't repeat the same traceback for no new information.

Fixes CCS-UHA-26.

Tested: rewrote the existing test_a_radio_that_will_not_answer_stops_the_service_saying_why (whose old assertion — that run() raises — was the bug's contract) to cover the new behaviour, and added a test that stopping produces no traceback for a radio unreachable from the start. Full suite passes (1211 tests).

On a host where AppArmor refuses the container's D-Bus connection to NetworkManager, `radio.recover()` in `run()` raised `RadioError` unguarded, so `main()` exited non-zero and compose restarted the container forever. `recover()` is best-effort tidy-up of a hotspot and uplink an earlier run died holding, not a precondition — nothing else the service does needs the radio, and a session that needs a hotspot already fails on its own. It's now guarded the same way `restore()` already is, with one clear `_LOGGER.error` line at start saying firmware updates won't work on this host, rather than a stack trace repeating every restart. `restore()` on the way out is downgraded to a debug line with no traceback when the radio was already known unreachable from `recover()`, so stopping doesn't repeat the same traceback for no new information. Fixes CCS-UHA-26. Tested: rewrote the existing `test_a_radio_that_will_not_answer_stops_the_service_saying_why` (whose old assertion — that `run()` raises — was the bug's contract) to cover the new behaviour, and added a test that stopping produces no traceback for a radio unreachable from the start. Full suite passes (1211 tests).

Claude added 1 commit

2026-09-21 09:30:19 +00:00

Don't let an unreachable radio restart-loop the updater

Lint, type check and test / hassfest (pull_request) Successful in 14s

Details

Lint, type check and test / quality (pull_request) Successful in 1m49s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

5d34c5b8e5

AppArmor can refuse the container's D-Bus connection to NetworkManager,
which made radio.recover() raise RadioError straight out of run() and
exit the container non-zero for ever. recover() is best-effort tidy-up,
not a precondition, so it's now guarded the same way restore() already
is, with one clear line saying firmware updates won't work on this host.

restore() on the way out is downgraded to a debug line with no traceback
when the radio was already known unreachable, so stopping doesn't repeat
the same stack trace for no new information.

Claude reviewed

2026-09-21 09:34:50 +00:00

Claude left a comment

Verified the fix end to end: with a FakeNmcli refusing connection show, run() now reaches serve(), stops cleanly on SIGTERM, returns 0, and logs one ERROR line naming the host as broken for updates; restore()'s repeat failure on the way out logs at DEBUG with no exc_info. unreachable is a local in run() so it can't leak between invocations. No flag threaded into serve()/UpdateSession. Tabs, British spelling, ruff and the full test_updater_radio.py suite (48 tests) all clean.

Two small, non-blocking points:

Verified the fix end to end: with a `FakeNmcli` refusing `connection show`, `run()` now reaches `serve()`, stops cleanly on SIGTERM, returns 0, and logs one ERROR line naming the host as broken for updates; `restore()`'s repeat failure on the way out logs at DEBUG with no `exc_info`. `unreachable` is a local in `run()` so it can't leak between invocations. No flag threaded into `serve()`/`UpdateSession`. Tabs, British spelling, ruff and the full `test_updater_radio.py` suite (48 tests) all clean. Two small, non-blocking points:

tests/test_updater_radio.py

					
				@ -644,1 +644,4 @@

					nmcli.refuse = "connection show"

					running = asyncio.create_task(run(Settings(tmp_path, ""), Radio(nmcli)))

					await asyncio.sleep(0)

					if running.done():

This asyncio.sleep(0) + if running.done(): running.result() guard is new to the file (other tests just await asyncio.sleep(0) and send the signal). It's doing real work — it protects against create_task+immediate-raise_signal sending a real SIGTERM before the task has run far enough to install its handlers, and it turns a regression back to the unguarded-recover() bug into a clean re-raised RadioError inside the test instead of a killed pytest process. Worth a one-line comment saying that, since a future editor is likely to "simplify" it away as dead code.

This `asyncio.sleep(0)` + `if running.done(): running.result()` guard is new to the file (other tests just `await asyncio.sleep(0)` and send the signal). It's doing real work — it protects against `create_task`+immediate-`raise_signal` sending a real SIGTERM before the task has run far enough to install its handlers, and it turns a regression back to the unguarded-`recover()` bug into a clean re-raised `RadioError` inside the test instead of a killed pytest process. Worth a one-line comment saying that, since a future editor is likely to "simplify" it away as dead code.

tests/test_updater_radio.py Outdated

					
				@ -650,0 +660,4 @@

				async def test_a_radio_unreachable_from_the_start_gets_one_traceback_not_two(

					tmp_path: Path, caplog: pytest.LogCaptureFixture

These two tests share identical setup (same FakeNmcli, same task, same sleep(0)/done() guard) and only differ in the caplog level and final assertion. Given this repo's preference for less structure, consider folding into one test asserting all three things (return 0, the ERROR line, no exc_info anywhere) under a single caplog.at_level(logging.DEBUG, ...).

These two tests share identical setup (same `FakeNmcli`, same task, same sleep(0)/done() guard) and only differ in the `caplog` level and final assertion. Given this repo's preference for less structure, consider folding into one test asserting all three things (return 0, the ERROR line, no `exc_info` anywhere) under a single `caplog.at_level(logging.DEBUG, ...)`.

rob added 1 commit

2026-09-21 09:36:22 +00:00

Fold the two radio-unreachable tests into one, per review

Lint, type check and test / hassfest (pull_request) Successful in 8s

Details

Lint, type check and test / quality (pull_request) Successful in 1m48s

Details

Lint, type check and test / release (pull_request) Has been skipped

Details

3d1b0c1019

Same setup, differing only in what they asserted; one test with both
assertions is less to maintain. Also notes why the done()/result() guard
matters, for a future editor tempted to simplify it away.

rob merged commit adbf16fb2f into main

2026-09-21 09:41:44 +00:00

rob deleted branch fix/radio-recover-restart-loop

2026-09-21 09:41:44 +00:00

rob referenced this pull request from a commit

2026-09-21 09:41:46 +00:00

Merge pull request 'Don't let an unreachable radio restart-loop the updater' (#85) from fix/radio-recover-restart-loop into main

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project