Context
Refactoring follow-up from a review of the v4.0.5 release changes (#163). (Finding F3 from that review.)
Problem
In Commands::SpawnWithTimeout, the decision "did this class isolate the subprocess into its own process group?" is derived twice:
#process_group_options computes it directly (timeout can fire, and the caller set neither pgroup nor new_pgroup)
#isolated_in_new_process_group? re-derives it from the merged spawn options via #process_group_leader? plus repeated :not_set checks
The two are equivalent in every combination of timeout and caller-supplied pgroup options, but a reader has to prove that. #process_group_leader? also recomputes spawn_options (rebuilding the merged hash) each time it runs on the kill path.
Proposed refactoring
- Define
isolated_in_new_process_group? as !process_group_options.empty?, making #process_group_options the single source of truth for the isolation decision
- Capture the effective spawn options once at spawn time (memoize or store in
#call) so the kill path inspects the options actually used rather than recomputing the merge
#process_group_leader? must keep honoring caller-supplied pgroup: true / pgroup: 0 / new_pgroup: true for the group-kill decision — only the isolation check collapses.
Acceptance criteria
Blocked by
None — can start immediately.
Context
Refactoring follow-up from a review of the v4.0.5 release changes (#163). (Finding F3 from that review.)
Problem
In
Commands::SpawnWithTimeout, the decision "did this class isolate the subprocess into its own process group?" is derived twice:#process_group_optionscomputes it directly (timeout can fire, and the caller set neitherpgroupnornew_pgroup)#isolated_in_new_process_group?re-derives it from the merged spawn options via#process_group_leader?plus repeated:not_setchecksThe two are equivalent in every combination of timeout and caller-supplied pgroup options, but a reader has to prove that.
#process_group_leader?also recomputesspawn_options(rebuilding the merged hash) each time it runs on the kill path.Proposed refactoring
isolated_in_new_process_group?as!process_group_options.empty?, making#process_group_optionsthe single source of truth for the isolation decision#call) so the kill path inspects the options actually used rather than recomputing the merge#process_group_leader?must keep honoring caller-suppliedpgroup: true/pgroup: 0/new_pgroup: truefor the group-kill decision — only the isolation check collapses.Acceptance criteria
Blocked by
None — can start immediately.