🐛 fix: non-transient reconcile error wedges a workload CR permanently (REQ-REL-2026-08) - #186
Conversation
…s (REQ-REL-2026-08)
A workload CR whose reconcile failed with an error that was neither
TransientError nor TerminalError (e.g. a bare context.DeadlineExceeded
escaping the mqrest adapter after an mqweb blip) returned
(ctrl.Result{}, nil): no requeue, no error, no log. Nothing re-enqueued
the object, so it stayed Synced=False forever (23h live on emb-test).
- REL-2: roundTrip/sleepWithContext wrap ctx.Err() in TransientError so
deadline expiry and cancellation flow down the retry path.
- REL-1: setSyncedError never returns (Result{}, nil) on error —
transient gets TransientRequeueInterval, terminal (bad spec) is the
only no-requeue path, anything unclassified is returned to
controller-runtime for rate-limited backoff.
- Terminal failures without an explicit reason get a distinct
ReasonTerminalError condition reason (greppable, no-retry semantics).
- REL-3: Synced=False transitions that do not return an error now log
the error and scheduled retry interval.
There was a problem hiding this comment.
Pull request overview
Fixes a controller-runtime wedge where certain non-transient reconcile failures (notably bare context.DeadlineExceeded escaping the mqrest adapter) could leave workload CRs stuck Synced=False indefinitely due to (ctrl.Result{}, nil) being returned (no requeue, no error) and predicates not re-enqueuing.
Changes:
- Ensure adapter context cancellation/deadline errors are wrapped as
mqadmin.TransientErrorso they follow the transient retry path. - Update
setSyncedErrorretry policy: transient errors keep fixedRequeueAfter, terminal errors remain no-requeue, and all other/unclassified errors are returned to controller-runtime for rate-limited backoff + logging. - Add/adjust unit+envtest coverage around unclassified error retries, terminal reason classification, and transient “retryAfter” logging.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/reconcile_shared.go | Revises error-to-retry mapping and adds logging for non-returned errors. |
| internal/controller/reconcile_shared_test.go | Adds regression tests for unclassified error retry behavior and transient retry logging. |
| internal/controller/reconcile_error_branches_test.go | Updates envtest expectations for missing connection to now return an error (rate-limited retry). |
| internal/controller/queue_controller_unit_test.go | Adds reconciler-level regression test for unclassified DeadlineExceeded retry behavior. |
| internal/controller/events.go | Differentiates terminal condition reason via ReasonTerminalError. |
| internal/controller/events_test.go | Updates expectations for terminal-without-reason classification. |
| internal/adapter/mqrest/resilience.go | Wraps ctx.Err() during request/backoff as mqadmin.TransientError to preserve retry semantics. |
| internal/adapter/mqrest/resilience_test.go | Strengthens/extends tests to assert context errors are transient and preserve errors.Is behavior. |
| api/v1beta1/queuemanagerconnection_types.go | Introduces shared ReasonTerminalError constant for non-retryable failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch { | ||
| case errors.Is(err, mqadmin.ErrTransient): | ||
| requeue = ctrl.Result{RequeueAfter: TransientRequeueInterval()} | ||
| case errors.Is(err, mqadmin.ErrTerminal): | ||
| default: | ||
| reconcileErr = err | ||
| } |
| // The 1h-TTL warning event is the only other trace; without this line a not-synced | ||
| // CR is invisible in the logs (controller-runtime only logs returned errors). |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…classify LTPA login ctx expiry transient Review follow-up on PR #186 (REQ-REL-2026-08): - D1: Synced=False reason tables (INSTALL_AND_USE), OPERATOR_RUNTIME error matrix, and ADR-0014 amendment now describe the amended retry policy and the distinct TerminalError condition reason. - R1: ltpa login wraps ctx.Err() in TransientError like roundTrip, with a pinning test.
task format import-reordered zz_generated.deepcopy.go and admin_mock.go in e83eb3f; controller-gen/mockery emit the original form, so 'task verify' went red. Restore both to the generated content.
|
REVIEW — mkurator/fix/rel-2026-08-terminal-error-wedge @ bce9df8 (PR #186) — APPROVE — 2026-08-06Independent /mkurator-review (tech-reviewer subagent); verdict only — maintainer merges. Review heads: e1fa9cf → e83eb3f → bce9df8.
Finding history:
Functional correctness: Full scope delivered and regression-pinned. REL-1: Next steps: clear to merge from this reviewer's standpoint once the fresh head's required checks are green. This review does not authorize the merge and no PR approval was submitted by the reviewer. |
…classify LTPA login ctx expiry transient Review follow-up on PR #186 (REQ-REL-2026-08): - D1: Synced=False reason tables (INSTALL_AND_USE), OPERATOR_RUNTIME error matrix, and ADR-0014 amendment now describe the amended retry policy and the distinct TerminalError condition reason. - R1: ltpa login wraps ctx.Err() in TransientError like roundTrip, with a pinning test.
P0 — REQ-REL-2026-08
Channel/logistics-appsat atSynced=Falsefor 23h onemb-testafter a QM restart: a barecontext.DeadlineExceededescaped the mqrest adapter,setSyncedErrortreated it as terminal and returned(ctrl.Result{}, nil)— no requeue, no error, no log — andworkloadReconcilePredicateshad nothing to re-enqueue it. Spec:agent-context/inbox/specs/REQ-REL-2026-08-terminal-error-wedge.md.Changes
roundTrip/sleepWithContextwrapctx.Err()inmqadmin.TransientError(withCause, soerrors.Is(err, context.Canceled/DeadlineExceeded)still holds). Deadline expiry now flows down the retry path like every other network failure.setSyncedErrornever returns(Result{}, nil)on error:ErrTransient→RequeueAfter: TransientRequeueInterval()(unchanged, 30s)ErrTerminal→ no requeue (bad spec; generation predicate re-enqueues on fix), now with a distinct default condition reasonTerminalErrorso no-retry states are greppableSynced=Falsetransitions that don't return an error now log the error, reason, andretryAfter(logr has no WARN; ERROR level chosen so the line survives anERROR|WARNscan).QueueManagerConnectionnow returns the error (retry + log) instead of relying solely on the QMC-ready watch fan-out; two envtest expectations updated.Not in this PR: REL-4 (drop
desiredMQSCprinter column — public-CRD surface, LGTM-gated/HOLD), Q2 (configurable reconcile deadline).Tests (TDD — all failed before the fix)
TestSetSyncedError_UnclassifiedReturnsError— the exact AC regression test: bareDeadlineExceededmust yieldRequeueAfter > 0 || err != nilTestQueueReconciler_UnclassifiedErrorSchedulesRetry— same at reconciler levelTestRoundTripContextDeadlineExceededIsTransient+ strengthened cancel/backoff/sleep tests assertingerrors.Is(err, ErrTransient)TestSetSyncedError_TransientLogsRetry— asserts the observable log lineclassifyReconcileErrorterminal-without-reason →TerminalErrorGates
test:run(race, coverage 92.6% internal/ ≥ 90% floor),format:check,lint,arch:lint,verify,test:schema,scrub:tree— all green locally.