fix(engines): narrow the health log field to what the probe did
Greptile P1: `failure=unavailable` reads as a classification of the cause, but SubprocessBackend.health_check() swallows its own exceptions by contract, so a dead sidecar and a package that was never installed both return (False, msg) and land in the same bucket. Rename to `probe=`, with `raised:<Class>` and `returned-unavailable` as the two values, so the field states what the probe did and claims nothing about why. The limitation and what it would take to fix it properly (structured failure metadata from the probes) are named in the comment and the test docstring. Greptile P2: drop the trailing arrow glyph from the Learn more button. It sat outside t(), and a bare "→" points the wrong way once the app switches to an RTL locale. InfoHint hardcodes the same glyph and would want the same treatment, but that is not this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Chang-Jin-Lee <ckdwls525@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
adfe7b8588
commit
09a582c65a
@@ -356,7 +356,7 @@ def engine_health(engine_id: str):
|
||||
t0 = perf_counter()
|
||||
# Stable exception class when the probe itself raised, None when it merely
|
||||
# returned not-available. Never the exception text — see the log line below.
|
||||
failure_class: str | None = None
|
||||
raised_class: str | None = None
|
||||
if hasattr(cls, "health_check"):
|
||||
# SubprocessBackend path — spawn sidecar (if not running) and ping.
|
||||
# ``health_check`` already swallows its own exceptions per Plan
|
||||
@@ -367,7 +367,7 @@ def engine_health(engine_id: str):
|
||||
ok, msg = instance.health_check()
|
||||
except Exception as exc:
|
||||
ok, msg = False, f"{type(exc).__name__}: {exc}"
|
||||
failure_class = type(exc).__name__
|
||||
raised_class = type(exc).__name__
|
||||
else:
|
||||
# In-process backend — `is_available()` is the classmethod-level
|
||||
# liveness check. Cheap and side-effect-free for every shipping
|
||||
@@ -376,7 +376,7 @@ def engine_health(engine_id: str):
|
||||
ok, msg = cls.is_available()
|
||||
except Exception as exc:
|
||||
ok, msg = False, f"{type(exc).__name__}: {exc}"
|
||||
failure_class = type(exc).__name__
|
||||
raised_class = type(exc).__name__
|
||||
|
||||
# Engine-owned output can contain much more than shaped HF tokens: local
|
||||
# paths, arbitrary credentials, source lines, or a nested traceback.
|
||||
@@ -387,14 +387,23 @@ def engine_health(engine_id: str):
|
||||
# The response tells the user to "check the backend log for details",
|
||||
# and docs/engines/*.md asks a user diagnosing an unavailable engine to
|
||||
# copy that engine's log lines. The old line named neither the engine
|
||||
# nor the kind of failure, so neither instruction could be followed
|
||||
# (#1866).
|
||||
# nor anything about the probe, so neither instruction could be
|
||||
# followed (#1866).
|
||||
#
|
||||
# `probe=` reports what the PROBE DID, not what went wrong. It cannot
|
||||
# classify the cause: SubprocessBackend.health_check() swallows its own
|
||||
# exceptions per Plan 02-01's contract, so a dead sidecar and a package
|
||||
# that was never installed both arrive here as `returned-unavailable`.
|
||||
# Separating those needs structured failure metadata from the probes
|
||||
# themselves, which is a wider change than this one.
|
||||
#
|
||||
# Still no diagnostic text and still not the caller-supplied id: the
|
||||
# engine id comes off the resolved registry class and the failure is a
|
||||
# stable class name, which is the same shape core.public_errors.
|
||||
# public_failure() logs. tests/test_response_safety.py pins that
|
||||
# boundary and passes unchanged.
|
||||
# engine id comes off the resolved registry class and a raised probe
|
||||
# contributes only its exception class, the same shape
|
||||
# core.public_errors.public_failure() logs as `class=`.
|
||||
# tests/test_response_safety.py pins that boundary and passes
|
||||
# unchanged.
|
||||
#
|
||||
# The id is a class attribute off the registry rather than caller
|
||||
# input, but this line is a log-injection surface either way, so it is
|
||||
# flattened to a single token before it goes in.
|
||||
@@ -403,9 +412,9 @@ def engine_health(engine_id: str):
|
||||
c if (c.isalnum() or c in "-_.") else "-" for c in engine_label
|
||||
)[:64]
|
||||
logger.warning(
|
||||
"Engine health check failed; engine=%s failure=%s, details withheld",
|
||||
"Engine health check failed; engine=%s probe=%s, details withheld",
|
||||
engine_label or "unknown",
|
||||
failure_class or "unavailable",
|
||||
f"raised:{raised_class}" if raised_class else "returned-unavailable",
|
||||
)
|
||||
return {
|
||||
"id": engine_id,
|
||||
|
||||
@@ -1496,8 +1496,11 @@ export default function EngineCompatibilityMatrix({
|
||||
a fixed string before it reaches here, so the row's own
|
||||
text can only ever be generic. The engine's doc page is
|
||||
the one place that does explain it, and nothing linked
|
||||
to it. Same "Learn more →" affordance the MCP and
|
||||
Remote GPU panels use, so no new string is needed. */}
|
||||
to it. Reuses the `common.learn_more` key InfoHint
|
||||
already renders for the MCP and Remote GPU panels, so
|
||||
no new string is needed. No trailing arrow glyph:
|
||||
InfoHint hardcodes one, and a bare "→" points the
|
||||
wrong way once the app is in an RTL locale. */}
|
||||
{b.docs_url && (
|
||||
<button
|
||||
type="button"
|
||||
@@ -1505,7 +1508,7 @@ export default function EngineCompatibilityMatrix({
|
||||
data-testid={`engine-docs-${b.id}`}
|
||||
onClick={() => openExternal(b.docs_url)}
|
||||
>
|
||||
{t('common.learn_more', 'Learn more')} →
|
||||
{t('common.learn_more', 'Learn more')}
|
||||
</button>
|
||||
)}
|
||||
{hasDiskDetails &&
|
||||
|
||||
@@ -115,7 +115,7 @@ def test_engine_health_log_names_the_engine_and_failure_class(monkeypatch, caplo
|
||||
engines.engine_health("broken-engine")
|
||||
|
||||
assert "engine=broken-engine" in caplog.text
|
||||
assert "failure=RuntimeError" in caplog.text
|
||||
assert "probe=raised:RuntimeError" in caplog.text
|
||||
assert caplog.text.count("\n") == 1 # one record, one line — nothing forged
|
||||
# Still nothing engine-owned: no message text, no path, no token.
|
||||
assert _PRIVATE not in caplog.text
|
||||
@@ -147,7 +147,14 @@ def test_engine_health_log_flattens_a_newline_bearing_engine_id(monkeypatch, cap
|
||||
def test_engine_health_log_distinguishes_not_available_from_a_raised_probe(
|
||||
monkeypatch, caplog
|
||||
):
|
||||
"""An optional engine that was never installed did not "fail"."""
|
||||
"""`probe=` says what the probe did, and claims nothing more.
|
||||
|
||||
A probe that returns rather than raises reads `returned-unavailable`
|
||||
whatever the cause — a package that was never installed and a sidecar that
|
||||
died both land here, because SubprocessBackend.health_check() swallows its
|
||||
own exceptions by contract. Telling those apart needs structured failure
|
||||
metadata from the probes, so the field deliberately does not pretend to.
|
||||
"""
|
||||
from api.routers import engines
|
||||
|
||||
class NotInstalled:
|
||||
@@ -162,7 +169,7 @@ def test_engine_health_log_distinguishes_not_available_from_a_raised_probe(
|
||||
engines.engine_health("not-installed")
|
||||
|
||||
assert "engine=not-installed" in caplog.text
|
||||
assert "failure=unavailable" in caplog.text
|
||||
assert "probe=returned-unavailable" in caplog.text
|
||||
assert "voxcpm package not installed" not in caplog.text
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user