[sunib]

A graceful stop (an upgrade, a rollout, a node drain) now finishes the saves it holds: every branch
worker closes its windows and pushes them, and once every worker has stopped, the outcome of each
save they settled is written to its CommitRequest's status before the process exits. Restart
recovery stays in memory: nothing is persisted.

The race, verified

Two defects, both on the clean-shutdown path. The second is the one the plan named; the first was
under it, and made it worse.

1. The shutdown push never reached the remote. A branch worker runs on a context derived from
the manager's, so the moment the manager is canceled every worker's context is canceled too, and
handleShutdown then finalized the window, committed locally, and pushed on that canceled context.
Over a real transport the push was refused at once (getAuthFromSecret reads uncached Secrets
through the API, and go-git's HTTP and SSH calls honor the context): the save attached to the window
failed in memory with "worker stopped before the commit reached the remote", and the edit was lost
with the pod's emptyDir checkout. Unit tests never saw it because go-git v6's file:// transport
ignores the context. TestShutdown_TheLastWindowIsPushedOnceTheManagerCancels (real git
http-backend) fails on the base with exactly that error, and passes here.

2. Nothing wrote the outcome. controller-runtime v0.25 stops runnables by group: non-leader
runnables, then every leader-election runnable at once, then the caches. The WorkerManager and the
CommitRequest controllers (and CommitRequestServer, and the wakes) are all in that one group. On
cancel the controllers shut their queues and finish in-flight reconciles on the canceled context; the
workers drain concurrently, and every outcome they decide lands in crOutcomes with no step left to
read it. The next pod's core resumes from status (Ready=Unknown, CollectingWindow), re-sends the
attach, and a worker that never saw the request registers it as new: NoWindow, or its message on a
later window of the same author. With CommitEmpty, a second commit.

The new e2e spec is queued locally against an unmodified copy of the base tree to show it end to end
there too; its result will follow as a PR comment.

Deployment facts that shape the budget: the chart sets terminationGracePeriodSeconds: 20 and
maxSurge: 0; the manager's GracefulShutdownTimeout is the 30s default (so the kubelet's SIGKILL is
the real bound); no leader election is configured.

The fix

• Workers stop on one context and work on another (BranchWorker.Start). The parent's cancel is
  the stop signal; the work context is WithoutCancel(parent), ended workerShutdownBudget (10s)
  after the stop, or when the loop exits. So the last window, its push and its saves' settlement run
  live, bounded. All workers see the cancel together, so the drain is about as long as the slowest
  worker, not the sum. A worker retired or replaced while the process runs gets the same last push,
  which RetireGitBranch already promised.
• The outcomes are written after the workers stop (CommitRequestsInFlight). The core remembers
  every request it attaches, with the path that attached it (control cluster, or a KubeProvider's
  watch through its own dynamic client), and forgets it once an outcome is written or the request is
  gone. WorkerManager.SetAfterWorkersStopped runs Settle once every worker has stopped, on a
  fresh context bounded at 5s, still inside the leader-election group's stop, so the caches the core
  reads are still running. Settle takes one step per remembered request (4 at a time): read it,
  ask the router for the outcome the holding worker decided (LookupCommitRequestOutcome, new: it
  sends nothing), and write it with the core's decide, so every rule holds: the owner path writes,
  the write is conditional on resourceVersion, and a conflict re-reads and re-applies the owner's
  outcome without restarting the save.

Why this shape over the alternatives:

• Reorder the runnables (stop the worker manager before the controllers, or have the controllers
  wait on a "workers drained" signal) needs either the worker manager outside the leader-election
  group, which would run workers on non-leaders, or a controller that keeps reconciling after its
  context is canceled. Neither is available from controller-runtime without wrapping its controller,
  and the reconcile queue would still be asynchronous: nothing tells you when the last outcome is
  written. A synchronous last pass knows when it is done and what it left.
• Have the worker write status itself would move status writing out of the core, and the
  source-cluster path's client into internal/git. The pass reuses the core's own decide, through
  the same store each path already uses.
• settle is narrower than a normal step on purpose: no attach, no withdraw, no phase write. A
  request no worker decided anything for keeps its status, so the next process resumes it exactly
  as it does today, rather than seeing a misleading WaitingForWorker or a "no branch worker within
  the safety window" failure written by a process that is exiting.

Total: 10s + 5s inside the chart's 20s. The audit ingress and the push receiver are
leader-election runnables too: the receiver's NeedLeaderElection returns true, and
auditServerRunnable defines none, so controller-runtime puts it in that group. The non-leader
group holds only the certificate watchers, which stop at once. So the two servers' HTTP drains
(at most 10s each, normally at once) run alongside the workers' drain rather than before it, and
the 15s still bounds the group. On overrun a push is cut off and its saves fail, or Settle stops early and logs the
requests it left; those behave as today.

What a deploy now looks like for an editor

• A save collecting its window when the pod gets SIGTERM is committed with its message, pushed, and
  reports Ready=True Committed with the SHA before the pod is gone. The next pod sees a finished
  request.
• A save whose window was committed but not yet pushed is pushed now too.
• A save whose last push fails reports Stalled=True with the reason, instead of starting over.
• A save still waiting for a window (or whose attach was still on the queue) is not committed; the
  next pod attaches it again from its status, with a fresh attachTimeout, and it attaches to the
  next window its author opens there. Unchanged.

Stated limits

• In memory only: nothing is persisted. A crash, an OOM kill, or a stop past its grace period
  behaves as before.
• fix(git): keep a withheld apply's values out of the next edit #466's withheld-field records are still lost on any restart.
• docs/TODO.md's "Persist the withheld-field record" is folded into one "Restart recovery: persist
  current state" entry: what must survive, the push-intent record that makes "Git took the push,
  then the process died" safe, and the two restore traps (the cold replay closing restored windows
  via resync_flush.go, and finalizeWindow dropping a window while render fidelity is Unknown),
  plus the emptyDir worker state root.
• The overlap of two pods. The chart's maxSurge: 0 does not stop Kubernetes from creating the
  replacement as soon as the old pod is Terminating, and no leader election is configured, so a fast
  new pod can re-attach a request the old one is still settling. The old pod's push and status write
  should take about a second after SIGTERM, inside a new pod's start, but it is a race. The e2e
  manifest (config/deployment.yaml) uses the default surge, so its replacement pod is Ready before
  the old one even stops. That is why the e2e spec scales to zero instead of running rollout restart.

Tests

• internal/git/shutdown_drain_test.go (real git http-backend): the last window is pushed after the
  manager cancels, and the save reports the remote's commit; a stalled last push ends at the budget
  and fails the save; the manager runs the settle step after every worker stopped, on a live
  bounded context, with the outcome already decided.
• internal/controller/commitrequest_settle_test.go: the reconciler's attached save is written from
  the worker's lookup alone, no attach; a shutdown failure is written; an undecided request is left
  untouched; a conflict re-applies the outcome; a source-cluster request is written on its own cluster
  through the dynamic client after the server stopped; a canceled context writes nothing.
• e2e Save Request across a controller restart (label commit-request-restart, Serial, registered
  in docs/spec/e2e-serial-registry.md; runs in the full-core leg): edit, attach a request with
  5m timers (the schema's maximum), scale the controller to zero, read Committed with its SHA while no controller runs,
  scale back up, assert exactly one commit with the message and that the request still reports it.

Validation on the final commit: task fmt, generate, manifests, vet, lint and test are green
(coverage 82.6%, baseline raised). The shutdown tests also pass 5x under -race. Not yet run locally:
e2e. The focused restart spec is queued on the shared e2e cluster, once on this branch and once on
the unfixed base tree, and the CI legs run the full suite. Results follow as a PR comment.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 982c9420-4862-4b2f-9614-6ca6146f90a2

Commits

Reviewing files that changed from the base of the PR and between f5e5e26 and 5ced158.

Files selected for processing: 23

• .coverage-baseline
• cmd/main.go
• docs/TODO.md
• docs/api-first-publication.md
• docs/configuration.md
• docs/design/branch-worker-event-model.md
• docs/spec/commitrequest-design.md
• docs/spec/e2e-serial-registry.md
• internal/controller/commitrequest_controller.go
• internal/controller/commitrequest_controller_unit_test.go
• internal/controller/commitrequest_core.go
• internal/controller/commitrequest_decide.go
• internal/controller/commitrequest_server.go
• internal/controller/commitrequest_settle.go
• internal/controller/commitrequest_settle_test.go
• internal/controller/commitrequest_wakes_test.go
• internal/git/branch_worker.go
• internal/git/shutdown_drain_test.go
• internal/git/worker_manager.go
• internal/git/worker_repo_identity_test.go
• internal/watch/event_router.go
• internal/watch/event_router_outcome_test.go
• test/e2e/commit_request_restart_e2e_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 90.52632% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/controller/commitrequest_settle.go	86.2%	8 Missing ⚠️
internal/controller/commitrequest_core.go	80.0%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[sunib]

Local e2e, focused on the new spec Save Request across a controller restart (label commit-request-restart), on the shared k3d cluster:

• This branch (2751e97): passed. Ran 1 of 177 Specs, 1 Passed. The stopping controller wrote Ready=True Committed with the SHA while it was scaled to zero. After it came back, main held exactly one commit with the request's message, and the request still reported that commit.
• Baseline, an unmodified copy of fix/withheld-apply-carry (ace401b) with only the spec added: failed as expected. With the controller at zero replicas, the request stayed at Ready=Unknown (reason="CollectingWindow") Stalled=False Pushed=Unknown commit="" until the 30s wait ran out. The save was in flight when the process exited, and nothing wrote its outcome.

An earlier baseline attempt failed for an unrelated reason: the spec asked for 10m timers, and the schema caps them at 5m. That was fixed before the push. The full suite runs in the CI legs.