fix(recovery): map vanished-session KeyError to the skip path in WebUI startup recovery (#7753)
This commit is contained in:
1 parent
6f6d7a6451
commit
5466d09e91
2 files changed
+117
-2
No files matched your search
@@ -1883,7 +1883,22 @@ def recover_processes_for_webui(process_registry=None, get_session_fn=None) -> i
|
|||||||
try:
|
try:
|
||||||
proc_session = process_registry.get(process_id)
|
proc_session = process_registry.get(process_id)
|
||||||
session_key = str(getattr(proc_session, "session_key", "") or "")
|
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
|
continue
|
||||||
except Exception:
|
except Exception:
|
||||||
logger.warning(
|
logger.warning(
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
"""Regression coverage for notify_on_complete across a WebUI restart."""
|
"""Regression coverage for notify_on_complete across a WebUI restart."""
|
||||||
|
|
||||||
|
import logging
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
from types import SimpleNamespace
|
from types import SimpleNamespace
|
||||||
@@ -158,4 +159,103 @@ def test_recovery_is_fail_soft_without_agent(monkeypatch):
|
|||||||
monkeypatch.setattr("builtins.__import__", fake_import)
|
monkeypatch.setattr("builtins.__import__", fake_import)
|
||||||
|
|
||||||
assert bp.recover_processes_for_webui() == 0
|
assert bp.recover_processes_for_webui() == 0
|
||||||
assert bp._PROCESS_RECOVERY_DONE is False
|
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