Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anirudh240 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea22558
cli: add error handling with panic recovery inside main()
anirudh240 Mar 30, 2026
28da5f4
change print statement
anirudh240 Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cli/kthena/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ package main

import (
"embed"
"fmt"
"os"

"github.com/volcano-sh/kthena/cli/kthena/cmd"
)
Expand All @@ -30,5 +32,13 @@ var templatesFS embed.FS

func main() {
cmd.InitTemplates(templatesFS)

defer func() {
if r := recover(); r != nil {
fmt.Println("Recovered:", r)
os.Exit(1)
Comment on lines +37 to +39
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
Comment on lines +36 to +40
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}()
cmd.Execute()
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

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

high

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.

os.Exit(0)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

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

medium

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.

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines 42 to +43
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

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)
}

Copilot uses AI. Check for mistakes.
}
Loading