cleanRequestFiles doesn't clean file when file size is reached

[ThallesP]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.21.0

Plugin version

7.7.3

Node.js version

18.16.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Ubuntu 22.04.2 LTS (wsl2)

Description

When calling cleanRequestFiles after the error "request file too large" happens, it doesn't clean the temp file, as i suggestion this file should be removed automatically if the file is bigger than the max size.

Steps to Reproduce

Set file maxSize, for example:

await request.saveRequestFiles({
  limits: {
    fileSize: 1, // 1 byte
  },
})

and after the error is thrown, call cleanRequestFiles.

Expected Behavior

The file should be cleared

[ThallesP]

Also, seems that the file is saved and then the file size limit is checked, sad that i can't use this library anymore because of such little thing.

[mcollina]

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[gurgunday]

When calling cleanRequestFiles after the error "request file too large" happens

I haven't reproduced it yet, but isn't the requestFileTooLarge error thrown while a file is being consumed and before it's even saved? So at a first glance, I can't see where the issue could be

[gurgunday]

Can you please share more details? The files aren’t saved before they are fully consumed, so there should be no reason to clean up anything

fastify-multipart/index.js

Lines 411 to 422 in 8e4b5e5

	files = await this.files(options)
	}
	this.savedRequestFiles = []
	const tmpdir = (options && options.tmpdir) || os.tmpdir()
	this.tmpUploads = []
	for await (const file of files) {
	const filepath = path.join(tmpdir, generateId() + path.extname(file.filename))
	const target = createWriteStream(filepath)
	try {
	await pump(file.file, target)
	this.savedRequestFiles.push({ ...file, filepath })
	this.tmpUploads.push(filepath)

I don’t think there are any errors in the current implementation

[ThallesP]

Hi, i'll use fastify-multipart somewhere in the next month, then I can give any information as I don't have any active projects with fastify-multipart.
*Just heads up that I saw the replies.

[yaneony]

I'm uncertain, yet my experience aligns with the original poster's. After establishing an upload field with the setting { fileSize: 1}, I attempted to upload a debian.iso file approximately 3.7Gb in size. The process lasts several minutes, influenced by connection speed, before an error message appears. However, it seems as though the entire file is completely processed prior to the error notification.

[Uzlopak]

Because we have stream the file into nirvana so that we can send the response that the file is too big ?

An alternative would be to destroy the underlying socket if a specific threshold is met?

[yaneony]

Then this comment is completely wrong #470 (comment) since error is being thrown not while consuming, but after the entire file is consumed. Destroying the socket after specific threshold would probably do this while file is consumed.

[Uzlopak]

Its my assumption and not to claim that the comment is wrong.

Yeah of course you have the drawback that destroying the socket will result in some bad user experience, because you wont know why the connection was interrupted. But better destroying the socket than clogging the servers bandwith.

So we have to investigate here further, what the actual case is.

[gurgunday]

Then this comment is completely wrong

It might depend on the configuration, which is why I asked for more information...

But don't take my word:

We consume the stream on line 411 before iterating on the files and saving them starting on line 416 - 411 is where the error should be thrown normally

fastify-multipart/index.js

Lines 398 to 430 in e2aaf59

	async function saveRequestFiles (options) {
	// Checks if this has already been run
	if (this.savedRequestFiles) {
	return this.savedRequestFiles
	}
	let files
	if (attachFieldsToBody === true) {
	// Skip the whole process if the body is empty
	if (!this.body) {
	return []
	}
	files = filesFromFields.call(this, this.body)
	} else {
	files = await this.files(options)
	}
	this.savedRequestFiles = []
	const tmpdir = (options && options.tmpdir) || os.tmpdir()
	this.tmpUploads = []
	for await (const file of files) {
	const filepath = path.join(tmpdir, generateId() + path.extname(file.filename))
	const target = createWriteStream(filepath)
	try {
	await pump(file.file, target)
	this.savedRequestFiles.push({ ...file, filepath })
	this.tmpUploads.push(filepath)
	} catch (err) {
	this.log.error({ err }, 'save request file')
	throw err
	}
	}
	return this.savedRequestFiles
	}

I will check again though, if you can provide a reproducible example