fix(qa-lab): handle stdout/stderr stream errors in gateway child spawns#102104
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 11:15 PM ET / 03:15 UTC. Summary PR surface: Source +65, Tests +151. Total +216 across 2 files. Reproducibility: yes. from source inspection: current main has piped QA Lab child streams with data listeners but no stream error listeners, and the PR adds focused tests that exercise that error-event path with real spawned child processes. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the localized QA Lab stream-error fix after ordinary maintainer review because it addresses the current-main gap without adding config, API, dependency, or workflow surface. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current main has piped QA Lab child streams with data listeners but no stream error listeners, and the PR adds focused tests that exercise that error-event path with real spawned child processes. Is this the best way to solve the issue? Yes. The owner-local handler is the narrow maintainable fix for this QA Lab process surface; a broader shared abstraction is not needed for this plugin-only test harness path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f77c2aafa095. Label changesLabel justifications:
Evidence reviewedPR surface: Source +65, Tests +151. Total +216 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (11 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review Fixed the round-2 P1: typed the helper against the push-only log collector. 148f3fd.
Verified locally on the PR head:
🤖 Generated with Claude Code |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
🦞🧹 I asked ClawSweeper to review this item again. |
Co-authored-by: wangmiao0668000666 <[email protected]>
914b9a8 to
d161f73
Compare
|
Land-ready maintainer pass complete at I rewrote this around one first-failure monitor shared by short-lived CLI commands and long-lived/replacement Gateway children. The final shape rejects and terminates CLI children on pipe failure, stops detached Gateway process trees, retains failures across steady-state operations and replacements, completes every ordered suite cleanup step, and preserves the primary infrastructure error as Proof:
Thanks @wangmiao0668000666 for the original report and fix direction. |
|
Merged via squash.
|
Co-authored-by: Peter Steinberger <[email protected]>
3 participants
What Problem This Solves
QA Lab spawns short-lived CLI commands and long-lived Gateway children with piped stdout and stderr. A parent-side pipe error had no listener, so Node could raise an uncaught error and crash the QA worker. Merely swallowing that event is also unsafe: the Gateway may keep running after QA can no longer observe its output, and a teardown-only failure could otherwise let a scenario report success.
Why This Change Was Made
This maintainer rewrite uses one first-failure monitor for process, stdout, and stderr errors across every QA Gateway child lifecycle. Short-lived CLI commands reject and stop immediately. Long-lived Gateway children record the failure, log it once, and stop the detached process tree. The active failure getter follows replacement children and is checked at startup, around Gateway operations, and after shutdown cleanup.
The suite cleanup path now runs every cleanup step in order even when the Gateway reports a pipe failure. If both the run and cleanup fail, the aggregate retains the primary suite error as its cause so the existing infrastructure retry policy still recognizes it.
Compared with the submitted head, this removes the duplicated per-stream settlement logic and module-reset spawn mocks, covers the steady-state false-positive case, stops detached process trees rather than only the direct child, and preserves retry classification while cleanup completes.
User Impact
QA runs now fail deterministically when their Gateway output pipes break, without uncaught exceptions, orphaned Gateway trees, false-positive scenario results, skipped cleanup, or lost infrastructure retries. No public CLI, config, plugin, or protocol contract changes.
Evidence
d161f73c71b48a7ef7b9aaabd5cb9609eb989862.run_19b7cea38d56: Linux Node 24 focused regression suite, 2 files / 102 tests passed after rebasing onto currentmain.run_954ae4389f96: changed-lane extension and test typechecks, lint, database-first and sidecar guards, and runtime import-cycle scan passed.git diff --checkand targeted oxfmt/oxlint passed.Changelog is not required because this is internal QA infrastructure hardening.
Thanks @wangmiao0668000666 for identifying the missing stream-error handling and providing the original fix direction.