Skip to content

fix(windows): make install and uninstall recoverable - #364

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/windows-install-uninstall-safety
Aug 2, 2026
Merged

fix(windows): make install and uninstall recoverable#364
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/windows-install-uninstall-safety

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Both flows could leave a machine in a state with no supported way out.

Uninstall removed the registry entries first. An unelevated uninstall of a Program Files install deleted the Add/Remove Programs key and the shortcuts, then spawned a batch script whose every del/rmdir was silenced with > nul 2>&1. The files stayed on disk, the app was gone from Add/Remove Programs, and handle.exit(0) ran regardless — uninstall_app always returned Ok(()), so the failure was never reported.

Now a deletability precheck runs before anything is touched, the script logs each step to a temp file instead of discarding the output, and it restores the uninstall entry if deletion fails. Registry cleanup happens last.

Install had no rollback. The executable was copied at step 2, but the Add/Remove Programs entry was not written until step 6. Any failure in between — most likely a shortcut in a locked-down Public Desktop — aborted the ? chain with the exe in place. is_installed() then reported true by path match, so the installer UI never appeared again, while nothing had ever been registered: uninstallable through Windows, unreachable through the app.

Elevation is now checked before the first mutation, and completed steps are undone in reverse on failure. An update/repair never deletes what it was updating.

Also in these paths

  • restore_file_association opened keys with create_subkey, so uninstalling materialised HKLM\Software\Classes\.md and .markdown on machines where Markpad had never registered them, then deleted a default value that did not exist. It now opens read/write and returns early when the key is absent. The other hive is cleaned too, but only when no installation is registered there — otherwise an all-users uninstall breaks a coexisting per-user install and leaves .md pointing at a dead ProgID.
  • wscript was launched by bare name, so the process search order checked the application and current directories before %PATH%. It is now resolved through %SystemRoot%\System32, as are taskkill, timeout and reg inside the batch. Temp script names carry random suffixes and use create_new rather than being fixed and overwritable.
  • Permission failures return an ELEVATION_REQUIRED: code. The installer UI matched the literal string Access is denied, which only appears on English Windows — localized systems showed the raw exception instead of the "run as administrator" message.

Validation

  • cargo test — 41/41 (Windows-only logic is extracted into pure functions gated cfg(any(target_os = "windows", test)), so ordering, rollback and script generation are exercised on any host)
  • cargo clippy — no new warnings; the Windows branches were additionally type-checked against x86_64-pc-windows-msvc and come out one warning below the current baseline
  • npm run check / npm test — clean, for the installer error-code change

Needs a pass on real Windows

The batch script text was reviewed by hand but never executed, and can_write_dir behaviour under UAC virtualization can only be observed on the real thing. Worth exercising before release: unelevated all-users install, a failure at the Public Desktop shortcut, update/repair failing midway, unelevated uninstall of a Program Files install, and .md association after an all-users uninstall with and without a coexisting per-user install.

Both flows could leave a machine in a state with no supported way out.

**Uninstall removed the registry entries first.** An unelevated uninstall of
a Program Files install deleted the Add/Remove Programs key and the
shortcuts, then spawned a batch script whose every `del` and `rmdir` was
silenced with `> nul 2>&1`. The files stayed on disk, the app was gone from
Add/Remove Programs, and `handle.exit(0)` ran regardless — `uninstall_app`
always returned `Ok(())`, so nothing was ever reported. A deletability
precheck now runs before anything is touched, the script logs each step to a
temp file instead of discarding the output, and it restores the uninstall
entry if deletion fails. Registry cleanup happens last.

**Install had no rollback.** The executable was copied at step 2, but the
Add/Remove Programs entry was not written until step 6. Any failure in
between — most likely creating a shortcut in a locked-down Public Desktop —
aborted the `?` chain with the exe in place, so `is_installed()` reported
true by path match and the installer UI never appeared again, while nothing
had ever been registered. Elevation is now checked before the first
mutation, and completed steps are undone in reverse on failure. An
update/repair never deletes what it was updating.

The script keeps the process handling introduced by #358: it waits for its
own process id to disappear and never matches on the image name, so a
portable copy, another user's session, or a second window with unsaved work
survives an uninstall. That wait is now bounded — reaching the deletion
attempt, which logs and restores the uninstall entry, beats an invisible
batch file spinning forever.

Also in these paths:

- `restore_file_association` opened keys with `create_subkey`, so
  uninstalling materialised `HKLM\Software\Classes\.md` and `.markdown` on
  machines where Markpad had never registered them, then deleted a default
  value that did not exist. It now opens read/write and returns early when
  the key is absent. The other hive is cleaned too, but only when no
  installation is registered there — otherwise an all-users uninstall would
  break a coexisting per-user install and leave `.md` pointing at a ProgID
  that no longer exists.
- The helper process was launched by bare name, so the process search order
  checked the application and current directories before `%PATH%` — a
  planted `wscript.exe` would run with whatever privileges the uninstall
  had. It, and `tasklist`, `find`, `timeout` and `reg` inside the batch, are
  now resolved through `%SystemRoot%\System32`. The temp script names carry
  random suffixes and are created with `create_new` instead of being fixed
  and overwritable.
- Permission failures return an `ELEVATION_REQUIRED:` code. The installer UI
  matched on the literal string `Access is denied`, which only appears on
  English Windows, so localized systems showed the raw exception instead of
  the "run as administrator" message.

The Windows-only logic is extracted into pure functions gated
`cfg(any(target_os = "windows", test))` so the ordering, rollback and script
generation are exercised by tests on any host.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao force-pushed the fix/windows-install-uninstall-safety branch from cf212d9 to 4462ae4 Compare August 2, 2026 17:39
@PathGao
PathGao merged commit e10cd61 into sftwrdotdev:master Aug 2, 2026
5 checks passed
@PathGao
PathGao deleted the fix/windows-install-uninstall-safety branch August 2, 2026 18:18
PathGao added a commit that referenced this pull request Aug 2, 2026
…ut (#400)

`create_shortcut` handed a path straight to `mslnk`, which does a plain
file create and will not build the path leading to it. When the Desktop
or Start Menu directory does not exist - trimmed images, redirected
`%PUBLIC%`, some corporate policies - the shortcut step fails and, since
it propagates with `?`, the whole install fails. That is the cause
underneath #122; #364 made the failure clean and named the directory,
but the directory still never got created.

Each missing level is created separately rather than through
`create_dir_all`, so every level can be recorded as its own
`InstallStep::CreatedDir` and #364's existing rollback removes them
deepest-first. `AlreadyExists` is tolerated but records nothing - we did
not create it, so we must not delete it.

A failure to create the directory still fails the install rather than
skipping the shortcut: #364 defined exactly two outcomes, complete or
rolled back, and a third one - "installed, minus the icon you asked
for" - is #122's symptom with the actionable error removed. Routing the
error through `io_error_message` also keeps the locale-independent
`ELEVATION_REQUIRED:` prefix the frontend branches on.

Uninstall does not remove these directories. Rollback runs seconds after
creation, inside one operation, while the directory is still empty;
at uninstall time arbitrary time has passed and the directory is the
user's.

Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant