From 5466d09e91548e07b3eac910f99bd09d31fc9c70 Mon Sep 17 00:00:00 2001 From: happy5318 <5318happy@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:23:32 +0800 Subject: [PATCH] fix(recovery): map vanished-session KeyError to the skip path in WebUI startup recovery (#7753) --- api/background_process.py | 17 ++- ...est_background_process_restart_recovery.py | 102 +++++++++++++++++- 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/api/background_process.py b/api/background_process.py index 6e6323af..eea15ebb 100644 --- a/api/background_process.py +++ b/api/background_process.py @@ -1883,7 +1883,22 @@ def recover_processes_for_webui(process_registry=None, get_session_fn=None) -> i try: proc_session = process_registry.get(process_id) session_key = str(getattr(proc_session, "session_key", "") or "") - if not session_key or get_session_fn(session_key, metadata_only=True) is None: + if not session_key: + continue + # The session resolver (``api.models._resolve_session_once``) + # raises ``KeyError(sid)`` for a missing session — that is its + # documented contract for the "owner gone" outcome, not a + # fault. Treat it identically to a ``None`` return: skip the + # process without logging, and let a neighbouring live + # process still rebind. The narrow ``except KeyError`` lives + # INSIDE the outer try so an unrelated ``KeyError`` from + # ``process_registry.get()`` (or any routing-index work) still + # surfaces in the existing warning path. + try: + resolved = get_session_fn(session_key, metadata_only=True) + except KeyError: + continue + if resolved is None: continue except Exception: logger.warning( diff --git a/tests/test_background_process_restart_recovery.py b/tests/test_background_process_restart_recovery.py index 0ab76e36..45ee82ff 100644 --- a/tests/test_background_process_restart_recovery.py +++ b/tests/test_background_process_restart_recovery.py @@ -1,5 +1,6 @@ """Regression coverage for notify_on_complete across a WebUI restart.""" +import logging import threading import time from types import SimpleNamespace @@ -158,4 +159,103 @@ def test_recovery_is_fail_soft_without_agent(monkeypatch): monkeypatch.setattr("builtins.__import__", fake_import) assert bp.recover_processes_for_webui() == 0 - assert bp._PROCESS_RECOVERY_DONE is False \ No newline at end of file + assert bp._PROCESS_RECOVERY_DONE is False + + +def test_recovery_skips_process_whose_session_is_gone(monkeypatch, caplog): + """A checkpointed process whose owner has been pruned (the resolver's + ``KeyError(sid)`` contract) must be silently skipped, while a neighbouring + live process in the same recovery sweep still rebinds. Regression for + issue #7753: previously every vanished owner logged a full WARNING + traceback, indistinguishable from a real fault. + """ + calls = {"registered": []} + + class FakeRegistry: + def recover_from_checkpoint(self): + return 2 # two checkpointed processes survive restart + + def list_sessions(self): + return [ + {"session_id": "proc_vanished", "detached": True}, + {"session_id": "proc_live", "detached": True}, + ] + + def get(self, process_id): + if process_id == "proc_vanished": + return _FakeProcessSession("vanished-session") + if process_id == "proc_live": + return _FakeProcessSession("live-session") + raise AssertionError(process_id) + + fake_registry = FakeRegistry() + monkeypatch.setattr(bp, "_PROCESS_CHECKPOINT_RECOVERED", False) + monkeypatch.setattr(bp, "_PROCESS_RECOVERY_DONE", False) + monkeypatch.setattr( + bp, + "register_process_session", + lambda key, sid: calls["registered"].append((key, sid)), + ) + + def get_session(sid, metadata_only=False): + # Mirrors api.models._resolve_session_once's contract: missing + # sessions raise KeyError, NOT None. + if sid == "vanished-session": + raise KeyError(sid) + return SimpleNamespace(id=sid) + + with caplog.at_level(logging.WARNING, logger="api.background_process"): + result = bp.recover_processes_for_webui(fake_registry, get_session) + + assert result == 2 + # The live process is rebound; the vanished one is silently skipped. + assert calls["registered"] == [("live-session", "live-session")] + # No WARNING emitted for the vanished-owner case. + warning_records = [ + r for r in caplog.records if r.levelno >= logging.WARNING + ] + assert warning_records == [], ( + "vanished-session KeyError must not produce a WARNING, got: " + f"{[r.getMessage() for r in warning_records]}" + ) + assert "Could not resolve recovered WebUI process" not in caplog.text + + +def test_recovery_still_warns_on_unexpected_resolve_failure(monkeypatch, caplog): + """Non-KeyError failures from the resolver (e.g. ``OSError`` on an + unreadable sidecar) must keep entering the existing WARNING path. This + pins that the fix for #7753 is narrow and does not over-broaden the + silent-skip contract. + """ + class FakeRegistry: + def recover_from_checkpoint(self): + return 1 + + def list_sessions(self): + return [{"session_id": "proc_unexpected", "detached": True}] + + def get(self, process_id): + assert process_id == "proc_unexpected" + return _FakeProcessSession("webui-session") + + fake_registry = FakeRegistry() + monkeypatch.setattr(bp, "_PROCESS_CHECKPOINT_RECOVERED", False) + monkeypatch.setattr(bp, "_PROCESS_RECOVERY_DONE", False) + + def get_session(sid, metadata_only=False): + raise OSError("sidecar unreadable") + + with caplog.at_level(logging.WARNING, logger="api.background_process"): + result = bp.recover_processes_for_webui(fake_registry, get_session) + + assert result == 1 + warning_records = [ + r for r in caplog.records if r.levelno >= logging.WARNING + ] + assert any( + "Could not resolve recovered WebUI process 'proc_unexpected'" in r.getMessage() + for r in warning_records + ), ( + "OSError from the resolver must still emit a WARNING, got: " + f"{[r.getMessage() for r in warning_records]}" + ) \ No newline at end of file