fix(commitrequest): settle every save before the controller exits

.coverage-baseline

@@ -1 +1 @@
-82.5
+82.6

cmd/main.go

@@ -331,6 +331,11 @@ func main() {
 	// A worker's change to a CommitRequest takes the request's next step at once, on either path.
 	commitRequestWakes := controller.NewCommitRequestWakes(workerManager.CommitRequestChanges())
 	fatalIfErr(mgr.Add(commitRequestWakes), "unable to add the CommitRequest wakes")
+	// At shutdown the workers settle every save they hold as they stop, and the CommitRequest
+	// controllers stop with them: the requests either path attached have their outcomes written once
+	// the workers have stopped, before the process exits.
+	commitRequestsInFlight := &controller.CommitRequestsInFlight{Log: ctrl.Log.WithName("commitrequests")}
+	workerManager.SetAfterWorkersStopped(commitRequestsInFlight.Settle)
 	commitRequestServer := &controller.CommitRequestServer{
 		Client:      mgr.GetClient(),
 		APIReader:   mgr.GetAPIReader(),
@@ -340,6 +345,7 @@ func main() {
 		FactStreams: watchMgr.FactStreams,
 		TTL:         cfg.commitRequestTTL,
 		Wakes:       commitRequestWakes,
+		InFlight:    commitRequestsInFlight,
 		Log:         ctrl.Log.WithName("commitrequests"),
 	}
 	fatalIfErr(mgr.Add(commitRequestServer), "unable to add the CommitRequest server")
@@ -429,6 +435,7 @@ func main() {
 		FactStreams: watchMgr.FactStreams,
 		TTL:         cfg.commitRequestTTL,
 		Wakes:       commitRequestWakes,
+		InFlight:    commitRequestsInFlight,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "CommitRequest")
 		os.Exit(1)

docs/TODO.md

@@ -17,9 +17,20 @@ should land before the next tag is cut (the last release is v0.55.0). Checked ag
 
 ## Applier claims and bi-directional editing
 
-- [ ] **Persist the withheld-field record.** The fields a withheld apply reverted are kept in memory
-  (`internal/git/withheld_fields.go`), so after a restart the next edit of such an object carries the
-  apply's values into Git again, until the `Repair` lands.
+- [ ] **Restart recovery: persist current state.** A graceful stop pushes its windows and writes the
+  outcomes of the saves they carried (`CommitRequestsInFlight.Settle`); everything else the branch
+  worker keeps in memory is lost on any restart, and a crash, or a stop past its grace period, loses
+  all of it. Nothing is persisted. What must survive: the
+  open windows with absolute deadlines, author, attribution, message and membership; each attached
+  save's `attachDeadline`, `seq` and window; save outcomes until their TTL; the pending writes; the
+  withheld-field records (`internal/git/withheld_fields.go`: after a restart the next edit of such an
+  object carries the apply's values into Git again, until the `Repair` lands). "Git took the push,
+  then the process died" is made safe by writing a push intent {old, new, request→SHA} before every
+  `PushAtomic` and comparing it with the remote tip on restart: commits carry no trailers, and SHAs
+  are not reproducible. Two restore traps: the startup cold replay (no Redis) closes any restored
+  window (`resync_flush.go`), and `finalizeWindow` drops a window while render fidelity is Unknown,
+  which it is after a restart. The worker state root `/tmp` is an `emptyDir` a new pod loses.
+  [branch-worker-event-model.md](design/branch-worker-event-model.md#saves-lose-execution-identity-across-a-restart).
 - [ ] **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/api-first-publication.md

@@ -310,8 +310,10 @@ sequenceDiagram
 A local no-diff result still consults the remote before reporting `AlreadyPresent`. A competing
 Git edit can make that same captured API object require a real commit after replay.
 
-A worker that stops while still holding the write fails the request with an error rather than
-leaving it to time out, so a shutdown mid-publication is reported as a failure and not as silence.
+A worker that stops while still holding the write pushes it first. If that last push fails, it fails
+the request with an error rather than leaving it to time out, so a shutdown mid-publication is
+reported as a failure and not as silence. Either outcome is written to the request's status before
+the process exits.
 
 Use `Pushed=True` plus `status.commit` when a particular commit must exist in Git. `Ready=True` also
 covers successful outcomes with no commit. Status visibility follows the controller's polling

docs/configuration.md

@@ -3337,7 +3337,12 @@ request produced a pushed commit:
   does not mean the commit was applied.
 
 A request is served again after GitOps Reverser restarts: the path that owns it finds its own
-claim, and resumes with the author it recorded.
+claim, and resumes with the author it recorded. A graceful stop, such as an upgrade or a rollout,
+first finishes the saves it holds: a request collecting its window is committed with its message,
+pushed, and its outcome written before the pod exits. A request still waiting for a window is
+picked up again by the next pod. A crash, or a stop that runs out of its grace period, loses the
+open window: the next pod's startup replay writes its edits under a generated message, and the
+request attaches again as if new, so it can end `NoWindow`.
 
 ### How long a request lives
 

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

@@ -24,8 +24,9 @@ The write-path refactor in [`gittarget-branch-worker-pending-writes.md`](gittarg
 what this section asked for, in the order it set: ordered pending writes, one materializer for the
 checkout, and one retry deadline; then the watch's admission boundary (5a), Git operation deadlines
 (6), intake pause and resume (5b), and the publication status (5c). None of it depends on a storage
-backend or on HA. A saturated branch pauses intake while its recovery loop stays alive; a process
-restart still loses the pending writes and unanswered saves held in memory.
+backend or on HA. A saturated branch pauses intake while its recovery loop stays alive. A graceful
+stop pushes the pending writes and writes the saves' outcomes before it exits; a crash, or a stop
+that runs out of time, still loses the pending writes and unanswered saves held in memory.
 
 What comes next is the [transition boundary](#prepare-the-transition-boundary-for-later-durability):
 recording timers, attaches, withdrawals, and refreshes as transitions, then the HA plan's journal.
@@ -554,9 +555,11 @@ The live producer is
 attribution and never independently supplies mirrored object state.
 
 The [event router](../../internal/watch/event_router.go) binds an already-held request to its
-original worker during retirement. Today shutdown is best effort: it tries to finalize and push,
-then fails unresolved requests and discards unhandled work. Durable worker retirement needs an
-explicit transfer or terminal decision before that responsibility can be released.
+original worker during retirement. Today shutdown is best effort, within a 10s budget: it finalizes
+and pushes, fails the requests riding a write it could not push, and discards unhandled work; the
+controller then writes every outcome the stopped workers decided (`CommitRequestsInFlight.Settle`).
+Durable worker retirement needs an explicit transfer or terminal decision before that responsibility
+can be released.
 
 Snapshots establish render fidelity even while ordinary live writes are refused. Suspension
 suppresses writes while allowing observation. A single dispatcher-level write gate would block

docs/spec/commitrequest-design.md

@@ -320,10 +320,17 @@ created successfully nor a later source snapshot proves that the worker accepted
 Resource watches and save commands arrive independently: a save is not a barrier covering every
 Kubernetes mutation before its creation.
 
-Pausing the live worker preserves accepted work in memory. Restarting the process does not preserve
-its attachments or outcome receipts. Startup replays current watched state, subject to pruning,
-but cannot recreate the same save history. A push accepted immediately before a crash can therefore be
-followed by another empty commit when the request is resent. A lost push response also needs remote
+Pausing the live worker preserves accepted work in memory. A graceful stop settles what it holds
+before the process exits: every worker closes its windows, pushes, and settles the saves it carries,
+and once every worker has stopped the controller writes each settled outcome to its request's status
+through the path that attached it, as one more conditional step (`CommitRequestsInFlight.Settle`).
+Both are bounded (10s for the workers, 5s for the statuses) to fit the chart's 20s grace period; what
+runs over is left as a crash leaves it. A request still waiting for a window, or whose attach was
+still queued, is not committed; the next process attaches it again from its status. Restarting the
+process does not otherwise preserve its attachments or outcome receipts: nothing is persisted.
+Startup replays current watched state, subject to pruning, but cannot recreate the same save history.
+A push accepted immediately before a crash can therefore be followed by another empty commit when
+the request is resent. A lost push response also needs remote
 evidence; equal tree content alone cannot prove that an empty save's message was published. Durable
 save recovery belongs to the [future HA plan](../future/ha-gittarget-distribution-plan.md).
 

docs/spec/e2e-serial-registry.md

@@ -11,7 +11,7 @@ under bounded Ginkgo parallelism (Phase 2.5 of
 > The two audit-consumer specs, `bi_directional`, and `aggregated_apiserver` have
 > all been de-serialized (see the "De-serialized" section). `crd_lifecycle` was
 > de-serialized and then **re-serialized** (commit `3d249e3`) when a residual
-> cluster-wide CRD-discovery churn re-flaked it. The current Serial set is the four
+> cluster-wide CRD-discovery churn re-flaked it. The current Serial set is the five
 > singleton-controller specs, `crd_lifecycle`, and `playground`.
 
 ## Rule
@@ -39,6 +39,7 @@ the `full-core` leg beside the restart specs instead of at the end of the longes
 | [test/e2e/restart_reconcile_e2e_test.go](../../test/e2e/restart_reconcile_e2e_test.go) | `Restart Reconcile Safety` | Rollout-restarts the controller deployment. | The controller is a singleton; restarting it disrupts in-flight reconciles/commits for every other spec. |
 | [test/e2e/flux_restart_recovery_e2e_test.go](../../test/e2e/flux_restart_recovery_e2e_test.go) | `Flux Restart Recovery` | Rollout-restarts the controller deployment while a Flux apply runs, and labels a node so that apply's Deployment can schedule. | The controller is a singleton, as for `Restart Reconcile Safety`; a node label is cluster-wide. |
 | [test/e2e/bi_directional_restart_e2e_test.go](../../test/e2e/bi_directional_restart_e2e_test.go) | `Bi Directional restart with work made while down (Flux)`, `(Argo CD)` | Scales the controller deployment to zero and back, to make edits and a Git-side push while the operator is down. | The controller is a singleton, as for `Restart Reconcile Safety`. |
+| [test/e2e/commit_request_restart_e2e_test.go](../../test/e2e/commit_request_restart_e2e_test.go) | `Save Request across a controller restart` | Scales the controller deployment to zero and back, to stop it while a save collects its window. | The controller is a singleton, as for `Restart Reconcile Safety`. |
 | [test/e2e/image_refresh_test.go](../../test/e2e/image_refresh_test.go) | `image refresh dependency chain` | Changes the controller image / redeploys the controller. | Same singleton controller; an image swap perturbs all concurrent specs. |
 | [test/e2e/crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go) | `CRD Lifecycle` | Installs/deletes a cluster-scoped CRD and watches all `customresourcedefinitions` cluster-wide, asserting exact Git file presence/absence. | A concurrent CRD install/delete bumps the global discovery-catalog generation and re-resolves every GitTarget's watched-type tables, delaying this spec's reconcile (CRD file appears late / post-delete sweep lags — the 649↔673 flake). A cluster-wide catalog bump cannot be scoped by name. |
 | [test/e2e/tilt_playground_e2e_test.go](../../test/e2e/tilt_playground_e2e_test.go) | `playground` | Reusable manual-playground fixture: fixed (non-randomized) `tilt-playground` namespace and `playground` repo/provider names, preserved across runs. | A singleton by construction — its resource names are intentionally stable so `task playground-*` can re-attach, so per-run name isolation does not apply. |

internal/controller/commitrequest_controller.go

@@ -38,6 +38,9 @@ type CommitRequestFinalizer interface {
 	// answer: resolved with git.ErrCommitRequestWithdrawn when it is cancelled, the real outcome
 	// when the worker got there first, or resolved=false with the worker's phase while it decides.
 	WithdrawCommitRequest(ctx context.Context, attach git.AttachCommitRequest) (git.FinalizeResult, bool, error)
+	// LookupCommitRequestOutcome reports the outcome the worker that holds the request decided, and
+	// false while it decided none or no worker holds it. It sends nothing to the worker.
+	LookupCommitRequestOutcome(ctx context.Context, attach git.AttachCommitRequest) (git.FinalizeResult, bool, error)
 	// LookupCommitRequestApply reports where the apply a request with catchUp:
 	// AfterPush asked for stands, and false while the worker knows nothing of it.
 	LookupCommitRequestApply(ctx context.Context, attach git.AttachCommitRequest) (git.SaveApply, bool, error)
@@ -169,6 +172,10 @@ type CommitRequestReconciler struct {
 	// request is polled only as the fallback. Nil polls every request.
 	Wakes *CommitRequestWakes
 
+	// InFlight remembers the requests this reconciler attached, so a shutdown writes the outcomes
+	// their workers decide while they drain. Nil remembers nothing.
+	InFlight *CommitRequestsInFlight
+
 	streamsOnce sync.Once
 	streams     *commitRequestStreams
 }
@@ -193,7 +200,7 @@ func (r *CommitRequestReconciler) core() *commitRequestCore {
 	r.streamsOnce.Do(func() { r.streams = &commitRequestStreams{set: r.FactStreams} })
 	return &commitRequestCore{
 		reader: r.Client, finalizer: r.Finalizer, authors: r.Authors, streams: r.streams, ttl: r.TTL,
-		wakeFallback: r.Wakes.fallback(),
+		wakeFallback: r.Wakes.fallback(), inFlight: r.InFlight,
 	}
 }
 

internal/controller/commitrequest_controller_unit_test.go

@@ -36,6 +36,8 @@ type fakeFinalizer struct {
 	err      error
 
 	calls []git.AttachCommitRequest
+	// outcomeLookups are the lookups of a decided outcome, which send nothing.
+	outcomeLookups []git.AttachCommitRequest
 
 	// The withdraw reply. Left unset, the withdraw answers like the router does before it cancels
 	// anything: with the worker's outcome, or its phase while unresolved. withdrawPending makes an
@@ -53,6 +55,15 @@ type fakeFinalizer struct {
 	applyLookups int
 }
 
+// LookupCommitRequestOutcome answers with the canned outcome, as a worker that decided it does, and
+// records the lookup: nothing is sent to the worker.
+func (f *fakeFinalizer) LookupCommitRequestOutcome(
+	_ context.Context, attach git.AttachCommitRequest,
+) (git.FinalizeResult, bool, error) {
+	f.outcomeLookups = append(f.outcomeLookups, attach)
+	return f.result, f.resolved, f.err
+}
+
 func (f *fakeFinalizer) LookupCommitRequestApply(
 	_ context.Context, _ git.AttachCommitRequest,
 ) (git.SaveApply, bool, error) {

internal/controller/commitrequest_core.go

@@ -110,6 +110,8 @@ type commitRequestCore struct {
 	streams *commitRequestStreams
 	// ttl is how long a finished request is kept, written as its delete-after time; zero writes none.
 	ttl time.Duration
+	// inFlight remembers the requests this core attached, for the shutdown to settle; nil remembers none.
+	inFlight *CommitRequestsInFlight
 }
 
 // serve takes one step for the request key on path p, and says how soon to take the next. Zero means
@@ -119,12 +121,17 @@ func (c *commitRequestCore) serve(
 ) (time.Duration, error) {
 	cr, err := p.store.get(ctx, key)
 	if err != nil {
+		if apierrors.IsNotFound(err) {
+			c.inFlight.forget(p, key)
+		}
 		return 0, client.IgnoreNotFound(err)
 	}
 	if !cr.DeletionTimestamp.IsZero() {
+		c.inFlight.forget(p, key)
 		return 0, nil
 	}
 	if commitRequestIsTerminal(cr) {
+		c.inFlight.forget(p, key)
 		return c.serveFinished(ctx, p, cr)
 	}
 	if c.finalizer == nil {
@@ -483,6 +490,8 @@ func (c *commitRequestCore) drive(
 	ctx context.Context, p commitRequestPath, cr *configbutleraiv1alpha3.CommitRequest,
 	target *configbutleraiv1alpha3.GitTarget, provider *configbutleraiv1alpha3.KubeProvider,
 ) (time.Duration, error) {
+	// Remembered before anything reaches a worker, so whatever it decides is written at shutdown.
+	c.inFlight.track(c, p, client.ObjectKeyFromObject(cr))
 	attach := c.attachFor(cr)
 	if target != nil && string(target.UID) != cr.Status.GitTargetUID {
 		admitted, refusal, err := c.admit(ctx, p, cr, target, provider)

internal/controller/commitrequest_decide.go

@@ -93,6 +93,7 @@ func (c *commitRequestCore) decide(
 	if err != nil || !written {
 		return commitRequestRequeueNow, err
 	}
+	c.inFlight.forget(p, client.ObjectKeyFromObject(current))
 	if !afterClaim {
 		recordCommitRequestOutcome(ctx, o.metricOutcome(), target)
 	}

internal/controller/commitrequest_server.go

@@ -108,7 +108,10 @@ type CommitRequestServer struct {
 	// Wakes wakes a request's steps as soon as its branch worker reports a change, so a registered
 	// request is polled only as the fallback. Nil polls every request.
 	Wakes *CommitRequestWakes
-	Log   logr.Logger
+	// InFlight remembers the requests the server attached, so a shutdown writes the outcomes their
+	// workers decide while they drain, through the identity that serves them. Nil remembers nothing.
+	InFlight *CommitRequestsInFlight
+	Log      logr.Logger
 
 	mu       sync.Mutex
 	watchers map[k8stypes.NamespacedName]*commitRequestWatcher
@@ -600,7 +603,7 @@ func (s *CommitRequestServer) core() *commitRequestCore {
 	s.streamsOnce.Do(func() { s.streams = &commitRequestStreams{set: s.FactStreams} })
 	return &commitRequestCore{
 		reader: s.Client, controlRequests: s.APIReader, finalizer: s.Finalizer, authors: s.Authors,
-		streams: s.streams, ttl: s.TTL, wakeFallback: s.Wakes.fallback(),
+		streams: s.streams, ttl: s.TTL, wakeFallback: s.Wakes.fallback(), inFlight: s.InFlight,
 	}
 }