Skip to content

Commit 683c1e5

Browse files
committed
Fix critical issues in socket-sdk-js
- Fix silent exception swallowing in NDJSON parsing - Fix unsafe file path resolution using __dirname - Add error handling for stream operations - Fix race condition in generator cleanup - Add response validation before access - Improve error context in retry logic
1 parent 52e912e commit 683c1e5

File tree

4 files changed

+62
-21
lines changed

4 files changed

+62
-21
lines changed

src/http-client.ts

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -337,27 +337,42 @@ export async function withRetry<T>(
337337
} catch (error) {
338338
lastError = error as Error
339339

340-
// Last attempt - throw error
340+
// Last attempt - throw error with retry context.
341341
if (attempt === retries) {
342-
break
342+
const enhancedError = new Error(
343+
`Request failed after ${retries + 1} attempts`,
344+
{ cause: lastError },
345+
)
346+
throw enhancedError
343347
}
344348

345-
// Check if error is retryable (network errors, 5xx responses)
349+
// Check if error is retryable (network errors, 5xx responses).
346350
if (error instanceof ResponseError) {
347351
const status = error.response.statusCode
348-
// Don't retry client errors (4xx)
352+
// Don't retry client errors (4xx).
349353
if (status && status >= 400 && status < 500) {
350354
throw error
351355
}
356+
debugLog(
357+
'withRetry',
358+
`Retrying after ${status} error (attempt ${attempt + 1}/${retries + 1})`,
359+
)
360+
} else {
361+
debugLog(
362+
'withRetry',
363+
`Retrying after network error (attempt ${attempt + 1}/${retries + 1})`,
364+
)
352365
}
353366

354-
// Exponential backoff
367+
// Exponential backoff.
355368
const delayMs = retryDelay * 2 ** attempt
369+
debugLog('withRetry', `Waiting ${delayMs}ms before retry`)
356370
// eslint-disable-next-line no-await-in-loop
357371
await new Promise(resolve => setTimeout(resolve, delayMs))
358372
}
359373
}
360374

375+
// Fallback error if lastError is somehow undefined.
361376
throw lastError || new Error('Request failed after retries')
362377
}
363378

src/quota-utils.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,11 @@ function loadRequirements(): Requirements {
2525
}
2626

2727
try {
28-
// Resolve path relative to current working directory
29-
const requirementsPath = join(process.cwd(), 'requirements.json')
28+
// Resolve path relative to this module file location.
29+
// When compiled, __dirname will point to dist/ directory.
30+
// In source, __dirname points to src/ directory.
31+
// requirements.json is always in the parent directory of dist/ or src/.
32+
const requirementsPath = join(__dirname, '..', 'requirements.json')
3033
const data = readFileSync(requirementsPath, 'utf8')
3134
requirements = JSON.parse(data) as Requirements
3235
return requirements

src/socket-sdk-class.ts

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,13 @@ export class SocketSdk {
143143
yield await this.#handleApiError<'batchPackageFetch'>(e)
144144
return
145145
}
146+
// Validate response before processing.
147+
if (!res) {
148+
throw new Error('Failed to get response from batch PURL request')
149+
}
146150
// Parse the newline delimited JSON response.
147151
const rli = readline.createInterface({
148-
input: res!,
152+
input: res,
149153
crlfDelay: Number.POSITIVE_INFINITY,
150154
signal: abortSignal,
151155
})
@@ -383,9 +387,13 @@ export class SocketSdk {
383387
} catch (e) {
384388
return await this.#handleApiError<'batchPackageFetch'>(e)
385389
}
390+
// Validate response before processing.
391+
if (!res) {
392+
throw new Error('Failed to get response from batch PURL request')
393+
}
386394
// Parse the newline delimited JSON response.
387395
const rli = readline.createInterface({
388-
input: res!,
396+
input: res,
389397
crlfDelay: Number.POSITIVE_INFINITY,
390398
signal: abortSignal,
391399
})
@@ -503,12 +511,13 @@ export class SocketSdk {
503511
const { generator, iteratorResult }: GeneratorStep = await Promise.race(
504512
running.map(entry => entry.promise),
505513
)
506-
// Remove generator.
507-
/* c8 ignore next 3 - Concurrent generator cleanup edge case. */
508-
running.splice(
509-
running.findIndex(entry => entry.generator === generator),
510-
1,
511-
)
514+
// Remove generator with safe index lookup.
515+
const index = running.findIndex(entry => entry.generator === generator)
516+
/* c8 ignore next 3 - Defensive check for concurrent generator cleanup edge case. */
517+
if (index === -1) {
518+
continue
519+
}
520+
running.splice(index, 1)
512521
// Yield the value if one is given, even when done:true.
513522
if (iteratorResult.value) {
514523
yield iteratorResult.value
@@ -1776,11 +1785,20 @@ export class SocketSdk {
17761785
}
17771786

17781787
if (typeof output === 'string') {
1779-
// Stream to file
1780-
res.pipe(createWriteStream(output))
1788+
// Stream to file with error handling.
1789+
const writeStream = createWriteStream(output)
1790+
res.pipe(writeStream)
1791+
writeStream.on('error', error => {
1792+
throw new Error(`Failed to write to file: ${output}`, {
1793+
cause: error,
1794+
})
1795+
})
17811796
} else if (output === true) {
1782-
// Stream to stdout
1797+
// Stream to stdout with error handling.
17831798
res.pipe(process.stdout)
1799+
process.stdout.on('error', error => {
1800+
throw new Error('Failed to write to stdout', { cause: error })
1801+
})
17841802
}
17851803

17861804
// If output is false or undefined, just return the response without streaming
@@ -1827,7 +1845,10 @@ export class SocketSdk {
18271845
try {
18281846
const data = JSON.parse(line) as ArtifactPatches
18291847
controller.enqueue(data)
1830-
} catch (e) {}
1848+
} catch (e) {
1849+
// Log parse errors for debugging invalid NDJSON lines.
1850+
debugLog('streamPatchesFromScan', `Failed to parse line: ${e}`)
1851+
}
18311852
}
18321853
})
18331854

test/http-client-retry.test.mts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ describe('HTTP Client - Retry Functionality', () => {
5555
throw new Error('Persistent error')
5656
})
5757

58-
await expect(withRetry(fn, 3, 10)).rejects.toThrow('Persistent error')
58+
await expect(withRetry(fn, 3, 10)).rejects.toThrow(
59+
'Request failed after 4 attempts',
60+
)
5961
// Initial + 3 retries
6062
expect(fn).toHaveBeenCalledTimes(4)
6163
})
@@ -141,7 +143,7 @@ describe('HTTP Client - Retry Functionality', () => {
141143
})
142144

143145
await expect(withRetry(fn, 0, 10)).rejects.toThrow(
144-
'Request failed after retries',
146+
'Request failed after 1 attempts',
145147
)
146148
})
147149
})

0 commit comments

Comments
 (0)