[Backport release-1.10] Fix Race on MR Status Between Async Callbacks & Managed Reconciler

.github/workflows/ci.yml

@@ -194,4 +194,58 @@ jobs:
         uses: codecov/codecov-action@eaaf4bedf32dbdc6b720b63067d99c4d77d6047d # v3
         with:
           flags: unittests
-          file: _output/tests/linux_amd64/coverage.txt
+          files: _output/tests/linux_amd64/coverage.txt
+          token: ${{ secrets.CODECOV_TOKEN }}
+
+  race-tests:
+    runs-on: ubuntu-latest
+    needs: detect-noop
+    if: needs.detect-noop.outputs.noop != 'true'
+    steps:
+      - name: Cleanup Disk
+        uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1
+        with:
+          large-packages: false
+          swap-storage: false
+
+      - name: Checkout
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+        with:
+          submodules: true
+
+      - name: Fetch History
+        run: git fetch --prune --unshallow
+
+      - name: Setup Go
+        uses: actions/setup-go@41dfa10bad2bb2ae585af6ee5bb4d7d973ad74ed # v5.1.0
+        with:
+          go-version: ${{ env.GO_VERSION }}
+
+      - name: Find the Go Build Cache
+        id: go-cache-paths
+        run: |
+          echo "go-build=$(make go.cachedir)" >> $GITHUB_OUTPUT
+          echo "go-mod=$(make go.mod.cachedir)" >> $GITHUB_OUTPUT
+
+      - name: Cache the Go Build Cache
+        uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
+        with:
+          path: ${{ steps.go-cache-paths.outputs.go-build }}
+          key: ${{ runner.os }}-build-race-tests-${{ hashFiles('**/go.sum') }}
+          restore-keys: ${{ runner.os }}-build-race-tests-
+
+      - name: Cache Go Dependencies
+        uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
+        with:
+          path: ${{ steps.go-cache-paths.outputs.go-mod }}
+          key: ${{ runner.os }}-pkg-race-tests-${{ hashFiles('**/go.sum') }}
+          restore-keys: ${{ runner.os }}-pkg-race-tests-
+
+      - name: Vendor Dependencies
+        run: make vendor vendor.check
+
+      # Runs the unit tests with the Go race detector enabled to guard against
+      # regressions of concurrency bugs such as the managed resource status
+      # update data race. See: https://github.com/crossplane/upjet/issues/472
+      - name: Run Race Tests
+        run: make test.race

Makefile

@@ -69,4 +69,14 @@ go.cachedir:
 go.mod.cachedir:
 	@go env GOMODCACHE
 
-.PHONY: reviewable submodules fallthrough go.mod.cachedir go.cachedir
+# test.race runs the unit tests with the Go race detector enabled. The race
+# detector requires cgo, so we enable it here.
+# Do NOT directly use the `go.test.unit` make target of
+# the build submodule here. There's a `pipefail` issue with that target,
+# which will mask any failing tests.
+test.race:
+	@$(INFO) go test race
+	@CGO_ENABLED=1 $(GOHOST) test -race $(GO_STATIC_FLAGS) $(GO_PACKAGES) || $(FAIL)
+	@$(OK) go test race
+
+.PHONY: reviewable submodules fallthrough go.mod.cachedir go.cachedir test.race

pkg/controller/external_async_tfpluginfw.go

@@ -164,6 +164,10 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Create(_ context.Context,
 	}
 
 	ctx, cancel := context.WithDeadline(context.Background(), n.opTracker.LastOperation.StartTime().Add(defaultAsyncTimeout))
+	// We deep-copy the managed resource to prevent a data race between the
+	// goroutine we are about to start below and the managed reconciler.
+	// Please see: https://github.com/crossplane/upjet/issues/472
+	mgCopy := mg.DeepCopyObject().(xpresource.Managed)
 	go func() {
 		// The order of deferred functions, executed last-in-first-out, is
 		// significant. The context should be canceled last, because it is
@@ -178,14 +182,14 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Create(_ context.Context,
 			n.opTracker.logger.Debug("Async create ended.", "error", err)
 
 			n.opTracker.LastOperation.MarkEnd()
-			if cErr := n.callback.Create(mg.GetName())(err, ctx); cErr != nil {
+			if cErr := n.callback.Create(mgCopy.GetName())(err, ctx); cErr != nil {
 				n.opTracker.logger.Info("Async create callback failed", "error", cErr.Error())
 			}
 		}()
 		defer ph.recoverIfPanic(ctx)
 
 		n.opTracker.logger.Debug("Async create starting...")
-		_, ph.err = n.terraformPluginFrameworkExternalClient.Create(ctx, mg)
+		_, ph.err = n.terraformPluginFrameworkExternalClient.Create(ctx, mgCopy)
 	}()
 
 	return managed.ExternalCreation{}, n.opTracker.LastOperation.Error()
@@ -197,6 +201,10 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context,
 	}
 
 	ctx, cancel := context.WithDeadline(context.Background(), n.opTracker.LastOperation.StartTime().Add(defaultAsyncTimeout))
+	// We deep-copy the managed resource to prevent a data race between the
+	// goroutine we are about to start below and the managed reconciler.
+	// Please see: https://github.com/crossplane/upjet/issues/472
+	mgCopy := mg.DeepCopyObject().(xpresource.Managed)
 	go func() {
 		// The order of deferred functions, executed last-in-first-out, is
 		// significant. The context should be canceled last, because it is
@@ -211,14 +219,14 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context,
 			n.opTracker.logger.Debug("Async update ended.", "error", err)
 
 			n.opTracker.LastOperation.MarkEnd()
-			if cErr := n.callback.Update(mg.GetName())(err, ctx); cErr != nil {
+			if cErr := n.callback.Update(mgCopy.GetName())(err, ctx); cErr != nil {
 				n.opTracker.logger.Info("Async update callback failed", "error", cErr.Error())
 			}
 		}()
 		defer ph.recoverIfPanic(ctx)
 
 		n.opTracker.logger.Debug("Async update starting...")
-		_, ph.err = n.terraformPluginFrameworkExternalClient.Update(ctx, mg)
+		_, ph.err = n.terraformPluginFrameworkExternalClient.Update(ctx, mgCopy)
 	}()
 
 	return managed.ExternalUpdate{}, n.opTracker.LastOperation.Error()