Skip to content

Non-blocking review findings from PR #351 (1) #352

Description

@twistedmelonman

Non-blocking concerns raised while reviewing PR #351 (perf(tests): stop an orphaned child holding the test's pipe open).

None of these blocked the merge. They are batched into one issue so a
review's findings stay one unit of attention rather than 1 separate
tracking issues; tick items off as they are addressed, and close this issue
when the list is done or the remaining items are judged not worth doing.

  • Watchdog fallback kills a pid rather than a process group, leaving orphaned children (PR author comments in the diff, echoed by the claude automated review, bash/tests/test-pre-push-scan-timeout.sh:326-352 (IGNORER fixture comment) and the run_bounded implementation in git/hooks/pre-push)
    The diff's own comments state that the watchdog fallback path signals a pid rather than a process group, so a killed command's children survive. This PR works around the symptom in the test harness by redirecting the driver's output to /dev/null so an orphaned child no longer holds the command substitution's pipe open; it does not change the kill behavior itself. The comments attribute the underlying defect to dotfiles#350 and state the fixture's child shape was kept deliberately so that issue's eventual fix has something to assert against. Suggested action: confirm dotfiles#350 is still open and tracks the process-group kill, since this PR closes the test-slowness symptom but not the cause.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    tech-debtTechnical debt to address

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions