Modernize the Windows MSI: WiX v5, x64, upgrade from 5.1.x

[vharseko]

Modernizes the Windows MSI end to end. Builds on #664 (branched from it) and includes the Windows script quoting fixes from #671 (they merge cleanly once #671 lands).

Toolchain: WiX 3.11 + wine → WiX v5, Windows-native

• WiX 3.11.1 (2017, EOL) → WiX v5.0.2 (.NET wix tool): heat/candle/light + wine + winetricks + dotnet40 collapse into a single wix build -arch x64. WiX v5 is the newest version without the Open Source Maintenance Fee (v7 refuses to run without accepting the OSMF EULA).
• The MSI is built on Windows only. Verified empirically (wix 5.0.2/6.0.1/7.0.0 on macOS): non-Windows builds fail on WIX0389 for any Directory/@Name and, with no names at all, on the missing msi.dll — the MSI database is written by the Windows Installer library, so no cross-platform build exists. wine is removed from build.yml, deploy.yml and release.yml.
• package.wxs rewritten to the v4+ schema: single <Package>, MediaTemplate CompressionLevel="high", InstallerVersion=500, <Files> harvest from Ant-staged payloads, empty instance directories shipped via CreateFolder components (the harvest ships files only).
• The opendj-msi module is pulled in by a distribution-windows-msi profile that activates only when the wix .NET tool is installed, so a plain mvn install on a contributor's Windows machine no longer fails mid-reactor.

Installer behavior

• x64: installs into C:\Program Files\OpenDJ (was x86 / Program Files (x86)).
• No Windows service is registered. The package lays the files down and nothing more, exactly as the WiX3-era one did: registering the server as a service stays an explicit windows-service.bat --enableService (or setup) step, and the service that results belongs to the administrator — the package never creates, removes or reconfigures one. An earlier revision of this PR registered an OpenDJ service from the installer; it was reverted in the sixth review round because it silently switched every MSI install into service mode, so start-ds.bat dispatched to the SCM and start-ds.bat -N failed outright even for a server that was never meant to run as a service.
• A running service is stopped before the files are replaced, or the upgrade is refused. RemoveExistingProducts empties the old tree during the sequence pass, and the JVM opens lib\*.jar without FILE_SHARE_DELETE (JDK-8224794, Won't Fix), so a running instance turns the nested uninstall into delete-on-reboot entries. An immediate best-effort net stop runs first, then an immediate check that lets the upgrade continue only when the service is gone or Stopped and otherwise refuses it (error 1722 naming CheckServiceStopped). The check polls rather than sampling the SCM once: net.exe returns the instant a control cannot be accepted, which is the case in every pending state, so a single "is it Running" test passed exactly the situations the stop had not handled — an administrator's own net stop still in flight, or a service still starting, which the wrapper gives 300 s while its JVM holds lib\*.jar. A stop already under way is waited out for 90 s; every other state refuses immediately. Both actions are gated on the service's own ImagePath pointing into the directory being installed to, so an unrelated instance's service is never touched. The stop succeeds from an elevated console, SCCM, SYSTEM or CI and cannot under the filtered token of a UAC double-click — which is exactly why the check exists rather than a silent half-upgrade.
• A server running without a service is refused too. The package registers none, so start-ds.bat is the default way an MSI-installed server runs, and that JVM holds lib\*.jar exactly as a service-hosted one does. A third check, CheckServerNotRunning, probes the exclusive byte-range lock the server holds on locks\server.lock — the same evidence wait-server-stopped.ps1 uses in CI, because a zero exit code from stop-ds is not proof the JVM let go (Flaky Test on Windows: winlauncher.exe stop can return success without stopping the server #768) — and refuses the upgrade while it is held, allowing 60 s for a stop already under way. It is gated on nothing but WIX_UPGRADE_DETECTED, so it covers both modes. Restart Manager is set to DisableShutdown rather than Disable: RM must not stop the service (it would restart the upgraded server before upgrade.bat has migrated the instance data, and would take a StartPending service away from the check above), but Disable also turns off RM's detection, leaving only the legacy in-use scan, which finds windowed applications and therefore never a headless JVM. DisableShutdown keeps the detection and leaves the shutting down to the checks.
• Upgrade over an existing install: the installer detects the location recorded in the registry by this package, or the legacy x86 default directory (two separate searches, registry wins), and installs there — instance data (config, db, logs) is preserved; an explicit OPENDJ=... always wins. During an upgrade the legacy directory has to prove it holds a server (a setup.bat in it) before it is adopted: Installed is unset throughout a major upgrade, so a bare existence test would have routed the new tree into a leftover empty directory while RemoveExistingProducts emptied the real installation somewhere else. A registered service survives untouched: it names the wrapper inside the installation directory, which the upgrade replaces in place, so the registration keeps working against the refreshed server. Verified in CI by upgrading from the released 5.1.2 MSI at its real x86 default with no OPENDJ given, with the service left running. AllowSameVersionUpgrades keeps a same-version hotfix re-release from producing two ARP entries.
• Two upgrades are refused rather than performed, both from the execute sequence after CostFinalize — the one point where OPENDJ is final whether the installer was started silently or with full UI:
  • The location cannot be determined. 5.1.x wrote no registry value, so an upgrade from a custom directory resolves no search and falls back to the default; installing there while RemoveExistingProducts empties the old tree would strand config/db/logs. The refusal is value-based — it fires when the resolved directory is that default and nothing recorded a location — so msiexec /i ... /quiet naming nothing still refuses, while a GUI administrator browses to the existing directory in InstallDirDlg and proceeds. (An earlier revision aborted in the UI sequence before WelcomeDlg, leaving a GUI-only administrator with nothing but a command line to retype.) The one case the wizard cannot resolve is an old server in the default directory itself: the "was it named" flag is read before AppSearch and InstallDirDlg never sets it, so browsing to the default leaves the guard with the values a "Next, Next, Install" session produces. The refusal message and the install guide say so and point at the command line, rather than promising a dialog that cannot help. The test is deliberately "nothing recorded it" rather than "no server is installed there": any unrelated OpenDJ tree — a zip install, a copy — satisfies a setup.bat search without being the product being upgraded, and keying the guard on that let such a decoy disarm it while RemoveExistingProducts gutted the real installation elsewhere. Naming the directory is the way through, including for an old server that really does live in the default one, which a registry-less 5.1.x is indistinguishable from the decoy without being told. The same decoy in the legacy default directory remains undetectable when nothing is named — nothing a registry-less 5.1.x left behind separates the two, and refusing every registry-less legacy-default upgrade would refuse the documented main upgrade path with it — so the install guide carries the workaround instead: name the real directory, which the relocation guard below now lets through.
  • The upgrade would relocate. Passing OPENDJ=<somewhere else> over a detected installation is not a move — the old tree is emptied while the new one is installed elsewhere. Refused whenever the existing location is known authoritatively (the recorded registry value, or a legacy default directory that really holds a server) and the requested directory is not the same one. The comparison is an equality test, not a substring one: "contains" would have admitted C:\opendj\v2 over a recorded C:\opendj, which is the relocation being guarded against. The legacy branch makes one exception, without which the two guards contradicted each other: a directory that was named on the command line and holds a server is taken as the installation being upgraded, not as a relocation target. That is exactly the workaround the decoy residual above prescribes, and refusing it left such a host with no upgrade path at all. Naming an empty or new directory is still a relocation and still refused, and the registry branch keeps refusing unconditionally — a location this package recorded is authoritative. The residual: a named directory holding some other OpenDJ tree is now accepted, which directory evidence cannot distinguish from the real one; between refusing the documented workaround and trusting an explicit instruction, the instruction wins.
• Java: the installer does not check for or install a JRE (a JAVA_HOME/PATH launch condition was tried and dropped — it blocked upgrades of working servers and never consulted OPENDJ_JAVA_HOME, see the review discussion and Add CI install-test for the Windows MSI + document MSI install/upgrade/uninstall #664). setup and the server require Java 11+; the install guide points at https://adoptium.net. The MSI is not code-signed (no certificate) — SmartScreen note added to the docs.

Release / deploy

• deploy.yml reuses the MSI already built by the triggering Build run (no rebuild).
• release.yml: new release-msi job (windows, continue-on-error) installs the released server zip into the local repository and packages only :opendj-msi-standard, then attaches the MSI to the GitHub release.
• opendj-msi-standard is intentionally no longer published to Maven Central: it was previously wine-built on Linux and deployed from the ubuntu deploy jobs; it is now built natively on Windows and distributed through GitHub Releases and the Package/Deploy OpenDJ MSI Package artifact. The Maven coordinate stays at 5.1.2 as its last published version. This also applies to sustaining/4.10.x: Package/Deploy always runs master's workflow, which no longer installs wine, while the 4.10.x pom still builds its MSI through it — accepted; the 4.10.x MSI remains available up to its last published release.
• The committed Windows launchers are refreshed by CI, not by hand. opendj-server-legacy/lib/*.exe is what every Linux-built server zip ships — and with it the tagged releases and the Maven Central artifacts — while only a Windows job can rebuild them, so a native source change that was never re-committed shipped the old wrapper while CI stayed green (master carried exactly that gap from April to July). Package/Deploy now downloads windows-exe-11 from the triggering Build run and commits the binaries back when their bytes differ. It runs only after a green push build on a release branch and already holds contents: write for the wiki push, so build-maven stays read-only; /Brepro in the Makefile keeps the output a function of the sources, without which it would commit on every run; and pushes made with GITHUB_TOKEN start no workflow run, so it cannot loop. The step sits ahead of the Maven deploy so the published snapshot carries the fresh launchers, which also means it must never be what costs that deploy: it retries the push three times over a rebase and then warns and lets the job continue. The warning-only comparison step and the standalone Native launchers workflow are removed.

CI

• test-msi: installs into the spaced x64 default directory, asserts the package registered no service and that lib\opendj_service.exe was laid down, runs setup --doNotStart, then starts and stops the server with start-ds.bat/stop-ds.bat — proving an MSI install that never asked for service mode behaves like a zip one. Only then does it enable the service with windows-service.bat, start it, ldapsearch, stop and disable it; uninstall.bat --cli must remove the instance and disable a service it finds; msiexec /x must leave nothing behind. The wait-for-the-lock helper the zip test uses after stop-ds (a zero exit code is not evidence the JVM let go — Flaky Test on Windows: winlauncher.exe stop can return success without stopping the server #768) moved to .github/scripts/wait-server-stopped.ps1 so both jobs share one copy.
• test-msi-upgrade: released 5.1.2 x86 MSI at its native Program Files (x86)\OpenDJ default → configure → service registered the pre-MSI way and left running → upgrade with the new x64 MSI with no OPENDJ given: the installer stops the running service (the stop+check pair, elevated here), keeps the same directory, leaves the data intact and the registration in place, then upgrade.bat, start through that same service, search, stop. A repair (REINSTALL=ALL) must not disturb the registration; disabling the service before uninstalling must leave no orphan. Plus fresh-install and refusal scenarios: the legacy default directory is auto-detected, the registry-recorded location wins over it, a silent upgrade from an undetectable custom directory refuses with guidance and the same upgrade succeeds once the directory is named, a leftover empty legacy directory is not adopted as the target of an upgrade, an upgrade that would relocate to another directory is refused on both branches of the guard, an install into a third directory leaves a zip instance's OpenDJ Server untouched despite a leftover legacy directory, an unrelated OpenDJ tree in the x64 default directory is not adopted as the target of an upgrade whose real installation is elsewhere (refused, then completed once the directory is named), a 5.1.x installed in that default directory refuses the silent upgrade and accepts the named one, an upgrade started while the service is StartPending is refused by CheckServiceStopped with the instance intact — that state produced deterministically by holding bat\start-ds.bat open, which keeps the wrapper reporting SERVICE_START_PENDING — a decoy in the legacy default directory does not block the documented workaround (naming an empty directory is still refused as a relocation, naming the real installation upgrades it), and an upgrade over a server running without a service is refused by CheckServerNotRunning and then succeeds once stop-ds.bat has run. The relocation scenarios install the released 5.1.2 package first: reinstalling this MSI over itself is maintenance mode, where FindRelatedProducts does not run, so WIX_UPGRADE_DETECTED would never be set and the guard could not fire.
• Neither MSI job depends on build-maven any more. The matrix's ubuntu legs run for about two hours, so every push inside that window cancelled the run before these two-minute jobs started — which is how several rounds of guard work reached review with no completed run behind them. Both now depend on a small wait-msi-artifact job that polls for the windows-latest-11 artifact and gives up early if the Windows leg finishes without publishing it. That job runs on ubuntu: waiting on windows-latest held a Windows runner from t=0 and competed for the capacity the leg being waited for needs (in run 31578978374 the windows/11 leg queued for 38 minutes while windows/26 started within one), and its fixed 45-minute budget covered the leg's runtime but not its queue time — in that same run the artifact appeared at +52 minutes, so both jobs would have failed a healthy build. One gate job also means one copy of the wait instead of two.
• The windows/11 job validates the built package with wix msi validate -sice ICE61 (ICE): wix build itself runs no ICE validation, so a green build alone would not catch sequencing mistakes like ICE63. ICE61 is suppressed because AllowSameVersionUpgrades authors it by design.

Windows script fixes surfaced by these tests (in #671)

Install paths with spaces/parentheses (C:\Program Files (x86)\OpenDJ — the old default!) broke the batch scripts in four places: unquoted java.io.tmpdir, setcp.bat argument parsing, "%VAR%" == "" checks with the now-quoted tmpdir, and the start-ds.bat tmp-cleanup block. All fixed and exercised by the MSI tests in this PR.

Deferred

• Authenticode signing — no certificate.
• A Burn bundle auto-installing the JRE — dropped; the installer neither bundles nor checks for a JRE (see above), the docs carry the download link.
• A release-time check that the committed launchers are not older than their sources: written and dropped once Package/Deploy started committing them automatically. It would cover the two residuals — a release cut in the window between a merge and the deploy run, and a master whose Build is red — and can be added on request.

The launcher binaries under opendj-server-legacy/lib were refreshed from the windows-exe-11 artifact in 24b1b7e. service.c has changed in code since — openScm tested scm where it meant *scm, so a failed OpenSCManager was never reported (pre-existing; the error still surfaced from the NULL handle, only without the message) — so the committed binaries are one fix behind their sources until Package/Deploy re-commits them on the next release-branch push. The Windows build leg compiles them itself before packaging, so the MSI and the Windows-built zip already carry the fix. The GUI wizard path (license-dialog skip, InstallDirDlg pre-population, the two upgrade refusals) is not covered by CI — one manual run before merge is planned.

Still outstanding: a completed Build run. The last one was 252d5ba9b (10 Aug); every run since was cancelled by the next push, so none of the upgrade guards — the two refusals, the stop-and-verify pair, the new CheckServerNotRunning, or the rework above — has ever executed in a finished run, and neither has wix msi validate -sice ICE61 on the current authoring. The gate job described under CI is what makes that reachable in ~20 minutes instead of two hours, and it no longer times out on a queue; the run on this head is the one to judge the guards by.

[vharseko]

Thanks for the sixth pass — the two blockers landed the PR in a much better shape than arguing them would have. Both are addressed, along with every major, medium and nit; notes follow your headings. Commits: 24b1b7e (launcher gate), b2de060 (service rollback), 92b30bd (upgrade guards, service state, ICE61, CI cleanup).

MSI installs unconditionally become service installs

Conceded outright and reverted rather than gated. ServiceInstall/ServiceControl and the event-log keys are gone; windows-service.bat is once again the only thing that registers a service, exactly as for the zip delivery, and the package neither creates, removes nor reconfigures one. Your consequence list was the whole argument: nothing about start-ds.bat -N or the SCM reroute is defensible for an administrator who ran setup --cli --doNotStart, and gating registration behind a property would have kept the machinery alive for a feature nobody asked for.

Everything that existed only to support an installer-owned service went with it: isMsiManagedService and return code 4, SERVICE_MSI_MANAGED in ConfigureWindowsService, Uninstaller and the control panel, both localized messages in all nine bundles, and serviceCmdsMatch with nextCmdToken/normalizeInstanceDir/expandLongPath — with no msiexec-written ImagePath left to parse, matching returns to the plain _stricmp it was. Those five Java files and all twenty message bundles are byte-identical to master again; the PR went from 44 changed files to 19.

RemoveLegacyServiceControl is gone too, and for a reason worth stating: an in-place upgrade replaces the wrapper the registration names, so the service keeps working against the refreshed tree and was never ours to remove. The stop-and-verify pair stays and is now gated on the ImagePath evidence alone rather than on LEGACY_MSI_DETECTED — the locking problem is the JVM's, not the packaging's, so a 5.2.x-to-5.2.y upgrade over a running instance needs it just as much (the detect-only <Upgrade> row became dead and was removed). The actions are renamed StopServiceBeforeUpgrade/CheckServiceStopped since "legacy" no longer describes what they cover.

On CI: test-msi now starts the server with start-ds.bat and stops it with stop-ds.bat before any service exists — the assertion whose absence you identified — and only then enables, exercises and disables the service. test-msi-upgrade asserts the administrator's registration survives the upgrade stopped-but-registered, that no OpenDJ service appears, and that the upgraded server starts through that same registration. The install guide documents the service as the opt-in step it is again, including disabling it before uninstalling.

Stale committed launchers can ship in a release

Taken, but neither way you offered. Both gates are removed — the warning-only step and the standalone workflow — and Package/Deploy now commits what the Build run already compiled: it downloads windows-exe-11 from the triggering run, copies it into opendj-server-legacy/lib and pushes when the bytes differ. A gate tells a human they forgot; this simply stops the drift existing. master proved the failure mode is real and not hypothetical, incidentally: it has been carrying launchers from 3 April against a service.c last changed on 28 July by #773, and neither gate existed then to say so.

Package/Deploy rather than Build for three reasons: it already holds contents: write for the wiki push, so build-maven — which runs the whole Maven plugin tree — stays read-only and no new write scope is granted anywhere; its job condition already restricts it to a successful push build on master/sustaining/4.10.x; and committing before the Maven steps means the snapshot it publishes carries the fresh launchers too. It rests on /Brepro (already in the Makefile), without which every run would produce different bytes and it would commit on every push; a toolchain bump still produces a refresh commit, which is correct — the committed binary then matches what CI verifies. Pushes made with GITHUB_TOKEN start no workflow run, so it cannot loop.

Two things I owe you here. The committed binaries in this PR are still stale and I will land the refresh from the windows-exe-11 artifact of the Build run on this head before merge — that also gives the new step a no-op first run rather than debuting on a real commit. And release.yml still has no gate: I wrote one (an ancestry test — the commit that last touched src/build-tools/windows must not come after the one that last refreshed lib/*.exe, on Linux, no MSVC, so a runner image bump cannot fail a release) and dropped it once the auto-commit landed. Say the word and it goes back as a second line of defence; the residual it covers is a release cut in the window between a merge and the deploy run, and a master whose Build is red.

SERVICE_LIST_UNAVAILABLE handled in only one of four callers

Fixed where it lies: serviceState() maps it to return code 2, which ConfigureWindowsService already maps to SERVICE_STATE_ERROR, so --serviceState, Uninstaller, the control panel and isRunningAsWindowsService() all stop acting on a fabricated "disabled".

startService() and serviceMain() I left alone deliberately, and I think the "three of four callers" framing overstates it: neither reports a state to anyone. Both already treat any non-OK from getServiceName as failure and return an error, so there is no lie to remove — only a less specific debug line.

isMsiManagedService fails open on a hardcoded registry key

Moot — the function is deleted. For the record your analysis was right and the bug was not fixable as written: opendj_service.exe is prebuilt and committed, one binary for every brand name, so $(var.name) is not available to it at compile time. Driving it from the same variable was impossible; reading a marker from the service key would have been the only correct shape.

The legacy ImagePath is REG_EXPAND_SZ, not REG_SZ

Confirmed, the comment was simply wrong. It now states that CreateService stores ImagePath as REG_EXPAND_SZ, that a raw search returns it unexpanded with the #% prefix, and that the substring test is therefore load-bearing rather than a stepping stone — neither an equality test nor << could ever match, so the tightening the old comment invited is explicitly ruled out.

A relocating legacy upgrade orphans the legacy service

Refused rather than repaired. RefuseRelocatingUpgrade fires when a location is known authoritatively — the value this package recorded, or a legacy default directory that really holds a server — and the requested OPENDJ is neither a prefix nor an extension of it. Your scenario now aborts with guidance instead of stranding config/db/logs and a service registration in the emptied tree, and CI covers it: install into C:\opendj-a, attempt an upgrade with OPENDJ=C:\opendj-b, assert the refusal, the intact original and that C:\opendj-b was never created.

Two honest residuals. The two-way substring test tolerates the trailing backslash that OPENDJ carries and a command-line value may not, at the cost of C:\dj vs C:\dj2 — the mirror of the residual already documented on the service check. And a 5.1.x installed in a custom directory with no legacy default present leaves us with no authoritative old location at all, so that relocation cannot be detected by construction; the install guide says the installer replaces in place and cannot move an installation.

The GUI upgrade path dead-ends

Agreed on the problem, implemented differently — because both fixes you suggested reintroduce the failure the guard exists for. In a full-UI session the UI sequence's CostFinalize resolves OPENDJ to the default and hands it to the execute sequence, so an execute-only or UILevel < 5 guard sees a perfectly ordinary directory value and cannot fire: "Next, Next, Install" then relocates to C:\Program Files\OpenDJ while RemoveExistingProducts empties the old tree. That is the data-loss path, not a corner case.

What actually distinguishes the two is not whether a directory was given but which one. So the guard moved to the execute sequence after CostFinalize and became value-based: refuse when the resolved directory is the default and no server is installed there (a new FileSearch for setup.bat under the x64 default). The InstallUISequence row is gone, so the wizard runs to InstallDirDlg and browsing to the existing installation makes the comparison fail and the install proceed — while msiexec /i new.msi /quiet with nothing named still refuses, because it resolves to that same empty default. CI asserts both halves now: the refusal, and that naming the directory makes the very same upgrade succeed.

Nits

• ICE61: wix msi validate -sice ICE61 "$msi", with a comment on why it fires by design.
• Three SCM enumerations: back to the two master always did — the third came with isMsiManagedService. The serviceNameInUse-after-getServiceName tautology predates this PR; happy to take it in a follow-up.
• Refusal scenario cleanup: the step now clears all three detection signals itself — both default directories and HKLM\SOFTWARE\OpenDJ — instead of inheriting the previous step's uninstall.
• isMsiManagedService duplicates serviceCmdsMatch: moot, both deleted.
• Hand-rolled path canonicalisation: moot. One correction for the record: getCanonicalDirectoryPath was not a drop-in replacement — it resolves .. and forward slashes but does not expand 8.3 names, which was the reason expandLongPath existed alongside it.
• Workflow duplication: moot, the standalone workflow is deleted and only one copy of the block remains.

Four things still need a green run on this head before I would call the round closed: the reworked test-msi/test-msi-upgrade scenarios, the -sice ICE61 spelling for the WiX 5 CLI, FileSearch nested in DirectorySearch under the v5 schema, and the launcher refresh commit. The Package/Deploy auto-commit cannot be exercised from a branch at all — its job only runs for a push build on a release branch — so it stays reviewed-not-run until the merge.

[maximthomas]

Reviewed the installer logic against the MSI docs — the design holds up well, and the comments in package.wxs are unusually good. Two of the new CI assertions cannot pass as written, and one guard hole is still open. Everything else is minor.

For the record, I checked and am withdrawing several concerns from the earlier round: the { exit 1 } block in ExeCommand is safe (Formatted only strips braces that wrap a [PROPERTY]), the exec-maven-plugin bump is behaviour-identical for mib-generation (verified by running it under both 1.3.2 and 3.6.3 — 20 generated files byte-identical), the post-/x payload assertion is real (uninstall.bat never deletes lib\), and the continue-on-error on the MSI deploy steps is fine since a missing MSI already fails Build upstream.

Refusal-message assertion never matches (blocker)

.github/workflows/build.yml greps for a string the installer does not emit:

if (-not (Select-String -Path upgrade-custom.log -Pattern "passed explicitly" -Quiet)) { ... }

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs says "pass it explicitly":

<CustomAction Id="RequireDirOnCustomUpgrade"
              Error="... or pass it explicitly: msiexec /i opendj.msi OPENDJ=&quot;C:\path\to\opendj&quot;"/>

The step throws even when the installer refuses exactly as designed. Suggest matching on something more stable, e.g. location could not be determined.

Relocation scenario cannot exercise the guard (blocker)

The step installs and then reinstalls the same MSI, i.e. the same ProductCode:

$msi = (Get-ChildItem -Recurse -Filter *.msi -Path opendj-packages/opendj-msi | Select-Object -First 1).FullName
Start-Process msiexec ... "/i `"$msi`" OPENDJ=C:\opendj-a ..."
Start-Process msiexec ... "/i `"$msi`" OPENDJ=C:\opendj-b ..."   # maintenance mode

Per FindRelatedProducts: "The FindRelatedProducts action only runs the first time the product is installed. The FindRelatedProducts action does not run during maintenance mode or uninstallation."

So WIX_UPGRADE_DETECTED is never set, RefuseRelocatingUpgrade cannot fire, and the step's throw "a relocating upgrade must be refused" always trips. AllowSameVersionUpgrades="yes" does not help — it concerns a different ProductCode at the same version. The scenario needs a second package with its own ProductCode, like the 5.1.2 MSI used in the neighbouring test.

Legacy directory adopted during an upgrade (major)

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:

<Property Id="OPENDJ_LEGACY" Secure="yes">
  <DirectorySearch Id="LegacyX86InstallDir" Path="[ProgramFilesFolder]$(var.name)"/>
</Property>

<SetProperty Id="OPENDJ" Action="SetOpendjFromLegacyDir" ...
             Value="[OPENDJ_LEGACY]"
             Condition="NOT Installed AND OPENDJ_LEGACY AND NOT OPENDJ"/>

The comment says the bare existence test is deliberate "because adopting an empty legacy directory on a fresh install is deliberate" — but Installed is unset during a major upgrade too, so NOT Installed does not express that.

Scenario: 5.1.x at C:\opendj, a leftover empty C:\Program Files (x86)\OpenDJ, and msiexec /i new.msi /quiet with no OPENDJ=. OPENDJ_REG is empty (5.1.x wrote no registry value), so OPENDJ becomes the legacy directory. RequireDirOnCustomUpgrade skips it (not OPENDJ_DEFAULT), and RefuseRelocatingUpgrade skips it too because OPENDJ_LEGACY_INSTALL needs a setup.bat the empty directory does not have. RemoveExistingProducts then removes the payload from C:\opendj — db/logs survive, but config\config.ldif goes and a pristine tree lands in the wrong place, leaving an unusable instance.

Note .github/workflows/build.yml deletes that directory right before testing the guard, so CI does not catch this:

Remove-Item -Recurse -Force "C:\Program Files (x86)\OpenDJ" -ErrorAction SilentlyContinue

Gating the fallback on OPENDJ_LEGACY_INSTALL when WIX_UPGRADE_DETECTED would close it while keeping the empty-directory adoption on fresh installs.

Service started without waiting for the lock (major)

.github/workflows/build.yml, test-msi ends a step with:

& "$root\bat\stop-ds.bat"
if ($LASTEXITCODE -ne 0) { throw "stop-ds.bat failed: $LASTEXITCODE" }

and the next step goes straight to windows-service.bat --enableService + net start. The zip test in the same file guards exactly this sequence:

opendj-server-legacy\target\package\opendj\bat\stop-ds.bat
Wait-ServerStopped 'opendj-server-legacy\target\package\opendj\locks\server.lock'
opendj-server-legacy\target\package\opendj\bat\windows-service.bat --enableService

Wait-ServerStopped was added for #768, where winlauncher.exe stop returned success without stopping the server — so the exit-code check is not a substitute. The helper is currently inline in build-maven; it needs extracting to be reused here.

Launcher commit can abort the whole deploy (major)

.github/workflows/deploy.yml places the new commit step before mvn deploy, the package uploads and the wiki push, under set -e and with no guard:

git push --quiet origin "HEAD:refs/heads/$BRANCH"

Every neighbouring step in that job carries continue-on-error: true; this one does not, and nothing later uses if: always(). master is not protected today and the job has a concurrency group, so a lost race is unlikely — but when it happens it silently costs the snapshot deploy, all packages and the docs. A retry (git push || (git fetch && git rebase && git push)) or continue-on-error: true covers it.

Nits (minor)

• serviceState() comment overstates the fix: in opendj-server-legacy/src/build-tools/windows/service.c, the new exit code 2 is correct, but the comment claims it fixes the uninstaller, the control panel and isRunningAsWindowsService(). It does not — SERVICE_STATE_ERROR is private in ConfigureWindowsService, and all three callers test == SERVICE_STATE_ENABLED, so ERROR and DISABLED stay indistinguishable to them. Only the CLI exit code and message change. Worth trimming the comment to what the change actually does.
• Subdirectory relocation slips RefuseRelocatingUpgrade: OPENDJ=C:\opendj\v2 over a recorded C:\opendj\ passes, because OPENDJ ~>< OPENDJ_REG is true. The comment names C:\dj vs C:\dj2 as the accepted residual but not the sub/parent-path case — worth naming it, since the tolerated "extension" is what admits it.
• release-msi writes the shared Maven cache: in .github/workflows/release.yml, mvn install:install-file without -DpomFile stores a dependency-less generated pom, and actions/cache@v5 then saves it under the same ${{ runner.os }}-m2-repository key build-maven restores from. Nothing resolves a released opendj-server-legacy version today, so it is inert — but actions/cache/restore here would avoid seeding the shared cache from a one-off job.

[vharseko]

Thanks for the seventh pass — and for the four withdrawals, which saved me from re-litigating them. Everything here is accepted: both blockers were mine, and both landed yesterday in the same commit that was supposed to be closing this class of problem. Addressed in 1cf5a88; notes follow your headings.

Worth recording alongside your review: the Build run on the head you reviewed never reached the MSI jobs — build-maven (ubuntu-latest, 25) hit the six-hour job limit (6h00m17s against its usual ~2h) and test-msi, test-msi-upgrade and the docker jobs were skipped. So neither blocker would have been red; you found them by reading, and the -sice ICE61 spelling and FileSearch under DirectorySearch are still unproven for the same reason. I have not diagnosed the six-hour job yet.

Refusal-message assertion never matches

Taken as suggested — the grep is now on location could not be determined. That is the clause stating the condition rather than the instruction, and the instruction is exactly the part I reworded last round while leaving the assertion behind. The relocation scenario's cannot move an existing installation is the same shape of phrase and I have left it, but it is now the only place a message edit can break a test, and the comment says so.

Relocation scenario cannot exercise the guard

Conceded completely, and it is worse than a mistake of fact: package.wxs has carried a comment since round five saying FindRelatedProducts does not run in maintenance mode — I wrote the scenario on top of a property I had already documented. Your reading of AllowSameVersionUpgrades is right too: it lifts the version equality restriction for a different ProductCode, and WiX regenerates the ProductCode per build, so two different builds would have worked where one file twice cannot.

Rewritten along your suggestion, with both branches of the guard covered: 5.1.2 goes into its native legacy default (its own ProductCode, so the new package is a genuine upgrade), then OPENDJ=C:\opendj-b is attempted twice — once with HKLM\SOFTWARE\OpenDJ\InstallDir set to the legacy directory, exercising the recorded-location branch, and once without it, exercising the branch that proves the old install by the setup.bat in the legacy default.

Legacy directory adopted during an upgrade

Confirmed, including the criticism of the comment: Installed refers to the product being installed, so NOT Installed is true throughout a major upgrade and never meant "fresh install". That is the same class of mistake as the second disjunct of RemoveLegacyServiceControl two rounds ago — a comment describing the intent while the condition expresses something wider.

Fixed as you suggested: SetOpendjFromLegacyDir now also requires OPENDJ_LEGACY_INSTALL when WIX_UPGRADE_DETECTED, so during an upgrade the directory has to hold a server, while a fresh install still adopts an empty one (which the auto-detect scenario relies on). The comment now states what the condition does rather than what I meant by it.

Your note about CI is the part that stings: the Remove-Item "C:\Program Files (x86)\OpenDJ" that hides this is one I added in the previous round, to satisfy the nit about the refusal scenario depending on the preceding step's cleanup. There is now a separate scenario for it — 5.1.2 at a custom directory, an empty legacy directory alongside, upgrade with no OPENDJ: it must refuse, the empty directory must stay empty and the original install must be intact.

Service started without waiting for the lock

Confirmed. Wait-ServerStopped exists precisely because #768 proved the exit code is not evidence, and I added a start-ds.bat/stop-ds.bat pair to test-msi without it — a flake waiting to be blamed on something else.

Extracted as you suggested rather than duplicated: the helper is now .github/scripts/wait-server-stopped.ps1, dot-sourced by both the zip test in build-maven and the new step in test-msi. test-msi had no checkout at all, so it gains one (first, since checkout cleans the workspace the artifact is unpacked into). The function also learned to leave absolute paths alone — the zip test passes a workspace-relative path, the MSI test an installed tree.

Launcher commit can abort the whole deploy

Confirmed, with one correction: continue-on-error is not on every neighbouring step — Build package local, Publish to the Maven Central Repository and most of the Upload artifacts steps do not carry it. That makes the point stronger rather than weaker: before this PR nothing ahead of mvn deploy could fail the job, and I put the one step that can at the very front, deliberately, so the published snapshot would carry fresh launchers.

Took the retry, and then some: three attempts with fetch + rebase between them, and if it still cannot land — or the rebase conflicts — a ::warning and exit 0 so the deploy proceeds. I avoided plain continue-on-error because a silent skip returns us to "the binaries are stale and nobody knows", which is the failure this whole mechanism exists to remove; a warning keeps it visible while costing nothing downstream. Nothing depends on the committed binaries being current within the run: the Build that produced them compiled its own.

Nits

• serviceState() comment overstates the fix — confirmed and trimmed. Verified the three callers (DirectoryServer:5284, ControlPanelInfo:420, Uninstaller:1352) all test == SERVICE_STATE_ENABLED with both other constants private, so the comment now says only what is true: the state is reported as unknown to --serviceState, and an unknown state still reads as "not enabled" everywhere else. Which means your nit carries a bigger finding than its label: the round-six major is only half closed. Making the distinction reach the uninstaller and the control panel means publishing SERVICE_STATE_ERROR and teaching those callers to tell "no service" from "could not tell" — a Java change, in a PR whose Java surface was deliberately emptied last round. I would rather file that separately than reopen it here; say the word if you disagree.
• Subdirectory relocation — confirmed, and I have treated it as more than a comment fix. The two-way substring test existed to tolerate a trailing backslash and, as you say, the tolerated "extension" is exactly what let C:\opendj\v2 through. It now compares for equality, with the legacy side built from [ProgramFilesFolder] by SetProperty rather than read from the search, so both sides are known to carry the backslash. That also retires the C:\dj vs C:\dj2 residual. The remaining assumption is that CostFinalize normalizes OPENDJ with a trailing backslash; the headline upgrade scenario is the canary for it, and a hand-edited registry value without one now refuses rather than proceeds — documented as the accepted residual.
• release-msi writes the shared Maven cache — confirmed and switched to actions/cache/restore@v5. Agreed it is inert today, and agreed it is the kind of thing that surfaces later as "Windows CI fails on missing transitive dependencies while everything is fine locally".

Still outstanding before merge, unchanged from last round plus one: the committed lib/*.exe refresh, a green run of the reworked MSI scenarios, and the two WiX 5 spellings above. And I owe the six-hour build-maven job a look — it is the reason none of this was exercised.

[maximthomas]

Thanks for the eighth pass' worth of fixes — the C side is down to three hunks and the guards are close. Two findings from my last comment I am withdrawing after re-verification: the OPENDJ_REG ~<> OPENDJ trailing-separator worry (Microsoft guarantees a Directory-table property ends in a separator after CostFinalize, and the guard is scheduled after it, so the round trip is a fixed point — strict equality is the right trade) and the head_repository job condition (documented required field, compared against github.repository, so forks are fine). The service.c comment point drops to a nit: enumeration failure already read as "not enabled" to all three Java callers on master, so nothing regressed.

What is left is two silent-failure defects, both in the upgrade guards, and both compounding: in the custom-directory state below, OPENDJ_SVC_IMAGEPATH ~>< OPENDJ fails, so the service guard is disarmed as well.

The service stop guard accepts every non-Running state (blocker)

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:197-211 samples the SCM once, the instant net.exe returns — no delay, no retry:

<CustomAction Id="StopServiceBeforeUpgrade" ... ExeCommand="... net.exe&quot; stop &quot;OpenDJ Server&quot;" Return="ignore"/>
<CustomAction Id="CheckServiceStopped"      ... ExeCommand="... -Command &quot;if ((Get-Service 'OpenDJ Server' -ErrorAction SilentlyContinue).Status -eq 'Running') { exit 1 }; exit 0&quot;" Return="check"/>

StopPending and StartPending are distinct enum values, so -eq 'Running' is $false and the CA exits 0. Two reachable routes:

• the service is already pending when the CA fires — ControlService fails immediately with ERROR_SERVICE_CANNOT_ACCEPT_CTRL, Return="ignore" eats it, the sample passes. chap-upgrade.adoc:260 tells administrators to net stop first, so this is the documented flow.
• StartPending — service.c:648 sets a 300000 ms start wait and service.c:667-672 reports pending with hint wait + 30000, so java.exe can hold lib\*.jar for ~5.5 minutes while the guard waves the upgrade through.

Consequence is exactly what the comment at package.wxs:170-177 describes, except worse: the jars are unversioned (opendj-archive-component.xml:26), so old and new share identical paths and PendingFileRenameOperations deletes the freshly installed jars at the next reboot. Install exits 0; the server breaks later.

Correction to my previous comment: my "net stop gives up at ~10 s" mechanism was wrong. service.c:854-876 increments dwCheckPoint every ~2 s under the 10 s hint, which is the documented "still making progress" signal, and the wrapper always resolves to STOPPED or RUNNING within ~61 s (service.c:1491, 1520). The defect survives because the check is an instantaneous sample, not because the state is durable.

The repo already owns the right probe — .github/scripts/wait-server-stopped.ps1:26-38 takes a byte-range lock on locks\server.lock, and its own comment cites #768 as "winlauncher.exe reported success without having stopped the server" — but the MSI never uses it. service.c:2256-2271 budgets 90 s for the same wait; the MSI budgets zero. Minimum fix, null-safe because a poll makes the missing-service case reachable:

$deadline = (Get-Date).AddSeconds(90)
do {
  $s = Get-Service 'OpenDJ Server' -ErrorAction SilentlyContinue
  if (-not $s -or $s.Status -eq 'Stopped') { exit 0 }
  Start-Sleep -Seconds 2
} while ((Get-Date) -lt $deadline)
exit 1

A custom-directory upgrade still destroys an unrelated installation (blocker)

OPENDJ_LEGACY_INSTALL (package.wxs:75-79) proves a setup.bat exists in a default directory — not that it belongs to the product being upgraded. Start state: 5.1.x at C:\opendj-custom, plus any foreign OpenDJ tree (zip, manual copy) in a Program Files default. msiexec /i new.msi /quiet, no OPENDJ:

• SetOpendjFromLegacyDir (package.wxs:113-115) fires — the foreign tree satisfies the new gate — and adopts C:\Program Files (x86)\OpenDJ\.
• RequireDirOnCustomUpgrade (:204-205): OPENDJ ~= OPENDJ_DEFAULT is false. Does not fire.
• RefuseRelocatingUpgrade (:206-207): OPENDJ_REG is empty and OPENDJ_LEGACY_DEFAULT ~<> OPENDJ is false — they are identical. Does not fire.

The x64 mirror slips the other guard: with a foreign tree in C:\Program Files\OpenDJ, NOT OPENDJ_X64DEFAULT at :205 is false, and the legacy branch never arms because OPENDJ_LEGACY_INSTALL is unset.

Both end the same way, with exit code 0: RemoveExistingProducts runs 5.1.x's own uninstall and guts C:\opendj-custom, while InstallFiles overwrites the foreign tree — whose old jars are not removed, and _script-util.bat:62 globs lib\*.jar, so that tree ends up with a duplicate-version classpath. HKLM\SOFTWARE\OpenDJ\InstallDir and ARP then point at the foreign tree, so a later msiexec /x deletes out of it.

Every premise checks out: git show 5.1.2:...package.wxs carries the same UpgradeCode="A3E82AC0-88E6-4DEE-9D8C-5AE3B7853274", 5.1.2 accepts a custom OPENDJ (your own build.yml:939 does it), and it writes no registry value. The refusal scenario at build.yml:926-949 passes only because it deletes both default directories first (:936-937) — the guard's correctness is conditional on the decoy being absent.

Two service side-effects fall out of the same hole: if the foreign instance owns a running OpenDJ Server, StopServiceBeforeUpgrade stops an unrelated production server; if the real 5.1.x owns it, OPENDJ_SVC_IMAGEPATH ~>< OPENDJ fails and CheckServiceStopped never runs at all.

The x64 half looks closable with one term — NOT OPENDJ_REG in place of NOT OPENDJ_X64DEFAULT at :205 keeps the legitimate 5.2.0→5.2.1 default-directory upgrade passing while making this case refuse (checked against every scenario in build.yml:785-1042, but please verify). The legacy branch is harder: a setup.bat existence test cannot distinguish a server from the server, so it is either "refuse when WIX_UPGRADE_DETECTED AND NOT OPENDJ_REG and no explicit OPENDJ was given", or state the residual. What it cannot stay is chap-upgrade.adoc:264, which promises unconditionally that the package "refuses to install into a directory other than the detected one".

The guard rework has never run green in CI (major)

test-msi and test-msi-upgrade last completed at 252d5ba9b (2026-08-10). Every Build run on the five commits since — 24b1b7ec4, b2de06075, 92b30bd20, 1cf5a88546, 10c99b94cf — was cancelled by the next push. RefuseRelocatingUpgrade did not exist at 252d5ba9b, so it has never executed in a completed run in any form; the same goes for the new refusal, relocation and empty-legacy scenarios and for wait-server-stopped.ps1. The head's run is ~60 min in with the MSI jobs still gated behind the ubuntu matrix — the same gate that swallowed them last round.

Worth pinning the MSI jobs to something that cannot be starved by the ubuntu matrix (or diagnosing the six-hour ubuntu-latest, 25 job) before this merges. Right now green CI is not available as evidence for any of the guard work.

An unguarded git fetch can abort the whole deploy (minor)

.github/workflows/deploy.yml:131 is a bare command in a for body. The step runs under bash --noprofile --norc -eo pipefail plus an explicit set -e (:101) and has no continue-on-error, and the retry loop wraps only the rebase (:132) and the push (:137) — so a transient fetch failure never reaches iteration 2 and kills the step. It is step 5 of ~19: mvn deploy, all eight artifact uploads, the MSI attach, the wiki push and the doc-site push are skipped. That is the exact loss 1cf5a88546's commit message says the rework prevents.

if ! git fetch --quiet origin "$BRANCH"; then
  echo "::warning::could not fetch $BRANCH; skipping the launcher refresh"
  exit 0
fi

Rare in practice — /Brepro means :109 only lets the block run when the launcher bytes actually changed. My earlier claim about the cp at :106 was wrong; the ls check at :102-105 guards it.

Nits

• wait-server-stopped.ps1:41 catches everything: the blanket catch treats UnauthorizedAccessException as "still locked", so a permissions problem under Program Files spins the full 60 s and then reports a lock that was never held. Catch IOException and let the rest surface.
• build.yml:688 full checkout for one file: actions/checkout@v6 clones the whole tree on a Windows runner to read a 38-line script, then the artifact is unpacked over it. sparse-checkout: .github/scripts removes both costs.
• build.yml:976 asserts nothing about the guard: the empty-legacy scenario checks only $p.ExitCode -ne 0, so it passes on any failure. Both neighbours grep the message; this one should grep location could not be determined too.
• build.yml:999 inherits the previous step's teardown: the relocation scenario stopped clearing C:\Program Files (x86)\OpenDJ itself, so it depends on :983 — the coupling you removed from the refusal scenario one round ago.
• deploy.yml:138 claims a push that may not have happened: an already-landed refresh makes the rebase drop the commit and the push a no-op, and the log still says "Refreshed launchers pushed to master." Compare HEAD against FETCH_HEAD first.
• service.c:2444 comment: pre-existing, not introduced here — master's enumeration failure returned 1 (DISABLED) and the same three callers read that as not-enabled, so 2 changes nothing for them. Worth one clause noting the uninstaller still skips --disableService in that state.

[vharseko]

Thanks for the eighth pass, and for re-verifying the two withdrawals rather than leaving them standing — the CostFinalize reasoning about the trailing separator is the one I would not have been able to defend on my own. Both blockers are confirmed and fixed in 90c4271; notes follow your headings, including the two places where I did not take the suggestion as written.

The service stop guard accepts every non-Running state

Confirmed in full, including the mechanism: net.exe returns the instant the SCM rejects a control it cannot accept, Return="ignore" eats it, and neither pending state is Running. Your correction about net stop is also right — I had the 10 s hint in mind and missed that service.c:854-876 re-arms it with a checkpoint every 2 s, so the state is short-lived rather than durable, and an instantaneous sample is what makes the defect reachable, not the length of the window.

Taken as a poll, with one deliberate difference from your patch: only StopPending is waited out. After net.exe has returned, Running means the stop was rejected or denied and StartPending means a JVM is on its way up — neither resolves into Stopped on its own, so waiting them out only delays a refusal that is already decided, and it would have turned the most common failure of all (the unelevated double-click, which refuses instantly today) into a 90-second stall before the same error. So: gone or Stopped passes, StopPending gets 90 s, everything else refuses immediately. Null-safe as you asked, which the poll makes necessary.

I did not route it through wait-server-stopped.ps1: the check runs before InstallFiles, and the script is not part of the payload, so the CA has no file to dot-source. The residual that leaves is the gap between "the SCM says Stopped" and "the JVM let go of the jars" — narrower than it looks, because the wrapper only reports SERVICE_STOPPED after isServerRunning has gone false, but it is not the byte-range lock and I am not claiming it is.

test-msi-upgrade now has a scenario for the state itself: 5.1.2 at the legacy default with the service registered, bat\start-ds.bat replaced by a script that blocks, sc start — the wrapper then reports SERVICE_START_PENDING for as long as the script runs (service.c:648), so the state is produced deterministically rather than raced for. The step asserts the service really is StartPending before running msiexec, that the upgrade fails, that the refusal came from CheckServiceStopped, and that the instance survived.

A custom-directory upgrade still destroys an unrelated installation

Confirmed, both halves, and the CI observation stings the most: the Remove-Item of both default directories in the refusal scenario is mine, added two rounds ago to satisfy a nit about inherited teardown, and it is exactly what hides this.

I took your x64 term — NOT OPENDJ_REG in place of NOT OPENDJ_X64DEFAULT — with one addition, because on its own it dead-ends: RequireDirOnCustomUpgrade compares the resolved OPENDJ, not the presence of the property, so OPENDJ="C:\Program Files\OpenDJ" refuses just as the silent run does. A 5.1.x that genuinely lives in the x64 default (5.1.2 accepts a custom OPENDJ — build.yml:939 is the proof) would then have no way to upgrade at all, in any UI mode. The condition is now ... AND NOT OPENDJ_REG AND NOT OpendjDirGiven, where OpendjDirGiven is set from OPENDJ before AppSearch — the one point where the property can only hold what the command line put there.

The part that took the longest is why that capture is Sequence="first" rather than execute-only. In a full-UI session the UI sequence runs first in the same process, so by the time the execute sequence starts, CostFinalize and InstallDirDlg have populated OPENDJ no matter what the administrator did — an execute-only capture would have set the flag on every GUI install and quietly disarmed the guard there. first gives the msidbCustomActionTypeFirstSequence bit: the UI sequence captures the command line, and the execute sequence only captures when no UI sequence ran in the same process. A GUI session that names nothing keeps the refusal, which is the behaviour the browse dialog was the answer to. The property is private (mixed case) so it cannot be handed in on the command line as a switch.

The legacy branch: option 2, the residual is stated. Your first alternative cannot be taken literally — "refuse when WIX_UPGRADE_DETECTED AND NOT OPENDJ_REG and no explicit OPENDJ" is precisely the documented headline path (5.1.2 at Program Files (x86)\OpenDJ, upgraded with nothing given), which test-msi-upgrade asserts must succeed. One correction on the doc, since it changes what needs rewording: chap-upgrade.adoc:264 promises refusal to install into a directory other than the detected one, and in your scenario the package does install into the detected one — what is unstated, and wrong, is the assumption that "detected" means "the one being upgraded". The sentence is reworded anyway, and a NOTE now says plainly to pass OPENDJ when another OpenDJ tree sits in the legacy default while the server being upgraded lives elsewhere.

Two scenarios cover the new behaviour: a decoy tree in the x64 default with the real 5.1.x at C:\opendj-real (must refuse with the guidance message, must not touch either tree, then must complete when the directory is named), and a 5.1.x installed in C:\Program Files\OpenDJ (silent upgrade must refuse, named upgrade must land in place). The second one is there to keep the trade-off honest and visible: that install used to upgrade silently and now refuses. I think fail-safe is the right side to err on when the alternative is gutting an installation at exit code 0, but it is a behaviour change and the scenario is where it is written down.

The guard rework has never run green in CI

Confirmed, and worse than stated: StopServiceBeforeUpgrade and CheckServiceStopped also postdate 252d5ba9b (they arrived in b2de060), so the entire guard set — not only RefuseRelocatingUpgrade — has never executed in a finished run.

I do not think the six-hour job is the cause, though. In the last completed run the ubuntu legs took 116-135 min, which is normal for them, while test-msi took 1.6 min and test-msi-upgrade 2.3 min. needs: build-maven is therefore a two-hour queue in front of a two-minute job, and any push inside that window cancels the run before the MSI jobs start — no six-hour outlier required. Both jobs now wait for the windows-latest-11 artifact instead of the matrix: it appears after ~15 min, and the wait gives up early if the Windows leg finishes without publishing it. The six-hour instance is still unexplained, but it is no longer on the path to MSI evidence.

An unguarded git fetch can abort the whole deploy

Confirmed and taken as written — warning plus exit 0, in the loop, so a transient fetch failure costs the refresh and nothing else. Your correction about the cp is right; the ls guard above it is what makes that safe.

Nits

• wait-server-stopped.ps1 catches everything — taken; it now catches IOException only, which is what both a held byte-range lock and a sharing violation raise, and UnauthorizedAccessException surfaces with its own message. One correction: the blanket catch is at :35 and the file is 38 lines long, so :41 does not exist — the finding is unaffected.
• build.yml:688 full checkout — taken, sparse-checkout: .github/scripts. It also brings the job in line with the two docker jobs, which already do this.
• build.yml:976 asserts nothing about the guard — taken; it greps location could not be determined like its neighbours.
• build.yml:999 inherits the previous step's teardown — taken; the relocation scenario now clears both default directories, the registry value and C:\opendj-b itself.
• deploy.yml:138 claims a push that may not have happened — taken; HEAD is compared with FETCH_HEAD and the message says which of the two happened.
• service.c:2444 comment — taken, with the clause you asked for: the uninstaller still skips --disableService in that state, and the comment now says so and marks it pre-existing.

None of this has been exercised yet — the MSI cannot be built off Windows, so a CI run is the only evidence available, and it is the first one that can reach the MSI jobs without two hours of matrix in front of it. That run, on this head, is the one to judge the guards by.

[maximthomas]

Reviewed ff54e5f on top of the 8th round — the two new commits (20a77b6 Restart Manager, ff54e5f 32-bit MSI / default-directory tests) plus a full pass over the rest of the diff.

The guard sequencing, the RemoveExistingProducts placement, the Formatted-field brace handling, the service.c exit-code mapping and the Maven/deploy.yml wiring all check out. Four things below, one of which blocks the documented upgrade path.

Relocation guard refuses the workaround its own comment prescribes (Blocker)

package.wxs:174-181 documents the residual and its remedy:

The install guide carries the workaround instead: pass OPENDJ explicitly when another OpenDJ tree sits in the legacy default directory.

package.wxs:269 then refuses exactly that:

Condition="WIX_UPGRADE_DETECTED AND ((OPENDJ_REG AND (OPENDJ_REG ~<> OPENDJ))
        OR (NOT OPENDJ_REG AND OPENDJ_LEGACY_INSTALL AND (OPENDJ_LEGACY_DEFAULT ~<> OPENDJ)))"

OPENDJ_LEGACY_INSTALL only proves some server sits in C:\Program Files (x86)\OpenDJ — not that it is the product being upgraded. So with a 5.1.x server at C:\opendj and any unrelated tree (zip install, old instance) in the legacy default:

msiexec /i opendj.msi OPENDJ="C:\opendj"

OPENDJ_REG unset, OPENDJ_LEGACY_INSTALL set by the stray tree, C:\Program Files (x86)\OpenDJ\ ~<> C:\opendj\ → refused, "cannot move an existing installation". The message names neither the stray tree nor a way through.

Guard 1 (:267) already has the escape hatch — AND NOT OpendjDirGiven. Guard 2 has none. Either give branch (b) the same bypass, or key it on the resolved directory instead of OPENDJ_LEGACY_DEFAULT when OPENDJ was named.

The CI scenario meant to cover this ("a stray OpenDJ tree in the default directory must not be adopted") puts its decoy in the x64 default and deletes the legacy directory first, so branch (b) is never exercised with a decoy.

Non-service servers lose all in-use detection (Major)

package.wxs:258 turns Restart Manager off:

<Property Id="MSIRESTARTMANAGERCONTROL" Value="Disable"/>

The reasoning for the service case is right. But the stop-and-verify pair replacing it is gated on a registered service:

Condition="WIX_UPGRADE_DETECTED AND (OPENDJ_SVC_IMAGEPATH ~>< OPENDJ)"

…and this PR promotes the non-service mode to the primary tested path — test-msi does setup --doNotStart then start-ds.bat, no service. For that server OPENDJ_SVC_IMAGEPATH is unset, so no guard runs, and with RM off nothing detects the JVM holding lib\*.jar (headless, no top-level window, so legacy FilesInUse misses it too). The upgrade proceeds silently into reboot-deferred file replacement.

Worth refusing on locks\server.lock being held, or at minimum documenting it in chap-upgrade.adoc next to the service instructions.

45-minute artifact wait is shorter than the queue it waits on (Major)

.github/workflows/build.yml:705 and :836:

$deadline = (Get-Date).AddMinutes(45)
...
throw 'windows-latest-11 was not published within 45 minutes'

45 min covers the Windows leg's runtime, not its queue time. In run 31578978374 that leg started at +39 min and finished at +52 min — both MSI jobs would have failed the run with nothing actually wrong.

It also feeds itself: each waiter holds a windows-latest runner from t=0 for up to 45 min, competing for the capacity the leg they wait on needs. Either raise the budget substantially, or key the wait on the leg's status (queued/in_progress) rather than a fixed timer.

Custom-upgrade refusal advertises a GUI route that cannot work (Minor)

package.wxs:199 says "Run the installer again and select the existing installation directory". That relies on OpendjDirGiven, set at package.wxs:118:

<SetProperty Id="OpendjDirGiven" Before="AppSearch" Sequence="first"
             Value="1" Condition="OPENDJ"/>

Before="AppSearch" means OPENDJ can only hold a command-line value there; InstallDirDlg runs far later and never sets it. So for a server in C:\Program Files\OpenDJ whose registry value is gone (key deleted, machine re-imaged), re-running the wizard and selecting that same directory hits the identical refusal — OPENDJ still equals OPENDJ_DEFAULT, OpendjDirGiven still unset. Only the command line works.

Same promise in the comment at :111-113 ("the browse dialog as its way out"). Either set OpendjDirGiven from InstallDirDlg's Next, or say the directory must be passed on the command line in this case.

Nits

• openScm null check (pre-existing): service.c tests scm == NULL instead of *scm == NULL, so it never reports failure. Untouched by this PR, but the new SERVICE_LIST_UNAVAILABLE branch is reachable through it.
• Duplicated wait block: build.yml:700-722 and :831-848 are the same ~20 lines of pwsh. A composite action would keep the two deadlines from drifting apart.

[vharseko]

Thanks for the ninth pass. All four are confirmed and fixed in 99a0116, and the blocker is worse than you wrote it: I have not taken either of your suggested fixes for it as written, nor the first one for the GUI route — notes follow your headings, with the reasoning in each case.

Relocation guard refuses the workaround its own comment prescribes

Confirmed, and the contradiction is not confined to the comment: chap-upgrade.adoc:274 is a NOTE telling administrators to do exactly what package.wxs:269 then refuses. Worse, that installation had no upgrade path at all. Left alone, SetOpendjFromLegacyDir adopts the decoy and neither guard fires, so the silent upgrade lands on the stray tree while RemoveExistingProducts empties the real one — the failure the NOTE describes — and the only escape it offers is the command that refuses. The undocumented way through was to delete the stray tree first, which nothing said.

Your CI observation is right too: the decoy sits in the x64 default and the legacy directory is deleted at build.yml:1097, so branch (b) never met one.

Not the first alternative, though: giving branch (b) AND NOT OpendjDirGiven disarms it completely rather than narrowing it. Branch (b) can only fire when OPENDJ differs from the legacy default, and when nothing is named SetOpendjFromLegacyDir has already made them equal — so every case it fires on is a command-line case, and the bypass would take the whole branch with it, including the relocation build.yml:1077 asserts must be refused.

So the second one, keyed on the named directory rather than on the fact that one was named: a new search asks whether [OPENDJ] holds a setup.bat, and branch (b) gains AND NOT (OpendjDirGiven AND OpendjGivenInstall). A named directory that holds a server is the installation being upgraded; a named directory that is empty or new is a relocation and still refused, so OPENDJ=C:\opendj-b behaves exactly as before. The registry branch is untouched — a location this package recorded is authoritative, and moving away from it is what uninstalling first is for.

Two details worth stating. The search reads [OPENDJ] at AppSearch, the same window OpendjDirGiven is read in, so "named" means the command line and not InstallDirDlg; when OPENDJ is empty the path does not expand to a full path, and with no parent and depth zero the installer looks for setup.bat in the root of each fixed drive instead — a handful of stat calls that cannot change a decision, since every use of the property is paired with OpendjDirGiven. And the property is private, not Secure: a public one could be handed in as OpendjGivenInstall=1 next to a relocating OPENDJ to switch the guard off, because AppSearch only overwrites what it finds.

Residual, stated in the comment: naming a directory that holds some OpenDJ tree while the product being upgraded lives elsewhere now proceeds and strands that installation. Directory evidence cannot tell those apart at all, and between refusing the documented workaround and trusting an explicit instruction I take the instruction.

You were also right that the message named neither the stray tree nor a way through — it now describes both.

New scenario covers the branch with a decoy where it belongs: 5.1.x at C:\opendj-mine, a decoy tree in the legacy default, OPENDJ=C:\opendj-b refused with the relocation message, then OPENDJ=C:\opendj-mine upgrading the named installation with the decoy untouched.

Non-service servers lose all in-use detection

Confirmed, and the comment at :255-257 was wrong in a way that matters to the fix: Disable stops the installer using Restart Manager at all, including for detection — "Windows Installer uses the FilesInUse Dialog" — so the fallback is the legacy in-use scan, not the WiX3-era behaviour, which had RM detection. The value that means what I wrote is DisableShutdown: "The installer still uses the Restart Manager to detect files in use by applications", with no shutdown and therefore no restart. Changed, and the comment now says which of the two it is doing and why.

I did not simply put RM back. It has no clean way to stop a headless JVM — no window to close, not RM-aware — so it terminates it, and an unclean stop of a directory server is worse than the refusal.

The refusal is CheckServerNotRunning, your locks\server.lock suggestion: the same Lock(0, 1) probe wait-server-stopped.ps1 uses, inline in the action because this runs before InstallFiles and the script is not payload. Gated on nothing but WIX_UPGRADE_DETECTED — the lock answers "is a server running out of this tree", which is the actual question, and the service pair has already dealt with the SCM's half. 60 s of grace, matching the script's budget: CheckServiceStopped hands over the moment the SCM says Stopped, which is not the moment the JVM releases the lock. Anything but a plain IOException — a read-only handle under Program Files, the file vanishing between the test and the open — means the question could not be asked and lets the upgrade through, the same fail-open the swallowed net stop has and for the same impersonation reason.

Documented next to the service instructions in chap-upgrade.adoc, as you asked. One visible consequence of restoring detection: in a full-UI upgrade over a running server the FilesInUse dialog can now appear before the check refuses, since RM runs at InstallValidate and the check sits before RemoveExistingProducts. Silent installs, where no dialog can be shown, are exactly why the check exists.

Scenario added: 5.1.2 in the legacy default, setup --doNotStart, start-ds.bat, no service registered — the auto-detected upgrade must fail naming CheckServerNotRunning, the instance must still answer ldapsearch, and the same upgrade must then succeed after stop-ds.bat.

45-minute artifact wait is shorter than the queue it waits on

Confirmed, and I checked the run you cited: it started 08:34:59, build-maven (windows-latest, 11) started 09:13:15 and finished 09:26:44, so the artifact appeared at +52 minutes against a deadline of +45. A healthy build, two red jobs.

Rather than raise the budget or key on the leg's status inside a Windows job, the wait moved out: a small wait-msi-artifact job on ubuntu-latest that both MSI jobs needs. That removes the self-competition you described — no Windows runner is held while waiting — and makes the budget cheap enough to be generous (120 min in the loop, timeout-minutes: 130), while the early exit on the leg's conclusion stays the usual way out. It also retires the duplicated block, so that nit goes with it.

actions: read is dropped from both MSI jobs along with the polling; build-docker already downloads a same-run artifact with contents: read alone.

Custom-upgrade refusal advertises a GUI route that cannot work

Confirmed. The same promise is in chap-upgrade.adoc:264 as well — "select that directory in the wizard… name the directory and the same upgrade proceeds" — which you did not name and which is the copy administrators actually read.

Not the first fix: publishing OpendjDirGiven from InstallDirDlg's Next restores "Next, Next, Install" as a data-loss path, which is why the guard moved to the execute sequence in round seven. I did consider publishing it from BrowseDlg's OK, which at least requires an administrator to open Change and confirm a directory — but "opened Change, pressed OK without changing anything" would still disarm the guard, and that is too close to the accident being guarded against for the convenience it buys.

So option two: the message, the comment at :111-113 and the install guide now say plainly that the wizard cannot confirm the default directory and that this one case needs the command line. The wizard route stays true where it is true — browsing to a different directory makes the comparison fail and the install proceed.

Nits

• openScm null check: fixed, *scm, with a comment on what the old test hid. One consequence worth flagging: service.c now differs from the committed lib/*.exe in code rather than in a comment, so those binaries are one fix behind their sources until Package/Deploy re-commits them. The Windows leg compiles the launchers itself before packaging, so the MSI and the Windows-built zip already carry it, and the behaviour change is confined to a debug message — the NULL handle always surfaced as an error from EnumServicesStatus. I have updated the PR text accordingly.
• Duplicated wait block: gone with the gate job above.

Verification, so it is not overstated: XML and YAML parse, and the strings the CI assertions grep for still match on both sides. Nothing is built — the MSI needs Windows — so wix msi validate, the new search, the new action and both new scenarios are unexercised. The gate job is the first change that lets the MSI jobs be reached without two hours of matrix in front of them; the run on this head is the one to judge all of it by.

[vharseko]

Correction to the first heading above: the property is public, not private, and the paragraph arguing that privacy is what closes the command-line bypass was wrong twice over — WIX0012 says a property AppSearch fills cannot be private, so it did not even compile. The Build on that head failed on exactly that line, with nothing else wrong in the leg.

Fixed in 777b773 by making the test value-based instead of truth-based. The search stores the full path of the setup.bat it finds, so the guard now requires that path to sit inside the directory that was named — OPENDJ_GIVEN_INSTALL ~>< OPENDJ — rather than requiring the property to be merely set. OPENDJ_GIVEN_INSTALL=1 on the command line therefore says nothing. A value spelled out as a path inside the named directory would still pass; that is deliberate construction rather than a slip, and the comment says so. Everything else in the reply stands, and no other behaviour changed.

One thing that run did establish, on the third heading: the gate job caught the Windows leg finishing without the artifact at +8 minutes and skipped both MSI jobs, instead of a 45-minute timer expiring on two held Windows runners. The rest of that heading — and all of the first, second and fourth — is still waiting on the run now in flight.

[vharseko]

Second correction, to the "Non-service servers lose all in-use detection" heading: DisableShutdown is not the middle ground I claimed, and CI said so before you had to.

The reading was that it keeps RM's detection — "The installer still uses the Restart Manager to detect files in use by applications" — while disabling both the shutting down and the restarting. Only the first half is true. In run 31615584455 the headline upgrade scenario finished with the service StartPending after an otherwise successful install: the guards stopped it, and the installer restarted it at the end anyway, because what the RM session listed at InstallValidate gets restarted whether RM stopped it or CheckServiceStopped did. That is precisely the failure the round-eight commit exists to prevent — the upgraded server coming up before upgrade.bat has migrated the instance data — so the setting is back to Disable in e6f0a44, with the observation recorded in the comment as a measurement rather than an argument.

Which means your finding is closed by CheckServerNotRunning alone, not by restoring detection: the byte-range lock on locks\server.lock answers for the start-ds.bat mode and the service mode alike, and answers under /quiet, where a FilesInUse dialog could not be shown even if RM had found the JVM. What stays uncovered with RM off is some other process holding payload files — the WiX3-era behaviour, now stated as the residual it is rather than implied to be something better.

Where the evidence stands after two runs on this branch. Green: the MSI builds, wix msi validate -sice ICE61 passes on the current authoring including the new search and the new action, the gate job releases both MSI jobs about 17 minutes into the run, test-msi passes end to end, and the gate's early exit was exercised for real when the Windows leg failed. Still unproven: every test-msi-upgrade scenario, which was skipped after the first one failed on the restart above — including the two new ones for the relocation exception and the running non-service server. The run on e6f0a44 is the one that reaches them.

[maximthomas]

Review: requesting changes — one blocker, and the new CI scenarios can't catch it

Reviewed at e6f0a44, continuing from the MSIRESTARTMANAGERCONTROL correction. I focused on
the paths added in the last two rounds that have never actually executed. Most of the round-nine
and round-ten work checks out — details at the bottom — but two of the new guards don't do what
they're written to do, and the tests added alongside them can't tell.

---

1. Blocker — CheckServerNotRunning can never refuse

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:362

catch { if ($_.Exception.GetType().FullName -ne 'System.IO.IOException') { exit 0 }; Start-Sleep -Seconds 2 }

When a .NET method or constructor throws inside PowerShell, an untyped catch doesn't unwrap
it — $_.Exception is System.Management.Automation.MethodInvocationException, and the real
exception is in .InnerException. Both throw sites here are invocations
(New-Object System.IO.FileStream(...) and $fs.Lock(0, 1)), so the comparison is always true,
the action always takes exit 0, and the exit 1 is unreachable.

The lock probe itself is right: LockFileManager.java:231,249 takes the lock with
RandomAccessFile(lockFile, "rw") + tryLock(0, Long.MAX_VALUE, false), a mandatory LockFileEx
range covering byte 0, and RandomAccessFile opens with FILE_SHARE_READ|FILE_SHARE_WRITE. So
the open succeeds by design and Lock(0, 1) is what raises IOException — exactly the
wrapping-sensitive call.

Empirical confirmation (PowerShell 7.5.0 and 6.2.4, identical output)

### VARIANT A: exact shipped code, untyped catch + FullName equality ###
  caught. FullName = System.Management.Automation.MethodInvocationException
  RESULT: guard tripped -> exit 0 (sleep branch NEVER reached)
### VARIANT B: typed catch [System.IO.IOException] ###
  RESULT: TYPED CATCH MATCHED -> Start-Sleep branch reached.

5.1 isn't runnable off-Windows, but the mechanism is in engine source 5.1 shares:
Compiler.cs::VisitTryStatement special-cases a lone catch-all and emits
$_ = new ErrorRecord(rte.ErrorRecord, rte) with no unwrapping, while typed catches go through
ExceptionHandlingOps.FindMatchingHandler, which tries InnerException first. Same code at
v6.0.0-alpha.9, the direct fork of the 5.1 codebase. The 5.1-moniker docs show the
MethodInvocationException message template for [System.IO.File]::ReadAllText.

This is a blocker rather than a nit because the comment at lines 322-328 is exactly right about
the exposure: with MSIRESTARTMANAGERCONTROL=Disable, no service key means
OPENDJ_SVC_IMAGEPATH is empty, ~>< is false, and StopServiceBeforeUpgrade /
CheckServiceStopped are skipped outright. CheckServerNotRunning is the only guard left for a
start-ds.bat server — and it always exits 0. The outcome is the one described at lines 266-269:
locked jars degrade to delete-on-reboot entries and /norestart returns 3010.

Fix — .github/scripts/wait-server-stopped.ps1:39 already has it:

catch [System.IO.IOException] { Start-Sleep -Seconds 2 }
catch { exit 0 }

One trap worth noting: because the open uses New-Object, $_.Exception stays
MethodInvocationException even inside a matching typed catch, so a FullName comparison in
that block would still be wrong. (An exact-FullName test is fragile regardless —
FileNotFoundException is an IOException subclass.)

---

2. The new CI scenarios pass either way

.github/workflows/build.yml:1313-1314, and the same pattern at :1235

if ($p.ExitCode -eq 0) { ...; throw "the upgrade must refuse while a server is running out of the tree" }
if (-not (Select-String -Path upgrade-running.log -Pattern "CheckServerNotRunning" -Quiet)) { ... }

• Line 1313 asserts only ExitCode -ne 0. A fail-open upgrade hits the locked jars and returns
  3010 under /norestart, which satisfies it.
• Line 1314 greps for an action name. MSI writes Action start ...: CheckServerNotRunning.
  when it runs and Skipping action: CheckServerNotRunning (condition is false) when it doesn't.
  The string is there either way.

So on the current head this step should print
Upgrade refused while a non-service server was running (exit 3010) and go green while the
upgrade it was meant to block has just renamed a live server's jars.

I'd argue this matters more than the blocker itself: while it holds, a green run isn't evidence
that any of these guards work, and careful reading is the only thing standing in.

Fix — assert 1722, grep for Return value 3 on the action line, and assert
PendingFileRenameOperations is absent afterwards. Both :1313 and :1235.

---

3. The relocation exception is unreachable in a full-UI session

package.wxs:368 — the exception is NOT (OpendjDirGiven AND (OPENDJ_GIVEN_INSTALL ~>< OPENDJ)),
and OpendjDirGiven has one writer, package.wxs:153:

<SetProperty Id="OpendjDirGiven" Before="AppSearch" Sequence="first" Value="1" Condition="OPENDJ"/>

Sequence="first" compiles to msidbCustomActionTypeFirstSequence, and per Custom Action
Execution Scheduling Options:

Always skips action in execute sequence if UI sequence has run. ... The action is not
required to be present or run in the UI sequence to be skipped in the execute sequence.

The skip keys on the UI sequence having run, not on the condition having been true. So in a
full-UI session OpendjDirGiven is unset when the guard evaluates — including when OPENDJ=
is passed on the command line, which is what the refusal message and chap-upgrade.adoc:266
both point users to. /qn and /qb never process the UI sequence, which is why every CI
scenario passes. (This half deserves one empirical check: a wizard run with /l*v, grepping
for OpendjDirGiven.)

The wizard also ends up inverted in the decoy topology — 5.1.x at D:\opendj, empty registry,
stray tree in C:\Program Files (x86)\OpenDJ:

wizard action	result
browse to the correct D:\opendj	OpendjDirGiven unset → branch (b) TRUE → refused
accept the pre-populated default	OPENDJ_LEGACY_DEFAULT ~<> OPENDJ false → no guard fires, installs into the decoy while RemoveExistingProducts empties D:\opendj

Fix — publish the "directory was named" signal so it survives into the execute sequence
(public Secure="yes", or re-derive it there from OPENDJ vs OPENDJ_LEGACY/OPENDJ_DEFAULT
plus OPENDJ_GIVEN_INSTALL ~>< OPENDJ). Worth noting all 75 msiexec calls in build.yml use
/qn, so no wizard behaviour is regression-tested at all.

---

Checked and clean

• service.c — SERVICE_LIST_UNAVAILABLE appended last, no renumbering, handled at all six
  getServiceName call sites. ControlPanelInfo.java:420, Uninstaller.java:1352 and
  DirectoryServer.java:5284 test only == SERVICE_STATE_ENABLED, so SERVICE_STATE_ERROR
  reads as "not enabled" just as SERVICE_STATE_DISABLED did. Only nit: the getServiceName doc
  comment at service.c:977-978 still says "SERVICE_RETURN_ERROR otherwise".
• openScm — if (*scm == NULL) is a real fix; the old test compared the address of the
  caller's variable, which is never NULL.
• Guard balance and precedence, ~= / ~<> / ~>< semantics, trailing-backslash construction on
  both sides of every comparison, the Formatted-field brace rules (no {} group in the command
  holds a [property]), and FindRelatedProducts → AppSearch ordering with the execute-sequence
  re-run — all correct.

Suggested order

1. Fix the CI assertions first, before touching a guard, and re-run against this head — the
   suite should go red on №1. If it doesn't, the assertions still aren't real.
2. Then the typed catch, and watch the now-honest suite go green.
3. Consider moving the PowerShell out of the XML attribute so it can be unit-tested — a Binary
   table entry, or a .ps1 as source of truth inlined at build time. I reproduced №1 in a
   container in about five minutes; that loop belongs in CI. It's also the direct cause here —
   wait-server-stopped.ps1 has the typed catch, and the inline copy lost it on the way into the
   attribute.
4. One full-UI scenario for №3.
5. Possibly split: service.c and openScm are independent and verified clean, and could go in
   now while the MSI guard work continues separately.

[maximthomas]

see the comment above #701 (comment)