Skip to content

ProcessRunner timeout blocks on shell descendants after terminating parent #930

Description

@chubes4

Problem

The timeout added in #929 is detected, but ProcessRunner::terminate_timed_out_process() can still block well beyond the configured budget when the command is passed as a shell string.

GitRunner::run() passes a string such as git -C ... fetch --quiet origin 2>&1 to proc_open(). PHP therefore owns the intermediate shell process. On timeout, proc_terminate() kills that shell, but the descendant git process retains the stdout/stderr pipes. The subsequent stream_get_contents() / proc_close() cleanup blocks until the descendant exits.

This means the five-second freshness-fetch budget from #929 does not currently bound end-to-end wall time.

Runtime evidence

After deploying release v0.51.13, which contains #929:

  • The deterministic injected timeout contract returned timed_out=true and timeout_seconds=5.
  • A live WorktreeStalenessProbe::fetch() against a valid HTTPS origin took 30.83 seconds despite receiving the five-second timeout budget.
  • A subsequent direct git fetch --quiet origin completed in 0.48 seconds.

The issue is therefore timeout cleanup/process-tree behavior, not normal repository fetch cost.

Coverage gap

tests/process-runner-command-spec.php verifies timeout using a direct argv PHP_BINARY -r while(true){} process. That process has no intermediate shell or descendant retaining the pipes, so the test passes while the GitRunner string-command path remains unbounded.

Expected behavior

A timeout bounds total wall-clock completion, including descendant termination and pipe cleanup, for both argv commands and shell-string commands.

Acceptance criteria

  1. Add deterministic coverage where a shell command starts a child process that retains stdout/stderr, and assert completion remains close to the configured timeout rather than the child lifetime.
  2. Terminate the relevant process tree/process group without leaving descendants or blocking on inherited pipes.
  3. Preserve captured timeout output and the existing WP_Error envelope.
  4. Cover the GitRunner string-command path, not only CommandSpec::from_argv().
  5. Keep behavior correct on macOS/Linux and account explicitly for Windows process termination.
  6. Re-run the live worktree freshness probe and show a five-second fetch budget produces bounded end-to-end completion.

Relationship

Follow-up to #927 and #929. The timeout propagation and error classification from #929 are correct; this issue repairs the lower-level timeout enforcement that prevents the budget from being real.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions