Skip to content

Refactor enqueueFileTransfer #420

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
proski wants to merge 1 commit into
base: filetransfer-enqueue-exceptions
Choose a base branch
from
Open

Refactor enqueueFileTransfer #420

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
24 changes: 11 additions & 13 deletions src/libstore/filetransfer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,26 +972,24 @@ struct curlFileTransfer : public FileTransfer
return ItemHandle(item.get_ptr());
}

ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
inline ref<TransferItem>
makeTransferItem(const FileTransferRequest & request, Callback<FileTransferResult> callback)
{
/* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
if (request.uri.scheme() == "s3") {
auto modifiedRequest = request;
modifiedRequest.setupForS3();
auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
try {
return enqueueItem(item);
} catch (const nix::BaseError & e) {
// NOTE(cole-h): catches both nix::Error and nix::Interrupted -- enqueueItem calls
// writeFull which may throw nix::Interrupted, and the rest of enqueueItem may throw
// nix::Error
item->fail(e);
return ItemHandle(item.get_ptr());
}
return make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
} else {
return make_ref<TransferItem>(*this, request, std::move(callback));
}
}

ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
{
const auto item = makeTransferItem(request, std::move(callback));
Comment on lines +975 to +991
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n src/libstore/filetransfer.cc | sed -n '970,1010p'

Repository: DeterminateSystems/nix-src

Length of output: 1870

🏁 Script executed:

rg -A 10 "void setupForS3" src/libstore/filetransfer.cc

Repository: DeterminateSystems/nix-src

Length of output: 52

🏁 Script executed:

rg "try\s*{" src/libstore/filetransfer.cc | head -20

Repository: DeterminateSystems/nix-src

Length of output: 147

🏁 Script executed:

rg "setupForS3" src/libstore/filetransfer.cc -B 2 -A 15

Repository: DeterminateSystems/nix-src

Length of output: 1544

🏁 Script executed:

rg "class TransferItem" src/libstore/filetransfer.cc -A 30

Repository: DeterminateSystems/nix-src

Length of output: 52

🏁 Script executed:

rg "make_ref" include/ -t cpp --max-count=5

Repository: DeterminateSystems/nix-src

Length of output: 118

🏁 Script executed:

rg "ParsedS3URL::parse" src/ -B 2 -A 10

Repository: DeterminateSystems/nix-src

Length of output: 5572

🏁 Script executed:

rg "struct TransferItem" src/libstore/filetransfer.cc -A 25

Repository: DeterminateSystems/nix-src

Length of output: 1143

🏁 Script executed:

rg "TransferItem\(" src/libstore/filetransfer.cc -B 2 -A 5 | head -40

Repository: DeterminateSystems/nix-src

Length of output: 1283

🏁 Script executed:

rg "TransferItem::" src/libstore/filetransfer.cc -A 20 | grep -A 20 "TransferItem("

Repository: DeterminateSystems/nix-src

Length of output: 52

🏁 Script executed:

rg "noexcept" src/libstore/filetransfer.cc | grep -i "transfer"

Repository: DeterminateSystems/nix-src

Length of output: 405

Remove noexcept from makeTransferItem() to allow proper error handling.

Line 976 marks makeTransferItem() as noexcept, but it calls setupForS3() and make_ref<TransferItem>(), both of which can throw. The setupForS3() method calls ParsedS3URL::parse(), which throws BadURL and other parsing exceptions. Since makeTransferItem() is invoked on line 991 before the try block (line 993), any of these exceptions will trigger std::terminate instead of being caught and translated into a failed transfer callback via item->fail(e). Remove the noexcept qualifier and ensure item construction is covered by the existing error-handling path.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/libstore/filetransfer.cc` around lines 975 - 991, Remove the noexcept
from makeTransferItem so exceptions from setupForS3() and
make_ref<TransferItem>() can propagate into the existing error-handling in
enqueueFileTransfer; update the declaration of makeTransferItem (the function
that calls setupForS3() and returns make_ref<TransferItem>) to be non-noexcept
so thrown BadURL or parsing exceptions are caught by the try/catch around the
call in enqueueFileTransfer and translated via item->fail(e).


auto item = make_ref<TransferItem>(*this, request, std::move(callback));
try {
return enqueueItem(item);
} catch (const nix::BaseError & e) {
Expand Down