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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
