From 3f7309507e5b1c8d1b27b1292bfc479c7caeeaac Mon Sep 17 00:00:00 2001 From: mgrani Date: Thu, 6 Aug 2026 17:11:54 +0200 Subject: [PATCH] fix(remote): report an unusable credential as auth, not as an empty scope F3. With a valid specifier and no usable LEXIS credential, `remote pull` said "0 datasets found" -- and, after F2 made matching nothing a failure, said "No datasets matched specifier 'all'" with ErrorType.DATA. That is worse than the original silence: it is confidently wrong about where the problem is, and sends an operator to check the scope when the fix is to log in. The credential failure was swallowed twice. LexisRepository.list caught every exception, logged it and returned []; AggregatedRepository.list then caught that per repository and carried on. Nothing above could tell "empty" from "rejected". - New owilix/core/exceptions.py: OwilixError, AuthenticationError, and looks_like_authentication_failure(). - LexisRepository.list re-raises as AuthenticationError when the failure is auth-shaped. Everything else still degrades to [], unchanged. - AggregatedRepository.list re-raises an auth failure only when it left nothing to return; a repository that answered still yields its datasets, so one broken backend does not sink a multi-repository listing. - remote_pull returns ErrorType.AUTH / ExitCode.AUTH_ERROR (3) with a message saying how to authenticate in both interactive and server contexts. Detection matches on type *and* text on purpose. py4lexis has a typed Py4LexisAuthException, but the failure the clusters actually produced arrives as Py4LexisAPIException -- the DDI API answers a missing credential with an errorString py4lexis does not recognise -- so type alone under-detects. The phrase list is kept specific; a bare "token" would misreport network faults as login problems, and there is a test for exactly that. Over HTTP this needs no API shape change: success=False already maps to a failed job, and error_type "auth" rides along in the existing result dict. No 'owilix remote login' command exists to point at, so the message describes what actually works: complete the browser login interactively, or set PY4LEXIS_TOKEN / POST /auth/device/start for a server. Not fixed here, recorded under F3: a caller still cannot tell that a listing was *partial* when one repository failed and another answered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017phbSd6D8u4iEQsCAPEw6s --- owilix/core/exceptions.py | 107 ++++++++++++ owilix/core/repository/aggregate.py | 21 +++ owilix/core/repository/lexis.py | 17 +- owilix/core/tasks/remote.py | 36 ++-- tests/owilix/core/test_exceptions.py | 235 +++++++++++++++++++++++++++ 5 files changed, 404 insertions(+), 12 deletions(-) create mode 100644 owilix/core/exceptions.py create mode 100644 tests/owilix/core/test_exceptions.py diff --git a/owilix/core/exceptions.py b/owilix/core/exceptions.py new file mode 100644 index 0000000..09bb457 --- /dev/null +++ b/owilix/core/exceptions.py @@ -0,0 +1,107 @@ +"""Typed errors for conditions a caller is expected to act on. + +Most failures inside owilix are reported as a ``CommandResult`` carrying an +``ErrorType``. The exceptions here exist for conditions that must survive the +journey *up* from a repository backend to the command layer without being +flattened into "no results" along the way -- see ``AuthenticationError``. +""" +from __future__ import annotations + +__all__ = [ + "OwilixError", + "AuthenticationError", + "looks_like_authentication_failure", +] + + +class OwilixError(Exception): + """Base class for owilix's own typed errors.""" + + +class AuthenticationError(OwilixError): + """No usable credential for a remote, or the remote rejected the one given. + + "Not logged in" is the single most likely operational state for a tool whose + credential is a device-login refresh token, so it deserves a first-class + error rather than an empty result set. + + Before this existed, ``LexisRepository.list`` caught every exception, logged + it, and returned ``[]``; ``AggregatedRepository.list`` did the same per + repository. An expired credential therefore surfaced as "0 datasets found" + -- and, once matching nothing became a failure (F2), as the actively + misleading "No datasets matched specifier". An operator reading the summary + concluded the scope was empty and went looking in the wrong place. + """ + + def __init__(self, message: str | None = None, *, repository: str | None = None, + cause: BaseException | None = None) -> None: + self.repository = repository + self.cause = cause + if message is None: + where = f" for repository '{repository}'" if repository else "" + # No 'login' subcommand exists to point at: interactively, any remote + # command falls back to a browser login on its own, and a server has + # to be given a credential instead. Say what actually works in each + # case rather than naming a command that does not exist. + message = ( + f"Not authenticated{where}. The credential is missing, expired or was " + "rejected. Interactively, rerun the command and complete the browser " + "login when prompted. Non-interactively, set PY4LEXIS_TOKEN or " + "complete a device login via POST /auth/device/start." + ) + if cause is not None: + message = f"{message} Underlying error: {cause}" + super().__init__(message) + + +# Phrases that mean "the credential is the problem", matched case-insensitively +# against the *rendered* exception message. +# +# Matching text is unpleasant but necessary here: py4lexis has a typed +# Py4LexisAuthException, yet the auth failure observed in production arrives as +# a Py4LexisAPIException, because the DDI API answers a missing credential with +# an errorString py4lexis does not recognise: +# +# API -- POST -- https://api.lexis.tech/api/ddiapi/v2/meta/search +# -- Unrecognized 'errorString' obtained in the content: +# Authorization header empty. -- FAILED +# +# So type alone under-detects. Keep these phrases specific: a bare "token" would +# match unrelated failures and misreport a network fault as a login problem. +_AUTH_PHRASES = ( + "authorization header empty", + "authorization header missing", + "unauthorized", + "invalid_grant", + "invalid token", + "token expired", + "token has expired", + "expired token", + "authentication failed", + "not authenticated", + "not logged in", + "permission denied", + "forbidden", + "http 401", + "status 401", + "401 client error", + "http 403", + "status 403", + "403 client error", +) + + +def looks_like_authentication_failure(exc: BaseException) -> bool: + """Whether ``exc`` reports a credential problem rather than a data problem. + + Checked by type first -- py4lexis' own auth and session exceptions -- and + then by message, for the API-shaped failures that carry the real reason in + their text. + """ + for cls_name in type(exc).__mro__: + name = getattr(cls_name, "__name__", "") + if name in ("Py4LexisAuthException", "AuthenticationError"): + return True + + text = str(exc).lower() + return any(phrase in text for phrase in _AUTH_PHRASES) diff --git a/owilix/core/repository/aggregate.py b/owilix/core/repository/aggregate.py index 851c5c2..53deaba 100644 --- a/owilix/core/repository/aggregate.py +++ b/owilix/core/repository/aggregate.py @@ -16,6 +16,7 @@ from typing import Dict, List, Union, Sequence import fsspec +from owilix.core.exceptions import AuthenticationError from owilix.core.repository.base import ( AbstractRepository, RepoPerformanceStats, @@ -121,6 +122,8 @@ class AggregatedRepository: if ignore_data_centers is not None: repos = [(k, v) for k, v in repos if k not in ignore_data_centers] + auth_failures: List[AuthenticationError] = [] + with ThreadPoolExecutor() as executor: future_to_dc = { executor.submit(self._list_repo_wrapper, r, access, day, duration, query, cb_progress): dc @@ -133,9 +136,27 @@ class AggregatedRepository: repo = self.repositories[dc] repo.update_performance_stats("list", elapsed_time, len(repo_result)) results.extend(repo_result) + except AuthenticationError as e: + logger.error(f"Could not list datasets in datacenter {dc}: {e}") + auth_failures.append(e) except Exception as e: logger.exception(f"Could not list datasets in datacenter {dc}: {e}") + # Being unable to log in is not the same as finding nothing. + # + # Per-repository resilience is right -- one unreachable backend should + # not sink a multi-repository listing -- but when it leaves us with + # *nothing* to return, the caller cannot tell an empty scope from a + # rejected credential, and reports the former. Re-raise so the command + # layer can say "not authenticated" instead of "no datasets matched". + # + # Narrow on purpose: if some repository did answer, the partial result + # is still returned, as before. A caller cannot currently tell that a + # listing was partial -- that is a wider gap in the list() contract, + # recorded under F3 in the mirror-operability review, not fixed here. + if auth_failures and not results: + raise auth_failures[0] + # Deduplicate by dataset ID, keeping best performing repo grouped: Dict[str, List[Dataset]] = {} for ds in results: diff --git a/owilix/core/repository/lexis.py b/owilix/core/repository/lexis.py index b065564..68179ef 100644 --- a/owilix/core/repository/lexis.py +++ b/owilix/core/repository/lexis.py @@ -19,6 +19,7 @@ from typing import List, Union, Sequence import fsspec +from owilix.core.exceptions import AuthenticationError, looks_like_authentication_failure from owilix.core.repository.base import AbstractRepository from owilix.core.models.dataset import Dataset from owilix.core.sync import normalize_inventory_entry, to_epoch_seconds @@ -585,8 +586,22 @@ class LexisRepository(AbstractRepository): except Exception as e: logger.error(f"Failed to list datasets: {e}") + # An unusable credential is not an empty result set. + # + # Swallowing it here turned "not logged in" into "0 datasets found", + # and once matching nothing became a failure it became the actively + # misleading "No datasets matched specifier" -- pointing the reader + # at the scope when the problem was the token. Raising keeps the one + # operational state most likely to be true of a device-login tool + # distinguishable from a scope that genuinely holds nothing. + # + # Everything else still degrades to an empty list, as before. + if looks_like_authentication_failure(e): + raise AuthenticationError( + repository=getattr(self, "repo_name", None), cause=e + ) from e return [] - + def files(self, dataset: Dataset, files_glob: str | Sequence[str] = None) -> list: """List files in a dataset. diff --git a/owilix/core/tasks/remote.py b/owilix/core/tasks/remote.py index 882f9f1..e0abe44 100644 --- a/owilix/core/tasks/remote.py +++ b/owilix/core/tasks/remote.py @@ -32,7 +32,8 @@ from owilix.core.sync import ( write_inventory_gz, write_sync_sidecar, ) -from owilix.core.types import CommandResult +from owilix.core.exceptions import AuthenticationError +from owilix.core.types import CommandResult, ErrorType, ExitCode from owilix.core.db.models import OWIlixSQLQuery from owilix.core.db.duckdb_executor import OWIDuckDBSelectExecutor from owilix.core.tasks.query_utils import extract_domain_components, parse_files_pattern @@ -244,14 +245,29 @@ def remote_pull( # # Keeping `ignore_data_centers` is right: when pushing to a remote, do not # also treat that remote as a source to pull from. - datasets = manager.remote_data.list( - spec.get("data_center"), - access, - day=spec.get("day"), - duration=spec.get("duration") or 0, - query=query, - ignore_data_centers=[push_to_remote] if push_to_remote else None - ) + try: + datasets = manager.remote_data.list( + spec.get("data_center"), + access, + day=spec.get("day"), + duration=spec.get("duration") or 0, + query=query, + ignore_data_centers=[push_to_remote] if push_to_remote else None + ) + except AuthenticationError as e: + # Reported as auth, not as data. Without this the credential failure + # arrives at the "no datasets matched" branch below and is described as + # an empty scope -- which sends the operator to check the specifier + # while the actual fix is to log in. + console.print(f"[red]{e}[/red]") + return CommandResult( + success=False, + object={"datasets": [], "specifier": specifier}, + msg=str(e), + error_type=ErrorType.AUTH, + exit_code=ExitCode.AUTH_ERROR, + command="remote pull", + ) # Print datasets (short format like ls) for d in datasets: @@ -297,7 +313,6 @@ def remote_pull( # datasets and simply finds their files present. What is reported here is # narrower: discovery matched no dataset, so nothing was even considered. if not datasets: - from owilix.core.types import ErrorType, ExitCode return CommandResult( success=False, object={"datasets": [], "specifier": specifier}, @@ -418,7 +433,6 @@ def remote_pull( console.print(f"\n[red bold]{len(failed_files)} file(s) failed to download:[/red bold]") for path, err in failed_files: console.print(f" [red]✗[/red] {path}: {err}") - from owilix.core.types import ErrorType, ExitCode return CommandResult( success=False, object={"datasets": datasets, "failed_files": failed_files}, diff --git a/tests/owilix/core/test_exceptions.py b/tests/owilix/core/test_exceptions.py new file mode 100644 index 0000000..875373c --- /dev/null +++ b/tests/owilix/core/test_exceptions.py @@ -0,0 +1,235 @@ +"""F3 -- an unusable credential must not arrive as an empty result set. + +Observed on the OpenWebSearch.eu clusters: with a valid specifier and no usable +LEXIS credential, ``LexisRepository.list`` caught the failure, logged it, and +returned ``[]``. ``AggregatedRepository.list`` swallowed it a second time. The +command layer then reported "0 datasets found" and -- once F2 made matching +nothing a failure -- the actively misleading "No datasets matched specifier", +which sends an operator to check the scope when the fix is to log in. +""" +from unittest.mock import MagicMock + +import pytest + +from owilix.core.exceptions import ( + AuthenticationError, + OwilixError, + looks_like_authentication_failure, +) + + +class TestAuthenticationFailureDetection: + def test_detects_the_production_lexis_auth_error(self): + """The exact exception the clusters produced. + + py4lexis has a typed Py4LexisAuthException, but a missing credential + comes back as a Py4LexisAPIException, because the DDI API answers with + an errorString py4lexis does not recognise. Detection by type alone + therefore misses the case that actually happens. + """ + from py4lexis.core.exceptions import Py4LexisAPIException + + exc = Py4LexisAPIException( + "Unrecognized 'errorString' obtained in the content: Authorization header empty.", + "POST", + "https://api.lexis.tech/api/ddiapi/v2/meta/search", + ) + + assert looks_like_authentication_failure(exc) + + def test_detects_py4lexis_auth_exception_by_type(self): + from py4lexis.core.exceptions import Py4LexisAuthException + + assert looks_like_authentication_failure(Py4LexisAuthException("Token expired")) + + @pytest.mark.parametrize( + "message", + [ + "401 Client Error: Unauthorized for url: https://example.test", + "invalid_grant: refresh token is expired", + "Permission denied", + ], + ) + def test_detects_common_auth_shapes(self, message): + assert looks_like_authentication_failure(RuntimeError(message)) + + @pytest.mark.parametrize( + "message", + [ + "Cannot get all datasets' status records!!! Returned records are 'None'!!!", + "Connection reset by peer", + "No such file or directory", + # Must not fire on the word 'token' alone -- a network fault + # misreported as a login problem is its own wrong turn. + "Failed to tokenize response body", + ], + ) + def test_does_not_fire_on_non_auth_failures(self, message): + assert not looks_like_authentication_failure(RuntimeError(message)) + + def test_authentication_error_is_owilix_error(self): + assert issubclass(AuthenticationError, OwilixError) + + def test_message_names_the_repository_and_the_way_out(self): + error = AuthenticationError(repository="lexis", cause=RuntimeError("boom")) + text = str(error) + + assert "lexis" in text + assert "PY4LEXIS_TOKEN" in text + assert "boom" in text, "the underlying cause must survive, not be replaced" + + +class TestAggregateDoesNotHideAuthFailures: + """The second layer of swallowing: per-repository resilience in the aggregate.""" + + def _aggregate(self, repositories): + from owilix.core.repository.aggregate import AggregatedRepository + + return AggregatedRepository(repositories=repositories) + + def test_auth_failure_with_no_results_is_raised(self): + failing = MagicMock() + failing.list.side_effect = AuthenticationError(repository="lexis") + aggregate = self._aggregate({"lexis": failing}) + + with pytest.raises(AuthenticationError): + aggregate.list("lexis", "public") + + def test_a_repository_that_answered_still_returns_its_datasets(self): + """Resilience is kept: one broken backend must not sink the listing.""" + dataset = MagicMock() + dataset.metadata.get.return_value = "ds-1" + # Deduplication scores the owning repository, so its performance stats + # have to be numbers rather than mocks. + stats = dataset.repository.performance_stats + stats.list_response_time.get_overall_average.return_value = 1.0 + stats.list_bandwidth.get_overall_average.return_value = 1.0 + + failing = MagicMock() + failing.list.side_effect = AuthenticationError(repository="lexis") + working = MagicMock() + working.list.return_value = [dataset] + + aggregate = self._aggregate({"lexis": failing, "owi-up": working}) + + result = aggregate.list(None, "public") + + assert result == [dataset] + + def test_non_auth_failures_still_degrade_to_empty(self): + """Unchanged behaviour, asserted so the narrowing is deliberate.""" + failing = MagicMock() + failing.list.side_effect = RuntimeError("backend exploded") + aggregate = self._aggregate({"lexis": failing}) + + assert aggregate.list("lexis", "public") == [] + + +class TestRemotePullReportsAuthNotData: + """The whole point of F3: the command layer must say 'auth', not 'no data'. + + With F2 in place, a swallowed credential failure does not merely produce a + quiet success any more -- it produces `No datasets matched specifier '...'` + with ErrorType.DATA, which reads as "your scope is empty". That is worse + than the original silence for anyone debugging, because it is confidently + wrong about where the problem is. + """ + + def test_auth_failure_is_reported_as_auth(self): + from owilix.core.tasks.remote import remote_pull + from owilix.core.types import ErrorType, ExitCode + + manager = MagicMock() + manager.parse_specifier.return_value = { + "data_center": None, + "query": {"access": "public"}, + "day": None, + "duration": 0, + } + manager.remote_data.list.side_effect = AuthenticationError(repository="lexis") + + result = remote_pull( + manager, specifier="all", auto_yes=True, console=MagicMock() + ) + + assert result.success is False + assert result.error_type == ErrorType.AUTH + assert result.exit_code == ExitCode.AUTH_ERROR + # Not the F2 message: the scope is not the problem. + assert "No datasets matched" not in result.msg + assert "PY4LEXIS_TOKEN" in result.msg + + def test_a_genuinely_empty_scope_is_still_a_data_error(self): + """F2's behaviour must survive F3 -- these are different faults.""" + from owilix.core.tasks.remote import remote_pull + from owilix.core.types import ErrorType, ExitCode + + manager = MagicMock() + manager.parse_specifier.return_value = { + "data_center": "dc1", + "query": {"access": "public"}, + "day": None, + "duration": 0, + } + manager.remote_data.list.return_value = [] + + result = remote_pull( + manager, specifier="dc1/public", auto_yes=True, console=MagicMock() + ) + + assert result.success is False + assert result.error_type == ErrorType.DATA + assert result.exit_code == ExitCode.DATA_ERROR + + +class TestLexisRepositoryRaisesRatherThanReturningEmpty: + """The entry point F3 is really about. + + `LexisRepository.list` caught every exception and returned `[]`. That is + where "not logged in" first became "no datasets", before the aggregate and + the command layer each had a chance to lose it again. + """ + + class DummySession: + def __init__(self): + self._zone = "IT4ILexisV2" + self.irods_http_api_url = "https://example.com/irods" + + class DummyManager: + def __init__(self): + outer = TestLexisRepositoryRaisesRatherThanReturningEmpty + + class SessionWrapper: + lexis = outer.DummySession() + + self.session = SessionWrapper() + + def _repo(self): + from owilix.core.repository import LexisRepository + + return LexisRepository(self.DummyManager()) + + def test_auth_failure_raises(self): + from py4lexis.core.exceptions import Py4LexisAPIException + + repo = self._repo() + repo._ddi_api = MagicMock() + repo._ddi_api.get_all_datasets.side_effect = Py4LexisAPIException( + "Unrecognized 'errorString' obtained in the content: Authorization header empty.", + "POST", + "https://api.lexis.tech/api/ddiapi/v2/meta/search", + ) + + with pytest.raises(AuthenticationError) as excinfo: + repo.list(access="public") + + # The operator-actionable original must not be thrown away. + assert "Authorization header empty" in str(excinfo.value) + + def test_other_failures_still_return_empty(self): + """Deliberately unchanged, so the new behaviour stays narrow.""" + repo = self._repo() + repo._ddi_api = MagicMock() + repo._ddi_api.get_all_datasets.side_effect = RuntimeError("backend exploded") + + assert repo.list(access="public") == [] -- 2.51.2