Skip to content

[rust] Fix arch misdetection on 32bit Windows environment #17297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gplm620 wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

[rust] Fix arch misdetection on 32bit Windows environment #17297

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rust/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl ManagerConfig {
_architecture = get_win_os_architecture();
}
}
if _architecture.contains("32") {
if _architecture.contains("x86") {
ARCH_X86.to_string()
} else if _architecture.contains("ARM") {
ARCH_ARM64.to_string()
Comment on lines 83 to 89
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Windows arch change lacks tests 📘 Rule violation ☼ Reliability

The PR changes Windows architecture detection/mapping logic but does not add/update tests to lock in
the new observable behavior (e.g., PROCESSOR_ARCHITECTURE=x86 must map to ARCH_X86). Without
coverage, this bug can regress silently across Windows environments.
Agent Prompt 
## Issue description
Windows architecture detection behavior was changed, but there is no test asserting the new mapping (e.g., `PROCESSOR_ARCHITECTURE=x86` => `arch == ARCH_X86`).

## Issue Context
This PR changes both the detection predicate (`contains("x86")`) and the Windows-native return strings (`"AMD64"`, `"x86"`). A small unit test (ideally platform-independent) should pin these mappings to prevent regressions.

## Fix Focus Areas
- rust/src/config.rs[78-92]
- rust/src/config.rs[313-327]
- rust/tests/config_tests.rs[28-59]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines 83 to 89
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Wow64 arch misdetected 🐞 Bug ≡ Correctness

On Windows, ManagerConfig::default now maps any PROCESSOR_ARCHITECTURE containing "x86" to ARCH_X86,
so a 32-bit selenium-manager process on 64-bit Windows will be treated as 32-bit and may select
win32 artifacts instead of win64. This happens because the code prefers PROCESSOR_ARCHITECTURE and
only calls GetNativeSystemInfo when the env var is empty, so it won’t detect the native OS
architecture in the common WOW64 case.
Agent Prompt 
### Issue description
On Windows, `ManagerConfig::default()` uses `PROCESSOR_ARCHITECTURE` as the primary source of truth for system architecture and only falls back to `GetNativeSystemInfo` when the env var is empty. Since this project explicitly notes the Windows selenium-manager is compiled as a 32-bit binary, that env var can reflect the process architecture (x86) even when the OS is 64-bit, causing the manager to select x86/win32 artifacts.

### Issue Context
This can change platform labels for downloads (e.g., `win32` vs `win64`) and may lead to selecting the wrong artifact set for some drivers.

### Fix Focus Areas
- rust/src/config.rs[77-93]
- rust/src/config.rs[312-327]

### Suggested fix
- On Windows, prefer `get_win_os_architecture()` (native OS architecture) over `PROCESSOR_ARCHITECTURE` for the default arch computation.
  - Simplest: always set `_architecture = get_win_os_architecture()` inside the `#[cfg(windows)]` block.
  - If you still want to allow env override, only use `PROCESSOR_ARCHITECTURE` when it clearly represents the native arch (or check `PROCESSOR_ARCHITEW6432` when `PROCESSOR_ARCHITECTURE` is `x86`).
- Ensure the mapping results in `ARCH_X64` for native AMD64 systems even when the process is 32-bit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down Expand Up @@ -316,8 +316,8 @@ fn get_win_os_architecture() -> String {
GetNativeSystemInfo(&mut system_info);

match system_info.u.s() {
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_AMD64 => "64-bit",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_INTEL => "32-bit",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_AMD64 => "AMD64",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_INTEL => "x86",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM => "ARM",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM64 => "ARM64",
si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_IA64 => "Itanium-based",
Expand Down
Loading