feat(hooks): add on_stop and on_exit

[gaojunran]

addresses #285.

[gaojunran]

@greptileai

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances Pitchfork's daemon lifecycle management by introducing on_stop and on_exit hooks. These additions provide users with more granular control over actions performed during daemon termination, allowing for more robust cleanup, logging, and notification strategies regardless of how or why a daemon stops. The accompanying PITCHFORK_EXIT_REASON environment variable further empowers these hooks by providing context for the termination event.

Highlights

• New Lifecycle Hooks: Introduced two new lifecycle hooks: on_stop and on_exit. The on_stop hook fires when a daemon is explicitly stopped by Pitchfork, while on_exit fires on any daemon termination (clean exit, crash, or explicit stop), providing comprehensive cleanup and notification capabilities.
• Enhanced Exit Information: Added a new environment variable, PITCHFORK_EXIT_REASON, available to on_stop and on_exit hooks, which indicates why a daemon terminated (e.g., 'stop', 'exit', 'fail'). The PITCHFORK_EXIT_CODE variable's availability was also extended to these new hooks.
• Documentation and Schema Updates: Updated CLI documentation, configuration reference, lifecycle hooks guide, and JSON schema to reflect the new on_stop and on_exit hooks, including usage examples and details on the new environment variables.
• Core Logic and Testing: Implemented the core logic in the supervisor to correctly fire these new hooks under various daemon termination scenarios and added comprehensive unit tests to ensure their proper functionality and environment variable propagation.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces on_stop and on_exit lifecycle hooks, which is a great feature for resource cleanup and notifications. The implementation is well-tested and the documentation is updated thoroughly across all relevant files. I've found one area in the core lifecycle logic with significant code duplication that could be refactored to improve maintainability. See my specific comment for details.

[greptile-apps]

Greptile Summary

This PR adds two new lifecycle hooks — on_stop (fires when a daemon is explicitly stopped by pitchfork) and on_exit (fires on any daemon termination: stop, clean exit, or crash-after-retries-exhausted) — completing the lifecycle hook surface for daemon supervision.

Key implementation decisions:

• on_exit correctly guards behind hook_retry_count >= hook_retry so it does not fire on intermediate crash-retry cycles, matching the documented semantics and backed by test_hook_on_exit_not_fired_during_retries.
• PITCHFORK_EXIT_CODE now uses -1 (not 0) for signal-killed processes, matching the updated documentation.
• PITCHFORK_EXIT_REASON ("stop" / "fail" / "exit") is injected into all exit-path hooks so scripts can act on the cause.
• The previous concern about on_stop/on_exit being silently dropped during supervisor shutdown is resolved: hooks.rs registers each JoinHandle in SUPERVISOR.hook_tasks, and close() now uses an active_monitors counter + Notify to wait for in-flight monitoring tasks before draining those handles (with a 30 s per-task timeout and a 5 s drain timeout).

The only minor issue found is a documentation inaccuracy: the env-var table describes PITCHFORK_EXIT_REASON as "Available in on_stop and on_exit", but the implementation also passes it to on_fail (always as "fail"). This is a cosmetic fix and does not affect runtime behaviour.

Confidence Score: 5/5

• Safe to merge; all prior review concerns are addressed and the only remaining note is a one-line documentation clarification.
• All three previously-flagged issues (exit-code 0 for signal-killed, on_exit firing during retries, shutdown race) are resolved with targeted fixes. Test coverage for the new hooks is thorough. The sole remaining finding is a minor doc inaccuracy about which hooks receive PITCHFORK_EXIT_REASON — it has no runtime impact.
• No files require special attention.

Important Files Changed

Filename	Overview
src/supervisor/lifecycle.rs	Core hook-firing logic for on_stop/on_exit added cleanly; correctly guards on_exit behind retry exhaustion; uses -1 (not 0) for signal-killed exit codes; active_monitors counter added to close() to await monitoring tasks before draining hook_tasks.
src/supervisor/hooks.rs	OnStop/OnExit variants added to HookType enum; hook_tasks JoinHandle registration added to fire_hook() so shutdown can await in-flight hooks; implementation is clean.
src/supervisor/mod.rs	close() now properly awaits all in-flight monitoring tasks using active_monitors/monitor_done before draining hook_tasks with a 30-second per-task timeout, addressing the prior shutdown-race concern.
tests/test_hooks.rs	Good test coverage: on_stop, on_exit-on-stop, on_exit-on-fail, on_exit-clean-exit, both-fire-on-stop, and crucially test_hook_on_exit_not_fired_during_retries all added.
docs/guides/lifecycle-hooks.md	Well-written documentation for new hooks; retry semantics note added; PITCHFORK_EXIT_CODE -1 for signal-killed documented; minor inaccuracy: PITCHFORK_EXIT_REASON availability omits on_fail.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Daemon process exits] --> B{is_stopping?}
    B -- yes --> C[exit_reason = stop]
    B -- no --> D{status.success?}
    D -- yes --> E[exit_reason = exit]
    D -- no --> F{hook_retry_count >= hook_retry?}
    F -- yes --> G[exit_reason = fail]
    F -- no --> H[No hooks fired\nDaemon marked Errored\ncheck_retry schedules restart]
    H --> I[on_retry fires before next attempt]

    C --> J[Fire on_stop + on_exit]
    E --> K[Fire on_exit only]
    G --> L[Fire on_fail + on_exit]

    J --> M[Register JoinHandles in hook_tasks]
    K --> M
    L --> M

    N[Supervisor shutdown / signal] --> O[close: stop each daemon]
    O --> P[Wait active_monitors == 0\nup to 5s]
    P --> Q[Drain hook_tasks\n30s per task]
    Q --> R[exit 0]

Loading

_{Reviews (8): Last reviewed commit: "feat(hooks): add `on_stop` and `on_exit`" | Re-trigger Greptile}

[gaojunran]

@greptileai

[gaojunran]

@greptileai

[gaojunran]

@greptileai

[aFuzzyBear]

Great work @gaojunran, really appreciate the speed 🔥
Quick question, how would one be able to test these changes out?
Got it figured out, gonna give this a wee shot =)

[aFuzzyBear]

Fuzzy Manual testing on this branch, it seems the hooks are indeed working correctly 🥳
Tested on WSL2 Ubuntu, pitchfork built from this branch.

Test 1: Explicit stop against a live process

[daemons.test]
run = "sleep 10"

[daemons.test.hooks]
on_stop = "sh -c 'echo on_stop: $PITCHFORK_EXIT_REASON >> /tmp/pf.log'"
on_exit = "sh -c 'echo on_exit: $PITCHFORK_EXIT_REASON >> /tmp/pf.log'"

Result: both on_stop: stop and on_exit: stop fired consistently across 10 runs. Hook ordering between on_stop and on_exit was non-deterministic run-to-run which is expected given the fire-and-forget async semantics discussed in the design,

Test 2: Natural process termination (no explicit stop)

[daemons.test-race]
run = "sleep 0.5"

[daemons.test.hooks]
on_stop = "sh -c 'echo on_stop: $PITCHFORK_EXIT_REASON >> /tmp/pf.log'"
on_exit = "sh -c 'echo on_exit: $PITCHFORK_EXIT_REASON >> /tmp/pf.log'"

Loop of 20 runs — start daemon, wait 0.4s, attempt stop (daemon already gone), wait 0.5s for async hooks to settle, read log.

for i in $(seq 1 20); do
  rm -f /tmp/pf-race.log
  pitchfork start test-race
  sleep 0.4
  pitchfork stop test-race
  sleep 0.5
  echo "Run $i: $(cat /tmp/pf-race.log 2>/dev/null | tr '\n' '|' || echo 'NO HOOKS')"
done

Result: on_exit: exit fired 20 out of 20. on_stop correctly absent since the process had already exited before pitchfork stop arrived, so no explicit stop occurred.
PITCHFORK_EXIT_REASON semantics confirmed correct sequence of events.

• exit on natural process termination
• stop on explicit pitchfork stop

One note for documentation: hooks are fire-and-forget async, so reading their output immediately after pitchfork stop returns can produce false negatives. A short settle delay is needed in the test harnesses that check hook side effects. Worth a note in the lifecycle hooks guide so users don't assume hooks have completed when the CLI returns.

Untested: the P1 race condition flagged by greptile (hooks dropping when stop() clears PID atomically before the monitoring task runs). The sub-second process lifetime approach test-race
couldn't isolate this cleanly it would appear that the supervisor registration latency dominated. The race window would need a targeted unit test at the Rust level to probe reliably.

Overall the happy path is rock solid!!!

PITCHFORK_EXIT_REASON 👌 being able to branch on termination cause in hook logic covers real use cases. Great work @gaojunran, I really do appreciate the speed on this one

🔥💚🔥

[gaojunran]

@aFuzzyBear thank you for your appreciation! there remains some CR comments and I will deal with them tomorrow.

[aFuzzyBear]

I cant say thank you enough

💚

[gaojunran]

@greptileai

[gaojunran]

@greptileai

[gaojunran]

@greptileai

[gaojunran]

@jdx Ready for a review. It seems greptileai has no more comments and now it's more stable.