Files
model-platform/runtime/tests/test_process.py
T
tao.chenandClaude Fable 5 4acfbb162f fix: P0-4 schedule node janitor + runtime reaper/lock invariants
S2: schedule worker add janitor task that force-terminals node_runs
whose deadline (timeout_seconds + retry_count*retry_interval + 120s
slack from started_at) has passed. Closes the gap where outbox retry
exhaustion (5 tries, capped 30s backoff) marked the *event* failed
but left the *node_run* stuck in queued/running forever. Re-reads the
row under FOR UPDATE before writing so a worker that races us to a
real terminal state is not overwritten; idempotency key uses
:timed_out variant so the :finished path cannot collide.

R1: extract _reap_once() from _reap_loop for testability; in the
dead-process branch, re-verify (process.pid, started_at) against the
live JUPYTER_PROCESSES entry before del. A start_workspace that
replaced the dead record mid-cycle used to have its new entry
silently erased by the reaper's stale snapshot — leaked the port.

R2: delete _drop_workspace_lock and its two call sites
(stop_workspace tail, get_workspace 404 path). Popping the lock
object after release breaks mutual exclusion for any coroutine still
holding the old reference while a fresh caller gets a new lock
object — same ws_id can race two starts. The dict is bounded by the
number of workspaces so the leak is negligible; invariant lives on
WORKSPACE_LOCKS in a comment.

Tests:
- schedule/tests/test_janitor.py — 8 tests covering normal kill /
  healthy-skip / worker-race / never-started / multi-row batch /
  cancellation propagation / per-iteration self-heal
- runtime/tests/test_process.py — 7 tests covering reaper identity
  match / replacement-skip / alive-preserved + lock
  same-object / concurrent-serialize / survives-stop /
  helper-removed guard

uv run --package schedule pytest schedule/tests → 14 passed
uv run --package runtime pytest runtime/tests  →  7 passed

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-20 10:49:47 +08:00

216 lines
8.1 KiB
Python

"""Tests for ``runtime.process`` P0-4 R1 + R2 fixes.
R1: ``_reap_once`` must verify a stuck dead-process record's identity
before deleting it — otherwise a ``start_workspace`` that replaced the
dead record mid-cycle would see its new entry silently erased by the
reaper's stale ``del``.
R2: ``WORKSPACE_LOCKS`` entries must survive ``stop_workspace`` —
popping the lock breaks mutual exclusion for any coroutine still
holding a reference to the old lock object.
"""
from __future__ import annotations
import asyncio
import time
from types import SimpleNamespace
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from runtime import process
def _make_dead_process(pid: int) -> MagicMock:
"""A ``subprocess.Popen``-shaped mock whose ``poll()`` says dead."""
proc = MagicMock(name=f"proc-{pid}")
proc.pid = pid
proc.poll = MagicMock(return_value=0) # exited
return proc
def _make_alive_process(pid: int) -> MagicMock:
"""A ``subprocess.Popen``-shaped mock whose ``poll()`` says alive."""
proc = MagicMock(name=f"proc-{pid}")
proc.pid = pid
proc.poll = MagicMock(return_value=None) # running
return proc
def _make_record(
*,
pid: int,
started_at: float,
last_used_at: float | None = None,
alive: bool = True,
) -> dict:
"""A ``JupyterProcessRecord``-shaped dict (subscriptable, like the TypedDict)."""
return {
"process": _make_alive_process(pid) if alive else _make_dead_process(pid),
"port": 8000 + pid,
"token": f"token-{pid}",
"base_url": "http://localhost",
"started_at": started_at,
"last_used_at": last_used_at if last_used_at is not None else started_at,
"meta_path": f"/tmp/{pid}.json",
}
@pytest.fixture(autouse=True)
def _isolate_registries():
"""Snapshot & restore the global registries so tests don't leak."""
saved_processes = dict(process.JUPYTER_PROCESSES)
saved_locks = dict(process.WORKSPACE_LOCKS)
yield
process.JUPYTER_PROCESSES.clear()
process.JUPYTER_PROCESSES.update(saved_processes)
process.WORKSPACE_LOCKS.clear()
process.WORKSPACE_LOCKS.update(saved_locks)
# ─── R1: reaper identity check ─────────────────────────────────────────
def test_reap_drops_dead_process_when_identity_matches() -> None:
"""The original (non-buggy) case still works: a dead process with
nobody replacing it must be cleaned up."""
now = time.time()
dead = _make_record(pid=100, started_at=now, alive=False)
process.JUPYTER_PROCESSES["ws1"] = dead
with patch.object(process, "_delete_meta") as mock_delete, \
patch.object(process, "stop_workspace", new=AsyncMock()):
asyncio.run(process._reap_once())
assert "ws1" not in process.JUPYTER_PROCESSES
mock_delete.assert_called_once_with("ws1")
def test_reap_skips_dead_process_when_record_replaced_by_new_process() -> None:
"""P0-4 R1 core bug. ``start_workspace`` detected the dead record
while the reaper was between snapshot and ``del``, removed it,
started a new process with a different pid, and wrote the new
record under the same ``ws_id``. The reaper's stale snapshot now
references the old (gone) pid; a naive ``del`` would erase the
new live process. Identity check must catch this and skip."""
now = time.time()
replacement = _make_record(pid=200, started_at=now, alive=True)
# Mutate the dict to simulate ``start_workspace`` having replaced
# the entry between snapshot time and now.
process.JUPYTER_PROCESSES["ws1"] = replacement
with patch.object(process, "_delete_meta") as mock_delete, \
patch.object(process, "stop_workspace", new=AsyncMock()):
asyncio.run(process._reap_once())
# The new live record must survive untouched.
assert process.JUPYTER_PROCESSES["ws1"] is replacement
mock_delete.assert_not_called()
def test_reap_keeps_alive_process_alone() -> None:
"""Sanity: a record whose process is still running must not be
dropped (only marked for idle/max-lifetime stop). With a fresh
``started_at`` neither threshold fires, so no victim either."""
now = time.time()
live = _make_record(pid=100, started_at=now, alive=True)
process.JUPYTER_PROCESSES["ws1"] = live
with patch.object(process, "stop_workspace", new=AsyncMock()) as mock_stop, \
patch.object(process, "_delete_meta") as mock_delete:
asyncio.run(process._reap_once())
assert process.JUPYTER_PROCESSES["ws1"] is live
mock_delete.assert_not_called()
mock_stop.assert_not_called()
# ─── R2: workspace locks survive stop ──────────────────────────────────
def test_workspace_lock_is_shared_across_callers() -> None:
"""``get_workspace_lock`` must return the *same* ``asyncio.Lock``
object every time — that is the whole point of the registry. If
a caller pops the entry between two ``get_workspace_lock`` calls,
a fresh lock would be returned and the two callers would no longer
be serialized."""
lock_a = process.get_workspace_lock("ws1")
lock_b = process.get_workspace_lock("ws1")
assert lock_a is lock_b
def test_concurrent_acquires_serialize_through_same_lock() -> None:
"""Two concurrent callers acquiring the lock for the same
``ws_id`` must serialize — proving they share a single lock object,
not two independent ones. With P0-4 R2 in place (lock never
popped) this is automatic; before the fix a popped entry would
let the second caller acquire a fresh, independent lock."""
lock = process.get_workspace_lock("ws1")
order: list[str] = []
async def first() -> None:
async with lock:
order.append("first-enter")
await asyncio.sleep(0)
order.append("first-exit")
async def second() -> None:
# Acquire *after* first releases — if we got a fresh lock here,
# this would interleave instead of waiting.
async with lock:
order.append("second-enter")
order.append("second-exit")
async def scenario() -> None:
await asyncio.gather(first(), second())
asyncio.run(scenario())
assert order == [
"first-enter", "first-exit", "second-enter", "second-exit",
], (
f"Lock leaked: order={order}. The two callers interleaved, "
"which means they held independent lock objects."
)
def test_workspace_lock_survives_manual_pop_simulating_stop() -> None:
"""Pre-fix, ``stop_workspace`` popped the lock from the registry
*after* the async-with exited. The lock object itself survived
(the dict entry just disappeared), but the next
``get_workspace_lock`` call would create a *new* lock and lose
serialization for any coroutine still holding the old reference.
This test simulates the pre-fix sequence manually: after one
``async with`` block exits, ``get_workspace_lock`` must still
return the *same* lock if we don't pop it; if we *did* pop it
(simulating the old buggy behavior), it would return a fresh
one. The test asserts the fixed invariant: even after the
stop-shaped sequence, the lock object is the same one."""
lock_a = process.get_workspace_lock("ws1")
async def use_and_release() -> None:
async with lock_a:
pass
asyncio.run(use_and_release())
# Pre-fix path: ``stop_workspace`` would pop the entry here.
# Post-fix: the helper is gone, so nothing pops the entry. Verify
# the invariant the fix preserves.
assert "ws1" in process.WORKSPACE_LOCKS
lock_b = process.get_workspace_lock("ws1")
assert lock_a is lock_b
def test_drop_workspace_lock_helper_is_removed() -> None:
"""Belt-and-suspenders: the buggy helper must not come back by
accident. ``_drop_workspace_lock`` removal is the entire R2 fix;
if someone re-adds it, this test fails before the leak does."""
assert not hasattr(process, "_drop_workspace_lock"), (
"_drop_workspace_lock was re-introduced — P0-4 R2 fix "
"regressed. Remove it again."
)
# Late import to avoid an unused-import lint complaint above.