Skip to content

Commit 88a0f9a

Browse files
aparajonampagent
andcommitted
fix(rollback): plan from requested apply state
Rollback now plans from the exact completed apply being rolled back instead of whichever completed apply happens to be latest for the PR/database. It stores whether original schema capture succeeded separately from the schema map so create-table rollbacks can target an empty original schema, while older plans without capture still fail closed. The confirmation path also keeps rollback locks distinct from normal apply locks, routes observers through the stored rollback deployment, makes rollback-confirm database-explicit, and rejects unsupported planning-time defer-cutover flags. Vitess VSchema rollback remains fail-closed until original VSchema capture is supported. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019eb380-ca81-749e-94a2-cb4982ed90dd
1 parent d18b950 commit 88a0f9a

36 files changed

Lines changed: 1177 additions & 445 deletions

pkg/api/control_handlers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1685,7 +1685,7 @@ func (s *Service) handleRollbackPlan(w http.ResponseWriter, r *http.Request) {
16851685
return
16861686
}
16871687

1688-
resp, err := s.ExecuteRollbackPlan(r.Context(), apply.Database, apply.Environment, apply.Deployment)
1688+
resp, err := s.ExecuteRollbackPlanForApply(r.Context(), apply)
16891689
if err != nil {
16901690
metrics.RecordControlOperation(r.Context(), "rollback_plan", apply.Database, apply.Deployment, apply.Environment, "error")
16911691
s.writeControlError(w, "rollback plan", apply, err)

pkg/api/handlers_test.go

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -596,9 +596,6 @@ func (m *mockTernClient) SkipRevert(ctx context.Context, req *ternv1.SkipRevertR
596596
}
597597
return nil, m.skipRevertErr
598598
}
599-
func (m *mockTernClient) RollbackPlan(ctx context.Context, database, environment string) (*ternv1.PlanResponse, error) {
600-
return nil, nil
601-
}
602599
func (m *mockTernClient) ResumeApply(ctx context.Context, apply *storage.Apply) error {
603600
m.resumeMu.Lock()
604601
m.resumeApply = apply
@@ -3656,3 +3653,47 @@ func TestDeriveErrorCode(t *testing.T) {
36563653
})
36573654
}
36583655
}
3656+
3657+
func TestRollbackSchemaFilesAllowsCapturedEmptyOriginalSchema(t *testing.T) {
3658+
plan := &storage.Plan{
3659+
Namespaces: map[string]*storage.NamespacePlanData{
3660+
"shop": {
3661+
OriginalSchema: map[string]string{},
3662+
OriginalSchemaCaptured: true,
3663+
},
3664+
},
3665+
}
3666+
3667+
schemaFiles, err := rollbackSchemaFiles(plan)
3668+
require.NoError(t, err)
3669+
require.Contains(t, schemaFiles, "shop")
3670+
assert.Empty(t, schemaFiles["shop"].Files)
3671+
}
3672+
3673+
func TestRollbackSchemaFilesRejectsLegacyPlanWithoutCapturedOriginalSchema(t *testing.T) {
3674+
plan := &storage.Plan{
3675+
Namespaces: map[string]*storage.NamespacePlanData{
3676+
"shop": {
3677+
OriginalSchema: map[string]string{},
3678+
},
3679+
},
3680+
}
3681+
3682+
schemaFiles, err := rollbackSchemaFiles(plan)
3683+
require.Error(t, err)
3684+
assert.Nil(t, schemaFiles)
3685+
assert.Contains(t, err.Error(), `no original schema available for rollback namespace "shop"`)
3686+
}
3687+
3688+
func TestPlanContainsVSchemaChange(t *testing.T) {
3689+
assert.True(t, planContainsVSchemaChange(&storage.Plan{
3690+
Namespaces: map[string]*storage.NamespacePlanData{
3691+
"commerce": {VSchema: []byte(`{"tables":{"users":{}}}`)},
3692+
},
3693+
}))
3694+
assert.False(t, planContainsVSchemaChange(&storage.Plan{
3695+
Namespaces: map[string]*storage.NamespacePlanData{
3696+
"commerce": {OriginalSchemaCaptured: true},
3697+
},
3698+
}))
3699+
}

pkg/api/plan_handlers.go

Lines changed: 133 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -466,27 +466,56 @@ func (s *Service) ExecutePlan(ctx context.Context, req PlanRequest) (*apitypes.P
466466
}
467467
}
468468

469-
// Store plan in SchemaBot's storage (idempotent — duplicate is ignored)
469+
route := storedPlanRoute{
470+
DatabaseType: resolvedTarget.DatabaseType,
471+
Deployment: deployment,
472+
Target: resolvedTarget.Target,
473+
}
474+
if err := s.storePlanResponse(ctx, req, resp, route); err != nil {
475+
return nil, err
476+
}
477+
478+
return planResponseFromProto(resp), nil
479+
}
480+
481+
type storedPlanRoute struct {
482+
DatabaseType string
483+
Deployment string
484+
Target string
485+
}
486+
487+
func (s *Service) storePlanResponse(ctx context.Context, req PlanRequest, resp *ternv1.PlanResponse, route storedPlanRoute) error {
488+
prInt := 0
489+
if req.PullRequest != nil {
490+
prInt = int(*req.PullRequest)
491+
}
492+
trustedSchemaPath := ""
493+
if req.SourceTrusted {
494+
trustedSchemaPath = req.SchemaPath
495+
}
496+
headSHA := ""
497+
if req.HeadSHA != nil {
498+
headSHA = *req.HeadSHA
499+
}
470500
storedPlan := &storage.Plan{
471501
PlanIdentifier: resp.PlanId,
472502
Database: req.Database,
473-
DatabaseType: resolvedTarget.DatabaseType,
474-
Deployment: deployment,
475-
Target: resolvedTarget.Target,
503+
DatabaseType: route.DatabaseType,
504+
Deployment: route.Deployment,
505+
Target: route.Target,
476506
Repository: req.Repository,
477507
PullRequest: prInt,
478508
SchemaPath: trustedSchemaPath,
479509
Environment: req.Environment,
480510
SchemaFiles: protoToSchemaFiles(req.SchemaFiles),
481-
Namespaces: protoChangesToNamespaces(resp.Changes),
482-
HeadSHA: ternReq.HeadSha,
511+
Namespaces: protoChangesToNamespaces(resp.Changes, req.SchemaFiles),
512+
HeadSHA: headSHA,
483513
CreatedAt: time.Now(),
484514
}
485515
if _, err := s.storage.Plans().Create(ctx, storedPlan); err != nil && !errors.Is(err, storage.ErrPlanIDExists) {
486-
return nil, fmt.Errorf("store plan: %w", err)
516+
return fmt.Errorf("store plan: %w", err)
487517
}
488-
489-
return planResponseFromProto(resp), nil
518+
return nil
490519
}
491520

492521
// handleApply handles POST /api/apply requests.
@@ -899,29 +928,114 @@ func applyTaskChanges(plan *storage.Plan) []storage.TableChange {
899928
return changes
900929
}
901930

902-
// ExecuteRollbackPlan generates a rollback plan via the Tern client.
903-
// The plan is automatically stored by the Tern client's RollbackPlan method
904-
// (which calls Plan internally). This is the shared implementation used by
905-
// both the HTTP handler and the webhook handler.
906-
func (s *Service) ExecuteRollbackPlan(ctx context.Context, database, environment, deployment string) (*apitypes.PlanResponse, error) {
907-
deployment, err := s.deploymentForDatabaseEnvironment(database, deployment, environment)
931+
// ExecuteRollbackPlanForApply generates a rollback plan for a specific apply.
932+
func (s *Service) ExecuteRollbackPlanForApply(ctx context.Context, apply *storage.Apply) (*apitypes.PlanResponse, error) {
933+
if apply == nil {
934+
return nil, fmt.Errorf("apply is required")
935+
}
936+
if !state.IsState(apply.State, state.Apply.Completed) {
937+
return nil, fmt.Errorf("apply %s is in state %q; only completed applies can be rolled back", apply.ApplyIdentifier, apply.State)
938+
}
939+
940+
plan, err := s.storage.Plans().GetByID(ctx, apply.PlanID)
941+
if err != nil {
942+
return nil, fmt.Errorf("get rollback source plan: %w", err)
943+
}
944+
if plan == nil {
945+
return nil, fmt.Errorf("plan not found for apply %s", apply.ApplyIdentifier)
946+
}
947+
if !rollbackSourcePlanMatchesApply(plan, apply) {
948+
return nil, fmt.Errorf("source plan %s belongs to %s/%s/%s, not apply %s for %s/%s/%s",
949+
plan.PlanIdentifier, plan.Database, plan.DatabaseType, plan.Environment,
950+
apply.ApplyIdentifier, apply.Database, apply.DatabaseType, apply.Environment)
951+
}
952+
if apply.DatabaseType == storage.DatabaseTypeVitess && planContainsVSchemaChange(plan) {
953+
return nil, fmt.Errorf("rollback of Vitess VSchema changes is not supported because the original VSchema was not captured; reconcile manually")
954+
}
955+
956+
schemaFiles, err := rollbackSchemaFiles(plan)
908957
if err != nil {
909-
return nil, fmt.Errorf("resolve deployment for rollback plan: %w", err)
958+
return nil, err
910959
}
911960

912-
client, err := s.TernClient(deployment, environment)
961+
deployment, err := storedDeploymentForApply(apply)
962+
if err != nil {
963+
return nil, err
964+
}
965+
if plan.Target == "" {
966+
return nil, fmt.Errorf("plan %s is missing server-side routing metadata field %q; create a new plan and retry rollback", plan.PlanIdentifier, "target")
967+
}
968+
client, err := s.TernClient(deployment, apply.Environment)
913969
if err != nil {
914-
return nil, fmt.Errorf("database %q (%s): %w", database, environment, err)
970+
return nil, fmt.Errorf("database %q (%s): %w", apply.Database, apply.Environment, err)
915971
}
916972

917-
resp, err := client.RollbackPlan(ctx, database, environment)
973+
prNumber := int32(apply.PullRequest)
974+
req := PlanRequest{
975+
Database: apply.Database,
976+
Environment: apply.Environment,
977+
Type: apply.DatabaseType,
978+
SchemaFiles: schemaFiles,
979+
Repository: apply.Repository,
980+
PullRequest: &prNumber,
981+
}
982+
resp, err := client.Plan(ctx, &ternv1.PlanRequest{
983+
Database: req.Database,
984+
Type: req.Type,
985+
SchemaFiles: req.SchemaFiles,
986+
Repository: req.Repository,
987+
PullRequest: prNumber,
988+
Environment: req.Environment,
989+
Target: plan.Target,
990+
})
918991
if err != nil {
919992
return nil, err
920993
}
994+
route := storedPlanRoute{
995+
DatabaseType: apply.DatabaseType,
996+
Deployment: deployment,
997+
Target: plan.Target,
998+
}
999+
if err := s.storePlanResponse(ctx, req, resp, route); err != nil {
1000+
return nil, err
1001+
}
9211002

9221003
return planResponseFromProto(resp), nil
9231004
}
9241005

1006+
func rollbackSourcePlanMatchesApply(plan *storage.Plan, apply *storage.Apply) bool {
1007+
return plan.Database == apply.Database &&
1008+
plan.DatabaseType == apply.DatabaseType &&
1009+
plan.Environment == apply.Environment
1010+
}
1011+
1012+
func planContainsVSchemaChange(plan *storage.Plan) bool {
1013+
for _, nsData := range plan.Namespaces {
1014+
if nsData != nil && len(nsData.VSchema) > 0 {
1015+
return true
1016+
}
1017+
}
1018+
return false
1019+
}
1020+
1021+
func rollbackSchemaFiles(plan *storage.Plan) (map[string]*ternv1.SchemaFiles, error) {
1022+
schemaFiles := make(map[string]*ternv1.SchemaFiles)
1023+
for ns, nsData := range plan.Namespaces {
1024+
if nsData == nil || !nsData.OriginalSchemaCaptured {
1025+
return nil, fmt.Errorf("no original schema available for rollback namespace %q (plan may predate rollback feature)", ns)
1026+
}
1027+
sqlFiles := make(map[string]string, len(nsData.OriginalSchema))
1028+
for tableName, createSQL := range nsData.OriginalSchema {
1029+
sqlFiles[tableName+".sql"] = createSQL
1030+
}
1031+
schemaFiles[ns] = &ternv1.SchemaFiles{Files: sqlFiles}
1032+
}
1033+
if len(schemaFiles) == 0 {
1034+
return nil, fmt.Errorf("no namespaces available for rollback")
1035+
}
1036+
return schemaFiles, nil
1037+
}
1038+
9251039
// validateSchemaFiles checks that schema_files has at least one namespace and
9261040
// that every namespace carries a non-null value. An empty Files map within a
9271041
// namespace is valid (signals "drop all tables"), so we only reject when

pkg/api/proto_helpers.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,26 +84,40 @@ func planResponseFromProto(resp *ternv1.PlanResponse) *apitypes.PlanResponse {
8484
}
8585

8686
// protoChangesToNamespaces converts proto SchemaChanges to storage namespace plan data.
87-
func protoChangesToNamespaces(changes []*ternv1.SchemaChange) map[string]*storage.NamespacePlanData {
87+
func protoChangesToNamespaces(changes []*ternv1.SchemaChange, schemaFiles map[string]*ternv1.SchemaFiles) map[string]*storage.NamespacePlanData {
8888
result := make(map[string]*storage.NamespacePlanData)
8989
for _, sc := range changes {
9090
ns := sc.Namespace
9191
if ns == "" {
9292
ns = "default"
9393
}
94-
nsData := &storage.NamespacePlanData{}
94+
nsData := &storage.NamespacePlanData{
95+
OriginalSchema: sc.OriginalSchema,
96+
OriginalSchemaCaptured: sc.OriginalSchemaCaptured,
97+
}
9598
for _, t := range sc.TableChanges {
9699
nsData.Tables = append(nsData.Tables, storage.TableChange{
97100
Table: t.TableName,
98101
DDL: t.Ddl,
99102
Operation: protoChangeTypeToOperation(t.ChangeType),
100103
})
101104
}
105+
if schemaChangeHasVSchema(sc) {
106+
if sf := schemaFiles[ns]; sf != nil {
107+
if raw := sf.Files["vschema.json"]; raw != "" {
108+
nsData.VSchema = []byte(raw)
109+
}
110+
}
111+
}
102112
result[ns] = nsData
103113
}
104114
return result
105115
}
106116

117+
func schemaChangeHasVSchema(sc *ternv1.SchemaChange) bool {
118+
return sc.Metadata["vschema"] != "" || sc.Metadata["vschema_changed"] == "true"
119+
}
120+
107121
// protoChangeTypeToOperation converts a proto ChangeType enum to a storage operation string.
108122
func protoChangeTypeToOperation(ct ternv1.ChangeType) string {
109123
switch ct {

pkg/api/proto_helpers_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,3 +115,19 @@ func TestValidateSchemaFiles_MissingRejected(t *testing.T) {
115115
assert.Empty(t, warning)
116116
assert.Contains(t, err.Error(), "schema_files is required")
117117
}
118+
119+
func TestProtoChangesToNamespacesPreservesVSchemaFile(t *testing.T) {
120+
changes := []*ternv1.SchemaChange{
121+
{
122+
Namespace: "commerce",
123+
Metadata: map[string]string{"vschema_changed": "true"},
124+
},
125+
}
126+
schemaFiles := map[string]*ternv1.SchemaFiles{
127+
"commerce": {Files: map[string]string{"vschema.json": `{"tables":{"users":{}}}`}},
128+
}
129+
130+
namespaces := protoChangesToNamespaces(changes, schemaFiles)
131+
132+
assert.JSONEq(t, `{"tables":{"users":{}}}`, string(namespaces["commerce"].VSchema))
133+
}

0 commit comments

Comments
 (0)