From 41b0c82f2e5e85f4260d971a2d4ea6adb71ce6dd Mon Sep 17 00:00:00 2001 From: indaco Date: Fri, 21 Aug 2026 08:56:31 +0200 Subject: [PATCH] fix(net): stop failing a cask install on a blip the download would have retried The HEAD walk that classifies a cask's artifact had no retry at all, so one reset connection was a hard install failure even though the download that follows would have retried the same hop three times. The download's wrapper had the opposite flaw: it retried every error, spending the full backoff before re-reporting a spent hop budget, a malformed redirect or an unparseable url. Both walks now share one rule - retry the transport, surface what the response already decided. The redirect decision and the classification walk each return a closed error set, so the split is derived from those sets rather than a hand written list, and the cask installer maps them exhaustively: a cancelled walk is no longer reported as a dead network. --- ...-walk-has-no-retry-parity-with-download.sh | 41 +++ src/cli/install.zig | 65 +++-- src/net/client.zig | 170 ++++++++++-- src/net/ghcr.zig | 8 +- tests/net_redirect_auth_test.zig | 262 +++++++++++++++++- 5 files changed, 486 insertions(+), 60 deletions(-) create mode 100755 scripts/regressions/head-classification-walk-has-no-retry-parity-with-download.sh diff --git a/scripts/regressions/head-classification-walk-has-no-retry-parity-with-download.sh b/scripts/regressions/head-classification-walk-has-no-retry-parity-with-download.sh new file mode 100755 index 00000000..4c694e45 --- /dev/null +++ b/scripts/regressions/head-classification-walk-has-no-retry-parity-with-download.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# Regression: a redirect walk must retry a transient fault and must not retry a +# deterministic one - on the classification walk and on the download alike. +# +# The HEAD walk that picks a cask's artifact type had no retry wrapper at all, +# so one reset connection during classification was a hard install failure even +# though the download that follows would have retried the identical hop three +# times. The download's wrapper had the opposite defect: it retried any error, +# so a chain that had already tripped the hop budget was re-walked twice more +# before surfacing the same, inevitable error. +# +# Both halves are behavioural, so this reruns the fixture binary rather than +# grepping the source: a precondition would pin the shape of the fix, not the +# property. Those tests already gate CI through `zig build test`; this script is +# the correlatable rerun. Runtime doubles as evidence - a suite that spends the +# backoff table on a deterministic failure takes seconds longer than one that +# does not. +# +# No network beyond loopback, no temp state, well under 30s. + +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) +cd "$ROOT" + +# Always rebuild so the binaries reflect current source. Zig's cache makes 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 + +BIN="$ROOT/zig-out/test-bin/net_redirect_auth_test" +OUT=$("$BIN" 2>&1) && STATUS=0 || STATUS=$? +if [[ "$STATUS" -ne 0 ]]; then + echo "FAIL: net_redirect_auth_test - a redirect walk retries the wrong class of failure" >&2 + printf '%s\n' "$OUT" | grep -iE "failed|leaked|panic" >&2 || true + exit 1 +fi + +echo "PASS: transient faults are retried on both walks, deterministic ones on neither" diff --git a/src/cli/install.zig b/src/cli/install.zig index feafc7e9..f2479dd4 100644 --- a/src/cli/install.zig +++ b/src/cli/install.zig @@ -1318,20 +1318,34 @@ fn mapApiFetchError(e: api_mod.ApiError) ?InstallError { /// Classify a failed artifact-URL resolution. A walk that never completed /// says nothing about the cask's format, so `.unknown` stays reserved for a -/// URL malt actually resolved and could not classify. -fn mapHeadResolveError(e: anyerror) InstallError { - // OOM is handled by the caller: it is not a property of the URL. +/// URL malt actually resolved and could not classify. `null` means the failure +/// is not a verdict on the URL, and the caller reports it itself. Exhaustive so +/// a new tag fails compilation here instead of defaulting to "network is down". +fn mapHeadResolveError(e: client_mod.HeadResolveError) ?InstallError { return switch (e) { // Both mean the artifact would be fetched in the clear. error.InsecureUrlScheme, error.TlsDowngradeRefused => InstallError.InsecureArchiveUrl, - else => InstallError.NetworkError, + error.OfflineRequired, + error.RequestFailed, + error.InvalidUrl, + error.HttpRedirectLocationMissing, + error.HttpRedirectLocationInvalid, + error.HttpRedirectLocationOversize, + error.TooManyHttpRedirects, + => InstallError.NetworkError, + // One is malt running out of memory, the other the user stopping. + error.OutOfMemory, error.Canceled => null, }; } /// HEAD-based fallback for extensionless cask URLs. /// Follows redirects to discover the real file extension. The walk's own /// error reaches the caller, which reports it before classifying. -fn resolveCaskArtifactViaHead(ctx: *const AppCtx, allocator: std.mem.Allocator, url: []const u8) !cask_mod.ArtifactType { +fn resolveCaskArtifactViaHead( + ctx: *const AppCtx, + allocator: std.mem.Allocator, + url: []const u8, +) client_mod.HeadResolveError!cask_mod.ArtifactType { var http = client_mod.HttpClient.init(ctx.io, ctx.environ, allocator); defer http.deinit(); http.offline = ctx.offline; @@ -1416,14 +1430,20 @@ fn installCask( // Extensionless URLs (e.g. download APIs that 302 to the real file): // resolve via HEAD to discover the final URL and Content-Disposition. if (artifact_type == .unknown) { - artifact_type = resolveCaskArtifactViaHead(ctx, allocator, cask.url) catch |e| switch (e) { - // Report the walk's own error — "NetworkError" would say less than - // the message already does. - error.OutOfMemory => return e, - else => { + artifact_type = resolveCaskArtifactViaHead(ctx, allocator, cask.url) catch |e| { + if (mapHeadResolveError(e)) |classified| { + // Report the walk's own error — "NetworkError" would say less + // than the message already does. sink.err("Could not resolve the download URL for '{s}': {s} — URL: {s}", .{ cask.token, @errorName(e), cask.url }); - return mapHeadResolveError(e); - }, + return classified; + } + // A cancelled walk is the user stopping: "could not resolve the + // download URL" would blame the tap for a Ctrl-C. + if (e == error.Canceled) { + sink.warn("Interrupted.", .{}); + return; + } + return e; }; } @@ -1646,15 +1666,24 @@ test "mapApiFetchError surfaces ApiUnreachable as NetworkError" { } test "mapHeadResolveError reports a dead walk as a network failure, not a format one" { - try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.RequestFailed)); - try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.OfflineRequired)); - try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.HttpRedirectLocationMissing)); - try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.TooManyHttpRedirects)); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.RequestFailed).?); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.OfflineRequired).?); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.HttpRedirectLocationMissing).?); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.HttpRedirectLocationInvalid).?); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.HttpRedirectLocationOversize).?); + try std.testing.expectEqual(InstallError.NetworkError, mapHeadResolveError(error.TooManyHttpRedirects).?); } test "mapHeadResolveError keeps a cleartext artifact URL distinct from a network failure" { - try std.testing.expectEqual(InstallError.InsecureArchiveUrl, mapHeadResolveError(error.InsecureUrlScheme)); - try std.testing.expectEqual(InstallError.InsecureArchiveUrl, mapHeadResolveError(error.TlsDowngradeRefused)); + try std.testing.expectEqual(InstallError.InsecureArchiveUrl, mapHeadResolveError(error.InsecureUrlScheme).?); + try std.testing.expectEqual(InstallError.InsecureArchiveUrl, mapHeadResolveError(error.TlsDowngradeRefused).?); +} + +test "mapHeadResolveError does not diagnose the cask when malt or the user stopped the walk" { + // Ctrl-C mid-classification used to be reported as "could not resolve the + // download URL", blaming the tap for the user's own interruption. + try std.testing.expect(mapHeadResolveError(error.Canceled) == null); + try std.testing.expect(mapHeadResolveError(error.OutOfMemory) == null); } test "mapApiFetchError leaves other ApiError variants for the path's own fallback" { diff --git a/src/net/client.zig b/src/net/client.zig index 7b02431a..8871d84b 100644 --- a/src/net/client.zig +++ b/src/net/client.zig @@ -27,6 +27,8 @@ pub const DownloadError = error{ pub const GetError = error{ OfflineRequired, RequestFailed, + /// The url does not parse. A property of the string, so no walk retries it. + InvalidUrl, TlsDowngradeRefused, /// A manifest handed us a cleartext origin for a payload nothing else /// vouches for. @@ -40,6 +42,30 @@ pub const GetError = error{ OutOfMemory, }; +/// What the shared redirect decision can produce. Closed (Rule U1) so the retry +/// policy can be derived from it: every member is settled by the response or by +/// memory, never by the transport - the invariant `isRetriableWalkError` leans +/// on. +pub const RedirectError = error{ + HttpRedirectLocationMissing, + HttpRedirectLocationInvalid, + HttpRedirectLocationOversize, + TooManyHttpRedirects, + TlsDowngradeRefused, + OutOfMemory, +}; + +/// Explicit error set for the classification walk. The cask installer switches +/// on it to tell a dead network from a cleartext artifact from a Ctrl-C - an +/// open set let one be reported as another. +pub const HeadResolveError = RedirectError || error{ + OfflineRequired, + RequestFailed, + InvalidUrl, + InsecureUrlScheme, + Canceled, +}; + /// What still vouches for a payload once the transport does not. /// /// Refusing every cleartext origin would cost the packages that are already @@ -305,6 +331,11 @@ pub const HttpClient = struct { /// it to exercise the cap without a multi-GiB fixture. blob_cap: usize = max_blob_bytes, + /// Backoff before each retried attempt; its length is the retry budget. + /// Defaults to the production schedule; tests shrink it so a retry can be + /// observed without paying its wall-clock. + retry_backoff_ms: []const u64 = &default_retry_backoff_ms, + /// Reused across requests; each HttpClient is borrowed single-threaded /// from a pool, so no concurrent access. zstd_window: ?[]u8 = null, @@ -490,7 +521,7 @@ pub const HttpClient = struct { /// /// `cli/tap.zig` already refuses `http://` when registering a tap; this /// applies the same rule to the URLs those taps hand back. - pub fn requireSecureOrigin(url: []const u8, integrity: Integrity) GetError!void { + pub fn requireSecureOrigin(url: []const u8, integrity: Integrity) error{InsecureUrlScheme}!void { if (std.mem.startsWith(u8, url, "https://")) return; // Only cleartext http is ever a candidate for the exemptions below; // `file://`, `ftp://` and an unparseable url stay refused either way. @@ -641,12 +672,34 @@ pub const HttpClient = struct { } /// HEAD with manual redirect follow — stdlib skips redirects on HEAD. - pub fn headResolved(self: *HttpClient, url: []const u8) !HeadResolved { + /// + /// Retried on the download's policy: this walk only classifies what the + /// download then fetches, so a blip the fetch would have survived must not + /// be fatal here. Each attempt re-walks from `url`; nothing pins a later + /// attempt to the chain an earlier one saw. + pub fn headResolved(self: *HttpClient, url: []const u8) HeadResolveError!HeadResolved { if (self.offline) return error.OfflineRequired; // The final url and Content-Disposition this returns pick a cask's // artifact type, and the pkg type reaches `sudo installer -target /`. // No digest covers a response header, so cleartext is refused outright. try requireSecureOrigin(url, .transport_only); + + var attempt: usize = 0; + while (true) { + if (self.headResolvedOnce(url)) |resolved| { + return resolved; + } else |err| { + if (isRetriableWalkError(err) and attempt < self.retry_backoff_ms.len) { + try self.retrySleep(attempt); + attempt += 1; + continue; + } + return err; + } + } + } + + fn headResolvedOnce(self: *HttpClient, url: []const u8) HeadResolveError!HeadResolved { // Build the result eagerly so a single errdefer covers every dupe // inside the redirect loop; on success the caller takes ownership. var resolved: HeadResolved = .{ @@ -661,7 +714,7 @@ pub const HttpClient = struct { // the caller picks a cask's artifact type from it. var hops: usize = 0; while (true) : (hops += 1) { - const uri = std.Uri.parse(resolved.final_url) catch return error.RequestFailed; + const uri = std.Uri.parse(resolved.final_url) catch return error.InvalidUrl; var req = self.client.request(.HEAD, uri, .{ .extra_headers = &.{}, @@ -696,8 +749,7 @@ pub const HttpClient = struct { // ---- internal helper ---- - const max_retries = 3; - const retry_delays_ms = [_]u64{ 1000, 2000, 4000 }; + const default_retry_backoff_ms = [_]u64{ 1000, 2000, 4000 }; /// Counts written bytes, enforces an upper bound mid-stream, and reports /// progress. On overflow `drain`/`sendFile` return `error.WriteFailed` @@ -869,13 +921,37 @@ pub const HttpClient = struct { return try body_writer.toOwnedSlice(); } + /// Failures no second walk can clear. Every redirect decision is one, plus + /// the refusals and exhaustions the walk raises on its own behalf; a tag + /// added to `RedirectError` joins this set without another edit. + const TerminalWalkError = RedirectError || error{ + /// The streaming walk's collapsed form of the malformed-redirect tags. + HttpRedirectInvalid, + InvalidUrl, + InsecureUrlScheme, + ResponseTooLarge, + OfflineRequired, + /// The user's answer, not a fault to sleep off. + Canceled, + }; + + /// Whether a second walk could plausibly reach a different answer. Only the + /// terminal side is closed: the buffered GET's set is still inferred, so an + /// unnamed tag arriving here defaults to retriable. + fn isRetriableWalkError(err: anyerror) bool { + inline for (@typeInfo(TerminalWalkError).error_set.?) |variant| { + if (err == @field(TerminalWalkError, variant.name)) return false; + } + return true; + } + /// Cancellable backoff between retry attempts. Common to every /// retry loop so a Ctrl-C during the wait surfaces immediately /// instead of being swallowed by std.Io.sleep's silent return. fn retrySleep(self: *HttpClient, attempt: usize) error{Canceled}!void { std.Io.sleep( self.io, - std.Io.Duration.fromNanoseconds(@intCast(retry_delays_ms[attempt] * std.time.ns_per_ms)), + std.Io.Duration.fromNanoseconds(@intCast(self.retry_backoff_ms[attempt] * std.time.ns_per_ms)), .awake, ) catch |e| switch (e) { error.Canceled => return error.Canceled, @@ -892,7 +968,7 @@ pub const HttpClient = struct { const result = self.doGetConditionalOnce(url, extra_headers); if (result) |resp| { if (classifyStatus(resp.status)) |dl_err| { - if (isTransientError(dl_err) and attempt < max_retries) { + if (isTransientError(dl_err) and attempt < self.retry_backoff_ms.len) { var r = resp; r.deinit(); try self.retrySleep(attempt); @@ -902,7 +978,7 @@ pub const HttpClient = struct { } return resp; } else |err| { - if (attempt < max_retries) { + if (isRetriableWalkError(err) and attempt < self.retry_backoff_ms.len) { try self.retrySleep(attempt); attempt += 1; continue; @@ -945,7 +1021,7 @@ pub const HttpClient = struct { /// Resolve a redirect `Location` (absolute or relative) against `base` /// into an owned absolute URL, dropping any userinfo. Uses std's own /// resolver so relative targets behave exactly as stdlib's auto-follow did. - fn resolveRedirectUrl(self: *HttpClient, base: std.Uri, location: []const u8) ![]const u8 { + fn resolveRedirectUrl(self: *HttpClient, base: std.Uri, location: []const u8) RedirectError![]const u8 { var buf: [8 * 1024]u8 = undefined; if (location.len > buf.len) return error.HttpRedirectLocationOversize; @memcpy(buf[0..location.len], location); @@ -969,7 +1045,7 @@ pub const HttpClient = struct { /// Shared by all three redirect loops so the rule cannot drift between them. /// Resolving first matters: a relative `Location` carries no scheme and /// would otherwise read as a downgrade. Caller owns the returned slice. - fn nextHopUrl(self: *HttpClient, base: std.Uri, location: []const u8) ![]const u8 { + fn nextHopUrl(self: *HttpClient, base: std.Uri, location: []const u8) RedirectError![]const u8 { const next = try self.resolveRedirectUrl(base, location); errdefer self.allocator.free(next); const next_uri = std.Uri.parse(next) catch return error.HttpRedirectLocationInvalid; @@ -989,7 +1065,7 @@ pub const HttpClient = struct { status: u16, location: ?[]const u8, hops: usize, - ) !?[]const u8 { + ) RedirectError!?[]const u8 { if (!isFollowableRedirect(status)) return null; const loc = location orelse return error.HttpRedirectLocationMissing; if (hops >= max_redirects) return error.TooManyHttpRedirects; @@ -1033,7 +1109,7 @@ pub const HttpClient = struct { var hops: usize = 0; while (true) : (hops += 1) { - const uri = try std.Uri.parse(current); + const uri = std.Uri.parse(current) catch return error.InvalidUrl; var req = try self.client.request(.GET, uri, .{ .extra_headers = live_creds, @@ -1096,25 +1172,21 @@ pub const HttpClient = struct { const result = self.doGetLimited(url, extra_headers, max_bytes, progress); if (result) |resp| { if (classifyStatus(resp.status)) |dl_err| { - if (isTransientError(dl_err) and attempt < max_retries) { + if (isTransientError(dl_err) and attempt < self.retry_backoff_ms.len) { resp.allocator.free(resp.body); // Cancellation is single-shot per task in std.Io — // swallowing it here means the caller's stop signal // is consumed by the backoff and never reaches the // next request, so propagate it as the result. - std.Io.sleep(self.io, std.Io.Duration.fromNanoseconds(@intCast(retry_delays_ms[attempt] * std.time.ns_per_ms)), .awake) catch |e| switch (e) { - error.Canceled => return error.Canceled, - }; + try self.retrySleep(attempt); attempt += 1; continue; } } return resp; } else |err| { - if (attempt < max_retries) { - std.Io.sleep(self.io, std.Io.Duration.fromNanoseconds(@intCast(retry_delays_ms[attempt] * std.time.ns_per_ms)), .awake) catch |e| switch (e) { - error.Canceled => return error.Canceled, - }; + if (isRetriableWalkError(err) and attempt < self.retry_backoff_ms.len) { + try self.retrySleep(attempt); attempt += 1; continue; } @@ -1157,7 +1229,7 @@ pub const HttpClient = struct { const result = self.followGetToWriter(url, extra_headers, sink, progress, &sink_committed); if (result) |status| { if (classifyStatus(status)) |dl_err| { - if (isTransientError(dl_err) and attempt < max_retries) { + if (isTransientError(dl_err) and attempt < self.retry_backoff_ms.len) { try self.retrySleep(attempt); attempt += 1; continue; @@ -1169,7 +1241,7 @@ pub const HttpClient = struct { // be rewound, so the caller owns recovery: surface the error // instead of retrying into a dirty sink. if (sink_committed) return err; - if (attempt < max_retries) { + if (isRetriableWalkError(err) and attempt < self.retry_backoff_ms.len) { try self.retrySleep(attempt); attempt += 1; continue; @@ -1198,7 +1270,7 @@ pub const HttpClient = struct { var hops: usize = 0; while (true) : (hops += 1) { - const uri = std.Uri.parse(current) catch return error.RequestFailed; + const uri = std.Uri.parse(current) catch return error.InvalidUrl; var req = self.client.request(.GET, uri, .{ .extra_headers = live_creds, @@ -1211,11 +1283,16 @@ pub const HttpClient = struct { var response = req.receiveHead(&redirect_buf) catch return error.RequestFailed; const status: u16 = @intFromEnum(response.head.status); + // Exhaustive on purpose: a new redirect outcome must be given a + // name here rather than defaulting into "malformed". const hop = self.nextRedirectHop(uri, status, response.head.location, hops) catch |e| switch (e) { error.OutOfMemory => return error.OutOfMemory, error.TlsDowngradeRefused => return error.TlsDowngradeRefused, error.TooManyHttpRedirects => return error.TooManyHttpRedirects, - else => return error.HttpRedirectInvalid, + error.HttpRedirectLocationMissing, + error.HttpRedirectLocationInvalid, + error.HttpRedirectLocationOversize, + => return error.HttpRedirectInvalid, }; if (hop) |next| { errdefer self.allocator.free(next); @@ -1896,6 +1973,51 @@ test "doGetWithRetry surfaces sleep cancellation on a later backoff" { try std.testing.expectEqual(@as(usize, 3), CancelSleepProbe.sleep_calls); } +test "headResolved surfaces sleep cancellation instead of finishing the backoff" { + // The classification walk gained a retry, so it also gained a window where + // Ctrl-C lands in the backoff. The cask installer reports that case as an + // interruption rather than a dead network, which only holds if the tag + // reaches it intact. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const cancel_io = CancelSleepProbe.wrap(threaded.io(), 1); + + var http = HttpClient.init(cancel_io, std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + + // Nothing listens on port 1, so the walk fails on connect and reaches the + // first backoff without waiting on the network. + const result = http.headResolved("http://127.0.0.1:1/extensionless"); + try std.testing.expectError(error.Canceled, result); + try std.testing.expectEqual(@as(usize, 1), CancelSleepProbe.sleep_calls); +} + +test "isRetriableWalkError: every redirect decision is terminal" { + // The predicate derives its terminal side from these sets. Asserting the + // derivation - rather than a hand-written list - is what stops a tag added + // to RedirectError from becoming retriable by omission. + inline for (@typeInfo(RedirectError).error_set.?) |variant| { + try std.testing.expect(!HttpClient.isRetriableWalkError(@field(RedirectError, variant.name))); + } +} + +test "isRetriableWalkError: transport faults retry, refusals and exhaustions do not" { + // The HEAD walk collapses every transport fault into RequestFailed, while + // the buffered GET leaks the stdlib tag - both must stay retriable. + try std.testing.expect(HttpClient.isRetriableWalkError(error.RequestFailed)); + try std.testing.expect(HttpClient.isRetriableWalkError(error.ReadFailed)); + try std.testing.expect(HttpClient.isRetriableWalkError(error.ConnectionResetByPeer)); + try std.testing.expect(HttpClient.isRetriableWalkError(error.WatchdogSpawnFailed)); + + // Re-walking any of these reaches the same answer, just later. + try std.testing.expect(!HttpClient.isRetriableWalkError(error.InvalidUrl)); + try std.testing.expect(!HttpClient.isRetriableWalkError(error.HttpRedirectInvalid)); + try std.testing.expect(!HttpClient.isRetriableWalkError(error.InsecureUrlScheme)); + try std.testing.expect(!HttpClient.isRetriableWalkError(error.ResponseTooLarge)); + try std.testing.expect(!HttpClient.isRetriableWalkError(error.OfflineRequired)); + try std.testing.expect(!HttpClient.isRetriableWalkError(error.Canceled)); +} + // ── Wake + watchdogLoop: poll(2)-based watchdog wake mechanism ───── test "Wake.signal: closing the write end produces POLLHUP on the read fd" { diff --git a/src/net/ghcr.zig b/src/net/ghcr.zig index 0259e98f..4aec28dd 100644 --- a/src/net/ghcr.zig +++ b/src/net/ghcr.zig @@ -275,10 +275,12 @@ pub const GhcrClient = struct { error.OfflineRequired, error.RequestFailed, error.TlsDowngradeRefused, - // A cleartext origin is a manifest defect, not a transient - // fault; it collapses here with the rest because the outer - // loop's retry is harmless (it will fail identically). + // A cleartext origin or an unparseable url is a manifest + // defect, not a transient fault; both collapse here with the + // rest because the outer loop's retry is harmless (it will + // fail identically). error.InsecureUrlScheme, + error.InvalidUrl, error.TooManyHttpRedirects, error.HttpRedirectInvalid, error.ResponseTooLarge, diff --git a/tests/net_redirect_auth_test.zig b/tests/net_redirect_auth_test.zig index ceb1fa9c..1f8a837e 100644 --- a/tests/net_redirect_auth_test.zig +++ b/tests/net_redirect_auth_test.zig @@ -27,6 +27,11 @@ const Hop = struct { // When set, the hop redirects this many times and answers 200 after, so a // single hop can stand in for a whole chain. redirects_left: ?usize = null, + // Drop the first request without answering it, so an otherwise healthy hop + // hands the client one transport failure. + fail_first: bool = false, + // Requests actually received, so a test can assert how many walks happened. + requests: usize = 0, }; fn serveOne(hop: *Hop) void { @@ -51,6 +56,11 @@ fn serveCount(hop: *Hop, count: usize) void { var req = srv.receiveHead() catch break; served_here = true; served += 1; + hop.requests += 1; + if (hop.fail_first) { + hop.fail_first = false; + break; // close mid-request: the client sees a dead connection + } answer(hop, &req); } // A connection carrying no request is `knock`: the client is done, so @@ -59,6 +69,24 @@ fn serveCount(hop: *Hop, count: usize) void { } } +// Serves `count` requests, then refuses: every further connection is accepted +// and closed at once. `serveCount` stops accepting at its quota, so an extra +// dial would block on the listen backlog instead of failing the test's count +// assertion. Ends on `knock`, like its bounded sibling. +fn serveCountThenRefuse(hop: *Hop, count: usize) void { + serveCount(hop, count); + // Knocked before the quota ran out: the test is already finished. + if (hop.requests < count) return; + while (true) { + const stream = hop.listener.accept(hop.io) catch return; + defer stream.close(hop.io); + var rbuf: [1024]u8 = undefined; + var reader = stream.reader(hop.io, &rbuf); + // A connection carrying no request is `knock`: the test is done. + _ = reader.interface.peekByte() catch return; + } +} + // Wakes a hop still parked in `accept`. Without it, a client that dials fewer // times than the fixture expects hangs the test instead of failing it - and CI // has no per-test timeout to cut that short. @@ -330,6 +358,8 @@ test "headResolved reports an unreachable origin instead of the untouched url" { var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); defer http.deinit(); + // The subject is the error surfaced, not the retry that precedes it. + http.retry_backoff_ms = &.{}; try std.testing.expectError(error.RequestFailed, http.headResolved(url)); } @@ -363,6 +393,9 @@ test "headResolved reports a dead hop instead of a half-walked resolution" { var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); defer http.deinit(); + // One walk only: hop 1 serves a single request, and the dead hop 2 behind it + // is what this asserts on. + http.retry_backoff_ms = &.{}; const result = http.headResolved(url); t1.join(); @@ -379,16 +412,20 @@ test "headResolved reports a redirect with no Location instead of the pre-hop ur defer l1.deinit(io); const p1 = l1.socket.address.getPort(); var hop1 = Hop{ .io = io, .listener = &l1, .status = .moved_permanently }; - const t1 = try std.Thread.spawn(.{}, serveOne, .{&hop1}); + // Refusing rather than bounded: a missing Location is settled by the + // response, so a walk that retried it would fail here instead of stalling. + const t1 = try std.Thread.spawn(.{}, serveCountThenRefuse, .{ &hop1, 1 }); var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); - defer http.deinit(); const result = http.headResolved(url); + // Drop the client, then knock: a refusing hop only stops on the knock. + http.deinit(); + knock(io, p1); t1.join(); try std.testing.expectError(error.HttpRedirectLocationMissing, result); @@ -418,16 +455,17 @@ test "headResolved reports an exhausted redirect walk instead of an un-fetched u .content_disposition = "attachment; filename=\"artifact.pkg\"", }; - // The walk sends exactly one request per hop in the cap pre- and post-fix, - // so the hop thread always drains and the join never hangs. - const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 1 }); - var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + // One walk's worth of requests; a second walk is refused rather than left + // to stall, so a loop that retries the tripped cap fails the count below. + const one_walk = client.HttpClient.max_redirects + 1; + const t1 = try std.Thread.spawn(.{}, serveCountThenRefuse, .{ &hop1, one_walk }); + const result = http.headResolved(url); // Drop the client first: the hop is parked reading the kept-alive // connection, and only closing it lets the hop reach `knock`. @@ -436,6 +474,9 @@ test "headResolved reports an exhausted redirect walk instead of an un-fetched u t1.join(); try std.testing.expectError(error.TooManyHttpRedirects, result); + // A spent budget is a property of the chain: re-walking it only delays the + // same answer. + try std.testing.expectEqual(one_walk, hop1.requests); } test "headResolved resolves a chain as long as the download can follow" { @@ -500,8 +541,9 @@ test "headResolved refuses a chain one hop longer than the download can follow" .redirects_left = client.HttpClient.max_redirects + 1, }; // Serves one request more than the walk may spend, so a loop that follows - // the extra hop reaches a terminal 200 and resolves instead of hanging. - const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 2 }); + // the extra hop reaches a terminal 200 and resolves instead of hanging; + // refusing after that keeps a retried cap trip from stalling either. + const t1 = try std.Thread.spawn(.{}, serveCountThenRefuse, .{ &hop1, client.HttpClient.max_redirects + 2 }); var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); @@ -514,10 +556,12 @@ test "headResolved refuses a chain one hop longer than the download can follow" knock(io, p1); t1.join(); - if (result) |r| { + // Assert before releasing: on failure `expectError` formats the payload, + // and a freed `final_url` would crash the report instead of printing it. + defer if (result) |r| { var resolved = r; resolved.deinit(); - } else |_| {} + } else |_| {}; try std.testing.expectError(error.TooManyHttpRedirects, result); } @@ -621,17 +665,17 @@ test "a streaming download names an over-long chain rather than calling it malfo .redirect_to = loc, .status = .found, }; - // A pre-body failure is retried, so the fixture has to outlast every - // attempt: each spends the whole budget before giving up. - const attempts = 4; - const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, attempts * (client.HttpClient.max_redirects + 1) }); - var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + // One walk's worth of requests; a second walk is refused rather than left + // to stall, so a loop that re-walks a spent cap fails the count below. + const one_walk = client.HttpClient.max_redirects + 1; + const t1 = try std.Thread.spawn(.{}, serveCountThenRefuse, .{ &hop1, one_walk }); + var sink: std.Io.Writer.Allocating = .init(std.testing.allocator); defer sink.deinit(); @@ -642,4 +686,192 @@ test "a streaming download names an over-long chain rather than calling it malfo try std.testing.expectError(error.TooManyHttpRedirects, status); try std.testing.expectEqualStrings("", sink.writer.buffered()); + // The download side of the same rule: no backoff spent on a chain whose + // budget is already gone. + try std.testing.expectEqual(one_walk, hop1.requests); +} + +test "a buffered download spends one walk on a chain whose budget is gone" { + // The third retry loop. Its two siblings are covered above; without this + // the buffered GET could keep re-walking a spent budget unnoticed. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/loop", .{p1}); + var hop1 = Hop{ .io = io, .listener = &l1, .redirect_to = loc, .status = .found }; + + const one_walk = client.HttpClient.max_redirects + 1; + const t1 = try std.Thread.spawn(.{}, serveCountThenRefuse, .{ &hop1, one_walk }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + + const result = http.getWithHeaders(url, &.{}, null, .transport_only); + http.deinit(); + knock(io, p1); + t1.join(); + + try std.testing.expectError(error.TooManyHttpRedirects, result); + try std.testing.expectEqual(one_walk, hop1.requests); +} + +test "a buffered download survives a hop that fails once" { + // The mirror of the classification test: the shared predicate is unit + // tested, but only a live fixture proves each loop is wired to it. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var hop1 = Hop{ .io = io, .listener = &l1, .fail_first = true }; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, 2 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/blob", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + http.retry_backoff_ms = &.{0}; + + const result = http.getWithHeaders(url, &.{}, null, .transport_only); + http.deinit(); + knock(io, p1); + t1.join(); + + var resp = try result; + defer resp.deinit(); + try std.testing.expectEqualStrings(blob_body, resp.body); + try std.testing.expectEqual(@as(usize, 2), hop1.requests); +} + +test "a url that cannot be parsed fails without spending the retry budget" { + // Deterministic: no attempt can parse what the first one could not. The + // walks used to call this a transport fault, which is retriable, so a + // malformed manifest url cost the whole backoff before failing. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + + // This pins the tag each walk reports; that the tag is terminal is asserted + // against the predicate in client.zig, where it can be checked directly. + // Passes the scheme guard, then fails `std.Uri.parse` on the port. + const bad = "https://example.com:port/artifact"; + try std.testing.expectError(error.InvalidUrl, http.headResolved(bad)); + try std.testing.expectError(error.InvalidUrl, http.getWithHeaders(bad, &.{}, null, .transport_only)); + + var sink: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer sink.deinit(); + try std.testing.expectError(error.InvalidUrl, http.getToWriter(bad, &.{}, &sink.writer, null)); +} + +test "a retried walk starts over at the origin rather than resuming mid-chain" { + // What makes retrying a classification safe: a fresh walk, not a resumed + // one. Resuming would hand back a url no attempt requested end to end - + // the half-walked resolution the cask installer must never classify from. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + var l2 = try bindIp4(io); + defer l2.deinit(io); + const p1 = l1.socket.address.getPort(); + const p2 = l2.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/artifact", .{p2}); + const cd = "attachment; filename=\"artifact.dmg\""; + var hop1 = Hop{ + .io = io, + .listener = &l1, + .redirect_to = loc, + .status = .found, + .content_disposition = cd, + }; + // Hop 2 drops the first request: the retry has to come back through hop 1. + var hop2 = Hop{ .io = io, .listener = &l2, .fail_first = true }; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, 2 }); + const t2 = try std.Thread.spawn(.{}, serveCount, .{ &hop2, 2 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + http.retry_backoff_ms = &.{0}; + + const result = http.headResolved(url); + http.deinit(); + knock(io, p1); + knock(io, p2); + t1.join(); + t2.join(); + + var resolved = try result; + defer resolved.deinit(); + + try std.testing.expectEqualStrings(loc, resolved.final_url); + try std.testing.expectEqualStrings(cd, resolved.content_disposition.?); + // Two full walks: the origin was re-requested, not skipped past. + try std.testing.expectEqual(@as(usize, 2), hop1.requests); + try std.testing.expectEqual(@as(usize, 2), hop2.requests); +} + +test "a hop that fails once is classified rather than reported as a network failure" { + // The classification walk feeds a download that retries the identical hop + // three times. Surfacing the first blip here fails an install that the very + // next step already knows how to survive. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + const cd = "attachment; filename=\"artifact.dmg\""; + var hop1 = Hop{ + .io = io, + .listener = &l1, + .fail_first = true, + .content_disposition = cd, + }; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, 2 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + // One retry, no wall-clock: the subject is whether the walk retries at all. + http.retry_backoff_ms = &.{0}; + + const result = http.headResolved(url); + http.deinit(); + knock(io, p1); + t1.join(); + + var resolved = try result; + defer resolved.deinit(); + + try std.testing.expectEqualStrings(url, resolved.final_url); + try std.testing.expectEqualStrings(cd, resolved.content_disposition.?); + try std.testing.expectEqual(@as(usize, 2), hop1.requests); }