Merge PR #7774 into stage/7774-vanished-owner
This commit is contained in:
commit
889d9b1f5d
2 files changed
+116
-1
No files matched your search
@@ -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(
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
"""Regression coverage for notify_on_complete across a WebUI restart."""
|
||||
|
||||
import logging
|
||||
import threading
|
||||
import time
|
||||
from types import SimpleNamespace
|
||||
@@ -159,3 +160,102 @@ def test_recovery_is_fail_soft_without_agent(monkeypatch):
|
||||
|
||||
assert bp.recover_processes_for_webui() == 0
|
||||
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]}"
|
||||
)
|
||||
Reference in new issue
Block a user