lsp: drop the reaper-race workaround, which has nothing left to race - #217
Merged
Conversation
#204 narrowed the daemon's backstop from draining `waitpid(-1)` to sweeping PTY-owned pids only, so it no longer touches LSP children at all. Three things in this crate still describe the world before that. `reap_backstop_status` was already an empty stub whose doc comment explained a collision it no longer prevents; it and its two call sites go, along with the `pid` bindings that existed only to feed it. The comment on the shutdown `try_wait` still warned that "the daemon's global reaper may win the race and steal the status". It cannot. What is left is the ordinary case — an `Err` means the child is already gone — so the comment says that instead. `docs/design/lsp.md` § Reaping described the backstop as reaping every child and parking statuses selectively. It now reaps selectively too, so the trap it names is gone. The replacement records the consequence rather than deleting the note, because the consequence is the part worth keeping: a subsystem that does not wait its own children now leaks zombies instead of being quietly mopped up, which is what made the engine's every-path `wait()` load-bearing rather than defensive. That same narrowing is what #211 had to fix for the audio pipeline. No behaviour change — the stub did nothing.
|
Coverage
|
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/#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_statuswas 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 thepidbindings that existed only to feed it.try_waitcomment 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 — anErrmeans 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_alivewas 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 .#lintexit 0,cargo test -p blit-lsp39 passing.