Skip to content

Raise instead of returning False from runner_ssh_tunnel - #4262

Merged
un-def merged 1 commit into
masterfrom
pr_runner_peer_connection_error
Sep 7, 2026
Merged

Raise instead of returning False from runner_ssh_tunnel#4262
un-def merged 1 commit into
masterfrom
pr_runner_peer_connection_error

Conversation

@un-def

@un-def un-def commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #4251 and #4261, which separated the errors reported by a peer's API from connection failures but left the two halves of that signal travelling by different mechanisms.

runner_ssh_tunnel returned Union[Literal[False], R] -- False if the peer could not be reached, the wrapped function's value otherwise. The API half is already a pair of exception families, so callers had to translate one channel into the other by hand:

except client.ShimResponseError as e:
    # Same outcome as a connection error,
    # `_handle_instance_unreachable()` below, but the cause is now
    # logged instead of being silently indistinguishable.
    logger.warning(...)
    shim_state = False
if shim_state is not False:

Raise PeerConnectionError instead, declared next to the families it complements. It composes with them in a single except clause, carries the reason False could not, and chains the underlying SSHError or RequestException as __cause__.

The sentinel had also leaked into the signatures of the functions the decorator wraps: four of them annotated their own return type as Union[X, Literal[False]], because they used the channel to say "treat this as unreachable". They now return X.

_RunnerAvailability is gone. #4251 introduced it to carry UNREACHABLE, a second encoding of the state False already meant. Without that member it is a two-member enum, so
_get_runner_availability() becomes _is_runner_available() -> bool and lets RunnerResponseError propagate.

Two defects the sentinel had produced:

  • stop_runner() wrapped its call in except SSHError, which nothing could reach: the decorator caught SSHError itself in one branch, and instance_connection_pool.get_or_open() swallowed it in the other.
  • Both metrics collectors tested isinstance(res, bool), since their wrapped functions return Optional[...], where if not res cannot tell None from False.

Follow-up to #4251 and #4261, which separated the errors reported by a
peer's API from connection failures but left the two halves of that
signal travelling by different mechanisms.

`runner_ssh_tunnel` returned `Union[Literal[False], R]` -- `False` if
the peer could not be reached, the wrapped function's value otherwise.
The API half is already a pair of exception families, so callers had to
translate one channel into the other by hand:

    except client.ShimResponseError as e:
        # Same outcome as a connection error,
        # `_handle_instance_unreachable()` below, but the cause is now
        # logged instead of being silently indistinguishable.
        logger.warning(...)
        shim_state = False
    if shim_state is not False:

Raise `PeerConnectionError` instead, declared next to the families it
complements. It composes with them in a single `except` clause, carries
the reason `False` could not, and chains the underlying `SSHError` or
`RequestException` as `__cause__`.

The sentinel had also leaked into the signatures of the functions the
decorator wraps: four of them annotated their own return type as
`Union[X, Literal[False]]`, because they used the channel to say "treat
this as unreachable". They now return `X`.

`_RunnerAvailability` is gone. #4251 introduced it to carry
`UNREACHABLE`, a second encoding of the state `False` already meant.
Without that member it is a two-member enum, so
`_get_runner_availability()` becomes `_is_runner_available() -> bool`
and lets `RunnerResponseError` propagate.

Two defects the sentinel had produced:

* `stop_runner()` wrapped its call in `except SSHError`, which nothing
  could reach: the decorator caught `SSHError` itself in one branch,
  and `instance_connection_pool.get_or_open()` swallowed it in the
  other.
* Both metrics collectors tested `isinstance(res, bool)`, since their
  wrapped functions return `Optional[...]`, where `if not res` cannot
  tell `None` from `False`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@un-def
un-def merged commit 69c6552 into master Sep 7, 2026
26 checks passed
@un-def
un-def deleted the pr_runner_peer_connection_error branch September 7, 2026 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant