From 48c01de88621a4078b02b10ee6f65021acaec625 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Thu, 27 Aug 2026 07:43:32 -0700 Subject: [PATCH] Refuse to persist an empty control-plane snapshot in rad shutdown Motivation: The repo-based deploy pipeline restores durable state with `rad startup` and persists it with `rad shutdown`. A prior fix gated `rad shutdown` on a `state-restored` signal so it only runs after `rad startup` succeeded earlier in the same run -- but that only proves startup ran, not that the control plane is still healthy by the time shutdown runs. If it degrades afterward (a PostgreSQL pod crash-loop, or `rad install` re-run mid-run), `rad shutdown` would still dump and commit an empty database, corrupting the shared archive for every future run. Approach: Add pgbackup.IsControlPlaneEmpty, which inspects the backed-up ucp.sql dump for the pg_dump COPY block of the "resources" table (the table that stores every UCP resource, including resource groups) and reports whether it has zero data rows, or the table is missing entirely. rad shutdown calls this after backing up the databases and before backing up Terraform state or committing to the archive; when the backup is empty it returns a clear clierrors.Message and leaves the existing archive untouched. This is defense-in-depth inside the command itself, independent of the workflow-level guard. Validation: go test ./pkg/cli/pgbackup/... ./pkg/cli/cmd/shutdown/... -v passes, including new tests for IsControlPlaneEmpty (no data rows, missing table, populated, missing file) and Test_Run_EmptyControlPlaneStopsBeforeCommit (asserts Run stops before BackupTerraform/Commit on an empty backup). Reverting only the two source files while keeping the test files causes a build failure and an unexpected-Commit-call test failure, confirming the new check is what prevents the commit. go vet and gofmt are clean, and golangci-lint (pinned repo version v2.13.1) reports 0 issues on both changed packages. The COPY-block parsing was additionally verified against a real, locally-run PostgreSQL 16 instance with the exact "resources" schema this codebase writes, using genuine pg_dump output for both an empty and a populated table; this environment has no live Radius cluster, so the change was not exercised end-to-end through a real control plane. Report: https://github.com/radius-project/radius/issues/12847 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- pkg/cli/cmd/shutdown/shutdown.go | 8 +++++ pkg/cli/cmd/shutdown/shutdown_test.go | 46 ++++++++++++++++++++++++--- pkg/cli/pgbackup/pgbackup.go | 29 +++++++++++++++++ pkg/cli/pgbackup/pgbackup_test.go | 36 +++++++++++++++++++++ 4 files changed, 114 insertions(+), 5 deletions(-) diff --git a/pkg/cli/cmd/shutdown/shutdown.go b/pkg/cli/cmd/shutdown/shutdown.go index 3eebbf92dc..fa487d25a4 100644 --- a/pkg/cli/cmd/shutdown/shutdown.go +++ b/pkg/cli/cmd/shutdown/shutdown.go @@ -126,6 +126,14 @@ func (r *Runner) Run(ctx context.Context) error { return fmt.Errorf("failed to back up control-plane databases: %w", err) } + empty, err := pgbackup.IsControlPlaneEmpty(stateDir) + if err != nil { + return fmt.Errorf("failed to inspect control-plane backup: %w", err) + } + if empty { + return clierrors.Message("Refusing to persist state: the control-plane backup contains no resources. This can happen if the control plane degraded after 'rad startup' succeeded (for example a postgres pod crash-loop, or 'rad install' being re-run mid-session). The existing state archive was left unchanged; investigate the control plane before retrying 'rad shutdown'.") + } + r.Output.LogInfo("Backing up Terraform recipe state...") if err := r.StateClient.BackupTerraform(ctx, kubeContext, pgbackup.DefaultNamespace, stateDir); err != nil { return fmt.Errorf("failed to back up Terraform state: %w", err) diff --git a/pkg/cli/cmd/shutdown/shutdown_test.go b/pkg/cli/cmd/shutdown/shutdown_test.go index a25627bff5..fbe2ac45d1 100644 --- a/pkg/cli/cmd/shutdown/shutdown_test.go +++ b/pkg/cli/cmd/shutdown/shutdown_test.go @@ -19,6 +19,8 @@ package shutdown import ( "context" "errors" + "os" + "path/filepath" "testing" "github.com/radius-project/radius/pkg/cli/framework" @@ -31,6 +33,13 @@ import ( "go.uber.org/mock/gomock" ) +// nonEmptyUCPDump is a minimal pg_dump-shaped dump of the "resources" table with one data row, +// standing in for a healthy control-plane backup. +const nonEmptyUCPDump = `COPY public.resources (id, original_id, resource_type, root_scope, routing_scope, etag, resource_data) FROM stdin; +/planes/radius/local/resourcegroups/default /planes/radius/local/resourcegroups/default resourcegroups /planes/radius/local resourcegroups/default abc123 {} +\. +` + func Test_CommandValidation(t *testing.T) { radcli.SharedCommandValidation(t, NewCommand) } @@ -71,19 +80,28 @@ workspaces: radcli.SharedValidateValidation(t, NewCommand, testcases) } -// fakeStateBackupClient records calls and returns canned errors. +// fakeStateBackupClient records calls, returns canned errors, and simulates BackupDatabases by +// writing ucpDump (defaulting to a non-empty dump) to stateDir/ucp.sql. type fakeStateBackupClient struct { backupDBErr error backupTFErr error dbCalled bool tfCalled bool stateDirSeen string + ucpDump string +} + +func newFakeStateBackupClient() *fakeStateBackupClient { + return &fakeStateBackupClient{ucpDump: nonEmptyUCPDump} } func (f *fakeStateBackupClient) BackupDatabases(ctx context.Context, kubeContext, namespace, stateDir string) error { f.dbCalled = true f.stateDirSeen = stateDir - return f.backupDBErr + if f.backupDBErr != nil { + return f.backupDBErr + } + return os.WriteFile(filepath.Join(stateDir, "ucp.sql"), []byte(f.ucpDump), 0o644) } func (f *fakeStateBackupClient) BackupTerraform(ctx context.Context, kubeContext, namespace, stateDir string) error { @@ -125,7 +143,7 @@ func Test_Run_BacksUpBothStoresAndCommits(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - client := &fakeStateBackupClient{} + client := newFakeStateBackupClient() r, session, stateDir := newTestRunner(t, ctrl, client) session.EXPECT().Commit(gomock.Any(), gomock.Any()).Return(nil).Times(1) @@ -137,6 +155,24 @@ func Test_Run_BacksUpBothStoresAndCommits(t *testing.T) { require.Equal(t, stateDir, client.stateDirSeen, "backup must target the archive path") } +// Test_Run_EmptyControlPlaneStopsBeforeCommit verifies that a degenerate backup (no rows in the +// "resources" table, e.g. from a postgres pod crash-loop after a successful "rad startup") is +// rejected before it is committed, so the durable archive is never overwritten with it. +func Test_Run_EmptyControlPlaneStopsBeforeCommit(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + client := newFakeStateBackupClient() + client.ucpDump = "COPY public.resources (id) FROM stdin;\n\\.\n" + // Commit is intentionally not expected: an empty backup must stop before committing. The + // deferred session.Close is still expected (verified by the mock at ctrl.Finish). + r, _, _ := newTestRunner(t, ctrl, client) + + err := r.Run(t.Context()) + require.ErrorContains(t, err, "Refusing to persist state") + require.False(t, client.tfCalled, "terraform backup should not run after an empty control-plane backup") +} + func Test_Run_DatabaseBackupFailureStopsBeforeCommit(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -155,7 +191,7 @@ func Test_Run_CommitFailureIsReturned(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - client := &fakeStateBackupClient{} + client := newFakeStateBackupClient() r, session, _ := newTestRunner(t, ctrl, client) session.EXPECT().Commit(gomock.Any(), gomock.Any()).Return(errors.New("push rejected")).Times(1) @@ -172,7 +208,7 @@ func Test_Run_ArchiveOpenFailureIsReturned(t *testing.T) { archive := statearchive.NewMockArchive(ctrl) archive.EXPECT().Open(gomock.Any(), pgbackup.StateBranchName()).Return(nil, errors.New("not a git repo")).Times(1) - client := &fakeStateBackupClient{} + client := newFakeStateBackupClient() r := &Runner{ Output: &output.MockOutput{}, Workspace: kubernetesWorkspace(), diff --git a/pkg/cli/pgbackup/pgbackup.go b/pkg/cli/pgbackup/pgbackup.go index 4dd6cbbe3a..1eb4be7b3a 100644 --- a/pkg/cli/pgbackup/pgbackup.go +++ b/pkg/cli/pgbackup/pgbackup.go @@ -29,6 +29,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "github.com/radius-project/radius/pkg/process" @@ -85,6 +86,34 @@ func StateBranchName() string { return StateArchiveName() } +// copyResourcesHeader matches the pg_dump COPY header for the "resources" table -- the table that +// holds every UCP resource, including resource groups -- in plain-format output, e.g.: +// +// COPY public.resources (id, original_id, resource_type, ...) FROM stdin; +var copyResourcesHeader = regexp.MustCompile(`(?m)^COPY\s+(?:[\w"]+\.)?"?resources"?\s*\(.*\)\s+FROM\s+stdin;\s*$`) + +// IsControlPlaneEmpty reports whether the backed-up ucp database dump in stateDir contains no rows +// in the "resources" table. A database in this state (e.g. after a postgres pod crash-loop, or +// "rad install" re-run mid-session) is not safe to persist: committing it would overwrite the +// durable archive with unrecoverable data loss, even though `rad startup` restored the previous +// snapshot successfully at the start of the run. +func IsControlPlaneEmpty(stateDir string) (bool, error) { + path := filepath.Join(stateDir, "ucp.sql") + data, err := os.ReadFile(path) + if err != nil { + return false, fmt.Errorf("failed to read backup file %q: %w", path, err) + } + + loc := copyResourcesHeader.FindIndex(data) + if loc == nil { + // No "resources" table in the dump at all is at least as degenerate as an empty one. + return true, nil + } + + rest := bytes.TrimLeft(data[loc[1]:], "\n") + return bytes.HasPrefix(rest, []byte(`\.`)), nil +} + // HasBackup reports whether a SQL dump exists for every database in the state directory. func HasBackup(stateDir string) bool { for _, db := range Databases { diff --git a/pkg/cli/pgbackup/pgbackup_test.go b/pkg/cli/pgbackup/pgbackup_test.go index c46a149ba0..9b00db7f3c 100644 --- a/pkg/cli/pgbackup/pgbackup_test.go +++ b/pkg/cli/pgbackup/pgbackup_test.go @@ -54,6 +54,42 @@ func Test_HasBackup_MissingDirectory(t *testing.T) { require.False(t, HasBackup(filepath.Join(t.TempDir(), "does-not-exist"))) } +func Test_IsControlPlaneEmpty_NoDataRows(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "ucp.sql"), + []byte("COPY public.resources (id, resource_type) FROM stdin;\n\\.\n"), 0o644)) + + empty, err := IsControlPlaneEmpty(dir) + require.NoError(t, err) + require.True(t, empty, "a COPY block with no data rows is an empty control plane") +} + +func Test_IsControlPlaneEmpty_NoResourcesTableInDump(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "ucp.sql"), []byte("-- empty dump, no tables\n"), 0o644)) + + empty, err := IsControlPlaneEmpty(dir) + require.NoError(t, err) + require.True(t, empty, "a dump with no resources table at all is at least as degenerate as an empty one") +} + +func Test_IsControlPlaneEmpty_HasDataRows(t *testing.T) { + dir := t.TempDir() + dump := "COPY public.resources (id, resource_type) FROM stdin;\n" + + "/planes/radius/local/resourcegroups/default\tresourcegroups\n" + + "\\.\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "ucp.sql"), []byte(dump), 0o644)) + + empty, err := IsControlPlaneEmpty(dir) + require.NoError(t, err) + require.False(t, empty, "a COPY block with at least one data row is not an empty control plane") +} + +func Test_IsControlPlaneEmpty_MissingFile(t *testing.T) { + _, err := IsControlPlaneEmpty(t.TempDir()) + require.ErrorContains(t, err, "failed to read backup file") +} + func Test_StateBranchName_DefaultsWhenUnset(t *testing.T) { // t.Setenv unsets after the test; explicitly clear to isolate from the ambient environment. t.Setenv(StateBranchEnvVar, "")