From 262041795ceba86ea53cab9e2ba4e21ea3c3ec54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D0=BD=D1=82=D0=BE=D0=BD=20=D0=A2=D1=80=D0=BE=D1=88?= =?UTF-8?q?=D0=B8=D0=BD?= Date: Wed, 14 Oct 2026 15:34:12 +0300 Subject: [PATCH] v3.28.0 audit: the mid-equip race The review of the connection-check fix found the second-order race it opened: _apply_equipped_card sets _CARD_CONNECTED = True and bumps card_session *before* it clears and reads the ICCID (and long before the network state / TERMINAL PROFILE / drain finish). A worker poll landing in that window saw connected + a fresh session + iccid = None and consumed the card with a bogus 'no ICCID' skip - the card was never installed. The `equipping` flag could not guard it either: the manual /api/command equip path never sets it. The fix marks the finished init instead of guessing: - _apply_equipped_card sets `server.card_ready_session = server.card_session` as its last statement; - the startup init (which inlines the post-equip work and never runs _apply_equipped_card) sets the same marker; - the worker requires the marker to match the card session, so it acts only on a fully initialized card - a genuinely unreadable ICCID still acts and reports its skip, as designed. Tests: a worker test that keeps the marker stale for a couple of polls (no attempt - verified to fail without the guard), the marker pins for both equip paths, and the fake server carries the marker (a real attribute) instead of inventing state. The unused `server` parameter of _ram_batch_start is gone, and the two comments that called this work a "review" now say audit. 915 frontend + 924 Python; sw cache simple-v438 (unchanged). --- pysim_simple_server/__main__.py | 4 +++ pysim_simple_server/server.py | 21 +++++++++--- tests/test_ram_batch.py | 61 ++++++++++++++++++++++++++++----- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/pysim_simple_server/__main__.py b/pysim_simple_server/__main__.py index 4dab33d..b6981a0 100644 --- a/pysim_simple_server/__main__.py +++ b/pysim_simple_server/__main__.py @@ -324,6 +324,10 @@ def main(): server.stk_pending = None server.card_present = card is not None server.card_session = 1 if card is not None else 0 + # The startup init inlines the post-equip work (it never runs + # _apply_equipped_card): the batch worker's ready marker must be set + # here too (v3.28.0 audit). + server.card_ready_session = server.card_session server.iccid = iccid # EF.ARR cache (v3.24.0): read by the startup init above (a card present # at server start); a later equip refreshes it in _apply_equipped_card. diff --git a/pysim_simple_server/server.py b/pysim_simple_server/server.py index 8d99de4..8a755b0 100644 --- a/pysim_simple_server/server.py +++ b/pysim_simple_server/server.py @@ -2701,8 +2701,13 @@ def _ram_batch_worker(state): session = getattr(server, 'card_session', 0) if server is not None else None # The connection state is the module global the whole server uses # (_apply_equipped_card / _handle_card_disconnect); the server - # object itself has no `connected` attribute (v3.28.0 review). - connected = bool(server is not None and _CARD_CONNECTED) + # object itself has no `connected` attribute. The equip must also + # have finished (`card_ready_session`, set at the end of + # _apply_equipped_card and by the startup init) - acting mid-equip + # would consume the card session with a bogus 'no ICCID' skip + # (v3.28.0 audit). + connected = bool(server is not None and _CARD_CONNECTED + and getattr(server, 'card_ready_session', None) == session) with _RAM_BATCH_LOCK: if state['stop']: break @@ -2732,9 +2737,9 @@ def _ram_batch_worker(state): sys.stderr.write('RAM-BATCH: %s after %d attempts\n' % (phase, total)) -def _ram_batch_start(server, body): +def _ram_batch_start(body): """Validate the CAP/parameters (the plan is card independent, so it is built - once) and spawn the worker. Raises ValueError (400) for a bad request and + once) and spawn the worker (the worker reads `_server_ref` itself). Raises ValueError (400) for a bad request and RuntimeError (409) when another batch or a test run is active.""" global _RAM_BATCH with _RAM_BATCH_LOCK: @@ -5106,6 +5111,12 @@ def _apply_equipped_card(server): server.sim_menu = sm server.event_list = el _tlog('equip: terminal profile done') + # The card session is now fully initialized (the ICCID read, the network + # state, the TERMINAL PROFILE and its drain are done). The batch worker + # waits for this marker before acting: _CARD_CONNECTED flips True before + # the ICCID is read, so a poll landing mid-equip would consume the session + # with a bogus 'no ICCID' skip (v3.28.0 audit). + server.card_ready_session = server.card_session def _card_reset_reinit(server, label='RESET'): @@ -10038,7 +10049,7 @@ class PysimHandler(BaseHTTPRequestHandler): body = self._read_body() self._log_req(body) try: - _ram_batch_start(self.server, body) + _ram_batch_start(body) except ValueError as e: resp = {'error': str(e)} self._send_json(resp, 400) diff --git a/tests/test_ram_batch.py b/tests/test_ram_batch.py index 6a778f1..57826ab 100644 --- a/tests/test_ram_batch.py +++ b/tests/test_ram_batch.py @@ -57,13 +57,17 @@ def _preset(iccid, name='Card', keyset=1): 'tars': [tar]} -def _fake_server(iccid, store, session=1): +def _fake_server(iccid, store, session=1, ready=None): """A server-shaped object carrying only the attributes the real server object has. The connection state is the module global `_CARD_CONNECTED` - (v3.28.0 review: a fake `connected` attribute here hid a bug - the worker - read it and never saw a card).""" + (v3.28.0 audit: a fake `connected` attribute here hid a bug - the worker + read it and never saw a card); `card_ready_session` is the marker + _apply_equipped_card sets when the session is fully initialized (it + defaults to the session, i.e. a finished equip).""" return SimpleNamespace(iccid=iccid, card_presets=store, - card_session=session, scc=object()) + card_session=session, + card_ready_session=session if ready is None else ready, + scc=object()) class BatchStateTests(unittest.TestCase): @@ -268,6 +272,47 @@ class RamInstallPlanTests(unittest.TestCase): class BatchWorkerSourceTests(unittest.TestCase): + def test_the_worker_waits_for_a_finished_equip(self): + """_CARD_CONNECTED flips True (and the session bumps) before the ICCID + is read: acting mid-equip would consume the card with a bogus 'no + ICCID' skip. The worker must wait for the ready marker (v3.28.0 + audit).""" + st = srv._ram_batch_new_state(_plan()) + preset = _preset('8970119000004600098', 'Alfa') + srv._RAM_BATCH = st + # the session is bumped but the equip has not finished yet + srv._server_ref = _fake_server('8970119000004600098', _FakeStore([preset]), + session=9, ready=8) + calls = [] + with mock.patch.object(srv, '_CARD_CONNECTED', True), \ + mock.patch.object(srv, '_ram_install_run', + lambda *a, **k: calls.append(1) or {'success': True}): + t = threading.Thread(target=srv._ram_batch_worker, args=(st,), daemon=True) + t.start() + time.sleep(0.8) # a couple of polls mid-equip + self.assertEqual(calls, []) + self.assertEqual(st['attempts'], []) + srv._server_ref.card_ready_session = 9 # the equip finished + for _ in range(60): + if st['attempts']: + break + time.sleep(0.05) + srv._ram_batch_stop() + t.join(timeout=3) + srv._server_ref = None + self.assertEqual(len(calls), 1) + self.assertEqual(st['attempts'][0]['result'], 'installed') + + def test_the_ready_marker_is_set_on_every_equip_path(self): + """_apply_equipped_card marks the finished session, and the startup + init (which never runs it) sets the same marker (v3.28.0 audit).""" + import inspect + src = inspect.getsource(srv._apply_equipped_card) + self.assertIn('server.card_ready_session = server.card_session', src) + main = (PROJECTS / 'simple' / 'pysim_simple_server' / '__main__.py').read_text( + encoding='utf-8') + self.assertIn('server.card_ready_session = server.card_session', main) + def test_the_worker_reads_the_module_connection_state(self): """The server object has no `connected` attribute - the worker must read `_CARD_CONNECTED`, the global the rest of the server uses @@ -288,17 +333,17 @@ class BatchLifecycleTests(unittest.TestCase): def test_start_refuses_a_second_batch_and_a_test_run(self): with mock.patch.object(srv, '_ram_install_plan', lambda body: _plan()), \ mock.patch.object(srv, '_ram_batch_worker', lambda state: None): - st = srv._ram_batch_start(None, {'cap_hex': 'AA'}) + st = srv._ram_batch_start({'cap_hex': 'AA'}) self.assertTrue(st['active']) with self.assertRaises(RuntimeError): - srv._ram_batch_start(None, {'cap_hex': 'AA'}) + srv._ram_batch_start({'cap_hex': 'AA'}) srv._ram_batch_stop() # a test run blocks the start srv._RAM_BATCH = None with mock.patch.object(srv, '_ram_install_plan', lambda body: _plan()), \ mock.patch.object(srv, '_TEST_RUNNING', True): with self.assertRaises(RuntimeError): - srv._ram_batch_start(None, {'cap_hex': 'AA'}) + srv._ram_batch_start({'cap_hex': 'AA'}) def test_the_status_payload_shape(self): idle = srv._ram_batch_status() @@ -308,7 +353,7 @@ class BatchLifecycleTests(unittest.TestCase): self.assertIsNone(srv._ram_batch_summary()) with mock.patch.object(srv, '_ram_install_plan', lambda body: _plan()), \ mock.patch.object(srv, '_ram_batch_worker', lambda state: None): - srv._ram_batch_start(None, {'cap_hex': 'AA'}) + srv._ram_batch_start({'cap_hex': 'AA'}) srv._ram_batch_record(srv._RAM_BATCH, {'iccid': '8970119000004600098', 'result': 'installed'}) st = srv._ram_batch_status()