Skip to content

Commit 2ec1bbb

Browse files
satyakighsatyakigh
authored
Handle DNE files more consistently (#553)
1 parent 4a78abb commit 2ec1bbb

6 files changed

Lines changed: 122 additions & 70 deletions

File tree

‎src/schema/GetSchemaTask.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ export class GetPublicSchemaTask extends GetSchemaTask {
2727
@Telemetry()
2828
private readonly telemetry!: ScopedTelemetry;
2929

30-
static readonly MaxAttempts = 3;
30+
static readonly MaxAttempts = 5;
3131
private attempts = 0;
3232

3333
constructor(

‎src/utils/File.ts‎

Lines changed: 30 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
import { readFileSync, existsSync, PathLike } from 'fs'; // eslint-disable-line no-restricted-imports
1+
import { readFileSync, PathLike } from 'fs'; // eslint-disable-line no-restricted-imports
22
import { readFile, rename, unlink, open } from 'fs/promises'; // eslint-disable-line no-restricted-imports
33
import { LoggerFactory } from '../telemetry/LoggerFactory';
44
import { isWindows } from './Environment';
55
import { DoesNotExist } from './Errors';
6-
import { sleep } from './Retry';
6+
import { calculateDelay, sleep } from './Retry';
77
import { toString } from './String';
88

99
const log = LoggerFactory.getLogger('File');
@@ -15,29 +15,31 @@ type Options =
1515
flag?: string | undefined;
1616
};
1717

18+
const ENOENT = 'ENOENT'; // No such file or directory
19+
const RETRIABLE_RENAME_CODES = new Set(['EPERM', 'EACCES', 'EBUSY']);
20+
21+
function wrapReadEnoentError(path: PathLike, err: unknown): never {
22+
log.error(err);
23+
if (isFileNotFoundError(err)) {
24+
throw new DoesNotExist(toString(path));
25+
}
26+
27+
throw err;
28+
}
29+
1830
export function readFileIfExists(path: PathLike, options: Options = 'utf8'): string {
1931
try {
20-
if (existsSync(path)) {
21-
return readFileSync(path, options);
22-
} else {
23-
throw new DoesNotExist(toString(path));
24-
}
32+
return readFileSync(path, options);
2533
} catch (err) {
26-
log.error(err);
27-
throw err;
34+
wrapReadEnoentError(path, err);
2835
}
2936
}
3037

3138
export async function readFileIfExistsAsync(path: PathLike, options: Options = 'utf8'): Promise<string> {
3239
try {
33-
if (existsSync(path)) {
34-
return await readFile(path, options);
35-
} else {
36-
throw new DoesNotExist(toString(path));
37-
}
40+
return await readFile(path, options);
3841
} catch (err) {
39-
log.error(err);
40-
throw err;
42+
wrapReadEnoentError(path, err);
4143
}
4244
}
4345

@@ -49,43 +51,31 @@ export function readBufferIfExists(
4951
} | null,
5052
): Buffer {
5153
try {
52-
if (existsSync(path)) {
53-
return readFileSync(path, options);
54-
} else {
55-
throw new DoesNotExist(toString(path));
56-
}
54+
return readFileSync(path, options);
5755
} catch (err) {
58-
log.error(err);
59-
throw err;
56+
wrapReadEnoentError(path, err);
6057
}
6158
}
6259

63-
export function readBufferIfExistsAsync(
60+
export async function readBufferIfExistsAsync(
6461
path: PathLike,
6562
options?: {
6663
encoding?: null | undefined;
6764
flag?: string | undefined;
6865
} | null,
6966
): Promise<Buffer> {
7067
try {
71-
if (existsSync(path)) {
72-
return readFile(path, options);
73-
} else {
74-
throw new DoesNotExist(toString(path));
75-
}
68+
return await readFile(path, options);
7669
} catch (err) {
77-
log.error(err);
78-
throw err;
70+
wrapReadEnoentError(path, err);
7971
}
8072
}
8173

82-
const RETRIABLE_RENAME_CODES = new Set(['EPERM', 'EACCES', 'EBUSY', 'ENOENT']);
83-
8474
export async function asyncRenameWithRetry(
8575
sourcePath: string,
8676
destinationPath: string,
8777
maxRetries = 10,
88-
retryDelayMs = 50,
78+
initialDelayMs = 50,
8979
): Promise<void> {
9080
for (let attempt = 0; attempt < maxRetries; attempt++) {
9181
try {
@@ -95,14 +85,15 @@ export async function asyncRenameWithRetry(
9585
const code = (error as NodeJS.ErrnoException).code;
9686
if (!code || !RETRIABLE_RENAME_CODES.has(code) || attempt === maxRetries - 1) {
9787
try {
98-
await unlink(sourcePath);
88+
if (!isFileNotFoundError(error)) {
89+
await unlink(sourcePath);
90+
}
9991
} catch (err) {
100-
// best-effort tmp cleanup
101-
log.error(err);
92+
log.error(err, `Best effort tmp cleanup failed for ${sourcePath}`);
10293
}
10394
throw error;
10495
}
105-
await sleep(retryDelayMs);
96+
await sleep(calculateDelay(attempt, initialDelayMs));
10697
}
10798
}
10899
}
@@ -133,7 +124,5 @@ export async function asyncDirSync(dirPath: string): Promise<void> {
133124
}
134125

135126
export function isFileNotFoundError(error: unknown): boolean {
136-
// File was deleted by another process (e.g. a concurrent IDE session sharing the same storage directory)
137-
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-explicit-any
138-
return (error as any).code === 'ENOENT';
127+
return error !== null && typeof error === 'object' && (error as NodeJS.ErrnoException).code === ENOENT;
139128
}

‎src/utils/LocalFile.ts‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { existsSync, statSync, unlinkSync } from 'fs';
2-
import { unlink, writeFile, readdir } from 'fs/promises';
2+
import { unlink, writeFile, readdir, stat } from 'fs/promises';
33
import { Stats } from 'node:fs';
44
import { Stream } from 'node:stream';
55
import { basename, dirname, join } from 'path';
@@ -31,7 +31,33 @@ const LOCK_OPTIONS: LockOptions = {
3131
randomize: true, // Add jitter to retry delays to avoid contention between concurrent writes
3232
},
3333
};
34-
34+
const STALE_TMP_FILE_THRESHOLD_MS = 30 * 60 * 1000;
35+
36+
/**
37+
* Cross-platform local file with atomic writes and lockless reads.
38+
*
39+
* ## Write path
40+
* Writes use a write-to-temp → fsync → rename pattern under a `proper-lockfile` advisory lock,
41+
* ensuring that the target file is always in a consistent state. The temp file is created in the
42+
* same directory as the target so the rename is always a same-volume operation.
43+
*
44+
* ## Read path — lockless by design
45+
* Reads do not acquire a lock. This is safe because `fs.rename` (libuv `MoveFileExW` on Windows,
46+
* `rename(2)` on POSIX) is an atomic replace on same-volume NTFS and all POSIX filesystems.
47+
* A concurrent reader will see either the old or the new content, never a partial or corrupt file.
48+
*
49+
* ### Risk profile
50+
* | Scenario | POSIX | Windows (NTFS) |
51+
* |-------------------------------------------|--------------|-----------------------------------------|
52+
* | Reader during rename | Old or new | Old or new (atomic via `MoveFileExW`) |
53+
* | Reader between delete+create (non-NTFS) | N/A | Transient `ENOENT` → returns `undefined`|
54+
* | Partial / corrupt read | Not possible | Not possible (rename is atomic) |
55+
*
56+
* The only scenario where a reader could observe a missing file is on a non-NTFS Windows filesystem
57+
* (e.g. FAT32, network share) where rename may not be atomic. Even then, the failure mode is
58+
* graceful: `readString` / `readBytes` return `undefined`, and all callers treat that as a cache
59+
* miss (e.g. falling back to defaults or re-fetching).
60+
*/
3561
export class LocalFile {
3662
private tempFileCounter = 0;
3763
readonly fileName: string;
@@ -40,6 +66,7 @@ export class LocalFile {
4066
constructor(readonly path: string) {
4167
this.fileName = basename(path);
4268
this.dirName = dirname(path);
69+
this.scheduleCleanup();
4370
}
4471

4572
exists() {
@@ -119,7 +146,7 @@ export class LocalFile {
119146
): Promise<boolean> {
120147
const release = await this.tryLock();
121148
try {
122-
await this.cleanupStaleTmpFiles();
149+
this.scheduleCleanup(); // Defer tmp file cleanup, so we don't block the main write operation
123150
const tmp = this.tmpPath();
124151

125152
try {
@@ -188,6 +215,12 @@ export class LocalFile {
188215
return `${this.path}.${processId()}.${this.tempFileCounter}.tmp`;
189216
}
190217

218+
private scheduleCleanup() {
219+
setTimeout(() => {
220+
this.cleanupStaleTmpFiles().catch(log.error);
221+
}, 60 * 1000).unref();
222+
}
223+
191224
private async cleanupStaleTmpFiles() {
192225
try {
193226
const entries = await readdir(this.dirName);
@@ -196,7 +229,12 @@ export class LocalFile {
196229
const fullPath = join(this.dirName, entry);
197230

198231
try {
199-
await unlink(fullPath);
232+
const fileStat = await stat(fullPath);
233+
// Only delete the file if it is significantly older than the current time.
234+
// This prevents process B from deleting process A's active temp file when stealing a lock.
235+
if (Date.now() - fileStat.mtimeMs > STALE_TMP_FILE_THRESHOLD_MS) {
236+
await unlink(fullPath);
237+
}
200238
} catch (err) {
201239
if (isFileNotFoundError(err)) {
202240
continue;

‎src/utils/Retry.ts‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,20 @@ export function sleep(ms: number): Promise<void> {
1717
});
1818
}
1919

20-
function calculateDelay(
20+
const DefaultRetryOptions = {
21+
maxRetries: 3,
22+
initialDelayMs: 1000,
23+
maxDelayMs: 5000,
24+
backoffMultiplier: 2,
25+
jitterFactor: 0.1,
26+
};
27+
28+
export function calculateDelay(
2129
attempt: number,
22-
initialDelayMs: number,
23-
jitterFactor: number,
24-
backoffMultiplier: number,
25-
maxDelayMs: number,
30+
initialDelayMs: number = DefaultRetryOptions.initialDelayMs,
31+
jitterFactor: number = DefaultRetryOptions.jitterFactor,
32+
backoffMultiplier: number = DefaultRetryOptions.backoffMultiplier,
33+
maxDelayMs: number = DefaultRetryOptions.maxDelayMs,
2634
): number {
2735
// 1. Exponential Backoff: initial * 2^0, initial * 2^1, etc.
2836
const exponentialDelay = initialDelayMs * Math.pow(backoffMultiplier, attempt);
@@ -45,11 +53,11 @@ export async function retryWithExponentialBackoff<T>(
4553
},
4654
): Promise<T> {
4755
const {
48-
maxRetries = 3,
49-
initialDelayMs = 1000,
50-
maxDelayMs = 5000,
51-
backoffMultiplier = 2,
52-
jitterFactor = 0.1,
56+
maxRetries = DefaultRetryOptions.maxRetries,
57+
initialDelayMs = DefaultRetryOptions.initialDelayMs,
58+
maxDelayMs = DefaultRetryOptions.maxDelayMs,
59+
backoffMultiplier = DefaultRetryOptions.backoffMultiplier,
60+
jitterFactor = DefaultRetryOptions.jitterFactor,
5361
operationName,
5462
totalTimeoutMs,
5563
} = options;

‎tst/unit/utils/File.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,8 @@ describe('File', () => {
9292
expect(Buffer.compare(result, binaryContent)).toBe(0);
9393
});
9494

95-
it('should throw DoesNotExist for nonexistent path', () => {
96-
// readBufferIfExistsAsync throws synchronously from existsSync check
97-
expect(() => readBufferIfExistsAsync(nonexistentPath)).toThrow('does not exist');
95+
it('should throw DoesNotExist for nonexistent path', async () => {
96+
await expect(readBufferIfExistsAsync(nonexistentPath)).rejects.toThrow('does not exist');
9897
});
9998
});
10099

‎tst/unit/utils/LocalFile.test.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
import { randomUUID as v4 } from 'crypto';
2-
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync, readdirSync, unlinkSync } from 'fs';
2+
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync, readdirSync, unlinkSync, utimesSync } from 'fs';
33
import { join } from 'path';
4-
import { describe, it, expect, beforeAll, afterEach, beforeEach } from 'vitest';
4+
import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest';
55
import { LocalFile } from '../../../src/utils/LocalFile';
6+
import { waitFor } from '../../utils/Utils';
67

78
describe('LocalFile', () => {
89
const testDir = join(process.cwd(), 'node_modules', '.cache', 'localfile-tests', v4());
910
const filePath = join(testDir, 'test-file.txt');
1011

11-
beforeAll(() => {
12+
beforeEach(() => {
13+
rmSync(testDir, { recursive: true, force: true });
1214
mkdirSync(testDir, { recursive: true });
1315
});
1416

@@ -111,16 +113,32 @@ describe('LocalFile', () => {
111113
expect(readFileSync(tmpFile).equals(Buffer.from(data))).toBe(true);
112114
});
113115

114-
it('should clean up stale tmp files before writing', async () => {
115-
writeFileSync(filePath, 'original');
116-
const staleTmp = `${filePath}.99999.1.tmp`;
117-
writeFileSync(staleTmp, 'stale');
116+
it('should clean up stale tmp files after writing', async () => {
117+
vi.useFakeTimers({ shouldAdvanceTime: true });
118+
try {
119+
writeFileSync(filePath, 'original');
120+
const staleTmp = `${filePath}.99999.1.tmp`;
121+
writeFileSync(staleTmp, 'stale');
118122

119-
const localFile = new LocalFile(filePath);
120-
await localFile.write('new-content');
123+
// Backdate the tmp file so it exceeds the 30-minute staleness threshold
124+
const oldTime = new Date(Date.now() - 31 * 60 * 1000);
125+
utimesSync(staleTmp, oldTime, oldTime);
121126

122-
expect(existsSync(staleTmp)).toBe(false);
123-
expect(readFileSync(filePath, 'utf8')).toBe('new-content');
127+
const localFile = new LocalFile(filePath);
128+
await localFile.write('new-content');
129+
130+
// Advance past the 60s setTimeout to trigger deferred cleanup
131+
await vi.advanceTimersByTimeAsync(60 * 1000);
132+
// Allow the async cleanup to complete
133+
await vi.advanceTimersByTimeAsync(0);
134+
135+
await waitFor(() => {
136+
expect(existsSync(staleTmp)).toBe(false);
137+
expect(readFileSync(filePath, 'utf8')).toBe('new-content');
138+
});
139+
} finally {
140+
vi.useRealTimers();
141+
}
124142
});
125143

126144
it('should not leave tmp files after successful write', async () => {

0 commit comments

Comments
 (0)