Skip to content

fix(git): retry a failed publication on its own schedule, and close the 0.51 docs gaps - #412

Merged
sunib merged 6 commits into
mainfrom
fix/publication-retry-backoff
Oct 2, 2026
Merged

sunib merged 6 commits into
mainfrom
fix/publication-retry-backoff

Conversation

@sunib

@sunib sunib commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

A failed publication retries by itself

After a failed push, or a failed rebuild before it, the branch worker stopped its push timer and armed nothing. Two consequences:

  • A quiet branch kept its work indefinitely. The refresher skips a branch that holds retained writes, and the controller never fails a CommitRequest the worker holds. So a save riding that write stayed WaitingForPush until some unrelated write arrived.
  • A busy branch tried the push on every commit. lastPushAt advances only on success, so once the cooldown had passed, each new commit tried the remote that had just refused.

Now the failure schedules the next attempt on the existing push timer: 10s, doubling to at most 5m (internal/git/publication_retry.go). Commits made before that deadline accumulate locally, and a successful push resets the schedule. A failure caused by a missing parent branch stays with parent recovery's probe schedule, so the two never both spend a connection. This is option C in docs/design/push-cooldown.md; the success cooldown is unchanged.

Tests (internal/git/publication_retry_test.go, real git http-backend server):

  • a quiet branch publishes after a failed push, with no further write;
  • new commits do not push before the retry is due, and a second failure doubles the wait;
  • the backoff doubles to its cap and resets on success;
  • a missing parent is left to parent recovery;
  • on the real event loop, a held CommitRequest resolves Committed with no further write.

With the retry turned into a no-op, the four behavior tests fail; the held-request test times out at 10s. They pass under -race.

Not addressed, and recorded in docs/TODO.md: when a rebuild's fetch fails, the open window is dropped. Unless the cause was a missing parent, nothing asks for a snapshot, so those writes wait for the next resync.

The 0.51 docs gaps

  • docs/UPGRADING.md, commit-window entry:
    • says to suspend a Flux or Argo CD source before removing commit.window;
    • adds the quickstart.gitTarget.commit.window chart value to the Was/Is table;
    • corrects the closeDelay claim: a client without strict field validation has the field pruned silently.
  • A new UPGRADING entry: a save on a suspended or render-gated target fails (fix(commit): fail a CommitRequest on a target that may not be written #411).
  • Stale "closes the window now" wording is fixed in the CommitRequest and GitTarget commit descriptions, so the CRD descriptions change, but no schema. Also fixed in configuration.md (including the generated settings row, via hack/crdfields/fields.yaml) and docs/spec/commitrequest-design.md.

Design records

  • docs/design/branch-worker-event-model.md and docs/future/ha-gittarget-distribution-plan.md: the branch worker's recovery boundary, durable admission and publication evidence. Proposals only; no behavior change.
  • docs/design/gittarget-state-of-affairs.md: what is built for GitTarget and the branch worker, which page owns which decision, the known gaps ranked by user impact, and the recommended order.
  • docs/INDEX.md lists them.

Review fixes

  • A failure after a missing parent returns no longer lets new commits retry at once. Recovery stays open until the work publishes, but its missing-parent hold is gone once the parent is found. Before this fix, every new commit retried the failing push: the review's reproduction saw three attempts where one was due, and it failed on main too. Such a failure now defers to recovery's next probe deadline. Pinned by TestPublicationRetry_AFailureAfterTheParentReturnsWaitsForRecovery, which fails without the change.
  • Docs corrected from the review:
    • UPGRADING names the reconciler to pause: the Flux Kustomization or HelmRelease, or Argo CD automated sync.
    • The push-cooldown analysis drops obsolete claims (the retry 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.

Validation

task lint and task test pass on the code (unit coverage 79.5%, at baseline). task lint-docs and the hack/crdfields check pass on the combined branch. Local e2e was not run; this relies on the CI e2e legs.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Failed Git publications now retry automatically, even when no new changes arrive. Retries start after 10 seconds and increase up to 5 minutes; new changes wait until the retry is due.
    • Missing-parent failures continue through parent recovery rather than publication retries.
  • Documentation

    • Clarified that commit requests attach to the author’s current or next commit window and use request-specific timers.
    • Documented the default commit window: 5 seconds of silence, capped at 1 minute.
    • Added upgrade guidance for changed commit-window settings and requests targeting suspended or not-yet-ready targets.

sunib and others added 3 commits October 2, 2026 07:48
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…get 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) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: da5be3dd-e7e3-4e8f-81e4-58cafbb7d2e4

📥 Commits

Reviewing files that changed from the base of the PR and between 230d602 and 3708b52.

📒 Files selected for processing (18)
  • api/v1alpha3/commitrequest_types.go
  • api/v1alpha3/gittarget_types.go
  • config/crd/bases/configbutler.ai_commitrequests.yaml
  • config/crd/bases/configbutler.ai_gittargets.yaml
  • docs/INDEX.md
  • docs/TODO.md
  • docs/UPGRADING.md
  • docs/api-first-publication.md
  • docs/configuration.md
  • docs/design/branch-worker-event-model.md
  • docs/design/gittarget-state-of-affairs.md
  • docs/design/push-cooldown.md
  • docs/future/ha-gittarget-distribution-plan.md
  • docs/spec/commitrequest-design.md
  • hack/crdfields/fields.yaml
  • internal/git/branch_worker.go
  • internal/git/publication_retry.go
  • internal/git/publication_retry_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The branch worker now schedules bounded retries after failed publication, including when no new writes arrive. The pull request updates CommitRequest and commit-window documentation and adds proposals for recoverable worker execution and GitTarget durability.

Changes

Branch publication and CommitRequest behavior

Layer / File(s) Summary
Commit-window descriptions and migration guidance
api/v1alpha3/commitrequest_types.go, api/v1alpha3/gittarget_types.go, config/crd/bases/configbutler.ai_commitrequests.yaml, config/crd/bases/configbutler.ai_gittargets.yaml, docs/configuration.md, docs/UPGRADING.md, hack/crdfields/fields.yaml
API descriptions now say CommitRequests attach to an author’s current or next window and use request-specific timers. The docs describe the 5-second silence window, 1-minute maximum, and migration changes.
Retained-write publication retries
internal/git/publication_retry.go, internal/git/branch_worker.go, internal/git/publication_retry_test.go, docs/api-first-publication.md, docs/design/push-cooldown.md, docs/spec/commitrequest-design.md
Failed publication schedules retries after 10 seconds, doubling up to 5 minutes. New writes wait until the retry deadline, missing-parent failures remain under parent recovery, and successful publication clears retry state. Tests cover retry timing and held CommitRequests.
Branch-worker event model and implementation status
docs/design/branch-worker-event-model.md, docs/design/gittarget-state-of-affairs.md, docs/INDEX.md, docs/TODO.md
The event-model proposal describes current worker behavior, proposed recovery contracts, and validation scenarios. The status snapshot, index, and backlog record completed retry work and remaining issues.
Durable journal and HA plan
docs/future/ha-gittarget-distribution-plan.md, docs/INDEX.md
The plan specifies proposed durable save and workflow records, publication recovery evidence, outage capacity requirements, and staged HA implementation and validation. The index describes the plan’s scope.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant branchWorkerEventLoop
  participant publicationRetry
  participant GitRemote
  participant CommitRequest
  branchWorkerEventLoop->>GitRemote: Push retained writes
  GitRemote-->>branchWorkerEventLoop: Return publication failure
  branchWorkerEventLoop->>publicationRetry: Record failure and arm retry deadline
  publicationRetry-->>branchWorkerEventLoop: Retry deadline expires
  branchWorkerEventLoop->>GitRemote: Retry retained writes
  GitRemote-->>branchWorkerEventLoop: Return successful publication
  branchWorkerEventLoop-->>CommitRequest: Report committed hash
Loading

Merge Risk: ⚪ Minimal · up to 3708b

Failed Git publications now retry on a bounded schedule, and the documentation is updated to match. No concrete merge-blocking issue was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3708b

Automatic retries improve recovery while retaining the existing destination and save-result semantics. No introduced security issue was established. Permission changes during a pending retry remain insufficiently verified, and retries do not provide recovery across process restarts.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The assessed publication scope is one worker's repository and branch, shared by its sibling targets. A retry can publish their accumulated retained commits together; it is not isolated to a single save request or target.

Trust Boundaries and Controls

  • observed — Each publication cycle rereads provider credentials. The provider UID and URL must match the worker's repository identity, preventing retained work from being published with credentials belonging to a replacement provider destination.
  • observed — Suspension or render-gate closure after a local commit is retained does not retract that commit. The focused test explicitly expects retained publication to continue. This continuation rule is distinct from admission checks on new writes.

Resilience and Maintainability Implications

  • observed — The backoff limits retry frequency, not total operation duration or outage retention. The design documentation separately identifies stalled-operation budgets and unbounded retained memory as remaining containment limitations.

Hardening Proposals

  • proposed — Specify and test whether narrowing allowedBranches or changing credentials cancels retained publication or only affects future admission. If cancellation is required, define an explicit enforcement point before delayed retries reach the remote.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. (13 skipped: 13…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the primary functional change, failed publication retries, and the related documentation updates.
Description check ✅ Passed The description provides a detailed change summary, testing results, known limitations, documentation updates, and review fixes. It does not follow the template headings and omits explicit Type of Cha…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

❤️ Share

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

…ch 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) <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/git/publication_retry.go 87.5% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

sunib and others added 2 commits October 2, 2026 08:49
…er 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@sunib
sunib merged commit 0daa371 into main Oct 2, 2026
23 checks passed
@sunib
sunib deleted the fix/publication-retry-backoff branch October 2, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant