src: stop running process as nohup to avoid missing logs

[adityamaru]

Important

Improve buildkitd process management by replacing nohup with spawn, adding backoff retry logic, and enhancing logging and error handling.

• Process Management:
  • Replace nohup with spawn in startBuildkitd() in setup_builder.ts for better buildkitd process management and logging.
  • Implement backoff retry logic in startBuildkitd() and shutdownBuildkitd() for starting and shutting down buildkitd.
• Error Handling:
  • Add warnings for errors in pgrep checks in main.ts to handle unexpected errors when checking for buildkitd processes.
  • Throw error if buildkitd fails to start within 10 seconds in startBuildkitd().
• Logging:
  • Pipe buildkitd stdout and stderr to buildkitd.log in setup_builder.ts for better log management.
  • Log contents of buildkitd.log on build error in main.ts.

^{This description was created by for 9302d2a. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 7d9d5fd in 1 minute and 55 seconds

More details

• Looked at 135 lines of code in 2 files
• Skipped 2 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. src/setup_builder.ts:131

• Draft comment:
  Ensure that errors from the spawn function are properly logged and handled. Consider using core.error to log the error message before throwing it.
• Reason this comment was not posted:
  Comment did not seem useful.

2. src/main.ts:493

• Draft comment:
  The check for pgrep should be on stdout instead of stderr. pgrep outputs to stdout, so the condition should be if (stdout) to confirm the process is running. This applies to other instances of pgrep usage as well.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_Lg4FUwxQdWo3e4Aa

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on 4035032 in 1 minute and 11 seconds

More details

• Looked at 135 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. src/setup_builder.ts:132

• Draft comment:
  Consider adding an exit event listener to handle cases where buildkitd exits with a non-zero code, indicating a failure to start.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code already has error handling through: 1) The 'error' event listener for spawn failures 2) A timeout check to verify buildkitd is running 3) A while loop that verifies the process exists via pgrep 4) Multiple layers of try-catch blocks. An exit handler would be redundant since process exit is already effectively monitored.
  The comment has some merit since explicitly handling the exit event could provide more detailed error information. The current error handling might miss some edge cases where buildkitd exits after starting but before being fully ready.
  While an exit handler could provide more detail, the existing multi-layered error handling is comprehensive and would catch any critical failures. The suggestion would add complexity without significant benefit.
  The comment should be removed as the code already has robust error handling through multiple mechanisms that effectively cover process failure scenarios.

2. src/setup_builder.ts:141

• Draft comment:
  pgrep outputs to stdout, not stderr. The check should be on stdout to determine if the process is running.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_0v0GS7jA1JW4sMuV

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 9302d2a in 1 minute and 9 seconds

More details

• Looked at 135 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. src/setup_builder.ts:130

• Draft comment:
  Consider adding an 'exit' event listener to handle cases where the 'buildkitd' process exits unexpectedly. This will help in capturing errors that occur after the process has started but before it is fully operational.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code has been changed to use spawn() instead of exec() for running buildkitd. While there is error handling for the initial startup via the 'error' event, there's no handling of unexpected exits. However, the code does verify the process is running and working through multiple checks - pgrep polling and buildctl debug workers checks. An exit handler might be redundant given these checks.
  The comment has merit since unexpected exits could occur between checks, and the current error handling might not catch all failure modes. The buildctl checks might fail for other reasons besides process exit.
  While valid, the existing checks are comprehensive - they verify both process existence and functionality. The exit handler would add complexity without significantly improving the error handling that's already in place.
  The comment should be removed since the code already has robust error detection through process checks and buildctl verification. An exit handler would be redundant.

2. src/setup_builder.ts:110

• Draft comment:
  Ensure consistent error handling for all 'execAsync' calls. Consider using try-catch blocks to handle potential errors and log them appropriately. This applies to other 'execAsync' calls in this file as well.
• Reason this comment was not posted:
  Comment did not seem useful.

3. src/main.ts:482

• Draft comment:
  Consider making the backoff interval in 'shutdownBuildkitd' configurable to allow for flexibility in different environments. This can help optimize the shutdown process based on specific needs.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The 'shutdownBuildkitd' function in 'main.ts' uses a backoff retry mechanism to wait for the process to terminate. However, the backoff interval is hardcoded, which might not be optimal for all environments. Making it configurable could improve flexibility and performance.

Workflow ID: wflow_ppMWKrJP7O7jWapR

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.