OADP-6536: Call WaitGroup.Done() once only when PVB changes to final status the first time to avoid panic

.github/workflows/crds-verify-kind.yaml

@@ -14,7 +14,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: '1.22.5'
+          go-version: '1.23.0'
         id: go
       # Look for a CLI that's made for this PR
       - name: Fetch built CLI

.github/workflows/e2e-test-kind.yaml

@@ -14,7 +14,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: '1.22.5'
+          go-version: '1.23.0'
         id: go
       # Look for a CLI that's made for this PR
       - name: Fetch built CLI
@@ -82,7 +82,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: '1.22.5'
+          go-version: '1.23.0'
         id: go
       - name: Check out the code
         uses: actions/checkout@v4

.github/workflows/pr-ci-check.yml

@@ -10,7 +10,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: '1.22.5'
+          go-version: '1.23.0'
         id: go
       - name: Check out the code
         uses: actions/checkout@v4

.github/workflows/push.yml

@@ -18,7 +18,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v5
       with:
-        go-version: '1.22.5'
+        go-version: '1.23.0'
       id: go
 
     - uses: actions/checkout@v4

Dockerfile

@@ -13,7 +13,7 @@
 # limitations under the License.
 
 # Velero binary build section
-FROM --platform=$BUILDPLATFORM golang:1.22.5-bookworm as velero-builder
+FROM --platform=$BUILDPLATFORM golang:1.23.0-bookworm as velero-builder
 
 ARG GOPROXY
 ARG BIN
@@ -47,7 +47,7 @@ RUN mkdir -p /output/usr/bin && \
     go clean -modcache -cache
 
 # Restic binary build section
-FROM --platform=$BUILDPLATFORM golang:1.22.5-bookworm as restic-builder
+FROM --platform=$BUILDPLATFORM golang:1.23.0-bookworm as restic-builder
 
 ARG BIN
 ARG TARGETOS

Tiltfile

@@ -52,7 +52,7 @@ git_sha = str(local("git rev-parse HEAD", quiet = True, echo_off = True)).strip(
 
 tilt_helper_dockerfile_header = """
 # Tilt image
-FROM golang:1.22.5 as tilt-helper
+FROM golang:1.23.0 as tilt-helper
 
 # Support live reloading with Tilt
 RUN wget --output-document /restart.sh --quiet https://raw.githubusercontent.com/windmilleng/rerun-process-wrapper/master/restart.sh  && \

hack/build-image/Dockerfile

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-FROM --platform=$TARGETPLATFORM golang:1.22.5-bookworm
+FROM --platform=$TARGETPLATFORM golang:1.23.0-bookworm
 
 ARG GOPROXY
 

pkg/podvolume/backupper.go

@@ -121,8 +121,8 @@ func newBackupper(
 
 	b.handlerRegistration, _ = pvbInformer.AddEventHandler(
 		cache.ResourceEventHandlerFuncs{
-			UpdateFunc: func(_, obj interface{}) {
-				pvb := obj.(*velerov1api.PodVolumeBackup)
+			UpdateFunc: func(oldObj, newObj interface{}) {
+				pvb := newObj.(*velerov1api.PodVolumeBackup)
 
 				if pvb.GetLabels()[velerov1api.BackupUIDLabel] != string(backup.UID) {
 					return
@@ -133,8 +133,24 @@ func newBackupper(
 					return
 				}
 
+				// Check if the previous state was already in a final status
+				statusChangedToFinal := true
+				if oldPvb, ok := oldObj.(*velerov1api.PodVolumeBackup); ok {
+					// If the PVB was already in a final status, no need to call WaitGroup.Done()
+					if oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCompleted ||
+						oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed {
+						statusChangedToFinal = false
+					}
+				}
+
 				b.result = append(b.result, pvb)
-				b.wg.Done()
+
+				// Call WaitGroup.Done() once only when the PVB changes to final status the first time.
+				// This avoids the cases where the handler gets multiple update events whose PVBs are all in final status
+				// which causes panic with "negative WaitGroup counter" error
+				if statusChangedToFinal {
+					b.wg.Done()
+				}
 			},
 		},
 	)

pkg/podvolume/backupper_test.go

@@ -678,3 +678,143 @@ func TestPVCBackupSummary(t *testing.T) {
 	assert.Empty(t, pbs.Skipped)
 	assert.Len(t, pbs.Backedup, 2)
 }
+
+func TestBackupperEventHandlerWaitGroupLogic(t *testing.T) {
+	scheme := runtime.NewScheme()
+	velerov1api.AddToScheme(scheme)
+	corev1api.AddToScheme(scheme)
+
+	tests := []struct {
+		name              string
+		oldPVBStatus      *velerov1api.PodVolumeBackupStatus
+		newPVBStatus      *velerov1api.PodVolumeBackupStatus
+		expectedDoneCalls int
+		description       string
+	}{
+		{
+			name:              "first transition to completed status",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted},
+			expectedDoneCalls: 1,
+			description:       "WaitGroup.Done() should be called once when transitioning to completed",
+		},
+		{
+			name:              "first transition to failed status",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed},
+			expectedDoneCalls: 1,
+			description:       "WaitGroup.Done() should be called once when transitioning to failed",
+		},
+		{
+			name:              "multiple updates with completed status",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted},
+			expectedDoneCalls: 0,
+			description:       "WaitGroup.Done() should NOT be called when PVB is already completed",
+		},
+		{
+			name:              "multiple updates with failed status",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed},
+			expectedDoneCalls: 0,
+			description:       "WaitGroup.Done() should NOT be called when PVB is already failed",
+		},
+		{
+			name:              "transition from new to completed",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseCompleted},
+			expectedDoneCalls: 1,
+			description:       "WaitGroup.Done() should be called when transitioning from new to completed",
+		},
+		{
+			name:              "transition from new to failed",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseFailed},
+			expectedDoneCalls: 1,
+			description:       "WaitGroup.Done() should be called when transitioning from new to failed",
+		},
+		{
+			name:              "no transition for non-final status",
+			oldPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseNew},
+			newPVBStatus:      &velerov1api.PodVolumeBackupStatus{Phase: velerov1api.PodVolumeBackupPhaseInProgress},
+			expectedDoneCalls: 0,
+			description:       "WaitGroup.Done() should NOT be called for non-final status transitions",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// Create backup with UID for label matching
+			backup := &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-backup",
+					Namespace: velerov1api.DefaultNamespace,
+					UID:       "test-backup-uid",
+				},
+			}
+
+			// Create old and new PVB objects
+			oldPVB := &velerov1api.PodVolumeBackup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-pvb",
+					Namespace: velerov1api.DefaultNamespace,
+					Labels: map[string]string{
+						velerov1api.BackupUIDLabel: string(backup.UID),
+					},
+				},
+				Status: *test.oldPVBStatus,
+			}
+
+			newPVB := &velerov1api.PodVolumeBackup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-pvb",
+					Namespace: velerov1api.DefaultNamespace,
+					Labels: map[string]string{
+						velerov1api.BackupUIDLabel: string(backup.UID),
+					},
+				},
+				Status: *test.newPVBStatus,
+			}
+
+			// Track Done() calls using a counter
+			doneCallCount := 0
+
+			// Create a custom UpdateFunc that mirrors the actual implementation
+			updateFunc := func(oldObj, newObj interface{}) {
+				pvb := newObj.(*velerov1api.PodVolumeBackup)
+
+				if pvb.GetLabels()[velerov1api.BackupUIDLabel] != string(backup.UID) {
+					return
+				}
+
+				if pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted &&
+					pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseFailed {
+					return
+				}
+
+				// Check if the previous state was already in a final status
+				statusChangedToFinal := true
+				if oldPvb, ok := oldObj.(*velerov1api.PodVolumeBackup); ok {
+					// If the PVB was already in a final status, no need to call WaitGroup.Done()
+					if oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseCompleted ||
+						oldPvb.Status.Phase == velerov1api.PodVolumeBackupPhaseFailed {
+						statusChangedToFinal = false
+					}
+				}
+
+				// Call WaitGroup.Done() once only when the PVB changes to final status the first time.
+				// This avoids the cases where the handler gets multiple update events whose PVBs are all in final status
+				// which causes panic with "negative WaitGroup counter" error
+				if statusChangedToFinal {
+					doneCallCount++
+				}
+			}
+
+			// Test the UpdateFunc directly
+			updateFunc(oldPVB, newPVB)
+
+			// Verify the expected number of Done() calls
+			assert.Equal(t, test.expectedDoneCalls, doneCallCount, test.description)
+		})
+	}
+}