From 8cdde569bf355841db8aada67fbe72b4ce98a656 Mon Sep 17 00:00:00 2001
From: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
Date: Tue, 29 Aug 2023 19:02:08 -0400
Subject: [PATCH] fix(core): improve error handling while waiting for PVE tasks
 to complete

---
 proxmox/api/client.go        | 12 ++++++------
 proxmox/api/errors.go        | 29 +++++++++++++++++++++++++++++
 proxmox/nodes/storage.go     |  1 -
 proxmox/nodes/tasks/tasks.go | 19 +++++++++++++++++--
 4 files changed, 52 insertions(+), 9 deletions(-)
 create mode 100644 proxmox/api/errors.go

diff --git a/proxmox/api/client.go b/proxmox/api/client.go
index 01e3d560e..0411a8e85 100644
--- a/proxmox/api/client.go
+++ b/proxmox/api/client.go
@@ -25,9 +25,6 @@ import (
 	"github.com/bpg/terraform-provider-proxmox/utils"
 )
 
-// ErrNoDataObjectInResponse is returned when the server does not include a data object in the response.
-var ErrNoDataObjectInResponse = errors.New("the server did not include a data object in the response")
-
 const (
 	basePathJSONAPI = "api2/json"
 )
@@ -292,7 +289,7 @@ func (c *client) IsRootTicket() bool {
 // validateResponseCode ensures that a response is valid.
 func validateResponseCode(res *http.Response) error {
 	if res.StatusCode < 200 || res.StatusCode >= 300 {
-		status := strings.TrimPrefix(res.Status, fmt.Sprintf("%d ", res.StatusCode))
+		msg := strings.TrimPrefix(res.Status, fmt.Sprintf("%d ", res.StatusCode))
 
 		errRes := &ErrorResponseBody{}
 		err := json.NewDecoder(res.Body).Decode(errRes)
@@ -304,10 +301,13 @@ func validateResponseCode(res *http.Response) error {
 				errList = append(errList, fmt.Sprintf("%s: %s", k, strings.TrimRight(v, "\n\r")))
 			}
 
-			status = fmt.Sprintf("%s (%s)", status, strings.Join(errList, " - "))
+			msg = fmt.Sprintf("%s (%s)", msg, strings.Join(errList, " - "))
 		}
 
-		return fmt.Errorf("received an HTTP %d response - Reason: %s", res.StatusCode, status)
+		return &HTTPError{
+			Code:    res.StatusCode,
+			Message: msg,
+		}
 	}
 
 	return nil
diff --git a/proxmox/api/errors.go b/proxmox/api/errors.go
new file mode 100644
index 000000000..75f5446ac
--- /dev/null
+++ b/proxmox/api/errors.go
@@ -0,0 +1,29 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+package api
+
+import "fmt"
+
+// Error is a sentinel error type for API errors.
+type Error string
+
+func (err Error) Error() string {
+	return string(err)
+}
+
+// ErrNoDataObjectInResponse is returned when the server does not include a data object in the response.
+const ErrNoDataObjectInResponse Error = "the server did not include a data object in the response"
+
+// HTTPError is a generic error type for HTTP errors.
+type HTTPError struct {
+	Code    int
+	Message string
+}
+
+func (err *HTTPError) Error() string {
+	return fmt.Sprintf("received an HTTP %d response - Reason: %s", err.Code, err.Message)
+}
diff --git a/proxmox/nodes/storage.go b/proxmox/nodes/storage.go
index 609d98796..bf4e6a9da 100644
--- a/proxmox/nodes/storage.go
+++ b/proxmox/nodes/storage.go
@@ -282,7 +282,6 @@ func (c *Client) APIUpload(
 	}
 
 	err = c.Tasks().WaitForTask(ctx, *resBody.UploadID, uploadTimeout, 5)
-
 	if err != nil {
 		return nil, fmt.Errorf("error uploading file to datastore %s: failed waiting for upload - %w", datastoreID, err)
 	}
diff --git a/proxmox/nodes/tasks/tasks.go b/proxmox/nodes/tasks/tasks.go
index 3e7e0a96f..7bccd86f5 100644
--- a/proxmox/nodes/tasks/tasks.go
+++ b/proxmox/nodes/tasks/tasks.go
@@ -8,6 +8,7 @@ package tasks
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -28,7 +29,7 @@ func (c *Client) GetTaskStatus(ctx context.Context, upid string) (*GetTaskStatus
 		resBody,
 	)
 	if err != nil {
-		return nil, fmt.Errorf("error retrievinf task status: %w", err)
+		return nil, fmt.Errorf("error retrieving task status: %w", err)
 	}
 
 	if resBody.Data == nil {
@@ -45,10 +46,24 @@ func (c *Client) WaitForTask(ctx context.Context, upid string, timeoutSec, delay
 	timeStart := time.Now()
 	timeElapsed := timeStart.Sub(timeStart)
 
+	isCriticalError := func(err error) bool {
+		var target *api.HTTPError
+		if errors.As(err, &target) {
+			if target.Code != http.StatusBadRequest {
+				// this is a special case to account for eventual consistency
+				// when creating a task -- the task may not be available via status API
+				// immediately after creation
+				return true
+			}
+		}
+
+		return err != nil
+	}
+
 	for timeElapsed.Seconds() < timeMax {
 		if int64(timeElapsed.Seconds())%timeDelay == 0 {
 			status, err := c.GetTaskStatus(ctx, upid)
-			if err != nil {
+			if isCriticalError(err) {
 				return err
 			}