fix(bounded-retry): address PR #393 second-round review
All 5 issues verified as REAL (not false positives) and fixed:
1. **CRITICAL: query() swallows timeouts → bounded-retry treats as success**
(CodeRabbit Critical, line 319 socket.ts)
waitForMessage() returns undefined on timeout instead of throwing.
queryInternal() returns that undefined unchanged. uploadToServer
was `await query(...)` then logging success regardless — a timed-out
upload would update lastUploadTime with no retry triggered.
Fix: validate response shape inside uploadToServer:
if (!result) throw new Boom('Pre-key upload query timed out (no response)', { statusCode: 408 })
Bounded-retry now sees the throw and properly retries.
2. **MAJOR: Outer Promise.race + bounded-retry can run in parallel**
(CodeRabbit Major + Copilot, line 674 socket.ts)
With safety margin (ttlMs=28s vs UPLOAD_TIMEOUT=30s) bounded-retry
should always reach give-up first, but if anything bypasses that
bound the outer race rejects while bounded-retry keeps running.
Fix: wire AbortController. Pass its signal to bounded-retry.
On outer race timeout, abort the controller → bounded-retry stops
cleanly with BoundedRetryAbortedError. uploadToServer also honours
the signal before logging success.
3. **MINOR: sleep-leak test does not observe the leak**
(CodeRabbit Minor)
Test only triggered 4 sleeps — below Node's 11-listener warning —
and never captured warning events.
Fix: spy on signal.addEventListener / removeEventListener and
assert add count == remove count.
4. **MINOR: structured-logger thenable .catch can throw**
(Copilot)
Code called (hookResult as Promise).catch(...) directly. Fails on
thenables that only implement .then.
Fix: use Promise.resolve(hookResult).catch(...) which adapts any
thenable safely.
5. **Internal: bounded-retry catch was double-removing outer-abort listener**
Found by the new spy-based test. Both catch and finally called
removeEventListener. Removed redundant catch-block remove; finally
is single source of truth.
Build clean, 35/35 suites, 809/809 tests pass.
This commit is contained in:
@@ -497,8 +497,12 @@ export class StructuredLogger implements ILogger {
|
||||
if (externalHook) {
|
||||
try {
|
||||
const hookResult = externalHook(entry) as unknown
|
||||
if (hookResult && typeof (hookResult as Promise<unknown>).then === 'function') {
|
||||
;(hookResult as Promise<unknown>).catch(() => {
|
||||
// Use Promise.resolve().catch() so any thenable (including
|
||||
// Promise-likes that only implement .then) is handled
|
||||
// safely — calling .catch directly would throw on such
|
||||
// objects (Copilot review on PR #393).
|
||||
if (hookResult && typeof (hookResult as PromiseLike<unknown>).then === 'function') {
|
||||
void Promise.resolve(hookResult as PromiseLike<unknown>).catch(() => {
|
||||
this.metrics.hookFailures++
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user