Match strong-name identity when resolving PSES dependencies#2303
Match strong-name identity when resolving PSES dependencies#2303JustinGrote merged 1 commit intomainPowerShell/PowerShellEditorServices:mainfrom andyleejordan/alc-isolation-investigationPowerShell/PowerShellEditorServices:andyleejordan/alc-isolation-investigationCopy head branch name to clipboard
Conversation
`PsesLoadContext.IsSatisfyingAssembly` decided whether a candidate DLL in `$PSHOME` or our bundled `Common` directory could satisfy a dependency request using only the simple name and `Version >=`. That ignores the rest of the assembly identity, so a same-named assembly with a different public key token (i.e. a genuinely different assembly) was treated as a drop-in replacement. When the runtime then bound against it, the mismatch surfaced later as a `FileLoadException`/`TypeLoadException` rather than being declined up front. Patrick and I had suspected for a while that the version-only matching was inadequate, so this is a focused trial of tightening it. We now also require the public key token and culture to match: - If the requested reference is strong-named, the candidate's public key token must match exactly; a non-strong-named reference imposes no token requirement. - The culture must match, so we never substitute a satellite resource assembly for the neutral one (or vice versa). The check can only return `false` in more cases than before, and only for assemblies that could not have satisfied the reference anyway. On a token mismatch we now decline to short-circuit and fall through to the default load context's own (laxer) resolution instead of forcing a copy that fails at load. Measured against a current build, no presently-bundled dependency changes resolution under the new rules, so this is purely added protection. I pulled the pure comparison into an `internal` overload taking two `AssemblyName`s and added `PsesLoadContextTests` covering the version, name, public key token, and culture cases. The Hosting assembly (and thus `PsesLoadContext`) is .NET Core only, so the project reference and tests are guarded to `net8.0`/`CoreCLR`. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens dependency resolution in the PsesLoadContext AssemblyLoadContext to avoid incorrectly treating same-named but differently strong-named assemblies as valid substitutes, preventing late runtime bind failures and improving resolver correctness.
Changes:
- Refines
PsesLoadContext.IsSatisfyingAssemblyto require matching public key token (when the reference is strong-named) and matching culture, in addition to name +candidate.Version >= required.Version. - Extracts the identity comparison into an
internal staticoverload that compares twoAssemblyNameinstances (enabling direct unit testing). - Adds
net8.0-only unit tests and wiring (project reference +InternalsVisibleTo) to validate strong-name and culture matching behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/PowerShellEditorServices.Hosting/Internal/PsesLoadContext.cs |
Tightens assembly “satisfies” logic to include public key token and culture matching, and factors it into a testable overload. |
src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj |
Exposes hosting internals to the test assembly via InternalsVisibleTo for unit testing. |
test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj |
Adds a net8.0-only project reference to the Hosting project so tests can target PsesLoadContext. |
test/PowerShellEditorServices.Test/Session/PsesLoadContextTests.cs |
Introduces unit tests covering version/name matching, token matching rules, and culture matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Would this have any issues with different PowerShell versions providing different base assemblies with different strongnames/publickey than what we are trying to match? That'd be my only concern. |
|
@JustinGrote that's a solid question worth testing... |
|
@JustinGrote Good news: tested and it holds up. Short answer — no, this doesn't cause issues across PowerShell versions, because a newer PowerShell changes the assembly version but never the strong-name (public key token). The matcher accepts newer versions ( How I testedBuilt this branch, linked it into a sibling
Plus the new The proof for your exact concernHere's the same base assembly under .NET 8 vs .NET 10 — version bumps The well-known tokens ( And the E2E run asserting the actual executable/version that answered, per version: Why the public-key-token check is still the right callThe token comparison only rejects a same-named but genuinely different assembly (different publisher key) — and in that case falling through to PSES's own bundled copy is the correct, safer behavior (that's the bug this PR fixes). It never rejects a legitimate, forward-compatible Note on 5.1
Drafted by Copilot, reviewed by @andyleejordan. |
|
@JustinGrote @SeeminglyScience I think this is worth landing into a preview and hope it alleviates some of the assembly conflicts users run into. |
JustinGrote
left a comment
There was a problem hiding this comment.
I agree, LGTM lets see if it breaks anything in preview. I tried it on a few different PS versions locally (5.1, 7.4.2, 7.5.3, 7.6.1, 7.6.2) without apparent issue.
…2303)" This reverts commit b9fd1b3. #2303 is what broke `CanAttachScriptWithPathMappings` on Windows. A clean bisection shows its parent (#2304, 6ad4f46) passed Windows E2E in ~12 minutes, while #2303 itself hung for 5h51m on that exact test -- and every commit built on top of it inherited the hang. Months of green Windows runs precede #2303. The mechanism is in `PsesLoadContext.Load`. #2303 tightened `IsSatisfyingAssembly` to also require a matching public key token and culture. When a `$PSHOME` assembly previously satisfied a dependency by name+version, `Load` returned `null` and PSES *shared* PowerShell's single copy. Under the stricter check a token mismatch now fails that first test, so `Load` falls through and loads our *own* bundled copy into the isolated `PsesLoadContext` instead -- producing two copies of the same assembly in two load contexts and a split type identity. The debugger-attach handshake (`Debug-Runspace` subscribing to `RunspaceBase.AvailabilityChanged`, plus the stopped-event plumbing in SMA) relies on cross-context event wiring that silently breaks under such a split, so the attach never completes and the test waits forever. It only trips on Windows because that is where the `$PSHOME`-versus-bundled token divergence occurs. #2303's "no bundled dependency changes resolution" check was static and missed an assembly loaded dynamically during attach. #2303 was self-described as "a focused trial of tightening" the matching, so reverting it restores the long-standing, known-good behavior. We can re-attempt the hardening later with this attach test as a guard. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce this branch to its one honest, effective change: a 30-minute `timeout-minutes` on the CI test job. A normal run finishes well under that (Windows, the slowest, is ~12-14 minutes), so the cap only bounds a hung test instead of letting it ride GitHub's 6-hour default. This un-reverts #2303 and drops the earlier `ReadScriptLogLineAsync` change, both of which were based on a per-commit bisection that has since been disproven. The Windows debugger-attach test `CanAttachScriptWithPathMappings` intermittently wedges on the attach handshake and rides the default timeout; the same hang reproduces on `main` (which contains #2303) and reproduced here with #2303 reverted, so #2303 is not the cause and is restored. The attach test wedges before it ever reaches `ReadScriptLogLineAsync`, so that change could not affect the hang and its short internal cap risked introducing new flakiness on a slow-but-healthy attach; it is reverted too. The intermittent attach hang is tracked separately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
A focused trial of tightening how
PsesLoadContextdecides whether a candidate assembly can satisfy a dependency request. Patrick and I have suspected for a while that our version-only matching is inadequate; this proposes we trial requiring the full strong-name identity to match.Problem
On .NET Core,
PsesLoadContext.IsSatisfyingAssemblygates whether a DLL in$PSHOMEor our bundledCommondirectory can satisfy a dependency request, using only:candidate.Version >= required.Version.That ignores the rest of the assembly identity. A same-named assembly with a different public key token (i.e. a genuinely different assembly) was treated as a drop-in replacement, and the mismatch only surfaced later as a
FileLoadException/TypeLoadExceptionat bind time instead of being declined up front.Change
Also require:
The pure comparison moved into an
internal staticoverload taking twoAssemblyNames so it can be unit-tested without DLLs on disk.Why this is safe to trial
falsein more cases, and only for assemblies that could not have satisfied the reference anyway.Commonvs$PSHOME), so today this is purely added protection.Tests
PsesLoadContextTests(net8.0 /CoreCLRonly, since the Hosting assembly is .NET Core only) covers exact match, newer/older version, name case-insensitivity, differing public key token, strong-named-vs-unsigned, no-required-token, and culture match/mismatch. All 10 pass;net462still compiles (reference and tests are guarded).Context / scope
Part of the broader ALC isolation investigation behind the recurring "Assembly with same name is already loaded" / "Could not load file or assembly" reports. This is the resolver-correctness piece only — it does not by itself address the feature-side eager-loading (completion /
Get-Helpimporting user modules) or the Windows PowerShell "no ALC at all" class of issues.Open questions
Version >=rule (e.g. require major-version compatibility) — deliberately left as-is here.🤖 Drafted by Copilot (Claude Opus 4.8) for @andyleejordan to review and edit before merging.