Skip to content

fix(win): preserve shared associations and restore failed upgrades - #627

Merged
knipknap merged 2 commits into
mainfrom
fix/win-installer-safety
Oct 23, 2026
Merged

knipknap merged 2 commits into
mainfrom
fix/win-installer-safety

Conversation

@knipknap

@knipknap knipknap commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR change?

Fixes two defects in the Windows installer/uninstaller. Both were found during
the review of the winget-pkgs package submission
(microsoft/winget-pkgs#460163),
which was closed until these are fixed upstream:

  1. Uninstaller destroyed shared file associations. The uninstaller called
    DeleteRegKey HKCR ".png" (likewise .bmp, .jpeg, .jpg, .svg,
    .svgz, .dxf), deleting whole shared extension keys together with values
    and subkeys owned by other applications. Rayforge no longer claims shared
    extensions as default at all: it registers its ProgIDs and lists them under
    each extension's OpenWithProgids subkey, which adds Rayforge to the
    "Open with" menu. Uninstalling removes exactly those registrations, plus
    the ProgID keys. The only time a shared extension's default value is
    touched is when it still points at a Rayforge ProgID (which would dangle
    after removal); it is cleared, never replaced with a guessed previous
    state. Rayforge-owned types (.ryp, .rfs, .rd) keep full default
    registration and whole-key cleanup.

  2. A failed upgrade could leave the old installation half-deleted. The
    installer removed the previous installation with RMDir /r before
    checking whether the deletion succeeded, so a single locked file (e.g. the
    app still running) aborted the upgrade with the old version already
    partially destroyed. The installer now:

    • checks for a running instance first and prompts with Retry/Cancel
      (in silent installs, e.g. winget, it aborts cleanly before deleting
      anything),
    • moves the previous installation aside with Rename to <dir>.old -
      which fails harmlessly while any file in it is locked, leaving the old
      version fully intact,
    • installs the new files into the fresh directory, and only removes the
      moved-aside copy once the installation is complete,
    • restores the previous installation via a new .onInstFailed callback
      (deletes partial new files, renames the old copy back) if the upgrade
      fails half-way, e.g. on a full disk.

Why did you choose this specific architectural approach?

  • For shared file types, the previous draft stored the replaced default
    association and restored it on uninstall behind an ownership guard. That is
    correct in the "another app claimed it later" case, but snapshotting and
    restoring mutable external state is fragile by design, so the final
    approach mutates no shared state at all: OpenWithProgids is additive,
    removal is exactly scoped to what Rayforge wrote, and there is no previous
    state to track. This follows the registration model documented by
    Microsoft for the "Open with" dialog.
  • Rename of the old install directory is atomic on the same volume and
    fails without side effects while a file in it is locked. That covers the
    reviewer's regression case (upgrade while the old app is running) without
    a full backup/restore of the installation, which would double the disk
    footprint and still risk partial state.
  • The running-instance check uses only stock NSIS (tasklist piped through
    find, excluding the installer process itself, which shares the Rayforge*
    name prefix). The bundled executable name contains the version, so matching
    a fixed file name would miss the old version's process during an upgrade.
    No third-party NSIS plugins are required, so the MSYS2 makensis build
    stays unchanged.

How did you manually test this locally?

  1. Compiled scripts/win/win_installer.nsi with makensis 3.12 (x86-unicode)
    against a stub icon and PyInstaller dist tree: zero warnings, valid
    installer produced.
  2. Verified the preprocessed output (-PPO): macros expand with the correct
    registry paths, the process check excludes the installer itself, and the
    LogicLib branches (IntCmp/StrCmp) match the intended logic.
  3. Desk-checked the edge cases: fresh install, in-place upgrade, upgrade with
    a running instance (interactive retry / silent abort), rename failure with
    locked files, failure during file copy (rollback via .onInstFailed),
    and uninstall after another application or the user changed a shared
    association.

Not yet exercised on a real Windows machine: before packaging the next
release, run an upgrade while the old version is still running and confirm
the old installation remains usable when the upgrade is aborted.

Checklist

  • I am not an AI. I have manually read the code and understand it.
  • I take full responsibility for the quality and architectural fit of
    this code.
  • I agree to publish this code under the MIT license.

Upgrading deleted the previous installation in place before checking
whether the deletion succeeded, so a locked file (e.g. because the app
was still running) left the old version half-deleted and unusable. The
installer now makes sure no instance is running before touching the
disk, moves the previous installation aside with a rename - which
fails harmlessly while files are locked - and renames it back if
writing the new files fails. The moved-aside copy is only removed once
the new installation is complete.

The uninstaller deleted whole file-extension registry keys that are
shared with other applications (.png, .bmp, .jpeg, .jpg, .svg, .svgz,
.dxf), discarding associations owned by other software. It now removes
only the Rayforge ProgID keys, remembers the default association that
the installer replaces, and restores it if the association still
belongs to Rayforge.
The uninstaller previously remembered the default file association it
had replaced and restored it on uninstall if the association still
belonged to Rayforge. While the ownership guard handles extensions
claimed by later installations, snapshotting and restoring shared
state is fragile and unnecessary: shared extensions are now never
claimed as default at all. Rayforge only registers its ProgIDs and
lists them under each extension's OpenWithProgids subkey, which adds
it to the "Open with" menu, and uninstalling removes exactly those
registrations. The default value is only cleared when it still points
at a Rayforge ProgID, which would dangle after removal. Rayforge-owned
formats (.ryp, .rfs, .rd) keep their default registration.
@knipknap
knipknap merged commit 710f79a into main Oct 23, 2026
34 checks passed
@github-actions

Copy link
Copy Markdown

🤖 Estimated LLM implementation cost

Replayed each of the 2 commits in this pull request and counted the code lines each commit added or rewrote:

Commit Message Lines (add/rewrite) Est. output tokens Est. output cost
469d69cc fix(win): preserve shared associations and restore failed upgrades 0 / 0 0 $0.0000
853a5c82 fix(win): register shared types via OpenWithProgids, not as defaults 0 / 0 0 $0.0000
Total 0 0 $0.0000

Full estimate

Component Est. tokens Est. cost
Context input (reading touched files) 4,642 $0.0139
Prompting (pull request description) 890 $0.0027
Lower bound (output only) 0 $0.0000
Best estimate 5,532 $0.0166

Cost model: ≈10 tokens per code line at $15/Mtok output and $3/Mtok input (assuming heavy prompt caching); input ≈ 15× output for conversation replay; a review round ≈ 42,000 tokens; a failed CI run ≈ 21,000 tokens; file reads capped at 20 kB each.

Active window from first commit to merge: <1 day.

⚠️ Caveat: the lower bound covers only the tokens for emitting the code. The best estimate adds modeled overhead for exploration, conversation replay, review and debug turns, but real sessions vary by tool and task — treat it as an order-of-magnitude figure. Human engineering time is not included.

This branch was previously deployed

1 inactive deployment
pypi — 853a5c82 Deployed Oct 23, 2026 by knipknap via Build and Publish Wheel #2466
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