Skip to content

fix: ignore npm registry 404 status response on sync process #740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 9, 2024
Merged

fix: ignore npm registry 404 status response on sync process #740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2285320
fix: ignore npm registry 404 status response on sync process
fengmk2 Dec 9, 2024
a609488
f
fengmk2 Dec 9, 2024
a761058
f
fengmk2 Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions app/core/service/PackageSyncerService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@
const { status, data } = remoteFetchResult;

// deleted or blocked
if (status === 404 || status === 451) {
if (status === 451) {
return true;
}

Expand All @@ -245,6 +245,23 @@

// security holder
// test/fixtures/registry.npmjs.org/security-holding-package.json
// {
// "_id": "xxx",
// "_rev": "9-a740a77bcd978abeec47d2d027bf688c",
// "name": "xxx",
// "time": {
// "modified": "2017-11-28T00:45:24.162Z",
// "created": "2013-09-20T23:25:18.122Z",
// "0.0.0": "2013-09-20T23:25:20.242Z",
// "1.0.0": "2016-06-22T00:07:41.958Z",
// "0.0.1-security": "2016-12-15T01:03:58.663Z",
// "unpublished": {
// "time": "2017-11-28T00:45:24.163Z",
// "versions": []
// }
// },
// "_attachments": {}
// }
let isSecurityHolder = true;
for (const versionInfo of Object.entries<{ _npmUser?: { name: string } }>(data.versions || {})) {
const [ v, info ] = versionInfo;
Expand Down Expand Up @@ -455,6 +472,19 @@
return;
}

if (status === 404) {
// ignore 404 status
// https://github.com/node-modules/detect-port/issues/57
task.error = `Package not found, status 404, data: ${JSON.stringify(data)}`;
logs.push(`[${isoNow()}] ❌ ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌ Synced ${fullname} fail, ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname} ❌❌❌❌❌`);
this.logger.info('[PackageSyncerService.executeTask:fail-request-error] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}
Comment on lines +475 to +486
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Align 404 status handling with PR objectives

Currently, when a 404 status is encountered, the task is marked as failed, and the process exits. According to the PR objectives, 404 responses from the npm registry during maintenance should be ignored to prevent unnecessary deletions. Instead of failing the task, consider logging the information and proceeding without marking the task as failed.

Apply the following diff to adjust the handling:

        if (status === 404) {
          // ignore 404 status
          // https://github.com/node-modules/detect-port/issues/57
-         task.error = `Package not found, status 404, data: ${JSON.stringify(data)}`;
-         logs.push(`[${isoNow()}] ❌ ${task.error}, log: ${logUrl}`);
-         logs.push(`[${isoNow()}] ❌ Synced ${fullname} fail, ${task.error}, log: ${logUrl}`);
-         logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname} ❌❌❌❌❌`);
-         this.logger.info('[PackageSyncerService.executeTask:fail-request-error] taskId: %s, targetName: %s, %s',
-           task.taskId, task.targetName, task.error);
-         await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
-         return;
+         logs.push(`[${isoNow()}] ⚠️ Package not found (404), but ignoring as per maintenance handling.`);
+         // Continue processing without marking the task as failed
+         await this.taskService.appendTaskLog(task, logs.join('\n'));
+         logs = [];
        }

Committable suggestion skipped: line range outside the PR's diff.


let readme = data.readme || '';
if (typeof readme !== 'string') {
readme = JSON.stringify(readme);
Expand Down Expand Up @@ -545,9 +575,9 @@
task.error = `invalid maintainers: ${JSON.stringify(maintainers)}`;
logs.push(`[${isoNow()}] ❌ ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ${failEnd}`);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail-invalid-maintainers] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}

Expand Down Expand Up @@ -575,9 +605,9 @@
task.error = 'There is no available specific versions, stop task.';
logs.push(`[${isoNow()}] ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname} ❌❌❌❌❌`);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail-empty-list] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));

Check warning on line 610 in app/core/service/PackageSyncerService.ts

View check run for this annotation

Codecov / codecov/patch

app/core/service/PackageSyncerService.ts#L610 

Added line #L610 was not covered by tests
return;
}
if (specificVersions) {
Expand Down Expand Up @@ -764,9 +794,9 @@
logs.push(`[${isoNow()}] ❌ All versions sync fail, package not exists, log: ${logUrl}`);
logs.push(`[${isoNow()}] ${failEnd}`);
task.error = lastErrorMessage;
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail] taskId: %s, targetName: %s, package not exists',
task.taskId, task.targetName);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}

Expand Down Expand Up @@ -924,8 +954,8 @@
logs.push(`[${isoNow()}] 📝 Log URL: ${logUrl}`);
logs.push(`[${isoNow()}] 🔗 ${url}`);
task.error = lastErrorMessage;
await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:success] taskId: %s, targetName: %s',
task.taskId, task.targetName);
await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure task completion state reflects success

The task is finished with TaskState.Success without checking if any versions were successfully synced. If all versions failed to sync, the task state should reflect failure to prevent misleading success states.

Verify if the task should be marked as failed:

-       await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
+       const hasSyncedVersions = updateVersions.length > 0;
+       const taskState = hasSyncedVersions ? TaskState.Success : TaskState.Fail;
+       await this.taskService.finishTask(task, taskState, logs.join('\n'));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
const hasSyncedVersions = updateVersions.length > 0;
const taskState = hasSyncedVersions ? TaskState.Success : TaskState.Fail;
await this.taskService.finishTask(task, taskState, logs.join('\n'));

}
}
2 changes: 1 addition & 1 deletion app/port/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export type CnpmcoreConfig = {
/**
* sync mode
* - none: don't sync npm package
* - admin: don't sync npm package,only admin can create sync task by sync contorller.
* - admin: don't sync npm package,only admin can create sync task by sync controller.
* - all: sync all npm packages
* - exist: only sync exist packages, effected when `enableCheckRecentlyUpdated` or `enableChangesStream` is enabled
*/
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
"ssri": "^8.0.1",
"type-fest": "^2.5.3",
"ua-parser-js": "^1.0.34",
"validate-npm-package-name": "^3.0.0"
"validate-npm-package-name": "^6.0.0"
},
"optionalDependencies": {
"s3-cnpmcore": "^1.1.2"
Expand All @@ -138,7 +138,7 @@
"@types/semver": "^7.3.12",
"@types/tar": "^6.1.4",
"@types/ua-parser-js": "^0.7.36",
"@types/validate-npm-package-name": "^4.0.0",
"@types/validate-npm-package-name": "^4.0.2",
"coffee": "^5.4.0",
"egg-bin": "^6.0.0",
"egg-mock": "^5.10.4",
Expand Down
52 changes: 48 additions & 4 deletions test/core/service/PackageSyncerService/executeTask.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync cnpmcore-test-sync-deprecated and mock 404', async () => {
it('should sync cnpmcore-test-sync-deprecated and mock 451', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/cnpmcore-test-sync-deprecated.json'),
persist: false,
Expand All @@ -209,9 +209,9 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!._hasShrinkwrap, false);
app.mockAgent().assertNoPendingInterceptors();

// mock 404 and unpublished
// mock 451 and unpublished
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
status: 404,
status: 451,
data: '{"error":"Not found"}',
persist: false,
});
Expand Down Expand Up @@ -260,6 +260,50 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync cnpmcore-test-sync-deprecated and ignore 404 in removed', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/cnpmcore-test-sync-deprecated.json'),
persist: false,
});
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated/-/cnpmcore-test-sync-deprecated-0.0.0.tgz', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/foobar/-/foobar-1.0.0.tgz'),
persist: false,
});
const name = 'cnpmcore-test-sync-deprecated';
await packageSyncerService.createTask(name);
let task = await packageSyncerService.findExecuteTask();
assert(task);
await packageSyncerService.executeTask(task);
const manifests = await packageManagerService.listPackageFullManifests('', name);
assert(manifests.data);
assert(manifests!.data!.versions['0.0.0']);

assert.equal(manifests.data.versions['0.0.0'].deprecated, 'only test for cnpmcore');
assert.equal(manifests.data.versions['0.0.0']._hasShrinkwrap, false);
const abbreviatedManifests = await packageManagerService.listPackageAbbreviatedManifests('', name);
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!.deprecated, 'only test for cnpmcore');
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!._hasShrinkwrap, false);
app.mockAgent().assertNoPendingInterceptors();

// mock 404 and no unpublished
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
status: 404,
data: '{"error":"Not found"}',
persist: false,
});
await packageSyncerService.createTask(name);
task = await packageSyncerService.findExecuteTask();
assert(task);
await packageSyncerService.executeTask(task);
const stream = await packageSyncerService.findTaskLog(task);
assert(stream);
const log = await TestUtil.readStreamToLog(stream);
// console.log(log);
assert(!log.includes(`] 🟢 Package "${name}" was removed in remote registry`));
assert(log.includes('Package not found, status 404'));
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync fail when package not exists', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-package-not-exists', 'GET', {
status: 404,
Expand All @@ -275,7 +319,7 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
assert(stream);
const log = await TestUtil.readStreamToLog(stream);
// console.log(log);
assert(log.includes('] ❌ Package not exists, response data: '));
assert(log.includes('Package not found, status 404'));
app.mockAgent().assertNoPendingInterceptors();
});

Expand Down
Loading