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

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet

[louiellan]

Fixes#64016

In the thrown exception, std::errc::permission_denied is already treated as an EPERM error, but it is not included in one of the omittable errors when retrying the RmSync operation. This commit includes it.

This also fixes the retryDelay calculation on Windows where the retryDelay is divided by 1000 but the win32's Sleep function takes the argument as an ms unit, dividing the supplied ms unit further to a much smaller delay.

I tried writing a test for this fix but node doesn't do file lock when opening a file, even if it's ran as another process, and running python as a child process in the test file yields race conditions (i.e., python does create the folder and the file to be locked but the js test fails because it can't do a rmSync because the file and the folder does not exist)

But I validated the reproducible steps mentioned in the issue with this fix, the rmSync now delays the retries and is around 15000ms, the same as the asynchronous fs.rm

[StefanStojanovic]

This change seems OK, but there should be a regression test to confirm that and keep us safe in the future.

[PickBas]

@louiellan You can lock the file with JS:

constfs=require('fs');constUV_FS_O_EXLOCK=0x10000000;fs.openSync(process.argv[1],fs.constants.O_RDWR|UV_FS_O_EXLOCK);process.stdout.write('locked');setInterval(()=>{},60_000);

This test should cover your changes:

'use strict';constcommon=require('../common');if(!common.isWindows)common.skip('Windows-specific: EPERM sharing-violation retry in rmSync');consttmpdir=require('../common/tmpdir');constassert=require('assert');const{ once }=require('events');const{ spawn }=require('child_process');constfs=require('fs');constpath=require('path');tmpdir.refresh();// UV_FS_O_EXLOCK opens with share mode 0, so deletion fails with EPERM// until this process kills the child.constlockerScript=` const fs = require('fs'); const UV_FS_O_EXLOCK = 0x10000000; fs.openSync(process.argv[1], fs.constants.O_RDWR | UV_FS_O_EXLOCK); process.stdout.write('locked'); setInterval(() => {}, 60_000);`;asyncfunctionspawnLocker(file){constchild=spawn(process.execPath,['-e',lockerScript,file],{stdio: ['ignore','pipe','inherit']});const[data]=awaitonce(child.stdout,'data');assert.strictEqual(data.toString(),'locked');returnchild;}// Sleep before retry i is i * retryDelay ms, so all retries take at least// retryDelay * (1 + 2 + ... + maxRetries) ms.functionminRetryTime({ maxRetries, retryDelay }){returnretryDelay*maxRetries*(maxRetries+1)/2;}functiontimedRmThrowsEPERM(dir,options){conststart=Date.now();assert.throws(()=>{fs.rmSync(dir,{recursive: true, ...options});},{code: 'EPERM',name: 'Error',syscall: 'rm',});returnDate.now()-start;}(async()=>{constdir=tmpdir.resolve('rm-eperm-retries');constfile=path.join(dir,'locked.txt');fs.mkdirSync(dir);fs.writeFileSync(file,'hello');constchild=awaitspawnLocker(file);try{// Proves the lock is effective: no retries means an immediate EPERM.timedRmThrowsEPERM(dir,{maxRetries: 0,retryDelay: 0});assert.strictEqual(fs.existsSync(file),true);constoptions={maxRetries: 4,retryDelay: 100};constexpected=minRetryTime(options);// 100+200+300+400 = 1000 ms.constelapsed=timedRmThrowsEPERM(dir,options);// Windows timer granularity may shave a few ms off each Sleep() call.constslack=16*options.maxRetries;assert.ok(elapsed>=expected-slack,`rmSync() gave up after ${elapsed}ms; expected it to spend at `+`least ~${expected}ms on ${options.maxRetries} retries of `+`${options.retryDelay}ms escalating delay`);// Catches unit confusion (e.g. seconds vs. milliseconds) in the delay.assert.ok(elapsed<common.platformTimeout(expected*10),`rmSync() gave up after ${elapsed}ms; expected roughly `+`${expected}ms for ${options.maxRetries} retries`);}finally{child.kill();}awaitonce(child,'exit');assert.strictEqual(fs.existsSync(file),true);})().then(common.mustCall());

[louiellan]

Thanks for the help❤️ @PickBas hadn't had the time for this yet