don't rely on exception for http 416

Fix a bug that could make resuming a download from the web fail when the
entire content of the file is actually already present locally.

What a mess that Request can throw exceptions or not, depending on how
it's configured. Makes it very hard if you need to handle some specific
http status codes in a function like this! Implementing everything two
ways did not seem appealing, if possible at all, so I decided to
override the Request if it did come configured to throw exception on
non-2xx http status. Other exceptions, like from http-client-restricted,
or due to a redirect to a non-http url, still get thrown.

This commit was sponsored by Luke Shumaker on Patreon.

diff --git a/CHANGELOG b/CHANGELOG
index fc0db5e3d020d2b226731118779b5b6030a1d750..e1521c31f17e6afac332eedb4787fbb9b4b2855c 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -15,6 +15,9 @@ git-annex (8.20201117) UNRELEASED; urgency=medium
   * Fix build on Windows.
   * Prevent windows assistant from trying (and failing) to upgrade
     itself, which has never been supported on windows.
+  * Fix a bug that could make resuming a download from the web fail
+    when the entire content of the file is actually already present
+    locally.
 
  -- Joey Hess <id@joeyh.name>  Mon, 16 Nov 2020 09:38:32 -0400
 

diff --git a/Utility/Url.hs b/Utility/Url.hs
index eacac2f122e352f27116a1a0a67cbfd1f170d777..7ef821ca056508c2b6e9879b7ddc0e48d93fcabf 100644 (file)
--- a/Utility/Url.hs
+++ b/Utility/Url.hs
@@ -452,7 +452,12 @@ download' nocurlerror meterupdate url file uo =
 {- Download a perhaps large file using conduit, with auto-resume
  - of incomplete downloads.
  -
- - Does not catch exceptions.
+ - A Request can be configured to throw exceptions for non-2xx http
+ - status codes, or not. That configuration is overridden by this,
+ - and if it is unable to download, it throws an exception containing
+ - a user-visible explanation of the problem. (However, exceptions
+ - thrown for reasons other than http status codes will still be thrown
+ - as usual.)
  -}
 downloadConduit :: MeterUpdate -> Request -> FilePath -> UrlOptions -> IO ()
 downloadConduit meterupdate req file uo =
@@ -480,28 +485,29 @@ downloadConduit meterupdate req file uo =
                        filter ((/= hAcceptEncoding) . fst)
                                (requestHeaders req)
                , decompress = const False
+               -- Avoid throwing exceptions non-2xx http status codes,
+               -- since we rely on parsing the Response to handle
+               -- several such codes.
+               , checkResponse = \_ _ -> return ()
                }
 
        -- Resume download from where a previous download was interrupted, 
        -- when supported by the http server. The server may also opt to
        -- send the whole file rather than resuming.
-       resumedownload sz = catchJust
-               (matchStatusCodeHeadersException (alreadydownloaded sz))
-               dl
-               (const noop)
-         where
-               dl = join $ runResourceT $ do
-                       let req'' = req' { requestHeaders = resumeFromHeader sz : requestHeaders req }
-                       liftIO $ debugM "url" (show req'')
-                       resp <- http req'' (httpManager uo)
-                       if responseStatus resp == partialContent206
+       resumedownload sz = join $ runResourceT $ do
+               let req'' = req' { requestHeaders = resumeFromHeader sz : requestHeaders req }
+               liftIO $ debugM "url" (show req'')
+               resp <- http req'' (httpManager uo)
+               if responseStatus resp == partialContent206
+                       then do
+                               store (toBytesProcessed sz) AppendMode resp
+                               return (return ())
+                       else if responseStatus resp == ok200
                                then do
-                                       store (toBytesProcessed sz) AppendMode resp
+                                       store zeroBytesProcessed WriteMode resp
                                        return (return ())
-                               else if responseStatus resp == ok200
-                                       then do
-                                               store zeroBytesProcessed WriteMode resp
-                                               return (return ())
+                               else if alreadydownloaded sz resp
+                                       then return (return ())
                                        else do
                                                rf <- extractFromResourceT (respfailure resp)
                                                if responseStatus resp == unauthorized401
@@ -510,8 +516,9 @@ downloadConduit meterupdate req file uo =
                                                                Just ba -> retryauthed ba
                                                        else return $ giveup rf
        
-       alreadydownloaded sz s h = s == requestedRangeNotSatisfiable416 
-               && case lookup hContentRange h of
+       alreadydownloaded sz resp
+               | responseStatus resp /= requestedRangeNotSatisfiable416 = False
+               | otherwise = case lookup hContentRange (responseHeaders resp) of
                        -- This could be improved by fixing
                        -- https://github.com/aristidb/http-types/issues/87
                        Just crh -> crh == B8.fromString ("bytes */" ++ show sz)

diff --git a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn
index 188691639f59bd0c2bbc80c0a7e54d6b24fd9197..6cb41f9439ef072201482c4fbd4dd80286367fcf 100644 (file)
--- a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn
+++ b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable.mdwn
@@ -41,3 +41,5 @@ Stock git-annex on Fedora 32:
 ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders)
 
 Yes! git-annex has been working great for me so far, and is powering the bioinformatics chat podcast (https://bioinformatics.chat/). Thanks!
+
+> [[fixed|done]] --[[Joey]]

diff --git a/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment
new file mode 100644 (file)
index 0000000..006eec9
--- /dev/null
+++ b/doc/bugs/download_failed__58___Requested_Range_Not_Satisfiable/comment_1_44bc0678a9978b62ab133d64d3614610._comment
@@ -0,0 +1,30 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 1"""
+ date="2020-11-19T16:56:45Z"
+ content="""
+To reproduce this, interrupt git-annex after it downloads the whole file,
+but before it moves it from the download location into the annex. (Or,
+let it get the file, then move the object back to the temp object location.)
+
+This is a tricky case, because if the total file size is not
+known when resuming the download, how can it detect if it's got it all
+already? And git-annex does not always know the total file size, eg when
+git-annex addurl --relaxed is used, and then git-annex get is later used
+to download the content.
+
+What git-annex already tried to do to detect this is,
+when it got a 416 it looks for a Content-Range header "bytes */$size"
+where $size is the same as the size of the file on disk.
+
+That relied on the http library throwing an exception for the 416.
+Thing is, http may or may not throw exceptions for non-2xx
+responses, depending on the input Request. IMHO that is a very bad design,
+it leads to this kind of bug, rather than making it evident with the data
+types what is going on.
+
+Currently downloadConduit takes a Request, and assumes it throws exceptions
+for 416, but not for 401. Both can't be right.
+
+Ok, fixed this mess..
+"""]]