fix(controller): back off and fail terminally when a claim can never activate#928
Merged
Conversation
…activate A claim whose husk activation kept failing (a missing or corrupt snapshot artifact, a digest that will never match) re-pended at the flat capacityPendingRequeue cadence forever, with no backoff and no terminal state. In production a stuck pair produced roughly 1750 futile forkd RPCs and 3500 apiserver writes in an hour. Anchor the first-failure instant on a durable annotation (mitos.run/activate-failing-since), mirroring the pool-missing machinery: the retry backs off as the failure persists (capacityPendingRequeue doubling to a 30s cap, derived from elapsed time so it is stateless across controller restarts), and after the same bounded grace as the capacity wait the claim fails terminally with the engine's own error and an actionable remediation (issue #28). The stamp is cleared on a successful activation so a later re-activation (failover, #177) starts a fresh clock. activateRetryBackoff is a pure function with table and monotonic-plus-capped unit tests; the existing activate-failure and pool-missing envtest suites cover the reconcile wiring and confirm no regression. Closes #894 Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
📝 WalkthroughWalkthroughThe controller replaces flat-cadence husk activation retries with durable failure timing, capped exponential backoff, and terminal failure after ChangesActivation retry handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant Husk
participant SandboxClaim
Reconciler->>Husk: Attempt activation
Husk-->>Reconciler: Return activation failure
Reconciler->>SandboxClaim: Persist failure timestamp
Reconciler->>Reconciler: Calculate bounded retry delay
Reconciler->>SandboxClaim: Requeue as pending or mark failed
Husk-->>Reconciler: Return successful activation
Reconciler->>SandboxClaim: Clear failure timestamp
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/sandboxclaim_controller.go`:
- Around line 1147-1154: In the successful activation path around
activateFailingSinceAnnotation, do not return immediately when the annotation
cleanup Update fails. Complete token writing and Ready persistence first, then
remove the annotation with a metadata patch; afterward propagate any cleanup
failure wrapped with fmt.Errorf("context: %w", err), preserving the completed
activation result.
- Around line 1129-1131: Handle the error returned by r.Status().Update in the
husk activation timeout path before returning from the controller. Wrap any
persistence error with fmt.Errorf using a descriptive context and return it so
reconciliation retries; only return the existing empty result when the terminal
status update succeeds.
- Around line 1098-1108: The activation timestamp handling around
activateFailingSinceAnnotation must replace malformed or future timestamps
durably instead of silently resetting failingSince in memory. Update the
annotation to now, persist it with r.Update, and wrap any update failure using
fmt.Errorf with %w; preserve valid, non-future timestamps and ensure the updated
timestamp is used for subsequent wait calculations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dda09abb-39e0-42c3-935d-ec1e98ee3da0
📒 Files selected for processing (2)
internal/controller/activate_backoff_test.gointernal/controller/sandboxclaim_controller.go
Comment on lines
+1098
to
+1108
| if stamp := claim.Annotations[activateFailingSinceAnnotation]; stamp != "" { | ||
| if parsed, perr := time.Parse(time.RFC3339, stamp); perr == nil { | ||
| failingSince = parsed | ||
| } | ||
| } else { | ||
| if claim.Annotations == nil { | ||
| claim.Annotations = map[string]string{} | ||
| } | ||
| claim.Annotations[activateFailingSinceAnnotation] = now.Format(time.RFC3339) | ||
| if uerr := r.Update(ctx, claim); uerr != nil { | ||
| return ctrl.Result{}, uerr |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace invalid activation timestamps instead of restarting the timer in memory.
A malformed annotation remains stored while failingSince resets to now each pass, so waited never reaches maxWait and the claim retries forever. Replace malformed or future timestamps durably, and wrap update errors.
Proposed fix
if stamp := claim.Annotations[activateFailingSinceAnnotation]; stamp != "" {
- if parsed, perr := time.Parse(time.RFC3339, stamp); perr == nil {
+ if parsed, perr := time.Parse(time.RFC3339, stamp); perr == nil && !parsed.After(now) {
failingSince = parsed
+ } else {
+ claim.Annotations[activateFailingSinceAnnotation] = now.Format(time.RFC3339)
+ if uerr := r.Update(ctx, claim); uerr != nil {
+ return ctrl.Result{}, fmt.Errorf("reset invalid activation failure timestamp: %w", uerr)
+ }
}As per coding guidelines, Go errors must be wrapped with fmt.Errorf("context: %w", err).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/sandboxclaim_controller.go` around lines 1098 - 1108, The
activation timestamp handling around activateFailingSinceAnnotation must replace
malformed or future timestamps durably instead of silently resetting
failingSince in memory. Update the annotation to now, persist it with r.Update,
and wrap any update failure using fmt.Errorf with %w; preserve valid, non-future
timestamps and ensure the updated timestamp is used for subsequent wait
calculations.
Source: Coding guidelines
Comment on lines
+1129
to
+1131
| _ = r.Status().Update(ctx, claim) | ||
| logger.Info("claim failed: husk activation did not succeed past bounded wait", "claim", claim.Name, "node", pod.Spec.NodeName, "waited", waited.Round(time.Second), "maxWait", maxWait) | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate failure to persist the terminal status.
Ignoring this update error and returning no requeue can leave the claim pending indefinitely even though the grace period expired.
Proposed fix
- _ = r.Status().Update(ctx, claim)
+ if err := r.Status().Update(ctx, claim); err != nil {
+ return ctrl.Result{}, fmt.Errorf("mark claim failed after activation timeout: %w", err)
+ }As per coding guidelines, Go errors must be wrapped with fmt.Errorf("context: %w", err).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ = r.Status().Update(ctx, claim) | |
| logger.Info("claim failed: husk activation did not succeed past bounded wait", "claim", claim.Name, "node", pod.Spec.NodeName, "waited", waited.Round(time.Second), "maxWait", maxWait) | |
| return ctrl.Result{}, nil | |
| if err := r.Status().Update(ctx, claim); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("mark claim failed after activation timeout: %w", err) | |
| } | |
| logger.Info("claim failed: husk activation did not succeed past bounded wait", "claim", claim.Name, "node", pod.Spec.NodeName, "waited", waited.Round(time.Second), "maxWait", maxWait) | |
| return ctrl.Result{}, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/sandboxclaim_controller.go` around lines 1129 - 1131,
Handle the error returned by r.Status().Update in the husk activation timeout
path before returning from the controller. Wrap any persistence error with
fmt.Errorf using a descriptive context and return it so reconciliation retries;
only return the existing empty result when the terminal status update succeeds.
Source: Coding guidelines
Comment on lines
+1147
to
+1154
| // A previously failing activation just succeeded: clear the grace stamp so a | ||
| // later re-activation (failover, #177) starts a fresh clock. The guard makes | ||
| // the common first-try success pay only a map lookup, never a write. | ||
| if claim.Annotations[activateFailingSinceAnnotation] != "" { | ||
| delete(claim.Annotations, activateFailingSinceAnnotation) | ||
| if uerr := r.Update(ctx, claim); uerr != nil { | ||
| return ctrl.Result{}, uerr | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not let annotation cleanup abort completion after successful activation.
If Update fails here, the function returns before joining the token writer or persisting Ready. The pod is already activated and claim-labeled, while subsequent selection skips claimed pods, potentially orphaning the activation permanently. Complete the token/status path first, then remove the annotation using a metadata patch; propagate any cleanup error with context afterward.
As per coding guidelines, Go errors must be wrapped with fmt.Errorf("context: %w", err).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/sandboxclaim_controller.go` around lines 1147 - 1154, In
the successful activation path around activateFailingSinceAnnotation, do not
return immediately when the annotation cleanup Update fails. Complete token
writing and Ready persistence first, then remove the annotation with a metadata
patch; afterward propagate any cleanup failure wrapped with fmt.Errorf("context:
%w", err), preserving the completed activation result.
Source: Coding guidelines
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
1 participant
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.
Thinking Path
Linked Issues or Issue Description
Closes #894.
What Changed
mitos.run/activate-failing-since(RFC3339) on the first failure, then either requeue withactivateRetryBackoff(waited)(capacityPendingRequeue doubling to a 30s cap) or, once pastmaxPendingDuration, transition to terminal Failed with FinishedAt and an actionable ActivateFailed condition carrying the engine error and the node. The stamp is cleared on a successful activation so a later re-activation starts a fresh clock.activateRetryBackoff(pure, elapsed-time-derived, stateless across restarts) plusactivateFailingSinceAnnotationandactivateFailingMaxBackoff.Verification
go build ./internal/controller/,go vet ./internal/controller/clean;gofmt -lclean.go test ./internal/controller/ -run TestActivateRetryBackoff(the new unit tests) pass.go test ./internal/controller/ -run 'TestHuskClaimActivateFailureNotReady|TestHuskClaimActivateFailureJoinsTheTokenSecretWrite|TestHuskClaimActivatesDormantPod|TestHuskClaimActivateCarriesExpectedDigest|TestClaimFailsTerminallyWhenPoolNeverAppears'(envtest) all pass: the activation failure path, happy path, token-write join, and pool-missing terminal are unaffected.Risks
Low risk. The retry path still releases the husk pod and re-pends within the grace period, so transient faults recover exactly as before, only at a backed-off cadence. The new terminal transition reuses the proven pool-missing grace bound and GC stamping (#630), so a permanently stuck claim is reaped like any other Failed sandbox. One extra metadata write occurs on the first failure and on recovery only.
Model Used
Claude Opus 4.8 (claude-opus-4-8), 1M context, extended thinking.
Summary by CodeRabbit
New Features
SandboxFailedwith actionable failure details.Bug Fixes