refactor(git): hold commit windows in a set, step their clock, and check them against a model

docs/TODO.md

@@ -11,6 +11,13 @@ Nothing open. The 0.54.0 release PR's changelog was checked against what ships o
 
 ## Applier claims and bi-directional editing
 
+- [ ] **A withheld apply rides the next edit.** S1 withholds an apply that lands over an edit decided
+  after the flush mark, and a `Repair` puts the edit back. But the apply did land on the cluster, so
+  the next edit of that object carries the apply's value for every field it did not change, and the
+  window commits the whole object under that author's name: the overwritten edit is lost, and the
+  `Repair` then applies the loss. The failing test is
+  `TestApplyWindow_AWithheldApplyIsNotCommittedUnderTheNextEdit`
+  (`internal/git/apply_window_carry_test.go`, skipped; `GITOPS_REVERSER_KNOWN_BUGS=1` runs it).
 - [ ] **Reopen the render-fidelity gate after a Git repair, automatically.** Today a target on
   `RenderMatchesLive=False` clears only on an explicit recheck (the
   `reconcile.configbutler.ai/requestedAt` annotation, which restarts every scope; the render

docs/design/branch-worker-event-model.md

@@ -324,6 +324,14 @@ Represent intent, decisions, and external work separately so recovery knows what
 These names are illustrative. Use a versioned journal schema when choosing the implementation.
 Facts describe decisions already made; a duplicate command must find its previous decision.
 
+**Give each decision a specific name and a reason**, in the product's words: `SaveAttached`,
+`WindowClosed` with its close reason, rather than a generic `StateChanged` that leaves the reader to
+compare two states. Today those names serve log lines, close reasons and tests, with the identities
+the decision is about (request UID, window ID). They are not a journal schema. Whether restart
+recovery stores durable current state (pending work, deadlines, receipts) or an event history is
+still open; the HA plan decides it from the guarantees it needs and their measured cost. Git
+already keeps the authored history of every published change, which weighs toward current state.
+
 ```mermaid
 flowchart TD
     I[Watch observations and save commands] --> A[Durable admission]

docs/design/concurrent-author-windows.md

@@ -162,10 +162,14 @@ generation change), the worker recomputes every open window's footprint against
 **before** the next write is admitted beside others. A footprint that is now unknown, or that now
 overlaps another window's, closes the windows involved, oldest first, with `path_conflict`.
 
-A last check runs at materialization: the files the write plan touched (`writeBatch.touched`) must
-be inside the window's footprint. A plan that touched more is a footprint bug. The worker cannot
-undo the order of what is already decided, so it logs the paths, counts it in a metric, and falls
-back to one window for the rest of its life. The reference model treats it as a test failure.
+A last check runs at materialization, **before the commit**: every path the write's commit would
+change must be inside the window's footprint. That is the whole batch's changed files;
+`writeBatch.touched` covers one event at a time and is not enough. A write that changes more is a
+footprint bug, and committing it could put an older value over a newer one, so the worker refuses
+it: it undoes the worktree, fails the window's save with the paths in its message, falls back to
+one window for the rest of its life, and asks a resync of the `GitTarget`, which brings Git back to
+the cluster without depending on the order of the windows. The reference model treats it as a test
+failure.
 
 ### Every close trigger keeps its exceptions
 
@@ -220,6 +224,25 @@ is the point of the change, and it needs saying in `docs/configuration.md` when
 - `SaveInProgress` is unchanged: it is already per author and `GitTarget`.
 - Each request settles exactly once, whichever trigger closes its window.
 
+### The window decisions have specific names
+
+Concurrent windows do not depend on persistence: they keep explicit identities, deadlines and
+decision reasons, and leave how a restart recovers them to the
+[event model](branch-worker-event-model.md#commands-facts-and-effects) and the HA plan. Each decision
+gets a specific name, used in log lines, close reasons and the reference model. A log line carries
+the branch with the window ID, since the ID resets with the worker; the ID is never a metric label.
+
+| Decision | Logged with | When |
+| --- | --- | --- |
+| `WindowOpened` | window ID, author, `GitTarget`, footprint, revision, opening time | a write opens a window |
+| `WriteJoinedWindow` | window ID, the write, the footprint after it | a write joins an open window |
+| `FootprintReproved` | window ID, new revision, the footprint or unknown | the revision changes under an open window |
+| `WindowClosed` | window ID, close reason, the writes it holds | a window closes |
+| `SingleWindowFallback` | the paths a plan touched outside its footprint | the materialization check fails |
+
+The window ID is the opening sequence number, unique for the life of one worker. Deadlines follow
+from the opening time, the latest write and the timers.
+
 ## How to test it: a reference model
 
 "Git equals the cluster at the end" is too weak a check for this change. A later write can repair a
@@ -300,8 +323,9 @@ the reference model checks every commit.
 - **P1. A footprint that is too small.** A write lands somewhere its footprint did not name (an
   existing document in an unexpected file, a generator source, a removal that edits a shared file),
   and two windows commit to one file in the wrong order. *Mitigation:* footprints are proven or
-  unknown, never guessed; the table's "no" rows stay serial; the reference model includes shared
-  files and generator layouts.
+  unknown, never guessed; the table's "no" rows stay serial; the check before the commit refuses a
+  write that changes more than its footprint; the reference model includes shared files and
+  generator layouts.
 - **P2. A close trigger loses its exception.** A push hint or an apply flush that closes a window
   carrying a save splits a save that is protected today. *Mitigation:* the trigger table above is the
   specification, and each exception has a test in stage 3.
@@ -314,8 +338,8 @@ the reference model checks every commit.
   every step, as well as once drained.
 - **P6. A stale footprint proof.** A proof made against one tree is used after an own commit, a
   replay or a `GitTarget` change moved the index. *Mitigation:* proofs carry their revision, are
-  recomputed before the next concurrent admission, and the materialization check falls back to one
-  window if a plan ever touches a file outside its footprint.
+  recomputed before the next concurrent admission, and the check before the commit refuses a write
+  that changes a file outside its footprint.
 - **P7. The flush starts waiting again.** A change that makes `apply_due` wait for windows to close
   on their own reintroduces the starvation S1 removed. *Mitigation:* the flush closes every window
   at once, as today; only the save exception holds it.

docs/design/window-field-ownership-and-apply-hold.md

@@ -329,7 +329,7 @@ its Kustomization suspended and the `ArgoCDClaim` keeps automated sync off.
 
 Window timers cannot bound the hold. A window's `max_duration` is not fixed: attaching a
 `CommitRequest` replaces the window's timers with the request's, running from the attach
-(`attachToOpenWindow`, `internal/git/commit_request_attach_loop.go`). A hold that waited for the
+(`attachToWindow`, `internal/git/commit_request_attach_loop.go`). A hold that waited for the
 windows' own deadlines could be extended by every attach.
 
 So the hold has an **absolute deadline, measured from when it began**. It ends at the first of:

internal/git/applier_claim.go

@@ -365,18 +365,23 @@ func (l *branchWorkerEventLoop) writeBusy() bool {
 	return len(l.pendingCRs) > 0 || l.recovery.active || l.w.worktreeDirty()
 }
 
-// closeWindowForPushHint closes the open commit window early when a Git host has said the branch may
-// have moved, so its writes are published, and replayed onto the move, now rather than when the
-// window's timers allow. A window a CommitRequest holds is left to its save.
+// closeWindowForPushHint closes the open commit windows early when a Git host has said the branch may
+// have moved, so their writes are published, and replayed onto the move, now rather than when the
+// windows' timers allow. A window a CommitRequest holds is left to its save.
 func (l *branchWorkerEventLoop) closeWindowForPushHint() {
 	hinted := l.pushHinted
 	l.pushHinted = false
-	if !hinted || l.openWindow == nil || l.openWindow.pendingCR != nil || l.w.applyRequester == nil {
+	if !hinted || l.w.applyRequester == nil {
 		return
 	}
-	l.w.Log.Info("A Git host reported a push; closing the commit window to publish it now",
-		"branch", l.w.Branch)
-	l.finalizeOpenWindowWithReason(windowFinalizeReasonForeignCommit)
+	for _, window := range l.commitWindows.all() {
+		if window.pendingCR != nil {
+			continue
+		}
+		l.w.Log.Info("A Git host reported a push; closing the commit window to publish it now",
+			"branch", l.w.Branch)
+		l.finalizeWindow(window, windowFinalizeReasonForeignCommit)
+	}
 }
 
 // serviceAppliers settles the reports that came back, and sends whatever attempts are due, after a

internal/git/applier_claim_test.go

@@ -379,6 +379,29 @@ func TestCloseWindowForPushHint_PublishesTheOpenWindowNow(t *testing.T) {
 	assert.Len(t, loop.pendingWrites, 1, "the window's write is decided, to be published")
 }
 
+// TestCloseWindowForPushHint_LeavesASavesWindowToTheSave: a window a CommitRequest holds closes
+// when its save says, not on a push hint.
+func TestCloseWindowForPushHint_LeavesASavesWindowToTheSave(t *testing.T) {
+	worker, _, _ := setupCommitPushSplitWorker(t)
+	createPlainGitTarget(t, worker, "team-a", "team-a")
+	worker.applyRequester = &recordingRequester{}
+	loop := newBranchWorkerEventLoop(worker, time.Hour)
+	t.Cleanup(loop.stopTimers)
+
+	loop.handleQueueItem(WorkItem{Request: &WriteRequest{
+		Events: []Event{configMapTargetEvent("edited", "alice", "team-a")},
+	}})
+	require.NotNil(t, loop.openWindow, "an hour-long window is still open")
+	id := commitRequestID{Namespace: "team-a", Name: "save", UID: "uid-save"}
+	loop.openWindow.pendingCR = &id
+
+	loop.handleRefreshRequest(&RefreshRequest{PushNotice: true})
+	loop.closeWindowForPushHint()
+
+	assert.NotNil(t, loop.openWindow, "the save's window is still open")
+	assert.Empty(t, loop.pendingWrites)
+}
+
 // applierLedgerFixture is a real Git server, a GitTarget naming an applier, and an event loop whose
 // requests are recorded.
 func applierLedgerFixture(t *testing.T, slug string) (*ledgerFixture, *branchWorkerEventLoop, *recordingRequester) {

internal/git/apply_window_carry_test.go

@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package git
+
+import (
+	"os"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3"
+	itypes "github.com/ConfigButler/gitops-reverser/internal/types"
+)
+
+// TestApplyWindow_AWithheldApplyIsNotCommittedUnderTheNextEdit is a known bug (docs/TODO.md, "A
+// withheld apply rides the next edit"), written now and skipped until it is fixed; set
+// GITOPS_REVERSER_KNOWN_BUGS=1 to run it.
+//
+// S1 withholds an apply that lands over an edit decided after the flush mark, and a Repair puts the
+// edit back once it is published. But the apply did land on the cluster, and a window holds whole
+// objects: the next edit of that object, by the same author, carries the apply's value for every
+// field it did not touch, and its commit writes that value under the author's name. The edit the
+// apply overwrote is lost, and the Repair puts the loss back on the cluster.
+func TestApplyWindow_AWithheldApplyIsNotCommittedUnderTheNextEdit(t *testing.T) {
+	if os.Getenv("GITOPS_REVERSER_KNOWN_BUGS") == "" {
+		t.Skip("known bug, docs/TODO.md: a withheld apply rides the next edit")
+	}
+
+	// edit is alice's live update of y, holding both of its keys.
+	edit := func(a, b string) Event {
+		event := configMapTargetEvent("y", "alice", echoTarget)
+		event.Operation = string(itypes.OperationUpdate)
+		event.Object.Object["data"] = map[string]interface{}{"a": a, "b": b}
+		return event
+	}
+	// lateApply is the applier writing y back to what the flushed commit holds, after the edit.
+	lateApply := func() Event {
+		event := edit("a1", "b1")
+		event.UserInfo.Username = "system:serviceaccount:flux-system:kustomize-controller"
+		event.LastFieldManagers = []string{FluxFieldManager}
+		event.LastFieldManagedAt = time.Now().Add(time.Hour)
+		return event
+	}
+	// flushOverAnEdit makes an apply due while x2 sits in the open window, and lets the flush publish
+	// it only after alice's edit a=a2 is decided: the attempt goes for a commit that lacks the edit.
+	flushOverAnEdit := func(t *testing.T, slug string) (*echoFixture, *recordingRequester) {
+		f := newEchoFixture(t, slug, configv1alpha3.PruneOnEvent, map[string]string{"x": "x1"})
+		f.writeWindow(edit("a1", "b1")) // y as Git holds it before anyone edits
+		requester := f.withClaim()
+		f.loop.lastPushAt = time.Now()
+
+		f.writeWindow(echoEvent("x", "x2"))
+		restore := failPushes(t, f.worker)
+		f.echoApplier().repair = true
+		f.loop.endWake(0)
+
+		f.writeWindow(edit("a2", "b1")) // E1, decided after the mark
+		restore()
+		f.loop.retry.due = time.Now().Add(-time.Second)
+		f.loop.runRetry()
+		f.loop.endWake(0)
+		require.Len(t, requester.taken(), 1, "the apply is sent without E1")
+		return f, requester
+	}
+
+	t.Run("in the window that holds the edit", func(t *testing.T) {
+		f, _ := flushOverAnEdit(t, "carry-same-window")
+		require.NotNil(t, f.loop.openWindow, "E1 is collecting in alice's window")
+
+		f.writeWindow(lateApply())
+		require.True(t, f.echoApplier().repair, "the apply over E1 is withheld, and a Repair is pending")
+		f.writeWindow(edit("a1", "b2")) // E2: alice changes b on the object the apply wrote
+		f.loop.lastPushAt = time.Time{}
+		require.True(t, f.loop.finalizeOpenWindow())
+		f.loop.pushPending()
+
+		require.Equal(t, "alice", gitIn(t, f.repoDir, "log", "-1", "--format=%an", "main"))
+		assert.Contains(t, f.onMain(echoFile("y")), "a: a2",
+			"E1 is alice's, and the apply that overwrote it was withheld; Git loses it")
+	})
+
+	t.Run("after the edit is published", func(t *testing.T) {
+		f, _ := flushOverAnEdit(t, "carry-next-window")
+		f.loop.lastPushAt = time.Time{}
+		require.True(t, f.loop.finalizeOpenWindow())
+		f.loop.pushPending()
+		require.Contains(t, f.onMain(echoFile("y")), "a: a2", "precondition: E1 is published")
+
+		f.writeWindow(lateApply())
+		require.True(t, f.echoApplier().repair, "the apply over E1 is withheld, and a Repair is pending")
+		f.writeWindow(edit("a1", "b2"))
+		f.loop.lastPushAt = time.Time{}
+		require.True(t, f.loop.finalizeOpenWindow())
+		f.loop.pushPending()
+
+		require.Equal(t, "alice", gitIn(t, f.repoDir, "log", "-1", "--format=%an", "main"))
+		assert.Contains(t, f.onMain(echoFile("y")), "a: a2",
+			"alice's commit of b reverts her own E1 to the withheld apply's a1")
+	})
+}