Skip to content

Commit 62aa6d1

Browse files
jdaltonjdalton
committed
fix(dlx-binary): add comprehensive error handling for binary downloads
Improve error messages for binary download and caching failures: - httpDownload failures: Include URL and destination with connectivity guidance - EACCES/EPERM: Permission denied with directory path and fix suggestions - EROFS: Read-only filesystem with SOCKET_DLX_DIR configuration hint Applied to both dlxBinary() and downloadBinary() functions for consistency. Also deprecates download-lock.ts in favor of process-lock.ts's superior directory-based locking mechanism.
1 parent 049af23 commit 62aa6d1

2 files changed

Lines changed: 125 additions & 38 deletions

File tree

‎src/dlx-binary.ts‎

Lines changed: 109 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ import path from 'node:path'
88
import { WIN32 } from '#constants/platform'
99

1010
import { generateCacheKey } from './dlx'
11-
import { downloadWithLock } from './download-lock'
11+
import { httpDownload } from './http-request'
1212
import { isDir, readJson, safeDelete } from './fs'
1313
import { isObjectObject } from './objects'
1414
import { normalizePath } from './path'
1515
import { getSocketDlxDir } from './paths'
16+
import { processLock } from './process-lock'
1617
import type { SpawnExtra, SpawnOptions } from './spawn'
1718
import { spawn } from './spawn'
1819

@@ -80,44 +81,74 @@ async function isCacheValid(
8081

8182
/**
8283
* Download a file from a URL with integrity checking and concurrent download protection.
83-
* Uses downloadWithLock to prevent multiple processes from downloading the same binary simultaneously.
84+
* Uses processLock to prevent multiple processes from downloading the same binary simultaneously.
8485
* Internal helper function for downloading binary files.
8586
*/
8687
async function downloadBinaryFile(
8788
url: string,
8889
destPath: string,
8990
checksum?: string | undefined,
9091
): Promise<string> {
91-
// Use downloadWithLock to handle concurrent download protection.
92-
// This prevents corruption when multiple processes try to download the same binary.
93-
await downloadWithLock(url, destPath, {
94-
// Align with npm's npx locking strategy.
95-
staleTimeout: 10_000,
96-
// Allow up to 2 minutes for large binary downloads.
97-
lockTimeout: 120_000,
98-
})
99-
100-
// Compute checksum of downloaded file.
101-
const fileBuffer = await fs.readFile(destPath)
102-
const hasher = createHash('sha256')
103-
hasher.update(fileBuffer)
104-
const actualChecksum = hasher.digest('hex')
105-
106-
// Verify checksum if provided.
107-
if (checksum && actualChecksum !== checksum) {
108-
// Clean up invalid file.
109-
await safeDelete(destPath)
110-
throw new Error(
111-
`Checksum mismatch: expected ${checksum}, got ${actualChecksum}`,
112-
)
113-
}
92+
// Use process lock to prevent concurrent downloads.
93+
// Lock is placed in the cache entry directory as 'concurrency.lock'.
94+
const cacheEntryDir = path.dirname(destPath)
95+
const lockPath = path.join(cacheEntryDir, 'concurrency.lock')
96+
97+
return await processLock.withLock(
98+
lockPath,
99+
async () => {
100+
// Check if file was downloaded while waiting for lock.
101+
if (existsSync(destPath)) {
102+
const stats = await fs.stat(destPath)
103+
if (stats.size > 0) {
104+
// File exists, compute and return checksum.
105+
const fileBuffer = await fs.readFile(destPath)
106+
const hasher = createHash('sha256')
107+
hasher.update(fileBuffer)
108+
return hasher.digest('hex')
109+
}
110+
}
114111

115-
// Make executable on POSIX systems.
116-
if (!WIN32) {
117-
await fs.chmod(destPath, 0o755)
118-
}
112+
// Download the file.
113+
try {
114+
await httpDownload(url, destPath)
115+
} catch (e) {
116+
throw new Error(
117+
`Failed to download binary from ${url}\n` +
118+
`Destination: ${destPath}\n` +
119+
'Check your internet connection or verify the URL is accessible.',
120+
{ cause: e },
121+
)
122+
}
123+
124+
// Compute checksum of downloaded file.
125+
const fileBuffer = await fs.readFile(destPath)
126+
const hasher = createHash('sha256')
127+
hasher.update(fileBuffer)
128+
const actualChecksum = hasher.digest('hex')
129+
130+
// Verify checksum if provided.
131+
if (checksum && actualChecksum !== checksum) {
132+
// Clean up invalid file.
133+
await safeDelete(destPath)
134+
throw new Error(
135+
`Checksum mismatch: expected ${checksum}, got ${actualChecksum}`,
136+
)
137+
}
138+
139+
// Make executable on POSIX systems.
140+
if (!WIN32) {
141+
await fs.chmod(destPath, 0o755)
142+
}
119143

120-
return actualChecksum
144+
return actualChecksum
145+
},
146+
{
147+
// Align with npm npx locking strategy.
148+
staleMs: 5000,
149+
touchIntervalMs: 2000,
150+
},
151+
)
121152
}
122153

123154
/**
@@ -267,8 +298,30 @@ export async function dlxBinary(
267298
}
268299

269300
if (downloaded) {
270-
// Ensure cache directory exists.
271-
await fs.mkdir(cacheEntryDir, { recursive: true })
301+
// Ensure cache directory exists before downloading.
302+
try {
303+
await fs.mkdir(cacheEntryDir, { recursive: true })
304+
} catch (e) {
305+
const code = (e as NodeJS.ErrnoException).code
306+
if (code === 'EACCES' || code === 'EPERM') {
307+
throw new Error(
308+
`Permission denied creating binary cache directory: ${cacheEntryDir}\n` +
309+
'Please check directory permissions or run with appropriate access.',
310+
{ cause: e },
311+
)
312+
}
313+
if (code === 'EROFS') {
314+
throw new Error(
315+
`Cannot create binary cache directory on read-only filesystem: ${cacheEntryDir}\n` +
316+
'Ensure the filesystem is writable or set SOCKET_DLX_DIR to a writable location.',
317+
{ cause: e },
318+
)
319+
}
320+
throw new Error(
321+
`Failed to create binary cache directory: ${cacheEntryDir}`,
322+
{ cause: e },
323+
)
324+
}
272325

273326
// Download the binary.
274327
computedChecksum = await downloadBinaryFile(url, binaryPath, checksum)
@@ -351,8 +404,30 @@ export async function downloadBinary(
351404
// Binary is cached and valid.
352405
downloaded = false
353406
} else {
354-
// Ensure cache directory exists.
355-
await fs.mkdir(cacheEntryDir, { recursive: true })
407+
// Ensure cache directory exists before downloading.
408+
try {
409+
await fs.mkdir(cacheEntryDir, { recursive: true })
410+
} catch (e) {
411+
const code = (e as NodeJS.ErrnoException).code
412+
if (code === 'EACCES' || code === 'EPERM') {
413+
throw new Error(
414+
`Permission denied creating binary cache directory: ${cacheEntryDir}\n` +
415+
'Please check directory permissions or run with appropriate access.',
416+
{ cause: e },
417+
)
418+
}
419+
if (code === 'EROFS') {
420+
throw new Error(
421+
`Cannot create binary cache directory on read-only filesystem: ${cacheEntryDir}\n` +
422+
'Ensure the filesystem is writable or set SOCKET_DLX_DIR to a writable location.',
423+
{ cause: e },
424+
)
425+
}
426+
throw new Error(
427+
`Failed to create binary cache directory: ${cacheEntryDir}`,
428+
{ cause: e },
429+
)
430+
}
356431

357432
// Download the binary.
358433
const computedChecksum = await downloadBinaryFile(url, binaryPath, checksum)

‎src/download-lock.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
1-
/** @fileoverview Download locking utilities to prevent concurrent downloads of the same resource. Uses file-based locking for cross-process synchronization. */
1+
/**
2+
* @fileoverview Download locking utilities to prevent concurrent downloads of the same resource. Uses file-based locking for cross-process synchronization.
3+
* @deprecated This module is deprecated in favor of `process-lock.ts` which uses
4+
* directory-based locking (via `mkdir()`) that's more reliable across all filesystems
5+
* including NFS. See process-lock.ts for detailed explanation of why directory-based
6+
* locking is superior to file-based locking.
7+
*
8+
* Migration: Use `processLock.withLock()` from `./process-lock` instead of
9+
* `downloadWithLock()` from this module.
10+
*/
211

312
import { existsSync } from 'node:fs'
413
import { mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'
@@ -38,11 +47,14 @@ export interface DownloadWithLockOptions extends HttpDownloadOptions {
3847

3948
/**
4049
* Get the path to the lock file for a destination path.
50+
* Uses npm npx's concurrency.lock naming convention.
51+
* For downloads, we use a subdirectory per destination to isolate locks.
4152
*/
4253
function getLockFilePath(destPath: string, locksDir?: string): string {
43-
const dir = locksDir || `${dirname(destPath)}/.locks`
44-
const filename = `${destPath.replace(/[^\w.-]/g, '_')}.lock`
45-
return join(dir, filename)
54+
// Create a unique subdirectory for each destination using sanitized path.
55+
const sanitized = destPath.replace(/[^\w.-]/g, '_')
56+
const dir = locksDir || join(dirname(destPath), '.locks', sanitized)
57+
return join(dir, 'concurrency.lock')
4658
}
4759

4860
/**

0 commit comments

Comments
 (0)