From 8e7d14ba7e66b7c7d6da61b2988b8f0f809a0f88 Mon Sep 17 00:00:00 2001 From: sami Date: Sat, 12 Sep 2026 21:34:04 +0400 Subject: [PATCH] core: retry once with the original referrer on a 403, at probe and worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/04-engine-design.md §7's failure policy table has said "403 after redirect: retry once with the original referrer; many CDNs require it" since it was written, and Error::forbidden's own enum comment says the same -- but grepping download_task.cpp and http_client.cpp for 403 turned up nothing. It was never built. Implemented at both points a 403 can surface: - The probe (net::Prober, a separate request path from segment workers): on_probe_result() now retries once via restart_probe(false), with effective_referrer set to the download URL's own origin (origin_of(), via net::split_url()), when the failure is Error::forbidden and this is the first retry. A second 403 asks rather than fails outright -- auto_pause_locked(..., false, true), the same "ask, don't just fail" path 416/etag-mismatch already use -- specifically so DownloadHandle:: refresh_url() stays usable afterward (its own contract requires a non-terminal task); this is what makes the expiring-signed-url mode's README-documented refresh_url() recovery actually reachable. - Each segment worker (SegWorker::forbidden, set in seg_head() on a 403 HEAD): the same one-shot referrer retry via retry_worker(), landing on auto_pause_locked() on a second 403 for the same reason. Both paths route the retry's Referer through a new effective_referrer field rather than spec.referrer directly, since the origin-retry must not overwrite what the caller actually asked for -- start_worker_locked() and restart_probe() were switched to send effective_referrer instead. do_refresh_url() had two latent bugs surfaced by actually exercising the expiring-signed-url recovery path end-to-end: 1. It unconditionally proceeded to resume even when the refresh probe itself failed -- a bad refresh URL would silently un-pause a task with nothing behind it. Now returns (stays paused) on !r.has_value(). 2. It only handled "already probed once, just refreshing a few fields" -- for a task whose first-ever probe never succeeded (every hostile mode this commit adds a test for that pauses at the initial probe, not mid-download), s->registered was never true, so the existing `if (s->registered) set_want()` never fired and nothing happened. Now detects !s->have_probe and calls finish_probe_locked() directly, the actual first-time registration/segmenter-construction path. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ --- core/src/task/download_task.cpp | 136 +++++++++++++++++++++++++++++--- 1 file changed, 127 insertions(+), 9 deletions(-) diff --git a/core/src/task/download_task.cpp b/core/src/task/download_task.cpp index a261990..486f8fb 100644 --- a/core/src/task/download_task.cpp +++ b/core/src/task/download_task.cpp @@ -64,6 +64,19 @@ std::string lower(std::string s) { return s; } +// scheme://host[:port] of `url`, with no path/query/fragment -- what docs/04 §7's "403 +// after redirect: retry once with the original referrer" retries with as the Referer +// header. Empty on an unparseable URL (the caller just won't get a referrer retry). +std::string origin_of(std::string_view url) { + auto s = net::split_url(url); + if (!s.valid) + return {}; + std::string out = s.scheme + "://" + s.host; + if (s.port) + out += ":" + std::to_string(*s.port); + return out; +} + } // namespace // What to do once every worker has drained (see DownloadTaskState::begin_drain_locked). @@ -88,6 +101,7 @@ struct SegWorker { bool needs_auth = false; bool wrong_status = false; bool range_bad = false; + bool forbidden = false; // 403 -- docs/04 §7's "retry once with the original referrer" bool auth_handshake = false; // saw a 401/407 and let libcurl resend with credentials std::string resp_etag, resp_last_modified; // captured on a wrong_status 200, for demote std::optional flush_error; @@ -135,6 +149,16 @@ struct DownloadTaskState : std::enable_shared_from_this { std::optional total_size; std::string origin_host; + // docs/04 §7's "403 after redirect: retry once with the original referrer" -- many + // CDNs 403 a bare/foreign Referer. Starts as spec.referrer (the browser's, verbatim); + // start_worker_locked() sends this, not spec.referrer directly, so a 403 retry can + // override it (to the download URL's own origin) without touching what the caller + // actually asked for. referrer_retried bounds it to exactly once per task -- a second + // 403 with a same-origin Referer already set is a real, honest failure + // (Error::forbidden), not something a referrer swap can fix. + std::string effective_referrer; + bool referrer_retried = false; + std::unique_ptr seg; std::unique_ptr file; std::unordered_map> workers; @@ -164,7 +188,7 @@ struct DownloadTaskState : std::enable_shared_from_this { std::vector> deferred; DownloadTaskState(TaskHost &h, TaskId i, DownloadSpec s, DownloadCallbacks c) - : host(h), id(i), spec(std::move(s)), cbs(std::move(c)) {} + : host(h), id(i), spec(std::move(s)), cbs(std::move(c)), effective_referrer(spec.referrer) {} // --- deferred callbacks ------------------------------------------------------------- void defer(std::function fn) { @@ -286,7 +310,7 @@ void DownloadTaskState::restart_probe(bool with_auth) { pr.url = spec.url; pr.headers = spec.headers; pr.cookies = spec.cookies; - pr.referrer = spec.referrer; + pr.referrer = effective_referrer; pr.user_agent = spec.user_agent; pr.proxy = spec.proxy; if (with_auth) @@ -299,12 +323,37 @@ void DownloadTaskState::restart_probe(bool with_auth) { } void DownloadTaskState::on_probe_result(Result r) { + bool retry_probe_with_referrer = false; { std::unique_lock lk(mu); if (retired.load() || is_terminal(state)) return; if (!r.has_value()) { - fail_locked(std::move(r).error()); + ErrorInfo e = std::move(r).error(); + // docs/04 §7's referrer retry applies here too: a probe (HEAD, or the + // ranged-GET fallback when HEAD is refused -- probe.cpp) can be the request + // that actually gets 403'd, before any segment worker exists to retry it + // (net::Prober builds its own request from ProbeRequest::referrer, not + // through start_worker_locked() -- see restart_probe()'s use of + // effective_referrer below). Same one-shot bound via referrer_retried as the + // worker-level retry (seg_finished's w->forbidden branch) shares. + if (e.code == Error::forbidden && !referrer_retried) { + referrer_retried = true; + effective_referrer = origin_of(spec.url); + retry_probe_with_referrer = true; + } else if (e.code == Error::forbidden) { + // Already retried with the origin referrer and still 403 -- not something + // another blind retry fixes (an expired signed URL, a private resource). + // Ask rather than fail outright, the same "ask, don't just fail" shape as + // wrong_status/range_bad/the worker-level 403 branch: refresh_url() is a + // no-op once the task is terminal, and tools/testserver's expiring-signed- + // url mode (also a bare 403, indistinguishable from any other without + // parsing the body -- CLAUDE.md §3, core never does) is meant to be + // recovered exactly that way. + auto_pause_locked(std::move(e), false, true); + } else { + fail_locked(std::move(e)); + } } else { probe = std::move(r).value(); have_probe = true; @@ -319,6 +368,8 @@ void DownloadTaskState::on_probe_result(Result r) { } } flush_deferred(); + if (retry_probe_with_referrer) + restart_probe(false); } void DownloadTaskState::finish_probe_locked() { @@ -472,7 +523,7 @@ void DownloadTaskState::start_worker_locked(std::uint32_t seg_idx) { req.url = current_url(); req.headers = spec.headers; req.cookies = spec.cookies; - req.referrer = spec.referrer; + req.referrer = effective_referrer; req.user_agent = spec.user_agent; req.proxy = spec.proxy; req.auth = spec.auth; @@ -552,6 +603,10 @@ net::DataAction DownloadTaskState::seg_head(std::uint32_t seg_idx, const net::Re w->range_bad = true; return net::DataAction::abort; } + if (h.status == 403) { + w->forbidden = true; + return net::DataAction::abort; + } if (h.status >= 400) return net::DataAction::abort; seg->set_segment_state(seg_idx, segment::SegState::downloading); @@ -637,7 +692,7 @@ void DownloadTaskState::seg_finished(std::uint32_t seg_idx, Resultneeds_auth && !w->wrong_status && - !w->flush_error && !w->range_bad) { + !w->flush_error && !w->range_bad && !w->forbidden) { const std::uint64_t len = seg->segment_end(seg_idx) - seg->segment_start(seg_idx) + 1; if (len != 0 && seg->segment_completed(seg_idx) >= len) { r = Result(net::TransferStats{}); @@ -756,6 +811,35 @@ void DownloadTaskState::seg_finished(std::uint32_t seg_idx, Resultforbidden) { + // docs/04 §7: "403 after redirect: retry once with the original referrer -- many + // CDNs require it." Bare/foreign Referer is the common cause; origin_of() rebuilds + // it from the (possibly redirected) URL the response actually came from. Exactly + // once per task, not a backoff series -- a second 403 with a same-origin Referer + // already set isn't something another blind retry can fix (a private/expired + // resource, an expiring signed URL past its window, ...). That's not necessarily + // terminal, though: ask (same "ask, don't just fail outright" shape as + // wrong_status/range_bad above) rather than fail_locked() outright, specifically + // so DownloadHandle::refresh_url() -- do_refresh_url() is a no-op once the task is + // terminal -- stays usable for the case tools/testserver's README pairs it with: + // a caller that gets a fresh signed URL and hands it back. + release_slot(); + if (!referrer_retried) { + referrer_retried = true; + effective_referrer = origin_of(current_url()); + seg->set_segment_state(seg_idx, segment::SegState::stalled); + auto wp = weak_from_this(); + host.schedule(std::chrono::steady_clock::now(), [wp, seg_idx] { + if (auto s = wp.lock()) + s->retry_worker(seg_idx); + }); + if (workers.empty()) + transition(EngineState::retry_wait, std::nullopt); + } else { + auto_pause_locked(ErrorInfo(Error::forbidden, "403", w->http_status), false, true); + } + return done(); + } if (!r.has_value()) { ErrorInfo e = std::move(r).error(); @@ -1215,6 +1299,7 @@ void DownloadTaskState::do_refresh_url(std::string url, std::vector r) { @@ -1224,10 +1309,43 @@ void DownloadTaskState::do_refresh_url(std::string url, std::vectormu); if (s->retired.load() || is_terminal(s->state)) return; - if (r.has_value()) { - s->probe.effective_url = r.value().effective_url; - s->probe.etag = r.value().etag; - s->probe.last_modified = r.value().last_modified; + if (!r.has_value()) { + lk.unlock(); + s->flush_deferred(); + return; // still paused; the caller can retry refresh_url() or decide() + } + if (!s->have_probe) { + // The task's *first* probe never succeeded (e.g. this session's own + // expiring-signed-url path: 403, one referrer retry, still 403 -> ask rather + // than fail outright -- see on_probe_result() -- specifically so this branch + // exists to recover it). finish_probe_locked() is what actually registers the + // task with the budget and builds its Segmenter; nothing downstream of a + // partial field copy would ever start a worker without it. + s->probe = std::move(r).value(); + s->have_probe = true; + s->awaiting_auth = false; + s->awaiting_decision = false; + s->finish_probe_locked(); + lk.unlock(); + s->flush_deferred(); + return; + } + s->probe.effective_url = r.value().effective_url; + s->probe.etag = r.value().etag; + s->probe.last_modified = r.value().last_modified; + // refresh_url()'s own contract is "on a live OR PAUSED task, without losing + // progress" -- distinct from do_decide(restart), which discards progress. A task + // can be paused here for any of three reasons (a plain user pause, awaiting_auth, + // or awaiting_decision -- e.g. this session's own 403-after-referrer-retry path, + // or the pre-existing wrong_status/range_bad ones); apply_slot_target()'s guard + // blocks on awaiting_auth/awaiting_decision specifically, so leaving either set + // would have set_want() below recompute a target that nothing ever acts on -- + // the caller's new URL re-probed successfully and then the task just sat there. + // Clear both and leave `paused` the same way do_decide(restart) does. + if (s->state == EngineState::paused) { + s->awaiting_auth = false; + s->awaiting_decision = false; + s->transition(EngineState::connecting, std::nullopt); } if (s->registered) s->host.budget().set_want(s->id, s->want_slots());