fix: ignore npm registry 404 status response on sync process #740
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes in this pull request focus on enhancing error handling and refining logic within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/core/service/PackageSyncerService.ts (2)
580-580: Avoid early task failure for invalid maintainersWhen no maintainers are found, the task is marked as failed immediately. Considering intermittent issues during maintenance, it might be better to log the error and retry instead of failing immediately.
Consider modifying the code to retry the task:
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n')); + // Optionally, you could choose to retry the task instead + // await this.taskService.retryTask(task, logs.join('\n')); return;
799-799: Handle complete sync failure gracefullyWhen all version syncs fail and the package does not exist locally, the task is marked as failed with the last error message. Consider providing more context in the logs to aid debugging and potentially implementing retry logic if appropriate.
Enhance the logs for clarity:
task.error = lastErrorMessage; logs.push(`[${isoNow()}] ❌ All versions failed to sync for ${fullname}, no local package found.`); logs.push(`[${isoNow()}] ❌ Last error: ${task.error}, log: ${logUrl}`); logs.push(`[${isoNow()}] ${failEnd}`);test/core/service/PackageSyncerService/executeTask.test.ts (2)
212-214: Clarify test case for handling451status codeThe test case mocks a
451status but is described as "mock 404". Update the description to accurately reflect that this test is for handling a451status code when syncing a deprecated package.Apply this change:
- it('should sync cnpmcore-test-sync-deprecated and mock 404', async () => { + it('should sync cnpmcore-test-sync-deprecated and mock 451', async () => {
321-322: Remove unnecessaryconsole.login testsThe
console.log(log);statement on line 321 is not necessary and can clutter test output. Consider removing it to keep the test output clean.Apply this diff to remove the log statement:
- console.log(log);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/core/service/PackageSyncerService.ts(7 hunks)app/port/config.ts(1 hunks)package.json(2 hunks)test/core/service/PackageSyncerService/executeTask.test.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/port/config.ts
🔇 Additional comments (4)
package.json (2)
141-141: LGTM: Type definitions update
The minor version bump of @types/validate-npm-package-name (4.0.0 → 4.0.2) aligns with the main package update and includes only backward-compatible changes.
122-122: Verify compatibility with validate-npm-package-name v6.0.0
The major version bump (3.x → 6.x) indicates breaking changes. Please ensure that the package sync logic in PackageSyncerService.ts accounts for any changes in validation rules or APIs.
✅ Verification successful
Let me gather more information about the breaking changes in validate-npm-package-name v6.0.0 and how it's used in the codebase.
Let me gather more information about the breaking changes in validate-npm-package-name v6.0.0 by checking the changelog or release notes.
Based on the gathered information, I can now provide a final response:
The validate-npm-package-name update appears safe to proceed
The code in SavePackageVersionController.ts uses the package in a way that remains compatible with v6.0.0:
- It only checks
validForNewPackagesproperty which exists in both v3 and v6 - The error handling uses optional chaining with
errors || warnings || []which safely handles both versions - The validation is only enforced for new packages, with existing packages being allowed to continue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check breaking changes and usage of validate-npm-package-name
# Check for validation rule usage
rg -A 5 "validate-npm-package-name"
# Check package validation calls that might be affected
ast-grep --pattern 'validateNpmPackageName($$$)'
Length of output: 2459
Script:
#!/bin/bash
# Check the validation result usage to understand how breaking changes might affect the code
ast-grep --pattern 'validateResult.$$$'
# Check for any other validation-related code that might be affected
rg -A 10 "validForNewPackages|validForOldPackages|errors|warnings" app/port/controller/package/SavePackageVersionController.ts
Length of output: 47567
Script:
#!/bin/bash
# Check package.json history to see the previous version
git show HEAD~1:package.json | grep "validate-npm-package-name"
# Check for any tests that might be affected by the validation changes
rg -l "validate-npm-package-name" test/
Length of output: 195
app/core/service/PackageSyncerService.ts (1)
231-232: Reconsider excluding 404 status code from removal check
In the isRemovedInRemote method, the check for status === 404 has been removed, and now only status === 451 is considered for determining if a package is removed. However, a 404 status code typically indicates that the resource is not found on the remote registry. Excluding it might lead to overlooking cases where a package has been deleted. Please verify if this change aligns with the intended behavior.
test/core/service/PackageSyncerService/executeTask.test.ts (1)
263-305: Enhance test coverage for 404 status code handling
The new test case effectively checks the behavior when a 404 status code is received, ensuring the package is not considered removed. Ensure that assertions properly validate that the task does not mark the package as removed and that appropriate logs are generated.
| this.logger.info('[PackageSyncerService.executeTask:success] taskId: %s, targetName: %s', | ||
| task.taskId, task.targetName); | ||
| await this.taskService.finishTask(task, TaskState.Success, logs.join('\n')); |
There was a problem hiding this comment.
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.
| 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')); |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #740 +/- ##
==========================================
- Coverage 96.44% 96.40% -0.05%
==========================================
Files 191 191
Lines 19017 19047 +30
Branches 2503 2498 -5
==========================================
+ Hits 18341 18362 +21
- Misses 676 685 +9 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [3.71.1](v3.71.0...v3.71.1) (2024-12-09) ### Bug Fixes * ignore npm registry 404 status response on sync process ([#740](#740)) ([57226c5](57226c5))
closes #739
Summary by CodeRabbit
New Features
Bug Fixes
syncModeproperty.Chores
package.json.Tests