From 6ca609e20b68fb2e5ef8177db116b84a339461fd Mon Sep 17 00:00:00 2001 From: milaninfy <111582375+milaninfy@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:20:00 -0400 Subject: [PATCH] fix: ping and doctor commands fix for checking if registry is online (#7789) Ping: Don't use cache so ping does not report ping sucess incorrectly if it's offline or no internet Doctor: Don't use cache for pinging the registry. Fixes: https://github.com/npm/cli/issues/5870 Fixes: https://github.com/npm/cli/issues/3576 Fixes: https://github.com/npm/cli/issues/4112
Testing of ping and doctor ```sh # -- current npm last ping resuts in cached request replying PONG ~/workarea/npm-cli $ npm ping --registry=http://localhost:4873 -ddd npm verbose cli /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/node /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/npm npm info using npm@10.8.3 npm info using node@v22.9.0 npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/lib/node_modules/npm/npmrc npm silly config load:file:/Users/milaninfy/workarea/npm-cli/.npmrc npm silly config load:file:/Users/milaninfy/.npmrc npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/etc/npmrc npm verbose title npm ping npm verbose argv "ping" "--registry" "http://localhost:4873" "--loglevel" "silly" npm verbose logfile logs-max:10 dir:/Users/milaninfy/.npm/_logs/2024-09-26T20_37_04_583Z- npm verbose logfile /Users/milaninfy/.npm/_logs/2024-09-26T20_37_04_583Z-debug-0.log npm notice PING http://localhost:4873/ npm silly logfile start cleaning logs, removing 1 files npm silly logfile done cleaning log files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 1 failed with ECONNREFUSED npm http fetch GET http://localhost:4873/-/ping?write=true attempt 2 failed with ECONNREFUSED npm http fetch GET http://localhost:4873/-/ping?write=true attempt 3 failed with ECONNREFUSED npm http fetch GET 200 http://localhost:4873/-/ping?write=true 70045ms (cache stale) npm notice PONG 70046ms npm verbose cwd /Users/milaninfy/workarea/npm-cli npm verbose os Darwin 23.6.0 npm verbose node v22.9.0 npm verbose npm v10.8.3 npm verbose exit 0 npm info ok # -- After the change npm last ping resuts in failure after retries ~/workarea/npm-cli $ lnpm ping --registry=http://localhost:4873 -ddd npm verbose cli /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/node /Users/milaninfy/workarea/npm-cli/index.js npm info using npm@10.8.3 npm info using node@v22.9.0 npm silly config load:file:/Users/milaninfy/workarea/npm-cli/npmrc npm silly config load:file:/Users/milaninfy/workarea/npm-cli/.npmrc npm silly config load:file:/Users/milaninfy/.npmrc npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/etc/npmrc npm verbose title npm ping npm verbose argv "ping" "--registry" "http://localhost:4873" "--loglevel" "silly" npm verbose logfile logs-max:10 dir:/Users/milaninfy/.npm/_logs/2024-09-26T20_38_51_059Z- npm verbose logfile /Users/milaninfy/.npm/_logs/2024-09-26T20_38_51_059Z-debug-0.log npm notice PING http://localhost:4873/ npm silly logfile start cleaning logs, removing 1 files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 1 failed with ECONNREFUSED npm silly logfile done cleaning log files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 2 failed with ECONNREFUSED npm http fetch GET http://localhost:4873/-/ping?write=true attempt 3 failed with ECONNREFUSED npm verbose type system npm verbose stack FetchError: request to http://localhost:4873/-/ping?write=true failed, reason: npm verbose stack at ClientRequest. (/Users/milaninfy/workarea/npm-cli/node_modules/minipass-fetch/lib/index.js:130:14) npm verbose stack at ClientRequest.emit (node:events:519:28) npm verbose stack at emitErrorEvent (node:_http_client:103:11) npm verbose stack at _destroy (node:_http_client:886:9) npm verbose stack at onSocketNT (node:_http_client:906:5) npm verbose stack at process.processTicksAndRejections (node:internal/process/task_queues:91:21) npm error code ECONNREFUSED npm error errno ECONNREFUSED npm error FetchError: request to http://localhost:4873/-/ping?write=true failed, reason: npm error at ClientRequest. (/Users/milaninfy/workarea/npm-cli/node_modules/minipass-fetch/lib/index.js:130:14) npm error at ClientRequest.emit (node:events:519:28) npm error at emitErrorEvent (node:_http_client:103:11) npm error at _destroy (node:_http_client:886:9) npm error at onSocketNT (node:_http_client:906:5) npm error at process.processTicksAndRejections (node:internal/process/task_queues:91:21) { npm error code: 'ECONNREFUSED', npm error errno: 'ECONNREFUSED', npm error type: 'system' npm error } npm error npm error If you are behind a proxy, please make sure that the npm error 'proxy' config is set properly. See: 'npm help config' npm verbose cwd /Users/milaninfy/workarea/npm-cli npm verbose os Darwin 23.6.0 npm verbose node v22.9.0 npm verbose npm v10.8.3 npm verbose exit 1 npm verbose code 1 npm error A complete log of this run can be found in: /Users/milaninfy/.npm/_logs/2024-09-26T20_38_51_059Z-debug-0.log # -- npm doctor ping resuts in success due to cache hit ~/workarea/npm-cli $ npm doctor --registry=http://localhost:4873 -ddd npm verbose cli /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/node /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/npm npm info using npm@10.8.3 npm info using node@v22.9.0 npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/lib/node_modules/npm/npmrc npm silly config load:file:/Users/milaninfy/workarea/npm-cli/.npmrc npm silly config load:file:/Users/milaninfy/.npmrc npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/etc/npmrc npm verbose title npm doctor npm verbose argv "doctor" "--registry" "http://localhost:4873" "--loglevel" "silly" npm verbose logfile logs-max:10 dir:/Users/milaninfy/.npm/_logs/2024-09-26T20_40_30_672Z- npm verbose logfile /Users/milaninfy/.npm/_logs/2024-09-26T20_40_30_672Z-debug-0.log npm info doctor Running checkup Connecting to the registry npm info doctor Pinging registry npm silly logfile start cleaning logs, removing 1 files npm silly logfile done cleaning log files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 1 failed with ECONNREFUSED npm http fetch GET 200 http://localhost:4873/-/ping?write=true 48ms (cache stale) Ok # -- after the changes npm doctor ping correctly resuts in failure after retires ~/workarea/npm-cli $ lnpm doctor --registry=http://localhost:4873 -ddd npm verbose cli /Users/milaninfy/.nvm/versions/node/v22.9.0/bin/node /Users/milaninfy/workarea/npm-cli/index.js npm info using npm@10.8.3 npm info using node@v22.9.0 npm silly config load:file:/Users/milaninfy/workarea/npm-cli/npmrc npm silly config load:file:/Users/milaninfy/workarea/npm-cli/.npmrc npm silly config load:file:/Users/milaninfy/.npmrc npm silly config load:file:/Users/milaninfy/.nvm/versions/node/v22.9.0/etc/npmrc npm verbose title npm doctor npm verbose argv "doctor" "--registry" "http://localhost:4873" "--loglevel" "silly" npm verbose logfile logs-max:10 dir:/Users/milaninfy/.npm/_logs/2024-09-26T20_41_05_904Z- npm verbose logfile /Users/milaninfy/.npm/_logs/2024-09-26T20_41_05_904Z-debug-0.log npm info doctor Running checkup Connecting to the registry npm info doctor Pinging registry npm silly logfile start cleaning logs, removing 1 files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 1 failed with ECONNREFUSED npm silly logfile done cleaning log files npm http fetch GET http://localhost:4873/-/ping?write=true attempt 2 failed with ECONNREFUSED npm http fetch GET http://localhost:4873/-/ping?write=true attempt 3 failed with ECONNREFUSED Not ok request to http://localhost:4873/-/ping?write=true failed, reason: ``` --- lib/utils/ping.js | 2 +- mock-registry/lib/index.js | 2 +- .../test/lib/commands/doctor.js.test.cjs | 8 ++-- test/lib/commands/doctor.js | 48 +++++++++---------- test/lib/commands/ping.js | 18 +++++++ 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/lib/utils/ping.js b/lib/utils/ping.js index 00956d0c1630c..1c8c9e827a4ea 100644 --- a/lib/utils/ping.js +++ b/lib/utils/ping.js @@ -2,6 +2,6 @@ // used by the ping and doctor commands const fetch = require('npm-registry-fetch') module.exports = async (flatOptions) => { - const res = await fetch('/-/ping?write=true', flatOptions) + const res = await fetch('/-/ping', { ...flatOptions, cache: false }) return res.json().catch(() => ({})) } diff --git a/mock-registry/lib/index.js b/mock-registry/lib/index.js index 39bc63504cbe1..e96c9503ca9d8 100644 --- a/mock-registry/lib/index.js +++ b/mock-registry/lib/index.js @@ -320,7 +320,7 @@ class MockRegistry { } ping ({ body = {}, responseCode = 200 } = {}) { - this.nock = this.nock.get(this.fullPath('/-/ping?write=true')).reply(responseCode, body) + this.nock = this.nock.get(this.fullPath('/-/ping')).reply(responseCode, body) } // full unpublish of an entire package diff --git a/tap-snapshots/test/lib/commands/doctor.js.test.cjs b/tap-snapshots/test/lib/commands/doctor.js.test.cjs index 0f5b9520516f2..134e2290b5e99 100644 --- a/tap-snapshots/test/lib/commands/doctor.js.test.cjs +++ b/tap-snapshots/test/lib/commands/doctor.js.test.cjs @@ -1166,7 +1166,7 @@ Object { exports[`test/lib/commands/doctor.js TAP ping 404 > ping 404 1`] = ` Connecting to the registry Not ok -404 404 Not Found - GET https://registry.npmjs.org/-/ping?write=true +404 404 Not Found - GET https://registry.npmjs.org/-/ping Checking npm version Ok current: v1.0.0, latest: v1.0.0 @@ -1226,7 +1226,7 @@ Object { exports[`test/lib/commands/doctor.js TAP ping 404 in color > ping 404 in color 1`] = ` Connecting to the registry Not ok -404 404 Not Found - GET https://registry.npmjs.org/-/ping?write=true +404 404 Not Found - GET https://registry.npmjs.org/-/ping Checking npm version Ok current: v1.0.0, latest: v1.0.0 @@ -1286,7 +1286,7 @@ Object { exports[`test/lib/commands/doctor.js TAP ping exception with code > ping failure 1`] = ` Connecting to the registry Not ok -request to https://registry.npmjs.org/-/ping?write=true failed, reason: Test Error +request to https://registry.npmjs.org/-/ping failed, reason: Test Error Checking npm version Ok current: v1.0.0, latest: v1.0.0 @@ -1346,7 +1346,7 @@ Object { exports[`test/lib/commands/doctor.js TAP ping exception without code > ping failure 1`] = ` Connecting to the registry Not ok -request to https://registry.npmjs.org/-/ping?write=true failed, reason: Test Error +request to https://registry.npmjs.org/-/ping failed, reason: Test Error Checking npm version Ok current: v1.0.0, latest: v1.0.0 diff --git a/test/lib/commands/doctor.js b/test/lib/commands/doctor.js index 0c58a09e20c57..5d912d8b82f76 100644 --- a/test/lib/commands/doctor.js +++ b/test/lib/commands/doctor.js @@ -89,7 +89,7 @@ t.test('all clear', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -108,7 +108,7 @@ t.test('all clear in color', async t => { }, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -127,7 +127,7 @@ t.test('silent success', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -146,7 +146,7 @@ t.test('silent errors', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(404, '{}') + .get('/-/ping').reply(404, '{}') await t.rejects(npm.exec('doctor', ['ping']), { message: /Check logs/, }) @@ -161,7 +161,7 @@ t.test('ping 404', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(404, '{}') + .get('/-/ping').reply(404, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -182,7 +182,7 @@ t.test('ping 404 in color', async t => { }, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(404, '{}') + .get('/-/ping').reply(404, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -198,7 +198,7 @@ t.test('ping exception with code', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').replyWithError({ message: 'Test Error', code: 'TEST' }) + .get('/-/ping').replyWithError({ message: 'Test Error', code: 'TEST' }) .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -214,7 +214,7 @@ t.test('ping exception without code', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').replyWithError({ message: 'Test Error', code: false }) + .get('/-/ping').replyWithError({ message: 'Test Error', code: false }) .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -230,7 +230,7 @@ t.test('npm out of date', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest('2.0.0')) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -255,7 +255,7 @@ t.test('node out of date - lts', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -280,7 +280,7 @@ t.test('node out of date - current', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -297,7 +297,7 @@ t.test('non-default registry', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -318,7 +318,7 @@ t.test('missing git', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -344,7 +344,7 @@ t.test('windows skips permissions checks', async t => { globalPrefixDir: {}, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -361,7 +361,7 @@ t.test('missing global directories', async t => { globalPrefixDir: {}, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -377,7 +377,7 @@ t.test('missing local node_modules', async t => { globalPrefixDir: dirs.globalPrefixDir, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -406,7 +406,7 @@ t.test('incorrect owner', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -430,7 +430,7 @@ t.test('incorrect permissions', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -458,7 +458,7 @@ t.test('error reading directory', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -481,7 +481,7 @@ t.test('cacache badContent', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -504,7 +504,7 @@ t.test('cacache reclaimedCount', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -527,7 +527,7 @@ t.test('cacache missingContent', async t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') .get('/npm').reply(200, npmManifest(npm.version)) tnock(t, 'https://nodejs.org') .get('/dist/index.json').reply(200, nodeVersions) @@ -558,7 +558,7 @@ t.test('discrete checks', t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') await npm.exec('doctor', ['ping']) t.matchSnapshot(joinedOutput(), 'output') t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') @@ -586,7 +586,7 @@ t.test('discrete checks', t => { ...dirs, }) tnock(t, npm.config.get('registry')) - .get('/-/ping?write=true').reply(200, '{}') + .get('/-/ping').reply(200, '{}') await npm.exec('doctor', ['registry']) t.matchSnapshot(joinedOutput(), 'output') t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') diff --git a/test/lib/commands/ping.js b/test/lib/commands/ping.js index 7f90ea394f9ae..aca1e730131df 100644 --- a/test/lib/commands/ping.js +++ b/test/lib/commands/ping.js @@ -1,6 +1,8 @@ const t = require('tap') const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') const MockRegistry = require('@npmcli/mock-registry') +const cacache = require('cacache') +const path = require('node:path') t.test('no details', async t => { const { npm, logs, joinedOutput } = await loadMockNpm(t) @@ -74,3 +76,19 @@ t.test('invalid json', async t => { details: {}, }) }) +t.test('fail when registry is unreachable even if request is cached', async t => { + const { npm } = await loadMockNpm(t, { + config: { registry: 'https://ur.npmlocal.npmtest/' }, + cacheDir: { _cacache: {} }, + }) + const url = `${npm.config.get('registry')}-/ping` + const cache = path.join(npm.cache, '_cacache') + await cacache.put(cache, + `make-fetch-happen:request-cache:${url}`, + '{}', { metadata: { url } } + ) + t.rejects(npm.exec('ping', []), { + code: 'ENOTFOUND', + }, + 'throws ENOTFOUND error') +})