Skip to content

Commit f24b691

Browse files
committed
fix: handle request errors when fetching CODEOWNERS
getCodeOwnersFile() listened for an 'err' event on the https request, which ClientRequest never emits; failures are emitted as 'error'. Any network-level failure (DNS, ECONNRESET, socket hang up, TLS) therefore had no listener, was thrown as an uncaught exception and terminated the bot through the uncaughtException handler in server.js, instead of rejecting the promise and letting the surrounding Aigle.retry() retry. Listen for 'error' on the request, also reject on errors emitted by the response stream (e.g. a connection dropped mid-body), and drain the body of non-200 responses so the socket is released. Add a unit test that makes the request fail at the network level and expects getCodeOwnersFile() to reject; it fails with an uncaught exception without this change. Fixes: #705 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
1 parent 2723715 commit f24b691

2 files changed

Lines changed: 29 additions & 1 deletion

File tree

‎lib/node-repo.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,8 @@ function getCodeOwnersFile (url, { logger }) {
117117
return new Promise((resolve, reject) => {
118118
https.get(url, (res) => {
119119
if (res.statusCode !== 200) {
120+
// discard the response body so the socket is released
121+
res.resume()
120122
logger.error(`Status code: ${res.statusCode}`, 'Error retrieving OWNERS')
121123
return reject(res.statusCode)
122124
}
@@ -127,7 +129,11 @@ function getCodeOwnersFile (url, { logger }) {
127129
res.on('end', () => {
128130
return resolve(body)
129131
})
130-
}).on('err', (err) => {
132+
res.on('error', (err) => {
133+
logger.error(err, 'Error retrieving OWNERS')
134+
return reject(err)
135+
})
136+
}).on('error', (err) => {
131137
logger.error(err, 'Error retrieving OWNERS')
132138
return reject(err)
133139
})

‎test/unit/node-repo-owners.test.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,28 @@ test('getCodeOwnersFile fail', async (t) => {
169169
scope.done()
170170
})
171171

172+
test('getCodeOwnersFile network error', async (t) => {
173+
const options = {
174+
owner: 'nodejs',
175+
repo: 'node-auto-test-6',
176+
prId: 12347,
177+
logger: { info: () => {}, error: () => {}, debug: () => {}, child: function () { return this } },
178+
retries: 1,
179+
defaultBranch: 'main',
180+
retryInterval: 10
181+
}
182+
183+
const base = 'https://localhost'
184+
const filePath = '/CODEOWNERS'
185+
const url = `${base}${filePath}`
186+
const scope = nock(base)
187+
.get(filePath)
188+
.replyWithError('socket hang up')
189+
190+
await t.assert.rejects(getCodeOwnersFile(url, options), { message: 'socket hang up' })
191+
scope.done()
192+
})
193+
172194
test('pingOwners success', async (t) => {
173195
const options = {
174196
owner: 'nodejs',

0 commit comments

Comments
 (0)