diff --git a/pkg/engine/planetscale/apply.go b/pkg/engine/planetscale/apply.go index 8cfbeb8fb..8a97db828 100644 --- a/pkg/engine/planetscale/apply.go +++ b/pkg/engine/planetscale/apply.go @@ -49,7 +49,7 @@ func vschemaDiffsFromChanges(changes []engine.SchemaChange) []vschemaKeyspaceDif // Apply starts executing a schema change plan. // Creates a PlanetScale branch, applies DDL via MySQL connection to the branch, // then creates and starts a deploy request. -func (e *Engine) Apply(ctx context.Context, req *engine.ApplyRequest) (*engine.ApplyResult, error) { +func (e *Engine) Apply(ctx context.Context, req *engine.ApplyRequest) (result *engine.ApplyResult, retErr error) { e.logger.Info("applying plan", "plan_id", req.PlanID, "database", req.Database, @@ -128,6 +128,24 @@ func (e *Engine) Apply(ctx context.Context, req *engine.ApplyRequest) (*engine.A var branchName string branchStart := time.Now() + // A branch SchemaBot creates exists only to carry this apply's DDL into a + // deploy request. Once that deploy request exists it owns the teardown + // (AutoDeleteBranch); until then nothing does, so an apply that fails while + // preparing the branch would strand it. Branches are quota'd, and the + // failures in this window are the ordinary ones — DDL the engine refuses — + // so the strand accumulates until branch creation itself starts failing for + // unrelated schema changes. ownedBranch names the branch this apply is + // responsible for; it is cleared once the deploy request takes ownership, + // and it is never set for an operator-supplied branch, which SchemaBot does + // not own. + ownedBranch := "" + defer func() { + if retErr == nil || ownedBranch == "" { + return + } + e.deleteOwnedBranch(ctx, client, org, req.Database, ownedBranch, retErr) + }() + if existingBranch != "" { // Reuse existing branch: wait for ready, refresh schema from main, wait again branchName = existingBranch @@ -187,6 +205,7 @@ func (e *Engine) Apply(ctx context.Context, req *engine.ApplyRequest) (*engine.A if err != nil { return nil, fmt.Errorf("create branch: %w", err) } + ownedBranch = branchName // Wait for branch to be ready if err := e.waitForBranchReady(ctx, client, org, req.Database, branchName); err != nil { @@ -280,6 +299,8 @@ func (e *Engine) Apply(ctx context.Context, req *engine.ApplyRequest) (*engine.A if err != nil { return nil, fmt.Errorf("create deploy request: %w", err) } + // The deploy request now owns the branch's teardown. + ownedBranch = "" emitEvent(engine.ApplyEvent{ Message: fmt.Sprintf("Deploy request #%d created, validating...", dr.Number), Metadata: map[string]string{ diff --git a/pkg/engine/planetscale/branch_cleanup.go b/pkg/engine/planetscale/branch_cleanup.go new file mode 100644 index 000000000..7924a8679 --- /dev/null +++ b/pkg/engine/planetscale/branch_cleanup.go @@ -0,0 +1,42 @@ +package planetscale + +import ( + "context" + "time" + + ps "github.com/planetscale/planetscale-go/planetscale" + + "github.com/block/schemabot/pkg/psclient" +) + +// branchDeleteTimeout bounds the cleanup delete so a slow or unreachable +// PlanetScale API cannot hold the apply's failure path open. +const branchDeleteTimeout = 30 * time.Second + +// deleteOwnedBranch removes a branch this apply created and no deploy request +// took ownership of, so a failure while preparing the branch does not strand +// quota. cause is the failure that triggered the cleanup; it is logged with the +// outcome so the two are readable together. +// +// The delete runs on its own deadline, detached from the apply's context, so a +// cancelled or timed-out apply still cleans up after itself. A branch that could +// not be deleted is logged at error level with the identifiers needed to remove +// it by hand — an undeletable branch must be visible, not silently retried. +func (e *Engine) deleteOwnedBranch(ctx context.Context, client psclient.PSClient, org, database, branch string, cause error) { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), branchDeleteTimeout) + defer cancel() + + err := client.DeleteBranch(ctx, &ps.DeleteDatabaseBranchRequest{ + Organization: org, + Database: database, + Branch: branch, + }) + if err != nil { + e.logger.Error("failed to delete the branch left behind by a failed apply; delete it manually to reclaim branch quota", + "organization", org, "database", database, "branch", branch, + "apply_error", cause, "error", err) + return + } + e.logger.Info("deleted the branch created for an apply that failed before its deploy request", + "organization", org, "database", database, "branch", branch, "apply_error", cause) +} diff --git a/pkg/engine/planetscale/branch_cleanup_test.go b/pkg/engine/planetscale/branch_cleanup_test.go new file mode 100644 index 000000000..19734f298 --- /dev/null +++ b/pkg/engine/planetscale/branch_cleanup_test.go @@ -0,0 +1,95 @@ +package planetscale + +import ( + "context" + "errors" + "sync" + "testing" + + ps "github.com/planetscale/planetscale-go/planetscale" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/schemabot/pkg/engine" + "github.com/block/schemabot/pkg/psclient" +) + +// branchLifecycleClient serves a branch that exists and is ready, and fails the +// credential request that follows it — the shape of an apply that dies while +// preparing its branch, before any deploy request exists. Deletions are +// recorded so the test can assert on the cleanup. +type branchLifecycleClient struct { + psclient.PSClient + + mu sync.Mutex + created []string + deleted []string +} + +func (c *branchLifecycleClient) GetBranch(context.Context, *ps.GetDatabaseBranchRequest) (*ps.DatabaseBranch, error) { + return &ps.DatabaseBranch{Ready: true, SafeMigrations: true}, nil +} + +func (c *branchLifecycleClient) CreateBranch(_ context.Context, req *ps.CreateDatabaseBranchRequest) (*ps.DatabaseBranch, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.created = append(c.created, req.Name) + return &ps.DatabaseBranch{Name: req.Name, Ready: true}, nil +} + +func (c *branchLifecycleClient) DeleteBranch(_ context.Context, req *ps.DeleteDatabaseBranchRequest) error { + c.mu.Lock() + defer c.mu.Unlock() + c.deleted = append(c.deleted, req.Branch) + return nil +} + +func (c *branchLifecycleClient) RefreshSchema(context.Context, string, string, string) error { + return nil +} + +func (c *branchLifecycleClient) CreateBranchPassword(context.Context, *ps.DatabaseBranchPasswordRequest) (*ps.DatabaseBranchPassword, error) { + return nil, errors.New("branch credentials unavailable") +} + +func (c *branchLifecycleClient) snapshot() (created, deleted []string) { + c.mu.Lock() + defer c.mu.Unlock() + return append([]string(nil), c.created...), append([]string(nil), c.deleted...) +} + +// A branch SchemaBot creates is stranded if the apply fails before a deploy +// request exists to own its teardown, and PlanetScale branches are quota'd — +// the strand eventually blocks unrelated schema changes. The apply must clean up +// after itself, while leaving an operator-supplied branch alone: SchemaBot did +// not create it and must not remove it. +func TestApplyDeletesItsOwnBranchWhenItFailsBeforeTheDeployRequest(t *testing.T) { + applyRequest := func(options map[string]string) *engine.ApplyRequest { + return &engine.ApplyRequest{ + PlanID: "plan-0123456789abcdef", + Database: "commerce", + Credentials: conformanceCredentials(), + Options: options, + } + } + + t.Run("a branch this apply created is deleted", func(t *testing.T) { + client := &branchLifecycleClient{} + _, err := conformanceEngine(client).Apply(t.Context(), applyRequest(nil)) + require.Error(t, err) + + created, deleted := client.snapshot() + require.Len(t, created, 1) + assert.Equal(t, created, deleted, "the apply deletes exactly the branch it created") + }) + + t.Run("an operator-supplied branch is left alone", func(t *testing.T) { + client := &branchLifecycleClient{} + _, err := conformanceEngine(client).Apply(t.Context(), applyRequest(map[string]string{"branch": "operator-branch"})) + require.Error(t, err) + + created, deleted := client.snapshot() + assert.Empty(t, created) + assert.Empty(t, deleted) + }) +}