diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 7c7dff5..5f9f240 100644 --- a/internal/controllers/localresource/localresource.go +++ b/internal/controllers/localresource/localresource.go @@ -512,8 +512,17 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al cr.Status.TargetCommitId = toLocalResourceCommitId cr.Status.TargetBranch = toRepo.CurrentBranch() - err = e.kube.Status().Update(ctx, cr) - if err != nil { + // SAME RETRY AS THE COMMITTED PATH, and this branch needs it MORE, not less. + // + // A bare Update here is byte-for-byte the failure #16 describes: lose the conflict and the + // sync returns with external-create-pending set and no recorded result. This variant is the + // one that cannot self-heal — the branch makes NO commit, so a later Observe finds no footer + // to recognise the resource by and cannot adopt it. + // + // The sync retry added in #17 makes this path MORE likely to be taken, not less: a retried + // attempt 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. + if err := e.updateStatusWithRetry(ctx, cr); err != nil { return fmt.Errorf("unable to update status: %w", err) } return nil diff --git a/internal/controllers/localresource/status_write_guard_test.go b/internal/controllers/localresource/status_write_guard_test.go new file mode 100644 index 0000000..2ef6562 --- /dev/null +++ b/internal/controllers/localresource/status_write_guard_test.go @@ -0,0 +1,54 @@ +package localresource + +import ( + "os" + "regexp" + "strings" + "testing" +) + +// Every status write in the sync path must go through updateStatusWithRetry. +// +// This is a source guard rather than a behavioural test on purpose. The bug it prevents is not a +// wrong result, it is a MISSED CALL SITE: #17 fixed the status write on the committed path and left +// the identical bare write in the `git.NoErrAlreadyUpToDate` branch a few lines above — same +// function, same failure, caught only in review. 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. +// +// Losing the conflict on either write strands a LocalResource with external-create-pending set and +// no recorded result. The already-up-to-date branch is the worse of the two: it makes no commit, so +// a later Observe has no footer to recognise the resource by and cannot adopt it. +func TestNoBareStatusUpdateOutsideTheRetryHelper(t *testing.T) { + const file = "localresource.go" + src, err := os.ReadFile(file) + if err != nil { + t.Fatalf("reading %s: %v", file, err) + } + + helper := regexp.MustCompile(`(?s)func \(e \*external\) updateStatusWithRetry\(.*?\n}`) + outside := helper.ReplaceAllString(string(src), "") + + var offenders []string + for i, line := range strings.Split(outside, "\n") { + if strings.Contains(line, "Status().Update(") { + offenders = append(offenders, strings.TrimSpace(line)+" (line ~"+itoa(i+1)+")") + } + } + if len(offenders) > 0 { + t.Fatalf("bare Status().Update outside updateStatusWithRetry — a lost conflict here strands the\n"+ + "resource with external-create-pending set and no recorded result:\n %s", + strings.Join(offenders, "\n ")) + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var b []byte + for n > 0 { + b = append([]byte{byte('0' + n%10)}, b...) + n /= 10 + } + return string(b) +}