fix(update): only an explicit hermes update retries channel reads
Passive checks (`hermes --version`, the banner, the Desktop/dashboard update check) now make one channel-read attempt again. Offline usually surfaces as DNS EAI_AGAIN or ENETUNREACH, which pm.network.is_transient classes as transient, so wrapping every read in retry_network added 7 s of backoff to the synchronous version line, stretched a hung CDN from 30 s to ~127 s, and logged a WARNING to stderr on every retry. `release_channels.retrying_reads()` opts a block in; update_cmd wraps the channel resolution of `hermes update` in it. Tests: keep the Retry-After retry test (now scoped to the update path) and replace the budget-exhaustion test with one pinning that passive reads and 404s make exactly one attempt.
This commit is contained in:
@@ -1,6 +1,8 @@
|
||||
"""R2 channel wire protocol. Names are data; no local channel registry exists."""
|
||||
from __future__ import annotations
|
||||
|
||||
from contextlib import contextmanager
|
||||
from contextvars import ContextVar
|
||||
from dataclasses import dataclass
|
||||
import hashlib
|
||||
import json
|
||||
@@ -253,6 +255,25 @@ class _NoRedirect(HTTPRedirectHandler):
|
||||
raise ChannelError("Channel archive redirects are not permitted")
|
||||
|
||||
|
||||
_RETRY_READS: ContextVar[bool] = ContextVar("release_channel_read_retries", default=False)
|
||||
|
||||
|
||||
@contextmanager
|
||||
def retrying_reads():
|
||||
"""Let channel reads inside the block wait out a CDN blip with PM's bounded retries.
|
||||
|
||||
Only an explicit ``hermes update`` opts in. Passive checks (``hermes --version``, the
|
||||
banner, the Desktop/dashboard update check) make one attempt: offline also reads as
|
||||
transient (DNS EAI_AGAIN, ENETUNREACH), so retrying there adds backoff to a synchronous
|
||||
status line, stretches a hung CDN from 30 s to ~2 min and logs a WARNING per retry.
|
||||
"""
|
||||
token = _RETRY_READS.set(True)
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
_RETRY_READS.reset(token)
|
||||
|
||||
|
||||
class ChannelReader:
|
||||
def __init__(self, base_url: str, repository: str | None = None, opener=None):
|
||||
self.base_url = public_base(base_url)
|
||||
@@ -264,8 +285,8 @@ class ChannelReader:
|
||||
if sha256 is not None:
|
||||
require_sha256(sha256)
|
||||
|
||||
# Channel reads are idempotent and precede the PM download path, so use the same bounded
|
||||
# transient-failure policy instead of maintaining a second retry classifier here.
|
||||
# Channel reads are idempotent and precede the PM download path, so an explicit update
|
||||
# uses the same bounded transient-failure policy instead of a second retry classifier.
|
||||
from pm.network import retry_network
|
||||
|
||||
def read() -> bytes:
|
||||
@@ -275,7 +296,7 @@ class ChannelReader:
|
||||
return response.read(MAX_METADATA + 1)
|
||||
|
||||
try:
|
||||
body = retry_network(read)
|
||||
body = retry_network(read) if _RETRY_READS.get() else read()
|
||||
except HTTPError as exc:
|
||||
if exc.code == 404:
|
||||
raise ChannelNotFound(f"Channel object not found: {key}") from exc
|
||||
|
||||
@@ -1309,6 +1309,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
||||
target_repository = None
|
||||
selected_channel = _source_update_channel(args)
|
||||
if not getattr(args, "branch", None):
|
||||
from hermes_cli.release_channels import retrying_reads
|
||||
from hermes_cli.source_releases import resolve_source_target
|
||||
|
||||
from copy import deepcopy
|
||||
@@ -1319,8 +1320,9 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
||||
Path(completion_request["home"]) / "config.yaml"), _m().PROJECT_ROOT))
|
||||
print(f"→ Update channel: {selected_channel}")
|
||||
try:
|
||||
target = resolve_source_target(
|
||||
selected_channel, None if use_zip_update else git_cmd, _m().PROJECT_ROOT)
|
||||
with retrying_reads():
|
||||
target = resolve_source_target(
|
||||
selected_channel, None if use_zip_update else git_cmd, _m().PROJECT_ROOT)
|
||||
except (OSError, ValueError, subprocess.SubprocessError) as exc:
|
||||
print(f"✗ Could not resolve the {selected_channel} source channel: {exc}. No update was applied.")
|
||||
_m()._resume_windows_gateways_after_update(_windows_gateway_resume)
|
||||
|
||||
@@ -72,10 +72,10 @@ def test_names_are_validated_without_normalizing(name):
|
||||
validate_name(name)
|
||||
|
||||
|
||||
def test_reader_retries_transient_http_and_honors_retry_after(monkeypatch):
|
||||
def test_update_reads_retry_transient_http_and_honor_retry_after(monkeypatch):
|
||||
from email.message import Message
|
||||
from urllib.error import HTTPError
|
||||
from hermes_cli.release_channels import ChannelReader
|
||||
from hermes_cli.release_channels import ChannelReader, retrying_reads
|
||||
|
||||
url = "http://127.0.0.1:12345/releases/fixture.json"
|
||||
headers = Message()
|
||||
@@ -105,34 +105,40 @@ def test_reader_retries_transient_http_and_honors_retry_after(monkeypatch):
|
||||
|
||||
monkeypatch.setattr("pm.network.time.sleep", waits.append)
|
||||
reader = ChannelReader("http://127.0.0.1:12345", opener=opener)
|
||||
assert reader.read_bytes("releases/fixture.json") == b"fixture"
|
||||
with retrying_reads():
|
||||
assert reader.read_bytes("releases/fixture.json") == b"fixture"
|
||||
assert attempts == [url, url]
|
||||
assert waits == [7.0]
|
||||
|
||||
|
||||
def test_reader_exhausts_transient_retry_budget_before_channel_error(monkeypatch):
|
||||
def test_passive_reads_and_missing_objects_make_one_attempt(monkeypatch):
|
||||
"""Offline looks transient (ENETUNREACH); a passive check must not back off on it."""
|
||||
import errno
|
||||
from email.message import Message
|
||||
from urllib.error import HTTPError
|
||||
from hermes_cli.release_channels import ChannelError, ChannelReader
|
||||
from pm import network
|
||||
from urllib.error import HTTPError, URLError
|
||||
from hermes_cli.release_channels import ChannelError, ChannelNotFound, ChannelReader, retrying_reads
|
||||
|
||||
url = "http://127.0.0.1:12345/releases/fixture.json"
|
||||
headers = Message()
|
||||
headers["Retry-After"] = "1"
|
||||
attempts = []
|
||||
waits = []
|
||||
monkeypatch.setattr("pm.network.time.sleep", waits.append)
|
||||
|
||||
def opener(request, timeout):
|
||||
assert timeout == 30
|
||||
attempts.append(request.full_url)
|
||||
raise HTTPError(url, 503, "unavailable", headers, None)
|
||||
def reader(fault, calls):
|
||||
def opener(request, timeout):
|
||||
calls.append(request.full_url)
|
||||
raise fault
|
||||
return ChannelReader("https://releases.example", opener=opener)
|
||||
|
||||
monkeypatch.setattr(network.time, "sleep", waits.append)
|
||||
reader = ChannelReader("http://127.0.0.1:12345", opener=opener)
|
||||
with pytest.raises(ChannelError, match="HTTP 503"):
|
||||
reader.read_bytes("releases/fixture.json")
|
||||
assert len(attempts) == network._ATTEMPTS
|
||||
assert len(waits) == network._ATTEMPTS - 1
|
||||
calls = []
|
||||
offline = URLError(OSError(errno.ENETUNREACH, "Network is unreachable"))
|
||||
with pytest.raises(ChannelError, match="unavailable"):
|
||||
reader(offline, calls).read_bytes("releases/channels/main.json")
|
||||
assert len(calls) == 1
|
||||
|
||||
calls.clear()
|
||||
missing = HTTPError("https://releases.example/x", 404, "missing", Message(), None)
|
||||
with retrying_reads(), pytest.raises(ChannelNotFound):
|
||||
reader(missing, calls).read_bytes("releases/channels/stable.json")
|
||||
assert len(calls) == 1
|
||||
assert waits == []
|
||||
|
||||
|
||||
def test_reader_rejects_cycles_identity_substitution_and_cross_authority():
|
||||
|
||||
Reference in New Issue
Block a user