mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F117] stop the orchestrator in lifespan shutdown BEFORE closing the DB
The lifespan shutdown closed OptimalService + the DB, and only THEN did bootstrap's finally block call orchestrator.stop() — so stop() ran with the DB already closed. stop() drains fire-and-forget _bg_tasks writes (respawn_tracker upserts, audit-log rows) and stop_agent finalizes work sessions / agent state, all needing the DB still open; closing it first silently dropped those final writes (the durable PM-respawn counter's last few strikes, the metrics-bearing audit trail tail). Move orchestrator.stop() into the lifespan shutdown path, BEFORE close_optimal_service + close_db, guarded by a new get_orchestrator_or_none() safe accessor (no crash when no orchestrator is wired — tests, skip_orchestrator). bootstrap's finally-block stop() becomes an idempotent safety net: stop() gains a _stopped flag (getattr-guarded so __new__- constructed test instances still stop) so the double-call is a clean no-op, not a re-stop of already-stopped agents / re-drain of an empty bg set.
This commit is contained in:
@@ -16,6 +16,7 @@ import pytest
|
||||
from fastapi import FastAPI
|
||||
from roboco.api.app import app as default_app
|
||||
from roboco.api.app import create_app, lifespan
|
||||
from roboco.api.deps import clear_orchestrator, set_orchestrator
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import AsyncIterator
|
||||
@@ -144,6 +145,62 @@ async def test_lifespan_startup_and_shutdown_happy_path() -> None:
|
||||
transcription_mock.stop.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lifespan_stops_orchestrator_before_closing_db_and_optimal() -> None:
|
||||
"""F117: orchestrator.stop() must run BEFORE close_optimal_service / close_db
|
||||
on shutdown. stop() drains fire-and-forget DB writes (respawn_tracker
|
||||
upserts, audit-log rows) and stop_agent finalizes work sessions / agent
|
||||
state — all needing the DB still open. Closing the DB first (the old order,
|
||||
where only bootstrap's finally called stop() after lifespan had already
|
||||
closed the DB) silently dropped those final writes."""
|
||||
order: list[str] = []
|
||||
|
||||
def _record(label: str) -> AsyncMock:
|
||||
async def _fn() -> None:
|
||||
order.append(label)
|
||||
|
||||
return AsyncMock(side_effect=_fn)
|
||||
|
||||
transcription_mock = MagicMock()
|
||||
transcription_mock.start = AsyncMock()
|
||||
transcription_mock.stop = AsyncMock()
|
||||
orchestrator_mock = MagicMock()
|
||||
orchestrator_mock.stop = _record("orchestrator.stop")
|
||||
|
||||
set_orchestrator(orchestrator_mock)
|
||||
try:
|
||||
with (
|
||||
patch("roboco.api.app.init_db", new=AsyncMock()),
|
||||
patch("roboco.api.app.close_db", new=_record("close_db")),
|
||||
patch(
|
||||
"roboco.api.app.close_optimal_service",
|
||||
new=_record("close_optimal_service"),
|
||||
),
|
||||
patch(
|
||||
"roboco.api.app.TranscriptionService", return_value=transcription_mock
|
||||
),
|
||||
patch("roboco.api.app.ExtractionService"),
|
||||
patch("roboco.api.app.ExtractionPipeline"),
|
||||
patch(
|
||||
"roboco.api.app.get_optimal_service",
|
||||
new=AsyncMock(return_value=MagicMock()),
|
||||
),
|
||||
):
|
||||
app = create_app()
|
||||
async with lifespan(app):
|
||||
pass
|
||||
finally:
|
||||
# Clear the global so it doesn't leak into other tests.
|
||||
clear_orchestrator()
|
||||
|
||||
# orchestrator.stop() ran, and it ran BEFORE close_optimal_service + close_db.
|
||||
assert "orchestrator.stop" in order
|
||||
assert order.index("orchestrator.stop") < order.index("close_optimal_service")
|
||||
assert order.index("orchestrator.stop") < order.index("close_db")
|
||||
# The DB is still the last thing closed (innermost resource).
|
||||
assert order.index("close_optimal_service") < order.index("close_db")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lifespan_handles_optimal_init_failure_gracefully() -> None:
|
||||
"""Optimal-service init failure → app.state.optimal=None, no raise."""
|
||||
|
||||
@@ -148,3 +148,29 @@ async def test_stop_failing_agent_does_not_skip_drain() -> None:
|
||||
await asyncio.wait_for(orch.stop(), timeout=5.0)
|
||||
|
||||
assert ran == [True], "failing stop_agent skipped the drain (data lost)"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stop_is_idempotent_double_call_is_noop() -> None:
|
||||
"""F117: stop() is idempotent. The lifespan shutdown path now stops the
|
||||
orchestrator before closing the DB, and bootstrap's finally block re-calls
|
||||
stop() as a safety net. The second call must be a clean no-op — not a
|
||||
re-drain, not a re-stop of already-stopped agents — guarded by ``_stopped``."""
|
||||
orch = _make_orchestrator()
|
||||
real_drain = orch._drain_bg_tasks
|
||||
drain_calls = 0
|
||||
|
||||
async def counting_drain() -> None:
|
||||
nonlocal drain_calls
|
||||
drain_calls += 1
|
||||
await real_drain()
|
||||
|
||||
orch._drain_bg_tasks = counting_drain
|
||||
|
||||
await orch.stop()
|
||||
assert drain_calls == 1, "first stop() drained the bg tasks"
|
||||
assert orch._stopped is True
|
||||
|
||||
await orch.stop() # safety-net double-call (lifespan already stopped it)
|
||||
assert drain_calls == 1, "second stop() must not re-drain (idempotent no-op)"
|
||||
assert orch._stopped is True
|
||||
|
||||
Reference in New Issue
Block a user