audio: reap the pipeline's children when nobody is watching - #211
Merged
Conversation
#204 narrowed `reap_zombies` from draining `waitpid(-1)` to sweeping only PTY-owned pids, which stopped it stealing exit statuses from the audio pipeline's own `try_wait`. It also stopped it covering for two places where the audio pipeline does not collect its own children. The first is a plain bug: the "dbus-daemon exited without printing an address" bail-out kills the child and returns without waiting it. Every other bail-out in `spawn` waits what it kills; this one was missed, so a dbus-daemon that started and then said nothing stayed a zombie for the life of the server. The second is structural, and the same duty-cycle problem #204 fixed for terminals. `is_alive` is what collects dead sub-processes, as a side effect of the `try_wait` calls it makes to decide whether to heal — and it is only called from the delivery tick, which does not run while no client is attached. So an idle server whose PipeWire died kept the corpse until somebody connected. Give the pipeline a `reap_children` that only collects, with none of `is_alive`'s restart behaviour, and call it from the supervisor, which is the loop that exists precisely because it runs when the tick does not. Verified the dbus path with a fake `dbus-daemon` on PATH that exits without printing: before, the server is left with a `dbus-daemon <defunct>` child; after, the same log line and no children at all. The `reap_children` half is not verified end-to-end here — PipeWire will not start on this machine ("pipewire exited before creating its socket"), so there is no live pipeline whose child I can kill and watch get collected. It is a `try_wait` on each stored `Child` with no other state change, called from a loop that already runs every 5s.
|
Review closed. |
Coverage
|
Contributor
Author
|
Both caveats from this PR are now closed. Neither needed a code change, but the first one deserved better than the shrug I gave it. 1.
|
| build | at t+10s |
|---|---|
5f26eb65 (before this PR) |
wireplumber <defunct> — still there |
98bce889 (after) |
reaped, no children in Z |
That is exactly the path the PR claims to fix, and it is the one I could not exercise before.
2. Unified child ownership — audited, not needed
The open question was whether to move the audio children onto the same owned-pid registry PTYs use, so one reaper covers everything. I went through every Command::spawn in the tree instead:
- git (
reads.rs:362,:496) —.status()on one path,.spawn()+ explicit.wait()on both branches of the other. Reaps itself. - LSP (
backend.rs:81) — every exit path istry_wait→kill→wait. Reaps itself. - audio — covered by this PR.
- PTY — covered by server: own the PTY lifecycle — refusals, group kill, supervisor, deadlines, retention #204's owned-pid set plus the abandoned set.
So nothing is uncovered, and the refactor would move ownership out of AudioPipeline for no behavioural gain. Dropping it rather than carrying it as a TODO.
One piece of drift found while auditing, not worth its own PR but worth knowing: crates/lsp/src/backend.rs:718 still says "The daemon's global reaper may win the race and steal the status", which stopped being true when #204 narrowed the reaper to PTY-owned pids. The reap_backstop_status it pairs with is already an empty stub (:1994). Happy to delete both in a passing commit if you want the file to stop describing a race that no longer exists.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
…217) Follow-up to #204/#211, found while auditing which subsystems still reap their own children. #204 narrowed the daemon's backstop from draining `waitpid(-1)` to sweeping PTY-owned pids only. It no longer touches LSP children at all, and three things in this crate still describe the world before that: - **`reap_backstop_status`** was already an empty stub (`fn reap_backstop_status(_pid: u32) {}`) whose doc comment explained a pid-recycling collision it no longer prevents. It and its two call sites go, along with the `pid` bindings that existed only to feed it. - **The `try_wait` comment** still warned that *"the daemon's global reaper may win the race and steal the status"*. It cannot. What's left is the ordinary case — an `Err` means the child is already gone — so the comment says that. - **`docs/design/lsp.md` § Reaping** described the backstop as reaping every child while parking statuses selectively. It reaps selectively now too. No behaviour change; the stub did nothing. ## On the doc I rewrote that bullet rather than deleting it, because the consequence is worth keeping and is not obvious: now that the backstop only sweeps what it owns, a subsystem that doesn't wait its own children **leaks zombies** instead of being quietly mopped up. That makes the engine's every-path `wait()` load-bearing rather than defensive. That's not hypothetical — it's exactly what #211 had to fix for the audio pipeline, where `is_alive` was the only thing collecting children and it only ran from the delivery tick. The LSP engine was already correct here; the note is so the next person adding a subprocess knows the floor moved. `nix run .#lint` exit 0, `cargo test -p blit-lsp` 39 passing.
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.
Follow-up to #204, which I flagged there while verifying it.
#204 narrowed
reap_zombiesfrom drainingwaitpid(-1)to sweeping only PTY-owned pids. That was the right call — the global drain was reaping other subsystems' children and discarding their statuses, which is what broke the audio pipeline's owntry_wait. But it also stopped it covering for two places where the audio pipeline does not collect its own children, and both are live onmainnow that #204 has merged.A missed wait. The
dbus-daemon exited without printing an addressbail-out kills the child and returns without waiting it. Every other bail-out inspawnwaits what it kills; this one was missed.A duty-cycle gap — the same one #204 fixed for terminals.
is_aliveis what collects dead sub-processes, as a side effect of thetry_waitcalls it makes to decide whether to heal, and it is only reached from the delivery tick. The tick does not run while no client is attached, so an idle server whose PipeWire died kept the corpse until somebody connected.reap_childrencollects and does nothing else — none ofis_alive's restart behaviour, which is not something to run on a timer nobody asked for — and the supervisor calls it, that loop existing precisely because it runs when the tick does not.Verification
The dbus path is A/B'd with a fake
dbus-daemononPATHthat exits without printing anything, which drives exactly that branch:[audio] failed to start pipeline: dbus-daemon exited without printing an address, and the server is left with adbus-daemon <defunct>childThe
reap_childrenhalf is not verified end-to-end. PipeWire does not start on the machine I have (pipewire exited before creating its socket), so there is no live pipeline whose child I can kill and watch get collected. What it does is atry_waiton each storedChildwith no other state change, called from a loop that already runs every 5s — but that is an argument, not a test, and someone with working audio should confirm it.nix run .#lintexit 0,cargo test -p blit-server287 passing.Note on scope
There is a third option I did not take: registering the audio children with the same owned-pid mechanism PTYs use, so one reaper covers everything. That is the tidier end state, but it means moving ownership out of
AudioPipeline, and this fix is small enough to stand alone and stop the bleeding first.