Skip to content

fix(test-runner): make runs cancellable, deadlock-free, and bounded - #735

Open
beruro wants to merge 3 commits into
developfrom
junyu/fix-test-runner-lifecycle
Open

fix(test-runner): make runs cancellable, deadlock-free, and bounded#735
beruro wants to merge 3 commits into
developfrom
junyu/fix-test-runner-lifecycle

Conversation

@beruro

@beruro beruro commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Problem

Test runs in the WorkStation Testing tab could not actually be stopped, could deadlock while draining child output, and could grow memory without bound. The frontend stop action cleared UI state without signalling the backend, command and runner layers generated different run ids, cancellation was not observed by the child lifecycle, stdout and stderr were drained sequentially, and both streams accumulated into unbounded strings.

The test event wire shape also emitted snake_case variant fields while the frontend expects camelCase, so run_started could arrive without a usable runId.

Solution

The branch now uses one canonical run id from command registration through every event and summary, with a drop guard that always deregisters the run. Cancellation is backed by CancellationToken and terminates the process tree: Unix uses a dedicated process group with TERM, a bounded grace period, then KILL; Windows uses taskkill tree termination plus start_kill.

Stdout and stderr are drained concurrently in chunk-based tasks with a 16 MiB tail bound per stream and explicit truncation state. Cancelled runs emit run_cancelled and resolve with a partial summary marked cancelled. Per-variant camelCase serialization pins the frontend wire contract.

The frontend stop action now resolves the active run, invokes stop_tests, and updates state only after backend confirmation. Pure lifecycle transitions cover stale parallel-run events and no-op stop attempts.

The CI follow-up formats the new lifecycle test, defaults the optional reusable FieldRow isActive prop to false for compatibility with the boolean-only pill-state helper on current develop, and merges current develop into the branch.

Potential risks

  • stop_tests changes from an erroring unit result to a boolean that reports whether a run was signalled; the only caller is updated in this branch.
  • Cancelled runs resolve with partial counts marked cancelled, so summary consumers must respect the flag.
  • Reporter output beyond the per-stream tail bound is truncated; JSON parsing may fall back to degraded line parsing for a single document larger than the budget.
  • The Windows tree-kill path compiles but is not exercised by the macOS CI environment.
  • The pre-existing stable Cargo libtest format limitation is unchanged.
  • The shared FieldRow default only converts an omitted isActive value from undefined to false, matching its prior falsey rendering behavior.

Rollback is a normal revert of this PR. It changes no persistence schema and requires no data recovery.

Verification

  • cargo test -p test_runner (this worktree): 28/28 passed — including new regression tests: stderr-flood no-deadlock, process-group kill (grandchild verified dead via kill(pid, 0)), pre-cancelled token, canonical-id event ordering, wire-format pinning, registry lifecycle/RunGuard.
  • cargo clippy -p test_runner --all-targets (this worktree): finished clean, no warnings.
  • pnpm vitest run src/services/test/__tests__/testRunLifecycle.test.ts (this worktree): 11/11 passed.
  • CI on this PR: Frontend (typecheck · lint · test) SUCCESS, Rust (clippy) SUCCESS, attribution check SUCCESS.
  • Local commit was made with hooks bypassed (fresh worktree lacked node_modules at commit time); the full pipeline ran in CI as linked above.

Effects: none added or modified (React useEffect surface untouched; useTestRunner only re-types stopTests).

@beruro
beruro marked this pull request as ready for review August 7, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants