Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.zig
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ pub fn build(b: *std.Build) void {
"tests/ghcr_401_retry_test.zig",
"tests/net_get_to_writer_test.zig",
"tests/bottle_download_test.zig",
"tests/install_download_cancel_test.zig",
"tests/linker_core_test.zig",
"tests/supervisor_pure_test.zig",
"tests/install_pure_test.zig",
Expand Down
58 changes: 58 additions & 0 deletions scripts/regressions/bottle-download-retry-swallows-ctrl-c.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
#!/usr/bin/env bash
# Regression: a Ctrl-C during a bottle download must end the retry loop, not
# be re-dialled and reported as a network failure.
#
# The bug: downloadBottleToStore's three-attempt loop had no cancellation
# input at all. Its only stop signal was a backoff helper reading cancellation
# off std.Io.sleep - a channel nothing in the process ever arms - while the
# real interrupt state lives in the signals module the same file already
# imports. A Ctrl-C mid-download therefore burned all three attempts, slept
# both backoffs, and ended with a per-formula "DownloadFailed (after 3
# attempts)" line before the install loop finally reported the interruption.
#
# The fix polls the interrupt flag once per attempt and breaks, which also
# skips the attempts-exhausted line, so the interruption is reported once by
# the caller.
#
# The CLI cannot reach this path offline: MALT_BOTTLE_DOMAIN is https-only, so
# no cleartext loopback registry is reachable from the binary, and driving real
# GHCR would need network plus a race-prone signal. The contract is pinned by
# the integration test instead; this script builds and runs only that binary.
# Hermetic, no network, well under 30s - the test owns and cleans its own temp
# prefix, so nothing is left behind here.

set -euo pipefail

ROOT=$(cd "$(dirname "$0")/../.." && pwd)
cd "$ROOT"

SRC="src/cli/install/download.zig"
TEST="tests/install_download_cancel_test.zig"

# A dropped guard or a dropped test would let the binary go green vacuously.
# Fail loudly instead: both must still be present.
if ! grep -Fqs -- "signals.isInterrupted()" "$SRC"; then
echo "FAIL: the interrupt poll is missing from the bottle retry loop" >&2
exit 1
fi
if ! grep -Fqs -- "setInterruptedForTest" "$TEST"; then
echo "FAIL: the download cancellation integration test is missing" >&2
exit 1
fi

BIN="$ROOT/zig-out/test-bin/install_download_cancel_test"
# Always rebuild so the binary reflects current source; zig's cache keeps a
# no-op rebuild cheap.
if ! zig build test-bin >/dev/null 2>&1; then
echo "FAIL: could not build the test binaries (zig build test-bin)" >&2
exit 1
fi

OUT=$("$BIN" 2>&1) && STATUS=0 || STATUS=$?
if [[ "$STATUS" -ne 0 ]]; then
echo "FAIL: a set interrupt flag did not stop the bottle retry loop after one attempt" >&2
printf '%s\n' "$OUT" | grep -iE "failed|expected|leaked|panic" >&2 || true
exit 1
fi

echo "PASS: a Ctrl-C during a bottle download stops the retry loop at one attempt"
9 changes: 8 additions & 1 deletion src/cli/install.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1006,10 +1006,17 @@ fn runInstall(
// each warmed bottle's `<prefix>/store/<sha>` path so a follow-up
// real install can consume the bytes.
if (flags.download_only) {
// This branch returns before the shared interrupt check below, so it
// owns its own: without it a Ctrl-C reads as a wall of download
// failures. Polled once - the flag drives every job in the loop.
const interrupted = signals.isInterrupted();
if (interrupted) sink.warn("Interrupted. Cleaning up...", .{});
for (all_jobs.items) |job| {
if (!job.succeeded) {
output.emitNdjsonEvent(.download_complete, job.name, "failed");
sink.err("Download failed for {s}", .{job.name});
// A cancelled job is not a download failure; the interruption
// is already reported once, above.
if (!interrupted) sink.err("Download failed for {s}", .{job.name});
failed_count += 1;
continue;
}
Expand Down
110 changes: 47 additions & 63 deletions src/cli/install/download.zig
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,16 @@ pub fn progressBridge(ctx: *anyopaque, bytes_so_far: u64, content_length: ?u64)
bar.update(clamped);
}

/// Sleeps for `ms` between retries. Returns false when the caller's
/// stop signal cancels the sleep, true otherwise. std.Io cancellation
/// is single-shot per task, so a swallowed Canceled would silently
/// consume the request and the loop would keep retrying.
fn cancellableBackoff(io: std.Io, ms: u64) bool {
std.Io.sleep(io, std.Io.Duration.fromNanoseconds(@intCast(ms * std.time.ns_per_ms)), .awake) catch |e| switch (e) {
error.Canceled => return false,
};
return true;
/// At file scope so the invariants between the two can be pinned by a unit
/// test, without standing up a registry.
const max_download_attempts: u8 = 3;
const download_retry_delays_ms = [_]u64{ 100, 400 };

/// True only when the loop spent its whole budget. Every early exit - an
/// interrupt included - leaves `dl_attempt` short of it, which is what keeps
/// the attempts-exhausted diagnostic off the cancellation path.
fn attemptsExhausted(dl_attempt: u8) bool {
return dl_attempt >= max_download_attempts;
}

/// Errors that re-running the download cannot rescue: the cause is in the
Expand Down Expand Up @@ -637,13 +638,16 @@ pub fn downloadBottleToStore(
.func = &progressBridge,
} else null;

const max_attempts: u8 = 3;
const retry_delays_ms = [_]u64{ 100, 400 };
var dl_attempt: u8 = 0;
var dl_ok = false;
var last_err: bottle_mod.BottleError = bottle_mod.BottleError.DownloadFailed;
var last_mismatch: ?bottle_mod.MismatchInfo = null;
while (dl_attempt < max_attempts) : (dl_attempt += 1) {
while (dl_attempt < max_download_attempts) : (dl_attempt += 1) {
// Re-dialling after a Ctrl-C reports the user's own stop as a
// network failure. Breaking short of the budget also suppresses the
// attempts-exhausted line, so the caller reports it once.
if (signals.isInterrupted()) break;

var mismatch: bottle_mod.MismatchInfo = undefined;
if (bottle_mod.download(
ctx.io,
Expand Down Expand Up @@ -672,8 +676,11 @@ pub fn downloadBottleToStore(
deps.sink.err(" {s}: {s}", .{ formula.name, @errorName(dl_err) });
break;
}
if (dl_attempt + 1 < max_attempts) {
if (!cancellableBackoff(ctx.io, retry_delays_ms[dl_attempt])) break;
if (dl_attempt + 1 < max_download_attempts) {
const delay = download_retry_delays_ms[dl_attempt] * std.time.ns_per_ms;
// A cancelled sleep needs no special case: the next iteration
// polls the flag before spending another attempt.
std.Io.sleep(ctx.io, std.Io.Duration.fromNanoseconds(@intCast(delay)), .awake) catch {};
}
}
}
Expand All @@ -691,8 +698,15 @@ pub fn downloadBottleToStore(

if (!dl_ok) {
if (deps.bar) |bar| bar.finish();
if (dl_attempt >= max_attempts) {
deps.sink.err(" {s}: {s} (after {d} attempts)", .{ formula.name, @errorName(last_err), max_attempts });
// A failed attempt wipes the temp dir itself, but an interrupt can
// break before the first one runs. deleteTree no-ops when it is
// already gone, so this covers every failing exit.
atomic.cleanupTempDir(ctx.io, tmp_dir);
// The flag can also be raised while the last attempt is in flight,
// which the loop's own poll can no longer catch - re-check so that
// window reports as a cancel rather than an exhausted budget.
if (attemptsExhausted(dl_attempt) and !signals.isInterrupted()) {
deps.sink.err(" {s}: {s} (after {d} attempts)", .{ formula.name, @errorName(last_err), max_download_attempts });
}
allocator.free(tmp_dir);
return InstallError.DownloadFailed;
Expand Down Expand Up @@ -946,56 +960,26 @@ test "MaterializeResult.kegPath reflects keg_path_len after write" {
try std.testing.expectEqualStrings(path, r.kegPath());
}

// Patches an inner Io's vtable so `sleep` reports cancellation on the
// configured call index; non-canceled sleeps return immediately so the
// retry-table delays don't pad the test runtime.
const CancelSleepProbe = struct {
var vtable: std.Io.VTable = undefined;
var sleep_calls: usize = 0;
var cancel_at: usize = 1;

fn wrap(inner: std.Io, cancel_at_call: usize) std.Io {
vtable = inner.vtable.*;
vtable.sleep = sleepMaybeCanceled;
sleep_calls = 0;
cancel_at = cancel_at_call;
return .{ .userdata = inner.userdata, .vtable = &vtable };
test "attemptsExhausted is false for every attempt an early break can reach" {
// The interrupt poll breaks from inside the loop body, so `dl_attempt` is
// always below the budget there. That is the whole mechanism keeping the
// "(after N attempts)" line off a cancelled download - if this ever went
// true early, a Ctrl-C would be misreported as a network failure again.
var i: u8 = 0;
while (i < max_download_attempts) : (i += 1) {
try std.testing.expect(!attemptsExhausted(i));
}

fn sleepMaybeCanceled(_: ?*anyopaque, _: std.Io.Timeout) std.Io.Cancelable!void {
sleep_calls += 1;
if (sleep_calls >= cancel_at) return error.Canceled;
}
};

test "cancellableBackoff returns true when sleep completes normally" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
// Zero-ms request keeps the test deterministic without exercising the
// real clock; a successful sleep must report true so the retry loop
// moves on to the next attempt.
try std.testing.expect(cancellableBackoff(threaded.io(), 0));
}

test "cancellableBackoff returns false when sleep is cancelled" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
const cancel_io = CancelSleepProbe.wrap(threaded.io(), 1);
try std.testing.expect(!cancellableBackoff(cancel_io, 100));
try std.testing.expectEqual(@as(usize, 1), CancelSleepProbe.sleep_calls);
try std.testing.expect(attemptsExhausted(max_download_attempts));
}

test "cancellableBackoff propagates cancellation when called repeatedly" {
var threaded: std.Io.Threaded = .init(std.testing.allocator, .{});
defer threaded.deinit();
// Cancel the third call: the prior two complete and report true,
// pinning that the helper isn't inadvertently latched after the
// first non-cancelled sleep.
const cancel_io = CancelSleepProbe.wrap(threaded.io(), 3);
try std.testing.expect(cancellableBackoff(cancel_io, 0));
try std.testing.expect(cancellableBackoff(cancel_io, 0));
try std.testing.expect(!cancellableBackoff(cancel_io, 0));
try std.testing.expectEqual(@as(usize, 3), CancelSleepProbe.sleep_calls);
test "the backoff table covers every gap between attempts" {
// The loop indexes the table by attempt number on all but the last
// attempt, so a budget raised without extending the table would panic on
// a real retry - a path no offline test would otherwise reach.
try std.testing.expectEqual(
@as(usize, max_download_attempts - 1),
download_retry_delays_ms.len,
);
}

// ---------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions tests/all.zig
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ comptime {
_ = @import("info_cli_test.zig");
_ = @import("info_test.zig");
_ = @import("install_ambiguity_test.zig");
_ = @import("install_download_cancel_test.zig");
_ = @import("install_download_only_test.zig");
_ = @import("install_execute_test.zig");
_ = @import("install_idempotent_test.zig");
Expand Down
Loading
Loading