fix(config): propagate only changed global defaults, in the same tx

internal/api/handler_config.go

@@ -72,11 +72,8 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 		return nil, NewClientError(400, "invalid request body")
 	}
 
-	// Presence map of the top-level keys the caller actually sent. Used to
-	// (a) replace (not merge) grace_period_days when present, and (b) gate the
-	// service-config propagation on the global defaults actually being sent, so
-	// a deliberately-partial PUT (e.g. the kill-switch toggle) does not rewrite
-	// per-service customizations.
+	// Presence map of the top-level keys the caller actually sent, used to
+	// replace (not merge) grace_period_days when present.
 	var present map[string]json.RawMessage
 	if err := json.Unmarshal([]byte(req.Body), &present); err != nil {
 		return nil, NewClientError(400, "invalid request body")
@@ -95,8 +92,9 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 	// laddering kill-switch toggle and a Settings save) from reading the same
 	// stale base and losing each other's update. The closure returns a
 	// ClientError(400) on a bad body or validation failure, which the store
-	// propagates unchanged; DB/transport errors surface as 500.
-	cfg, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error {
+	// propagates unchanged; DB/transport errors surface as 500. A changed
+	// default reaches the service configs inside the same transaction.
+	_, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error {
 		// Snapshot before the wholesale unmarshal: this body writes onto the
 		// entire GlobalConfig, so ri_exchange_mode / ri_exchange_enabled reach
 		// the scheduler through this endpoint exactly as they do through the
@@ -124,14 +122,6 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR
 		return nil, err
 	}
 
-	// Propagate global defaults to every service config ONLY when the caller
-	// actually sent at least one of those defaults. A partial PUT that omits
-	// them (the kill-switch toggle sends only { laddering_enabled }) must not
-	// overwrite money-affecting per-service term/payment/coverage/ramp overrides.
-	if anyKeyPresent(present, "default_term", "default_payment", "default_coverage", "default_ramp_schedule") {
-		h.propagateGlobalDefaults(ctx, cfg)
-	}
-
 	return &StatusResponse{Status: "updated"}, nil
 }
 
@@ -147,39 +137,6 @@ func (h *Handler) requireGlobalConfigScope(ctx context.Context, session *Session
 		[]auth.PermissionConstraints{{StrictScope: true}})
 }
 
-// anyKeyPresent reports whether any of keys is present in m.
-func anyKeyPresent(m map[string]json.RawMessage, keys ...string) bool {
-	for _, k := range keys {
-		if _, ok := m[k]; ok {
-			return true
-		}
-	}
-	return false
-}
-
-// propagateGlobalDefaults overlays the global term/payment/coverage/ramp
-// defaults onto every service config. Best-effort: failures are logged, not
-// returned, because the global config was already saved.
-func (h *Handler) propagateGlobalDefaults(ctx context.Context, cfg *config.GlobalConfig) {
-	services, err := h.config.ListServiceConfigs(ctx)
-	if err != nil {
-		logging.Warnf("Failed to list service configs for propagation: %v", err)
-		return
-	}
-	// Index-based iteration mutates each element in place and avoids copying the
-	// (large) ServiceConfig struct per iteration (gocritic rangeValCopy).
-	for i := range services {
-		svc := &services[i]
-		svc.Term = cfg.DefaultTerm
-		svc.Payment = cfg.DefaultPayment
-		svc.Coverage = cfg.DefaultCoverage
-		svc.RampSchedule = cfg.DefaultRampSchedule
-		if saveErr := h.config.SaveServiceConfig(ctx, svc); saveErr != nil {
-			logging.Warnf("Failed to update service config %s/%s: %v", svc.Provider, svc.Service, saveErr)
-		}
-	}
-}
-
 // checkCommitmentOptionCombo rejects saves that carry a (term, payment)
 // combination we've dynamically confirmed the cloud doesn't sell. Returns
 // nil when: no probe service is wired, the service hasn't persisted data

internal/api/handler_config_propagation_integration_test.go

@@ -0,0 +1,92 @@
+//go:build integration
+
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/jackc/pgx/v5"
+	"github.com/stretchr/testify/require"
+)
+
+// TestUpdateConfigPropagatesOnlyChangedDefaults is the issue #225 scenario
+// against Postgres: a global-defaults PUT must keep each service's own term,
+// payment and ramp, and a failed propagation must not save the global change.
+func TestUpdateConfigPropagatesOnlyChangedDefaults(t *testing.T) {
+	ctx := context.Background()
+	pg, err := testhelpers.SetupPostgresContainer(ctx, t)
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, pg.Cleanup(ctx)) })
+	require.NoError(t, migrations.RunMigrations(ctx, pg.DB.Pool(), "../database/postgres/migrations", "", ""))
+	store := config.NewPostgresStore(pg.DB)
+
+	global, err := store.GetGlobalConfig(ctx)
+	require.NoError(t, err)
+	global.EnabledProviders = []string{"aws"}
+	global.DefaultTerm, global.DefaultPayment, global.DefaultCoverage = 1, "no-upfront", 80
+	global.DefaultRampSchedule = config.RampImmediate
+	require.NoError(t, store.SaveGlobalConfig(ctx, global))
+	require.NoError(t, store.SaveServiceConfig(ctx, &config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, Term: 3, Payment: "all-upfront", Coverage: 90, RampSchedule: config.RampWeekly25Pct}))
+	require.NoError(t, store.SaveServiceConfig(ctx, &config.ServiceConfig{Provider: "aws", Service: "rds", Enabled: true, Term: 1, Payment: "partial-upfront", Coverage: 60, RampSchedule: config.RampMonthly10Pct}))
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil)
+	mockAuth.grantAdmin()
+	handler := &Handler{config: store, auth: mockAuth}
+	put := func(body string) error {
+		_, putErr := handler.updateConfig(ctx, &events.LambdaFunctionURLRequest{
+			Headers: map[string]string{"Authorization": "Bearer admin-token"},
+			Body:    body,
+		})
+		return putErr
+	}
+	service := func(name string) config.ServiceConfig {
+		svc, getErr := store.GetServiceConfig(ctx, "aws", name)
+		require.NoError(t, getErr)
+		return *svc
+	}
+
+	require.NoError(t, put(`{"default_coverage": 70}`))
+	ec2, rds := service("ec2"), service("rds")
+	require.Equal(t, []any{3, "all-upfront", 70.0, config.RampWeekly25Pct}, []any{ec2.Term, ec2.Payment, ec2.Coverage, ec2.RampSchedule})
+	require.Equal(t, []any{1, "partial-upfront", 70.0, config.RampMonthly10Pct}, []any{rds.Term, rds.Payment, rds.Coverage, rds.RampSchedule})
+
+	// Both writes must share one transaction, or a failed commit could leave
+	// the services rewritten while the global change rolls back.
+	var globalXmin string
+	require.NoError(t, pg.DB.Pool().QueryRow(ctx, `SELECT xmin::text FROM global_config`).Scan(&globalXmin))
+	rows, err := pg.DB.Pool().Query(ctx, `SELECT DISTINCT xmin::text FROM service_configs WHERE coverage = 70`)
+	require.NoError(t, err)
+	serviceXmins, err := pgx.CollectRows(rows, pgx.RowTo[string])
+	require.NoError(t, err)
+	require.Equal(t, []string{globalXmin}, serviceXmins, "propagation must run in the global config transaction")
+
+	// The recommendations lookback control round-trips the whole config, so
+	// every default key is present but none changed.
+	current, err := store.GetGlobalConfig(ctx)
+	require.NoError(t, err)
+	current.RecommendationsLookbackDays = 30
+	roundTrip, err := json.Marshal(current)
+	require.NoError(t, err)
+	require.NoError(t, put(string(roundTrip)))
+	require.Equal(t, ec2, service("ec2"))
+	require.Equal(t, rds, service("rds"))
+
+	_, err = pg.DB.Pool().Exec(ctx, `
+		CREATE FUNCTION reject_service_config_update() RETURNS trigger AS $$
+		BEGIN RAISE EXCEPTION 'service_configs update rejected'; END $$ LANGUAGE plpgsql;
+		CREATE TRIGGER reject_service_config_update BEFORE UPDATE ON service_configs
+		FOR EACH ROW EXECUTE FUNCTION reject_service_config_update();`)
+	require.NoError(t, err)
+	require.ErrorContains(t, put(`{"default_term": 3}`), "failed to propagate global defaults")
+	after, err := store.GetGlobalConfig(ctx)
+	require.NoError(t, err)
+	require.Equal(t, 1, after.DefaultTerm, "global change must roll back with the failed propagation")
+	require.Equal(t, ec2, service("ec2"))
+}

internal/api/handler_config_test.go

@@ -63,8 +63,6 @@ func TestHandler_updateConfig(t *testing.T) {
 	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
 	mockAuth.grantAdmin()
 	mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).Return(nil)
-	// Mock ListServiceConfigs for propagation of global defaults
-	mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil)
 	// updateConfig now calls GetGlobalConfig when recommendations_cache_stale_hours
 	// or recommendations_lookback_days is omitted from the request body, so the
 	// existing persisted value can be preserved rather than zeroed out (PR #308
@@ -124,7 +122,6 @@ func TestHandler_updateConfig_LadderingEnabledPreservation(t *testing.T) {
 			RecommendationsLookbackDays:    config.DefaultRecommendationsLookbackDays,
 			LadderingEnabled:               existingLaddering,
 		}, nil)
-		mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil)
 
 		var saved config.GlobalConfig
 		mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).
@@ -1045,127 +1042,65 @@ func TestHandler_getServiceConfig_InvalidProvider(t *testing.T) {
 	assert.Contains(t, err.Error(), "invalid provider")
 }
 
-func TestHandler_updateConfig_WithPropagation(t *testing.T) {
-	ctx := context.Background()
-	mockStore := new(MockConfigStore)
-	mockAuth := new(MockAuthService)
-
-	adminSession := &Session{
-		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
-		Email:  "admin@example.com",
-	}
-
-	serviceConfigs := []config.ServiceConfig{
-		{Provider: "aws", Service: "rds", Enabled: true},
-		{Provider: "aws", Service: "ec2", Enabled: true},
-	}
-
-	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
-	mockAuth.grantAdmin()
-	mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).Return(nil)
-	// updateConfig calls GetGlobalConfig before save to preserve persisted
-	// values for fields omitted from the request body (PR #308 CR pass-2).
-	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
-		RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours,
-		RecommendationsLookbackDays:    config.DefaultRecommendationsLookbackDays,
-	}, nil)
-	mockStore.On("ListServiceConfigs", ctx).Return(serviceConfigs, nil)
-	mockStore.On("SaveServiceConfig", ctx, mock.AnythingOfType("*config.ServiceConfig")).Return(nil)
-
-	handler := &Handler{config: mockStore, auth: mockAuth}
-
-	body := `{"enabled_providers": ["aws"], "default_term": 3, "default_coverage": 80}`
-	req := &events.LambdaFunctionURLRequest{
-		Headers: map[string]string{
-			"Authorization": "Bearer admin-token",
-		},
-		Body: body,
-	}
-	result, err := handler.updateConfig(ctx, req)
-	require.NoError(t, err)
-	assert.Equal(t, "updated", result.Status)
-
-	// Verify SaveServiceConfig was called for each service
-	mockStore.AssertNumberOfCalls(t, "SaveServiceConfig", 2)
-}
-
-func TestHandler_updateConfig_PropagationServiceSaveError(t *testing.T) {
+// TestHandler_updateConfig_DefaultsPUTDoesNotRewriteServiceConfigs is the
+// issue #225 regression: a global-defaults PUT must not rewrite service
+// configs itself; the store propagates only changed defaults inside its tx.
+func TestHandler_updateConfig_DefaultsPUTDoesNotRewriteServiceConfigs(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) })
 
-	adminSession := &Session{
-		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
-		Email:  "admin@example.com",
-	}
-
-	serviceConfigs := []config.ServiceConfig{
-		{Provider: "aws", Service: "rds", Enabled: true},
-	}
-
-	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
+	mockAuth.On("ValidateSession", ctx, "admin-token").
+		Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil)
 	mockAuth.grantAdmin()
-	mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).Return(nil)
-	// updateConfig calls GetGlobalConfig before save to preserve persisted
-	// values for fields omitted from the request body (PR #308 CR pass-2).
 	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
+		DefaultTerm:                    1,
+		DefaultPayment:                 "no-upfront",
+		DefaultCoverage:                80,
 		RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours,
 		RecommendationsLookbackDays:    config.DefaultRecommendationsLookbackDays,
 	}, nil)
-	mockStore.On("ListServiceConfigs", ctx).Return(serviceConfigs, nil)
-	// Simulate failure when saving service config during propagation
-	mockStore.On("SaveServiceConfig", ctx, mock.AnythingOfType("*config.ServiceConfig")).Return(assert.AnError)
+	var saved config.GlobalConfig
+	mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).
+		Run(func(args mock.Arguments) { saved = *args.Get(1).(*config.GlobalConfig) }).
+		Return(nil)
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
-
-	body := `{"enabled_providers": ["aws"], "default_term": 3}`
-	req := &events.LambdaFunctionURLRequest{
-		Headers: map[string]string{
-			"Authorization": "Bearer admin-token",
-		},
-		Body: body,
-	}
-	// Should still succeed even if service config propagation fails
-	result, err := handler.updateConfig(ctx, req)
+	result, err := handler.updateConfig(ctx, &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    `{"default_coverage": 70}`,
+	})
 	require.NoError(t, err)
 	assert.Equal(t, "updated", result.Status)
+	assert.Equal(t, 70.0, saved.DefaultCoverage)
+	mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything)
+	mockStore.AssertNotCalled(t, "SaveServiceConfig", mock.Anything, mock.Anything)
 }
 
-func TestHandler_updateConfig_PropagationListError(t *testing.T) {
+// TestHandler_updateConfig_PropagationErrorIsReturned asserts a failed
+// default propagation (which rolls back the store tx) is not reported as
+// "updated".
+func TestHandler_updateConfig_PropagationErrorIsReturned(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockConfigStore)
 	mockAuth := new(MockAuthService)
+	t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) })
 
-	adminSession := &Session{
-		UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa",
-		Email:  "admin@example.com",
-	}
-
-	mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil)
+	mockAuth.On("ValidateSession", ctx, "admin-token").
+		Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil)
 	mockAuth.grantAdmin()
-	mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")).Return(nil)
-	// updateConfig calls GetGlobalConfig before save to preserve persisted
-	// values for fields omitted from the request body (PR #308 CR pass-2).
-	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
-		RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours,
-		RecommendationsLookbackDays:    config.DefaultRecommendationsLookbackDays,
-	}, nil)
-	// Simulate failure when listing service configs for propagation
-	mockStore.On("ListServiceConfigs", ctx).Return(nil, assert.AnError)
+	propErr := fmt.Errorf("failed to propagate global defaults to service configs: %w", assert.AnError)
+	mockStore.On("UpdateGlobalConfigAtomic", ctx, mock.AnythingOfType("func(*config.GlobalConfig) error")).
+		Return(nil, propErr)
 
 	handler := &Handler{config: mockStore, auth: mockAuth}
-
-	body := `{"enabled_providers": ["aws"], "default_term": 3}`
-	req := &events.LambdaFunctionURLRequest{
-		Headers: map[string]string{
-			"Authorization": "Bearer admin-token",
-		},
-		Body: body,
-	}
-	// Should still succeed even if listing fails - global config was saved
-	result, err := handler.updateConfig(ctx, req)
-	require.NoError(t, err)
-	assert.Equal(t, "updated", result.Status)
+	result, err := handler.updateConfig(ctx, &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"Authorization": "Bearer admin-token"},
+		Body:    `{"enabled_providers": ["aws"], "default_term": 3}`,
+	})
+	require.ErrorIs(t, err, assert.AnError)
+	assert.Nil(t, result)
 }
 
 // Regression tests for 02-M4: GET /api/config and GET /api/config/service/*

internal/config/interfaces.go

@@ -19,6 +19,8 @@ type StoreInterface interface {
 	// global_config singleton under an advisory-locked transaction so
 	// concurrent partial PUTs cannot lose each other's updates. apply mutates
 	// the loaded config in place; its error aborts the write and is propagated.
+	// Each default_* value apply changes is copied onto every service config
+	// in the same transaction.
 	UpdateGlobalConfigAtomic(ctx context.Context, apply func(*GlobalConfig) error) (*GlobalConfig, error)
 
 	// Service configuration