From 831202b38ccf134efd1935108bbcf94aa165f2ae Mon Sep 17 00:00:00 2001 From: Carton He Date: Thu, 3 Sep 2026 17:35:11 +0800 Subject: [PATCH] Fix model download resume by sending RFC-compliant Range headers The contribute client already tried to resume interrupted model downloads by appending to the partial file and sending a Range header for the remaining bytes. However, the header omitted the byte-range unit required by RFC 7233, for example: Range: 100000000-211558521 instead of: Range: bytes=100000000-211558521 RFC-compliant servers treat the malformed header as if no Range was sent and reply 200 with the full content. The client then appended the full body at the old offset, corrupting the file. The receiver stopped once totalDataSize exceeded the expected size, and the "totalDataSize >= modelInfo.bytes" entry condition then returned a bogus success. This surfaced as the confusing error "Model file was incompletely downloaded, only got 271379114 bytes out of 271375520", after which the outer retry loop restarted the entire download. On flaky links this made every interruption cost a full re-download. The model host (media.katagotraining.org, GCS behind Cloudflare) supports Range requests when the header is well formed, returning 206 to a curl -r request. Fixes: - Send "Range: bytes=start-end" so compliant servers return 206 and the append-and-continue logic resumes correctly. - Validate status and Content-Range before streaming the response body. If a resumed request receives 200, or receives 206 with a missing or unexpected Content-Range, reject the body, discard the partial file, remember that Range is unavailable, and restart from byte 0. - Replace the "totalDataSize >= modelInfo.bytes" early return with an explicit overshoot check. Only retained bytes count as partial success; discarded data and interrupted full downloads after Range fallback consume the normal retry budget rather than resetting it indefinitely. - Reject non-200/206 response bodies before they can be written to the model file. - Log "Resuming download of model at byte N" when a retry resumes. Verification included the official katagotraining.org API and a local model mirror simulating two failure modes: (A) Range honored plus a mid-stream connection drop, resuming via 206 from the dropped offset; and (B) Range ignored with a 200 response plus a connection drop, falling back to a clean full download. Both produced the expected sha256. The combined change also builds successfully and passes the built-in test suite. --- cpp/distributed/client.cpp | 105 ++++++++++++++++++++++++++++++++++--- 1 file changed, 97 insertions(+), 8 deletions(-) diff --git a/cpp/distributed/client.cpp b/cpp/distributed/client.cpp index d6aad928aa..49c4800fe6 100644 --- a/cpp/distributed/client.cpp +++ b/cpp/distributed/client.cpp @@ -207,11 +207,15 @@ static httplib::Result oneShotDownload( const Url& proxyUrl, size_t startByte, //inclusive size_t endByte, //inclusive - std::function f + httplib::ResponseHandler responseHandler, + httplib::ContentReceiver contentReceiver ) { httplib::Headers headers; if(startByte > 0) { - headers.insert(std::make_pair("Range", Global::uint64ToString(startByte) + "-" + Global::uint64ToString(endByte))); + //Per RFC 7233 the Range header requires an explicit byte-range unit. Servers that + //receive a Range header without one treat it as malformed and ignore it, replying + //with 200 and the full content, which corrupts resumed downloads. + headers.insert(std::make_pair("Range", "bytes=" + Global::uint64ToString(startByte) + "-" + Global::uint64ToString(endByte))); } if(!url.isSSL) { @@ -224,7 +228,7 @@ static httplib::Result oneShotDownload( } //Avoid automatically decompressing .bin.gz files that get sent to us with "content-encoding: gzip" httpClient->set_decompress(false); - return httpClient->Get(url.path.c_str(),headers,f); + return httpClient->Get(url.path.c_str(),headers,responseHandler,contentReceiver); } else { std::unique_ptr httpsClient = std::make_unique(url.host, url.port); @@ -238,7 +242,7 @@ static httplib::Result oneShotDownload( httpsClient->enable_server_certificate_verification(true); //Avoid automatically decompressing .bin.gz files that get sent to us with "content-encoding: gzip" httpsClient->set_decompress(false); - httplib::Result response = httpsClient->Get(url.path.c_str(),headers,f); + httplib::Result response = httpsClient->Get(url.path.c_str(),headers,responseHandler,contentReceiver); if(response == nullptr) { auto result = httpsClient->get_openssl_verify_result(); if(result) { @@ -1012,16 +1016,83 @@ bool Connection::actuallyDownloadModel( double lastTime = timer.getSeconds(); size_t totalDataSize = 0; + bool serverIgnoresRange = false; + + //Throw away all partial data so far and begin again from byte 0. Used whenever we cannot + //trust that the bytes we have so far are a prefix of the true file, e.g. when a retry got + //a response that was not a valid continuation of the ranged request we made. + auto restartFromBeginning = [&]() { + out.close(); + FileUtils::open(out,tmpPath,ios::binary); + totalDataSize = 0; + }; auto fInner = [&](int& innerLoopFailMode) { - if(totalDataSize >= modelInfo.bytes) + if(totalDataSize > modelInfo.bytes) { + restartFromBeginning(); + throw StringError( + "Model download received more bytes than the model contains, discarding partial download and restarting from the beginning" + ); + } + //If we already learned that the server will not honor Range headers, resuming is not + //possible at all, so any partial data must be thrown away and downloaded in one piece. + if(serverIgnoresRange && totalDataSize > 0) + restartFromBeginning(); + if(totalDataSize == modelInfo.bytes) return; const size_t oldTotalDataSize = totalDataSize; const size_t startByte = oldTotalDataSize; const size_t endByte = modelInfo.bytes-1; + const bool requestResume = startByte > 0; + if(requestResume) + logger->write("Resuming download of model at byte " + Global::uint64ToString(startByte) + " of " + Global::uint64ToString(modelInfo.bytes) + ": " + urlToActuallyUse.originalString); const Url proxyToUse = mirrorUseProxy ? proxyUrl : Url(); + + //Validate response headers before the content receiver appends any bytes. In particular, + //cpp-httplib returns a null Result if the content receiver cancels after an overshoot, so + //waiting until after the body has been streamed would lose the 200 status that tells us + //the server ignored Range. + string responseHeaderError; + bool disableRangeResume = false; + httplib::ResponseHandler responseHandler = [&](const httplib::Response& headerResponse) { + if(headerResponse.status != 200 && headerResponse.status != 206) { + ostringstream outs; + outs << "Server gave response status code " << headerResponse.status << " instead of 200 OK or 206 Partial Content"; + for(const auto& header : headerResponse.headers) + outs << "\nHeader: " << header.first << ": " << header.second; + responseHeaderError = outs.str(); + return false; + } + + if(requestResume && headerResponse.status == 200) { + disableRangeResume = true; + responseHeaderError = + "Server ignored the Range header on a resumed download and sent the full file, " + "discarding partial download and restarting from the beginning"; + return false; + } + + if(requestResume && headerResponse.status == 206) { + string contentRange; + for(const auto& header : headerResponse.headers) { + if(Global::toLower(header.first) == "content-range") + contentRange = header.second; + } + const string expectedPrefix = string("bytes ") + Global::uint64ToString(startByte) + "-"; + if(contentRange == "" || Global::toLower(contentRange).rfind(expectedPrefix,0) != 0) { + disableRangeResume = true; + responseHeaderError = + "Server returned 206 Partial Content with " + + (contentRange == "" ? string("no Content-Range header") : string("unexpected Content-Range \"") + contentRange + "\"") + + ", discarding partial download and restarting from the beginning"; + return false; + } + } + return true; + }; + httplib::Result response = oneShotDownload( - logger, urlToActuallyUse, caCertsFile, proxyToUse, startByte, endByte, + logger, urlToActuallyUse, caCertsFile, proxyToUse, startByte, endByte, responseHandler, [&out,&totalDataSize,&shouldStop,this,&timer,&lastTime,&urlToActuallyUse,&modelInfo](const char* data, size_t data_length) { out.write(data, data_length); totalDataSize += data_length; @@ -1043,8 +1114,26 @@ bool Connection::actuallyDownloadModel( if(shouldStop()) throw StringError("Stopping because shouldStop is true"); - if(totalDataSize > oldTotalDataSize) - innerLoopFailMode = LOOP_PARTIAL_SUCCESS; + if(responseHeaderError != "") { + if(disableRangeResume) { + serverIgnoresRange = true; + restartFromBeginning(); + } + throw StringError(responseHeaderError); + } + + if(totalDataSize > modelInfo.bytes) { + restartFromBeginning(); + throw StringError( + "Model download received more bytes than the model contains, discarding partial download and restarting from the beginning" + ); + } + + if(totalDataSize > oldTotalDataSize) { + //Only retained bytes count as partial success. If Range is unavailable, the next retry + //must discard this incomplete full response, so it must consume the normal retry budget. + innerLoopFailMode = serverIgnoresRange ? LOOP_RETRYABLE_FAIL : LOOP_PARTIAL_SUCCESS; + } if(response == nullptr) throw StringError("No response from server");