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

discardUnlessAccepted unlinks from a res.on('close') handler, so nothing awaits it and nothing can. `await request()` resolves when the response completes, which is when close fires — so a read taken straight afterwards races the unlink it is meant to observe. The property is an eventual one and the assertions were synchronous.

The issue counted three tests. Injecting a 400ms delay into the unlink to make the race deterministic showed five: "leaves nothing on the volume when it refuses the content" and "discards the valid files from a request that also carried an invalid one" race too, and are not in the describe block the issue named.

It also showed that polling alone is not enough. The race runs in both directions, and the second direction is easy to miss: a deletion still pending from the *previous* test corrupts the next test's baseline before its request is even sent. No amount of waiting fixes a baseline that is already wrong. My first attempt waited for the directory to look quiet, which only works while the unlink is faster than the wait — precisely the assumption this issue is about, and it still failed four tests under the injected delay.

So the baseline is removed as a variable: beforeEach empties the directory, every test starts from empty, and the assertions poll for the expected count. Any orphan from an earlier suite goes with it, which is correct — the directory is temporary and nothing outside these tests owns it. discardUploads already catches per-file errors, so a pending unlink finding its file gone logs and moves on.

Verified by injecting the 400ms delay again: four to five failures before, twelve passing after, with the production file restored untouched.

Closes #228

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/tests/integration/uploadValidation.integration.test.ts

@@ -35,6 +35,51 @@ async function storedFiles(): Promise<string[]> {
   return fs.readdir(UPLOADS_DIR);
 }
 
+const tick = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
+
+/**
+ * Waits for the uploads directory to reach the expected state, then returns it.
+ *
+ * `discardUnlessAccepted` unlinks from a `res.on('close')` handler, so nothing
+ * awaits it and nothing can — it hangs off a response event. `await request()`
+ * resolves when the response completes, which is when `close` fires, so a read
+ * taken straight afterwards races the unlink it is meant to observe. The
+ * property is an eventual one, so asserting it has to be eventual too. See #228.
+ *
+ * The final read is returned rather than a boolean, so the caller still asserts
+ * on real contents and a failure names the files that were actually there.
+ */
+async function filesSettlingTo(matches: (files: string[]) => boolean): Promise<string[]> {
+  const deadline = Date.now() + 3000;
+  let files = await storedFiles();
+
+  while (!matches(files) && Date.now() < deadline) {
+    await tick(25);
+    files = await storedFiles();
+  }
+  return files;
+}
+
+/**
+ * Empties the uploads directory before each test in this file.
+ *
+ * The race runs in both directions, and this is the half that is easy to miss:
+ * a deletion still pending from the *previous* test corrupts the next test's
+ * baseline before its request is even sent. Polling cannot fix that — the
+ * baseline is already wrong — and waiting for the directory to look quiet only
+ * works while the unlink is faster than the wait, which is precisely the
+ * assumption #228 is about.
+ *
+ * Starting from empty removes the baseline as a variable entirely. Any orphan
+ * left by an earlier suite goes with it, which is correct: this directory is a
+ * temporary one and nothing outside these tests owns its contents.
+ */
+beforeEach(async () => {
+  for (const name of await storedFiles()) {
+    await fs.unlink(path.join(UPLOADS_DIR, name)).catch(() => undefined);
+  }
+});
+
 function createItem() {
   return request(app)
     .post('/api/admin/items')
@@ -106,27 +151,27 @@ describe('what it refuses', () => {
   });
 
   it('leaves nothing on the volume when it refuses the content', async () => {
-    const before = await storedFiles();
+    const before: string[] = [];
 
     await createItem().attach('images', HTML_BYTES, {
       filename: 'photo.jpg',
       contentType: 'image/jpeg'
     });
 
-    expect(await storedFiles()).toHaveLength(before.length);
+    expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]);
   });
 
   // A rejection must take the whole request with it. Accepting the good file
   // from a refused upload would leave a file behind that nothing references.
   it('discards the valid files from a request that also carried an invalid one', async () => {
-    const before = await storedFiles();
+    const before: string[] = [];
 
     const res = await createItem()
       .attach('images', REAL_PNG, { filename: 'good.png', contentType: 'image/png' })
       .attach('images', HTML_BYTES, { filename: 'bad.jpg', contentType: 'image/jpeg' });
 
     expect(res.status).toBe(400);
-    expect(await storedFiles()).toHaveLength(before.length);
+    expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]);
   });
 
   it('does not create the item when the upload is refused', async () => {
@@ -164,7 +209,7 @@ describe('the uploads directory', () => {
  */
 describe('what it leaves on disk', () => {
   it('removes the upload when the request is refused for its other fields', async () => {
-    const before = await storedFiles();
+    const before: string[] = [];
 
     const res = await request(app)
       .post('/api/admin/items')
@@ -177,11 +222,11 @@ describe('what it leaves on disk', () => {
       .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' });
 
     expect(res.status).toBe(400);
-    expect(await storedFiles()).toEqual(before);
+    expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]);
   });
 
   it('removes the upload when the tags field is refused', async () => {
-    const before = await storedFiles();
+    const before: string[] = [];
 
     const res = await request(app)
       .post('/api/admin/items')
@@ -192,13 +237,13 @@ describe('what it leaves on disk', () => {
       .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' });
 
     expect(res.status).toBe(400);
-    expect(await storedFiles()).toEqual(before);
+    expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]);
   });
 
   // The accepted case must not be swept up by the same cleanup: these files are
   // the ones the item now points at.
   it('keeps the upload when the request succeeds', async () => {
-    const before = await storedFiles();
+    const before: string[] = [];
 
     const res = await createItem().attach('images', REAL_PNG, {
       filename: 'photo.png',
@@ -206,6 +251,7 @@ describe('what it leaves on disk', () => {
     });
 
     expect(res.status).toBe(200);
-    expect((await storedFiles()).length).toBe(before.length + 1);
+    // No unlink is scheduled for an accepted request, so this settles at once.
+    expect(await filesSettlingTo((files) => files.length === 1)).toHaveLength(1);
   });
 });