Repository navigation
fix: the already-up-to-date branch needs the status retry too - #18
Merged
Merged
Conversation
#17 fixed the status write on the committed path and left the identical bare `e.kube.Status().Update(ctx, cr)` in the `git.NoErrAlreadyUpToDate` branch a few lines above — same function, same failure mode, missed. Caught in review by the installer session before tagging. That branch needs it MORE, not less: * It is the variant that cannot self-heal. It makes NO commit, so a later Observe has no footer to recognise the resource by and cannot adopt it — it falls through to the terminal default branch. * #17 makes this path MORE likely to be taken. A retried sync re-clones a tree that may already contain the file from the attempt that actually won the push, so the commit becomes a no-op and lands exactly here. Also adds a SOURCE guard rather than a behavioural test, because the bug is a missed call site, not a wrong result: a behavioural test would need to mock git deeply enough to reach each branch and would still say nothing about a branch added later. The guard asserts no bare Status().Update survives outside updateStatusWithRetry. Verified it actually catches the regression — reverting line 515 fails it, restoring passes. go build, go vet clean; full suite 9 packages ok. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #17, caught in review by the installer session before tagging. Thank you — this was a real miss, not a nit.
#17 fixed the status write on the committed path and left the identical bare
e.kube.Status().Update(ctx, cr)in thegit.NoErrAlreadyUpToDatebranch a few lines above. Same function, same failure mode.That branch needs it more, not less
It is the variant that cannot self-heal. It makes no commit, so a later
Observehas no footer to recognise the resource by and cannot adopt it — it falls through to the terminal default branch.And #17 makes this path more likely to be taken. A retried sync re-clones a tree that may already contain the file from the attempt that actually won the push, so the commit becomes a no-op and lands exactly here. My own fix increased traffic through the branch it left unfixed.
A source guard, not a behavioural test
Deliberate. The bug is a missed call site, not a wrong result. A behavioural test would have to mock git deeply enough to reach each branch, and would still say nothing about a third branch added later.
The guard asserts no bare
Status().Updatesurvives outsideupdateStatusWithRetry. I verified it actually catches the regression it exists for — reverting line 515 fails it, restoring passes.Corrections to #17's description
Both raised in the same review, both verified, both mine to own:
1. The permanence fix is not in #17. It is in the already-merged base
18afa8b(provider-runtime v1.2.1 → v1.3.1), which changes the create handshake from terminal refusal to adopt-on-Observe. #17 narrows the race; an exhausted conflict still escapesCreateconflict-typed and still leavespendingset. Worth stating plainly because tagging ships that handshake change toLocalResourceandRepoas a rider.2. "The apiserver rejected every event this controller emitted" was too broad. True only of the two
Eventfsites inlocalresource.go.repo.gohas noEventfcalls at all, and the reconciler passes actions.Verification
go build,go vetclean; full suite 9 packages ok.🤖 Generated with Claude Code
https://claude.ai/code/session_01JumEqTGGJ2hf3vjE3xFQsv