test(uploads): upload cleanup assertions race the cleanup they assert on #228

New Issue

2026-08-29T11:54:51-05:00

bermudalamb commented

2026-08-29 11:54:51 -05:00

uploadValidation.integration.test.ts failed once in three full integration runs, on "removes the upload when the request is refused for its other fields". Re-running it alone passed, and two further full runs passed. Found while adding image re-encoding in #226.

It is a race in the test, not a leak in the code

discardUnlessAccepted cleans up like this:

res.on('close', () => {
  if (res.writableEnded && res.statusCode < 400) return;
  void discardUploads((req.files as Express.Multer.File[]) || []);
});

The unlink is deliberately fire-and-forget — nothing awaits it, and nothing can, because it hangs off a response event. The tests then assert like this:

const res = await request(app).post('/api/admin/items')...;
expect(res.status).toBe(400);
expect(await storedFiles()).toEqual(before);

await request(...) resolves when the response completes, which is when close fires — so the assertion and the unlink are started at effectively the same moment and the test can read the directory first. The assertion has always been able to lose this; it simply used to win reliably because nothing between the write and the response took any real time.

The property itself still holds in production. The server keeps running, the unlink completes, and #180's guarantee is intact. What is wrong is the test's assumption that an asynchronous cleanup has finished synchronously.

Why it surfaced now

#226 re-encodes every accepted upload through libvips, adding roughly 100ms of work and a threadpool to each request. That is enough to perturb the timing and lose the race occasionally. It did not create the race; it made an existing one observable.

The same reasoning applies to any future change that adds latency to the upload path, so this is worth fixing rather than waiting out.

Options

Fixing the test is right; the production code is behaving as designed and documented.

Poll for the expected state with a short timeout, since the test is asserting an eventual property. Smallest change, and honest about what is being asserted.
Expose a completion signal from discardUnlessAccepted — for example a promise the test can await. More precise, but it puts a test affordance into production code, which is the tradeoff to weigh.

Affects three tests in the "what it leaves on disk" describe block.

Related to #180, found in #226.

`uploadValidation.integration.test.ts` failed once in three full integration runs, on "removes the upload when the request is refused for its other fields". Re-running it alone passed, and two further full runs passed. Found while adding image re-encoding in #226. ## It is a race in the test, not a leak in the code `discardUnlessAccepted` cleans up like this: ```ts res.on('close', () => { if (res.writableEnded && res.statusCode < 400) return; void discardUploads((req.files as Express.Multer.File[]) || []); }); ``` The unlink is deliberately fire-and-forget — nothing awaits it, and nothing can, because it hangs off a response event. The tests then assert like this: ```ts const res = await request(app).post('/api/admin/items')...; expect(res.status).toBe(400); expect(await storedFiles()).toEqual(before); ``` `await request(...)` resolves when the response completes, which is when `close` fires — so the assertion and the unlink are started at effectively the same moment and the test can read the directory first. The assertion has always been able to lose this; it simply used to win reliably because nothing between the write and the response took any real time. **The property itself still holds in production.** The server keeps running, the unlink completes, and #180's guarantee is intact. What is wrong is the test's assumption that an asynchronous cleanup has finished synchronously. ## Why it surfaced now #226 re-encodes every accepted upload through libvips, adding roughly 100ms of work and a threadpool to each request. That is enough to perturb the timing and lose the race occasionally. It did not create the race; it made an existing one observable. The same reasoning applies to any future change that adds latency to the upload path, so this is worth fixing rather than waiting out. ## Options Fixing the test is right; the production code is behaving as designed and documented. - **Poll for the expected state** with a short timeout, since the test is asserting an eventual property. Smallest change, and honest about what is being asserted. - **Expose a completion signal** from `discardUnlessAccepted` — for example a promise the test can await. More precise, but it puts a test affordance into production code, which is the tradeoff to weigh. Affects three tests in the "what it leaves on disk" describe block. Related to #180, found in #226.

bermudalamb referenced this issue

2026-08-29 13:53:31 -05:00

fix(uploads): re-encode uploaded images to strip EXIF and cut stored bytes (#226) #229

bermudalamb referenced this issue from a commit

2026-09-02 09:59:55 -05:00

test(uploads): stop the cleanup assertions racing the cleanup (#228)

bermudalamb referenced a pull request that will close this issue

2026-09-02 10:00:16 -05:00

test(uploads): stop the cleanup assertions racing the cleanup (#228) #268

bermudalamb closed this issue

2026-09-02 10:00:29 -05:00

bermudalamb referenced this issue from a commit