From 2e33c7c6e81a1a7eb176bcc965529845fba5f28f Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Mon, 13 Jul 2026 13:43:27 +0000 Subject: [PATCH 1/7] feat(pipeline-run): add task-status and task-wait commands task-status prints the task-name to status map for a root execution by walking its child executions; container state falls back to graph-state reduction for nested pipelines. task-wait polls until every task is terminal. SUCCEEDED and SKIPPED are non-failure terminals; failures exit 2 with a stderr summary and the full status map on stdout. Wait bounds are validated up front and the deadline applies while resolving children. Transient 5xx on the per-poll root details fetch are retried within the wait deadline instead of aborting the wait; unbounded waits give up after a few consecutive failures. Also adjusts the shared status reducer used by the existing run-level status/wait: WAITING_FOR_UPSTREAM and UNINITIALIZED now count as active so a run is not reported terminal while a task still waits upstream, mixed terminal aggregates reduce to the failure terminal ahead of SKIPPED/SUCCEEDED, and wait bounds reject NaN/inf. --- .../src/tangle_cli/pipeline_run_manager.py | 313 +++++++++- .../src/tangle_cli/pipeline_runs_cli.py | 79 +++ tests/test_pipeline_runs_cli.py | 558 ++++++++++++++++++ 3 files changed, 940 insertions(+), 10 deletions(-) diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py index 24742fa..4cab2d9 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py @@ -12,6 +12,7 @@ import copy import inspect import json +import math import re import time import uuid @@ -21,6 +22,7 @@ from pathlib import Path from typing import Any, Mapping +import requests import yaml from .handler import TangleCliHandler @@ -32,9 +34,42 @@ from .pipeline_run_search import PipelineRunSearch from .utils import dump_yaml -_TERMINAL_STATUSES = ("FAILED", "SYSTEM_ERROR", "CANCELLED", "CANCELED", "SKIPPED", "SUCCEEDED", "INVALID") -_ACTIVE_STATUSES = ("RUNNING", "CANCELLING", "CANCELING", "PENDING", "QUEUED") +# Precedence order for reducing a mixed terminal aggregate to a single status: +# failure terminals must precede the non-failure terminals SKIPPED and SUCCEEDED +# so a mixed aggregate (e.g. {SKIPPED, INVALID}) reduces to the failure instead +# of masking it (a fully-SKIPPED map still reduces to SKIPPED). +_TERMINAL_PRECEDENCE = ("FAILED", "SYSTEM_ERROR", "CANCELLED", "CANCELED", "INVALID", "SKIPPED", "SUCCEEDED") +# Membership set for is_terminal_status. +_TERMINAL_STATUSES = frozenset(_TERMINAL_PRECEDENCE) +# Nonterminal statuses outrank any terminal count when reducing an aggregate; +# WAITING_FOR_UPSTREAM/UNINITIALIZED are backend enum members and nonterminal. +_ACTIVE_STATUSES = ( + "RUNNING", + "CANCELLING", + "CANCELING", + "PENDING", + "QUEUED", + "WAITING_FOR_UPSTREAM", + "UNINITIALIZED", +) _FAILURE_EARLY_EXIT_STATUSES = ("FAILED", "SYSTEM_ERROR") +# The only terminals that are not per-task failures; every other terminal makes +# a root-execution `task-wait` exit non-zero. A single SKIPPED task (e.g. a +# conditional branch) is not a failure, unlike a run whose reduced status is +# SKIPPED at the run level. +_NON_FAILURE_TERMINAL_STATUSES = frozenset({"SKIPPED", "SUCCEEDED"}) +_TASK_FAILURE_STATUSES = _TERMINAL_STATUSES - _NON_FAILURE_TERMINAL_STATUSES +# A child execution's status is unknown until its state row is written; 404 +# means "wrong endpoint type" (container vs graph) and transient 5xx can be +# returned while the orchestrator is still settling. Both fall through to the +# next endpoint / report UNKNOWN so the wait loop polls again. +_RETRYABLE_EXEC_STATE_STATUSES = frozenset({404, 500, 502, 503, 504}) +# An unbounded task-wait (max_wait=None) has no deadline to stop retrying a +# failing root details fetch, so give up after this many consecutive server +# errors instead of hiding a persistent outage behind endless retries. +_MAX_UNBOUNDED_ROOT_FETCH_FAILURES = 3 +_UNKNOWN_TASK_STATUS = "UNKNOWN" +_ROOT_TASK_NAME = "root" _EXECUTION_STATE_TIMINGS_METADATA_KEY = "execution_state_timings" _EXECUTION_STATE_TIMING_MONOTONIC_METADATA_KEY = "_execution_state_timing_monotonic" _SUBMISSION_ID_ANNOTATION_KEY = "tangle-cli/submission-id" @@ -54,6 +89,51 @@ class AmbiguousPipelineRunRecoveryError(PipelineRunError): """Raised when submit recovery finds multiple runs for one submission id.""" +class TransientServerError(PipelineRunError): + """An HTTP 5xx failure that a deadline-bounded polling loop may retry. + + One-shot callers (e.g. ``task-status``) still fail fast on it like any + other PipelineRunError. + """ + + +def _transport_error(context: str, exc: requests.RequestException) -> PipelineRunError: + """Wrap a transport/HTTP failure as a clean CLI-surfaced PipelineRunError. + + Root-execution status resolution talks to the API directly, so connection + resets/timeouts and non-retryable 4xx/5xx (including 429) would otherwise + escape task-status/task-wait as raw ``requests`` tracebacks. The CLI maps + PipelineRunError to a non-zero exit with the message instead. Server errors + are typed TransientServerError so polling loops can retry them. + """ + + response = getattr(exc, "response", None) + status_code = getattr(response, "status_code", None) + if status_code is not None: + message = f"{context}: request failed with HTTP {status_code}" + if status_code >= 500: + return TransientServerError(message) + return PipelineRunError(message) + return PipelineRunError(f"{context}: {exc}") + + +class TaskStatusesFailed(PipelineRunError): + """Raised when a root execution reaches terminal state with failed tasks. + + Carries the full final ``{task_name: status}`` map alongside the ``failures`` + subset so callers can still print the complete map while reporting failures. + """ + + def __init__(self, root_execution_id: str, statuses: dict[str, str], failures: dict[str, str]): + self.root_execution_id = root_execution_id + self.statuses = statuses + self.failures = failures + summary = ", ".join(f"{name}={status}" for name, status in sorted(failures.items())) + super().__init__( + f"Root execution {root_execution_id} had {len(failures)} failed task(s): {summary}" + ) + + @dataclass class PipelineSubmitPayload: """Prepared submit payload state before calling ``pipeline_runs_create``. @@ -817,11 +897,30 @@ def status_from_counts(status_counts: Mapping[str, int]) -> str | None: for status in _ACTIVE_STATUSES: if int(status_counts.get(status, 0) or 0) > 0: return status - for status in _TERMINAL_STATUSES: + for status in _TERMINAL_PRECEDENCE: if int(status_counts.get(status, 0) or 0) > 0: return status return None + @staticmethod + def _status_from_container_state(state: Mapping[str, Any] | Any) -> str | None: + plain = PipelineRunManager.to_plain(state) + if isinstance(plain, Mapping): + status = plain.get("status") + if isinstance(status, str) and status: + return status + return None + + @classmethod + def _status_from_graph_state(cls, state: Mapping[str, Any] | Any) -> str | None: + # A nested graph/subpipeline execution carries no direct ``status``; its + # status is derived from child execution counts the same way run-level + # graph polling does. + direct = cls._status_from_container_state(state) + if direct is not None: + return direct + return cls.status_from_counts(cls.status_counts_from_graph_state(cls.to_plain(state))) + @staticmethod def status_from_run(run: Mapping[str, Any]) -> str | None: summary = run.get("execution_summary") @@ -839,7 +938,7 @@ def status_from_run(run: Mapping[str, Any]) -> str | None: for status in _ACTIVE_STATUSES: if int(stats.get(status, 0) or 0) > 0: return status - for status in _TERMINAL_STATUSES: + for status in _TERMINAL_PRECEDENCE: if int(stats.get(status, 0) or 0) > 0: return status return None @@ -1261,6 +1360,174 @@ def graph_state_output(self, run_ids: list[str], *, timeout: float = 30.0) -> di def logs(self, execution_id: str) -> dict[str, Any]: return self.to_plain(self.hooks.fetch_logs(self.client, execution_id)) + def _exec_status(self, execution_id: str, *, deadline: float | None = None) -> str | None: + """Resolve a single execution's status, trying container then graph state. + + 404 means the execution is the other kind (container vs graph); a + transient 5xx means the state row is still being written. Both cases + fall through to the next endpoint and, if neither resolves, return + ``None`` so the caller reports UNKNOWN. Once past ``deadline`` (a + ``time.monotonic()`` timestamp) no further requests are issued, so one + poll cannot overrun the caller's wait budget. + """ + + endpoints = ( + (self.client.executions_container_state, self._status_from_container_state), + (self.client.executions_graph_execution_state, self._status_from_graph_state), + ) + context = f"Resolving status for execution {execution_id}" + last_server_error: int | None = None + for endpoint, extract_status in endpoints: + if deadline is not None and time.monotonic() >= deadline: + return None + try: + state = endpoint(execution_id) + except requests.HTTPError as exc: + response = exc.response + if response is not None and response.status_code in _RETRYABLE_EXEC_STATE_STATUSES: + if response.status_code >= 500: + last_server_error = response.status_code + continue + raise _transport_error(context, exc) from exc + except requests.RequestException as exc: + raise _transport_error(context, exc) from exc + status = extract_status(state) + if status is not None: + return status + if last_server_error is not None: + # Without this a wait that times out reports UNKNOWN with no hint + # that the state API was failing the whole time. + self.logger.warn( + f"Status for execution {execution_id} is UNKNOWN: state endpoints " + f"kept failing with HTTP {last_server_error}" + ) + return None + + def task_statuses( + self, root_execution_id: str, *, deadline: float | None = None + ) -> dict[str, str]: + """Return a ``{task_name: status}`` map for a root execution. + + Walks ``child_task_execution_ids`` from the root execution details and + resolves each child's status. A leaf/root-only execution (no children) + reports a single ``{"root": status}`` entry; unresolved child statuses + are reported as ``UNKNOWN``. A missing root id (404) is a clean + not-found error; other transport/HTTP failures surface cleanly too. + Past ``deadline`` (a ``time.monotonic()`` timestamp) remaining statuses + are reported as ``UNKNOWN`` without further requests. + """ + + try: + details = self.to_plain(self.client.executions_details(root_execution_id)) + except requests.HTTPError as exc: + response = exc.response + if response is not None and response.status_code == 404: + raise PipelineRunError(f"Root execution {root_execution_id} not found") from exc + raise _transport_error(f"Fetching root execution {root_execution_id}", exc) from exc + except requests.RequestException as exc: + raise _transport_error(f"Fetching root execution {root_execution_id}", exc) from exc + children = details.get("child_task_execution_ids") if isinstance(details, Mapping) else None + if not isinstance(children, Mapping) or not children: + return { + _ROOT_TASK_NAME: self._exec_status(root_execution_id, deadline=deadline) + or _UNKNOWN_TASK_STATUS + } + return { + str(task_name): self._child_status(execution_id, deadline=deadline) + for task_name, execution_id in children.items() + } + + def _child_status(self, execution_id: Any, *, deadline: float | None = None) -> str: + # A None/empty/whitespace-only child id has no execution to query; + # report UNKNOWN instead of polling a literal blank id forever. + child_id = str(execution_id).strip() if execution_id else "" + if not child_id: + return _UNKNOWN_TASK_STATUS + return self._exec_status(child_id, deadline=deadline) or _UNKNOWN_TASK_STATUS + + def wait_for_task_statuses( + self, + root_execution_id: str, + *, + max_wait: float | None = 1800.0, + poll_interval: float = 5.0, + allow_failure: bool = False, + ) -> dict[str, str]: + """Poll ``task_statuses`` until every task is terminal. + + Returns the final ``{task_name: status}`` map. Raises + ``TaskStatusesFailed`` when any task ends in a failure status unless + ``allow_failure`` is set, and ``PipelineRunError`` on timeout. + ``max_wait=None`` waits indefinitely. + """ + + self._validate_wait_params( + max_wait=max_wait, + poll_interval=poll_interval, + timeout_clock="monotonic", + allow_zero_poll_interval=False, + ) + deadline = None if max_wait is None else time.monotonic() + max_wait + root_fetch_failures = 0 + while True: + try: + statuses = self.task_statuses(root_execution_id, deadline=deadline) + except TransientServerError as exc: + # A 5xx on the root details fetch is tolerated like child-state + # 5xx (retried next poll) so one blip cannot abort a long wait. + # The deadline still bounds the retries; an unbounded wait gives + # up after a few consecutive failures instead of masking a + # persistent outage forever. + root_fetch_failures += 1 + if deadline is not None and time.monotonic() >= deadline: + raise + if deadline is None and root_fetch_failures >= _MAX_UNBOUNDED_ROOT_FETCH_FAILURES: + raise + if root_fetch_failures == 1: + # Warn once per outage, not once per poll, so a long outage + # does not flood the logs. + self.logger.warn(f"{exc}; retrying on the next poll") + self._sleep_before_next_poll(poll_interval, deadline) + continue + root_fetch_failures = 0 + if statuses and all(self.is_terminal_status(status) for status in statuses.values()): + return self._handle_terminal_task_statuses(root_execution_id, statuses, allow_failure) + if deadline is not None and time.monotonic() >= deadline: + pending = ", ".join( + f"{name}={status}" + for name, status in sorted(statuses.items()) + if not self.is_terminal_status(status) + ) + raise PipelineRunError( + f"Root execution {root_execution_id} still has non-terminal tasks " + f"after {max_wait:g}s: {pending}" + ) + self._sleep_before_next_poll(poll_interval, deadline) + + @staticmethod + def _sleep_before_next_poll(poll_interval: float, deadline: float | None) -> None: + if deadline is None: + time.sleep(poll_interval) + return + # Clamp the final sleep to the remaining budget so the loop times out + # at max_wait instead of overshooting by up to a poll interval. + time.sleep(min(poll_interval, max(0.0, deadline - time.monotonic()))) + + def _handle_terminal_task_statuses( + self, + root_execution_id: str, + statuses: dict[str, str], + allow_failure: bool, + ) -> dict[str, str]: + failures = { + name: status + for name, status in statuses.items() + if status.upper() in _TASK_FAILURE_STATUSES + } + if failures and not allow_failure: + raise TaskStatusesFailed(root_execution_id, statuses, failures) + return statuses + def search_runs( self, *, @@ -1452,6 +1719,32 @@ def _poll_run_status( execution_state_timings=execution_state_timings, ) + @staticmethod + def _validate_wait_params( + *, + max_wait: float | None, + poll_interval: float, + timeout_clock: str, + allow_zero_poll_interval: bool, + ) -> None: + """Validate wait/poll parameters, raising before any run is submitted.""" + + # NaN/inf pass the sign checks below (NaN compares False, inf is + # "positive") and would become a never-firing deadline or a raw + # ValueError from time.sleep; reject them up front. An unbounded wait is + # spelled max_wait=None, not an explicit inf. + if max_wait is not None: + if not math.isfinite(max_wait): + raise PipelineRunError("--max-wait must be a finite number") + if max_wait < 0: + raise PipelineRunError("--max-wait must be non-negative") + if not math.isfinite(poll_interval): + raise PipelineRunError("--poll-interval must be a finite number") + if poll_interval < 0 or (poll_interval == 0 and not allow_zero_poll_interval): + raise PipelineRunError("--poll-interval must be positive") + if timeout_clock not in {"monotonic", "wall"}: + raise PipelineRunError("timeout_clock must be 'monotonic' or 'wall'") + def wait_for_completion( self, run_id: str, @@ -1467,12 +1760,12 @@ def wait_for_completion( wait_context = context or PipelineRunContext(run_id=run_id, start_time=time.time()) if exit_on_first_failure: wait_context.metadata["exit_on_first_failure"] = True - if max_wait is not None and max_wait < 0: - raise PipelineRunError("--max-wait must be non-negative") - if poll_interval < 0 or (poll_interval == 0 and not allow_zero_poll_interval): - raise PipelineRunError("--poll-interval must be positive") - if timeout_clock not in {"monotonic", "wall"}: - raise PipelineRunError("timeout_clock must be 'monotonic' or 'wall'") + self._validate_wait_params( + max_wait=max_wait, + poll_interval=poll_interval, + timeout_clock=timeout_clock, + allow_zero_poll_interval=allow_zero_poll_interval, + ) enforce_max_wait = max_wait is not None and self.hooks.should_enforce_max_wait(wait_context) poll_started_at = time.monotonic() deadline_now: Callable[[], float] = time.time if timeout_clock == "wall" else time.monotonic diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py index 2da0eb5..33d50c7 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py @@ -4,6 +4,7 @@ import json import pathlib +import sys from typing import Annotated, Any from cyclopts import App, Parameter @@ -32,6 +33,7 @@ PipelineRunError, PipelineRunHooks, PipelineRunManager, + TaskStatusesFailed, parse_json_or_key_values, parse_key_value_entries, ) @@ -355,6 +357,83 @@ def pipeline_runs_wait( ) +@app.command(name="task-status") +def pipeline_runs_task_status( + root_execution_id: str | None = None, + *, + base_url: BaseUrlOption = None, + token: TokenOption = None, + auth_header: AuthHeaderOption = None, + header: HeaderOption = None, + config: ConfigOption = None, + log_type: LogTypeOption = "console", +) -> None: + """Print the {task_name: status} map for a root execution id, without polling. + + Walks the root execution's child task executions. A leaf/root-only execution + reports a single ``{"root": status}`` entry; unresolved children are UNKNOWN. + """ + specs = { + "root_execution_id": (root_execution_id,), + "log_type": (log_type, "console"), + **api_arg_specs(base_url=base_url, token=token, auth_header=auth_header, header=header), + } + _run_manager_action( + config, + base_url, + specs, + lambda manager, args: manager.task_statuses(args.root_execution_id), + ) + + +@app.command(name="task-wait") +def pipeline_runs_task_wait( + root_execution_id: str | None = None, + *, + max_wait: float = 1800.0, + poll_interval: float = 5.0, + allow_failure: bool = False, + base_url: BaseUrlOption = None, + token: TokenOption = None, + auth_header: AuthHeaderOption = None, + header: HeaderOption = None, + config: ConfigOption = None, + log_type: LogTypeOption = "console", +) -> None: + """Poll a root execution's task-status map until all tasks are terminal. + + Prints the final {task_name: status} map. Exit codes: 0 when every task + ends in a non-failure terminal status, 2 when any task fails and + --allow-failure is not set (the map is still printed), and non-zero on + timeout or API errors. + """ + specs = { + "root_execution_id": (root_execution_id,), + "max_wait": (max_wait, 1800.0), + "poll_interval": (poll_interval, 5.0), + "allow_failure": (allow_failure, False), + "log_type": (log_type, "console"), + **api_arg_specs(base_url=base_url, token=token, auth_header=auth_header, header=header), + } + + def action(manager: PipelineRunManager, args: ArgsContainer) -> dict[str, Any] | None: + try: + return manager.wait_for_task_statuses( + args.root_execution_id, + max_wait=float(args.max_wait), + poll_interval=float(args.poll_interval), + allow_failure=bool(args.allow_failure), + ) + except TaskStatusesFailed as exc: + # Print the full final map (failures retained on the exception drive + # the non-zero exit code) so succeeded/skipped tasks are not dropped. + print(str(exc), file=sys.stderr) + print_json(exc.statuses) + raise SystemExit(2) from exc + + _run_manager_action(config, base_url, specs, action) + + @app.command(name="logs") def pipeline_runs_logs( execution_id: str | None = None, diff --git a/tests/test_pipeline_runs_cli.py b/tests/test_pipeline_runs_cli.py index b74b0ad..bb348b3 100644 --- a/tests/test_pipeline_runs_cli.py +++ b/tests/test_pipeline_runs_cli.py @@ -2,16 +2,22 @@ import copy import json +import math +import time from contextlib import nullcontext from pathlib import Path from types import SimpleNamespace from typing import Any import pytest +import requests import yaml from tangle_cli import cli, pipeline_run_manager, pipeline_runs_cli +from tangle_cli.logger import CaptureLogger from tangle_cli.pipeline_run_manager import ( + _TASK_FAILURE_STATUSES, + _TERMINAL_STATUSES, AmbiguousPipelineRunRecoveryError, PipelineRunContext, PipelineRunError, @@ -19,6 +25,8 @@ PipelineRunManager, PipelineWaitOutcome, PipelineWaitPoll, + TaskStatusesFailed, + TransientServerError, ) from tangle_cli.pipeline_runner import PipelineRunner, PipelineRunnerHooks @@ -2790,3 +2798,553 @@ def cleanup_prepared_pipeline(self, preparation, *, error=None): # type: ignore assert cleaned == [(temp_effective_path, "Pipeline validation failed:\n - boom")] assert not temp_effective_path.exists() + + +def _http_error(status_code: int) -> requests.HTTPError: + response = requests.Response() + response.status_code = status_code + return requests.HTTPError(response=response) + + +class ExecutionStateClient: + """Configurable fake for the root-execution task-map endpoints. + + ``details`` maps a root execution id to its child_task_execution_ids; each + child/leaf id is resolved against ``container_state`` then + ``graph_execution_state``. Endpoint values may be a status dict, an HTTP + error to raise, or absent (KeyError-style 404). + """ + + def __init__( + self, + *, + details: dict[str, Any], + container_state: dict[str, Any] | None = None, + graph_state: dict[str, Any] | None = None, + ) -> None: + self.base_url = "https://tangle.example" + self._details = details + self._container_state = container_state or {} + self._graph_state = graph_state or {} + self.details_calls: list[str] = [] + self.container_calls: list[str] = [] + self.graph_calls: list[str] = [] + + def executions_details(self, id: str) -> dict[str, Any]: + return self._resolve(self._details, self.details_calls, id) + + def _resolve(self, store: dict[str, Any], calls: list[str], id: str) -> dict[str, Any]: + calls.append(id) + if id not in store: + raise _http_error(404) + value = store[id] + if isinstance(value, requests.RequestException): + raise value + return value + + def executions_container_state(self, id: str, **_: Any) -> dict[str, Any]: + return self._resolve(self._container_state, self.container_calls, id) + + def executions_graph_execution_state(self, id: str) -> dict[str, Any]: + return self._resolve(self._graph_state, self.graph_calls, id) + + +def test_task_statuses_walks_children_via_container_state() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a", "b": "exec-b"}}}, + container_state={ + "exec-a": {"status": "SUCCEEDED"}, + "exec-b": {"status": "RUNNING"}, + }, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == {"a": "SUCCEEDED", "b": "RUNNING"} + # Container state resolves first, so graph state is never queried. + assert client.graph_calls == [] + + +def test_task_statuses_falsy_child_id_is_unknown_without_api_call() -> None: + # A None/empty/whitespace-only child execution id maps straight to UNKNOWN; + # it must not be stringified to "None"/blank and queried against the state + # endpoints. + client = ExecutionStateClient( + details={ + "root-1": {"child_task_execution_ids": {"a": None, "b": "", "c": "exec-c", "d": " "}} + }, + container_state={"exec-c": {"status": "RUNNING"}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == { + "a": "UNKNOWN", + "b": "UNKNOWN", + "c": "RUNNING", + "d": "UNKNOWN", + } + # Only the real child id reaches the state API. + assert client.container_calls == ["exec-c"] + assert client.graph_calls == [] + + +def test_task_statuses_root_only_execution_reports_root_key() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {}}}, + container_state={"root-1": {"status": "SUCCEEDED"}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == {"root": "SUCCEEDED"} + + +def test_task_statuses_falls_back_to_graph_state_on_404() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={}, # exec-a missing -> 404 from container state + graph_state={"exec-a": {"status": "RUNNING"}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == {"a": "RUNNING"} + assert client.container_calls == ["exec-a"] + assert client.graph_calls == ["exec-a"] + + +def test_task_statuses_persistent_5xx_reported_as_unknown() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": _http_error(503)}, + graph_state={"exec-a": _http_error(503)}, + ) + logger = CaptureLogger() + manager = PipelineRunManager(client=client, logger=logger) + + assert manager.task_statuses("root-1") == {"a": "UNKNOWN"} + # The persistent 5xx is announced so an UNKNOWN result is diagnosable. + logs = logger.get_logs() or "" + assert "exec-a" in logs + assert "503" in logs + + +def test_task_statuses_past_deadline_reports_unknown_without_fetching() -> None: + # Past the caller's deadline no further state requests are issued, so one + # task-statuses poll cannot overrun the wait budget of task-wait. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": {"status": "SUCCEEDED"}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1", deadline=time.monotonic() - 1) == {"a": "UNKNOWN"} + assert client.container_calls == [] + assert client.graph_calls == [] + + +def test_failure_statuses_are_the_non_skipped_non_succeeded_terminals() -> None: + # _TASK_FAILURE_STATUSES is derived from _TERMINAL_STATUSES; guard the intended + # membership so a new failure terminal can't be silently treated as success. + assert _TASK_FAILURE_STATUSES == {"FAILED", "SYSTEM_ERROR", "CANCELLED", "CANCELED", "INVALID"} + assert _TASK_FAILURE_STATUSES == frozenset(_TERMINAL_STATUSES) - {"SKIPPED", "SUCCEEDED"} + + +def test_task_statuses_wraps_non_retryable_http_error() -> None: + # A non-retryable state-endpoint status (429/403/etc.) becomes a clean + # PipelineRunError instead of a raw requests traceback out of task-status. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": _http_error(429)}, + ) + manager = PipelineRunManager(client=client) + + with pytest.raises(PipelineRunError, match="HTTP 429"): + manager.task_statuses("root-1") + + +def test_task_statuses_wraps_connection_error() -> None: + # A transport failure (no HTTP response) is surfaced cleanly rather than + # escaping as a raw requests.ConnectionError. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": requests.ConnectionError("connection refused")}, + ) + manager = PipelineRunManager(client=client) + + with pytest.raises(PipelineRunError, match="connection refused"): + manager.task_statuses("root-1") + + +def test_task_statuses_missing_root_is_not_found() -> None: + # A 404 on the root execution details is a clean not-found error, not a + # masked {"root": "UNKNOWN"} map. + client = ExecutionStateClient(details={"root-1": _http_error(404)}) + manager = PipelineRunManager(client=client) + + with pytest.raises(PipelineRunError, match="not found"): + manager.task_statuses("root-1") + + +def test_task_statuses_root_details_transport_error_surfaces_cleanly() -> None: + client = ExecutionStateClient(details={"root-1": requests.ConnectionError("boom")}) + manager = PipelineRunManager(client=client) + + with pytest.raises(PipelineRunError, match="boom"): + manager.task_statuses("root-1") + + +def test_task_statuses_root_details_5xx_is_transient_error() -> None: + # A one-shot task-status still fails fast on a root details 5xx, but the + # error is typed so the task-wait polling loop can retry it. + client = ExecutionStateClient(details={"root-1": _http_error(503)}) + manager = PipelineRunManager(client=client) + + with pytest.raises(TransientServerError, match="HTTP 503"): + manager.task_statuses("root-1") + + +def test_task_statuses_derives_graph_status_from_child_stats() -> None: + # A child that is itself a graph/subpipeline has no direct `status`; derive + # it from child_execution_status_stats counts rather than reporting UNKNOWN. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, # 404 -> fall back to graph state + graph_state={"exec-sub": {"child_execution_status_stats": {"leaf": {"SUCCEEDED": 1}}}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == {"sub": "SUCCEEDED"} + + +def test_task_statuses_derives_graph_status_from_status_totals() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, + graph_state={"exec-sub": {"status_totals": {"RUNNING": 1}}}, + ) + manager = PipelineRunManager(client=client) + + # An active count outranks terminal counts, matching status_from_counts. + assert manager.task_statuses("root-1") == {"sub": "RUNNING"} + + +def test_task_statuses_prefers_direct_graph_status_over_counts() -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, + graph_state={"exec-sub": {"status": "FAILED", "status_totals": {"SUCCEEDED": 1}}}, + ) + manager = PipelineRunManager(client=client) + + assert manager.task_statuses("root-1") == {"sub": "FAILED"} + + +@pytest.mark.parametrize( + "counts, expected", + [ + # An active/nonterminal status must outrank terminal success. + ({"SUCCEEDED": 1, "WAITING_FOR_UPSTREAM": 1}, "WAITING_FOR_UPSTREAM"), + ({"SUCCEEDED": 1, "UNINITIALIZED": 1}, "UNINITIALIZED"), + # Failure/invalid terminal statuses must outrank SUCCEEDED. + ({"SUCCEEDED": 1, "INVALID": 1}, "INVALID"), + ({"SUCCEEDED": 1, "FAILED": 1}, "FAILED"), + # INVALID must also outrank the other non-failure terminal, SKIPPED, so a + # mixed graph aggregate does not mask an invalid child behind SKIPPED. + ({"SKIPPED": 1, "INVALID": 1}, "INVALID"), + ({"SKIPPED": 1, "FAILED": 1}, "FAILED"), + ], +) +def test_status_from_counts_nonterminal_and_failure_outrank_success(counts, expected) -> None: + assert PipelineRunManager.status_from_counts(counts) == expected + + +def test_task_statuses_nested_graph_waiting_is_nonterminal_and_blocks_wait() -> None: + # {"SUCCEEDED": 1, "WAITING_FOR_UPSTREAM": 1} must not collapse to SUCCEEDED + # and must not be treated as terminal by task-wait. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, + graph_state={ + "exec-sub": {"child_execution_status_stats": {"leaf": {"SUCCEEDED": 1, "WAITING_FOR_UPSTREAM": 1}}} + }, + ) + manager = PipelineRunManager(client=client) + + statuses = manager.task_statuses("root-1") + assert statuses == {"sub": "WAITING_FOR_UPSTREAM"} + assert not PipelineRunManager.is_terminal_status(statuses["sub"]) + + +def test_task_statuses_nested_graph_invalid_is_failure(monkeypatch) -> None: + # {"SUCCEEDED": 1, "INVALID": 1} reduces to INVALID, so task-wait fails. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, + graph_state={"exec-sub": {"status_totals": {"SUCCEEDED": 1, "INVALID": 1}}}, + ) + manager = PipelineRunManager(client=client) + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: None) + + with pytest.raises(TaskStatusesFailed) as exc_info: + manager.wait_for_task_statuses("root-1", poll_interval=1) + assert exc_info.value.statuses == {"sub": "INVALID"} + + +def test_task_statuses_nested_graph_skipped_and_invalid_collapses_to_invalid(monkeypatch) -> None: + # A nested-graph child whose counts mix SKIPPED and INVALID must reduce to + # INVALID (the failure), not SKIPPED, so task-wait surfaces the invalid child. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"sub": "exec-sub"}}}, + container_state={}, + graph_state={"exec-sub": {"status_totals": {"SKIPPED": 1, "INVALID": 1}}}, + ) + manager = PipelineRunManager(client=client) + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: None) + + assert manager.task_statuses("root-1") == {"sub": "INVALID"} + with pytest.raises(TaskStatusesFailed) as exc_info: + manager.wait_for_task_statuses("root-1", poll_interval=1) + assert exc_info.value.failures == {"sub": "INVALID"} + + +def test_wait_for_task_statuses_skipped_is_successful_terminal() -> None: + # SKIPPED is a non-failure terminal (a skipped task, e.g. a conditional + # branch, is not a failure), so a fully SKIPPED task map returns + # successfully (exit 0), not as a failure. + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + manager.task_statuses = lambda root_id, deadline=None: {"a": "SKIPPED"} # type: ignore[method-assign] + + assert manager.wait_for_task_statuses("root-1") == {"a": "SKIPPED"} + + +def test_wait_for_task_statuses_returns_terminal_map(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: sleeps.append(value)) + statuses = iter([{"a": "RUNNING"}, {"a": "SUCCEEDED"}]) + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + monkeypatch.setattr(manager, "task_statuses", lambda root_id, deadline=None: next(statuses)) + + assert manager.wait_for_task_statuses("root-1", poll_interval=1) == {"a": "SUCCEEDED"} + assert sleeps == [1] + + +def test_wait_for_task_statuses_max_wait_none_waits_until_terminal(monkeypatch) -> None: + # max_wait=None waits indefinitely: no deadline is computed and the plain + # poll interval is slept between checks. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: sleeps.append(value)) + statuses = iter([{"a": "RUNNING"}, {"a": "RUNNING"}, {"a": "SUCCEEDED"}]) + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + monkeypatch.setattr(manager, "task_statuses", lambda root_id, deadline=None: next(statuses)) + + result = manager.wait_for_task_statuses("root-1", max_wait=None, poll_interval=3) + + assert result == {"a": "SUCCEEDED"} + assert sleeps == [3, 3] + + +def test_wait_for_task_statuses_raises_on_failure() -> None: + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + manager.task_statuses = lambda root_id, deadline=None: {"a": "SUCCEEDED", "b": "FAILED"} # type: ignore[method-assign] + + with pytest.raises(TaskStatusesFailed) as exc_info: + manager.wait_for_task_statuses("root-1") + # Failures drive the exit code, but the full final map is retained too. + assert exc_info.value.failures == {"b": "FAILED"} + assert exc_info.value.statuses == {"a": "SUCCEEDED", "b": "FAILED"} + + +def test_wait_for_task_statuses_allow_failure_returns_map() -> None: + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + final = {"a": "SUCCEEDED", "b": "CANCELLED"} + manager.task_statuses = lambda root_id, deadline=None: final # type: ignore[method-assign] + + assert manager.wait_for_task_statuses("root-1", allow_failure=True) == final + + +def test_wait_for_task_statuses_timeout(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: None) + # deadline calc, first check, sleep-clamp read, then second check past deadline. + ticks = iter([0.0, 0.0, 0.0, 100.0]) + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.monotonic", lambda: next(ticks)) + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + manager.task_statuses = lambda root_id, deadline=None: {"a": "RUNNING"} # type: ignore[method-assign] + + with pytest.raises(PipelineRunError, match="non-terminal"): + manager.wait_for_task_statuses("root-1", max_wait=10, poll_interval=1) + + +def test_wait_for_task_statuses_clamps_final_sleep_to_remaining(monkeypatch) -> None: + # Near the deadline the final sleep must shrink to the remaining budget so the + # loop times out at max_wait instead of overshooting by a full poll interval. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: sleeps.append(value)) + # deadline calc -> 0 (deadline=10); check -> 8 (not past); sleep clamp -> 8 + # (10-8=2 < poll_interval 5); next check -> 10 (timeout). + ticks = iter([0.0, 8.0, 8.0, 10.0]) + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.monotonic", lambda: next(ticks)) + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + manager.task_statuses = lambda root_id, deadline=None: {"a": "RUNNING"} # type: ignore[method-assign] + + with pytest.raises(PipelineRunError, match="non-terminal"): + manager.wait_for_task_statuses("root-1", max_wait=10, poll_interval=5) + assert sleeps == [2.0] + + +def test_wait_for_task_statuses_retries_transient_root_fetch_5xx(monkeypatch) -> None: + # A single 5xx blip on the per-poll root details fetch must not abort a + # long wait; it is tolerated like child-state 5xx and retried next poll. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: sleeps.append(value)) + outcomes = iter([TransientServerError("Fetching root execution root-1: request failed with HTTP 503"), {"a": "SUCCEEDED"}]) + + def fetch(root_id: str, deadline: float | None = None) -> dict[str, str]: + value = next(outcomes) + if isinstance(value, Exception): + raise value + return value + + logger = CaptureLogger() + manager = PipelineRunManager(client=ExecutionStateClient(details={}), logger=logger) + monkeypatch.setattr(manager, "task_statuses", fetch) + + assert manager.wait_for_task_statuses("root-1", poll_interval=1) == {"a": "SUCCEEDED"} + assert sleeps == [1] + # The tolerated blip is announced so a later timeout is diagnosable. + assert "HTTP 503" in (logger.get_logs() or "") + + +def test_wait_for_task_statuses_persistent_root_fetch_5xx_raises_at_deadline(monkeypatch) -> None: + # Retrying the root fetch is bounded by the wait deadline: a persistent + # outage surfaces as the underlying server error once max_wait elapses, + # and the warning is logged once per outage rather than once per poll. + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: None) + # deadline calc, first-failure deadline check, sleep-clamp read, then + # second-failure deadline check past the deadline. + ticks = iter([0.0, 0.0, 0.0, 100.0]) + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.monotonic", lambda: next(ticks)) + logger = CaptureLogger() + manager = PipelineRunManager(client=ExecutionStateClient(details={}), logger=logger) + + def fetch(root_id: str, deadline: float | None = None) -> dict[str, str]: + raise TransientServerError("Fetching root execution root-1: request failed with HTTP 502") + + monkeypatch.setattr(manager, "task_statuses", fetch) + + with pytest.raises(TransientServerError, match="HTTP 502"): + manager.wait_for_task_statuses("root-1", max_wait=10, poll_interval=1) + assert (logger.get_logs() or "").count("HTTP 502") == 1 + + +def test_wait_for_task_statuses_unbounded_wait_caps_root_fetch_retries(monkeypatch) -> None: + # With max_wait=None there is no deadline to stop the retries, so a + # persistent root fetch outage must still surface after a bounded number + # of consecutive failures instead of being retried forever. + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda value: None) + calls = 0 + + def fetch(root_id: str, deadline: float | None = None) -> dict[str, str]: + nonlocal calls + calls += 1 + raise TransientServerError("Fetching root execution root-1: request failed with HTTP 500") + + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + monkeypatch.setattr(manager, "task_statuses", fetch) + + with pytest.raises(TransientServerError, match="HTTP 500"): + manager.wait_for_task_statuses("root-1", max_wait=None, poll_interval=1) + assert calls == pipeline_run_manager._MAX_UNBOUNDED_ROOT_FETCH_FAILURES + + +def test_wait_for_task_statuses_rejects_invalid_bounds() -> None: + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + with pytest.raises(PipelineRunError, match="max-wait"): + manager.wait_for_task_statuses("root-1", max_wait=-1) + with pytest.raises(PipelineRunError, match="poll-interval"): + manager.wait_for_task_statuses("root-1", poll_interval=-1) + + +def test_wait_for_task_statuses_rejects_non_finite_bounds() -> None: + # NaN/inf bounds slip past the sign checks; they must reject cleanly instead + # of reaching time.sleep (raw ValueError) or creating an unbounded deadline. + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + for bad in (math.nan, math.inf): + with pytest.raises(PipelineRunError, match="max-wait must be a finite number"): + manager.wait_for_task_statuses("root-1", max_wait=bad) + with pytest.raises(PipelineRunError, match="poll-interval must be a finite number"): + manager.wait_for_task_statuses("root-1", max_wait=1, poll_interval=bad) + + +def test_wait_for_task_statuses_rejects_zero_poll_interval_without_spinning(monkeypatch) -> None: + # A zero interval must be rejected before any polling, not busy-loop. + calls = 0 + + def counting_statuses(root_id: str, deadline: float | None = None) -> dict[str, str]: + nonlocal calls + calls += 1 + return {"a": "RUNNING"} + + manager = PipelineRunManager(client=ExecutionStateClient(details={})) + monkeypatch.setattr(manager, "task_statuses", counting_statuses) + + with pytest.raises(PipelineRunError, match="poll-interval must be positive"): + manager.wait_for_task_statuses("root-1", max_wait=0.02, poll_interval=0) + assert calls == 0 + + +def test_cli_task_status_outputs_json(monkeypatch, capsys) -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": {"status": "SUCCEEDED"}}, + ) + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: client) + + run_app(cli.build_app(), ["sdk", "pipeline-runs", "task-status", "root-1"]) + + assert json.loads(capsys.readouterr().out) == {"a": "SUCCEEDED"} + + +def test_cli_task_wait_failure_exits_nonzero_and_prints_full_map(monkeypatch, capsys) -> None: + # Mixed outcome: the failure exit code must not drop the succeeded task. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a", "b": "exec-b"}}}, + container_state={ + "exec-a": {"status": "SUCCEEDED"}, + "exec-b": {"status": "FAILED"}, + }, + ) + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: client) + + with pytest.raises(SystemExit) as exc_info: + # All tasks are already terminal, so the first poll returns without sleeping. + run_app(cli.build_app(), ["sdk", "pipeline-runs", "task-wait", "root-1", "--poll-interval", "1"]) + + assert exc_info.value.code == 2 + captured = capsys.readouterr() + assert json.loads(captured.out) == {"a": "SUCCEEDED", "b": "FAILED"} + # A human-readable failure summary goes to stderr; stdout stays pure JSON. + assert "failed task" in captured.err + + +def test_cli_task_wait_success_outputs_map(monkeypatch, capsys) -> None: + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": {"status": "SUCCEEDED"}}, + ) + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: client) + + run_app(cli.build_app(), ["sdk", "pipeline-runs", "task-wait", "root-1", "--poll-interval", "1"]) + + assert json.loads(capsys.readouterr().out) == {"a": "SUCCEEDED"} + + +def test_cli_task_wait_skipped_exits_zero(monkeypatch, capsys) -> None: + # SKIPPED is a non-failure terminal, so task-wait must exit 0 rather than + # raise SystemExit. + client = ExecutionStateClient( + details={"root-1": {"child_task_execution_ids": {"a": "exec-a"}}}, + container_state={"exec-a": {"status": "SKIPPED"}}, + ) + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: client) + + run_app(cli.build_app(), ["sdk", "pipeline-runs", "task-wait", "root-1", "--poll-interval", "1"]) + + assert json.loads(capsys.readouterr().out) == {"a": "SKIPPED"} From d13ce2eeea4a05e23532002586928088118c789b Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Mon, 13 Jul 2026 14:00:53 +0000 Subject: [PATCH 2/7] feat(pipeline-run): stream execution logs Add --stream to tangle sdk pipeline-runs logs to follow container logs live instead of fetching a one-shot snapshot. Opening the stream has its own bounded retry budget for transport errors and retryable 5xx responses, spent before the first line is yielded. An established stream is never reopened, so output cannot be duplicated; mid-stream drops, open failures, BrokenPipe and Ctrl-C all exit without tracebacks. --- packages/tangle-cli/src/tangle_cli/client.py | 164 ++++++- .../src/tangle_cli/pipeline_run_manager.py | 64 ++- .../src/tangle_cli/pipeline_runs_cli.py | 23 + tests/test_client.py | 426 ++++++++++++++++++ tests/test_pipeline_runs_cli.py | 202 +++++++++ 5 files changed, 865 insertions(+), 14 deletions(-) diff --git a/packages/tangle-cli/src/tangle_cli/client.py b/packages/tangle-cli/src/tangle_cli/client.py index 060c960..e1dac0f 100644 --- a/packages/tangle-cli/src/tangle_cli/client.py +++ b/packages/tangle-cli/src/tangle_cli/client.py @@ -9,7 +9,7 @@ from __future__ import annotations import time -from collections.abc import Iterable, Mapping +from collections.abc import Iterable, Iterator, Mapping from dataclasses import asdict, is_dataclass from email.utils import parsedate_to_datetime from typing import Any @@ -53,6 +53,14 @@ class TangleApiClient(GeneratedTangleApiOperations): _MAX_RATE_LIMIT_RETRIES = 3 _RATE_LIMIT_BACKOFF_SECONDS = 1.0 _MAX_RETRY_AFTER_SECONDS = 60.0 + # Opening a log stream retries transient failures (transport-open errors + # and retryable 5xx) with doubling backoff, spent entirely before any line + # is yielded. An already-open stream that drops is the caller's to handle; + # never re-opening an established stream means lines cannot be duplicated. + _RETRYABLE_STREAM_STATUSES = frozenset({500, 502, 503, 504}) + _MAX_STREAM_OPEN_ATTEMPTS = 7 + _STREAM_OPEN_BACKOFF_SECONDS = 1.0 + _MAX_STREAM_OPEN_BACKOFF_SECONDS = 30.0 def __init__( self, @@ -132,6 +140,12 @@ def _make_request( request_kwargs=kwargs, ) if response.status_code == 401: + # The 401 response is discarded by the auth-refresh retry. For a + # streamed request it is an open streamed connection, so close it + # before refreshing auth and issuing the second request to avoid + # leaking it. ``response.headers`` stays available after close. + if kwargs.get("stream"): + response.close() self._refresh_auth() response = self._request_with_rate_limit_retries( request_method, @@ -152,7 +166,7 @@ def _request_with_rate_limit_retries( params: Mapping[str, Any] | None, json_data: Any, extra_headers: Mapping[str, str] | None, - timeout: float, + timeout: float | tuple[float, float | None], request_kwargs: Mapping[str, Any], ) -> requests.Response: response: requests.Response | None = None @@ -168,6 +182,13 @@ def _request_with_rate_limit_retries( ) if response.status_code != 429 or attempt == self._MAX_RATE_LIMIT_RETRIES: return response + # The 429 response is discarded by the retry. For a streamed request + # it is an open streamed connection, so close it before the sleep to + # avoid holding it open while we wait. ``response.headers`` stays + # available after close, so ``_sleep_for_rate_limit`` can still read + # Retry-After. + if request_kwargs.get("stream"): + response.close() self._sleep_for_rate_limit(response, attempt) return response @@ -181,6 +202,14 @@ def _sleep_for_rate_limit(self, response: requests.Response, attempt: int) -> No self.logger.info(f"429 rate limited; retrying in {delay:.1f}s") time.sleep(delay) + def _sleep_for_stream_open_retry(self, backoff: float, next_attempt: int, reason: str) -> None: + delay = min(backoff, self._MAX_STREAM_OPEN_BACKOFF_SECONDS) + self.logger.warn( + f"transient {reason} opening log stream; retrying in {delay:.1f}s " + f"(attempt {next_attempt}/{self._MAX_STREAM_OPEN_ATTEMPTS})" + ) + time.sleep(delay) + @staticmethod def _retry_after_delay(value: str | None) -> float | None: if not value: @@ -205,7 +234,7 @@ def _request_with_same_origin_redirects( params: Mapping[str, Any] | None, json_data: Any, extra_headers: Mapping[str, str] | None, - timeout: float, + timeout: float | tuple[float, float | None], request_kwargs: Mapping[str, Any], ) -> requests.Response: """Send one request, following only same-origin redirects. @@ -235,6 +264,16 @@ def _request_with_same_origin_redirects( **request_kwargs, ) if self.verbose: + # For streamed responses, reading ``response.text`` would buffer + # the entire body here, defeating callers that stream via + # ``iter_content``/``iter_lines``; a followed container-log + # stream may never terminate. Log a placeholder and leave the + # body unread. + response_body = ( + "" + if request_kwargs.get("stream") + else response.text + ) log_http_exchange( self.logger, method=current_method, @@ -243,7 +282,7 @@ def _request_with_same_origin_redirects( request_body=current_json, response_status=response.status_code, response_headers=dict(response.headers), - response_body=response.text, + response_body=response_body, ) if response.status_code not in self._REDIRECT_STATUSES: return response @@ -254,6 +293,13 @@ def _request_with_same_origin_redirects( next_url = urljoin(response.url, location) if not self._same_origin(response.url, next_url): + # Close before raising so callers that catch this and fall back + # to another route (or a streamed open) do not leak the open + # streamed redirect response and its pooled connection. + try: + response.close() + except Exception: + pass raise requests.HTTPError( f"Refusing to follow cross-origin redirect from {response.url} to {next_url}", response=response, @@ -358,16 +404,108 @@ def get_execution_details(self, execution_id: str) -> GetExecutionInfoResponse: return details def stream_execution_container_log(self, execution_id: str) -> requests.Response: - response = self._make_request( - "GET", - self._format_path( - "/api/executions/{id}/stream_container_log", - {"id": execution_id}, - ), - stream=True, + """Open the streaming container-log response for ``execution_id``. + + The endpoint delivers raw log lines over a long-lived chunked HTTP + response; the client streams those lines as-is and does no + event-protocol parsing. + + Establishing the stream (open + status check) follows a transient-error + retry budget: transport-open errors (connection/timeout) and retryable + 5xx responses are retried with exponential backoff before any line is + read. Same-origin redirect protection errors (cross-origin ``HTTPError`` + / ``TooManyRedirects``) are not transport blips and propagate + immediately. Once the stream is open the caller owns the response and + must close it; :meth:`iter_execution_container_log_lines` does that. + + The request uses ``(connect, read)`` timeouts of ``(self.timeout, + None)``: the connect timeout still bounds opening the stream, but there + is no per-read timeout, because a healthy follow stream stays silent for + as long as the container emits no output. + """ + + path = self._format_path( + "/api/executions/{id}/stream_container_log", + {"id": execution_id}, ) - response.raise_for_status() - return response + backoff = self._STREAM_OPEN_BACKOFF_SECONDS + last_exc: requests.RequestException | None = None + last_error_response: requests.Response | None = None + for attempt in range(1, self._MAX_STREAM_OPEN_ATTEMPTS + 1): + try: + response = self._make_request( + "GET", path, stream=True, timeout=(self.timeout, None) + ) + except (requests.HTTPError, requests.TooManyRedirects) as exc: + # Same-origin redirect guard errors carry the rejected streamed + # response and are intentionally not retried. No iterator ever + # receives that response, so close it before re-raising. + if exc.response is not None: + exc.response.close() + raise + except (requests.ConnectionError, requests.Timeout) as exc: + last_exc = exc + last_error_response = None + if attempt == self._MAX_STREAM_OPEN_ATTEMPTS: + break + self._sleep_for_stream_open_retry(backoff, attempt + 1, type(exc).__name__) + backoff *= 2.0 + continue + if response.status_code in self._RETRYABLE_STREAM_STATUSES: + response.close() + last_error_response = response + last_exc = None + if attempt == self._MAX_STREAM_OPEN_ATTEMPTS: + break + self._sleep_for_stream_open_retry( + backoff, attempt + 1, f"HTTP {response.status_code}" + ) + backoff *= 2.0 + continue + try: + response.raise_for_status() + except requests.HTTPError: + # Non-retryable status (e.g. 400/403/404): close the open + # streamed response before propagating so it is not leaked. + response.close() + raise + return response + if last_exc is not None: + raise last_exc + if last_error_response is not None: + last_error_response.raise_for_status() + # Defensive: every exhausted attempt records either a transport error + # (re-raised above) or a retryable-status response (raise_for_status + # always raises for those), so this cannot be reached. + raise RuntimeError( # pragma: no cover + "log stream open retries exhausted without a failure to re-raise" + ) + + def iter_execution_container_log_lines(self, execution_id: str) -> Iterable[str]: + """Return an iterator of decoded container-log lines for ``execution_id``. + + The stream is opened eagerly, so open failures (HTTP status or transport + errors) raise from this call rather than on first iteration; anything + raised while iterating is a drop of an already-open stream. The + underlying streaming response is always closed when iteration finishes + or the consumer stops early. + """ + + response = self.stream_execution_container_log(execution_id) + + def lines() -> Iterator[str]: + try: + # Decode whole ``bytes`` lines as UTF-8 explicitly rather than + # via ``decode_unicode=True``: requests' charset guessing falls + # back to latin-1 when the response declares no charset and + # would mojibake non-ASCII output. Whole-line decoding also + # reassembles multibyte sequences split across stream chunks. + for raw in response.iter_lines(): + yield raw.decode("utf-8", "replace") + finally: + response.close() + + return lines() def get_component_spec(self, digest: str) -> ComponentSpec: """Return a parsed domain component spec from the generated component endpoint.""" diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py index 24742fa..66e17a1 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py @@ -15,12 +15,13 @@ import re import time import uuid -from collections.abc import Callable +from collections.abc import Callable, Iterable from contextlib import AbstractContextManager, nullcontext from dataclasses import dataclass, field from pathlib import Path from typing import Any, Mapping +import requests import yaml from .handler import TangleCliHandler @@ -54,6 +55,27 @@ class AmbiguousPipelineRunRecoveryError(PipelineRunError): """Raised when submit recovery finds multiple runs for one submission id.""" +def _describe_http_error(exc: requests.HTTPError) -> str: + """Summarize an HTTP status failure with the attempted request target. + + ``str(exc)`` alone can omit the URL (e.g. errors constructed by client + helpers), so read the status, reason, and method/URL off the attached + response when present. + """ + + response = exc.response + if response is None: + return str(exc) + request = response.request + target = ( + f"{request.method} {request.url}" + if request is not None and request.url + else (response.url or "Tangle API") + ) + reason = f" {response.reason}" if response.reason else "" + return f"HTTP {response.status_code}{reason} for {target}" + + @dataclass class PipelineSubmitPayload: """Prepared submit payload state before calling ``pipeline_runs_create``. @@ -599,6 +621,14 @@ def fetch_logs(self, client: Any, execution_id: str) -> Any: """Hook for alternate TD log providers; OSS uses the Tangle API only.""" return client.executions_container_log(execution_id) + def stream_logs(self, client: Any, execution_id: str) -> Iterable[str]: + """Hook for alternate TD log providers; OSS streams via the Tangle API. + + Failures to open the stream must raise from this call; exceptions raised + while iterating are reported as mid-stream interruptions. + """ + return client.iter_execution_container_log_lines(execution_id) + @dataclass class PipelineRunManager(TangleCliHandler): @@ -1261,6 +1291,38 @@ def graph_state_output(self, run_ids: list[str], *, timeout: float = 30.0) -> di def logs(self, execution_id: str) -> dict[str, Any]: return self.to_plain(self.hooks.fetch_logs(self.client, execution_id)) + def stream_logs(self, execution_id: str) -> Iterable[str]: + try: + lines = self.hooks.stream_logs(self.client, execution_id) + except requests.HTTPError as exc: + # A definitive non-2xx answer to the stream-open request (404 for a + # missing execution or endpoint, 403, ...): keep the status and the + # attempted target visible, since that is what the caller acts on. + raise PipelineRunError( + f"Failed to open log stream for execution {execution_id}: " + f"{_describe_http_error(exc)}" + ) from exc + except requests.RequestException as exc: + # Non-HTTP transport failures (connection refused, timeout) raise + # from the open call itself; surface them as a clean open failure. + raise PipelineRunError( + f"Failed to open log stream for execution {execution_id}: {exc}" + ) from exc + return self._relabel_stream_drops(lines, execution_id) + + @staticmethod + def _relabel_stream_drops(lines: Iterable[str], execution_id: str) -> Iterable[str]: + try: + yield from lines + except requests.RequestException as exc: + # The stream opened (the hook call succeeded), so a transport + # failure here is a drop of the live follow, possibly before the + # first line arrived. Surface it as an interruption rather than a + # fetch failure, which would wrongly imply the initial open failed. + raise PipelineRunError( + f"Log stream for execution {execution_id} was interrupted: {exc}" + ) from exc + def search_runs( self, *, diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py index 2da0eb5..bb9052b 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py @@ -3,7 +3,9 @@ from __future__ import annotations import json +import os import pathlib +import sys from typing import Annotated, Any from cyclopts import App, Parameter @@ -359,6 +361,14 @@ def pipeline_runs_wait( def pipeline_runs_logs( execution_id: str | None = None, *, + stream: Annotated[ + bool | None, + Parameter( + help="Follow the live log stream instead of fetching a one-shot snapshot. " + "The follow has no read timeout and stays open silently while the " + "container emits no output." + ), + ] = None, base_url: BaseUrlOption = None, token: TokenOption = None, auth_header: AuthHeaderOption = None, @@ -369,11 +379,24 @@ def pipeline_runs_logs( """Print Tangle API container logs for an execution id.""" specs = { "execution_id": (execution_id,), + "stream": (stream, None), "log_type": (log_type, "console"), **api_arg_specs(base_url=base_url, token=token, auth_header=auth_header, header=header), } def action(manager: PipelineRunManager, args: ArgsContainer) -> object: + if args.stream: + try: + for line in manager.stream_logs(args.execution_id): + print(line, flush=True) + except BrokenPipeError: + # The downstream reader closed the pipe (e.g. `... | head`). + # Point stdout at devnull so the interpreter's exit-time flush + # of the closed pipe cannot raise a second BrokenPipeError. + devnull_fd = os.open(os.devnull, os.O_WRONLY) + os.dup2(devnull_fd, sys.stdout.fileno()) + os.close(devnull_fd) + return None result = manager.logs(args.execution_id) if isinstance(result, dict) and isinstance(result.get("log_text"), str): print(result["log_text"], end="" if result["log_text"].endswith("\n") else "\n") diff --git a/tests/test_client.py b/tests/test_client.py index 5456ac0..881ac0f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,13 +1,47 @@ from __future__ import annotations +import io +import json from types import SimpleNamespace +from typing import Any from unittest.mock import MagicMock +import pytest +import requests + import tangle_cli.client as client_module from tangle_cli.client import TangleApiClient +from tangle_cli.logger import CaptureLogger from tangle_cli.models import ComponentInfo +def _response(payload: Any = None, status_code: int = 200) -> requests.Response: + r = requests.Response() + r.status_code = status_code + if payload is None: + r._content = b"" + else: + r._content = json.dumps(payload).encode("utf-8") + r.headers["Content-Type"] = "application/json" + r.request = requests.Request("GET", "https://api.test").prepare() + return r + + +class _FakeSession: + def __init__(self, responses: list[requests.Response | Exception] | None = None) -> None: + self.calls: list[dict[str, Any]] = [] + self.responses = responses or [] + + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + self.calls.append({"method": method, "url": url, **kwargs}) + if self.responses: + next_response = self.responses.pop(0) + if isinstance(next_response, Exception): + raise next_response + return next_response + return _response({}) + + def test_find_existing_components_matches_exact_names_case_insensitively() -> None: client = TangleApiClient("https://api.test") client.list_published_component_infos = MagicMock( @@ -53,3 +87,395 @@ def fail_from_dict(*args, **kwargs): assert client.get_run_pipeline_spec("run-1") is task_spec client.executions_details.assert_called_once_with("root-exec-1") + + +def _tracked_stream_response(raw: Any, status_code: int = 200) -> requests.Response: + """A streaming-style response reading from ``raw`` that records ``close()`` in ``_closed``.""" + + r = requests.Response() + r.status_code = status_code + r.raw = raw + r.headers["Content-Type"] = "text/event-stream" + r.request = requests.Request("GET", "https://api.test").prepare() + r._closed = False + original_close = r.close + + def tracked_close() -> None: + r._closed = True + original_close() + + r.close = tracked_close # type: ignore[method-assign] + return r + + +def _stream_response(lines: list[bytes] | None = None, status_code: int = 200) -> requests.Response: + body = b"\n".join(lines) if lines else b"" + return _tracked_stream_response(io.BytesIO(body), status_code) + + +def test_stream_execution_container_log_yields_lines_and_closes() -> None: + stream = _stream_response([b"line-1", b"line-2", b"line-3"]) + session = _FakeSession([stream]) + client = TangleApiClient("https://api.test", session=session) + + lines = list(client.iter_execution_container_log_lines("exec-1")) + + assert lines == ["line-1", "line-2", "line-3"] + assert stream._closed is True + assert session.calls[0]["url"] == "https://api.test/api/executions/exec-1/stream_container_log" + assert session.calls[0]["stream"] is True + # The follow stream keeps the connect timeout but has no per-read timeout, + # so a healthy stream that is idle (container quiet) is never killed. + assert session.calls[0]["timeout"] == (client.timeout, None) + + +def test_stream_execution_container_log_closes_on_early_break() -> None: + stream = _stream_response([b"a", b"b", b"c"]) + session = _FakeSession([stream]) + client = TangleApiClient("https://api.test", session=session) + + gen = client.iter_execution_container_log_lines("exec-1") + assert next(iter(gen)) == "a" + gen.close() # type: ignore[union-attr] + + assert stream._closed is True + + +def test_stream_open_retries_transient_status_then_succeeds(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + bad = _stream_response(status_code=503) + ok = _stream_response([b"recovered"]) + session = _FakeSession([bad, ok]) + logger = CaptureLogger() + client = TangleApiClient("https://api.test", session=session, logger=logger) + + lines = list(client.iter_execution_container_log_lines("exec-1")) + + assert lines == ["recovered"] + assert bad._closed is True + assert sleeps == [1.0] + assert len(session.calls) == 2 + # Every stream-open retry sleep is announced through the client logger. + assert "transient HTTP 503 opening log stream; retrying in 1.0s (attempt 2/7)" in ( + logger.get_logs() or "" + ) + + +def test_stream_open_retries_transport_error_then_succeeds(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + ok = _stream_response([b"after-blip"]) + calls = {"n": 0} + + class FlakySession(_FakeSession): + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + calls["n"] += 1 + if calls["n"] == 1: + raise requests.ConnectionError("transient transport blip") + return ok + + client = TangleApiClient("https://api.test", session=FlakySession()) + + lines = list(client.iter_execution_container_log_lines("exec-1")) + + assert lines == ["after-blip"] + assert calls["n"] == 2 + assert sleeps == [1.0] + + +def test_stream_open_backoff_doubles_and_is_capped(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + attempts = TangleApiClient._MAX_STREAM_OPEN_ATTEMPTS + session = _FakeSession([_stream_response(status_code=503) for _ in range(attempts)]) + logger = CaptureLogger() + client = TangleApiClient("https://api.test", session=session, logger=logger) + + with pytest.raises(requests.HTTPError): + client.stream_execution_container_log("exec-1") + + assert sleeps == [1.0, 2.0, 4.0, 8.0, 16.0, 30.0] + # The last retry announces the final attempt; the exhausted 7th attempt + # raises without announcing an 8th. + logs = logger.get_logs() or "" + assert "(attempt 7/7)" in logs + assert "8/7" not in logs + + +def test_stream_open_raises_non_retryable_status_immediately(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + bad = _stream_response(status_code=404) + session = _FakeSession([bad]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.HTTPError): + client.stream_execution_container_log("exec-1") + + # The streamed response must be closed before the non-retryable error + # propagates so the open connection is not leaked. + assert bad._closed is True + assert sleeps == [] + assert len(session.calls) == 1 + + +def test_stream_open_exhausts_retries_and_raises_last_status(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _seconds: None) + attempts = TangleApiClient._MAX_STREAM_OPEN_ATTEMPTS + session = _FakeSession([_stream_response(status_code=502) for _ in range(attempts)]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.HTTPError) as exc_info: + client.stream_execution_container_log("exec-1") + + assert exc_info.value.response.status_code == 502 + assert len(session.calls) == attempts + + +def test_stream_open_exhausts_retries_and_raises_last_transport_error(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _seconds: None) + + class AlwaysFailingSession(_FakeSession): + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + raise requests.ConnectionError("permanent transport failure") + + client = TangleApiClient("https://api.test", session=AlwaysFailingSession()) + + with pytest.raises(requests.ConnectionError, match="permanent transport failure"): + client.stream_execution_container_log("exec-1") + + +def test_stream_open_cross_origin_redirect_is_not_retried(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + redirect = _stream_response(status_code=307) + redirect.url = "https://api.test/api/executions/exec-1/stream_container_log" + redirect.headers["Location"] = "https://attacker.example/leak" + session = _FakeSession([redirect]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.HTTPError, match="cross-origin redirect") as exc_info: + client.stream_execution_container_log("exec-1") + + # Same-origin redirect protection must propagate immediately, not be retried. + assert sleeps == [] + assert len(session.calls) == 1 + # The rejected streamed response is attached to the guard error and no + # iterator ever receives it, so it must be closed before the error + # propagates to avoid leaking the open connection. + assert exc_info.value.response is redirect + assert redirect._closed is True + + +def test_stream_open_too_many_redirects_is_not_retried(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + + def same_origin_redirect() -> requests.Response: + r = _stream_response(status_code=307) + r.url = "https://api.test/api/executions/exec-1/stream_container_log" + r.headers["Location"] = "/api/executions/exec-1/stream_container_log" + return r + + redirect_calls = TangleApiClient._MAX_REDIRECTS + 1 + responses = [same_origin_redirect() for _ in range(redirect_calls)] + session = _FakeSession(list(responses)) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.TooManyRedirects) as exc_info: + client.stream_execution_container_log("exec-1") + + # One stream-open attempt that exhausts redirects; no retry of the open. + assert sleeps == [] + assert len(session.calls) == redirect_calls + # Every streamed redirect response must be closed; the final one is + # attached to the guard error and must not leak. + assert all(r._closed is True for r in responses) + assert exc_info.value.response is responses[-1] + + +def test_stream_open_verbose_does_not_read_streamed_body(monkeypatch) -> None: + monkeypatch.setenv("TANGLE_VERBOSE", "1") + stream = _stream_response([b"line-1", b"line-2"]) + text_reads: list[int] = [] + original_text = type(stream).text + + def tracked_text(self: requests.Response) -> str: + text_reads.append(1) + return original_text.fget(self) # type: ignore[attr-defined] + + monkeypatch.setattr(type(stream), "text", property(tracked_text)) + logger = CaptureLogger() + session = _FakeSession([stream]) + client = TangleApiClient("https://api.test", session=session, logger=logger) + + response = client.stream_execution_container_log("exec-1") + + # Verbose logging must not drain the streamed body before the caller can + # iterate it; the log stream stays readable. + assert text_reads == [] + assert response._closed is False + assert list(response.iter_lines()) == [b"line-1", b"line-2"] + logs = logger.get_logs() or "" + assert "" in logs + + +def test_rate_limit_retry_closes_streamed_response_before_sleep(monkeypatch) -> None: + closed_at_sleep: list[bool] = [] + rate_limited = _stream_response(status_code=429) + rate_limited.headers["Retry-After"] = "0" + ok = _stream_response([b"recovered"]) + + def tracking_sleep(_seconds: float) -> None: + # Record whether the 429 stream is already closed when the rate-limit + # sleep runs; it must not be held open during the sleep. + closed_at_sleep.append(rate_limited._closed) + + monkeypatch.setattr("tangle_cli.client.time.sleep", tracking_sleep) + closed_at_retry: list[bool] = [] + + class TrackingSession(_FakeSession): + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + # Record whether the prior 429 stream was already closed by the + # time the successful retry is issued. + if self.calls: + closed_at_retry.append(rate_limited._closed) + return super().request(method, url, **kwargs) + + session = TrackingSession([rate_limited, ok]) + client = TangleApiClient("https://api.test", session=session) + + response = client.stream_execution_container_log("exec-1") + + assert response is ok + assert len(session.calls) == 2 + # The intermediate 429 streamed response must be closed before sleeping and + # before the retry is issued. + assert closed_at_sleep == [True] + assert closed_at_retry == [True] + assert rate_limited._closed is True + assert list(response.iter_lines()) == [b"recovered"] + + +def test_auth_refresh_closes_streamed_response_before_retry() -> None: + unauthorized = _stream_response(status_code=401) + ok = _stream_response([b"authorized"]) + closed_at_refresh: list[bool] = [] + closed_at_retry: list[bool] = [] + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + # On the auth-refresh triggered by the 401, the streamed 401 + # response must already be closed (not held open during refresh). + if self.refreshes == 2: + closed_at_refresh.append(unauthorized._closed) + self.headers["Authorization"] = f"Bearer refreshed-{self.refreshes}" + + class TrackingSession(_FakeSession): + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + # Record whether the prior 401 stream was already closed by the + # time the successful retry is issued. + if self.calls: + closed_at_retry.append(unauthorized._closed) + return super().request(method, url, **kwargs) + + session = TrackingSession([unauthorized, ok]) + client = RefreshingClient("https://api.test", session=session) + + response = client._make_request("GET", "/api/users/me", stream=True) + + assert response is ok + assert client.refreshes == 2 + assert len(session.calls) == 2 + # The intermediate 401 streamed response must be closed before the auth + # refresh and before the retry is issued. + assert closed_at_refresh == [True] + assert closed_at_retry == [True] + assert unauthorized._closed is True + # The successful retry stream remains open and readable for the caller. + assert response._closed is False + assert list(response.iter_lines()) == [b"authorized"] + + +def test_stream_open_synthetic_http_error_from_make_request_is_not_retried(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + client = TangleApiClient("https://api.test", session=_FakeSession()) + calls = {"n": 0} + + def fake_make_request(*args: Any, **kwargs: Any) -> requests.Response: + calls["n"] += 1 + raise requests.HTTPError("redirect guard tripped") + + monkeypatch.setattr(client, "_make_request", fake_make_request) + + with pytest.raises(requests.HTTPError, match="redirect guard tripped"): + client.stream_execution_container_log("exec-1") + + assert sleeps == [] + assert calls["n"] == 1 + + +class _ScriptedRaw: + """A raw stream whose ``read`` replays scripted byte chunks/exceptions. + + Each ``read`` returns the next queued ``bytes`` chunk verbatim (ignoring the + requested size, so a multi-byte char can be split across reads) or raises a + queued exception, modelling a mid-stream transport failure. + """ + + def __init__(self, chunks: list[bytes | Exception]) -> None: + self._chunks = list(chunks) + + def read(self, _size: int = -1) -> bytes: + if not self._chunks: + return b"" + item = self._chunks.pop(0) + if isinstance(item, Exception): + raise item + return item + + def close(self) -> None: + self._chunks.clear() + + +def _scripted_stream_response(chunks: list[bytes | Exception]) -> requests.Response: + return _tracked_stream_response(_ScriptedRaw(chunks)) + + +def test_stream_decodes_multibyte_char_split_across_chunks() -> None: + # "café" and "日本語" each contain multi-byte UTF-8 sequences; feeding the + # stream one byte at a time splits those sequences across chunk reads. + # Decoding whole lines as UTF-8 must reassemble them rather than yield + # replacement characters or mojibake. + payload = "café\n日本語\n".encode("utf-8") + stream = _scripted_stream_response([payload[i : i + 1] for i in range(len(payload))]) + client = TangleApiClient("https://api.test", session=_FakeSession([stream])) + + lines = list(client.iter_execution_container_log_lines("exec-1")) + + assert lines == ["café", "日本語"] + assert stream._closed is True + + +def test_stream_read_error_mid_iteration_propagates_and_closes() -> None: + # Once the stream is open the retry budget is spent; a transport failure + # during iteration must propagate (not be retried or swallowed) and the + # streamed response must still be closed by the iterator's finally block. + stream = _scripted_stream_response( + [b"line-1\n", requests.exceptions.ChunkedEncodingError("connection broken mid-stream")] + ) + client = TangleApiClient("https://api.test", session=_FakeSession([stream])) + + gen = iter(client.iter_execution_container_log_lines("exec-1")) + assert next(gen) == "line-1" + with pytest.raises(requests.exceptions.ChunkedEncodingError, match="connection broken mid-stream"): + next(gen) + + assert stream._closed is True diff --git a/tests/test_pipeline_runs_cli.py b/tests/test_pipeline_runs_cli.py index b74b0ad..6d642df 100644 --- a/tests/test_pipeline_runs_cli.py +++ b/tests/test_pipeline_runs_cli.py @@ -1,13 +1,17 @@ from __future__ import annotations import copy +import io import json +import os +import sys from contextlib import nullcontext from pathlib import Path from types import SimpleNamespace from typing import Any import pytest +import requests import yaml from tangle_cli import cli, pipeline_run_manager, pipeline_runs_cli @@ -114,6 +118,10 @@ def executions_graph_execution_state(self, id: str) -> dict[str, Any]: def executions_container_log(self, id: str) -> dict[str, Any]: return {"log_text": f"logs for {id}\n"} + def iter_execution_container_log_lines(self, id: str): + yield f"stream {id} line 1" + yield f"stream {id} line 2" + def pipeline_runs_list(self, **kwargs: Any) -> dict[str, Any]: self.list_calls.append(kwargs) return {"pipeline_runs": [{"id": "run-1"}], "next_page_token": None} @@ -2790,3 +2798,197 @@ def cleanup_prepared_pipeline(self, preparation, *, error=None): # type: ignore assert cleaned == [(temp_effective_path, "Pipeline validation failed:\n - boom")] assert not temp_effective_path.exists() + + +def test_pipeline_runs_logs_stream_prints_lines(monkeypatch, capsys) -> None: + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app(app, ["sdk", "pipeline-runs", "logs", "exec-7", "--stream"]) + + assert capsys.readouterr().out == "stream exec-7 line 1\nstream exec-7 line 2\n" + + +def test_pipeline_runs_logs_snapshot_is_default_when_stream_absent(monkeypatch, capsys) -> None: + class TrackingClient(FakeClient): + def __init__(self) -> None: + super().__init__() + self.streamed_ids: list[str] = [] + + def iter_execution_container_log_lines(self, id: str): + self.streamed_ids.append(id) + yield from () + + fake_client = TrackingClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app(app, ["sdk", "pipeline-runs", "logs", "exec-1"]) + + assert capsys.readouterr().out == "logs for exec-1\n" + assert fake_client.streamed_ids == [] + + +def test_stream_logs_open_transport_error_is_clean() -> None: + class RefusingClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + raise requests.ConnectionError("connection refused") + + manager = pipeline_run_manager.PipelineRunManager(client=RefusingClient()) + + with pytest.raises( + PipelineRunError, match="Failed to open log stream for execution exec-1" + ): + manager.stream_logs("exec-1") + + +def test_pipeline_runs_logs_stream_open_failure_exits_cleanly(monkeypatch) -> None: + class RefusingClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + raise requests.ConnectionError("connection refused") + + fake_client = RefusingClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + with pytest.raises(SystemExit) as exc_info: + app(["sdk", "pipeline-runs", "logs", "exec-1", "--stream"]) + + assert "Failed to open log stream for execution exec-1" in str(exc_info.value) + assert "connection refused" in str(exc_info.value) + + +def test_pipeline_runs_logs_stream_broken_pipe_exits_cleanly(monkeypatch) -> None: + # Simulates `... logs --stream | head`: the downstream reader closes the + # pipe, so the first stdout write raises BrokenPipeError. The command must + # stop following and exit cleanly instead of tracebacking. + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + devnull_fd = os.open(os.devnull, os.O_WRONLY) + write_attempts = {"count": 0} + + class ClosedPipeStdout(io.TextIOBase): + def write(self, _text: str) -> int: + write_attempts["count"] += 1 + raise BrokenPipeError + + def fileno(self) -> int: + return devnull_fd + + monkeypatch.setattr(sys, "stdout", ClosedPipeStdout()) + app = cli.build_app() + + try: + run_app(app, ["sdk", "pipeline-runs", "logs", "exec-7", "--stream"]) + finally: + os.close(devnull_fd) + + assert write_attempts["count"] == 1 + + +def test_pipeline_runs_logs_stream_ctrl_c_exits_cleanly(monkeypatch, capsys) -> None: + # An interactive Ctrl-C is the normal way to stop a live follow; it must + # exit with the conventional interrupt code (128 + SIGINT), not a + # KeyboardInterrupt traceback. cyclopts provides this by default + # (suppress_keyboard_interrupt); this locks that contract in for --stream. + class InterruptedStreamClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + yield "line-1" + raise KeyboardInterrupt + + fake_client = InterruptedStreamClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + with pytest.raises(SystemExit) as exc_info: + app(["sdk", "pipeline-runs", "logs", "exec-1", "--stream"]) + + assert exc_info.value.code == 130 + assert capsys.readouterr().out == "line-1\n" + + +def test_pipeline_runs_logs_help_documents_stream(capsys) -> None: + app = cli.build_app() + run_app(app, ["sdk", "pipeline-runs", "logs", "--help"]) + assert "--stream" in capsys.readouterr().out + + +def test_pipeline_runs_stream_logs_uses_client_iterator() -> None: + fake_client = FakeClient() + manager = PipelineRunManager(client=fake_client) + + assert list(manager.stream_logs("exec-9")) == ["stream exec-9 line 1", "stream exec-9 line 2"] + + +def test_pipeline_runs_stream_logs_midstream_error_is_interruption() -> None: + # A transport failure after some lines have flowed must surface as a + # mid-stream interruption, not the "failed to open" wording reserved for a + # stream that never opened. + class MidStreamDropClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + yield "line-1" + raise requests.exceptions.ChunkedEncodingError("connection broken mid-stream") + + manager = PipelineRunManager(client=MidStreamDropClient()) + + gen = iter(manager.stream_logs("exec-9")) + assert next(gen) == "line-1" + with pytest.raises(PipelineRunError, match="interrupted") as exc_info: + next(gen) + assert "Failed to open" not in str(exc_info.value) + + +def test_pipeline_runs_stream_logs_zero_line_drop_is_interruption() -> None: + # A connection that opens successfully but drops before the first line is + # still a mid-stream interruption; only open failures (raised from the + # hook call itself) get the "failed to open" wording. + class ZeroLineDropClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + raise requests.exceptions.ChunkedEncodingError("Response ended prematurely") + yield # pragma: no cover + + manager = PipelineRunManager(client=ZeroLineDropClient()) + + with pytest.raises(PipelineRunError, match="interrupted") as exc_info: + list(manager.stream_logs("exec-9")) + assert "Failed to open" not in str(exc_info.value) + + +def test_pipeline_runs_logs_stream_honors_log_type_none(monkeypatch, capsys) -> None: + # --log-type controls the CLI's diagnostic logger and is orthogonal to + # --stream; the streamed log content still reaches stdout under any mode. + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app(app, ["sdk", "pipeline-runs", "logs", "exec-7", "--stream", "--log-type", "none"]) + + assert capsys.readouterr().out == "stream exec-7 line 1\nstream exec-7 line 2\n" + + +def _logs_http_error(status: int, path: str) -> requests.HTTPError: + response = requests.Response() + response.status_code = status + response.request = requests.Request("GET", f"https://api.test{path}").prepare() + return requests.HTTPError(f"{status} Client Error", response=response) + + +def test_pipeline_runs_logs_stream_missing_endpoint_is_clean(monkeypatch) -> None: + class NoStreamClient(FakeClient): + def iter_execution_container_log_lines(self, id: str): + # Like the real client, open failures raise from the call itself + # rather than on first iteration. + raise _logs_http_error(404, f"/api/executions/{id}/stream_container_log") + + fake_client = NoStreamClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + # A clean SystemExit (not a raw requests.HTTPError) means no traceback. + with pytest.raises(SystemExit) as exc_info: + app(["sdk", "pipeline-runs", "logs", "exec-1", "--stream"]) + + message = str(exc_info.value) + assert "404" in message + assert "/api/executions/exec-1/stream_container_log" in message From 4c0acfb3c883ef28d4084442379c0e4c9d620737 Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Tue, 14 Jul 2026 12:17:59 +0000 Subject: [PATCH 3/7] Add --arg-secret to pipeline-runs submit for secret-backed inputs Support binding a pipeline input to a Tangle secret via `--arg-secret INPUT=SECRET_NAME` (repeatable) and a matching `arg_secrets` config mapping. Each reference encodes the OSS dynamic-data payload {"dynamicData":{"secret":{"name":...}}} under the input in root_task arguments. Validation runs before any file read or network call: inputs and secret names must be non-empty after trimming, duplicates are rejected, and an input supplied both as a plain value and a secret reference is rejected rather than silently overwritten. CLI `--arg-secret` overrides config `arg_secrets`. Existing --arg, --args-json, config args, hydration, dry-run, and submit-recovery behavior are unchanged. --- .../src/tangle_cli/pipeline_run_manager.py | 77 ++++ .../src/tangle_cli/pipeline_runs_cli.py | 21 +- tests/test_pipeline_runs_cli.py | 336 ++++++++++++++++++ 3 files changed, 433 insertions(+), 1 deletion(-) diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py index 24742fa..aa7d6ad 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py @@ -2100,3 +2100,80 @@ def parse_json_or_key_values( result.update(loaded) result.update(parse_key_value_entries(entries)) return result + + +def secret_argument_value(secret_name: str) -> dict[str, Any]: + """Return the OSS dynamic-data payload for a Tangle secret reference.""" + + return {"dynamicData": {"secret": {"name": secret_name}}} + + +def parse_arg_secret_entries(entries: list[str] | None) -> dict[str, str]: + """Parse ``INPUT=SECRET_NAME`` secret references into a mapping. + + Trimmed input and secret names must be non-empty, and an input may not be + repeated. Raising here keeps validation before any file read or network + call in the submit path. + """ + + parsed: dict[str, str] = {} + for entry in entries or []: + if "=" not in entry: + raise PipelineRunError(f"Expected INPUT=SECRET_NAME for --arg-secret, got {entry!r}") + raw_input, raw_secret = entry.split("=", 1) + input_name = raw_input.strip() + secret_name = raw_secret.strip() + if not input_name or not secret_name: + raise PipelineRunError( + f"--arg-secret requires a non-empty input and secret name, got {entry!r}" + ) + if input_name in parsed: + raise PipelineRunError(f"Duplicate --arg-secret for input {input_name!r}") + parsed[input_name] = secret_name + return parsed + + +def normalize_arg_secret_config(value: Any) -> dict[str, str]: + """Normalize a config ``arg_secrets`` mapping of input -> secret name.""" + + if value is None: + return {} + if not isinstance(value, Mapping): + raise PipelineRunError("arg_secrets config must be a mapping of INPUT to SECRET_NAME") + normalized: dict[str, str] = {} + for raw_input, raw_secret in value.items(): + input_name = str(raw_input).strip() + if not isinstance(raw_secret, str): + raise PipelineRunError( + f"arg_secrets[{raw_input!r}] must be a secret name string, " + f"got {type(raw_secret).__name__}" + ) + secret_name = raw_secret.strip() + if not input_name or not secret_name: + raise PipelineRunError("arg_secrets entries require a non-empty input and secret name") + normalized[input_name] = secret_name + return normalized + + +def merge_secret_run_args( + run_args: dict[str, Any], + secret_names: Mapping[str, str], +) -> dict[str, Any]: + """Merge secret references into run args, rejecting input conflicts. + + An input may be supplied as a plain value (``--arg`` / ``--args-json`` / + config ``args``) or as a secret reference (``--arg-secret`` / config + ``arg_secrets``), never both. Overlap is rejected rather than silently + overwriting one form with the other. + """ + + conflicts = sorted(set(run_args) & set(secret_names)) + if conflicts: + raise PipelineRunError( + "Input(s) given as both a value and a secret reference: " + f"{', '.join(conflicts)}. Use only one of --arg/--args-json or --arg-secret per input." + ) + merged = dict(run_args) + for input_name, secret_name in secret_names.items(): + merged[input_name] = secret_argument_value(secret_name) + return merged diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py index 2da0eb5..2f072ae 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py @@ -32,6 +32,9 @@ PipelineRunError, PipelineRunHooks, PipelineRunManager, + merge_secret_run_args, + normalize_arg_secret_config, + parse_arg_secret_entries, parse_json_or_key_values, parse_key_value_entries, ) @@ -134,6 +137,17 @@ def pipeline_runs_submit( Parameter(help="Pipeline argument as KEY=VALUE. Repeat for multiple.", negative_iterable=()), ] = None, args_json: Annotated[str | None, Parameter(help="Pipeline arguments as a JSON object.")] = None, + arg_secret: Annotated[ + list[str] | None, + Parameter( + name="--arg-secret", + help=( + "Pipeline argument bound to a Tangle secret as INPUT=SECRET_NAME. " + "Repeat for multiple." + ), + negative_iterable=(), + ), + ] = None, annotation: Annotated[ list[str] | None, Parameter(help="Run annotation as KEY=VALUE. Repeat for multiple.", negative_iterable=()), @@ -185,6 +199,8 @@ def pipeline_runs_submit( "arg": (arg, None), "args_json": (args_json, None), "args_config": ("args", None, None, True), + "arg_secret": (arg_secret, None), + "arg_secrets_config": ("arg_secrets", None, None, True), "annotation": (annotation, None), "hydrate": (hydrate, True), "dry_run": (dry_run, None), @@ -197,8 +213,11 @@ def pipeline_runs_submit( } def action(manager: PipelineRunManager, args: ArgsContainer) -> dict[str, Any]: + run_args = parse_json_or_key_values(args.args_json or args.args_config, args.arg) + secret_names = normalize_arg_secret_config(args.arg_secrets_config) + secret_names.update(parse_arg_secret_entries(args.arg_secret)) kwargs = { - "run_args": parse_json_or_key_values(args.args_json or args.args_config, args.arg), + "run_args": merge_secret_run_args(run_args, secret_names), "annotations": parse_key_value_entries(args.annotation), "hydrate": bool(args.hydrate), "run_as": args.run_as, diff --git a/tests/test_pipeline_runs_cli.py b/tests/test_pipeline_runs_cli.py index b74b0ad..5a56a67 100644 --- a/tests/test_pipeline_runs_cli.py +++ b/tests/test_pipeline_runs_cli.py @@ -19,6 +19,10 @@ PipelineRunManager, PipelineWaitOutcome, PipelineWaitPoll, + merge_secret_run_args, + normalize_arg_secret_config, + parse_arg_secret_entries, + secret_argument_value, ) from tangle_cli.pipeline_runner import PipelineRunner, PipelineRunnerHooks @@ -2790,3 +2794,335 @@ def cleanup_prepared_pipeline(self, preparation, *, error=None): # type: ignore assert cleaned == [(temp_effective_path, "Pipeline validation failed:\n - boom")] assert not temp_effective_path.exists() + + +def _write_secret_pipeline(path: Path) -> Path: + path.write_text( + yaml.safe_dump( + { + "name": "Secret Pipeline", + "inputs": [ + {"name": "query", "type": "String", "default": "default"}, + {"name": "api_key", "type": "String"}, + {"name": "db_password", "type": "String", "optional": True}, + {"name": "required", "type": "String"}, + ], + "implementation": {"graph": {"tasks": {}}}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + return path + + +def test_secret_argument_value_matches_oss_dynamic_data_contract() -> None: + assert secret_argument_value("OPENAI_KEY") == { + "dynamicData": {"secret": {"name": "OPENAI_KEY"}} + } + + +def test_parse_arg_secret_entries_trims_and_maps_inputs() -> None: + assert parse_arg_secret_entries([" api_key = OPENAI_KEY ", "db_password=PG"]) == { + "api_key": "OPENAI_KEY", + "db_password": "PG", + } + + +def test_parse_arg_secret_entries_defaults_to_empty() -> None: + assert parse_arg_secret_entries(None) == {} + + +@pytest.mark.parametrize("entry", ["api_key", "", " ", "=SECRET", "api_key=", "api_key= "]) +def test_parse_arg_secret_entries_rejects_malformed(entry: str) -> None: + with pytest.raises(PipelineRunError): + parse_arg_secret_entries([entry]) + + +def test_parse_arg_secret_entries_rejects_duplicate_input() -> None: + with pytest.raises(PipelineRunError, match="Duplicate --arg-secret for input 'api_key'"): + parse_arg_secret_entries(["api_key=A", "api_key=B"]) + + +def test_normalize_arg_secret_config_accepts_mapping() -> None: + assert normalize_arg_secret_config({"api_key": "OPENAI_KEY", " db ": " PG "}) == { + "api_key": "OPENAI_KEY", + "db": "PG", + } + + +def test_normalize_arg_secret_config_defaults_to_empty() -> None: + assert normalize_arg_secret_config(None) == {} + + +def test_normalize_arg_secret_config_rejects_non_mapping() -> None: + with pytest.raises(PipelineRunError, match="arg_secrets config must be a mapping"): + normalize_arg_secret_config(["api_key=OPENAI_KEY"]) + + +def test_normalize_arg_secret_config_rejects_non_string_secret() -> None: + with pytest.raises(PipelineRunError, match="must be a secret name string"): + normalize_arg_secret_config({"api_key": 123}) + + +def test_merge_secret_run_args_injects_dynamic_data() -> None: + merged = merge_secret_run_args({"required": "value"}, {"api_key": "OPENAI_KEY"}) + assert merged == { + "required": "value", + "api_key": {"dynamicData": {"secret": {"name": "OPENAI_KEY"}}}, + } + + +def test_merge_secret_run_args_rejects_value_and_secret_conflict() -> None: + with pytest.raises(PipelineRunError, match="both a value and a secret reference: api_key"): + merge_secret_run_args({"api_key": "plain"}, {"api_key": "OPENAI_KEY"}) + + +def test_pipeline_runs_submit_arg_secret_encodes_dynamic_data(monkeypatch, tmp_path: Path, capsys): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app( + app, + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--arg", + "required=value", + "--arg-secret", + "api_key=OPENAI_KEY", + ], + ) + + capsys.readouterr() + arguments = fake_client.created[0]["root_task"]["arguments"] + assert arguments["required"] == "value" + assert arguments["api_key"] == {"dynamicData": {"secret": {"name": "OPENAI_KEY"}}} + + +def test_pipeline_runs_submit_multiple_arg_secrets(monkeypatch, tmp_path: Path, capsys): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app( + app, + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--arg", + "required=value", + "--arg-secret", + "api_key=OPENAI_KEY", + "--arg-secret", + "db_password=PG_PASSWORD", + ], + ) + + capsys.readouterr() + arguments = fake_client.created[0]["root_task"]["arguments"] + assert arguments["api_key"] == {"dynamicData": {"secret": {"name": "OPENAI_KEY"}}} + assert arguments["db_password"] == {"dynamicData": {"secret": {"name": "PG_PASSWORD"}}} + + +def test_pipeline_runs_submit_arg_secret_dry_run_no_network(monkeypatch, tmp_path: Path, capsys): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app( + app, + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--dry-run", + "--arg", + "required=value", + "--arg-secret", + "api_key=OPENAI_KEY", + ], + ) + + payload = json.loads(capsys.readouterr().out) + assert fake_client.created == [] + arguments = payload["root_task"]["arguments"] + assert arguments["api_key"] == {"dynamicData": {"secret": {"name": "OPENAI_KEY"}}} + + +def test_pipeline_runs_submit_arg_secret_conflict_rejected_without_submit( + monkeypatch, tmp_path: Path +): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + with pytest.raises(SystemExit) as excinfo: + app( + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--arg", + "api_key=plain", + "--arg-secret", + "api_key=OPENAI_KEY", + ] + ) + + assert "both a value and a secret reference" in str(excinfo.value) + assert fake_client.created == [] + + +def test_pipeline_runs_submit_arg_secret_conflicts_with_args_json(monkeypatch, tmp_path: Path): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + with pytest.raises(SystemExit) as excinfo: + app( + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--args-json", + json.dumps({"api_key": "plain"}), + "--arg-secret", + "api_key=OPENAI_KEY", + ] + ) + + assert "both a value and a secret reference" in str(excinfo.value) + assert fake_client.created == [] + + +def test_pipeline_runs_submit_arg_secret_malformed_skips_file_and_network( + monkeypatch, tmp_path: Path +): + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + missing_path = tmp_path / "does-not-exist.yaml" + + with pytest.raises(SystemExit) as excinfo: + app( + [ + "sdk", + "pipeline-runs", + "submit", + str(missing_path), + "--no-hydrate", + "--arg-secret", + "api_key=", + ] + ) + + message = str(excinfo.value) + assert "non-empty input and secret name" in message + assert not missing_path.exists() + assert fake_client.created == [] + + +def test_pipeline_runs_submit_arg_secret_duplicate_rejected(monkeypatch, tmp_path: Path): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + with pytest.raises(SystemExit) as excinfo: + app( + [ + "sdk", + "pipeline-runs", + "submit", + str(pipeline_path), + "--no-hydrate", + "--arg-secret", + "api_key=A", + "--arg-secret", + "api_key=B", + ] + ) + + assert "Duplicate --arg-secret for input 'api_key'" in str(excinfo.value) + assert fake_client.created == [] + + +def test_pipeline_runs_submit_arg_secret_from_config(monkeypatch, tmp_path: Path): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + config = tmp_path / "pipeline.config.yaml" + config.write_text( + yaml.safe_dump( + { + "pipeline_path": str(pipeline_path), + "hydrate": False, + "args": {"required": "value"}, + "arg_secrets": {"api_key": "OPENAI_KEY"}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app(app, ["sdk", "pipeline-runs", "submit", "--config", str(config)]) + + arguments = fake_client.created[0]["root_task"]["arguments"] + assert arguments["required"] == "value" + assert arguments["api_key"] == {"dynamicData": {"secret": {"name": "OPENAI_KEY"}}} + + +def test_pipeline_runs_submit_cli_arg_secret_overrides_config(monkeypatch, tmp_path: Path): + pipeline_path = _write_secret_pipeline(tmp_path / "pipeline.yaml") + config = tmp_path / "pipeline.config.yaml" + config.write_text( + yaml.safe_dump( + { + "pipeline_path": str(pipeline_path), + "hydrate": False, + "args": {"required": "value"}, + "arg_secrets": {"api_key": "FROM_CONFIG"}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + fake_client = FakeClient() + monkeypatch.setattr(pipeline_runs_cli, "LazyTangleApiClient", lambda **kwargs: fake_client) + app = cli.build_app() + + run_app( + app, + [ + "sdk", + "pipeline-runs", + "submit", + "--config", + str(config), + "--arg-secret", + "api_key=FROM_CLI", + ], + ) + + arguments = fake_client.created[0]["root_task"]["arguments"] + assert arguments["api_key"] == {"dynamicData": {"secret": {"name": "FROM_CLI"}}} From ff8f85925b41a61a4c6e8ac7247df5d136d3a2a0 Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Mon, 13 Jul 2026 13:43:10 +0000 Subject: [PATCH 4/7] feat(pipeline-run): add programmatic submit_and_wait submit_and_wait_prepared_body submits a prepared run body and waits for terminal state in one call. Wait parameters are validated before creating a run, max_wait=None remains unbounded, and submit_recovery_attempts passes through to failed-submit recovery so a run registered before a transport failure can be adopted instead of duplicated. The caller body is never mutated. Also fixes a pre-existing KeyError in submit_prepared_body on locator-style bodies without an inline componentRef.spec, and tightens wait_for_completion to reject non-finite max_wait/poll_interval instead of treating inf as a never-firing deadline. --- .../src/tangle_cli/pipeline_run_manager.py | 182 ++++++- tests/test_submit_and_wait_prepared_body.py | 461 ++++++++++++++++++ 2 files changed, 635 insertions(+), 8 deletions(-) create mode 100644 tests/test_submit_and_wait_prepared_body.py diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py index 24742fa..e992864 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_run_manager.py @@ -12,6 +12,7 @@ import copy import inspect import json +import math import re import time import uuid @@ -739,6 +740,25 @@ def normalize_submit_body_in_place(body: dict[str, Any]) -> dict[str, Any]: def is_terminal_status(status: str | None) -> bool: return bool(status and status.upper() in _TERMINAL_STATUSES) + @staticmethod + def _validate_prepared_body(body: dict[str, Any]) -> dict[str, Any]: + """Fail fast on malformed prepared bodies, returning the componentRef. + + Locator-style bodies (a componentRef with ``name``/``digest`` and no + inline ``spec``) are valid; only the ``root_task``/``componentRef`` + mappings themselves are required. + """ + + root_task = body.get("root_task") + if not isinstance(root_task, dict): + raise PipelineRunError("Prepared submit body must contain a 'root_task' mapping") + component_ref = root_task.get("componentRef") + if not isinstance(component_ref, dict): + raise PipelineRunError( + "Prepared submit body 'root_task' must contain a 'componentRef' mapping" + ) + return component_ref + @staticmethod def status_counts_from_run(run: Mapping[str, Any]) -> dict[str, int]: stats = run.get("execution_status_stats") @@ -1122,7 +1142,7 @@ def submit_prepared_body( notify_submit_error: bool = True, ) -> dict[str, Any]: self.normalize_submit_body_in_place(body) - pipeline_spec = body["root_task"]["componentRef"]["spec"] + pipeline_spec = self._validate_prepared_body(body).get("spec") submit_context = context or PipelineRunContext( pipeline_path=pipeline_path, start_time=time.time(), @@ -1452,6 +1472,36 @@ def _poll_run_status( execution_state_timings=execution_state_timings, ) + @staticmethod + def _validate_wait_params( + *, + max_wait: float | None, + poll_interval: float, + timeout_clock: str, + allow_zero_poll_interval: bool, + ) -> None: + """Validate wait/poll parameters, raising before any run is submitted. + + The CLI wait/run commands and the programmatic API share this path, so + error messages name both the parameter and its CLI flag. + """ + + # NaN/inf pass the sign checks below (NaN compares False, inf is + # "positive") and would become a never-firing deadline or a raw + # ValueError from time.sleep; reject them up front. An unbounded wait is + # spelled max_wait=None, not an explicit inf. + if max_wait is not None: + if not math.isfinite(max_wait): + raise PipelineRunError("max_wait (--max-wait) must be a finite number") + if max_wait < 0: + raise PipelineRunError("max_wait (--max-wait) must be non-negative") + if not math.isfinite(poll_interval): + raise PipelineRunError("poll_interval (--poll-interval) must be a finite number") + if poll_interval < 0 or (poll_interval == 0 and not allow_zero_poll_interval): + raise PipelineRunError("poll_interval (--poll-interval) must be positive") + if timeout_clock not in {"monotonic", "wall"}: + raise PipelineRunError("timeout_clock must be 'monotonic' or 'wall'") + def wait_for_completion( self, run_id: str, @@ -1467,12 +1517,12 @@ def wait_for_completion( wait_context = context or PipelineRunContext(run_id=run_id, start_time=time.time()) if exit_on_first_failure: wait_context.metadata["exit_on_first_failure"] = True - if max_wait is not None and max_wait < 0: - raise PipelineRunError("--max-wait must be non-negative") - if poll_interval < 0 or (poll_interval == 0 and not allow_zero_poll_interval): - raise PipelineRunError("--poll-interval must be positive") - if timeout_clock not in {"monotonic", "wall"}: - raise PipelineRunError("timeout_clock must be 'monotonic' or 'wall'") + self._validate_wait_params( + max_wait=max_wait, + poll_interval=poll_interval, + timeout_clock=timeout_clock, + allow_zero_poll_interval=allow_zero_poll_interval, + ) enforce_max_wait = max_wait is not None and self.hooks.should_enforce_max_wait(wait_context) poll_started_at = time.monotonic() deadline_now: Callable[[], float] = time.time if timeout_clock == "wall" else time.monotonic @@ -1751,6 +1801,15 @@ def _run_body_factory( if max_attempts < 1: raise PipelineRunError("max_attempts must be at least 1") + if wait: + # Validate wait/poll params up front so an invalid request never + # submits a run it can't wait on. + self._validate_wait_params( + max_wait=max_wait, + poll_interval=poll_interval, + timeout_clock=timeout_clock, + allow_zero_poll_interval=allow_zero_poll_interval, + ) last_error: Exception | None = None previous_context: PipelineRunContext | None = None attempts: list[PipelineRunContext] = [] @@ -1797,7 +1856,9 @@ def _run_body_factory( context.metadata["submission_id"] = submission_id if metadata_factory is not None: context.metadata.update(metadata_factory(attempt, previous_context, last_error)) - pipeline_spec = body.get("root_task", {}).get("componentRef", {}).get("spec") + # Validate before the tolerant spec extraction so a malformed body + # raises PipelineRunError instead of an AttributeError. + pipeline_spec = self._validate_prepared_body(body).get("spec") context.submit_body = body context.pipeline_spec = pipeline_spec if isinstance(pipeline_spec, dict) else None if context.pipeline_spec is not None: @@ -2076,6 +2137,111 @@ def body_factory( ) +def _build_default_manager(*, logger: Logger | None = None) -> PipelineRunManager: + """Construct a manager backed by the native ``TangleApiClient``. + + Imported lazily so the lightweight top-level package and local-only SDK + commands stay native-free. Credentials/base URL come from the standard + ``TangleApiClient`` environment defaults. + """ + + try: + from .client import TangleApiClient + except ModuleNotFoundError as exc: + # Catch both the top-level package and any missing submodule (e.g. a + # partially-installed ``tangle_api.generated``). + if exc.name is not None and (exc.name == "tangle_api" or exc.name.startswith("tangle_api.")): + raise PipelineRunError( + "Native generated Tangle API bindings are required to submit a prepared " + "body without an explicit client. Install tangle-cli[native], provide a " + "local tangle_api.generated package, or pass client=/manager=." + ) from exc + raise + + hooks = PipelineRunHooks(logger=logger) if logger is not None else PipelineRunHooks() + return PipelineRunManager(client=TangleApiClient(), hooks=hooks, logger=logger or hooks.logger) + + +def submit_and_wait_prepared_body( + body: dict[str, Any], + *, + manager: PipelineRunManager | None = None, + client: Any | None = None, + logger: Logger | None = None, + wait: bool = True, + max_wait: float | None = 600.0, + poll_interval: float = 10.0, + use_graph_state: bool = False, + allow_zero_poll_interval: bool = False, + timeout_clock: str = "monotonic", + exit_on_first_failure: bool = False, + metadata: dict[str, Any] | None = None, + submit_recovery_attempts: int = _DEFAULT_SUBMIT_RECOVERY_ATTEMPTS, +) -> dict[str, Any]: + """Submit an already-prepared API submit body and optionally wait for completion. + + Thin wrapper over :meth:`PipelineRunManager.run_prepared_body` for callers + that already hold a fully formed submit ``body`` (``{"root_task": {...}}``); + it reuses the existing submit/wait/poll and submit-recovery logic and never + mutates ``body``. Provide ``manager`` to reuse a configured manager, + ``client`` to wrap an existing API client, or neither to build a + :class:`tangle_cli.client.TangleApiClient` from the environment (requires + the native extra). ``logger`` applies only when this helper constructs the + manager (the ``client`` and default paths); a supplied ``manager`` keeps + its own configured logger and ``logger`` is ignored. + + Returns a dict with ``response``, ``run_id``, ``root_execution_id``, and + ``wait`` (when ``wait=True``); the result is JSON-serializable whenever the + API responses are. With ``wait=True``, a submit response carrying no run id + raises :class:`PipelineRunError`; use ``wait=False`` to inspect such + responses. + """ + + if manager is not None and client is not None: + raise PipelineRunError("Pass at most one of manager= or client=") + + if manager is None: + if client is not None: + hooks = PipelineRunHooks(logger=logger) if logger is not None else PipelineRunHooks() + manager = PipelineRunManager(client=client, hooks=hooks, logger=logger or hooks.logger) + else: + manager = _build_default_manager(logger=logger) + + raw = manager.run_prepared_body( + body, + wait=wait, + max_wait=max_wait, + poll_interval=poll_interval, + use_graph_state=use_graph_state, + allow_zero_poll_interval=allow_zero_poll_interval, + timeout_clock=timeout_clock, + exit_on_first_failure=exit_on_first_failure, + metadata=metadata, + submit_recovery_attempts=submit_recovery_attempts, + ) + + context: PipelineRunContext | None = raw.get("context") + result: dict[str, Any] = { + "response": raw.get("response"), + "run_id": context.run_id if context is not None else None, + "root_execution_id": context.root_execution_id if context is not None else None, + } + if "wait" in raw: + result["wait"] = raw["wait"] + elif wait: + # The underlying run skips waiting when the submit response carries no + # run id; surface that instead of silently returning without waiting. + raise PipelineRunError( + "Run was submitted but the submit response did not include a run id, " + "so completion cannot be awaited; use wait=False to inspect the response." + ) + return result + + +# Short public alias; ``submit_and_wait_prepared_body`` is the definition. +submit_and_wait = submit_and_wait_prepared_body + + def parse_key_value_entries(entries: list[str] | None) -> dict[str, str]: parsed: dict[str, str] = {} for entry in entries or []: diff --git a/tests/test_submit_and_wait_prepared_body.py b/tests/test_submit_and_wait_prepared_body.py new file mode 100644 index 0000000..04877e4 --- /dev/null +++ b/tests/test_submit_and_wait_prepared_body.py @@ -0,0 +1,461 @@ +from __future__ import annotations + +import builtins +import copy +import json +from typing import Any + +import pytest +from tangle_cli.logger import CaptureLogger +from tangle_cli.pipeline_run_manager import ( + PipelineRunError, + PipelineRunManager, + submit_and_wait_prepared_body, +) + +RUN_ID = "run-1" +ROOT_EXECUTION_ID = "exec-1" + +# The submit lifecycle injects a submission-id annotation into the (deep-copied) +# body before submit so post-failure recovery can find the created run. Strip it +# when comparing the submitted body to the caller's original. +_SUBMISSION_ANNOTATION_KEY = "tangle-cli/submission-id" + + +def _without_submission_annotation(body: dict[str, Any]) -> dict[str, Any]: + stripped = copy.deepcopy(body) + annotations = stripped.get("annotations") + if isinstance(annotations, dict): + annotations.pop(_SUBMISSION_ANNOTATION_KEY, None) + if not annotations: + stripped.pop("annotations", None) + return stripped + + +def _prepared_body() -> dict[str, Any]: + return { + "root_task": { + "componentRef": { + "spec": {"name": "Prepared", "implementation": {"graph": {"tasks": {}}}} + }, + "arguments": {"query": "value"}, + } + } + + +class _SubmitClient: + """Minimal API client: submit succeeds, status reports terminal SUCCEEDED.""" + + def __init__(self) -> None: + self.created: list[Any] = [] + self.get_calls: int = 0 + + def pipeline_runs_create(self, body: Any = None) -> dict[str, Any]: + self.created.append(copy.deepcopy(body)) + return {"id": RUN_ID, "root_execution_id": ROOT_EXECUTION_ID} + + def pipeline_runs_get(self, id: str, include_execution_stats: bool | None = None) -> dict[str, Any]: + self.get_calls += 1 + return { + "id": id, + "root_execution_id": ROOT_EXECUTION_ID, + "execution_summary": {"has_ended": True}, + "execution_status_stats": {"SUCCEEDED": 1}, + } + + +def _locator_body() -> dict[str, Any]: + """A fully-formed submit body that references a component by name/digest. + + This shape has no inline ``componentRef.spec``; the helper must submit it + verbatim instead of failing while trying to read a spec. + """ + return { + "root_task": { + "componentRef": {"name": "my-pipeline", "digest": "sha256:abc123"}, + "arguments": {"query": "value"}, + } + } + + +def test_submit_only_returns_run_metadata_without_wait() -> None: + client = _SubmitClient() + result = submit_and_wait_prepared_body(_prepared_body(), client=client, wait=False) + + assert result["run_id"] == RUN_ID + assert result["root_execution_id"] == ROOT_EXECUTION_ID + assert result["response"] == {"id": RUN_ID, "root_execution_id": ROOT_EXECUTION_ID} + assert "wait" not in result + assert client.get_calls == 0 + # No PipelineRunContext / attempts leak into the default output. + assert set(result) == {"response", "run_id", "root_execution_id"} + assert json.dumps(result) + + +def test_submit_and_wait_success_includes_serializable_wait_result() -> None: + client = _SubmitClient() + result = submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, poll_interval=0.01 + ) + + assert result["run_id"] == RUN_ID + assert result["wait"]["status"] == "SUCCEEDED" + assert result["wait"]["timed_out"] is False + assert client.get_calls >= 1 + assert json.dumps(result) + + +def test_timeout_metadata_preserved_and_serializable() -> None: + class _NeverTerminalClient(_SubmitClient): + def pipeline_runs_get( + self, id: str, include_execution_stats: bool | None = None + ) -> dict[str, Any]: + self.get_calls += 1 + return { + "id": id, + "root_execution_id": ROOT_EXECUTION_ID, + "execution_summary": {"has_ended": False}, + "execution_status_stats": {"RUNNING": 1}, + } + + result = submit_and_wait_prepared_body( + _prepared_body(), + client=_NeverTerminalClient(), + wait=True, + max_wait=0.0, + poll_interval=0.01, + ) + + assert result["run_id"] == RUN_ID + assert result["wait"]["timed_out"] is True + assert "status_counts" in result["wait"] + assert json.dumps(result) + + +def test_exit_on_first_failure_returns_serializable_early_exit() -> None: + class _FailingGraphClient(_SubmitClient): + # Run stays nonterminal at the top level; graph state reports a FAILED + # child alongside a still-running one, so exit_on_first_failure trips. + def pipeline_runs_get( + self, id: str, include_execution_stats: bool | None = None + ) -> dict[str, Any]: + self.get_calls += 1 + return { + "id": id, + "root_execution_id": ROOT_EXECUTION_ID, + "execution_status_stats": {"RUNNING": 1}, + } + + def executions_graph_execution_state(self, id: str) -> dict[str, Any]: + return {"status_totals": {"RUNNING": 1, "FAILED": 1}} + + result = submit_and_wait_prepared_body( + _prepared_body(), + client=_FailingGraphClient(), + wait=True, + use_graph_state=True, + exit_on_first_failure=True, + poll_interval=0.01, + ) + + assert result["run_id"] == RUN_ID + assert result["wait"]["early_exit"] is True + assert result["wait"]["timed_out"] is False + assert result["wait"]["failed_count"] == 1 + # Default output stays JSON-serializable and free of context/attempts leakage. + assert "context" not in result + assert "attempts" not in result + assert json.dumps(result) + + +def test_invalid_poll_interval_raises_before_submit() -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, poll_interval=0 + ) + # The run must never be submitted when the wait params are invalid. + assert client.created == [] + + +def test_negative_max_wait_raises_before_submit() -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, max_wait=-1.0 + ) + assert client.created == [] + + +@pytest.mark.parametrize("bad", [float("nan"), float("inf"), float("-inf")]) +def test_non_finite_max_wait_raises_before_submit(bad: float) -> None: + # NaN/inf pass sign checks (NaN compares False, inf is "positive") and + # would become a never-firing deadline; they must be rejected up front. + client = _SubmitClient() + with pytest.raises(PipelineRunError, match=r"max_wait \(--max-wait\) must be a finite number"): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, max_wait=bad + ) + assert client.created == [] + + +@pytest.mark.parametrize("bad", [float("nan"), float("inf"), float("-inf")]) +def test_non_finite_poll_interval_raises_before_submit(bad: float) -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError, match=r"poll_interval \(--poll-interval\) must be a finite number"): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, poll_interval=bad + ) + assert client.created == [] + + +def test_invalid_timeout_clock_raises_before_submit() -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError): + submit_and_wait_prepared_body( + _prepared_body(), + client=client, + wait=True, + poll_interval=0.01, + timeout_clock="bogus", + ) + assert client.created == [] + + +def test_wait_true_without_run_id_raises_instead_of_silent_no_wait() -> None: + class _NoRunIdClient(_SubmitClient): + def pipeline_runs_create(self, body: Any = None) -> dict[str, Any]: + self.created.append(copy.deepcopy(body)) + return {"root_execution_id": ROOT_EXECUTION_ID} + + client = _NoRunIdClient() + with pytest.raises(PipelineRunError, match="did not include a run id"): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, poll_interval=0.01 + ) + # The run was submitted; only the wait was refused. + assert len(client.created) == 1 + assert client.get_calls == 0 + + # wait=False keeps the id-less response inspectable. + result = submit_and_wait_prepared_body(_prepared_body(), client=client, wait=False) + assert result["run_id"] is None + assert "wait" not in result + + +@pytest.mark.parametrize( + "body", [{"arguments": {}}, {"root_task": None}, {"root_task": "not-a-mapping"}] +) +def test_body_without_root_task_mapping_fails_before_submit(body: dict) -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError, match="root_task"): + submit_and_wait_prepared_body(body, client=client, wait=False) + assert client.created == [] + + +@pytest.mark.parametrize( + "root_task", + [{"arguments": {}}, {"componentRef": None}, {"componentRef": ["not-a-mapping"]}], +) +def test_body_without_component_ref_mapping_fails_before_submit(root_task: dict) -> None: + client = _SubmitClient() + with pytest.raises(PipelineRunError, match="componentRef"): + submit_and_wait_prepared_body( + {"root_task": root_task}, client=client, wait=False + ) + assert client.created == [] + + +def test_caller_body_not_mutated() -> None: + body = _prepared_body() + original = copy.deepcopy(body) + submit_and_wait_prepared_body(body, client=_SubmitClient(), wait=True, poll_interval=0.01) + assert body == original + + +def test_manager_and_client_are_mutually_exclusive() -> None: + manager = PipelineRunManager(client=_SubmitClient()) + with pytest.raises(PipelineRunError): + submit_and_wait_prepared_body( + _prepared_body(), manager=manager, client=_SubmitClient() + ) + + +def test_locator_body_without_inline_spec_submits_original_body() -> None: + client = _SubmitClient() + body = _locator_body() + original = copy.deepcopy(body) + + result = submit_and_wait_prepared_body(body, client=client, wait=False) + + # The client receives the original locator body (modulo the submission-id + # annotation the submit lifecycle injects for post-failure recovery). + assert len(client.created) == 1 + assert _without_submission_annotation(client.created[0]) == original + assert result["run_id"] == RUN_ID + assert result["root_execution_id"] == ROOT_EXECUTION_ID + assert body == original + + +def test_manager_submit_prepared_body_accepts_locator_body() -> None: + """Regression for the shipped submit path (not just the new helper): + ``PipelineRunManager.submit_prepared_body`` used to raise ``KeyError`` on a + locator-style body with no inline ``componentRef.spec``; it must now submit + the body verbatim with a spec-less run context.""" + + client = _SubmitClient() + manager = PipelineRunManager(client=client) + body = _locator_body() + original = copy.deepcopy(body) + + response = manager.submit_prepared_body(body) + + # submit_prepared_body normalizes/submits the caller's body directly (no + # submission-id annotation injection, which lives in the run lifecycle). + assert client.created == [original] + assert response == {"id": RUN_ID, "root_execution_id": ROOT_EXECUTION_ID} + assert body == original + + +def test_partial_native_package_raises_actionable_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A partially-installed native package (missing ``tangle_api.generated``) + should surface the actionable install hint, not a raw ModuleNotFoundError.""" + + real_import = builtins.__import__ + + def fake_import( + name: str, + globals: Any = None, + locals: Any = None, + fromlist: Any = (), + level: int = 0, + ) -> Any: + # Intercept the lazy ``from .client import TangleApiClient`` and fail as + # if a native submodule were missing rather than the top-level package. + if level == 1 and name == "client" and fromlist and "TangleApiClient" in fromlist: + raise ModuleNotFoundError( + "No module named 'tangle_api.generated'", name="tangle_api.generated" + ) + return real_import(name, globals, locals, fromlist, level) + + monkeypatch.setattr(builtins, "__import__", fake_import) + + # No manager/client -> the default manager path performs the native import. + with pytest.raises(PipelineRunError, match="Native generated Tangle API bindings"): + submit_and_wait_prepared_body(_prepared_body()) + + +def test_explicit_manager_is_reused() -> None: + client = _SubmitClient() + manager = PipelineRunManager(client=client) + result = submit_and_wait_prepared_body( + _prepared_body(), manager=manager, wait=False + ) + assert result["run_id"] == RUN_ID + assert client.created # the provided manager's client handled the submit + + +class _RecoveringClient(_SubmitClient): + """Submit dies without a response although the run was actually created; + the run is then discoverable via the submission-id list lookup.""" + + def __init__(self) -> None: + super().__init__() + self.list_calls: list[dict[str, Any]] = [] + + def pipeline_runs_create(self, body: Any = None) -> dict[str, Any]: + self.created.append(copy.deepcopy(body)) + raise TimeoutError("submit connection dropped") + + def pipeline_runs_list(self, **kwargs: Any) -> dict[str, Any]: + self.list_calls.append(kwargs) + return { + "pipeline_runs": [{"id": RUN_ID, "root_execution_id": ROOT_EXECUTION_ID}] + } + + +def test_submit_failure_adopts_run_recovered_by_submission_id(monkeypatch) -> None: + # Recovery finds the already-created run by the injected submission-id + # annotation and adopts it instead of resubmitting a duplicate. + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda _delay: None) + client = _RecoveringClient() + + result = submit_and_wait_prepared_body(_prepared_body(), client=client, wait=False) + + assert result["run_id"] == RUN_ID + assert result["root_execution_id"] == ROOT_EXECUTION_ID + # Exactly one submit was attempted; the run was adopted, not resubmitted. + assert len(client.created) == 1 + submission_id = client.created[0]["annotations"][_SUBMISSION_ANNOTATION_KEY] + assert len(client.list_calls) == 1 + assert submission_id in client.list_calls[0]["filter_query"] + + +def test_submit_recovery_attempts_zero_disables_lookup(monkeypatch) -> None: + class _FailingClient(_SubmitClient): + def __init__(self) -> None: + super().__init__() + self.list_calls: list[dict[str, Any]] = [] + + def pipeline_runs_create(self, body: Any = None) -> dict[str, Any]: + self.created.append(copy.deepcopy(body)) + raise TimeoutError("submit connection dropped") + + def pipeline_runs_list(self, **kwargs: Any) -> dict[str, Any]: + self.list_calls.append(kwargs) + return {"pipeline_runs": []} + + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda _delay: None) + client = _FailingClient() + + with pytest.raises(TimeoutError): + submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=False, submit_recovery_attempts=0 + ) + + assert client.list_calls == [] + + +def test_logger_used_when_helper_builds_the_manager(monkeypatch) -> None: + # The recovery path logs through the manager's logger, so it makes the + # logger= wiring observable: with client=, the helper must hand the caller's + # logger to the manager it builds. + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda _delay: None) + logger = CaptureLogger() + + result = submit_and_wait_prepared_body( + _prepared_body(), client=_RecoveringClient(), logger=logger, wait=False + ) + + assert result["run_id"] == RUN_ID + assert "Recovered existing pipeline run" in (logger.get_logs() or "") + + +def test_logger_ignored_when_manager_is_supplied(monkeypatch) -> None: + # Documented contract: a supplied manager keeps its own configured logger. + monkeypatch.setattr("tangle_cli.pipeline_run_manager.time.sleep", lambda _delay: None) + manager_logger = CaptureLogger() + ignored_logger = CaptureLogger() + manager = PipelineRunManager(client=_RecoveringClient(), logger=manager_logger) + + result = submit_and_wait_prepared_body( + _prepared_body(), manager=manager, logger=ignored_logger, wait=False + ) + + assert result["run_id"] == RUN_ID + assert "Recovered existing pipeline run" in (manager_logger.get_logs() or "") + assert ignored_logger.get_logs() is None + + +def test_wait_with_max_wait_none_waits_without_deadline() -> None: + client = _SubmitClient() + + result = submit_and_wait_prepared_body( + _prepared_body(), client=client, wait=True, max_wait=None, poll_interval=0.01 + ) + + assert result["wait"]["status"] == "SUCCEEDED" + assert result["wait"]["timed_out"] is False + assert client.get_calls >= 1 From d5aede2a878bb81316bc1ab66b87611d4de76a51 Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Tue, 14 Jul 2026 10:24:19 +0000 Subject: [PATCH 5/7] Add configurable TLS verification for all HTTP transports Introduce a single TLS verification contract shared by the requests client, the httpx schema/operation transport, and the dynamic-discovery client. TANGLE_API_CA_BUNDLE verifies against a custom CA bundle and TANGLE_API_VERIFY_TLS toggles verification, with verification enabled by default. Precedence is explicit argument, CA bundle, verify flag, then the secure default; unset settings preserve requests' environment and caller-supplied session behavior. --- .github/workflows/ci.yml | 9 +- README.md | 51 ++ packages/tangle-cli/src/tangle_cli/api_cli.py | 16 +- .../tangle-cli/src/tangle_cli/api_schema.py | 8 + .../src/tangle_cli/api_transport.py | 126 +++ packages/tangle-cli/src/tangle_cli/cli.py | 69 +- .../tangle-cli/src/tangle_cli/cli_options.py | 24 + packages/tangle-cli/src/tangle_cli/client.py | 10 +- .../tangle_cli/dynamic_discovery_client.py | 15 + tests/test_packaging.py | 7 +- tests/test_tls_verification.py | 764 ++++++++++++++++++ 11 files changed, 1089 insertions(+), 10 deletions(-) create mode 100644 tests/test_tls_verification.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dfd5787..d2f954c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,11 @@ jobs: test: runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + python-version: ["3.12", "3.13"] + steps: - name: Check out repository uses: actions/checkout@v4 @@ -15,7 +20,7 @@ jobs: - name: Set up Python uses: actions/setup-python@v5 with: - python-version: "3.12" + python-version: ${{ matrix.python-version }} - name: Set up uv uses: astral-sh/setup-uv@v5 @@ -29,4 +34,4 @@ jobs: run: git diff --check - name: Run tests - run: uv run pytest + run: uv run --python ${{ matrix.python-version }} pytest diff --git a/README.md b/README.md index 4a67052..bd78841 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,57 @@ API-backed commands commonly accept these options. Explicit CLI options win over | `--config` | YAML/JSON defaults. Many commands accept a single object, a list of objects, or `_defaults` + `configs`. | | `--log-type` | SDK progress logs: `console`, `none`, or `file`. Logs go to stderr or a temp log file so structured stdout stays parseable. | | `TANGLE_VERBOSE=1` | Redacted HTTP request/response diagnostics only. This is separate from normal progress logging. | +| `--ca-bundle` | Global CLI flag: path to a PEM CA bundle used as the TLS trust store for every transport. Overrides `TANGLE_API_CA_BUNDLE`. Place before the subcommand. | +| `--verify-tls` / `--no-verify-tls` | Global CLI flag: enable or disable TLS verification for every transport. Overrides `TANGLE_API_VERIFY_TLS`. `--no-verify-tls` is local-development only. Place before the subcommand. | +| `TANGLE_API_CA_BUNDLE` | Path to a PEM CA bundle used to verify TLS for every transport. Use this to trust a private or corporate CA without disabling verification. | +| `TANGLE_API_VERIFY_TLS` | TLS verification toggle. Values `0`, `false`, or `no` (case/space-insensitive) disable verification; any other nonempty value keeps it on. | + +### TLS verification + +TLS certificate verification is enabled by default for all HTTP transports (schema +fetches, `tangle api` calls, and the programmatic clients). The effective setting is +resolved with the following precedence, highest to lowest: + +1. An explicit `verify=` argument to the Python clients (a `bool` or a path to a CA bundle). +2. The global CLI flags `--ca-bundle` / `--verify-tls` / `--no-verify-tls`. +3. `TANGLE_API_CA_BUNDLE` — verify against the given CA bundle. +4. `TANGLE_API_VERIFY_TLS` — enable or disable verification. +5. The secure default: verification enabled against the system trust store. + +The global CLI flags are true root options that apply to every command — the static +`tangle sdk ...` clients, the dynamic `tangle api ...` commands, and `tangle api refresh`. +Place them **before** the subcommand, for example `tangle --ca-bundle ca.pem api ...` or +`tangle --no-verify-tls sdk ...`. They are honored even by the dynamic OpenAPI schema +discovery that runs before command dispatch. A defaulted (absent) flag does not override +the environment: when a flag is not supplied, the `TANGLE_API_*` variables and the standard +`REQUESTS_CA_BUNDLE` / `CURL_CA_BUNDLE` handling still apply. `--ca-bundle` combined with an +explicit `--no-verify-tls` is contradictory and fails fast before any request; `--ca-bundle` +with `--verify-tls` is redundant but accepted. + +If both env vars are set, `TANGLE_API_CA_BUNDLE` wins and TLS stays verified against the +bundle. Empty values are treated as unset. A `--ca-bundle` or `TANGLE_API_CA_BUNDLE` that +does not point to an existing file fails fast with an actionable error before any request is +made. When no Tangle-specific setting is provided, the standard `REQUESTS_CA_BUNDLE` / +`CURL_CA_BUNDLE` handling and any caller-supplied `requests.Session.verify` are left +untouched. + +For a private CA, prefer `--ca-bundle` / `TANGLE_API_CA_BUNDLE` over disabling verification: +it keeps certificates verified against a trusted root. `--no-verify-tls` / +`TANGLE_API_VERIFY_TLS=0` disables verification entirely and is intended for local +development only — never use it against production endpoints. + +```bash +# Trust a private CA with the global flag (recommended for internal/self-hosted APIs) +uv run tangle --ca-bundle /etc/ssl/private-ca.pem \ + api refresh --base-url https://internal.example + +# Or via environment variable +TANGLE_API_CA_BUNDLE=/etc/ssl/private-ca.pem \ + uv run tangle api refresh --base-url https://internal.example + +# Disable verification (local development only) +uv run tangle --no-verify-tls api refresh --base-url https://localhost:8443 +``` Examples for protected APIs: diff --git a/packages/tangle-cli/src/tangle_cli/api_cli.py b/packages/tangle-cli/src/tangle_cli/api_cli.py index 4a80863..db98df8 100644 --- a/packages/tangle-cli/src/tangle_cli/api_cli.py +++ b/packages/tangle-cli/src/tangle_cli/api_cli.py @@ -640,10 +640,22 @@ def _argv_dispatches_dynamic_command(argv: list[str]) -> bool: def _api_argv_tail(argv: list[str]) -> list[str] | None: - """Return args after the root `api` command, or None for non-API invocations.""" + """Return args after the root `api` command, or None for non-API invocations. + + Global TLS flags (``--ca-bundle``/``--verify-tls``/``--no-verify-tls``) may + precede the subcommand, so they are skipped before locating `api`. + """ args = list(argv[1:]) - for index, arg in enumerate(args): + index = 0 + while index < len(args): + arg = args[index] + if arg == "--ca-bundle" and index + 1 < len(args): + index += 2 + continue + if arg.startswith("--ca-bundle=") or arg in {"--verify-tls", "--no-verify-tls"}: + index += 1 + continue if arg == "--": if index + 1 < len(args) and args[index + 1] == "api": return args[index + 2 :] diff --git a/packages/tangle-cli/src/tangle_cli/api_schema.py b/packages/tangle-cli/src/tangle_cli/api_schema.py index 5475ea8..72ed2cc 100644 --- a/packages/tangle-cli/src/tangle_cli/api_schema.py +++ b/packages/tangle-cli/src/tangle_cli/api_schema.py @@ -18,7 +18,9 @@ _normalize_base_url, _openapi_url, _request_headers, + _VERIFY_UNSET, default_base_url, + httpx_verify, ) SUPPORTED_METHODS = {"get", "post", "put", "patch", "delete"} @@ -122,6 +124,7 @@ def fetch_schema( auth_header: str | None = None, headers: dict[str, str] | None = None, include_env_credentials: bool = True, + verify: Any = _VERIFY_UNSET, ) -> dict[str, Any]: """Fetch ``/openapi.json``, applying bearer and custom auth headers.""" @@ -136,6 +139,7 @@ def fetch_schema( include_env_credentials=include_env_credentials, ), timeout=DEFAULT_TIMEOUT_SECONDS, + verify=httpx_verify(verify), ) response.raise_for_status() payload = response.text @@ -152,6 +156,7 @@ def refresh_schema( auth_header: str | None = None, headers: dict[str, str] | None = None, include_env_credentials: bool = True, + verify: Any = _VERIFY_UNSET, ) -> tuple[dict[str, Any], Path]: """Fetch and cache the latest schema for a backend.""" @@ -163,6 +168,7 @@ def refresh_schema( auth_header, headers, include_env_credentials=include_env_credentials, + verify=verify, ) path = write_cached_schema(schema, base_url) return schema, path @@ -175,6 +181,7 @@ def load_or_fetch_schema( auth_header: str | None = None, headers: dict[str, str] | None = None, include_env_credentials: bool = True, + verify: Any = _VERIFY_UNSET, ) -> dict[str, Any]: """Use a cached schema when available, otherwise fetch once and cache it.""" @@ -188,6 +195,7 @@ def load_or_fetch_schema( auth_header, headers, include_env_credentials=include_env_credentials, + verify=verify, ) return schema diff --git a/packages/tangle-cli/src/tangle_cli/api_transport.py b/packages/tangle-cli/src/tangle_cli/api_transport.py index b128070..42c76f2 100644 --- a/packages/tangle-cli/src/tangle_cli/api_transport.py +++ b/packages/tangle-cli/src/tangle_cli/api_transport.py @@ -5,6 +5,7 @@ import json import os import re +import ssl import sys import urllib.parse from pathlib import Path @@ -16,6 +17,20 @@ DEFAULT_TIMEOUT_SECONDS = 30.0 _HEADER_NAME_RE = re.compile(r"^[!#$%&'*+.^_`|~0-9A-Za-z-]+$") _MISSING = object() + +# Canonical resolved TLS verification value: ``True``/``False`` or a CA bundle +# path. Both requests and httpx transports adapt this single contract. +VerifyValue = bool | str +VerifyArgument = bool | str | os.PathLike[str] | None +_VERIFY_UNSET = object() +_TLS_FALSE_VALUES = frozenset({"0", "false", "no"}) + +# Process-wide TLS override set from global CLI flags (``--ca-bundle`` / +# ``--verify-tls`` / ``--no-verify-tls``). It sits between an explicit +# ``verify=`` argument and the environment variables in :func:`resolve_verify`, +# so a single resolver serves every transport, including the schema discovery +# that runs before normal command dispatch. +_CLI_VERIFY_OVERRIDE: Any = _VERIFY_UNSET _SENSITIVE_HEADER_NAMES = {"authorization", "cloud-auth", "cookie", "x-api-key"} _SENSITIVE_KEY_RE = re.compile( r"(authorization|authentication|(^|[-_])auth($|[-_])|cloud[-_]?auth|cookie|x[-_]?api[-_]?key|token|secret|password|credential|pre[-_]?signed[-_]?url|signed[-_]?url)", @@ -40,6 +55,115 @@ def tangle_verbose_enabled() -> bool: return value.strip().lower() in {"1", "true", "yes", "on"} +def _parse_verify_flag(raw: str) -> bool: + """Interpret ``TANGLE_API_VERIFY_TLS``. + + Only the case/space-insensitive values ``0``, ``false``, and ``no`` disable + verification; any other nonempty value keeps it enabled. + """ + + return raw.strip().lower() not in _TLS_FALSE_VALUES + + +def _validate_ca_bundle(path: str, source: str) -> str: + """Return an existing CA bundle path or fail before any network request.""" + + candidate = Path(path).expanduser() + if not candidate.is_file(): + raise SystemExit( + f"{source} points to a CA bundle that does not exist: {path!r}. " + "Provide a path to an existing PEM file, or unset it to use the " + "system trust store." + ) + return str(candidate) + + +def _coerce_explicit_verify(value: bool | str | os.PathLike[str]) -> VerifyValue: + if isinstance(value, bool): + return value + if isinstance(value, (str, os.PathLike)): + return _validate_ca_bundle(os.fspath(value), "verify") + raise SystemExit("verify must be a bool or a path to a CA bundle file") + + +def configure_cli_verify( + ca_bundle: str | os.PathLike[str] | None = None, + verify_tls: bool | None = None, +) -> None: + """Install (or clear) the process-wide TLS override from global CLI flags. + + ``ca_bundle`` is a path to a PEM trust store; ``verify_tls`` is the tri-state + ``--verify-tls`` / ``--no-verify-tls`` flag where ``None`` means the flag was + not supplied. A ``--ca-bundle`` combined with an explicit + ``--no-verify-tls`` is contradictory and fails fast before any network + request. Passing neither clears any previously installed override. + + The resolved value is validated here so an invalid or missing CA bundle + fails before dynamic schema discovery. It is consulted by + :func:`resolve_verify` for every transport. + """ + + global _CLI_VERIFY_OVERRIDE + if ca_bundle is not None: + if verify_tls is False: + raise SystemExit( + "--ca-bundle cannot be combined with --no-verify-tls: a CA " + "bundle only takes effect when verification is enabled. Drop " + "one of the two flags." + ) + _CLI_VERIFY_OVERRIDE = _validate_ca_bundle(os.fspath(ca_bundle), "--ca-bundle") + return + if verify_tls is not None: + _CLI_VERIFY_OVERRIDE = bool(verify_tls) + return + _CLI_VERIFY_OVERRIDE = _VERIFY_UNSET + + +def resolve_verify(verify: Any = _VERIFY_UNSET) -> Any: + """Resolve the effective TLS verification setting. + + Precedence, highest to lowest: an explicit ``verify`` argument, the global + CLI override (``--ca-bundle`` / ``--verify-tls`` / ``--no-verify-tls``), a + nonempty ``TANGLE_API_CA_BUNDLE``, ``TANGLE_API_VERIFY_TLS``, then a secure + enabled default. When none of these are set, ``_VERIFY_UNSET`` is returned + so callers can preserve library and caller defaults (for example requests' + ``REQUESTS_CA_BUNDLE``/``CURL_CA_BUNDLE`` handling and a caller-supplied + ``Session.verify``). Empty environment values are treated as unset. + """ + + if verify is not _VERIFY_UNSET and verify is not None: + return _coerce_explicit_verify(verify) + if _CLI_VERIFY_OVERRIDE is not _VERIFY_UNSET: + return _CLI_VERIFY_OVERRIDE + ca_bundle = os.environ.get("TANGLE_API_CA_BUNDLE", "") + if ca_bundle.strip(): + return _validate_ca_bundle(ca_bundle.strip(), "TANGLE_API_CA_BUNDLE") + flag = os.environ.get("TANGLE_API_VERIFY_TLS", "") + if flag.strip(): + return _parse_verify_flag(flag) + return _VERIFY_UNSET + + +def resolve_verify_default(verify: Any = _VERIFY_UNSET) -> VerifyValue: + """Like :func:`resolve_verify`, but fall back to the secure ``True`` default.""" + + resolved = resolve_verify(verify) + return True if resolved is _VERIFY_UNSET else resolved + + +def httpx_verify(verify: Any = _VERIFY_UNSET) -> bool | ssl.SSLContext: + """Adapt the resolved verify value to what httpx expects. + + httpx 0.28 deprecates string CA-bundle paths, so a path is turned into an + :class:`ssl.SSLContext`; booleans pass through unchanged. + """ + + resolved = resolve_verify_default(verify) + if isinstance(resolved, bool): + return resolved + return ssl.create_default_context(cafile=resolved) + + def _redact_headers(headers: dict[str, Any] | None) -> dict[str, Any]: redacted: dict[str, Any] = {} for name, value in (headers or {}).items(): @@ -280,6 +404,7 @@ def request_operation( timeout: float = DEFAULT_TIMEOUT_SECONDS, allow_body_file_references: bool = False, include_env_credentials: bool = True, + verify: Any = _VERIFY_UNSET, ) -> httpx.Response: """Dispatch one normalized OpenAPI operation as an HTTP request. @@ -306,6 +431,7 @@ def request_operation( content=content, headers=request_headers, timeout=timeout, + verify=httpx_verify(verify), ) if tangle_verbose_enabled(): log_http_exchange( diff --git a/packages/tangle-cli/src/tangle_cli/cli.py b/packages/tangle-cli/src/tangle_cli/cli.py index e9c1fd4..ad9541c 100644 --- a/packages/tangle-cli/src/tangle_cli/cli.py +++ b/packages/tangle-cli/src/tangle_cli/cli.py @@ -1,4 +1,9 @@ -from cyclopts import App +from __future__ import annotations + +import sys +from typing import Annotated + +from cyclopts import App, Parameter from . import ( __version__, @@ -11,6 +16,8 @@ quickstart, secrets_cli, ) +from .api_transport import configure_cli_verify +from .cli_options import CaBundleOption, VerifyTlsOption def version() -> None: @@ -19,6 +26,41 @@ def version() -> None: print(__version__) +def _configure_tls_from_argv(argv: list[str]) -> None: + """Parse the global TLS flags and install the process-wide override. + + Runs before the `api` app is built so the override is in place for the + dynamic schema discovery that happens during command construction, ahead of + normal Cyclopts dispatch. Only tokens before the first subcommand are + considered; flag validation and conflict detection live in + :func:`configure_cli_verify`. + """ + + ca_bundle: str | None = None + verify_tls: bool | None = None + index = 1 + while index < len(argv): + arg = argv[index] + if arg == "--ca-bundle" and index + 1 < len(argv): + ca_bundle = argv[index + 1] + index += 2 + continue + if arg.startswith("--ca-bundle="): + ca_bundle = arg.split("=", 1)[1] + index += 1 + continue + if arg == "--verify-tls": + verify_tls = True + index += 1 + continue + if arg == "--no-verify-tls": + verify_tls = False + index += 1 + continue + break + configure_cli_verify(ca_bundle, verify_tls) + + def build_sdk_app() -> App: """Build the SDK command group.""" @@ -35,8 +77,15 @@ def build_sdk_app() -> App: return sdk_app -def build_app() -> App: - """Build the root CLI app lazily for the current invocation.""" +def build_app(argv: list[str] | None = None) -> App: + """Build the root CLI app lazily for the current invocation. + + Global TLS flags are parsed from *argv* (defaulting to ``sys.argv``) and + installed before the `api` app is built, because building it can trigger + dynamic OpenAPI schema discovery over the network. + """ + + _configure_tls_from_argv(sys.argv if argv is None else argv) app = App( help="CLI for Tangle, the open-source ML pipeline orchestration platform.", @@ -46,11 +95,23 @@ def build_app() -> App: app.command(quickstart.app) app.command(api_cli.build_app()) app.command(build_sdk_app()) + + @app.meta.default + def launcher( + *tokens: Annotated[str, Parameter(allow_leading_hyphen=True)], + ca_bundle: CaBundleOption = None, + verify_tls: VerifyTlsOption = None, + ) -> None: + """Apply global TLS options, then dispatch the requested command.""" + + configure_cli_verify(ca_bundle, verify_tls) + app(tokens) + return app def main() -> None: - build_app()() + build_app().meta() if __name__ == "__main__": diff --git a/packages/tangle-cli/src/tangle_cli/cli_options.py b/packages/tangle-cli/src/tangle_cli/cli_options.py index f6080ed..5edfbb2 100644 --- a/packages/tangle-cli/src/tangle_cli/cli_options.py +++ b/packages/tangle-cli/src/tangle_cli/cli_options.py @@ -2,6 +2,7 @@ from __future__ import annotations +from pathlib import Path from typing import Annotated from cyclopts import Parameter @@ -50,3 +51,26 @@ str, Parameter(help="Log output: console, none, file."), ] +CaBundleOption = Annotated[ + Path | None, + Parameter( + name="--ca-bundle", + help=( + "Path to a PEM CA bundle used as the TLS trust store for every " + "transport. Overrides TANGLE_API_CA_BUNDLE. Place before the " + "subcommand, e.g. `tangle --ca-bundle ca.pem api ...`." + ), + ), +] +VerifyTlsOption = Annotated[ + bool | None, + Parameter( + name="--verify-tls", + help=( + "Enable (--verify-tls) or disable (--no-verify-tls) TLS " + "certificate verification for every transport. Overrides " + "TANGLE_API_VERIFY_TLS. --no-verify-tls is for local development " + "only. Place before the subcommand." + ), + ), +] diff --git a/packages/tangle-cli/src/tangle_cli/client.py b/packages/tangle-cli/src/tangle_cli/client.py index 060c960..c0e494a 100644 --- a/packages/tangle-cli/src/tangle_cli/client.py +++ b/packages/tangle-cli/src/tangle_cli/client.py @@ -19,11 +19,14 @@ from .api_transport import ( DEFAULT_TIMEOUT_SECONDS, + VerifyArgument, _join_operation_url, _normalize_base_url, _request_headers, + _VERIFY_UNSET, default_base_url, log_http_exchange, + resolve_verify, tangle_verbose_enabled, ) from tangle_api.generated.operations import GeneratedTangleApiOperations @@ -67,6 +70,7 @@ def __init__( timeout: float = DEFAULT_TIMEOUT_SECONDS, session: requests.Session | None = None, include_env_credentials: bool = True, + verify: VerifyArgument = None, ) -> None: self.base_url = _normalize_base_url(base_url or default_base_url()) env_verbose = tangle_verbose_enabled() @@ -79,6 +83,7 @@ def __init__( self.timeout = timeout self.session = session or requests.Session() self.include_env_credentials = include_env_credentials + self._verify = resolve_verify(verify) def _response_model(self, model_name: str, default: Any) -> Any: """Use CLI-composed models for generated operation deserialization.""" @@ -224,6 +229,9 @@ def _request_with_same_origin_redirects( for _ in range(self._MAX_REDIRECTS + 1): request_headers = self._headers(extra_headers) + call_kwargs = dict(request_kwargs) + if self._verify is not _VERIFY_UNSET and "verify" not in call_kwargs: + call_kwargs["verify"] = self._verify response = self.session.request( current_method, current_url, @@ -232,7 +240,7 @@ def _request_with_same_origin_redirects( headers=request_headers, timeout=timeout, allow_redirects=False, - **request_kwargs, + **call_kwargs, ) if self.verbose: log_http_exchange( diff --git a/packages/tangle-cli/src/tangle_cli/dynamic_discovery_client.py b/packages/tangle-cli/src/tangle_cli/dynamic_discovery_client.py index d3a5582..3270919 100644 --- a/packages/tangle-cli/src/tangle_cli/dynamic_discovery_client.py +++ b/packages/tangle-cli/src/tangle_cli/dynamic_discovery_client.py @@ -18,6 +18,7 @@ ) from .api_transport import ( DEFAULT_TIMEOUT_SECONDS, + VerifyArgument, _normalize_base_url, default_base_url, request_operation, @@ -43,6 +44,7 @@ def __init__( auth_header: str | None = None, header: list[str] | str | None = None, timeout: float = DEFAULT_TIMEOUT_SECONDS, + verify: VerifyArgument = None, ) -> None: self.schema = schema self.base_url = _normalize_base_url(base_url or default_base_url()) @@ -51,6 +53,7 @@ def __init__( self.auth_header = auth_header self.header = _header_list(header) self.timeout = timeout + self.verify = verify self._operations = operation_map(schema) self._aliases = self._build_alias_map(self._operations) self._groups = self._build_groups(self._operations) @@ -66,6 +69,7 @@ def from_schema( auth_header: str | None = None, header: list[str] | str | None = None, timeout: float = DEFAULT_TIMEOUT_SECONDS, + verify: VerifyArgument = None, ) -> TangleDynamicDiscoveryClient: """Create a client from an already loaded OpenAPI schema.""" @@ -77,6 +81,7 @@ def from_schema( auth_header=auth_header, header=header, timeout=timeout, + verify=verify, ) @classmethod @@ -89,6 +94,7 @@ def from_cache( auth_header: str | None = None, header: list[str] | str | None = None, timeout: float = DEFAULT_TIMEOUT_SECONDS, + verify: VerifyArgument = None, ) -> TangleDynamicDiscoveryClient: """Create a client from the local schema cache without network access.""" @@ -107,6 +113,7 @@ def from_cache( auth_header=auth_header, header=header, timeout=timeout, + verify=verify, ) @classmethod @@ -119,6 +126,7 @@ def from_url( auth_header: str | None = None, header: list[str] | str | None = None, timeout: float = DEFAULT_TIMEOUT_SECONDS, + verify: VerifyArgument = None, ) -> TangleDynamicDiscoveryClient: """Fetch ``/openapi.json`` and create a client without writing the cache.""" @@ -129,6 +137,7 @@ def from_url( header=header, auth_header=auth_header, headers=headers, + verify=verify, ) return cls.from_schema( schema, @@ -138,6 +147,7 @@ def from_url( auth_header=auth_header, header=header, timeout=timeout, + verify=verify, ) @classmethod @@ -150,6 +160,7 @@ def from_cache_or_refresh( auth_header: str | None = None, header: list[str] | str | None = None, timeout: float = DEFAULT_TIMEOUT_SECONDS, + verify: VerifyArgument = None, ) -> TangleDynamicDiscoveryClient: """Create a client from cache, fetching and caching the schema on miss.""" @@ -162,6 +173,7 @@ def from_cache_or_refresh( header=header, auth_header=auth_header, headers=headers, + verify=verify, ) return cls.from_schema( schema, @@ -171,6 +183,7 @@ def from_cache_or_refresh( auth_header=auth_header, header=header, timeout=timeout, + verify=verify, ) @property @@ -197,6 +210,7 @@ def request(self, operation_name: str, **params: Any) -> httpx.Response: headers = {**self.headers, **dict(headers_override or {})} body = params.pop("body", None) timeout = params.pop("timeout", self.timeout) + verify = params.pop("verify", self.verify) return request_operation( operation, params, @@ -207,6 +221,7 @@ def request(self, operation_name: str, **params: Any) -> httpx.Response: headers=headers, body=body, timeout=timeout, + verify=verify, ) def call(self, operation_name: str, **params: Any) -> Any: diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 4092937..946b73c 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -40,7 +40,7 @@ def _write_import_stubs(path: Path) -> None: _write_runtime_stubs(path) (path / "cyclopts.py").write_text( "class App:\n" - " def __init__(self, *args, **kwargs): pass\n" + " def __init__(self, *args, **kwargs): self._meta = None\n" " def command(self, obj=None, **kwargs):\n" " if obj is not None:\n" " return obj\n" @@ -48,6 +48,11 @@ def _write_import_stubs(path: Path) -> None: " return decorator\n" " def __call__(self, *args, **kwargs): pass\n" " def default(self, fn): return fn\n" + " @property\n" + " def meta(self):\n" + " if self._meta is None:\n" + " self._meta = App()\n" + " return self._meta\n" "\n" "def Parameter(*args, **kwargs): return object()\n", encoding="utf-8", diff --git a/tests/test_tls_verification.py b/tests/test_tls_verification.py new file mode 100644 index 0000000..46318f5 --- /dev/null +++ b/tests/test_tls_verification.py @@ -0,0 +1,764 @@ +"""Tests for centralized TLS verification configuration across transports. + +Covers environment parsing, explicit precedence, path handling, preservation of +caller-supplied session settings, and propagation into the requests client, the +httpx schema/operation transport, and the dynamic-discovery client. A real +local HTTPS server with a generated private CA exercises the end-to-end +behavior of the secure default, ``TANGLE_API_CA_BUNDLE``, and +``TANGLE_API_VERIFY_TLS=0``. +""" + +from __future__ import annotations + +import http.server +import json +import os +import shutil +import ssl +import subprocess +import sys +import threading +from pathlib import Path +from types import SimpleNamespace +from typing import Any, Iterator + +import httpx +import pytest +import requests + +from tangle_cli import cli +from tangle_cli.api_cli import _api_argv_tail +from tangle_cli.api_schema import fetch_schema +from tangle_cli.api_transport import ( + _VERIFY_UNSET, + configure_cli_verify, + httpx_verify, + request_operation, + resolve_verify, + resolve_verify_default, +) +from tangle_cli.client import TangleApiClient +from tangle_cli.dynamic_discovery_client import TangleDynamicDiscoveryClient + +_TLS_ENV_VARS = ("TANGLE_API_VERIFY_TLS", "TANGLE_API_CA_BUNDLE") + + +@pytest.fixture(autouse=True) +def _clear_tls_env(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + for name in _TLS_ENV_VARS: + monkeypatch.delenv(name, raising=False) + configure_cli_verify() # ensure no leaked process-wide CLI override + yield + configure_cli_verify() + + +# --------------------------------------------------------------------------- # +# Environment parsing and precedence +# --------------------------------------------------------------------------- # + + +def test_resolve_verify_defaults_to_unset() -> None: + assert resolve_verify() is _VERIFY_UNSET + assert resolve_verify_default() is True + assert httpx_verify() is True + + +@pytest.mark.parametrize("value", ["0", "false", "FALSE", " no ", "No", "nO"]) +def test_verify_tls_false_values_disable_verification( + monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", value) + assert resolve_verify() is False + assert httpx_verify() is False + + +@pytest.mark.parametrize("value", ["1", "true", "yes", "on", "enabled", "anything"]) +def test_verify_tls_other_nonempty_values_keep_verification( + monkeypatch: pytest.MonkeyPatch, value: str +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", value) + assert resolve_verify() is True + + +def test_empty_verify_tls_is_treated_as_unset(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", " ") + assert resolve_verify() is _VERIFY_UNSET + + +def test_ca_bundle_env_resolves_to_path( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", str(bundle)) + assert resolve_verify() == str(bundle) + + +def test_empty_ca_bundle_is_treated_as_unset(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", " ") + assert resolve_verify() is _VERIFY_UNSET + + +def test_missing_ca_bundle_fails_early(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", "/nonexistent/ca.pem") + with pytest.raises(SystemExit, match="TANGLE_API_CA_BUNDLE"): + resolve_verify() + + +def test_ca_bundle_wins_over_verify_tls( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", str(bundle)) + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "0") + # CA bundle wins and TLS stays verified against the bundle. + assert resolve_verify() == str(bundle) + + +def test_explicit_argument_wins_over_environment( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", str(bundle)) + assert resolve_verify(False) is False + assert resolve_verify(True) is True + + +def test_explicit_pathlike_argument_is_accepted(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + assert resolve_verify(bundle) == str(bundle) + + +def test_explicit_missing_path_argument_fails_early(tmp_path: Path) -> None: + with pytest.raises(SystemExit, match="verify"): + resolve_verify(str(tmp_path / "missing.pem")) + + +def test_none_argument_falls_through_to_environment( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "0") + assert resolve_verify(None) is False + + +# --------------------------------------------------------------------------- # +# httpx adapter +# --------------------------------------------------------------------------- # + + +def test_httpx_verify_converts_path_to_ssl_context(tmp_path: Path) -> None: + ca = _generate_private_ca(tmp_path) + context = httpx_verify(str(ca.ca_pem)) + assert isinstance(context, ssl.SSLContext) + + +def test_httpx_verify_passes_booleans_through() -> None: + assert httpx_verify(True) is True + assert httpx_verify(False) is False + + +# --------------------------------------------------------------------------- # +# Transport propagation (mocked) +# --------------------------------------------------------------------------- # + + +def _operation(path: str, *, method: str = "GET") -> SimpleNamespace: + return SimpleNamespace( + method=method, + path=path, + parameters=[], + group_name="test", + command_name="op", + has_request_body=False, + ) + + +def test_request_operation_passes_verify_to_httpx( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, Any] = {} + + def fake_request(method: str, url: str, **kwargs: Any) -> httpx.Response: + captured.update(kwargs) + return httpx.Response(200, json={}, request=httpx.Request(method, url)) + + monkeypatch.setattr("tangle_cli.api_transport.httpx.request", fake_request) + request_operation( + _operation("/api/ping"), + {}, + base_url="https://api.test", + verify=False, + ) + assert captured["verify"] is False + + +def test_request_operation_defaults_to_secure_verify( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, Any] = {} + + def fake_request(method: str, url: str, **kwargs: Any) -> httpx.Response: + captured.update(kwargs) + return httpx.Response(200, json={}, request=httpx.Request(method, url)) + + monkeypatch.setattr("tangle_cli.api_transport.httpx.request", fake_request) + request_operation(_operation("/api/ping"), {}, base_url="https://api.test") + assert captured["verify"] is True + + +def test_fetch_schema_passes_verify_to_httpx(monkeypatch: pytest.MonkeyPatch) -> None: + captured: dict[str, Any] = {} + + def fake_get(url: str, **kwargs: Any) -> httpx.Response: + captured.update(kwargs) + return httpx.Response( + 200, + json={"openapi": "3.1.0", "paths": {}}, + request=httpx.Request("GET", url), + ) + + monkeypatch.setattr("tangle_cli.api_schema.httpx.get", fake_get) + fetch_schema("https://api.test", verify=False) + assert captured["verify"] is False + + +class _RecordingSession: + """Minimal session without a ``.verify`` attribute.""" + + def __init__(self) -> None: + self.calls: list[dict[str, Any]] = [] + + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + self.calls.append({"method": method, "url": url, **kwargs}) + r = requests.Response() + r.status_code = 200 + r._content = b"{}" + r.headers["Content-Type"] = "application/json" + r.request = requests.Request(method, url).prepare() + return r + + +def test_static_client_injects_verify_when_configured( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "0") + session = _RecordingSession() + client = TangleApiClient("https://api.test", session=session) + + client._make_request("GET", "/api/ping") + + assert session.calls[0]["verify"] is False + + +def test_static_client_preserves_session_when_unset() -> None: + session = _RecordingSession() + client = TangleApiClient("https://api.test", session=session) + + client._make_request("GET", "/api/ping") + + # No Tangle TLS setting: do not pass ``verify`` so requests' own + # session/environment handling (REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE) applies. + assert "verify" not in session.calls[0] + + +def test_static_client_explicit_verify_argument_wins() -> None: + session = _RecordingSession() + client = TangleApiClient("https://api.test", session=session, verify=False) + + client._make_request("GET", "/api/ping") + + assert session.calls[0]["verify"] is False + + +# --------------------------------------------------------------------------- # +# Real local HTTPS end-to-end tests with a generated private CA +# --------------------------------------------------------------------------- # + + +class _GeneratedCa(SimpleNamespace): + ca_pem: Path + server_crt: Path + server_key: Path + + +def _generate_private_ca(directory: Path) -> _GeneratedCa: + """Generate a private CA and a localhost server cert using openssl.""" + + ca_key = directory / "ca.key" + ca_pem = directory / "ca.pem" + server_key = directory / "server.key" + server_csr = directory / "server.csr" + server_crt = directory / "server.crt" + ext = directory / "ext.cnf" + # Python 3.13's TLS stack rejects a leaf missing an Authority Key Identifier + # ("Missing Authority Key Identifier"), so the leaf must carry the full, + # standards-conformant extension set (AKI/SKI, CA:FALSE, key usage, and the + # server-auth EKU) rather than only a SAN. This keeps the CA-bundle success + # cases valid across every supported interpreter (3.10-3.13), not just 3.12. + ext.write_text( + "subjectAltName=DNS:localhost,IP:127.0.0.1\n" + "basicConstraints=critical,CA:FALSE\n" + "keyUsage=critical,digitalSignature,keyEncipherment\n" + "extendedKeyUsage=serverAuth\n" + "subjectKeyIdentifier=hash\n" + "authorityKeyIdentifier=keyid,issuer\n", + encoding="utf-8", + ) + + def _run(args: list[str]) -> None: + subprocess.run( + args, + check=True, + capture_output=True, + ) + + _run([ + "openssl", "req", "-x509", "-newkey", "rsa:2048", "-nodes", + "-keyout", str(ca_key), "-out", str(ca_pem), + "-subj", "/CN=Tangle Test CA", "-days", "2", + "-addext", "basicConstraints=critical,CA:TRUE", + "-addext", "keyUsage=critical,keyCertSign,cRLSign", + # A subject key identifier on the CA lets the leaf's + # ``authorityKeyIdentifier=keyid`` resolve to it. + "-addext", "subjectKeyIdentifier=hash", + ]) + _run([ + "openssl", "req", "-newkey", "rsa:2048", "-nodes", + "-keyout", str(server_key), "-out", str(server_csr), + "-subj", "/CN=localhost", + ]) + _run([ + "openssl", "x509", "-req", "-in", str(server_csr), + "-CA", str(ca_pem), "-CAkey", str(ca_key), "-CAcreateserial", + "-out", str(server_crt), "-days", "2", "-extfile", str(ext), + ]) + return _GeneratedCa(ca_pem=ca_pem, server_crt=server_crt, server_key=server_key) + + +class _SchemaHandler(http.server.BaseHTTPRequestHandler): + def do_GET(self) -> None: # noqa: N802 - required by BaseHTTPRequestHandler + body = json.dumps({"openapi": "3.1.0", "info": {}, "paths": {}}).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args: Any) -> None: # silence test server logging + pass + + +@pytest.fixture(scope="module") +def https_server(tmp_path_factory: pytest.TempPathFactory) -> Iterator[dict[str, Any]]: + if shutil.which("openssl") is None: # pragma: no cover - environment guard + pytest.skip("openssl is required for the real HTTPS TLS tests") + + directory = tmp_path_factory.mktemp("tls") + ca = _generate_private_ca(directory) + + context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + context.load_cert_chain(certfile=str(ca.server_crt), keyfile=str(ca.server_key)) + + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _SchemaHandler) + server.socket = context.wrap_socket(server.socket, server_side=True) + port = server.server_address[1] + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield { + "base_url": f"https://localhost:{port}", + "ca_pem": str(ca.ca_pem), + } + finally: + server.shutdown() + thread.join(timeout=5) + server.server_close() + + +def test_httpx_schema_default_verification_fails( + https_server: dict[str, Any], +) -> None: + with pytest.raises(httpx.ConnectError): + fetch_schema(https_server["base_url"]) + + +def test_httpx_schema_ca_bundle_succeeds( + monkeypatch: pytest.MonkeyPatch, https_server: dict[str, Any] +) -> None: + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", https_server["ca_pem"]) + schema = fetch_schema(https_server["base_url"]) + assert schema["paths"] == {} + + +def test_httpx_schema_verify_off_succeeds( + monkeypatch: pytest.MonkeyPatch, https_server: dict[str, Any] +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "0") + schema = fetch_schema(https_server["base_url"]) + assert schema["paths"] == {} + + +def test_dynamic_client_from_url_uses_ca_bundle( + monkeypatch: pytest.MonkeyPatch, https_server: dict[str, Any] +) -> None: + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", https_server["ca_pem"]) + client = TangleDynamicDiscoveryClient.from_url(https_server["base_url"]) + assert client.operations == () + + +def test_dynamic_client_from_url_default_verification_fails( + https_server: dict[str, Any], +) -> None: + with pytest.raises(httpx.ConnectError): + TangleDynamicDiscoveryClient.from_url(https_server["base_url"]) + + +def test_requests_client_default_verification_fails( + https_server: dict[str, Any], +) -> None: + client = TangleApiClient(https_server["base_url"]) + with pytest.raises(requests.exceptions.SSLError): + client._make_request("GET", "/api/ping") + + +def test_requests_client_ca_bundle_succeeds( + monkeypatch: pytest.MonkeyPatch, https_server: dict[str, Any] +) -> None: + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", https_server["ca_pem"]) + client = TangleApiClient(https_server["base_url"]) + response = client._make_request("GET", "/api/ping") + assert response.status_code == 200 + + +def test_requests_client_verify_off_succeeds( + monkeypatch: pytest.MonkeyPatch, https_server: dict[str, Any] +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "0") + client = TangleApiClient(https_server["base_url"]) + response = client._make_request("GET", "/api/ping") + assert response.status_code == 200 + + +# --------------------------------------------------------------------------- # +# Global CLI override: configure_cli_verify and resolver precedence +# --------------------------------------------------------------------------- # + + +def test_cli_override_disables_verification() -> None: + configure_cli_verify(verify_tls=False) + assert resolve_verify() is False + + +def test_cli_override_enables_verification() -> None: + configure_cli_verify(verify_tls=True) + assert resolve_verify() is True + + +def test_cli_override_ca_bundle_resolves_to_path(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + configure_cli_verify(ca_bundle=bundle) + assert resolve_verify() == str(bundle) + + +def test_cli_override_wins_over_environment( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "1") + configure_cli_verify(verify_tls=False) + assert resolve_verify() is False + + +def test_cli_ca_bundle_wins_over_environment( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + env_bundle = tmp_path / "env.pem" + env_bundle.write_text("cert", encoding="utf-8") + cli_bundle = tmp_path / "cli.pem" + cli_bundle.write_text("cert", encoding="utf-8") + monkeypatch.setenv("TANGLE_API_CA_BUNDLE", str(env_bundle)) + configure_cli_verify(ca_bundle=cli_bundle) + assert resolve_verify() == str(cli_bundle) + + +def test_explicit_python_argument_wins_over_cli_override() -> None: + configure_cli_verify(verify_tls=False) + # A library caller's explicit verify= stays highest precedence. + assert resolve_verify(True) is True + + +def test_cli_override_conflict_ca_bundle_and_no_verify_fails(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + with pytest.raises(SystemExit, match="ca-bundle"): + configure_cli_verify(ca_bundle=bundle, verify_tls=False) + + +def test_cli_override_ca_bundle_and_verify_true_is_accepted(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + configure_cli_verify(ca_bundle=bundle, verify_tls=True) + assert resolve_verify() == str(bundle) + + +def test_cli_override_missing_ca_bundle_fails_early() -> None: + with pytest.raises(SystemExit, match="ca-bundle"): + configure_cli_verify(ca_bundle="/nonexistent/ca.pem") + + +def test_cli_override_clears_back_to_unset(monkeypatch: pytest.MonkeyPatch) -> None: + configure_cli_verify(verify_tls=False) + assert resolve_verify() is False + configure_cli_verify() + assert resolve_verify() is _VERIFY_UNSET + + +# --------------------------------------------------------------------------- # +# argv pre-parse and placement +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize( + "argv, expected", + [ + (["tangle", "--no-verify-tls"], False), + (["tangle", "--verify-tls"], True), + ], +) +def test_configure_tls_from_argv_parses_boolean_flags( + argv: list[str], expected: bool +) -> None: + cli._configure_tls_from_argv(argv) + assert resolve_verify() is expected + + +def test_configure_tls_from_argv_parses_ca_bundle(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + cli._configure_tls_from_argv(["tangle", "--ca-bundle", str(bundle), "api", "ping"]) + assert resolve_verify() == str(bundle) + + +def test_configure_tls_from_argv_parses_ca_bundle_equals(tmp_path: Path) -> None: + bundle = tmp_path / "ca.pem" + bundle.write_text("cert", encoding="utf-8") + cli._configure_tls_from_argv(["tangle", f"--ca-bundle={bundle}", "sdk"]) + assert resolve_verify() == str(bundle) + + +def test_configure_tls_from_argv_stops_at_subcommand( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("TANGLE_API_VERIFY_TLS", "1") + # A --no-verify-tls after the subcommand is not a global flag: it must not + # be consumed here, leaving the env default to apply. + cli._configure_tls_from_argv(["tangle", "api", "ping", "--no-verify-tls"]) + assert resolve_verify() is True + + +@pytest.mark.parametrize( + "argv, expected_tail", + [ + (["tangle", "--no-verify-tls", "api", "ping"], ["ping"]), + (["tangle", "--ca-bundle", "ca.pem", "api", "foo", "bar"], ["foo", "bar"]), + (["tangle", "--ca-bundle=ca.pem", "api", "foo"], ["foo"]), + (["tangle", "--verify-tls", "api"], []), + ], +) +def test_api_argv_tail_skips_global_tls_flags( + argv: list[str], expected_tail: list[str] +) -> None: + assert _api_argv_tail(argv) == expected_tail + + +# --------------------------------------------------------------------------- # +# Global override propagation into every transport (mocked) +# --------------------------------------------------------------------------- # + + +def test_cli_override_propagates_to_static_requests_client() -> None: + configure_cli_verify(verify_tls=False) + session = _RecordingSession() + client = TangleApiClient("https://api.test", session=session) + client._make_request("GET", "/api/ping") + assert session.calls[0]["verify"] is False + + +def test_cli_override_propagates_to_request_operation( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, Any] = {} + + def fake_request(method: str, url: str, **kwargs: Any) -> httpx.Response: + captured.update(kwargs) + return httpx.Response(200, json={}, request=httpx.Request(method, url)) + + monkeypatch.setattr("tangle_cli.api_transport.httpx.request", fake_request) + configure_cli_verify(verify_tls=False) + request_operation(_operation("/api/ping"), {}, base_url="https://api.test") + assert captured["verify"] is False + + +def test_cli_override_propagates_to_schema_fetch( + monkeypatch: pytest.MonkeyPatch, +) -> None: + captured: dict[str, Any] = {} + + def fake_get(url: str, **kwargs: Any) -> httpx.Response: + captured.update(kwargs) + return httpx.Response( + 200, + json={"openapi": "3.1.0", "paths": {}}, + request=httpx.Request("GET", url), + ) + + monkeypatch.setattr("tangle_cli.api_schema.httpx.get", fake_get) + configure_cli_verify(verify_tls=False) + fetch_schema("https://api.test") + assert captured["verify"] is False + + +# --------------------------------------------------------------------------- # +# Real local HTTPS end-to-end tests driven by the global CLI override +# --------------------------------------------------------------------------- # + + +def test_cli_override_ca_bundle_schema_fetch_succeeds( + https_server: dict[str, Any], +) -> None: + configure_cli_verify(ca_bundle=https_server["ca_pem"]) + schema = fetch_schema(https_server["base_url"]) + assert schema["paths"] == {} + + +def test_cli_override_verify_off_schema_fetch_succeeds( + https_server: dict[str, Any], +) -> None: + configure_cli_verify(verify_tls=False) + schema = fetch_schema(https_server["base_url"]) + assert schema["paths"] == {} + + +def test_cli_override_default_schema_fetch_fails( + https_server: dict[str, Any], +) -> None: + configure_cli_verify() + with pytest.raises(httpx.ConnectError): + fetch_schema(https_server["base_url"]) + + +def test_cli_override_ca_bundle_dynamic_client_succeeds( + https_server: dict[str, Any], +) -> None: + configure_cli_verify(ca_bundle=https_server["ca_pem"]) + client = TangleDynamicDiscoveryClient.from_url(https_server["base_url"]) + assert client.operations == () + + +def test_cli_override_ca_bundle_requests_client_succeeds( + https_server: dict[str, Any], +) -> None: + configure_cli_verify(ca_bundle=https_server["ca_pem"]) + client = TangleApiClient(https_server["base_url"]) + response = client._make_request("GET", "/api/ping") + assert response.status_code == 200 + + +# --------------------------------------------------------------------------- # +# Live CLI subprocess: global flags reach `tangle api refresh` over real HTTPS +# --------------------------------------------------------------------------- # + + +def _run_tangle( + args: list[str], cache_dir: Path, extra_env: dict[str, str] | None = None +) -> subprocess.CompletedProcess[str]: + env = {**os.environ, "TANGLE_CLI_CACHE_DIR": str(cache_dir)} + if extra_env: + env.update(extra_env) + return subprocess.run( + [ + sys.executable, + "-c", + "import sys; from tangle_cli.cli import main; " + "sys.argv = ['tangle', *sys.argv[1:]]; main()", + *args, + ], + capture_output=True, + text=True, + env=env, + ) + + +def test_cli_subprocess_ca_bundle_refresh_succeeds( + https_server: dict[str, Any], tmp_path: Path +) -> None: + result = _run_tangle( + [ + "--ca-bundle", + https_server["ca_pem"], + "api", + "refresh", + "--base-url", + https_server["base_url"], + ], + tmp_path, + ) + assert result.returncode == 0, result.stderr + assert "Cached OpenAPI schema" in result.stdout + + +def test_cli_subprocess_default_refresh_fails( + https_server: dict[str, Any], tmp_path: Path +) -> None: + result = _run_tangle( + ["api", "refresh", "--base-url", https_server["base_url"]], + tmp_path, + ) + assert result.returncode != 0 + assert "Failed to fetch" in result.stderr + + +def test_cli_subprocess_verify_off_refresh_succeeds( + https_server: dict[str, Any], tmp_path: Path +) -> None: + result = _run_tangle( + [ + "--no-verify-tls", + "api", + "refresh", + "--base-url", + https_server["base_url"], + ], + tmp_path, + ) + assert result.returncode == 0, result.stderr + assert "Cached OpenAPI schema" in result.stdout + + +def test_cli_subprocess_ca_bundle_and_no_verify_conflict_fails( + https_server: dict[str, Any], tmp_path: Path +) -> None: + result = _run_tangle( + [ + "--ca-bundle", + https_server["ca_pem"], + "--no-verify-tls", + "api", + "refresh", + "--base-url", + https_server["base_url"], + ], + tmp_path, + ) + assert result.returncode != 0 + assert "ca-bundle" in result.stderr + + +def test_cli_root_help_lists_global_tls_flags(tmp_path: Path) -> None: + result = _run_tangle(["--help"], tmp_path) + assert result.returncode == 0 + assert "--ca-bundle" in result.stdout + assert "--no-verify-tls" in result.stdout From d6eea86ad77f27084de1484831d8ad0e6fd49d07 Mon Sep 17 00:00:00 2001 From: Arseniy Dugin Date: Mon, 13 Jul 2026 13:03:13 +0000 Subject: [PATCH 6/7] feat(client): retry transient GET failures with backoff Idempotent GETs now retry on 500/502/503/504 and transient transport errors (connection reset/refused, timeout, truncated body) with doubling backoff capped at 30s. Mutating methods are never duplicated and streamed GETs are excluded so stream consumers keep control of the open path. SSL certificate errors raise on the first attempt because they are deterministic and retrying only delays the report. The transient, 429 rate-limit, and 401 auth-refresh layers share one per-logical-request budget so a composed outage cannot multiply their per-layer limits together. That budget counts logical attempts and physical sends separately against a single 120s deadline: 7 attempts, and a pool of 14 session.request calls. The pool is the larger of two floors -- two full _MAX_REDIRECTS-deep chains (12) and two sends per logical attempt (14) -- so at current constants the attempt term is what sets it. Sends need their own pool because a redirect hop is not a retry; spending hops from the attempt count would break ordinary healthy traffic, since a chain three or more hops deep followed by one 401 refresh needs at least 8 sends with no outage involved. Pooling them still bounds worst-case amplification at 14 requests rather than attempts times chain length. Every session.request charges the send pool at the send boundary, including each same-origin redirect hop, and the check and the charge happen together immediately before the send so a long Retry-After or backoff cannot carry the clock past the deadline and still let a request out. A wait that would not fit in the remaining deadline ends the sequence instead of being slept. When the pool runs out part-way through a redirect chain, the last completed non-redirect response is reported so raise_for_status still surfaces the real backend status; only exhaustion during the very first chain, with nothing completed to report, raises RetryError, and that error names whether the deadline or the send pool was the cause. A replayable GET spends the shared budget on rate limiting too, so a sustained 429 without Retry-After now takes 7 sends and waits 1+2+4+8+16+32 = 63s; the 429 backoff is capped at 60s, which the 32s step never reaches. The 5xx path over the same 7 sends waits 1+2+4+8+16+30 = 61s under its lower 30s cap. Both stay inside the 120s deadline. Requests that cannot be replayed -- mutating methods and streamed GETs -- keep the existing four-attempt 429 cap and its 1/2/4s backoff. Every superseded 429 is closed before its backoff so a pooled connection is not held across the wait, which matters most for streamed GETs whose bodies are never read; the response actually returned to the caller is left open. Retry warnings go through the client logger; pipeline-run, artifact, pipeline hydration, published-component, and secret commands thread their --log-type logger into the client so the warnings follow the configured sink, and logger-less programmatic clients stay silent. RetryError renders its destination credential-safely: only the method and scheme://host[:port]/path survive, with query, fragment, and authority userinfo removed, because a same-origin redirect can land on a signed URL (access_token, X-Amz-Signature) and the exhaustion error flows into CLI output and logs. Authorities the parser rejects, such as an unclosed IPv6 bracket, degrade to a placeholder instead of letting error formatting raise, and malformed ports pass through untouched since the port is never parsed. --- .../src/tangle_cli/artifacts_cli.py | 1 + packages/tangle-cli/src/tangle_cli/client.py | 346 ++++++- .../src/tangle_cli/pipeline_runs_cli.py | 16 +- .../src/tangle_cli/pipelines_cli.py | 1 + .../tangle_cli/published_components_cli.py | 9 +- .../tangle-cli/src/tangle_cli/secrets_cli.py | 7 +- tests/test_api_cli.py | 2 + tests/test_artifacts_cli.py | 2 + tests/test_client.py | 954 ++++++++++++++++++ tests/test_components_cli.py | 3 + tests/test_secrets_cli.py | 3 + 11 files changed, 1313 insertions(+), 31 deletions(-) diff --git a/packages/tangle-cli/src/tangle_cli/artifacts_cli.py b/packages/tangle-cli/src/tangle_cli/artifacts_cli.py index a6d59e1..bb9f496 100644 --- a/packages/tangle-cli/src/tangle_cli/artifacts_cli.py +++ b/packages/tangle-cli/src/tangle_cli/artifacts_cli.py @@ -80,6 +80,7 @@ def artifacts_get( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="artifact commands", + logger=logger, ) if require_available := getattr(client, "require_available", None): require_available() diff --git a/packages/tangle-cli/src/tangle_cli/client.py b/packages/tangle-cli/src/tangle_cli/client.py index 060c960..db45527 100644 --- a/packages/tangle-cli/src/tangle_cli/client.py +++ b/packages/tangle-cli/src/tangle_cli/client.py @@ -13,7 +13,7 @@ from dataclasses import asdict, is_dataclass from email.utils import parsedate_to_datetime from typing import Any -from urllib.parse import quote, urljoin, urlparse +from urllib.parse import quote, urljoin, urlparse, urlsplit, urlunsplit import requests @@ -40,6 +40,69 @@ ) +class _RetryBudget: + """Shared attempt/send/deadline budget for one logical request. + + The transient-5xx, 429 rate-limit, and 401 auth-refresh retry layers all + draw from a single instance so a composed outage cannot multiply their + per-layer limits. Attempts and sends are counted separately because they + bound different things: ``attempts`` is how many logical tries remain (one + per entry into the redirect helper) while ``sends`` is how many physical + ``session.request`` calls remain, every same-origin redirect hop included. + Keeping ``sends`` larger than ``attempts`` is what lets a legal redirect + chain still be followed on a retry, instead of its hops being spent as if + they were tries. ``deadline`` is a ``time.monotonic`` value past which no + further send is admitted. + """ + + __slots__ = ("attempts", "sends", "deadline") + + def __init__(self, max_attempts: int, max_sends: int, deadline: float) -> None: + self.attempts = max_attempts + self.sends = max_sends + self.deadline = deadline + + def start_attempt(self) -> None: + """Charge one logical try; callers gate on :meth:`can_retry` first.""" + + self.attempts -= 1 + + def try_consume_send(self) -> bool: + """Atomically admit one physical send, charging the budget for it. + + Checking and charging together, immediately before the send, is what + stops a request from slipping out after a long ``Retry-After`` or + backoff has already carried the clock past the deadline. + """ + + if self.sends <= 0 or time.monotonic() >= self.deadline: + return False + self.sends -= 1 + return True + + def exhaustion_reason(self) -> str: + """Why :meth:`try_consume_send` refused, for the exhaustion error text. + + Mirrors that method's check order so the reported cause is the one that + actually stopped the send. + """ + + return "send pool exhausted" if self.sends <= 0 else "deadline exceeded" + + def can_retry(self) -> bool: + return self.attempts > 0 and self.sends > 0 and time.monotonic() < self.deadline + + def allows_wait(self, delay: float) -> bool: + """True when sleeping ``delay`` would still leave time to send again. + + Waits are bounded by the remaining deadline rather than truncated to it: + a sleep that would reach the deadline can only be followed by a send the + budget must refuse, so the caller stops immediately instead. + """ + + return delay < self.deadline - time.monotonic() + + class TangleApiClient(GeneratedTangleApiOperations): """Single public API wrapper for Tangle backends. @@ -50,9 +113,35 @@ class TangleApiClient(GeneratedTangleApiOperations): _REDIRECT_STATUSES = {301, 302, 303, 307, 308} _MAX_REDIRECTS = 5 - _MAX_RATE_LIMIT_RETRIES = 3 _RATE_LIMIT_BACKOFF_SECONDS = 1.0 _MAX_RETRY_AFTER_SECONDS = 60.0 + # Requests that cannot be replayed after a transient failure (mutating + # methods and streamed GETs) keep the historical four-attempt 429 allowance + # and its 1/2/4s backoff rather than spending the larger shared GET budget + # on rate limiting alone. + _MAX_RATE_LIMIT_RETRIES = 3 + _RETRYABLE_GET_STATUSES = frozenset({500, 502, 503, 504}) + _MAX_GET_RETRIES = 6 + _GET_RETRY_BACKOFF_SECONDS = 1.0 + _MAX_GET_RETRY_BACKOFF_SECONDS = 30.0 + # A single logical request may issue at most ``_MAX_GET_RETRIES + 1`` + # attempts and ``_MAX_PHYSICAL_SENDS`` physical sends -- counting every + # same-origin redirect hop -- shared across the transient-5xx, 429 + # rate-limit, and 401 auth-refresh layers, and must not spend more than + # ``_MAX_RETRY_ELAPSED_SECONDS`` retrying. One shared budget prevents the + # layers from multiplying into a large physical request count during an + # outage (e.g. interleaved 503/429 responses, a 401 mid-sequence, or a + # redirect chain in front of every retry). + # + # Sends are pooled separately from attempts because one attempt behind a + # redirect chain costs several sends. The pool is the larger of two floors: + # two full ``_MAX_REDIRECTS``-deep chains (12), so such a chain stays + # followable both before and after a 401 refresh, and two sends per logical + # attempt (14), so a one-hop gateway does not halve the effective retry + # count. The second dominates at current constants. The worst case stays far + # below attempts x chain length. + _MAX_PHYSICAL_SENDS = max(2 * (_MAX_REDIRECTS + 1), 2 * (_MAX_GET_RETRIES + 1)) + _MAX_RETRY_ELAPSED_SECONDS = 120.0 def __init__( self, @@ -121,6 +210,12 @@ def _make_request( clean_params = self._clean_mapping(params) request_method = method.upper() + budget = _RetryBudget( + self._MAX_GET_RETRIES + 1, + self._MAX_PHYSICAL_SENDS, + time.monotonic() + self._MAX_RETRY_ELAPSED_SECONDS, + ) + self._refresh_auth() response = self._request_with_rate_limit_retries( request_method, @@ -130,18 +225,28 @@ def _make_request( extra_headers=extra_headers, timeout=timeout, request_kwargs=kwargs, + budget=budget, ) - if response.status_code == 401: + # The auth-refresh retry draws from the same budget, so a 401 late in a + # transient/rate-limit sequence cannot start a fresh round of retries. + if response.status_code == 401 and budget.can_retry(): self._refresh_auth() - response = self._request_with_rate_limit_retries( - request_method, - url, - params=clean_params, - json_data=json_data, - extra_headers=extra_headers, - timeout=timeout, - request_kwargs=kwargs, - ) + try: + response = self._request_with_rate_limit_retries( + request_method, + url, + params=clean_params, + json_data=json_data, + extra_headers=extra_headers, + timeout=timeout, + request_kwargs=kwargs, + budget=budget, + ) + except requests.exceptions.RetryError: + # The retry ran out of sends before any response came back. The + # 401 already in hand is a real backend answer, so report it + # rather than an exhaustion error. + return response return response def _request_with_rate_limit_retries( @@ -154,10 +259,97 @@ def _request_with_rate_limit_retries( extra_headers: Mapping[str, str] | None, timeout: float, request_kwargs: Mapping[str, Any], + budget: _RetryBudget, ) -> requests.Response: - response: requests.Response | None = None - for attempt in range(self._MAX_RATE_LIMIT_RETRIES + 1): - response = self._request_with_same_origin_redirects( + # Replayable GETs are bounded by the shared budget alone; everything else + # bypasses the transient layer and so keeps its historical 429 cap. + max_rounds = ( + None + if self._is_transient_retryable(method, request_kwargs) + else self._MAX_RATE_LIMIT_RETRIES + ) + rate_limit_round = 0 + last_response: requests.Response | None = None + while True: + try: + response = self._request_with_transient_retries( + method, + url, + params=params, + json_data=json_data, + extra_headers=extra_headers, + timeout=timeout, + request_kwargs=request_kwargs, + budget=budget, + ) + except requests.exceptions.RetryError: + # Sends ran out part-way through a redirect chain. The 429 from + # the previous round is a real backend answer and reports better + # than the exhaustion error. + if last_response is None: + raise + return last_response + if response.status_code != 429: + return response + if max_rounds is not None and rate_limit_round >= max_rounds: + return response + delay = self._rate_limit_delay(response, rate_limit_round) + # A 429 retry re-enters the transient layer, so it must draw from the + # shared budget rather than a per-round allowance. A ``Retry-After`` + # that would outlast the deadline ends the sequence here instead of + # sleeping into a send the budget must then refuse. + if not budget.can_retry() or not budget.allows_wait(delay): + return response + # Release the superseded 429 so its connection is not held for the + # whole wait. Streamed responses are released here too: a 429 is + # never the stream the caller asked for, and nothing has read its + # body. The status stays readable afterwards, so this response is + # still reportable if a later round exhausts the budget mid-chain. + last_response = response + self._release_response(response) + self._sleep_for_rate_limit(delay) + rate_limit_round += 1 + + @staticmethod + def _is_transient_retryable(method: str, request_kwargs: Mapping[str, Any]) -> bool: + """Only non-streamed GETs may be replayed after a transient failure. + + Mutating methods must never be duplicated, and a streamed GET's consumer + owns any stream-open retries. + """ + + return method.upper() == "GET" and not request_kwargs.get("stream") + + def _request_with_transient_retries( + self, + method: str, + url: str, + *, + params: Mapping[str, Any] | None, + json_data: Any, + extra_headers: Mapping[str, str] | None, + timeout: float, + request_kwargs: Mapping[str, Any], + budget: _RetryBudget, + ) -> requests.Response: + """Retry idempotent GETs on transient 5xx and transport errors. + + Mutating methods are sent once (never duplicated). Streamed GETs bypass + this layer so their consumer owns any stream-open retries. 429s are left + to the rate-limit layer, whose retries re-enter this layer with a fresh + backoff. ``SSLError`` raises immediately: certificate failures are + deterministic, so retrying only delays the report. Every physical send + draws from the shared ``budget`` at the send boundary, so the transient, + rate-limit, and auth-refresh layers cannot multiply into a large request + count. Each doubling sleep is capped at + ``_MAX_GET_RETRY_BACKOFF_SECONDS``, is skipped entirely when it would + outlast the deadline, and is announced through ``self.logger`` (a null + logger on non-verbose clients built without one), so a stalled GET is + bounded. + """ + + if not self._is_transient_retryable(method, request_kwargs): + return self._request_with_same_origin_redirects( method, url, params=params, @@ -165,18 +357,82 @@ def _request_with_rate_limit_retries( extra_headers=extra_headers, timeout=timeout, request_kwargs=request_kwargs, + budget=budget, ) - if response.status_code != 429 or attempt == self._MAX_RATE_LIMIT_RETRIES: - return response - self._sleep_for_rate_limit(response, attempt) - return response - def _sleep_for_rate_limit(self, response: requests.Response, attempt: int) -> None: + backoff = self._GET_RETRY_BACKOFF_SECONDS + attempt = 0 + last_response: requests.Response | None = None + while True: + attempt += 1 + delay = min(backoff, self._MAX_GET_RETRY_BACKOFF_SECONDS) + backoff *= 2.0 + try: + response = self._request_with_same_origin_redirects( + method, + url, + params=params, + json_data=json_data, + extra_headers=extra_headers, + timeout=timeout, + request_kwargs=request_kwargs, + budget=budget, + ) + except requests.exceptions.RetryError: + # Sends ran out part-way through a redirect chain. A completed + # 5xx from an earlier attempt tells the caller what the backend + # actually said, so ``raise_for_status`` still reports the true + # status instead of an exhaustion error. + if last_response is None: + raise + return last_response + # Transient transport failures (reset/refused, timeout, truncated or + # corrupt body) can succeed on retry; other request errors surface. + except ( + requests.ConnectionError, + requests.Timeout, + requests.exceptions.ChunkedEncodingError, + requests.exceptions.ContentDecodingError, + ) as exc: + # SSLError subclasses ConnectionError but signals a certificate + # or TLS configuration problem that no retry can fix. + if isinstance(exc, requests.exceptions.SSLError): + raise + # Budget exhausted (attempts or deadline), or the backoff alone + # would outlast the deadline: surface the failure. + if not budget.can_retry() or not budget.allows_wait(delay): + raise + self._sleep_for_transient_retry(delay, attempt, type(exc).__name__) + else: + if response.status_code not in self._RETRYABLE_GET_STATUSES: + return response + # Budget exhausted, or the backoff alone would outlast the + # deadline: return the final 5xx for raise_for_status. + if not budget.can_retry() or not budget.allows_wait(delay): + return response + # Release the intermediate response so its connection returns to + # the pool. Its body is already buffered (these GETs are never + # streamed), so it stays reportable if the retry runs out of + # sends mid-chain. + last_response = response + self._release_response(response) + self._sleep_for_transient_retry(delay, attempt, f"HTTP {response.status_code}") + + def _sleep_for_transient_retry(self, delay: float, attempt: int, reason: str) -> None: + self.logger.warn( + f"transient {reason} on GET; retrying in {delay:.1f}s " + f"(attempt {attempt + 1}/{self._MAX_GET_RETRIES + 1})" + ) + time.sleep(delay) + + def _rate_limit_delay(self, response: requests.Response, attempt: int) -> float: retry_after = response.headers.get("Retry-After") delay = self._retry_after_delay(retry_after) if delay is None: delay = self._RATE_LIMIT_BACKOFF_SECONDS * (2 ** attempt) - delay = min(delay, self._MAX_RETRY_AFTER_SECONDS) + return min(delay, self._MAX_RETRY_AFTER_SECONDS) + + def _sleep_for_rate_limit(self, delay: float) -> None: if self.verbose: self.logger.info(f"429 rate limited; retrying in {delay:.1f}s") time.sleep(delay) @@ -207,6 +463,7 @@ def _request_with_same_origin_redirects( extra_headers: Mapping[str, str] | None, timeout: float, request_kwargs: Mapping[str, Any], + budget: _RetryBudget, ) -> requests.Response: """Send one request, following only same-origin redirects. @@ -214,8 +471,16 @@ def _request_with_same_origin_redirects( ``requests`` does not strip those custom credentials on cross-origin redirects, so redirects are handled manually and constrained to the original origin. + + This helper is entered once per logical attempt, so it charges the + shared budget one attempt on entry and one send per hop immediately + before that hop goes out. Charging hops keeps a redirect in front of + every retry from multiplying the total request count, while charging + them against the send pool rather than the attempt count leaves a legal + chain followable on every attempt. """ + budget.start_attempt() current_method = method current_url = url current_params = params @@ -223,6 +488,11 @@ def _request_with_same_origin_redirects( response: requests.Response | None = None for _ in range(self._MAX_REDIRECTS + 1): + if not budget.try_consume_send(): + raise requests.exceptions.RetryError( + f"Retry budget exhausted ({budget.exhaustion_reason()}) " + f"while sending {current_method} {self._credential_safe_url(current_url)}" + ) request_headers = self._headers(extra_headers) response = self.session.request( current_method, @@ -259,10 +529,7 @@ def _request_with_same_origin_redirects( response=response, ) - try: - response.close() - except Exception: - pass + self._release_response(response) if response.status_code == 303 or ( response.status_code in {301, 302} and current_method not in {"GET", "HEAD"} ): @@ -276,6 +543,35 @@ def _request_with_same_origin_redirects( response=response, ) + @staticmethod + def _credential_safe_url(url: str) -> str: + """Render a URL for error text with every credential-bearing part removed. + + A same-origin redirect can land on a signed URL whose query, fragment, + or authority userinfo carries credentials, and this rendering flows + into CLI output and logs. Only scheme, host[:port], and path survive. + ``urlsplit`` rejects some malformed authorities (an unclosed IPv6 + bracket, for one), and ``.hostname``/``.port`` reject malformed ports, + so the authority is taken from the raw netloc and parse failures fall + back to a placeholder rather than letting error formatting raise. + """ + + try: + parts = urlsplit(url) + except ValueError: + return "" + netloc = parts.netloc.rpartition("@")[2] + return urlunsplit((parts.scheme, netloc, parts.path, "", "")) or "" + + @staticmethod + def _release_response(response: requests.Response) -> None: + """Return an abandoned response's connection to the pool.""" + + try: + response.close() + except Exception: + pass + @staticmethod def _same_origin(left: str, right: str) -> bool: left_parts = urlparse(left) diff --git a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py index 2da0eb5..5394f72 100644 --- a/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py +++ b/packages/tangle-cli/src/tangle_cli/pipeline_runs_cli.py @@ -69,7 +69,9 @@ def _allow_all_hydration_for_args(args: ArgsContainer) -> bool: return bool(config.get("allow_all", False)) -def _api_client(args: ArgsContainer, *, cli_base_url: str | None, command_name: str) -> LazyTangleApiClient: +def _api_client( + args: ArgsContainer, *, cli_base_url: str | None, command_name: str, logger: Logger | None = None +) -> LazyTangleApiClient: return LazyTangleApiClient( base_url=args.base_url, token=args.token, @@ -77,12 +79,15 @@ def _api_client(args: ArgsContainer, *, cli_base_url: str | None, command_name: header=args.header, include_env_credentials=include_env_credentials_for_args(args, cli_base_url), command_name=command_name, + logger=logger, ) def _manager(args: ArgsContainer, *, cli_base_url: str | None, logger: Logger) -> PipelineRunManager: return PipelineRunManager( - client=_api_client(args, cli_base_url=cli_base_url, command_name="pipeline-run commands"), + client=_api_client( + args, cli_base_url=cli_base_url, command_name="pipeline-run commands", logger=logger + ), hooks=PipelineRunHooks( logger=logger, trusted_python_sources=_trusted_sources_for_args(args), @@ -117,7 +122,12 @@ def _run_annotation_action(config: str | None, cli_base_url: str | None, specs: raise SystemExit(str(exc)) from exc try: manager = AnnotationManager( - client=_api_client(args, cli_base_url=cli_base_url, command_name="pipeline-run annotation commands"), + client=_api_client( + args, + cli_base_url=cli_base_url, + command_name="pipeline-run annotation commands", + logger=logger, + ), logger=logger, ) print_json(fn(manager, args)) diff --git a/packages/tangle-cli/src/tangle_cli/pipelines_cli.py b/packages/tangle-cli/src/tangle_cli/pipelines_cli.py index 3a30e57..4ce9006 100644 --- a/packages/tangle-cli/src/tangle_cli/pipelines_cli.py +++ b/packages/tangle-cli/src/tangle_cli/pipelines_cli.py @@ -232,6 +232,7 @@ def pipelines_hydrate( ), header=_header_entries(header, config_values), include_env_credentials=include_env_credentials, + logger=logger, ), ) except PipelineValidationError as exc: diff --git a/packages/tangle-cli/src/tangle_cli/published_components_cli.py b/packages/tangle-cli/src/tangle_cli/published_components_cli.py index de4f70a..733eb02 100644 --- a/packages/tangle-cli/src/tangle_cli/published_components_cli.py +++ b/packages/tangle-cli/src/tangle_cli/published_components_cli.py @@ -24,7 +24,7 @@ TokenOption, ) from .component_publisher import ComponentPublisher, deprecate_component -from .logger import logger_for_log_type +from .logger import Logger, logger_for_log_type def _client_from_options( @@ -35,6 +35,7 @@ def _client_from_options( header: list[str] | str | None = None, include_env_credentials: bool = True, command_name: str = "published-component commands", + logger: Logger | None = None, ) -> LazyTangleApiClient: """Create a lazy static client proxy for published-component commands. @@ -49,6 +50,7 @@ def _client_from_options( header=header, include_env_credentials=include_env_credentials, command_name=command_name, + logger=logger, ) @@ -97,6 +99,7 @@ def published_components_search( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="published-component commands", + logger=logger, ) if require_available := getattr(client, "require_available", None): require_available() @@ -163,6 +166,7 @@ def published_components_inspect( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="published-component commands", + logger=logger, ) if require_available := getattr(client, "require_available", None): require_available() @@ -219,6 +223,7 @@ def published_components_library( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="published-component commands", + logger=logger, ) if require_available := getattr(client, "require_available", None): require_available() @@ -288,6 +293,7 @@ def published_components_publish( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="published-component commands", + logger=logger, ) publisher = ComponentPublisher( dry_run=bool(args.dry_run), @@ -358,6 +364,7 @@ def published_components_deprecate( header=args.header, include_env_credentials=include_env_credentials_for_args(args, base_url), command_name="published-component commands", + logger=logger, ) result = deprecate_component( client, diff --git a/packages/tangle-cli/src/tangle_cli/secrets_cli.py b/packages/tangle-cli/src/tangle_cli/secrets_cli.py index d6c2da1..48ba9ff 100644 --- a/packages/tangle-cli/src/tangle_cli/secrets_cli.py +++ b/packages/tangle-cli/src/tangle_cli/secrets_cli.py @@ -58,7 +58,9 @@ app = App(name="secrets", help="Manage Tangle secrets.") -def _client(args: ArgsContainer, *, cli_base_url: str | None, command_name: str) -> LazyTangleApiClient: +def _client( + args: ArgsContainer, *, cli_base_url: str | None, command_name: str, logger: Logger | None = None +) -> LazyTangleApiClient: return LazyTangleApiClient( base_url=args.base_url, token=args.token, @@ -66,6 +68,7 @@ def _client(args: ArgsContainer, *, cli_base_url: str | None, command_name: str) header=args.header, include_env_credentials=include_env_credentials_for_args(args, cli_base_url), command_name=command_name, + logger=logger, ) @@ -79,7 +82,7 @@ def _run_secret_action( for args in load_args_or_exit(config, **specs): logger, finalize_logs = logger_for_log_type(getattr(args, "log_type", "console")) try: - client = _client(args, cli_base_url=cli_base_url, command_name="secret commands") + client = _client(args, cli_base_url=cli_base_url, command_name="secret commands", logger=logger) try: results.append(fn(client, args, logger)) except SecretValueError as exc: diff --git a/tests/test_api_cli.py b/tests/test_api_cli.py index c93a575..8993cc6 100644 --- a/tests/test_api_cli.py +++ b/tests/test_api_cli.py @@ -1,6 +1,7 @@ import importlib import json import sys +from unittest.mock import ANY import httpx import pytest @@ -460,6 +461,7 @@ def fake_client_from_options(**kwargs): "header": ["X-Config: yes"], "include_env_credentials": False, "command_name": "published-component commands", + "logger": ANY, } diff --git a/tests/test_artifacts_cli.py b/tests/test_artifacts_cli.py index b7b3ec1..f635497 100644 --- a/tests/test_artifacts_cli.py +++ b/tests/test_artifacts_cli.py @@ -5,6 +5,7 @@ import json import sys from typing import Any +from unittest.mock import ANY from tangle_cli import artifacts as artifacts_module from tangle_cli import artifacts_cli, cli @@ -80,6 +81,7 @@ def fake_get_artifacts(self, run_id: str, query: dict[str, Any]) -> dict[str, ob "header": ["X-Config: yes"], "include_env_credentials": False, "command_name": "artifact commands", + "logger": ANY, } ] assert get_calls == [ diff --git a/tests/test_client.py b/tests/test_client.py index 5456ac0..243eab1 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,13 +1,46 @@ from __future__ import annotations +import json from types import SimpleNamespace +from typing import Any from unittest.mock import MagicMock +import pytest +import requests + import tangle_cli.client as client_module from tangle_cli.client import TangleApiClient +from tangle_cli.logger import CaptureLogger from tangle_cli.models import ComponentInfo +def _response(payload: Any = None, status_code: int = 200) -> requests.Response: + r = requests.Response() + r.status_code = status_code + if payload is None: + r._content = b"" + else: + r._content = json.dumps(payload).encode("utf-8") + r.headers["Content-Type"] = "application/json" + r.request = requests.Request("GET", "https://api.test").prepare() + return r + + +class _FakeSession: + def __init__(self, responses: list[requests.Response | Exception] | None = None) -> None: + self.calls: list[dict[str, Any]] = [] + self.responses = responses or [] + + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + self.calls.append({"method": method, "url": url, **kwargs}) + if self.responses: + next_response = self.responses.pop(0) + if isinstance(next_response, Exception): + raise next_response + return next_response + return _response({}) + + def test_find_existing_components_matches_exact_names_case_insensitively() -> None: client = TangleApiClient("https://api.test") client.list_published_component_infos = MagicMock( @@ -53,3 +86,924 @@ def fail_from_dict(*args, **kwargs): assert client.get_run_pipeline_spec("run-1") is task_spec client.executions_details.assert_called_once_with("root-exec-1") + + +def test_get_retries_transient_5xx_then_succeeds(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + ok = _response({"ok": True}) + session = _FakeSession([_response(status_code=503), _response(status_code=500), ok]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is ok + assert len(session.calls) == 3 + assert sleeps == [1.0, 2.0] + + +def test_get_closes_intermediate_5xx_responses_before_retrying(monkeypatch) -> None: + events: list[str] = [] + + def tracking_response(status_code: int, marker: str) -> requests.Response: + r = _response(status_code=status_code) + r.close = lambda: events.append(f"close-{marker}") # type: ignore[method-assign] + return r + + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: events.append("sleep")) + session = _FakeSession( + [tracking_response(503, "1"), tracking_response(500, "2"), tracking_response(200, "3")] + ) + client = TangleApiClient("https://api.test", session=session) + + client._make_request("GET", "/api/test") + + # Intermediate 5xx are closed before each retry; the returned one is left open. + assert events == ["close-1", "sleep", "close-2", "sleep"] + + +def test_get_retries_transport_error_then_succeeds(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + ok = _response({"ok": True}) + session = _FakeSession( + [ + requests.ConnectionError("connection reset"), + requests.Timeout("read timed out"), + requests.exceptions.ChunkedEncodingError("incomplete chunked read"), + requests.exceptions.ContentDecodingError("failed to decode gzip stream"), + ok, + ] + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is ok + assert len(session.calls) == 5 + assert sleeps == [1.0, 2.0, 4.0, 8.0] + + +def test_get_raises_after_exhausting_transport_retries(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + budget = TangleApiClient._MAX_GET_RETRIES + final_attempt_error = requests.exceptions.ChunkedEncodingError("final permitted attempt") + surplus = [requests.ConnectionError("never reached") for _ in range(3)] + queued = [requests.ConnectionError("blip") for _ in range(budget)] + [final_attempt_error] + surplus + assert len(queued) > budget + 1 + session = _FakeSession(queued) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.ChunkedEncodingError) as exc_info: + client._make_request("GET", "/api/test") + + assert exc_info.value is final_attempt_error + assert len(session.calls) == budget + 1 + assert len(session.responses) == len(surplus) + + +def test_get_returns_final_5xx_after_exhausting_status_retries(monkeypatch) -> None: + closed: list[str] = [] + + def tracking_5xx(marker: str) -> requests.Response: + r = _response(status_code=503) + r.close = lambda: closed.append(marker) # type: ignore[method-assign] + return r + + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + budget = TangleApiClient._MAX_GET_RETRIES + errors = [tracking_5xx(str(i)) for i in range(budget + 1)] + trailing_ok = _response({"ok": True}) + session = _FakeSession([*errors, trailing_ok]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is errors[budget] + assert len(session.calls) == budget + 1 + assert closed == [str(i) for i in range(budget)] + assert session.responses == [trailing_ok] + + +def test_post_is_not_retried_on_transient_5xx(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + server_error = _response(status_code=503) + session = _FakeSession([server_error, _response({"ok": True})]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("POST", "/api/pipeline_runs/", json_data={"a": 1}) + + assert result is server_error + assert len(session.calls) == 1 + assert sleeps == [] + + +def test_streamed_get_bypasses_transient_retry(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + server_error = _response(status_code=503) + session = _FakeSession([server_error, _response({"ok": True})]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/logs", stream=True) + + assert result is server_error + assert len(session.calls) == 1 + assert sleeps == [] + + +def test_transient_retry_decision_is_method_case_insensitive(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + def call(method: str) -> tuple[requests.Response, int]: + ok = _response({"ok": True}) + session = _FakeSession([_response(status_code=503), ok]) + client = TangleApiClient("https://api.test", session=session) + budget = client_module._RetryBudget( + client._MAX_GET_RETRIES + 1, + client._MAX_PHYSICAL_SENDS, + client_module.time.monotonic() + client._MAX_RETRY_ELAPSED_SECONDS, + ) + result = client._request_with_transient_retries( + method, + "https://api.test/api/test", + params=None, + json_data=None, + extra_headers=None, + timeout=client.timeout, + request_kwargs={}, + budget=budget, + ) + return result, len(session.calls) + + get_result, get_calls = call("get") + assert get_result.status_code == 200 + assert get_calls == 2 + + post_result, post_calls = call("post") + assert post_result.status_code == 503 + assert post_calls == 1 + + +def test_get_retries_proxy_errors(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + ok = _response({"ok": True}) + session = _FakeSession( + [ + requests.exceptions.ProxyError("proxy refused"), + ok, + ] + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is ok + assert len(session.calls) == 2 + assert sleeps == [1.0] + + +def test_get_does_not_retry_ssl_errors(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + error = requests.exceptions.SSLError("certificate verify failed") + session = _FakeSession([error, _response({"ok": True})]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.SSLError) as exc_info: + client._make_request("GET", "/api/test") + + assert exc_info.value is error + assert len(session.calls) == 1 + assert sleeps == [] + + +def test_get_transient_and_rate_limit_retry_layers_compose(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + ok = _response({"ok": True}) + session = _FakeSession( + [ + _response(status_code=503), + _response(status_code=429), + _response(status_code=503), + ok, + ] + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is ok + assert len(session.calls) == 4 + # transient 1.0s, rate-limit 1.0s (no Retry-After), fresh transient 1.0s + assert sleeps == [1.0, 1.0, 1.0] + + +def test_get_retry_sleeps_are_capped_and_announced_without_verbose(monkeypatch) -> None: + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + budget = TangleApiClient._MAX_GET_RETRIES + session = _FakeSession([_response(status_code=503) for _ in range(budget + 1)]) + logger = CaptureLogger() + client = TangleApiClient("https://api.test", session=session, logger=logger) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 503 + assert sleeps == [1.0, 2.0, 4.0, 8.0, 16.0, 30.0] # final sleep capped, not 32.0 + messages = (logger.get_logs() or "").splitlines() + assert len(messages) == budget + assert all(m.startswith("transient HTTP 503 on GET; retrying in ") for m in messages) + assert messages[-1] == "transient HTTP 503 on GET; retrying in 30.0s (attempt 7/7)" + + +def test_get_retries_are_silent_on_default_non_verbose_client(monkeypatch, capsys) -> None: + # A non-verbose client built without a logger stays silent; callers that + # want retry announcements pass a logger (as the CLI command layer does). + monkeypatch.delenv("TANGLE_VERBOSE", raising=False) + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + session = _FakeSession([_response(status_code=503), _response({"ok": True})]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 200 + captured = capsys.readouterr() + assert captured.out == "" + assert captured.err == "" + + +def test_shared_budget_caps_total_requests_across_transient_and_rate_limit(monkeypatch) -> None: + # Interleaved 503/429 responses must not let the rate-limit layer hand the + # transient layer a fresh budget each round: the total physical request + # count is bounded by the single shared budget, not their product. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + budget = TangleApiClient._MAX_GET_RETRIES + # Far more responses than the budget allows, alternating retryable states. + session = _FakeSession([_response(status_code=503 if i % 2 == 0 else 429) for i in range(40)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + # Exactly the advertised budget of physical requests is spent, then the last + # response surfaces for the caller's raise_for_status (no amplification). + assert len(session.calls) == budget + 1 + assert result.status_code in {503, 429} + + +def test_auth_refresh_shares_transient_retry_budget(monkeypatch) -> None: + # A 401 that triggers an auth refresh must continue on the same budget + # rather than starting a fresh transient-retry round. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + + budget = TangleApiClient._MAX_GET_RETRIES + # One 401 (consumes a request) followed by an unbroken run of 503s. + session = _FakeSession( + [_response(status_code=401)] + [_response(status_code=503) for _ in range(budget + 5)] + ) + client = RefreshingClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 503 + # Refresh fired once for the 401 (plus the unconditional pre-request refresh). + assert client.refreshes == 2 + # The 401 request plus the post-refresh retries share one budget: the total + # never exceeds the shared cap (a fresh budget would allow budget+1 more). + assert len(session.calls) == budget + 1 + + +def test_shared_budget_caps_total_requests_across_transient_rate_limit_and_auth(monkeypatch) -> None: + # The worst case the reviewer flagged: a 401 auth refresh, 429 rate limits, + # and transient 503s all interleaved for one logical GET. A single shared + # budget must bound the total physical request count instead of letting the + # three layers multiply their per-layer limits together. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + + budget = TangleApiClient._MAX_GET_RETRIES + # Lead with a 401 (drives one refresh), then alternate 429/503 far past the + # budget so the cap, not the response list, is what stops the retries. + responses = [_response(status_code=401)] + responses += [_response(status_code=429 if i % 2 == 0 else 503) for i in range(40)] + session = _FakeSession(responses) + client = RefreshingClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + # Exactly the advertised shared budget of physical requests is spent. + assert len(session.calls) == budget + 1 + assert result.status_code in {429, 503} + # Pre-request refresh plus exactly one refresh for the single 401; the 401's + # retry continues on the shared budget rather than opening a fresh round. + assert client.refreshes == 2 + + +def _redirect(url: str, location: str, status_code: int = 307) -> requests.Response: + r = _response(status_code=status_code) + r.url = url + r.headers["Location"] = location + return r + + +def _redirect_chain(hops: int, final: requests.Response) -> list[requests.Response]: + """A ``hops``-deep same-origin 307 chain ending in ``final``.""" + + return [ + _redirect(f"https://api.test/api/hop{hop}", f"/api/hop{hop + 1}") for hop in range(hops) + ] + [final] + + +class _ClockAdvancingSession(_FakeSession): + """Session whose sends are the only thing that advances the fake clock. + + Deadline behaviour is then a pure function of the response sequence, with no + dependence on how fast the test host runs. + """ + + def __init__( + self, + responses: list[requests.Response | Exception], + clock: SimpleNamespace, + step: float, + ) -> None: + super().__init__(responses) + self.clock = clock + self.step = step + self.send_times: list[float] = [] + + def request(self, method: str, url: str, **kwargs: Any) -> requests.Response: + self.send_times.append(self.clock.now) + response = super().request(method, url, **kwargs) + self.clock.now += self.step + return response + + +def test_shared_budget_deadline_halts_composed_retries_without_wallclock(monkeypatch) -> None: + # The shared budget bounds retries by BOTH an attempt count and a + # _MAX_RETRY_ELAPSED_SECONDS wall-time deadline. This pins the deadline + # clause deterministically: a fake monotonic clock advances only when a + # request is sent (never the real wall clock), so the elapsed-time cap, not + # the attempt cap, is what stops the composed auth-refresh / rate-limit / + # transient retry sequence. Without this every other budget test would still + # pass on the attempt cap alone, so the deadline could be removed silently. + clock = SimpleNamespace(now=1_000.0) + step = 50.0 + window = TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + deadline = clock.now + window + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + + sleep_times: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: sleep_times.append(clock.now)) + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + + # A 401 (auth-refresh path), a 429 (rate-limit path), then unbroken 503s + # (transient path): all three layers draw on the one shared budget. Far more + # responses are queued than either cap allows, so the cap that fires first is + # what stops the sequence. + responses = [_response(status_code=401), _response(status_code=429)] + responses += [_response(status_code=503) for _ in range(20)] + session = _ClockAdvancingSession(responses, clock, step) + client = RefreshingClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + # The sequence genuinely retried across layers but stopped short of the + # attempt cap with responses still queued: the deadline, not the attempt + # count or the response list, is what ended it. + attempt_cap = TangleApiClient._MAX_GET_RETRIES + 1 + assert 1 < len(session.calls) < attempt_cap + assert session.responses, "unused responses prove the queue did not stop the retries" + # Time actually crossed the deadline, yet no send or sleep happened at/after + # it: can_retry gates every physical send and every sleep across the + # transient and rate-limit layers. + assert clock.now >= deadline + assert all(t < deadline for t in session.send_times) + assert all(t < deadline for t in sleep_times) + # The composed auth path ran on the shared budget (pre-request refresh plus + # one for the single 401), and the final 5xx surfaces for the caller. + assert client.refreshes == 2 + assert result.status_code == 503 + + +def test_long_retry_after_is_not_slept_when_it_would_cross_the_deadline(monkeypatch) -> None: + # A Retry-After longer than the time left on the shared deadline must end the + # sequence immediately. Sleeping it out would only be followed by a send the + # budget is then obliged to refuse, so the wait is pure dead time. + clock = SimpleNamespace(now=1_000.0) + deadline = clock.now + TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + + rate_limited = _response(status_code=429) + rate_limited.headers["Retry-After"] = "60" + # One send burns most of the window, leaving less than the 60s Retry-After. + session = _ClockAdvancingSession( + [rate_limited] + [_response({"ok": True}) for _ in range(3)], clock, step=80.0 + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result is rate_limited + assert len(session.calls) == 1 + assert sleeps == [] + # Attempts and the deadline itself both still had room: only the fact that the + # wait would have crossed the deadline stopped the retry. + assert clock.now < deadline + assert deadline - clock.now < 60.0 + + +def test_transient_backoff_that_would_cross_the_deadline_stops_retrying(monkeypatch) -> None: + # Same rule on the transient-5xx layer: once the doubling backoff no longer + # fits inside the remaining deadline, the final 5xx surfaces instead of the + # client sleeping into a send it cannot make. + clock = SimpleNamespace(now=1_000.0) + deadline = clock.now + TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + + session = _ClockAdvancingSession( + [_response(status_code=503) for _ in range(20)], clock, step=39.9 + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 503 + # Two backoffs fitted in the window (1s, then 2s); the third would have been + # 4s with only 0.3s left, so the sequence ends there. + assert sleeps == [1.0, 2.0] + assert len(session.calls) == 3 + # The attempt cap was nowhere near reached and the clock had not yet passed + # the deadline, so neither of those is what stopped it. + assert len(session.calls) < TangleApiClient._MAX_GET_RETRIES + 1 + assert clock.now < deadline + assert session.responses, "unused responses prove the queue did not stop the retries" + + +def test_redirect_hops_are_charged_to_the_shared_send_budget(monkeypatch) -> None: + # Every physical send counts, including same-origin redirect hops. Without + # charging them, a 307 in front of each 503 would double the number of + # requests an outage can provoke (and a full 5-hop chain would multiply it + # sixfold). Hops are charged to the send pool rather than to the attempt + # count, so the chain is still followed on every attempt. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + responses: list[requests.Response] = [] + for _ in range(20): + responses.append(_redirect("https://api.test/api/test", "/api/moved")) + responses.append(_response(status_code=503)) + session = _FakeSession(responses) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + # Seven logical attempts of two sends each spend the pool exactly, and the + # real 503 surfaces instead of a budget-exhaustion error. + assert result.status_code == 503 + assert len(session.calls) == TangleApiClient._MAX_PHYSICAL_SENDS == 14 + assert [call["url"] for call in session.calls] == [ + "https://api.test/api/test", + "https://api.test/api/moved", + ] * 7 + + +@pytest.mark.parametrize("method", ["GET", "POST"]) +@pytest.mark.parametrize("hops", range(TangleApiClient._MAX_REDIRECTS + 1)) +def test_legal_redirect_chain_survives_an_auth_refresh(monkeypatch, hops, method) -> None: + # The client advertises support for chains up to _MAX_REDIRECTS deep, and a + # 401 refresh replays the whole request. Both together are ordinary healthy + # traffic -- no outage -- so every depth must still reach the backend. This + # is why physical sends are pooled separately from logical attempts: charging + # hops against the attempt count would fail depth 3 and beyond outright. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + + ok = _response({"ok": True}) + responses = _redirect_chain(hops, _response(status_code=401)) + responses += _redirect_chain(hops, ok) + session = _FakeSession(responses) + client = RefreshingClient("https://api.test", session=session) + + result = client._make_request( + method, "/api/test", json_data={"a": 1} if method == "POST" else None + ) + + assert result is ok + # Pre-request refresh plus exactly one for the 401. + assert client.refreshes == 2 + assert len(session.calls) == 2 * (hops + 1) + assert not session.responses + # Two full legal chains is one of the two floors the send pool is sized to. + assert 2 * (hops + 1) <= TangleApiClient._MAX_PHYSICAL_SENDS + + +def test_worst_case_redirect_chain_stays_within_the_physical_send_cap(monkeypatch) -> None: + # The reviewer's amplification concern, at its worst: a full-depth chain in + # front of every retryable 5xx. The send pool, not the chain length, is what + # bounds the total. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + hops = TangleApiClient._MAX_REDIRECTS + + responses: list[requests.Response] = [] + for _ in range(10): + responses += _redirect_chain(hops, _response(status_code=503)) + session = _FakeSession(responses) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert len(session.calls) == TangleApiClient._MAX_PHYSICAL_SENDS == 14 + # Far below the attempts x chain-length product an uncharged chain would allow. + assert len(session.calls) < (TangleApiClient._MAX_GET_RETRIES + 1) * (hops + 1) + # The pool runs out part-way through the third chain, but a completed 503 + # from the second is still the truthful answer for the caller. + assert result.status_code == 503 + with pytest.raises(requests.HTTPError): + result.raise_for_status() + + +def test_redirect_then_5xx_reports_the_backend_status_not_budget_exhaustion(monkeypatch) -> None: + # End to end through a public operation: a 307 in front of a 503 must still + # be reported as HTTP 503. Replacing the completed response with a + # RetryError would hide the real backend failure from raise_for_status -- + # and RetryError is not an HTTPError, so the 404 fallbacks would not see it + # either. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + responses: list[requests.Response] = [] + for _ in range(10): + responses += _redirect_chain(TangleApiClient._MAX_REDIRECTS, _response(status_code=503)) + session = _FakeSession(responses) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.HTTPError) as exc_info: + client.pipeline_runs_get("run-1") + + assert exc_info.value.response is not None + assert exc_info.value.response.status_code == 503 + + +def test_direct_get_5xx_still_stops_at_the_logical_attempt_cap(monkeypatch) -> None: + # The send pool is deliberately larger than the attempt count so redirect + # chains fit. It must not become extra retries for a request that never + # redirects. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + session = _FakeSession([_response(status_code=503) for _ in range(20)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 503 + assert len(session.calls) == TangleApiClient._MAX_GET_RETRIES + 1 == 7 + assert len(session.calls) < TangleApiClient._MAX_PHYSICAL_SENDS + + +def test_deadline_hit_mid_first_redirect_chain_raises_a_clean_retry_error(monkeypatch) -> None: + # Exhaustion inside the very first chain has no completed backend response to + # fall back on, so the exhaustion error itself is the honest answer. It must + # stay a requests-family error and must not be an HTTPError, or the 404 + # fallbacks in the public helpers would inspect a response that is not there. + clock = SimpleNamespace(now=1_000.0) + deadline = clock.now + TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + responses = _redirect_chain(TangleApiClient._MAX_REDIRECTS, _response({"ok": True})) + session = _ClockAdvancingSession(responses, clock, step=50.0) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.RetryError, match="Retry budget exhausted"): + client._make_request("GET", "/api/test") + + # Three hops fitted inside the window; the fourth was refused at the send + # boundary, before the request went out. + assert len(session.calls) == 3 + assert clock.now >= deadline + assert all(t < deadline for t in session.send_times) + assert not issubclass(requests.exceptions.RetryError, requests.HTTPError) + + +def test_post_429_keeps_legacy_four_attempt_cap_and_backoff(monkeypatch) -> None: + # POSTs never enter the replay-on-5xx layer, so they keep the historical + # four-attempt 429 allowance and its 1/2/4s backoff rather than spending the + # larger shared GET budget on rate limiting alone. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + session = _FakeSession([_response(status_code=429) for _ in range(10)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("POST", "/api/test", json_data={"a": 1}) + + assert result.status_code == 429 + assert len(session.calls) == TangleApiClient._MAX_RATE_LIMIT_RETRIES + 1 == 4 + assert sleeps == [1.0, 2.0, 4.0] + + +def test_streamed_get_429_keeps_legacy_four_attempt_cap_and_backoff(monkeypatch) -> None: + # A streamed GET is not replayable either (its consumer owns stream-open + # retries), so it gets the same legacy 429 treatment as a POST. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + session = _FakeSession([_response(status_code=429) for _ in range(10)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test", stream=True) + + assert result.status_code == 429 + assert len(session.calls) == TangleApiClient._MAX_RATE_LIMIT_RETRIES + 1 == 4 + assert sleeps == [1.0, 2.0, 4.0] + + +def test_ordinary_get_429_uses_the_shared_budget_not_the_legacy_cap(monkeypatch) -> None: + # The counterpart to the two tests above: a replayable GET still gets the + # full shared budget, so restoring the legacy cap did not narrow it. Its + # doubling backoff is capped by _MAX_RETRY_AFTER_SECONDS (60s), which 32s + # never reaches, so the worst-case wall time is 63s over seven sends. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + session = _FakeSession([_response(status_code=429) for _ in range(20)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 429 + assert len(session.calls) == TangleApiClient._MAX_GET_RETRIES + 1 == 7 + assert sleeps == [1.0, 2.0, 4.0, 8.0, 16.0, 32.0] + assert sum(sleeps) == 63.0 + assert max(sleeps) <= TangleApiClient._MAX_RETRY_AFTER_SECONDS + + +def test_ordinary_get_5xx_backoff_is_capped_lower_than_the_429_backoff(monkeypatch) -> None: + # Same seven sends as the 429 case, but the transient layer clamps at + # _MAX_GET_RETRY_BACKOFF_SECONDS (30s), so the final wait is 30s not 32s and + # the worst-case wall time is 61s. Both stay inside the 120s deadline. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + session = _FakeSession([_response(status_code=503) for _ in range(20)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test") + + assert result.status_code == 503 + assert len(session.calls) == TangleApiClient._MAX_GET_RETRIES + 1 == 7 + assert sleeps == [1.0, 2.0, 4.0, 8.0, 16.0, 30.0] + assert sum(sleeps) == 61.0 + assert max(sleeps) == TangleApiClient._MAX_GET_RETRY_BACKOFF_SECONDS + assert sum(sleeps) < TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + + +@pytest.mark.parametrize("stream", [False, True], ids=["plain", "streamed"]) +def test_intermediate_429_responses_are_closed_but_the_final_one_is_usable( + monkeypatch, stream: bool +) -> None: + # A superseded 429 holds a pooled connection for the whole backoff if it is + # never released, and a streamed one is never read at all. Only the response + # actually handed back to the caller must stay open. + closed: list[str] = [] + + def tracking_response(status_code: int, marker: str, payload: Any = None) -> requests.Response: + r = _response(payload, status_code=status_code) + r.close = lambda: closed.append(marker) # type: ignore[method-assign] + return r + + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + session = _FakeSession( + [ + tracking_response(429, "429-1"), + tracking_response(429, "429-2"), + tracking_response(200, "final", {"ok": True}), + ] + ) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("GET", "/api/test", stream=stream) + + assert closed == ["429-1", "429-2"] + assert result.status_code == 200 + assert result.json() == {"ok": True} + + +def test_a_returned_429_is_not_closed_when_the_rate_limit_rounds_run_out(monkeypatch) -> None: + # The counterpart to the test above on the give-up path: the last 429 is the + # answer, so it must survive for the caller to inspect. + closed: list[str] = [] + + def tracking_response(marker: str) -> requests.Response: + r = _response({"detail": marker}, status_code=429) + r.close = lambda: closed.append(marker) # type: ignore[method-assign] + return r + + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + session = _FakeSession([tracking_response(f"r{i}") for i in range(4)]) + client = TangleApiClient("https://api.test", session=session) + + result = client._make_request("POST", "/api/test", json_data={"a": 1}) + + assert closed == ["r0", "r1", "r2"] + assert result.status_code == 429 + assert result.json() == {"detail": "r3"} + + +def test_retry_error_names_the_deadline_rather_than_the_send_pool(monkeypatch) -> None: + # Both exhaustion causes stop a send at the same boundary; the message has to + # say which one, or a slow-backend incident reads as a redirect-loop bug. + clock = SimpleNamespace(now=1_000.0) + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + responses = _redirect_chain(TangleApiClient._MAX_REDIRECTS, _response({"ok": True})) + session = _ClockAdvancingSession(responses, clock, step=50.0) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.RetryError, match="deadline exceeded"): + client._make_request("GET", "/api/test") + + +def test_retry_error_names_the_send_pool_when_the_deadline_is_untouched(monkeypatch) -> None: + # The other branch of the message, with the clock standing still so only the + # pool can be the cause. The real pool is sized to outlast any single chain, + # so it is narrowed here to reach the branch at all. + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: 1_000.0) + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + monkeypatch.setattr(TangleApiClient, "_MAX_PHYSICAL_SENDS", 3) + + responses = _redirect_chain(TangleApiClient._MAX_REDIRECTS, _response({"ok": True})) + session = _FakeSession(responses) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.RetryError, match="send pool exhausted"): + client._make_request("GET", "/api/test") + + assert len(session.calls) == 3 + + +@pytest.mark.parametrize( + ("redirect_url", "location", "secrets", "expected"), + [ + pytest.param( + "https://api.test/api/test", + "/download?access_token=tok-SECRET", + ["tok-SECRET", "access_token"], + "https://api.test/download", + id="signed-query", + ), + pytest.param( + "https://api.test/api/test", + "/blob?X-Amz-Signature=amz-SECRET&X-Amz-Credential=AKIA-SECRET", + ["amz-SECRET", "AKIA-SECRET", "X-Amz-Signature"], + "https://api.test/blob", + id="aws-sigv4-query", + ), + pytest.param( + "https://api.test/api/test", + "/asset#token=frag-SECRET", + ["frag-SECRET", "#"], + "https://api.test/asset", + id="fragment-credential", + ), + pytest.param( + "https://alice:hunter2@api.test/api/test", + "https://alice:hunter2@api.test/next", + ["alice", "hunter2"], + "https://api.test/next", + id="userinfo", + ), + pytest.param( + "https://api.test:9x9/api/test", + "https://api.test:9x9/next?X-Amz-Signature=port-SECRET", + ["port-SECRET", "X-Amz-Signature"], + "https://api.test:9x9/next", + id="malformed-port", + ), + ], +) +def test_retry_error_after_redirect_keeps_credentials_out_of_the_message( + monkeypatch, redirect_url: str, location: str, secrets: list[str], expected: str +) -> None: + # A same-origin redirect can land on a signed URL, and the exhaustion error + # flows into CLI output and logs. The destination stays nameable through + # scheme/host/path, but query, fragment, and userinfo must not survive -- + # and a malformed port must not turn the error itself into a ValueError. + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + monkeypatch.setattr(TangleApiClient, "_MAX_PHYSICAL_SENDS", 1) + session = _FakeSession([_redirect(redirect_url, location)]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.RetryError, match="send pool exhausted") as exc_info: + client._make_request("GET", "/api/test") + + message = str(exc_info.value) + for secret in secrets: + assert secret not in message + assert expected in message + + +def test_deadline_retry_error_after_redirect_keeps_credentials_out_of_the_message( + monkeypatch, +) -> None: + # Same guarantee on the other exhaustion branch: a backoff-free deadline hit + # right after the redirect hop must not report the signed query either. + clock = SimpleNamespace(now=1_000.0) + monkeypatch.setattr("tangle_cli.client.time.monotonic", lambda: clock.now) + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + + responses = [_redirect("https://api.test/api/test", "/blob?X-Amz-Signature=amz-SECRET")] + session = _ClockAdvancingSession( + responses, clock, step=TangleApiClient._MAX_RETRY_ELAPSED_SECONDS + 10.0 + ) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.exceptions.RetryError, match="deadline exceeded") as exc_info: + client._make_request("GET", "/api/test") + + message = str(exc_info.value) + assert "amz-SECRET" not in message + assert "X-Amz-Signature" not in message + assert "https://api.test/blob" in message + + +def test_credential_safe_url_never_raises_on_authorities_urlsplit_rejects() -> None: + # Error formatting runs while an exhaustion error is already being raised, + # so an authority the parser rejects must degrade to a placeholder. + assert TangleApiClient._credential_safe_url("https://[::1/api") == "" + assert TangleApiClient._credential_safe_url("") == "" + + +def test_post_401_then_429_shares_the_budget_across_auth_and_rate_limit(monkeypatch) -> None: + # Mixed composition on the non-replayable path: the auth refresh restarts the + # rate-limit rounds, so the shared budget is the only thing bounding the + # total number of physical POSTs. + sleeps: list[float] = [] + monkeypatch.setattr("tangle_cli.client.time.sleep", sleeps.append) + + class RefreshingClient(TangleApiClient): + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.refreshes = 0 + + def _refresh_auth(self) -> None: + self.refreshes += 1 + + session = _FakeSession( + [_response(status_code=401)] + [_response(status_code=429) for _ in range(10)] + ) + client = RefreshingClient("https://api.test", session=session) + + result = client._make_request("POST", "/api/test", json_data={"a": 1}) + + assert result.status_code == 429 + # The 401 send plus one full legacy 429 sequence, still inside the shared cap. + assert len(session.calls) == 1 + TangleApiClient._MAX_RATE_LIMIT_RETRIES + 1 == 5 + assert len(session.calls) <= TangleApiClient._MAX_GET_RETRIES + 1 + assert sleeps == [1.0, 2.0, 4.0] + assert client.refreshes == 2 + + +def test_get_5xx_exhaustion_raises_http_error_from_public_operation(monkeypatch) -> None: + monkeypatch.setattr("tangle_cli.client.time.sleep", lambda _delay: None) + budget = TangleApiClient._MAX_GET_RETRIES + session = _FakeSession([_response(status_code=503) for _ in range(budget + 1)]) + client = TangleApiClient("https://api.test", session=session) + + with pytest.raises(requests.HTTPError) as exc_info: + client.pipeline_runs_get("run-1") + + assert exc_info.value.response is not None + assert exc_info.value.response.status_code == 503 + assert len(session.calls) == budget + 1 diff --git a/tests/test_components_cli.py b/tests/test_components_cli.py index 5764695..c52b5d8 100644 --- a/tests/test_components_cli.py +++ b/tests/test_components_cli.py @@ -138,6 +138,7 @@ def fake_client_from_options(**kwargs: Any) -> object: "header": ["X-Config: yes"], "include_env_credentials": False, "command_name": "published-component commands", + "logger": ANY, } ] assert FakePublisher.instances[0].kwargs["client"] is fake_client @@ -271,6 +272,7 @@ def fake_client_from_options(**kwargs: Any) -> object: "header": None, "include_env_credentials": False, "command_name": "published-component commands", + "logger": ANY, } ] assert [publisher.kwargs for publisher in FakePublisher.instances] == [ @@ -341,6 +343,7 @@ def fake_deprecate_component(client: object, digest: str, **kwargs: Any) -> dict "header": ["X-Test: yes"], "include_env_credentials": False, "command_name": "published-component commands", + "logger": ANY, } ] assert deprecate_calls == [ diff --git a/tests/test_secrets_cli.py b/tests/test_secrets_cli.py index 6072131..f61b17f 100644 --- a/tests/test_secrets_cli.py +++ b/tests/test_secrets_cli.py @@ -6,6 +6,7 @@ import sys from types import SimpleNamespace from typing import Any +from unittest.mock import ANY import pytest @@ -341,6 +342,7 @@ def test_sdk_secrets_config_array_and_config_base_url_credential_isolation( "header": ["X-Config: yes"], "include_env_credentials": False, "command_name": "secret commands", + "logger": ANY, }, { "base_url": "https://config.example", @@ -349,6 +351,7 @@ def test_sdk_secrets_config_array_and_config_base_url_credential_isolation( "header": ["X-Config: yes"], "include_env_credentials": False, "command_name": "secret commands", + "logger": ANY, }, ] assert [instance.calls[0]["secret_name"] for instance in FakeLazyTangleApiClient.instances] == [ From 1e04c5202291266f010412d66a7eee2cf97b93ad Mon Sep 17 00:00:00 2001 From: Volv G Date: Tue, 18 Aug 2026 16:24:36 -0700 Subject: [PATCH 7/7] Add fail-closed aggregate test check Assisted-By: devx/3facbcec-c222-4a3d-be16-c136d12fb965 --- .github/workflows/ci.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2f954c..9db89c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,7 +5,8 @@ on: push: jobs: - test: + test-matrix: + name: test (${{ matrix.python-version }}) runs-on: ubuntu-latest strategy: @@ -35,3 +36,19 @@ jobs: - name: Run tests run: uv run --python ${{ matrix.python-version }} pytest + + test: + name: test + needs: test-matrix + if: ${{ always() }} + runs-on: ubuntu-latest + + steps: + - name: Require all supported Python tests to pass + env: + MATRIX_RESULT: ${{ needs.test-matrix.result }} + run: | + if [ "$MATRIX_RESULT" != "success" ]; then + echo "The supported-Python test matrix finished with: $MATRIX_RESULT" >&2 + exit 1 + fi