tus-js-client ignores failed PATCH or POST if HEAD is successful

[Murderlon]

Problem

When using the pre-finish hook from tusd or the onUploadFinish hook from tus-node-server, which both allow an error to be returned and send to the client, will still result in a successful upload even when failed.

Reproduction

tusd

hooks/pre-finish

#!/bin/bash

echo "Error: no filename provided"
exit 1

./tusd -upload-dir=./files -hooks-dir=./hooks -hooks-enabled-events=pre-finish

[tusd] 2023/02/28 16:56:22.520854 event="HookInvocationStart" type="pre-finish" id="ad30e19a7175bbe7335f7b4f28acbfe4" 
[tusd] 2023/02/28 16:56:22.527312 event="HookInvocationError" type="pre-finish" id="ad30e19a7175bbe7335f7b4f28acbfe4" error="exit status 1" 
[tusd] 2023/02/28 16:56:22.527403 event="ResponseOutgoing" status="500" method="PATCH" path="ad30e19a7175bbe7335f7b4f28acbfe4" error="pre-finish hook failed: exit status 1
Error: no filename provided
" requestId="" 

Result with @uppy/core and @uppy/tus (but the same happens with just tus-js-client):

tus-node-server

Same result.

Solutions

Note:
On the server the upload was successful, all bytes have been written to storage, but before a response was sent the post-processing failed. This means the next HEAD request, which looks up the file in storage, will be successful and this request triggers the successful state.

1. Fail the upload on POST if the creation-with-upload extension is used and on PATCH if HTTP 500 is returned. The subpar thing with this is that it's indistinguishable from other HTTP 500 problems, which do warrant a retry, while a retry on a post-processing issue will likely keep resulting in the same result?
2. Change the servers to always send HTTP 400, which will fail correctly on the client.
3. Accept that we can't do post-processing and change the hooks accordingly.

[Acconut]

This is a bit of a tricky topic. From the tus perspective, the upload completed fine because all bytes were transferred and saved, as you correctly noted. As such, tus-js-client considers the upload successful. It does not have knowledge of the potential post-processing happening on the server-side.

3. Accept that we can't do post-processing and change the hooks accordingly.

I am not sure what you mean exactly here, but I agree that a proper solution must involve the server. Right now, tusd and tus-node-server do not preserve the result from any hooks. Maybe it is time to think about a more proper solution to post-processing than the post-finish and pre-finish hooks. Something which is retried by the tus server and stores a post-processing result (or error) which is then communicated back to the client. What do you think?

[nh2]

Sharing one approach:

In our custom server, automatic finalization is done with the last PATCH, communicated via a response header (since TUS's PATCH unfortunately mandates HTTP 204 No Content, thus forbidding us to return JSON), which is stored in tus-js-client's onAfterResponse().

But like a hook, our finalization can fail after the saving of the bytes, thus leaving offset === length and thus tus-js-client considers the upload done and will send no further PATCH.

This can be solved by detecting this situation in tus-js-client's onSuccess() whether onAfterResponse() stored the mentioned success-indicating header; if not, one can send a manual 0-length PATCH, or alternative use a non-TUS API to re-trigger finalization. We went with the latter.

---

It would definitely good to explicitly point out this hooks failure mode out in tus-js-client's docs somewhere, as it seems to be a common error (I did not notice this at first, and coded a but that would keep unfinalized uploads with fully transmitted length stuck forever).
Perhaps in the docs of onSuccess()?