refactor(git): key worker checkouts by GitBranch UID and reclaim orphans

charts/gitops-reverser/README.md

@@ -485,6 +485,27 @@ resources:
     memory: 1Gi
 ```
 
+### Keeping Git checkouts across pod restarts
+
+The controller keeps one Git checkout per `GitBranch` under `/tmp/gitops-reverser-workers`. By
+default that is the pod's `emptyDir`: it survives a container restart, and a new pod clones every
+branch again. To keep the checkouts across pod replacement too, mount a volume there:
+
+```yaml
+volumes:
+  - name: git-checkouts
+    persistentVolumeClaim:
+      claimName: gitops-reverser-git-checkouts
+volumeMounts:
+  - name: git-checkouts
+    mountPath: /tmp/gitops-reverser-workers
+```
+
+Create the `PersistentVolumeClaim` yourself, in the release namespace. `ReadWriteOnce` is enough: the
+chart runs one replica, and the default `deploymentStrategy` stops the old pod before it starts the
+new one. A checkout found on disk is a cache only. The controller fetches and resets it before
+its first write, and deletes every checkout whose `GitBranch` no longer exists.
+
 ## Release Strategy
 
 New versions are automatically released via GitHub Actions:

docs/architecture.md

@@ -1560,19 +1560,25 @@ pushed at once, cooldown or not, because the apply waits for them.
 
 ### Local clones and conflict retry
 
-Local clones live under `/tmp/gitops-reverser-workers/{provider UID}/{branch-component}`. The first
-segment is the `GitProvider`'s `metadata.uid`: globally unique, and minted fresh by the API server for
-every object, so it says which **incarnation** of the provider this is. `spec.url` is immutable and a
-repoint is therefore a recreate, which means a replacement worker cannot inherit, share, or delete the
-directory its predecessor was using. Two providers naming one repository do not share a checkout
-either. A worker built with no provider behind it (the CLI, and tests) has no UID to be unique by and
-falls back to a digest of its remote.
-
-`{branch-component}` is the branch rendered as **one** path component
-([`branchPathComponent`](../internal/git/branch_worker.go)): a readable prefix for whoever is looking at
-the directory, plus a digest of the raw name. Branch names contain slashes, and joining one in would make
-`release/v1`'s directory a child of `release`'s, which is data loss the moment a retired worker deletes
-its own tree. [PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the
+Local clones live under `/tmp/gitops-reverser-workers/{GitBranch UID}`: one directory per
+`GitBranch`, named by its `metadata.uid`. A `GitBranch` pins its branch (`spec.branch` is immutable),
+the branch claim gives a repository branch to one `GitBranch` at a time, and a UID is minted fresh for
+every object, so no two live workers share a directory and a `GitBranch` recreated under its name starts
+in a new one. The branch name is not part of the path, so its slashes cannot nest one checkout inside
+another. The repository is the one thing the UID does not pin: a `GitProvider` recreated against
+another URL keeps the `GitBranch`, so its replacement worker reuses the directory, after the old worker
+has removed the clone in it. A worker never trusts a checkout it finds on disk: its first cycle repoints
+`origin`, fetches and resets.
+
+A retiring worker removes its own directory. The worker sweep, which runs every minute and after
+every `GitTarget` delete, also removes every directory that no live worker uses and no existing
+`GitBranch` names. That covers a `GitBranch` deleted while no worker ran for it, which happens across
+a restart that keeps the disk. The root is an `emptyDir` by default, so the checkouts survive a
+container restart but not a new pod. The
+[chart README](../charts/gitops-reverser/README.md#keeping-git-checkouts-across-pod-restarts) shows how to
+mount a volume there instead.
+
+[PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the
 remote diverged it smart fetches the latest tip, hard resets the local clone, replays the retained pending
 writes against the fresh tip (refreshing commit hashes), and retries up to the attempt limit. This is valid
 because every pending write is rebuilt from sanitized API state; nothing depends on locally edited files.

internal/controller/git_branch_helpers_test.go

@@ -24,7 +24,7 @@ var shopRepo1Main = k8stypes.NamespacedName{Namespace: "shop", Name: gitBranchNa
 func claimWorkerForTest(m *git.WorkerManager, repo git.RepoIdentity) git.WorkerRequest {
 	m.Claims().Observe(git.BranchKeyFor(repo.URL, "main"), git.BranchClaimant{
 		GitBranch: shopRepo1Main,
-		UID:       k8stypes.UID(shopRepo1Main.String()),
+		UID:       "uid-shop-repo1-main",
 		Created:   time.Unix(0, 0),
 	}, true)
 	return git.WorkerRequest{

internal/controller/main_test.go

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package controller
+
+import (
+	"os"
+	"testing"
+
+	"github.com/ConfigButler/gitops-reverser/internal/git"
+)
+
+// TestMain gives this binary its own worker state root: the WorkerManagers these tests start sweep
+// every directory under it that their GitBranches do not name (git.SetWorkerStateRoot).
+func TestMain(m *testing.M) {
+	root, err := os.MkdirTemp("", "gitops-reverser-workers-")
+	if err != nil {
+		panic(err)
+	}
+	git.SetWorkerStateRoot(root)
+
+	code := m.Run()
+	_ = os.RemoveAll(root)
+	os.Exit(code)
+}

internal/git/branch_worker.go

@@ -70,10 +70,12 @@ var (
 // It can serve multiple GitTargets that write to different paths in the same branch.
 // This design ensures serialized commits per branch, preventing merge conflicts.
 type BranchWorker struct {
-	// Identity (immutable after creation). GitBranch is the claim holder this worker writes for;
-	// GitProviderRef and GitProviderNamespace name that GitBranch's GitProvider, whose credential
-	// and signing key it uses.
+	// Identity (immutable after creation). GitBranch is the claim holder this worker writes for, and
+	// GitBranchUID its metadata.uid, which names the checkout; GitProviderRef and
+	// GitProviderNamespace name that GitBranch's GitProvider, whose credential and signing key it
+	// uses.
 	GitBranch            types.NamespacedName
+	GitBranchUID         types.UID
 	GitProviderRef       string
 	GitProviderNamespace string
 	Branch               string
@@ -459,61 +461,39 @@ func NewBranchWorker(
 //nolint:gochecknoglobals // a test seam, like pushAtomicFn above
 var workerStateRoot = filepath.Join("/tmp", "gitops-reverser-workers")
 
-// branchPathComponent renders a branch name as ONE directory component.
-//
-// Git branch names contain slashes — `release/v1` is ordinary — and joining one into a path makes
-// its worker's directory a CHILD of the `release` worker's. That is harmless while nothing deletes
-// anything, and it is a data-loss bug the moment something does: retiring `release` would take the
-// live `release/v1` worker's checkout with it.
-//
-// The readable prefix is for whoever is looking at the directory; the hash of the raw name is what
-// makes it unambiguous, including for the names sanitising would otherwise collapse together
-// (`release/v1` and `release-v1`).
-func branchPathComponent(branch string) string {
-	sum := sha256.Sum256([]byte(branch))
-	digest := hex.EncodeToString(sum[:8])
-
-	readable := make([]rune, 0, len(branch))
-	for _, r := range branch {
-		switch {
-		case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_':
-			readable = append(readable, r)
-		default:
-			readable = append(readable, '-')
-		}
-	}
-	if len(readable) > branchComponentReadableMax {
-		readable = readable[:branchComponentReadableMax]
-	}
-	return string(readable) + "-" + digest
-}
+// SetWorkerStateRoot points every worker's on-disk state at dir, for the whole process. The
+// operator never calls it. A test binary that runs a WorkerManager calls it from TestMain, before
+// any worker exists: the manager's sweep deletes every directory under the root that its own
+// GitBranches do not name, so under the shared default it would delete the checkouts of every
+// other process using that root.
+func SetWorkerStateRoot(dir string) { workerStateRoot = dir }
 
-// branchComponentReadableMax caps the readable half so a long branch name cannot push the path
-// past what a filesystem accepts; the digest that follows is what carries the identity.
-const branchComponentReadableMax = 40
-
-// repoRootPath is where this worker keeps its checkout: the GitProvider's UID, then the branch.
+// repoRootPath is where this worker keeps its checkout: one directory, named by the UID of the
+// GitBranch it writes for.
+//
+// The GitBranch pins the branch (spec.branch is immutable), the claim gives a repository branch to
+// one GitBranch at a time, and EnsureWorker keeps at most one live worker per GitBranch, so no two
+// live workers share a directory. A UID is minted fresh for every object, so a GitBranch deleted
+// and recreated under its name starts in a new directory. And because the branch is not part of
+// the path, a branch name's slashes cannot nest one worker's checkout inside another's.
 //
-// The UID is the whole of the first component, and it replaces three things that were there
-// before: the provider's namespace, its name, and a digest of the remote URL. A UID is globally
-// unique and is minted fresh by the API server for every object, so it says everything those
-// three said and one thing they could not — WHICH INCARNATION of the provider this is. A
-// repointed provider is a recreated one, so its worker cannot inherit, share, or delete the
-// directory its predecessor was using, and no digest has to stand in for that.
+// The repository is the one thing the UID does not pin: the GitProvider is named, not owned, and
+// can be recreated against another URL. EnsureWorker removes the old worker's checkout before the
+// replacement starts in the same directory, and a checkout left behind by a restart is never
+// trusted: a new worker's first cycle repoints origin, fetches and resets.
 func (w *BranchWorker) repoRootPath() string {
-	return filepath.Join(workerStateRoot, w.stateComponent(), branchPathComponent(w.Branch))
+	return filepath.Join(workerStateRoot, w.stateComponent())
 }
 
 // stateComponent names this worker's state directory.
 //
-// A worker built with no GitProvider behind it — the CLI, and tests — has no UID to be unique by,
-// and every one of them would otherwise share one directory per branch. Those workers fall back to
-// a digest of the remote they were built for, which is the only identity they have.
+// A worker built with no GitBranch behind it — tests — has no UID to be unique by. It falls back
+// to a digest of its remote and branch, which is the only identity it has.
 func (w *BranchWorker) stateComponent() string {
-	if w.repo.ProviderUID != "" {
-		return string(w.repo.ProviderUID)
+	if w.GitBranchUID != "" {
+		return string(w.GitBranchUID)
 	}
-	sum := sha256.Sum256([]byte(strings.TrimSpace(w.repo.URL)))
+	sum := sha256.Sum256([]byte(strings.TrimSpace(w.repo.URL) + "\x00" + w.Branch))
 	return "url-" + hex.EncodeToString(sum[:16])
 }
 
@@ -529,18 +509,17 @@ func (w *BranchWorker) stateComponent() string {
 // have that happen to it; the WorkerManager replaces it instead.
 func (w *BranchWorker) repoPath() string { return w.repoRootPath() }
 
-// removeLocalState deletes everything this worker kept on disk: the clone of every remote it
-// worked with, under its own (provider UID, branch) directory.
+// removeLocalState deletes everything this worker kept on disk: its GitBranch's directory.
 //
 // It is called only when a worker is being retired because nothing needs it any more, never on
 // shutdown — a restart wants the clones it left behind, and re-cloning every branch on every
 // restart is exactly the cost the on-disk cache exists to avoid.
 //
-// The identity guard is not defensive programming. repoRootPath joins the UID and the branch into
-// a fixed prefix, so a worker with neither — which tests build — resolves to the ROOT of every
-// worker's state, and deleting that would take out the live workers next to it.
+// Only a worker with a GitBranch UID reclaims anything. That is every worker the WorkerManager
+// starts; one a test builds by hand has a directory derived from whatever identity it was given,
+// down to none at all, and deleting a directory nobody can name is how a live worker's goes too.
 func (w *BranchWorker) removeLocalState() {
-	if w.repo.IsZero() || w.Branch == "" {
+	if w.GitBranchUID == "" {
 		w.Log.V(1).Info("Not removing worker state: the worker has no complete identity")
 		return
 	}

internal/git/claim_helpers_test.go

@@ -21,7 +21,7 @@ func claimForTest(m *WorkerManager, provider, namespace, branch string, repo Rep
 	gitBranch := testGitBranch(provider, namespace, branch)
 	m.Claims().Observe(BranchKeyFor(repo.URL, branch), BranchClaimant{
 		GitBranch: gitBranch,
-		UID:       k8stypes.UID(gitBranch.String()),
+		UID:       testGitBranchUID(gitBranch),
 		Created:   time.Unix(0, 0),
 	}, true)
 	return WorkerRequest{
@@ -32,5 +32,11 @@ func claimForTest(m *WorkerManager, provider, namespace, branch string, repo Rep
 	}
 }
 
+// testGitBranchUID is the UID claimForTest gives gitBranch. Like a real one, it holds no slash: it
+// names the worker's checkout directory.
+func testGitBranchUID(gitBranch k8stypes.NamespacedName) k8stypes.UID {
+	return k8stypes.UID("uid-" + gitBranch.Namespace + "-" + gitBranch.Name)
+}
+
 // testGitBranchName is the name of testGitBranch(provider, namespace, branch).
 func testGitBranchName(provider, branch string) string { return provider + "-" + branch }

internal/git/git_operations_test.go

@@ -48,12 +48,21 @@ func skipIfPublicNetworkUnavailable(t *testing.T, err error) {
 
 // TestMain initializes the controller-runtime logger before running tests.
 // This prevents "log.SetLogger(...) was never called" warnings when code uses log.FromContext().
+// It also gives this binary its own worker state root (SetWorkerStateRoot).
 func TestMain(m *testing.M) {
 	// Initialize controller-runtime logger for all tests
 	logf.SetLogger(zap.New(zap.UseDevMode(true)))
 
+	root, err := os.MkdirTemp("", "gitops-reverser-workers-")
+	if err != nil {
+		panic(err)
+	}
+	SetWorkerStateRoot(root)
+
 	// Run all tests
-	os.Exit(m.Run())
+	code := m.Run()
+	_ = os.RemoveAll(root)
+	os.Exit(code)
 }
 
 func TestCheckRepo_ConnectivityAndMetadata(t *testing.T) {

internal/git/helpers_test.go

@@ -237,6 +237,10 @@ func newTestBranchWorker(
 		nil,
 		BranchWorkerLimits{},
 	)
+	// The GitBranch's UID names the checkout, as it does for every worker the WorkerManager starts,
+	// so it differs wherever the provider UID above does, and per branch too.
+	branchDigest := sha256.Sum256([]byte(string(providerUID) + "\x00" + branch))
+	worker.GitBranchUID = k8stypes.UID("uid-gitbranch-" + hex.EncodeToString(branchDigest[:8]))
 	worker.ctx = context.Background()
 	return worker, nil
 }