Skip to content

fix: don't leave hooks permanently marked as running when a run fails - #13

Closed
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/hook-run-wedge
Closed

fix: don't leave hooks permanently marked as running when a run fails#13
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/hook-run-wedge

Conversation

@usefulish

Copy link
Copy Markdown
Contributor

hook::run inserts the hook id into running_hooks before spawning, but never removes it if the spawn fails or if handing the child to the reaper fails (reaper gone → closed channel). Either failure leaves the hook permanently marked as running: with allow_concurrent unset it never runs again for the daemon's lifetime.

This unmarks the hook on both failure paths, and makes the reaper survive a try_wait error instead of exiting its loop (a dead reaper closes the handoff channel, which is what makes the second path reachable; that last part has no unit test since try_wait can't easily be made to fail).

Red-green tests: failed_spawn_unmarks_hook and reaper_handoff_failure_unmarks_hook both fail on master (the id stays in running_hooks), pass with the fix.

🤖 Generated with Claude Code

A hook whose command failed to spawn, or whose process could not be
handed to the reaper thread, stayed marked as running forever: with
allow_concurrent unset the hook never ran again. Unmark it on both
failure paths, and make the reaper survive a process status check
error instead of exiting, which was the main way the handoff channel
could close in the first place.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
desbma added a commit that referenced this pull request Jul 15, 2026
@desbma

desbma commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Thanks for the report, the leak on the failure paths is real, but I reworked the fix differently in ea283e1, so closing this.

Two notes on the premise:

  • "it never runs again for the daemon's lifetime" doesn't actually happen: every hook::run call site in main propagates errors with ?, so a failed spawn or send exits the daemon immediately. Exiting is also the behavior I want for the reaper handoff case, rather than unmarking and continuing, so that part of the fix and its test are dropped.
  • Surviving try_wait errors traded an explicit failure for a silent inconsistent state. With no signal handling in the daemon, waitpid(WNOHANG) can only fail with ECHILD if something external reaped our child, and I prefer to crash loudly on that.

The committed fix instead ties the "running" mark to the child process lifetime: the reaper channel carries a RunningHook owning the child and an Arc<()> token, and the map holds Weak<()>, so the mark can't outlive the process being tracked, whatever the failure path.

@desbma desbma closed this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants