kptdev#998: Do not allow deletion of a PR that is an upstream PR of a cloned PR (kptdev#453)

* Block upstream revision getting deleted if it has a downstream dependent

* Add unit tests for upstream dependency check during deletes

* Add log for dependency check failures

* Remove dependency checks for main package revision

* Add test checkUpstreamDependencies in porch registry

* Update pkg/registry/porch/packagerevision.go

Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

* Remove edit task from upstream dependency checks

* Address comments and fix safemap's Range function

* Add docs for upstream delete protection and update unit test

* Add package tracking for delete log

* Update unit tests and remove references to 'dependent' in docs

* Change all refs of 'dependency/dependent'

---------

Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

Author: kushnaidu

docs/content/en/docs/2_concepts/upstream-downstream.md

@@ -93,3 +93,28 @@ management through Porch.
 - Customizations in downstream packages are preserved during updates
 - A package can be both upstream (to others) and downstream (from another)
 - PackageVariant automates the upstream → downstream lifecycle
+- **Deletion protection**: Upstream PackageRevisions cannot be deleted while downstream PackageRevisions exist
+
+## Deletion Protection
+
+Porch prevents deletion of upstream PackageRevisions when downstream PackageRevisions exist.
+
+**Protection mechanism**: Attempting to delete an upstream PackageRevision with existing downstream PackageRevisions will:
+- Fail with a Forbidden error
+- Identify which downstream PackageRevision blocks the deletion
+- Require deletion of downstream PackageRevisions first
+
+**Deletion order**: Always delete from downstream to upstream:
+1. Delete all downstream PackageRevisions
+2. Delete the upstream PackageRevision
+
+**Example**:
+```
+blueprints/nginx-template:v1 (upstream)
+         ↓
+deployments/prod-nginx:v1 (downstream)
+```
+
+To delete nginx-template:v1, first delete prod-nginx:v1, then delete nginx-template:v1.
+
+This ensures downstream packages don't lose their source reference.

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/deleting-packages.md

@@ -319,10 +319,11 @@ graph TD
 - Published deletions remove Git tags and references
 - Deletion is permanent and cannot be undone
 
-**Dependency Considerations:**
+**Upstream Reference Considerations:**
 
-- Check if other PackageRevisions depend on the one being deleted
-- Deleting upstream packages may affect downstream clones
+- Upstream PackageRevisions cannot be deleted while downstream PackageRevisions reference them
+- Delete downstream PackageRevisions first, then you can delete the upstream PackageRevisions
+- The error message identifies which downstream PackageRevision is blocking deletion
 - Consider the impact on deployed workloads
 
 ---
@@ -348,6 +349,15 @@ Error: cannot delete published package revision directly, use propose-delete fir
 - Check RBAC permissions with the `kubectl auth can-i delete packagerevisions -n default` command
 - Verify your service account has proper deletion roles
 
+**Cannot delete upstream PackageRevision:**
+
+```bash
+Error from server (Forbidden): cannot delete package revision, it is referenced as upstream by: <downstream-packagerevision-name>
+```
+
+- See "Upstream Reference Considerations" in Safety Considerations above
+- Delete the downstream PackageRevision first, then retry
+
 **Deletion proposal stuck:**
 
 - Check the PackageRevision status with the `porchctl rpkg get <name> -o yaml` command

pkg/cache/crcache/cache.go

@@ -119,3 +119,41 @@ func (c *Cache) GetRepository(repoKey repository.RepositoryKey) repository.Repos
 func (c *Cache) CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error {
 	return externalrepo.CheckRepositoryConnection(ctx, repositorySpec, c.options.ExternalRepoOptions)
 }
+
+func (c *Cache) FindAllUpstreamReferencesInRepositories(ctx context.Context, namespace, prName string) (string, error) {
+	var downstreamName string
+	c.repositories.Range(func(key, value any) bool {
+		cachedRepo := value.(*cachedRepository)
+		if cachedRepo.Key().Namespace != namespace {
+			return true
+		}
+		cachedRepo.mutex.RLock()
+		for _, pr := range cachedRepo.cachedPackageRevisions {
+			// Skip main branch packages (revision = -1) as they are auto-managed
+			if pr.Key().Revision == -1 {
+				continue
+			}
+			apiPR, err := pr.GetPackageRevision(ctx)
+			if err != nil {
+				continue
+			}
+			for _, task := range apiPR.Spec.Tasks {
+				var matched bool
+				switch task.Type {
+				case "clone":
+					matched = task.Clone != nil && task.Clone.Upstream.UpstreamRef != nil && task.Clone.Upstream.UpstreamRef.Name == prName
+				case "upgrade":
+					matched = task.Upgrade != nil && task.Upgrade.NewUpstream.Name == prName
+				}
+				if matched {
+					downstreamName = pr.KubeObjectName()
+					cachedRepo.mutex.RUnlock()
+					return false
+				}
+			}
+		}
+		cachedRepo.mutex.RUnlock()
+		return true
+	})
+	return downstreamName, nil
+}

pkg/cache/crcache/cache_test.go

@@ -30,9 +30,12 @@ import (
 	fakecache "github.com/nephio-project/porch/pkg/cache/fake"
 	"github.com/nephio-project/porch/pkg/cache/repomap"
 	cachetypes "github.com/nephio-project/porch/pkg/cache/types"
+	"github.com/nephio-project/porch/pkg/externalrepo/fake"
 	"github.com/nephio-project/porch/pkg/externalrepo/git"
 	externalrepotypes "github.com/nephio-project/porch/pkg/externalrepo/types"
 	"github.com/nephio-project/porch/pkg/repository"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	k8sfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -323,3 +326,122 @@ func createMetadataStoreFromArchive(t *testing.T, testPath, name string) meta.Me
 		Metas: metas,
 	}
 }
+
+func TestFindUpstreamReference(t *testing.T) {
+	ctx := context.Background()
+	cache := &Cache{repositories: repomap.SafeRepoMap{}}
+	repoKey := repository.RepositoryKey{Namespace: "test-ns", Name: "test-repo"}
+	mockRepo := &cachedRepository{
+		key:                    repoKey,
+		cachedPackageRevisions: make(map[repository.PackageRevisionKey]*cachedPackageRevision),
+	}
+	_, _ = cache.repositories.LoadOrCreate(repoKey, func() (repository.Repository, error) {
+		return mockRepo, nil
+	})
+
+	// Create second repository for cross-repo testing
+	repo2Key := repository.RepositoryKey{Namespace: "test-ns", Name: "test-repo2"}
+	mockRepo2 := &cachedRepository{
+		key:                    repo2Key,
+		cachedPackageRevisions: make(map[repository.PackageRevisionKey]*cachedPackageRevision),
+	}
+	_, _ = cache.repositories.LoadOrCreate(repo2Key, func() (repository.Repository, error) {
+		return mockRepo2, nil
+	})
+
+	addDownstream := func(repo *cachedRepository, downstreamPkgName, taskType, upstreamRefName string) {
+		key := repository.PackageRevisionKey{
+			PkgKey:        repository.PackageKey{RepoKey: repo.key, Package: downstreamPkgName},
+			WorkspaceName: "v1",
+		}
+		var task porchapi.Task
+		switch taskType {
+		case "clone":
+			task = porchapi.Task{
+				Type:  "clone",
+				Clone: &porchapi.PackageCloneTaskSpec{Upstream: porchapi.UpstreamPackage{UpstreamRef: &porchapi.PackageRevisionRef{Name: upstreamRefName}}},
+			}
+		case "upgrade":
+			task = porchapi.Task{
+				Type:    "upgrade",
+				Upgrade: &porchapi.PackageUpgradeTaskSpec{NewUpstream: porchapi.PackageRevisionRef{Name: upstreamRefName}},
+			}
+		}
+		repo.cachedPackageRevisions[key] = &cachedPackageRevision{
+			PackageRevision: &fake.FakePackageRevision{
+				PrKey:           key,
+				PackageRevision: &porchapi.PackageRevision{Spec: porchapi.PackageRevisionSpec{Tasks: []porchapi.Task{task}}},
+			},
+		}
+	}
+
+	tests := map[string]struct {
+		namespace           string
+		upstreamPkgToDelete string
+		repo                *cachedRepository
+		downstreamPkgName   string
+		upstreamRefName     string
+		taskType            string
+		wantDep             string
+	}{
+		"no downstream": {
+			namespace:           "test-ns",
+			upstreamPkgToDelete: "test-repo.upstream.v1",
+			wantDep:             "",
+		},
+		"find clone downstream": {
+			namespace:           "test-ns",
+			upstreamPkgToDelete: "test-repo.upstream.v1",
+			repo:                mockRepo,
+			downstreamPkgName:   "downstream",
+			upstreamRefName:     "test-repo.upstream.v1",
+			taskType:            "clone",
+			wantDep:             "test-repo.downstream.v1",
+		},
+		"find upgrade downstream": {
+			namespace:           "test-ns",
+			upstreamPkgToDelete: "test-repo.upstream.v1",
+			repo:                mockRepo,
+			downstreamPkgName:   "upgrade",
+			upstreamRefName:     "test-repo.upstream.v1",
+			taskType:            "upgrade",
+			wantDep:             "test-repo.upgrade.v1",
+		},
+		"task without downstream": {
+			namespace:           "test-ns",
+			upstreamPkgToDelete: "test-repo.upstream.v1",
+			repo:                mockRepo,
+			downstreamPkgName:   "other",
+			upstreamRefName:     "test-repo.different.v1",
+			taskType:            "clone",
+			wantDep:             "",
+		},
+		"different namespace": {
+			namespace:           "other-ns",
+			upstreamPkgToDelete: "test-repo.upstream.v1",
+			wantDep:             "",
+		},
+		"cross-repo downstream": {
+			namespace:           "test-ns",
+			upstreamPkgToDelete: "test-repo.base-pkg.v1",
+			repo:                mockRepo2,
+			downstreamPkgName:   "derived",
+			upstreamRefName:     "test-repo.base-pkg.v1",
+			taskType:            "clone",
+			wantDep:             "test-repo2.derived.v1",
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			mockRepo.cachedPackageRevisions = make(map[repository.PackageRevisionKey]*cachedPackageRevision)
+			mockRepo2.cachedPackageRevisions = make(map[repository.PackageRevisionKey]*cachedPackageRevision)
+			if tt.taskType != "" {
+				addDownstream(tt.repo, tt.downstreamPkgName, tt.taskType, tt.upstreamRefName)
+			}
+			dep, err := cache.FindAllUpstreamReferencesInRepositories(ctx, tt.namespace, tt.upstreamPkgToDelete)
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantDep, dep)
+		})
+	}
+}

pkg/cache/dbcache/dbcache.go

@@ -151,3 +151,7 @@ func (c *dbCache) GetRepository(repoKey repository.RepositoryKey) repository.Rep
 func (c *dbCache) CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error {
 	return externalrepo.CheckRepositoryConnection(ctx, repositorySpec, c.options.ExternalRepoOptions)
 }
+
+func (c *dbCache) FindAllUpstreamReferencesInRepositories(ctx context.Context, namespace, prName string) (string, error) {
+	return findUpstreamRefsFromDB(ctx, namespace, prName)
+}

pkg/cache/dbcache/dbpackagerevisionsql.go

@@ -454,3 +454,28 @@ func pkgRevDeleteFromDB(ctx context.Context, prk repository.PackageRevisionKey)
 
 	return err
 }
+
+func findUpstreamRefsFromDB(ctx context.Context, namespace, prName string) (string, error) {
+	_, span := tracer.Start(ctx, "dbpackagerevisionsql::findUpstreamRefsFromDB")
+	defer span.End()
+
+	// Match newUpstreamRef (upgrade) or nested upstreamRef (clone)
+	// Exclude main branch packages (revision = -1) as they are auto-managed
+	sqlStatement := `
+		SELECT k8s_name FROM package_revisions
+		WHERE k8s_name_space=$1
+		  AND revision != -1
+		  AND tasks::text ~ ('"(upstreamRef|newUpstreamRef)":\{"name":"' || $2 || '"')
+		LIMIT 1
+	`
+
+	var downstreamName string
+	err := GetDB().db.QueryRow(ctx, sqlStatement, namespace, prName).Scan(&downstreamName)
+	if err == sql.ErrNoRows {
+		return "", nil
+	}
+	if err != nil {
+		return "", err
+	}
+	return downstreamName, nil
+}

pkg/cache/dbcache/dbpackagerevisionsql_test.go

@@ -16,6 +16,7 @@ package dbcache
 
 import (
 	"database/sql"
+	"fmt"
 	"time"
 
 	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
@@ -656,3 +657,100 @@ func (t *DbTestSuite) assertPackageRevLatestIs(expectedLatest int, prList []*dbP
 		t.Equal(expectedLatest, latestPrRev)
 	}
 }
+
+func (t *DbTestSuite) TestFindUpstreamReference() {
+	mockCache := mockcachetypes.NewMockCache(t.T())
+	cachetypes.CacheInstance = mockCache
+	mockCache.EXPECT().GetRepository(mock.Anything).Return(&dbRepository{})
+
+	upstreamRepo := t.createTestRepo("test-ns", "upstream")
+	downstreamRepo := t.createTestRepo("test-ns", "downstream")
+
+	upstreamPkg := t.createTestPkg(upstreamRepo.Key(), "basepkg")
+	upstreamPkg.repo = upstreamRepo
+
+	upstreamPR := dbPackageRevision{
+		pkgRevKey: repository.PackageRevisionKey{
+			PkgKey:        upstreamPkg.Key(),
+			WorkspaceName: "v1",
+			Revision:      1,
+		},
+		meta:      metav1.ObjectMeta{Name: "upstream.basepkg.v1", Namespace: "test-ns"},
+		lifecycle: porchapi.PackageRevisionLifecyclePublished,
+	}
+	err := pkgRevWriteToDB(t.Context(), &upstreamPR)
+	t.Require().NoError(err)
+
+	tests := map[string]struct {
+		namespace string
+		pkgName   string
+		wsName    string
+		taskType  string
+		wantDep   string
+	}{
+		"no downstream": {
+			namespace: "test-ns",
+			wantDep:   "",
+		},
+		"clone task": {
+			namespace: "test-ns",
+			pkgName:   "edge-cluster",
+			wsName:    "v1",
+			taskType:  "clone",
+			wantDep:   "downstream.edge-cluster.v1",
+		},
+		"upgrade task": {
+			namespace: "test-ns",
+			pkgName:   "edge-cluster",
+			wsName:    "v2",
+			taskType:  "upgrade",
+			wantDep:   "downstream.edge-cluster.v2",
+		},
+		"different namespace": {
+			namespace: "other-ns",
+			wantDep:   "",
+		},
+		"non-existent upstream": {
+			namespace: "test-ns",
+			wantDep:   "",
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func() {
+			if tt.taskType != "" {
+				downstreamPkg := t.createTestPkg(downstreamRepo.Key(), tt.pkgName)
+				downstreamPkg.repo = downstreamRepo
+
+				var task porchapi.Task
+				switch tt.taskType {
+				case "clone":
+					task = porchapi.Task{Type: porchapi.TaskTypeClone, Clone: &porchapi.PackageCloneTaskSpec{
+						Upstream: porchapi.UpstreamPackage{UpstreamRef: &porchapi.PackageRevisionRef{Name: upstreamPR.meta.Name}},
+					}}
+				case "upgrade":
+					task = porchapi.Task{Type: porchapi.TaskTypeUpgrade, Upgrade: &porchapi.PackageUpgradeTaskSpec{
+						NewUpstream: porchapi.PackageRevisionRef{Name: upstreamPR.meta.Name},
+					}}
+				}
+				pr := dbPackageRevision{
+					pkgRevKey: repository.PackageRevisionKey{PkgKey: downstreamPkg.Key(), WorkspaceName: tt.wsName},
+					meta:      metav1.ObjectMeta{Name: fmt.Sprintf("downstream.%s.%s", tt.pkgName, tt.wsName), Namespace: "test-ns"},
+					lifecycle: porchapi.PackageRevisionLifecycleDraft,
+					tasks:     []porchapi.Task{task},
+				}
+				t.Require().NoError(pkgRevWriteToDB(t.Context(), &pr))
+				defer pkgRevDeleteFromDB(t.Context(), pr.Key())
+				defer pkgDeleteFromDB(t.Context(), downstreamPkg.Key())
+			}
+			downstream, err := findUpstreamRefsFromDB(t.Context(), tt.namespace, upstreamPR.meta.Name)
+			t.Require().NoError(err)
+			t.Equal(tt.wantDep, downstream)
+		})
+	}
+
+	err = repoDeleteFromDB(t.Context(), upstreamRepo.Key())
+	t.NoError(err)
+	err = repoDeleteFromDB(t.Context(), downstreamRepo.Key())
+	t.NoError(err)
+}

pkg/cache/repomap/saferepomap.go

@@ -42,7 +42,14 @@ func (s *SafeRepoMap) LoadAndDelete(key repository.RepositoryKey) (repository.Re
 }
 
 func (s *SafeRepoMap) Range(f func(key, value any) bool) {
-	s.syncMap.Range(f)
+	s.syncMap.Range(func(key, value any) bool {
+		loader := value.(*repoLoader)
+		// Skip nil loaders (defensive) or repos that failed creation or haven't completed initialization
+		if loader == nil || loader.repo == nil {
+			return true
+		}
+		return f(key, loader.repo)
+	})
 }
 
 type repoLoader struct {