MSI file upgrade fix - #9164
Draft
gleocadie wants to merge 2 commits into
Draft
MSI file upgrade fix#9164gleocadie wants to merge 2 commits into
gleocadie wants to merge 2 commits into
Conversation
datadog_profiling_ffi.dll and ddwaf.dll carry no VERSIONINFO resource of their
own, so Windows Installer's file-replacement logic on upgrade falls back to
comparing created-vs-modified timestamps instead of versions. Combined with
MajorUpgrade having no Schedule (RemoveExistingProducts runs at 1401,
afterInstallValidate, before components get a chance to be removed outright),
an in-place upgrade could leave a stale copy of either DLL on disk next to
freshly-upgraded, version-matched managed/native modules -- this is exactly
what happened in the field: a two-year-old libdatadog v20.0.0
datadog_profiling_ffi.dll survived an upgrade to a tracer built against
libdatadog v25.0.0, and the resulting FFI struct-layout (ABI) mismatch
crashed the process with an access violation inside a static initializer at
startup, before the managed `catch (Exception)` around it could ever run.
Make both DLLs `CompanionFile`s of the versioned native module they already
ship alongside in the same directory (Datadog.Trace.ClrProfiler.Native and
Datadog.Tracer.Native respectively), so their upgrade replacement decision
always inherits that file's real version instead of falling back to the
unversioned-file heuristic:
- shared/Files.wxs: delete the standalone Shared.Files.Libdatadog.{32,64}
component groups; move the libdatadog `<File>` into
Datadog.Trace.ClrProfiler.Native{,.32}'s own component as a CompanionFile
(a companion must live in the same component as the file whose version
governs it -- that's the whole mechanism).
- Product.wxs: drop the two now-dangling ComponentGroupRefs.
- Tracer/Files.wxs: ddwaf.dll (x86) was already co-located with
Datadog.Tracer.Native.32 in one component, so that side is a one-attribute
change. ddwaf.dll (x64) was NOT actually co-located with Datadog.Tracer.Native
despite looking that way in the component group -- they were two separate
<Component> elements -- so that side needed an actual merge into one
component first. (Confirmed independently from the current
msi-x64.verified.yml File table: LibDdwaf.32's Component_ is already
Datadog.Tracer.Native.32, but LibDdwaf's is still LibDdwaf.)
- Both merged/modified components get an explicit Component `Id` matching
their primary file's Id, mirroring the existing Datadog.Tracer.Native.32
precedent, since WiX's keypath-by-first-file default is otherwise implicit.
- Companion files drop `Checksum="yes"` (mutually exclusive with companion
versioning); the primary/versioned files keep it.
- No `File` Id was renamed (WiX derives the 8.3 short name from it).
Bonus effect: removing the standalone libdatadog components (and merging
ddwaf.dll's x64 component into Datadog.Tracer.Native) changes their
auto-generated component GUIDs out of existence, so on the transition upgrade
specifically, RemoveExistingProducts deletes the old orphaned files outright
and the new ones install fresh -- CompanionFile is what then keeps every
upgrade after that one correct.
Still needed before merge (requires a Windows machine, not available in this
environment): regenerate tracer/build/_build/MsiValidation/msi-x64.verified.yml
via `.\tracer\build.cmd BuildMsi`. Expected diff: libdatadog{,.32} and LibDdwaf's
`Version` column goes from '' to their companion's file Id (the File table's
Version column doubles as the companion key), their `Component_` changes to
the merged component, and the two removed components disappear from the
Component table.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There is currently no upgrade test anywhere in the Windows MSI smoke suite -- every existing scenario does a single fresh `msiexec /qn /i` into a clean container, which is exactly why the previous commit's bug shipped undetected. Add a "WindowsMsi" scenario that installs the last release to ship libdatadog v20.0.0 (3.33.0, the exact "old" version from the crash report this is a regression test for) first, to create a genuinely aged on-disk state, then upgrades in place to the locally-built MSI under test: - smoke.windows.upgrade.dockerfile (new): modeled on smoke.windows.dockerfile. Downloads and installs the pinned previous release from GitHub releases (same URL shape already used by tracer/samples/WindowsContainer/Dockerfile), upgrades to the local build, then asserts -- as a build-time step, so a mismatch fails the image build itself -- that datadog_profiling_ffi.dll and ddwaf.dll under the installed win-x64 both hash-match this same build's own monitoring home (windows-tracer-home.zip, already staged into the smoke test artifacts dir alongside the MSI for the existing WindowsTracerHome scenario). SHA256 comparison is cheaper and more robust than parsing the PE debug GUID, and catches a stale file regardless of whether it happens to crash on startup. - SmokeTestScenario.cs: add an optional `PreviousReleaseVersion` to WindowsMsiScenario; only set for this one scenario. - SmokeTestScenarios.cs: one extra representative scenario (x64, net8.0) in WindowsMsiScenarios() -- the upgrade mechanism under test lives entirely in the MSI/WiX layer, not in the .NET runtime, so the full runtime x 32-bit-or-not matrix the other WindowsMsi scenarios use would be redundant here. - SmokeTestRunner.Builder.cs: BuildWindowsMsiImageAsync branches on PreviousReleaseVersion to build from the new Dockerfile instead (no dd-dotnet variant for this one -- irrelevant to what's under test). No changes needed to .azure-pipelines/ultimate-pipeline.yml: smoke_win_msi_matrix is generated dynamically from WindowsMsiScenarios() by Build.VariableGenerations.cs's EmitMatrix, and the existing smoke_win_msi_tests stage (gated on isMainOrReleaseBranch / run_all_installer_tests / isDockerImageBumpPr) already consumes that matrix generically -- this new scenario picks up a CI job automatically. Verified the touched C# compiles clean (`dotnet build tracer/build/_build`); the Docker/msiexec/PowerShell path itself needs a Windows machine with Docker Windows-container support to actually exercise, which isn't available in this environment.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9164) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-09-02 12:21:43 Comparing candidate commit 0ee6819 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 68 known flaky benchmarks, 58 flaky benchmarks without significant changes.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reason for change
Implementation details
Test coverage
Other details