From c4d421ca8996ae43ecb6a747833550579c668cea Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:08:04 +0000 Subject: [PATCH 01/15] docs(changelog): restore #388 and its three breaking changes to 0.50.0 release-please dropped a0879014 entirely: its body could not be parsed, and the parse failure is only logged at debug level. The entry and its breaks (closeDelaySeconds renamed to closeDelay, commit.window validated, and --base-trust-max-age removed) are added to the 0.50.0 section by hand. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) 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) From 6d0047e95607a5fe867d1701ff55bc4591be10eb Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:08:13 +0000 Subject: [PATCH 02/15] ci: fail a pull request whose squash message release-please cannot parse release-please parses every commit on main with @conventional-commits/parser and drops one that throws, logging it only at debug level. A body line that opens with a word followed by nested parentheses reads as an unclosed type-and-scope token; one such line in #388 cost 0.50.0 its changelog entry and three breaking changes. The new job rebuilds the COMMIT_MESSAGES squash body and parses it with the parser version release-please 17 uses. RELEASES.md records the symptom, the cause, the local check, and the repair after a release. Co-Authored-By: Claude Opus 5.5 --- .github/RELEASES.md | 24 ++++++++++ .github/workflows/pr-title.yml | 40 ++++++++++++++++ .gitignore | 1 + hack/release-notes/check.mjs | 37 +++++++++++++++ hack/release-notes/package-lock.json | 68 ++++++++++++++++++++++++++++ hack/release-notes/package.json | 9 ++++ 6 files changed, 179 insertions(+) create mode 100644 hack/release-notes/check.mjs create mode 100644 hack/release-notes/package-lock.json create mode 100644 hack/release-notes/package.json diff --git a/.github/RELEASES.md b/.github/RELEASES.md index a9c043dd..dc900c19 100644 --- a/.github/RELEASES.md +++ b/.github/RELEASES.md @@ -259,6 +259,30 @@ 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) +git log --reverse --format='* %B' origin/main..HEAD | node hack/release-notes/check.mjs +``` + +**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..cc9805b8 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,41 @@ 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, 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: 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: | + # GitHub's COMMIT_MESSAGES body: each commit, oldest first, as "* ". + { + printf '%s\n\n' "$PR_TITLE" + git log --reverse --format='* %B' "$(git merge-base "$BASE_SHA" "$HEAD_SHA")..$HEAD_SHA" + } > "$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/hack/release-notes/check.mjs b/hack/release-notes/check.mjs new file mode 100644 index 00000000..9a2cf03b --- /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: git log --format='* %B' 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" + } +} From a237a0a8fa2bee8ef365edb51cdd9b2a7252645d Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:08:14 +0000 Subject: [PATCH 03/15] fix(watch): log a pass waiting for API discovery as a wait, not an error Every rollout logged one ERROR per GitTarget while shared discovery had not yet observed the cluster API surface; the retry resolves it on its own. The pass still fails and is still recorded in the watch-plane status and metrics, only the log level changes. Co-Authored-By: Claude Opus 5.5 --- internal/watch/owner.go | 7 ++++++- internal/watch/owner_test.go | 4 +++- internal/watch/target_watch.go | 7 +++++-- 3 files changed, 14 insertions(+), 4 deletions(-) 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) From 30358b701a4c13d404e0a9d385b3c6984b8d3285 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:08:14 +0000 Subject: [PATCH 04/15] feat(commit): restart a CommitRequest's closeDelay when it claims a window A request created before the writes it saves spent its closeDelay waiting, and the window those writes opened was then finalized moments after the first of them, splitting one save across two commits. The worker now restarts the deadline at the claim, so a claimed window always collects for the full delay. A request whose window is already open at receipt is unaffected. The worst case from receipt to commit becomes twice the delay, so the controller's resolve timeout now covers two 5m phases. UPGRADING.md records the behaviour change and the 0.50 field report's RBAC question. Co-Authored-By: Claude Opus 5.5 --- api/v1alpha3/commitrequest_types.go | 10 ++++--- .../bases/configbutler.ai_commitrequests.yaml | 10 ++++--- docs/UPGRADING.md | 17 ++++++++++- docs/architecture.md | 5 ++-- docs/configuration.md | 11 +++++-- docs/spec/commitrequest-design.md | 5 +++- .../controller/commitrequest_controller.go | 7 +++-- internal/git/commit_request_attach.go | 16 ++++++---- internal/git/commit_request_attach_loop.go | 17 +++++++---- internal/git/commit_request_attach_test.go | 29 +++++++++++++++++++ 10 files changed, 99 insertions(+), 28 deletions(-) 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/UPGRADING.md b/docs/UPGRADING.md index ed6f2922..f9916f3f 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,19 @@ 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 land up to one `closeDelay` later than it did, and the worst case from receipt to the +commit is twice the delay. If you had raised `closeDelay` to make room for a slow first write, you may +be able to lower it again. 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 +31,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..58510d00 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,10 @@ 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 therefore sizes the delay +to the gap before its first write alone. The worst case to the commit is twice the delay. + 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/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/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..59bd6aeb 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,21 @@ 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. func (l *branchWorkerEventLoop) attachToOpenWindow(pcr *pendingCommitRequest) { l.openWindow.pendingMessage = pcr.message id := pcr.id l.openWindow.pendingCR = &id pcr.attached = true + pcr.finalizeAt = time.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..7771fdce 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -224,6 +224,35 @@ 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_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. From ac89fcdc20cea20b3664e208f8a3af0f01e8e2bb Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:35:36 +0000 Subject: [PATCH 05/15] fix(commit): never restart an expired wait at the claim, and size closeDelay for the whole save The deadline timer and a matching event can be ready on the same loop wake. When the event won, the overdue request claimed the window and was handed a second full closeDelay. It now keeps its expired deadline, so it is finalized in that pass with its message, as on main; an explicit 0s relies on the same path. The sizing guide said a save that creates its request first only needs the gap before its first write. It needs the span of its writes too, and twice the delay bounds the finalize, not the push. Co-Authored-By: Claude Opus 5.5 --- docs/UPGRADING.md | 7 +- docs/configuration.md | 8 ++- internal/git/commit_request_attach_loop.go | 10 ++- internal/git/commit_request_attach_test.go | 77 ++++++++++++++++++++++ 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index f9916f3f..f6d63411 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -16,9 +16,10 @@ waiting for a matching window came out of it. It now bounds two phases: the requ 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 land up to one `closeDelay` later than it did, and the worst case from receipt to the -commit is twice the delay. If you had raised `closeDelay` to make room for a slow first write, you may -be able to lower it again. See [sizing `closeDelay`](configuration.md#sizing-closedelay). +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 diff --git a/docs/configuration.md b/docs/configuration.md index 58510d00..83688ad5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1922,8 +1922,12 @@ the write still ships, as a window that names no actor, and a request naming a s 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 therefore sizes the delay -to the gap before its first write alone. The worst case to the commit is twice the delay. +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 diff --git a/internal/git/commit_request_attach_loop.go b/internal/git/commit_request_attach_loop.go index 59bd6aeb..838194a2 100644 --- a/internal/git/commit_request_attach_loop.go +++ b/internal/git/commit_request_attach_loop.go @@ -174,12 +174,20 @@ func (l *branchWorkerEventLoop) attachWaitingCommitRequests() { // 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 - pcr.finalizeAt = time.Now().Add(pcr.closeDelay) + 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, diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index 7771fdce..c701582a 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -253,6 +253,83 @@ func TestAttach_ClaimRestartsTheDeadline(t *testing.T) { "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. From 61dd8e35c090f06ca144698459c22114e2ced653 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:35:36 +0000 Subject: [PATCH 06/15] ci: build the squash message the way GitHub does for a single commit With COMMIT_OR_PR_TITLE a single-commit PR squashes to that commit's own message, so the check parsed a message GitHub never writes: a bad commit subject passed behind a good PR title. squash-message.sh now owns the rule for both the workflow and the local command, and test.sh covers both shapes. Co-Authored-By: Claude Opus 5.5 --- .github/RELEASES.md | 8 +++-- .github/workflows/pr-title.yml | 15 ++++----- hack/release-notes/check.mjs | 2 +- hack/release-notes/squash-message.sh | 28 +++++++++++++++++ hack/release-notes/test.sh | 46 ++++++++++++++++++++++++++++ 5 files changed, 89 insertions(+), 10 deletions(-) create mode 100755 hack/release-notes/squash-message.sh create mode 100755 hack/release-notes/test.sh diff --git a/.github/RELEASES.md b/.github/RELEASES.md index dc900c19..45bb9281 100644 --- a/.github/RELEASES.md +++ b/.github/RELEASES.md @@ -259,7 +259,7 @@ 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 +### 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. @@ -276,9 +276,13 @@ text, in the commit that holds it. To check a branch locally: ```bash (cd hack/release-notes && npm ci --ignore-scripts) -git log --reverse --format='* %B' origin/main..HEAD | node hack/release-notes/check.mjs +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. diff --git a/.github/workflows/pr-title.yml b/.github/workflows/pr-title.yml index cc9805b8..90b36f84 100644 --- a/.github/workflows/pr-title.yml +++ b/.github/workflows/pr-title.yml @@ -51,8 +51,8 @@ jobs: 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, and release-please parses - # the whole of it. A body the parser cannot read drops the commit from the changelog and the + # 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. # @@ -74,16 +74,17 @@ jobs: 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: | - # GitHub's COMMIT_MESSAGES body: each commit, oldest first, as "* ". - { - printf '%s\n\n' "$PR_TITLE" - git log --reverse --format='* %B' "$(git merge-base "$BASE_SHA" "$HEAD_SHA")..$HEAD_SHA" - } > "$RUNNER_TEMP/squash-message.txt" + # 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/hack/release-notes/check.mjs b/hack/release-notes/check.mjs index 9a2cf03b..604580b4 100644 --- a/hack/release-notes/check.mjs +++ b/hack/release-notes/check.mjs @@ -9,7 +9,7 @@ // 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: git log --format='* %B' origin/main..HEAD | node hack/release-notes/check.mjs +// 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'; 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..6fc46ea9 --- /dev/null +++ b/hack/release-notes/test.sh @@ -0,0 +1,46 @@ +#!/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 + local got=pass + (cd "$repo" && "$here/squash-message.sh" HEAD~$# HEAD "$title") \ + | node "$here/check.mjs" >/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" From 9dd6e7bec6090286d06e9692be23ff6b2826d4e2 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:35:37 +0000 Subject: [PATCH 07/15] docs(design): propose one timing vocabulary for the target and the save Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 3 +- docs/design/commit-timing-surface.md | 148 +++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 docs/design/commit-timing-surface.md diff --git a/docs/INDEX.md b/docs/INDEX.md index e1f079bc..965232bb 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.** The follow-up to #403: `closeDelay` both waits for a window and collects after the claim, and cannot outlast the target's silence timer. Proposes one field per job (`waitFor.timeout`, a request `window` that replaces the target's for the claimed window, and `closeDelay` as the fixed cutoff), keeps the fixed cutoff as the default, and lands inside the save-wait timer model. Open: whether a request `window` may shorten as well as lengthen, and whether `closeDelay` should become `maxWindow` | | [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md new file mode 100644 index 00000000..dcdeda7f --- /dev/null +++ b/docs/design/commit-timing-surface.md @@ -0,0 +1,148 @@ +# Commit timing: one vocabulary for the target and the save + +> **design**: a proposal for the PR after #403, not built. Index: [`../INDEX.md`](../INDEX.md) +> +> The short version: three timers decide when a claimed window closes, and today two fields spread +> across two kinds carry them under two unrelated names. `CommitRequest.spec.closeDelay` does two +> jobs, waiting for a window and collecting after the claim, and it cannot keep a window open past +> the target's own silence timer. The proposal gives each job one field, reuses `window` for the one +> rolling timer wherever it appears, and keeps today's fixed cutoff as the default. It extends the +> timer model already decided in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md); +> it does not replace it. + +## What decides when a window closes today + +After #403, these are the timers and boundaries that end a commit window: + +| What | Field | Starts from | Resets on a matching change? | Default | +|---|---|---|---|---| +| Target silence | `GitTarget.spec.commit.window` | each event | yes | `5s`, at most `24h` | +| Request wait | `CommitRequest.spec.closeDelay` | receipt by the branch worker | no | `2s`, at most `5m` | +| Request collection | the same `closeDelay` | the claim | no | the same value | + +Other boundaries close a window regardless of any timer: a change of author or target, the branch +buffer's memory limit (`buffer-limit`), a resync or atomic apply that needs the window drained +first, `window: 0s`, and shutdown. The `windowFinalizeReason` values in +[`branch_worker.go`](../../internal/git/branch_worker.go) name them all. + +## What does not line up + +**One field, two jobs.** `closeDelay` bounds the wait for a window and, since #403, the collection +after the claim. Those two phases need different sizes. The wait covers the cluster's audit latency +plus however long the client takes to make its first write. The collection covers the span of the +save's own writes. One number is either too long for one of them or too short for the other. + +**A request cannot hold a window open longer than the target allows.** The target's silence timer +still runs on a claimed window (`windowFinalizeReasonTimer` in the event loop). With the default +`window: 5s`, a `closeDelay: 30s` collects for at most 5 seconds of silence, not 30. The request +asked for a longer interval and the shorter one wins without saying so. + +**Nothing caps a busy window except memory.** The target's `window` is purely rolling: continuous +activity keeps a window open until `buffer-limit` trips. There is no time bound. A request gets a +fixed cutoff from `closeDelay`; a target cannot express one. + +**The names do not say what the fields do.** A [commit window](../definitions.md) is the interval a +branch worker batches writes over. On the target, `window` is that interval's silence timeout. On +the request, the comparable timer is called `closeDelay`. In the worker's code and comments it is +called a "grace" (`collect grace`, `grace elapsed`), which collides with the +[grace window](../definitions.md) the definitions page reserves for the attribution wait. + +**Omission and `2s` look the same.** The CRD fills an omitted `closeDelay` with `"2s"` on write, so +the controller cannot tell a request that chose `2s` from one that left it out. Any inherited default, +for example one set on the target, needs that distinction. + +## Constraints the next PR inherits + +- **The decided timer model.** [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md#the-timer-model) + already settled "give each field one job": a `waitFor.timeout` anchored at receipt as the give-up + bound, and `closeDelay` measured from satisfaction. #403 moved `closeDelay`'s collection anchor to + the claim, which is the same idea for a request that names no write. This proposal must land + inside that model. +- **The binding vocabulary.** [`definitions.md`](../definitions.md) owns field names. Flux is the + precedent for durations (`timeout`, `interval`), and every duration is a Go duration string with a + CEL bound ([API durations](../UPGRADING.md#every-duration-in-the-api-is-a-go-duration-string)). +- **An immutable spec.** A `CommitRequest`'s whole spec is immutable, and a request lives at most 48 + hours after it finishes. A field change reaches new requests only; there is no stored object to + migrate beyond that window. +- **The attach invariant.** A request finalizes its own submitter's work and nobody else's. No + timer change may widen what a request can claim. + +## Proposal + +Three concepts, one name each, and the same name wherever the concept appears: + +| Concept | Field | Starts from | Resets? | Default | +|---|---|---|---|---| +| Wait timeout | `CommitRequest.spec.waitFor.timeout` | receipt | no | the value of `closeDelay`, which is today's behaviour | +| Rolling collection | `CommitRequest.spec.window` | the claim, then each matching event | yes | omitted: no rolling collection | +| Collection cutoff | `CommitRequest.spec.closeDelay` | the claim | no | `2s` | + +**`waitFor.timeout`** is the field the save-wait design already names, shipped first without its +resource reference. A request with only a timeout waits for any matching window, as today. It takes +over the waiting job from `closeDelay`. While it is omitted it defaults to the request's +`closeDelay`, so a request written for 0.50 behaves the same. + +**`window` on a request** has exactly the meaning `GitTarget.spec.commit.window` has: the silence +after which the window closes. For a claimed window it **replaces** the target's silence timer, so a +request can ask for a longer interval, or a shorter one, and get it. Author or target changes, +`buffer-limit`, drain-before-apply and shutdown remain boundaries; the request changes only the +timer. + +**`closeDelay`** keeps its name and ends up meaning exactly what the name says: close this long +after the claim, whatever arrives. With `window` set it is the cap that keeps continuous activity +from holding the window open. Without `window` it is today's fixed cutoff. + +**Keep the fixed cutoff the default.** Rolling collection groups a save whose writes trickle in, but +it has no predictable end. It should be something a caller asks for, bounded by `closeDelay`. +Making `closeDelay` itself reset on each change would lose the one bound a caller can rely on. + +### What stays out of this PR + +- **A time cap on the target's own window** (`GitTarget.spec.commit.maxWindow` or similar). The + table shows the gap, but nobody has hit it; `buffer-limit` covers the memory risk. It can reuse the + request's vocabulary later if it is needed. +- **Inherited defaults** (a target-level or cluster-level default for request timing). That needs + omission to be distinguishable, which means moving the `2s` default out of the schema and into the + controller. Do that in the PR that adds the inherited default, not before. Until then the schema + default is the more readable contract: `kubectl get -o yaml` shows the value that applied. +- **The named-write wait** (`waitFor` with a resource reference). That is phase 2 of the save-wait + design and depends on its phase 1. + +### Status + +The single progress reason `WaitingForCloseDelay` covers both phases today. Split it so a stuck +request says which phase it is in: `WaitingForWindow` before the claim, and `Collecting` after it. +Both are progress reasons under `Reconciling=True`, following +[`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). Rename "grace" to "wait" or +"collection" in the worker's code and comments in the same PR, so the attribution grace window is +the only grace left. + +## Plan + +1. **API.** Add `spec.window` and `spec.waitFor.timeout` to `CommitRequest`, both optional, both Go + duration strings with the same pattern and CEL bounds as today (`window` at most `24h`, as on the + target; `timeout` at most `5m`). Additive, so no new API version. +2. **Worker.** Carry the three values on `AttachCommitRequest`. Stamp the wait deadline from + `waitFor.timeout`, falling back to `closeDelay`. On the claim, set the cutoff at claim + + `closeDelay`, and when `window` is set, arm the claimed window's silence timer from it instead of + the target's. The claim must still check expiry first; #403's boundary test covers that. +3. **Controller.** Resolve the fallback, split the progress reason, and size + `commitRequestResolveTimeout` from the largest wait plus the largest cutoff. +4. **Docs.** Replace the sizing section in [`configuration.md`](../configuration.md#sizing-closedelay) + with one paragraph per field, add the upgrade entry, and add a definition for the request's + collection to [`definitions.md`](../definitions.md). +5. **Tests.** Wait expiry versus claim on the same wake; writes arriving after the claim join one + commit; a request `window` longer than the target's keeps the window open; a request `window` + shorter than the target's closes it sooner; continuous activity stops at `closeDelay`; a change of + author still closes a claimed window. + +## Open questions + +- Should a request `window` also be allowed to shorten collection below the target's, or only lengthen + it? Replacing both ways is simpler to explain; lengthening only is harder to misuse. +- Is `closeDelay` still the right name once it is only the cutoff, or should it become `maxWindow`, + mirroring a future target field? Renaming costs one more migration for every integrator. Keeping it + keeps a name that now means what it says. +- Does `waitFor.timeout` defaulting to `closeDelay` read as a surprise once the two are separate, or + should it default to its own `2s`? The fallback keeps 0.50 requests identical; a fixed default is + easier to document. From 307b3f6e111a02110a1512b2335b89e70caab920 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:39:37 +0000 Subject: [PATCH 08/15] docs(design): name every commit timer as a timeout, and weigh the candidates The follow-up now takes the freedom to rename. One windowTimeouts block, with idle and max, is shared by GitTarget.spec.commit and CommitRequest.spec, and the request's block replaces the target's for the window it claims. waitFor.timeout carries the wait. closeDelay and the string commit.window are retained for one release as loud rejections, because a type change under the stored name would break decoding. Six naming candidates are weighed against the definitions rulebook and the precedents from proxies, debounce, batch exporters and Kubernetes. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 2 +- docs/design/commit-timing-surface.md | 285 +++++++++++++++++---------- 2 files changed, 184 insertions(+), 103 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 965232bb..adcbc9b2 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -139,7 +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.** The follow-up to #403: `closeDelay` both waits for a window and collects after the claim, and cannot outlast the target's silence timer. Proposes one field per job (`waitFor.timeout`, a request `window` that replaces the target's for the claimed window, and `closeDelay` as the fixed cutoff), keeps the fixed cutoff as the default, and lands inside the save-wait timer model. Open: whether a request `window` may shorten as well as lengthen, and whether `closeDelay` should become `maxWindow` | +| [`commit-timing-surface.md`](design/commit-timing-surface.md) | **design.** The follow-up to #403: `closeDelay` both waits for a window and collects after the claim, and cannot outlast the target's silence timer. Proposes one `windowTimeouts: {idle, max}` block shared by `GitTarget.spec.commit` and `CommitRequest.spec` (the request's replaces the target's for the window it claims), `waitFor.timeout` for the wait, and retiring `closeDelay` and the string `commit.window` as loud rejections. Weighs six naming candidates. Open: `max` or `maxDuration`, whether a request may shorten a window, and whether the target's `max` ships in the same PR | | [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index dcdeda7f..9b759fe4 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -2,17 +2,29 @@ > **design**: a proposal for the PR after #403, not built. Index: [`../INDEX.md`](../INDEX.md) > -> The short version: three timers decide when a claimed window closes, and today two fields spread -> across two kinds carry them under two unrelated names. `CommitRequest.spec.closeDelay` does two -> jobs, waiting for a window and collecting after the claim, and it cannot keep a window open past -> the target's own silence timer. The proposal gives each job one field, reuses `window` for the one -> rolling timer wherever it appears, and keeps today's fixed cutoff as the default. It extends the -> timer model already decided in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md); -> it does not replace it. +> The short version: three timers decide when a claimed window closes, and today two fields on two +> kinds carry them under unrelated names. `CommitRequest.spec.closeDelay` does two jobs, and it cannot +> keep a window open past the target's own silence timer. The proposal names every timing knob as a +> **timeout**, named for what runs out, and puts the two timers that close a commit window in one +> block with the same shape on both kinds: +> +> ```yaml +> # GitTarget # CommitRequest +> spec: spec: +> commit: waitFor: +> windowTimeouts: timeout: 2s # give up if no window arrives +> idle: 5s windowTimeouts: # replaces the target's, for the window it claims +> max: 10m idle: 1s +> max: 30s +> ``` +> +> Both `closeDelay` and the string `commit.window` go away, each kept for one release as a loud +> rejection. It extends the timer model already decided in +> [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md), which it does not replace. ## What decides when a window closes today -After #403, these are the timers and boundaries that end a commit window: +After #403: | What | Field | Starts from | Resets on a matching change? | Default | |---|---|---|---|---| @@ -28,121 +40,190 @@ first, `window: 0s`, and shutdown. The `windowFinalizeReason` values in ## What does not line up **One field, two jobs.** `closeDelay` bounds the wait for a window and, since #403, the collection -after the claim. Those two phases need different sizes. The wait covers the cluster's audit latency +after the claim. The two phases need different sizes. The wait covers the cluster's audit latency plus however long the client takes to make its first write. The collection covers the span of the -save's own writes. One number is either too long for one of them or too short for the other. +save's own writes. **A request cannot hold a window open longer than the target allows.** The target's silence timer still runs on a claimed window (`windowFinalizeReasonTimer` in the event loop). With the default -`window: 5s`, a `closeDelay: 30s` collects for at most 5 seconds of silence, not 30. The request -asked for a longer interval and the shorter one wins without saying so. +`window: 5s`, a `closeDelay: 30s` collects for at most 5 seconds of silence. The request asked for a +longer interval and the shorter one wins without saying so. **Nothing caps a busy window except memory.** The target's `window` is purely rolling: continuous -activity keeps a window open until `buffer-limit` trips. There is no time bound. A request gets a -fixed cutoff from `closeDelay`; a target cannot express one. +activity keeps a window open until `buffer-limit` trips. A request gets a fixed cutoff from +`closeDelay`; a target cannot express one. **The names do not say what the fields do.** A [commit window](../definitions.md) is the interval a -branch worker batches writes over. On the target, `window` is that interval's silence timeout. On -the request, the comparable timer is called `closeDelay`. In the worker's code and comments it is -called a "grace" (`collect grace`, `grace elapsed`), which collides with the -[grace window](../definitions.md) the definitions page reserves for the attribution wait. +branch worker batches writes over, but `commit.window` holds only its silence timeout. The request's +comparable timer is called `closeDelay`. In the worker's code it is called a "grace" (`collect +grace`, `grace elapsed`), which collides with the [grace window](../definitions.md) the definitions +page reserves for the attribution wait. **Omission and `2s` look the same.** The CRD fills an omitted `closeDelay` with `"2s"` on write, so -the controller cannot tell a request that chose `2s` from one that left it out. Any inherited default, -for example one set on the target, needs that distinction. +the controller cannot tell a request that chose `2s` from one that left it out. -## Constraints the next PR inherits +## Constraints - **The decided timer model.** [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md#the-timer-model) - already settled "give each field one job": a `waitFor.timeout` anchored at receipt as the give-up - bound, and `closeDelay` measured from satisfaction. #403 moved `closeDelay`'s collection anchor to - the claim, which is the same idea for a request that names no write. This proposal must land - inside that model. -- **The binding vocabulary.** [`definitions.md`](../definitions.md) owns field names. Flux is the - precedent for durations (`timeout`, `interval`), and every duration is a Go duration string with a - CEL bound ([API durations](../UPGRADING.md#every-duration-in-the-api-is-a-go-duration-string)). -- **An immutable spec.** A `CommitRequest`'s whole spec is immutable, and a request lives at most 48 - hours after it finishes. A field change reaches new requests only; there is no stored object to - migrate beyond that window. -- **The attach invariant.** A request finalizes its own submitter's work and nobody else's. No - timer change may widen what a request can claim. + settled "give each field one job": a `waitFor.timeout` anchored at receipt as the give-up bound, + and a collection measured from satisfaction. For a request that names no write, satisfaction is + the claim. +- **The binding vocabulary.** [`definitions.md`](../definitions.md): one concept keeps one word + everywhere (rule 1), a rename removes the old spelling instead of aliasing it, and two or more + facts about one subject may form a block (rule 2). `total` and other count words are taken + (rule 3). Flux is the precedent for durations (`timeout`, `interval`), and every duration is a Go + duration string with a CEL bound. +- **No type change under a stored name.** `commit.window` is stored as a string. Redefining that name + as an object leaves stored GitTargets that no typed client can decode, which breaks GET and LIST + for the whole kind. That is the failure the duration CEL bounds exist to prevent. The new shape + needs a new name. [`crd-upgrade-strategies.md`](../facts/crd-upgrade-strategies.md) has the two + honest options for the old one. +- **An immutable request spec.** A `CommitRequest`'s whole spec is immutable, and a request lives at + most 48 hours after it finishes. +- **The attach invariant.** A request finalizes its own submitter's work and nobody else's. No timer + change may widen what a request can claim. + +## Naming the timers + +### Where the words come from + +Every system that batches has the same two timers, and most name them after one of three +traditions: + +| Tradition | Rolling timer (resets on activity) | Fixed cap | Waiting for the batch to start | +|---|---|---|---| +| Debounce (lodash `debounce`, RxJS) | `wait` | `maxWait` | n/a | +| Network proxies (Envoy, NGINX, HTTP clients) | `idleTimeout` | `maxDuration`, `maxConnectionDuration` | `connectTimeout` | +| Batch exporters (OpenTelemetry, Kafka) | n/a | `timeout`, `linger.ms` | n/a | +| Alert grouping (Alertmanager) | `group_interval` | n/a | `group_wait` | +| Kubernetes | none rolling | `activeDeadlineSeconds`, `progressDeadlineSeconds` | n/a | +| CI triggers (Jenkins) | `quietPeriod` | n/a | n/a | +| Flux | none rolling | `timeout` | n/a | + +The proxy tradition is the only one that covers both timers with words most operators already know, +and "timeout" is also Flux's word. "Idle" is universally understood as "resets on activity". +"Wait" is taken by `waitFor`, so the debounce words would collide. "Deadline" is Kubernetes' word +for a fixed cap, but it reads as an absolute point in time, not a duration. + +### Candidates + +| # | GitTarget | CommitRequest | For | Against | +|---|---|---|---|---| +| A | `commit.windowTimeouts.{idle, max}` | `windowTimeouts.{idle, max}` | keeps "window" (rule 1); every timing knob is a timeout; one shape on both kinds | `max` alone is terse; a plural block name is unusual | +| B | `commit.window.{idleTimeout, maxDuration}` | `window.{idleTimeout, maxDuration}` | the cleanest YAML; Envoy's exact words | a type change under a stored name: ruled out by the constraints | +| C | `commit.idleTimeout`, `commit.maxWindowDuration` (flat) | `idleTimeout`, `maxWindowDuration` | no block; rule 2 allows two flat fields | "idle" of what? The request's pair loses its subject, and the two kinds stop looking alike | +| D | `commit.batching.{idleTimeout, maxDuration}` | `batching.{…}` | reads naturally | drops "window", the defined word for the concept (rule 1) | +| E | `commit.windowClose.{afterIdle, afterOpen}` | `windowClose.{…}` | says what the timers do | `afterOpen` is wrong for the request, whose cap runs from the claim; verbs make poor field names | +| F | `commit.window.{idle, max}` in a new API version | same | B's shape | needs a conversion webhook for one field; [not a real option](../facts/crd-upgrade-strategies.md) | + +**Recommendation: A.** It is the only candidate that keeps the defined word, avoids the stored-type +trap, and gives both kinds the same block. `windowTimeouts.max` reads fine in context: the +block name says it is a timeout, and `max` says which one. The alternatives for `max`: `total` +is a count word (rule 3), `hard` is jargon, and `cap` is less known. `idle` stays as it is; it is +the one word every reader already maps to "resets on activity". + +With A, the request's `waitFor.timeout` completes a single family: every timing knob on both kinds +is a timeout, named for what runs out. ## Proposal -Three concepts, one name each, and the same name wherever the concept appears: +### The fields -| Concept | Field | Starts from | Resets? | Default | +| Field | Starts from | Resets? | Default | Bound | |---|---|---|---|---| -| Wait timeout | `CommitRequest.spec.waitFor.timeout` | receipt | no | the value of `closeDelay`, which is today's behaviour | -| Rolling collection | `CommitRequest.spec.window` | the claim, then each matching event | yes | omitted: no rolling collection | -| Collection cutoff | `CommitRequest.spec.closeDelay` | the claim | no | `2s` | - -**`waitFor.timeout`** is the field the save-wait design already names, shipped first without its -resource reference. A request with only a timeout waits for any matching window, as today. It takes -over the waiting job from `closeDelay`. While it is omitted it defaults to the request's -`closeDelay`, so a request written for 0.50 behaves the same. - -**`window` on a request** has exactly the meaning `GitTarget.spec.commit.window` has: the silence -after which the window closes. For a claimed window it **replaces** the target's silence timer, so a -request can ask for a longer interval, or a shorter one, and get it. Author or target changes, -`buffer-limit`, drain-before-apply and shutdown remain boundaries; the request changes only the -timer. - -**`closeDelay`** keeps its name and ends up meaning exactly what the name says: close this long -after the claim, whatever arrives. With `window` set it is the cap that keeps continuous activity -from holding the window open. Without `window` it is today's fixed cutoff. - -**Keep the fixed cutoff the default.** Rolling collection groups a save whose writes trickle in, but -it has no predictable end. It should be something a caller asks for, bounded by `closeDelay`. -Making `closeDelay` itself reset on each change would lose the one bound a caller can rely on. - -### What stays out of this PR - -- **A time cap on the target's own window** (`GitTarget.spec.commit.maxWindow` or similar). The - table shows the gap, but nobody has hit it; `buffer-limit` covers the memory risk. It can reuse the - request's vocabulary later if it is needed. -- **Inherited defaults** (a target-level or cluster-level default for request timing). That needs - omission to be distinguishable, which means moving the `2s` default out of the schema and into the - controller. Do that in the PR that adds the inherited default, not before. Until then the schema - default is the more readable contract: `kubectl get -o yaml` shows the value that applied. -- **The named-write wait** (`waitFor` with a resource reference). That is phase 2 of the save-wait - design and depends on its phase 1. - -### Status - -The single progress reason `WaitingForCloseDelay` covers both phases today. Split it so a stuck -request says which phase it is in: `WaitingForWindow` before the claim, and `Collecting` after it. -Both are progress reasons under `Reconciling=True`, following -[`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). Rename "grace" to "wait" or -"collection" in the worker's code and comments in the same PR, so the attribution grace window is -the only grace left. +| `GitTarget.spec.commit.windowTimeouts.idle` | each event | yes | `5s`; `0s` means a commit per event | `24h` | +| `GitTarget.spec.commit.windowTimeouts.max` | the window opening | no | omitted: no cap, as today | `24h` | +| `CommitRequest.spec.waitFor.timeout` | receipt by the branch worker | no | `2s` | `5m` | +| `CommitRequest.spec.windowTimeouts.idle` | the claim, then each matching event | yes | omitted: no idle close | `5m` | +| `CommitRequest.spec.windowTimeouts.max` | the claim | no | `2s` | `5m` | + +### The rules + +**The request's block replaces the target's, whole.** For the window a request claims, the target's +`idle` and `max` stop applying and the request's take over. There is no merging field by field. An +omitted request `idle` therefore means "no idle close": a request with only `max` is a pure fixed +cutoff, which is exactly today's `closeDelay`, without the target's silence timer cutting it short. +Author or target changes, `buffer-limit`, drain-before-apply and shutdown remain boundaries; a +request changes only the timers. + +**A rolling request needs a cap.** CEL on the request: `max` is required when `idle` is set, and +`idle` must not exceed `max`. Rolling collection groups a save whose writes trickle in, but it has +no predictable end, so it is always bounded. Letting `max` reset on each change would remove the one +bound a caller can rely on. + +**The fixed cutoff stays the default.** An omitted `windowTimeouts` on a request is `{max: 2s}`, +today's behaviour. Rolling collection is something a caller asks for. + +**The claim checks expiry first.** A request whose wait has run out when it claims keeps its expired +deadline and is finalized in the same pass, as #403 does. The boundary test carries over. + +**Waiting and collecting are separate numbers.** `waitFor.timeout` no longer borrows from the +collection value. A save that creates its request before its writes sets `waitFor.timeout` to the +gap before its first write, and `windowTimeouts` to the span of its writes. The worst case to the +finalize is the sum. + +### Migration + +Both old fields are retained for one release as loud rejections. Removing them outright is the +silent option: a re-applied GitTarget manifest or an integrator's save code would be accepted with +the value dropped, and the behaviour would change on every write. + +- `CommitRequest.spec.closeDelay`: CEL `!has(self.closeDelay)`, message "renamed: use + `windowTimeouts.max` for the cutoff and `waitFor.timeout` for the wait". A request is created + fresh for every save, so the integrator's code fails on its first save after the upgrade, with a + message that says what to change. +- `GitTarget.spec.commit.window`: the same CEL rule, pointing at `windowTimeouts.idle`. A stored + target keeps its value under validation ratcheting until someone edits it; the controller + **honours** a stored `window` as `windowTimeouts.idle` for that release and reports the rename + in a condition message, so an unedited target changes nothing on upgrade. +- UPGRADING gets one `jq` query per field listing the objects still carrying it, which the 0.50 + field report singled out as the part of the guide that worked. + +### Status, code and metrics + +- **Reasons.** `WaitingForCloseDelay` splits into `WaitingForWindow` (before the claim) and + `CollectingWindow` (after it), both progress reasons under `Reconciling=True`, following + [`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). +- **Code.** "Grace" leaves the commit path: `finalizeAt` becomes a wait deadline and a close + deadline, and comments say wait or collection. The attribution grace window is then the only grace. +- **Metrics.** The `timer` finalize reason splits into `idle-timeout` and `max-timeout`, and a + request's cutoff reports `max-timeout` instead of `finalize-signal`. That changes label values, so + [`interpreting-metrics.md`](../interpreting-metrics.md) and UPGRADING both record it. + +### What stays out + +- **Inherited defaults** (a target-level default for its requests' timeouts). With "replace whole" + semantics a request never needs to know the target's values. If a default is wanted later, it + needs omission to be distinguishable, which means moving the `2s` out of the schema. Until then the + schema default is the more readable contract: `kubectl get -o yaml` shows the value that applied. +- **The named-write wait** (`waitFor` with a resource reference). Phase 2 of the save-wait design; + `waitFor.timeout` is shaped so it can arrive later without a rename. ## Plan -1. **API.** Add `spec.window` and `spec.waitFor.timeout` to `CommitRequest`, both optional, both Go - duration strings with the same pattern and CEL bounds as today (`window` at most `24h`, as on the - target; `timeout` at most `5m`). Additive, so no new API version. -2. **Worker.** Carry the three values on `AttachCommitRequest`. Stamp the wait deadline from - `waitFor.timeout`, falling back to `closeDelay`. On the claim, set the cutoff at claim + - `closeDelay`, and when `window` is set, arm the claimed window's silence timer from it instead of - the target's. The claim must still check expiry first; #403's boundary test covers that. -3. **Controller.** Resolve the fallback, split the progress reason, and size - `commitRequestResolveTimeout` from the largest wait plus the largest cutoff. -4. **Docs.** Replace the sizing section in [`configuration.md`](../configuration.md#sizing-closedelay) - with one paragraph per field, add the upgrade entry, and add a definition for the request's - collection to [`definitions.md`](../definitions.md). -5. **Tests.** Wait expiry versus claim on the same wake; writes arriving after the claim join one - commit; a request `window` longer than the target's keeps the window open; a request `window` - shorter than the target's closes it sooner; continuous activity stops at `closeDelay`; a change of - author still closes a claimed window. +1. **API.** Add `GitTarget.spec.commit.windowTimeouts` and `CommitRequest.spec.{waitFor, windowTimeouts}` + as one shared Go type for the block. Retain `commit.window` and `closeDelay` as refusals. +2. **Worker.** Carry a resolved `windowTimeouts` on each open window. Arm the idle timer and the max + timer from it; on a claim, swap in the request's. Stamp the wait deadline from `waitFor.timeout`. +3. **Controller.** Honour a stored `commit.window` for one release, split the progress reason, and + size `commitRequestResolveTimeout` from the largest wait plus the largest cutoff. +4. **Docs.** One paragraph per field in [`configuration.md`](../configuration.md), a definition for + "window timeouts" in [`definitions.md`](../definitions.md), UPGRADING with the queries, and the + metrics label change. +5. **Tests.** Wait expiry versus claim on the same wake; writes after the claim join one commit; a + request `idle` longer than the target's keeps the window open; a shorter one closes it sooner; a + request with only `max` is not closed by the target's idle timer; continuous activity stops at + `max` on both kinds; a change of author still closes a claimed window; the retained fields are + refused with their message; a stored `commit.window` keeps working. ## Open questions -- Should a request `window` also be allowed to shorten collection below the target's, or only lengthen - it? Replacing both ways is simpler to explain; lengthening only is harder to misuse. -- Is `closeDelay` still the right name once it is only the cutoff, or should it become `maxWindow`, - mirroring a future target field? Renaming costs one more migration for every integrator. Keeping it - keeps a name that now means what it says. -- Does `waitFor.timeout` defaulting to `closeDelay` read as a surprise once the two are separate, or - should it default to its own `2s`? The fallback keeps 0.50 requests identical; a fixed default is - easier to document. +- **`max` or something longer?** `windowTimeouts.max` is short and reads well in YAML, but a lone + `max` is unusual in Kubernetes APIs. `maxDuration` is Envoy's word and removes the ambiguity, at + the cost of `windowTimeouts.maxDuration` reading twice as long as it needs to. +- **May a request shorten as well as lengthen?** "Replace whole" lets a request close a window + sooner than the target would. That is the simpler rule to explain; the alternative, lengthen only, + is harder to misuse but needs a merge rule. +- **Should a target get `max` in the same PR?** It shares the type and the timer code with the + request, so it is nearly free, but nobody has hit the gap yet. Shipping it together keeps the two + kinds' blocks identical from day one. From 56a2b0ca43adff2c6ca3f439729624cb37d1893f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:50:01 +0000 Subject: [PATCH 09/15] docs(design): one window block for targets and saves, with attach and emptyCommit Only writes open a commit window; a save attaches, never opens, and replaces the target's idle and max timers for the window it attaches to, longer or shorter. The save gains attach (Current or Next) with an attachTimeout, and emptyCommit (Skip or Create) so every save can end in one commit. With migration no longer driving the names, the block is simply window, and the migration note shrinks to a corner. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 2 +- docs/design/commit-timing-surface.md | 387 ++++++++++++++------------- 2 files changed, 197 insertions(+), 192 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index adcbc9b2..66cdc44c 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -139,7 +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.** The follow-up to #403: `closeDelay` both waits for a window and collects after the claim, and cannot outlast the target's silence timer. Proposes one `windowTimeouts: {idle, max}` block shared by `GitTarget.spec.commit` and `CommitRequest.spec` (the request's replaces the target's for the window it claims), `waitFor.timeout` for the wait, and retiring `closeDelay` and the string `commit.window` as loud rejections. Weighs six naming candidates. Open: `max` or `maxDuration`, whether a request may shorten a window, and whether the target's `max` ships in the same PR | +| [`commit-timing-surface.md`](design/commit-timing-surface.md) | **design.** One configuration surface for commit windows and saves: only writes open a window; a `window` block with `idleTimeout` and `maxDuration` on both kinds, which a `CommitRequest` replaces whole for the window it attaches to; plus `attach: Current \| Next`, `attachTimeout`, and `emptyCommit: Skip \| Create` so every save can produce one commit. Replaces `closeDelay` and the string `commit.window`. Open: the `attach` value names, `emptyCommit` on `WindowMismatch`, and whether the target's `maxDuration` ships first | | [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index 9b759fe4..b24ba1a4 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -1,229 +1,234 @@ -# Commit timing: one vocabulary for the target and the save +# 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: three timers decide when a claimed window closes, and today two fields on two -> kinds carry them under unrelated names. `CommitRequest.spec.closeDelay` does two jobs, and it cannot -> keep a window open past the target's own silence timer. The proposal names every timing knob as a -> **timeout**, named for what runs out, and puts the two timers that close a commit window in one -> block with the same shape on both kinds: +> The short version: a commit window is opened only by writes, and it closes on two timers. A save +> (`CommitRequest`) attaches to one window and may replace both timers for it. Both kinds describe +> the window in one block with the same words, and the save adds how it attaches and whether it +> commits when nothing came in: > > ```yaml -> # GitTarget # CommitRequest -> spec: spec: -> commit: waitFor: -> windowTimeouts: timeout: 2s # give up if no window arrives -> idle: 5s windowTimeouts: # replaces the target's, for the window it claims -> max: 10m idle: 1s -> max: 30s +> # GitTarget # CommitRequest +> spec: spec: +> commit: window: +> window: attach: Current # or Next +> idleTimeout: 5s attachTimeout: 2s +> maxDuration: 10m idleTimeout: 1s +> maxDuration: 30s +> emptyCommit: Skip # or Create > ``` > -> Both `closeDelay` and the string `commit.window` go away, each kept for one release as a loud -> rejection. It extends the timer model already decided in -> [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md), which it does not replace. +> It replaces `CommitRequest.spec.closeDelay` and the string `GitTarget.spec.commit.window`, and it +> builds on the timer model decided in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md). ## What decides when a window closes today After #403: -| What | Field | Starts from | Resets on a matching change? | Default | +| What | Field | Starts from | Resets on a matching write? | Default | |---|---|---|---|---| -| Target silence | `GitTarget.spec.commit.window` | each event | yes | `5s`, at most `24h` | +| Target silence | `GitTarget.spec.commit.window` | each write | yes | `5s`, at most `24h` | | Request wait | `CommitRequest.spec.closeDelay` | receipt by the branch worker | no | `2s`, at most `5m` | -| Request collection | the same `closeDelay` | the claim | no | the same value | +| Request collection | the same `closeDelay` | the attach | no | the same value | Other boundaries close a window regardless of any timer: a change of author or target, the branch buffer's memory limit (`buffer-limit`), a resync or atomic apply that needs the window drained -first, `window: 0s`, and shutdown. The `windowFinalizeReason` values in +first, and shutdown. The `windowFinalizeReason` values in [`branch_worker.go`](../../internal/git/branch_worker.go) name them all. ## What does not line up -**One field, two jobs.** `closeDelay` bounds the wait for a window and, since #403, the collection -after the claim. The two phases need different sizes. The wait covers the cluster's audit latency -plus however long the client takes to make its first write. The collection covers the span of the -save's own writes. - -**A request cannot hold a window open longer than the target allows.** The target's silence timer -still runs on a claimed window (`windowFinalizeReasonTimer` in the event loop). With the default -`window: 5s`, a `closeDelay: 30s` collects for at most 5 seconds of silence. The request asked for a -longer interval and the shorter one wins without saying so. - -**Nothing caps a busy window except memory.** The target's `window` is purely rolling: continuous -activity keeps a window open until `buffer-limit` trips. A request gets a fixed cutoff from -`closeDelay`; a target cannot express one. - -**The names do not say what the fields do.** A [commit window](../definitions.md) is the interval a -branch worker batches writes over, but `commit.window` holds only its silence timeout. The request's -comparable timer is called `closeDelay`. In the worker's code it is called a "grace" (`collect -grace`, `grace elapsed`), which collides with the [grace window](../definitions.md) the definitions -page reserves for the attribution wait. - -**Omission and `2s` look the same.** The CRD fills an omitted `closeDelay` with `"2s"` on write, so -the controller cannot tell a request that chose `2s` from one that left it out. - -## Constraints - -- **The decided timer model.** [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md#the-timer-model) - settled "give each field one job": a `waitFor.timeout` anchored at receipt as the give-up bound, - and a collection measured from satisfaction. For a request that names no write, satisfaction is - the claim. -- **The binding vocabulary.** [`definitions.md`](../definitions.md): one concept keeps one word - everywhere (rule 1), a rename removes the old spelling instead of aliasing it, and two or more - facts about one subject may form a block (rule 2). `total` and other count words are taken - (rule 3). Flux is the precedent for durations (`timeout`, `interval`), and every duration is a Go - duration string with a CEL bound. -- **No type change under a stored name.** `commit.window` is stored as a string. Redefining that name - as an object leaves stored GitTargets that no typed client can decode, which breaks GET and LIST - for the whole kind. That is the failure the duration CEL bounds exist to prevent. The new shape - needs a new name. [`crd-upgrade-strategies.md`](../facts/crd-upgrade-strategies.md) has the two - honest options for the old one. -- **An immutable request spec.** A `CommitRequest`'s whole spec is immutable, and a request lives at - most 48 hours after it finishes. -- **The attach invariant.** A request finalizes its own submitter's work and nobody else's. No timer - change may widen what a request can claim. - -## Naming the timers - -### Where the words come from - -Every system that batches has the same two timers, and most name them after one of three -traditions: - -| Tradition | Rolling timer (resets on activity) | Fixed cap | Waiting for the batch to start | +**One field, two jobs.** `closeDelay` bounds both the wait for a window and the collection after +the attach. The two need different sizes. The wait covers the cluster's audit latency plus the +client's time to its first write; the collection covers the span of the save's own writes. + +**A save cannot hold a window open longer than the target allows.** The target's silence timer +still runs on an attached window. With the default `5s`, a `closeDelay: 30s` collects for at most 5 +seconds of silence, and nothing says so. + +**Nothing caps a busy window except memory.** The target's timer only resets. Continuous activity +keeps a window open until `buffer-limit` trips. + +**A save cannot choose which window it covers.** It always takes the window open at receipt, which +may hold the author's earlier, unrelated edits. + +**A save can end with no commit, three ways.** `NoWindowInGrace`, `WindowMismatch` and +`AlreadyPresent` all resolve `Ready=True` with nothing in Git. A UI that shows "saved as `abc123`" +must handle each one. + +**The words do not match.** The target's timer is called `window`, though it holds only the +window's silence timeout. The save's is `closeDelay`. The worker's code calls it a "grace", which +[`definitions.md`](../definitions.md) reserves for the attribution wait. The code says both +"attach" and "claim" for the same act, and "claimed" already names something on the read side. + +## Principles + +1. **Only writes open a window.** A commit window opens on the first write of an author to a target + and collects that author's writes until it closes. Nothing else opens one: not a save, not a + timer. This is today's behaviour, stated as a rule so the save's settings can lean on it. +2. **A save attaches; it never opens.** It waits for a window it may attach to, attaches to at most + one, and gives that window its message and its timers. "Attach" is the one word for this, 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. Author or target changes, `buffer-limit`, drain-before-apply and shutdown remain + boundaries; a save changes only the timers. +4. **Every timer is named for what runs out.** `attachTimeout`, `idleTimeout`, `maxDuration`. +5. **Choices are enums, not booleans.** Per the Kubernetes API conventions: a boolean that later + needs a third state is a breaking change, and an enum value names the behaviour it selects. +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. + +## The surface + +### `GitTarget.spec.commit.window` + +| Field | Meaning | Default | Bound | |---|---|---|---| -| Debounce (lodash `debounce`, RxJS) | `wait` | `maxWait` | n/a | -| Network proxies (Envoy, NGINX, HTTP clients) | `idleTimeout` | `maxDuration`, `maxConnectionDuration` | `connectTimeout` | -| Batch exporters (OpenTelemetry, Kafka) | n/a | `timeout`, `linger.ms` | n/a | -| Alert grouping (Alertmanager) | `group_interval` | n/a | `group_wait` | -| Kubernetes | none rolling | `activeDeadlineSeconds`, `progressDeadlineSeconds` | n/a | -| CI triggers (Jenkins) | `quietPeriod` | n/a | n/a | -| Flux | none rolling | `timeout` | n/a | +| `idleTimeout` | close after this much silence; resets on each write | `5s`; `0s` commits every write on its own | `24h` | +| `maxDuration` | close this long after the window opened, whatever arrives | none | `24h` | + +### `CommitRequest.spec.window` -The proxy tradition is the only one that covers both timers with words most operators already know, -and "timeout" is also Flux's word. "Idle" is universally understood as "resets on activity". -"Wait" is taken by `waitFor`, so the debounce words would collide. "Deadline" is Kubernetes' word -for a fixed cap, but it reads as an absolute point in time, not a duration. +| Field | Meaning | Default | Bound | +|---|---|---|---| +| `attach` | which window the save covers: `Current` or `Next` (below) | `Current` | enum | +| `attachTimeout` | stop waiting for a window this long after receipt | `2s` | `5m` | +| `idleTimeout` | close the attached window after this much silence | none: no idle close | `5m` | +| `maxDuration` | close the attached window this long after the attach | `2s` | `5m` | -### Candidates +CEL: `idleTimeout` must not exceed `maxDuration`. The defaults give today's fixed cutoff: no idle +close, close 2 seconds after the attach. A save that wants rolling collection sets `idleTimeout`, +and `maxDuration` still ends it. -| # | GitTarget | CommitRequest | For | Against | -|---|---|---|---|---| -| A | `commit.windowTimeouts.{idle, max}` | `windowTimeouts.{idle, max}` | keeps "window" (rule 1); every timing knob is a timeout; one shape on both kinds | `max` alone is terse; a plural block name is unusual | -| B | `commit.window.{idleTimeout, maxDuration}` | `window.{idleTimeout, maxDuration}` | the cleanest YAML; Envoy's exact words | a type change under a stored name: ruled out by the constraints | -| C | `commit.idleTimeout`, `commit.maxWindowDuration` (flat) | `idleTimeout`, `maxWindowDuration` | no block; rule 2 allows two flat fields | "idle" of what? The request's pair loses its subject, and the two kinds stop looking alike | -| D | `commit.batching.{idleTimeout, maxDuration}` | `batching.{…}` | reads naturally | drops "window", the defined word for the concept (rule 1) | -| E | `commit.windowClose.{afterIdle, afterOpen}` | `windowClose.{…}` | says what the timers do | `afterOpen` is wrong for the request, whose cap runs from the claim; verbs make poor field names | -| F | `commit.window.{idle, max}` in a new API version | same | B's shape | needs a conversion webhook for one field; [not a real option](../facts/crd-upgrade-strategies.md) | +### `CommitRequest.spec.emptyCommit` -**Recommendation: A.** It is the only candidate that keeps the defined word, avoids the stored-type -trap, and gives both kinds the same block. `windowTimeouts.max` reads fine in context: the -block name says it is a timeout, and `max` says which one. The alternatives for `max`: `total` -is a count word (rule 3), `hard` is jargon, and `cap` is less known. `idle` stays as it is; it is -the one word every reader already maps to "resets on activity". +`Skip` (default) or `Create`. It sits outside `window` because it decides the commit, not the +window. -With A, the request's `waitFor.timeout` completes a single family: every timing knob on both kinds -is a timeout, named for what runs out. +### `attach: Current` or `Next` -## Proposal +- **`Current`** attaches to the author's window if one is open at receipt, and otherwise to the next + one that opens. This is today's behaviour, and 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 a window open at receipt, the worker closes it first, + under the target's ordinary message, and the save attaches to the next window. This is for a save + created *before* its writes, which should not sweep up the author's earlier, unrelated edits. -### The fields +`Next` means "reaches the branch worker after receipt", not "was made after the request". A write +still held for its audit fact when the request arrives lands in the next window. The docs must say +so, and the sizing advice covers it: a save that needs a hard line should create its request only +after the previous save resolved. -| Field | Starts from | Resets? | Default | Bound | -|---|---|---|---|---| -| `GitTarget.spec.commit.windowTimeouts.idle` | each event | yes | `5s`; `0s` means a commit per event | `24h` | -| `GitTarget.spec.commit.windowTimeouts.max` | the window opening | no | omitted: no cap, as today | `24h` | -| `CommitRequest.spec.waitFor.timeout` | receipt by the branch worker | no | `2s` | `5m` | -| `CommitRequest.spec.windowTimeouts.idle` | the claim, then each matching event | yes | omitted: no idle close | `5m` | -| `CommitRequest.spec.windowTimeouts.max` | the claim | no | `2s` | `5m` | - -### The rules - -**The request's block replaces the target's, whole.** For the window a request claims, the target's -`idle` and `max` stop applying and the request's take over. There is no merging field by field. An -omitted request `idle` therefore means "no idle close": a request with only `max` is a pure fixed -cutoff, which is exactly today's `closeDelay`, without the target's silence timer cutting it short. -Author or target changes, `buffer-limit`, drain-before-apply and shutdown remain boundaries; a -request changes only the timers. - -**A rolling request needs a cap.** CEL on the request: `max` is required when `idle` is set, and -`idle` must not exceed `max`. Rolling collection groups a save whose writes trickle in, but it has -no predictable end, so it is always bounded. Letting `max` reset on each change would remove the one -bound a caller can rely on. - -**The fixed cutoff stays the default.** An omitted `windowTimeouts` on a request is `{max: 2s}`, -today's behaviour. Rolling collection is something a caller asks for. - -**The claim checks expiry first.** A request whose wait has run out when it claims keeps its expired -deadline and is finalized in the same pass, as #403 does. The boundary test carries over. - -**Waiting and collecting are separate numbers.** `waitFor.timeout` no longer borrows from the -collection value. A save that creates its request before its writes sets `waitFor.timeout` to the -gap before its first write, and `windowTimeouts` to the span of its writes. The worst case to the -finalize is the sum. - -### Migration - -Both old fields are retained for one release as loud rejections. Removing them outright is the -silent option: a re-applied GitTarget manifest or an integrator's save code would be accepted with -the value dropped, and the behaviour would change on every write. - -- `CommitRequest.spec.closeDelay`: CEL `!has(self.closeDelay)`, message "renamed: use - `windowTimeouts.max` for the cutoff and `waitFor.timeout` for the wait". A request is created - fresh for every save, so the integrator's code fails on its first save after the upgrade, with a - message that says what to change. -- `GitTarget.spec.commit.window`: the same CEL rule, pointing at `windowTimeouts.idle`. A stored - target keeps its value under validation ratcheting until someone edits it; the controller - **honours** a stored `window` as `windowTimeouts.idle` for that release and reports the rename - in a condition message, so an unedited target changes nothing on upgrade. -- UPGRADING gets one `jq` query per field listing the objects still carrying it, which the 0.50 - field report singled out as the part of the guide that worked. - -### Status, code and metrics - -- **Reasons.** `WaitingForCloseDelay` splits into `WaitingForWindow` (before the claim) and - `CollectingWindow` (after it), both progress reasons under `Reconciling=True`, following - [`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). -- **Code.** "Grace" leaves the commit path: `finalizeAt` becomes a wait deadline and a close - deadline, and comments say wait or collection. The attribution grace window is then the only grace. -- **Metrics.** The `timer` finalize reason splits into `idle-timeout` and `max-timeout`, and a - request's cutoff reports `max-timeout` instead of `finalize-signal`. That changes label values, so - [`interpreting-metrics.md`](../interpreting-metrics.md) and UPGRADING both record it. +### `emptyCommit: Skip` or `Create` -### What stays out +With `Create`, a save always produces one commit carrying its message: -- **Inherited defaults** (a target-level default for its requests' timeouts). With "replace whole" - semantics a request never needs to know the target's values. If a default is wanted later, it - needs omission to be distinguishable, which means moving the `2s` out of the schema. Until then the - schema default is the more readable contract: `kubectl get -o yaml` shows the value that applied. -- **The named-write wait** (`waitFor` with a resource reference). Phase 2 of the save-wait design; - `waitFor.timeout` is shaped so it can arrive later without a rename. +| Situation | `Skip` (today) | `Create` | +|---|---|---| +| Writes arrived and changed files | commit | commit | +| Nothing arrived before `attachTimeout` | `NoWindowInGrace`, no commit | empty commit | +| Writes arrived but the files already matched | `AlreadyPresent`, no commit | empty commit | +| Only another author's window was open | `WindowMismatch`, no commit | empty commit | + +The empty commit is not a window: principle 1 still holds, and the commit holds nothing but the +message. Its author is the request's submitter when admission captured one, otherwise the +committer, the same rule as any other commit. The reason stays specific (`CommittedEmpty`), so a +caller can still tell "saved your changes" from "recorded your message". Empty commits already +exist in this codebase: #383 reverts refused edits with one. + +This is what makes a save a reliable primitive for an integrator: every successful save has a +commit hash to show. The cost is noise in history for saves that change nothing, which is why it is +opt-in. + +## Naming, and why these words + +### Precedents + +Every system that batches has the same two timers, and a few also have a wait for the batch to +start: + +| Tradition | Rolling timer (resets on activity) | Fixed cap | Waiting to start | +|---|---|---|---| +| Network proxies (Envoy, Gateway API, NGINX) | `idleTimeout` | `maxDuration`, `maxConnectionDuration` | `connectTimeout` | +| Debounce (lodash, RxJS) | `wait` | `maxWait` | n/a | +| Batch exporters (OpenTelemetry, Kafka) | n/a | `timeout`, `linger.ms` | n/a | +| Alert grouping (Alertmanager) | `group_interval` | n/a | `group_wait` | +| Kubernetes | n/a | `activeDeadlineSeconds` | n/a | +| Flux | n/a | `timeout` | n/a | + +The proxy words are the ones operators already know, and "timeout" is Flux's word as well. "Idle" +means "resets on activity" to almost every reader. "Deadline" reads as a point in time, not a +duration. The debounce words collide with "wait" as a verb. + +### Decisions + +- **The block is `window`,** the word [`definitions.md`](../definitions.md) already defines. The + timers are properties of the window, so they live in it (rule 1: one concept, one word). +- **Full field names:** `idleTimeout` and `maxDuration` rather than `idle` and `max`. Inside a + `window` block nothing stutters, and a lone `max` is unusual in Kubernetes APIs. +- **`attach` for the act, on both the enum and its timer.** The code's "claim" goes, because + "claimed" already means a rule's demand on the read side. +- **The same block shape on both kinds.** The save's block is a superset: `attach` and + `attachTimeout` have no meaning on a target, which has nothing to wait for. + +Rejected along the way: `windowTimeouts.{idle, max}`, which existed only to dodge the stored +string; flat `idleTimeout` on `commit`, which loses its subject; `batching`, which drops the defined +word; `closeDelay` kept as the cutoff, which describes an action rather than a duration. + +## Status, code and metrics + +- **Reasons.** `WaitingForCloseDelay` splits into `WaitingForWindow` (before the attach) and + `CollectingWindow` (after it). `CommittedEmpty` joins the terminal reasons. All follow + [`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). +- **Code.** "Grace" and "claim" leave the commit path. `finalizeAt` becomes an attach deadline and a + close deadline. +- **Metrics.** The `timer` finalize reason splits into `idle-timeout` and `max-duration`, a save's + cutoff reports `max-duration` instead of `finalize-signal`, and `attach: Next` closing a window + gets its own reason. [`interpreting-metrics.md`](../interpreting-metrics.md) records the labels. +- **Definitions.** [`definitions.md`](../definitions.md) gains the rule that only writes open a + window, and "attach". + +## Out of scope + +- **Inherited defaults** (a target-level default for its saves' timers). With "replace whole", a save + never needs the target's values. +- **The named-write wait** (`waitFor` with a resource reference): phase 2 of the save-wait design. + With this surface, `waitFor` names *which* write, and `window.attachTimeout` stays the one bound on + waiting for it. ## Plan -1. **API.** Add `GitTarget.spec.commit.windowTimeouts` and `CommitRequest.spec.{waitFor, windowTimeouts}` - as one shared Go type for the block. Retain `commit.window` and `closeDelay` as refusals. -2. **Worker.** Carry a resolved `windowTimeouts` on each open window. Arm the idle timer and the max - timer from it; on a claim, swap in the request's. Stamp the wait deadline from `waitFor.timeout`. -3. **Controller.** Honour a stored `commit.window` for one release, split the progress reason, and - size `commitRequestResolveTimeout` from the largest wait plus the largest cutoff. -4. **Docs.** One paragraph per field in [`configuration.md`](../configuration.md), a definition for - "window timeouts" in [`definitions.md`](../definitions.md), UPGRADING with the queries, and the - metrics label change. -5. **Tests.** Wait expiry versus claim on the same wake; writes after the claim join one commit; a - request `idle` longer than the target's keeps the window open; a shorter one closes it sooner; a - request with only `max` is not closed by the target's idle timer; continuous activity stops at - `max` on both kinds; a change of author still closes a claimed window; the retained fields are - refused with their message; a stored `commit.window` keeps working. +1. **API.** `GitTarget.spec.commit.window` becomes the block; `CommitRequest.spec` gains `window` + and `emptyCommit` and drops `closeDelay`. +2. **Worker.** Carry resolved timers on each open window and arm an idle and a max timer from them; + on attach, swap in the save's. Stamp the attach deadline from `attachTimeout`. `Next` closes the + author's open window at receipt. `Create` commits empty with the message on each no-commit + outcome. +3. **Controller.** Split the progress reason, add `CommittedEmpty`, and size + `commitRequestResolveTimeout` from the largest `attachTimeout` plus the largest `maxDuration`. +4. **Docs.** One paragraph per field in [`configuration.md`](../configuration.md), the definitions, + the metric labels, and the migration note. +5. **Tests.** Attach expiry versus a write on the same wake; writes after the attach join one commit; + 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; an + author change still closes an attached window; `Next` commits the earlier window separately under + its ordinary message; `Create` produces one empty commit for each no-commit situation above. ## Open questions -- **`max` or something longer?** `windowTimeouts.max` is short and reads well in YAML, but a lone - `max` is unusual in Kubernetes APIs. `maxDuration` is Envoy's word and removes the ambiguity, at - the cost of `windowTimeouts.maxDuration` reading twice as long as it needs to. -- **May a request shorten as well as lengthen?** "Replace whole" lets a request close a window - sooner than the target would. That is the simpler rule to explain; the alternative, lengthen only, - is harder to misuse but needs a merge rule. -- **Should a target get `max` in the same PR?** It shares the type and the timer code with the - request, so it is nearly free, but nobody has hit the gap yet. Shipping it together keeps the two - kinds' blocks identical from day one. +- **`Current` and `Next`, or other values?** `Current` hides "or the next one if none is open". + `OpenOrNext` is exact but clumsy. +- **Should `emptyCommit: Create` apply to `WindowMismatch`?** It records a message while another + author's work is still in flight. That is honest, since the commit carries none of their work, but + it may confuse a reader of the history. +- **Does a target need `maxDuration` in the first PR?** It shares the timer code with the save, so + it is nearly free, but nobody has asked for it. + +## Migration (small) + +There are no production users outside our control, so this is a note, not a strategy. Both removed +fields are **deleted outright**. A stored `GitTarget` whose `commit.window` is still a string cannot +be decoded after the CRD changes, so it is patched to the block before the upgrade (one `jq` +query in UPGRADING lists them). `closeDelay: X` becomes `window.maxDuration: X`, plus +`attachTimeout: X` for a save that relied on the wait. From 907bf94414c1cfeb16347721cca6dad8c366449b Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 10:54:49 +0000 Subject: [PATCH 10/15] docs(design): record that a save never opens a commit window Co-Authored-By: Claude Opus 5.5 --- docs/design/commit-timing-surface.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index b24ba1a4..2c031bd5 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -63,9 +63,16 @@ window's silence timeout. The save's is `closeDelay`. The worker's code calls it ## Principles -1. **Only writes open a window.** A commit window opens on the first write of an author to a target - and collects that author's writes until it closes. Nothing else opens one: not a save, not a - timer. This is today's behaviour, stated as a rule so the save's settings can lean on it. +1. **Only writes open a window. Decided.** A commit window opens on the first write of an author to + a target and collects that author's writes until it closes. Nothing else opens one: not a save, + not a timer. This is today's behaviour, stated as a rule so the save's settings can lean on it. + + The alternative was weighed and rejected: a save that opens its own window, so the user gets a + window to respond into. It would have to cut off whatever window is open at that moment, it would + create windows that contain no writes, and it would give a save two ways to start collecting + instead of one. What it was for is covered without it: `attach: Next` gives a save a fresh start, + `attachTimeout` gives the user time to make the first write, and `emptyCommit: Create` records + the message when nothing comes. 2. **A save attaches; it never opens.** It waits for a window it may attach to, attaches to at most one, and gives that window its message and its timers. "Attach" is the one word for this, in the API, the code and the docs. From ebe6e14010d00a8730fdbf38cefa61171c3be196 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 11:11:58 +0000 Subject: [PATCH 11/15] docs(design): rework the timing surface as one v1alpha4, around the review The consolidated review drops the compatibility weight: window is an object on both kinds, the target gets a finite maxDuration, and the save's timers replace the target's in both directions. It adds what the draft was missing: precise clock and zero rules, attach before a zero idle timer fires, CurrentOrNext, Next's once-per-save close and first-come order, empty commits that keep their cause and never mask a mismatch, and the documented duplicate-after-restart limit. Migration shrinks to a note. The save-wait timer model now points here. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 2 +- docs/design/commit-timing-surface.md | 403 ++++++++++-------- .../design/commitrequest-save-wait-options.md | 5 + 3 files changed, 222 insertions(+), 188 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 66cdc44c..7f8aa773 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -139,7 +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: only writes open a window; a `window` block with `idleTimeout` and `maxDuration` on both kinds, which a `CommitRequest` replaces whole for the window it attaches to; plus `attach: Current \| Next`, `attachTimeout`, and `emptyCommit: Skip \| Create` so every save can produce one commit. Replaces `closeDelay` and the string `commit.window`. Open: the `attach` value names, `emptyCommit` on `WindowMismatch`, and whether the target's `maxDuration` ships first | +| [`commit-timing-surface.md`](design/commit-timing-surface.md) | **design.** One configuration surface for commit windows and saves, shipped as one `v1alpha4`: 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 `emptyCommit: Create` that keeps the cause in the reason. Precise clock and zero rules. Supersedes the save-wait timer model. Open: the target's `maxDuration` value, and what else rides the version bump | | [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index 2c031bd5..8ef371f6 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -2,89 +2,63 @@ > **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 it closes on two timers. A save -> (`CommitRequest`) attaches to one window and may replace both timers for it. Both kinds describe -> the window in one block with the same words, and the save adds how it attaches and whether it -> commits when nothing came in: +> 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 ships as one coherent +> `v1alpha4`: > > ```yaml -> # GitTarget # CommitRequest -> spec: spec: -> commit: window: -> window: attach: Current # or Next -> idleTimeout: 5s attachTimeout: 2s -> maxDuration: 10m idleTimeout: 1s -> maxDuration: 30s -> emptyCommit: Skip # or Create +> apiVersion: configbutler.ai/v1alpha4 apiVersion: configbutler.ai/v1alpha4 +> 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 +> emptyCommit: Skip # or Create > ``` > > It replaces `CommitRequest.spec.closeDelay` and the string `GitTarget.spec.commit.window`, and it -> builds on the timer model decided in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md). +> supersedes the timer model in [`commitrequest-save-wait-options.md`](commitrequest-save-wait-options.md#the-timer-model). -## What decides when a window closes today +## Why -After #403: - -| What | Field | Starts from | Resets on a matching write? | Default | -|---|---|---|---|---| -| Target silence | `GitTarget.spec.commit.window` | each write | yes | `5s`, at most `24h` | -| Request wait | `CommitRequest.spec.closeDelay` | receipt by the branch worker | no | `2s`, at most `5m` | -| Request collection | the same `closeDelay` | the attach | no | the same value | - -Other boundaries close a window regardless of any timer: a change of author or target, the branch -buffer's memory limit (`buffer-limit`), a resync or atomic apply that needs the window drained -first, and shutdown. The `windowFinalizeReason` values in -[`branch_worker.go`](../../internal/git/branch_worker.go) name them all. - -## What does not line up - -**One field, two jobs.** `closeDelay` bounds both the wait for a window and the collection after -the attach. The two need different sizes. The wait covers the cluster's audit latency plus the -client's time to its first write; the collection covers the span of the save's own writes. - -**A save cannot hold a window open longer than the target allows.** The target's silence timer -still runs on an attached window. With the default `5s`, a `closeDelay: 30s` collects for at most 5 -seconds of silence, and nothing says so. - -**Nothing caps a busy window except memory.** The target's timer only resets. Continuous activity -keeps a window open until `buffer-limit` trips. - -**A save cannot choose which window it covers.** It always takes the window open at receipt, which -may hold the author's earlier, unrelated edits. - -**A save can end with no commit, three ways.** `NoWindowInGrace`, `WindowMismatch` and -`AlreadyPresent` all resolve `Ready=True` with nothing in Git. A UI that shows "saved as `abc123`" -must handle each one. - -**The words do not match.** The target's timer is called `window`, though it holds only the -window's silence timeout. The save's is `closeDelay`. The worker's code calls it a "grace", which -[`definitions.md`](../definitions.md) reserves for the attribution wait. The code says both -"attach" and "claim" for the same act, and "claimed" already names something on the read side. +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 -1. **Only writes open a window. Decided.** A commit window opens on the first write of an author to - a target and collects that author's writes until it closes. Nothing else opens one: not a save, - not a timer. This is today's behaviour, stated as a rule so the save's settings can lean on it. - - The alternative was weighed and rejected: a save that opens its own window, so the user gets a - window to respond into. It would have to cut off whatever window is open at that moment, it would - create windows that contain no writes, and it would give a save two ways to start collecting - instead of one. What it was for is covered without it: `attach: Next` gives a save a fresh start, - `attachTimeout` gives the user time to make the first write, and `emptyCommit: Create` records - the message when nothing comes. -2. **A save attaches; it never opens.** It waits for a window it may attach to, attaches to at most - one, and gives that window its message and its timers. "Attach" is the one word for this, in the - API, the code and the docs. +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 `emptyCommit: Create` 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. Author or target changes, `buffer-limit`, drain-before-apply and shutdown remain - boundaries; a save changes only the timers. -4. **Every timer is named for what runs out.** `attachTimeout`, `idleTimeout`, `maxDuration`. -5. **Choices are enums, not booleans.** Per the Kubernetes API conventions: a boolean that later - needs a third state is a breaking change, and an enum value names the behaviour it selects. -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. + 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 @@ -92,150 +66,205 @@ window's silence timeout. The save's is `closeDelay`. The worker's code calls it | Field | Meaning | Default | Bound | |---|---|---|---| -| `idleTimeout` | close after this much silence; resets on each write | `5s`; `0s` commits every write on its own | `24h` | -| `maxDuration` | close this long after the window opened, whatever arrives | none | `24h` | +| `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: `Current` or `Next` (below) | `Current` | enum | -| `attachTimeout` | stop waiting for a window this long after receipt | `2s` | `5m` | -| `idleTimeout` | close the attached window after this much silence | none: no idle close | `5m` | +| `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` | -CEL: `idleTimeout` must not exceed `maxDuration`. The defaults give today's fixed cutoff: no idle -close, close 2 seconds after the attach. A save that wants rolling collection sets `idleTimeout`, -and `maxDuration` still ends it. +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.emptyCommit` `Skip` (default) or `Create`. It sits outside `window` because it decides the commit, not the -window. - -### `attach: Current` or `Next` - -- **`Current`** attaches to the author's window if one is open at receipt, and otherwise to the next - one that opens. This is today's behaviour, and 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 a window open at receipt, the worker closes it first, - under the target's ordinary message, and the save attaches to the next window. This is for a save - created *before* its writes, which should not sweep up the author's earlier, unrelated edits. +window. CEL: `Create` 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` | finalize right after the attach | +| `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 afterwards is not its window. When the deadline and a matching write + are ready on the same loop wake, the expiry is decided first. This is #403's boundary test. +- **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. +- **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 afterwards. 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 cover exactly +one write is what the named-write wait in the save-wait design is for. + +## `emptyCommit: Create` + +With `Create`, a save records its message even when its writes changed nothing: + +| Outcome | `Skip` (default) | `Create` | +|---|---|---| +| 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 | -`Next` means "reaches the branch worker after receipt", not "was made after the request". A write -still held for its audit fact when the request arrives lands in the next window. The docs must say -so, and the sizing advice covers it: a save that needs a hard line should create its request only -after the previous save resolved. +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. -### `emptyCommit: Skip` or `Create` +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. -With `Create`, a save always produces one commit carrying its message: +**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. -| Situation | `Skip` (today) | `Create` | -|---|---|---| -| Writes arrived and changed files | commit | commit | -| Nothing arrived before `attachTimeout` | `NoWindowInGrace`, no commit | empty commit | -| Writes arrived but the files already matched | `AlreadyPresent`, no commit | empty commit | -| Only another author's window was open | `WindowMismatch`, no commit | empty commit | +## Status -The empty commit is not a window: principle 1 still holds, and the commit holds nothing but the -message. Its author is the request's submitter when admission captured one, otherwise the -committer, the same rule as any other commit. The reason stays specific (`CommittedEmpty`), so a -caller can still tell "saved your changes" from "recorded your message". Empty commits already -exist in this codebase: #383 reverts refused edits with one. +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. -This is what makes a save a reliable primitive for an integrator: every successful save has a -commit hash to show. The cost is noise in history for saves that change nothing, which is why it is -opt-in. +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. -## Naming, and why these words +The controller's safety timeout covers the largest `attachTimeout`, the largest `maxDuration`, and +the push cooldown and retries. -### Precedents +## Naming -Every system that batches has the same two timers, and a few also have a wait for the batch to -start: +Every system that batches has the same timers, and a few also wait for the batch to start: -| Tradition | Rolling timer (resets on activity) | Fixed cap | Waiting to start | +| Tradition | Resets on activity | Fixed cap | Waiting to start | |---|---|---|---| -| Network proxies (Envoy, Gateway API, NGINX) | `idleTimeout` | `maxDuration`, `maxConnectionDuration` | `connectTimeout` | +| 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 | | Alert grouping (Alertmanager) | `group_interval` | n/a | `group_wait` | -| Kubernetes | n/a | `activeDeadlineSeconds` | n/a | -| Flux | n/a | `timeout` | n/a | - -The proxy words are the ones operators already know, and "timeout" is Flux's word as well. "Idle" -means "resets on activity" to almost every reader. "Deadline" reads as a point in time, not a -duration. The debounce words collide with "wait" as a verb. - -### Decisions - -- **The block is `window`,** the word [`definitions.md`](../definitions.md) already defines. The - timers are properties of the window, so they live in it (rule 1: one concept, one word). -- **Full field names:** `idleTimeout` and `maxDuration` rather than `idle` and `max`. Inside a - `window` block nothing stutters, and a lone `max` is unusual in Kubernetes APIs. -- **`attach` for the act, on both the enum and its timer.** The code's "claim" goes, because - "claimed" already means a rule's demand on the read side. -- **The same block shape on both kinds.** The save's block is a superset: `attach` and - `attachTimeout` have no meaning on a target, which has nothing to wait for. - -Rejected along the way: `windowTimeouts.{idle, max}`, which existed only to dodge the stored -string; flat `idleTimeout` on `commit`, which loses its subject; `batching`, which drops the defined -word; `closeDelay` kept as the cutoff, which describes an action rather than a duration. - -## Status, code and metrics - -- **Reasons.** `WaitingForCloseDelay` splits into `WaitingForWindow` (before the attach) and - `CollectingWindow` (after it). `CommittedEmpty` joins the terminal reasons. All follow - [`spec/status-conditions-guide.md`](../spec/status-conditions-guide.md). -- **Code.** "Grace" and "claim" leave the commit path. `finalizeAt` becomes an attach deadline and a - close deadline. -- **Metrics.** The `timer` finalize reason splits into `idle-timeout` and `max-duration`, a save's - cutoff reports `max-duration` instead of `finalize-signal`, and `attach: Next` closing a window - gets its own reason. [`interpreting-metrics.md`](../interpreting-metrics.md) records the labels. -- **Definitions.** [`definitions.md`](../definitions.md) gains the rule that only writes open a - window, and "attach". - -## Out of scope - -- **Inherited defaults** (a target-level default for its saves' timers). With "replace whole", a save - never needs the target's values. -- **The named-write wait** (`waitFor` with a resource reference): phase 2 of the save-wait design. - With this surface, `waitFor` names *which* write, and `window.attachTimeout` stays the one bound on - waiting for it. +| 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. + +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 -1. **API.** `GitTarget.spec.commit.window` becomes the block; `CommitRequest.spec` gains `window` - and `emptyCommit` and drops `closeDelay`. -2. **Worker.** Carry resolved timers on each open window and arm an idle and a max timer from them; - on attach, swap in the save's. Stamp the attach deadline from `attachTimeout`. `Next` closes the - author's open window at receipt. `Create` commits empty with the message on each no-commit - outcome. -3. **Controller.** Split the progress reason, add `CommittedEmpty`, and size - `commitRequestResolveTimeout` from the largest `attachTimeout` plus the largest `maxDuration`. -4. **Docs.** One paragraph per field in [`configuration.md`](../configuration.md), the definitions, - the metric labels, and the migration note. -5. **Tests.** Attach expiry versus a write on the same wake; writes after the attach join one commit; - 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; an - author change still closes an attached window; `Next` commits the earlier window separately under - its ordinary message; `Create` produces one empty commit for each no-commit situation above. +One `v1alpha4` change, 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.** `emptyCommit`, 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 `emptyCommit`. +- 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. +- 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, once, against stored `v1alpha3` objects. ## Open questions -- **`Current` and `Next`, or other values?** `Current` hides "or the next one if none is open". - `OpenOrNext` is exact but clumsy. -- **Should `emptyCommit: Create` apply to `WindowMismatch`?** It records a message while another - author's work is still in flight. That is honest, since the commit carries none of their work, but - it may confuse a reader of the history. -- **Does a target need `maxDuration` in the first PR?** It shares the timer code with the save, so - it is nearly free, but nobody has asked for it. - -## Migration (small) - -There are no production users outside our control, so this is a note, not a strategy. Both removed -fields are **deleted outright**. A stored `GitTarget` whose `commit.window` is still a string cannot -be decoded after the CRD changes, so it is patched to the block before the upgrade (one `jq` -query in UPGRADING lists them). `closeDelay: X` becomes `window.maxDuration: X`, plus -`attachTimeout: X` for a save that relied on the wait. +- **Is one minute the right target `maxDuration`?** It needs a look at real write bursts; the + decision that matters is that it is finite. +- **What else rides the `v1alpha4` bump?** Any break the vocabulary cleanup still owes costs less in + the same version change than in the next one. + +## Migration + +No production users outside our control, so this stays small. All CRDs move to `v1alpha4` together, +with no conversion webhook and no aliases. The version label alone does not rewrite stored fields: +with `None` conversion a stored string `commit.window` still fails to decode, so the upgrade rewrites +stored objects into the new shape before the new CRDs serve them, and deletes finished +`CommitRequest`s rather than converting them. That procedure is tested once against real stored +`v1alpha3` objects and written up in UPGRADING. 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 From 28721f80351755ce203d842b57f350f7301ad17f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 11:23:31 +0000 Subject: [PATCH 12/15] docs(design): name the empty-commit choice for its situation and consequence emptyCommit: Skip | Create becomes whenNothingToCommit: Resolve | CommitEmpty, following Kubernetes' whenUnsatisfiable and whenDeleted pattern and Git's own "nothing to commit". Resolve rather than Close, because closing is what a window does. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 2 +- docs/design/commit-timing-surface.md | 30 ++++++++++++++++++---------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 7f8aa773..65a7285a 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -139,7 +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, shipped as one `v1alpha4`: 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 `emptyCommit: Create` that keeps the cause in the reason. Precise clock and zero rules. Supersedes the save-wait timer model. Open: the target's `maxDuration` value, and what else rides the version bump | +| [`commit-timing-surface.md`](design/commit-timing-surface.md) | **design.** One configuration surface for commit windows and saves, shipped as one `v1alpha4`: 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, and what else rides the version bump | | [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index 8ef371f6..f1e28952 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -18,7 +18,7 @@ > maxDuration: 1m attachTimeout: 2s > idleTimeout: 1s # omitted: no idle close > maxDuration: 10s -> emptyCommit: Skip # or Create +> whenNothingToCommit: Resolve # or CommitEmpty > ``` > > It replaces `CommitRequest.spec.closeDelay` and the string `GitTarget.spec.commit.window`, and it @@ -43,7 +43,7 @@ These are decided. 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 `emptyCommit: Create` cover what it was + 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. @@ -86,11 +86,15 @@ The default is a fixed two-second cutoff, so an explicit save finishes promptly. 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.emptyCommit` +### `CommitRequest.spec.whenNothingToCommit` -`Skip` (default) or `Create`. It sits outside `window` because it decides the commit, not the -window. CEL: `Create` requires `spec.message`, because a message is the only thing an empty commit -records. +What a save does when its time runs out with nothing to commit: `Resolve` (default) finishes without +a commit, and `CommitEmpty` records the message in an empty commit. 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 @@ -154,11 +158,11 @@ waiting for the previous save to resolve does not exclude a delayed write from t That is acceptable for this feature, and the docs say so plainly. A save that must cover exactly one write is what the named-write wait in the save-wait design is for. -## `emptyCommit: Create` +## `whenNothingToCommit: CommitEmpty` -With `Create`, a save records its message even when its writes changed nothing: +With `CommitEmpty`, a save records its message even when nothing it collected changed Git: -| Outcome | `Skip` (default) | `Create` | +| Outcome | `Resolve` (default) | `CommitEmpty` | |---|---|---| | Eligible writes changed files | commit | commit | | Eligible writes already matched Git | no commit, `AlreadyPresent` | empty commit, `AlreadyPresent` | @@ -218,6 +222,10 @@ with "wait" as a verb. 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 @@ -231,7 +239,7 @@ One `v1alpha4` change, built in three steps on one branch: 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.** `emptyCommit`, the message requirement, the outcome table, and the documented +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 @@ -242,7 +250,7 @@ gains "only writes open a window" and "attach"; the save-wait design's timer mod Tests: - Omitted blocks, `{}`, partial blocks, every zero value, and duration round-trips. -- The whole request spec is immutable, including `attach` and `emptyCommit`. +- 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 From 24919e5acc2828ce4782affde80928408e044c4d Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 11:32:36 +0000 Subject: [PATCH 13/15] docs(design): stay on v1alpha3, and tighten the edges the review found An in-place v1alpha3 break with one executable procedure: remove the stored string commit.window while the old schema is still served, clear finished requests, upgrade, restore the timing. A version bump would not rewrite stored objects, so it bought nothing. The strict expiry rule is marked as new behavior with a test that keeps the late write out; Next with attachTimeout 0s is refused; a target maxDuration of 0s is defined; whenNothingToCommit covers early closes; the named-write wait promises observation, not isolation; and the Alertmanager comparison, which did not hold, is gone. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 2 +- docs/design/commit-timing-surface.md | 62 ++++++++++++++++++---------- 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index 65a7285a..bb2a474c 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -139,7 +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, shipped as one `v1alpha4`: 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, and what else rides the version bump | +| [`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/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index f1e28952..a6de0960 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -5,11 +5,11 @@ > 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 ships as one coherent -> `v1alpha4`: +> 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/v1alpha4 apiVersion: configbutler.ai/v1alpha4 +> apiVersion: configbutler.ai/v1alpha3 apiVersion: configbutler.ai/v1alpha3 > kind: GitTarget kind: CommitRequest > spec: spec: > commit: message: "fix: raise the checkout memory limit" @@ -88,8 +88,10 @@ still ends. CEL: `idleTimeout` must not exceed `maxDuration`. ### `CommitRequest.spec.whenNothingToCommit` -What a save does when its time runs out with nothing to commit: `Resolve` (default) finishes without -a commit, and `CommitEmpty` records the message in an empty commit. The field names the situation and +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` @@ -116,7 +118,8 @@ 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` | finalize right after the attach | +| `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 | @@ -127,7 +130,11 @@ 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 afterwards is not its window. When the deadline and a matching write - are ready on the same loop wake, the expiry is decided first. This is #403's boundary test. + 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. @@ -148,6 +155,8 @@ Two rules keep the edges honest: 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. @@ -155,8 +164,8 @@ What `Next` guarantees is narrower than its name suggests. It separates work the collected from work that reaches the worker afterwards. 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 cover exactly -one write is what the named-write wait in the save-wait design is for. +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` @@ -209,7 +218,6 @@ Every system that batches has the same timers, and a few also wait for the batch | 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 | -| Alert grouping (Alertmanager) | `group_interval` | n/a | `group_wait` | | 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 @@ -233,7 +241,7 @@ type), flat fields on `commit` (they lose their subject), `batching` (drops the ## Plan -One `v1alpha4` change, built in three steps on one branch: +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. @@ -254,25 +262,37 @@ Tests: - 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. + `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, once, against stored `v1alpha3` objects. +- 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. -- **What else rides the `v1alpha4` bump?** Any break the vocabulary cleanup still owes costs less in - the same version change than in the next one. ## Migration -No production users outside our control, so this stays small. All CRDs move to `v1alpha4` together, -with no conversion webhook and no aliases. The version label alone does not rewrite stored fields: -with `None` conversion a stored string `commit.window` still fails to decode, so the upgrade rewrites -stored objects into the new shape before the new CRDs serve them, and deletes finished -`CommitRequest`s rather than converting them. That procedure is tested once against real stored -`v1alpha3` objects and written up in UPGRADING. +No production users outside our control, so this stays small: an in-place break of `v1alpha3`, as +#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`. From 034e07274e2e42a2012464700149ec02519702e4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 12:02:50 +0000 Subject: [PATCH 14/15] docs(design): keep a PR number from opening a line Co-Authored-By: Claude Opus 5.5 --- docs/design/commit-timing-surface.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index a6de0960..039de31d 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -279,7 +279,7 @@ Tests: ## Migration No production users outside our control, so this stays small: an in-place break of `v1alpha3`, as -#388, #397 and #398 were, with an UPGRADING entry and no conversion machinery. A version bump would +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. From 3b6ab967bca38ee711ee85a686ad9ca1287ed7cf Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 30 Sep 2026 12:36:58 +0000 Subject: [PATCH 15/15] fix(ci): make the squash-message self-test fail when the builder crashes A crash in squash-message.sh used to read as the parse failure a 'want fail' case expects, so those cases could pass on a broken script. The message is now built on its own line, where set -e stops the run. Also: afterward, in American spelling. Co-Authored-By: Claude Opus 5.5 --- docs/design/commit-timing-surface.md | 4 ++-- hack/release-notes/test.sh | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index 039de31d..b18af08d 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -129,7 +129,7 @@ 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 afterwards is not its window. When the deadline and a matching write + 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 @@ -161,7 +161,7 @@ Two rules keep the edges honest: 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 afterwards. It cannot prove a write was made after the +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 diff --git a/hack/release-notes/test.sh b/hack/release-notes/test.sh index 6fc46ea9..45043f90 100755 --- a/hack/release-notes/test.sh +++ b/hack/release-notes/test.sh @@ -22,9 +22,11 @@ expect() { 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 - local got=pass - (cd "$repo" && "$here/squash-message.sh" HEAD~$# HEAD "$title") \ - | node "$here/check.mjs" >/dev/null || got=fail + # 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