Error out if source provides less data than expected

[sdhull]

Hopefully I can get some help with this one. We're experiencing Companion sending insane amounts of zero-byte patch requests.

We are guessing that this is due to some files Content-Length being over-reported when doing HEAD request in Companion to get the file size, essentially doing what this test does (setting the options.uploadSize too large).

I tried applying the diff from #556 but that does not appear to fix the issue. Would appreciate any help or ideas here. It feels like it should not be "too hard" to simply fail an upload where the next patch would be length=0 but maybe I'm missing something here.

[Acconut]

Thanks for bringing this up!

fail an upload where the next patch would be length=0

I don't think this is the correct approach here. First of all, there are legitimate reasons for sending an empty PATCH request when Upload-Defer-Length is used, for example. So one has to be careful to not disturb this mechanism. Secondly, this would tackle the symptoms, but not the underlying root cause. As mentioned in #556 (comment), the likely cause is that Companion reports a wrong upload size (uploadSize is larger than file length). This should be fixed in Companion itself (cc @mifi). If a correct uploadSize cannot be determined upfront, uploadLengthDeferred should be used.

In tus-js-client, we can do nothing to solve this situation, I think. As you proposed, we can error out if we detect that uploadSize does not match the file length. This would help and make this error case more discoverable. While I agree that we should have such a check, this does not help the upload to complete. In the end, this needs to be fixed in Companion.

[sdhull]

@Acconut thanks for the response! Just to add some color, this is what we're seeing on our tusd server that prompted this:

We tracked the increase of requests to our instance of companion (we're running v4.1.1, so we're a few minor versions behind... we looked at changelog in companion and have no reason to believe this would be solved in the latest version, though we intend to upgrade soon nonetheless).

there are legitimate reasons for sending an empty PATCH request

Is the only legitimate reason for sending an empty PATCH request when uploadLengthDeferred=true?

[Acconut]

Is the only legitimate reason for sending an empty PATCH request when uploadLengthDeferred=true?

I think so, yes.

In tus-js-client we can add an error condition if the we notice that the file input ends before uploadSize. This won't make the uploads work, but at least than Companion has a way to find out why uploadSize is wrong sometimes.

[mifi]

In tus-js-client we can add an error condition if the we notice that the file input ends before uploadSize. This won't make the uploads work, but at least than Companion has a way to find out why uploadSize is wrong sometimes.

I think that would be very nice, because then Companion could receive an error and abort the upload instead of hanging forever and flooding the tus server with patch requests.

As for how to fix the actual problem, should we always set uploadLengthDeferred to true in Companion if we are dealing with a non-file stream? This effectively means we won't trust the size reported by the provider/URL anymore and we won't set uploadSize anymore, right? Does setting this flag to true have any consequences for the user or pose any limitations or change in behaviour?

[sdhull]

@mifi it kinda sounds like setting uploadLengthDeferred: true in companion may completely fix the issue we've been experiencing. Or maybe at least allow it to be set via options. Or maybe it should be inferred to be true if streaming uploads is turned on? I'm not sure. And I don't know all the implications around turning that on. Maybe @Acconut can give us some more insight here 🙏

[Acconut]

@mifi @sdhull I just pushed some commits that cause tus-js-client to error out if uploadSize is bigger than it should be. Please have a look at it and and review to see if that is what you had in mind as well.

Does setting this flag to true have any consequences for the user or pose any limitations or change in behaviour?

The only problem I could see is that not every tus server might support uploadLengthDeferred. It is an optional extension, so some tus servers will not be compatible with it. Otherwise, it should be fine. I agree that it makes more sense to set uploadLengthDeferred than to guess the upload size upfront if there is a risk of getting it wrong.

[sdhull]

@Acconut this looks awesome, thanks! Looks like exactly what we're looking for 😎

[Acconut]

I just published v3.1.1, which includes this patch. Please try it out and let me know if you find the source of the wrong uploadSize.