Skip to content

[Reviewed] fix: halt deployment on command load error (Issue #40 - Item 1)#42

Open
Ramanand-Shirbhate wants to merge 2 commits into
IN3PIRE:mainfrom
Ramanand-Shirbhate:patch-1
Open

[Reviewed] fix: halt deployment on command load error (Issue #40 - Item 1)#42
Ramanand-Shirbhate wants to merge 2 commits into
IN3PIRE:mainfrom
Ramanand-Shirbhate:patch-1

Conversation

@Ramanand-Shirbhate
Copy link
Copy Markdown
Contributor

This PR addresses Item #1 from the Critical & Enhancement Bounty Board (#40).The Issue:
Previously, if src/deploy-commands.js failed to load commands, it would catch the error, log it, and continue the deployment process. This resulted in silent failures where incomplete command sets were deployed to Discord. The Fix:
Added process.exit(1) inside the catch block to ensure the Node process halts immediately upon a command load failure, preventing partial deployments. Fixes #40 (Item 1)

Copy link
Copy Markdown
Contributor

@TrivCodez TrivCodez left a comment

Choose a reason for hiding this comment

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

🔴 Critical: Deployment Fix Has Fatal Flaw

While the intention to prevent partial deployments is good, process.exit(1) in the catch block violates Node.js event loop contract and can corrupt in-flight operations. Node.js deployments (PM2, systemd, Docker) expect graceful shutdowns, not hard exits.

✅ Good

  • Error message now includes context: 'Error loading commands:', error
  • Addresses the silent failure issue

🟡 Warnings

  • Missing newline at EOF causes "No newline at end of file" warning

🔴 Critical Issues

File: src/deploy-commands.js - Line 23

Issue: Using process.exit(1) crashes the process immediately
Consequence:

  • Forces host managers into hard-kill recovery loops
  • Prevents graceful shutdown and cleanup
  • May lose process exit code context
    Fix: Use process.exitCode = 1 and allow natural exit

@Ramanand-Shirbhate
Copy link
Copy Markdown
Contributor Author

Great catch regarding the Node.js event loop contract! You are entirely right—hard exiting can definitely cause issues with PM2/Docker cleanups.
I've pushed a commit that swaps it to process.exitCode = 1 to allow for a graceful shutdown, and I also added the missing EOF newline to clear out that warning. Let me know if it looks good to merge now!

Copy link
Copy Markdown
Contributor

@TrivCodez TrivCodez left a comment

Choose a reason for hiding this comment

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

This change is a step in the right direction, but process.exitCode = 1 does not actually stop execution. The script will continue to run and exit cleanly after the IIFE finishes. This can still result in partial deployments if the error occurs early.

The correct approach is to use process.exit(1) (without the Code suffix) to immediately terminate the Node.js process and prevent any further execution.

Suggested change:

- process.exitCode = 1;
+ process.exit(1);

This ensures the deployment halts immediately on any command load failure, preventing silent partial deployments as intended.

@TrivCodez TrivCodez changed the title fix: halt deployment on command load error (Issue #40 - Item 1) [Reviewed] fix: halt deployment on command load error (Issue #40 - Item 1) May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Critical & Enhancement Issues - Bounty Board

2 participants