Re: [PR] feat: implement panic diagnostics and FODC crash reporting pipeline [skywalking-banyandb]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #1084:
URL: https://github.com/apache/skywalking-banyandb/pull/1084#discussion_r3223294064


##########
pkg/cmdsetup/data.go:
##########
@@ -111,8 +111,11 @@ func newDataCmd(runners ...run.Unit) *cobra.Command {
 				return err
 			}
 			logger.GetLogger().Info().Msg("starting as a data server")
-			// Spawn our go routines and wait for shutdown.
-			if err := dataGroup.Run(context.WithValue(context.Background(), common.ContextNodeKey, node)); err != nil {
+			// Spawn our go routines and wait for shutdown. The Run context is
+			// derived from SupervisorContext so that a panic recovered anywhere
+			// in the process, including goroutines spawned via run.Go, fires
+			// cancellation here.
+			if err := dataGroup.Run(context.WithValue(SupervisorContext(), common.ContextNodeKey, node)); err != nil {
 				logger.GetLogger().Error().Err(err).Stack().Str("name", dataGroup.Name()).Msg("Exit")
 				os.Exit(-1)
 			}

Review Comment:
   This RunE path calls os.Exit(-1) on error, which bypasses Cobra’s PersistentPostRunE. Since root.go now uses PersistentPostRunE to call ShutdownSupervisor() (draining the panic artifact sink), os.Exit here can drop queued artifacts on error exits. Prefer returning the error (letting Cobra unwind and run post-run hooks) or explicitly calling ShutdownSupervisor() before exiting.



##########
pkg/timestamp/scheduler.go:
##########
@@ -39,9 +40,10 @@ var (
 	ErrTaskDuplicated = errors.New("the task is duplicated")
 )
 
-// SchedulerAction is an executable when a trigger is fired
-// now is the trigger time, logger has a context indicating the task's identity.
-type SchedulerAction func(now time.Time, logger *logger.Logger) bool
+// SchedulerAction is an executable when a trigger is fired.
+// ctx is canceled when the scheduler is closed, now is the trigger time,
+// and logger carries the task's identity.
+type SchedulerAction func(ctx context.Context, now time.Time, logger *logger.Logger) bool

Review Comment:
   SchedulerAction’s doc says the passed ctx is canceled when the scheduler is closed, but Register stores the caller ctx in task.parentCtx and Close() only closes t.closer (it never cancels parentCtx). As a result, action goroutines started via run.GoWithSignal may keep running after Scheduler.Close(), and callers relying on ctx cancellation won’t observe shutdown. Consider deriving a task context via context.WithCancel (or using t.closer.Ctx()) and canceling it in task.close(), and update run.Go / GoWithSignal to use that derived ctx.



##########
pkg/cmdsetup/standalone.go:
##########
@@ -118,8 +118,11 @@ func newStandaloneCmd(runners ...run.Unit) *cobra.Command {
 				return err
 			}
 			logger.GetLogger().Info().Msg("starting as a standalone server")
-			// Spawn our go routines and wait for shutdown.
-			if err := standaloneGroup.Run(context.WithValue(context.Background(), common.ContextNodeKey, nodeID)); err != nil {
+			// Spawn our go routines and wait for shutdown. The Run context is
+			// derived from SupervisorContext so that a panic recovered anywhere
+			// in the process, including goroutines spawned via run.Go, fires
+			// cancellation here.
+			if err := standaloneGroup.Run(context.WithValue(SupervisorContext(), common.ContextNodeKey, nodeID)); err != nil {
 				logger.GetLogger().Error().Err(err).Stack().Str("name", standaloneGroup.Name()).Msg("Exit")
 				os.Exit(-1)
 			}

Review Comment:
   This RunE path calls os.Exit(-1) on error, which bypasses Cobra’s PersistentPostRunE. Since root.go now uses PersistentPostRunE to call ShutdownSupervisor() (draining the panic artifact sink), os.Exit here can drop queued artifacts on error exits. Prefer returning the error (letting Cobra unwind and run post-run hooks) or explicitly calling ShutdownSupervisor() before exiting.



##########
pkg/cmdsetup/liaison.go:
##########
@@ -157,9 +160,9 @@ func newLiaisonCmd(runners ...run.Unit) *cobra.Command {
 				return err
 			}
 			logger.GetLogger().Info().Msg("starting as a liaison server")
-			ctx = context.WithValue(ctx, common.ContextNodeKey, node)
+			runCtx = context.WithValue(runCtx, common.ContextNodeKey, node)
 			// Spawn our go routines and wait for shutdown.
-			if err := liaisonGroup.Run(ctx); err != nil {
+			if err := liaisonGroup.Run(runCtx); err != nil {
 				logger.GetLogger().Error().Err(err).Stack().Str("name", liaisonGroup.Name()).Msg("Exit")
 				os.Exit(-1)
 			}

Review Comment:
   This RunE path calls os.Exit(-1) on error, which bypasses Cobra’s PersistentPostRunE. Since root.go now uses PersistentPostRunE to call ShutdownSupervisor() (draining the panic artifact sink), os.Exit here can drop queued artifacts on error exits. Prefer returning the error (letting Cobra unwind and run post-run hooks) or explicitly calling ShutdownSupervisor() before exiting.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org