fix(driver-manager): stop flagging installed drivers as stale duplicates - #262
Conversation
The stale-driver scan grouped driver packages by provider + device class and declared every package in a group except the highest-versioned one stale, unless it was bound to a device that was present at scan time. Both halves of that are wrong, and the scan pre-selects its results, so a Scan then Clean pass ran `pnputil /delete-driver` over them. Provider + class is not a driver identity. Vendors ship several unrelated drivers in one class (Intel's Ethernet and Wi-Fi packages are both Net), so unrelated packages were treated as one another's versions. The originalName fallback chain ended at the provider name, which collapsed the identity further when pnputil reported no original INF name. "Not bound to a present device" is not evidence of staleness either. Virtual network adapters only materialise a device while they are up, so a mesh/VPN tunnel driver looks unbound whenever the client is idle; removable hardware looks unbound whenever it is unplugged. pnputil only refuses to delete a package whose device is present, so nothing stopped the removal. Group by the original INF name instead, keying packages that report none on themselves so they can never look like a duplicate, and only call a package superseded when the same INF has a strictly newer package that is actually bound to hardware. A driver nothing is bound to is now left alone, and a failed bound-driver query yields an empty stale list rather than marking everything stale. cleanDrivers re-checks bindings before removing, since the list it acts on can be minutes old. Fixes #242
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0be855d78c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function driverIdentityKey(d: RawDriver): string { | ||
| const published = d.publishedName.trim().toLowerCase() | ||
| const original = d.originalName.trim().toLowerCase() | ||
| if (original.endsWith('.inf') && original !== published) return `inf:${original}` | ||
| return `pkg:${published}` |
There was a problem hiding this comment.
On systems where unrelated driver packages use the same original filename (for example, two vendors both shipping driver.inf), this key merges them because original INF names are not globally unique. If the higher-versioned package is bound, the other package is classified as stale and pre-selected for deletion even though it is a different driver; include provider or another package identity signal in the key rather than relying on the filename alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83ca106 — the key is now inf:<original>|<provider>, and a package missing either half is keyed on itself so it can never look like a duplicate. Regression tests cover two vendors shipping driver.inf and the unknown-publisher case.
| // Strictly older than the bound copy. Equal, missing or unparseable | ||
| // versions compare as 0 and are left alone rather than guessed at. | ||
| if (compareVersions(anchor.version, d.version) > 0) { | ||
| superseded.add(d.publishedName.toLowerCase()) |
There was a problem hiding this comment.
When the bound package has a valid version but another package in its group has a missing or nonnumeric version, compareVersions converts the unknown value to zero, so this condition marks that package superseded—for example, 2.0 compares greater than an empty string. The scan then pre-selects an unverified package for deletion despite the stated fail-safe behavior; validate both version strings before ordering them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83ca106 — both the anchor and the candidate must now have a readable dotted-numeric version before they are ordered, so a package with a missing or non-numeric version is never marked superseded. Three tests added for the mixed cases; the doc comment was wrong about this too and has been corrected.
…own versions Two gaps in the supersession rule, both from review. The original INF name is a filename, not a globally unique id: generic names such as driver.inf ship from more than one vendor, so keying on it alone could still group two unrelated packages and mark the unbound one stale. Include the publisher in the key, and fall back to keying a package on itself when either half is missing. compareVersions() reads an absent or non-numeric version as zero, so a bound package at "2.0" ordered above a package whose version pnputil did not report and marked it superseded — the opposite of the documented fail-safe. Only order two packages when both versions are readable dotted numbers, on both the anchor and the candidate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83ca106d25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| originalName: | ||
| fields['original name'] || | ||
| fields['original inf'] || | ||
| fields['driver package provider'] || | ||
| publishedName, |
There was a problem hiding this comment.
On Windows versions where successful legacy pnputil -e output omits Original Name, this fallback makes originalName equal the unique oem*.inf published name. driverIdentityKey consequently assigns every package its own pkg: key, so no group reaches two entries and the scan reports zero stale drivers even when an older package was replaced. I checked the scanDrivers command order: it still runs the legacy -e form first and tries /enum-drivers only when that command fails, so successful but information-poor output is never retried; prefer /enum-drivers first or recover original INF identities before applying this fallback.
Useful? React with 👍 / 👎.
1 participant
Fixes #242.
What was wrong
scanDrivers()decided which driver packages are "stale" like this:provider + device class.pnputil /delete-driver.Both of the signals it relied on are wrong.
provider + classis not a driver identity. Vendors routinely ship several unrelated drivers in one class — Intel's Ethernet and Wi-Fi packages are both classNet. Grouping them makes the lower-versioned one look like a stale duplicate of the higher-versioned one.parseEnumDriversmade this worse:originalName's fallback chain ended atDriver Package Provider, so when pnputil reported no original INF name, the "identity" collapsed to the vendor name outright."Not bound to a device" is not evidence of staleness.
getActiveDriverNames()readsWin32_PnPSignedDriver, which only covers devices that are present right now. A mesh/VPN tunnel adapter only materialises a device while the tunnel is up, so its driver looks unbound whenever the client is idle. Unplugged docks, printers, phones and dongles look the same.pnputil /delete-driveronly refuses a package whose device is present, so nothing downstream stopped the removal either.Together: a working Tailscale install with its tunnel down had its Wintun package grouped against another Tailscale-provided
Netpackage, marked stale, pre-selected, and deleted. The client can then no longer create its adapter, which is theNoState/ "Tailscale is starting, please wait" loop in the issue that only a reinstall clears.The fix
wintun.inf), which is what actually makes two packages versions of the same driver. Packages that report no original name are keyed on themselves, so they can never look like a duplicate of anything.Driver Package Providerfrom theoriginalNamefallback chain.cleanDrivers()re-queries bindings and refuses anything currently bound, since the list it acts on can be minutes old.Note there's no vendor allowlist here. The issue suggested excluding known VPN/mesh drivers by name; that turned out to be unnecessary — under the new rule a lone Wintun package is a group of one and is never removable, and the same protection extends to every vendor rather than a hardcoded list.
Tests
New
src/main/ipc/driver-manager.ipc.test.ts— 27 cases coveringparseEnumDrivers,driverIdentityKey,findSupersededDrivers,scanDriversandcleanDrivers, including the two #242 regressions (unbound tunnel driver kept; two INFs from one vendor+class not treated as versions of each other), the fail-safe path, andcleanDriversrefusing a bound package.Full suite: 2243 passing.
Not addressed
The issue also reports
ollama app.exebeing deleted from%LOCALAPPDATA%\Programs\Ollama\. I traced every file-deletion path in the main process and could not find one that reaches a single executable inside an install directory without the user explicitly selecting that file: the rule-driven cleaners only touch paths declared inrules/, the leftover scanners operate on whole folders and never auto-select, and the duplicate/large-file finders require the user to choose a directory and tick individual files. Worth keeping the issue open for that half, or splitting it out — I don't want to claim a fix I can't point at.