From 25f1c2bfc407a2ed156082b3cfde4186cafbe75c Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 6 Jan 2020 14:43:23 +0000
Subject: [PATCH] Delete temporary file when error in writing to stream

---
 services/filestore/app/js/LocalFileWriter.js  | 13 ++++-
 .../test/unit/js/LocalFileWriterTests.js      | 48 +++++++++++++++----
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/services/filestore/app/js/LocalFileWriter.js b/services/filestore/app/js/LocalFileWriter.js
index 44f3f9433a..22957e15d1 100644
--- a/services/filestore/app/js/LocalFileWriter.js
+++ b/services/filestore/app/js/LocalFileWriter.js
@@ -32,6 +32,8 @@ async function writeStream(stream, key) {
     logger.log({ fsPath }, 'finished writing file locally')
     return fsPath
   } catch (err) {
+    await deleteFile(fsPath)
+
     logger.err({ err, fsPath }, 'problem writing file locally')
     throw new WriteError({
       message: 'problem writing file locally',
@@ -45,7 +47,16 @@ async function deleteFile(fsPath) {
     return
   }
   logger.log({ fsPath }, 'removing local temp file')
-  await promisify(fs.unlink)(fsPath)
+  try {
+    await promisify(fs.unlink)(fsPath)
+  } catch (err) {
+    if (err.code !== 'ENOENT') {
+      throw new WriteError({
+        message: 'failed to delete file',
+        info: { fsPath }
+      }).withCause(err)
+    }
+  }
 }
 
 function _getPath(key) {
diff --git a/services/filestore/test/unit/js/LocalFileWriterTests.js b/services/filestore/test/unit/js/LocalFileWriterTests.js
index 5d7008a91f..ad4d73bce6 100644
--- a/services/filestore/test/unit/js/LocalFileWriterTests.js
+++ b/services/filestore/test/unit/js/LocalFileWriterTests.js
@@ -49,6 +49,26 @@ describe('LocalFileWriter', function() {
         done()
       })
     })
+
+    describe('when there is an error', function() {
+      const error = new Error('not enough ketchup')
+      beforeEach(function() {
+        stream.pipeline.yields(error)
+      })
+
+      it('should wrap the error', function() {
+        LocalFileWriter.writeStream(readStream, filename, err => {
+          expect(err).to.exist
+          expect(err.cause).to.equal(error)
+        })
+      })
+
+      it('should delete the temporary file', function() {
+        LocalFileWriter.writeStream(readStream, filename, () => {
+          expect(fs.unlink).to.have.been.calledWith(fsPath)
+        })
+      })
+    })
   })
 
   describe('deleteFile', function() {
@@ -60,14 +80,6 @@ describe('LocalFileWriter', function() {
       })
     })
 
-    it('should not do anything if called with an empty path', function(done) {
-      fs.unlink = sinon.stub().yields(new Error('failed to reticulate splines'))
-      LocalFileWriter.deleteFile(fsPath, err => {
-        expect(err).to.exist
-        done()
-      })
-    })
-
     it('should not call unlink with an empty path', function(done) {
       LocalFileWriter.deleteFile('', err => {
         expect(err).not.to.exist
@@ -75,5 +87,25 @@ describe('LocalFileWriter', function() {
         done()
       })
     })
+
+    it('should not throw a error if the file does not exist', function(done) {
+      const error = new Error('file not found')
+      error.code = 'ENOENT'
+      fs.unlink = sinon.stub().yields(error)
+      LocalFileWriter.deleteFile(fsPath, err => {
+        expect(err).not.to.exist
+        done()
+      })
+    })
+
+    it('should wrap the error', function(done) {
+      const error = new Error('failed to reticulate splines')
+      fs.unlink = sinon.stub().yields(error)
+      LocalFileWriter.deleteFile(fsPath, err => {
+        expect(err).to.exist
+        expect(err.cause).to.equal(error)
+        done()
+      })
+    })
   })
 })