test: Introduce E2ESubprocess for better process handling

[niteshpurohit]

• Added E2ESubprocess class to manage subprocess execution and output capturing.
• Replaced Open3 calls with E2ESubprocess in CLI worker specs for consistency and improved error handling.
• Updated framework runtime control support to utilize E2ESubprocess for better process management.
• Introduced tests for E2ESubprocess to ensure proper timeout handling and output capturing.

[Copilot]

🟡 Changes recommended

E2ESubprocess.capture(timeout:) can exceed the requested timeout and has a duplicated-keyword edge case, and the new timeout spec does not currently assert the bounded-time behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces a dedicated E2ESubprocess helper for E2E/integration specs and migrates existing specs away from direct Open3 usage to standardize subprocess execution, output capture, and cleanup.

Changes:

• Added spec/support/e2e_subprocess.rb (E2ESubprocess) to manage subprocess lifecycle and capture stdout/stderr.
• Replaced Open3.capture3 / Open3.popen2e usage in CLI worker and framework runtime control E2E specs with E2ESubprocess.
• Added an E2E spec intended to validate timeout/output behavior for forked descendants.

File summaries

File	Description
spec/support/framework_runtime_control_e2e_support.rb	Switches framework runtime control E2E support from Open3 to E2ESubprocess.
spec/support/e2e_subprocess.rb	Introduces the subprocess wrapper used by E2E specs.
gems/karya/spec/e2e/karya/e2_e_subprocess_spec.rb	Adds a spec meant to exercise timeout/output bounding behavior.
gems/karya/spec/e2e/karya/cli_worker_spec.rb	Uses E2ESubprocess for CLI runtime/worker E2E subprocesses.
gems/karya/spec/e2e/karya/cli_worker_sqlite_spec.rb	Replaces Open3.capture3 with E2ESubprocess.capture in SQLite E2E worker spec.
gems/karya/spec/e2e/karya/cli_worker_redis_spec.rb	Replaces Open3.capture3 with E2ESubprocess.capture in Redis E2E worker spec.
gems/karya/spec/e2e/karya/cli_worker_mysql_spec.rb	Replaces Open3.capture3 with E2ESubprocess.capture in MySQL E2E worker spec.

Review details

Suppressed comments (1)

spec/support/framework_runtime_control_e2e_support.rb:403

• The worker_env keyword argument list has inconsistent indentation on worker_name:/worker:; aligning these improves readability and avoids layout lint failures.

        database_url:,
        namespace:,
          worker_name:,
          worker: true
      )

• Files reviewed: 7/7 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new subprocess integration introduces a couple of failure-masking/cleanup issues (and one redundant cleanup call) that should be addressed to avoid flaky e2e failures and hard-to-diagnose timeouts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

gems/karya/spec/e2e/karya/cli_worker_spec.rb:79

• process.wait_for_output can raise Timeout::Error if a descendant keeps an output pipe open, which would fail the spec before it can assert on runtime state / include partial output in the failure message. Rescuing that timeout here makes failures more actionable while still allowing cleanup via with_force_stop_worker’s ensure.

This issue also appears on line 182 of the same file.
spec/support/framework_runtime_control_e2e_support.rb:173

• If the worker exits early and a forked descendant keeps stdout/stderr open, process.wait_for_output can raise Timeout::Error here and mask the more actionable "worker exited" failure. Consider rescuing the output timeout and raising the exit error with whatever output has been captured so far.
  spec/support/e2e_subprocess.rb:11
• spec/support helpers appear to consistently define namespaced modules (e.g., SQLiteE2ESupport in spec/support/sqlite_e2e_support.rb:10) rather than introducing new top-level constants. Defining E2ESubprocess as a global class increases the chance of constant collisions/leakage across the spec suite; consider namespacing it (e.g., under KaryaE2EHelpers or a dedicated support module) and updating call sites accordingly.

spec/support/e2e_subprocess.rb:85

• close does not verify that the process group actually terminated after TERM/KILL and the subsequent joins; it can return while the subprocess (or a descendant holding pipes) is still alive. That can leak stray processes across the spec run and make later tests non-deterministic.

  def close
    terminate_process_group if alive? || output_readers_alive?
    close_output_streams
    reap_process
    reap_output_readers
  end

gems/karya/spec/e2e/karya/cli_worker_spec.rb:185

• with_force_stop_worker already closes the process in an ensure, so calling process.close here is redundant and can lead to double termination/join work (and makes it harder to reason about where cleanup happens).

        expect(force_stop_status.exitstatus).to eq(0), -> { "stdout:\n#{force_stop_stdout}\n\nstderr:\n#{force_stop_stderr}" }
        runtime_phase = wait_for_runtime_phase(state_file, 'force_stopping', 'stopped').fetch('phase')
        expect(runtime_phase).to match(/\A(?:force_stopping|stopped)\z/)
        process.close

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

wait_for_framework_runtime_start can mis-handle the outer wait_until timeout by rescuing async Timeout::Error inside the polling block, producing incorrect failure behavior/messages.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new subprocess helper and updated specs have cleanup/termination and exception-masking issues that can leave process groups running or hide the real test failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

gems/karya/spec/e2e/karya/cli_worker_spec.rb:65

• If E2ESubprocess#close raises CleanupError during this ensure, it will replace any exception raised inside the block, which can make failures harder to diagnose (the original failure gets masked).

Catching CleanupError here and re-raising the original exception (when present) preserves the primary failure while still surfacing cleanup problems.
spec/support/framework_runtime_control_e2e_support.rb:381

• If process.close raises CleanupError in this ensure, it will mask the exception being raised from the example body (including the Timeout handling above), making the root cause harder to see.

Consider preserving the original exception when cleanup fails.

This issue also appears on line 449 of the same file.
gems/karya/spec/e2e/karya_spec_support/e2_e_subprocess_spec.rb:11

• The spec filename e2_e_subprocess_spec.rb looks like a typo/inconsistency (the class is E2ESubprocess and the support file is e2e_subprocess.rb). Renaming the spec to e2e_subprocess_spec.rb would make it easier to discover and keep naming consistent.

spec/support/framework_runtime_control_e2e_support.rb:449

• If process.close raises CleanupError here, it will override any failure raised in the example body (or during the Timeout rescue), which can obscure the actual test failure.

Rescuing CleanupError and re-raising the original exception (when present) keeps the primary signal intact while still reporting cleanup issues.

        process.close

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new subprocess helper’s core “happy path” stdout/stderr capture behavior isn’t directly asserted in the added spec coverage, and the timeout error message would be significantly more actionable if it included the command being run.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

gems/karya/spec/e2e/karya_spec_support/e2_e_subprocess_spec.rb:54

• This spec file exercises timeouts and cleanup behavior, but it doesn’t currently assert that capture actually returns stdout/stderr for a normal successful command. Adding a small “happy path” assertion would better cover the core output-capturing behavior this helper is meant to provide.
  spec/support/e2e_subprocess.rb:31
• The timeout error raised by capture doesn’t include which command was being executed, which makes E2E failures harder to diagnose (especially when multiple subprocesses run in the same spec). Consider including a compact command description in the exception message.

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite