fix(@redhat-cloud-services/frontend-components-config): clean up properly with CTRL+C - #2397
Conversation
WalkthroughThe development proxy scripts now recursively terminate child processes, remove proxy and Chrome containers during cleanup, handle termination signals, and return a failure status when concurrent processes fail. ChangesDevelopment proxy shutdown and container lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Around line 368-377: Move the SIGINT and SIGTERM handlers to immediately after
the cleanup function is defined, before concurrently() or exec() starts any
child processes. Preserve both handlers’ existing behavior of calling cleanup()
and exiting with status 0.
- Around line 368-377: Update cleanup() to accept a completion callback, invoke
treeKill(pid, 'SIGKILL', callback) for each process, and call the callback only
after all process-tree kills finish. Replace the separate SIGINT/SIGTERM
handlers with a shared one-shot shutdown handler that waits for cleanup to
complete before calling process.exit(0), preventing repeated cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c2add71-5ac0-4f13-bfc1-a94b12079428
📒 Files selected for processing (1)
packages/config/src/bin/dev-proxy-script.ts
|
Hi @adonispuente Please update the commit message to Commit messages now need to include the full package names of affected packages. |
charlesmulder
left a comment
charlesmulder
left a comment
There was a problem hiding this comment.
Please tend to CodeRabbit comments.
|
@charlesmulder thank you for the quick look, I believe Ive addressed the code rabbit comments as well as fixed the commit message! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/config/src/bin/dev-proxy-script.ts (1)
259-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the lifecycle helpers.
Annotate both
cleanupandshutdownwith: void.Proposed fix
- const cleanup = (onComplete?: () => void) => { + const cleanup = (onComplete?: () => void): void => { ... - const shutdown = (exitCode = 0) => { + const shutdown = (exitCode = 0): void => {As per path instructions:
packages/*/src/**/*.{ts,tsx}requires checking for missing return types on functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/config/src/bin/dev-proxy-script.ts` around lines 259 - 299, Annotate the lifecycle helper functions cleanup and shutdown with explicit : void return types, preserving their existing control flow and callback behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Around line 291-299: Replace both direct process exits in the Chrome/container
startup catch and the outer catch around concurrently()/exec() with shutdown(1),
ensuring all failure paths use the centralized cleanup in shutdown. Preserve the
existing successful shutdown behavior and exit-code handling.
---
Nitpick comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Around line 259-299: Annotate the lifecycle helper functions cleanup and
shutdown with explicit : void return types, preserving their existing control
flow and callback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c042574-81d2-4823-93a9-1df8fbb2824f
📒 Files selected for processing (1)
packages/config/src/bin/dev-proxy-script.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@adonispuente I tested on local by checking out your branch to local. I can see the commit. Then building and packaging the package with your commit. # build
nvm use
npx nx run @redhat-cloud-services/frontend-components-config:build
# package
cd dist/@redhat-cloud-services/frontend-components-config
npm pack # redhat-cloud-services-frontend-components-config-6.14.1.tgzThen ran it from npm install --save ../frontend-components/dist/@redhat-cloud-services/frontend-components-config/redhat-cloud-services-frontend-components-config-6.14.1.tgz
npx fec dev-proxyOnce started I killed the process with lsof -i :1337
lsof -i :8003
lsof -i :9998Tried it again. Ran lsof -i :1337
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
pasta.avx 570158 chmulder 152u IPv6 7723274 0t0 TCP *:menandmice-dns (LISTEN) |
|
@charlesmulder there were 2 issues
With the new changes, after testing iop mode and with your instructions I got the desire behavior repeatedly |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Around line 400-401: Update the rejection callback passed to concurrently in
the dev proxy script so it calls shutdown with a nonzero failure exit code
instead of 0. Keep the success callback unchanged with shutdown(0), ensuring
BUILD/SERVE/PROXY failures propagate as unsuccessful exits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e25b71dd-679d-4f83-b3da-087b069f3edc
📒 Files selected for processing (1)
packages/config/src/bin/dev-proxy-script.ts
# ctrl+c
^C[fec] Info: Closing frontend CRD watcher
[SERVE] http-server stopped.
[BUILD] [fec] Info: Closing frontend CRD watcher
[BUILD] [fec] Info: Closing frontend CRD watcher
[fec] Info: Closing frontend CRD watcher
[fec] Error: Chrome server stopped unexpectedly! The server port 9998 is already in use!
# waited and ran it multiple times
$ lsof -i :1337
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
chrome 1160275 chmulder 63u IPv4 14140054 0t0 TCP localhost:53562->localhost:menandmice-dns (ESTABLISHED)
pasta.avx 1198183 chmulder 88u IPv6 14130960 0t0 TCP localhost:menandmice-dns->localhost:53562 (ESTABLISHED)
pasta.avx 1198183 chmulder 154u IPv6 14159986 0t0 TCP *:menandmice-dns (LISTEN)
$ podman ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
a13c92516d9f quay.io/redhat-user-workloads/hcc-platex-services-tenant/frontend-development-proxy:latest 32 seconds ago Up 32 seconds 0.0.0.0:1337->1337/tcp, 80/tcp, 443/tcp, 2019/tcp, 443/udp frontend-development-proxyProof it's using the updated script grep -rn removeContainer node_modules/@redhat-cloud-services/frontend-components-config/
node_modules/@redhat-cloud-services/frontend-components-config/bin/dev-proxy-script.js:128:function removeContainer(containerName) {
node_modules/@redhat-cloud-services/frontend-components-config/bin/dev-proxy-script.js:335: removeContainer(DEV_PROXY_CONTAINER_NAME);
node_modules/@redhat-cloud-services/frontend-components-config/bin/dev-proxy-script.js:343: removeContainer(DEV_PROXY_CONTAINER_NAME);
node_modules/@redhat-cloud-services/frontend-components-config/bin/dev-proxy-script.js:346: removeContainer(serve_chrome_1.CONTAINER_NAME); |
|
@charlesmulder im unsure why i cant reproduce this on my end, taking another wack at it now |
|
@charlesmulder Running it your way as well as in IOP mode, i was able to confirm the ports are always removed. Its a little overkill, and often times in the terminal you can see '''[fec] Error: Failed to remove the container: frontend-development-proxy''' , but thats because after the -rm, im also do removeContainer JUST INCASE rm doesnt work for whatever reason. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/config/src/bin/dev-proxy-script.ts`:
- Around line 25-43: Update killProcessTree to use a cross-platform process-tree
termination mechanism such as tree-kill, or equivalent platform-specific
cleanup, instead of invoking pgrep -P. Ensure cleanup terminates the target
process and all descendants on both Unix-like systems and Windows, while
preserving the existing tolerance for already-terminated processes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1820916-0628-4ef1-93b6-47ac960d3529
📒 Files selected for processing (2)
packages/config/src/bin/dev-proxy-script.tspackages/config/src/bin/serve-chrome.ts
|
@adonispuente Is the |
charlesmulder
changed the title
Its essentially overkill, --rm handles the normal case, but removeContainer is kept as a safety net for edge I was only testing originally in IOP mode, but with the devproxy script or however else itd be ran, i figured it be better to be safe than sorry |
2 participants
Theres an issue in the dev-proxy script where when trying to kill the terminal with CTRL+C, it hangs up and doesnt clean up properly. This just fixes that
Summary by CodeRabbit