jest-runtime: atomic cache write, and check validity of data (#4088)

packages/jest-runtime/package.json

@@ -22,6 +22,7 @@
     "json-stable-stringify": "^1.0.1",
     "micromatch": "^2.3.11",
     "strip-bom": "3.0.0",
+    "write-file-atomic": "^2.1.0",
     "yargs": "^7.0.2"
   },
   "devDependencies": {

packages/jest-runtime/src/__tests__/script_transformer.test.js

@@ -9,9 +9,11 @@
  */
 'use strict';
 
+const crypto = require('crypto');
 const slash = require('slash');
 
 jest
+  .mock('fs')
   .mock('graceful-fs')
   .mock('jest-haste-map', () => ({
     getCacheFilePath: (cacheDir, baseDir, version) => cacheDir + baseDir,
@@ -100,6 +102,14 @@ let fs;
 let mockFs;
 let object;
 let vm;
+let writeFileAtomic;
+
+jest.mock('write-file-atomic', () => ({
+  sync: jest.fn().mockImplementation((filePath, data) => {
+    const normalizedPath = require('slash')(filePath);
+    mockFs[normalizedPath] = data;
+  }),
+}));
 
 describe('ScriptTransformer', () => {
   const reset = () => {
@@ -145,6 +155,8 @@ describe('ScriptTransformer', () => {
 
     fs.existsSync = jest.fn(path => !!mockFs[path]);
 
+    writeFileAtomic = require('write-file-atomic');
+
     config = {
       cache: true,
       cacheDirectory: '/cache/',
@@ -290,11 +302,10 @@ describe('ScriptTransformer', () => {
       mapCoverage: true,
     });
     expect(result.sourceMapPath).toEqual(expect.any(String));
-    expect(fs.writeFileSync).toBeCalledWith(
-      result.sourceMapPath,
-      JSON.stringify(map),
-      'utf8',
-    );
+    const mapStr = JSON.stringify(map);
+    expect(writeFileAtomic.sync).toBeCalledWith(result.sourceMapPath, mapStr, {
+      encoding: 'utf8',
+    });
   });
 
   it('writes source map if preprocessor inlines it', () => {
@@ -320,11 +331,9 @@ describe('ScriptTransformer', () => {
       mapCoverage: true,
     });
     expect(result.sourceMapPath).toEqual(expect.any(String));
-    expect(fs.writeFileSync).toBeCalledWith(
-      result.sourceMapPath,
-      sourceMap,
-      'utf8',
-    );
+    expect(
+      writeFileAtomic.sync,
+    ).toBeCalledWith(result.sourceMapPath, sourceMap, {encoding: 'utf8'});
   });
 
   it('does not write source map if mapCoverage option is false', () => {
@@ -348,7 +357,7 @@ describe('ScriptTransformer', () => {
       mapCoverage: false,
     });
     expect(result.sourceMapPath).toBeFalsy();
-    expect(fs.writeFileSync).toHaveBeenCalledTimes(1);
+    expect(writeFileAtomic.sync).toHaveBeenCalledTimes(1);
   });
 
   it('reads values from the cache', () => {
@@ -359,8 +368,8 @@ describe('ScriptTransformer', () => {
     scriptTransformer.transform('/fruits/banana.js', {});
 
     const cachePath = getCachePath(mockFs, config);
-    expect(fs.writeFileSync).toBeCalled();
-    expect(fs.writeFileSync.mock.calls[0][0]).toBe(cachePath);
+    expect(writeFileAtomic.sync).toBeCalled();
+    expect(writeFileAtomic.sync.mock.calls[0][0]).toBe(cachePath);
 
     // Cache the state in `mockFsCopy`
     const mockFsCopy = mockFs;
@@ -375,7 +384,7 @@ describe('ScriptTransformer', () => {
     expect(fs.readFileSync.mock.calls.length).toBe(2);
     expect(fs.readFileSync).toBeCalledWith('/fruits/banana.js', 'utf8');
     expect(fs.readFileSync).toBeCalledWith(cachePath, 'utf8');
-    expect(fs.writeFileSync).not.toBeCalled();
+    expect(writeFileAtomic.sync).not.toBeCalled();
 
     // Don't read from the cache when `config.cache` is false.
     jest.resetModuleRegistry();
@@ -388,6 +397,6 @@ describe('ScriptTransformer', () => {
     expect(fs.readFileSync.mock.calls.length).toBe(1);
     expect(fs.readFileSync).toBeCalledWith('/fruits/banana.js', 'utf8');
     expect(fs.readFileSync).not.toBeCalledWith(cachePath, 'utf8');
-    expect(fs.writeFileSync).toBeCalled();
+    expect(writeFileAtomic.sync).toBeCalled();
   });
 });

packages/jest-runtime/src/script_transformer.js

@@ -28,6 +28,7 @@ import stableStringify from 'json-stable-stringify';
 import slash from 'slash';
 import {version as VERSION} from '../package.json';
 import shouldInstrument from './should_instrument';
+import writeFileAtomic from 'write-file-atomic';
 
 export type Options = {|
   collectCoverage: boolean,
@@ -42,6 +43,9 @@ const configToJsonMap = new Map();
 // Cache regular expressions to test whether the file needs to be preprocessed
 const ignoreCache: WeakMap<ProjectConfig, ?RegExp> = new WeakMap();
 
+// To reset the cache for specific changesets (rather than package version).
+const CACHE_VERSION = '1';
+
 class ScriptTransformer {
   static EVAL_RESULT_VARIABLE: string;
   _config: ProjectConfig;
@@ -67,16 +71,23 @@ class ScriptTransformer {
     const transformer = this._getTransformer(filename);
 
     if (transformer && typeof transformer.getCacheKey === 'function') {
-      return transformer.getCacheKey(fileData, filename, configString, {
-        instrument,
-      });
+      return crypto
+        .createHash('md5')
+        .update(
+          transformer.getCacheKey(fileData, filename, configString, {
+            instrument,
+          }),
+        )
+        .update(CACHE_VERSION)
+        .digest('hex');
     } else {
       return crypto
         .createHash('md5')
         .update(fileData)
         .update(configString)
         .update(instrument ? 'instrument' : '')
         .update(mapCoverage ? 'mapCoverage' : '')
+        .update(CACHE_VERSION)
         .digest('hex');
     }
   }
@@ -184,11 +195,13 @@ class ScriptTransformer {
     );
     let sourceMapPath = cacheFilePath + '.map';
     // Ignore cache if `config.cache` is set (--no-cache)
-    let code = this._config.cache
-      ? readCacheFile(filename, cacheFilePath)
-      : null;
+    let code = this._config.cache ? readCodeCacheFile(cacheFilePath) : null;
 
     if (code) {
+      // This is broken: we return the code, and a path for the source map
+      // directly from the cache. But, nothing ensures the source map actually
+      // matches that source code. They could have gotten out-of-sync in case
+      // two separate processes write concurrently to the same cache files.
       return {
         code,
         sourceMapPath,
@@ -248,7 +261,7 @@ class ScriptTransformer {
       sourceMapPath = null;
     }
 
-    writeCacheFile(cacheFilePath, code);
+    writeCodeCacheFile(cacheFilePath, code);
 
     return {
       code,
@@ -343,9 +356,46 @@ const stripShebang = content => {
   }
 };
 
+/**
+ * This is like `writeCacheFile` but with an additional sanity checksum. We
+ * cannot use the same technique for source maps because we expose source map
+ * cache file paths directly to callsites, with the expectation they can read
+ * it right away. This is not a great system, because source map cache file
+ * could get corrupted, out-of-sync, etc.
+ */
+function writeCodeCacheFile(cachePath: Path, code: string) {
+  const checksum = crypto.createHash('md5').update(code).digest('hex');
+  writeCacheFile(cachePath, checksum + '\n' + code);
+}
+
+/**
+ * Read counterpart of `writeCodeCacheFile`. We verify that the content of the
+ * file matches the checksum, in case some kind of corruption happened. This
+ * could happen if an older version of `jest-runtime` writes non-atomically to
+ * the same cache, for example.
+ */
+function readCodeCacheFile(cachePath: Path): ?string {
+  const content = readCacheFile(cachePath);
+  if (content == null) {
+    return null;
+  }
+  const code = content.substr(33);
+  const checksum = crypto.createHash('md5').update(code).digest('hex');
+  if (checksum === content.substr(0, 32)) {
+    return code;
+  }
+  return null;
+}
+
+/**
+ * Writing to the cache atomically relies on 'rename' being atomic on most
+ * file systems. Doing atomic write reduces the risk of corruption by avoiding
+ * two processes to write to the same file at the same time. It also reduces
+ * the risk of reading a file that's being overwritten at the same time.
+ */
 const writeCacheFile = (cachePath: Path, fileData: string) => {
   try {
-    fs.writeFileSync(cachePath, fileData, 'utf8');
+    writeFileAtomic.sync(cachePath, fileData, {encoding: 'utf8'});
   } catch (e) {
     e.message =
       'jest: failed to cache transform results in: ' +
@@ -357,7 +407,7 @@ const writeCacheFile = (cachePath: Path, fileData: string) => {
   }
 };
 
-const readCacheFile = (filename: Path, cachePath: Path): ?string => {
+const readCacheFile = (cachePath: Path): ?string => {
   if (!fs.existsSync(cachePath)) {
     return null;
   }
@@ -366,7 +416,11 @@ const readCacheFile = (filename: Path, cachePath: Path): ?string => {
   try {
     fileData = fs.readFileSync(cachePath, 'utf8');
   } catch (e) {
-    e.message = 'jest: failed to read cache file: ' + cachePath;
+    e.message =
+      'jest: failed to read cache file: ' +
+      cachePath +
+      '\nFailure message: ' +
+      e.message;
     removeFile(cachePath);
     throw e;
   }