Skip to content

More details in snapshot fetch errors #7638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
achamayou wants to merge 9 commits into
base: main
Choose a base branch
from
+15 −7
Open

More details in snapshot fetch errors #7638

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7a4cce8
More details in snapshot fetch errors
achamayou Feb 2, 2026
1b4c3e2
Merge branch 'main' into details_in_http_response_status
achamayou Feb 2, 2026
4e373cb
Merge branch 'main' into details_in_http_response_status
achamayou Feb 3, 2026
e719c1f
Merge branch 'main' into details_in_http_response_status
achamayou Feb 3, 2026
8a6ccb7
Apply suggestions from code review
achamayou Feb 4, 2026
c4c8ce7
fix
achamayou Feb 4, 2026
004e4b9
Update src/snapshots/fetch.h
eddyashton Feb 5, 2026
655a119
Merge branch 'main' into details_in_http_response_status
eddyashton Feb 5, 2026
3d7e23c
Merge branch 'main' into details_in_http_response_status
eddyashton Feb 5, 2026
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
22 changes: 15 additions & 7 deletions src/snapshots/fetch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,21 @@
#include <string>
#include <vector>

#define EXPECT_HTTP_RESPONSE_STATUS(request, status_code, expected) \
#define EXPECT_HTTP_RESPONSE_STATUS( \
request, status_code, expected, response_body) \
do \
{ \
if (status_code != expected) \
{ \
std::string response_content( \
response_body->buffer.begin(), response_body->buffer.end()); \
Comment on lines +27 to +28
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the response buffer contains binary data or non-UTF8 content, constructing a string directly from the buffer could result in invalid UTF-8 sequences or unprintable characters in the error message. Consider sanitizing the content for display, for example by using a hex dump for binary data or escaping non-printable characters.

Suggested change
std::string response_content( \
response_body->buffer.begin(), response_body->buffer.end()); \
std::string response_content; \
bool has_non_printable = false; \
for (const auto& ch : response_body->buffer) \
{ \
unsigned char uch = static_cast<unsigned char>(ch); \
if ((uch >= 0x20 && uch <= 0x7e) || uch == '\r' || uch == '\n' || uch == '\t') \
{ \
response_content.push_back(static_cast<char>(uch)); \
} \
else \
{ \
has_non_printable = true; \
break; \
} \
} \
if (has_non_printable) \
{ \
response_content = fmt::format( \
"<{} bytes of non-text response data>", response_body->buffer.size()); \
} \

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +28
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response_body pointer could be null according to the CurlRequest constructor (line 456-463 in curl.h). Dereferencing it without a null check will cause a crash. Consider adding a null check before accessing response_body->buffer, or handle the case where response_body is null by providing a default message like "(no response body)".

Suggested change
std::string response_content( \
response_body->buffer.begin(), response_body->buffer.end()); \
std::string response_content; \
if (response_body != nullptr) \
{ \
response_content.assign( \
response_body->buffer.begin(), response_body->buffer.end()); \
} \
else \
{ \
response_content = "(no response body)"; \
} \

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +28
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing a string from the entire response buffer could be problematic for large responses or binary data. Since snapshots can be "extremely large" (as noted in the comment at lines 198-199), this could cause memory issues or create an unwieldy error message. Consider either: (1) truncating the response content to a reasonable size (e.g., first 500 bytes), or (2) only including the response body for specific error codes that are expected to have small, informative responses (like 400 Bad Request).

Copilot uses AI. Check for mistakes.
throw std::runtime_error(fmt::format( \
"Expected {} response from {} {}, instead received {}", \
"Expected {} response from {} {}, instead received {} ({})", \
ccf::http_status_str(expected), \
request->get_method().c_str(), \
request->get_url(), \
status_code)); \
status_code, \
response_content)); \
} \
} while (0)

Expand Down Expand Up @@ -191,8 +195,8 @@ namespace snapshots

// Get snapshot. This may be redirected multiple times, and we follow
// these redirects ourself so we can extract the final URL. Once the
// redirects terminate, the final response is likely to be extremely large
// so is fetched over multiple requests for a sub-range, returning
// redirects terminate, the final response is likely to be extremely
// large so is fetched over multiple requests for a sub-range, returning
// PARTIAL_CONTENT each time.
std::string snapshot_url =
fmt::format("https://{}/node/snapshot", peer_address);
Expand Down Expand Up @@ -318,7 +322,10 @@ namespace snapshots
}

EXPECT_HTTP_RESPONSE_STATUS(
request, status_code, HTTP_STATUS_PERMANENT_REDIRECT);
request,
status_code,
HTTP_STATUS_PERMANENT_REDIRECT,
request->get_response_ptr());

char* redirect_url = nullptr;
CHECK_CURL_EASY_GETINFO(
Expand Down Expand Up @@ -383,7 +390,8 @@ namespace snapshots
EXPECT_HTTP_RESPONSE_STATUS(
snapshot_range_request,
snapshot_range_status_code,
HTTP_STATUS_PARTIAL_CONTENT);
HTTP_STATUS_PARTIAL_CONTENT,
snapshot_range_request->get_response_ptr());

process_partial_response(*snapshot_range_request);

Expand Down