Skip to content

Commit bf0f62f

Browse files
committed
fix(prism): fail loud on unexpected combined-mode worker exit
In combined mode an unexpected background worker-loop exit (raise OR clean return, but NOT CancelledError) now logs CRITICAL AND raises SIGTERM so Swarm restarts the single combined service, instead of leaving a live API with a dead eval drainer (silent outage). A cancelled task on shutdown stays a silent no-op. The lifespan shutdown now consumes finished tasks via gather(return_exceptions), eliminating the cosmetic double-log when a task fails just before shutdown. The standalone prism-worker CLI (run_worker, resilient=False) is unchanged.
1 parent c5b00f3 commit bf0f62f

2 files changed

Lines changed: 77 additions & 16 deletions

File tree

src/prism_challenge/sdk/app_factory.py

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import asyncio
44
import logging
5+
import signal
56
from collections.abc import Awaitable, Callable, Coroutine, Sequence
67
from contextlib import asynccontextmanager
78
from time import time
@@ -26,11 +27,13 @@ async def close(self) -> None: ...
2627

2728

2829
def _log_unexpected_background_exit(task: asyncio.Task[None]) -> None:
29-
"""Surface an UNEXPECTED background-task exit loudly (never silently swallowed).
30+
"""Fail loud on an UNEXPECTED background-task exit (never silently swallowed).
3031
31-
A task cancelled during shutdown is expected (no log). A task that finishes on its own while
32-
the service is still up -- with or without an exception -- means the drainer died under a live
33-
API (a silent eval outage), so it is logged CRITICAL.
32+
A task cancelled during shutdown is expected -> silent no-op. Any other completion --
33+
whether it RAISED or RETURNED normally -- means the sole eval-queue drainer died while the
34+
co-hosted API keeps serving ``/health`` 200 (a silent eval outage), so we log CRITICAL AND
35+
raise SIGTERM so uvicorn runs graceful shutdown and Swarm restarts the single combined
36+
service (mirrors agent-challenge's ``_handle_worker_task_done``).
3437
"""
3538
if task.cancelled():
3639
return
@@ -39,6 +42,7 @@ def _log_unexpected_background_exit(task: asyncio.Task[None]) -> None:
3942
_logger.critical("background task exited unexpectedly", exc_info=exc)
4043
else:
4144
_logger.critical("background task exited unexpectedly without error")
45+
signal.raise_signal(signal.SIGTERM)
4246

4347

4448
def create_challenge_app(
@@ -62,13 +66,12 @@ async def lifespan(app: FastAPI):
6266
finally:
6367
for task in tasks:
6468
task.cancel()
65-
for task in tasks:
66-
try:
67-
await task
68-
except asyncio.CancelledError:
69-
pass
70-
except Exception:
71-
_logger.exception("background task crashed during shutdown")
69+
if tasks:
70+
# gather(return_exceptions=True) awaits every task and consumes its outcome
71+
# (CancelledError on clean shutdown, or an error already surfaced by the
72+
# done-callback) without re-raising -- so a task that failed just before shutdown
73+
# is not logged a second time.
74+
await asyncio.gather(*tasks, return_exceptions=True)
7275
await database.close()
7376

7477
app = FastAPI(title=settings.name, version=settings.version, lifespan=lifespan)

tests/test_combined_mode.py

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22

33
import asyncio
44
import logging
5+
import signal
56
from pathlib import Path
67
from types import SimpleNamespace
78
from unittest.mock import AsyncMock
89

910
import pytest
11+
from fastapi import APIRouter
1012
from fastapi.testclient import TestClient
1113

1214
from prism_challenge import worker as worker_module
@@ -26,6 +28,10 @@ def _settings(tmp_path: Path, **overrides: object) -> PrismSettings:
2628
)
2729

2830

31+
async def _empty_weights() -> dict[str, float]:
32+
return {}
33+
34+
2935
def test_combined_mode_default_is_off() -> None:
3036
settings = PrismSettings(shared_token="x")
3137
assert settings.combined_mode is False
@@ -167,9 +173,12 @@ async def fake_sleep(_seconds: float) -> None:
167173
assert process_next.await_count == 1
168174

169175

170-
async def test_log_unexpected_background_exit_logs_on_error(
171-
caplog: pytest.LogCaptureFixture,
176+
async def test_log_unexpected_background_exit_logs_and_signals_on_error(
177+
caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
172178
) -> None:
179+
raised: list[signal.Signals] = []
180+
monkeypatch.setattr(signal, "raise_signal", lambda sig: raised.append(sig))
181+
173182
async def boom() -> None:
174183
raise RuntimeError("worker died")
175184

@@ -182,11 +191,15 @@ async def boom() -> None:
182191

183192
assert any(r.levelno == logging.CRITICAL for r in caplog.records)
184193
assert any("exited unexpectedly" in r.message for r in caplog.records)
194+
assert raised == [signal.SIGTERM]
185195

186196

187-
async def test_log_unexpected_background_exit_logs_on_clean_return(
188-
caplog: pytest.LogCaptureFixture,
197+
async def test_log_unexpected_background_exit_logs_and_signals_on_clean_return(
198+
caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
189199
) -> None:
200+
raised: list[signal.Signals] = []
201+
monkeypatch.setattr(signal, "raise_signal", lambda sig: raised.append(sig))
202+
190203
async def clean() -> None:
191204
return None
192205

@@ -197,11 +210,15 @@ async def clean() -> None:
197210
app_factory._log_unexpected_background_exit(task)
198211

199212
assert any("without error" in r.message for r in caplog.records)
213+
assert raised == [signal.SIGTERM]
200214

201215

202216
async def test_log_unexpected_background_exit_ignores_cancelled(
203-
caplog: pytest.LogCaptureFixture,
217+
caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
204218
) -> None:
219+
raised: list[signal.Signals] = []
220+
monkeypatch.setattr(signal, "raise_signal", lambda sig: raised.append(sig))
221+
205222
async def sleeper() -> None:
206223
await asyncio.sleep(3600)
207224

@@ -215,3 +232,44 @@ async def sleeper() -> None:
215232
app_factory._log_unexpected_background_exit(task)
216233

217234
assert not [r for r in caplog.records if r.levelno == logging.CRITICAL]
235+
assert raised == []
236+
237+
238+
def test_combined_shutdown_no_double_log_when_task_fails(
239+
caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch
240+
) -> None:
241+
"""A background task that dies just before shutdown is surfaced exactly once.
242+
243+
The done-callback logs CRITICAL + raises SIGTERM; the lifespan shutdown then consumes the
244+
already-failed task without re-logging it (no cosmetic double-log).
245+
"""
246+
raised: list[signal.Signals] = []
247+
monkeypatch.setattr(signal, "raise_signal", lambda sig: raised.append(sig))
248+
249+
class _DB:
250+
async def init(self) -> None:
251+
return None
252+
253+
async def close(self) -> None:
254+
return None
255+
256+
async def _boom(app: object) -> None:
257+
raise RuntimeError("drainer died")
258+
259+
app = app_factory.create_challenge_app(
260+
settings=PrismSettings(shared_token="x"),
261+
database=_DB(),
262+
public_router=APIRouter(),
263+
get_weights_fn=_empty_weights,
264+
background_tasks=(_boom,),
265+
)
266+
267+
with caplog.at_level(logging.CRITICAL, logger="prism.sdk.app_factory"):
268+
with TestClient(app) as client:
269+
assert client.get("/health").status_code == 200
270+
271+
critical = [r for r in caplog.records if r.levelno == logging.CRITICAL]
272+
assert len(critical) == 1
273+
assert "exited unexpectedly" in critical[0].message
274+
assert "crashed during shutdown" not in caplog.text
275+
assert raised == [signal.SIGTERM]

0 commit comments

Comments
 (0)