Fix daemon-lifecycle signal handling and unbounded stats growth#101
Conversation
Closes #76: on_sigchld no longer tears the daemon down whenever the last child dies. active_processes() still prunes exited children, but the daemon only stops when child_abort (now set on a failed spawn), max_messages, or is_shutting_down is true; otherwise the next poll respawns via check_process_counts. Preserves the max_messages shutdown path. Closes #89: replace signal.pause() in run() with a bounded time.sleep loop that re-checks is_running and the new stop_requested flag each iteration, closing the lost-wakeup race where a signal delivered between the loop check and pause() could wedge the process forever. Closes #90: controller.run() now stops children in a finally, so the KeyboardInterrupt and propagating-exception paths both invoke stop_processes() and non-daemon children cannot leak or hang the parent at atexit. Closes #91: SIGHUP/SIGTERM handlers no longer call stop_processes() from the signal handler. They cancel the itimer and set mcp.stop_requested; the main flow performs stop_processes() once the run loop unwinds, so a poll can no longer interleave and respawn orphaned children. check_process_counts()/start_processes() also bail when not is_running. Closes #92: calculate_stats() no longer mutates its input (reads the timestamp instead of deleting it) and returns a fresh process_data dict instead of aliasing last_poll_results. retire_poll_results() prunes a dead process's per-process entry (bounding memory under churn) while folding its final counts into a per-consumer baseline, keeping the per-consumer totals monotonic so prometheus.update's clamped deltas do not drop counter increments. state.py: set_state now rejects the specific STOPPED->ACTIVE transition (raises ValueError) to prevent resurrecting a stopped object; combined with a controller guard that skips run() when shutdown was requested during MCP construction (#99). No other transitions were restricted, so process.py/connection.py flows are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughController and MasterControlProgram coordinate shutdown via flags ( ChangesMCP and Controller shutdown/stats overhaul
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SignalHandler
participant Controller
participant MCP
participant ChildProcess
SignalHandler-->>Controller: SIGTERM
Controller->>Controller: _shutdown_requested = True
Controller->>MCP: stop_requested = True
MCP->>MCP: sleep loop checks stop_requested
MCP-->>Controller: run() returns
Controller->>MCP: stop_processes() (finally)
MCP->>ChildProcess: terminate/join
sequenceDiagram
participant MCP
participant ChildProcess
participant Stats
ChildProcess-->>MCP: SIGCHLD (child exits)
MCP->>MCP: check child_abort/max_messages/is_shutting_down
alt should stop
MCP->>MCP: set_state(STOPPED)
else respawn
MCP->>MCP: retire_poll_results(consumer, name)
MCP->>Stats: fold counts into consumer_baselines
MCP->>MCP: wait for next poll to respawn
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Fixes 6 verified daemon-lifecycle bugs in
mcp.py,controller.py, andstate.py, centered on a coherent signal/shutdown flow and bounded stats growth.Problem / Solution
on_sigchldshut the daemon down when the last child died instead of respawning → only stops onchild_abort/max_messages/is_shutting_down; otherwise prunes and lets the next poll respawn. Wired up the previously-deadchild_abortflag.is_runningcheck andsignal.pause()→ replaced with a bounded wait loop that re-checksis_running/stop_requested.KeyboardInterrupt/exception exit paths never stopped children (parent hung at atexit join) →controller.run()stops processes in afinally.stop_processes()in a signal handler could interleave withpoll()and respawn orphaned children → handlers now only set a flag + cancel the itimer;check_process_counts/start_processesbail when not running; shutdown happens on the main thread.last_poll_resultsgrew without bound and inflated logged stats → dead-process entries are pruned with their final counts folded into a retained per-consumer baseline (keeps Prometheus deltas correct); also fixed input mutation and theprocess_dataaliasing.run()was nullified andset_stateallowed STOPPED→ACTIVE → checks_shutdown_requestedbeforerun();set_staterejects the STOPPED→ACTIVE transition.Note on
state.pyset_statenow raisesValueErroron exactly one transition (STATE_STOPPED → STATE_ACTIVE). Verified safe forprocess.py/connection.py, which never make that transition. No general transition table was added.Testing
Full suite green (235 tests) + ruff clean. Tests in
tests/test_mcp.py,tests/test_controller.py,tests/test_state.py.Closes #76
Closes #89
Closes #90
Closes #91
Closes #92
Closes #99
Summary by CodeRabbit