gabriel / muse public
mwp-5-clone-retry-503.md markdown
437 lines 20.5 KB
Raw
sha256:37af4ab271a64f11d7bf14b4871ebaaf5dd68fb0247521007c6b034d35aa01be Merge branch 'fix/hub-user-read-update-path' into dev Human 9 days ago

MWP-5 — Clone/fetch/pull honor Retry-After with a bounded poll (fixes RC-5)

Master tracker: muse#58 — https://staging.musehub.ai/gabriel/muse/issues/58 Predecessors (all closed, server side):

  • musehub#106 (MWP-1, generation authority — fixes RC-1)
  • musehub#107 (MWP-2, walk fallback — fixes RC-2)
  • musehub#108 (MWP-3, job-enqueue idempotency — fixes RC-3)
  • musehub#109 (MWP-4, prebuild-after-index ordering — fixes RC-4)

Repo target: muse (the client). Every predecessor hardened the server so the 503 window is now as small as it can be. MWP-5 is the client half: when the server legitimately answers 503 + Retry-After during the brief prebuild/index window, the client must wait and retry instead of destroying the half-created clone and exiting hard.

This ticket is written to be picked up cold by another agent. It is dense on purpose. Every phase is TDD: write the test, watch it fail, implement until green, never skip ahead. Symbol anchors point code-intelligence at the exact call sites — run muse code cat "<anchor>" --json to read each one before you touch it.


Background

The symptom

A muse clone <url> (or muse fetch / muse pull) issued seconds after a push fails hard:

❌ Fetch failed: Objects not yet indexed: 412 missing. Retry shortly.

The clone then rmtrees the half-created target directory and exits 3 (INTERNAL_ERROR). A second clone a moment later works — because by then the server's mpack.index + fetch.mpack.prebuild jobs have completed. The data is never missing; the client just refuses to wait the second or two the server explicitly asked it to.

The server is behaving correctly — RC-5 is a client bug

The fetch endpoint musehub/api/routes/wire.py::fetch_mpack (the single endpoint clone, fetch, and pull all hit) returns a well-formed 503 with a Retry-After header while the async prebuild/index pipeline is still in flight. There are exactly two retryable 503 shapes, both confirmed in musehub/api/routes/wire.py:694-705:

Server exception Status Retry-After Body
MPackNotReadyError 503 30 mpack not ready — prebuild in progress. Retry shortly.
FetchNotIndexedError 503 60 Objects not yet indexed: N missing. Retry shortly.

This is RFC 7231 §6.6.4 + §7.1.3 textbook behavior: "service temporarily unavailable, come back after N seconds." The client is supposed to honor it.

Where the client throws the contract away — two precise defects

Defect A — the Retry-After header is silently dropped at the transport seam.

muse/core/transport.py::_urllib_do catches the 503 and converts it to a TransportError, but reads only err.code — never err.headers:

    except urllib.error.HTTPError as err:
        raise TransportError(_http_error_message(err), err.code) from err   # ← Retry-After lost

muse/core/transport.py::_execute_fetch has the identical pattern. So even though TransportError carries status_code = 503, the number of seconds the server asked us to wait is gone before any caller can see it.

muse/core/transport.py::TransportError.__init__ has no field for it either:

    def __init__(self, message: str, status_code: int) -> None:
        super().__init__(message)
        self.status_code = status_code

Defect B — every caller treats the first 503 as terminal.

The POST in muse/core/transport.py::HttpTransport.fetch_mpack goes through _urllib_do; a 503 propagates straight out as TransportError(…, 503) with no retry. Each command then hard-fails on the first one:

  • muse/cli/commands/clone.py::runexcept TransportErrorshutil.rmtree(target, ignore_errors=True)raise SystemExit(INTERNAL_ERROR). This deletes the freshly-created repo on a transient, self-healing 503.
  • muse/cli/commands/fetch.py::_fetch_oneexcept TransportError → print ❌ Fetch failedraise SystemExit(INTERNAL_ERROR).
  • muse/cli/commands/pull.py::run (around line 149-156) — same pattern.

None of them inspect status_code == 503, none honor Retry-After, none poll.


Goal — definition of done

muse clone, muse fetch, and muse pull, issued immediately after a push, succeed via a bounded retry loop that honors the server's Retry-After:

  1. On a 503 from /fetch/mpack, the client waits Retry-After seconds (clamped to a sane range) and retries, up to a total wall-clock budget (default 120s — see Budget math below), emitting clear progress to stderr.
  2. A clone is never rmtreed on a retryable 503 — the target survives across retries and is removed only after the budget is genuinely exhausted, with a message that says so ("remote still preparing data after Ns").
  3. Non-503 errors (401/404/422/network) still fail immediately — zero behavioral change, zero added latency.
  4. --json mode keeps stdout clean: only the final envelope is printed there; all retry chatter goes to stderr.
  5. The retry budget is configurable (env + optional flag) and a --no-retry escape hatch restores single-attempt behavior for deterministic CI.
  6. The whole thing is provable with mocked seams and one real-urllib E2E that exercises actual HTTPError header parsing.

Design

D1 — TransportError gains retry_after

muse/core/transport.py::TransportError:

class TransportError(Exception):
    def __init__(
        self,
        message: str,
        status_code: int,
        *,
        retry_after: int | None = None,   # NEW — seconds, parsed from Retry-After
    ) -> None:
        super().__init__(message)
        self.status_code = status_code
        self.retry_after = retry_after

Keyword-only and defaulted to None → every existing TransportError(msg, code) call site stays valid. No call site needs editing except the two that learn the header (D2).

D2 — capture Retry-After at both HTTPError seams

Add a module-level parser and use it in _urllib_do and _execute_fetch:

def _parse_retry_after(headers) -> int | None:
    """Parse an RFC 7231 Retry-After header into whole seconds.

    Accepts the delta-seconds form (``"60"``). The HTTP-date form is rare for
    this endpoint and is intentionally treated as ``None`` (caller falls back to
    its default backoff) rather than risking a brittle date parse. Returns None
    on a missing or non-integer value — never raises.
    """
    if headers is None:
        return None
    raw = headers.get("Retry-After")
    if raw is None:
        return None
    try:
        secs = int(str(raw).strip())
    except (TypeError, ValueError):
        return None
    return secs if secs >= 0 else None
    except urllib.error.HTTPError as err:
        raise TransportError(
            _http_error_message(err), err.code,
            retry_after=_parse_retry_after(err.headers),
        ) from err

D3 — bounded retry helper + test seams

Two module-level seams so tests never sleep for real and budgets are tunable:

_FETCH_RETRY_BUDGET_DEFAULT_S: float = 120.0
_RETRY_AFTER_CEILING_S: float = 60.0   # never sleep longer than this per attempt
_RETRY_AFTER_FLOOR_S: float = 1.0      # never busy-spin if server says 0
_RETRY_DEFAULT_BACKOFF_S: float = 5.0  # used when 503 carries no Retry-After

def _sleep(seconds: float) -> None:
    """Indirection over time.sleep — patched in tests to assert wait schedule."""
    time.sleep(seconds)

def _resolve_retry_budget(explicit: float | None) -> float:
    """explicit arg > MUSE_FETCH_RETRY_BUDGET_S env > module default."""
    if explicit is not None:
        return max(0.0, explicit)
    env = os.environ.get("MUSE_FETCH_RETRY_BUDGET_S")
    if env:
        try:
            return max(0.0, float(env))
        except ValueError:
            pass
    return _FETCH_RETRY_BUDGET_DEFAULT_S

def _next_wait(retry_after: int | None) -> float:
    base = float(retry_after) if retry_after is not None else _RETRY_DEFAULT_BACKOFF_S
    return min(max(base, _RETRY_AFTER_FLOOR_S), _RETRY_AFTER_CEILING_S)

D4 — the loop lives in HttpTransport.fetch_mpack (single implementation)

make_transport always returns HttpTransport (confirmed — muse/core/transport.py::make_transport), so putting the loop in HttpTransport.fetch_mpack gives all three commands the fix for free with zero duplication. Extract the current single-attempt body into _fetch_mpack_once(...) and wrap it:

def fetch_mpack(self, url, signing, want, have, *, ttl_seconds=3600,
                retry_budget_s: float | None = None) -> FetchMPackResult:
    budget = _resolve_retry_budget(retry_budget_s)
    start = time.monotonic()
    attempt = 0
    while True:
        attempt += 1
        try:
            return self._fetch_mpack_once(url, signing, want, have, ttl_seconds=ttl_seconds)
        except TransportError as exc:
            if exc.status_code != 503:
                raise                      # non-retryable — fail immediately
            elapsed = time.monotonic() - start
            wait = _next_wait(exc.retry_after)
            if elapsed + wait > budget:
                raise                      # budget exhausted — terminal 503
            print(
                f"⏳ remote preparing fetch data (server busy, attempt {attempt}); "
                f"retrying in {int(wait)}s … (waited {int(elapsed)}s / {int(budget)}s)",
                file=sys.stderr, flush=True,
            )
            _sleep(wait)

MuseTransport.fetch_mpack is an abstract/base variant not used in production (make_transport never returns it). Do not duplicate the loop there; add a one-line note that retry lives in the concrete HttpTransport.

Budget math — why 120s, not the "~60s" the tracker hand-waved. A single FetchNotIndexedError carries Retry-After: 60. With a 60s budget the first sleep (60s) already meets the cap, so elapsed + wait > budget trips and the client gives up after one attempt — no real retry. 120s permits: attempt 1 at t=0 → 503(60) → sleep 60 → attempt 2 at t=60 → on a second 503(60) the next sleep would reach t=120 and we stop cleanly. That is two genuine attempts plus a guaranteed retry of the common single-503 case. The default is configurable; document the deviation in the closing comment.

D5 — command-layer integration

The retry now lives below the commands, so the commands' existing except TransportError blocks become the terminal path (budget already exhausted). Required changes:

  • clone.py::run — keep the rmtree + INTERNAL_ERROR, but only as the genuinely-terminal outcome. Improve the message when exc.status_code == 503 to: ❌ Remote still preparing clone data after {budget}s — try again shortly. The directory survived every retry; it is removed only here. JSON error envelope: add "retryable": true / a timed_out marker on the 503 branch.
  • fetch.py::_fetch_one and pull.py::run — same terminal-message refinement and JSON marker. No rmtree (they mutate an existing repo).
  • Optional CLI surface (add if cheap, else env-only): --retry-timeout SECONDS (maps to retry_budget_s) and --no-retry (maps to retry_budget_s=0, i.e. single attempt) on clone, fetch, pull. register is the arg-parser hook in each command module.

Phases — load-bearing, TDD, never skip ahead

All tests live in tests/test_mwp5_clone_retry.py (muse repo). Test IDs appear in both this plan and the test file. The network seam is patched exactly like the existing suite does it — unittest.mock.patch("muse.core.transport._urllib_do", side_effect=…) (see tests/test_core_transport.py:348-436 for the canonical helper) — and _sleep is patched to a recorder so no test waits in real time.

Phase 0 — RED harness ✅

  • [x] Create tests/test_mwp5_clone_retry.py with a _seq_urllib_do(...) helper that raises TransportError(msg, 503, retry_after=R) for the first k calls then returns a valid mpack POST body, plus a _record_sleeps() fixture patching muse.core.transport._sleep.
  • [x] MWP5_00 — characterization: with current code, one 503 from _urllib_do makes HttpTransport().fetch_mpack(...) raise TransportError immediately and _sleep is never called. Asserts the bug. RED→stays red until Phase 3, then flips to assert the new behavior. Commits: sha256:11eb9c76826dc95e3b1cd109b04f538a6739f1b50c897cc0d7b19db5d2bec9bb

Phase 1 — capture Retry-After (Defect A) ✅

Anchors: muse/core/transport.py::TransportError, muse/core/transport.py::_urllib_do, muse/core/transport.py::_execute_fetch, muse/core/transport.py::_http_error_message.

  • [x] Add retry_after to TransportError.__init__ (D1).
  • [x] Add _parse_retry_after and call it in both HTTPError handlers (D2).
  • [x] MWP5_01_parse_retry_after({"Retry-After": "60"})60. ✅
  • [x] MWP5_02 — missing header → None; non-integer ("Wed, 21 Oct") → None. ✅
  • [x] MWP5_03 — a real HTTPError(code=503, headers={"Retry-After":"30"}) driven through _urllib_do yields TransportError with status_code==503 and retry_after==30. ✅
  • [x] MWP5_04 — a 404 HTTPError yields retry_after is None and is unaffected (regression guard for non-retryable codes). ✅ Commits: sha256:8be791449fd7d6c466a1ebd875c55866c8c41d81eae6778b1c5a46eda3470dff

Phase 2 — retry primitives (D3) ✅

Anchors: new module-level symbols in muse/core/transport.py.

  • [x] Add _sleep, _resolve_retry_budget, _next_wait, the four module constants.
  • [x] MWP5_05_next_wait(0) floors to 1.0; _next_wait(999) ceilings to 60.0; _next_wait(None)_RETRY_DEFAULT_BACKOFF_S. ✅
  • [x] MWP5_06_resolve_retry_budget: explicit arg wins; else MUSE_FETCH_RETRY_BUDGET_S env; else 120.0; negatives clamp to 0.0; junk env falls through to default. ✅ Commits: sha256:0fdd6beaff2a25b2eda7d4c8b3034444bbc124619339ed6d91a836ffaafb5608

Phase 3 — wire the loop into fetch_mpack (Defect B core) ✅

Anchors: muse/core/transport.py::HttpTransport.fetch_mpack (extract _fetch_mpack_once).

  • [x] Extract the existing single-attempt body verbatim into _fetch_mpack_once; add the loop per D4.
  • [x] MWP5_07 — one 503(Retry-After:30) then success: returns the FetchMPackResult, _sleep called once with 30.0. ✅
  • [x] MWP5_08 — 503(30) then 503(60) then success: two sleeps [30.0, 60.0], result returned. ✅
  • [x] MWP5_09 — 503 with no Retry-After: sleeps _RETRY_DEFAULT_BACKOFF_S. ✅
  • [x] MWP5_10 — budget exhausted (503 forever, budget=120): raises TransportError(status_code=503); total slept <= 120; sleep count bounded; the final raise is the server's 503, not a synthetic one. ✅
  • [x] MWP5_11 — a 404 mid-loop raises immediately; _sleep never called (no added latency for real errors). ✅
  • [x] MWP5_12retry_budget_s=0 (== --no-retry): exactly one attempt, zero sleeps, original behavior preserved. ✅
  • [x] Flipped MWP5_00 to assert the success-after-retry behavior. ✅ Commits: sha256:1df101e06206778511bc1b961d78270ac494c2986bfdf12104bb1d7d8a0474b2

Phase 4 — command integration (D5) ✅

Anchors: muse/cli/commands/clone.py::run, muse/cli/commands/fetch.py::_fetch_one, muse/cli/commands/pull.py::run, and each module's register.

  • [x] Refined terminal 503 messaging in all three; added JSON retryable marker. Added --retry-timeout / --no-retry flags to all three commands.
  • [x] MWP5_13 — CLI clone, transport patched to 503-then-success: exits 0, target directory exists and is populated, was never rmtreed. ✅
  • [x] MWP5_14 — CLI clone, 503-forever with budget=0 (--no-retry): exits INTERNAL_ERROR, target is removed, stderr says "still preparing … after Ns", JSON envelope (when --json) carries the retryable marker. ✅
  • [x] MWP5_15 — CLI fetch, 503-then-success: exits 0, commits applied. ✅
  • [x] MWP5_16 — CLI pull, 503-then-success: exits 0, ff/merge proceeds. ✅
  • [x] MWP5_17--json clone with retries: stdout contains exactly one JSON object (the final envelope); all lines are on stderr. ✅ Commits: sha256:1df101e06206778511bc1b961d78270ac494c2986bfdf12104bb1d7d8a0474b2

Phase 5 — real-urllib E2E + docs + close ✅

  • [x] MWP5_18 (E2E, highest value) — stood up a throwaway http.server.BaseHTTPRequestHandler on 127.0.0.1:0 that answers the first POST with 503 + Retry-After: 1 and the second with a valid mpack body. Drives a real HttpTransport().fetch_mpack (real _urllib_do, real urllib.error.HTTPError, real header parsing) with _sleep patched to no-op. Proves the header survives the actual urllib path end-to-end — not just a mocked side_effect. ✅ Commits: sha256:1df101e06206778511bc1b961d78270ac494c2986bfdf12104bb1d7d8a0474b2
  • [x] Update muse#58: tick RC-5 in Confirmed root causes and the acceptance criterion "muse clone issued immediately after muse push succeeds via bounded retry, never hard-fails on the first 503 (RC-5)". (Edit the local docs/issues/mwp-mvp-master.md first, then push --body-file per the local-file rule.) ✅
  • [x] Post a closing comment on this issue documenting the final budget default, the deviation from the tracker's "~60s", and the commit range; close it. ✅

Acceptance criteria

  • [x] muse clone immediately after a push succeeds via bounded retry and never rmtrees the target on a transient 503 (MWP5_13). ✅
  • [x] muse fetch / muse pull immediately after a push succeed via the same loop (MWP5_15, MWP5_16). ✅
  • [x] Both server 503 shapes are honored: Retry-After: 30 (MPackNotReadyError) and Retry-After: 60 (FetchNotIndexedError) (MWP5_07, MWP5_08). ✅
  • [x] Budget exhaustion ends in a clear terminal failure that slept ≤ budget and surfaced the server's real 503 (MWP5_10, MWP5_14). ✅
  • [x] Non-503 errors fail immediately with zero added sleeps (MWP5_04, MWP5_11). ✅
  • [x] --no-retry / retry_budget_s=0 restores exact single-attempt behavior (MWP5_12). ✅
  • [x] --json stdout carries only the final envelope during retries (MWP5_17). ✅
  • [x] The real-urllib E2E proves Retry-After survives the actual HTTPError path (MWP5_18). ✅
  • [x] muse#58 RC-5 + its acceptance criterion are ticked; this issue closed with evidence. ✅

Out of scope

  • 429 rate-limit retry. The same retry_after plumbing makes a future 429 loop trivial, but WIRE_FETCH_LIMIT tuning and 429 policy are a separate concern. Capturing the header for 429 (Phase 1 does this incidentally) is fine; acting on 429 is not part of MWP-5.
  • The HTTP-date form of Retry-After. The endpoint only emits delta-seconds; date parsing is deliberately None-and-fall-back.
  • Any change to the server 503 contract (that surface is frozen by MWP-1..4).
  • Retrying the R2 GET (post-presign download) or integrity failures — those are not 503 and not part of this ticket.
  • fetch_remote_info retry — ref listing needs no prebuild and never 503s here.

Symbol-anchor appendix — read before you touch

# Client transport (all edits land here except the command-layer messaging)
muse code cat "muse/core/transport.py::TransportError" --json
muse code cat "muse/core/transport.py::_urllib_do" --json
muse code cat "muse/core/transport.py::_execute_fetch" --json
muse code cat "muse/core/transport.py::_http_error_message" --json
muse code cat "muse/core/transport.py::HttpTransport.fetch_mpack" --json
muse code cat "muse/core/transport.py::make_transport" --json

# Command layer (terminal-path messaging + optional flags)
muse code cat "muse/cli/commands/clone.py::run" --json
muse code cat "muse/cli/commands/fetch.py::_fetch_one" --json
muse code cat "muse/cli/commands/pull.py::run" --json

# Blast radius before editing TransportError / fetch_mpack
muse code impact "muse/core/transport.py::TransportError" --json
muse code impact "muse/core/transport.py::HttpTransport.fetch_mpack" --json

# Test seam reference (copy the _urllib_do patch idiom)
muse code cat "tests/test_core_transport.py" --all --json

Server-side contract (read-only; do NOT edit — frozen by MWP-1..4): musehub/api/routes/wire.py:684-706 — the fetch_mpack route's two 503 branches and their exact Retry-After values.

File History 3 commits
sha256:37af4ab271a64f11d7bf14b4871ebaaf5dd68fb0247521007c6b034d35aa01be Merge branch 'fix/hub-user-read-update-path' into dev Human 9 days ago
sha256:b7be56ec091919a612cffe7f3c8b600209d5155517443fdac0e16954c8c7d81f Merge 'task/git-export-ignored-file-deletion' into 'dev' — … Human 12 days ago
sha256:8de4334a98c945aace420969d389ad678aa926d4ab4e886b2ac4c4241cb3bf2b revert: keep pyproject.toml in canonical PEP 440 form Sonnet 4.6 patch 15 days ago