Raise instead of returning False from runner_ssh_tunnel - #4262
Merged
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_tunnelreturnedUnion[Literal[False], R]--Falseif 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:Raise
PeerConnectionErrorinstead, declared next to the families it complements. It composes with them in a singleexceptclause, carries the reasonFalsecould not, and chains the underlyingSSHErrororRequestExceptionas__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 returnX._RunnerAvailabilityis gone. #4251 introduced it to carryUNREACHABLE, a second encoding of the stateFalsealready meant. Without that member it is a two-member enum, so_get_runner_availability()becomes_is_runner_available() -> booland letsRunnerResponseErrorpropagate.Two defects the sentinel had produced:
stop_runner()wrapped its call inexcept SSHError, which nothing could reach: the decorator caughtSSHErroritself in one branch, andinstance_connection_pool.get_or_open()swallowed it in the other.isinstance(res, bool), since their wrapped functions returnOptional[...], whereif not rescannot tellNonefromFalse.