diff --git a/.github/RELEASES.md b/.github/RELEASES.md index a9c043dd..45bb9281 100644 --- a/.github/RELEASES.md +++ b/.github/RELEASES.md @@ -259,6 +259,34 @@ Publishing it instead freezes a release whose own notes link to assets it does n `last-release-sha` does not rescue this: release-please reads it only as a top-level key, and this repo's copy sits under `packages["."]`, where it is ignored. +### A merged PR is missing from the changelog + +**Symptom:** a `feat` or `fix` merged since the last release has no changelog line, and none +of its breaking changes appear. The Release Please job log says nothing at its default level. + +**Cause:** release-please parses each commit with `@conventional-commits/parser` and, when the +parse throws, drops the whole commit with only a debug log. A body line that starts with a word +followed by nested parentheses, such as `` `^([0-9]+(\.[0-9]+)?`` or `f(a(b))`, reads as an +unclosed `type(scope` token and throws. The squash body carries every commit message on the +branch, so one such line in any commit is enough. This dropped #388 from 0.50.0. + +**Prevention:** the "Squash message parses" check in `pr-title.yml` rebuilds the squash message +and parses it before merge. Fix a failure by indenting the named line, or starting it with other +text, in the commit that holds it. To check a branch locally: + +```bash +(cd hack/release-notes && npm ci --ignore-scripts) +hack/release-notes/squash-message.sh origin/main HEAD "" | node hack/release-notes/check.mjs +``` + +`squash-message.sh` builds the message the way GitHub does for this repository: a single-commit PR +squashes to that commit's own message, and the PR title is not used; several commits squash to the +PR title over the bulleted commit messages. `hack/release-notes/test.sh` covers both shapes. + +**Repair after a release:** add the missing entries to that release's section of `CHANGELOG.md` +by hand (release-please only prepends, so it never rewrites them) and edit the GitHub Release +notes to match. + ### Docker Build Failed **Check:** diff --git a/.github/workflows/pr-title.yml b/.github/workflows/pr-title.yml index f77f04d6..90b36f84 100644 --- a/.github/workflows/pr-title.yml +++ b/.github/workflows/pr-title.yml @@ -1,3 +1,5 @@ +# Two checks on what a pull request will become once squash-merged. The first is the title. +# # PR titles become the squash-merge commit subject (the repository squash-merges with # COMMIT_OR_PR_TITLE), and release-please reads those subjects to decide the version bump and # to write the changelog. A title that is not a Conventional Commit is therefore not a style @@ -47,3 +49,42 @@ jobs: echo '`fix(webhook)!: reject unsigned requests`. Edit the title and this check re-runs.' } | tee -a "$GITHUB_STEP_SUMMARY" exit 1 + + # The squash commit's BODY matters as much as its title. The repository squash-merges with + # COMMIT_MESSAGES, so the body is every commit message on the branch (or, for a single commit, + # that commit's whole message, title included), and release-please parses the whole of it. A body the parser cannot read drops the commit from the changelog and the + # version bump without a word; that is how #388 went missing from 0.50.0. This rebuilds the + # message GitHub will write and parses it with the parser release-please uses. + # + # Commit messages are untrusted text: they reach the parser through a file, never through an + # expression in the script. + squash-message: + name: Squash message parses + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Check out the branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + persist-credentials: false + + - name: Install the parser + working-directory: hack/release-notes + run: npm ci --ignore-scripts --no-audit --no-fund + + - name: Self-test the check + run: hack/release-notes/test.sh + + - name: Parse the squash message + env: + PR_TITLE: ${{ github.event.pull_request.title }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + # squash-message.sh owns GitHub's rule: one commit keeps its own message, several get + # the PR title over the bulleted commit messages. + hack/release-notes/squash-message.sh "$BASE_SHA" "$HEAD_SHA" "$PR_TITLE" > "$RUNNER_TEMP/squash-message.txt" + node hack/release-notes/check.mjs < "$RUNNER_TEMP/squash-message.txt" | tee -a "$GITHUB_STEP_SUMMARY" + exit "${PIPESTATUS[0]}" diff --git a/.gitignore b/.gitignore index 7727d879..c7dd73d1 100644 --- a/.gitignore +++ b/.gitignore @@ -62,3 +62,4 @@ loadtest .agents/* /external-sources/ /docs/local-only/* +hack/release-notes/node_modules diff --git a/CHANGELOG.md b/CHANGELOG.md index 75719d36..30dd44e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * expire finished CommitRequests and name the placement fallback the built-in path ([#400](https://github.com/ConfigButler/gitops-reverser/issues/400)) * **api:** rules[].operations is removed. A rule that listed a subset now also mirrors updates and removals; set spec.prune.mode: Never on the GitTarget before upgrading to keep documents on removal. See UPGRADING.md. * **api:** kubectl get -o wide no longer prints PROVIDERREADY, CLUSTERPROVIDERREADY (GitTarget) or GITTARGETREADY (WatchRule, ClusterWatchRule). +* `CommitRequest.spec.closeDelaySeconds` is renamed to `spec.closeDelay`, a Go duration string (`"2s"`, at most `"5m"`). The old field is pruned on write, not refused. See UPGRADING.md. ([#388](https://github.com/ConfigButler/gitops-reverser/issues/388)) +* `GitTarget.spec.commit.window` is validated at admission as a Go duration string of at most `"24h"`; unit-less or out-of-range values are refused. ([#388](https://github.com/ConfigButler/gitops-reverser/issues/388)) +* the `--base-trust-max-age` flag is removed; a Deployment that sets it by hand fails to start. The periodic Git refresh (`--git-refresh-interval`, default `10m`) replaces it. ([#388](https://github.com/ConfigButler/gitops-reverser/issues/388)) ### Features @@ -22,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * expire finished CommitRequests and name the placement fallback the built-in path ([#400](https://github.com/ConfigButler/gitops-reverser/issues/400)) ([cd0c94c](https://github.com/ConfigButler/gitops-reverser/commit/cd0c94c823721888642d0227ac011f0b3aedf2b5)) * key a branch worker to its repository, and give a repository its own status surface ([#396](https://github.com/ConfigButler/gitops-reverser/issues/396)) ([71eb515](https://github.com/ConfigButler/gitops-reverser/commit/71eb515273590f4d75d1118a7eb881fece673a0d)) * report a folder somebody else broke, instead of discovering it on the next write ([#391](https://github.com/ConfigButler/gitops-reverser/issues/391)) ([ae4c823](https://github.com/ConfigButler/gitops-reverser/commit/ae4c82391ce67c6ade1806fb1aaf4fdb08d1ca6c)) +* refresh an idle target from Git, and make every API duration a Go duration string ([#388](https://github.com/ConfigButler/gitops-reverser/issues/388)) ([a087901](https://github.com/ConfigButler/gitops-reverser/commit/a0879014c8a036fe4a7843e0cee636496c45001d)) ## [0.49.1](https://github.com/ConfigButler/gitops-reverser/compare/v0.49.0...v0.49.1) (2026-09-22) diff --git a/api/v1alpha3/commitrequest_types.go b/api/v1alpha3/commitrequest_types.go index d5c83592..7323dab2 100644 --- a/api/v1alpha3/commitrequest_types.go +++ b/api/v1alpha3/commitrequest_types.go @@ -49,10 +49,12 @@ type CommitRequestSpec struct { // The pattern, the CEL bound and the wider-than-Flux unit set are the shape every duration in // this API takes, and GitTargetCommitSpec.Window carries the reasoning for all three. - // CloseDelay sets the finalize deadline from the worker's first receipt, as a Go duration - // string ("2s", "750ms", "1m"). Time waiting for a matching window consumes this delay; - // repeated receipt keeps the deadline. Normal flush triggers can close an attached window - // early, carrying its message. A request claims at most one open window and cannot rename a + // CloseDelay is how long the request waits for a matching window from the worker's first + // receipt, and how long a window it claims keeps collecting from the claim, as a Go duration + // string ("2s", "750ms", "1m"). Time spent waiting does not shorten the collection, so a + // request created before its writes still gets the whole delay after them; repeated receipt + // keeps the waiting deadline. Normal flush triggers can close an attached window early, + // carrying its message. A request claims at most one open window and cannot rename a // finalized commit. At most "5m". // Defaults to "2s", which covers the time a write spends waiting for its audit fact before // the commit window opens; an explicit "0s" requests immediate finalization and will usually diff --git a/config/crd/bases/configbutler.ai_commitrequests.yaml b/config/crd/bases/configbutler.ai_commitrequests.yaml index b7d7f80b..92638800 100644 --- a/config/crd/bases/configbutler.ai_commitrequests.yaml +++ b/config/crd/bases/configbutler.ai_commitrequests.yaml @@ -77,10 +77,12 @@ spec: closeDelay: default: 2s description: |- - CloseDelay sets the finalize deadline from the worker's first receipt, as a Go duration - string ("2s", "750ms", "1m"). Time waiting for a matching window consumes this delay; - repeated receipt keeps the deadline. Normal flush triggers can close an attached window - early, carrying its message. A request claims at most one open window and cannot rename a + CloseDelay is how long the request waits for a matching window from the worker's first + receipt, and how long a window it claims keeps collecting from the claim, as a Go duration + string ("2s", "750ms", "1m"). Time spent waiting does not shorten the collection, so a + request created before its writes still gets the whole delay after them; repeated receipt + keeps the waiting deadline. Normal flush triggers can close an attached window early, + carrying its message. A request claims at most one open window and cannot rename a finalized commit. At most "5m". Defaults to "2s", which covers the time a write spends waiting for its audit fact before the commit window opens; an explicit "0s" requests immediate finalization and will usually diff --git a/docs/INDEX.md b/docs/INDEX.md index e1f079bc..bb2a474c 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -124,7 +124,7 @@ 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 — 19 pages +### Open — 23 pages | Doc | Open question | |---|---| @@ -139,6 +139,7 @@ so you never have to read a proposal to find out it already shipped. | [`label-selection-follow-ups.md`](design/label-selection-follow-ups.md) | **design, open.** What the label-selection review rounds left open, with the evidence for each: real deletions in a selected collection wait the full 3s attribution grace (the watch cannot tell them from a label exit, measured), the resync fence that stops overlapping collections from coalescing (kept on purpose), same-second selector-conflict ties, and one store rebuild per object coming back. Also lists what was fixed on the branch | | [`attribution-removal-wait-options.md`](design/attribution-removal-wait-options.md) | a removal now waits for evidence about the DELETION rather than accepting the object's last write, which stopped it naming whoever last edited the object as the author of a deletion they did not perform. Enumerates the eight situations a resolution can be in and shows the cost is concentrated in exactly one: a removal for which no delete fact will ever arrive (a graceful pod delete, a status-only removal, a type the audit policy skips) spends the whole grace to return the answer it had at t=0, measured at ~3.1s against ~70ms when evidence is present. Prices five options against that, and recommends a per-route watermark — stop waiting once the fact stream has demonstrably moved past this event — over a second timeout flag whose right value lives in the API server's config rather than ours. Open: the decision, and how common the case is outside the e2e suite | | [`commitrequest-save-wait-options.md`](design/commitrequest-save-wait-options.md) | **design, decided** — a five-phase plan, reported from a live cluster: a `CommitRequest` can expire before the write it exists to publish arrives, because the close delay defaulted to `0` while the write is still held head-of-line waiting for an audit fact, so the save resolves `Ready=True`/`NoWindowInGrace` and the edit commits seconds later under the target's `liveTemplate`. The decision is to repair the default (`spec.closeDelay`, now a duration string defaulting to `"2s"`, **shipped as phase 0**) and then let a request **name the write it is waiting for**, reusing the `(uid, resourceVersion)` identity the attribution join already keys on, including its settled `ExactCapable` rule. Records why the fact index cannot BE the wait (it sits upstream of the watch event, so a fact-keyed wait fires before the window opens) and what to take from it anyway — the register-then-check waiter discipline, and a best-effort retrospective oracle bounded by a TUNABLE TTL. Twelve situations, and the page is deliberate about the two it cannot fix cheaply: row 11 (a saturated queue is rejected in the WATCH path and never reaches the worker, so no worker-side wait can diagnose it) is an accepted, named gap rather than a promise, and row 10 is fixed only by a hook in the fidelity-drop branch that marks a wait terminally UNSATISFIABLE — satisfying before that gate would arm a finalize for a write that never commits. Carries a **preconditions chapter** for saves on today's build (audit policy first, then delay sizing, fidelity warm-up, save-button debounce, and assert on the commit rather than on `Ready`). `CommitterActivity` is deferred, not recommended: by its own arrival-not-existence rule it cannot satisfy the already-arrived case. Open: situation 12 (one actor, two saves, one window — blocks phase 2), whether the fact-index lookup earns its coupling, and how an unsatisfied wait is graded | +| [`commit-timing-surface.md`](design/commit-timing-surface.md) | **design.** One configuration surface for commit windows and saves, an in-place `v1alpha3` break with one tested upgrade procedure: only writes open a window; a `window` block (`idleTimeout`, `maxDuration`, target default `1m`) on both kinds, which a `CommitRequest` replaces whole for the window it attaches to; `attach: CurrentOrNext \| Next` with `attachTimeout`; opt-in `whenNothingToCommit: CommitEmpty` that keeps the cause in the reason. Precise clock and zero rules. Supersedes the save-wait timer model. Open: the target's `maxDuration` value | | [`watch-and-catalog-architecture.md`](design/watch-and-catalog-architecture.md) | the target three-layer watch model — **needs a human call before building** | | [`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 | diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ed6f2922..f6d63411 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,20 @@ 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 `CommitRequest`'s `closeDelay` restarts when it claims a window + +`closeDelay` used to be one deadline counted from when the worker received the request. Time spent +waiting for a matching window came out of it. It now bounds two phases: the request waits up to +`closeDelay` from receipt for a matching window, and a window it claims keeps collecting for +`closeDelay` from the claim. The field, its default and its bounds are unchanged. + +A request whose window is already open when it arrives behaves as before. A request created before +the writes it is saving now gets the whole delay after its first write, instead of whatever was left. +Its commit can be finalized up to one `closeDelay` later than it was, and the worst case from receipt +to the finalize is twice the delay; the push follows as before. If you had raised `closeDelay` to make +room for a slow first write, it now only needs to cover the longer of that gap and the span of the +save's writes. See [sizing `closeDelay`](configuration.md#sizing-closedelay). + ## Finished CommitRequests are deleted after 48 hours A `CommitRequest` that finishes after the upgrade gets a `configbutler.ai/delete-after` annotation @@ -18,7 +32,9 @@ before the upgrade have no annotation and are kept. To keep new ones as well, se The manager's ClusterRole includes the `patch` and `delete` verbs on `commitrequests`. A deployment that renders its own RBAC must add them. Without `patch` no annotation is written and requests -are kept. Without `delete`, requests stay after their time has passed. +are kept, and each such failure is logged. Without `delete`, requests stay after their time has +passed, and the controller logs a `Reconciler error` with a Forbidden cause for each one it tries to +delete. ## Placement `source="canonical"` is now `source="builtin"` diff --git a/docs/architecture.md b/docs/architecture.md index d6062c2a..66399c5f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1363,8 +1363,9 @@ immediately. That is not a failure: 1. The controller stamps the in-progress conditions (`Reconciling=True`) and settles `AuthorAttributed` synchronously from the admission author cache. There is no audit wait on this path. 2. The controller eagerly **attaches** the request to the worker (`AttachCommitRequest`), anchoring the - finalize at `receipt + closeDelay`. The worker binds it to an open window only when the author - state and GitTarget match. It **never finalizes another author's window**; a window carries at most one request. + wait for a window at `receipt + closeDelay`. The worker binds it to an open window only when the author + state and GitTarget match, and restarts the deadline at `claim + closeDelay`, so time spent waiting + never shortens the collection. It **never finalizes another author's window**; a window carries at most one request. 3. The window finalizes on the deadline (or when it closes for any other reason). If a finalize closes an open window, the worker always schedules a push, so a window closed by an otherwise no-op resync is not stranded. diff --git a/docs/configuration.md b/docs/configuration.md index bf5591aa..83688ad5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1876,9 +1876,10 @@ The important fields are: - `spec.gitTargetRef.name`: target whose open window should be finalized - `spec.message`: optional literal commit message, preserved verbatim -- `spec.closeDelay`: a Go duration string, at most `"5m"`, offsetting the deadline from the - worker's first receipt, including time waiting for a matching window; repeated registration keeps - the original deadline. Defaults to `"2s"` +- `spec.closeDelay`: a Go duration string, at most `"5m"`. The request waits this long from the + worker's first receipt for a matching window, and a window it claims keeps collecting this long + from the claim, so a request created before its writes still gets the whole delay after them. + Repeated registration keeps the original waiting deadline. Defaults to `"2s"` Example: @@ -1920,6 +1921,14 @@ Do not size the delay against `--author-attribution-grace`. When that grace expi the write still ships, as a window that names no actor, and a request naming a submitter can never claim it: the outcome is `WindowMismatch` no matter how long the request waits. +The delay applies to each phase separately: once waiting for the window, and again collecting +after the claim. A save that creates its request before making its writes needs a delay that covers +both the gap before its first write and the span from its first write to its last. With `"2s"`, +writes arriving 1 and 4 seconds after the request still split: the window is claimed at 1s and +finalized at 3s. The target's `commit.window` also still applies, so a silence longer than it +between two writes closes the window early. Twice the delay bounds the finalize; the push after it +can add its cooldown and any retries. + The two directions are not symmetric. Overshooting costs a few seconds of latency on the commit; undershooting resolves the request `Ready=True` with reason `NoWindowInGrace` while the edit commits seconds later under the target's `liveTemplate`, which reads as a save button that did diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md new file mode 100644 index 00000000..b18af08d --- /dev/null +++ b/docs/design/commit-timing-surface.md @@ -0,0 +1,298 @@ +# Commit windows and saves: one configuration surface + +> **design**: a proposal for the PR after #403, not built. Index: [`../INDEX.md`](../INDEX.md) +> +> The short version: a commit window is opened only by writes and closes on two timers. A save (a +> `CommitRequest`) attaches to one window and replaces both timers for it, longer or shorter. Both +> kinds describe the window in one `window` block with the same words, and a save adds which window +> it attaches to and whether it records its message when nothing changed. It is an in-place break of +> `v1alpha3`, with one tested upgrade procedure: +> +> ```yaml +> apiVersion: configbutler.ai/v1alpha3 apiVersion: configbutler.ai/v1alpha3 +> kind: GitTarget kind: CommitRequest +> spec: spec: +> commit: message: "fix: raise the checkout memory limit" +> window: window: +> idleTimeout: 5s attach: CurrentOrNext # or Next +> maxDuration: 1m attachTimeout: 2s +> idleTimeout: 1s # omitted: no idle close +> maxDuration: 10s +> whenNothingToCommit: Resolve # or CommitEmpty +> ``` +> +> It replaces `CommitRequest.spec.closeDelay` and the string `GitTarget.spec.commit.window`, and it +> supersedes the timer model in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md#the-timer-model). + +## Why + +Today one field, `closeDelay`, both bounds a save's wait for a window and, since #403, its +collection after the attach. The two need different sizes: the wait covers audit latency and the +client's time to its first write, and the collection covers the span of the save's writes. The +target's silence timer still runs on an attached window, so a save asking for 30 seconds gets at +most the target's 5 seconds of silence, silently. Nothing but the memory limit caps a busy window. A +save cannot choose to skip the author's earlier edits. And the words do not match: the target's +timer is called `window`, the save's `closeDelay`, and the worker's code calls it a "grace" and +says both "attach" and "claim". + +## Principles + +These are decided. + +1. **Only writes open a window.** A commit window opens on an author's first write to a target and + collects that author's writes until it closes. Nothing else opens one: not a save, not a timer. + A save that opens its own window, so the user has one to respond into, was weighed and rejected: + it would cut off the window already open, create windows without writes, and give a save two ways + to start collecting. `attach: Next`, `attachTimeout` and `whenNothingToCommit: CommitEmpty` cover what it was + for. +2. **A save attaches; it never opens.** It attaches to at most one window and gives that window its + message and its timers. "Attach" is the one word for the act, in the API, the code and the docs. +3. **The save's timers replace the target's, whole.** For the window a save attaches to, the save's + `idleTimeout` and `maxDuration` apply and the target's do not. A save may lengthen or shorten + either. The target's settings are the automatic policy, not a ceiling on explicit saves. +4. **Every timer is named for what runs out:** `attachTimeout`, `idleTimeout`, `maxDuration`. The two + collection timers mean the same thing on both kinds. +5. **Choices are enums.** A boolean that later needs a third state is a breaking change, and an enum + value names the behavior it selects (Kubernetes API conventions). +6. **A save never widens what it may take.** It finalizes its own submitter's work and nobody else's. + No setting here changes which windows match. +7. **Settings are snapshotted.** A window takes the target's timers when it opens; a save takes its + own when the worker first registers it. Editing a `GitTarget` never moves a deadline already + running. A `CommitRequest`'s whole spec stays immutable, the new fields included. + +## The surface + +### `GitTarget.spec.commit.window` + +| Field | Meaning | Default | Bound | +|---|---|---|---| +| `idleTimeout` | close after this much silence; each write restarts it | `5s` | `24h` | +| `maxDuration` | close this long after the window opened, whatever arrives | `1m` | `24h` | + +The finite `maxDuration` default is a product choice: continuous activity should not postpone a +commit until memory pressure forces one. One minute is a starting value to validate, not a +measurement. + +### `CommitRequest.spec.window` + +| Field | Meaning | Default | Bound | +|---|---|---|---| +| `attach` | which window the save covers: `CurrentOrNext` or `Next` | `CurrentOrNext` | enum | +| `attachTimeout` | stop waiting for a window this long after the worker registers the save | `2s` | `5m` | +| `idleTimeout` | close the attached window after this much silence | omitted: no idle close | `5m` | +| `maxDuration` | close the attached window this long after the attach | `2s` | `5m` | + +The default is a fixed two-second cutoff, so an explicit save finishes promptly. Rolling collection +is one setting away: `idleTimeout: 1s, maxDuration: 10s` collects a save whose writes trickle in and +still ends. CEL: `idleTimeout` must not exceed `maxDuration`. + +### `CommitRequest.spec.whenNothingToCommit` + +What a save does when it ends with nothing to commit, because its `attachTimeout` ran out without an +eligible window, or because the window it attached to closed, for any reason, without changing Git: +`Resolve` (default) finishes without a commit, and `CommitEmpty` records the message in an empty +commit. The outcome table below is the contract. The field names the situation and +the values name the consequence, the pattern Kubernetes uses for +`topologySpreadConstraints.whenUnsatisfiable` and StatefulSet's `whenDeleted`. "Nothing to commit" +is Git's own phrase, and `CommitEmpty` mirrors `git commit --allow-empty`. It sits outside `window` +because it decides the commit, not the window. CEL: `CommitEmpty` requires `spec.message`, because a +message is the only thing an empty commit records. + +## Clock rules + +- A window's `maxDuration` starts when a write opens it. A save's starts when it attaches. +- A save's attach starts its first idle interval. Each later matching write restarts the idle + interval only; nothing restarts `maxDuration`. +- A repeated delivery of the same save (the controller re-sends until it reads an outcome) restarts + nothing and is recognized by the save's identity. +- The first deadline reached closes the window. An author or target change, the memory limit, a + drain before a resync or atomic apply, and shutdown can close it earlier; a save does not change + those. +- A deadline bounds when the window is finalized. The push follows, with its cooldown and retries, + and no timer here promises when it lands. + +### Zero and omission + +Each value means one thing, and none borrows from another: + +| Setting | Meaning | +|---|---| +| `attachTimeout: 0s` | attach to an eligible window already open, or give up at once; do not wait | +| `maxDuration: 0s` (save) | finalize right after the attach | +| `maxDuration: 0s` (target) | close right after the write that opened the window: a commit per write | +| `idleTimeout` omitted (save) | no idle close; `maxDuration` alone ends collection | +| `idleTimeout: 0s` | close on the first silence, which is right after the write that opened or extended it | + +A zero `attachTimeout` never forces a zero collection: a save that attaches immediately still +collects for its `maxDuration`. + +Two rules keep the edges honest: + +- **An expired save cannot take a later window.** Once `attachTimeout` has run out, the save + resolves; a window that opens afterward is not its window. When the deadline and a matching write + are ready on the same loop wake, the expiry is decided first, and the write opens a window of its + own under the target's message. **This is new behavior.** #403 lets the overdue save attach to that + window and finalizes it at once with the save's message; it only stops the save from getting a + second delay. The stricter rule is simpler to state, and a test proves the late write stays out of + the expired save. +- **A save gets its attach before a zero timer fires.** With the target's `idleTimeout: 0s` the + worker finalizes a window in the same step the write opens it, so a waiting save never sees it. + The worker must offer a new window to waiting saves before it applies the target's timers. + Otherwise the promised override vanishes in exactly that configuration. + +## `attach: CurrentOrNext` or `Next` + +- **`CurrentOrNext`** attaches to the author's window if one is open at registration, and otherwise + to the next one a write opens. It is what a save made *after* its writes wants: the README example + applies the resources, then creates the request. +- **`Next`** starts clean. If the author has an eligible window open at registration, the worker + closes it first, and the save waits for a write to open another. It is for a save created + *before* its writes, which should not sweep up the author's earlier, unrelated edits. + +`Next` has three rules: + +- **The closed window keeps its message.** If another save already attached to it, that save's + message goes with it, and that save resolves exactly as if its own deadline had fired. +- **The close happens once per save.** It is keyed by the save's identity, so a controller re-send + never closes a second window. +- **`Next` needs time to wait.** CEL refuses `attach: Next` with `attachTimeout: 0s`: the save would + close the author's window and give up in the same instant, collecting nothing. +- **Competing saves are served first come, first served,** in the order the worker first registered + them. A window carries at most one save; the next waits for the next window. + +What `Next` guarantees is narrower than its name suggests. It separates work the worker already +collected from work that reaches the worker afterward. It cannot prove a write was made after the +request: a write still held for its audit fact arrives later and lands in the next window, and +waiting for the previous save to resolve does not exclude a delayed write from that save either. +That is acceptable for this feature, and the docs say so plainly. A save that must wait for a +specific write to be observed is what the named-write wait in the save-wait design is for. + +## `whenNothingToCommit: CommitEmpty` + +With `CommitEmpty`, a save records its message even when nothing it collected changed Git: + +| Outcome | `Resolve` (default) | `CommitEmpty` | +|---|---|---| +| Eligible writes changed files | commit | commit | +| Eligible writes already matched Git | no commit, `AlreadyPresent` | empty commit, `AlreadyPresent` | +| No eligible window before `attachTimeout` | no commit, `NoWindow` | empty commit, `NoWindow` | +| Only another author's window was open | no commit, `WindowMismatch` | no commit, `WindowMismatch` | +| Commit or push failed, or the target is suspended | the failure | the failure | + +The cause stays in the `Ready` reason, and `status.commit` with `Pushed=True` says a commit was +recorded. A caller reads both: a hash proves the message is in Git, and `NoWindow` says the save saw +no writes, so a write that was delayed past `attachTimeout` is not in that commit. A mismatch never +falls back to an empty commit, because recording a save while another author's work is in flight +would make the history claim more than happened. + +The empty commit is not a window, and principle 1 holds. Its author follows the same rule as any +commit: the request's submitter when admission captured one, otherwise the committer. Empty commits +already exist in this codebase: #383 reverts refused edits with one. + +**Duplicates across a restart are possible, and documented.** The worker suppresses a repeated save +while it runs. A crash after the push and before the status write loses that memory, and the +re-sent save can record a second empty commit. Preventing that needs durable recovery state, which +this feature does not justify. + +## Status + +Conditions stay the interface. The progress reasons under `Reconciling=True` become +`WaitingForWindow` (before the attach), `CollectingWindow` (after it) and `WaitingForPush` (committed +locally, not yet on the remote). The terminal `Ready` reasons tell apart an ordinary commit +(`Committed`), the two no-change causes (`AlreadyPresent`, `NoWindow`, which replaces +`NoWindowInGrace`), `WindowMismatch`, and failures. `Pushed=True` and `status.commit` appear only +after the remote confirmed the push. + +Every one of these comes from the worker. Sending an attach does not prove an attach, so the +controller reports a phase only when the worker says the save reached it. + +The controller's safety timeout covers the largest `attachTimeout`, the largest `maxDuration`, and +the push cooldown and retries. + +## Naming + +Every system that batches has the same timers, and a few also wait for the batch to start: + +| Tradition | Resets on activity | Fixed cap | Waiting to start | +|---|---|---|---| +| Network proxies (Envoy, Gateway API, NGINX) | `idleTimeout` | `maxDuration` | `connectTimeout` | +| Debounce (lodash, RxJS) | `wait` | `maxWait` | n/a | +| Batch exporters (OpenTelemetry, Kafka) | n/a | `timeout`, `linger.ms` | n/a | +| Kubernetes, Flux | n/a | `activeDeadlineSeconds`, `timeout` | n/a | + +The proxy words are the ones operators already know, and "timeout" is Flux's. "Idle" means "resets +on activity" to almost every reader; "deadline" reads as a point in time; the debounce words collide +with "wait" as a verb. + +- **The block is `window`,** the word [`definitions.md`](../definitions.md) already defines, on both + kinds. The timers are properties of the window. +- **Full names:** `idleTimeout` and `maxDuration`. Inside `window` nothing stutters, and a lone `max` + is unusual in Kubernetes APIs. +- **"Attach", not "claim".** "Claimed" already names a rule's demand on the read side. +- **`CurrentOrNext`, not `Current`.** The longer value states the fallback instead of hiding it. +- **`whenNothingToCommit: Resolve | CommitEmpty`, not `emptyCommit: Skip | Create`.** A setting + about an edge case reads best as the situation and its consequence. The default is `Resolve` + rather than `Close`, because "close" is what a window does (rule 1), and a save finishing is + already called resolving in the status and the code. + +Rejected: `windowTimeouts` and `windowPolicy` (both existed only to avoid changing a stored field's +type), flat fields on `commit` (they lose their subject), `batching` (drops the defined word), and +`closeDelay` as the cutoff (names an action, not a duration). + +## Plan + +One in-place `v1alpha3` break, built in three steps on one branch: + +1. **Timers.** The `window` blocks, the clock rules, the zero table, attach before a zero timer, the + snapshot, the target's `maxDuration`. `closeDelay` and the string `window` are gone. +2. **Attach selection.** `CurrentOrNext` and `Next`, the once-per-identity close, first-come + ordering, and the split progress reasons with worker-sourced observations. +3. **Empty commits.** `whenNothingToCommit`, the message requirement, the outcome table, and the documented + restart limitation. + +Alongside: "grace" and "claim" leave the commit path's code and comments; the finalize-reason +metric label splits into `idle-timeout` and `max-duration`, and `Next`'s close gets its own reason, +recorded in [`interpreting-metrics.md`](../interpreting-metrics.md); [`definitions.md`](../definitions.md) +gains "only writes open a window" and "attach"; the save-wait design's timer model points here. + +Tests: + +- Omitted blocks, `{}`, partial blocks, every zero value, and duration round-trips. +- The whole request spec is immutable, including `attach` and `whenNothingToCommit`. +- A save's `idleTimeout` longer and shorter than the target's; a save with only `maxDuration` is not + closed by the target's idle timer; continuous activity stops at `maxDuration` on both kinds. +- Expiry versus a matching write on the same wake; a waiting save attaches under the target's + `idleTimeout: 0s`; `attachTimeout: 0s` with an open window still collects; a write arriving after + an expired save's deadline commits separately, without the save's message; `Next` with + `attachTimeout: 0s` is refused. +- Overlapping saves in first-come order; a re-send restarts nothing and closes nothing; `Next` + keeps another save's message on the window it closes. +- A forced close (author change, memory limit, drain) of an attached window; push replay; each row + of the empty-commit table, and a restart between push and status. +- The upgrade procedure below, once, against stored objects from the previous release. + +## Open questions + +- **Is one minute the right target `maxDuration`?** It needs a look at real write bursts; the + decision that matters is that it is finite. + +## Migration + +No production users outside our control, so this stays small: an in-place break of `v1alpha3`, as +PRs #388, #397 and #398 were, with an UPGRADING entry and no conversion machinery. A version bump would +not help here. With `None` conversion it relabels objects without rewriting them, so a stored string +`commit.window` would fail to decode under a new version exactly as it does under the old one. + +That stored string is the one real hazard: once the new CRD is applied, a `GitTarget` still holding +it cannot be decoded, and a single such object breaks listing the kind. A removed `closeDelay` is +not a hazard: `kubectl apply` refuses it by name, and a stored one is simply unknown to the new +schema. The procedure, run once against a cluster on the previous release before it ships: + +1. **Remove the old field while the old schema is still served.** List the targets that set it + (`kubectl get gittargets -A -o json | jq ...`), note their values, and remove + `spec.commit.window` from each with a JSON patch. The old schema accepts that, and the running + controller falls back to its `5s` default in the meantime. +2. **Clear finished requests.** Delete finished `CommitRequest`s instead of carrying them across, and + let in-flight ones resolve first. +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`. diff --git a/docs/design/commitrequest-save-wait-options.md b/docs/design/commitrequest-save-wait-options.md index f7236d35..73851628 100644 --- a/docs/design/commitrequest-save-wait-options.md +++ b/docs/design/commitrequest-save-wait-options.md @@ -247,6 +247,11 @@ indistinguishable from one that never happened. ## The timer model +> **Superseded** by [`commit-timing-surface.md`](commit-timing-surface.md), which keeps the idea +> below (one job per field) and gives the fields their final names: `window.attachTimeout` is the +> give-up bound, and `window.maxDuration` and `window.idleTimeout` bound the collection from the +> attach. `waitFor` keeps only the named write. + Give each field one job, rather than reinterpreting `closeDelay` as a ceiling (R6): - `waitFor.timeoutSeconds` is the give-up bound, anchored at receipt. This is where audit-fact diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index 2499c07f..cb49070b 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -25,7 +25,10 @@ window, but never to a named actor’s window. A request with a named submitter actor’s named window. Therefore one user’s request never finalizes another user’s work. On its first receipt, the worker sets the deadline to receipt plus `closeDelay`. Repeated reconciles -are idempotent and keep that first deadline. Time spent waiting for a matching window consumes the delay. +are idempotent and keep that first deadline. When the request claims a window, the worker restarts the +deadline at the claim plus `closeDelay`: time spent waiting for a matching window does not shorten the +collection, so a request created before its writes still collects for the whole delay after them. A +request that claims a window already open at receipt keeps the deadline it was first given. The default is `"2s"` rather than `"0s"` because the write a request exists to publish reaches the worker strictly after the request does: a watch event is held until its audit fact arrives, so a diff --git a/hack/release-notes/check.mjs b/hack/release-notes/check.mjs new file mode 100644 index 00000000..604580b4 --- /dev/null +++ b/hack/release-notes/check.mjs @@ -0,0 +1,37 @@ +// Reads a commit message on stdin and fails when release-please would drop it. +// +// release-please parses every commit on main with @conventional-commits/parser and catches a parse +// failure at DEBUG level, so a commit that does not parse disappears from the changelog and from the +// version bump without a word. That is how #388 and its three breaking changes went missing from +// 0.50.0: one body line opened with `^([0-9]+(\.[0-9]+)?...`, which the grammar reads as a +// `type(scope` token and cannot close. +// +// The parser is pinned to the version release-please 17 depends on, the major that +// googleapis/release-please-action v5 bundles. Bump it with the action, not on its own. +// +// Usage: hack/release-notes/squash-message.sh origin/main HEAD "" | node hack/release-notes/check.mjs +import { readFileSync } from 'node:fs'; +import { parser } from '@conventional-commits/parser'; + +const message = readFileSync(0, 'utf8'); +try { + parser(message); + console.log('ok: release-please can parse this message'); +} catch (err) { + const match = /at (\d+):(\d+)/.exec(err.message); + const line = match ? message.split('\n')[Number(match[1]) - 1] : undefined; + console.log('## release-please would drop this commit from the changelog'); + console.log(''); + console.log(`The Conventional Commits parser fails: ${err.message}`); + if (line !== undefined) { + console.log(''); + console.log('```'); + console.log(line); + console.log('```'); + } + console.log(''); + console.log('A body line that starts with a word followed by nested parentheses, like `f(a(b))`, reads as'); + console.log('an unclosed `type(scope` token. Indent the line, or start it with other text, in the commit'); + console.log('message that holds it (the squash message is built from every commit on the branch).'); + process.exit(1); +} diff --git a/hack/release-notes/package-lock.json b/hack/release-notes/package-lock.json new file mode 100644 index 00000000..9141e501 --- /dev/null +++ b/hack/release-notes/package-lock.json @@ -0,0 +1,68 @@ +{ + "name": "release-notes-check", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "release-notes-check", + "dependencies": { + "@conventional-commits/parser": "0.4.1" + } + }, + "node_modules/@conventional-commits/parser": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/@conventional-commits/parser/-/parser-0.4.1.tgz", + "integrity": "sha512-H2ZmUVt6q+KBccXfMBhbBF14NlANeqHTXL4qCL6QGbMzrc4HDXyzWuxPxPNbz71f/5UkR5DrycP5VO9u7crahg==", + "license": "ISC", + "dependencies": { + "unist-util-visit": "^2.0.3", + "unist-util-visit-parents": "^3.1.1" + } + }, + "node_modules/@types/unist": { + "version": "2.0.11", + "resolved": "https://registry.npmjs.org/@types/unist/-/unist-2.0.11.tgz", + "integrity": "sha512-CmBKiL6NNo/OqgmMn95Fk9Whlp2mtvIv+KNpQKN2F4SjvrEesubTRWGYSg+BnWZOnlCaSTU1sMpsBOzgbYhnsA==", + "license": "MIT" + }, + "node_modules/unist-util-is": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/unist-util-is/-/unist-util-is-4.1.0.tgz", + "integrity": "sha512-ZOQSsnce92GrxSqlnEEseX0gi7GH9zTJZ0p9dtu87WRb/37mMPO2Ilx1s/t9vBHrFhbgweUwb+t7cIn5dxPhZg==", + "license": "MIT", + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, + "node_modules/unist-util-visit": { + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/unist-util-visit/-/unist-util-visit-2.0.3.tgz", + "integrity": "sha512-iJ4/RczbJMkD0712mGktuGpm/U4By4FfDonL7N/9tATGIF4imikjOuagyMY53tnZq3NP6BcmlrHhEKAfGWjh7Q==", + "license": "MIT", + "dependencies": { + "@types/unist": "^2.0.0", + "unist-util-is": "^4.0.0", + "unist-util-visit-parents": "^3.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, + "node_modules/unist-util-visit-parents": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/unist-util-visit-parents/-/unist-util-visit-parents-3.1.1.tgz", + "integrity": "sha512-1KROIZWo6bcMrZEwiH2UrXDyalAa0uqzWCxCJj6lPOvTve2WkfgCytoDTPaMnodXh1WrXOq0haVYHj99ynJlsg==", + "license": "MIT", + "dependencies": { + "@types/unist": "^2.0.0", + "unist-util-is": "^4.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + } + } +} diff --git a/hack/release-notes/package.json b/hack/release-notes/package.json new file mode 100644 index 00000000..08bf1bc6 --- /dev/null +++ b/hack/release-notes/package.json @@ -0,0 +1,9 @@ +{ + "name": "release-notes-check", + "private": true, + "description": "Proves a squash-merge message parses under the Conventional Commits parser release-please uses", + "type": "module", + "dependencies": { + "@conventional-commits/parser": "0.4.1" + } +} diff --git a/hack/release-notes/squash-message.sh b/hack/release-notes/squash-message.sh new file mode 100755 index 00000000..abbe895e --- /dev/null +++ b/hack/release-notes/squash-message.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# Prints the commit message GitHub writes when it squash-merges BASE..HEAD, for this repository's +# settings (squash_merge_commit_title COMMIT_OR_PR_TITLE, squash_merge_commit_message +# COMMIT_MESSAGES): +# +# one commit → that commit's own message; the PR title is not used +# several commits → the PR title, a blank line, then every commit message oldest first as "* " +# +# Usage: hack/release-notes/squash-message.sh BASE HEAD [PR_TITLE] +# Locally, the title defaults to a placeholder, which is enough to prove the body parses; CI passes +# the real one. +set -euo pipefail + +base=${1:?usage: squash-message.sh BASE HEAD [PR_TITLE]} +head=${2:?usage: squash-message.sh BASE HEAD [PR_TITLE]} +title=${3:-"chore: local squash-message check"} + +range="$(git merge-base "$base" "$head")..$head" +count=$(git rev-list --count "$range") + +case "$count" in + 0) echo "no commits in $range" >&2; exit 1 ;; + 1) git log -1 --format=%B "$head" ;; + *) + printf '%s\n\n' "$title" + git log --reverse --format='* %B' "$range" + ;; +esac diff --git a/hack/release-notes/test.sh b/hack/release-notes/test.sh new file mode 100755 index 00000000..45043f90 --- /dev/null +++ b/hack/release-notes/test.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +# Self-test for squash-message.sh and check.mjs: builds throwaway repositories and asserts the +# verdict for each shape of squash message. Needs `npm ci` in this directory first. +set -euo pipefail + +here=$(cd "$(dirname "$0")" && pwd) +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +failures=0 + +# expect NAME WANT(pass|fail) TITLE SUBJECT... +# Creates a repository with an empty base commit plus one commit per SUBJECT, then checks the +# squash message GitHub would write for merging them under TITLE. +expect() { + local name=$1 want=$2 title=$3 + shift 3 + local repo="$work/$name" + git init -q "$repo" + git -C "$repo" -c commit.gpgsign=false -c user.name=t -c user.email=t@t commit -q --allow-empty -m "chore: base" + local subject + for subject in "$@"; do + git -C "$repo" -c commit.gpgsign=false -c user.name=t -c user.email=t@t commit -q --allow-empty -m "$subject" + done + # Built first, and on its own: under set -e a crash in squash-message.sh aborts the run here, + # instead of reading as the parse failure a "want fail" case expects. + local message got=pass + message=$(cd "$repo" && "$here/squash-message.sh" "HEAD~$#" HEAD "$title") + node "$here/check.mjs" <<<"$message" >/dev/null || got=fail + if [[ "$got" == "$want" ]]; then + echo "ok $name" + else + echo "FAIL $name: want $want, got $got" + failures=$((failures + 1)) + fi +} + +# One commit: GitHub uses the commit's own message, so a good PR title cannot hide a bad subject. +expect single-good pass "feat: title" "feat: work" +expect single-bad-subject fail "feat: good title" "feat(a(b)): work" +expect single-bad-body fail "feat: good title" $'feat: work\n\nf(a(b)) opens this line' +# Several commits: the PR title leads, and each message is bulleted, so a line-leading token +# inside a commit's subject is shielded by "* " while one in a body is not. +expect multi-good pass "feat: title" "feat: one" "fix: two" +expect multi-bad-title fail "feat(a(b)): title" "feat: one" "fix: two" +expect multi-bad-body fail "feat: title" "feat: one" $'fix: two\n\nf(a(b)) opens this line' + +exit "$failures" diff --git a/internal/controller/commitrequest_controller.go b/internal/controller/commitrequest_controller.go index 51644b85..a8084ff4 100644 --- a/internal/controller/commitrequest_controller.go +++ b/internal/controller/commitrequest_controller.go @@ -62,12 +62,13 @@ const ( commitRequestPollInterval = 2 * time.Second // commitRequestResolveTimeout bounds the attach-then-poll wait, measured from - // object creation: it must cover the maximum collect-grace (closeDelay ≤ 5m, - // anchored at attribution) and the push cooldown plus retries. Authorship is now + // object creation: it must cover the longest a request can take (closeDelay ≤ 5m + // waiting for a window from receipt, then closeDelay again collecting from the + // claim) and the push cooldown plus retries. Authorship is now // settled synchronously at first sight (no attribution wait), so the former // +60s attribution component is gone. Past it, a request the worker never resolved // (e.g. a vanished worker) fails closed instead of polling forever. - commitRequestResolveTimeout = 300*time.Second + 120*time.Second + commitRequestResolveTimeout = 2*300*time.Second + 120*time.Second // defaultCloseDelay mirrors the +kubebuilder:default on // CommitRequest.spec.closeDelay. The API server fills the field in, so nil diff --git a/internal/git/commit_request_attach.go b/internal/git/commit_request_attach.go index f193022a..750c3f0f 100644 --- a/internal/git/commit_request_attach.go +++ b/internal/git/commit_request_attach.go @@ -86,9 +86,9 @@ type AttachCommitRequest struct { // Message is the verbatim commit message to attach to the window. Empty keeps // the generated grouped-commit message. Message string - // CloseDelay is the close-delay collect window: the worker closes the - // attached window and finalizes it at receipt + CloseDelay (the delay is - // anchored at attach receipt). + // CloseDelay bounds both phases of a request: it waits at most receipt + CloseDelay + // for a matching window, and once it claims one the worker finalizes that window at + // claim + CloseDelay. CloseDelay time.Duration } @@ -115,9 +115,15 @@ type pendingCommitRequest struct { gitTargetName string gitTargetNamespace string message string - // finalizeAt is receipt + CloseDelay, stamped once on first registration - // (idempotent re-sends keep it). + // finalizeAt is receipt + closeDelay while the request waits for a window, stamped once on + // first registration (idempotent re-sends keep it), and is re-stamped to claim + closeDelay + // when the request attaches. finalizeAt time.Time + // closeDelay is kept so the attach can re-stamp finalizeAt. A submitter that creates the + // request before making its writes would otherwise spend the delay waiting, and the window + // its writes open would be finalized moments after the first of them, splitting one save + // across two commits. + closeDelay time.Duration // attached is true once this request's message is bound to the open window. attached bool // committed is true once the window this request claimed has been finalized into a local diff --git a/internal/git/commit_request_attach_loop.go b/internal/git/commit_request_attach_loop.go index c3e33dca..838194a2 100644 --- a/internal/git/commit_request_attach_loop.go +++ b/internal/git/commit_request_attach_loop.go @@ -77,9 +77,10 @@ func (l *branchWorkerEventLoop) handleAttachCommitRequest(req *AttachCommitReque if _, exists := l.pendingCRs[id]; exists { return // idempotent re-send: keep the first finalize deadline. } - // Anchor the grace at receipt (≈ the attribution moment), not at object - // creation: under a delayed ingestion pipeline this lets delaySeconds cover only - // the inter-stream spread instead of the absolute latency. + // Anchor the wait at receipt (≈ the attribution moment), not at object + // creation: under a delayed ingestion pipeline this lets closeDelay cover only + // the inter-stream spread instead of the absolute latency. attachToOpenWindow + // restarts it at the claim. l.pendingCRs[id] = &pendingCommitRequest{ id: id, author: req.Author, @@ -88,6 +89,7 @@ func (l *branchWorkerEventLoop) handleAttachCommitRequest(req *AttachCommitReque gitTargetNamespace: req.GitTargetNamespace, message: req.Message, finalizeAt: time.Now().Add(req.CloseDelay), + closeDelay: req.CloseDelay, } l.w.Log.Info("CommitRequest registered with worker", "request", id.Namespace+"/"+id.Name, @@ -168,16 +170,29 @@ func (l *branchWorkerEventLoop) attachWaitingCommitRequests() { } } -// attachToOpenWindow binds a request's message to the currently-open window. +// attachToOpenWindow binds a request's message to the currently-open window, and restarts its +// deadline at the claim: the window then collects for the full closeDelay, however much of it the +// request spent waiting. When the window was already open at receipt the claim IS the receipt, +// so the deadline is the one first stamped. +// +// A request whose wait has already run out is attached WITHOUT a restart. The deadline timer and a +// matching event can be ready on the same loop wake, and this pass runs before +// processDueCommitRequests, so restarting here would hand an expired request a second full delay +// depending on which of the two the select picked. Left alone, it is finalized in this same pass, +// carrying its message, which is also what an explicit "0s" relies on. func (l *branchWorkerEventLoop) attachToOpenWindow(pcr *pendingCommitRequest) { l.openWindow.pendingMessage = pcr.message id := pcr.id l.openWindow.pendingCR = &id pcr.attached = true + if now := time.Now(); pcr.finalizeAt.After(now) { + pcr.finalizeAt = now.Add(pcr.closeDelay) + } l.w.Log.Info("CommitRequest attached to open window", "request", id.Namespace+"/"+id.Name, "author", pcr.author, - "target", pcr.gitTargetNamespace+"/"+pcr.gitTargetName) + "target", pcr.gitTargetNamespace+"/"+pcr.gitTargetName, + "closeDelay", pcr.closeDelay.String()) } // processDueCommitRequests finalizes the windows of attached requests whose grace diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index 850fedb2..c701582a 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -224,6 +224,112 @@ func TestAttach_CollectGraceJoinsLaterWindow(t *testing.T) { assert.Nil(t, loop.openWindow) } +// TestAttach_ClaimRestartsTheDeadline pins that the time a request spent waiting for its window +// does not come out of that window's collection: a request created before the writes it is saving +// gets the full closeDelay after it claims the window, not whatever was left of it. +func TestAttach_ClaimRestartsTheDeadline(t *testing.T) { + worker, _, _ := setupCommitPushSplitWorker(t) + createPlainGitTarget(t, worker, "team-a", "team-a") + + loop := newBranchWorkerEventLoop(worker, time.Hour) + defer loop.stopTimers() + + const closeDelay = 60 * time.Second + serviceAttach(loop, attachReq("alice", closeDelay)) + id := commitRequestID{Namespace: "default", Name: crName, UID: "uid-" + crName} + // Almost all of the delay spent waiting: the writes arrive with a second of it left. + loop.pendingCRs[id].finalizeAt = time.Now().Add(time.Second) + + loop.handleQueueItem(WorkItem{Request: &WriteRequest{ + Events: []Event{configMapTargetEvent("late", "alice", "team-a")}, + CommitMode: CommitModePerEvent, + }}) + claimed := time.Now() + loop.serviceCommitRequests() + require.NotNil(t, loop.openWindow) + require.NotNil(t, loop.openWindow.pendingCR, "precondition: the request claimed the window") + + assert.False(t, loop.pendingCRs[id].finalizeAt.Before(claimed.Add(closeDelay)), + "the claim must restart the deadline at claim + closeDelay") +} + +// TestAttach_AnExpiredWaitIsNotRestartedByALateClaim pins the deadline boundary: when the deadline +// timer and a matching event are ready on the same loop wake and the event is served first, the +// overdue request still claims the window, but it is finalized in that pass rather than handed a +// second full closeDelay. +func TestAttach_AnExpiredWaitIsNotRestartedByALateClaim(t *testing.T) { + worker, _, _ := setupCommitPushSplitWorker(t) + createPlainGitTarget(t, worker, "team-a", "team-a") + + loop := newBranchWorkerEventLoop(worker, time.Hour) + defer loop.stopTimers() + + req := attachReq("alice", 60*time.Second) + req.Message = "late save" + serviceAttach(loop, req) + forceDue(loop) // the wait ran out, and its timer has not been served yet + + loop.handleQueueItem(WorkItem{Request: &WriteRequest{ + Events: []Event{configMapTargetEvent("late", "alice", "team-a")}, + CommitMode: CommitModePerEvent, + }}) + loop.serviceCommitRequests() + + res, ok := outcome(t, worker) + require.True(t, ok, "the overdue request must resolve in this pass, not wait out a fresh delay") + require.NoError(t, res.Err) + assert.Equal(t, FinalizeCommitted, res.Outcome, "it claimed the window, so the commit carries its message") + assert.Nil(t, loop.openWindow) +} + +// TestAttach_WritesAfterTheClaimJoinOneCommit pins the save the claim restart exists for: a request +// created before its writes, whose writes arrive one after another, publishes them as ONE commit +// carrying its message. +func TestAttach_WritesAfterTheClaimJoinOneCommit(t *testing.T) { + worker, _, _ := setupCommitPushSplitWorker(t) + createPlainGitTarget(t, worker, "team-a", "team-a") + + loop := newBranchWorkerEventLoop(worker, time.Hour) + defer loop.stopTimers() + + req := attachReq("alice", 60*time.Second) + req.Message = "save: two writes" + serviceAttach(loop, req) + + for _, name := range []string{"first", "second"} { + loop.handleQueueItem(WorkItem{Request: &WriteRequest{ + Events: []Event{configMapTargetEvent(name, "alice", "team-a")}, + CommitMode: CommitModePerEvent, + }}) + loop.serviceCommitRequests() + _, resolved := outcome(t, worker) + require.False(t, resolved, "a write inside the delay must not close the window: %s", name) + } + + forceDue(loop) + loop.serviceCommitRequests() + res, ok := outcome(t, worker) + require.True(t, ok) + require.NoError(t, res.Err) + require.Equal(t, FinalizeCommitted, res.Outcome) + + repo, err := gogit.PlainOpen(worker.repoPath()) + require.NoError(t, err) + commit, err := repo.CommitObject(plumbing.NewHash(res.Commit)) + require.NoError(t, err) + assert.Equal(t, "save: two writes", commit.Message) + stats, err := commit.Stats() + require.NoError(t, err) + changed := make([]string, 0, len(stats)) + for _, stat := range stats { + changed = append(changed, stat.Name) + } + assert.Subset(t, changed, []string{ + "team-a/default/configmaps/first.yaml", + "team-a/default/configmaps/second.yaml", + }, "both writes must land in the one commit carrying the message") +} + // TestAttach_ForeignWindowIsNotStolen verifies an attach for a different author // parks (never finalizes another author's window) and resolves WindowMismatch once // its grace elapses, leaving the foreign window open. diff --git a/internal/watch/owner.go b/internal/watch/owner.go index a65aaa22..cfb37109 100644 --- a/internal/watch/owner.go +++ b/internal/watch/owner.go @@ -4,6 +4,7 @@ package watch import ( "context" + "errors" "sort" "sync" "time" @@ -709,7 +710,11 @@ func (m *Manager) runTargetPass(ctx context.Context, log logr.Logger, entry *dir err := m.applyTargetPlan(passCtx, planned) m.recordPassOutcome(planned.ref, started, err) - if err != nil { + switch { + case errors.Is(err, errAPISurfaceNotObserved): + log.Info("GitTarget plan pass is waiting for API discovery; it will be retried", + "gitDest", planned.ref.String(), "reasons", sortedKeys(entry.reasons)) + case err != nil: log.Error(err, "GitTarget plan pass failed; the target stays dirty", "gitDest", planned.ref.String(), "reasons", sortedKeys(entry.reasons)) } diff --git a/internal/watch/owner_test.go b/internal/watch/owner_test.go index a5ff39d2..c167aee7 100644 --- a/internal/watch/owner_test.go +++ b/internal/watch/owner_test.go @@ -296,7 +296,9 @@ func TestOwner_AFailedPassLeavesTheRunningPlanUntouched(t *testing.T) { // This pass cannot gather: nothing has observed the cluster's API surface. err := m.applyTargetPlan(context.Background(), declareIntent{ref: ref, clusterID: configPlaneClusterID}) - require.Error(t, err, "a pass that cannot observe the surface fails rather than planning against nothing") + require.ErrorIs(t, err, errAPISurfaceNotObserved, + "a pass that cannot observe the surface fails rather than planning against nothing, "+ + "and says so in a way the owner loop can log as a wait") assert.False(t, *cancelled, "the running stream is left alone; the failure installs nothing") assert.Len(t, m.targetWatchSet(ref).streams, 1, "and the plan it was built from still stands") } diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 5cfbaf82..288b0baa 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -37,6 +37,10 @@ const ( var ( errTargetWatchClosed = errors.New("target watch result channel closed") errTargetWatchExpired = errors.New("target watch resourceVersion expired") + // errAPISurfaceNotObserved is the pass failing because shared discovery has not yet run for the + // target's source cluster. Every rollout meets it once per target, and the retry resolves it, so + // it is logged as the wait it is rather than as an error. + errAPISurfaceNotObserved = errors.New("the cluster API surface has not been observed yet") ) // targetWatchClosedErr distinguishes a watch that died under us from one that closed because @@ -221,8 +225,7 @@ func (m *Manager) ensureGitTargetWatches( return err } if !m.registryForGitTarget(gitDest).Ready() { - return fmt.Errorf("aborting watch setup for %s: the cluster API surface has not been observed yet", - gitDest.String()) + return fmt.Errorf("aborting watch setup for %s: %w", gitDest.String(), errAPISurfaceNotObserved) } table := m.residentWatchedTypeTable(gitDest)