Serve the manifest and cached images to the nodes #56

Claude · 2026-09-19T08:25:03Z

Claude commented

2026-09-19 08:25:03 +00:00

CCS-UHA-4. The node-facing half of an update session, spec section 11: a node
joins the hotspot, fetches /manifest.json, finds itself by node type, and
fetches its image.

The manifest is ours to define — it never goes on the bus — and is a single
object holding one entry per node type:

{
  "images": [
    {
      "environment": "node_lighting",
      "node_type": 16,
      "path": "/firmware/node_lighting/2026.9.3/node_lighting-2026.9.3-firmware.bin",
      "sha256": "2cf24d...",
      "size": 421376,
      "version": "2026.9.3"
    }
  ]
}

node_type is byte 0 of the node's own SYS_ANNOUNCE, so no node carries
configuration to find its image, and sha256 is what it checks the bytes
against before switching boot slot. No version field: an unknown key is ignored
rather than rejected, as everywhere else here.

Only the newest image the cache holds for a node type is offered, and only an
offered image can be fetched. A request path is looked up against the paths the
manifest advertises rather than joined onto the cache root, so a request has no
way out of the tree. There is no authentication, deliberately: the trust
boundary is the network, which during an update is a hotspot with a per-session
key.

Bound on every interface at port 8080, not on the hotspot's address — the
ticket said otherwise, but it also says this has to be reachable over the
hotspot and the ordinary LAN alike, and binding one address defeats that. The
read-back hotspot address is only what goes into SYS_WIFI_CONTROL, which is
CCS-UHA-5.

Tested without binding a port: the real server is built without its bind and
requests go through the real handler over a socket pair. The suite gets an
autouse fake in its place, so starting the service in a test no longer puts a
firmware server on the machine running it.

CCS-UHA-6 adds its own loopback control API to updater/; the only shared file
is the two lines in updater/service.py that start each server.

CCS-UHA-4. The node-facing half of an update session, spec section 11: a node joins the hotspot, fetches `/manifest.json`, finds itself by node type, and fetches its image. The manifest is ours to define — it never goes on the bus — and is a single object holding one entry per node type: ```json { "images": [ { "environment": "node_lighting", "node_type": 16, "path": "/firmware/node_lighting/2026.9.3/node_lighting-2026.9.3-firmware.bin", "sha256": "2cf24d...", "size": 421376, "version": "2026.9.3" } ] } ``` `node_type` is byte 0 of the node's own `SYS_ANNOUNCE`, so no node carries configuration to find its image, and `sha256` is what it checks the bytes against before switching boot slot. No version field: an unknown key is ignored rather than rejected, as everywhere else here. Only the newest image the cache holds for a node type is offered, and only an offered image can be fetched. A request path is looked up against the paths the manifest advertises rather than joined onto the cache root, so a request has no way out of the tree. There is no authentication, deliberately: the trust boundary is the network, which during an update is a hotspot with a per-session key. Bound on every interface at port `8080`, not on the hotspot's address — the ticket said otherwise, but it also says this has to be reachable over the hotspot and the ordinary LAN alike, and binding one address defeats that. The read-back hotspot address is only what goes into `SYS_WIFI_CONTROL`, which is CCS-UHA-5. Tested without binding a port: the real server is built without its bind and requests go through the real handler over a socket pair. The suite gets an autouse fake in its place, so starting the service in a test no longer puts a firmware server on the machine running it. CCS-UHA-6 adds its own loopback control API to `updater/`; the only shared file is the two lines in `updater/service.py` that start each server.

Claude added 1 commit

2026-09-19 08:25:03 +00:00

Serve the manifest and cached images to the nodes

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

Details

Lint, type check and test / quality (pull_request) Successful in 2m14s

Details

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

Details

00b65ba421

Claude reviewed

2026-09-19 08:31:12 +00:00

Claude left a comment

serve_firmware can hang the updater forever on stop, and the lifecycle tests can't see it because they run the fake. Details inline.

`serve_firmware` can hang the updater forever on stop, and the lifecycle tests can't see it because they run the fake. Details inline.

tests/test_node_file_server.py

					
				@ -0,0 +237,4 @@

					assert [reply.status for reply in climbs] == [404, 404, 404]

					assert not any(b"not for a node" in reply.body for reply in climbs)

This and test_a_port_that_will_not_bind_does_not_take_the_service_down run against FakeFileServer, not the real one: the autouse no_real_listening_socket fixture in conftest.py patches file_server.FirmwareFileServer, and serve_firmware looks the name up on the module. So the assertion here is that threading.Event.set() unblocks threading.Event.wait(), which is why the hang above passes CI.

The module docstring's "the server is the real one" only holds for the request tests. Cover the real stop path — unpatching this module and binding 127.0.0.1:0, or injecting the server into serve_firmware, either works.

This and `test_a_port_that_will_not_bind_does_not_take_the_service_down` run against `FakeFileServer`, not the real one: the autouse `no_real_listening_socket` fixture in `conftest.py` patches `file_server.FirmwareFileServer`, and `serve_firmware` looks the name up on the module. So the assertion here is that `threading.Event.set()` unblocks `threading.Event.wait()`, which is why the hang above passes CI. The module docstring's "the server is the real one" only holds for the request tests. Cover the real stop path — unpatching this module and binding `127.0.0.1:0`, or injecting the server into `serve_firmware`, either works.

updater/file_server.py

					
				@ -0,0 +55,4 @@

				FIRMWARE_PREFIX = "/firmware"

				EVERY_INTERFACE = ""

Nothing asserts this. EVERY_INTERFACE could be changed to the hotspot address and the whole suite would still pass, yet reachability over both the hotspot and the ordinary LAN is the requirement. FirmwareFileServer(cache, bind_and_activate=False).server_address == ("", NODE_FILE_SERVER_PORT) costs nothing and takes no port.

Nothing asserts this. `EVERY_INTERFACE` could be changed to the hotspot address and the whole suite would still pass, yet reachability over both the hotspot and the ordinary LAN is the requirement. `FirmwareFileServer(cache, bind_and_activate=False).server_address == ("", NODE_FILE_SERVER_PORT)` costs nothing and takes no port.

updater/file_server.py Outdated

					
				@ -0,0 +121,4 @@

							body = image.path.read_bytes()

						except OSError as error:

							_LOGGER.warning("Could not read %s: %s", image.path, error)

							self.send_error(HTTPStatus.NOT_FOUND)

An advertised image that will not read answers 404, the same as one that was never offered, so a node cannot tell "not for me" from "try again later". 500 is the honest answer here.

updater/file_server.py

					
				@ -0,0 +133,4 @@

						self.end_headers()

						self.wfile.write(body)

					def log_message(self, format: str, *args: Any) -> None:

log_message is routed to the logger but handle_error is not, so the one failure that will actually happen in the van escapes it. A node walking off the hotspot mid-image raises BrokenPipeError out of do_GET (confirmed against the real handler with a 4 MB image and a closed peer), and socketserver prints a traceback to stderr. Override handle_error to log it at info — it is an ordinary outcome, not a fault.

`log_message` is routed to the logger but `handle_error` is not, so the one failure that will actually happen in the van escapes it. A node walking off the hotspot mid-image raises `BrokenPipeError` out of `do_GET` (confirmed against the real handler with a 4 MB image and a closed peer), and `socketserver` prints a traceback to stderr. Override `handle_error` to log it at info — it is an ordinary outcome, not a fault.

updater/file_server.py Outdated

					
				@ -0,0 +182,4 @@

					try:

						await stop.wait()

					finally:

						server.shutdown()

server.shutdown() on the event loop can hang the process forever. await stop.wait() does not yield if stop is already set, so the serving task never gets its first step, serve_forever is never entered, and shutdown() blocks on an event only serve_forever sets. The loop is blocked synchronously, so asyncio.wait_for cannot break it either.

Reproduced through service.serve by setting the stop one scheduling hop earlier than a signal does:

serving = asyncio.create_task(service.serve(settings, stop))
asyncio.get_running_loop().call_soon(stop.set)
await asyncio.wait_for(serving, timeout=5)  # never returns

Today's signal path happens to be safe only because the two tasks land in _ready ahead of the stop.set callback. That is scheduling luck, not a guarantee, and the failure is a hard hang until Docker's SIGKILL at 30 s.

Even on the happy path this blocks the loop for up to serve_forever's 0.5 s poll interval. await asyncio.to_thread(server.shutdown) fixes both: the flag is set synchronously, the wait moves off the loop, and the serving task is then free to enter and exit serve_forever.

`server.shutdown()` on the event loop can hang the process forever. `await stop.wait()` does not yield if `stop` is already set, so the `serving` task never gets its first step, `serve_forever` is never entered, and `shutdown()` blocks on an event only `serve_forever` sets. The loop is blocked synchronously, so `asyncio.wait_for` cannot break it either. Reproduced through `service.serve` by setting the stop one scheduling hop earlier than a signal does: ```python serving = asyncio.create_task(service.serve(settings, stop)) asyncio.get_running_loop().call_soon(stop.set) await asyncio.wait_for(serving, timeout=5) # never returns ``` Today's signal path happens to be safe only because the two tasks land in `_ready` ahead of the `stop.set` callback. That is scheduling luck, not a guarantee, and the failure is a hard hang until Docker's SIGKILL at 30 s. Even on the happy path this blocks the loop for up to `serve_forever`'s 0.5 s poll interval. `await asyncio.to_thread(server.shutdown)` fixes both: the flag is set synchronously, the wait moves off the loop, and the serving task is then free to enter and exit `serve_forever`.

rob added 1 commit

2026-09-19 08:34:51 +00:00

Take the review: stop cleanly, and log a request that died

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

Details

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

Details

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

Details

da1c5ca2ad

Claude reviewed

2026-09-19 08:37:49 +00:00

Claude left a comment

All five are addressed. Reverting the to_thread and re-running tests/test_node_file_server.py fails test_stopping_before_the_serving_thread_ran_still_stops, so that one bites, and faulthandler_timeout turns a regression into a stack dump rather than a hung job. One small thing left, inline.

All five are addressed. Reverting the `to_thread` and re-running `tests/test_node_file_server.py` fails `test_stopping_before_the_serving_thread_ran_still_stops`, so that one bites, and `faulthandler_timeout` turns a regression into a stack dump rather than a hung job. One small thing left, inline.

updater/file_server.py Outdated

					
				@ -0,0 +169,4 @@

						outcome, and the next session fetches it again.

						"""

						_LOGGER.info(

							"Request from %s did not finish: %r", client_address, sys.exception()

%r on sys.exception() gives the type and message but no stack, so a genuine bug in do_GET now logs one line with no line number and surfaces nowhere else — on a box in a van with no network that is all you get. exc_info=True keeps the traceback for the unexpected case and costs nothing for the BrokenPipeError this is really for.

`%r` on `sys.exception()` gives the type and message but no stack, so a genuine bug in `do_GET` now logs one line with no line number and surfaces nowhere else — on a box in a van with no network that is all you get. `exc_info=True` keeps the traceback for the unexpected case and costs nothing for the `BrokenPipeError` this is really for.

rob added 1 commit

2026-09-19 08:39:07 +00:00

Log the stack with a request that died, not just its type

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

Details

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

Details

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

Details

4b20336cef

Claude reviewed

2026-09-19 08:40:05 +00:00