cli: add error handling with panic recovery inside main()

[anirudh240]

What type of PR is this?
/kind enhancement

/kind enhancement

What this PR does / why we need it:
PR adds proper error handling to the Kthena CLI main function using defer/recover to catch panics from cmd.Execute().

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[volcano-sh-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign git-malu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• cli/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemini-code-assist]

Code Review

This pull request introduces a panic recovery mechanism in the main function of the kthena CLI to handle unexpected errors gracefully. Feedback suggests that the explicit call to os.Exit(0) is unnecessary and prevents deferred functions from executing. Furthermore, it was noted that the current implementation of cmd.Execute may bypass the recovery logic by calling os.Exit internally, and a refactor to return errors is recommended for more robust handling.

[gemini-code-assist]

The defer/recover block is a good addition for handling unexpected panics. However, cmd.Execute() (as defined in cli/kthena/cmd/root.go) calls os.Exit(1) directly when a command returns an error. Since os.Exit() terminates the program immediately, it will bypass this deferred function. This means your centralized panic/error handling logic in main won't be executed for regular errors.

To make this more robust, consider changing cmd.Execute() to return an error, which can then be handled here in main. This would centralize all exit logic.

[gemini-code-assist]

This call to os.Exit(0) is not necessary, as a Go program automatically exits with status 0 when the main function returns. More importantly, os.Exit() prevents deferred functions from running. By calling it here, you prevent the defer block above from executing on a successful run path. It's better to let main return naturally.

[Copilot]

Pull request overview

Adds panic recovery/error reporting to the kthena CLI entrypoint (main()), aiming to make CLI failures more robust during command execution.

Changes:

• Add defer + recover() around cmd.Execute() to catch panics and print an error to stderr.
• Introduce explicit os.Exit(...) calls for both failure and success paths.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Avoid calling os.Exit(0) at the end of main. os.Exit bypasses all deferred functions, which makes it easy to accidentally skip future cleanup/log flushing (and means this new deferred recovery block will never run on the success path). Prefer letting main return normally, or manage an explicit exit code variable and call os.Exit once at the very end if needed.

Suggested change

os.Exit(0)

[Copilot]

The recovered panic message formatting looks off ("Error :" has an extra space and differs from other stderr messages like "warning:"). Also, printing only %v loses useful context for debugging panics; consider including a stack trace (e.g., runtime/debug.Stack) and using a consistent prefix like "panic:" or "error:".

[Copilot]

Note that this deferred recovery won’t run for the common non-panic error path of cmd.Execute(): Execute() currently calls os.Exit(1) when rootCmd.Execute() returns an error, and os.Exit bypasses defers in main. If the goal is centralized error handling (and to preserve defers), consider changing cmd.Execute() to return an error instead of exiting, and handle the exit code in main.

Suggested change

	cmd.Execute()
	os.Exit(0)
	if err := cmd.Execute(); err != nil {
	fmt.Fprintf(os.Stderr, "Error : %v\n", err)
	os.Exit(1)
	}

[hzxuzhonghu]

We donot need to recover from panic in cli. Allowing panic is good here

[anirudh240]

@hzxuzhonghu hey thanks for the quick review and i understand should I remove all error handling entirely and let it panic, or just remove the panic recovery and keep the error checking?

[LiZhenCheng9527]

IMO, fmt.Println("Recovered:", r)

[hzxuzhonghu]

@anirudh240 Which specific error check you mean?

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The panic recovery message is printed to stdout via fmt.Println and only shows Recovered: <value>, which is easy to miss and lacks context. For CLI failure paths this should go to stderr and ideally include the panic value with a stack trace (e.g., via runtime/debug.Stack()) so crashes are actionable.

[anirudh240]

@hzxuzhonghu
I meant whether to keep the cmd.Execute() error return refactor that gemini and copilot suggested or revert the entire PR to the original code. Based on your feedback that panics are fine in CLI so I think the right call is to close this PR entirely. let me know if youd prefer otherwise