Repository navigation
fix: avoid cmd.exe spawn on Windows by using os.hostname() before execSync fallback - #7841
Conversation
…cSync fallback
On Windows, execSync() wraps the command in cmd.exe /d /s /c,
spawning a new cmd.exe process. getMachineIdRaw() called
execSync("hostname") as Strategy 4 before trying os.hostname()
as Strategy 5 -- meaning every dashboard API call spawned an
unnecessary cmd.exe process.
This commit:
- Moves os.hostname() to Strategy 4 (no child process, native binding)
- Keeps execSync("hostname") as Strategy 5 fallback
- Adds module-level caching so getMachineIdRaw() only runs once per
process lifetime since the machine ID never changes at runtime
- Caches all strategy results at the first successful return
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a module-level cache ('cachedRawId') in 'src/shared/utils/machineId.ts' to avoid expensive shell executions on subsequent calls, and refactors the hostname fallback strategies to prioritize 'os.hostname()' over spawning a child process. The review feedback recommends exporting a cache-reset helper function to improve unit testability and reminds the author to add or update unit tests for these changes as required by the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * cmd.exe which is expensive. Cache the result after first call | ||
| * since the machine ID never changes at runtime. | ||
| */ | ||
| let cachedRawId: string | null = null; |
There was a problem hiding this comment.
The introduction of the module-level cachedRawId variable makes it difficult to unit test different strategies or mock the environment across multiple test cases, as the cache cannot be cleared. To improve testability, consider exporting a helper function to reset the cache.
let cachedRawId: string | null = null;
/**
* Resets the cached machine ID. Used primarily for testing.
*/
export function resetMachineIdCache(): void {
cachedRawId = null;
}| function getMachineIdRaw(): string { | ||
| // Return cached result immediately (machine identity is stable at runtime) | ||
| if (cachedRawId !== null) return cachedRawId; |
There was a problem hiding this comment.
According to the Repository Style Guide (Rule 9 under Hard Rules), you must always include tests when changing production code under src/. Since src/shared/utils/machineId.ts has been modified, please add or update the corresponding unit tests in the tests/ directory to cover the new caching behavior and strategy fallback order.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
|
Thanks for tracking this down — spawning
One thing that needs fixing before merge: running the new test file directly — — 2 of the 10 tests fail here (and would fail the same way on this repo's Could you neutralize Strategy 3 the same way |
…cSync fallback Prioritize os.hostname() (sync, no subprocess) over execSync hostname fallback. Cache the result so subsequent calls never spawn. Export resetMachineIdCache() for test isolation. Tests: 10 tests covering cache behavior, strategy fallback order, and consistent machine ID hashing. The 2 tests that mock os.hostname() now also stub fs.readFileSync for /etc/machine-id to throw, so Strategy 3 (Linux machine-id file) does not preempt Strategy 4 on real Linux runners.
5c5f6b5 to
1fe78bb
Compare
|
Fixed in 1fe78bb. disableWindowsRegistryStrategy() now also stubs fs.readFileSync for /etc/machine-id and /var/lib/dbus/machine-id to throw. All 10 new tests pass, all 5 existing tests pass. |
286628a
into
cheaper-inference:release/v3.8.49
|
Merged — thanks @tientien17! os.hostname() before the execSync fallback kills a real per-call cmd.exe spawn, and you turned the hermetic-test feedback around fast. Validated in the local merge-train (14-PR combined tree, full unit suite + vitest green) on tomni-proxmox-113 @ 0fda37bb4fdb26ba1345fd7b4cac44fbe235cda4, then squash-merged. |
…cSync fallback (cheaper-inference#7841) * fix: avoid cmd.exe spawn on Windows by using os.hostname() before execSync fallback On Windows, execSync() wraps the command in cmd.exe /d /s /c, spawning a new cmd.exe process. getMachineIdRaw() called execSync("hostname") as Strategy 4 before trying os.hostname() as Strategy 5 -- meaning every dashboard API call spawned an unnecessary cmd.exe process. This commit: - Moves os.hostname() to Strategy 4 (no child process, native binding) - Keeps execSync("hostname") as Strategy 5 fallback - Adds module-level caching so getMachineIdRaw() only runs once per process lifetime since the machine ID never changes at runtime - Caches all strategy results at the first successful return Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: avoid cmd.exe spawn on Windows by using os.hostname() before execSync fallback Prioritize os.hostname() (sync, no subprocess) over execSync hostname fallback. Cache the result so subsequent calls never spawn. Export resetMachineIdCache() for test isolation. Tests: 10 tests covering cache behavior, strategy fallback order, and consistent machine ID hashing. The 2 tests that mock os.hostname() now also stub fs.readFileSync for /etc/machine-id to throw, so Strategy 3 (Linux machine-id file) does not preempt Strategy 4 on real Linux runners. --------- Co-authored-by: tientien17 <tientien17@users.noreply.github.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…cSync fallback (cheaper-inference#7841) * fix: avoid cmd.exe spawn on Windows by using os.hostname() before execSync fallback On Windows, execSync() wraps the command in cmd.exe /d /s /c, spawning a new cmd.exe process. getMachineIdRaw() called execSync("hostname") as Strategy 4 before trying os.hostname() as Strategy 5 -- meaning every dashboard API call spawned an unnecessary cmd.exe process. This commit: - Moves os.hostname() to Strategy 4 (no child process, native binding) - Keeps execSync("hostname") as Strategy 5 fallback - Adds module-level caching so getMachineIdRaw() only runs once per process lifetime since the machine ID never changes at runtime - Caches all strategy results at the first successful return Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: avoid cmd.exe spawn on Windows by using os.hostname() before execSync fallback Prioritize os.hostname() (sync, no subprocess) over execSync hostname fallback. Cache the result so subsequent calls never spawn. Export resetMachineIdCache() for test isolation. Tests: 10 tests covering cache behavior, strategy fallback order, and consistent machine ID hashing. The 2 tests that mock os.hostname() now also stub fs.readFileSync for /etc/machine-id to throw, so Strategy 3 (Linux machine-id file) does not preempt Strategy 4 on real Linux runners. --------- Co-authored-by: tientien17 <tientien17@users.noreply.github.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Problem
On Windows,
execSync()wraps the command incmd.exe /d /s /c, spawning a new cmd.exe process.getMachineIdRaw()calledexecSync("hostname")as Strategy 4 before tryingos.hostname()as Strategy 5. This meant:Fix
os.hostname()(native Node.js binding, no child process) is now Strategy 4, tried BEFOREexecSync("hostname")(Strategy 5)getMachineIdRaw()caches its result after the first successful call -- the machine ID never changes at runtime, so repeated API calls no longer re-query the OSVerification
A runtime monkey-patch tracer confirmed
execSync("hostname")was not called during startup or API requests after the fix on Windows Node.js 24.Related
execSyncdoes not spawncmd.exeon Linux/macOS -- it uses fork/exec directly)