PreFinishResponseCallback is not called when retrying last PATCH request

[Deimvis]

Describe the bug

• On last PATCH request UnroutedHandler.finishUploadIfComplete calls FinishUpload and PreFinishResponseCallback
• If PreFinishResponseCallback returns an error, then error response is returned to the client (500, in fact)
• If client retries this last PATCH request, then UnroutedHandler.PatchFile checks whether offset == size (which is true, because otherwise UnroutedHandler.finishUploadIfComplete wouldn't start FinishUpload on previous request) and just returns successful response without calling neither data store's FinishUpload, nor PreFinishResponseCallback
  • Retrying seems natural, at least all errors are retriable in tus-js-client and tus-py-client

I also assume that data store's FinishUpload that returned an error wouldn't be called on retry and may also cause an issue

To Reproduce
Steps to reproduce the behavior:

1. Register PreFinishResponseCallback which always returns an error
2. Start the upload using any client with retries
3. Upload will succeed retrying last PATCH request, but PreFinishResponseCallback won't be called the second time

Minimal Setup To Reproduce

• tusd server

  package main
  
  import (
  	"errors"
  	"log"
  	"net/http"
  
  	"github.com/tus/tusd/v2/pkg/filelocker"
  	"github.com/tus/tusd/v2/pkg/filestore"
  
  	tusd "github.com/tus/tusd/v2/pkg/handler"
  )
  
  func main() {
  	ds := filestore.New("./uploads")
  	locker := filelocker.New("./uploads")
  
  	composer := tusd.NewStoreComposer()
  	ds.UseIn(composer)
  	locker.UseIn(composer)
  
  	config := tusd.Config{
  		BasePath:      "/files/",
  		StoreComposer: composer,
  		PreFinishResponseCallback: func(hook tusd.HookEvent) (tusd.HTTPResponse, error) {
  			return tusd.HTTPResponse{}, errors.New("always fail")
  		},
  	}
  
  	handler, err := tusd.NewHandler(config)
  	if err != nil {
  		log.Fatalf("unable to create handler: %s", err)
  	}
  
  	http.Handle("/files/", http.StripPrefix("/files/", handler))
  	http.Handle("/files", http.StripPrefix("/files", handler))
  	log.Println("Will listen :8080")
  	err = http.ListenAndServe(":8080", nil)
  	if err != nil {
  		log.Fatalf("unable to listen: %s", err)
  	}
  }

• upload script

  #!/usr/bin/env bash
  
  set -e
  
  # init upload
  loc=$(curl -si -X POST localhost:8080/files -H "Tus-Resumable: 1.0.0" -H "Content-Length: 0" -H "Upload-Length: 3" | grep -i "^Location:" | cut -d' ' -f2- | tr -d '\r')
  # upload last chunk
  curl -v -X PATCH "$loc"  -H "Tus-Resumable: 1.0.0" -H "Content-Type: application/offset+octet-stream" -H "Content-Length: 3" -H "Upload-Offset: 0" -d 'abc'
  # > got 500 since error returned from finish callback
  # retry
  curl -v -X PATCH "$loc"  -H "Tus-Resumable: 1.0.0" -H "Content-Type: application/offset+octet-stream" -H "Content-Length: 3" -H "Upload-Offset: 3" -d 'abc'
  # > got 204 since finish callback wasn't called
  
  upload_id="${loc##*/}"
  echo "Upload succeeded, content: `cat ./uploads/$upload_id`"

Expected behavior

• I expected that PreFinishResponseCallback is required for successful upload since its function type returns an error
• Also, the documentation for PreFinishResponseCallback says that "This can be used to implement post-processing validation" and hints that successful callback run is required in order to finish upload

Setup details
Please provide following details, if applicable to your situation:

• Operating System: macOS
• Used tusd version: v2.8.0
• Used tusd data storage: custom (can be reproduced on any)
• Used tusd configuration: —
• Used tus client library: custom (can be reproduced on any)

Possible Solution
Always call UnroutedHandler.finishUploadIfComplete instead of just returning successful response thereby finishing upload. Although, it would break some clients since it would require data store's FinishUpload and PreFinishResponseCallback to be idempotent