From be8fda90a3625686d9a2bd01fce6c919c182d376 Mon Sep 17 00:00:00 2001 From: lucarin91 Date: Tue, 11 Nov 2025 16:43:37 +0100 Subject: [PATCH] fix(update): return better errors --- cmd/arduino-app-cli/system/system.go | 8 +++- internal/api/handlers/update.go | 11 +++-- internal/update/apt/service.go | 59 ++++++++------------------ internal/update/arduino/arduino.go | 62 +++++++--------------------- internal/update/errors.go | 55 ++++++++++++++++++++++++ internal/update/event.go | 31 +++++++++++++- internal/update/update.go | 28 ++++--------- 7 files changed, 137 insertions(+), 117 deletions(-) create mode 100644 internal/update/errors.go diff --git a/cmd/arduino-app-cli/system/system.go b/cmd/arduino-app-cli/system/system.go index 54c89128..c23d5317 100644 --- a/cmd/arduino-app-cli/system/system.go +++ b/cmd/arduino-app-cli/system/system.go @@ -113,7 +113,13 @@ func newUpdateCmd() *cobra.Command { events := updater.Subscribe() for event := range events { - feedback.Printf("[%s] %s", event.Type.String(), event.Data) + if event.Type == update.ErrorEvent { + // TODO: add colors to error messages + err := event.GetError() + feedback.Printf("Error: %s [%s]", err.Error(), update.GetUpdateErrorCode(err)) + } else { + feedback.Printf("[%s] %s", event.Type.String(), event.GetData()) + } if event.Type == update.DoneEvent { break diff --git a/internal/api/handlers/update.go b/internal/api/handlers/update.go index 41ac992b..3c7475f8 100644 --- a/internal/api/handlers/update.go +++ b/internal/api/handlers/update.go @@ -128,14 +128,19 @@ func HandleUpdateEvents(updater *update.Manager) http.HandlerFunc { return } if event.Type == update.ErrorEvent { + err := event.GetError() + code := render.InternalServiceErr + if c := update.GetUpdateErrorCode(err); c != update.UnknownError { + code = render.SSEErrCode(string(c)) + } sseStream.SendError(render.SSEErrorData{ - Code: render.InternalServiceErr, - Message: event.Data, + Code: code, + Message: err.Error(), }) } else { sseStream.Send(render.SSEEvent{ Type: event.Type.String(), - Data: event.Data, + Data: event.GetData(), }) } diff --git a/internal/update/apt/service.go b/internal/update/apt/service.go index f3d3984e..81dcbb13 100644 --- a/internal/update/apt/service.go +++ b/internal/update/apt/service.go @@ -25,7 +25,6 @@ import ( "regexp" "strings" "sync" - "time" "github.com/arduino/go-paths-helper" "go.bug.st/f" @@ -84,79 +83,55 @@ func (s *Service) UpgradePackages(ctx context.Context, names []string) (<-chan u defer s.lock.Unlock() defer close(eventsCh) - ctx, cancel := context.WithTimeout(ctx, 10*time.Minute) - defer cancel() - - eventsCh <- update.Event{Type: update.StartEvent, Data: "Upgrade is starting"} + eventsCh <- update.NewDataEvent(update.StartEvent, "Upgrade is starting") stream := runUpgradeCommand(ctx, names) for line, err := range stream { if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error running upgrade command", - } - slog.Error("error processing upgrade command output", "error", err) + eventsCh <- update.NewErrorEvent(fmt.Errorf("error running upgrade command: %w", err)) return } - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: line} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, line) } - eventsCh <- update.Event{Type: update.StartEvent, Data: "apt cleaning cache is starting"} + + eventsCh <- update.NewDataEvent(update.StartEvent, "apt cleaning cache is starting") for line, err := range runAptCleanCommand(ctx) { if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error running apt clean command", - } - slog.Error("error processing apt clean command output", "error", err) + eventsCh <- update.NewErrorEvent(fmt.Errorf("error running apt clean command: %w", err)) return } - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: line} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, line) } - // TEMPORARY PATCH: stopping and destroying docker containers and images since IDE does not implement it yet. - // TODO: Remove this workaround once IDE implements it. - // Tracking issue: https://github.com/arduino/arduino-app-cli/issues/623 - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: "Stop and destroy docker containers and images ..."} + + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, "Apt upgrade completed successfully.") streamCleanup := cleanupDockerContainers(ctx) for line, err := range streamCleanup { if err != nil { // TODO: maybe we should retun an error or a better feedback to the user? // currently, we just log the error and continue considenring not blocking - slog.Error("Error stopping and destroying docker containers", "error", err) + slog.Warn("Error stopping and destroying docker containers", "error", err) + } else { + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, line) } - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: line} } - // TEMPORARY PATCH: Install the latest docker images and show the logs to the users. // TODO: Remove this workaround once docker image versions are no longer hardcoded in arduino-app-cli. // Tracking issue: https://github.com/arduino/arduino-app-cli/issues/600 // Currently, we need to launch `arduino-app-cli system init` to pull the latest docker images because // the version of the docker images are hardcoded in the (new downloaded) version of the arduino-app-cli. - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: "Pulling the latest docker images ..."} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, "Pulling the latest docker images ...") streamDocker := pullDockerImages(ctx) for line, err := range streamDocker { if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error upgrading docker images", - } - slog.Error("error upgrading docker images", "error", err) + eventsCh <- update.NewErrorEvent(fmt.Errorf("error pulling docker images: %w", err)) return } - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: line} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, line) } - eventsCh <- update.Event{Type: update.RestartEvent, Data: "Upgrade completed. Restarting ..."} + eventsCh <- update.NewDataEvent(update.RestartEvent, "Upgrade completed. Restarting ...") err := restartServices(ctx) if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error restart services after upgrade", - } - slog.Error("failed to restart services", "error", err) + eventsCh <- update.NewErrorEvent(fmt.Errorf("error restarting services after upgrade: %w", err)) return } }() diff --git a/internal/update/arduino/arduino.go b/internal/update/arduino/arduino.go index 01076dff..0c4e5d29 100644 --- a/internal/update/arduino/arduino.go +++ b/internal/update/arduino/arduino.go @@ -18,9 +18,9 @@ package arduino import ( "context" "errors" + "fmt" "log/slog" "sync" - "time" "github.com/arduino/arduino-cli/commands" "github.com/arduino/arduino-cli/commands/cmderrors" @@ -134,42 +134,31 @@ func (a *ArduinoPlatformUpdater) UpgradePackages(ctx context.Context, names []st downloadProgressCB := func(curr *rpc.DownloadProgress) { data := helpers.ArduinoCLIDownloadProgressToString(curr) slog.Debug("Download progress", slog.String("download_progress", data)) - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: data} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, data) } taskProgressCB := func(msg *rpc.TaskProgress) { data := helpers.ArduinoCLITaskProgressToString(msg) slog.Debug("Task progress", slog.String("task_progress", data)) - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: data} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, data) } go func() { defer a.lock.Unlock() defer close(eventsCh) - ctx, cancel := context.WithTimeout(ctx, 10*time.Minute) - defer cancel() - - eventsCh <- update.Event{Type: update.StartEvent, Data: "Upgrade is starting"} + eventsCh <- update.NewDataEvent(update.StartEvent, "Upgrade is starting") logrus.SetLevel(logrus.ErrorLevel) // Reduce the log level of arduino-cli srv := commands.NewArduinoCoreServer() if err := setConfig(ctx, srv); err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error setting additional URLs", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error setting config: %w", err)) return } var inst *rpc.Instance if resp, err := srv.Create(ctx, &rpc.CreateRequest{}); err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error creating Arduino instance", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error creating arduino-cli instance: %w", err)) return } else { inst = resp.GetInstance() @@ -185,19 +174,11 @@ func (a *ArduinoPlatformUpdater) UpgradePackages(ctx context.Context, names []st { stream, _ := commands.UpdateIndexStreamResponseToCallbackFunction(ctx, downloadProgressCB) if err := srv.UpdateIndex(&rpc.UpdateIndexRequest{Instance: inst}, stream); err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error updating index", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error updating index: %w", err)) return } if err := srv.Init(&rpc.InitRequest{Instance: inst}, commands.InitStreamResponseToCallbackFunction(ctx, nil)); err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error initializing Arduino instance", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error initializing instance: %w", err)) return } } @@ -219,17 +200,13 @@ func (a *ArduinoPlatformUpdater) UpgradePackages(ctx context.Context, names []st ); err != nil { var alreadyPresent *cmderrors.PlatformAlreadyAtTheLatestVersionError if errors.As(err, &alreadyPresent) { - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: alreadyPresent.Error()} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, alreadyPresent.Error()) return } var notFound *cmderrors.PlatformNotFoundError if !errors.As(err, ¬Found) { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error upgrading platform", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error upgrading platform: %w", err)) return } // If the platform is not found, we will try to install it @@ -246,23 +223,16 @@ func (a *ArduinoPlatformUpdater) UpgradePackages(ctx context.Context, names []st ), ) if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error installing platform", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error installing platform: %w", err)) return } } else if respCB().GetPlatform() == nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Data: "platform upgrade failed", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("platform upgrade failed")) return } cbw := orchestrator.NewCallbackWriter(func(line string) { - eventsCh <- update.Event{Type: update.UpgradeLineEvent, Data: line} + eventsCh <- update.NewDataEvent(update.UpgradeLineEvent, line) }) err := srv.BurnBootloader( @@ -274,11 +244,7 @@ func (a *ArduinoPlatformUpdater) UpgradePackages(ctx context.Context, names []st commands.BurnBootloaderToServerStreams(ctx, cbw, cbw), ) if err != nil { - eventsCh <- update.Event{ - Type: update.ErrorEvent, - Err: err, - Data: "Error burning bootloader", - } + eventsCh <- update.NewErrorEvent(fmt.Errorf("error burning bootloader: %w", err)) return } }() diff --git a/internal/update/errors.go b/internal/update/errors.go new file mode 100644 index 00000000..c81e50e9 --- /dev/null +++ b/internal/update/errors.go @@ -0,0 +1,55 @@ +package update + +import "errors" + +type ErrorCode string + +const ( + NoInternetConnection ErrorCode = "NO_INTERNET_CONNECTION" + OperationInProgress ErrorCode = "OPERATION_IN_PROGRESS" + UnknownError ErrorCode = "UNKNOWN_ERROR" +) + +var ( + ErrOperationAlreadyInProgress = &UpdateError{ + Code: OperationInProgress, + Details: " an operation is already in progress", + } + ErrNoInternetConnection = &UpdateError{ + Code: NoInternetConnection, + Details: "no internet connection available", + } +) + +type UpdateError struct { + Code ErrorCode `json:"code"` + Details string `json:"details"` + + err error +} + +func (e *UpdateError) Error() string { + return e.Details +} + +func (e *UpdateError) Unwrap() error { + return e.err +} + +func NewUnkownError(err error) *UpdateError { + return &UpdateError{ + Code: "UNKNOWN_ERROR", + Details: err.Error(), + err: err, + } +} + +func GetUpdateErrorCode(err error) ErrorCode { + var updateError *UpdateError + if errors.As(err, &updateError) { + if updateError.Code != "" { + return updateError.Code + } + } + return UnknownError +} diff --git a/internal/update/event.go b/internal/update/event.go index 0f6c1a51..2aac04ee 100644 --- a/internal/update/event.go +++ b/internal/update/event.go @@ -15,6 +15,8 @@ package update +import "go.bug.st/f" + // EventType defines the type of upgrade event. type EventType int @@ -29,8 +31,9 @@ const ( // Event represents a single event in the upgrade process. type Event struct { Type EventType - Data string - Err error // Optional error field for error events + + data string + err error // error field for error events } func (t EventType) String() string { @@ -50,6 +53,30 @@ func (t EventType) String() string { } } +func NewDataEvent(t EventType, data string) Event { + return Event{ + Type: t, + data: data, + } +} + +func NewErrorEvent(err error) Event { + return Event{ + Type: ErrorEvent, + err: err, + } +} + +func (e Event) GetData() string { + f.Assert(e.Type != ErrorEvent, "not a data event") + return e.data +} + +func (e Event) GetError() error { + f.Assert(e.Type == ErrorEvent, "not an error event") + return e.err +} + type PackageType string const ( diff --git a/internal/update/update.go b/internal/update/update.go index 7a254478..f4b95ea4 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -17,7 +17,6 @@ package update import ( "context" - "errors" "fmt" "log/slog" "net/http" @@ -28,8 +27,6 @@ import ( "golang.org/x/sync/errgroup" ) -var ErrOperationAlreadyInProgress = errors.New("an operation is already in progress") - var MatchArduinoPackage = func(p UpgradablePackage) bool { return strings.HasPrefix(p.Name, "arduino-") || (p.Name == "adbd" && strings.Contains(p.ToVersion, "arduino")) // NOTE: changing this check could remove the adbd package, breaking the device access. @@ -78,7 +75,7 @@ func (m *Manager) ListUpgradablePackages(ctx context.Context, matcher func(Upgra // Make sure to be connected to the internet, before checking for updates. // This is needed because the checks below work also when offline (using cached data). if !isConnected() { - return nil, errors.New("no internet connectivity") + return nil, ErrNoInternetConnection } // Get the list of upgradable packages from two sources (deb and platform) in parallel. @@ -141,12 +138,7 @@ func (m *Manager) UpgradePackages(ctx context.Context, pkgs []UpgradablePackage) // in the middle the upgrade of the cores. arduinoEvents, err := m.arduinoPlatformUpdateService.UpgradePackages(ctx, arduinoPlatform) if err != nil { - m.broadcast( - Event{ - Type: ErrorEvent, - Data: "failed to upgrade Arduino packages", - Err: err, - }) + m.broadcast(NewErrorEvent(fmt.Errorf("failed to upgrade Arduino packages: %w", err))) return } for e := range arduinoEvents { @@ -155,18 +147,13 @@ func (m *Manager) UpgradePackages(ctx context.Context, pkgs []UpgradablePackage) aptEvents, err := m.debUpdateService.UpgradePackages(ctx, debPkgs) if err != nil { - m.broadcast( - Event{ - Type: ErrorEvent, - Data: "failed to upgrade APT packages", - Err: err, - }) + m.broadcast(NewErrorEvent(fmt.Errorf("failed to upgrade APT packages: %w", err))) return } for e := range aptEvents { m.broadcast(e) } - m.broadcast(Event{Type: DoneEvent, Data: "Upgrade completed successfully"}) + m.broadcast(NewDataEvent(DoneEvent, "Upgrade completed successfully")) }() return nil } @@ -175,17 +162,17 @@ func (m *Manager) UpgradePackages(ctx context.Context, pkgs []UpgradablePackage) func (b *Manager) Subscribe() chan Event { eventCh := make(chan Event, 100) b.mu.Lock() + defer b.mu.Unlock() b.subs[eventCh] = struct{}{} - b.mu.Unlock() return eventCh } // Unsubscribe removes the channel from the list of subscribers and closes it. func (b *Manager) Unsubscribe(eventCh chan Event) { b.mu.Lock() + defer b.mu.Unlock() delete(b.subs, eventCh) close(eventCh) - b.mu.Unlock() } func (b *Manager) broadcast(event Event) { @@ -201,8 +188,7 @@ func (b *Manager) broadcast(event Event) { default: slog.Warn("Discarding event (channel full)", slog.String("type", event.Type.String()), - slog.String("data", fmt.Sprintf("%v", event.Data)), - slog.Any("error", event.Err), + slog.Any("event", event), ) } }