Aborting tx if prebuild creation/deletion fails

dannykopping · dannykopping · commit b32197937f81 · 2025-02-14T12:17:12.000Z
Signed-off-by: Danny Kopping &lt;danny@coder.com&gt;
diff --git a/coderd/prebuilds/controller.go b/coderd/prebuilds/controller.go
@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/go-multierror"
 	"golang.org/x/exp/slices"
 
 	"github.com/coder/coder/v2/coderd/audit"
@@ -297,6 +298,7 @@ func (c Controller) reconcileTemplate(ctx context.Context, template database.Tem
 			return xerrors.Errorf("failed to retrieve template's prebuild states: %w", err)
 		}
 
+		var lastErr multierror.Error
 		for _, state := range versionStates {
 			vlogger := logger.With(slog.F("template_version_id", state.TemplateVersionID))
 
@@ -329,20 +331,24 @@ func (c Controller) reconcileTemplate(ctx context.Context, template database.Tem
 			// i.e. we hold the advisory lock until all reconciliatory actions have been taken.
 			// TODO: max per reconciliation iteration?
 
+			// TODO: probably need to split these to have a transaction each... rolling back would lead to an
+			// 		 inconsistent state if 1 of n creations/deletions fail.
 			for _, id := range actions.createIDs {
 				if err := c.createPrebuild(ownerCtx, db, id, template); err != nil {
 					vlogger.Error(ctx, "failed to create prebuild", slog.Error(err))
+					lastErr.Errors = append(lastErr.Errors, err)
 				}
 			}
 
 			for _, id := range actions.deleteIDs {
 				if err := c.deletePrebuild(ownerCtx, db, id, template); err != nil {
 					vlogger.Error(ctx, "failed to delete prebuild", slog.Error(err))
+					lastErr.Errors = append(lastErr.Errors, err)
 				}
 			}
 		}
 
-		return nil
+		return lastErr.ErrorOrNil()
 	}, &database.TxOptions{
 		// TODO: isolation
 		TxIdentifier: "template_prebuilds",

Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,7 @@ import (`
`10`	`10`	`"strings"`
`11`	`11`	`"time"`
`12`	`12`
	`13`	`+ "github.com/hashicorp/go-multierror"`
`13`	`14`	`"golang.org/x/exp/slices"`
`14`	`15`
`15`	`16`	`"github.com/coder/coder/v2/coderd/audit"`
`@@ -297,6 +298,7 @@ func (c Controller) reconcileTemplate(ctx context.Context, template database.Tem`
`297`	`298`	`return xerrors.Errorf("failed to retrieve template's prebuild states: %w", err)`
`298`	`299`	`}`
`299`	`300`
	`301`	`+ var lastErr multierror.Error`
`300`	`302`	`for _, state := range versionStates {`
`301`	`303`	`vlogger := logger.With(slog.F("template_version_id", state.TemplateVersionID))`
`302`	`304`
`@@ -329,20 +331,24 @@ func (c Controller) reconcileTemplate(ctx context.Context, template database.Tem`
`329`	`331`	`// i.e. we hold the advisory lock until all reconciliatory actions have been taken.`
`330`	`332`	`// TODO: max per reconciliation iteration?`
`331`	`333`
	`334`	`+ // TODO: probably need to split these to have a transaction each... rolling back would lead to an`
	`335`	`+ // inconsistent state if 1 of n creations/deletions fail.`
`332`	`336`	`for _, id := range actions.createIDs {`
`333`	`337`	`if err := c.createPrebuild(ownerCtx, db, id, template); err != nil {`
`334`	`338`	`vlogger.Error(ctx, "failed to create prebuild", slog.Error(err))`
	`339`	`+ lastErr.Errors = append(lastErr.Errors, err)`
`335`	`340`	`}`
`336`	`341`	`}`
`337`	`342`
`338`	`343`	`for _, id := range actions.deleteIDs {`
`339`	`344`	`if err := c.deletePrebuild(ownerCtx, db, id, template); err != nil {`
`340`	`345`	`vlogger.Error(ctx, "failed to delete prebuild", slog.Error(err))`
	`346`	`+ lastErr.Errors = append(lastErr.Errors, err)`
`341`	`347`	`}`
`342`	`348`	`}`
`343`	`349`	`}`
`344`	`350`
`345`		`- return nil`
	`351`	`+ return lastErr.ErrorOrNil()`
`346`	`352`	`}, &database.TxOptions{`
`347`	`353`	`// TODO: isolation`
`348`	`354`	`TxIdentifier: "template_prebuilds",`