Graceful rimraf in updater

ts/test-node/updater/common_test.ts

@@ -17,6 +17,7 @@ import {
   getTempDir,
   deleteTempDir,
 } from '../../updater/common';
+import * as log from '../../logging/log';
 
 describe('updater/signatures', () => {
   const windows = parseYaml(`version: 1.23.2
@@ -164,7 +165,7 @@ releaseDate: '2021-12-03T19:00:23.754Z'
       const dir = await createTempDir();
       assert.isTrue((await stat(dir)).isDirectory());
 
-      await deleteTempDir(dir);
+      await deleteTempDir(log, dir);
 
       assert.isFalse(await pathExists(dir), 'Directory should be deleted');
     });
@@ -181,7 +182,7 @@ releaseDate: '2021-12-03T19:00:23.754Z'
 
       await mkdir(dir);
 
-      await deleteTempDir(dir);
+      await deleteTempDir(log, dir);
 
       assert.isFalse(await pathExists(dir), 'Directory should be deleted');
     });

ts/test-node/updater/signature_test.ts

@@ -17,6 +17,7 @@ import {
 } from '../../updater/signature';
 import { createTempDir, deleteTempDir } from '../../updater/common';
 import { keyPair } from '../../updater/curve';
+import * as log from '../../logging/log';
 
 describe('updater/signatures', () => {
   it('_getFileHash returns correct hash', async () => {
@@ -48,7 +49,7 @@ describe('updater/signatures', () => {
       );
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });
@@ -86,7 +87,7 @@ describe('updater/signatures', () => {
       assert.strictEqual(verified, true);
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });
@@ -123,7 +124,7 @@ describe('updater/signatures', () => {
       assert.strictEqual(verified, false);
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });
@@ -160,7 +161,7 @@ describe('updater/signatures', () => {
       assert.strictEqual(verified, false);
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });
@@ -202,7 +203,7 @@ describe('updater/signatures', () => {
       assert.strictEqual(verified, false);
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });
@@ -239,7 +240,7 @@ describe('updater/signatures', () => {
       assert.strictEqual(verified, false);
     } finally {
       if (tempDir) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(log, tempDir);
       }
     }
   });

ts/updater/common.ts

@@ -18,8 +18,6 @@ import { gt, lt } from 'semver';
 import config from 'config';
 import got from 'got';
 import { v4 as getGuid } from 'uuid';
-import pify from 'pify';
-import rimraf from 'rimraf';
 import type { BrowserWindow } from 'electron';
 import { app, ipcMain } from 'electron';
 
@@ -41,7 +39,7 @@ import type { SettingsChannel } from '../main/settingsChannel';
 
 import type { LoggerType } from '../types/Logging';
 import { getGotOptions } from './got';
-import { checkIntegrity, gracefulRename } from './util';
+import { checkIntegrity, gracefulRename, gracefulRimraf } from './util';
 import type { PrepareDownloadResultType as DifferentialDownloadDataType } from './differential';
 import {
   prepareDownload as prepareDifferentialDownload,
@@ -50,8 +48,6 @@ import {
   isValidPreparedData as isValidDifferentialData,
 } from './differential';
 
-const rimrafPromise = pify(rimraf);
-
 const INTERVAL = 30 * durations.MINUTE;
 
 type JSONUpdateSchema = {
@@ -604,7 +600,7 @@ export abstract class Updater {
         // We could have failed to update differentially due to low free disk
         // space. Remove all cached updates since we are doing a full download
         // anyway.
-        await rimrafPromise(cacheDir);
+        await gracefulRimraf(this.logger, cacheDir);
         cacheDir = await createUpdateCacheDirIfNeeded();
 
         await this.downloadAndReport(
@@ -650,7 +646,7 @@ export abstract class Updater {
       }
 
       try {
-        await deleteTempDir(restoreDir);
+        await deleteTempDir(this.logger, restoreDir);
       } catch (error) {
         this.logger.warn(
           'downloadUpdate: Failed to remove backup folder, ignoring',
@@ -661,7 +657,7 @@ export abstract class Updater {
       return { updateFilePath: targetUpdatePath, signature };
     } finally {
       if (!tempPathFailover) {
-        await deleteTempDir(tempDir);
+        await deleteTempDir(this.logger, tempDir);
       }
     }
   }
@@ -906,7 +902,10 @@ export async function createUpdateCacheDirIfNeeded(): Promise<string> {
   return targetDir;
 }
 
-export async function deleteTempDir(targetDir: string): Promise<void> {
+export async function deleteTempDir(
+  logger: LoggerType,
+  targetDir: string
+): Promise<void> {
   if (await pathExists(targetDir)) {
     const pathInfo = await stat(targetDir);
     if (!pathInfo.isDirectory()) {
@@ -923,7 +922,7 @@ export async function deleteTempDir(targetDir: string): Promise<void> {
     );
   }
 
-  await rimrafPromise(targetDir);
+  await gracefulRimraf(logger, targetDir);
 }
 
 export function getCliOptions<T>(options: ParserConfiguration['options']): T {

ts/updater/util.ts

@@ -5,6 +5,8 @@ import { createReadStream } from 'fs';
 import { rename } from 'fs/promises';
 import { pipeline } from 'stream/promises';
 import { createHash } from 'crypto';
+import rimraf from 'rimraf';
+import { promisify } from 'util';
 
 import * as Errors from '../types/errors';
 import type { LoggerType } from '../types/Logging';
@@ -12,6 +14,8 @@ import * as durations from '../util/durations';
 import { isOlderThan } from '../util/timestamp';
 import { sleep } from '../util/sleep';
 
+const rimrafPromise = promisify(rimraf);
+
 export type CheckIntegrityResultType = Readonly<
   | {
       ok: true;
@@ -48,30 +52,32 @@ export async function checkIntegrity(
   }
 }
 
-async function doGracefulRename({
+async function doGracefulFSOperation<Args extends ReadonlyArray<unknown>>({
+  name,
+  operation,
+  args,
   logger,
-  fromPath,
-  toPath,
   startedAt,
   retryCount,
   retryAfter = 5 * durations.SECOND,
   timeout = 5 * durations.MINUTE,
 }: {
+  name: string;
+  operation: (...args: Args) => Promise<void>;
+  args: Args;
   logger: LoggerType;
-  fromPath: string;
-  toPath: string;
   startedAt: number;
   retryCount: number;
   retryAfter?: number;
   timeout?: number;
 }): Promise<void> {
+  const logId = `gracefulFS(${name})`;
   try {
-    await rename(fromPath, toPath);
+    await operation(...args);
 
     if (retryCount !== 0) {
       logger.info(
-        `gracefulRename: succeeded after ${retryCount} retries, renamed ` +
+        `${logId}: succeeded after ${retryCount} retries, ${args.join(', ')}`
-          `${fromPath} to ${toPath}`
       );
     }
   } catch (error) {
@@ -80,25 +86,22 @@ async function doGracefulRename({
     }
 
     if (isOlderThan(startedAt, timeout)) {
-      logger.warn(
+      logger.warn(`${logId}: timed out, ${args.join(', ')}`);
-        'gracefulRename: timed out while retrying renaming ' +
-          `${fromPath} to ${toPath}`
-      );
       throw error;
     }
 
     logger.warn(
-      `gracefulRename: got ${error.code} when renaming ` +
+      `${logId}: got ${error.code} when running on ${args.join(', ')}; ` +
-        `${fromPath} to ${toPath}, retrying in one second. ` +
+        `retrying in one second. (retryCount=${retryCount})`
-        `(retryCount=${retryCount})`
     );
 
     await sleep(retryAfter);
 
-    return doGracefulRename({
+    return doGracefulFSOperation({
+      name,
+      operation,
+      args,
       logger,
-      fromPath,
-      toPath,
       startedAt,
       retryCount: retryCount + 1,
       retryAfter,
@@ -112,10 +115,25 @@ export async function gracefulRename(
   fromPath: string,
   toPath: string
 ): Promise<void> {
-  return doGracefulRename({
+  return doGracefulFSOperation({
+    name: 'rename',
+    operation: rename,
+    args: [fromPath, toPath],
+    logger,
+    startedAt: Date.now(),
+    retryCount: 0,
+  });
+}
+
+export async function gracefulRimraf(
+  logger: LoggerType,
+  path: string
+): Promise<void> {
+  return doGracefulFSOperation({
+    name: 'rimraf',
+    operation: rimrafPromise,
+    args: [path],
     logger,
-    fromPath,
-    toPath,
     startedAt: Date.now(),
     retryCount: 0,
   });