fix: prevent E2E worker teardown timeout with close deadline#10528
fix: prevent E2E worker teardown timeout with close deadline#10528SebTardif wants to merge 3 commits into
Conversation
The E2E tests intermittently fail with 'Worker teardown timeout of 60000ms exceeded' because app.close() hangs when VS Code's Electron process doesn't exit cleanly (extension host busy with file watchers, terminal cleanup, etc.). Fix by racing app.close() against a 15s deadline. If the deadline fires, force-kill the Electron process with SIGKILL so fixture teardown completes well within the 60s worker budget. Also add a 5s deadline to page.close() for defense in depth — if the renderer is unresponsive, skip page close and let app.close() handle process termination. The teardown budget is now bounded: page.close() ≤ 5s app.close() ≤ 15s rmSync cleanup ~1s Total ≤ 21s (vs 60s budget)
|
Greptile SummaryThis PR fixes intermittent E2E worker teardown timeouts by wrapping Confidence Score: 4/5Safe to merge; only a minor timer-leak style issue found, no functional regressions. All findings are P2 (style/best-practice). The fix is logically correct and well-bounded. The sole concern is that the losing setTimeout handles in both race blocks are never cleared, which can keep Node's event loop alive transiently but won't break tests. src/test/e2e/utils/helpers.ts — orphaned timer handles in both fixture teardown blocks.
|
| Filename | Overview |
|---|---|
| src/test/e2e/utils/helpers.ts | Added Promise.race timeout guards to both app and page fixture teardown; one minor timer-leak issue (uncleared setTimeout handles) but the overall fix is correct and well-reasoned. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/test/e2e/utils/helpers.ts:294-303
**Orphaned timers after successful close**
Both `setTimeout` handles created inside the losing `Promise` are never cleared. When `app.close()` (or `page.close()`) completes before the deadline, the timer is still running and will fire after the race settles — keeping Node's event loop alive unnecessarily. While this won't break the test in most cases, it can delay teardown and is easily avoided by clearing the handle.
```typescript
let appCloseTimer: ReturnType<typeof setTimeout> | undefined;
try {
await Promise.race([
app.close(),
new Promise<never>((_, reject) => {
appCloseTimer = setTimeout(() => reject(new Error("app.close() timed out")), 15_000);
}),
]);
} catch {
try { app.process().kill("SIGKILL") } catch {}
} finally {
clearTimeout(appCloseTimer);
}
```
The same applies to the 5 s timer in the `page` fixture.
Reviews (1): Last reviewed commit: "fix: prevent E2E worker teardown timeout..." | Re-trigger Greptile
| try { | ||
| await Promise.race([ | ||
| app.close(), | ||
| new Promise<never>((_, reject) => setTimeout(() => reject(new Error("app.close() timed out")), 15_000)), | ||
| ]) | ||
| } catch { | ||
| try { | ||
| app.process().kill("SIGKILL") | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Both setTimeout handles created inside the losing Promise are never cleared. When app.close() (or page.close()) completes before the deadline, the timer is still running and will fire after the race settles — keeping Node's event loop alive unnecessarily. While this won't break the test in most cases, it can delay teardown and is easily avoided by clearing the handle.
let appCloseTimer: ReturnType<typeof setTimeout> | undefined;
try {
await Promise.race([
app.close(),
new Promise<never>((_, reject) => {
appCloseTimer = setTimeout(() => reject(new Error("app.close() timed out")), 15_000);
}),
]);
} catch {
try { app.process().kill("SIGKILL") } catch {}
} finally {
clearTimeout(appCloseTimer);
}The same applies to the 5 s timer in the page fixture.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/test/e2e/utils/helpers.ts
Line: 294-303
Comment:
**Orphaned timers after successful close**
Both `setTimeout` handles created inside the losing `Promise` are never cleared. When `app.close()` (or `page.close()`) completes before the deadline, the timer is still running and will fire after the race settles — keeping Node's event loop alive unnecessarily. While this won't break the test in most cases, it can delay teardown and is easily avoided by clearing the handle.
```typescript
let appCloseTimer: ReturnType<typeof setTimeout> | undefined;
try {
await Promise.race([
app.close(),
new Promise<never>((_, reject) => {
appCloseTimer = setTimeout(() => reject(new Error("app.close() timed out")), 15_000);
}),
]);
} catch {
try { app.process().kill("SIGKILL") } catch {}
} finally {
clearTimeout(appCloseTimer);
}
```
The same applies to the 5 s timer in the `page` fixture.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 3c7ce10 — both Promise.race blocks now store the timer handle and clear it in a finally block, preventing orphaned timers from keeping the event loop alive after successful close.
Call app.evaluate(app => app.quit()) before app.close() so Electron fires its before-quit/will-quit lifecycle events. This gives the VS Code extension host a chance to clean up (file watchers, terminal processes, etc.) rather than just waiting for the process to exit. The timeout + SIGKILL fallback remains as a last resort for stuck IPC handlers, but app.quit() should resolve most cases gracefully. This follows the Playwright-recommended pattern for Electron apps (see microsoft/playwright#12189).
The setTimeout handles inside the losing Promise.race branch were never cleared, keeping Node's event loop alive unnecessarily after successful close. Now both app and page close blocks clear their timer in a finally block.
Problem
E2E tests intermittently fail with:
This affects ~13% of E2E runs on
main(2/15 recent runs). The test itself passes — the failure occurs during fixture teardown whenapp.close()hangs because VS Code's Electron process doesn't exit cleanly.Root Cause
Playwright fixtures tear down in reverse order:
sidebar→page.close()→app.close()→ temp dir cleanup. The worker has a 60s budget for all teardown combined.app.close()sends a close signal to the Electron process and waits for it to exit. If the VS Code extension host is busy (file watchers, terminal process cleanup, background tasks), the process can hang indefinitely, consuming the entire teardown budget.retries: 1in the Playwright config doesn't help because it retries tests, not fixture teardown.Fix
Race both close operations against deadlines, with force-kill as fallback:
page.close()app.close()SIGKILLthe Electron processTotal worst-case teardown: ≤21s (page 5s + app 15s + cleanup ~1s) vs the 60s budget.
Why these timeouts
Files Changed
src/test/e2e/utils/helpers.ts— Added timeout + force-kill toappfixture teardown, timeout guard topagefixture teardown