Repository navigation
fix(windows): repair install.ps1, honest dry-run status, deterministic daemon_ipc - #1320
Merged
Merged
Conversation
install.ps1 did not run. It failed to PARSE, dying with a cascade of syntax errors before its first statement, and it has been in that state on main since 61a432c. Windows PowerShell 5.1 decodes a BOM-less .ps1 as ANSI, not UTF-8. The three em-dashes in this file are 3 UTF-8 bytes each, and cp1252 renders them as a sequence ENDING IN A DOUBLE QUOTE. Two sat in comments and were harmless; the third sat inside a string literal: "error: could not retire the existing $BinName - close all running" where the injected quote closed the string early and left the rest of the line as garbage expression, taking the enclosing if/foreach blocks with it. This shipped broken because nothing executed the script. The smoke covers it in Phase 13, but every Windows smoke run since the launcher removal aborted at an earlier phase, and the VirusTotal check only ever scanned the bytes. Since install.ps1 is now the sole Windows install AND update path, a parse error there is a total loss of both. Fixed as ASCII rather than by adding a BOM: the documented install pipes the file straight into the parser (irm ... | iex), where a BOM would become part of the string and break that path instead. The bundle contract now rejects any non-ASCII codepoint in install.ps1 and names it, so this cannot return silently. Signed-off-by: Martin Vogel <[email protected]>
…atus The Windows smoke guards that an invalid PATH seam must fail closed rather than silently falling back to the live HKCU\Environment\Path. That guard went green in a way that meant nothing: the seam still refused, and install still printed "PATH configuration failed", but the process exited 0. Windows release builds used to compile a separate managed-install path. With one binary per platform they compile the shared cbm_cmd_install, which since 30b5f12 downgraded ANY failed dry-run plan check to CLI_OK. That commit was fixing a real complaint -- the dry-run summary was being skipped entirely on Windows -- but it bundled the exit status into the fix. Keeping the summary is right; reporting success is not. --dry-run exists to answer "would this install work?", so a caller that only sees the exit code could not tell a refused PATH probe from a clean plan. The summary and the triage note still print; only the status changes. A real install already failed non-zero here, so the fail-closed behaviour itself was never at risk -- just its observability. The smoke also still encoded the launcher-era portable-vs-managed split, which does not survive one-binary-per-platform: * 6b expected uninstall to be REFUSED. That refusal existed because a portable extracted bundle was a different artifact from the launcher-managed install it would have torn down. The extracted binary now IS the installed one. * 6c expected update to be REFUSED, contradicting the contract Phase 14a asserts against a real update: Windows hands off to install.ps1, exits 0 and prints the command. 6c now pins that same handoff. * 14a compared binaries with cmp, which is absent from the Windows MSYS shell, so it reported "different" for two copies of one file. Hashes instead, the way Phase 12 already verifies the release archive. * 14f seeded the MCP entry at the RETIRED binary. The old launcher-managed update rewrote that entry in-process; the handoff does not, so uninstall correctly declined to remove an entry owned by a different installation and the phase demanded the one thing it must never do. It now seeds the installed binary, which is what install.ps1 leaves a real user holding. Signed-off-by: Martin Vogel <[email protected]>
…ling for it Two Windows tests needed the startup thread to be observed HOLDING the startup lock, and looked for it by polling the lock for BUSY up to 200 times. That state has no lower bound: whenever the rendezvous handoff does not block, acquire -> handoff -> release completes in microseconds, so the window can close between two polls and startup_observed asserts on a coin flip. This blocked release run 30374135923 on test-windows (windows-2025, CLANG64, x86_64). Budget-tuning the poll was tried and rejected -- a wider budget only lowers the failure rate, and a transient window is never a fixed test. Instead the production path exposes a gate that fires once the startup lock is held and before the handoff runs, matching the hook pattern already used for POSIX publication. The test parks the startup thread there and releases it when it is ready, so the interleaving is pinned by construction. The acquired flag is monotonic, so the observation cannot be missed; the remaining bound only catches a thread that never started at all. The gate is inert unless a test installs it. Also fixes an unrelated use-after-free this exposed. ipc_forever_wait_server closed the connection and NULLed it from the server thread, while the test was about to call cbm_daemon_ipc_connection_interrupt on the pointer it had already loaded -- and receive_frame can return on its own before that call lands. It crashed roughly one Windows run in five, independent of the change above. Ownership now stays with the test, which closes after joining, so interrupt can only ever see a live connection. Verified on real Windows: 5 consecutive daemon_ipc runs, no failure and no crash. macOS 46 passed x5 under ASan/UBSan; Linux 6606 passed, 0 failed. Signed-off-by: Martin Vogel <[email protected]>
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unblocks the 0.9.1-rc.1 release. Three independent fixes, one of them
user-facing and serious.
install.ps1 could not run at all
install.ps1failed to PARSE and has been in that state on main since61a432c. Windows PowerShell 5.1 decodes a BOM-less.ps1as ANSI, so a UTF-8em-dash arrives as cp1252 bytes ending in a double quote. One of the three in
this file sat inside a string literal, where that quote closed the string early
and took the enclosing blocks with it.
It shipped because nothing ever executed the script: the smoke covers it in
Phase 13, but every Windows smoke run since the launcher removal aborted at an
earlier phase, and the VirusTotal check only scanned the bytes. Since
install.ps1is now the sole Windows install AND update path, this was a totalloss of both.
Fixed as ASCII rather than with a BOM, because the documented install pipes the
file straight into the parser (
irm ... | iex). The bundle contract now rejectsany non-ASCII codepoint and names it.
install --dry-run reported success on a failed plan check
The Windows guard that an invalid PATH seam must fail closed was passing
vacuously. The seam still refused and still printed
PATH configuration failed,but the process exited 0, because Windows now compiles the shared
cbm_cmd_install, which since30b5f12downgraded any failed dry-run plan checkto
CLI_OK. Keeping the dry-run summary was right; reporting success was not.Only the status changes -- a real install already failed non-zero, so
fail-closed behaviour was never at risk, just its observability.
The smoke also still encoded the launcher-era portable-vs-managed split, which
does not survive one-binary-per-platform:
6b/6cexpected refusals that nolonger apply (and
6ccontradicted14a),14ausedcmp, which is absentfrom the Windows MSYS shell, and
14fseeded the MCP entry at the retiredbinary that the old in-process update used to rewrite.
daemon_ipc was decided by a coin flip
Two Windows tests polled for the startup thread HOLDING the startup lock -- a
state with no lower bound, since acquire -> handoff -> release completes in
microseconds when the handoff does not block. This blocked release run
30374135923. Widening the budget was tried and rejected; a transient window is
never a fixed test. The production path now exposes a gate that fires with the
lock held and before the handoff, matching the existing POSIX publication hook,
so the interleaving is pinned by construction and inert unless a test installs
it.
Also fixes an unrelated use-after-free it exposed:
ipc_forever_wait_serverclosed and NULLed the connection from the server thread while the test was about
to
interruptthe pointer it had already loaded. It crashed roughly one Windowsrun in five.
Verification
Full 3-OS local, on the final tree:
daemon_ipc46 passed x5 under ASan/UBSan, full smoke exit 0daemon_ipc5/5 consecutive, smoke ALL PASSED (Phases 1-17)install.ps1verified executing end to end on Windows (13g: binary placed,13h: binary runs).lint-ciclean. The new ASCII guard verified in bothdirections -- passes clean, fails with
U+2014when an em-dash is reintroduced.