diff --git a/apps/minds/changelog/gabriel-escalate-to-recovery.md b/apps/minds/changelog/gabriel-escalate-to-recovery.md new file mode 100644 index 0000000000..bc468364dc --- /dev/null +++ b/apps/minds/changelog/gabriel-escalate-to-recovery.md @@ -0,0 +1,3 @@ +Opening a workspace that never finished loading no longer sits forever on the "Loading workspace" spinner when its compute provider has become unreachable. When discovery reports the workspace's provider as errored, the app now escalates to the recovery page ("Can't connect to " with a Retry button and automatic recovery) instead of spinning indefinitely, and returns you to the workspace once the provider recovers. + +A still-resolving healthy workspace that is simply slow to warm up is unaffected -- it is only escalated when its provider has actually errored, so a legitimately-slow cold load is not misreported as stuck. diff --git a/apps/minds/imbue/minds/cli/run.py b/apps/minds/imbue/minds/cli/run.py index 418518a343..4ffc9133a9 100644 --- a/apps/minds/imbue/minds/cli/run.py +++ b/apps/minds/imbue/minds/cli/run.py @@ -431,11 +431,15 @@ def run( system_interface_health_tracker = SystemInterfaceHealthTracker() # The plugin reports every non-2xx response; minds decides which ones count. - # Only connection-level failures and infrastructure 5xx enroll a suspect -- - # application errors (and UNRESOLVED, a routeless warm-up) are left alone. + # Connection-level failures and infrastructure 5xx enroll a suspect; + # application errors are left alone. UNRESOLVED (a routeless agent) enrolls + # only when its provider is errored in discovery -- a doomed cold load that + # will never resolve -- and not a healthy warm-up that still will. consumer.add_on_system_interface_backend_failure_callback( lambda agent_id, reason, status_code: system_interface_health_tracker.record_failure(agent_id) - if should_enroll_suspect_for_backend_failure(reason, status_code) + if should_enroll_suspect_for_backend_failure( + reason, status_code, is_provider_errored=backend_resolver.is_agent_provider_errored(agent_id) + ) else None ) diff --git a/apps/minds/imbue/minds/desktop_client/backend_resolver.py b/apps/minds/imbue/minds/desktop_client/backend_resolver.py index c94c3839e9..bdeab5261a 100644 --- a/apps/minds/imbue/minds/desktop_client/backend_resolver.py +++ b/apps/minds/imbue/minds/desktop_client/backend_resolver.py @@ -249,6 +249,17 @@ def get_provider_errors(self) -> dict[ProviderInstanceName, DiscoveryError]: """ return {} + def is_agent_provider_errored(self, agent_id: AgentId) -> bool: + """Whether ``agent_id``'s provider has a surfaced discovery error. + + Joins the agent's provider name to :meth:`get_provider_errors`. Default + implementation returns False (resolvers without provider state never + report errors); ``MngrCliBackendResolver`` overrides it. Gates recovery + enrollment for a routeless (UNRESOLVED) agent: only a doomed load under an + errored provider escalates, not a still-resolving warm-up. + """ + return False + def get_freshness_timestamps(self) -> tuple[datetime | None, datetime | None]: """Return ``(last_event_at, aggregate_last_snapshot_at)`` from discovery. @@ -822,6 +833,22 @@ def get_provider_errors(self) -> dict[ProviderInstanceName, DiscoveryError]: with self._lock: return dict(self._error_by_provider_name) + def is_agent_provider_errored(self, agent_id: AgentId) -> bool: + """Whether ``agent_id``'s provider has a surfaced discovery error. + + A single cheap in-memory join under the lock: find the agent's provider in + the current snapshot, then test membership in the per-provider error map. + Returns False when the agent is absent from the snapshot -- its provider is + unknown, so no error can be attributed to it (mirroring the recovery page, + which likewise can only classify providers it can resolve for the agent). + """ + with self._lock: + provider_name = next( + (agent.provider_name for agent in self._agents_result.discovered_agents if agent.agent_id == agent_id), + None, + ) + return provider_name is not None and provider_name in self._error_by_provider_name + def get_freshness_timestamps(self) -> tuple[datetime | None, datetime | None]: """Return ``(last_event_at, aggregate_last_snapshot_at)`` for the providers panel. diff --git a/apps/minds/imbue/minds/desktop_client/backend_resolver_test.py b/apps/minds/imbue/minds/desktop_client/backend_resolver_test.py index 7020ae7ec6..dc8aaa8d2c 100644 --- a/apps/minds/imbue/minds/desktop_client/backend_resolver_test.py +++ b/apps/minds/imbue/minds/desktop_client/backend_resolver_test.py @@ -19,8 +19,10 @@ from imbue.minds.desktop_client.conftest import make_agents_json from imbue.minds.desktop_client.conftest import make_resolver_with_data from imbue.minds.desktop_client.conftest import make_service_log +from imbue.minds.desktop_client.conftest import seed_provider_snapshots from imbue.minds.desktop_client.workspace_color import DEFAULT_WORKSPACE_COLOR from imbue.minds.primitives import ServiceName +from imbue.mngr.api.discovery_events import DiscoveryError from imbue.mngr.primitives import AgentId from imbue.mngr.primitives import AgentName from imbue.mngr.primitives import DiscoveredAgent @@ -1051,6 +1053,51 @@ def list_services_for_agent(self, agent_id: AgentId) -> tuple[ServiceName, ...]: assert resolver.get_agent_display_info(_AGENT_A) is None +# -- is_agent_provider_errored tests -- + + +def _local_provider_error() -> DiscoveryError: + return DiscoveryError( + type_name="RuntimeError", + message="limactl crashed", + provider_name=ProviderInstanceName("local"), + ) + + +def test_is_agent_provider_errored_true_when_agents_provider_errored() -> None: + """A discovered agent whose provider has a surfaced discovery error reports True.""" + resolver = make_resolver_with_data(agents_json=make_agents_json(_AGENT_A)) + seed_provider_snapshots(resolver, error_by_provider_name={ProviderInstanceName("local"): _local_provider_error()}) + + assert resolver.is_agent_provider_errored(_AGENT_A) is True + + +def test_is_agent_provider_errored_false_when_provider_healthy() -> None: + """A discovered agent whose provider has no surfaced error reports False.""" + resolver = make_resolver_with_data(agents_json=make_agents_json(_AGENT_A)) + + assert resolver.is_agent_provider_errored(_AGENT_A) is False + + +def test_is_agent_provider_errored_false_for_unknown_agent() -> None: + """An agent absent from the live snapshot has no attributable provider, so False. + + This is the cold-start edge: even with the provider errored, an agent that + discovery has not enumerated cannot be tied to that provider. + """ + resolver = make_resolver_with_data(agents_json=make_agents_json(_AGENT_A)) + seed_provider_snapshots(resolver, error_by_provider_name={ProviderInstanceName("local"): _local_provider_error()}) + + assert resolver.is_agent_provider_errored(_AGENT_B) is False + + +def test_is_agent_provider_errored_default_false_for_static_resolver() -> None: + """Resolvers without provider state (the interface default) never report an error.""" + resolver = StaticBackendResolver(url_by_agent_and_service={str(_AGENT_A): {"web": "http://127.0.0.1:9100"}}) + + assert resolver.is_agent_provider_errored(_AGENT_A) is False + + # -- workspace name override (optimistic rename) tests ---------------- # # A UI rename writes the new name via ``mngr label`` / ``mngr rename``, but diff --git a/apps/minds/imbue/minds/desktop_client/system_interface_health.py b/apps/minds/imbue/minds/desktop_client/system_interface_health.py index c8d1eeb479..e0be1d1d77 100644 --- a/apps/minds/imbue/minds/desktop_client/system_interface_health.py +++ b/apps/minds/imbue/minds/desktop_client/system_interface_health.py @@ -69,6 +69,7 @@ def should_enroll_suspect_for_backend_failure( reason: SystemInterfaceBackendFailureReason, status_code: int | None, + is_provider_errored: bool, ) -> bool: """Whether a ``system_interface_backend_failure`` should enroll a probe suspect. @@ -80,18 +81,27 @@ def should_enroll_suspect_for_backend_failure( they are left alone; the background probe still catches a genuinely-wrong or wedged backend. - ``UNRESOLVED`` is ignored outright: it means the forward has no route for the - agent at all. A restart routes *through* the forward, so it cannot help a - routeless agent regardless. In practice ``UNRESOLVED`` is either a cold-start - / fresh-forward warm-up (the forward has not caught up to discovery yet -- - this self-resolves the moment it does, so enrolling would only mark a healthy - workspace STUCK and needlessly restart it) or a genuinely-gone agent (which a - restart cannot revive). A workspace that is present but unreachable does NOT - land here: discovery retains its (stale) route, so the dial failure surfaces - as ``CONNECT_ERROR`` / a 5xx, which still enrolls and still drives recovery. + ``UNRESOLVED`` means the forward has no route for the agent at all, which has + two very different causes distinguished by ``is_provider_errored`` (whether + the agent's provider currently has a surfaced discovery error): + + - Provider healthy (``is_provider_errored`` False): a warm-up. The route is + still resolving (a cold remote workspace can take ~tens of seconds) and + will arrive on its own, so enrolling would only flag a healthy workspace + STUCK. Left alone -- today's behavior. + - Provider errored (``is_provider_errored`` True): a doomed cold load. The + provider is unreachable, so the route will never resolve; enrolling lets + the probe loop confirm it (a routeless agent keeps returning the forward's + 5xx) and drive the recovery page's "Can't connect to " screen. + Self-debouncing: if the provider error was a blip and the route resolves + within the stuck threshold, the probe succeeds and no STUCK fires. + + Once a workspace has loaded, a present-but-unreachable outage surfaces as + ``CONNECT_ERROR`` / a 5xx instead (discovery retains its stale route), which + enrolls regardless of ``is_provider_errored``. """ if reason == SystemInterfaceBackendFailureReason.UNRESOLVED: - return False + return is_provider_errored return status_code is None or status_code in _BACKEND_UNREACHABLE_STATUSES diff --git a/apps/minds/imbue/minds/desktop_client/system_interface_health_test.py b/apps/minds/imbue/minds/desktop_client/system_interface_health_test.py index cdeb32a45b..7d8e743592 100644 --- a/apps/minds/imbue/minds/desktop_client/system_interface_health_test.py +++ b/apps/minds/imbue/minds/desktop_client/system_interface_health_test.py @@ -17,34 +17,40 @@ @pytest.mark.parametrize( - "reason,status_code,expected", + "reason,status_code,is_provider_errored,expected", [ - # Connection-level failure (no HTTP status) enrolls. - (SystemInterfaceBackendFailureReason.CONNECT_ERROR, None, True), - (SystemInterfaceBackendFailureReason.SSE_EOF, None, True), - # Infrastructure 5xx: the backend is unreachable / not serving. - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 502, True), - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 503, True), - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 504, True), + # Connection-level failure (no HTTP status) enrolls, regardless of provider state. + (SystemInterfaceBackendFailureReason.CONNECT_ERROR, None, False, True), + (SystemInterfaceBackendFailureReason.SSE_EOF, None, False, True), + # Infrastructure 5xx: the backend is unreachable / not serving. Enrolls + # regardless of provider state (the provider flag only gates UNRESOLVED). + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 502, False, True), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 503, False, True), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 504, False, True), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 503, True, True), # Application errors: the backend is alive and responding, so they - # don't enroll -- the background probe adjudicates a wedged backend. - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 500, False), - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 404, False), - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 401, False), - (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 400, False), - # UNRESOLVED means the forward has no route for the agent at all (a - # cold-start warm-up or a genuinely-gone agent); a restart routes through - # the forward so it cannot help either way. Never enroll on it -- even - # though it carries a None status code that would otherwise enroll. - (SystemInterfaceBackendFailureReason.UNRESOLVED, None, False), + # don't enroll -- the background probe adjudicates a wedged backend. A + # concurrently-errored provider does not change that (only UNRESOLVED cares). + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 500, False, False), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 500, True, False), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 404, False, False), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 401, False, False), + (SystemInterfaceBackendFailureReason.ERROR_RESPONSE, 400, False, False), + # UNRESOLVED (a routeless agent) enrolls only when its provider is errored + # in discovery -- a doomed cold load. A healthy provider means a warm-up + # whose route will still resolve, so it is left alone despite the None + # status code that would otherwise enroll. + (SystemInterfaceBackendFailureReason.UNRESOLVED, None, True, True), + (SystemInterfaceBackendFailureReason.UNRESOLVED, None, False, False), ], ) def test_should_enroll_suspect_for_backend_failure( reason: SystemInterfaceBackendFailureReason, status_code: int | None, + is_provider_errored: bool, expected: bool, ) -> None: - assert should_enroll_suspect_for_backend_failure(reason, status_code) is expected + assert should_enroll_suspect_for_backend_failure(reason, status_code, is_provider_errored) is expected def _sleep(seconds: float) -> None: