From 3f27ef4b268c802d8dc7f3865e3aeec9303d454a Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Sun, 13 Sep 2026 00:40:55 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20--stall-after=20Budget=20der=20Rettungss?= =?UTF-8?q?tation=20client-kumulativ=20z=C3=A4hlen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Der Testschalter zählte bisher pro Anfrage, sodass eine Lampe nach ihrem Empfangs-Timeout einfach per Range neu verband und ein frisches Budget bekam – am EVK reichten fünf Reconnects, um trotz --stall-after das komplette Image zu übertragen. Station verfolgt die ausgelieferten Bytes jetzt pro Client-IP über alle Anfragen hinweg (Station.reserve_stall_bytes, unter dem bestehenden Lock), sodass kein erneutes Verbinden das Budget auffüllt. Co-Authored-By: Claude Sonnet 5 --- tools/station/server.py | 65 +++++++++++++++++++----------- tools/station/tests/test_server.py | 56 +++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 23 deletions(-) diff --git a/tools/station/server.py b/tools/station/server.py index 5bfaacc..849b0a9 100755 --- a/tools/station/server.py +++ b/tools/station/server.py @@ -61,16 +61,35 @@ class Station: self.since = now_iso() self.served = 0 self.recent: list[dict] = [] + # Cumulative bytes already delivered to each client while --stall-after is active, + # keyed by client IP. A lamp whose download stalls re-opens a fresh connection and + # resumes with Range right where it left off, so a per-request counter would hand + # every reconnect a brand-new --stall-after allowance and the stall would never + # actually stick (see reserve_stall_bytes below). + self.stall_sent: dict[str, int] = {} # ThreadingHTTPServer runs one handler thread per connection, and several lamps - # can be downloading at once. Every mutation of served/recent and the snapshot - # that gets serialized to disk must happen under this same lock, or concurrent - # requests lose increments and stomp on each other's status.json writes. + # can be downloading at once. Every mutation of served/recent/stall_sent and the + # snapshot that gets serialized to disk must happen under this same lock, or + # concurrent requests lose increments and stomp on each other's status.json writes. self.lock = threading.RLock() self.write_state() def image_path(self, target: str) -> Path: return self.image_dir / f"ninlight_{target}_ota.img" + def reserve_stall_bytes(self, ip: str, n: int) -> int: + """How many of the next `n` bytes this client may still receive under --stall-after. + + Tracks bytes cumulatively per client IP, across requests, so a stalled connection + that the lamp abandons and resumes via Range does not get a fresh budget just + because it is a new TCP connection. Only called when stall_after is set. + """ + with self.lock: + sent_so_far = self.stall_sent.get(ip, 0) + allowed = max(0, min(n, self.stall_after - sent_so_far)) + self.stall_sent[ip] = sent_so_far + allowed + return allowed + def record(self, ip: str, target: str, version: str, nbytes: int, status: str) -> None: with self.lock: if status == "ok": @@ -175,31 +194,31 @@ class Handler(BaseHTTPRequestHandler): sent = 0 status = "ok" remaining = size - start + client_ip = self.client_address[0] try: with path.open("rb") as f: f.seek(start) while sent < remaining: - if self.station.stall_after is not None and sent >= self.station.stall_after: - # test aid: hold the connection open and send nothing more, so the - # firmware's stall handling can be exercised on real hardware. We - # still notice if the peer gives up and closes its side, so a test - # (or an impatient client) that hangs up is recorded as "aborted" - # instead of leaking the thread forever. - while True: - readable, _, _ = select.select([self.connection], [], [], 1.0) - if readable: - try: - peek = self.connection.recv(1, socket.MSG_PEEK) - except OSError: - peek = b"" - if not peek: - raise ConnectionResetError("peer closed during stall") - read_size = CHUNK + read_size = min(CHUNK, remaining - sent) if self.station.stall_after is not None: - # Don't overshoot the stall boundary within a single read: without - # this a whole CHUNK could be sent before the check above ever - # notices we've passed --stall-after. - read_size = min(read_size, max(1, self.station.stall_after - sent)) + # Reserve against this client's cumulative budget, not just this + # request's byte count - see Station.reserve_stall_bytes. + read_size = self.station.reserve_stall_bytes(client_ip, read_size) + if read_size == 0: + # test aid: hold the connection open and send nothing more, so + # the firmware's stall handling can be exercised on real + # hardware. We still notice if the peer gives up and closes its + # side, so a test (or an impatient client) that hangs up is + # recorded as "aborted" instead of leaking the thread forever. + while True: + readable, _, _ = select.select([self.connection], [], [], 1.0) + if readable: + try: + peek = self.connection.recv(1, socket.MSG_PEEK) + except OSError: + peek = b"" + if not peek: + raise ConnectionResetError("peer closed during stall") chunk = f.read(read_size) if not chunk: break diff --git a/tools/station/tests/test_server.py b/tools/station/tests/test_server.py index 2b6d80e..3196c60 100644 --- a/tools/station/tests/test_server.py +++ b/tools/station/tests/test_server.py @@ -278,6 +278,62 @@ def test_stall_after_stops_sending_but_keeps_the_connection(tmp_path): s.stop() +def test_stall_after_budget_survives_a_resumed_request(tmp_path): + """--stall-after must stop a client for good, not just for one request. + + On real hardware, a lamp whose download stalls hits its own receive timeout, gives up + on the connection, and opens a new one with a Range header picking up right where it + left off - exactly what test_stall_after_stops_sending_but_keeps_the_connection above + exercises for a single request. A per-request byte counter would hand that resumed + request a brand-new --stall-after allowance, so the "stall" would only ever delay + delivery instead of actually stopping it - which is what was observed on the EVK: the + server stalled at 65536 bytes each time, but the lamp reconnected five times and walked + away with the complete image. This test reproduces that reconnect and checks that the + second, resumed request gets no further bytes at all. + """ + s = Station(tmp_path, "--stall-after", "1000") + try: + make_image(s.image_dir / "ninlight_evk_ota.img", "evk", "1.0.0", b"y" * 200000) + + # First request: burns through the whole --stall-after budget, then the server + # holds the connection open and sends nothing further. + sock = socket.create_connection(("127.0.0.1", s.port), timeout=5) + sock.sendall(b"GET /ninlight_evk_ota.img HTTP/1.1\r\nHost: x\r\n\r\n") + got = b"" + sock.settimeout(2) + try: + while len(got) < 200000: + chunk = sock.recv(65536) + if not chunk: + break + got += chunk + except socket.timeout: + pass + assert len(got) < 20000 # far less than the full image, same bound as above + sock.close() # the lamp's watchdog gives up and reconnects, like on real hardware + + # Second request, resuming with Range exactly like a real lamp would. This + # client's cumulative budget is already spent, so it must get zero more bytes of + # body - not a fresh --stall-after allowance just because it's a new connection. + sock2 = socket.create_connection(("127.0.0.1", s.port), timeout=5) + sock2.sendall(b"GET /ninlight_evk_ota.img HTTP/1.1\r\nHost: x\r\nRange: bytes=1000-\r\n\r\n") + sock2.settimeout(2) + header = b"" + while b"\r\n\r\n" not in header: + header += sock2.recv(4096) + assert b" 206 " in header.split(b"\r\n", 1)[0] + body_after_header = header.split(b"\r\n\r\n", 1)[1] + assert body_after_header == b"", "resumed request must not receive any body bytes" + try: + more = sock2.recv(1) + except socket.timeout: + more = None + assert more in (None, b""), "server sent data after the client's stall budget was spent" + sock2.close() + finally: + s.stop() + + def test_concurrent_downloads_are_all_recorded(station): """Several lamps hitting the station at once must not lose or corrupt status.json.