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 modules stay in every reactor, and the distribution-windows-msi profile that requires Windows plus an installed wix tool sits inside opendj-msi-standard, around its <build>: inactive, the module is a pom that builds nothing, so a plain mvn install on a contributor's Windows machine no longer fails mid-reactor with "Cannot run program wix". Gating the module list instead - as an earlier revision of this PR did - would have frozen both MSI poms at their parent version forever, because maven-release-plugin only rewrites ${reactorProjects} and release.yml prepares the release on ubuntu-latest with no -P; Maven does not fail on that skew, it resolves the parent from the repository, so the module would have kept building as the old version - unpacking the previous line's published snapshot instead of the server zip built beside it. maven.deploy.skip keeps the modules out of Maven Central, which reactor membership would otherwise put them back into.

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. Until the tenth review round it could not refuse at all: the inline catch compared $_.Exception.GetType().FullName with System.IO.IOException, and PowerShell wraps anything thrown out of a .NET method or constructor in a MethodInvocationException, so every exception — the held lock included — took the fail-open branch and the exit 1 was unreachable. It is a typed catch [System.IO.IOException] now, with the [\[]/[\]] escapes a Formatted field needs so the type literal is not substituted away, and CI runs the command directly against a held lock instead of inferring it from an install. Restart Manager is set to 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 — and DisableShutdown is not a middle ground, which is measured rather than reasoned. It keeps RM's detection, which is exactly why it was tried, but the restart is not part of what it disables: the headline upgrade scenario came out of that run with the service StartPending, because the installer restarts whatever the RM session listed whether RM stopped it or the guards did. Turning RM off takes its detection with it, leaving the legacy in-use scan, which finds windowed applications and therefore never a headless JVM — so the lock check is what covers both modes instead, including /quiet, where a FilesInUse dialog could not be shown anyway.
• 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 covers the registry-less case as a whole — nothing recorded a location and no server sits in the legacy default — and lets an upgrade through only on directory evidence: the target holds a setup.bat and either it is not the fallback default or it was named. So msiexec /i ... /quiet naming nothing still refuses; so does a directory that was named but holds no server, which is what a typo looks like (OPENDJ=C:\opendj-custmo over a server in C:\opendj-custom used to pass both guards and strand the data); while a GUI administrator browses to the existing directory in InstallDirDlg and proceeds on the evidence that directory carries. (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. That flag is OPENDJ_GIVEN, public and secure and holding the named path rather than a boolean: it was a private property until the tenth review round, and a private property set in the UI sequence never reaches the execute sequence — which is processed in the installer service, not in the client — so every full-UI session read it as empty at the guards, including one started with OPENDJ= on the command line, and the wizard therefore refused the very workaround the message prescribes. OPENDJ can now be passed with or without /quiet. The guards require the resolved directory to start with the value, which keeps the property from being a switch: OPENDJ_GIVEN=1 disarms nothing, and a value that does pass has spelled out the directory the install resolved to. Evidence alone is deliberately not enough in the default directory: 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 alone let such a decoy disarm it while RemoveExistingProducts gutted the real installation elsewhere, which is what the "or it was named" half is for. 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 (maven.deploy.skip, since the module is in every reactor so the release plugin keeps versioning it): 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 also checks the guards without installing anything, before the install steps: it reads the MSI authoring — wix copies sequence conditions and ExeCommand into the tables verbatim, and reading the built package's tables through the WindowsInstaller.Installer COM API returned no rows at all under pwsh 7 — and asserts that the two refusal conditions name only public properties (a lower-case letter in a condition is a private property, which reads as empty in the installer service and silently inverts the guard), that OPENDJ and OPENDJ_GIVEN are declared Secure, that OPENDJ_GIVEN is still captured with Sequence="first" (without it the property is re-set after CostFinalize in a full-UI install and the guards' prefix test compares a value with itself), and then runs CheckServerNotRunning's command exactly as msiexec runs it — Formatted field resolved, handed to cmd.exe, so Windows PowerShell 5.1 executes it — against a real held byte-range lock: exit 0 with no lock file, 0 with an unlocked one, 1 while a second process holds it, and not before the 60 s grace is up. The scenario suite could see neither failure on its own: it reaches the inline PowerShell only through a ten-minute install and can then judge it by msiexec's exit code alone, and all 42 of its msiexec calls are /qn, which runs no UI sequence at all.
• 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. The two check-based refusals assert the exact 1603 and the failing action's Return value 3 in the log, and the running-server one also fails on any new PendingFileRenameOperations entry: -ne 0 accepts 3010, which is precisely what a guard that fails open produces, and the action's name appears in the log whether it refused or waved the upgrade through — which is how a completely inert CheckServerNotRunning stayed green for two rounds. An upgrade passing OPENDJ_GIVEN=1 must still be refused, as must one naming a directory that holds no server - the mistyped-target case, asserted on the topology the undetectable-directory scenario already builds.
• 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; the property-visibility half of it, a guard reading something the installer service never receives, is now asserted statically against the built MSI.

The guards have now executed in finished runs, and one of those runs is what exposed the fail-open described above. Run 31619923691 (12 Aug, head e6f0a44) finished with test-msi, test-msi-upgrade and wix msi validate -sice ICE61 green — the only red job was build-docker-alpine, which has nothing to do with the MSI — and its log shows the running-server scenario printing exit 3010 where every other refusal prints 1603: the upgrade went through and deferred the locked jars to a reboot, while a -ne 0 assertion accepted it.

Run 31703288399 (13 Aug, head 6bcada5) is green end to end - test-msi, test-msi-upgrade, docker and the ICE validation - with the guard probe reporting CheckServerNotRunning: 0 with no lock file, 0 unlocked, 1 while held (after 61s). The run before it, 31689733634 (head 59b7570), is the one that answered the fail-open: test-msi-upgrade green with Upgrade refused while a non-service server was running (exit 1603), the failing action's Return value 3 in the log and no delete-on-reboot entry left behind. test-msi failed in that run on the new guard step's MSI-table read rather than on anything it was testing; that read is replaced in 6bcada5, and the run on this head is what confirms it. The gate job described under CI is what made finished runs reachable in ~20 minutes instead of two hours.

[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)

[vharseko]

All three confirmed, all three fixed in 59b7570. №1 and №2 turned out to be provable from the run that was already sitting on e6f0a44, which makes your point about the assertions sharper than you put it: the evidence was in the log the whole time.

№1 — the guard could not refuse. Confirmed twice

Reproduced the mechanism the way you describe it (containers, pwsh 7.4.6 and 6.2.4, identical): with a catch-all, $_.Exception is System.Management.Automation.MethodInvocationException and the real exception sits in .InnerException — for the New-Object constructor, for a static method, for ::new(), and for an instance method throwing a genuine IOException. The shipped comparison was therefore true in every case. A typed catch [System.IO.IOException] matches through the wrapper in all of them, including through the inner try/finally.

Then the same command end to end, extracted from the wxs and MSI-formatted, against a real cross-process byte-range lock:

command	outcome under a held lock
shipped	exit 0 after 3 s — fail-open
typed catch	exit 1 after 62 s — refusal

And the proof it already happened in production CI — run 31619923691, job test-msi-upgrade, exactly e6f0a44:

Upgrade refused with guidance, original install untouched (exit 1603)
Empty legacy directory not adopted, upgrade refused          (exit 1603)
Upgrade refused while the service was StartPending           (exit 1603)
Upgrade refused while a non-service server was running       (exit 3010)   <-- fail-open

3010 is ERROR_SUCCESS_REBOOT_REQUIRED: the upgrade went through and left the jars it could not replace to a reboot, which is the outcome the comment above CheckServerNotRunning predicts. Every real refusal is 1603.

One trap in the fix as prescribed. catch [System.IO.IOException] cannot be inlined verbatim: ExeCommand is a Formatted field, so [System.IO.IOException] is a property reference and gets substituted away — leaving two catch-all blocks and a script that fails to parse, which would have refused every upgrade rather than none. It is authored as catch [\[]System.IO.IOException[\]], and the CI step below runs the resolved command, so a literal that got substituted away would show up as the very first probe failing rather than as a guard that quietly refuses everything.

Semantics are preserved, near enough to be worth stating: UnauthorizedAccessException is not an IOException, so a denied open still fails open; FileNotFoundException is one, so a file vanishing between the test and the open now costs a single 2 s sleep before Test-Path lets the next pass out at exit 0.

№2 — the assertions. Confirmed by the same log

The job was green while the guard did nothing, exactly as you say. Both scenarios now assert the exact 1603, grep CheckServerNotRunning. Return value 3 / CheckServiceStopped. Return value 3 rather than the bare action name, and the running-server one additionally snapshots PendingFileRenameOperations around the upgrade and fails on any new entry.

On your suggested order — assertions first, then the guard — the red run already exists in the form of that exit 3010 line, so I did both in one commit rather than pushing a knowingly-red head. What replaces the "watch it go red" step is the direct probe in №4 below: it fails on the old command in about 60 s without installing anything.

№3 — confirmed, and the mechanism is worse than the scheduling bit

You are right about msidbCustomActionTypeFirstSequence, and WiX v5.0.2 does put the row in both tables (Compiler_Package.cs::ParseSetPropertyElement: case "first" only sets executionType, sequences stays at the {InstallUISequence, InstallExecuteSequence} default). But the flag is only half of it — the property would not have survived even if the action had run in the UI sequence:

Custom Action Execution Scheduling Options: "…Is a service registered by default when using Windows 2000 and, in this case, the execute sequence table is processed in the installer service."

Private Properties: "you cannot set a private property in the user interface phase of the installation and then pass the value to the execution phase."

So the comment I had written — "the guards are immediate actions running client-side, where the property is visible" — was wrong in its premise: immediate means impersonated, not client-side. Two independent mechanisms each empty the flag in a full-UI session, and /qn (all 42 Start-Process msiexec calls in build.yml) runs no UI sequence at all, so nothing could see it.

Fixed by replacing the private OpendjDirGiven flag with a public, secure OPENDJ_GIVEN holding the named path. To keep it from becoming a switch, the guards require the resolved OPENDJ to start with it (~<<, not ~=: the captured value is the raw command-line string, the resolved one is a directory property with a trailing backslash). OPENDJ_GIVEN=1 disarms nothing — no resolved path begins with 1 — and a value that does pass has spelled out the directory the install resolved to, which is what naming it does. Starts-with rather than contains for that same reason: 1 occurs inside plenty of real paths and at the start of none. A silent scenario asserts the =1 case is still refused.

Your wizard table, row by row, on this head: row 2 is unchanged — a decoy in the legacy default with nothing named is the residual documented at package.wxs:239-247, and it behaves identically silently; nothing in this round makes it better or worse. Row 1 is unchanged too, deliberately: browsing to the real directory still refuses, because browsing sets no property the guard can read, and I would rather leave the exception needing OPENDJ_GIVEN than let a hand-passed OPENDJ_GIVEN_INSTALL stand alone. It is the safe half — nothing installed, nothing removed — and the message names the command line, which now works in a wizard session. Both are written down at package.wxs:285-292. Say the word if you would rather widen the exception to "the target holds a server, however it was chosen"; it is a two-token change.

№4 — the loop you asked for, in CI

New test-msi step, before anything is installed, reading the built MSI:

• the two guard conditions may name only public properties (a lower-case letter in a condition = a private property = empty in the service — this exact class of bug, caught statically);
• OPENDJ and OPENDJ_GIVEN must be in SecureCustomProperties;
• CheckServerNotRunning's command, resolved the way msiexec resolves it ([\[]/[\]] unescaped, [property] substituted) and handed to cmd.exe so it is Windows PowerShell 5.1 that runs it, must exit 0 with no lock file, 0 with an unlocked one and 1 while a second process holds a LockFileEx range over byte 0 — and not before the 60 s grace is up.

It costs ~70 s and would have failed on the previous head. Moving the script out of the XML attribute entirely (your Binary-table / build-time-inlined .ps1 suggestion) is the better shape and I have not done it — this gets the loop into CI without another round of authoring changes.

Nits

getServiceName's doc comment now names SERVICE_LIST_UNAVAILABLE alongside the other two returns.

On splitting service.c and openScm out: happy to do that if you prefer to land them now — they are independent of everything above.

Verified locally before pushing: WiX 5.0.2 compiles the authoring (on Linux it gets as far as the Windows-only WIX0389 on Directory/@Name, with nothing reported against these changes), the workflow YAML parses, all 22 pwsh steps pass Parser::ParseFile, and the probe behaves as tabulated above. The rest is for the run on this head.

[vharseko]

Run 31689733634 on 59b7570 — the artifact-level answer to №1 and №2, and one correction to what I said about the new step.

The guard refuses now

test-msi-upgrade green end to end, with the line that was 3010 on the previous head:

Upgrade refused with guidance, original install untouched (exit 1603)
Empty legacy directory not adopted, upgrade refused       (exit 1603)
Relocating upgrade refused on both branches, original install untouched
Stray default-directory tree ignored: refused, then upgraded where told
Default-directory upgrade refused silently, accepted when named
Upgrade refused while the service was StartPending        (exit 1603)
Legacy-default decoy: empty target refused, named installation upgraded
Upgrade refused while a non-service server was running    (exit 1603)
The same upgrade proceeded once the server was stopped

So on the same scenario that printed exit 3010 before: 1603, with CheckServerNotRunning. Return value 3 in the log and no new PendingFileRenameOperations entry. The OPENDJ_GIVEN rework left every other scenario alone, including the new one asserting that OPENDJ_GIVEN=1 still gets refused.

Correction: the new step reads the authoring, not the MSI tables

I said it was "extracted from the built MSI". It was, and it did not work: the WindowsInstaller.Installer COM view returned zero rows in pwsh 7 without raising anything, so the step failed on its first assertion — RequireDirOnCustomUpgrade is not scheduled in InstallExecuteSequence — while test-msi-upgrade was proving on that same head that it is scheduled and does fire. Most likely if (-not $record) around View.Fetch: Record's default member is the parameterized StringData, and pwsh 7's COM adapter does not convert that to $true the way 5.1 does. I could not verify that without a Windows box and did not want to spend a second red run guessing, so 6bcada5 drops the COM read: the two guard conditions, the Secure declarations and CheckServerNotRunning's ExeCommand now come from package.wxs, which wix copies into the tables verbatim.

What that costs is a wix-level transformation between authoring and package that nobody has ever seen happen for these three fields; what still judges the artifact is the scenario suite above, and it now judges it on the refusal's own 1603 and Return value 3 rather than on any non-zero exit code. The probe itself is unchanged — the command is resolved exactly as msiexec resolves it ([\[]/[\]] unescaped, [property] substituted), written to a .cmd and run through cmd.exe, so Windows PowerShell 5.1 executes it against a real held byte-range lock: 0 / 0 / 1, and not before the 60 s grace is up. Dry-run of the whole step on the Linux stand-in (only cmd.exe and the path separators swapped):

RequireDirOnCustomUpgrade : WIX_UPGRADE_DETECTED AND (OPENDJ ~= OPENDJ_DEFAULT) AND NOT OPENDJ_REG AND NOT (OPENDJ_GIVEN AND (OPENDJ ~<< OPENDJ_GIVEN))
RefuseRelocatingUpgrade : WIX_UPGRADE_DETECTED AND ((OPENDJ_REG AND (OPENDJ_REG ~<> OPENDJ)) OR (NOT OPENDJ_REG AND OPENDJ_LEGACY_INSTALL AND (OPENDJ_LEGACY_DEFAULT ~<> OPENDJ) AND NOT (OPENDJ_GIVEN AND (OPENDJ ~<< OPENDJ_GIVEN) AND (OPENDJ_GIVEN_INSTALL ~>< OPENDJ))))
CheckServerNotRunning: 0 with no lock file, 0 unlocked, 1 while held (after 61s)

If you would rather have the assertions read the package itself, that is a follow-up with a properly debugged reader — P/Invoke on msi.dll rather than IDispatch, most likely — and I would want a Windows shell to develop it against rather than the CI queue.

[maximthomas]

Reviewed at 6bcada5. The three blockers from the last round are genuinely fixed, and I confirmed the typed catch, the ~<</~<>/~>< operand order, OPENDJ_GIVEN public+Secure, the service.c SCM fixes, and the release.yml/deploy.yml publish paths. Three things should be settled before merge — one of them outside the MSI entirely.

Release drops opendj-msi from the reactor, freezing the MSI POM versions (blocker)

opendj-packages/pom.xml removes <module>opendj-msi</module> from distribution-unix and distribution-mac, and the replacement profile activates only on Windows with WiX present:

<activation>
    <os><family>windows</family></os>
    <file><exists>${env.USERPROFILE}/.dotnet/tools/wix.exe</exists></file>
</activation>

.github/workflows/release.yml runs release:prepare release:perform on ubuntu-latest with no -P, and maven-release-plugin only rewrites ${reactorProjects}. So opendj-packages/opendj-msi/pom.xml and .../opendj-msi-standard/pom.xml — both hard-coding parent 5.2.0-SNAPSHOT — will never be version-bumped again. Every past release (5.1.0/5.1.1/5.1.2) did rewrite them.

Maven does not fail on parent version skew; it silently falls back to the repository copy with no warning, and opendj-packages:5.2.0-SNAPSHOT + opendj-server-legacy:5.2.0-SNAPSHOT are both currently published. After the next release the MSI module pins itself to 5.2.0-SNAPSHOT, unpacks the pre-release server zip, and emits opendj-5.2.0-SNAPSHOT.msi — which release.yml attaches to the 5.2.0 release via target/*.msi. continue-on-error: true hides it.

Suggested fix: keep opendj-msi in the Linux module list and move the WiX/OS activation down onto opendj-msi-standard, so the release plugin still sees both POMs.

A relocating upgrade is unguarded when nothing recorded the old location (major)

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs: RequireDirOnCustomUpgrade triggers on OPENDJ ~= OPENDJ_DEFAULT, and RefuseRelocatingUpgrade's second branch needs OPENDJ_LEGACY_INSTALL. With 5.1.x in a custom directory, no registry entry and no legacy directory (the topology already in .github/workflows/build.yml), a mistyped target slips past both:

msiexec /i opendj.msi OPENDJ=C:\opendj-custmo /qn

Guard 1: resolved dir ≠ x64 default → silent. Guard 2: no OPENDJ_REG, no OPENDJ_LEGACY_INSTALL → silent. Exit 0, RemoveExistingProducts strips the program files from C:\opendj-custom, the new tree lands at the typo path, and config/db/logs are stranded with no server and no upgrade.bat. The same typo is refused when an unrelated tree happens to sit in the legacy default, so today the protection depends on an incidental leftover directory.

Related: OPENDJ_GIVEN is a prefix test, so OPENDJ_GIVEN=C disarms guard 1 outright — the comment's "a value that does pass has spelled out the directory the install resolved to" holds for 1 but not for C, C: or C:\. Low on its own (it reaches nothing the documented OPENDJ= route doesn't), but it falls out of the same fix.

Giving guard 1 the evidence test guard 2 already uses closes both, with guard 2 and the SetProperty untouched:

<Custom Action="RequireDirOnCustomUpgrade" After="CostFinalize"
        Condition="WIX_UPGRADE_DETECTED AND NOT OPENDJ_REG AND NOT OPENDJ_LEGACY_INSTALL AND NOT ((OPENDJ_GIVEN_INSTALL ~&gt;&lt; OPENDJ) AND ((OPENDJ ~&lt;&gt; OPENDJ_DEFAULT) OR (OPENDJ_GIVEN AND (OPENDJ ~&lt;&lt; OPENDJ_GIVEN))))"/>

Read as: during an upgrade where nothing recorded a location and no server sits in the legacy default, refuse unless the target holds a server and was actively chosen. NOT OPENDJ_LEGACY_INSTALL is load-bearing — without it the headline upgrade scenario and the two Return value 3 scenarios refuse. I walked this against all 43 msiexec calls in build.yml; none change outcome. It also closes the wizard form (browse to an empty directory over a registry-less custom install).

opendj_service.exe does not match service.c (major)

The committed binary was last refreshed in 252d5ba9b8; opendj-server-legacy/src/build-tools/windows/service.c has changed in six commits since. It still contains a function the PR itself reverted away:

$ git show HEAD:opendj-server-legacy/lib/opendj_service.exe | strings | grep isMsiManagedService
isMsiManagedService: no HKLM\SOFTWARE\OpenDJ key, treating '%s' as orphaned.
Refusing to remove the MSI-managed service '%s'.
$ git grep isMsiManagedService -- opendj-server-legacy/src
(no match)

and lacks the new string from serviceState() ("no service list"), while its sibling "Service '%s' is enabled." is present.

The functional delta is small — the openScm NULL check and the SERVICE_LIST_UNAVAILABLE → 2 mapping, both diagnostic-only; removeService() returning 3 already ships. And deploy.yml's new commit-back supersedes the bytes on the first post-merge push. But that step has never run, all its failure paths are continue-on-error, and a Release dispatch before the first Package/Deploy would ship the stale binary to Maven Central (it copies lib/*.exe verbatim into the zip, deb, rpm and Docker image). One more refresh commit, as the branch has already done seven times.

Worth noting the stale exe is an intermediate branch state carrying two behaviours this PR deliberately reverted, so it is not equivalent to any reviewed source.

Nits

• build.yml OPENDJ_GIVEN=1 scenario asserts only a non-zero exit: it is the only one of eight refusal scenarios with no message assertion, and -eq 0 also accepts 3010 — the fail-open outcome the neighbouring scenarios use -ne 1603 to exclude. The guard it expects already has a grep three lines above: "location could not be determined".
• chap-upgrade.adoc over-promises: "The installer further refuses to install into a directory other than the detected one" is false in the case above, and contradicts the NOTE ten lines below telling the reader to name a directory. Worth scoping once the guard is fixed.
• Stale comments in package.wxs: "build.yml asserts that in the built MSI" and "extracted from the built MSI" — since 6bcada5 the step reads package.wxs itself, so nothing inspects the built package. Also "all 42 msiexec calls" — there are 43 (that count was off when written).
• Sequence="first" is untested: all 43 msiexec calls are /qn, so reverting it to both is invisible to CI even though it is what keeps the OPENDJ_GIVEN prefix test from being a tautology in a UI install. One regex in the step that already reads the .wxs would pin it.
• Maintenance-mode in-use detection (pre-existing, follow-up): CheckServerNotRunning is gated on WIX_UPGRADE_DETECTED, and FindRelatedProducts does not run in maintenance mode, so repair/reinstall/uninstall get no in-use check. Not a regression — master had no guards and RM could not shut down the detached JVM either — and MSIRESTARTMANAGERCONTROL=Disable fixes a reproduced failure, so it should stay. If widened later, note that a bare OR Installed would make a running server un-uninstallable; it needs to exclude REMOVE="ALL".

[vharseko]

All three confirmed and two of them fixed — ad4ba90 (reactor) and c22a4da (guard 1 plus the nits). The exe is deliberately left to the deploy job; reasoning below.

1. Reactor — fixed, and your one-liner needed two additions

Confirmed exactly as described: master carried opendj-msi in distribution-unix:58, distribution-mac:72 and distribution-windows:82; this PR left it only in a profile that cannot activate on the runner release.yml:38 uses, and both MSI poms hard-code 5.2.0-SNAPSHOT.

Keeping opendj-msi in the Linux list is not sufficient on its own, though: opendj-msi-standard/pom.xml declares its own parent (opendj-msi:5.2.0-SNAPSHOT), so it has to be in the reactor too or it stays frozen while its parent moves. So the gate is on neither module list now — it moved down onto the plugins, as a distribution-windows-msi profile inside opendj-msi-standard wrapping its <build>. Same activation as before (Windows AND the wix.exe file test), same -P name for release.yml:228, and inactive it is a pom that builds nothing, which keeps the "no Cannot run program wix on a contributor's machine" property the profile existed for.

Second addition: reactor membership would have put both poms straight back into the Central deploy this PR removed them from, so maven.deploy.skip=true in opendj-msi/pom.xml, inherited by the standard module.

Verified with release:prepare -DdryRun=true:

opendj-packages/opendj-msi/pom.xml.tag                     → <version>9.9.9</version>
opendj-packages/opendj-msi/opendj-msi-standard/pom.xml.tag → <version>9.9.9</version>

The same run also demonstrates the mechanism you named: the poms that got a .tag are exactly the active reactor — on macOS opendj-docker, opendj-svr4 and both MSI modules, but not opendj-deb/opendj-rpm, which sit in distribution-unix and its <name>Linux</name>.

Two details worth adding to the record, because they make the blocker worse than "the MSI is misnamed":

• Nothing would have failed. release.yml:219-221 installs the release zip as opendj-server-legacy:${releaseVersion}, but the module asks for ${project.version} (opendj-packages/pom.xml:121), which the frozen parent pins to the old snapshot — and the root pom declares central-portal-snapshots with snapshots enabled (pom.xml:129-137), so that zip resolves over the network. The carefully installed release zip is simply never requested; continue-on-error does not even come into it.
• The daily build breaks next, not the release. After 5.2.0 the tree moves to 5.2.1-SNAPSHOT while the MSI module stays at 5.2.0-SNAPSHOT, so unpack-archive starts downloading the previous line's published zip instead of taking the one built beside it in the same reactor. test-msi and test-msi-upgrade would have gone on passing against bits that have nothing to do with the branch under test — including the freshly compiled opendj_service.exe. That is what tipped the decision against fixing this inside release.yml (a versions:set there would have covered the release path only).

2. Guard 1 — your condition, taken as written

Condition="WIX_UPGRADE_DETECTED AND NOT OPENDJ_REG AND NOT OPENDJ_LEGACY_INSTALL AND NOT ((OPENDJ_GIVEN_INSTALL ~>< OPENDJ) AND ((OPENDJ ~<> OPENDJ_DEFAULT) OR (OPENDJ_GIVEN AND (OPENDJ ~<< OPENDJ_GIVEN))))"

I walked it against every scenario in build.yml before taking it and agree with your reading: nothing changes outcome, NOT OPENDJ_LEGACY_INSTALL is what keeps the headline upgrade and both Return value 3 scenarios out of it, and the wizard form closes as a side effect — which also settles the "browse to the real directory is still refused" residual I documented last round, so that comment is gone rather than reworded.

You are right about OPENDJ_GIVEN=C too: a one-character prefix matches every resolved path, and my comment claiming a passing value "has spelled out the directory" was wrong for C, C: and C:\. With the evidence test a conjunct rather than an alternative it stops mattering — the directory still has to hold a server.

CI gets the typo case on the topology the neighbouring scenario already builds:

msiexec /i opendj.msi OPENDJ=C:\opendj-custmo   # over a server in C:\opendj-custom

asserted at exactly 1603, with the guard's message in the log, no C:\opendj-custmo created and C:\opendj-custom\setup.bat intact.

The refusal message now says the directory holds no OpenDJ server and suggests checking for a typo, and chap-upgrade.adoc no longer promises a refusal it did not make — both sentences you flagged are rescoped rather than deleted.

3. opendj_service.exe — deliberately left to Package/Deploy

Confirmed in the bytes: the committed binary carries isMsiManagedService: … and Refusing to remove the MSI-managed service '%s' — a function b2de060 reverted — and lacks no service list; service.c has moved in six commits since 252d5ba9b8. It is an intermediate branch state, as you say, not a state that was ever reviewed.

We are still going to let deploy.yml commit it back on the first push after merge rather than adding an eighth refresh commit, on the grounds that nothing CI judges uses the committed binary: build.yml:79-86 runs nmake all and xcopy /Y *.exe ..\..\..\lib\ before Maven on every Windows leg, so the zip and the MSI in windows-latest-11 — the artifact both MSI jobs download — are built from current sources. What is stale is the Linux-built zip and everything downstream of it, which is exactly what the commit-back exists to fix.

Your residual stands and we are taking it as a process constraint rather than arguing it away: that step has never executed and every failure path in it is a warning, so no Release gets dispatched until the refresh commit is visible on master.

Nits

• OPENDJ_GIVEN=1 now asserts 1603 and greps location could not be determined, like its neighbours.
• The three stale comments are corrected to say the authoring rather than the built MSI. The msiexec count is gone rather than updated — it was 43 when you counted, is 44 with the typo scenario, and was never going to survive another round.
• Sequence="first" is pinned in the step that already reads the .wxs, with the reason in a comment: without it the property is re-set after CostFinalize in a full-UI install and the prefix test compares a value with itself.
• Maintenance-mode in-use detection: left alone, and the REMOVE="ALL" trap noted for whoever widens it.

[maximthomas]

Round 11 items 1 and 2 look right — ad4ba90 keeps both MSI poms versioned, and c22a4da closes the mistyped/relocating upgrade hole (I re-derived the guard state space from scratch; it holds, including the OPENDJ_GIVEN=1/=C bypasses, both decoy topologies, and the documented workaround).

One major in the new reactor fix: the mechanism that is supposed to keep the modules off Maven Central does nothing.

Maven Central guard is inert (major)

opendj-packages/opendj-msi/pom.xml:43 relies on maven.deploy.skip, but that property is dead in this build. pom.xml:336-345 declares central-publishing-maven-plugin:0.8.0 with <extensions>true</extensions>, whose lifecycle participant unconditionally wipes the deploy plugin's executions:

private void maybeSkipMavenDeployPlugin(Model, Plugin);
   7: ifnull        24          // only condition: plugin absent
  14: invokeinterface List.clear:()V
  21: invokespecial setUpCentralPublishingExecution   // publish @ deploy

maven.deploy.skip gated only maven-deploy-plugin:deploy, which no longer has a binding — the string does not occur anywhere in the plugin jar. Reproduced at head: mvn deploy -pl :opendj-msi-standard never runs deploy:deploy, and central-publishing:publish is still reached.

Effect on a 5.2.0 release — opendj-msi-standard is now an unconditional module and the wine install is gone, so Central gets:

opendj-msi/5.2.0/opendj-msi-5.2.0.pom
opendj-msi-standard/5.2.0/opendj-msi-standard-5.2.0.pom

and no .msi. So the PR's stated outcome — "the Maven coordinate stays at its last published version (5.1.2)" — is not what happens. opendj-msi-standard-5.1.2.msi resolves today; a consumer moving to 5.2.0:msi gets a resolvable pom and a hard Could not find artifact ...:msi, rather than the clean "version does not exist" the PR intends.

Fix: excludeArtifacts on the existing root plugin config. It is matched against the bare artifactId (excludeArtifacts.contains(artifact.getArtifactId())), so groupId:artifactId would silently never match:

<excludeArtifacts>
    <excludeArtifact>opendj-msi</excludeArtifact>
    <excludeArtifact>opendj-msi-standard</excludeArtifact>
</excludeArtifacts>

Verified: this yields [INFO] No files to stage! and no bundle, with the rest of the reactor still published. Bumping the plugin to >= 0.9.0 (where skipPublishing becomes per-module) also works. Please do not use skipPublishing at 0.8.0 — it is session-global there: in a non-last module it is a no-op, and in the last module it skips the entire release with BUILD SUCCESS.

Uninstall still reports "already disabled" when the SCM is unreadable (minor)

opendj-server-legacy/src/build-tools/windows/service.c:2553 now returns 3 when the service list cannot be read, which fixes the no-service path. The path where a service is found still fails open: removeServiceWithServiceName calls serviceNameInUse(), whose own failure branch (service.c:1990) returns SERVICE_RETURN_ERROR, and everything that is not SERVICE_IN_USE maps to exit 1 / "Service does not exist" — which ConfigureWindowsService.disableService reads as SERVICE_ALREADY_DISABLED.

If the SCM becomes unreadable between the getServiceName enumeration and the serviceNameInUse enumeration, --disableService claims the service was already disabled and uninstall proceeds, leaving a registered auto-start service pointing at a removed tree — the same lie service.c:2499 set out to remove.

Launcher refresh rebases onto unvalidated commits (minor)

.github/workflows/deploy.yml:140 runs git rebase FETCH_HEAD before the Maven publish at .github/workflows/deploy.yml:196 and before the artifact uploads. If the branch moved after checkout, the tree fast-forwards onto commits outside workflow_run.head_sha that the triggering Build never validated, and those bits are what get deployed. Two pushes landing close together are enough. Either move the refresh after the Maven steps, or reset the worktree back to HEAD_SHA for the build.

Nits

• Stale package.wxs comment: opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:125 still says additional instances fall back to WiX3-era delete-on-reboot. CheckServerNotRunning is not ImagePath-gated (package.wxs:455-459) and fires on WIX_UPGRADE_DETECTED alone, so a running OpenDJ Server-2 holds locks\server.lock and the upgrade is refused with 1603 instead. Same for a ko/zh_TW service registered as OpenDS. The residual is only that the stop is not attempted — a refusal, not damage.
• Stale -P justification: .github/workflows/release.yml:225 says -P guards against "Could not find the selected project in the reactor". Since the profile moved into opendj-msi-standard's <build>, that failure can no longer occur. What -P now buys is forcing the plugins on when wix.exe is not under %USERPROFILE%\.dotnet\tools; without it the job builds a pom that produces nothing and fail_on_unmatched_files is the only symptom.
• Unrelated plugin bump: dropping <version>1.3.2</version> from the mib-generation execution at opendj-server-legacy/pom.xml:1406 moves it onto the managed exec-maven-plugin 3.6.3. That execution lives in the snmp profile, which auto-activates whenever ${opendmk.lib.dir}/jdmkrt.jar exists — i.e. on nearly every build on every platform, not just the MSI leg. Worth its own commit or a line in the description.