Skip to content

fix(commitrequest): keep the creating fact past a no-op write, and order the 0.54 upgrade - #460

Merged
sunib merged 3 commits into
fix/commitrequest-creation-author-nothing-to-applyfrom
fix/release-gate-review-0-54
Oct 27, 2026
Merged

sunib merged 3 commits into
fix/commitrequest-creation-author-nothing-to-applyfrom
fix/release-gate-review-0-54

Conversation

@sunib

@sunib sunib commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #459 (which is on #458 → #456 → #455). Addresses the five release-gate findings from the combined review of the stack at e806a931.

Changes

  1. A no-op reapply no longer erases a request's creator. The fact index files a creating fact (Created: a create, or a patch answered 201) in its own uid-keyed slot (factKindCreation), swept by the TTL like the ordinary tiers. lookupCreation reads only that slot. The exact tier could not serve it: a no-op patch returns the object at the resourceVersion the create stamped, so its fact landed on the same (uid, rv) and replaced the creator's. TestFactIndex_CreationSurvivesANoOpWriteAtItsVersion reproduces it.
  2. The documented audit level can attribute saves. The attribution guide and the tuned policy's comment require RequestResponse capture of commitrequests create and patch. The author is matched by the uid and resourceVersion in responseObject, which Metadata level leaves out, and server-side apply creates with a patch. The UPGRADING admission entry says the same.
  3. Ordered upgrade from 0.54.0. UPGRADING gains "Upgrading from 0.54.0: drain the CommitRequests first": stop new saves, list and wait out open requests, adjust the policy and values, apply crds.yaml and upgrade, remove the admission webhook's leftovers, then delete and recreate any request still open. That last step matters most for a control-cluster request for a mirroring GitTarget, which the new ownership check never refuses once 0.54 wrote conditions into it. Helm removes the ValidatingWebhookConfiguration and Certificate but not cert-manager's Secret; install.yaml (kubectl apply) removes nothing, so all three are deleted by name. It says none of the 0.53.0 recreation applies. The chart README's Upgrading section links to it before helm upgrade.
  4. The deletecollection corpus is stable on v1.37.1. The normalizer writes a list's own metadata.resourceVersion as <list-rv>, outside the numbered <rv-N> space. That revision is when the API server read the list: on v1.37.1 it is the last item's revision in some runs and a newer one in others, which shifted every later placeholder and made the nightly capture report drift. CLUSTER.md's provenance note says so. Only audit.deletecollection.yaml carries a list.
  5. Docs match the code on competing saves. configuration.md, commitrequest-design.md and commit-timing-surface.md say a later save of the same author on the same GitTarget is refused with SaveInProgress while the earlier one waits or collects, and that a Next save closes a collecting window under the earlier message. architecture.md no longer says Redis holds records for request authorship; a stale key_prefix.go comment goes too.

Validation

  • task lint and task test pass locally.
  • task lab-capture repeated on the lab cluster (v1.37.1+k3s1) to check the corpus no longer alternates; result in a comment below.
  • e2e: CI legs on this branch.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Behavior Changes

    • Apply operations that remain unsettled for 10 minutes after becoming Ready are marked as timed out. The commit remains pushed, but the request stops tracking the apply.
    • A later save from the same author and target is refused while an earlier save is waiting or collecting a window. A “Next” save closes the current window before waiting for its own.
  • Documentation

    • Updated upgrade and audit-policy guidance, including steps for upgrades from version 0.54.0 and requirements for identifying request authors.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

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

Next included review available in 59 minutes.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b9e1a402-c1c3-40d7-bd3b-62424e3c59b5
Commits

Reviewing files that changed from the base of the PR and between 0bd4689 and 3b9628f.

Files selected for processing: 25
  • api/v1alpha3/commitrequest_schema_test.go
  • api/v1alpha3/commitrequest_types.go
  • charts/gitops-reverser/README.md
  • config/crd/bases/configbutler.ai_commitrequests.yaml
  • docs/UPGRADING.md
  • docs/architecture.md
  • docs/attribution-setup-guide.md
  • docs/configuration.md
  • docs/design/commit-timing-surface.md
  • docs/spec/commitrequest-design.md
  • internal/controller/commitrequest_apply.go
  • internal/controller/commitrequest_apply_test.go
  • internal/controller/commitrequest_server.go
  • internal/controller/commitrequest_server_test.go
  • internal/git/save_apply.go
  • internal/git/save_apply_test.go
  • internal/mutationlab/normalize/normalize.go
  • internal/mutationlab/normalize/normalize_test.go
  • internal/queue/fact_index.go
  • internal/queue/fact_index_store.go
  • internal/queue/fact_index_test.go
  • internal/queue/key_prefix.go
  • test/e2e/cluster/audit/policy.yaml
  • test/mutationlab/corpus/CLUSTER.md
  • test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml
Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

❤️ Share

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

@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.16393% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/queue/fact_index_store.go 78.5% 3 Missing ⚠️
internal/mutationlab/normalize/normalize.go 90.0% 2 Missing ⚠️
internal/controller/commitrequest_apply.go 92.8% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@sunib

sunib commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Lab check for finding 4: task lab-capture ran four times in a row on the lab cluster (v1.37.1+k3s1) with this branch. All four passed against the committed corpus, TestDeletecollection included (before the normalizer change the reviewer saw two failures in four runs). The working tree was clean afterwards.

@sunib
sunib added this pull request to stack #457 October 27, 2026 16:35
sunib and others added 3 commits October 27, 2026 16:47
…der the 0.54 upgrade

- The fact index files a creating fact in its own uid-keyed slot. A no-op
  patch returns the object at the resourceVersion the create stamped, so in
  the exact tier its fact replaced the creator's, and a request reapplied
  before its author resolved settled AuthorUnresolved.
- The attribution guide and the tuned policy require RequestResponse capture
  of commitrequests create and patch: the author is matched by the uid and
  resourceVersion in responseObject, which Metadata level leaves out, and
  server-side apply creates with a patch.
- UPGRADING gains an ordered "Upgrading from 0.54.0" checklist: stop new
  saves, drain open requests, upgrade, remove the admission webhook's
  leftovers (Helm keeps cert-manager's Secret; install.yaml removes
  nothing), and delete and recreate a request still open afterwards. The
  chart's upgrade section links to it before helm upgrade.
- The mutation-lab normalizer writes a list's own resourceVersion as
  <list-rv>, outside the <rv-N> numbering: on k3s v1.37.1 the deletecollection
  list returned the last item's revision in some runs and a newer one in
  others, which made the nightly capture report drift.
- configuration.md and the design text say a competing save of the same
  author is refused with SaveInProgress, not queued; architecture.md drops
  the Redis records for request authorship.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he source streams once

A request followed its apply until the worker's record aged out, which only happened when another
save on the same worker resolved: an apply running past 15 minutes was then reported Lost, with a
message blaming a restart, and one on a quiet worker was followed for ever. The follow now has an
explicit deadline, git.SaveApplyTimeout (10 minutes from Ready): past it Applied settles Unknown,
reason TimedOut, naming the commit and where the apply last stood. The worker keeps an unsettled
record until past that deadline, so Lost again means only a restart.

The CommitRequestServer rewrote its fact-stream set under its own mutex on every step while
concurrent steps read it without one; it is now set once, as the reconciler already does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every limit a request runs into was documented apart, and the safety window before the attach not
at all. One table now lists each stage, what ends it, and what the status says then; and it says
why the push alone has no limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sunib
sunib force-pushed the fix/release-gate-review-0-54 branch from 1c5a425 to 3b9628f Compare October 27, 2026 16:50
@sunib
sunib merged commit 04c3790 into main Oct 27, 2026
23 checks passed
@sunib
sunib deleted the fix/release-gate-review-0-54 branch October 27, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant