Repository navigation
refactor(wework): isolate desktop control extension boundary #2082
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| const PROCESS_STOP_TIMEOUT_MS = 10_000 | ||
|
|
||
| function withTimeout(promise, timeoutMs, message) { | ||
| let timeout | ||
| const timeoutPromise = new Promise((_, reject) => { | ||
| timeout = setTimeout(() => reject(new Error(message)), timeoutMs) | ||
| }) | ||
| return Promise.race([promise, timeoutPromise]).finally(() => clearTimeout(timeout)) | ||
| } | ||
|
|
||
| function waitForProcessExit(child, timeoutMs) { | ||
| if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() | ||
| return withTimeout( | ||
| new Promise(resolvePromise => child.once('exit', resolvePromise)), | ||
| timeoutMs, | ||
| `Timed out waiting for process ${child.pid ?? 'unknown'} to exit` | ||
| ) | ||
| } | ||
|
|
||
| export async function stopProcess(child) { | ||
| if (!child || child.exitCode !== null || child.signalCode !== null) return | ||
| child.kill('SIGTERM') | ||
| try { | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| } catch { | ||
| child.kill('SIGKILL') | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| } | ||
| } | ||
|
|
||
| export async function stopProcessGroup(child) { | ||
| if (!child) return | ||
| if (process.platform === 'win32' || !Number.isInteger(child.pid)) { | ||
| await stopProcess(child) | ||
| return | ||
| } | ||
|
|
||
| signalProcessGroup(child.pid, 'SIGTERM') | ||
| if (child.exitCode === null && child.signalCode === null) { | ||
| try { | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| } catch { | ||
| signalProcessGroup(child.pid, 'SIGKILL') | ||
| await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) | ||
| return | ||
| } | ||
| } | ||
| signalProcessGroup(child.pid, 'SIGKILL') | ||
| } | ||
|
|
||
| function signalProcessGroup(processGroupId, signal) { | ||
| try { | ||
| process.kill(-processGroupId, signal) | ||
| } catch (error) { | ||
| if (error?.code !== 'ESRCH') throw error | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| export async function loadDesktopScenario(moduleUrl, options) { | ||
| if (!moduleUrl) return null | ||
|
|
||
| const scenarioModule = await import(moduleUrl) | ||
| if (typeof scenarioModule.createDesktopScenario !== 'function') { | ||
| throw new Error(`Desktop scenario module must export createDesktopScenario: ${moduleUrl}`) | ||
| } | ||
|
|
||
| return scenarioModule.createDesktopScenario(options) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid 20-second teardown hangs for processes that failed to spawn.
If a
ChildProcessfails to spawn,child.pidwill beundefined, and the process will never emit anexitevent. Currently,waitForProcessExitwill wait for this non-existent event until the timeout expires twice (once for SIGTERM, once for SIGKILL fallback), causing an unnecessary 20-second delay per failed process during test teardown.Add an early return for missing
pidto prevent these hangs and speed up teardown on failure.⚡ Proposed fix
function waitForProcessExit(child, timeoutMs) { - if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() + if (child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return Promise.resolve() return withTimeout( new Promise(resolvePromise => child.once('exit', resolvePromise)), timeoutMs, `Timed out waiting for process ${child.pid ?? 'unknown'} to exit` ) } export async function stopProcess(child) { - if (!child || child.exitCode !== null || child.signalCode !== null) return + if (!child || child.pid === undefined || child.exitCode !== null || child.signalCode !== null) return child.kill('SIGTERM') try { await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) } catch { child.kill('SIGKILL') await waitForProcessExit(child, PROCESS_STOP_TIMEOUT_MS) } }📝 Committable suggestion
🤖 Prompt for AI Agents