From b7cd336798ab477cf8cfbe4ad8eced1100db7ce9 Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Tue, 11 Aug 2026 23:50:53 +0200
Subject: [PATCH] Download: never leave the target file behind after a failure

The wget method removed a partial target file, the curl method did not, and
the SingleHTTPRequest method wrote the response body -- an error page, say --
to the target and then reported failure.  So whether the file existed after a
failed download, and what was in it, depended on which method happened to be
available.

Fixed in curl and SingleHTTPRequest, and enforced in 'Download' as well so it
holds for methods added to 'Download_Methods' from elsewhere.  Documented.

The new test drives each available method directly; going through 'Download'
alone hides the bug whenever a well-behaved method runs last.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 doc/download.xml |  2 ++
 lib/download.gi  | 26 ++++++++++++++++++++++----
 tst/download.tst | 20 ++++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/doc/download.xml b/doc/download.xml
index def4273..cf8548c 100644
--- a/doc/download.xml
+++ b/doc/download.xml
@@ -68,6 +68,8 @@ The following components are supported.
   that is a local filename,
   and the function writes the downloaded contents to this file;
   the returned record does not have a <C>result</C> component in this case.
+  <P/>
+  If the download fails then this file is not left behind.
 </Item>
 <Mark><C>verifyCert</C></Mark>
 <Item>
diff --git a/lib/download.gi b/lib/download.gi
index 1e01b2e..e051763 100644
--- a/lib/download.gi
+++ b/lib/download.gi
@@ -125,10 +125,16 @@ Add( Download_Methods, rec(
       res:= ValueGlobal( "SingleHTTPRequest" )( domain, port, "GET", uri,
                 rec( Host:= authority ), false, false );
     fi;
-    if res.statuscode = 0 then
-      return rec( success:= false,
-                  error:= res.status );
-    elif res.statuscode >= 400 then
+    if res.statuscode = 0 or res.statuscode >= 400 then
+      # 'SingleHTTPRequest' has already written the response body, for example
+      # an error page, to the target file; remove it.
+      if IsBound( opt.target ) and IsString( opt.target ) and
+         IsExistingFile( opt.target ) then
+        RemoveFile( opt.target );
+      fi;
+      if res.statuscode = 0 then
+        return rec( success:= false, error:= res.status );
+      fi;
       return rec( success:= false,
                   error:= Concatenation( "HTTP error code ",
                                          String( res.statuscode ) ) );
@@ -219,6 +225,11 @@ Add( Download_Methods, rec(
     code:= Process( DirectoryCurrent(), exec, InputTextNone(), outstream, args );
     CloseStream( outstream );
     if code <> 0 then
+      # curl may have created the target file; remove it, as wget's method does
+      if IsBound( opt.target ) and IsString( opt.target ) and
+         IsExistingFile( opt.target ) then
+        RemoveFile( opt.target );
+      fi;
       return rec( success:= false,
                   error:= Concatenation( "Process returned ", String( code ) ) );
     elif not ( IsBound( opt.target ) and IsString( opt.target ) ) then
@@ -270,6 +281,13 @@ InstallMethod( Download,
         if res.success = true then
           return res;
         fi;
+        # A failed method may have left a partial or bogus target file behind.
+        # Remove it here, so that the guarantee holds for every method,
+        # including ones added to 'Download_Methods' from outside.
+        if IsBound( opt.target ) and IsString( opt.target ) and
+           IsExistingFile( opt.target ) then
+          RemoveFile( opt.target );
+        fi;
         Info( InfoUtils, 2, "Download method ", r.name, " failed with\n",
               "#I    ", res.error );
         Add( errors, Concatenation( r.name, ": ", res.error ) );
diff --git a/tst/download.tst b/tst/download.tst
index e52cc6e..f94824e 100644
--- a/tst/download.tst
+++ b/tst/download.tst
@@ -139,6 +139,26 @@ gap> res1:= Download( url, rec( maxTime:= 5 ) );;
 gap> res1.success = true;
 true
 
+##  A failed download must not leave the target file behind,
+##  whichever method was used.
+gap> file:= Filename( DirectoryTemporary(), "target" );;
+gap> for r in meths do
+>      res1:= r.download( Concatenation( baseurl, "/missing" ),
+>                         rec( target:= file ) );
+>      if res1.success = true then
+>        Print( "unexpected success for ", r.name, "\n" );
+>      elif IsExistingFile( file ) then
+>        Print( "leftover target file after ", r.name, "\n" );
+>        RemoveFile( file );
+>      fi;
+>    od;
+gap> res1:= Download( Concatenation( baseurl, "/missing" ),
+>                     rec( target:= file ) );;
+gap> res1.success = false;
+true
+gap> IsExistingFile( file );
+false
+
 ##  test errors and redirects
 gap> res1:= Download( Concatenation( baseurl, "/missing" ) );;
 gap> res1.success = false;