diff --git a/scripts/install.sh b/scripts/install.sh index ac99222641..1f972b08f3 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1484,7 +1484,7 @@ EOF local clone_ok=false local attempt=0 local max_attempts=4 - for attempt in 1 2 3 4; do + for attempt in $(seq 1 "$max_attempts"); do [ "$attempt" -gt 1 ] && log_info "Retrying HTTPS clone (attempt $attempt/$max_attempts)..." if git clone --depth 1 --single-branch --branch "$BRANCH" \ "$REPO_URL_HTTPS" "$INSTALL_DIR"; then @@ -1496,18 +1496,26 @@ EOF done if [ "$clone_ok" != true ]; then log_info "Direct clone throttled — trying blobless partial clone..." + # --no-checkout keeps the clone itself to commits+trees (small, + # gets past the pack throttle). Without it the blob fetch runs + # inside `git clone`'s own checkout step, the throttle kills + # the whole clone, and this fallback degrades to one more + # failed clone. The blobs are fetched by the reset below — a + # separate request the retry can actually wrap. if git clone --depth 1 --single-branch --filter=blob:none \ - --branch "$BRANCH" "$REPO_URL_HTTPS" "$INSTALL_DIR"; then - # Materialize the working tree (fetches blobs in small - # packs; one retry helps when the fetch itself is - # throttled). The checkout git printed during the partial - # clone shows missing-content placeholders until this - # reset runs. - cd "$INSTALL_DIR" 2>/dev/null || true - git reset --hard HEAD >/dev/null 2>&1 \ - || git reset --hard HEAD >/dev/null 2>&1 \ - || true - clone_ok=true + --no-checkout --branch "$BRANCH" "$REPO_URL_HTTPS" "$INSTALL_DIR"; then + # Materialize the working tree: on a --no-checkout clone + # this reset is the step that fetches the blobs (several + # small packs instead of one big one). Fail closed — a + # half-materialized checkout must not report success and + # hand the rest of the installer an unusable tree. + if (cd "$INSTALL_DIR" \ + && (git reset --hard HEAD >/dev/null 2>&1 \ + || { sleep 5; git reset --hard HEAD >/dev/null 2>&1; })); then + clone_ok=true + else + rm -rf "$INSTALL_DIR" 2>/dev/null # unusable checkout + fi else rm -rf "$INSTALL_DIR" 2>/dev/null fi diff --git a/tests/test_install_clone_throttle_fallback.py b/tests/test_install_clone_throttle_fallback.py index a57c2d7952..cb47688932 100644 --- a/tests/test_install_clone_throttle_fallback.py +++ b/tests/test_install_clone_throttle_fallback.py @@ -11,9 +11,16 @@ The contract pinned here: - The HTTPS clone is retried with backoff before giving up. - A failed direct attempt is retried after removing the partial clone. - When every direct attempt fails, the installer degrades to a blobless - partial clone (`--filter=blob:none`) and materializes the working tree - with `git reset --hard HEAD` — many small packs instead of one big one, - which is what gets past the throttle. + partial clone (`--filter=blob:none --no-checkout`) and materializes the + working tree with `git reset --hard HEAD` — the clone itself is + commits+trees only (small, passes the throttle) and the reset becomes + the separate blob fetch the retry can wrap (review of #89629: without + --no-checkout the blob fetch runs inside `git clone`'s own checkout, + so the throttle kills the whole clone and the fallback degrades to one + more failed clone). +- Materialization fails closed: both reset attempts failing must remove + the checkout and report a clone failure, never report success over an + unusable tree. """ from __future__ import annotations @@ -46,8 +53,9 @@ def _https_branch() -> str: def test_https_clone_is_retried_with_backoff(): branch = _https_branch() - assert re.search(r"for attempt in 1 2 3 4", branch), ( - "the HTTPS clone must be retried a bounded number of times" + assert re.search(r"for attempt in \$\(seq 1 \"\$max_attempts\"\)", branch), ( + "the HTTPS clone must be retried a bounded number of times, with the " + "loop bound driven by the same variable the messages report" ) assert re.search(r"sleep \$\(\(attempt \* 5\)\)", branch), ( "retries must back off between attempts" @@ -66,12 +74,45 @@ def test_blobless_partial_clone_fallback_exists(): "after direct attempts fail, degrade to a blobless partial clone " "(many small packs — what gets past the repo-scoped 429)" ) + assert re.search( + r"git clone --depth 1 --single-branch --filter=blob:none \\\n" + r"\s*--no-checkout --branch \"\$BRANCH\"", + branch, + ), ( + "the partial clone must defer the checkout (--no-checkout): the blob " + "fetch otherwise runs inside git clone's own checkout step, the " + "throttle kills the whole clone, and the fallback never engages" + ) assert re.search(r"git reset --hard HEAD", branch), ( "the partial clone's working tree must be materialized so the rest " "of the installer sees the normal files" ) +def test_materialization_fails_closed(): + """A failed blob materialization must not report a successful clone. + + The reset on a --no-checkout clone is the step that fetches the blobs, + so it is the step most likely to be throttled. `|| true` plus an + unconditional `clone_ok=true` would hand the rest of the installer a + half-materialized tree while printing "Cloned via HTTPS". + """ + branch = _https_branch() + fallback = branch.split('log_info "Direct clone throttled')[1] + assert "|| true" not in fallback, ( + "the materialization retry must not swallow a hard failure" + ) + m = re.search( + r"if \(cd \"\$INSTALL_DIR\" \\\n" + r"\s*&& \(git reset --hard HEAD", + fallback, + ) + assert m is not None, ( + "the reset must be guarded: its success is the condition that sets " + "clone_ok, and a failed reset must clean up the checkout" + ) + + def test_partial_clone_failure_still_cleans_up_and_exits(): branch = _https_branch() m = re.search(