fix: Time out external commands and network requests - #75
Conversation
Nothing in the installer had an upper bound on how long it would wait. Every external command went through execFile with no timeout, and every request went through node-fetch with no timeout. Each of those talks to something that can stop answering without ever failing, so a single sick component hung the entire installation forever. This was observed on a Windows lab node: the log ended after the Chrome line and never produced another, because the next installer's 'adb shell dumpsys' never returned for a wedged Android device. The service that runs the installer at startup sat wedged behind it until the process was killed by hand. Skipping one browser is far better than that, and main.js already catches per-installer errors and moves on. Cap commands at 60s, metadata requests at 60s, and archive downloads at 5 minutes. The limits are deliberately generous, since a false timeout means a driver silently doesn't get installed. Two details matter for the kill actually working: Run our own timer instead of execFile's 'timeout' option, because that option kills only the process we started. On Windows, tools installed through Chocolatey (adb included, via shaka-lab-browsers) run behind a generated shim, so that would kill the launcher and orphan the tool that is actually stuck, which is the stray process this is meant to prevent. taskkill /T covers the tree there, as it already does elsewhere. Destroy the output pipes before killing. We wait on the streams to close, and a leftover grandchild holding the write end open means that never happens; the timer would fire and the promise would still hang. child_process does the same in its own timeout handling. Timeouts are reported as timeouts rather than as generic command failures, and getMacAppVersion and getAndroidAppVersion now let them propagate instead of reporting the browser as absent, so a hang is visible in a log instead of looking like a missing browser. Co-Authored-By: Claude Code (Claude Opus 5) <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
This should fix the root cause of the hung update on Windows. The companion change in shaka-project/shaka-lab#94 should prevent the Windows service from hanging on any misbehaving update tooling in the future. |
|
The Opera driver failed to install in CI on macOS. I tried it on my macbook and it worked fine. I'll follow up in another PR if I can figure that out. |
Appears to just be flakiness. It worked on the release PR. |
2 participants
Nothing in the installer had an upper bound on how long it would wait. Every external command went through execFile with no timeout, and every request went through node-fetch with no timeout. Each of those talks to something that can stop answering without ever failing, so a single sick component hung the entire installation forever.
This was observed on a Windows lab node: the log ended after the Chrome line and never produced another, because the next installer's 'adb shell dumpsys' never returned. The service that runs the installer at startup sat wedged behind it until the process was killed by hand. Skipping one browser is far better than that, and main.js already catches per-installer errors and moves on.
Cap commands at 60s, metadata requests at 60s, and archive downloads at 5 minutes. The limits are deliberately generous, since a false timeout means a driver silently doesn't get installed.
Two details matter for the kill actually working:
Run our own timer instead of execFile's 'timeout' option, because that option kills only the process we started. On Windows, tools installed through Chocolatey (adb included, via shaka-lab-browsers) run behind a generated shim, so that would kill the launcher and orphan the tool that is actually stuck, which is the stray process this is meant to prevent. taskkill /T covers the tree there, as it already does elsewhere.
Destroy the output pipes before killing. We wait on the streams to close, and a leftover grandchild holding the write end open means that never happens; the timer would fire and the promise would still hang. child_process does the same in its own timeout handling.
Timeouts are reported as timeouts rather than as generic command failures, and getMacAppVersion and getAndroidAppVersion now let them propagate instead of reporting the browser as absent, so a hang is visible in a log instead of looking like a missing browser.