Merge pull request 'test(uploads): stop the cleanup assertions racing the cleanup (#228)' (#268) from fix/228-upload-cleanup-race into main

Reviewed-on: #268

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);
   });
 });