refactor: derive the process-group isolation decision from a single source (#191) - #200
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A small refactor detail changes exception-wrapping behavior by moving spawn option evaluation outside the Process.spawn rescue block, which should be corrected to keep behavior strictly preserved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors ProcessExecuter::Commands::SpawnWithTimeout to make the “isolated into a new process group by this class” decision derived from a single source, and adds characterization specs to ensure process-group behavior remains unchanged across caller-supplied pgroup/new_pgroup options.
Changes:
- Add specs covering caller-supplied
pgroup: true,pgroup: 0, andnew_pgroup: truefor spawn option passthrough, timeout kill behavior, and abandoned-wait cleanup behavior. - Refactor isolation detection to
!process_group_options.empty?and capture the effective spawn options used forProcess.spawnfor later inspection in kill/cleanup paths.
File summaries
| File | Description |
|---|---|
| spec/process_executer_spawn_with_timeout_spec.rb | Adds characterization tests for caller-supplied process-group options to pin behavior during refactor. |
| lib/process_executer/commands/spawn_with_timeout.rb | Makes process-group isolation a single-source decision and captures effective spawn options for kill/cleanup logic. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Tests do not verify that spawn options are captured once and reused by the kill path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
from
August 31, 2026 01:32
7e83165 to
b9c346c
Compare
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
2 times, most recently
from
August 31, 2026 17:54
27d05e1 to
c269b9c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Subclass-overridden spawn options can cause incorrect cleanup behavior, and the new documentation contradicts the implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lib/process_executer/commands/spawn_with_timeout.rb:340
spawn_optionsis an explicit subclass override point, so a subclass can remove or override the automatically added group option (for example, returnsuper.merge(pgroup: false)). In that caseprocess_group_optionsremains nonempty even though the options actually passed toProcess.spawndid not make the child a group leader. The previous implementation returned false because it also checkedprocess_group_leader?; this version makes abandoned-wait cleanup unexpectedly kill and reap that child. Retain the leader check, which now reads the captured effective options.
!process_group_options.empty?
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
from
August 31, 2026 20:01
c269b9c to
1f98123
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The subclass-removal test fails on Windows because it overrides pgroup instead of the platform’s new_pgroup option.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
from
August 31, 2026 20:11
1f98123 to
7557b76
Compare
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
from
August 31, 2026 20:17
7557b76 to
4f25c05
Compare
…ource (#191) Commands::SpawnWithTimeout derived the decision "did this class isolate the subprocess into its own process group?" twice: #process_group_options computed it directly, and #isolated_in_new_process_group? re-derived it from the merged spawn options via #process_group_leader? plus repeated :not_set checks. The two were equivalent, but a reader had to prove it. Now #process_group_options is the single source of truth for whether this class asked for isolation, and #isolated_in_new_process_group? is !process_group_options.empty? && process_group_leader? -- the ask, plus confirmation (over the options actually used) that it took effect, so a subclass #spawn_options override that removes the added group option cannot make the abandoned-wait cleanup kill a child whose signal semantics this class never changed. The effective spawn options are captured once in #call -- after any subclass #spawn_options additions, such as Commands::Run's redirection overrides -- as a plain attr_reader written only by #call, so the kill path inspects the options actually passed to Process.spawn instead of rebuilding the merged hash on each check. #process_group_options is documented as deterministic (it depends only on the options, not mutated during #call, and the platform) and as never reflecting a subclass override, so an option a subclass contributes can make the subprocess a group leader for the timeout kill but never counts as isolation by this class. Characterization tests pin the behavior this refactor could silently break: caller-supplied pgroup: true / pgroup: 0 / new_pgroup: true still group-kill on timeout, pass through unchanged, and are left alone by the abandoned-wait cleanup; a subclass that removes the added group option (both platform keys) prevents the cleanup; and a subclass-contributed group option with no timeout is not isolation -- the one case where the new definition and the old one disagree, which makes the definitional change detectable by the suite. The kill-path unit specs drive a real (stubbed) spawn-then-kill lifecycle instead of a fabricated pre-spawn state. No public API or documented behavior changes; the only semantic difference from the old derivation is in the subclass-override corner documented above, which no subclass exercises.
jcouball
force-pushed
the
refactor-191-process-group-isolation
branch
from
August 31, 2026 20:26
4f25c05 to
d3807ec
Compare
3 tasks
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.
Implements #191.
Commands::SpawnWithTimeoutderived the decision "did this class isolate the subprocess into its own process group?" twice:#process_group_optionscomputed it directly, and#isolated_in_new_process_group?re-derived it from the merged spawn options via#process_group_leader?plus repeated:not_setchecks. The two were equivalent, but a reader had to prove it.Changes
#process_group_optionsis now the single source of truth for whether this class asked for isolation, and#isolated_in_new_process_group?is!process_group_options.empty? && process_group_leader?— the ask, plus confirmation over the captured options that it took effect. The leader check (a Copilot review finding) matters only when a subclass#spawn_optionsoverride removes the added group option: then no isolation happened and the abandoned-wait cleanup leaves the child alone, matching the cleanup's documented contract.#call— after any subclass#spawn_optionsadditions, e.g.Commands::Run's redirection overrides — as a plain attr_reader written only by#call, so the kill path inspects the options actually passed toProcess.spawninstead of rebuilding the merged hash on each check.#process_group_leader?still honors caller-suppliedpgroup: true/pgroup: 0/new_pgroup: truefor the group-kill decision.#process_group_optionsis deterministic (depends only on the options, not mutated during#call, and the platform), and an option a subclass contributes never counts as isolation by this class, though it can still make the subprocess a group leader for the timeout kill.new_pgroupis covered) prevents the cleanup; and a subclass-contributed option with no timeout is not isolation — the one case where the new definition and the old one disagree, making the definitional change detectable by the suite. The kill-path unit specs drive a real (stubbed) spawn-then-kill lifecycle rather than a fabricated pre-spawn state.No public API or documented behavior changes; the only semantic difference from the old derivation is the subclass-override corner above, which no subclass exercises.
Validation
bundle exec rake(default ruby)ASDF_RUBY_VERSION=jruby-10.0.6.0 bundle exec rake specASDF_RUBY_VERSION=truffleruby-24.2.1 bundle exec rake spec!truffleruby?gate)bundle exec rake(Windows MRI)