Merge remote-tracking branch 'sharelatex/master'

services/filestore/.gitignore

@@ -55,6 +55,7 @@ public/stylesheets/mainStyle.css
 public/minjs/
 test/unit/js/
 test/acceptence/js
+cluster.js
 
 user_files/*
 template_files/*

services/filestore/.travis.yml

@@ -10,13 +10,6 @@ install:
     - npm install
     - grunt install
 
-before_script:
-    - grunt forever:app:start
-
 script:
     - grunt test:unit
-    - grunt test:acceptance
 
-services:
-    - redis-server
-    - mongodb

services/filestore/app.coffee

@@ -14,6 +14,7 @@ Metrics = require "metrics-sharelatex"
 Metrics.initialize("filestore")
 Metrics.open_sockets.monitor(logger)
 Metrics.event_loop?.monitor(logger)
+Metrics.memory.monitor(logger)
 
 app.configure ->
 	app.use express.bodyParser()
@@ -85,7 +86,6 @@ app.post "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey
 app.put "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.copyFile
 app.del "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, fileController.deleteFile
 
-
 app.get "/heapdump", (req, res)->
 	require('heapdump').writeSnapshot '/tmp/' + Date.now() + '.filestore.heapsnapshot', (err, filename)->
 		res.send filename
@@ -140,3 +140,10 @@ server.listen port, host, ->
 process.on 'SIGTERM', () ->
 	logger.log("filestore got SIGTERM, shutting down gracefully")
 	beginShutdown()
+
+if global.gc?
+	gcTimer = setInterval () ->
+		global.gc()
+		logger.log process.memoryUsage(), "global.gc"
+	, 3 * oneMinute = 60 * 1000
+	gcTimer.unref()

services/filestore/app/coffee/Errors.coffee

@@ -0,0 +1,9 @@
+NotFoundError = (message) ->
+	error = new Error(message)
+	error.name = "NotFoundError"
+	error.__proto__ = NotFoundError.prototype
+	return error
+NotFoundError.prototype.__proto__ = Error.prototype
+
+module.exports = Errors =
+	NotFoundError: NotFoundError

services/filestore/app/coffee/FSPersistorManager.coffee

@@ -1,12 +1,12 @@
 logger = require("logger-sharelatex")
 fs = require("fs")
 LocalFileWriter = require("./LocalFileWriter")
+Errors = require('./Errors')
 rimraf = require("rimraf")
-response = require ("response")
 
 filterName = (key) ->
   return key.replace /\//g, "_"
-  
+
 
 module.exports =
   sendFile: ( location, target, source, callback = (err)->) ->
@@ -27,19 +27,20 @@ module.exports =
         return callback err
       @sendFile location, target, fsPath, callback
 
-  getFileStream: (location, name, _callback = (err, res)->) ->
+  # opts may be {start: Number, end: Number}
+  getFileStream: (location, name, opts, _callback = (err, res)->) ->
     callback = (args...) ->
       _callback(args...)
       _callback = () ->
     filteredName = filterName name
     logger.log location:location, name:filteredName, "getting file"
-    sourceStream = fs.createReadStream "#{location}/#{filteredName}"
+    sourceStream = fs.createReadStream "#{location}/#{filteredName}", opts
     sourceStream.on 'error', (err) ->
       logger.err err:err, location:location, name:name, "Error reading from file"
-      if err.code = 'ENOENT'
+      if err.code == 'ENOENT'
-        callback null, response().html('NoSuchKey: file not found\n')
+        callback new Errors.NotFoundError(err.message), null
       else
-        callback err
+        callback err, null
     sourceStream.on 'readable', () ->
       # This can be called multiple times, but the callback wrapper
       # ensures the callback is only called once

services/filestore/app/coffee/FileController.coffee

@@ -3,20 +3,37 @@ settings = require("settings-sharelatex")
 logger = require("logger-sharelatex")
 FileHandler = require("./FileHandler")
 metrics = require("metrics-sharelatex")
-oneDayInSeconds = 60 * 60 * 24
+parseRange = require('range-parser')
+Errors = require('./Errors')
 
-module.exports =
+oneDayInSeconds = 60 * 60 * 24
+maxSizeInBytes = 1024 * 1024 * 1024 # 1GB
+
+module.exports = FileController =
 
 	getFile: (req, res)->
-		metrics.inc "getFile"
 		{key, bucket} = req
 		{format, style} = req.query
-		logger.log key:key, bucket:bucket, format:format, style:style, "receiving request to get file"
+		options = {
-		FileHandler.getFile bucket, key, {format:format,style:style}, (err, fileStream)->
+			key: key,
+			bucket: bucket,
+			format: format,
+			style: style,
+		}
+		metrics.inc "getFile"
+		logger.log key:key, bucket:bucket, format:format, style: style, "reciving request to get file"
+		if req.headers.range?
+			range = FileController._get_range(req.headers.range)
+			options.start = range.start
+			options.end = range.end
+			logger.log start: range.start, end: range.end, "getting range of bytes from file"
+		FileHandler.getFile bucket, key, options, (err, fileStream)->
 			if err?
 				logger.err err:err, key:key, bucket:bucket, format:format, style:style, "problem getting file"
-				if !res.finished and res?.send? 
+				if err instanceof Errors.NotFoundError
-					res.send 500
+					return res.send 404
+				if !res.finished and res?.send?
+					return res.send 500
 			else if req.query.cacheWarm
 				logger.log key:key, bucket:bucket, format:format, style:style, "request is only for cache warm so not sending stream"
 				res.send 200
@@ -29,6 +46,9 @@ module.exports =
 		{key, bucket} = req
 		logger.log key:key, bucket:bucket, "reciving request to insert file"
 		FileHandler.insertFile bucket, key, req, (err)->
+			if err?
+				logger.log err: err, key: key, bucket: bucket, "error inserting file"
+				res.send 500
 			res.send 200
 
 	copyFile: (req, res)->
@@ -38,7 +58,7 @@ module.exports =
 		oldFile_id = req.body.source.file_id
 		logger.log key:key, bucket:bucket, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "reciving request to copy file"
 		PersistorManager.copyFile bucket, "#{oldProject_id}/#{oldFile_id}", key, (err)->
-			if err? 
+			if err?
 				logger.log err:err, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "something went wrong copying file"
 				res.send 500
 			else
@@ -55,5 +75,10 @@ module.exports =
 			else
 				res.send 204
 
-
+	_get_range: (header) ->
-
+		parsed = parseRange(maxSizeInBytes, header)
+		if parsed == -1 or parsed == -2 or parsed.type != 'bytes'
+			null
+		else
+			range = parsed[0]
+			{start: range.start, end: range.end}

services/filestore/app/coffee/FileHandler.coffee

@@ -30,7 +30,7 @@ module.exports =
 			@_getConvertedFile bucket, key, opts, callback
 
 	_getStandardFile: (bucket, key, opts, callback)->
-		PersistorManager.getFileStream bucket, key, (err, fileStream)->
+		PersistorManager.getFileStream bucket, key, opts, (err, fileStream)->
 			if err?
 				logger.err  bucket:bucket, key:key, opts:opts, "error getting fileStream"
 			callback err, fileStream
@@ -38,6 +38,8 @@ module.exports =
 	_getConvertedFile: (bucket, key, opts, callback)->
 		convertedKey = KeyBuilder.addCachingToKey key, opts
 		PersistorManager.checkIfFileExists bucket, convertedKey, (err, exists)=>
+			if err?
+				return callback err
 			if exists
 				PersistorManager.getFileStream bucket, convertedKey, callback
 			else
@@ -58,10 +60,19 @@ module.exports =
 		], (err)->
 			if err?
 				return callback(err)
+<<<<<<< HEAD
 			PersistorManager.getFileStream bucket, convertedKey, callback
 
 	_convertFile: (bucket, originalKey, opts, callback)->
 		@_writeS3FileToDisk bucket, originalKey, (err, originalFsPath)->
+=======
+			PersistorManager.getFileStream bucket, convetedKey, opts, callback
+
+	_convertFile: (bucket, origonalKey, opts, callback)->
+		@_writeS3FileToDisk bucket, origonalKey, opts, (err, origonalFsPath)->
+			if err?
+				return callback(err)
+>>>>>>> sharelatex/master
 			done = (err, destPath)->
 				if err?
 					logger.err err:err, bucket:bucket, originalKey:originalKey, opts:opts, "error converting file"
@@ -76,10 +87,11 @@ module.exports =
 			else if opts.style == "preview"
 				FileConverter.preview originalFsPath, done
 			else
-				throw new Error("should have specified opts to convert file with #{JSON.stringify(opts)}")
+				return callback(new Error("should have specified opts to convert file with #{JSON.stringify(opts)}"))
 
 
-	_writeS3FileToDisk: (bucket, key, callback)->
+	_writeS3FileToDisk: (bucket, key, opts, callback)->
-		PersistorManager.getFileStream bucket, key, (err, fileStream)->
+		PersistorManager.getFileStream bucket, key, opts, (err, fileStream)->
+			if err?
+				return callback(err)
 			LocalFileWriter.writeStream fileStream, key, callback
-

services/filestore/app/coffee/S3PersistorManager.coffee

@@ -9,6 +9,7 @@ fs = require("fs")
 knox = require("knox")
 path = require("path")
 LocalFileWriter = require("./LocalFileWriter")
+Errors = require("./Errors")
 _ = require("underscore")
 
 thirtySeconds = 30 * 1000
@@ -57,17 +58,28 @@ module.exports =
 				logger.err  bucketName:bucketName, key:key, fsPath:fsPath, err:err, "something went wrong writing stream to disk"
 				return callback(err)
 			@sendFile bucketName, key, fsPath, callback
-			
+
-	getFileStream: (bucketName, key, callback = (err, res)->)->
+	# opts may be {start: Number, end: Number}
+	getFileStream: (bucketName, key, opts, callback = (err, res)->)->
+		opts = opts || {}
+		headers = {}
+		if opts.start? and opts.end?
+			headers['Range'] = "bytes=#{opts.start}-#{opts.end}"
 		callback = _.once callback
 		logger.log bucketName:bucketName, key:key, "getting file from s3"
 		s3Client = knox.createClient
 			key: settings.filestore.s3.key
 			secret: settings.filestore.s3.secret
 			bucket: bucketName
-		s3Stream = s3Client.get(key)
+		s3Stream = s3Client.get(key, headers)
 		s3Stream.end()
 		s3Stream.on 'response', (res) ->
+			if res.statusCode == 404
+				logger.log bucketName:bucketName, key:key, "file not found in s3"
+				return callback new Errors.NotFoundError("File not found in S3: #{bucketName}:#{key}"), null
+			if res.statusCode not in [200, 206]
+				logger.log bucketName:bucketName, key:key, "error getting file from s3: #{res.statusCode}"
+				return callback new Error("Got non-200 response from S3: #{res.statusCode}"), null
 			callback null, res
 		s3Stream.on 'error', (err) ->
 			logger.err err:err, bucketName:bucketName, key:key, "error getting file stream from s3"
@@ -125,4 +137,3 @@ module.exports =
 			exists = res.statusCode == 200
 			logger.log bucketName:bucketName, key:key, exists:exists, "checked if file exsists in s3"
 			callback(err, exists)
-

services/filestore/package.json

@@ -17,18 +17,19 @@
     "grunt-mocha-test": "~0.8.2",
     "heapdump": "^0.3.2",
     "knox": "~0.9.1",
-    "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#master",
+    "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#v1.1.0",
     "longjohn": "~0.2.2",
     "lynx": "0.0.11",
-    "metrics-sharelatex": "git+https://github.com/sharelatex/metrics-sharelatex.git#master",
+    "metrics-sharelatex": "git+https://github.com/sharelatex/metrics-sharelatex.git#v1.3.0",
     "node-transloadit": "0.0.4",
     "node-uuid": "~1.4.1",
     "pngcrush": "0.0.3",
+    "range-parser": "^1.0.2",
     "recluster": "^0.3.7",
     "request": "2.14.0",
     "response": "0.14.0",
     "rimraf": "2.2.8",
-    "settings-sharelatex": "git+https://github.com/sharelatex/settings-sharelatex.git#master",
+    "settings-sharelatex": "git+https://github.com/sharelatex/settings-sharelatex.git#v1.0.0",
     "stream-buffers": "~0.2.5",
     "underscore": "~1.5.2"
   },

services/filestore/test/acceptence/coffee/SendingFileTest.coffee

@@ -32,7 +32,7 @@ describe "Filestore", ->
 
 
 	it "should send a 200 for status endpoing", (done)->
-		request "#{@filestoreUrl}/status", (err, response, body)-> 
+		request "#{@filestoreUrl}/status", (err, response, body)->
 			response.statusCode.should.equal 200
 			body.indexOf("filestore").should.not.equal -1
 			body.indexOf("up").should.not.equal -1
@@ -50,18 +50,46 @@ describe "Filestore", ->
 			writeStream.on "end", done
 			fs.createReadStream(@localFileReadPath).pipe writeStream
 
+		it "should return 404 for a non-existant id", (done) ->
+			@timeout(1000 * 20)
+			options =
+				uri: @fileUrl + '___this_is_clearly_wrong___'
+			request.get options, (err, response, body) =>
+				response.statusCode.should.equal 404
+				done()
+
 		it "should be able get the file back", (done)->
 			@timeout(1000 * 10)
 			request.get @fileUrl, (err, response, body)=>
 				body.should.equal @constantFileContent
 				done()
 
+		it "should be able to get back the first 8 bytes of the file", (done) ->
+			@timeout(1000 * 10)
+			options =
+				uri: @fileUrl
+				headers:
+					'Range': 'bytes=0-8'
+			request.get options, (err, response, body)=>
+				body.should.equal 'hello wor'
+				done()
+
+		it "should be able to get back bytes 4 through 10 of the file", (done) ->
+			@timeout(1000 * 10)
+			options =
+				uri: @fileUrl
+				headers:
+					'Range': 'bytes=4-10'
+			request.get options, (err, response, body)=>
+				body.should.equal 'o world'
+				done()
+
 		it "should be able to delete the file", (done)->
 			@timeout(1000 * 20)
 			request.del @fileUrl, (err, response, body)=>
 				response.statusCode.should.equal 204
 				request.get @fileUrl, (err, response, body)=>
-					body.indexOf("NoSuchKey").should.not.equal -1
+					response.statusCode.should.equal 404
 					done()
 
 		it "should be able to copy files", (done)->
@@ -85,5 +113,59 @@ describe "Filestore", ->
 						body.should.equal @constantFileContent
 						done()
 
+	describe "with a pdf file", ->
 
+		beforeEach (done)->
+			@timeout(1000 * 10)
+			@file_id = Math.random()
+			@fileUrl = "#{@filestoreUrl}/project/acceptence_tests/file/#{@file_id}"
+			@localFileReadPath = __dirname + '/../../fixtures/test.pdf'
 
+			writeStream = request.post(@fileUrl)
+
+			writeStream.on "end", done
+			fs.createReadStream(@localFileReadPath).pipe writeStream
+
+		it "should be able get the file back", (done)->
+			@timeout(1000 * 10)
+			request.get @fileUrl, (err, response, body)=>
+				expect(body.substring(0, 8)).to.equal '%PDF-1.5'
+				done()
+
+		describe "getting the preview image", ->
+
+			beforeEach ->
+				@fileUrl = @fileUrl + '?style=preview'
+
+			it "should not time out", (done) ->
+				@timeout(1000 * 20)
+				request.get @fileUrl, (err, response, body) =>
+					expect(response).to.not.equal null
+					done()
+
+			it "should respond with image data", (done) ->
+				# note: this test relies of the imagemagick conversion working
+				@timeout(1000 * 20)
+				request.get @fileUrl, (err, response, body) =>
+					expect(response.statusCode).to.equal 200
+					expect(body.length).to.be.greaterThan 400
+					done()
+
+		describe "warming the cache", ->
+
+			beforeEach ->
+				@fileUrl = @fileUrl + '?style=preview&cacheWarm=true'
+
+			it "should not time out", (done) ->
+				@timeout(1000 * 20)
+				request.get @fileUrl, (err, response, body) =>
+					expect(response).to.not.equal null
+					done()
+
+			it "should respond with only an 'OK'", (done) ->
+				# note: this test relies of the imagemagick conversion working
+				@timeout(1000 * 20)
+				request.get @fileUrl, (err, response, body) =>
+					expect(response.statusCode).to.equal 200
+					body.should.equal 'OK'
+					done()

services/filestore/test/unit/coffee/FSPersistorManagerTests.coffee

@@ -29,6 +29,8 @@ describe "FSPersistorManagerTests", ->
         err:->
       "response":response
       "rimraf":@Rimraf
+      "./Errors": @Errors =
+        NotFoundError: sinon.stub()
     @location = "/tmp"
     @name1 = "530f2407e7ef165704000007/530f838b46d9a9e859000008"
     @name1Filtered ="530f2407e7ef165704000007_530f838b46d9a9e859000008"
@@ -69,14 +71,63 @@ describe "FSPersistorManagerTests", ->
         done()
 
   describe "getFileStream", ->
+    beforeEach ->
+      @opts = {}
+
     it "should use correct file location", (done) ->
-      @Fs.createReadStream.returns(
+      @Fs.createReadStream.returns({on: ->})
-        on:->
+      @FSPersistorManager.getFileStream @location, @name1, @opts, (err,res) =>
-      )
-      @FSPersistorManager.getFileStream @location, @name1, (err,res)=>
       @Fs.createReadStream.calledWith("#{@location}/#{@name1Filtered}").should.equal.true
       done()
 
+    describe "with start and end options", ->
+
+      beforeEach ->
+        @opts = {start: 0, end: 8}
+
+      it 'should pass the options to createReadStream', (done) ->
+        @Fs.createReadStream.returns({on: ->})
+        @FSPersistorManager.getFileStream @location, @name1, @opts, (err,res)=>
+        @Fs.createReadStream.calledWith("#{@location}/#{@name1Filtered}", @opts).should.equal true
+        done()
+
+    describe "error conditions", ->
+
+      beforeEach ->
+        @fakeCode = 'ENOENT'
+        @Fs.createReadStream.returns(
+          on: (key, callback) =>
+            err = new Error()
+            err.code = @fakeCode
+            callback(err, null)
+        )
+
+      describe "when the file does not exist", ->
+
+        beforeEach ->
+          @fakeCode = 'ENOENT'
+
+        it "should give a NotFoundError", (done) ->
+          @FSPersistorManager.getFileStream @location, @name1, @opts, (err,res)=>
+            expect(res).to.equal null
+            expect(err).to.not.equal null
+            expect(err instanceof @Errors.NotFoundError).to.equal true
+            done()
+
+      describe "when some other error happens", ->
+
+        beforeEach ->
+          @fakeCode = 'SOMETHINGHORRIBLE'
+
+        it "should give an Error", (done) ->
+          @FSPersistorManager.getFileStream @location, @name1, @opts, (err,res)=>
+            expect(res).to.equal null
+            expect(err).to.not.equal null
+            expect(err instanceof Error).to.equal true
+            done()
+
+
+
   describe "copyFile", ->
     beforeEach ->
       @ReadStream=
@@ -157,5 +208,3 @@ describe "FSPersistorManagerTests", ->
       @FSPersistorManager.checkIfFileExists @location, @name1, (err,exists) =>
         exists.should.be.false
         done()
-
-

services/filestore/test/unit/coffee/FileControllerTests.coffee

@@ -42,6 +42,7 @@ describe "FileController", ->
 			params:
 				project_id:@project_id
 				file_id:@file_id
+			headers: {}
 		@res =
 			setHeader: ->
 		@fileStream = {}
@@ -70,6 +71,19 @@ describe "FileController", ->
 				done()
 			@controller.getFile @req, @res
 
+		describe "with a 'Range' header set", ->
+
+			beforeEach ->
+				@req.headers.range = 'bytes=0-8'
+
+			it "should pass 'start' and 'end' options to FileHandler", (done) ->
+				@FileHandler.getFile.callsArgWith(3, null, @fileStream)
+				@fileStream.pipe = (res)=>
+					expect(@FileHandler.getFile.lastCall.args[2].start).to.equal 0
+					expect(@FileHandler.getFile.lastCall.args[2].end).to.equal 8
+					done()
+				@controller.getFile @req, @res
+
 	describe "insertFile", ->
 
 		it "should send bucket name key and res to PersistorManager", (done)->
@@ -101,7 +115,7 @@ describe "FileController", ->
 			@res.send = (code)=>
 				code.should.equal 500
 				done()
-			@controller.copyFile @req, @res	
+			@controller.copyFile @req, @res
 
 	describe "delete file", ->
 
@@ -119,3 +133,22 @@ describe "FileController", ->
 				code.should.equal 500
 				done()
 			@controller.deleteFile @req, @res
+
+	describe "_get_range", ->
+
+		it "should parse a valid Range header", (done) ->
+			result = @controller._get_range('bytes=0-200')
+			expect(result).to.not.equal null
+			expect(result.start).to.equal 0
+			expect(result.end).to.equal 200
+			done()
+
+		it "should return null for an invalid Range header", (done) ->
+			result = @controller._get_range('wat')
+			expect(result).to.equal null
+			done()
+
+		it "should return null for any type other than 'bytes'", (done) ->
+			result = @controller._get_range('carrots=0-200')
+			expect(result).to.equal null
+			done()

services/filestore/test/unit/coffee/FileHandlerTests.coffee

@@ -93,6 +93,13 @@ describe "FileHandler", ->
 				@handler._getConvertedFile.called.should.equal false
 				done()
 
+		it "should pass options to _getStandardFile", (done) ->
+			options = {start: 0, end: 8}
+			@handler.getFile @bucket, @key, options, =>
+				expect(@handler._getStandardFile.lastCall.args[2].start).to.equal 0
+				expect(@handler._getStandardFile.lastCall.args[2].end).to.equal 8
+				done()
+
 		it "should call _getConvertedFile if a format is defined", (done)->
 			@handler.getFile @bucket, @key, format:"png", =>
 				@handler._getStandardFile.called.should.equal false
@@ -104,7 +111,7 @@ describe "FileHandler", ->
 
 		beforeEach ->
 			@fileStream = {on:->}
-			@PersistorManager.getFileStream.callsArgWith(2, "err", @fileStream)
+			@PersistorManager.getFileStream.callsArgWith(3, "err", @fileStream)
 
 		it "should get the stream", (done)->
 			@handler.getFile @bucket, @key, null, =>
@@ -117,11 +124,18 @@ describe "FileHandler", ->
 				stream.should.equal @fileStream
 				done()
 
+		it "should pass options to PersistorManager", (done) ->
+			@handler.getFile @bucket, @key, {start: 0, end: 8}, =>
+				expect(@PersistorManager.getFileStream.lastCall.args[2].start).to.equal 0
+				expect(@PersistorManager.getFileStream.lastCall.args[2].end).to.equal 8
+				done()
+
+
 	describe "_getConvertedFile", ->
 
 		it "should getFileStream if it does exists", (done)->
 			@PersistorManager.checkIfFileExists.callsArgWith(2, null, true)
-			@PersistorManager.getFileStream.callsArgWith(2)
+			@PersistorManager.getFileStream.callsArgWith(3)
 			@handler._getConvertedFile @bucket, @key, {}, =>
 				@PersistorManager.getFileStream.calledWith(@bucket).should.equal true
 				done()
@@ -138,7 +152,7 @@ describe "FileHandler", ->
 		it "should _convertFile ", (done)->
 			@stubbedStream = {"something":"here"}
 			@PersistorManager.sendFile = sinon.stub().callsArgWith(3)
-			@PersistorManager.getFileStream = sinon.stub().callsArgWith(2, null, @stubbedStream)
+			@PersistorManager.getFileStream = sinon.stub().callsArgWith(3, null, @stubbedStream)
 			@convetedKey = @key+"converted"
 			@handler._convertFile = sinon.stub().callsArgWith(3, null, @stubbedPath)
 			@ImageOptimiser.compressPng = sinon.stub().callsArgWith(1)
@@ -155,7 +169,7 @@ describe "FileHandler", ->
 			@FileConverter.convert.callsArgWith(2, null, @formattedStubbedPath)
 			@FileConverter.thumbnail.callsArgWith(1, null, @formattedStubbedPath)
 			@FileConverter.preview.callsArgWith(1, null, @formattedStubbedPath)
-			@handler._writeS3FileToDisk = sinon.stub().callsArgWith(2, null, @stubbedPath)
+			@handler._writeS3FileToDisk = sinon.stub().callsArgWith(3, null, @stubbedPath)
 			@LocalFileWriter.deleteFile.callsArgWith(1)
 
 		it "should call thumbnail on the writer path if style was thumbnail was specified", (done)->
@@ -178,7 +192,3 @@ describe "FileHandler", ->
 				@FileConverter.convert.calledWith(@stubbedPath, @format).should.equal true
 				@LocalFileWriter.deleteFile.calledWith(@stubbedPath).should.equal true
 				done()
-
-
-
-				

services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee

@@ -35,6 +35,8 @@ describe "S3PersistorManagerTests", ->
 			"logger-sharelatex":
 				log:->
 				err:->
+			"./Errors": @Errors =
+				NotFoundError: sinon.stub()
 		@key = "my/key"
 		@bucketName = "my-bucket"
 		@error = "my errror"
@@ -42,17 +44,77 @@ describe "S3PersistorManagerTests", ->
 	describe "getFileStream", ->
 		beforeEach ->
 			@S3PersistorManager = SandboxedModule.require modulePath, requires: @requires
-
+			@opts = {}
 
 		it "should use correct key", (done)->
 			@stubbedKnoxClient.get.returns(
 				on:->
 				end:->
 			)
-			@S3PersistorManager.getFileStream @bucketName, @key, @fsPath, (err)=>
+			@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback
 			@stubbedKnoxClient.get.calledWith(@key).should.equal true
 			done()
 
+		describe "with start and end options", ->
+			beforeEach ->
+				@opts =
+					start: 0
+					end: 8
+			it "should pass headers to the knox.Client.get()", (done) ->
+				@stubbedKnoxClient.get.returns(
+					on:->
+					end:->
+				)
+				@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback
+				@stubbedKnoxClient.get.calledWith(@key, {'Range': 'bytes=0-8'}).should.equal true
+				done()
+
+		describe "error conditions", ->
+
+			beforeEach ->
+				@fakeResponse =
+					statusCode: 500
+				@stubbedKnoxClient.get.returns(
+					on: (key, callback) =>
+						if key == 'response'
+							callback(@fakeResponse)
+					end: ->
+				)
+
+			describe "when the file doesn't exist", ->
+
+				beforeEach ->
+					@fakeResponse =
+						statusCode: 404
+
+				it "should produce a NotFoundError", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						expect(stream).to.equal null
+						expect(err).to.not.equal null
+						expect(err instanceof @Errors.NotFoundError).to.equal true
+						done()
+
+				it "should have bucket and key in the Error message", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						error_message = @Errors.NotFoundError.lastCall.args[0]
+						expect(error_message).to.not.equal null
+						error_message.should.match(new RegExp(".*#{@bucketName}.*"))
+						error_message.should.match(new RegExp(".*#{@key}.*"))
+						done()
+
+			describe "when the S3 service produces an error", ->
+				beforeEach ->
+					@fakeResponse =
+						statusCode: 500
+
+				it "should produce an error", (done) ->
+					@S3PersistorManager.getFileStream @bucketName, @key, @opts, (err, stream)=> # empty callback
+						expect(stream).to.equal null
+						expect(err).to.not.equal null
+						expect(err instanceof Error).to.equal true
+						@Errors.NotFoundError.called.should.equal false
+						done()
+
 	describe "sendFile", ->
 
 		beforeEach ->