merge(runtime): integrate process and task state fixes

tests/unit/admin_console/test_task_queue_service.py

@@ -52,7 +52,7 @@ def clean_state(tmp_path, monkeypatch):
     state.current_profile = None
     state.active_session_id = None
     state.active_connections.clear()
-    state.was_stopped_manually = False
+    state.manually_stopped_run_ids.clear()
     state.cancelled_session_ids.clear()
     if state.worker_task and not state.worker_task.done():
         state.worker_task.cancel()
@@ -66,7 +66,7 @@ def clean_state(tmp_path, monkeypatch):
     state.current_process = None
     state.current_goal = None
     state.current_profile = None
-    state.was_stopped_manually = False
+    state.manually_stopped_run_ids.clear()
     if hasattr(state, "recent_submissions"):
         state.recent_submissions.clear()
     if state.worker_task and not state.worker_task.done():
@@ -448,7 +448,7 @@ def test_stop_tasks_terminates_external_global_owner_and_preserves_local_waiter(
     local_waiter.kill.assert_not_called()
     assert state.current_process is local_waiter
     assert state.queue_items[0]["session_id"] == "frontend-waiter"
-    assert state.was_stopped_manually is False
+    assert not state.manually_stopped_run_ids
     assert "frontend-waiter" not in state.cancelled_session_ids
     assert "mcp-session" not in state.active_connections
     update_status.assert_called_once()
@@ -921,6 +921,111 @@ async def test_enqueue_tasks_deduplicates_by_session_id():
         assert res2["tasks"][0]["session_id"] == "sid-dedup-1"
 
 
+@pytest.mark.asyncio
+async def test_manual_stop_of_one_run_does_not_pollute_concurrent_run():
+    """Stopping run A must not flip run B's terminal status or its payload.
+
+    Regression test for the process-global ``was_stopped_manually`` flag that
+    used to mark *every* in-flight run as manually stopped. Two concurrent runs
+    on different devices: device A's task is stopped manually, device B's task
+    then finishes normally and must still be reported as completed.
+    """
+    ended_payloads: dict[str, dict] = {}
+
+    def capture(event_type, data):
+        if event_type == "session_ended":
+            ended_payloads[str(data.get("session_id"))] = dict(data)
+
+    class FakeProc:
+        def __init__(self, pid):
+            self.pid = pid
+            self.returncode = None
+            self.stdout = None
+            self._done = asyncio.Event()
+
+        async def wait(self):
+            await self._done.wait()
+            return self.returncode
+
+        def finish(self, returncode):
+            self.returncode = returncode
+            self._done.set()
+
+        def kill(self):
+            self.finish(-9)
+
+    procs: dict[str, FakeProc] = {}
+
+    async def fake_subprocess_exec(*args, **kwargs):
+        goal = args[3]
+        # Use our own pid so the reaped-process watchdog keeps waiting.
+        proc = FakeProc(os.getpid())
+        procs[goal] = proc
+        return proc
+
+    item_a = {
+        "session_id": "run-a",
+        "goal": "Goal A",
+        "status": "pending",
+        "device_serial": "dev-a",
+    }
+    item_b = {
+        "session_id": "run-b",
+        "goal": "Goal B",
+        "status": "pending",
+        "device_serial": "dev-b",
+    }
+    state.queue_items = [item_a, item_b]
+    state.ipc_subscribers.append(capture)
+
+    try:
+        with (
+            patch("asyncio.create_subprocess_exec", side_effect=fake_subprocess_exec),
+            patch("apps.admin_console.services.task_queue_service.session_repo") as mock_repo,
+            patch("apps.admin_console.services.task_queue_service.media_service"),
+            patch(
+                "apps.admin_console.services.task_queue_service.process_supervisor.terminate_tree",
+                return_value=True,
+            ),
+        ):
+            mock_repo.get_session_status.return_value = "running"
+            mock_repo.get_video_recording_for_session.return_value = {"status": "ready"}
+
+            task_a = asyncio.create_task(TaskQueueService._execute_task_item(item_a))
+            task_b = asyncio.create_task(TaskQueueService._execute_task_item(item_b))
+            for _ in range(40):
+                if "run-a" in state.active_runs and "run-b" in state.active_runs:
+                    break
+                await asyncio.sleep(0.02)
+            assert {"run-a", "run-b"} <= set(state.active_runs)
+            # B launched last, so the legacy single-task mirror points at B.
+            assert state.active_session_id == "run-b"
+
+            # Manually stop device A's task without naming its session id.
+            assert task_queue_service.stop_tasks(clear_all=False, device_id="dev-a") is True
+            await asyncio.wait_for(task_a, timeout=5.0)
+
+            # B finishes normally afterwards.
+            procs["Goal B"].finish(0)
+            await asyncio.wait_for(task_b, timeout=5.0)
+
+        assert ended_payloads["run-a"]["status"] == "cancelled"
+        assert ended_payloads["run-a"]["was_stopped_manually"] is True
+        assert ended_payloads["run-b"]["status"] == "completed"
+        assert ended_payloads["run-b"]["was_stopped_manually"] is False
+
+        persisted = {
+            call.args[0]: call.args[1]
+            for call in mock_repo.update_session_status.call_args_list
+        }
+        assert persisted.get("run-a") == "cancelled"
+        assert persisted.get("run-b") == "completed"
+        assert "run-b" not in state.cancelled_session_ids
+        assert "run-b" not in state.manually_stopped_run_ids
+    finally:
+        state.ipc_subscribers.remove(capture)
+
+
 @pytest.mark.asyncio
 async def test_enqueue_tasks_debounces_rapid_identical_submissions():
     state.queue_items = []

tests/unit/mcp/test_trace_store.py

@@ -14,9 +14,12 @@
 
 """Unit tests for MCP trace store."""
 
+import glob
+import logging
 import os
 import shutil
 import tempfile
+import threading
 import uuid
 import pytest
 
@@ -91,6 +94,125 @@ def test_read_nonexistent_status(temp_trace_env):
     assert trace_store.read_status(non_existent) is None
 
 
+def test_leftover_atomic_temp_file_does_not_affect_reads(temp_trace_env):
+    """A crash between temp-write and replace leaves a .tmp file that readers ignore."""
+    trace_id = str(uuid.uuid4())
+    trace_store.init_trace(trace_id, "Crash sim", "Flash")
+
+    status_path = trace_store.get_status_path(trace_id)
+    # Simulate a writer that crashed after writing its temp file.
+    with open(f"{status_path}.{uuid.uuid4().hex}.tmp", "w", encoding="utf-8") as f:
+        f.write('{"status": "half-writ')
+
+    data = trace_store.read_status(trace_id)
+    assert data is not None
+    assert data["status"] == "running"
+    assert data["task_desc"] == "Crash sim"
+
+
+def test_failed_atomic_replace_preserves_original(temp_trace_env):
+    """If the atomic replace fails, the previous status.json stays intact."""
+    from unittest.mock import patch
+
+    trace_id = str(uuid.uuid4())
+    trace_store.init_trace(trace_id, "Original", "Flash")
+
+    def broken_replace(src, dst):
+        raise OSError("simulated replace failure")
+
+    with patch.object(trace_store, "_replace_with_retry", broken_replace):
+        with pytest.raises(OSError):
+            trace_store.write_status(trace_id, {"status": "clobbered"})
+
+    data = trace_store.read_status(trace_id)
+    assert data is not None
+    assert data["task_desc"] == "Original"
+    # The failed writer's temp file must not linger.
+    assert glob.glob(os.path.join(trace_store.get_trace_dir(trace_id), "*.tmp")) == []
+
+
+def test_corrupt_status_is_quarantined_and_read_returns_none(temp_trace_env, caplog):
+    trace_id = str(uuid.uuid4())
+    trace_store.init_trace(trace_id, "Corrupt me", "Flash")
+
+    status_path = trace_store.get_status_path(trace_id)
+    with open(status_path, "w", encoding="utf-8") as f:
+        f.write('{"status": "running", "trace_id"')  # torn write
+
+    with caplog.at_level(logging.WARNING, logger="mcp_server.utils.trace_store"):
+        assert trace_store.read_status(trace_id) is None
+
+    assert any("Corrupt status.json" in rec.message for rec in caplog.records)
+    assert os.path.exists(f"{status_path}.corrupt")
+    assert not os.path.exists(status_path)
+
+
+def test_missing_status_stays_silent(temp_trace_env, caplog):
+    with caplog.at_level(logging.WARNING, logger="mcp_server.utils.trace_store"):
+        assert trace_store.read_status(str(uuid.uuid4())) is None
+    assert caplog.records == []
+
+
+def test_update_on_corrupt_status_logs_and_drops(temp_trace_env, caplog):
+    trace_id = str(uuid.uuid4())
+    trace_store.init_trace(trace_id, "Corrupt update", "Flash")
+
+    status_path = trace_store.get_status_path(trace_id)
+    with open(status_path, "w", encoding="utf-8") as f:
+        f.write("not json at all")
+
+    with caplog.at_level(logging.WARNING, logger="mcp_server.utils.trace_store"):
+        assert trace_store.update_trace_status(trace_id, "completed") is None
+
+    assert any("Dropping status update" in rec.message for rec in caplog.records)
+    # Updating a trace that never existed stays a silent no-op.
+    caplog.clear()
+    with caplog.at_level(logging.WARNING, logger="mcp_server.utils.trace_store"):
+        assert trace_store.update_trace_status(str(uuid.uuid4()), "completed") is None
+    assert not any("Dropping status update" in rec.message for rec in caplog.records)
+
+
+def test_concurrent_read_modify_write_does_not_lose_updates(temp_trace_env):
+    """Interleaved RMW updates from two writers must not lose each other's fields."""
+    trace_id = str(uuid.uuid4())
+    trace_store.init_trace(trace_id, "Concurrency", "Flash")
+    iterations = 25
+    failures: list[BaseException] = []
+
+    def set_errors():
+        try:
+            for i in range(iterations):
+                assert trace_store.update_trace_status(
+                    trace_id, "running", error=f"err-{i}"
+                ) is not None
+        except BaseException as exc:  # noqa: BLE001 - surfaced in the main thread
+            failures.append(exc)
+
+    def set_serials():
+        try:
+            for i in range(iterations):
+                assert trace_store.update_trace_device_serial(
+                    trace_id, f"serial-{i}"
+                ) is not None
+        except BaseException as exc:  # noqa: BLE001 - surfaced in the main thread
+            failures.append(exc)
+
+    threads = [threading.Thread(target=set_errors), threading.Thread(target=set_serials)]
+    for t in threads:
+        t.start()
+    for t in threads:
+        t.join(timeout=30.0)
+
+    assert failures == []
+    final = trace_store.read_status(trace_id)
+    assert final is not None
+    # Every serialized RMW preserves the other writer's latest field, so both
+    # final values must survive regardless of which thread finished last.
+    assert final["error"] == f"err-{iterations - 1}"
+    assert final["device_serial"] == f"serial-{iterations - 1}"
+    assert final["status"] == "running"
+
+
 def test_trace_paths(temp_trace_env):
     trace_id = "test-paths-id"
     trace_dir = trace_store.get_trace_dir(trace_id)

tests/unit/runtime/test_device_lock.py

@@ -185,6 +185,78 @@ def test_submission_reservations_preserve_order_before_workers_start():
     thread.join(timeout=2.0)
 
 
+def test_pending_reservation_at_queue_head_does_not_block_other_devices():
+    """An unclaimed pending submission must not gate every device's queue head.
+
+    Regression: a reservation whose device is still "pending" used to be
+    counted into every device's FIFO queue, so one unclaimed head-of-queue
+    ticket blocked all devices at once. It must now park on at most one idle
+    candidate device (the first in sorted order) and leave the others
+    schedulable.
+    """
+    pending_ticket = DeviceExecutionLock.reserve("unclaimed submission")
+    # device-alpha is a known idle device (a queued submission targets it), so
+    # the pending ticket parks there; device-beta must stay schedulable.
+    alpha_ticket = DeviceExecutionLock.reserve(
+        "task for device alpha", device_id="device-alpha"
+    )
+    try:
+        beta = DeviceExecutionLock("device-beta", "task for device beta")
+        # Must acquire immediately even though the older pending ticket is at
+        # the head of the global queue directory.
+        beta.acquire(blocking=False)
+        beta.release()
+    finally:
+        DeviceExecutionLock.cancel_reservation(pending_ticket)
+        DeviceExecutionLock.cancel_reservation(alpha_ticket)
+
+
+def test_pending_reservation_is_served_with_original_fifo_position_after_claim():
+    """A pending reservation is not starved: once its worker claims a device it
+    keeps its original submission-order position in that device's queue."""
+    holder = DeviceExecutionLock("emulator-5554", "current holder")
+    holder.acquire()
+
+    # Submitted while the device is busy; the reservation stays "pending".
+    first_ticket = DeviceExecutionLock.reserve("first submitted task")
+
+    claimed = DeviceExecutionLock("emulator-5554", "claimed pending task", first_ticket)
+    youngest = DeviceExecutionLock("emulator-5554", "youngest task")
+    order = []
+    claimed_acquired = threading.Event()
+    youngest_acquired = threading.Event()
+
+    def run_claimed():
+        claimed.acquire()
+        order.append("claimed")
+        claimed_acquired.set()
+
+    def run_youngest():
+        youngest.acquire()
+        order.append("youngest")
+        youngest_acquired.set()
+
+    t1 = threading.Thread(target=run_claimed)
+    t2 = threading.Thread(target=run_youngest)
+    t1.start()
+    t2.start()
+    time.sleep(0.2)
+    assert not claimed_acquired.is_set()
+    assert not youngest_acquired.is_set()
+
+    holder.release()
+    # The claimed pending ticket kept its original (oldest) FIFO timestamp and
+    # must win the device before the younger concrete ticket.
+    assert claimed_acquired.wait(timeout=2.0)
+    assert not youngest_acquired.is_set()
+    claimed.release()
+    assert youngest_acquired.wait(timeout=2.0)
+    youngest.release()
+    t1.join(timeout=2.0)
+    t2.join(timeout=2.0)
+    assert order == ["claimed", "youngest"]
+
+
 def test_cancelled_reservation_cannot_execute():
     ticket = DeviceExecutionLock.reserve("cancelled task")
     assert DeviceExecutionLock.cancel_reservation(ticket)

tests/unit/runtime/test_process_probe.py

@@ -0,0 +1,65 @@
+# Copyright 2026 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unit tests for the unified PID liveness probe."""
+
+import os
+from unittest.mock import MagicMock, patch
+
+import psutil
+
+from artemis.runtime.process_probe import pid_is_alive
+
+
+def test_own_pid_is_alive():
+    assert pid_is_alive(os.getpid()) is True
+
+
+def test_own_pid_with_matching_create_time_is_alive():
+    created_at = psutil.Process(os.getpid()).create_time()
+    assert pid_is_alive(os.getpid(), created_at) is True
+
+
+def test_recycled_pid_with_mismatched_create_time_is_dead():
+    assert pid_is_alive(os.getpid(), 1.0) is False
+
+
+def test_invalid_pids_are_dead():
+    assert pid_is_alive(0) is False
+    assert pid_is_alive(-5) is False
+    assert pid_is_alive(None) is False  # type: ignore[arg-type]
+    assert pid_is_alive("not-a-pid") is False  # type: ignore[arg-type]
+
+
+def test_no_such_process_is_dead():
+    with patch("psutil.Process", side_effect=psutil.NoSuchProcess(424242)):
+        assert pid_is_alive(424242) is False
+
+
+def test_zombie_process_is_dead():
+    proc = MagicMock()
+    proc.is_running.return_value = True
+    proc.status.return_value = psutil.STATUS_ZOMBIE
+    with patch("psutil.Process", return_value=proc):
+        assert pid_is_alive(1234) is False
+
+
+def test_access_denied_defaults_to_alive():
+    with patch("psutil.Process", side_effect=psutil.AccessDenied(1234)):
+        assert pid_is_alive(1234) is True
+
+
+def test_unexpected_error_defaults_to_alive():
+    with patch("psutil.Process", side_effect=RuntimeError("boom")):
+        assert pid_is_alive(1234) is True