Skip to content

fix(e2e): escalate to SIGKILL when the certificate probe hangs - #1792

Merged
vastsa merged 4 commits into
vastsa:mainfrom
muzimu217:fix/e2e-runner-kill-escalation
Oct 21, 2026
Merged

vastsa merged 4 commits into
vastsa:mainfrom
muzimu217:fix/e2e-runner-kill-escalation

Conversation

@muzimu217

Copy link
Copy Markdown
Contributor

What this fixes

The Trusted extensions Electron E2E job can hang instead of failing. On #1751 it ran 30m18s and was cancelled by the job budget with no failing step to read — conclusion: cancelled, zero failed steps.

The cause is this runner's only timeout:

const timer = setTimeout(() => child.kill(), 60_000);

SIGTERM is a request. An Electron child still initialising its renderer can ignore it, and the close event this script awaits then never arrives — so the runner's own 60-second budget is never enforced and the step holds the job until GitHub cancels it.

Change

Escalate, and say which signal fired:

  • SIGTERM at 60s, SIGKILL at 65s
  • assert killedBy === null, so a probe that cannot start fails this step in about a minute with its output attached instead of burning the job budget

Verification

  • Normal path unchanged: a real run on this branch prints baseline: Electron 43 net.fetch fails before HTTP then provider client certificate E2E passed.
  • Timeout path terminates: with a stub child that ignores SIGTERM, the escalation force-kills it at ~804ms in an isolated harness, confirming the timeout no longer hangs.
  • node --check clean; branch is one commit on current main (a6f4ba121), 1 file +23/−2.

Why this matters beyond one job

The same escalation exists on #1759's branch (its author hit the same hang) but that PR is still open, so main carries the hazard. Any Electron E2E step that hangs here costs 30 minutes of runner time and returns no diagnostic — this makes it a one-minute failure that names the signal.

vastsa#1751's Electron E2E job ran 30m18s and was cancelled by the job budget with
no failing step to read. The cause is this runner's only timeout:

    const timer = setTimeout(() => child.kill(), 60_000);

SIGTERM is a request. An Electron child still initialising its renderer can
ignore it, and the 'close' event this script awaits then never arrives, so the
step holds the job until GitHub cancels it. The runner's own 60s budget is
never enforced.

Escalate: SIGTERM at 60s, SIGKILL at 65s, and report which one fired. A probe
that cannot start now fails this step in about a minute with its output
attached, instead of burning the job budget.

Verified: normal path still passes ("provider client certificate E2E
passed"); with a child that ignores SIGTERM the escalation force-kills it at
~800ms in an isolated harness, so the timeout path terminates rather than
hanging.
@muzimu217
muzimu217 force-pushed the fix/e2e-runner-kill-escalation branch from abee78b to 434e06f Compare October 21, 2026 07:03
Integrate the reviewed timeout fix with the latest main before final validation.
Keep the normal Electron path unchanged while asserting that a child
ignoring SIGTERM receives SIGKILL after the grace period.
@vastsa
vastsa merged commit b73b6df into vastsa:main Oct 21, 2026
4 of 5 checks passed
Tioit-Wang pushed a commit that referenced this pull request Oct 23, 2026
…o-column-ticker

Four files conflicted, all because main had already re-solved the same
problems this branch carried:

- `.github/workflows/ci.yml` and `scripts/e2e-provider-client-certificate.mjs`
  keep main's side. #1792 landed the same virtual-display fix here and then
  replaced the hand-rolled SIGTERM-then-SIGKILL timer with
  `waitForChildExitWithEscalation()`, so the branch's copy is a strictly older
  version of the same repair.
- `docs/spec/08-meta/decisions-log.md` and its zh-CN mirror keep both sides.
  The branch's decision was numbered D665, which main has held since
  2026-10-16 (macOS opaque window surface, amended by D666/D667) and has since
  grown to D670; the marketplace entry is renumbered D671 in the table and in
  the appended section of both mirrors, and its two `07-plugin-marketplace.md`
  cross-references follow.

The merge also folded main's copy of the marketplace spec's monogram-glyph
paragraph back to one copy: the branch carried it twice, which hid the
duplicate from the three-way merge.

Marketplace grid, ticker, chip, and locale changes merge cleanly and stay as
they are.
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