From 51a21e5d7b61ff50859b79182acd9670dfe31572 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 07:48:15 +0000 Subject: [PATCH 1/6] fix(git): retry a failed publication on its own schedule A failed push, or a failed rebuild before it, stopped the push timer and armed nothing. A branch that then went quiet kept its work indefinitely, and a CommitRequest riding it stayed WaitingForPush, because the controller never fails a request the worker holds. A branch that kept receiving commits did the opposite: lastPushAt advances only on success, so every new commit tried the remote that had just refused. The failure now schedules the next attempt itself: 10s, doubling to 5m, on the existing push timer. Commits before that deadline accumulate locally, and a success resets it. A failure caused by a missing parent stays with parent recovery's probe schedule, so the two never both spend a connection. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/api-first-publication.md | 12 +- docs/design/push-cooldown.md | 4 +- internal/git/branch_worker.go | 37 +++-- internal/git/publication_retry.go | 84 +++++++++++ internal/git/publication_retry_test.go | 185 +++++++++++++++++++++++++ 5 files changed, 301 insertions(+), 21 deletions(-) create mode 100644 internal/git/publication_retry.go create mode 100644 internal/git/publication_retry_test.go diff --git a/docs/api-first-publication.md b/docs/api-first-publication.md index 856aeba4..0023fe95 100644 --- a/docs/api-first-publication.md +++ b/docs/api-first-publication.md @@ -385,12 +385,12 @@ request-rate limiter. A contention cycle attempts up to three pushes without a d between them, fetching and rebuilding when the remote moved. Different branch workers can publish independently. -After a push or retained-write recovery fails, `pushPending` stops its timer and keeps the work. -There is currently no dedicated retry timer. Later work that schedules publication can retry it; -a quiet branch has no bounded retry time. A `CommitRequest` poll alone does not schedule another -push. Conversely, continued arrivals after an expired cooldown can provoke frequent failed -attempts. An independent, bounded failure backoff would address both cases while preserving -silence when there is no pending work. +After a push or retained-write recovery fails, the worker keeps the work and schedules the next +attempt itself: 10s after the first failure, doubling to at most 5m, whether or not anything else +arrives. Commits made before that deadline accumulate locally rather than each trying the remote +again, and a successful push resets the schedule. A failure caused by a missing parent branch is +retried on parent recovery's probe schedule instead, so the two never both spend a connection. A +branch with nothing pending stays silent. The byte threshold forces finalization but does not free retained writes during a remote outage. It is not a hard memory ceiling. The separate branch queue has `1000` slots by default and drops diff --git a/docs/design/push-cooldown.md b/docs/design/push-cooldown.md index b5116004..930cd36e 100644 --- a/docs/design/push-cooldown.md +++ b/docs/design/push-cooldown.md @@ -1,6 +1,8 @@ # The push cooldown: what it still buys, and what removing it would cost -> **design**: open, nothing built. Index: [`../INDEX.md`](../INDEX.md) +> **design**: open. Option C (§7), the failure backoff, is built in +> [`publication_retry.go`](../../internal/git/publication_retry.go); the success cooldown is +> unchanged and options D and E remain open. Index: [`../INDEX.md`](../INDEX.md) > Date: 2026-09-21. > Related: [`../api-first-publication.md`](../api-first-publication.md), > [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index ca7a920f..882c4619 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -1085,7 +1085,12 @@ type branchWorkerEventLoop struct { lastPushAt time.Time commitTimer *time.Timer - pushTimer *time.Timer + // pushTimer fires the next push: when the success cooldown ends, or when a failed + // publication's retry is due (publicationRetry). + pushTimer *time.Timer + // publicationRetry is the schedule of a failed publication's next attempt. See + // publication_retry.go. + publicationRetry publicationRetry // deferredHeals holds heal resyncs (periodic re-anchors, removed-type sweeps) parked while a // commit window is open, so a heal never force-finalizes (steals) that window — including a @@ -1768,11 +1773,16 @@ func (l *branchWorkerEventLoop) dropOpenWindow(pendingCR *commitRequestID, cause // cooldown has elapsed (or has never fired), and otherwise schedules a // one-shot pushTimer to fire when the cooldown expires. While the cooldown // is active, additional commits accumulate locally; when the timer fires, -// all pending commits go to the remote in a single push. +// all pending commits go to the remote in a single push. After a failed +// publication the retry's deadline governs instead, so new commits do not +// each spend a push on a remote that just refused one. func (l *branchWorkerEventLoop) maybeSchedulePush() { if len(l.pendingWrites) == 0 { return } + if l.awaitingPublicationRetry() { + return + } if l.lastPushAt.IsZero() { l.pushPending() return @@ -1790,16 +1800,12 @@ func (l *branchWorkerEventLoop) maybeSchedulePush() { // pushPending publishes any retained pending writes that already exist as local // commits. On success, pendingWrites is cleared and lastPushAt advances. On // failure (transient or after exhausting replay retries), pendingWrites stays -// in place, safe but unpublished. -// -// Nothing schedules its own retry: the push timer is stopped on failure and no replacement is -// armed, so the work moves again only when the next commit calls maybeSchedulePush. On a branch -// that then goes quiet, it waits indefinitely. That gap predates the conditional fetch and is not -// fixed here; docs/design/push-cooldown.md §7 option C is the bounded failure backoff that closes -// it, and says why it is a change of its own. +// in place, safe but unpublished, and the push timer is armed for the next +// attempt (publication_retry.go), so the work lands without another commit. func (l *branchWorkerEventLoop) pushPending() { if len(l.pendingWrites) == 0 { l.stopPushTimer() + l.notePublicationSettled() return } if l.w.awaitingParentProbe() { @@ -1813,21 +1819,23 @@ func (l *branchWorkerEventLoop) pushPending() { // report success without sending anything, and settle work that exists nowhere. Rebuild first, // and keep the writes rather than publish a lie if that fails. if err := l.recoverRetainedWrites(); err != nil { - l.w.Log.Error(err, "Cannot publish until the retained writes are rebuilt; keeping them", - "pendingWrites", len(l.pendingWrites)) l.stopPushTimer() l.noteParentUnavailable(err) + l.notePublicationFailed() + l.w.Log.Error(err, "Cannot publish until the retained writes are rebuilt; keeping them", + "pendingWrites", len(l.pendingWrites), "retryAt", l.publicationRetry.nextAttempt) return } if err := l.w.pushPendingCommits(l.pendingWrites); err != nil { - l.noteParentUnavailable(err) - l.w.Log.Error(err, "Push failed; pending writes retained for retry", - "pendingWrites", len(l.pendingWrites)) // Leave pendingWrites in place; do NOT advance lastPushAt — the // design specifies lastPushAt only advances on a successful push. A // CommitRequest riding a retained write stays unresolved while we retry. l.stopPushTimer() + l.noteParentUnavailable(err) + l.notePublicationFailed() + l.w.Log.Error(err, "Push failed; pending writes retained for retry", + "pendingWrites", len(l.pendingWrites), "retryAt", l.publicationRetry.nextAttempt) return } @@ -1839,6 +1847,7 @@ func (l *branchWorkerEventLoop) pushPending() { l.pendingWritesBytes = 0 l.lastPushAt = time.Now() l.stopPushTimer() + l.notePublicationSettled() l.noteRecoveryPublished() } diff --git a/internal/git/publication_retry.go b/internal/git/publication_retry.go new file mode 100644 index 00000000..8f98a72f --- /dev/null +++ b/internal/git/publication_retry.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import "time" + +// This file is the worker's retry of a publication that failed: retained writes whose push, or +// whose rebuild before the push, did not succeed. +// +// The cooldown in maybeSchedulePush spaces SUCCESSFUL pushes; it says nothing about failed ones, +// because lastPushAt advances only on success. Without a schedule of its own a failure had two +// outcomes, both wrong: a branch that then went quiet kept its work indefinitely (and any +// CommitRequest riding it stayed WaitingForPush, since the controller never fails a request the +// worker holds), and a branch that kept receiving commits spent one failed push on each of them. +// +// So a failure opens a retry: the next attempt is due one backoff later, 10s doubling to 5m, and +// the push timer is armed for it whether or not anything else arrives. Until then new commits +// accumulate locally instead of pushing. A successful push, or nothing left to push, closes it. +// +// A failure caused by a missing parent is not retried here. Parent recovery owns that work, with +// its own probe schedule shared across the worker's targets (parent_recovery.go); retrying it on a +// second schedule would spend connections that schedule exists to save. The same holds for any +// failure while that obligation is open: its deadline retries the retained writes. + +//nolint:gochecknoglobals // vars, not consts, so a test can run the real loop on a short schedule. +var ( + publicationRetryInitialBackoff = 10 * time.Second + publicationRetryMaxBackoff = 5 * time.Minute +) + +const publicationRetryBackoffFactor = 2 + +// publicationRetry is the loop's schedule for retrying a failed publication. The zero value is +// "no failure outstanding". Loop-goroutine only. +type publicationRetry struct { + backoff time.Duration + nextAttempt time.Time +} + +// pending reports whether a failed publication is waiting for its next attempt. +func (r publicationRetry) pending() bool { + return !r.nextAttempt.IsZero() +} + +// notePublicationFailed schedules the next attempt after a failed push or rebuild, unless parent +// recovery owns the retry. +func (l *branchWorkerEventLoop) notePublicationFailed() { + if l.recovery.active { + l.publicationRetry = publicationRetry{} + return + } + r := &l.publicationRetry + if r.backoff == 0 { + r.backoff = publicationRetryInitialBackoff + } else { + r.backoff = min(publicationRetryBackoffFactor*r.backoff, publicationRetryMaxBackoff) + } + r.nextAttempt = l.w.now().Add(r.backoff) + l.armPushTimerAt(r.nextAttempt) +} + +// notePublicationSettled closes the retry: the work was published, or nothing is retained. +func (l *branchWorkerEventLoop) notePublicationSettled() { + l.publicationRetry = publicationRetry{} +} + +// awaitingPublicationRetry reports that a failed publication's next attempt is not due yet, so a +// new commit must not push early. It makes sure the attempt is still scheduled. +func (l *branchWorkerEventLoop) awaitingPublicationRetry() bool { + r := l.publicationRetry + if !r.pending() || !l.w.now().Before(r.nextAttempt) { + return false + } + if l.pushTimer == nil { + l.armPushTimerAt(r.nextAttempt) + } + return true +} + +// armPushTimerAt (re)arms the push timer to fire at the given time on the worker's clock. +func (l *branchWorkerEventLoop) armPushTimerAt(at time.Time) { + l.stopPushTimer() + l.pushTimer = time.NewTimer(max(at.Sub(l.w.now()), 0)) +} diff --git a/internal/git/publication_retry_test.go b/internal/git/publication_retry_test.go new file mode 100644 index 00000000..05a0397e --- /dev/null +++ b/internal/git/publication_retry_test.go @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// A failed publication schedules its own retry: a branch that goes quiet after the failure still +// publishes, a branch that keeps receiving commits does not spend a push on each of them, and a +// failure caused by a missing parent stays with parent recovery's schedule. These pin all three, +// and the CommitRequest that rides the retained write. + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// failNextPushes makes the next n push attempts fail as an unreachable remote would, and counts every +// attempt, failed or not. +func failNextPushes(t *testing.T, n int32) *atomic.Int32 { + t.Helper() + original := pushAtomicFn + var attempts atomic.Int32 + pushAtomicFn = func(ctx context.Context, repo *git.Repository, rootHash plumbing.Hash, + rootBranch plumbing.ReferenceName, auth []gitclient.Option) (PushOutcome, error) { + if attempts.Add(1) <= n { + return PushOutcome{}, errors.New("dial tcp: connection refused") + } + return original(ctx, repo, rootHash, rootBranch, auth) + } + t.Cleanup(func() { pushAtomicFn = original }) + return &attempts +} + +// firePushTimer does what the event loop does when the push timer fires. +func firePushTimer(l *branchWorkerEventLoop) { + l.pushTimer = nil + l.pushPending() +} + +func seedMain(t *testing.T) func(string) { + return func(repoDir string) { + simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "main\n") + } +} + +// The gap this closes: after one failed push, a branch that receives nothing else used to keep its +// work indefinitely. The retry is armed by the failure itself. +func TestPublicationRetry_AQuietBranchPublishesAfterAFailedPush(t *testing.T) { + f := newRealServerNewBranch(t, "retry-quiet", seedMain(t)) + clock := useTestClock(f.worker) + attempts := failNextPushes(t, 1) + + liveWrite(f.loop, "cm1") + require.Equal(t, int32(1), attempts.Load(), "the first push was attempted, and failed") + require.Len(t, f.loop.pendingWrites, 1, "the work is retained") + assert.True(t, f.ref("feature").IsZero()) + require.NotNil(t, f.loop.pushTimer, "the failure armed the push timer") + assert.Equal(t, clock.Now().Add(publicationRetryInitialBackoff), f.loop.publicationRetry.nextAttempt) + + clock.advance(publicationRetryInitialBackoff) + firePushTimer(f.loop) + + assert.Equal(t, int32(2), attempts.Load()) + assert.False(t, f.ref("feature").IsZero(), "published with no further write") + assert.Empty(t, f.loop.pendingWrites) + assert.False(t, f.loop.publicationRetry.pending(), "a success closes the retry") + assert.Nil(t, f.loop.pushTimer) +} + +// The mirror image: lastPushAt advances only on success, so before this an expired cooldown let +// every new commit try the remote that had just refused. Commits now accumulate until the retry is +// due, and a second failure doubles the wait. +func TestPublicationRetry_NewCommitsDoNotBypassTheBackoff(t *testing.T) { + f := newRealServerNewBranch(t, "retry-arrivals", seedMain(t)) + clock := useTestClock(f.worker) + attempts := failNextPushes(t, 2) + + liveWrite(f.loop, "cm1") + require.Equal(t, int32(1), attempts.Load()) + + liveWrite(f.loop, "cm2") + liveWrite(f.loop, "cm3") + assert.Equal(t, int32(1), attempts.Load(), "commits before the deadline do not push") + require.Len(t, f.loop.pendingWrites, 3) + require.NotNil(t, f.loop.pushTimer, "and the retry stays scheduled") + + clock.advance(publicationRetryInitialBackoff) + firePushTimer(f.loop) + require.Equal(t, int32(2), attempts.Load(), "the retry fails again") + assert.Equal(t, clock.Now().Add(2*publicationRetryInitialBackoff), f.loop.publicationRetry.nextAttempt, + "and the next wait is twice as long") + + clock.advance(publicationRetryInitialBackoff) + liveWrite(f.loop, "cm4") + assert.Equal(t, int32(2), attempts.Load(), "still not due") + + clock.advance(publicationRetryInitialBackoff) + firePushTimer(f.loop) + assert.Equal(t, int32(3), attempts.Load()) + assert.Empty(t, f.loop.pendingWrites, "all four writes published in one push") + assert.False(t, f.ref("feature").IsZero()) + assert.False(t, f.loop.publicationRetry.pending()) +} + +// The schedule doubles up to its cap, and a settled publication starts it over. +func TestPublicationRetry_BackoffDoublesToTheCapAndResets(t *testing.T) { + w := &BranchWorker{Log: logr.Discard()} + clock := useTestClock(w) + loop := newBranchWorkerEventLoop(w, time.Second) + t.Cleanup(loop.stopTimers) + + var waits []time.Duration + for range 8 { + loop.notePublicationFailed() + waits = append(waits, loop.publicationRetry.nextAttempt.Sub(clock.Now())) + } + assert.Equal(t, []time.Duration{ + 10 * time.Second, 20 * time.Second, 40 * time.Second, 80 * time.Second, + 160 * time.Second, 5 * time.Minute, 5 * time.Minute, 5 * time.Minute, + }, waits) + + loop.notePublicationSettled() + assert.False(t, loop.publicationRetry.pending()) + loop.notePublicationFailed() + assert.Equal(t, clock.Now().Add(publicationRetryInitialBackoff), loop.publicationRetry.nextAttempt) +} + +// A push that fails because the parent branch is missing belongs to parent recovery, which retries +// it on the worker's probe schedule. A second schedule here would spend the connections that one +// exists to save. +func TestPublicationRetry_ParentRecoveryOwnsAMissingParent(t *testing.T) { + f := newRealServerNewBranch(t, "retry-parent", func(repoDir string) { + simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "main\n") + gitIn(t, repoDir, "branch", "release", "main") + }) + useTestClock(f.worker) + f.worker.SetParentBranch("release") + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + + f.loop.lastPushAt = time.Now() + liveWrite(f.loop, "cm1") + gitIn(t, f.repoDir, "update-ref", "-d", "refs/heads/release") + f.loop.pushPending() + + require.Len(t, f.loop.pendingWrites, 1) + open, _ := f.worker.ParentRecovery() + require.True(t, open, "parent recovery holds the work") + assert.False(t, f.loop.publicationRetry.pending(), "and the publication retry stays out of it") + assert.Nil(t, f.loop.pushTimer) +} + +// The user-facing half, on the real event loop: a CommitRequest riding a write whose push failed is +// held (the controller never fails a held request), so before this it stayed WaitingForPush until +// some unrelated write came along. It now resolves Committed on its own. +func TestPublicationRetry_AHeldCommitRequestIsCommittedWithoutAnotherWrite(t *testing.T) { + original := publicationRetryInitialBackoff + publicationRetryInitialBackoff = 50 * time.Millisecond + t.Cleanup(func() { publicationRetryInitialBackoff = original }) + + f := newRealServerNewBranch(t, "retry-held-commit-request", seedMain(t)) + attempts := failNextPushes(t, 1) + require.NoError(t, f.worker.Start(context.Background())) + t.Cleanup(f.worker.Stop) + + require.True(t, f.worker.Enqueue(configMapTargetEvent("cm1", "alice", newBranchTarget))) + f.worker.EnqueueAttach(standbyRequest()) + + require.Eventually(t, func() bool { + _, resolved := outcome(t, f.worker) + return resolved + }, 10*time.Second, 20*time.Millisecond, "the request resolves with no further write") + res, _ := outcome(t, f.worker) + require.NoError(t, res.Err) + assert.Equal(t, FinalizeCommitted, res.Outcome) + assert.Equal(t, f.ref("feature").String(), res.Commit) + assert.Equal(t, int32(2), attempts.Load(), "one failed push, then the retry") +} From 9570e47e26c653e1b0ca580f0b4d7b504f21e5be Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 07:48:16 +0000 Subject: [PATCH 2/6] docs: close the 0.51 upgrade-note gaps and the stale save wording UPGRADING tells readers to suspend a Flux or Argo CD source before removing commit.window, adds the quickstart chart value to the Was/Is table, corrects the closeDelay claim (a client without strict field validation has it pruned silently), and records that a save on a target that may not be written now fails. The CommitRequest and GitTarget commit descriptions, configuration.md and the CommitRequest spec stop describing a save as closing the window now. Co-Authored-By: Claude Opus 5.5 (1M context) --- api/v1alpha3/commitrequest_types.go | 9 ++++--- api/v1alpha3/gittarget_types.go | 4 +-- .../bases/configbutler.ai_commitrequests.yaml | 9 ++++--- .../crd/bases/configbutler.ai_gittargets.yaml | 4 +-- docs/TODO.md | 26 +++++-------------- docs/UPGRADING.md | 20 ++++++++++++-- docs/configuration.md | 8 +++--- docs/spec/commitrequest-design.md | 10 +++---- hack/crdfields/fields.yaml | 2 +- 9 files changed, 49 insertions(+), 43 deletions(-) diff --git a/api/v1alpha3/commitrequest_types.go b/api/v1alpha3/commitrequest_types.go index 6fbb4bb5..6c28d19d 100644 --- a/api/v1alpha3/commitrequest_types.go +++ b/api/v1alpha3/commitrequest_types.go @@ -23,7 +23,7 @@ import ( // +kubebuilder:validation:XValidation:rule="self.gitTargetRef == oldSelf.gitTargetRef && has(self.message) == has(oldSelf.message) && (!has(self.message) || self.message == oldSelf.message) && has(self.window) == has(oldSelf.window) && has(self.whenNothingToCommit) == has(oldSelf.whenNothingToCommit) && (!has(self.whenNothingToCommit) || self.whenNothingToCommit == oldSelf.whenNothingToCommit)",message="CommitRequest spec is immutable after creation" // +kubebuilder:validation:XValidation:rule="!has(self.whenNothingToCommit) || self.whenNothingToCommit != 'CommitEmpty' || has(self.message)",message="whenNothingToCommit: CommitEmpty requires spec.message, because a message is all an empty commit records" type CommitRequestSpec struct { - // GitTargetRef names the GitTarget whose open commit window to finalize. + // GitTargetRef names the GitTarget whose commit window this request attaches to. // The GitTarget must be in the same namespace as this CommitRequest. // +required // +kubebuilder:validation:XValidation:rule="self.name != ''",message="spec.gitTargetRef.name must not be empty" @@ -192,9 +192,10 @@ type CommitRequestStatus struct { // +kubebuilder:printcolumn:name="Message",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` -// CommitRequest is a one-shot "save" signal: creating one finalizes the open -// commit window for the referenced GitTarget instead of waiting for the -// silence timer. The resulting commit hash is reported back in status. +// CommitRequest is a one-shot "save" signal. It attaches to its author's current or next +// commit window on the referenced GitTarget, closes that window on its own timers instead of the +// target's, and gives the resulting commit its message. The commit hash is reported in status +// once the commit is on the remote. type CommitRequest struct { metav1.TypeMeta `json:",inline"` diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index e54b6ac4..7633ebb5 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -247,8 +247,8 @@ type GitTargetSpec struct { // the identity talking to the remote and stay on GitProvider. Migration: docs/UPGRADING.md. // Commit configures how this target's writes are batched into commits, and how those commits - // are phrased. Omitted, writes coalesce over a 5s rolling silence window and use the built-in - // message templates. + // are phrased. Omitted, writes coalesce over a 5s rolling silence window, a window closes at + // most 1m after it opened, and commits use the built-in message templates. // +optional Commit *GitTargetCommitSpec `json:"commit,omitempty"` diff --git a/config/crd/bases/configbutler.ai_commitrequests.yaml b/config/crd/bases/configbutler.ai_commitrequests.yaml index 9afec705..9786b34d 100644 --- a/config/crd/bases/configbutler.ai_commitrequests.yaml +++ b/config/crd/bases/configbutler.ai_commitrequests.yaml @@ -50,9 +50,10 @@ spec: schema: openAPIV3Schema: description: |- - CommitRequest is a one-shot "save" signal: creating one finalizes the open - commit window for the referenced GitTarget instead of waiting for the - silence timer. The resulting commit hash is reported back in status. + CommitRequest is a one-shot "save" signal. It attaches to its author's current or next + commit window on the referenced GitTarget, closes that window on its own timers instead of the + target's, and gives the resulting commit its message. The commit hash is reported in status + once the commit is on the remote. properties: apiVersion: description: |- @@ -76,7 +77,7 @@ spec: properties: gitTargetRef: description: |- - GitTargetRef names the GitTarget whose open commit window to finalize. + GitTargetRef names the GitTarget whose commit window this request attaches to. The GitTarget must be in the same namespace as this CommitRequest. properties: name: diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index bafbcae5..d8558cc4 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -137,8 +137,8 @@ spec: commit: description: |- Commit configures how this target's writes are batched into commits, and how those commits - are phrased. Omitted, writes coalesce over a 5s rolling silence window and use the built-in - message templates. + are phrased. Omitted, writes coalesce over a 5s rolling silence window, a window closes at + most 1m after it opened, and commits use the built-in message templates. properties: message: description: |- diff --git a/docs/TODO.md b/docs/TODO.md index df26fba3..2e8b8aa4 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -8,29 +8,15 @@ PR #404 is merged; these need fix PRs against main, in this order. (Its CommitRe finding is fixed in #407: the controller no longer fails a request the worker holds.) The two `CommitEmpty` findings (a suspended target reporting `Ready=True` with no commit, and a -record made while the render-fidelity gate was closed) are fixed in `fix/branch-worker-write-gates`: +record made while the render-fidelity gate was closed) are fixed in #411: a request on a target that may not be written fails, whatever path ends it (`internal/git/write_gate.go`). -**Next:** retry progress on quiet branches. A failed push stops the push timer and the retained work -waits for another commit (`pushPending` in `internal/git/branch_worker.go`); parent recovery drives -itself, and general publication recovery needs the same guarantee -([`docs/design/push-cooldown.md`](design/push-cooldown.md) §7 option C). - -**Before the next release** (docs only): - -- [ ] `docs/UPGRADING.md`, the v1alpha3 window entry: - - step 1: tell readers to suspend the Flux or Argo source of a GitOps-managed GitTarget (or remove - the field there) until step 4; otherwise that tool re-applies the string `commit.window` and - breaks LIST; - - the Was/Is table needs a row for the chart value `quickstart.gitTarget.commit.window`, now an - object; - - "a removed `closeDelay` is not a hazard" is too strong: the API server prunes it silently for - programmatic clients (see `duration_fields_admission_test.go`). -- [ ] Descriptions that still say a save "closes the window now": the CommitRequest godoc and CRD - description, `docs/configuration.md` (lines 14, 32, 123), and the GitTarget `Commit` godoc, which - also omits the 1m `maxDuration` default. `docs/spec/commitrequest-design.md:126` still says - `NoOpenWindow` and "close deadline". +Retry progress on quiet branches is fixed: a failed publication schedules its own bounded retry +(`internal/git/publication_retry.go`, [`push-cooldown.md`](design/push-cooldown.md) §7 option C). +Still open: when a rebuild's fetch fails, the open window is dropped, and unless the cause was a +missing parent nothing records the dropped scope or asks for a snapshot, so those writes wait for +the next resync. **Later, non-blocking:** diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index fc0ca8eb..ba1b3ec8 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,15 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## A save on a target that may not be written fails + +A `CommitRequest` on a suspended `GitTarget`, or on one whose render fidelity is not established, +resolves `Ready=False` with `FinalizeFailed` and the reason, whatever `whenNothingToCommit` says. +It used to resolve `Ready=True` (`NoWindow` or `AlreadyPresent`) with nothing written, or record an +empty commit under `CommitEmpty`. Automation that waits for a save's `Ready` condition now sees the +failure; unsuspend the target, or wait for `RenderMatchesLive=True`, and create the request again. +A commit made before the target was suspended is still pushed, and its request resolves with it. + ## Commit windows get a `window` block on both kinds; `closeDelay` is gone **Breaking.** Two fields change shape, and the rules around them are new. @@ -15,6 +24,7 @@ We are pre-1.0, so breaking changes bump the **minor** version (release-please i |---|---| | `GitTarget.spec.commit.window: "5s"` | `GitTarget.spec.commit.window.idleTimeout: "5s"`, plus `maxDuration` (default `1m`) | | `CommitRequest.spec.closeDelay: "2s"` | `CommitRequest.spec.window.attachTimeout: "2s"` and `window.maxDuration: "2s"` | +| chart value `quickstart.gitTarget.commit.window: "5s"` | `quickstart.gitTarget.commit.window.idleTimeout: "5s"` and `.maxDuration: "1m"` | | Ready reason `NoWindowInGrace` | `NoWindow` | | progress reason `WaitingForCloseDelay` | `Progressing`, then the worker's phase: `WaitingForWorker` (the GitTarget has no branch worker yet), `WaitingForWindow`, `CollectingWindow`, `WaitingForPush` | @@ -33,8 +43,10 @@ What behaves differently: **The upgrade procedure.** The one hazard is a stored `GitTarget` whose `commit.window` is still a string: once the new CRD is served, that object cannot be decoded, and a single one breaks listing -every `GitTarget`. A removed `closeDelay` is not a hazard: `kubectl apply` refuses it by name, and a -stored one is dropped. In order: +every `GitTarget`. A removed `closeDelay` cannot break listing, but it is not always refused: +`kubectl apply` refuses it by name, while a client that does not validate fields strictly (a +controller, or a script using a typed or dynamic client) has it pruned silently, and its request +then collects for the `2s` default. In order: 1. **Remove the old field while the old schema is still served**, noting each value: @@ -47,6 +59,10 @@ stored one is dropped. In order: ``` The running controller uses its `5s` default meanwhile. + + If Flux or Argo CD applies the `GitTarget`, suspend that source first, or remove the field in + its Git source, and keep it that way until step 4. Otherwise it re-applies the string + `commit.window`, and once the new CRD is served that object breaks listing again. 2. **Clear finished requests** rather than carrying them across: `kubectl delete commitrequests -A --all` once in-flight saves have resolved. 3. **Upgrade** the chart: new CRDs and new controller together. diff --git a/docs/configuration.md b/docs/configuration.md index a61feab4..29db9e62 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -11,7 +11,8 @@ The short version: - `WatchRule` defines which namespaced resources should produce Git writes, and in which source namespaces - `ClusterWatchRule` does the same for cluster-scoped resources -- `CommitRequest` optionally asks the operator to close the current commit window now +- `CommitRequest` optionally saves: it closes its author's commit window on its own timers, with + its own message if it has one The chart's optional `quickstart` values are a convenience layer that creates starter instances of those same resources. @@ -29,7 +30,8 @@ The usual flow is: 1. Create a `GitProvider` for repository access and commit behavior. 2. Create a `GitTarget` that points at the Git provider, source cluster, branch, and repository path. 3. Create one or more `WatchRule` or `ClusterWatchRule` objects that point at that target. -4. Create a `CommitRequest` only when you want to flush an open window before the normal timer. +4. Create a `CommitRequest` only when you want a save: your writes in one commit, on the request's + timers rather than the target's, and optionally with your message. That means one repository connection can back multiple targets, and one target can be fed by multiple watch rules. @@ -121,7 +123,7 @@ The same shape as a `WatchRule`, for cluster-scoped types. It selects no namespa | Field | Default | What it does | |---|---|---| -| `gitTargetRef` | **required** | The target whose open window to close | +| `gitTargetRef` | **required** | The target whose commit window the request attaches to | | `message` | the target's templates | Commit message, committed verbatim unless `requestTemplate` frames it | | `window.attach` | `CurrentOrNext` | Which window to attach to: the author's current or next one, or only the next. See [the request window](#the-request-window-specwindow) | | `window.attachTimeout` | `2s` | How long to wait for a window to attach to | diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index 54efd0ae..60f090eb 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -135,9 +135,9 @@ nothing to add, and `Committed` when the replay produced a commit after all. resolved in that interval, and the controller keeps re-sending its attach every couple of seconds until it reads an outcome. The worker marks such a request committed rather than forgetting it, so the re-send is recognized as the same request: it cannot register afresh and expire into -`NoOpenWindow` while its commit waits out the push cooldown, and it cannot claim the next -same-author window and stamp its message on somebody else's commit. Its close deadline is spent and -no longer arms anything. +`NoWindow` while its commit waits out the push cooldown, and it cannot claim the next same-author +window and stamp its message on somebody else's commit. Its attach deadline is spent and no longer +arms anything. **A worker that stops while still holding the write fails the request**, because the controller does not time out a request the worker holds, and "the worker stopped before the commit reached the @@ -166,7 +166,7 @@ The gates are read when the commit is made, as `suspend` already was for every w made before a gate closed is pushed rather than kept back, because a commit that never left the operator's checkout would surface later, out of order. -A failed push is retried when a later commit on the branch schedules the next one. A branch that then -goes quiet holds the work, and its requests stay `WaitingForPush`, until something else commits. +A failed push schedules its own retry (10s, doubling to 5m), so a request riding the retained write +stays `WaitingForPush` only until a retry succeeds, with or without further commits on the branch. The complete status vocabulary is in the [status conditions guide](status-conditions-guide.md). diff --git a/hack/crdfields/fields.yaml b/hack/crdfields/fields.yaml index cdad4efd..7aaa7344 100644 --- a/hack/crdfields/fields.yaml +++ b/hack/crdfields/fields.yaml @@ -199,7 +199,7 @@ kinds: title: "`CommitRequest` (namespaced): save now" fields: - path: gitTargetRef - doc: The target whose open window to close + doc: The target whose commit window the request attaches to - path: message default: the target's templates doc: Commit message, committed verbatim unless `requestTemplate` frames it From 5208e7cbf833a358055c15f91eecfa4d3e48c523 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 07:48:16 +0000 Subject: [PATCH 3/6] docs(design): the branch worker's recovery boundary, and where GitTarget stands The branch worker event model and the HA plan describe durable admission, save identity across restarts and publication evidence. gittarget-state-of-affairs.md records what is built, which page owns which decision, the known gaps ranked by user impact, and the next steps. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/INDEX.md | 10 +- docs/design/branch-worker-event-model.md | 927 +++++++++--------- docs/design/gittarget-state-of-affairs.md | 175 ++++ docs/future/ha-gittarget-distribution-plan.md | 371 +++++-- 4 files changed, 928 insertions(+), 555 deletions(-) create mode 100644 docs/design/gittarget-state-of-affairs.md diff --git a/docs/INDEX.md b/docs/INDEX.md index 543ea379..1d0e06d0 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -124,11 +124,12 @@ decided), **design, decided** (decision made, not built), **partly built**, **bu **deferred** (parked, kept as a decision record). The label is the first thing in the page, so you never have to read a proposal to find out it already shipped. -### Open (27 pages) +### Open (28 pages) | Doc | Open question | |---|---| -| [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **built in #407, not merged yet.** `spec.parentBranch`: a target stays on standby until a write needs a branch, then creates its write branch from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: parent per `GitProvider.status.branches` entry. | +| [`branch-worker-event-model.md`](design/branch-worker-event-model.md) | **proposal against merged #407 and #411.** Lead with the quiet-branch `WaitingForPush` failure and the retry/deadline fix. Worker ownership, FIFO inputs, save and deadline semantics, and the transition boundary for later durability. Journal storage and rollout belong to the HA plan. | +| [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407.** `spec.parentBranch`: a target stays on standby until a write needs a branch, then creates its write branch from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: parent per `GitProvider.status.branches` entry. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | | [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **deferred.** An empty repository's default branch: the server reports it over protocol v2 but go-git discards it (upstream fix first); then a separate policy decision on which branch the first commit may create. Assuming `main` was considered and rejected. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | @@ -197,8 +198,9 @@ than install manifests. What changed is that they are now checked. [`idea-application-editing.md`](future/idea-application-editing.md) is where the whole edit-through-the-API workstream started, and still holds the branch/session grouping strategies nothing else covers. -[`ha-gittarget-distribution-plan.md`](future/ha-gittarget-distribution-plan.md) is -the HA plan `architecture.md` cites three times (and the reason Redis is required). +[`ha-gittarget-distribution-plan.md`](future/ha-gittarget-distribution-plan.md) owns the durable +journal, publication recovery, outage retention, and persistence/HA rollout. The branch worker +event model owns execution semantics and the immediate retry/deadline fix. [`least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md) has three open RBAC items. [`config-surface-for-a-structured-repository.md`](future/config-surface-for-a-structured-repository.md) diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index e230d6f3..5aa04a40 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -1,69 +1,86 @@ -# Branch worker events and recovery - -This review maps everything that can influence a branch worker: queued requests, timer deadlines, -lifecycle signals, operation results, and configuration read outside the queue. It brings the -`GitTarget` design records together around one question: how can the worker remain driven by -events while preserving order and making progress when Git cannot accept a push? - -The recommended direction is to keep one owner of the checkout and make its inputs and recovery -obligations explicit. A failed push must preserve pending work. Whether it should block new work, -how much work can accumulate, and what event retries publication are separate decisions. - -Status: review and proposals, based on the code following #407 and the -`fix/branch-worker-write-gates` changes. The write-gate section identifies that branch's behavior -separately. This document adds no implementation. Its findings come from reading source and design -records; no tests or validation commands were run for this review. - -## The ownership rule - -One worker serves `(GitProvider namespace, GitProvider name, write branch)`. Several `GitTarget`s -can share it. The provider's UID and URL identify the repository incarnation; replacing that -identity replaces the worker and its checkout. +# Branch worker events, durable execution, and recovery + +A single failed push on a quiet branch can leave a `CommitRequest` in `WaitingForPush` +indefinitely, even after Git recovers. Repair that liveness gap first: schedule publication retries +and bound Git operations while keeping the branch worker as the owner of its checkout. + +The longer-term direction is recoverable execution of accepted writes, save commands, window +decisions, deadlines, and publication outcomes. This document owns the worker's event semantics, +current failure analysis, and immediate implementation scope. The +[HA and durable delivery plan](../future/ha-gittarget-distribution-plan.md) is the sole owner of +journal storage, acknowledgments, publication recovery, retention, and the persistence/HA rollout. +The broader ownership contract is in [the architecture](../architecture.md#git-write-architecture). + +Status: design proposal against the code after merged PRs #407 and #411. Current-behavior sections +describe that implementation. Event names and the transition boundary are proposals. This +document changes no runtime behavior and does not establish an outage or exactly-once guarantee. + +## Next implementation: restore publication progress + +Ship retry scheduling and Git operation deadlines before the journal work. Neither fix depends +on choosing a storage backend, changing the save contract, or implementing HA. + +1. Give retained publication work a bounded failure backoff that runs without another source + edit. Coordinate it with parent recovery and the successful-push cooldown so arrivals cannot + bypass the backoff or multiply the remote probe budget. Stop scheduling when the obligation + settles; a canceled worker must not rearm it. +2. Carry cancellation through ref listing and fetch, and impose operation deadlines across the + push cycle. Bound the cycle's total occupancy as well as its individual calls. Repeated + contention retries must not repeatedly reset the whole budget. Keep timed-out work pending; + a lost push response can mean the remote already accepted it. +3. Preserve held-save semantics. The worker must resolve publication or establish that the save + cannot publish before it reports a terminal failure. A controller timeout alone cannot prove + that. Keep `WaitingForPush` while a recoverable publication obligation remains. +4. Use an injectable clock and scheduler to test silence, continuous arrivals, cancellation, and + stale firings. Keep one owner of the checkout while testing the transport cancellation path. + Returning from a timeout wrapper while a goroutine still mutates the checkout is unsafe. + +The completion test is a save whose first push fails, followed by silence and remote recovery: +without another resource edit, its commit reaches Git and the same request reaches its terminal +status with the published SHA. Also prove that a request can safely outlive the controller's +safety window while retries continue, and that a stalled remote call returns control within the +operation budget. Use a controllable remote for those failures and a fake clock for retry timing. + +This change improves progress within a running worker. Restart durability follows the separate +journal plan. An indefinitely unavailable remote can still leave a save pending; the immediate +fix promises an active recovery schedule, not a time by which an unavailable server must accept it. + +## Why the branch worker remains the right owner + +One worker currently serves `(GitProvider namespace, GitProvider name, write branch)`. Several +`GitTarget`s can share it. The provider's UID and URL identify the repository incarnation; +replacing that identity replaces the worker and its checkout. The event loop owns the open window, pending writes, deferred snapshots, request attachment, and -parent recovery. Git operations run synchronously on that loop in the active controller paths. -Publication order therefore follows the same owner that created and retained the commits. - -The proposed rule for further work is: +parent recovery. One owner can decide whether a write joins a window, whether a save attaches, +and which commits a push publishes. Sequential file edits also protect resources that share a +YAML file. Splitting those decisions among controllers would require coordinating the same state +across more places. -> Every change to the worker's execution state has an identifiable input event or is the result of -> an operation started while handling one. The worker alone applies that change to its checkout, -> retained work, and publication state. +This is a single-writer event loop, close to the +[Singular Update Queue pattern](https://martinfowler.com/articles/patterns-of-distributed-systems/singular-update-queue.html). +It has an actor-like ownership boundary, with exceptions for shared configuration and setters +described below. Serialization is useful independently of persistence. -Here, an event includes a command, a deadline becoming due, and shutdown. It is broader than the -`git.Event` struct, which carries a mirrored resource change. Results of synchronous Git calls are -currently handled within the initiating event; they have no separate completion message. +The proposed ownership rule is: -This rule describes the intended ownership boundary. The current implementation also reads shared -state and accepts direct setters. Those inputs are listed below so that an event-only design does -not quietly depend on an undocumented exception. +> The worker decides each transition from explicit inputs and recorded observations. Its state +> and outstanding obligations are recoverable before the corresponding external work starts. -```mermaid -flowchart TD - W[Watch delivery and replay] --> Q[Per-branch FIFO] - C[CommitRequest controller] --> Q - R[GitTarget refresh request] --> Q - Q --> L[Branch worker event loop] - T[Window, push, attach, refusal, recovery deadlines] --> L - S[Start and cancellation] --> L - X[Parent setter and shared configuration] -. observed outside FIFO order .-> L - L --> G[Checkout, commit, fetch, push] - G -->|synchronous result| L - L --> O[Observations, request outcomes, snapshot requests] - O --> P[Controllers and watch plane] - P -->|later requests| Q -``` +Git operations can initially remain synchronous on this owner. Separating decision logic from +effects does not require another goroutine or a workflow service. A later asynchronous executor +would need immutable work, attempt identities, and completion messages. It must never mutate the +same checkout concurrently with the worker. -The implementation map starts at [BranchWorker](../../internal/git/branch_worker.go), -[WorkItem and request types](../../internal/git/types.go), and -[WorkerManager](../../internal/git/worker_manager.go). The broader ownership contract is in -[the architecture](../architecture.md#git-write-architecture). +The implementation starts at [BranchWorker](../../internal/git/branch_worker.go), +[work and event types](../../internal/git/types.go), and +[WorkerManager](../../internal/git/worker_manager.go). ## Inputs on the FIFO `WorkItem` has five alternatives: `Request`, `Attach`, `Withdraw`, `Resync`, and `Refresh`. -`Request` has two commit modes, giving the six rows below. These are the alternatives the -dispatcher recognizes; the struct itself does not enforce that exactly one field is set. +`Request` has two commit modes, producing the six dispatcher paths below. The struct itself does +not enforce that exactly one alternative is set. | Input | Producer or entry point | Loop handler | |---|---|---| @@ -74,428 +91,440 @@ dispatcher recognizes; the struct itself does not enforce that exactly one field | Complete scoped snapshot | Watch replay through `EnqueueResync` | `handleResyncRequest` | | Observe remote and folder | `GitTarget` reconcile through `EnqueueRefresh` | `handleRefreshRequest` | -### Resource writes - -The active live producer is -[GitTargetEventStream](../../internal/reconcile/git_target_event_stream.go). It forwards sanitized -objects, identity-only deletes, and field patches such as a translated scale update. Attribution -travels with the event. Audit facts supply identity; they do not independently supply mirrored -object state to the worker. - -Watch bookmarks, watch errors, and reconnect decisions stay in the watch plane. They can change -which writes or replay snapshots arrive, but they are not additional branch-worker message kinds. -Likewise, an admission request does not directly ask this worker to mirror its object. - -A live event joins the one window for its author and target. An identity change closes the previous -window before opening another. Repeated writes to one path coalesce inside that window. New windows -are offered to waiting saves before a zero-duration deadline or buffer threshold can close them. - -Atomic requests close the current window, drain deferred heals at that boundary, and create a -separate retained write. They bypass window collection but use the same publication lifecycle. -The API and handler remain in the tree; this review found no non-test caller of `EnqueueRequest`. -Treat atomic requests as a supported worker path, not evidence of a second active ingestion source. - -### Snapshots - -The current producer in [target_watch.go](../../internal/watch/target_watch.go) supplies a complete -snapshot from watch initialization or replay. Watch recovery and forced target rechecks can produce -another replay. The worker receives the resulting snapshot; it does not independently list cluster -objects or interpret the original reconcile trigger. - -`ResyncRequest` carries the desired objects, their collection resource version, target, scope, and -reply channel. Scope includes the collection identity used for coalescing; the served API version -is data. The deletion boundary must match the population gathered. An empty desired set can -authorize a sweep inside that boundary. - -The worker also supports whole-target snapshots and `Heal=true`. A heal waits while any live window -is open, including a sibling target's window, and runs at an idle boundary. The inspected production -replay call passes `Heal=false`; do not infer an active periodic heal producer from older comments -or tests of this supported path. - -[Resync handling](../../internal/git/resync_flush.go) refreshes the base before judging a snapshot. -A no-op snapshot may never push, so it needs its own remote evidence. Retained writes are replayed -when that refresh resets the checkout. `RefreshRemote` marks a forced recheck and avoids paying -for a second refresh in the same preparation step. - -Snapshots also establish render fidelity. They must be able to evaluate the target while its gate -holds ordinary live writes back. A suspended target still scans its folder and suppresses the write. -A common guard at the top of the dispatcher would therefore need to distinguish observation and -recovery from permission to create a new content commit. - -The reply describes local application. A changed snapshot joins pending writes for publication; -its successful reply alone does not prove a push. A no-op snapshot retains no write of its own. -The handler may attempt an immediate push before replying, but callers cannot treat the reply as -a publication acknowledgment. - -### Saves and withdrawal - -[The event router](../../internal/watch/event_router.go) registers saves on the FIFO and polls the -worker's outcome. It routes an already-owned request back to its owner even when that worker has -been removed from the manager's active map. - -An attach carries request identity, target, author information, message, attach mode, and deadlines. -`CurrentOrNext` can claim a matching window. `Next` closes an eligible existing window before waiting -for a later one. Registration is idempotent; another controller poll does not restart the deadlines. -Only resource writes open a live window. Competing saves attach in registration order. An expired -wait cannot claim a newly opened window, and processing a due timer does not preempt a Git call. - -At expiry, a request can resolve without a commit or create an empty record when `CommitEmpty` -allows it. A foreign-window mismatch does not create that record. An attached request follows its -retained write through replay and resolves against remote publication, including a remote-confirmed -no-op. The details belong to [the commit-window contract](commit-timing-surface.md) and -[the attach loop](../../internal/git/commit_request_attach_loop.go). - -A withdrawal cancels only a request the worker has not acted on. A request collecting a window or -waiting for a push remains held. The controller cannot independently fail it while the worker can -still publish it. An exited worker can answer withdrawal directly because its loop can no longer -act; that is a lifecycle exception to FIFO handling. - -### Refresh - -[Refresh handling](../../internal/git/refresh.go) observes the branch and scans the named target's -folder. It creates no new mirrored content. Fresh observations can avoid a connection; an idle -checkout that has moved may be fetched and reset. Targets sharing a branch share the remote -observation, while each target's folder scan still has its own meaning. - -An ordinary refresh skips a branch with an open window, retained writes, a dirty worktree, or an -unfinished replay. It cannot reset such a checkout as if it were idle. During active parent -recovery, however, a refresh can service a due recovery probe, and that handler may publish already -retained work. Calling every refresh path strictly read-only would miss that distinction. - -The controller supplies these requests through its existing reconcile schedule. -`--git-refresh-interval=0` disables that periodic refresh input. It does not disable window timers, -push cooldowns, or parent recovery. - -## Ordering and admission - -The FIFO orders accepted queue items. It does not establish a global order between independent -watches, controller delivery, and timer channels. The loop selects among ready inputs; a deadline -is eligible to run after it expires, but cannot interrupt an in-progress handler. - -Snapshot coalescing preserves a more specific rule. A newer snapshot can replace a queued snapshot -for the same target and scope while no relevant work has crossed its queue position. Live writes, -overlapping snapshots, and attaches can fence that replacement. The replacement keeps the marker's -position and preserves a requested remote refresh. The old caller receives `ErrResyncSuperseded`. -It must not interpret that reply as its own snapshot having been applied. - -Deferred heals have a separate coalescing list after dequeue. They wait for an idle window boundary, -and a newer heal for the same target and scope replaces the older one. This is another reason to -describe ordering in terms of the request contract, rather than assuming every item executes fully -at its original FIFO position. - -Queue admission is nonblocking and bounded by item count, with a default of 1,000 items per worker. - -| Input refused by a full or stopping queue | Caller behavior | +This inventory is the starting point for extracting explicit transitions. The atomic handler is +supported, but this review found no non-test producer. `Refresh` normally observes an idle branch; +during parent recovery its handler can service a due probe and publish retained work. + +Timer channels, shutdown, synchronous Git results, and shared configuration are additional inputs +outside this FIFO. A refactor must account for them as well as these six paths. Snapshot replies +currently describe local application, so they are not remote-publication receipts. + +## Architectural names and replay guarantees + +The proposed direction is an **event-sourced state machine per branch, with durable execution of +external operations**. These terms describe separate properties: + +| Term | Property | |---|---| -| Live event | `Enqueue` returns false; the watch path keeps its cursor and reconnects for redelivery | -| Atomic request | Drop is logged and counted; `EnqueueRequest` exposes no acceptance result | -| Snapshot | False return and an error reply; the producer must not claim successful admission | -| Attach or withdrawal | Controller polling sends it again; an exited worker can settle withdrawal | -| Refresh | Dropped; a later reconcile can request another | +| Single-writer event loop | One owner orders state changes | +| Durable queue | Accepted messages survive the stated storage failures | +| Event sourcing | An ordered history of facts reconstructs execution state | +| Durable execution | Pending operations and deadlines resume after failure | +| Status projection | Recorded outcomes produce the Kubernetes status view | -Successful admission means an item entered in-memory processing. It is not a durable receipt or a -promise that the event will become a commit. Handler refusal, process loss, and shutdown still -matter. The durable queue and cursor-acknowledgment work in [the backlog](../TODO.md) remains open. +[Event sourcing](https://learn.microsoft.com/en-us/azure/architecture/patterns/event-sourcing) +requires an authoritative history of transitions. Saving today's `WorkItem` queue only preserves +requests to execute. It leaves decisions and results to be derived again. A queue that discards +acknowledged messages also needs a durable checkpoint and retained history to reconstruct state. -## Deadlines and lifecycle events +[Durable workflow execution](https://docs.temporal.io/workflow-execution) is the relevant model for +resuming outstanding work, including timers and external calls. The recommendation here concerns +those semantics; adopting Temporal or another workflow engine is a separate infrastructure choice. -Five timer sources wake the loop. Identity switches and memory thresholds are additional decisions -inside a write handler, not independently queued inputs. +The word replay currently covers several different operations: -| Deadline | What makes it due | Effect | -|---|---|---| -| Commit window | Earlier of idle timeout and maximum duration | Close the window and schedule publication | -| Push cooldown | Remainder of 5 seconds since the last successful push | Attempt publication of retained writes | -| Attach timeout | Earliest waiting save's deadline | Service waiting saves, including an allowed empty record | -| Refusal action | Earliest deferred refusal action | Recheck consent and create an eligible empty commit | -| Parent recovery | Recovery deadline, with backoff | Probe, retry retained work, or request owed snapshots | +| Replay goal | Current guarantee | +|---|---| +| Reapply retained writes after the remote branch moves | Supported while that work survives in memory | +| Recover current Kubernetes state after a watch gap | Snapshots can converge state; intermediate history can disappear | +| Restart with the same save attachments, deadlines, and outcomes | Incomplete; execution state is volatile | +| Reproduce identical Git commits and SHAs | Unsupported; parents and commit timestamps can change | + +Historical replay reconstructs decisions using recorded observations and results. Resuming an +unfinished push contacts Git again and records a new result. Replaying a historical success must +not send another push or recreate an empty commit. + +An unconditional promise to replay everything forever would also require unlimited history, +compatible readers, and retained dependencies. Define a recovery boundary: a durable checkpoint +plus its journal tail, with a stated retention and failure policy. Kubernetes watch history is +not an archive of every mutation. + +## Problems in the current implementation + +These gaps are visible in the current code. Making the queue durable addresses only some of them. + +### A failed push can strand a save in `WaitingForPush` + +`pushPending` retains failed writes but stops the push timer without arming a general retry. +If no more writes arrive, a quiet branch can remain unpublished indefinitely. Ordinary refresh +skips a branch with retained work, so periodic `GitTarget` reconciliation does not repair this. + +For an attached save that committed locally, this leaves `WaitingForPush` with no time limit. +After its safety window, `withdrawCommitRequest` in the +[controller](../../internal/controller/commitrequest_controller.go) sends withdrawal and continues +polling when the worker still holds the request. `handleWithdrawCommitRequest` in the +[attach loop](../../internal/git/commit_request_attach_loop.go) leaves attached or committed +requests unchanged. Neither path retries the push. The API continues to show `Ready=False`, +`Reconciling=True`, `Stalled=False`, and `Pushed=Unknown`, with reason `WaitingForPush`. + +That withdrawal rule protects against reporting failure and then publishing the save later. +`TestCommitRequestReconcile_AHeldRequestOutlivesTheBound` explicitly preserves it. The defect is +that the live worker can retain responsibility without scheduling progress. Restore retries; +do not turn this into a controller-only timeout that can give a false terminal result. + +Continued writes can cause the opposite problem. Only successful pushes advance `lastPushAt`. +Once the successful-push cooldown has elapsed, later commits can repeatedly attempt the same +unavailable remote. The [push-cooldown design](push-cooldown.md#7-options) records this gap. + +[Parent recovery](../../internal/git/parent_recovery.go) already models a useful alternative: an +obligation remains open and schedules its next attempt. Its probe starts at 10 seconds and backs +off to five minutes, sharing the budget across targets on the worker. General publication failure +needs the same liveness property, with scheduling coordinated so the two paths cannot multiply +remote attempts. + +### Slow Git operations stop the whole branch loop + +Git calls run synchronously. While one is running, the branch cannot handle queued saves, +withdrawals, snapshots, deadlines, or later writes. Every target on that worker shares the delay. +Producers continue admitting work until the FIFO fills. + +The gap covers both remote observation and fetch. In +[smart fetch](../../internal/git/git_smart_fetch.go), `listRemoteRefs` calls `remote.List` and +`SmartFetchFrom` calls `repo.Fetch` without passing the worker context to either network call. +The `ctx` parameter on `SmartFetchFrom` does not make those operations cancelable by the worker. +The separate connection check in [git.go](../../internal/git/git.go) also uses `remote.List`. + +The [atomic push](../../internal/git/git_atomic_push.go) does pass context through handshake, +advertisement, and upload, but `internal/git` sets no operation deadline. Worker shutdown +cancellation is not an elapsed-time budget. These code findings do not establish that every +transport lacks its own timeout; they establish that the worker has no end-to-end bound. +A timer on the event loop cannot interrupt a call that has not returned. + +### Admission is volatile, and retained memory has no outage bound + +Successful enqueue means an item entered process memory. The watch path can advance its resume +cursor before the write reaches Git. A crash between those steps can make a replacement resume +past work that existed only in RAM. The local checkout is disposable. + +The FIFO defaults to 1,000 items. The default 8 MiB branch buffer threshold closes a window early, +but closing moves its data into `pendingWrites`. Failed pushes retain that data. Queued payloads +and deferred snapshots also consume memory, so neither setting bounds total outage retention. + +Queue-full handling differs by input. Live writes return a refusal to the watch producer, which +keeps its cursor and reconnects. Attaches and withdrawals can be resent. Refreshes can be retried +by a later reconcile. The supported atomic enqueue API logs a dropped request without returning +an acceptance result. A durable admission contract needs explicit receipts for every supported +producer. See the [durable queue backlog](../TODO.md). + +### Saves lose execution identity across a restart -Window settings are captured when the window opens; an attached save supplies its own timing. -Editing target settings does not move an already-running window's deadlines. After ordinary loop -wakes, the worker services saves, drains eligible deferred heals, and updates retained-work -visibility. A queue item can therefore cause follow-on work beyond its primary handler. +The worker remembers registration order, attachment, calculated deadlines, and outcomes in memory. +Repeated delivery is idempotent within that lifetime. An attached request follows its retained +write through conflict replay, including the refreshed commit SHA. Those are useful foundations. -Start creates the loop after checking that its provider can be read. Startup failure exits and -settles known requests. Stop closes admission under the enqueue lock, cancels the worker context, -and waits for exit. The shutdown handler attempts to finalize the open window, apply deferred heals, -and publish pending work. It then fails requests still awaiting publication and discards unhandled -queue items. This is best effort; cancellation and remote failure can prevent publication. +The gap is a push that succeeds before its result reaches Kubernetes status. A crash loses the +worker's receipt, and a resent request can create another empty commit. This is explicitly +documented in [the commit-window contract](commit-timing-surface.md). The outcome cache can also +collect entries older than 15 minutes when no attach is queued. Collection is not tied to a +durable receipt proving that status was written. -The manager retires workers on repository replacement or when no target needs them. Its periodic -orphan sweep is a lifecycle producer outside the branch loop. An already-held request remains bound -to its original worker during retirement. +Authorship has a separate lifetime. The +[admission author store](../../internal/queue/command_author_store.go) retains records for an hour; +the `CommitRequest` object cannot reconstruct the authenticated submitter by itself. Recovering a +previously accepted save must preserve its resolved attribution, including an unresolved result. +Re-running the lookup later can produce a different decision. -## State that currently enters outside the FIFO +### Queue payloads do not determine every decision -These inputs prevent a literal claim that the queue payloads completely determine worker behavior. +The loop selects among FIFO items and timer channels. It also calls `time.Now`, reads live target +and provider configuration, consults a shared fidelity gate, and observes remote Git responses. +Replaying the same resource messages tomorrow can therefore produce different windows or saves. -| Input | Current boundary | Consequence for an event-only design | +Some replay inputs are already retained: `PendingWrite` includes events, commit configuration, +resolved target planning data, and the attached request identity. However, its signer interface, +process pointers, and the reply channels in request types are implementation objects. Persisting +these Go structs directly would not define a portable journal format. + +### Arrival order does not establish a save barrier + +One FIFO orders the inputs accepted by that worker. Independent watches and the `CommitRequest` +controller have no global source ordering. A user's save can arrive before an earlier resource +mutation reaches the worker. Overlapping watches can also deliver duplicate observations. + +Journaling arrival order makes it reproducible. It does not prove that a save includes everything +the user changed before submitting it. Keep the existing attachment contract unless a separate +barrier or per-stream watermark design can establish that stronger claim. A timestamp or a +`resourceVersion` from one collection does not supply a universal barrier. + +## Proposed recovery contract + +Make durable acceptance the point at which the system owes a recoverable answer: + +> Once work is durably accepted, a restart preserves its ordering and save obligations. +> Publication resumes when Git becomes available, within configured storage limits and subject +> to the recorded write-policy decisions. Status can be rebuilt from durable outcomes. + +An accepted item may still be refused by policy or resolve without a commit. Record that outcome +instead of silently losing the item. A transient publication failure keeps the obligation open. +Define explicit behavior for target deletion, repository replacement, and administrator +cancellation; never reroute accepted work to a new object merely because its name matches. + +This contract preserves the product's existing coalescing. It does not require one commit per +Kubernetes mutation. Preserve the chosen window boundaries, messages, and request membership; +replanning unpublished work onto a moved remote may still change its SHA or find no remaining diff. + +The first implementation can scope recovery to pending work and retained terminal receipts. +Rebuilding arbitrary old execution states requires a longer archive and compatible code. Keep +that historical audit requirement separate from the ability to survive a controller restart. + +## Commands, facts, and effects + +Represent intent, decisions, and external work separately so recovery knows what remains to do. + +| Category | Example | Meaning | |---|---|---| -| Parent configuration | `SetParentBranch` stores an atomic name and generation | Preserve the push admission rule when choosing where a change becomes effective | -| Render fidelity | Watch plane updates a shared `RenderFidelityGate` | Specify which gate revision each write decision observes | -| Target write policy | Handlers resolve target metadata from the Kubernetes client | Specify when suspension, placement, messages, encryption, and prune policy are captured | -| Provider and Secrets | Read for credentials, signing, and connection policy | Credential changes can affect a later attempt without a distinct queue item | -| Type resolution | Injected mapper and source-cluster resolver | Planning observes discovery state outside the request payload | -| Lifecycle | Manager start, stop, replacement, and cleanup | Admission and cancellation already act outside FIFO order | -| Bootstrap helper | Exported `EnsurePathBootstrapped` locks and stages the checkout directly | No production caller found; review this escape hatch before declaring exclusive loop access | - -`SetParentBranch` is especially important. A target's `parentBranch` is immutable, but replacing the -targets on a still-live shared worker can change its parent. A configuration update before push -admission invalidates a plan based on the old generation. An admitted push is not recalled. -The setter itself does not enqueue a wake; handlers and recovery checks observe the new generation. - -Putting that setter behind the same FIFO would change its timing. An update arriving while a -handler performs Git work could wait until after that handler's push. A refactor must either retain -the current admission check or explicitly decide a different configuration-activation contract. -A renamed message alone does not preserve the existing race guarantees. - -The same question applies to suspension and render fidelity. The current write-gate branch checks -them at commit decisions, and allows a commit already made locally to reach the remote. It gives -no promise to recall that commit when a gate closes later. Replay also has its own policy reads, -including tightening retained prune permissions; configuration is not frozen uniformly across the -entire pipeline. - -Recommended review boundary: retain existing behavior while naming each observation and its -effective point. Consider versioned configuration or gate-change events only after the ordering -contract is explicit. Avoid copying Secrets or adding public configuration revisions merely to -make every input look like a self-contained queue message. - -## Internal results and outputs - -Several important transitions are produced by handlers rather than received as `WorkItem`s. - -| Result | Worker action | -|---|---| -| Local commit succeeds | Retain the write and its commit identity until publication | -| Partial write or replay fails | Mark the checkout dirty or replay incomplete; recover before another commit | -| Advertisement or rejected upload reveals a moved remote | Fetch, reset, and replay retained writes within the bounded push cycle | -| Push proves publication or a no-op | Settle carried requests and clear the published batch | -| Missing or unresolved parent prevents work | Retain a recovery obligation and remember affected dropped scopes | -| Write plan is refused | Report the refusal; optionally schedule an authorized refusal action | +| Command | `SaveRequested` | A caller asks to attach a save | +| Fact | `SaveAttached` | The worker bound that request to a particular window | +| Effect intent | `PublishBatch` | A recorded obligation to attempt a push | +| Effect result | `PublicationConfirmed` | Evidence that a particular attempt published its work | +| Projection obligation | `ProjectSaveOutcome` | Kubernetes status still needs the recorded result | -The checkout's `baseTrusted`, `worktreeDirty`, and `replayRequired` flags answer different questions. -A clean checkout at the remote tip can still have lost the local commits behind retained writes. -It must not settle those writes merely because pushing that tip reports success. +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. -The [write-gate branch](../../internal/git/write_gate.go) addresses a separate question: -whether a target may receive a new write. It makes a refused save fail instead of reporting a -successful empty save that was never made. That work is not a replacement for checkout recovery. +```mermaid +flowchart TD + I[Watch observations and save commands] --> A[Durable admission] + A --> J[Branch journal and checkpoint] + J --> W[Branch state machine] + W -->|Persist decisions and effect intents| J + J -->|Outstanding Git work| G[Serialized Git executor] + G -->|Recorded observations and results| J + J -->|Outstanding deadlines| T[Deadline scheduler] + T -->|Accepted deadline firings| J + J -->|Durable outcomes| P[Status projector] + P --> K[Kubernetes status] +``` -[Refusal actions](../../internal/git/refusal_touch.go) are also distinct from `CommitEmpty` saves. -`onRefusal: PushEmptyCommit` authorizes an empty commit for an eligible write-boundary refusal to -prompt the GitOps reconciler to reapply. Consent and suspension are checked again when a delayed -action runs. The interval is one minute per target, and pending observations are tracked per target -and collection. A successful resync clears only the refusal scopes it covers. +The diagram describes ownership and persistence boundaries. Components may share a process and +goroutine. The state machine applies recorded facts without reading the clock, calling Kubernetes, +or contacting Git. Decision handling can request those observations as effects, then continue +when their results have been recorded. + +A transition must leave its next obligation recoverable. The HA plan defines the +[atomic processing and acknowledgment boundary](../future/ha-gittarget-distribution-plan.md#atomic-handoff-and-acknowledgment) +that preserves this across a crash. Redelivery must find the prior decision or resume unfinished +work. Recording a push intent alone does not prove remote success. + +### Inputs that the durable boundary must preserve + +The worker needs resource observations with attribution, save identity and timing policy, chosen +window membership, policy decisions, and operation results. The HA plan owns the concrete +[journal records](../future/ha-gittarget-distribution-plan.md#journal-record) that retain these. +Live interfaces, process pointers, and reply channels remain adapters outside that format. + +Snapshot replacement has execution semantics of its own. Live writes, overlapping snapshots, +and save attachments can fence replacement of an earlier queue position. Deferred heals wait +behind an open window. Preserve those decisions when persisting the +[snapshot protocol](../future/ha-gittarget-distribution-plan.md#snapshot-and-replay-delivery). +[Resync handling](../../internal/git/resync_flush.go) still reports local application separately +from publication. + +## CommitRequest is a workflow with a status projection + +A `CommitRequest` changes window selection, timing, commit messages, and possibly whether an empty +commit exists. Its lifecycle belongs in the same branch history as resource work. + +The current attach loop and controller already preserve several rules: + +- Only resource writes open live windows. Saves attach to an eligible author and target. +- Competing saves attach in registration order; repeated delivery does not restart their wait. +- `Next` closes an eligible current window before waiting for a later one. +- A save waiting for publication remains held. Controller timeout initiates withdrawal, and the + worker decides whether withdrawal is still possible. +- A request's published SHA belongs to its own retained write, which may be below the branch tip. + +Retain those rules in a durable lifecycle. One illustrative successful path is: + +```text +SaveRequested(request UID, target UID, attribution, message, timing policy) +SaveRegistered(request UID, sequence, registration time, attach deadline) +SaveAttached(request UID, window ID) +WindowClosed(window ID, included inputs, reason) +PublicationPlanned(batch ID, window ID, request UID) +PublicationConfirmed(batch ID, attempt ID, per-write commit mapping) +SaveResolved(request UID, outcome, branch, commit) +``` -Outputs leave through request outcomes, snapshot replies, remote observations, and reporting hooks -for placement, path acceptance, and render fidelity. The watch plane and controllers own their -status projections. A branch observation says where the remote was proved; it does not prove that -every target folder was scanned or published. +Refusal, withdrawal, and no-window expiry have their own terminal facts. `CommitEmpty` is a +publication obligation even though its tree matches the parent. A foreign-window mismatch keeps +its existing no-commit meaning. A remote-confirmed no-op and a local plan with no diff remain +different observations. -Parent recovery also publishes a per-target snapshot-request sequence. The controller observes a -new sequence and forces a watch recheck, whose replay sends a snapshot back through the FIFO. -The worker never substitutes a cached dropped event for a fresh cluster snapshot. +Write terminal facts before projecting status. If a status update fails, retry the same projection +by request UID. Do not attach the request again or calculate a new outcome. The status writer must +reject a replacement object with the same name and must not overwrite a later outcome with an +older phase update. -## What happens when Git cannot accept a push +Keep the durable receipt until projection and the supported redelivery period are accounted for. +Deleting the API object must not erase the only deduplication evidence while old commands can +still return. Whether longer receipt retention is needed is a storage-policy decision. -There are three different failure modes behind the impression that the worker chokes. +Notifications can replace routine outcome polling once this receipt exists. Periodic reconcile +can still repair a missed notification by reading the same durable result. Status is a view of +the workflow; an inability to update it must not restart the Git operation. -### A Git operation has not returned +## Deadlines are recoverable events -Network and repository operations occupy the loop synchronously. While one runs, that branch cannot -handle queued saves, snapshots, deadlines, or subsequent live writes. Producers continue admitting -items until the FIFO fills. Every target on that branch shares the delay; other branch loops have -their own execution, although manager lifecycle operations can still serialize replacement. +Keep timers as wakeup mechanisms and persist the deadlines that give those wakeups meaning. +Recording every periodic tick would add history without explaining a decision. Record meaningful +scheduling, cancellation, and accepted firings instead. -This follows from the chosen owner model. Before adding concurrency, review operation deadlines -and cancellation end to end. For example, `advertiseRemoteBranch` reaches `listRemoteRefs`, which -calls `remote.List` without the worker context. This review does not establish a universal maximum -duration for all transports. A timer on the loop cannot interrupt a call that has not returned. +The current worker has five timer sources: -### The push returned a failure +| Deadline | Existing purpose | +|---|---| +| Commit window | Close on idle timeout or maximum duration | +| Push cooldown | Space successful publication cycles | +| Attach timeout | Settle a save still waiting for an eligible window | +| Refusal action | Recheck consent before an eligible empty commit | +| Parent recovery | Probe and service retained work or owed snapshots | -The loop can resume handling inputs, but the retained batch remains unpublished. Ordinary push or -replay failure stops the push timer without arming a general retry. If the branch then becomes -quiet, work can wait indefinitely. Normal refresh deliberately skips retained work, so enabling -periodic refresh does not repair this case. +Add a general publication retry obligation. Keep successful-push cooldown and failure backoff +distinct, while giving the branch one policy for when it may spend its next remote attempt. -Continued writes can cause the opposite problem. Since only success advances `lastPushAt`, an -expired cooldown does not space failed attempts. Later commits may repeatedly retry the same -unavailable remote. This gap is recorded in -[the push-cooldown design](push-cooldown.md#7-options), and is independent of whether the success -cooldown remains useful. +An illustrative deadline protocol is: -These two outcomes are scheduling policy. Neither is required by serialization. A due retry is a -valid event for the same loop and can make progress without a new cluster edit. +```text +DeadlineScheduled(timer ID, purpose, owner ID, generation, dueAt) +DeadlineCanceled(timer ID, generation) +DeadlineFired(timer ID, generation, acceptedAt) +``` + +The generation makes an old firing harmless after an idle deadline moves or a window closes. +Persist first registration and window times so restart cannot grant a fresh full timeout. +Record whichever firing or input the sequencer accepts first; wall-clock timestamps alone do not +establish that ordering. + +Recovery first reconstructs recorded decisions without applying today's clock to historical +inputs. It then makes outstanding overdue deadlines eligible. Define how those deadlines compete +with admitted but unprocessed messages: processing-time window semantics and original source-time +semantics can choose different contents. Preserve the existing processing-time contract initially +and test the restart boundary explicitly. -### Publication stays unavailable while new work arrives +A durable scheduler must recover a scheduled deadline even if no in-memory timer was created +before the crash. Conversely, a delivered firing must be harmless when the corresponding +transition was already committed. Missed retries during downtime should produce one eligible +attempt, followed by the normal backoff, rather than a burst of historical retry attempts. -The FIFO has a count limit. The default 8 MiB branch buffer threshold closes a live window early, -but moving those events into `pendingWrites` does not free the data needed for replay. Failed pushes -retain that data. The threshold is therefore not a hard bound on memory during a sustained outage; -queued payloads and deferred snapshots also consume memory outside that counter. +Not every input advances a phase. Duplicates and stale firings can be no-ops; a failed push records +an obligation and a next opportunity. The liveness rule is that every unresolved obligation has +an identified event that can service it, including when the branch receives no more writes. -The available choices have different product costs: +## Configuration and observations need explicit effective points -| Choice | Benefit | Cost or unresolved contract | +Record the inputs that explain decisions without freezing every dependency forever. The current +worker already mixes captured planning policy with checks made at later boundaries. + +| Input | Current behavior | Required design decision | |---|---|---| -| Keep current admission and retention | Preserves retained write detail within the process | Memory can grow during a prolonged outage | -| Refuse more writes at a retained-work limit | Bounds worker retention | Requires reliable redelivery and a separate path for control and recovery | -| Collapse blocked work into a fresh snapshot | Bounds retained history | Loses intermediate attribution, messages, and save boundaries | -| Persist an ordered journal | Allows bounded memory and restart recovery | Needs durable acknowledgment, quotas, replay, and ownership design | -| Run publication separately | Could improve control responsiveness | Requires immutable publication batches and explicit completion ordering | - -A newer snapshot cannot silently replace a save-bearing batch: final state equality does not retain -the same history or prove that the requested commit was published. Durable queuing and HA are -already related backlog items. Raising queue limits does not settle either contract. - -## Parent recovery is the existing model for an obligation - -[Parent recovery](../../internal/git/parent_recovery.go) already provides a useful example of -event-driven progress. A missing configured parent or an unresolved default branch can prevent a -write branch from being created. The worker remembers both retained writes and dropped scopes that -still need a snapshot. - -The recovery deadline starts at 10 seconds and doubles up to 5 minutes. Targets sharing the worker -share one probe budget. Before a due probe, ordinary arrivals cannot repeatedly fetch the missing -parent. Once the branch can be based, recovery retries retained work and asks for snapshots still -owed. It keeps scheduling while the obligation remains. - -Seeing the parent again does not complete recovery. A changed snapshot must be published, or a -fresh no-op snapshot must settle its scope. A second outage invalidates publication credit from a -snapshot that predates newly dropped work. Otherwise publishing the older snapshot could falsely -clear the newer obligation. - -This establishes a useful requirement for general publication recovery: pending work needs an -identified next opportunity to run, even when no external event arrives. Reusing the concept does -not require merging every kind of recovery into one flag. A missing parent, a failed push, and a -scope awaiting a fresh snapshot have different completion conditions. - -## Decisions carried from the GitTarget records - -The records below remain the detailed sources. This section carries their constraints into the -event review and distinguishes implemented behavior from open work. - -### Parent branch contract and hardening - -[The parent-branch contract](gittarget-parent-branch.md) and -[the completed hardening record](gittarget-parent-hardening.md) establish: - -- An existing write branch receives subsequent commits on its own history. Its parent is followed - only while the write branch is absent. -- Standby and ordinary no-op writes create no remote branch. An explicitly requested empty commit - is a separate authorized write. -- First publication checks the selected parent's tip using the push advertisement. The server's - compare-and-swap protects creation or update of the write branch. The parent can still move after - the advertisement; no parent lock is promised. -- A trusted checkout gets no unconditional fetch before publication. Contention and invalid state - trigger bounded fetch, reset, and replay. -- Omitted parent means the remote default branch as last discovered. A push advertisement cannot - rediscover a default-branch switch; discovery, refresh, or a probe does that. -- An explicit missing parent blocks creation, even when it equals the write branch. An omitted - parent permits a root commit only when the remote is empty. Tags count as refs; a nonempty remote - with an unresolved default branch must not be treated as empty. -- Targets sharing a worker must agree on the configured parent. Omitted and explicitly named - parents are distinct configuration values even when they currently resolve to the same branch. -- Recovery must survive another transient failure after the parent returns and another outage - while an older snapshot awaits publication. Retained replay must respect the probe budget too. - -Open work includes showing the parent in each `GitProvider.status.branches` entry. Resetting, -deleting, or force-pushing a surviving write branch after a PR merge remains a separate policy -decision. A parent observation cannot authorize any of those mutations. - -### Parent observations - -[The observation proposal](gittarget-parent-observation.md) is deferred. Its first step keeps parent -availability and its own timestamp visible after the write branch exists. It reuses existing -advertisements, adds no connections, and changes neither readiness nor publication permission. - -Ancestry is a later step, dependent on measuring the pinned go-git shallow-object behavior. Compare -advertised remote tips; unpublished local `HEAD` is not an alternative input. Equal tips and proved -ancestry can yield `SameTip`, `WriteAhead`, or `ParentAhead`; incomplete evidence yields `Unknown`. -Negative ancestry claims require stronger evidence. No extra history fetch or deepening is budgeted. - -Exact fields, unresolved-default representation, and local work limits remain open. Any cached -comparison must belong to the repository identity and exact observed tips. A successful write push -cannot advance the parent's timestamp without evidence about that parent. - -### Empty repositories - -[The empty-repository record](gittarget-parent-empty-repository.md) separates protocol evidence -from bootstrap policy. It records an experiment against go-git `v6.0.0-alpha.5` in which unborn -`HEAD` information was discarded, plus a proposed upstream fix. That recorded dependency finding -was not reverified against upstream during this review. - -Recovering the advertised default name is the first proposed step. Choosing what the first commit -may create is a later compatibility decision: keep today's write-branch bootstrap, refuse until -the advertised default exists, or explicitly authorize bootstrapping it. Guessing `main` is rejected. -Protocol and host behavior still needing verification must stay unknown in status. - -### Status and configuration freshness - -[The red-status plan](gittarget-red-status-plan.md) keeps the writer responsible for proving a -refusal and the controllers responsible for publishing it. Message changes and scoped recovery -already matter; a sibling scope's success cannot clear another scope's failure. Notifications are -best effort, with periodic reconcile as the fallback. - -The remaining work is a complete reason/status contract and the decision about retaining refusals -across restart. Shared queue saturation and background failures need attributable evidence before -being projected onto one target. A healthy target can coexist with a broken selecting rule. -Suspension is an intentional stop to new writes while observation remains useful. - -[Configuration freshness](gittarget-configuration-freshness.md) remains deferred. A watch plan being -applied is distinct from a branch's pending writes being published. Any future plan marker belongs -to the watch owner and must describe only inputs that owner applied. Provider credentials do not -belong in that marker merely because a worker reads them. - -Open decisions include the input set, observed-input revisions, one marker versus two, the meaning -of pending during normal coalescing, readiness effects, and compatibility across canonicalization -changes. Revisit when operator incidents or multi-rule waits show a concrete need; a new branch -event model does not itself justify new public freshness fields. - -### Earlier API work and adjacent backlog - -[The API-wave record](gittarget-api-wave.md) preserves the separation between target policy and -provider connection settings, plus suspension and explicit recheck semantics. Its opening status -is older than its step 8, which closes the remaining riders. Do not turn that historical checklist -into new worker requirements. Asserted save authorship and resolution-change Events were declined. - -The backlog still records save details: expiry order for simultaneous empty records, full author -identity, and record-message templates. It also records worker observability, cross-provider writes -to the same repository, and a durable queue for restart recovery and HA. - -[Push notifications and reconcile triggers](push-notification-and-reconcile-trigger.md) contain -additional unchosen inbound and outbound designs. There is no branch-worker Git webhook input in -the inventory above. A future remote-change notification should request observation through the -owner; it must not become another writer of the checkout. The older options are not approval to -hold pushes for an external reconciler or to infer that a remote commit has reached the cluster. - -## Decisions to make before another implementation branch - -The first decision is how strictly to apply the event rule to configuration. The current owner -model is clear for the checkout, but a fully queued configuration model would need explicit -activation and admission semantics. Keep the parent-generation race contract visible while -reviewing that choice. - -For failed publication, the smallest proposed change is a bounded retry deadline owned by this -same loop, while preserving the successful-push cooldown. It should run only while work is owed, -space attempts under continued arrivals, and stop when that obligation is settled. Its interaction -with the existing parent deadline needs one scheduling decision so two timers cannot multiply the -remote probe budget. Delay values, reset rules, and failure classification are still design work. - -Keep synchronous checkout ownership for now. Before considering asynchronous publication, establish -where a slow call blocks and which cancellation or timeout guarantee is missing. If a separate -publisher later becomes necessary, it needs an immutable batch and a completion event carrying -worker incarnation, attempt identity, and published evidence. Letting it share a mutable checkout -would remove the ordering guarantee this design depends on. - -Treat sustained-outage storage as its own decision. Define whether the product preserves every -accepted attributed change, final state only, or explicit save boundaries before choosing a hard -admission limit, snapshot compaction, or a journal. Recovery and lifecycle inputs must still have a -way to run when data admission is constrained. - -For each proposed event, review five things: who produces it, what ordering it needs, which state -it may change, what constitutes completion, and who supplies the next event after failure. Apply -that review to remote failure followed by silence, failure under continuous arrivals, parent loss -and recovery, gate changes during a window, replacement with a held save, and saturated admission. -These are future review and validation cases, not work executed by this document. +| Parent configuration | Atomic name and generation outside the FIFO | Preserve the push-admission cutover | +| Target planning policy | Much of it retained in `ResolvedTargetMetadata` | Capture a stable policy snapshot or resolvable version | +| Prune permission | Replay can tighten retained permissions | Record that tightening before the new attempt | +| Render fidelity | Shared gate read at write decisions | Identify the observed gate state and scope | +| Credentials and signing | Provider and Secret reads | Separate current access from retained authoring intent | +| Type resolution | Shared mapper and source-cluster registry | Record planning inputs needed for recovery | +| Remote Git | Advertisements, fetches, push results | Tie observations to repository and attempt | + +A configuration version reference works only while its contents remain recoverable. Kubernetes +`resourceVersion` alone does not retain the corresponding historical object. Keep the necessary +non-secret policy data or provide a versioned store with an explicit retention contract. + +`SetParentBranch` currently changes the generation before push admission. Moving the setter onto +the FIFO could postpone its effect until after a synchronous handler pushes. An event refactor +must retain that admission check or explicitly change the activation contract. Naming a new +message does not preserve the race guarantee by itself. + +The [write gates](../../internal/git/write_gate.go) apply suspension and render fidelity when a +commit is decided. A commit already created locally can still publish after a gate closes. +Delayed [refusal actions](../../internal/git/refusal_touch.go) recheck consent and suspension. +Preserve those distinct effective points and record their decisions. Applying current gates +while reconstructing a previously completed decision would rewrite its history. + +Fresh execution may use rotated credentials or discover that a permission has been revoked. +Record the result of that new attempt. Historical replay consumes the old result without +reconnecting or trying to recover the old credential. Exact Git bytes additionally depend on +parents, timestamps, encryption output, and signatures; the proposed workflow guarantee does not +require regenerating identical bytes from resource messages alone. + +## Prepare the transition boundary for later durability + +After the immediate liveness fix, extract explicit commands, observations, facts, and effect +intents around the FIFO inventory and the inputs outside it. Keep the Git executor synchronous +initially. Capture first registration, window membership, policy decisions, and operation results. +Make reply channels and live clients adapters outside the serializable state. + +Test a transition from recorded state and input, then replay its facts without network calls. +Duplicate commands and stale deadline generations should reproduce the existing answer or be +no-ops. Compare behavior with the existing attach, gate, parent, resync, and conflict tests. +The exit criterion is that the same retained history reconstructs the same workflow state and +outstanding effects without reading mutable dependencies. + +The HA plan owns the next steps: persist that boundary in single-active mode, prove +[publication recovery](../future/ha-gittarget-distribution-plan.md#publication-recovery), validate +[hours of outage](../future/ha-gittarget-distribution-plan.md#surviving-hours-without-git), and then +add failover. The proposed stronger save contract remains a decision for that plan. Agree on +retained outcomes and ambiguous-publication behavior before claiming restart durability. + +## Validation scenarios + +These are worker implementation acceptance cases, not tests executed by this documentation change. +Journal crashes, retention, encryption, and HA failure tests belong to the +[durable-delivery acceptance suite](../future/ha-gittarget-distribution-plan.md#ha-4-fault-injection-acceptance-suite). + +| Scenario | Required observation | +|---|---| +| Save's first push fails, then the branch goes quiet | Retry publishes and resolves the same request without another edit | +| Held save outlives the controller's safety window | Withdrawal cannot falsely fail it; recovery remains scheduled | +| Push fails under continuous arrivals | Arrivals cannot bypass the retry budget | +| Ref listing, fetch, or push stalls | Operation and cycle deadlines return control to the worker | +| Worker is canceled during a failed attempt | No retry is rearmed and no abandoned task retains checkout access | +| Timer moves or fires twice | Stale and duplicate firings do not close another window | +| Recorded decisions are replayed | Identical state returns without external effects | +| Fresh snapshot follows an accepted save | Coalescing preserves the save's membership and obligation | +| Parent disappears again during recovery | Earlier snapshot publication cannot clear newly owed work | +| Policy changes during a window or push | Its specified effective point remains unchanged | +| Resource update and save arrive on independent paths | Only the documented attachment ordering is claimed | + +Use fake clocks and recorded effects for transition tests, and a controllable remote for Git +operation deadlines. Full implementation validation follows [`AGENTS.md`](../../AGENTS.md). + +## Existing contracts to preserve + +The live producer is +[GitTargetEventStream](../../internal/reconcile/git_target_event_stream.go); audit supplies +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. + +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 +the recovery that can reopen a target. `Heal=true` snapshots defer behind any open window; the +inspected production replay producer uses `Heal=false`. + +[Refresh handling](../../internal/git/refresh.go) normally skips a busy checkout. During parent +recovery it can service a due probe and publish retained work. Disabling periodic refresh does +not disable commit windows or worker recovery. Keep `baseTrusted`, `worktreeDirty`, and +`replayRequired` separate: a clean checkout can still have lost the commits behind retained work. + +The detailed design records continue to own adjacent policy: + +- [Parent contract](gittarget-parent-branch.md) and [hardening](gittarget-parent-hardening.md): + parent advertisement checks, empty-repository boundaries, bounded replay, and recovery + obligations remain intact. Git compare-and-swap protects the write branch and does not lock + the parent after its advertisement. +- [Parent observation](gittarget-parent-observation.md): deferred status work that adds no remote + connections or publication permission. Incomplete ancestry evidence remains `Unknown`. +- [Empty repositories](gittarget-parent-empty-repository.md): default-branch protocol discovery + and authorization to bootstrap it remain separate decisions. +- [Target status](gittarget-red-status-plan.md): the writer proves refusals and controllers + project them. A sibling scope's success cannot clear another scope's refusal. +- [Configuration freshness](gittarget-configuration-freshness.md): watch-plan application and + publication are separate observations. This design does not require new public freshness fields. +- [API boundaries](gittarget-api-wave.md) and + [push notifications](push-notification-and-reconcile-trigger.md): a remote-change notification + requests work through the owner. It does not become another checkout writer or prove that Git + changes reached the source cluster. diff --git a/docs/design/gittarget-state-of-affairs.md b/docs/design/gittarget-state-of-affairs.md new file mode 100644 index 00000000..d610f825 --- /dev/null +++ b/docs/design/gittarget-state-of-affairs.md @@ -0,0 +1,175 @@ +# GitTarget and the branch worker: state of affairs + +> **Snapshot**, taken 2026-10-02 at `230d602a` (#411 on `main`), with the uncommitted rewrite of +> [`branch-worker-event-model.md`](branch-worker-event-model.md) in the working tree. It describes +> what is built, which page owns which decision, which edge cases are known not to work, and a +> recommended order for what comes next. It decides nothing by itself; each linked page stays the +> owner of its own contract. Every claim about behavior below was checked against the code or a +> named test on that commit. + +## Where the release stands + +Release PR [#406](https://github.com/ConfigButler/gitops-reverser/pull/406) (`0.51.0`) is open and +carries four merged changes since `0.50.0`: + +| PR | What it changed | Breaking | +|---|---|---| +| #403 | A save's delay restarts when it claims a window; #388 restored to the changelog | no | +| #404 | One commit window surface: `spec.commit.window.{idleTimeout,maxDuration}` on `GitTarget`, `spec.window` on `CommitRequest`, `closeDelay` gone | **yes** | +| #407 | `GitTarget.spec.parentBranch`; a target on standby creates its write branch from the parent's current tip, checked at publication; the hardening pass | no (additive) | +| #411 | A `CommitRequest` on a suspended target, or one whose render fidelity is not established, fails instead of reporting `Ready=True` with nothing written | no, but user-visible | + +The release PR body does not list #411 yet; release-please adds it on its next run. + +## What is built + +| Capability | Code | Pinned by | +|---|---|---| +| One commit window per branch worker, with idle and maximum-duration timers; a save's timers replace the target's for the window it attaches to | [`open_window.go`](../../internal/git/open_window.go), [`commit_request_attach_loop.go`](../../internal/git/commit_request_attach_loop.go) | `TestCommitEmpty_*`, `TestAttach_*` | +| `whenNothingToCommit: CommitEmpty` records a save's message in an empty commit; outcomes `Committed` and `AlreadyPresent` are decided by the push | [`branch_worker.go`](../../internal/git/branch_worker.go) (`resolvePushedCommitRequests`) | `TestCommitEmpty_*` | +| The controller never fails a save the worker holds; past its safety window it only asks the worker to withdraw it | [`commitrequest_controller.go`](../../internal/controller/commitrequest_controller.go) | `TestParentRecovery_AHeldCommitRequestIsCommittedAfterALongRecovery` | +| Write gates: suspension and render fidelity are read when the commit is made; a commit made before a gate closed is still pushed | [`write_gate.go`](../../internal/git/write_gate.go) | `TestWriteGate_*` (6), `TestSuspend_*`, `test/e2e/suspend_e2e_test.go` | +| Standby: an absent write branch is created from the parent's tip as advertised on the push session; a no-op write leaves it absent | [`git_atomic_push.go`](../../internal/git/git_atomic_push.go), [`branch_worker.go`](../../internal/git/branch_worker.go) | 20 `TestBranchWorker_*Parent*` tests, `test/e2e/new_branch_parent_e2e_test.go` (3 contexts) | +| A parent change on a live worker takes effect at the push admission boundary (`{name, gen}` snapshot) | [`branch_worker.go`](../../internal/git/branch_worker.go) | `TestBranchWorker_AParentChange*` (8) | +| An empty repository that gains a ref is never orphaned; a dangling `HEAD` is `Missing` and only a repository with no refs is `Unborn` | [`git_atomic_push.go`](../../internal/git/git_atomic_push.go), [`git_smart_fetch.go`](../../internal/git/git_smart_fetch.go) | `TestBranchWorker_AnEmptyRepositoryThatGains*`, `TestBranchWorker_ADanglingRemoteHeadIsMissingNotUnborn` | +| Parent recovery is an obligation, not an observation: retained writes plus per-`(target, collection)` dropped scopes, one probe per worker (10s doubling to 5m) | [`parent_recovery.go`](../../internal/git/parent_recovery.go), [`gittarget_parent_recovery.go`](../../internal/controller/gittarget_parent_recovery.go) | `TestParentRecovery_*` (8), the missing-parent e2e context | +| Cost gate: connections per operation, including the four new standby and probe rows | [`git-roundtrip-ledger.golden`](../../internal/git/testdata/git-roundtrip-ledger.golden) rows 13 to 16 | `TestGitRoundTripLedger` | +| Idle refresh: reads one advertisement, never writes, skips a branch mid-cycle, shares the probe deadline during recovery | [`refresh.go`](../../internal/git/refresh.go) | `TestRefresh_*` (25) | +| `status.remote.parent` (`Found`, `Missing`, `Unborn`) while the write branch is absent; `ParentBranchNotFound` (stalled) and `RecoveringParentBranch` (reconciling) | [`gittarget_types.go`](../../api/v1alpha3/gittarget_types.go), [`gittarget_controller.go`](../../internal/controller/gittarget_controller.go) | `internal/controller/gittarget_parent_branch_test.go` | +| Red status, first slice: refusal messages republish when the offender changes; a scoped refusal clears only on the same scope's success; "Why a `GitTarget` is not committing" in [`configuration.md`](../configuration.md#why-a-gittarget-is-not-committing) | [`git_path_acceptance.go`](../../internal/watch/git_path_acceptance.go) | `TestGitTargetKstatusContract`, the unsupported-folder e2e | + +## The documents on this topic + +| Page | Status | Owns | Note from this review | +|---|---|---|---| +| [`gittarget-parent-branch.md`](gittarget-parent-branch.md) | built (#407) | The parent-branch contract as built | Accurate against the code and ledger. One open question (parent per `GitProvider.status.branches` entry). | +| [`gittarget-parent-hardening.md`](gittarget-parent-hardening.md) | done (#407) | The races, recovery and cost rules the contract depends on | Every §4 item has a named test and its ledger row. Mostly history now, including agent handoff instructions in §0; a candidate for `docs/finished/` after the release, with §7 kept as the "do not re-raise" list. | +| [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) | deferred | Which branch an empty repository's first commit may create | Blocked on go-git discarding the server's unborn `HEAD`. The upstream issue and PR are the only step that can start now, and it runs in the background. | +| [`gittarget-parent-observation.md`](gittarget-parent-observation.md) | deferred | The parent after the write branch exists; ancestry status | Informational only. Step 1 (availability) is the current backlog item in [`TODO.md`](../TODO.md). | +| [`gittarget-red-status-plan.md`](gittarget-red-status-plan.md) | partly built | Why a target is red, and what the message must say | Step 5 (the operator section) shipped and the page does not say so. The step 1 cross-package reason-agreement test was not found. Open question 2 (refusal state across a restart) is still open: `GitPathAccepted` lives in watch-manager memory. | +| [`gittarget-configuration-freshness.md`](gittarget-configuration-freshness.md) | deferred | Whether a target's running watch plan matches its configuration | Deferred with explicit pick-up triggers; neither trigger has fired. | +| [`gittarget-api-wave.md`](gittarget-api-wave.md) | effectively done | The `GitTarget` field boundaries | Every step shipped, was dropped, or was declined. A candidate for `docs/finished/`. | +| [`branch-worker-event-model.md`](branch-worker-event-model.md) | proposal, **uncommitted rewrite** | The branch worker's recovery boundary: liveness, durable admission, save identity across restarts, publication evidence | Reviewed in detail below. Not listed in [`INDEX.md`](../INDEX.md). | +| [`push-cooldown.md`](push-cooldown.md) | design | Success cooldown versus failure backoff | §7 option C (failure backoff) is the recommended fix for gap 1 below. | +| [`../spec/commitrequest-design.md`](../spec/commitrequest-design.md) | spec | The save lifecycle | Still says `NoOpenWindow` and "close deadline" (around line 138). | +| [`../future/ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) | future | HA, durable delivery, journal record kinds | Overlaps the event model's stages 4 to 6; the two need one owner for the journal design. | +| [`../TODO.md`](../TODO.md) | backlog | Release blockers and follow-ups | Its "Before the next release" list is still open (checked below). | + +## Known gaps, ranked by what a user would hit + +Each was checked in the code at `230d602a`. + +1. **A failed push on a quiet branch strands the work, and any save riding it waits forever.** + `pushPending` stops the push timer on failure and arms nothing; its own doc comment says so + ([`branch_worker.go`](../../internal/git/branch_worker.go), `pushPending`). The refresher skips + a branch with retained writes (`refresh.go`, "this branch is mid-cycle"). The controller does + not time out a save the worker holds, and the worker ignores a withdraw for a request it holds + (`handleWithdrawCommitRequest`). So after one transient push failure with no further edits, + the commit stays local and the `CommitRequest` stays `WaitingForPush` until some other write + arrives. Parent recovery is the only failure path that schedules its own retry. +2. **A down remote under steady traffic costs one failed push per commit.** `lastPushAt` advances + only on success, so once the cooldown has passed every new commit attempts the push again. +3. **No Git network call has a deadline.** `remote.List` (`listRemoteRefs`, `CheckRepo`) and + `repo.Fetch` (`SmartFetchFrom`) take no context; the push uses the worker's context, which has + no deadline; nothing in `internal/git` sets a timeout. A stalled server can block the whole + branch loop, and every target on that worker with it. Whether go-git's transports apply a + default timeout was not established here. +4. **A failed rebuild drops the open window.** When retained writes need a replay and its fetch + fails, `recoverRetainedWrites` returns an error and the finalize drops the window + (`dropFailedWindow`). Only a missing parent records the dropped scope and asks for a snapshot, + so under any other failure those writes wait for the next resync. While the rebuild keeps + failing, every new window spends a fetch on it. +5. **Accepted work is volatile.** The FIFO (1,000 items) and retained writes are process memory. + The watch cursor can advance before a write reaches Git, so a crash in between can skip work on + restart. The 8 MiB buffer threshold closes a window early; it does not bound retained memory. +6. **A save's identity does not survive a restart.** A push that succeeds immediately before a crash + loses the worker's receipt, and a resent save can create a second empty commit. The outcome + cache keeps entries 15 minutes; the admission author store keeps records one hour. +7. **Refusal state does not survive a restart.** After a manager restart a refused folder reads as + accepted until the next write or resync refuses it again. +8. **An empty repository with a non-default write branch** gets the write branch as its first, + parentless branch (documented as today's bootstrap contract; deferred policy). +9. **The parent disappears from status once the write branch exists**, so an operator cannot see + that the parent has moved ahead of an open review branch. +10. **Two `GitProvider`s naming one repository** are two workers and two checkouts on the same + remote; nothing coordinates them beyond the server's compare-and-swap. The advice remains one + `GitProvider` per repository. +11. **With `parentBranch` omitted, a push cannot see a switch of the remote's `HEAD`**; the next + refresh follows it with one fetch. This is a documented choice, not a defect. + +Gaps 1 to 4 are liveness defects in the current worker and can be fixed without any persistence. +Gaps 5 and 6 are the durable-execution problem. Gaps 7 to 10 are separate, smaller decisions. + +## Review of `branch-worker-event-model.md` + +The rewrite turns the committed page (an inventory of the worker's inputs, deadlines and failure +behavior) into a proposal for an event-sourced state machine per branch with durable execution. +Its factual claims hold against the code: the five timer sources, the 10s to 5m probe, the 1,000 +item queue and 8 MiB threshold, the 15-minute outcome cache, the one-hour author store, +`EnqueueRequest` returning no receipt and having no non-test caller, and the write gates' effective +points. The distinction between the four meanings of "replay", and the refusal to promise +exactly-once publication before the evidence protocol exists, are its strongest parts. + +Findings, most important first: + +1. **The user-visible consequence of the liveness gap is missing.** "A failed push can wait + indefinitely" describes retained writes. It should also say that a save on that write stays + `WaitingForPush` with no bound, because the controller does not time out a held request and + the worker ignores a withdraw for it. That is the strongest argument for stage 2 and the one a + reader acts on. +2. **The timeout gap is wider than stated.** The page names only `remote.List`. `repo.Fetch` is + also called without a context, and no deadline is set anywhere in `internal/git`. +3. **The actionable part is hard to find.** Stage 2 (bounded publication backoff, operation + deadlines, an injectable clock) is a small change to the current worker. Stages 3 to 6 are a + multi-release program that overlaps the HA plan. The page should lead with stage 2 as the + near-term change and say which page owns the journal design, or the two pages will drift. +4. **The status line is dated.** It refers to the `fix/branch-worker-write-gates` changes, which + merged as #411. +5. **The input inventory went away.** The committed version's "Inputs on the FIFO" and "State that + currently enters outside the FIFO" were the most useful reference material in it; the rewrite + compresses them into "Existing contracts to preserve". Keep them, here or in + [`architecture.md`](../architecture.md), because stage 3 needs exactly that inventory. +6. **It is not in [`INDEX.md`](../INDEX.md)**, and INDEX still describes #407 as "not merged yet". + +## Recommended order + +### Before cutting 0.51.0 + +1. **Close the docs blockers in [`TODO.md`](../TODO.md).** Checked, still open: + - [`UPGRADING.md`](../UPGRADING.md)'s commit-window entry has no step telling readers to + suspend the Flux or Argo source of a GitOps-managed `GitTarget`; its Was/Is table has no row + for the chart value `quickstart.gitTarget.commit.window`; and it still says a removed + `closeDelay` "is not a hazard", which is too strong for programmatic clients. + - [`configuration.md`](../configuration.md) line 14 still says a `CommitRequest` closes the + window "now"; [`commitrequest-design.md`](../spec/commitrequest-design.md) still says + `NoOpenWindow` and "close deadline". + - Consider one `UPGRADING.md` line for #411: a save on a suspended target now fails where it + used to report `Ready=True`, and automation that waits for `Ready` will see that. +2. **Fix gap 1, and gap 2 with it: a bounded publication-failure backoff** (push-cooldown §7 + option C, event model stage 2). It is one timer, parent recovery is the model, and it is the + only known defect that leaves a user's object waiting forever. The acceptance test is the + event model's first two scenarios: a push fails, the branch goes quiet, the work publishes + without another edit, and the held save resolves `Committed`; under continuous arrivals the + backoff still spaces attempts. Coordinate it with the parent probe so the two never both + spend a connection on one deadline. If it cannot land within days, ship 0.51.0 without it and + make it 0.51.1, with the gap stated in the release notes. + +### Next, after the release + +1. **Operation deadlines (gap 3).** Pass the worker context to `List` and `Fetch` and give each + remote operation a bounded deadline. Measure first how go-git's HTTP and SSH transports behave + today, against a server that stalls. +2. **Event model stage 1 only**: agree the recovery contract (final-state convergence, or also + preserved save outcomes) and write the failure scenarios as executable tests. Do not start the + journal before that contract is agreed and the HA plan and the event model have one owner. +3. **Cheap, independent items**: open the go-git unborn-`HEAD` issue and PR; parent observation + step 1 (availability after the write branch exists, no extra connections); the red-status + reason-agreement test. +4. **Housekeeping**: move the hardening plan and the API-wave page to `docs/finished/`, list the + event model and this page in INDEX, and mark red-status step 5 as shipped. + +### Leave deferred + +Configuration freshness, ancestry classification, the empty-repository bootstrap policy, and the +durable journal (stages 3 to 6) all wait on a decision or on evidence that does not exist yet. None +of them is needed to make 0.51.0 better than 0.50.0. diff --git a/docs/future/ha-gittarget-distribution-plan.md b/docs/future/ha-gittarget-distribution-plan.md index 64e67de8..4fb83745 100644 --- a/docs/future/ha-gittarget-distribution-plan.md +++ b/docs/future/ha-gittarget-distribution-plan.md @@ -1,14 +1,19 @@ -# High Availability and Durable Delivery Plan +# High availability and durable delivery plan Status: **proposed** (not started) -## Scope and Definition of Done +This is the owning document for the durable journal, publication recovery, retention, and the +persistence/HA rollout. The [branch worker event model](../design/branch-worker-event-model.md) +owns worker transitions, deadline semantics, and the immediate retry and operation-timeout fix. +That fix can ship independently of this plan. + +## Scope and definition of done This plan updates the previous HA proposal for the current watch-first architecture. Kubernetes WATCH is the source of mirrored object state. Audit is optional attribution only; it is not a source of object state or a write queue. -The first release is active/passive HA, not active/active scheduling: +The first HA release uses active/passive ownership: - Run at least two controller Pods. - One elected Pod owns controllers, target watches, and Git branch workers. @@ -17,15 +22,22 @@ The first release is active/passive HA, not active/active scheduling: - Losing one controller Pod must not silently drop a Kubernetes-to-Git state change. The replacement may replay a change or create a no-op Git attempt. -The durability contract is eventual state convergence: after recovery, Git -matches the watched Kubernetes state. It is not a promise of one Git commit for -every Kubernetes mutation, nor an exactly-once event history. +The baseline durability contract is eventual state convergence: after recovery, Git matches the +watched Kubernetes state. The proposed extension preserves accepted save obligations, including +window membership, attribution, messages, deadlines, and outcomes. The event model defines those +execution semantics; this plan owns how their records survive failure. + +Before implementing the extension, agree on retention, target replacement, and the response to +an indeterminate publication result. Existing coalescing still applies, and rebuilding unpublished +work onto a moved remote can change its SHA or leave no diff. This is not a promise of one commit +per Kubernetes mutation or exactly-once publication under arbitrary history loss. -This contract assumes the Kubernetes API, Git remote, and durable queue remain -available. A single Redis or Valkey Pod is therefore not sufficient for an -installation that also needs to survive loss of any one dependency Pod. +A Git outage is recoverable within the configured storage budget if the remote eventually +returns. The journal must survive the advertised storage failures, and Kubernetes must become +available for source recovery and status projection. A single Redis or Valkey Pod is insufficient +when the installation must also survive loss of that dependency Pod. -## Current State +## Current state The repository has useful foundations, but it is not HA today. @@ -53,7 +65,7 @@ The repository has useful foundations, but it is not HA today. - The chart has a PDB and preferred Pod anti-affinity, but these only improve placement; they do not make the data path durable or coordinate writers. -## Target Architecture: HA v1 +## Target architecture: HA v1 HA v1 retains one active data-plane owner for the whole release. This is the smallest design that satisfies loss of one controller Pod without introducing @@ -82,9 +94,12 @@ Controller-runtime leader election owns the active/passive transition. The leader lock must use a release-scoped Kubernetes Lease in the release namespace. On lock loss, the old owner stops reading new durable work and cancels its watches. A stalled old owner may still finish an already-started push; the -remote compare-and-swap rejects it if a newer owner has moved the ref. +remote compare-and-swap rejects it if a newer owner has moved the ref. It cannot reject an old +owner solely because its lease expired while the ref stayed unchanged. Require an ownership +epoch for journal transitions, reject stale writers there, and recover already-started pushes +through the publication protocol below. A stronger Git fence needs server-side enforcement. -## Durable Write Journal +## Durable write journal Redis or Valkey becomes mandatory in HA mode. Add a versioned durable journal under a branch-write-shard key, with Redis Streams used for delivery and @@ -103,53 +118,87 @@ for key and Lease names. It must be exposed in logs, metrics, and target status. Every GitTarget maps to exactly one branch write shard. Targets sharing a remote branch use one journal and one worker, even when they have different paths. -Overlapping paths must remain a reconciliation-time and writer-time refusal. +Overlapping paths must remain a reconciliation-time and writer-time refusal. Retain provider and +target incarnations alongside the shard key; sharing a destination must not silently combine +incompatible policy or reroute old work to a replacement object with the same name. ### Journal record -Introduce a versioned record that includes: - -- target UID, namespace, and name; -- source cluster, GVR, namespace scope, resource identity, operation, and - resource version; -- a deterministic idempotency key; -- the sanitized object or delete/field-patch payload; -- author attribution when available; -- the resolved branch-shard identity and target path; -- a kind for live event, snapshot member, snapshot start, or snapshot complete. - -The object payload must be encrypted before it is written to Redis when it -contains a sensitive resource. Today plaintext sensitive content is only -transient in the Pod before the Git writer encrypts it. A durable queue changes -that boundary. Queue encryption needs a Kubernetes Secret-backed key, rotation -plan, TLS in transit, and tests proving plaintext secret fields are absent from -Redis values. - -### Atomic handoff and acknowledgement - -For each watched GVR and scope, persist the journal record and its resume cursor -in one idempotent Redis operation. A Lua script or transaction must make a -successful enqueue and cursor advancement inseparable. The key layout must also -work in Redis Cluster, including its same-hash-slot constraints. - -The source watch may advance only after this durable handoff succeeds: - -1. WATCH receives an event. -2. The active leader derives a journal record. -3. It atomically records the event and the new cursor. -4. The branch worker consumes the record and may create local commits. -5. It acknowledges the record only after the corresponding state reaches the - remote Git branch successfully. - -If the leader dies before step 3, the cursor is not advanced and the watch -replays. If it dies after step 3 but before step 5, the consumer group reclaims -the unacknowledged record. If it dies after the remote push but before the -acknowledgement, replay is harmless because the write is idempotent against the -current remote tree. - -The worker must not rely on its local clone for recovery. A new owner fetches -the remote branch, replays retained journal records, and lets the existing -PushAtomic conflict/rebuild behavior settle external changes. +Give every record a payload schema version, branch identity, stable record identity, and branch +sequence. Version decision behavior as well as data layout so an upgrade can interpret pending +work. Carry the originating command or operation ID for deduplication. Include target and provider +incarnations where identity affects routing or policy. Branch sequence is execution order; +source metadata records provenance. + +Retain the inputs, decisions, and results needed to reconstruct the workflow: + +- Resource observations: sanitized object or field patch, resource UID and delete identity, + source cluster incarnation, GVR and namespace scope, `resourceVersion`, target UID, path, + operation, and resolved attribution. +- Snapshots: snapshot ID, start marker, members, completion marker, collection, and resource + version. Preserve supersession decisions that affect execution or producer receipts. +- Saves: request UID, target UID, attribution decision, message, attachment and timing policies, + first registration, and each withdrawal's answer. +- Windows: stable window ID, included input references or materialized coalesced content, save + membership, closing reason, and captured planning policy. +- Operations: stable batch and attempt IDs, repository identity, intended base, resulting commit + mapping, and observations that support success, failure, or uncertainty. +- Deadlines: purpose, owner, absolute due time, generation, and accepted firing or cancellation. +- Outcomes: per-request terminal result, pending status projection, and projection receipt. + +This extends the earlier resource-only journal proposal. Use explicit data records instead of +serializing `WorkItem` or `PendingWrite`: their live interfaces, process pointers, and channels +have no meaning in another process. Keep reconstructable references to credentials and signers. +Historical policy references also need retained contents; a Kubernetes `resourceVersion` alone +cannot retrieve an arbitrary old configuration. + +Encrypt sensitive resource payloads before writing them to the journal. Queue encryption needs +a Kubernetes Secret-backed key, a rotation and retention policy, TLS in transit, and tests that +plaintext secret fields are absent from Redis values. Keep private credentials out of records. + +### Atomic handoff and acknowledgment + +For each watched GVR and scope, persist the journal record and its resume cursor in one +idempotent Redis operation. A transaction or Lua script must make successful admission and cursor +advancement inseparable. The layout must work with Redis Cluster's same-hash-slot constraints. +Every supported producer needs an explicit acceptance result, including save commands. + +Separate three durable milestones: + +| Milestone | What it proves | +|---|---| +| Accepted | Input is recoverable; its source cursor may advance | +| Processed | Decisions and remaining obligations are recoverable | +| Completed | Publication or an explicit terminal outcome is proved | + +Persist the consumed input position, resulting decisions, and new effect obligations together. +Otherwise a crash can consume a command but lose its push, or create an obligation twice. +Stable effect identities permit redelivery. A durable outbox holds external operations and +status updates still requiring execution; writing it does not prove those operations succeeded. + +A complete path is: + +1. WATCH receives an observation, or the controller submits a save command. +2. Admission durably records it and any source cursor in one idempotent operation. +3. The worker commits its processing position, decisions, and effects together. +4. The executor performs outstanding work and records the result, using the publication + recovery protocol when the remote outcome is uncertain. +5. A completed input can be acknowledged while the outcome and any unfinished status projection + remain recoverable. Terminal refusal and no-commit outcomes also need receipts. + +A crash before step 2 cannot advance the durable cursor. After step 2, the consumer can reclaim +accepted work. A lost admission acknowledgment returns the same record on retry. A crash after +step 3 resumes the recorded effects without deciding window membership again. A crash after Git +accepted the push requires publication recovery before repeating an operation or reporting success. + +Status projection progresses independently after completion. Retain checkpoints, payload +references, pending effects, and receipts needed by every unfinished step. Acknowledging delivery +or deleting a `CommitRequest` must not remove the only deduplication evidence while commands can +still be redelivered. Receipt collection needs a stated projection and redelivery boundary. + +The local clone remains disposable. Recovery reconstructs the recorded workflow, uses the +remote as the Git base, and only replans unfinished publication. Raw resource redelivery alone +cannot restore the same window boundaries or attached saves. ### Snapshot and replay delivery @@ -165,7 +214,84 @@ superseded snapshot remains replayable; a new leader can also enqueue a fresh complete replay after the retained journal tail. HTTP 410 Gone continues to mean fresh replay, never loss of the old journal tail. -## Implementation Phases +Tie every member and completion marker to a snapshot ID and its exact scope. An incomplete +snapshot cannot authorize deletion; an explicitly complete empty one can authorize a scoped +sweep. Preserve ordering fences where live writes, overlapping snapshots, or saves prevent +replacement of an earlier queue position. Persist supersession and deferred-heal state when +needed to reproduce the worker's decisions. A successful local resync reply does not acknowledge +remote publication. + +## Publication recovery + +Git and the journal cannot participate in one ordinary storage transaction. There is always a +boundary between the remote ref update and recording its result. Treat a lost response as an +uncertain outcome, because a timeout can follow a successful server-side update. + +The failure sequence is: + +1. The worker durably records an intent to publish a batch. +2. Git accepts the push. +3. The worker dies before recording publication evidence. +4. Recovery finds an outstanding intent and must determine whether it already happened. + +Neither a durable outbox nor an in-memory deduplication map resolves step 4. The earlier plan +relied on idempotent writes against the current tree for convergence. That does not preserve +empty commits, save boundaries, or the original request SHA. +Replaying several already-published intermediate states can also create redundant history even +when the final tree converges. + +Use stable batch, attempt, and request identities. Before pushing, persist the intended commit +mapping and enough recoverable evidence to recognize that attempt. Candidate evidence includes +retained immutable Git artifacts or an agreed operation marker in Git. A marker changes the +repository contract and needs its own design; choosing it is not implicit in this proposal. +Local artifacts identify what was attempted. Publication evidence must come from the remote or +a durable record of its acknowledgment. + +On recovery, seek evidence that the intended commits were published, allowing later commits above +them. If a competing update forces a rebuild, record a new attempt and its refreshed mapping. +Only publish the request outcome once evidence supports that outcome. Equal tree content alone +cannot prove that a particular message or empty save was recorded. + +Shallow history, branch deletion, force pushes, and unreachable objects can remove the evidence. +Define how long evidence is retained and what happens when the outcome remains indeterminate. +Do not promise exactly-once publication until the protocol handles those cases within a stated +failure model. The existing remote compare-and-swap protects ref updates; it does not atomically +acknowledge the journal or prove a request's historical execution. + +## Surviving hours without Git + +State an outage budget in workload and storage terms. A useful first estimate is admitted bytes +per second multiplied by outage duration, plus snapshot, index, encryption, and retention costs. +Recovery also needs enough publication throughput to drain the backlog while new work arrives. + +Use the atomic admission and cursor handoff defined above. Enforce byte quotas and a retention +policy from the first durable implementation. Stop accepting +new payloads when the durable store cannot honor its contract. This does not guarantee recovery +of every later Kubernetes mutation: a prolonged ingestion stop can outlast watch history. + +Give recovery and lifecycle work a way to run under saturation, with its causal ordering intact. +Reserved capacity must not let a withdrawal overtake an already accepted attach. Bound the active +in-memory working set independently of journal size; keeping every persisted payload in +`pendingWrites` would preserve the current memory problem. + +Snapshot compaction must respect save boundaries. Replacing a request-bearing batch with the +latest object state loses its message, attribution, and publication obligation. Define explicitly +which ordinary writes can coalesce, and preserve references needed by retained windows. + +The store's persistence, replication, retention, and eviction settings are part of the guarantee. +Naming Redis Streams as the delivery mechanism does not establish the storage failure model. +Sensitive resource payloads must be encrypted before persistence; queue key rotation must retain +access to accepted work for its recovery lifetime. + +Report durable backlog bytes, oldest pending work, outstanding saves, current retry deadline, +publication uncertainty, and status-projection lag. These proposed measurements distinguish a +healthy outage backlog from a worker that has stopped making progress. + +## Implementation phases + +The event model owns the immediate retry/deadline fix and extraction of deterministic worker +transitions. These phases own persistence and HA after that boundary is available. Each phase +needs its own failure tests; adding leadership must not be the first recovery test of the journal. ### HA-0: Specify and expose branch ownership @@ -184,21 +310,45 @@ Before changing delivery: Add a journal package beside the existing Redis store: -- publish idempotent work records; -- create consumer groups, claim abandoned pending records, and acknowledge only - remote-successful work; -- encode and encrypt sensitive payloads; -- atomically couple cursor advancement to durable publication; -- route both live events and snapshot/replay markers through the journal. - -Refactor EventRouter so it publishes durable work instead of directly calling a -local GitTargetEventStream. Refactor BranchWorker so a durable consumer, rather -than its process-local FIFO, owns the acknowledgement lifecycle. The commit -window may remain in memory because unacknowledged records reconstruct it after -failover; local commits must be treated as disposable. - -Ship and exercise this phase with one active Pod first. It removes the existing -crash-loss window independently of multi-Pod scheduling. +- publish idempotent records with explicit admission receipts; +- atomically couple watch cursor advancement to durable admission; +- persist worker decisions and effect obligations with their processing position; +- route live observations, snapshots, saves, withdrawals, deadlines, and results through the + recoverable boundary; +- encode and encrypt sensitive payloads, enforce quotas, and page retained payloads to bound memory; +- create consumer groups, reclaim abandoned work, and preserve terminal receipts and pending + status projections when acknowledging completed delivery. + +Refactor EventRouter and the worker adapters to use this boundary. The open window can remain +an in-memory working view only if recorded decisions or a checkpoint reconstruct its membership, +save attachment, and deadlines. An unacknowledged list of raw resource events is insufficient. +Local commits remain disposable except for artifacts explicitly retained by the publication +recovery protocol. + +Persist schema and decision-behavior versions from the first journal implementation. Test +checkpoint positions, compaction, and compatible readers against pending effects and receipts. +Make target replacement and worker retirement explicit transfer or terminal decisions. + +Exit criterion: a replacement process with an empty checkout restores pending windows and saves, +overdue deadlines, and unprojected outcomes. Lost acknowledgments find the original records; +a crash after recording a deadline or effect still allows it to run. Ship and exercise this +phase with one active Pod before adding leader failover. + +### HA-1a: Prove publication recovery and outage capacity + +Implement the chosen evidence protocol before claiming preserved save outcomes across restart. +Inject crashes before pushing, after Git accepts the push, after recording the result, and +before status projection. Exercise both changed trees and empty commits. Reconstructing a +historical result must cause no external action. + +Size an outage workload to the stated duration and admission rate. Restart during it, fill the +quota, recover Git, and keep new traffic arriving while the backlog drains. Measure memory, +journal growth, remote attempts, and time to settle saves. Sleeping for hours at low volume +would establish little about capacity. + +Exit criterion: the workload fits its storage and memory budgets and preserves the agreed save +contract. Record an explicit outcome for cases whose publication remains indeterminate within +the supported failure model. Keep that limitation visible before enabling HA. ### HA-2: Enable active/passive controller failover @@ -214,12 +364,17 @@ durations rather than relying on undocumented defaults. unacknowledged records for the next leader. - On startup, claim pending records, rebuild workers from remote Git, then establish watches from their durable cursors or fresh replay. -- Persist CommitRequest terminal results in Kubernetes status. A failover must - not depend on an in-memory outcome map to decide whether a command completed. +- Enforce journal ownership epochs and reject stale writers at the durable transition boundary. +- Project recorded `CommitRequest` outcomes into Kubernetes status by UID. Recover the terminal + receipt and any pending projection after failover, without deciding the save outcome again. + +Exit criterion: failover and supported rolling upgrades preserve the single-active contract, +including already-started pushes, overdue timers, and unprojected outcomes. Readers must remain +compatible with retained schema and behavior versions. ### HA-3: Release the supported Helm mode -Only after HA-1 and HA-2 pass end-to-end fault tests: +Only after HA-1, HA-1a, and HA-2 pass end-to-end fault tests: - remove the replica-count rejection; - reject HA configuration without a Redis endpoint, queue-encryption key, and @@ -230,39 +385,50 @@ Only after HA-1 and HA-2 pass end-to-end fault tests: maxSurge one, and a PDB with minAvailable one; - make hostname anti-affinity and topology spread required for the HA profile; offer a zone-spread profile where clusters have multiple zones; -- document a supported Redis or Valkey durability topology. It must survive the - failure model being advertised, not merely pass a ping. +- document a supported Redis or Valkey topology that survives the advertised storage failures. The existing PDB can remain for single-Pod installs, but it must not be described as HA by itself. ### HA-4: Fault-injection acceptance suite -Add e2e tests for each durable boundary: - -- kill the active Pod before durable publication; -- kill it after publication and cursor advancement, before local commit; -- kill it after local commit, before remote push; -- kill it after remote push, before journal acknowledgement; -- force a slow push and leader handoff to exercise a stale writer; -- force remote branch movement and verify replay plus PushAtomic; -- force expired watch cursors and verify replay plus mark-and-sweep; -- roll the deployment with queued, replay, delete, and sensitive-resource work. - -Every case must assert final remote Git state, no lost deletion, no divergent -ref, no permanently pending journal record, and a healthy replacement leader. -Run the normal repository validation sequence after implementation: task fmt, -task generate where needed, task vet, task lint, task test, and task test-e2e. - -## Optional Follow-on: Active/Active Branch Shards +Add fault tests for each durable boundary before enabling the supported HA mode: + +| Scenario | Required observation | +|---|---| +| Crash before durable admission | No durable cursor skips an absent record | +| Crash after admission or a lost admission acknowledgment | Same accepted record returns in branch order | +| Crash after local commit, before push | Unpublished work and its save remain recoverable | +| Crash with an attached save | Same author decision, window membership, and deadlines return | +| Restart with overdue deadlines and queued writes | The defined processing-time ordering is preserved | +| Crash after push, before recording success | Publication is recovered or explicitly indeterminate | +| Status API fails after publication | Same outcome is projected later without another commit | +| Empty save is redelivered | Original result is recovered; missing evidence remains explicit | +| Snapshot lacks a complete marker | No sweep runs | +| Watch cursor expires | Fresh replay preserves the old journal tail and scoped deletion rules | +| Journal quota fills during an outage | Admission stops explicitly; recovery retains capacity | +| Target or provider is recreated | Old work cannot bind to the replacement by name | +| Old owner completes an in-flight push | Replacement recovers the outcome and rejects stale journal transitions | +| Remote branch moves | Compare-and-swap and recorded rebuild attempts preserve the pending work | +| Upgrade or compaction leaves pending work | State, payloads, receipts, and compatible readers remain available | +| Sensitive-resource backlog crosses key rotation | Accepted work remains decryptable without plaintext storage | + +Assert final Git state and deletion handling where the scenario establishes publication, and +explicit uncertainty where evidence is missing. No case may invent a success or silently discard +an accepted save. Exercise the actual storage persistence failure model, including lost +acknowledgments, as well as process interruption. Run the normal repository validation sequence +after implementation: `task fmt`, `task generate` where needed, `task vet`, `task lint`, `task test`, +and `task test-e2e`. + +## Optional follow-on: active/active branch shards Do not make this a prerequisite for HA v1. It improves throughput and isolation, but creates a second distributed-systems problem. When needed, assign each BranchWriteShard its own Kubernetes Lease. Any Pod may publish durable records, but only the shard-Lease holder consumes that shard and -pushes its Git branch. The lease is coordination, not the final Git fence; the -remote compare-and-swap remains mandatory. +pushes its Git branch. The lease coordinates ownership; remote compare-and-swap remains mandatory +with the publication-recovery limits described above. Tracking and watching can remain leader-owned initially. Distributing target watches or GVR scopes should happen only after the snapshot markers, replay @@ -270,16 +436,17 @@ watermarks, deduplication, and completeness state are proven durable. A target with several watched scopes must never sweep Git state until every required scope has completed its authoritative replay. -## Acceptance Criteria for Supported HA +## Acceptance criteria for supported HA - A two-Pod controller deployment survives loss of the active Pod without silently skipping Kubernetes state. - No durable cursor can advance past work that is absent from the journal. -- No journal record is acknowledged before its state is recoverable from remote - Git. +- No completed delivery is acknowledged without recoverable publication evidence or an explicit + terminal outcome; pending status projection and required deduplication receipts remain durable. - A stale or partitioned owner cannot overwrite a newer Git ref. -- Replays, queue claims, and post-push-before-ack crashes converge without - divergent commits. +- Replays and queue claims preserve accepted save obligations. Post-push-before-ack crashes use + the publication protocol; missing evidence produces explicit uncertainty rather than an + unsupported success or duplicate-publication guarantee. - Sensitive resource content is never stored as plaintext in the durable queue. - The chart prevents unsupported HA configurations and documents dependencies that must themselves be highly available. From 3708b529f7a5e40182b0a253696d0f0ef5b0c6fb Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 07:51:10 +0000 Subject: [PATCH 4/6] docs(design): describe the publication retry as built where this branch ships it The event model, its INDEX row and the HA plan still described the quiet-branch retry as the next fix. They now say #412 built it, keep the Git operation deadlines as the next step, and record that a publication that keeps failing shows only in logs and the push metric. The state-of-affairs page marks gaps 1 and 2 fixed, records the event-model review as resolved, and is listed in INDEX. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/INDEX.md | 8 +- docs/design/branch-worker-event-model.md | 63 +++++----- docs/design/gittarget-state-of-affairs.md | 112 +++++++----------- docs/future/ha-gittarget-distribution-plan.md | 11 +- 4 files changed, 87 insertions(+), 107 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 1d0e06d0..39d005ff 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -124,11 +124,12 @@ decided), **design, decided** (decision made, not built), **partly built**, **bu **deferred** (parked, kept as a decision record). The label is the first thing in the page, so you never have to read a proposal to find out it already shipped. -### Open (28 pages) +### Open (29 pages) | Doc | Open question | |---|---| -| [`branch-worker-event-model.md`](design/branch-worker-event-model.md) | **proposal against merged #407 and #411.** Lead with the quiet-branch `WaitingForPush` failure and the retry/deadline fix. Worker ownership, FIFO inputs, save and deadline semantics, and the transition boundary for later durability. Journal storage and rollout belong to the HA plan. | +| [`branch-worker-event-model.md`](design/branch-worker-event-model.md) | **proposal; its first fix is built in #412.** The quiet-branch `WaitingForPush` failure (a failed push now retries on its own backoff) and the Git operation deadlines still to do. Worker ownership, FIFO inputs, save and deadline semantics, and the transition boundary for later durability. Journal storage and rollout belong to the HA plan. | +| [`gittarget-state-of-affairs.md`](design/gittarget-state-of-affairs.md) | **snapshot, 2026-10-02.** What is built for `GitTarget` and the branch worker, which page owns which decision, the known gaps ranked by user impact (verified in code), and the recommended order. Read it first to find the right page on this topic. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407.** `spec.parentBranch`: a target stays on standby until a write needs a branch, then creates its write branch from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: parent per `GitProvider.status.branches` entry. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | | [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **deferred.** An empty repository's default branch: the server reports it over protocol v2 but go-git discards it (upstream fix first); then a separate policy decision on which branch the first commit may create. Assuming `main` was considered and rejected. | @@ -200,7 +201,8 @@ whole edit-through-the-API workstream started, and still holds the branch/sessio grouping strategies nothing else covers. [`ha-gittarget-distribution-plan.md`](future/ha-gittarget-distribution-plan.md) owns the durable journal, publication recovery, outage retention, and persistence/HA rollout. The branch worker -event model owns execution semantics and the immediate retry/deadline fix. +event model owns execution semantics and the Git operation-deadline fix; its publication retry +shipped in #412. [`least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md) has three open RBAC items. [`config-surface-for-a-structured-repository.md`](future/config-surface-for-a-structured-repository.md) diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index 5aa04a40..6a50c150 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -1,8 +1,8 @@ # Branch worker events, durable execution, and recovery -A single failed push on a quiet branch can leave a `CommitRequest` in `WaitingForPush` -indefinitely, even after Git recovers. Repair that liveness gap first: schedule publication retries -and bound Git operations while keeping the branch worker as the owner of its checkout. +A single failed push on a quiet branch used to leave a `CommitRequest` in `WaitingForPush` +indefinitely, even after Git recovered. #412 repairs the retry half of that liveness gap. Bounding +Git operations is the next step, with the branch worker still the owner of its checkout. The longer-term direction is recoverable execution of accepted writes, save commands, window decisions, deadlines, and publication outcomes. This document owns the worker's event semantics, @@ -11,8 +11,8 @@ current failure analysis, and immediate implementation scope. The journal storage, acknowledgments, publication recovery, retention, and the persistence/HA rollout. The broader ownership contract is in [the architecture](../architecture.md#git-write-architecture). -Status: design proposal against the code after merged PRs #407 and #411. Current-behavior sections -describe that implementation. Event names and the transition boundary are proposals. This +Status: design proposal against the code after #407, #411 and #412 (publication retry, built). +Current-behavior sections describe that implementation. Event names and the transition boundary are proposals. This document changes no runtime behavior and does not establish an outage or exactly-once guarantee. ## Next implementation: restore publication progress @@ -20,10 +20,10 @@ document changes no runtime behavior and does not establish an outage or exactly Ship retry scheduling and Git operation deadlines before the journal work. Neither fix depends on choosing a storage backend, changing the save contract, or implementing HA. -1. Give retained publication work a bounded failure backoff that runs without another source - edit. Coordinate it with parent recovery and the successful-push cooldown so arrivals cannot - bypass the backoff or multiply the remote probe budget. Stop scheduling when the obligation - settles; a canceled worker must not rearm it. +1. **Built in #412.** Retained publication work has a bounded failure backoff (10s, doubling to + 5m) on the push timer, in [publication_retry.go](../../internal/git/publication_retry.go). + Commits before the deadline do not push; a success resets it; a missing parent stays with + parent recovery's probe; a stopped worker's timers are stopped with it. 2. Carry cancellation through ref listing and fetch, and impose operation deadlines across the push cycle. Bound the cycle's total occupancy as well as its individual calls. Repeated contention retries must not repeatedly reset the whole budget. Keep timed-out work pending; @@ -37,7 +37,8 @@ on choosing a storage backend, changing the save contract, or implementing HA. The completion test is a save whose first push fails, followed by silence and remote recovery: without another resource edit, its commit reaches Git and the same request reaches its terminal -status with the published SHA. Also prove that a request can safely outlive the controller's +status with the published SHA. `TestPublicationRetry_AHeldCommitRequestIsCommittedWithoutAnotherWrite` +covers that on the real event loop. Still to prove that a request can safely outlive the controller's safety window while retries continue, and that a stalled remote call returns control within the operation budget. Use a controllable remote for those failures and a fake clock for retry timing. @@ -143,34 +144,37 @@ not an archive of every mutation. These gaps are visible in the current code. Making the queue durable addresses only some of them. -### A failed push can strand a save in `WaitingForPush` +### A failed push stranded a save in `WaitingForPush` (fixed in #412) -`pushPending` retains failed writes but stops the push timer without arming a general retry. -If no more writes arrive, a quiet branch can remain unpublished indefinitely. Ordinary refresh -skips a branch with retained work, so periodic `GitTarget` reconciliation does not repair this. +Before #412, `pushPending` retained failed writes but stopped the push timer without arming a +retry. If no more writes arrived, a quiet branch stayed unpublished indefinitely, and ordinary +refresh skips a branch with retained work, so periodic `GitTarget` reconciliation did not repair it. +The failure now schedules its own retry. The rest of this section records why the fix had to be +on the worker. -For an attached save that committed locally, this leaves `WaitingForPush` with no time limit. +For an attached save that committed locally, this left `WaitingForPush` with no time limit. After its safety window, `withdrawCommitRequest` in the [controller](../../internal/controller/commitrequest_controller.go) sends withdrawal and continues polling when the worker still holds the request. `handleWithdrawCommitRequest` in the [attach loop](../../internal/git/commit_request_attach_loop.go) leaves attached or committed -requests unchanged. Neither path retries the push. The API continues to show `Ready=False`, +requests unchanged. Neither path retries the push. The API showed `Ready=False`, `Reconciling=True`, `Stalled=False`, and `Pushed=Unknown`, with reason `WaitingForPush`. That withdrawal rule protects against reporting failure and then publishing the save later. `TestCommitRequestReconcile_AHeldRequestOutlivesTheBound` explicitly preserves it. The defect is -that the live worker can retain responsibility without scheduling progress. Restore retries; -do not turn this into a controller-only timeout that can give a false terminal result. +that the live worker retained responsibility without scheduling progress. The fix restores +retries rather than adding a controller-only timeout that could give a false terminal result. -Continued writes can cause the opposite problem. Only successful pushes advance `lastPushAt`. -Once the successful-push cooldown has elapsed, later commits can repeatedly attempt the same -unavailable remote. The [push-cooldown design](push-cooldown.md#7-options) records this gap. +Continued writes caused the opposite problem: only successful pushes advance `lastPushAt`, so +once the cooldown had elapsed every later commit attempted the same unavailable remote. The retry +deadline now governs those commits too ([push-cooldown design](push-cooldown.md#7-options), +option C). -[Parent recovery](../../internal/git/parent_recovery.go) already models a useful alternative: an -obligation remains open and schedules its next attempt. Its probe starts at 10 seconds and backs -off to five minutes, sharing the budget across targets on the worker. General publication failure -needs the same liveness property, with scheduling coordinated so the two paths cannot multiply -remote attempts. +The retry follows [parent recovery](../../internal/git/parent_recovery.go), which already kept an +obligation open and scheduled its next attempt (10 seconds, backing off to five minutes, shared +across targets on the worker). A failure caused by a missing parent stays on that schedule, so +the two paths cannot multiply remote attempts. A publication that keeps failing is still visible +only in logs and `gitopsreverser_git_pushes_total{outcome="failed"}`, not in `GitTarget` status. ### Slow Git operations stop the whole branch loop @@ -375,13 +379,14 @@ The current worker has five timer sources: | Deadline | Existing purpose | |---|---| | Commit window | Close on idle timeout or maximum duration | -| Push cooldown | Space successful publication cycles | +| Push timer | Space successful publication cycles, or retry a failed one on its backoff | | Attach timeout | Settle a save still waiting for an eligible window | | Refusal action | Recheck consent before an eligible empty commit | | Parent recovery | Probe and service retained work or owed snapshots | -Add a general publication retry obligation. Keep successful-push cooldown and failure backoff -distinct, while giving the branch one policy for when it may spend its next remote attempt. +The publication retry (#412) shares the push timer with the success cooldown, and keeps its own +backoff, so the branch has one policy for when it may spend its next remote attempt. A durable +design must persist that retry deadline like any other. An illustrative deadline protocol is: diff --git a/docs/design/gittarget-state-of-affairs.md b/docs/design/gittarget-state-of-affairs.md index d610f825..b2e186f1 100644 --- a/docs/design/gittarget-state-of-affairs.md +++ b/docs/design/gittarget-state-of-affairs.md @@ -1,7 +1,8 @@ # GitTarget and the branch worker: state of affairs -> **Snapshot**, taken 2026-10-02 at `230d602a` (#411 on `main`), with the uncommitted rewrite of -> [`branch-worker-event-model.md`](branch-worker-event-model.md) in the working tree. It describes +> **Snapshot**, taken 2026-10-02 at `230d602a` (#411 on `main`), and updated for #412, which fixes +> gaps 1 and 2 below, closes the 0.51 docs gaps, and lands the rewritten +> [`branch-worker-event-model.md`](branch-worker-event-model.md) and HA plan. It describes > what is built, which page owns which decision, which edge cases are known not to work, and a > recommended order for what comes next. It decides nothing by itself; each linked page stays the > owner of its own contract. Every claim about behavior below was checked against the code or a @@ -49,17 +50,18 @@ The release PR body does not list #411 yet; release-please adds it on its next r | [`gittarget-red-status-plan.md`](gittarget-red-status-plan.md) | partly built | Why a target is red, and what the message must say | Step 5 (the operator section) shipped and the page does not say so. The step 1 cross-package reason-agreement test was not found. Open question 2 (refusal state across a restart) is still open: `GitPathAccepted` lives in watch-manager memory. | | [`gittarget-configuration-freshness.md`](gittarget-configuration-freshness.md) | deferred | Whether a target's running watch plan matches its configuration | Deferred with explicit pick-up triggers; neither trigger has fired. | | [`gittarget-api-wave.md`](gittarget-api-wave.md) | effectively done | The `GitTarget` field boundaries | Every step shipped, was dropped, or was declined. A candidate for `docs/finished/`. | -| [`branch-worker-event-model.md`](branch-worker-event-model.md) | proposal, **uncommitted rewrite** | The branch worker's recovery boundary: liveness, durable admission, save identity across restarts, publication evidence | Reviewed in detail below. Not listed in [`INDEX.md`](../INDEX.md). | +| [`branch-worker-event-model.md`](branch-worker-event-model.md) | proposal; retry built in #412 | Worker execution semantics: the FIFO inputs, deadlines, saves, the transition boundary, and the operation-deadline fix still to do | Leads with the liveness fix, names both unbounded network calls, and hands the journal to the HA plan. | | [`push-cooldown.md`](push-cooldown.md) | design | Success cooldown versus failure backoff | §7 option C (failure backoff) is the recommended fix for gap 1 below. | -| [`../spec/commitrequest-design.md`](../spec/commitrequest-design.md) | spec | The save lifecycle | Still says `NoOpenWindow` and "close deadline" (around line 138). | -| [`../future/ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) | future | HA, durable delivery, journal record kinds | Overlaps the event model's stages 4 to 6; the two need one owner for the journal design. | -| [`../TODO.md`](../TODO.md) | backlog | Release blockers and follow-ups | Its "Before the next release" list is still open (checked below). | +| [`../spec/commitrequest-design.md`](../spec/commitrequest-design.md) | spec | The save lifecycle | Current as of #412. | +| [`../future/ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) | future | The durable journal, publication recovery, outage retention, and the persistence and HA rollout | The single owner of the journal since #412. | +| [`../TODO.md`](../TODO.md) | backlog | Follow-ups | Its "Before the next release" list is closed by #412. | ## Known gaps, ranked by what a user would hit Each was checked in the code at `230d602a`. -1. **A failed push on a quiet branch strands the work, and any save riding it waits forever.** +1. **Fixed in #412: a failed push on a quiet branch stranded the work, and any save riding it + waited forever.** `pushPending` stops the push timer on failure and arms nothing; its own doc comment says so ([`branch_worker.go`](../../internal/git/branch_worker.go), `pushPending`). The refresher skips a branch with retained writes (`refresh.go`, "this branch is mid-cycle"). The controller does @@ -67,7 +69,8 @@ Each was checked in the code at `230d602a`. (`handleWithdrawCommitRequest`). So after one transient push failure with no further edits, the commit stays local and the `CommitRequest` stays `WaitingForPush` until some other write arrives. Parent recovery is the only failure path that schedules its own retry. -2. **A down remote under steady traffic costs one failed push per commit.** `lastPushAt` advances +2. **Fixed in #412: a down remote under steady traffic cost one failed push per commit.** + `lastPushAt` advances only on success, so once the cooldown has passed every new commit attempts the push again. 3. **No Git network call has a deadline.** `remote.List` (`listRemoteRefs`, `CheckRepo`) and `repo.Fetch` (`SmartFetchFrom`) take no context; the push uses the worker's context, which has @@ -79,97 +82,66 @@ Each was checked in the code at `230d602a`. (`dropFailedWindow`). Only a missing parent records the dropped scope and asks for a snapshot, so under any other failure those writes wait for the next resync. While the rebuild keeps failing, every new window spends a fetch on it. -5. **Accepted work is volatile.** The FIFO (1,000 items) and retained writes are process memory. +5. **A publication that keeps failing is not in status.** Since #412 it retries every 5m at most, + with a save riding it held in `WaitingForPush`, but the `GitTarget` shows nothing: only the + logs and `gitopsreverser_git_pushes_total{outcome="failed"}` do. A revoked credential or a + branch-protection refusal therefore looks like a slow save. +6. **Accepted work is volatile.** The FIFO (1,000 items) and retained writes are process memory. The watch cursor can advance before a write reaches Git, so a crash in between can skip work on restart. The 8 MiB buffer threshold closes a window early; it does not bound retained memory. -6. **A save's identity does not survive a restart.** A push that succeeds immediately before a crash +7. **A save's identity does not survive a restart.** A push that succeeds immediately before a crash loses the worker's receipt, and a resent save can create a second empty commit. The outcome cache keeps entries 15 minutes; the admission author store keeps records one hour. -7. **Refusal state does not survive a restart.** After a manager restart a refused folder reads as +8. **Refusal state does not survive a restart.** After a manager restart a refused folder reads as accepted until the next write or resync refuses it again. -8. **An empty repository with a non-default write branch** gets the write branch as its first, +9. **An empty repository with a non-default write branch** gets the write branch as its first, parentless branch (documented as today's bootstrap contract; deferred policy). -9. **The parent disappears from status once the write branch exists**, so an operator cannot see +10. **The parent disappears from status once the write branch exists**, so an operator cannot see that the parent has moved ahead of an open review branch. -10. **Two `GitProvider`s naming one repository** are two workers and two checkouts on the same +11. **Two `GitProvider`s naming one repository** are two workers and two checkouts on the same remote; nothing coordinates them beyond the server's compare-and-swap. The advice remains one `GitProvider` per repository. -11. **With `parentBranch` omitted, a push cannot see a switch of the remote's `HEAD`**; the next +12. **With `parentBranch` omitted, a push cannot see a switch of the remote's `HEAD`**; the next refresh follows it with one fetch. This is a documented choice, not a defect. -Gaps 1 to 4 are liveness defects in the current worker and can be fixed without any persistence. -Gaps 5 and 6 are the durable-execution problem. Gaps 7 to 10 are separate, smaller decisions. +Gaps 1 to 5 are liveness and visibility defects in the current worker and need no persistence; +1 and 2 are fixed. Gaps 6 and 7 are the durable-execution problem. Gaps 8 to 12 are separate, +smaller decisions. ## Review of `branch-worker-event-model.md` -The rewrite turns the committed page (an inventory of the worker's inputs, deadlines and failure -behavior) into a proposal for an event-sourced state machine per branch with durable execution. -Its factual claims hold against the code: the five timer sources, the 10s to 5m probe, the 1,000 -item queue and 8 MiB threshold, the 15-minute outcome cache, the one-hour author store, -`EnqueueRequest` returning no receipt and having no non-test caller, and the write gates' effective -points. The distinction between the four meanings of "replay", and the refusal to promise -exactly-once publication before the evidence protocol exists, are its strongest parts. - -Findings, most important first: - -1. **The user-visible consequence of the liveness gap is missing.** "A failed push can wait - indefinitely" describes retained writes. It should also say that a save on that write stays - `WaitingForPush` with no bound, because the controller does not time out a held request and - the worker ignores a withdraw for it. That is the strongest argument for stage 2 and the one a - reader acts on. -2. **The timeout gap is wider than stated.** The page names only `remote.List`. `repo.Fetch` is - also called without a context, and no deadline is set anywhere in `internal/git`. -3. **The actionable part is hard to find.** Stage 2 (bounded publication backoff, operation - deadlines, an injectable clock) is a small change to the current worker. Stages 3 to 6 are a - multi-release program that overlaps the HA plan. The page should lead with stage 2 as the - near-term change and say which page owns the journal design, or the two pages will drift. -4. **The status line is dated.** It refers to the `fix/branch-worker-write-gates` changes, which - merged as #411. -5. **The input inventory went away.** The committed version's "Inputs on the FIFO" and "State that - currently enters outside the FIFO" were the most useful reference material in it; the rewrite - compresses them into "Existing contracts to preserve". Keep them, here or in - [`architecture.md`](../architecture.md), because stage 3 needs exactly that inventory. -6. **It is not in [`INDEX.md`](../INDEX.md)**, and INDEX still describes #407 as "not merged yet". +An earlier version of this page reviewed the uncommitted rewrite and found six issues: the missing +user-visible `WaitingForPush` consequence, the understated timeout gap, the buried near-term fix, +a dated status line, the dropped input inventory, and no INDEX entry. The version in #412 addresses +all six, and moves the journal, publication recovery and outage retention into the HA plan, so +each topic now has one owner. Its factual claims were rechecked against the code. ## Recommended order ### Before cutting 0.51.0 -1. **Close the docs blockers in [`TODO.md`](../TODO.md).** Checked, still open: - - [`UPGRADING.md`](../UPGRADING.md)'s commit-window entry has no step telling readers to - suspend the Flux or Argo source of a GitOps-managed `GitTarget`; its Was/Is table has no row - for the chart value `quickstart.gitTarget.commit.window`; and it still says a removed - `closeDelay` "is not a hazard", which is too strong for programmatic clients. - - [`configuration.md`](../configuration.md) line 14 still says a `CommitRequest` closes the - window "now"; [`commitrequest-design.md`](../spec/commitrequest-design.md) still says - `NoOpenWindow` and "close deadline". - - Consider one `UPGRADING.md` line for #411: a save on a suspended target now fails where it - used to report `Ready=True`, and automation that waits for `Ready` will see that. -2. **Fix gap 1, and gap 2 with it: a bounded publication-failure backoff** (push-cooldown §7 - option C, event model stage 2). It is one timer, parent recovery is the model, and it is the - only known defect that leaves a user's object waiting forever. The acceptance test is the - event model's first two scenarios: a push fails, the branch goes quiet, the work publishes - without another edit, and the held save resolves `Committed`; under continuous arrivals the - backoff still spaces attempts. Coordinate it with the parent probe so the two never both - spend a connection on one deadline. If it cannot land within days, ship 0.51.0 without it and - make it 0.51.1, with the gap stated in the release notes. +Both steps are in #412: the docs gaps in `UPGRADING.md`, `configuration.md`, the CRD descriptions +and the CommitRequest spec, and the publication retry with its tests. What is left is merging it +once its e2e legs are green. ### Next, after the release 1. **Operation deadlines (gap 3).** Pass the worker context to `List` and `Fetch` and give each remote operation a bounded deadline. Measure first how go-git's HTTP and SSH transports behave today, against a server that stalls. -2. **Event model stage 1 only**: agree the recovery contract (final-state convergence, or also - preserved save outcomes) and write the failure scenarios as executable tests. Do not start the - journal before that contract is agreed and the HA plan and the event model have one owner. -3. **Cheap, independent items**: open the go-git unborn-`HEAD` issue and PR; parent observation +2. **The transition boundary, not the journal**: extract the event model's explicit transitions + and test them from recorded state, and agree the save contract the HA plan asks for (final-state + convergence, or also preserved save outcomes). Start the journal only after both. +3. **Gaps 4 and 5**: decide what a failed rebuild owes a dropped window, and whether a publication + that keeps failing belongs in `GitTarget` status. +4. **Cheap, independent items**: open the go-git unborn-`HEAD` issue and PR; parent observation step 1 (availability after the write branch exists, no extra connections); the red-status reason-agreement test. -4. **Housekeeping**: move the hardening plan and the API-wave page to `docs/finished/`, list the - event model and this page in INDEX, and mark red-status step 5 as shipped. +5. **Housekeeping**: move the hardening plan and the API-wave page to `docs/finished/`, and mark + red-status step 5 as shipped. ### Leave deferred Configuration freshness, ancestry classification, the empty-repository bootstrap policy, and the -durable journal (stages 3 to 6) all wait on a decision or on evidence that does not exist yet. None +durable journal (the HA plan's phases) all wait on a decision or on evidence that does not exist yet. None of them is needed to make 0.51.0 better than 0.50.0. diff --git a/docs/future/ha-gittarget-distribution-plan.md b/docs/future/ha-gittarget-distribution-plan.md index 4fb83745..9de470d9 100644 --- a/docs/future/ha-gittarget-distribution-plan.md +++ b/docs/future/ha-gittarget-distribution-plan.md @@ -4,8 +4,8 @@ Status: **proposed** (not started) This is the owning document for the durable journal, publication recovery, retention, and the persistence/HA rollout. The [branch worker event model](../design/branch-worker-event-model.md) -owns worker transitions, deadline semantics, and the immediate retry and operation-timeout fix. -That fix can ship independently of this plan. +owns worker transitions, deadline semantics, the publication retry (built in #412), and the +operation-timeout fix, which can ship independently of this plan. ## Scope and definition of done @@ -289,9 +289,10 @@ healthy outage backlog from a worker that has stopped making progress. ## Implementation phases -The event model owns the immediate retry/deadline fix and extraction of deterministic worker -transitions. These phases own persistence and HA after that boundary is available. Each phase -needs its own failure tests; adding leadership must not be the first recovery test of the journal. +The event model owns the operation-deadline fix (its publication retry shipped in #412) and +extraction of deterministic worker transitions. These phases own persistence and HA after that +boundary is available. Each phase needs its own failure tests; adding leadership must not be the +first recovery test of the journal. ### HA-0: Specify and expose branch ownership From c9c14ec518e3738307b1c026868f10c12b2d5758 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 08:49:34 +0000 Subject: [PATCH 5/6] fix(git): pace a failed publication by parent recovery's deadline after the parent returns A publication that failed while parent recovery was open cleared the retry, on the grounds that recovery owns it. Once the parent is found, recovery's missing-parent hold is gone while its obligation stays open until the work publishes, so every new commit retried the failing push at once: three attempts where one was due. Such a failure now defers to recovery's next probe deadline; new commits wait for it, and recovery's timer makes the attempt. Found in review; it also failed on main. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/git/publication_retry.go | 27 +++++++++----- internal/git/publication_retry_test.go | 49 ++++++++++++++++++++++---- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/internal/git/publication_retry.go b/internal/git/publication_retry.go index 8f98a72f..708ce868 100644 --- a/internal/git/publication_retry.go +++ b/internal/git/publication_retry.go @@ -17,10 +17,11 @@ import "time" // the push timer is armed for it whether or not anything else arrives. Until then new commits // accumulate locally instead of pushing. A successful push, or nothing left to push, closes it. // -// A failure caused by a missing parent is not retried here. Parent recovery owns that work, with -// its own probe schedule shared across the worker's targets (parent_recovery.go); retrying it on a -// second schedule would spend connections that schedule exists to save. The same holds for any -// failure while that obligation is open: its deadline retries the retained writes. +// A failure while parent recovery is open is not retried here. Recovery owns that work, with its +// own probe schedule shared across the worker's targets (parent_recovery.go); retrying it on a +// second schedule would spend connections that schedule exists to save. Its deadline still paces +// new commits, though: once the parent is found, recovery's missing-parent hold is gone, so +// without deferring to its deadline every new commit would retry the failing push at once. //nolint:gochecknoglobals // vars, not consts, so a test can run the real loop on a short schedule. var ( @@ -35,18 +36,21 @@ const publicationRetryBackoffFactor = 2 type publicationRetry struct { backoff time.Duration nextAttempt time.Time + // deferToRecovery marks a failure while parent recovery was open: the next attempt is + // recovery's next probe deadline, and recovery's timer, not the push timer, drives it. + deferToRecovery bool } // pending reports whether a failed publication is waiting for its next attempt. func (r publicationRetry) pending() bool { - return !r.nextAttempt.IsZero() + return !r.nextAttempt.IsZero() || r.deferToRecovery } -// notePublicationFailed schedules the next attempt after a failed push or rebuild, unless parent -// recovery owns the retry. +// notePublicationFailed schedules the next attempt after a failed push or rebuild. While parent +// recovery is open the attempt is recovery's, and new commits wait for its deadline. func (l *branchWorkerEventLoop) notePublicationFailed() { if l.recovery.active { - l.publicationRetry = publicationRetry{} + l.publicationRetry = publicationRetry{deferToRecovery: true} return } r := &l.publicationRetry @@ -68,6 +72,13 @@ func (l *branchWorkerEventLoop) notePublicationSettled() { // new commit must not push early. It makes sure the attempt is still scheduled. func (l *branchWorkerEventLoop) awaitingPublicationRetry() bool { r := l.publicationRetry + if r.deferToRecovery { + if !l.recovery.active { + l.publicationRetry = publicationRetry{} + return false + } + return l.w.now().Before(l.recovery.nextProbeAt) + } if !r.pending() || !l.w.now().Before(r.nextAttempt) { return false } diff --git a/internal/git/publication_retry_test.go b/internal/git/publication_retry_test.go index 05a0397e..8f734005 100644 --- a/internal/git/publication_retry_test.go +++ b/internal/git/publication_retry_test.go @@ -51,6 +51,13 @@ func seedMain(t *testing.T) func(string) { } } +func seedMainAndRelease(t *testing.T) func(string) { + return func(repoDir string) { + simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "main\n") + gitIn(t, repoDir, "branch", "release", "main") + } +} + // The gap this closes: after one failed push, a branch that receives nothing else used to keep its // work indefinitely. The retry is armed by the failure itself. func TestPublicationRetry_AQuietBranchPublishesAfterAFailedPush(t *testing.T) { @@ -137,10 +144,7 @@ func TestPublicationRetry_BackoffDoublesToTheCapAndResets(t *testing.T) { // it on the worker's probe schedule. A second schedule here would spend the connections that one // exists to save. func TestPublicationRetry_ParentRecoveryOwnsAMissingParent(t *testing.T) { - f := newRealServerNewBranch(t, "retry-parent", func(repoDir string) { - simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "main\n") - gitIn(t, repoDir, "branch", "release", "main") - }) + f := newRealServerNewBranch(t, "retry-parent", seedMainAndRelease(t)) useTestClock(f.worker) f.worker.SetParentBranch("release") require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) @@ -153,8 +157,41 @@ func TestPublicationRetry_ParentRecoveryOwnsAMissingParent(t *testing.T) { require.Len(t, f.loop.pendingWrites, 1) open, _ := f.worker.ParentRecovery() require.True(t, open, "parent recovery holds the work") - assert.False(t, f.loop.publicationRetry.pending(), "and the publication retry stays out of it") - assert.Nil(t, f.loop.pushTimer) + assert.True(t, f.loop.publicationRetry.deferToRecovery, "the publication retry defers to it") + assert.Nil(t, f.loop.pushTimer, "and arms no timer of its own") +} + +// Once the parent is back, recovery's missing-parent hold is gone but its obligation stays open +// until the work publishes. A publication that fails then must not be retried by every new commit: +// the commits wait for recovery's next deadline. Found in review of #412; it failed on main too. +func TestPublicationRetry_AFailureAfterTheParentReturnsWaitsForRecovery(t *testing.T) { + f := newRealServerNewBranch(t, "retry-parent-found", seedMainAndRelease(t)) + clock := useTestClock(f.worker) + f.worker.SetParentBranch("release") + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + f.loop.lastPushAt = time.Now() + liveWrite(f.loop, "cm1") + gitIn(t, f.repoDir, "update-ref", "-d", "refs/heads/release") + f.loop.pushPending() + require.True(t, f.loop.recovery.active) + require.False(t, f.loop.recovery.found) + + gitIn(t, f.repoDir, "branch", "release", "main") // the parent returns + attempts := failNextPushes(t, 100) + clock.advance(parentProbeInitialBackoff) + f.loop.lastPushAt = time.Time{} + f.loop.runParentProbe() + require.True(t, f.loop.recovery.found) + require.Equal(t, int32(1), attempts.Load(), "recovery's attempt failed") + require.True(t, clock.Now().Before(f.loop.recovery.nextProbeAt)) + + liveWrite(f.loop, "cm2") + liveWrite(f.loop, "cm3") + assert.Equal(t, int32(1), attempts.Load(), "commits before recovery's deadline do not retry") + + clock.advance(parentProbeMaxBackoff) + f.loop.runParentProbe() + assert.Equal(t, int32(2), attempts.Load(), "recovery's next deadline does") } // The user-facing half, on the real event loop: a CommitRequest riding a write whose push failed is From bc36d737845778507ee98ad398e5b44cc111d9d0 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 2 Oct 2026 08:49:35 +0000 Subject: [PATCH 6/6] docs: act on the #412 review UPGRADING names the reconciler to pause (the Flux Kustomization or HelmRelease, or Argo CD automated sync) and restores Git-managed values before resuming. The push-cooldown analysis drops obsolete claims: the retry now shares the push timer, and serialization does not batch queued commits. The failure-status gap is narrowed to the worker's publication failure and retry deadline. The HA plan states that its phases assume preserved save outcomes and that combining providers needs explicit repository-identity and credential decisions. Rebuild recovery and publication visibility now come before the transition refactor. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/INDEX.md | 2 +- docs/UPGRADING.md | 14 +- docs/api-first-publication.md | 6 +- docs/design/branch-worker-event-model.md | 56 ++++--- docs/design/gittarget-state-of-affairs.md | 83 ++++++----- docs/design/push-cooldown.md | 137 +++++++++--------- docs/future/ha-gittarget-distribution-plan.md | 13 ++ docs/spec/commitrequest-design.md | 6 +- 8 files changed, 183 insertions(+), 134 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 39d005ff..0ee9466a 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -150,7 +150,7 @@ so you never have to read a proposal to find out it already shipped. | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | **built.** the canonical metrics document, rewritten 2026-09 and shipped in the same wave. Reads the product as two ingestion paths — object state through watch, authorship through audit — joining at the resolver, and gives each stage one counter with a bounded `outcome` so the flow and its loss paths are drawable. Records what three review rounds corrected, including two reversals worth not re-litigating: the fact-loss counters must NOT be merged (an eviction counts a fact, a trim gap an occurrence, a decode error a whole batch), and a cumulative counter cannot replace a current count, which is why `status.streams` and `status.retainedDocuments` stayed. The instrument inventory lives in [`interpreting-metrics.md`](interpreting-metrics.md), not here. Open: the Grafana JSON and alert rules | | [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | | [`push-notification-and-reconcile-trigger.md`](design/push-notification-and-reconcile-trigger.md) | **part shipped, the rest is a menu.** Merges the old `inbound-push-notification.md` and `support-boundary/orchestrator-reconcile-trigger.md`, which argued two halves of one subject: the messages Reverser and the reconciler do not send each other. Part 1 is built and measured: the pre-push fetch is gone because the compare-and-swap push already reads the advertisement and a cycle that commits nothing still reaches it, carried by a three-flag invariant (`baseTrusted`/`worktreeDirty`/`replayRequired`, each bracketing a different span), a measured round-trip ledger, and a per-reason fetch counter. Part 2 is the two gaps: nothing tells an idle target its branch moved, and a refused write produces no commit so nothing reverts it. It also states **which side wins** (a captured API write beats Git silently; the cell worth protecting is the object a push changed that we hold no intent about) and why wiring a webhook to the `requestedAt` annotation is a correctness bug rather than a shortcut. Part 3 is six options with consequences, **doing nothing first**: a maximum age on `baseTrusted`, telling the reconciler after we push (which closes the API-first loop with **no Git-host webhook at all**), receiving a notification, taking the webhook over and forwarding it (so **we never have to work out which orchestrator object syncs a folder** — neither Argo CD nor Flux derives that mapping either), holding the push as an optimization that cannot be a barrier, triggering a reconcile to revert a refused edit, and **pushing an empty commit**, which closes that same gap for nothing: a new SHA defeats Argo CD's `selfHeal: false` skip legitimately (the skip is gated on having already synced *this revision*) and gives Flux a new artifact revision, so neither needs us to name an object, patch one, or hold an Argo sync authority. Its costs are a branch-wide blast radius and what it does under `prune: true`. Option 8 hands the action to the installer instead, and records that **executing a script mounted from a ConfigMap is declined**: it is arbitrary code execution as a process holding Git credentials, SSH and age keys and remote kubeconfigs, and Argo CD removed exactly that pattern in 2.8 in favour of a sidecar. The seam is a structured Event carrying the refused object and reason, which the existing `EventRecorder` makes nearly free. Options 7 and 8 together are why option 6, and the ownership interpreters it needs, may never be built | -| [`push-cooldown.md`](design/push-cooldown.md) | **design, open.** Whether the fixed `5s` push cooldown still earns its place now that the commit window exists. For ONE author writing one target it is provably inert at the defaults, because the window is a rolling silence timer of the same length. But there is only one open window per worker, so a second identity finalizes it on arrival: under alternating authors the silence timer never fires and the cooldown is the only thing batching, which makes a multi-user attributed cluster the normal case rather than the exception. Prices what removal would delete (about thirty lines and two fields) against what it would not touch (the retained-write, base-trust and replay machinery, which exists for push failure), and concludes that the unambiguous gap is the missing failure backoff rather than the cooldown itself | +| [`push-cooldown.md`](design/push-cooldown.md) | **partly built.** Whether the fixed `5s` push cooldown still earns its place now that the commit window exists. Ordinary timer closures already space one author writing one target, but identity changes, saves with shorter timers and buffer-limit closures commit sooner, and serialization does not batch queued commits, so the cooldown is what batches them. Option C, the failure backoff, is built in #412 and shares the push timer; removing the success wait would now remove only `lastPushAt` and its branch, so that decision waits on the event-loop measurements in §9 | | [`multi-source-audit-ingress-hardening.md`](design/multi-source-audit-ingress-hardening.md) | how independent sources authenticate to a named audit route, when annotation routing is trustworthy, and how multi-provider ingestion remains fair | | [`release-image-reuse-plan.md`](design/release-image-reuse-plan.md) | PRs 2–5 unstarted | | [`e2e-coverage-gaps-and-improvements-plan.md`](design/e2e-coverage-gaps-and-improvements-plan.md) | tests A/B/C still proposals | diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ba1b3ec8..40739f24 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -50,6 +50,14 @@ then collects for the `2s` default. In order: 1. **Remove the old field while the old schema is still served**, noting each value: + If GitOps manages the `GitTarget`, first pause the reconciler that applies it: the Flux + `Kustomization` or `HelmRelease`, or automated sync on the Argo CD `Application`. Keep that + pause in the managing configuration too if another controller would undo it. Suspending only + a Flux `GitRepository` leaves the applying reconciler able to use its existing artifact. + Alternatively, remove the field from the Git source and wait for that revision to apply. + Keep the old string out of the cluster until step 4; restoring it before the CRD upgrade + recreates the listing failure. + ```bash kubectl get gittargets -A -o json \ | jq -r '.items[] | select(.spec.commit.window != null) @@ -60,16 +68,14 @@ then collects for the `2s` default. In order: The running controller uses its `5s` default meanwhile. - If Flux or Argo CD applies the `GitTarget`, suspend that source first, or remove the field in - its Git source, and keep it that way until step 4. Otherwise it re-applies the string - `commit.window`, and once the new CRD is served that object breaks listing again. 2. **Clear finished requests** rather than carrying them across: `kubectl delete commitrequests -A --all` once in-flight saves have resolved. 3. **Upgrade** the chart: new CRDs and new controller together. 4. **Restore the timing** on the targets noted in step 1, as `commit.window.idleTimeout`. A value over `1m` also needs `commit.window.maxDuration` raised to at least that value (at most `24h`), or admission refuses it. Update anything that creates `CommitRequest`s to set `window` instead of - `closeDelay`. + `closeDelay`. Update the Git-managed manifests or chart values before resuming their + reconciler, then confirm the restored object has the new `window` block. A GitOps source that still carries `window: "5s"` is refused at apply once the new CRD is in place; update it in step 4 along with the rest. diff --git a/docs/api-first-publication.md b/docs/api-first-publication.md index 0023fe95..e0915c36 100644 --- a/docs/api-first-publication.md +++ b/docs/api-first-publication.md @@ -389,8 +389,10 @@ After a push or retained-write recovery fails, the worker keeps the work and sch attempt itself: 10s after the first failure, doubling to at most 5m, whether or not anything else arrives. Commits made before that deadline accumulate locally rather than each trying the remote again, and a successful push resets the schedule. A failure caused by a missing parent branch is -retried on parent recovery's probe schedule instead, so the two never both spend a connection. A -branch with nothing pending stays silent. +retried on parent recovery's probe schedule instead, and so is any failure while that recovery is +open, including after the parent returns: new commits wait for its deadline too. The backoff does +not rate-limit fetches needed by a new window or snapshot. A branch with nothing pending stays +silent. The byte threshold forces finalization but does not free retained writes during a remote outage. It is not a hard memory ceiling. The separate branch queue has `1000` slots by default and drops diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index 6a50c150..526ea30c 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -12,18 +12,20 @@ journal storage, acknowledgments, publication recovery, retention, and the persi The broader ownership contract is in [the architecture](../architecture.md#git-write-architecture). Status: design proposal against the code after #407, #411 and #412 (publication retry, built). -Current-behavior sections describe that implementation. Event names and the transition boundary are proposals. This -document changes no runtime behavior and does not establish an outage or exactly-once guarantee. +Current-behavior sections describe that implementation. Event names and the transition boundary +are proposals. This document changes no runtime behavior and does not establish an outage or +exactly-once guarantee. ## Next implementation: restore publication progress -Ship retry scheduling and Git operation deadlines before the journal work. Neither fix depends -on choosing a storage backend, changing the save contract, or implementing HA. +Complete retry scheduling and add Git operation deadlines before the journal work. Neither fix +depends on choosing a storage backend, changing the save contract, or implementing HA. 1. **Built in #412.** Retained publication work has a bounded failure backoff (10s, doubling to 5m) on the push timer, in [publication_retry.go](../../internal/git/publication_retry.go). - Commits before the deadline do not push; a success resets it; a missing parent stays with - parent recovery's probe; a stopped worker's timers are stopped with it. + Commits before the deadline do not push; a success resets it. While parent recovery is open + its probe deadline is the retry, and new commits wait for it too, including after the parent + returns. A stopped worker's timers are stopped with it. 2. Carry cancellation through ref listing and fetch, and impose operation deadlines across the push cycle. Bound the cycle's total occupancy as well as its individual calls. Repeated contention retries must not repeatedly reset the whole budget. Keep timed-out work pending; @@ -37,10 +39,11 @@ on choosing a storage backend, changing the save contract, or implementing HA. The completion test is a save whose first push fails, followed by silence and remote recovery: without another resource edit, its commit reaches Git and the same request reaches its terminal -status with the published SHA. `TestPublicationRetry_AHeldCommitRequestIsCommittedWithoutAnotherWrite` -covers that on the real event loop. Still to prove that a request can safely outlive the controller's -safety window while retries continue, and that a stalled remote call returns control within the -operation budget. Use a controllable remote for those failures and a fake clock for retry timing. +status with the published SHA. +`TestPublicationRetry_AHeldCommitRequestIsCommittedWithoutAnotherWrite` covers that on the event +loop. Still to prove that a request can safely outlive the controller's safety window while +retries continue, and that a stalled remote call returns control within the operation budget. +Use a controllable remote for those failures and a fake clock for retry timing. This change improves progress within a running worker. Restart durability follows the separate journal plan. An indefinitely unavailable remote can still leave a save pending; the immediate @@ -167,14 +170,30 @@ retries rather than adding a controller-only timeout that could give a false ter Continued writes caused the opposite problem: only successful pushes advance `lastPushAt`, so once the cooldown had elapsed every later commit attempted the same unavailable remote. The retry -deadline now governs those commits too ([push-cooldown design](push-cooldown.md#7-options), +deadline now governs those commits ([push-cooldown design](push-cooldown.md#7-options), option C). The retry follows [parent recovery](../../internal/git/parent_recovery.go), which already kept an obligation open and scheduled its next attempt (10 seconds, backing off to five minutes, shared -across targets on the worker). A failure caused by a missing parent stays on that schedule, so -the two paths cannot multiply remote attempts. A publication that keeps failing is still visible -only in logs and `gitopsreverser_git_pushes_total{outcome="failed"}`, not in `GitTarget` status. +across targets on the worker). While the parent is missing, that schedule suppresses early +attempts. After the parent returns, the recovery obligation stays open until the work publishes, +but its missing-parent hold is removed. A publication that fails while the obligation is open +therefore defers to recovery's next probe deadline: new commits wait for it, and recovery's timer +makes the attempt. Review of #412 found the handoff gap (each new commit retried at once); +`TestPublicationRetry_AFailureAfterTheParentReturnsWaitsForRecovery` pins the fix. + +The backoff schedules publication attempts, not every Git connection. A new window can still +fetch to rebuild retained writes before reaching `maybeSchedulePush`; if the rebuild fails, the +window is dropped and its attached save fails. Except for a missing parent, no dropped-scope +recovery requests a replacement snapshot. This existing gap belongs beside the deadline work, +before the durable-journal refactor. + +There is no worker publication-failure condition or retry deadline in `GitTarget` status. A +push-specific refusal, such as branch protection with working read access, can leave a save in +`WaitingForPush` without explaining the cause. Push-cycle failures appear in logs and +`gitopsreverser_git_pushes_total{outcome="failed"}`. A rebuild that fails before entering the push +cycle is not counted by that metric. Provider connectivity or credential failures can separately +surface through `GitProviderReady`, and parent recovery already has status. ### Slow Git operations stop the whole branch loop @@ -384,9 +403,10 @@ The current worker has five timer sources: | Refusal action | Recheck consent before an eligible empty commit | | Parent recovery | Probe and service retained work or owed snapshots | -The publication retry (#412) shares the push timer with the success cooldown, and keeps its own -backoff, so the branch has one policy for when it may spend its next remote attempt. A durable -design must persist that retry deadline like any other. +The publication retry (#412) shares the push timer with the success cooldown and keeps its own +backoff. Parent recovery has a separate timer, and owns the retry while its obligation is open. +A durable design must persist both deadlines and that ownership rule. This scheduling does not +limit the fetches other handlers can initiate. An illustrative deadline protocol is: @@ -482,6 +502,8 @@ Journal crashes, retention, encryption, and HA failure tests belong to the | Save's first push fails, then the branch goes quiet | Retry publishes and resolves the same request without another edit | | Held save outlives the controller's safety window | Withdrawal cannot falsely fail it; recovery remains scheduled | | Push fails under continuous arrivals | Arrivals cannot bypass the retry budget | +| Parent returns, but publication still fails | New commits respect the recovery deadline while the obligation remains open | +| Retained-write rebuild fails before a new window commits | Recovery accounts for the dropped window and its attached save | | Ref listing, fetch, or push stalls | Operation and cycle deadlines return control to the worker | | Worker is canceled during a failed attempt | No retry is rearmed and no abandoned task retains checkout access | | Timer moves or fires twice | Stale and duplicate firings do not close another window | diff --git a/docs/design/gittarget-state-of-affairs.md b/docs/design/gittarget-state-of-affairs.md index b2e186f1..71a764c4 100644 --- a/docs/design/gittarget-state-of-affairs.md +++ b/docs/design/gittarget-state-of-affairs.md @@ -1,12 +1,12 @@ # GitTarget and the branch worker: state of affairs -> **Snapshot**, taken 2026-10-02 at `230d602a` (#411 on `main`), and updated for #412, which fixes -> gaps 1 and 2 below, closes the 0.51 docs gaps, and lands the rewritten -> [`branch-worker-event-model.md`](branch-worker-event-model.md) and HA plan. It describes +> **Snapshot**, taken 2026-10-02 at `230d602a` (#411 on `main`), and reviewed against #412 at +> `3708b529`. That branch fixes gap 1 and most of gap 2 below, closes the 0.51 docs gaps, and adds +> the rewritten [`branch-worker-event-model.md`](branch-worker-event-model.md) and HA plan. It describes > what is built, which page owns which decision, which edge cases are known not to work, and a > recommended order for what comes next. It decides nothing by itself; each linked page stays the -> owner of its own contract. Every claim about behavior below was checked against the code or a -> named test on that commit. +> owner of its own contract. Current-behavior claims below refer to the reviewed branch; fixed +> gaps describe the earlier behavior explicitly. ## Where the release stands @@ -20,12 +20,13 @@ carries four merged changes since `0.50.0`: | #407 | `GitTarget.spec.parentBranch`; a target on standby creates its write branch from the parent's current tip, checked at publication; the hardening pass | no (additive) | | #411 | A `CommitRequest` on a suspended target, or one whose render fidelity is not established, fails instead of reporting `Ready=True` with nothing written | no, but user-visible | -The release PR body does not list #411 yet; release-please adds it on its next run. +The release PR body includes #411. #412 remains a separate open fix PR at this snapshot. ## What is built | Capability | Code | Pinned by | |---|---|---| +| Failed publication retries without new writes, with 10s to 5m backoff; while parent recovery is open, its deadline paces the retry | [`publication_retry.go`](../../internal/git/publication_retry.go) | `TestPublicationRetry_*` (6) | | One commit window per branch worker, with idle and maximum-duration timers; a save's timers replace the target's for the window it attaches to | [`open_window.go`](../../internal/git/open_window.go), [`commit_request_attach_loop.go`](../../internal/git/commit_request_attach_loop.go) | `TestCommitEmpty_*`, `TestAttach_*` | | `whenNothingToCommit: CommitEmpty` records a save's message in an empty commit; outcomes `Committed` and `AlreadyPresent` are decided by the push | [`branch_worker.go`](../../internal/git/branch_worker.go) (`resolvePushedCommitRequests`) | `TestCommitEmpty_*` | | The controller never fails a save the worker holds; past its safety window it only asks the worker to withdraw it | [`commitrequest_controller.go`](../../internal/controller/commitrequest_controller.go) | `TestParentRecovery_AHeldCommitRequestIsCommittedAfterALongRecovery` | @@ -35,7 +36,7 @@ The release PR body does not list #411 yet; release-please adds it on its next r | An empty repository that gains a ref is never orphaned; a dangling `HEAD` is `Missing` and only a repository with no refs is `Unborn` | [`git_atomic_push.go`](../../internal/git/git_atomic_push.go), [`git_smart_fetch.go`](../../internal/git/git_smart_fetch.go) | `TestBranchWorker_AnEmptyRepositoryThatGains*`, `TestBranchWorker_ADanglingRemoteHeadIsMissingNotUnborn` | | Parent recovery is an obligation, not an observation: retained writes plus per-`(target, collection)` dropped scopes, one probe per worker (10s doubling to 5m) | [`parent_recovery.go`](../../internal/git/parent_recovery.go), [`gittarget_parent_recovery.go`](../../internal/controller/gittarget_parent_recovery.go) | `TestParentRecovery_*` (8), the missing-parent e2e context | | Cost gate: connections per operation, including the four new standby and probe rows | [`git-roundtrip-ledger.golden`](../../internal/git/testdata/git-roundtrip-ledger.golden) rows 13 to 16 | `TestGitRoundTripLedger` | -| Idle refresh: reads one advertisement, never writes, skips a branch mid-cycle, shares the probe deadline during recovery | [`refresh.go`](../../internal/git/refresh.go) | `TestRefresh_*` (25) | +| Idle refresh observes without writing content and skips a branch mid-cycle; during parent recovery it can service a due probe and publish retained writes | [`refresh.go`](../../internal/git/refresh.go) | `TestRefresh_*` (25) | | `status.remote.parent` (`Found`, `Missing`, `Unborn`) while the write branch is absent; `ParentBranchNotFound` (stalled) and `RecoveringParentBranch` (reconciling) | [`gittarget_types.go`](../../api/v1alpha3/gittarget_types.go), [`gittarget_controller.go`](../../internal/controller/gittarget_controller.go) | `internal/controller/gittarget_parent_branch_test.go` | | Red status, first slice: refusal messages republish when the offender changes; a scoped refusal clears only on the same scope's success; "Why a `GitTarget` is not committing" in [`configuration.md`](../configuration.md#why-a-gittarget-is-not-committing) | [`git_path_acceptance.go`](../../internal/watch/git_path_acceptance.go) | `TestGitTargetKstatusContract`, the unsupported-folder e2e | @@ -51,27 +52,28 @@ The release PR body does not list #411 yet; release-please adds it on its next r | [`gittarget-configuration-freshness.md`](gittarget-configuration-freshness.md) | deferred | Whether a target's running watch plan matches its configuration | Deferred with explicit pick-up triggers; neither trigger has fired. | | [`gittarget-api-wave.md`](gittarget-api-wave.md) | effectively done | The `GitTarget` field boundaries | Every step shipped, was dropped, or was declined. A candidate for `docs/finished/`. | | [`branch-worker-event-model.md`](branch-worker-event-model.md) | proposal; retry built in #412 | Worker execution semantics: the FIFO inputs, deadlines, saves, the transition boundary, and the operation-deadline fix still to do | Leads with the liveness fix, names both unbounded network calls, and hands the journal to the HA plan. | -| [`push-cooldown.md`](push-cooldown.md) | design | Success cooldown versus failure backoff | §7 option C (failure backoff) is the recommended fix for gap 1 below. | +| [`push-cooldown.md`](push-cooldown.md) | partly built | Success cooldown versus failure backoff | §7 option C is built in #412, including the parent-recovery handoff. Success-cooldown changes still need measurement. | | [`../spec/commitrequest-design.md`](../spec/commitrequest-design.md) | spec | The save lifecycle | Current as of #412. | | [`../future/ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) | future | The durable journal, publication recovery, outage retention, and the persistence and HA rollout | The single owner of the journal since #412. | | [`../TODO.md`](../TODO.md) | backlog | Follow-ups | Its "Before the next release" list is closed by #412. | ## Known gaps, ranked by what a user would hit -Each was checked in the code at `230d602a`. +Fixed behavior is marked separately from gaps that remain at `3708b529`. 1. **Fixed in #412: a failed push on a quiet branch stranded the work, and any save riding it waited forever.** - `pushPending` stops the push timer on failure and arms nothing; its own doc comment says so - ([`branch_worker.go`](../../internal/git/branch_worker.go), `pushPending`). The refresher skips - a branch with retained writes (`refresh.go`, "this branch is mid-cycle"). The controller does - not time out a save the worker holds, and the worker ignores a withdraw for a request it holds - (`handleWithdrawCommitRequest`). So after one transient push failure with no further edits, - the commit stays local and the `CommitRequest` stays `WaitingForPush` until some other write - arrives. Parent recovery is the only failure path that schedules its own retry. + Before #412, `pushPending` stopped its timer and armed nothing. Refresh skipped retained work, + and withdrawal could not fail a held save. A transient failure could therefore strand the + commit and its request until another write arrived. The worker now schedules publication + retries itself; parent recovery retains its own schedule. 2. **Fixed in #412: a down remote under steady traffic cost one failed push per commit.** - `lastPushAt` advances - only on success, so once the cooldown has passed every new commit attempts the push again. + The publication retry now holds new commits until its deadline. Review found one case the + first version missed: after a missing parent returns, recovery stays open but its hold is gone, + so each new commit retried a failing push (three attempts where one was due, on `main` too). + A failure while recovery is open now defers to recovery's deadline, pinned by + `TestPublicationRetry_AFailureAfterTheParentReturnsWaitsForRecovery`. The backoff paces + publication attempts only; fetches a new window or snapshot needs are not on that schedule. 3. **No Git network call has a deadline.** `remote.List` (`listRemoteRefs`, `CheckRepo`) and `repo.Fetch` (`SmartFetchFrom`) take no context; the push uses the worker's context, which has no deadline; nothing in `internal/git` sets a timeout. A stalled server can block the whole @@ -82,10 +84,14 @@ Each was checked in the code at `230d602a`. (`dropFailedWindow`). Only a missing parent records the dropped scope and asks for a snapshot, so under any other failure those writes wait for the next resync. While the rebuild keeps failing, every new window spends a fetch on it. -5. **A publication that keeps failing is not in status.** Since #412 it retries every 5m at most, - with a save riding it held in `WaitingForPush`, but the `GitTarget` shows nothing: only the - logs and `gitopsreverser_git_pushes_total{outcome="failed"}` do. A revoked credential or a - branch-protection refusal therefore looks like a slow save. +5. **Publication failures have no dedicated status.** The normal retry delay starts at 10s and + reaches a 5m cap after repeated failures. A save riding retained work stays in `WaitingForPush`. + The worker exposes no publication-failure condition or retry deadline on `GitTarget`. + Branch-protection refusals with working read access can therefore look like a slow save. + Provider connectivity and credential failures can separately surface through + `GitProviderReady`; parent recovery also has status. Logs and + `gitopsreverser_git_pushes_total{outcome="failed"}` cover push-cycle failures, but a failed + rebuild before the cycle does not increment that push metric. 6. **Accepted work is volatile.** The FIFO (1,000 items) and retained writes are process memory. The watch cursor can advance before a write reaches Git, so a crash in between can skip work on restart. The 8 MiB buffer threshold closes a window early; it does not bound retained memory. @@ -105,35 +111,38 @@ Each was checked in the code at `230d602a`. refresh follows it with one fetch. This is a documented choice, not a defect. Gaps 1 to 5 are liveness and visibility defects in the current worker and need no persistence; -1 and 2 are fixed. Gaps 6 and 7 are the durable-execution problem. Gaps 8 to 12 are separate, -smaller decisions. +1 and 2 are fixed. Gaps 6 and 7 are the durable-execution +problem. Gaps 8 to 12 are separate, smaller decisions. ## Review of `branch-worker-event-model.md` An earlier version of this page reviewed the uncommitted rewrite and found six issues: the missing -user-visible `WaitingForPush` consequence, the understated timeout gap, the buried near-term fix, -a dated status line, the dropped input inventory, and no INDEX entry. The version in #412 addresses -all six, and moves the journal, publication recovery and outage retention into the HA plan, so -each topic now has one owner. Its factual claims were rechecked against the code. +user-visible `WaitingForPush` consequence, the understated timeout gap, the buried near-term fix, a +dated status line, the dropped input inventory, and no INDEX entry. The version in #412 addresses +all six, and moves the journal, publication recovery and outage retention into the HA plan, so each +topic now has one owner. The follow-up review corrected the failure-status claim (gap 5) and found +the retry handoff in gap 2, since fixed. Neither the backoff nor serialization establishes a budget +for every Git connection. ## Recommended order ### Before cutting 0.51.0 Both steps are in #412: the docs gaps in `UPGRADING.md`, `configuration.md`, the CRD descriptions -and the CommitRequest spec, and the publication retry with its tests. What is left is merging it -once its e2e legs are green. +and the CommitRequest spec, and the publication retry with its tests. CI lint, unit tests, and all +six e2e legs passed at `3708b529`; the handoff fix in gap 2 landed after that run. ### Next, after the release 1. **Operation deadlines (gap 3).** Pass the worker context to `List` and `Fetch` and give each remote operation a bounded deadline. Measure first how go-git's HTTP and SSH transports behave today, against a server that stalls. -2. **The transition boundary, not the journal**: extract the event model's explicit transitions - and test them from recorded state, and agree the save contract the HA plan asks for (final-state - convergence, or also preserved save outcomes). Start the journal only after both. -3. **Gaps 4 and 5**: decide what a failed rebuild owes a dropped window, and whether a publication - that keeps failing belongs in `GitTarget` status. +2. **Rebuild recovery and publication visibility (gaps 4 and 5)**: decide what a failed rebuild + owes a dropped window and expose a publication failure with its retry deadline. These improve + the current worker without waiting for a journal or a broad transition refactor. +3. **The transition boundary**: extract the event model's explicit transitions and test them + from recorded state. Agree the save contract the HA plan asks for: final-state convergence, + or preserved save outcomes too. Start the journal only after both decisions. 4. **Cheap, independent items**: open the go-git unborn-`HEAD` issue and PR; parent observation step 1 (availability after the write branch exists, no extra connections); the red-status reason-agreement test. @@ -143,5 +152,5 @@ once its e2e legs are green. ### Leave deferred Configuration freshness, ancestry classification, the empty-repository bootstrap policy, and the -durable journal (the HA plan's phases) all wait on a decision or on evidence that does not exist yet. None -of them is needed to make 0.51.0 better than 0.50.0. +durable journal (the HA plan's phases) all wait on a decision or on evidence that does not exist +yet. None of them is needed to make 0.51.0 better than 0.50.0. diff --git a/docs/design/push-cooldown.md b/docs/design/push-cooldown.md index 930cd36e..1fa87529 100644 --- a/docs/design/push-cooldown.md +++ b/docs/design/push-cooldown.md @@ -2,8 +2,8 @@ > **design**: open. Option C (§7), the failure backoff, is built in > [`publication_retry.go`](../../internal/git/publication_retry.go); the success cooldown is -> unchanged and options D and E remain open. Index: [`../INDEX.md`](../INDEX.md) -> Date: 2026-09-21. +> unchanged. Option D needs measurement; option E remains rejected without that evidence. +> Index: [`../INDEX.md`](../INDEX.md). Reviewed against #412 on 2026-10-02. > Related: [`../api-first-publication.md`](../api-first-publication.md), > [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), > [`../spec/commit-window-refactor.md`](../spec/commit-window-refactor.md) @@ -12,17 +12,16 @@ into one commit, which is most of what the cooldown was invented to do. Is the cooldown still earning its place, and would removing it simplify the worker? -**The short answer.** For a single author writing a single target it is provably inert at the -defaults, so the instinct that it is redundant has a real basis. But that is the *quiet* case. The -moment a second identity touches the branch the commit window stops spacing commits entirely (§3) -and the cooldown becomes the only thing batching. On a multi-user cluster with attribution enabled, -which is what this product is for, that is the normal case rather than the exception. +**The short answer.** For a single author writing a single target, ordinary timer closures at the +defaults already space commits enough to avoid the success cooldown. Identity changes (§3), saves +with shorter timers, and buffer-limit closures can produce commits sooner. The cooldown batches +those commits when they arrive within five seconds of a successful push. -Removing it would also delete about thirty lines and two fields and **none** of the state that makes -this worker complicated, because that state exists for push *failure*, not for push cadence. +Removing the success wait would remove `lastPushAt` and its scheduling branch. The shared push +timer and retained-write recovery would remain because publication retries now use them too. -So the clear win is not removal. It is that the worker has a timer on the path that mostly does not -need one and **no timer on the path that does**. +Option C is built in #412. The remaining decision is whether changing the success cooldown earns +its extra publication cost; that needs the measurements in §9. ## 1. What it is today @@ -30,18 +29,22 @@ need one and **no timer on the path that does**. ```go if len(l.pendingWrites) == 0 { return } +if l.awaitingPublicationRetry() { return } // a failed publication has its own deadline if l.lastPushAt.IsZero() { l.pushPending(); return } // never pushed: go now if time.Since(l.lastPushAt) >= PushCooldown { l.pushPending(); return } if l.pushTimer == nil { l.pushTimer = time.NewTimer(...) } // otherwise wait out the remainder ``` `lastPushAt` advances **only on a successful push**, so the cooldown paces successful publication -and does not pace retries. +and does not pace retries. The publication backoff does that separately. Parent recovery owns +its attempts while active, and new commits wait for its deadline; see the +[worker event model](branch-worker-event-model.md). -## 2. The finding: at the defaults it never batches a single identity +## 2. Ordinary timer closures already space a single identity `DefaultCommitWindow` and `PushCooldown` are both `5s`. The window is a rolling **silence** timer -per `(author, GitTarget)`. That makes the cooldown unreachable for one author writing one target: +per `(author, GitTarget)`. With no save or other early closure, a new window opens only after the +previous synchronous push returns, so its silence deadline already falls outside the cooldown: ```mermaid sequenceDiagram @@ -66,8 +69,9 @@ The next commit for the same identity cannot finalize until at least `window` af one, and the cooldown is measured from the push that followed that previous commit. When `window >= cooldown`, the cooldown has always expired before there is anything to hold. -**So the general rule is:** the cooldown only does work when the commit window is *shorter* than -it, or when several identities share the branch. At the shipped defaults those are equal. +Shorter window timers, early closures, and several identities on a branch can all make the +cooldown engage. Equal default durations alone do not establish that every single-author workload +avoids it. ## 3. Where it does still work, and it is the normal case @@ -111,7 +115,7 @@ which is why that log line exists). | --- | --- | | An author or target change | The identity boundary finalizes the open window on arrival of the next event | | Several `GitTarget`s on one branch | One window, so every switch between them is an identity change | -| `commit.window: 0s` | Every event commits on arrival | +| `commit.window.idleTimeout: 0s` | Every event commits on arrival | | Atomic writes | They bypass the window by contract | | Resync snapshots | They finalize and commit outside the window | @@ -124,7 +128,7 @@ worker behaves. Once the mechanism is right, the multi-identity case is not an e cluster with attribution enabled, the configuration this product is built for, produces one commit per identity change, which is close to one commit per event. -## 4. What removing it would actually simplify +## 4. What removing the success cooldown would simplify This is the part worth being precise about, because the hope ("it would clean up a lot of state") is half right. @@ -133,17 +137,18 @@ is half right. | Item | Size | | --- | --- | -| `lastPushAt`, `pushTimer` fields | 2 fields | -| `maybeSchedulePush` collapses to "push if anything is retained" | ~12 lines to ~4 | -| `stopPushTimer` and its select arm in the event loop | ~14 lines | +| `lastPushAt` field | 1 field | +| Successful-push wait in `maybeSchedulePush` | The elapsed-time check and cooldown scheduling | | `PushCooldown` constant and its documentation | 1 constant, several doc paragraphs | -Roughly **thirty lines and two fields**, plus one fewer timer in a loop that has three. +The failure backoff shares `pushTimer`, so its stop helper and select arm remain. The worker still +has five timer sources: window, push, attach, refusal action, and parent recovery. **Stays exactly as it is:** | Item | Why it is not the cooldown's | | --- | --- | +| `publicationRetry`, `pushTimer`, and `stopPushTimer` | Failed publication still needs a scheduled attempt | | `pendingWrites` retention | A push can fail or be rejected; the writes must survive to be replayed | | `baseTrusted` | The head-of-cycle fetch decision, unrelated to push cadence | | `worktreeDirty` | A write that failed part-way, unrelated to push cadence | @@ -154,11 +159,11 @@ Roughly **thirty lines and two fields**, plus one fewer timer in a loop that has flowchart TD subgraph COOLDOWN["Owned by the cooldown - would be deleted"] LPA["lastPushAt"] - PT["pushTimer + stopPushTimer"] MSP["the wait branch in maybeSchedulePush"] end subgraph FAILURE["Owned by push failure - would remain"] + PT["publicationRetry + pushTimer + stopPushTimer"] PW["pendingWrites retention"] BT["baseTrusted"] WD["worktreeDirty"] @@ -186,30 +191,28 @@ That cuts both ways, and neither direction is decisive: only reached on failure is code whose bugs are found later. The five defects PR #382 fixed were all in paths the cooldown makes routine. -A concrete symptom of how routine it is: **36 of the 45 test references to the cooldown are -`loop.lastPushAt = time.Now()`**, used purely as a lever to hold the push back so a test can inspect -retained state. Remove the cooldown and those tests need a different lever. That is a real -migration cost and also evidence of how central the state is. +Tests use `loop.lastPushAt = time.Now()` to hold the push back so they can inspect retained state. +Removing the cooldown requires another way to reach that state without changing the failure path +each test exercises. -## 6. Backpressure already coalesces, which weakens the storm argument +## 6. Serialization limits concurrency, but does not batch queued commits The branch worker is a single goroutine, and Git operations run synchronously on it. While a push -is in flight, arriving events sit in the queue. When it returns, they are processed, become -commits, and **one** push covers all of them. +is in flight, arriving events sit in the queue. After it returns, each dequeued item runs its +handler, which can finalize a window and call `maybeSchedulePush` before the next item is read. -So "push immediately" is not the same as "push per event". Under sustained load the worker -self-limits to one push per push-duration, and the coalescing the cooldown was invented for arrives -for free from the serialization. The cooldown's distinctive contribution is limited to a **slow -trickle** across identities: arrivals frequent enough to commit often, but slow enough that each -push completes before the next commit appears. +Without the success wait, queued alternating identities or zero-window writes can therefore each +cause a push. Serialization limits concurrent Git work to one operation; it does not automatically +combine queued commits into one publication. The cooldown can batch both a slow trickle and a +backlog that drains after a slow push. ## 7. Options | Option | What it is | Complexity | Verdict | | --- | --- | --- | --- | -| **A. Keep it** | Status quo | Unchanged | Honest default. It is inert where it is inert and useful where it is not | -| **B. Delete it** | Push after every commit; rely on serialization to batch | **-30 lines, -2 fields, -1 timer** | **Riskiest.** §3 means this is one push per identity change on a multi-user branch | -| **C. Add a failure backoff, keep the cooldown** | A bounded backoff after a **failed** push or recovery, cleared when nothing is retained | **+1 timer**, but it guards a real gap | **Recommended, and independent of the rest** | +| **A. Keep the success cooldown** | Preserve successful-push spacing | Unchanged | Current default alongside C | +| **B. Delete the success cooldown** | Push after every commit unless failure backoff applies | Removes `lastPushAt` and the success wait; keeps the timer | §3 can become one push per identity change | +| **C. Add a failure backoff, keep the cooldown** | Schedule failed publications independently of new writes | Reuses `pushTimer`; adds backoff state | Built in #412, including the parent-recovery handoff | | **D. Add the backoff AND shorten or drop the success cooldown** | C, plus reducing the `5s` wait once §9 has priced it | Depends on the outcome | The measured follow-up to C | | **E. Make it conditional or configurable** | Engage only when more than one identity is active, or expose it on `GitProvider` | **+complexity, +API surface** | Rejected unless D measurably fails | @@ -221,15 +224,12 @@ half of that: the cooldown is inert only while one identity is writing, and it i soon as two are. Dropping the success wait is therefore a **measured** decision, not an obvious one, and it belongs in option D behind the numbers §9 asks for. -What survives the correction intact is the second half, and it is worth doing on its own. Today the -worker has a timer on success and **none on failure**. -[`../api-first-publication.md`](../api-first-publication.md) and the PR #382 review both record the -gap: `pushPending` stops its timer on failure and arms no replacement, so a branch that goes quiet -after a failed push retains its work indefinitely and a flat `recovery` counter cannot prove -progress. The mirror-image problem is that because `lastPushAt` advances only on success, an expired -cooldown does not space failed attempts, so continued arrivals against a down remote provoke a -failed push per commit. One bounded backoff answers both, and it neither needs nor blocks any -decision about the success cooldown: +Before #412, `pushPending` stopped its timer on failure and armed no replacement outside parent +recovery. A quiet branch retained its work indefinitely, while continued arrivals could provoke +a failed push per commit because `lastPushAt` advances only on success. #412 adds the independent +failure backoff. Its ordinary publication path follows this lifecycle; while parent recovery is +open, recovery's deadline takes the backoff's place. The worker event model records the fetches +outside that schedule: ```mermaid stateDiagram-v2 @@ -260,12 +260,12 @@ Ordered by how much they should worry you. 2. **Hosted Git rate limits.** With serialization the bound is one push per push-duration, which on a fast remote is several per second sustained. GitHub's secondary rate limits are real and are not documented as a fixed number, so this needs observation rather than arithmetic. -3. **`commit.window: 0s` becomes a push per event.** Today the cooldown is the only thing standing - between that setting and one publication per watch event. Anyone who set `0s` for prompt commits - did not necessarily ask for prompt *pushes*. +3. **`commit.window.idleTimeout: 0s` becomes a push per event.** On a healthy remote the cooldown + spaces publications even with that setting. Anyone who set `0s` for prompt commits did not + necessarily ask for prompt *pushes*. 4. **Losing a well-exercised path.** §5. The retained-write machinery stays; its production exposure shrinks; its bug-discovery rate shrinks with it. -5. **Test migration.** 36 tests use `lastPushAt` to reach retained state. They need a replacement +5. **Test migration.** Tests use `lastPushAt` to reach retained state. They need a replacement lever, and a careless one (for example, stubbing `pushAtomicFn` to fail) changes what is being tested. 6. **A latency expectation somebody may already rely on.** Removing the cooldown makes publication @@ -278,33 +278,28 @@ The ledger built in PR #382 answers most of this, and turns the argument into nu | # | Operation to add to the ledger | What it settles | | --- | --- | --- | -| 1 | One author, one target, a burst at default window, driven through the **event loop** | Confirms §2: the cooldown never engages, so B and C cost nothing here | +| 1 | One author, one target, a burst at default window, with no early closure | Confirms §2 for ordinary timer closures | | 2 | Two authors alternating on one branch | Prices the identity-boundary case that §3 claims is the real one | | 3 | Three `GitTarget`s on one branch, edited together | The multi-tenant bill for option B | -| 4 | `commit.window: 0s`, ten events | Risk 3, as a number | -| 5 | A failing remote with continued arrivals | Shows today's per-commit failed push, and what a backoff changes | +| 4 | `commit.window.idleTimeout: 0s`, ten events | Risk 3, as a number | +| 5 | A failing remote with continued arrivals, including parent recovery | Confirms retry pacing, including across the parent-recovery handoff | -Rows 1 to 4 need an event-loop-driven fixture rather than today's direct `commit()` calls, because -what is under test is the *scheduling*, not the request count of one cycle. That fixture does not -exist yet and is most of the work in answering this question. +Rows 1 to 4 need connection counts driven through the event loop because scheduling determines +how many publication cycles occur. Extend the existing event-loop fixtures, including #412's +held-request retry test, to measure those workloads. ## 10. Recommendation -1. **Do not change it as part of PR #382.** That branch is green and reviewed; this alters - publication cadence and deserves its own change and its own e2e run. -2. **Take option C on its own.** Add a bounded failure backoff. It closes a documented gap with no - upside, it needs no measurement to justify, and it does not depend on any decision about the - success cooldown. -3. **Build the event-loop ledger rows in §9 next.** Nothing currently measures scheduling, only the - cost of one cycle, so the multi-identity question cannot be answered today. Row 2 is the - important one: it prices the case §3 says is normal. +1. **Keep the success cooldown while evaluating its cost.** Changing publication cadence needs + a separate change and its own validation. +2. **Option C is complete.** #412 built the failure backoff and, after review, the + parent-recovery handoff: new commits respect retry pacing after a missing parent returns. +3. **Build the event-loop ledger rows in §9 before changing the success wait.** Retry tests prove + progress, but the ledger still needs the cost of these scheduled workloads. Row 2 prices the + multi-identity case. 4. **Only then consider option D.** If the numbers show the multi-identity case is rare in practice or the amplification is small, shortening or dropping the success cooldown becomes defensible. If they show what §3 predicts, keep it and the question is settled with evidence. -The honest summary, after §3's correction: the cooldown is redundant only while one identity is -writing, and load-bearing as soon as two are. It is also not the source of this worker's -complexity, so removing it was never the simplification it looked like: the saving is thirty lines -against the retained-write machinery that stays either way. The unambiguous finding is -narrower and more useful: **the worker times the wrong event.** Adding the missing failure timer is -worth doing whatever happens to the cooldown. +Removing the success wait keeps the retry timer and retained-write machinery. Its benefit must +therefore come from measured save latency, weighed against the additional publications. diff --git a/docs/future/ha-gittarget-distribution-plan.md b/docs/future/ha-gittarget-distribution-plan.md index 9de470d9..f5d23045 100644 --- a/docs/future/ha-gittarget-distribution-plan.md +++ b/docs/future/ha-gittarget-distribution-plan.md @@ -32,6 +32,11 @@ an indeterminate publication result. Existing coalescing still applies, and rebu work onto a moved remote can change its SHA or leave no diff. This is not a promise of one commit per Kubernetes mutation or exactly-once publication under arbitrary history loss. +The phases and acceptance criteria below assume the stronger save contract. Agree on that scope +before HA-1. A convergence-only first release needs narrower phases and explicit save limitations; +it cannot claim the save-recovery criteria in this plan. Recovering pending work and terminal +receipts does not require retaining every historical transition forever. + A Git outage is recoverable within the configured storage budget if the remote eventually returns. The journal must survive the advertised storage failures, and Kubernetes must become available for source recovery and status projection. A single Redis or Valkey Pod is insufficient @@ -116,12 +121,20 @@ multiple GitProvider objects can name the same repository and branch. The canonical remote identity should normalize the resolved Git URL and be hashed for key and Lease names. It must be exposed in logs, metrics, and target status. +URL normalization alone cannot prove that SSH and HTTPS URLs, redirects, or host aliases name the +same repository. HA-0 must define the supported identity and alias rules and document any aliases +it cannot unify. Strip credential material before deriving or exposing the identity. + Every GitTarget maps to exactly one branch write shard. Targets sharing a remote branch use one journal and one worker, even when they have different paths. Overlapping paths must remain a reconciliation-time and writer-time refusal. Retain provider and target incarnations alongside the shard key; sharing a destination must not silently combine incompatible policy or reroute old work to a replacement object with the same name. +Before combining providers, define credential selection and compatibility for branch permissions, +parent choice, and signing policy. Refuse incompatible bindings explicitly. A shared destination +does not authorize one provider's work to use another provider's permissions. + ### Journal record Give every record a payload schema version, branch identity, stable record identity, and branch diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index 60f090eb..337651af 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -166,7 +166,9 @@ The gates are read when the commit is made, as `suspend` already was for every w made before a gate closed is pushed rather than kept back, because a commit that never left the operator's checkout would surface later, out of order. -A failed push schedules its own retry (10s, doubling to 5m), so a request riding the retained write -stays `WaitingForPush` only until a retry succeeds, with or without further commits on the branch. +A failed push schedules its own retry (10s, doubling to 5m), with parent recovery owning attempts +while its obligation remains active. A request riding the retained write stays `WaitingForPush` +until publication resolves it or worker shutdown fails it. An unavailable remote can leave it +pending indefinitely, but retries no longer depend on another commit arriving. The complete status vocabulary is in the [status conditions guide](status-conditions-guide.md).