Graceful shutdown on SIGTERM + fix rootfs mount leak that wedges slots#16
Merged
Merged
Conversation
Two fixes for issue #15: 1. Replace the no-op SIGTERM/SIGINT handler with a real shutdown path: the signal sets a global flag, a watcher thread SIGKILLs registered Firecracker process groups so blocked child.wait() calls return, and each slot loop tears down (unmount, PID file) and exits, letting the manager exit 0 instead of hanging until systemd's stop timeout and getting SIGKILLed with everything it owns. 2. Fix the rootfs mount leak that wedges slots: setup_run() fetched the GitHub registration token and injected config while the rootfs was mounted, so any error in that window (a GitHub API hiccup suffices) returned with the mount held -- making every subsequent boot cycle fail 'lvcreate snapshot failed: contains a filesystem in use' forever. The token is now fetched before mounting, the unmount runs on the error path too, and each cycle defensively cleans up a stale mount left by a crash or SIGKILL before touching LVM. Three staging slots wedged this way within 5 days (appsignal/appsignal-ansible#806). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✔️ All good! |
matsimitsu
approved these changes
Jul 15, 2026
guifranchi
marked this pull request as ready for review
July 16, 2026 20:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15. Draft for review discussion, not to be merged/deployed yet.
What
Two independent fixes, one per defect described in #15:
1. Graceful shutdown (new
manager/src/shutdown.rs)The v0.1.11 handler ignored SIGTERM/SIGINT (
SigHandler::SigIgn), so the manager could never exit on a stop signal — everysystemctl stop/restarthung forTimeoutStopSecand ended in systemd SIGKILLing the manager and all Firecracker VMs (how ci-hel3 ended up dead for 4 days).Now: the signal sets an
AtomicBool; a watcher thread SIGKILLs each registered Firecracker process group (theysetsid()at spawn) so the slots' blockedchild.wait()calls return; each slot loop observes the flag, runs teardown (stale-mount cleanup, PID file removal), and exits; the manager joins all slots and exits 0. The 20s boot-failure back-off is now interruptible, and reattached VMs (PID-file recovery path) are registered with the watcher too.Design choice: running VMs are killed, not drained. Same job impact as today's SIGKILL, but with clean teardown and a fast, bounded stop. A drain-with-grace-period can be layered on later if wanted.
2. Rootfs mount leak
setup_run()fetched the GitHub registration token and injected config while the rootfs was mounted; any error in that window (a GitHub API hiccup is enough) returned with the mount still held. Mounts are kernel state. From then on every cycle failslvcreate snapshot failed: ... contains a filesystem in use, retried every 20s forever. Three staging slots wedged this way in 5 days (appsignal/appsignal-ansible#806: ci-hel1 slot 0, ci-hel2 slot 7, ci-hel3 slot 6).Fixes:
mntleft by a crash/SIGKILL before touching LVM, so a wedged slot self-heals on the next cycle instead of requiring manualfuser -km/umount/lvremove.Testing
cargo check,cargo test --workspace(12 tests),cargo clippy, all clean.systemctl restart actions-runnermid-job --> unit stops in seconds, exits 0 (noFailed with result 'timeout'), no stale/srv/runners/*/*/mntmounts, slots come back cleanly.Related: #14 (cache LV fixes), ideally both ship together in v0.1.12 so the fleet needs a single rollout. Ansible-side interim mitigation: appsignal/appsignal-ansible#811.