Skip to content

Ensure stop() kills moonraker supervisor process - #55

Open
jcastle97 wants to merge 1 commit into
rinkhals-community:developfrom
jcastle97:fix/moonraker-single-instance
Open

Ensure stop() kills moonraker supervisor process#55
jcastle97 wants to merge 1 commit into
rinkhals-community:developfrom
jcastle97:fix/moonraker-single-instance

Conversation

@jcastle97

Copy link
Copy Markdown

Summary

Fixes duplicate moonraker instances that were caused by app.sh stop() not killing their supervisors.

Why

app.sh stop() ran only kill_by_name moonraker.py, killing the moonraker python but not its supervisor. So a "stop" did not actually stop moonraker, and any restart (stop_app + start_app to reload) left the old immortal supervisor to respawn its original moonraker instance and race to bind port :7125.

Changes

  • 40-moonraker/app.sh: When stopping moonraker app, kill the supervisor first, then the python.

Testing

This was reproduced and the fix verified on two different KS1 printers. Known compatible on KS1 / 2.7.2.7 running VK app.

  • Existing tests in tests/ still pass (if applicable)
  • Manually tested on a real printer (model and firmware)
  • Documentation updated if user-visible behaviour changed

Notes for reviewers

There is a separate open question on whether moonraker should ever be allowed to have more than one instance / immortal supervisor. If not, then moonraker.sh itself can be hardened with a single-instance guard.

app.sh stop() ran only `kill_by_name moonraker.py`, killing the moonraker python but not its supervisor. So a "stop" did not actually stop moonraker, and any restart (stop_app + start_app to reload) left the old immortal supervisor to respawn its original moonraker instance and race to bind port :7125. Kill supervisor, then the python.

Reproduced and verified on two separate KS1 printers.
@lachlan-stevens

Copy link
Copy Markdown

I've also independently tested this change successfully on a KS1 running 2.7.0.9 w/ vanilla-klipper.

@martinbogo
martinbogo changed the base branch from master to develop July 6, 2026 21:51

@martinbogo martinbogo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, and the diagnosis is spot on. Confirmed on our side: moonraker.sh runs a while true restart loop that treats any non-zero exit as a crash and respawns after 10s, so killing only moonraker.py does trigger a respawn and the duplicate-instance race you described. The fix targets a real bug. (I've also retargeted the PR base from master to develop, which is where we take changes.)

One change before we merge: kill_by_name defaults to signal 9 (SIGKILL), so kill_by_name moonraker.sh hard-kills the supervisor and bypasses the graceful cleanup() trap it already implements (SIGTERM -> wait up to 10s -> SIGKILL). Moonraker keeps a SQLite database, and that trap exists specifically so it can close cleanly; a hard kill risks cutting a write short.

Could you send the supervisor a SIGTERM instead so its cleanup path runs, e.g. kill_by_name moonraker.sh 15? Note that the trap itself already stops moonraker.py gracefully (and force-kills it after 10s if needed), so with a SIGTERM to the supervisor the immediate kill_by_name moonraker.py afterwards would pre-empt that 10s window with a SIGKILL. Please rework the ordering so the graceful shutdown gets its chance, with a hard kill only as a fallback for stragglers. Happy to re-review once updated.

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.

3 participants