Build: Enhance run-registry script to kill existing Verdaccio processes #33005
Conversation
|
View your CI Pipeline Execution ↗ for commit 3907cd9
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds a new cross-platform utility function to terminate processes bound to specific ports, then integrates it into the registry startup script to clean ports 6001 and 6002 before Verdaccio initialization. The utility handles both Windows and Unix-like systems with platform-specific command execution, error handling, and logging. Changes
Sequence DiagramsequenceDiagram
participant User
participant startVerdaccio
participant killProcessOnPort
participant OS Command
participant Verdaccio
User->>startVerdaccio: Call startVerdaccio()
startVerdaccio->>killProcessOnPort: killProcessOnPort(6001)
killProcessOnPort->>OS Command: Execute process discovery<br/>(netstat/lsof)
OS Command-->>killProcessOnPort: Process PIDs (if any)
alt Processes Found
killProcessOnPort->>OS Command: Terminate via taskkill/kill -9
OS Command-->>killProcessOnPort: Success
Note over killProcessOnPort: Wait 500ms for port release
else No Processes
killProcessOnPort->>killProcessOnPort: Graceful no-op
end
killProcessOnPort->>startVerdaccio: Return
startVerdaccio->>killProcessOnPort: killProcessOnPort(6002)
killProcessOnPort-->>startVerdaccio: Return
startVerdaccio->>Verdaccio: Initialize server
Verdaccio-->>User: Server ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/run-registry.ts(2 hunks)scripts/utils/kill-process-on-port.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
scripts/utils/kill-process-on-port.tsscripts/run-registry.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
scripts/utils/kill-process-on-port.tsscripts/run-registry.ts
scripts/**/*.{ts,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In Node.js scripts, use
storybook/internal/node-loggerinstead ofconsole.*
Files:
scripts/utils/kill-process-on-port.tsscripts/run-registry.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
scripts/utils/kill-process-on-port.tsscripts/run-registry.ts
🧬 Code graph analysis (1)
scripts/run-registry.ts (1)
scripts/utils/kill-process-on-port.ts (1)
killProcessOnPort(7-54)
🪛 ast-grep (0.39.9)
scripts/utils/kill-process-on-port.ts
[warning] 15-15: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(TCP.*:${port}.*LISTENING\\s+(\\d+), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
| const { stdout } = await execa('netstat', ['-ano']); | ||
| const lines = stdout.split('\n'); | ||
| const regex = new RegExp(`TCP.*:${port}.*LISTENING\\s+(\\d+)`, 'i'); | ||
|
|
||
| for (const line of lines) { | ||
| const match = line.match(regex); | ||
| if (match && match[1]) { | ||
| pids.push(match[1]); | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Fix Windows PID parsing to avoid killing unrelated ports
The current regex matches any substring :${port}, so asking for port 6001 will also match 60012/60010 and can terminate the wrong process. Split the netstat columns and ensure the local address ends with the exact port before you add the PID to the kill list.
- const { stdout } = await execa('netstat', ['-ano']);
- const lines = stdout.split('\n');
- const regex = new RegExp(`TCP.*:${port}.*LISTENING\\s+(\\d+)`, 'i');
-
- for (const line of lines) {
- const match = line.match(regex);
- if (match && match[1]) {
- pids.push(match[1]);
- }
- }
+ const { stdout } = await execa('netstat', ['-ano']);
+ const lines = stdout.split('\n');
+
+ for (const line of lines) {
+ const parts = line.trim().split(/\s+/);
+ if (
+ parts.length >= 5 &&
+ parts[0]?.toUpperCase().startsWith('TCP') &&
+ parts[1]?.endsWith(`:${port}`) &&
+ parts[3]?.toUpperCase() === 'LISTENING'
+ ) {
+ pids.push(parts[4]);
+ }
+ }📝 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.
| const { stdout } = await execa('netstat', ['-ano']); | |
| const lines = stdout.split('\n'); | |
| const regex = new RegExp(`TCP.*:${port}.*LISTENING\\s+(\\d+)`, 'i'); | |
| for (const line of lines) { | |
| const match = line.match(regex); | |
| if (match && match[1]) { | |
| pids.push(match[1]); | |
| } | |
| } | |
| } catch { | |
| const { stdout } = await execa('netstat', ['-ano']); | |
| const lines = stdout.split('\n'); | |
| for (const line of lines) { | |
| const parts = line.trim().split(/\s+/); | |
| if ( | |
| parts.length >= 5 && | |
| parts[0]?.toUpperCase().startsWith('TCP') && | |
| parts[1]?.endsWith(`:${port}`) && | |
| parts[3]?.toUpperCase() === 'LISTENING' | |
| ) { | |
| pids.push(parts[4]); | |
| } | |
| } | |
| } catch { |
🧰 Tools
🪛 ast-grep (0.39.9)
[warning] 15-15: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(TCP.*:${port}.*LISTENING\\s+(\\d+), 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In scripts/utils/kill-process-on-port.ts around lines 14–24, the netstat parsing
regex can match substrings of ports (e.g., 6001 matching 60010), so replace the
regex approach with column-based parsing: split each netstat line on whitespace,
take the Local Address column (the second column), check that it ends exactly
with `:${port}` (so IPv4/IPV6 forms like [::]:80 still match), and only then
read the PID from the last column (or columns[columns.length-1]) after
validating it is numeric before pushing to pids; this prevents killing processes
bound to ports that merely contain the queried port as a substring.
| if (pids.length > 0) { | ||
| console.log(`☠️ killing process(es) on port ${port}: ${pids.join(', ')}`); | ||
|
|
||
| if (isWindows) { | ||
| await Promise.all( | ||
| pids.map((pid) => execa('taskkill', ['/PID', pid, '/F']).catch(() => {})) | ||
| ); | ||
| } else { | ||
| await Promise.all(pids.map((pid) => execa('kill', ['-9', pid]).catch(() => {}))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Swap to Storybook’s node logger
Per our scripts guidelines we should log through storybook/internal/node-logger, not console.log. Please route this message through the shared logger for consistency. As per coding guidelines
import { execa } from 'execa';
+import { logger } from 'storybook/internal/node-logger';
@@
- console.log(`☠️ killing process(es) on port ${port}: ${pids.join(', ')}`);
+ logger.info(`☠️ killing process(es) on port ${port}: ${pids.join(', ')}`);📝 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.
| if (pids.length > 0) { | |
| console.log(`☠️ killing process(es) on port ${port}: ${pids.join(', ')}`); | |
| if (isWindows) { | |
| await Promise.all( | |
| pids.map((pid) => execa('taskkill', ['/PID', pid, '/F']).catch(() => {})) | |
| ); | |
| } else { | |
| await Promise.all(pids.map((pid) => execa('kill', ['-9', pid]).catch(() => {}))); | |
| import { execa } from 'execa'; | |
| import { logger } from 'storybook/internal/node-logger'; | |
| // ... other code ... | |
| if (pids.length > 0) { | |
| logger.info(`☠️ killing process(es) on port ${port}: ${pids.join(', ')}`); | |
| if (isWindows) { | |
| await Promise.all( | |
| pids.map((pid) => execa('taskkill', ['/PID', pid, '/F']).catch(() => {})) | |
| ); | |
| } else { | |
| await Promise.all(pids.map((pid) => execa('kill', ['-9', pid]).catch(() => {}))); | |
| } | |
| } |
🤖 Prompt for AI Agents
In scripts/utils/kill-process-on-port.ts around lines 37 to 45, replace the
console.log call with Storybook’s shared node logger: import the logger from
'storybook/internal/node-logger' at the top of the file and use logger.info (or
logger.warn) to emit the "killing process(es) on port" message, keeping the same
string interpolation; leave the rest of the PID-killing logic unchanged and
ensure the import is added only once.
Closes #
What I did
This PR makes it so that the infamous EADDRESS error won't occur anymore.
p.s. Needs testing on Windows
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit