Skip to content

Commit 587b767

Browse files
committed
fix: prevent cancelled builds from executing by checking updateStatus result
Fixed a race condition where a build could still execute after being cancelled. The issue was: 1. runBuild checks if build is in terminal state 2. CancelBuild is called, setting status to "cancelled" 3. runBuild calls updateStatus(StatusBuilding) which silently returns 4. runBuild proceeds to executeBuild, wasting compute resources The fix: - updateStatus now returns bool indicating if the update was applied - runBuild checks this return value and aborts if false - This prevents creating builder VMs for already-cancelled builds
1 parent 6fbb831 commit 587b767

1 file changed

Lines changed: 12 additions & 5 deletions

File tree

lib/builds/manager.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,12 @@ func (m *manager) runBuild(ctx context.Context, id string, req CreateBuildReques
268268
return
269269
}
270270

271-
// Update status to building (will be skipped if already terminal)
272-
m.updateStatus(id, StatusBuilding, nil)
271+
// Update status to building - if this returns false, the build was cancelled
272+
// between our check above and now, so we should abort to avoid wasting resources
273+
if !m.updateStatus(id, StatusBuilding, nil) {
274+
m.logger.Info("build status update failed (likely cancelled), aborting", "id", id)
275+
return
276+
}
273277

274278
// Create timeout context
275279
buildCtx, cancel := context.WithTimeout(ctx, time.Duration(policy.TimeoutSeconds)*time.Second)
@@ -603,18 +607,19 @@ func (c *bufferedConn) Read(p []byte) (int, error) {
603607

604608
// updateStatus updates the build status
605609
// It checks for terminal states to prevent race conditions (e.g., cancelled build being overwritten)
606-
func (m *manager) updateStatus(id string, status string, err error) {
610+
// Returns true if the status was updated, false if skipped (e.g., build already in terminal state)
611+
func (m *manager) updateStatus(id string, status string, err error) bool {
607612
meta, readErr := readMetadata(m.paths, id)
608613
if readErr != nil {
609614
m.logger.Error("read metadata for status update", "id", id, "error", readErr)
610-
return
615+
return false
611616
}
612617

613618
// Don't overwrite terminal states - prevents race condition where cancelled
614619
// build gets overwritten by a concurrent goroutine setting it to building
615620
if isTerminalStatus(meta.Status) {
616621
m.logger.Debug("skipping status update for terminal build", "id", id, "current", meta.Status, "requested", status)
617-
return
622+
return false
618623
}
619624

620625
meta.Status = status
@@ -629,10 +634,12 @@ func (m *manager) updateStatus(id string, status string, err error) {
629634

630635
if writeErr := writeMetadata(m.paths, meta); writeErr != nil {
631636
m.logger.Error("write metadata for status update", "id", id, "error", writeErr)
637+
return false
632638
}
633639

634640
// Notify subscribers of status change
635641
m.notifyStatusChange(id, status)
642+
return true
636643
}
637644

638645
// isTerminalStatus returns true if the status represents a completed build

0 commit comments

Comments
 (0)