From f518bc8555aa5f296119eb4a8d514d91fe731877 Mon Sep 17 00:00:00 2001 From: indaco Date: Fri, 21 Aug 2026 21:26:42 +0200 Subject: [PATCH 1/2] fix(net): stop a Ctrl-C during a retry backoff from being slept off --- src/net/client.zig | 112 ++++++++++++++++++++++++++++--- tests/net_get_to_writer_test.zig | 59 ++++++++++++++++ 2 files changed, 161 insertions(+), 10 deletions(-) diff --git a/src/net/client.zig b/src/net/client.zig index fd766798..9c3a2ddc 100644 --- a/src/net/client.zig +++ b/src/net/client.zig @@ -1033,17 +1033,31 @@ pub const HttpClient = struct { 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. + /// How long the backoff may nap before re-checking `cancel`. Short + /// enough that a Ctrl-C is honoured promptly, long enough that a 4 s + /// wait is not thousands of wakeups. + const backoff_poll_slice_ms: u64 = 50; + + /// Cancellable backoff between retry attempts, shared by every retry + /// loop. std.Io cancellation is never armed in this process, so the wait + /// polls `cancel` itself - trusting `std.Io.sleep` alone would sleep a + /// Ctrl-C off and re-dial. fn retrySleep(self: *HttpClient, attempt: usize) error{Canceled}!void { - std.Io.sleep( - self.io, - 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, - }; + const total_ms = self.retry_backoff_ms[attempt]; + var waited: u64 = 0; + while (true) { + if (self.cancel) |cancelled| if (cancelled()) return error.Canceled; + if (waited >= total_ms) return; + const step: u64 = @min(backoff_poll_slice_ms, total_ms - waited); + std.Io.sleep( + self.io, + std.Io.Duration.fromNanoseconds(@intCast(step * std.time.ns_per_ms)), + .awake, + ) catch |e| switch (e) { + error.Canceled => return error.Canceled, + }; + waited += step; + } } fn doGetConditionalWithRetry( @@ -2619,3 +2633,81 @@ test "CountingWriter.sendFile: clamps the source so a file cannot overshoot the try std.testing.expect(cw.limit_exceeded); try std.testing.expectEqual(@as(usize, 10), inner.writer.end); } + +// Predicates for the retry-backoff cancellation tests. `TripsLater` proves the +// backoff keeps polling while it waits: a cancel that arrives after the sleep +// has already started is the real Ctrl-C case, and an up-front-only check +// would sleep straight through it. +const AlwaysCancel = struct { + fn cancel() bool { + return true; + } +}; +const NeverCancel = struct { + fn cancel() bool { + return false; + } +}; +const TripsLater = struct { + var calls: usize = 0; + fn reset() void { + calls = 0; + } + fn cancel() bool { + calls += 1; + return calls > 1; + } +}; + +test "retrySleep reports a cancel that is already pending" { + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = threaded.io() }; + var http = HttpClient.initWith(&inner, threaded.io(), std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + // A long budget: without the predicate poll this waits it out and reports + // success, which is the bug. + http.retry_backoff_ms = &.{4000}; + http.cancel = &AlwaysCancel.cancel; + + try std.testing.expectError(error.Canceled, http.retrySleep(0)); +} + +test "retrySleep observes a cancel that arrives mid-backoff" { + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = threaded.io() }; + var http = HttpClient.initWith(&inner, threaded.io(), std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + http.retry_backoff_ms = &.{200}; + TripsLater.reset(); + http.cancel = &TripsLater.cancel; + + try std.testing.expectError(error.Canceled, http.retrySleep(0)); + // More than one poll: the wait is sliced, not a single up-front check. + try std.testing.expect(TripsLater.calls > 1); +} + +test "retrySleep waits out the backoff when nothing cancels" { + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = threaded.io() }; + var http = HttpClient.initWith(&inner, threaded.io(), std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + http.retry_backoff_ms = &.{10}; + http.cancel = &NeverCancel.cancel; + + try http.retrySleep(0); +} + +test "retrySleep completes when no cancel predicate is wired" { + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = threaded.io() }; + var http = HttpClient.initWith(&inner, threaded.io(), std.process.Environ.empty, std.testing.allocator); + defer http.deinit(); + http.retry_backoff_ms = &.{10}; + http.cancel = null; + + try http.retrySleep(0); +} diff --git a/tests/net_get_to_writer_test.zig b/tests/net_get_to_writer_test.zig index 3c7ec6fc..6bbfe2df 100644 --- a/tests/net_get_to_writer_test.zig +++ b/tests/net_get_to_writer_test.zig @@ -20,6 +20,9 @@ const Mode = enum { ok, /// First GET answers 503 (a transient error net retries), later ones 200. err_then_ok, + /// Every GET answers 503, so the loop retries until its budget or a + /// cancel stops it. + always_err, /// First GET answers 401 (a non-transient status the caller re-auths on), /// later ones 200 — mirrors GHCR's re-auth handshake. unauth_then_ok, @@ -31,6 +34,15 @@ const Mode = enum { redirect_then_ok, }; +// Set by the always_err stub once it has answered an attempt; the cancel +// predicate below reads it so the cancel lands on the retry, not before the +// first request is even sent. +var served_one: std.atomic.Value(bool) = .init(false); + +fn cancelAfterFirstServe() bool { + return served_one.load(.acquire); +} + const Stub = struct { io: std.Io, listener: *net.Server, @@ -62,6 +74,12 @@ fn serve(s: *Stub) void { .status = .found, .extra_headers = &.{.{ .name = "location", .value = "/final" }}, }) catch return, + .always_err => { + // Signal that an attempt has been served, so the test's cancel + // predicate only trips once a retry is actually pending. + served_one.store(true, .release); + req.respond("service unavailable\n", .{ .status = .service_unavailable }) catch return; + }, .err_then_ok => if (s.get_count == 1) req.respond("service unavailable\n", .{ .status = .service_unavailable }) catch return else @@ -372,3 +390,44 @@ test "getToWriter trips ResponseTooLarge when the 200 body exceeds the blob cap" try std.testing.expectError(error.ResponseTooLarge, status); try std.testing.expect(sink.writer.buffered().len <= 8); } + +test "getToWriter stops re-dialling when a cancel lands on the retry backoff" { + // The backoff sits between a transient failure and the next attempt. Until + // it honoured the cancel predicate, a Ctrl-C there was slept off and the + // request re-dialled through the whole budget. One GET, not four, is the + // user-visible contract; the sliced polling itself is pinned by the unit + // tests next to `retrySleep`. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var addr = try net.IpAddress.parseIp4("127.0.0.1", 0); + var listener = try addr.listen(io, .{ .reuse_address = true }); + defer listener.deinit(io); + const port = listener.socket.address.getPort(); + + served_one.store(false, .release); + var stub = Stub{ .io = io, .listener = &listener, .mode = .always_err }; + const server_thread = try std.Thread.spawn(.{}, serve, .{&stub}); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/blobs/sha256:abc", .{port}); + + 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); + // A budget long enough that sleeping it off would be unmistakable. + http.retry_backoff_ms = &.{ 4000, 4000, 4000 }; + http.cancel = &cancelAfterFirstServe; + + var sink: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer sink.deinit(); + + const status = http.getToWriter(url, &.{}, &sink.writer, null); + + http.deinit(); + server_thread.join(); + + try std.testing.expectError(error.Canceled, status); + // The retry never happened: the cancel ended the wait instead of it. + try std.testing.expectEqual(@as(usize, 1), stub.get_count); +} From b026a0649079a4d223d0eb8c8136e5f92dfbaaa6 Mon Sep 17 00:00:00 2001 From: indaco Date: Fri, 21 Aug 2026 21:26:42 +0200 Subject: [PATCH 2/2] test(regressions): pin that a cancel ends the retry backoff --- .../retry-backoff-sleeps-off-ctrl-c.sh | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100755 scripts/regressions/retry-backoff-sleeps-off-ctrl-c.sh diff --git a/scripts/regressions/retry-backoff-sleeps-off-ctrl-c.sh b/scripts/regressions/retry-backoff-sleeps-off-ctrl-c.sh new file mode 100755 index 00000000..5dd9be28 --- /dev/null +++ b/scripts/regressions/retry-backoff-sleeps-off-ctrl-c.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# Regression: a Ctrl-C arriving during an HTTP retry backoff must end the wait, +# not be slept off and followed by another re-dial. +# +# The bug: HttpClient.retrySleep - the one backoff shared by all four retry +# loops - read cancellation only off std.Io.sleep's error.Canceled. Nothing in +# this process ever arms that channel, while the live stop signal is the +# `cancel` predicate main wires to the interrupt flag. So a cancel landing +# during a backoff was ignored for the whole schedule (up to 1s + 2s + 4s) and +# the request was re-dialled anyway. The doc comment claimed the opposite, +# which is what let it survive. +# +# The fix polls `cancel` before the wait and between short slices of it, so the +# stop is honoured promptly. The error.Canceled return plumbing already existed +# at every call site; only the trigger was dead. +# +# Driven through the inline unit tests: retrySleep is private and the contract +# is about a predicate plus a clock, so no registry or network is needed. Grep +# guards first so a deleted poll cannot go green vacuously. + +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) +cd "$ROOT" + +SRC="src/net/client.zig" + +# Both halves must survive: the predicate poll inside the backoff, and the +# sliced wait that lets a mid-backoff cancel be seen at all. +if ! grep -Fqs -- "backoff_poll_slice_ms" "$SRC"; then + echo "FAIL: the retry backoff no longer waits in cancellable slices" >&2 + exit 1 +fi +if ! grep -Fqs -- "retrySleep observes a cancel that arrives mid-backoff" "$SRC"; then + echo "FAIL: the mid-backoff cancellation test is missing" >&2 + exit 1 +fi + +BIN="$ROOT/zig-out/test-bin/lib_tests" +# 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 unit test binary (zig build test-bin)" >&2 + exit 1 +fi + +OUT=$("$BIN" 2>&1) && STATUS=0 || STATUS=$? +if [[ "$STATUS" -ne 0 ]]; then + echo "FAIL: a cancel during a retry backoff was slept off" >&2 + printf '%s\n' "$OUT" | grep -iE "FAIL \(|[0-9]+ failed|expected .* found|leaked" >&2 || true + exit 1 +fi + +echo "PASS: a Ctrl-C during a retry backoff ends the wait instead of re-dialling"