Stop retrying a failure that has already repeated N times (#25) - #26
Merged
Conversation
`max_restarts` is a budget for how many times a fix may fail to stick. It cannot tell a transient apart from a deterministic error, because both merely decrement it. So a config mistake — a missing binary, a bad path, an unwritable directory — consumes the entire budget one identical failure at a time, and on a long pipeline that is measured in hours. Reported in #25: a stage failed with `pixi: command not found` (exit 127, a PATH problem that could never resolve itself) and retried 55 times over ~14 hours, each attempt re-running a completed job's 10-minute evaluation step. `max_restarts: 50` behaved exactly as documented; nothing was watching for the fact that it was the same failure every time. Adds `restart_policy.max_identical_failures` (default 5, `0` disables). The monitor tracks consecutive detections carrying the same signature and stops once they exceed the cap, regardless of remaining restarts. Two design points worth flagging for review: The signature collapses digits — `f"{detector}:{kind}:{re.sub(r'[0-9]+','#',message)[:200]}"`. The failure that motivated this carried a changing pid and timestamp on every retry, so comparing raw message text would have treated all 55 occurrences as distinct and never fired. There is a test for that specifically. A *different* detection resets the streak. That is the guard's safety property: a flaky error appearing occasionally among successful restarts must never trip the cap and stop a supervisor that is recovering perfectly well. Also tested. The guard sets `_stop` rather than only clearing the outcome. Clearing the outcome alone declines to heal, but `_restart_policy_fallback` would still restart the child on the no-action path — which is the loop this exists to break. Verified end to end: with `max_restarts: 50` and a cap of 2, the monitor restarts twice and then stops on the third identical failure. Tests: 6 new, covering the cap boundary, streak reset, digit collapsing, detector isolation, `0` disabling the guard, and the default being non-zero (a guard that ships off protects nobody who has not already been bitten). Full suite 354 passed; ruff and pyrefly clean. Not addressed here: the pipeline-level stage timeout also requested in #25. That one needs a deadline threaded into the blocking `Monitor.run()` and is worth its own PR and its own discussion about where the deadline belongs.
📝 WalkthroughWalkthroughAdds ChangesConsecutive Identical Failure Guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Detection
participant Monitor
participant RestartPolicy
participant Healer
Detection->>Monitor: trigger failure handling
Monitor->>Monitor: normalize and count failure signature
Monitor->>RestartPolicy: read repeat-failure cap
Monitor->>Healer: suppress healing when cap is exceeded
Monitor-->>Detection: emit exit notification and stop
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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: 1
🤖 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 `@src/autosentry/monitor.py`:
- Around line 738-756: Add an early return at the start of _fire_detection when
self._stop is already set, preventing later detections from reaching
supervisor.apply_action(). Ensure the existing repeat-exhaustion branch still
sets _stop, and add coverage for two simultaneous detections so the second is
ignored after the first trips the guard.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa2c20f0-8a41-4100-86e2-b81c69ae32f8
📒 Files selected for processing (4)
README.mdsrc/autosentry/config.pysrc/autosentry/monitor.pytests/test_repeat_failure_guard.py
Comment on lines
+738
to
+756
| repeat_exhausted = self._note_repeat(det) | ||
| if repeat_exhausted: | ||
| cap = self.cfg.process.restart_policy.max_identical_failures | ||
| log().error( | ||
| f"[{det.detector}] this exact failure has now repeated " | ||
| f"{self._repeat_count} times in a row (cap {cap}) — it is not " | ||
| f"transient. Recording the incident and stopping; restarting " | ||
| f"again would only repeat it. Fix the cause, then relaunch." | ||
| ) | ||
| self._notify( | ||
| "exit", | ||
| f"repeated failure: {det.message[:100]}", | ||
| f"{self._repeat_count} identical failures in a row (cap {cap}); " | ||
| f"supervisor stopping rather than consuming the restart budget", | ||
| ) | ||
| # Stop for real. Clearing `outcome` alone only declines to heal; | ||
| # `_restart_policy_fallback` would still restart the child on the | ||
| # no-action path, which is the loop this guard exists to break. | ||
| self._stop = True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent later detections from restarting after the guard trips.
Setting _stop here does not stop _handle_line or _tick from processing subsequent detectors in the same pass. A later detection can still reach supervisor.apply_action() and restart the child after this guard has declared the monitor stopped. Add an early _stop return at _fire_detection entry (and cover two simultaneous detections).
Proposed fix
def _fire_detection(self, det: Detection) -> None:
+ if self._stop:
+ return
if det.kind == "anomaly":📝 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
| repeat_exhausted = self._note_repeat(det) | |
| if repeat_exhausted: | |
| cap = self.cfg.process.restart_policy.max_identical_failures | |
| log().error( | |
| f"[{det.detector}] this exact failure has now repeated " | |
| f"{self._repeat_count} times in a row (cap {cap}) — it is not " | |
| f"transient. Recording the incident and stopping; restarting " | |
| f"again would only repeat it. Fix the cause, then relaunch." | |
| ) | |
| self._notify( | |
| "exit", | |
| f"repeated failure: {det.message[:100]}", | |
| f"{self._repeat_count} identical failures in a row (cap {cap}); " | |
| f"supervisor stopping rather than consuming the restart budget", | |
| ) | |
| # Stop for real. Clearing `outcome` alone only declines to heal; | |
| # `_restart_policy_fallback` would still restart the child on the | |
| # no-action path, which is the loop this guard exists to break. | |
| self._stop = True | |
| if self._stop: | |
| return | |
| repeat_exhausted = self._note_repeat(det) | |
| if repeat_exhausted: | |
| cap = self.cfg.process.restart_policy.max_identical_failures | |
| log().error( | |
| f"[{det.detector}] this exact failure has now repeated " | |
| f"{self._repeat_count} times in a row (cap {cap}) — it is not " | |
| f"transient. Recording the incident and stopping; restarting " | |
| f"again would only repeat it. Fix the cause, then relaunch." | |
| ) | |
| self._notify( | |
| "exit", | |
| f"repeated failure: {det.message[:100]}", | |
| f"{self._repeat_count} identical failures in a row (cap {cap}); " | |
| f"supervisor stopping rather than consuming the restart budget", | |
| ) | |
| # Stop for real. Clearing `outcome` alone only declines to heal; | |
| # `_restart_policy_fallback` would still restart the child on the | |
| # no-action path, which is the loop this guard exists to break. | |
| self._stop = True |
🤖 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 `@src/autosentry/monitor.py` around lines 738 - 756, Add an early return at the
start of _fire_detection when self._stop is already set, preventing later
detections from reaching supervisor.apply_action(). Ensure the existing
repeat-exhaustion branch still sets _stop, and add coverage for two simultaneous
detections so the second is ignored after the first trips the guard.
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.
max_restartsis a budget for how many times a fix may fail to stick. Itcannot tell a transient apart from a deterministic error, because both merely
decrement it. So a config mistake — a missing binary, a bad path, an
unwritable directory — consumes the entire budget one identical failure at a
time, and on a long pipeline that is measured in hours.
Reported in #25: a stage failed with
pixi: command not found(exit 127, aPATH problem that could never resolve itself) and retried 55 times over ~14
hours, each attempt re-running a completed job's 10-minute evaluation step.
max_restarts: 50behaved exactly as documented; nothing was watching for thefact that it was the same failure every time.
Adds
restart_policy.max_identical_failures(default 5,0disables). Themonitor tracks consecutive detections carrying the same signature and stops
once they exceed the cap, regardless of remaining restarts.
Two design points worth flagging for review:
The signature collapses digits —
f"{detector}:{kind}:{re.sub(r'[0-9]+','#',message)[:200]}". The failure thatmotivated this carried a changing pid and timestamp on every retry, so
comparing raw message text would have treated all 55 occurrences as distinct
and never fired. There is a test for that specifically.
A different detection resets the streak. That is the guard's safety
property: a flaky error appearing occasionally among successful restarts must
never trip the cap and stop a supervisor that is recovering perfectly well.
Also tested.
The guard sets
_stoprather than only clearing the outcome. Clearing theoutcome alone declines to heal, but
_restart_policy_fallbackwould stillrestart the child on the no-action path — which is the loop this exists to
break. Verified end to end: with
max_restarts: 50and a cap of 2, themonitor restarts twice and then stops on the third identical failure.
Tests: 6 new, covering the cap boundary, streak reset, digit collapsing,
detector isolation,
0disabling the guard, and the default being non-zero (aguard that ships off protects nobody who has not already been bitten). Full
suite 354 passed; ruff and pyrefly clean.
Not addressed here: the pipeline-level stage timeout also requested in #25.
That one needs a deadline threaded into the blocking
Monitor.run()and isworth its own PR and its own discussion about where the deadline belongs.
Summary by CodeRabbit
New Features
0to disable this safeguard.Documentation