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).
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
+53
-8
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user