Stop systemd killing the agent's filesystem utilities - #24
Draft
artemnikitin wants to merge 1 commit into
Draft
Conversation
Volume work runs mkfs.ext4, e2fsck, and resize2fs as child processes, and the agent detaches them from its own shutdown so a restart or node drain cannot interrupt a resize partway through — the case its resize transaction and e2fsck -f -y exist to recover from. That protection was defeated here. This unit sets neither KillMode nor TimeoutStopSec, so systemd's default KillMode=control-group signals every process in the cgroup on stop, including the detached utility, and force-kills the whole group once the default stop timeout expires. A drain during a shrink killed resize2fs exactly as if the agent had never detached it. KillMode=mixed sends SIGTERM to the agent alone. TimeoutStopSec=2100 exceeds the agent's 30-minute destructive-command deadline plus its grace period; the two are a contract, and raising one without the other reopens the gap. The requirement is documented in docs/persistent-volumes.md in the firework repository, which is where the deadline lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
artemnikitin
added a commit
to artemnikitin/firework
that referenced
this pull request
Aug 26, 2026
All five reproduce; each in-repo fix has a test that fails without it. 1. Retained manifests with a non-positive applied size bypassed pool capacity. readRetained fed AppliedSizeBytes straight into the reserved total, and a negative one *subtracts* — a single corrupt or hand-edited manifest admitted a 150 MiB volume into a 100 MiB pool. There is no safe number to assume for such a record, so it fails closed, and the summation additionally rejects a non-positive size from any source: a total that can be driven downwards is a capacity bypass whatever produced it. 2. A stale heartbeat reopened a withdrawn refusal. acknowledgeVolumeRecords matched on the generation alone, but a record whose refusal was withdrawn sits at that same generation — so a stale observation set it back to rejected, the next desired-state pass cleared it again, and the pair repeated every tick. Two durable writes per tick is bad; a crash between them leaves the degraded state behind. A rejected observation is now accepted only while the refusal is still outstanding. 3. An explicit zero-volume prior render was mistaken for a missing snapshot. Substituting the desired configuration there rendered exactly the unvalidated volume config the hold exists to gate. Zero volumes is valid prior state; a genuinely missing snapshot cannot reach that code at all, because heldPlacementUnrecoverable stops the cycle before anything is published. 4. ValidateNodeVolumes checked volume declarations but not the service name, which the agent also turns into a path component — so a service named "bad/name" passed configcheck and failed agent preflight. It now goes through volumeDir, the same function the agent uses, rather than restating the pattern. The fifth finding cannot be fixed here. Detaching a destructive command from the Go context does not escape the agent's systemd cgroup: under the default KillMode=control-group, stopping the unit signals the detached mkfs or resize2fs directly and force-kills it at TimeoutStopSec. The required supervision contract — KillMode=mixed and a TimeoutStopSec above destructiveCommandTimeout — is now documented in docs/persistent-volumes.md and on the constant itself, and implemented in artemnikitin/firework-deployment-example#24. Refs #39 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Found reviewing artemnikitin/firework#50, which hardens persistent local volumes. One of its fixes is only half-effective without this change, and the other half is here rather than there.
The gap
Creating, checking, or resizing a volume runs
mkfs.ext4,e2fsck, andresize2fsas child processes of the agent. Interrupting one partway is what the resize transaction ande2fsck -f -yexist to recover from, so firework#50 detaches those commands from the agent's own shutdown: they survive its context being cancelled, and get their own deadline with a SIGTERM-then-wait rather than an immediate kill.This unit defeats that. It sets neither
KillModenorTimeoutStopSec, so systemd appliesKillMode=control-group— its default. On stop or restart systemd signals every process in the unit's control group, the detached utility included, and escalates to SIGKILL for the whole group once the default stop timeout expires. A node drain during a shrink killsresize2fsexactly as if the agent had never detached it, which is the scenario the firework-side fix names.Detaching from a Go context cannot escape a cgroup. The supervision contract is the only place this can be fixed.
The change
KillMode=mixedsends SIGTERM to the main process only — the agent still shuts down promptly — while a utility it detached keeps running.TimeoutStopSec=2100(35 minutes) exceeds the agent'sdestructiveCommandTimeoutof 30 minutes plus its grace period; below that, systemd force-kills the utility at the timeout and nothing has been gained.The two numbers are a contract across repositories.
destructiveCommandTimeoutlives ininternal/volumein the firework repo and bounds the command;TimeoutStopSecmust stay above it. Raising either alone reopens the gap. firework#50 documents this indocs/persistent-volumes.mdand in a comment on the constant itself.Trade-off worth knowing
A stop or restart can now block for up to 35 minutes if a large
resize2fsis genuinely in flight, where previously it was killed promptly. That is the intended exchange: the alternative is a filesystem interrupted mid-resize. In steady state nothing is running and the stop is immediate.Validation
bash -nandshellcheckclean. Not validated on a live node — this changes how the unit behaves under stop/restart, and the live SIGTERM-during-shrink test that would exercise it is one of the paths firework#50 lists as still blocked on the issue #42 validation lab. Reviewing this against a real drain is the thing I would most want a second pair of eyes on.