feat: replace supervised roles that die - #16
Conversation
Greptile SummaryThe PR releases version 0.8.0 with automatic replacement of crashed supervised roles and periodic dead-process cleanup.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant S as Supervisor
participant R as Role Thread
participant M as Monitor Thread
participant P as ProcessRegistry
S->>R: start supervised role
S->>M: start monitor
loop while supervisor is started
M->>R: check alive?
alt role thread died
M->>M: lock lifecycle and re-check started
M->>R: construct and start replacement
end
M->>P: cleanup dead processes when due
M->>M: sleep monitor interval
end
S->>S: lock lifecycle and clear started
S->>M: join, then terminate if blocked
S->>R: request shutdown and join
Reviews (6): Last reviewed commit: "chore: prepare 0.8.0 release" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
| rescue | ||
| retry if @started | ||
| end |
There was a problem hiding this comment.
If dead-process cleanup encounters a persistent database failure, this rescue retries before reaching the configured sleep, causing the monitor to consume CPU and preventing subsequent role-replacement checks until the failure clears.
| rescue | |
| retry if @started | |
| end | |
| rescue | |
| sleep SolidObjects.configuration.supervisor_monitor_interval if @started | |
| retry if @started | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 81-83
Comment:
**Maintenance failures spin the monitor**
If dead-process cleanup encounters a persistent database failure, this rescue retries before reaching the configured sleep, causing the monitor to consume CPU and preventing subsequent role-replacement checks until the failure clears.
```suggestion
rescue
sleep SolidObjects.configuration.supervisor_monitor_interval if @started
retry if @started
end
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. The rescue retried immediately, so a persistently failing pass, an unreachable database being the obvious case, would spin without sleeping and starve the supervision it exists to perform.
The rescue now sits inside the loop, emits solid_objects.supervisor.monitor_failed, and the interval sleep always runs. A test asserts the failure count over a fixed window stays bounded by the interval, so spinning fails the assertion rather than merely looking slow.
|
@greptileai review |
|
@greptileai review |
| # replacement either completes before shutdown reads the component | ||
| # list, or never starts. | ||
| @lifecycle.synchronize { @started = false } | ||
| @monitor&.join(SolidObjects.configuration.supervisor_monitor_interval * 2) |
There was a problem hiding this comment.
When cleanup_dead_processes remains blocked longer than twice supervisor_monitor_interval, the timed join expires and stop continues, causing the monitor thread and its database maintenance operation to remain alive after supervisor shutdown returns.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/solid_objects/supervisor.rb
Line: 59
Comment:
**Monitor outlives supervisor shutdown**
When `cleanup_dead_processes` remains blocked longer than twice `supervisor_monitor_interval`, the timed join expires and `stop` continues, causing the monitor thread and its database maintenance operation to remain alive after supervisor shutdown returns.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. The join was bounded by twice the monitor interval, so a pass blocked on the database, exactly the situation the pacing fix anticipated, outlived the supervisor and stop returned with the thread still running.
Shutdown now joins for the configured shutdown timeout and kills whatever remains. Killing is safe here specifically because the monitor only performs maintenance: it replaces threads and prunes dead process records, and holds no committed work that could be lost. Test blocks a maintenance pass, stops, and asserts the monitor is not alive afterwards.
|
@greptileai review |
1 similar comment
|
@greptileai review |
A role that raised left its thread dead while the process kept running and quietly did less work, with no signal beyond reduced throughput. The supervisor now watches its threads and restarts any that stopped before shutdown was requested, and prunes dead process records on the same monitor. Both intervals are configurable, a failing maintenance pass cannot stop the monitor, and no restart happens once shutdown begins.
Worker and ReminderScheduler run their shutdown cleanup in an ensure, so a crashed role reports itself stopped exactly like one that was asked to stop. Skipping stopped components therefore skipped every real crash and left the process permanently below its role count, which is the case the feature exists for. Key replacement on the supervisor still running, and build a fresh instance because the crashed one has already released its process record. Reject a non-positive monitor interval during validation, and merge the duplicated unreleased changelog section.
The monitor rescued and retried immediately, so a persistently failing maintenance pass, such as an unreachable database, would spin without sleeping and starve the supervision it was meant to perform. Rescue inside the loop, instrument the failure, and always sleep the monitor interval.
The monitor could pass its started check immediately before stop cleared the flag, then swap in a replacement while shutdown walked the component list, leaving a role that never received request_shutdown alive after stop returned. Flip the flag and perform the swap under one lock, so a replacement either completes before shutdown reads the list or never starts.
The monitor was joined for twice its interval, so a pass blocked on the database outlived the supervisor that owned it and stop returned while the thread was still alive. Join for the shutdown timeout, then kill what remains, since the monitor only performs maintenance and has no committed work to lose.
02e0758 to
bf39d20
Compare
1 participant
Roadmap milestone 1, and the gate on the production-readiness claim.
The problem
Supervisor#startspawned a thread per role and never looked at them again. A role that raised left its thread dead while the process kept running, so the deployment quietly did less work with no signal beyond reduced throughput. Nothing pruned dead process records either, so rows from crashed or killed processes accumulated.The fix
A monitor thread that restarts any role whose thread stopped before shutdown was requested, and prunes dead process records on an interval.
supervisor_monitor_interval(default 1.0s) governs how often threads are checkeddead_process_cleanup_interval(default 60.0s) governs pruning, and setting it to zero disables itsolid_objects.supervisor.role_replacedwith the role classTests
Seven integration tests: a crashed role is re-run, a repeatedly crashing role keeps being replaced, a healthy role is never restarted, no restart happens after shutdown, the replacement is instrumented, a process whose heartbeat stopped is pruned, and a maintenance pass that raises leaves the role running.
The crashing role is a test double with a bounded crash count, so the tests assert real thread death and real restart rather than stubbed calls.
Roadmap
docs/roadmap.mdmoves supervision out of "Partially implemented" into "Implemented and tested" and removes milestone 1, renumbering the rest, per the requirement added in #15.Compatibility
No database change, no migration, no initializer regeneration. Two additive settings with defaults. Existing applications gain replacement automatically, which is the intended behaviour; setting
dead_process_cleanup_intervalto zero opts out of pruning.Validation
Standard Ruby, RuboCop, RBS, Steep, and Brakeman clean. No version bump; say the word and I will add one.