[8.10] RollingFileAppender: fix file moving mechanism (#164688) (#164716)

# Backport This will backport the following commits from `main` to `8.10`: - [RollingFileAppender: fix file moving mechanism (#164688)](https://github.com/elastic/kibana/pull/164688)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Pierre Gayvallet <pierre.gayvallet@elastic.co>
2025-04-24 17:59:23 -04:00 · 2023-08-24 11:08:30 -04:00 · 2023-08-24 11:08:30 -04:00 · e776ad88d9
commit e776ad88d9
parent 321c33fd73
4 changed files with 134 additions and 3 deletions
--- a/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/rolling_tasks.ts
+++ b/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/rolling_tasks.ts
@ -7,8 +7,9 @@
 */

 import { join } from 'path';
-import { readdir, rename, unlink, access } from 'fs/promises';
+import { readdir, unlink, access } from 'fs/promises';
 import { getFileNameMatcher, getRollingFileName } from './pattern_matcher';
+import { moveFile } from './utils';

 export const shouldSkipRollout = async ({ logFilePath }: { logFilePath: string }) => {
  // in case of time-interval triggering policy, we can have an entire
@ -70,7 +71,7 @@ export const rollPreviousFilesInOrder = async ({
  for (let i = filesToRoll.length - 1; i >= 0; i--) {
    const oldFileName = filesToRoll[i];
    const newFileName = getRollingFileName(logFileBaseName, pattern, i + 2);
-    await rename(join(logFileFolder, oldFileName), join(logFileFolder, newFileName));
+    await moveFile(join(logFileFolder, oldFileName), join(logFileFolder, newFileName));
  }
 };

@ -84,5 +85,5 @@ export const rollCurrentFile = async ({
  pattern: string;
 }) => {
  const rolledBaseName = getRollingFileName(logFileBaseName, pattern, 1);
-  await rename(join(logFileFolder, logFileBaseName), join(logFileFolder, rolledBaseName));
+  await moveFile(join(logFileFolder, logFileBaseName), join(logFileFolder, rolledBaseName));
 };
--- a/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.test.mocks.ts
+++ b/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.test.mocks.ts
@ -0,0 +1,21 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+export const copyFileMock = jest.fn();
+export const renameMock = jest.fn();
+export const unlinkMock = jest.fn();
+
+jest.doMock('fs/promises', () => {
+  const actual = jest.requireActual('fs/promises');
+  return {
+    ...actual,
+    copyFile: copyFileMock,
+    rename: renameMock,
+    unlink: unlinkMock,
+  };
+});
--- a/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.test.ts
+++ b/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.test.ts
@ -0,0 +1,85 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { unlinkMock, renameMock, copyFileMock } from './utils.test.mocks';
+import { moveFile } from './utils';
+
+describe('moveFile', () => {
+  beforeEach(() => {
+    unlinkMock.mockReset();
+    renameMock.mockReset();
+    copyFileMock.mockReset();
+  });
+
+  it('only calls `rename` when call succeeds', async () => {
+    await moveFile('from', 'to');
+
+    expect(renameMock).toHaveBeenCalledTimes(1);
+    expect(renameMock).toHaveBeenCalledWith('from', 'to');
+
+    expect(copyFileMock).not.toHaveBeenCalled();
+    expect(unlinkMock).not.toHaveBeenCalled();
+  });
+
+  const createError = (code: string) => {
+    const err = new Error(code);
+    (err as any).code = code;
+    return err;
+  };
+
+  it('throws error if `rename` throws a non-EXDEV error', async () => {
+    renameMock.mockRejectedValue(createError('something'));
+
+    await expect(moveFile('from', 'to')).rejects.toThrowError('something');
+
+    expect(renameMock).toHaveBeenCalledTimes(1);
+    expect(copyFileMock).not.toHaveBeenCalled();
+    expect(unlinkMock).not.toHaveBeenCalled();
+  });
+
+  it('fallback to copy+unlink when `rename` throws a EXDEV error', async () => {
+    renameMock.mockRejectedValue(createError('EXDEV'));
+
+    await moveFile('from', 'to');
+
+    expect(renameMock).toHaveBeenCalledTimes(1);
+    expect(renameMock).toHaveBeenCalledWith('from', 'to');
+
+    expect(copyFileMock).toHaveBeenCalledTimes(1);
+    expect(copyFileMock).toHaveBeenCalledWith('from', 'to');
+
+    expect(unlinkMock).toHaveBeenCalledTimes(1);
+    expect(unlinkMock).toHaveBeenCalledWith('from');
+  });
+
+  it('throws if copyFile call throws', async () => {
+    renameMock.mockRejectedValue(createError('EXDEV'));
+    copyFileMock.mockRejectedValue(createError('anything'));
+
+    await expect(moveFile('from', 'to')).rejects.toThrowError('anything');
+
+    expect(renameMock).toHaveBeenCalledTimes(1);
+
+    expect(copyFileMock).toHaveBeenCalledTimes(1);
+
+    expect(unlinkMock).not.toHaveBeenCalled();
+  });
+
+  it('throws if unlink call throws', async () => {
+    renameMock.mockRejectedValue(createError('EXDEV'));
+    unlinkMock.mockRejectedValue(createError('something-else'));
+
+    await expect(moveFile('from', 'to')).rejects.toThrowError('something-else');
+
+    expect(renameMock).toHaveBeenCalledTimes(1);
+
+    expect(copyFileMock).toHaveBeenCalledTimes(1);
+
+    expect(unlinkMock).toHaveBeenCalledTimes(1);
+  });
+});
--- a/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.ts
+++ b/packages/core/logging/core-logging-server-internal/src/appenders/rolling_file/strategies/numeric/utils.ts
@ -0,0 +1,24 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { copyFile, rename, unlink } from 'fs/promises';
+
+export const moveFile = async (oldPath: string, newPath: string): Promise<void> => {
+  try {
+    await rename(oldPath, newPath);
+  } catch (err) {
+    // rename isn't supported on some file systems / volumes
+    // so we fallback to copy+delete
+    if (err.code === 'EXDEV') {
+      await copyFile(oldPath, newPath);
+      await unlink(oldPath);
+    } else {
+      throw err;
+    }
+  }
+};