fs: treat `std::errc::permission_denied` as `EPERM` error

Project: nodejs/node | Language: JavaScript, C++ | Concepts: error-handling, retry-backoff, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/64698

The problem

On Windows, when deleting files or directories, certain file locking errors (specifically std::errc::permission_denied) were not being treated as retriable EPERM errors by the fs.rmSync operation. This meant that in some cases, rmSync would fail immediately instead of retrying as intended. Additionally, the retry delay calculation for Windows was incorrect, causing the actual sleep time between retries to be much shorter than specified.

Why it's non-trivial

A naive fix might just add permission_denied to the list of retriable errors everywhere, but this could have unintended consequences on non-Windows platforms. Also, retry logic is sensitive to time units—mixing up milliseconds and seconds can make retries either too fast or too slow, and such bugs are easy to overlook.

The approach

1. Add permission_denied as a retriable error, but only on Windows:

+#ifdef _WIN32
+            error == std::errc::permission_denied ||
+#endif

This snippet adds std::errc::permission_denied to the set of errors that should trigger a retry, but only when compiling for Windows. This is important because permission_denied maps to EPERM on Windows in the context of file locking, but not necessarily on other platforms.

2. Fix the retry delay calculation for Windows:

-      Sleep(i * retryDelay / 1000);
+      // No conversion needed: Sleep() takes milliseconds.
+      Sleep(i * retryDelay);

The original code divided the delay by 1000, which is only necessary if the sleep function expects seconds. On Windows, Sleep() expects milliseconds, so the division is removed to ensure the correct delay.

3. Add a comprehensive test to verify retry behavior and timing:

A new test file is added to ensure that the retry logic works as expected when encountering a locked file on Windows. The test:

Spawns a child process that locks a file.
Attempts to remove the directory containing the locked file with and without retries.
Asserts that the elapsed time matches the expected retry backoff, catching both logic and unit errors.

Key snippet from the test:

const options = { maxRetries: 4, retryDelay: 100 };
const expected = minRetryTime(options);  // 100+200+300+400 = 1000 ms.
const elapsed = timedRmThrowsEPERM(dir, options);

assert.ok(elapsed >= expected - slack, ...);
assert.ok(elapsed < common.platformTimeout(expected * 10), ...);

This ensures that the retry logic waits the correct amount of time and that the units are handled properly.

From the review thread

jasnell: A comment on this would be helpful.

PickBas: Added the comments, thank you

Takeaways

Always match error handling and retry logic to platform-specific error codes and behaviors.
Carefully check and document time unit conversions when working with cross-platform APIs.
Use targeted, timing-aware tests to catch subtle bugs in retry and backoff logic, especially around error handling.

fs: treat std::errc::permission_denied as EPERM error