Fix pipeline stall after a stage's child exits (#22) - #23
Merged
Conversation
The monitor's exit edge was `state.last_exit_code != status.exit_code`, compared against a field that persists across Monitor instances. A pipeline builds a fresh Monitor per stage over one shared state.json, so stage N's clean exit left `last_exit_code: 0` on disk and stage N+1's clean exit compared equal: the edge never fired, `_handle_exit` never ran, the stage never completed, and the supervisor looped forever over a dead child while the heartbeat kept advancing. Every liveness check read healthy throughout. The same trap caught any second `autosentry run` in a directory whose previous run exited 0. The edge now keys on the child's `started_at`, which every supervisor stamps fresh in `start()`, so it fires once per child regardless of what a previous stage or run left behind. Reproduced pre-fix against a real three-stage pipeline (dead child, live heartbeat, log ending at "Starting process"); all three stages complete after. Two follow-ons in the same failure class: - `lifecycle: restart_always` now relaunches after a clean exit. The default exit_code detector is non-zero-only, so a clean exit fired no detection, no healer ran, and the restart_policy fallback (which hangs off the detection path) never got a chance - the same silent wedge by a different road. The relaunch is audited but doesn't spend the unverified-restart budget; a service that exits 0 and comes back hasn't failed at anything. - `monitor.dead_child_grace_seconds` (default 900) stops a supervisor that has had no child for that long, converting any future variant from silent idle hours into a loud nonzero exit. A heartbeat only proves a thread is scheduling, so state.json now records `child_running` / `child_started_at` / `child_dead_since` beside it, plus `stage` / `stage_index` / `stage_count`. `autosentry probe` reduces the pair to `monitor.wedged` - pid alive, heartbeat fresh, no child - and exits 1 on it, so it drops into a cron liveness hook unchanged. Probe schema_version is now 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe supervisor now detects child exits by child identity, relaunches clean exits under ChangesSupervisor liveness and recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant Monitor
participant StateStore
participant Probe
Supervisor->>Monitor: report child status
Monitor->>StateStore: save child and stage liveness
Monitor->>Monitor: detect exit edge or watchdog timeout
Monitor->>Supervisor: restart child or stop monitor
Probe->>StateStore: load persisted state
Probe-->>Probe: derive wedged status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/autosentry/monitor.py (1)
549-566: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExhausted budget from unrelated restarts blocks
restart_always's clean-exit relaunch.
_restart_after_clean_exitis only reached if the pre-existingbudget_exhaustedcheck passes.state.restartscan already be at/pastmax_restartsfrom earlier rule/healer-driven restarts on the action-apply path (_fire_detection, which increments viarecord_restart(unverified=True)without an immediate exhaustion check), yet the monitor doesn't necessarily stop at that point. When the child later exits cleanly, this check fires first and gives up instead of relaunching — even though clean-exit relaunches are documented to never spend this budget. This defeatsrestart_always's "keep restarting" contract for a scenario the current tests don't exercise (they only run relaunches withstate.restartsstaying at 0).🔧 Proposed fix: check the always-relaunch path before the budget gate
- if budget_exhausted(self.state.restarts, self.state.max_restarts): - log().error(f"max restarts ({self.state.max_restarts}) reached — giving up") - self._notify( - "exit", - "max restarts reached", - f"giving up after {self.state.restarts} restart(s)", - ) - self._record_vault_exhaustion(detector=None) - return False - if lifecycle == "restart_always" and exit_code == 0: + if lifecycle == "restart_always" and exit_code == 0: + # Clean-exit relaunches never spend the unverified budget, so + # they must not be gated by it either — otherwise an earlier, + # unrelated run of failed restarts can permanently wedge a + # lifecycle that promises to always come back. return self._restart_after_clean_exit(exit_code) + if budget_exhausted(self.state.restarts, self.state.max_restarts): + log().error(f"max restarts ({self.state.max_restarts}) reached — giving up") + self._notify( + "exit", + "max restarts reached", + f"giving up after {self.state.restarts} restart(s)", + ) + self._record_vault_exhaustion(detector=None) + return False🤖 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 549 - 566, Move the `restart_always` clean-exit branch in the monitor’s lifecycle handling before the `budget_exhausted` guard, so `_restart_after_clean_exit(exit_code)` is invoked for `exit_code == 0` without consuming or being blocked by the restart budget. Preserve the existing budget exhaustion notification and return behavior for all other lifecycle and exit-code paths.
🤖 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 `@README.md`:
- Around line 277-279: Update the README cron example around autosentry probe so
it restarts the supervisor only when monitor liveness is unhealthy, not for
every nonzero probe exit. Use a liveness-only probe mode if available; otherwise
parse the probe’s monitor liveness fields and preserve pending-incident exit
statuses without restarting.
In `@src/autosentry/cli/commands/probe.py`:
- Around line 174-184: Update the wedge threshold logic near the wedged
calculation to preserve an explicit cfg.monitor.dead_child_grace_seconds value
of 0 as disabled rather than replacing it with stall_threshold. Ensure the probe
cannot report wedged from this backstop when the configured grace period is
zero, while retaining the configured positive threshold and existing fallback
behavior for an unset value.
- Around line 20-26: Update the probe output example containing last_exit_code,
child_running, wedged, stage, stage_index, and stage_count so it remains valid
JSON by removing the inline # annotations; preserve the field values and move
explanations outside the JSON sample or explicitly label the sample as JSONC.
---
Outside diff comments:
In `@src/autosentry/monitor.py`:
- Around line 549-566: Move the `restart_always` clean-exit branch in the
monitor’s lifecycle handling before the `budget_exhausted` guard, so
`_restart_after_clean_exit(exit_code)` is invoked for `exit_code == 0` without
consuming or being blocked by the restart budget. Preserve the existing budget
exhaustion notification and return behavior for all other lifecycle and
exit-code paths.
🪄 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: 03318895-626f-42f3-a937-433378c212d1
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdsrc/autosentry/cli/commands/probe.pysrc/autosentry/cli/commands/status.pysrc/autosentry/config.pysrc/autosentry/monitor.pysrc/autosentry/pipeline.pysrc/autosentry/state.pysrc/autosentry/templates/autosentry.yaml.tmplsrc/autosentry/tui.pytests/test_issue_22_pipeline_stall.pytests/test_issue_5_lifecycle_and_state_save.pytests/test_pipeline.pytests/test_restart_budget.py
Comment on lines
+277
to
+279
| ```bash | ||
| */5 * * * * cd /path/to/run && autosentry probe -q >/dev/null || systemctl restart autosentry | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not restart on every nonzero probe exit.
autosentry probe exits 1 for pending incidents as well as DOWN/STALE/WEDGED monitors. This cron entry will restart a healthy supervisor whenever a normal incident is waiting, potentially interrupting recovery or causing restart loops. Parse the monitor liveness fields or add a liveness-only probe mode before restarting.
🤖 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 `@README.md` around lines 277 - 279, Update the README cron example around
autosentry probe so it restarts the supervisor only when monitor liveness is
unhealthy, not for every nonzero probe exit. Use a liveness-only probe mode if
available; otherwise parse the probe’s monitor liveness fields and preserve
pending-incident exit statuses without restarting.
Comment on lines
+20
to
+26
| "last_exit_code": null, | ||
| "child_running": true, # is there actually a child under it? | ||
| "child_dead_seconds": null, # how long there hasn't been one | ||
| "wedged": false, # heartbeating fine, supervising nothing | ||
| "stage": "pretrain", # pipeline position, null off-pipeline | ||
| "stage_index": 1, | ||
| "stage_count": 3 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the probe example valid JSON.
The # annotations make this block invalid JSON even though it is presented as probe JSON output. Move the explanations outside the sample or label it as JSONC.
🤖 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/cli/commands/probe.py` around lines 20 - 26, Update the probe
output example containing last_exit_code, child_running, wedged, stage,
stage_index, and stage_count so it remains valid JSON by removing the inline #
annotations; preserve the field values and move explanations outside the JSON
sample or explicitly label the sample as JSONC.
Comment on lines
+174
to
+184
| # The monitor's own backstop threshold when set; otherwise fall back to | ||
| # the stall threshold so the probe still reports something useful for | ||
| # users who disabled the backstop. | ||
| wedge_threshold = float(cfg.monitor.dead_child_grace_seconds or stall_threshold) | ||
| wedged = ( | ||
| pid_alive | ||
| and not stale | ||
| and not child_running | ||
| and child_dead_seconds is not None | ||
| and child_dead_seconds > wedge_threshold | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the documented dead_child_grace_seconds: 0 disablement.
cfg.monitor.dead_child_grace_seconds or stall_threshold converts an explicit 0 into the stall timeout, so autosentry probe can still report wedged and exit nonzero. This contradicts the configuration reference, which says 0 disables the backstop; either preserve that semantics here or document the probe as intentionally independent.
🤖 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/cli/commands/probe.py` around lines 174 - 184, Update the
wedge threshold logic near the wedged calculation to preserve an explicit
cfg.monitor.dead_child_grace_seconds value of 0 as disabled rather than
replacing it with stall_threshold. Ensure the probe cannot report wedged from
this backstop when the configured grace period is zero, while retaining the
configured positive threshold and existing fallback behavior for an unset value.
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.
Fixes #22.
Diagnosis
The monitor's exit edge was:
last_exit_codepersists across Monitor instances. A pipeline builds a freshMonitorper stage over one sharedstate.json, so stage N's clean exit leaveslast_exit_code: 0on disk, and when stage N+1's child also exits 0 the comparison is0 != 0— false. The edge never fires,_handle_exitnever runs, the stage never completes, and the loop spins forever over a dead child while the heartbeat keeps advancing.That accounts for every symptom in the report: the log ending at
Starting processwith nothing after,last_exit_code: 0, zero children, a live heartbeat 18h paststarted_at, and the first stage advancing normally (fresh state →0 != None→ edge fires). The same trap caught any secondautosentry runin a directory whose previous run exited 0, pipeline or not.Reproduced, then fixed
Pre-fix, on a real three-stage pipeline of
sh -c 'echo …; sleep 1':It also confirms the reporter's note that SIGTERM marks the current stage complete and skips ahead —
timeout 60fired, stage_two was marked complete, and stage_three wedged the same way. Post-fix the same pipeline runs all three stages to completion and exits 0. The smoke script is kept at~/Workspace/scripts/autosentry-issue-22-smoke.sh.What changed
The stall (suggestion 1). The exit edge now keys on the child's
started_at— every supervisor stamps a fresh one instart()— so it fires exactly once per child regardless of what a previous stage or run left behind. This also retires thelast_exit_code = Nonehack in_restart_policy_fallback, so that field can go back to meaning the last exit code actually observed.restart_always+ clean exit — the same wedge by a different road. The defaultexit_codedetector is non-zero-only, so a clean exit fired no detection, no healer ran, and therestart_policyfallback (which hangs off the detection path) never got a chance.restart_alwaysnow does the relaunch itself. It's audited inrestart_historybut doesn't spend the unverified-restart budget: that counter is the kill-switch for a healer that can't land a fix, and a service that exits 0 and comes back hasn't failed at anything.Detectability (suggestion 2). A heartbeat only proves a thread is scheduling.
state.jsonnow carrieschild_running,child_started_at, andchild_dead_sincenext tolast_heartbeat, plusstage/stage_index/stage_countso the current stage is answerable from state alone (previously the only record was areset_historyentry for the advance into the stage, which reads identically whether the stage is running or wedged).autosentry probeexposes them and reduces the pair to one field:pid_alive && !staleis exactly what a naive check calls healthy, so that's the case worth naming.probeexits 1 on it, so it drops into a cron liveness hook unchanged. Probeschema_version→ 2.autosentry statusand the TUI show child liveness beside the heartbeat.Stage-level backstop (suggestion 3).
monitor.dead_child_grace_seconds(default 900,0disables): past that long with no live child, the monitor logs, notifies, and exits nonzero. It's a backstop, not a normal path — every legitimate route out of a dead child resolves in seconds, and slow healer work blocks the loop rather than ticking through it. Any future variant of this failure becomes a loud pipeline failure instead of silent idle hours.On the template comment: the shipped
autosentry.yaml.tmpldoes configure astalldetector and has no "hang coverage comes from an external log-mtime watchdog" note — that looks like local config text rather than something autosentry ships. The template now documentsdead_child_grace_secondsso the in-box coverage is explicit either way.Tests
17 new regression tests in
tests/test_issue_22_pipeline_stall.pycovering the edge with pre-seededlast_exit_code: 0, per-child edge semantics across a restart, the fullrun()loop returning instead of spinning,restart_alwaysrelaunch and budget accounting, the watchdog (trips, resets, disables), state round-tripping, stage context in and out of the pipeline, andprobereporting wedged vs healthy. Full suite: 348 passed, lint + format + pyrefly clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
autosentry probewith wedged-monitor detection, stage information, and schema version 2.0to disable).Bug Fixes
restart_alwaysreliably relaunches after clean exits.