Merge pull request #8595 from backstage/rugvip/cleanfilter

backend-common: make sure readTree temp dirs are cleaned up

.changeset/friendly-seals-peel.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-common': patch
+---
+
+Ensure temporary directories are cleaned up if an error is thrown in the `filter` callback of the `UrlReader.readTree` options.

packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts

@@ -147,4 +147,40 @@ describe('TarArchiveResponse', () => {
       fs.pathExists(resolvePath(dir, 'docs/index.md')),
     ).resolves.toBe(false);
   });
+
+  it('should leave temporary directories in place in the case of an error', async () => {
+    const stream = fs.createReadStream('/test-archive.tar.gz');
+
+    const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', () => {
+      throw new Error('NOPE');
+    });
+
+    const tmpDir = await fs.mkdtemp('/tmp/test');
+    // selects the wrong overload by default
+    const mkdtemp = jest.spyOn(fs, 'mkdtemp') as unknown as jest.SpyInstance<
+      Promise<string>,
+      []
+    >;
+    mkdtemp.mockResolvedValue(tmpDir);
+
+    await expect(fs.pathExists(tmpDir)).resolves.toBe(true);
+    await expect(res.dir()).rejects.toThrow('NOPE');
+    await expect(fs.pathExists(tmpDir)).resolves.toBe(false);
+
+    mkdtemp.mockRestore();
+  });
+
+  it('should leave directory in place if provided in the case of an error', async () => {
+    const stream = fs.createReadStream('/test-archive.tar.gz');
+
+    const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', () => {
+      throw new Error('NOPE');
+    });
+
+    const tmpDir = await fs.mkdtemp('/tmp/test');
+
+    await expect(fs.pathExists(tmpDir)).resolves.toBe(true);
+    await expect(res.dir({ targetDir: tmpDir })).rejects.toThrow('NOPE');
+    await expect(fs.pathExists(tmpDir)).resolves.toBe(true);
+  });
 });

packages/backend-common/src/reading/tree/TarArchiveResponse.ts

@@ -150,12 +150,19 @@ export class TarArchiveResponse implements ReadTreeResponse {
     // When no subPath is given, remove just 1 top level directory
     const strip = this.subPath ? this.subPath.split('/').length : 1;
 
+    let filterError: Error | undefined = undefined;
+
     await pipeline(
       this.stream,
       tar.extract({
         strip,
         cwd: dir,
         filter: (path, stat) => {
+          // Filter errors will short-circuit the rest of the filtering and then throw
+          if (filterError) {
+            return false;
+          }
+
           // File path relative to the root extracted directory. Will remove the
           // top level dir name from the path since its name is hard to predetermine.
           const relativePath = stripFirstDirectoryFromPath(path);
@@ -164,13 +171,27 @@ export class TarArchiveResponse implements ReadTreeResponse {
           }
           if (this.filter) {
             const innerPath = path.split('/').slice(strip).join('/');
-            return this.filter(innerPath, { size: stat.size });
+            try {
+              return this.filter(innerPath, { size: stat.size });
+            } catch (error) {
+              filterError = error;
+              return false;
+            }
           }
           return true;
         },
       }),
     );
 
+    if (filterError) {
+      // If the dir was provided we don't want to remove it, but if it wasn't it means
+      // we created a temporary directory and we should remove it.
+      if (!options?.targetDir) {
+        await fs.remove(dir).catch(() => {});
+      }
+      throw filterError;
+    }
+
     return dir;
   }
 }