Skip to content

Commit f53da71

Browse files
authored
perf: Use a more aggressive retry loop for recursiveDelete (#84444)
**Update:** This also fixes a bug pointed out by Graphite where the deletion function could hang forever because the `t` variable used for tracking attempts wasn't properly incremented. --- https://vercel.slack.com/archives/C04KC8A53T7/p1759360239193629 This is a critical path for `next dev` and `next build` initialization. On Windows, it's quite common to get an error while trying to recursively delete directories for one of two reasons: **Temporarily open files:** 1. You open the directory. 2. The user's AV detects the directory as opened and quickly scans the files in that directory. 3. You try to delete a file, but on Windows, files cannot be deleted when open. **Deletion is not instant/atomic:** It can take some time after deleting files for the directory to become empty. For this reason, we perform two retries with a 100ms delay after the first attempt and a 200ms delay after the second attempt. * Node does not use retries by default, but has an option to do so with a `100ms * attemptNumber` (linear backoff) delay, similar to what we do: https://github.com/nodejs/node/blob/b5638e1765d3d1241d07457e767f27e3ee6b9dd2/lib/internal/fs/utils.js#L774-L779 * Node used to use 3 retries by default, but that was changed in v14: nodejs/node@7e85f06 * Rust tries to delete 64 times in a loop without sleeping, but with a call to `thread::yield_now`: https://github.com/rust-lang/rust/blob/4da69dfff1929cc79872b3d05ab7112d84753dba/library/std/src/sys/fs/windows/remove_dir_all.rs#L154-L171 In the vein of reducing time spent sleeping in the critical initialization path (@timneutkens), I think we should be more aggressive, I'm thinking using a 10ms delay (arbitrary number), but with an exponential backoff, so that we still retry over the same 300ms period as we currently do? Thoughts? The difficulty is that this is on Windows, so it's hard for me to get good numbers about what impact this would have (if any).
1 parent 047e641 commit f53da71

File tree

2 files changed

+39
-5
lines changed

2 files changed

+39
-5
lines changed

packages/next/src/lib/recursive-delete.ts

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,30 @@ import { join, isAbsolute, dirname } from 'path'
44
import isError from './is-error'
55
import { wait } from './wait'
66

7-
const unlinkPath = async (p: string, isDir = false, t = 1): Promise<void> => {
7+
// We use an exponential backoff. See the unit test for example values.
8+
//
9+
// - Node's `fs` module uses a linear backoff, starting with 100ms.
10+
// - Rust tries 64 times with only a `thread::yield_now` in between.
11+
//
12+
// We want something more aggressive, as `recursiveDelete` is in the critical
13+
// path of `next dev` and `next build` startup.
14+
const INITIAL_RETRY_MS = 8
15+
const MAX_RETRY_MS = 64
16+
const MAX_RETRIES = 6
17+
18+
/**
19+
* Used in unit test.
20+
* @ignore
21+
*/
22+
export function calcBackoffMs(attempt: number): number {
23+
return Math.min(INITIAL_RETRY_MS * Math.pow(2, attempt), MAX_RETRY_MS)
24+
}
25+
26+
const unlinkPath = async (
27+
p: string,
28+
isDir = false,
29+
attempt = 0
30+
): Promise<void> => {
831
try {
932
if (isDir) {
1033
await promises.rmdir(p)
@@ -18,10 +41,12 @@ const unlinkPath = async (p: string, isDir = false, t = 1): Promise<void> => {
1841
code === 'ENOTEMPTY' ||
1942
code === 'EPERM' ||
2043
code === 'EMFILE') &&
21-
t < 3
44+
attempt < MAX_RETRIES
2245
) {
23-
await wait(t * 100)
24-
return unlinkPath(p, isDir, t++)
46+
// retrying is unlikely to succeed on POSIX platforms, but Windows can
47+
// fail due to temporarily-open files or due to
48+
await wait(calcBackoffMs(attempt))
49+
return unlinkPath(p, isDir, attempt + 1)
2550
}
2651

2752
if (code === 'ENOENT') {

test/unit/recursive-delete.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* eslint-env jest */
22
import fs from 'fs-extra'
3-
import { recursiveDelete } from 'next/dist/lib/recursive-delete'
3+
import { recursiveDelete, calcBackoffMs } from 'next/dist/lib/recursive-delete'
44
import { recursiveReadDir } from 'next/dist/lib/recursive-readdir'
55
import { recursiveCopy } from 'next/dist/lib/recursive-copy'
66
import { join } from 'path'
@@ -48,3 +48,12 @@ describe('recursiveDelete', () => {
4848
}
4949
})
5050
})
51+
52+
describe('calcBackoffMs', () => {
53+
it('returns expected values', () => {
54+
let backoffValuesMs = Array.from({ length: 6 }, (_, attempt) =>
55+
calcBackoffMs(attempt)
56+
)
57+
expect(backoffValuesMs).toEqual([8, 16, 32, 64, 64, 64])
58+
})
59+
})

0 commit comments

Comments
 (0)