From 1e7d6581098c3eb8e8c16a7eb3ea6d25bfbbb155 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Thu, 2 Mar 2023 09:57:36 -0800 Subject: [PATCH] Graceful rimraf in updater --- ts/test-node/updater/common_test.ts | 5 ++- ts/test-node/updater/signature_test.ts | 13 +++--- ts/updater/common.ts | 19 ++++---- ts/updater/util.ts | 60 +++++++++++++++++--------- 4 files changed, 58 insertions(+), 39 deletions(-) diff --git a/ts/test-node/updater/common_test.ts b/ts/test-node/updater/common_test.ts index d48047f21d..2288ff9697 100644 --- a/ts/test-node/updater/common_test.ts +++ b/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'); }); diff --git a/ts/test-node/updater/signature_test.ts b/ts/test-node/updater/signature_test.ts index 421eaccf8d..094301fcc1 100644 --- a/ts/test-node/updater/signature_test.ts +++ b/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); } } }); diff --git a/ts/updater/common.ts b/ts/updater/common.ts index 2a9ba562cd..265bdcd95d 100644 --- a/ts/updater/common.ts +++ b/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 { return targetDir; } -export async function deleteTempDir(targetDir: string): Promise { +export async function deleteTempDir( + logger: LoggerType, + targetDir: string +): Promise { if (await pathExists(targetDir)) { const pathInfo = await stat(targetDir); if (!pathInfo.isDirectory()) { @@ -923,7 +922,7 @@ export async function deleteTempDir(targetDir: string): Promise { ); } - await rimrafPromise(targetDir); + await gracefulRimraf(logger, targetDir); } export function getCliOptions(options: ParserConfiguration['options']): T { diff --git a/ts/updater/util.ts b/ts/updater/util.ts index f165b7a078..85b78cb11d 100644 --- a/ts/updater/util.ts +++ b/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>({ + name, + operation, + args, logger, - fromPath, - toPath, startedAt, retryCount, retryAfter = 5 * durations.SECOND, timeout = 5 * durations.MINUTE, }: { + name: string; + operation: (...args: Args) => Promise; + args: Args; logger: LoggerType; - fromPath: string; - toPath: string; startedAt: number; retryCount: number; retryAfter?: number; timeout?: number; }): Promise { + const logId = `gracefulFS(${name})`; try { - await rename(fromPath, toPath); + await operation(...args); if (retryCount !== 0) { logger.info( - `gracefulRename: succeeded after ${retryCount} retries, renamed ` + - `${fromPath} to ${toPath}` + `${logId}: succeeded after ${retryCount} retries, ${args.join(', ')}` ); } } catch (error) { @@ -80,25 +86,22 @@ async function doGracefulRename({ } if (isOlderThan(startedAt, timeout)) { - logger.warn( - 'gracefulRename: timed out while retrying renaming ' + - `${fromPath} to ${toPath}` - ); + logger.warn(`${logId}: timed out, ${args.join(', ')}`); throw error; } logger.warn( - `gracefulRename: got ${error.code} when renaming ` + - `${fromPath} to ${toPath}, retrying in one second. ` + - `(retryCount=${retryCount})` + `${logId}: got ${error.code} when running on ${args.join(', ')}; ` + + `retrying in one second. (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 { - 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 { + return doGracefulFSOperation({ + name: 'rimraf', + operation: rimrafPromise, + args: [path], logger, - fromPath, - toPath, startedAt: Date.now(), retryCount: 0, });