Skip to content

Correctly handle import errors in LoadImage() and ImportImage(), fix broken ExportContainer test case #14483

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
OneBlue wants to merge 1 commit into
base: feature/wsl-for-apps
Choose a base branch
from
+89 −22
Open

Correctly handle import errors in LoadImage() and ImportImage(), fix broken ExportContainer test case #14483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/windows/inc/docker_schema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ struct ErrorResponse
NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(ErrorResponse, message);
};

struct ImageLoadResult
{
std::optional<std::string> stream;
std::optional<ErrorResponse> errorDetail;

NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT(ImageLoadResult, stream, errorDetail);
};

struct EmptyRequest
{
using TResponse = void;
Expand Down
62 changes: 45 additions & 17 deletions src/windows/wslasession/WSLASession.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,28 +597,56 @@ void WSLASession::ImportImageImpl(DockerHTTPClient::HTTPRequestContext& Request,

auto io = CreateIOContext();

std::optional<boost::beast::http::status> importResult;

std::optional<std::string> pendingErrorJson;
auto onHttpResponse = [&](const boost::beast::http::message<false, boost::beast::http::buffer_body>& response) {
WSL_LOG("ImageImportHttpResponse", TraceLoggingValue(static_cast<int>(response.result()), "StatusCode"));

importResult = response.result();
if (response.result_int() != 200)
{
auto it = response.find(boost::beast::http::field::content_type);

THROW_HR_IF_MSG(
E_UNEXPECTED,
it == response.end() || !it->value().starts_with("application/json"),
"Received HTTP %i but Content-Type is not json",
response.result_int());

pendingErrorJson.emplace();
}
};

std::string errorJson;
std::optional<std::string> errorMessage;
auto onProgress = [&](const gsl::span<char>& buffer) {
WI_ASSERT(importResult.has_value());
if (pendingErrorJson.has_value())
{
// If we received a non-200 status code, then the response body is an error message. Accumulate to the error message.
pendingErrorJson->append(buffer.data(), buffer.size());
return;
}

auto parsed = shared::FromJson<docker_schema::ImageLoadResult>(std::string(buffer.begin(), buffer.end()).c_str());

if (importResult.value() != boost::beast::http::status::ok)
if (parsed.errorDetail.has_value())
{
// If the import failed, accumulate the error message.
errorJson.append(buffer.data(), buffer.size());
if (errorMessage.has_value())
{
LOG_HR_MSG(
E_UNEXPECTED,
"Overriding previous error message '%hs' with new message '%hs'",
errorMessage->c_str(),
parsed.errorDetail->message.c_str());
}

errorMessage = std::move(parsed.errorDetail->message);
}
else
else if (parsed.stream.has_value())
{
// TODO: report progress to caller.
std::string entry = {buffer.begin(), buffer.end()};
WSL_LOG("ImageImportProgress", TraceLoggingValue(entry.c_str(), "Content"));
WSL_LOG("ImageImportProgress", TraceLoggingValue(parsed.stream->c_str(), "Content"));
}
else
{
LOG_HR_MSG(E_UNEXPECTED, "Failed to parse import progress: %.*hs", static_cast<int>(buffer.size()), buffer.data());
}
};

Expand All @@ -631,16 +659,16 @@ void WSLASession::ImportImageImpl(DockerHTTPClient::HTTPRequestContext& Request,

io.Run({});

THROW_HR_IF(E_UNEXPECTED, !importResult.has_value());

if (importResult.value() != boost::beast::http::status::ok)
// Look for an error message returned as an HTTP response (non HTTP 200)
if (pendingErrorJson.has_value())
{
// Import failed, parse the error message.
auto error = wsl::shared::FromJson<docker_schema::ErrorResponse>(errorJson.c_str());
auto error = wsl::shared::FromJson<docker_schema::ErrorResponse>(pendingErrorJson->c_str());

// TODO: Return error message to client.
THROW_HR_WITH_USER_ERROR(E_FAIL, error.message);
}

// Otherwise look for an error message returned via the progress stream (HTTP 200 followed by a stream error).
THROW_HR_WITH_USER_ERROR_IF(E_FAIL, errorMessage.value(), errorMessage.has_value());
}

HRESULT WSLASession::SaveImage(ULONG OutHandle, LPCSTR ImageNameOrID, IProgressCallback* ProgressCallback, HANDLE CancelEvent)
Expand Down
41 changes: 36 additions & 5 deletions test/windows/WSLATests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,6 @@ class WSLATests
ExpectImagePresent(*m_defaultSession, "debian:latest", true);
}

// TODO: Test that invalid tars are correctly handled.
TEST_METHOD(LoadImage)
{
WSL2_TEST_ONLY();
Expand All @@ -768,9 +767,18 @@ class WSLATests

VERIFY_ARE_EQUAL(0, result.Code);
VERIFY_IS_TRUE(result.Output[1].find("Hello from Docker!") != std::string::npos);

// Validate that invalid tars fail with proper error message and code.
{
auto currentExecutableHandle = wil::open_file(wil::GetModuleFileNameW<std::wstring>().c_str());
VERIFY_IS_TRUE(GetFileSizeEx(currentExecutableHandle.get(), &fileSize));

VERIFY_ARE_EQUAL(m_defaultSession->LoadImage(HandleToULong(currentExecutableHandle.get()), nullptr, fileSize.QuadPart), E_FAIL);

ValidateCOMErrorMessage(L"archive/tar: invalid tar header");
}
}

// TODO: Test that invalid tars are correctly handled.
TEST_METHOD(ImportImage)
{
WSL2_TEST_ONLY();
Expand Down Expand Up @@ -802,6 +810,20 @@ class WSLATests
VERIFY_ARE_EQUAL(
m_defaultSession->ImportImage(HandleToULong(imageTarFileHandle.get()), "my-hello-world", nullptr, fileSize.QuadPart), E_INVALIDARG);
}

// Validate that invalid tars fail with proper error message and code.
{
auto currentExecutableHandle = wil::open_file(wil::GetModuleFileNameW<std::wstring>().c_str());

VERIFY_IS_TRUE(GetFileSizeEx(currentExecutableHandle.get(), &fileSize));

VERIFY_ARE_EQUAL(
m_defaultSession->ImportImage(
HandleToULong(currentExecutableHandle.get()), "invalid-image:test", nullptr, fileSize.QuadPart),
E_FAIL);

ValidateCOMErrorMessage(L"archive/tar: invalid tar header");
}
}

TEST_METHOD(DeleteImage)
Expand Down Expand Up @@ -1785,10 +1807,19 @@ class WSLATests
VERIFY_IS_FALSE(INVALID_HANDLE_VALUE == containerTarFileHandle.get());
LARGE_INTEGER fileSize{};
VERIFY_IS_TRUE(GetFileSizeEx(containerTarFileHandle.get(), &fileSize));
Copy link
Collaborator Author

@OneBlue OneBlue Mar 19, 2026

Choose a reason for hiding this comment

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

This test case was actually silently failing to load the image and was reusing an image from another test case

VERIFY_SUCCEEDED(m_defaultSession->LoadImage(HandleToULong(containerTarFileHandle.get()), nullptr, fileSize.QuadPart));

auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() {
WSLADeleteImageOptions options{.Image = "test-imported-container:latest", .Flags = WSLADeleteImageFlagsNone};
wil::unique_cotaskmem_array_ptr<WSLADeletedImageInformation> deletedImages;
LOG_IF_FAILED(m_defaultSession->DeleteImage(&options, &deletedImages, deletedImages.size_address<ULONG>()));
});

VERIFY_SUCCEEDED(m_defaultSession->ImportImage(
HandleToULong(containerTarFileHandle.get()), "test-imported-container:latest", nullptr, fileSize.QuadPart));

// Verify that the image is in the list of images.
ExpectImagePresent(*m_defaultSession, "hello-world:latest");
WSLAContainerLauncher launcher("hello-world:latest", "wsla-hello-world-container");
ExpectImagePresent(*m_defaultSession, "test-imported-container:latest");
WSLAContainerLauncher launcher("test-imported-container:latest", "wsla-hello-world-container", {"/hello"});
auto container = launcher.Launch(*m_defaultSession);
auto result = container.GetInitProcess().WaitAndCaptureOutput();
VERIFY_ARE_EQUAL(0, result.Code);
Expand Down
Loading